Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add whatsapp method #679

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

ryaustin
Copy link

@ryaustin ryaustin commented Nov 2, 2023

This change takes advantage of the available phone methods and Twilio gateway to allow the user to select WhatsApp as their token method.

Description

In the same pattern as the SMS method, the new WhatsApp method is added along with some user-definable settings. The change also exposes the ability to set the size of the token input field which was a bit small by default. Appropriate documentation has also been added.

Motivation and Context

Fixes #678. WhatsApp has become a major communication protocol globally due to its reach, speed, and security. It seems natural that a user can receive a token via WhatsApp. It is also more secure than SMS.

How Has This Been Tested?

Using existing mock tests in the pattern of SMS. Sending a WhatsApp is akin to sending a SMS. The example app implementation also works as intended. The documentation was tested within a vanilla example app for corectness.

Screenshots (if appropriate):

Screenshot 2023-11-02 at 10 29 01 AM
Screenshot 2023-11-02 at 10 29 14 AM
Screenshot 2023-11-02 at 10 29 44 AM

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [ x ] My code follows the code style of this project.
  • [ x] My change requires a change to the documentation.
  • [ x ] I have updated the documentation accordingly.
  • [ x ] I have added tests to cover my changes.
  • [ x ] All new and existing tests passed.

Copy link
Collaborator

@moggers87 moggers87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good start, but there's a lot of stuff that shouldn't be here.

@@ -1,5 +1,5 @@
[bumpversion]
current_version = 1.15.5
current_version = 1.16
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the commits that change the version string? This is something that's done at release time.

@@ -59,7 +59,7 @@
#

# The full version, including alpha/beta/rc tags.
release = '1.15.5'
release = '1.16'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the commits that change the version string? This is something that's done at release time.

@@ -2,7 +2,7 @@

setup(
name='django-two-factor-auth',
version='1.15.5',
version='1.16',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the commits that change the version string? This is something that's done at release time.


WHATSAPP = 'wa'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what this is for, it's only used in a few places. Also why are we using "wa" rather than "whatsapp"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explicit is better. agreed.

token.widget.attrs.update({'autofocus': 'autofocus',
'inputmode': 'numeric',
'autocomplete': 'one-time-code'})
'autocomplete': 'one-time-code',
'style': f'width:{TWO_FACTOR_INPUT_WIDTH}px; height:{TWO_FACTOR_INPUT_HEIGHT}px;'})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work if a site is using CSP and disallows inline styles

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, I will remove this functionality from this pr.

@@ -11,11 +11,12 @@
from .plugins.registry import registry
from .utils import totp_digits

TWO_FACTOR_INPUT_WIDTH = getattr(settings, 'TWO_FACTOR_INPUT_WIDTH', 100)
TWO_FACTOR_INPUT_HEIGHT = getattr(settings, 'TWO_FACTOR_INPUT_HEIGHT', 30)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear that we need this

@@ -1,5 +1,10 @@
# Changelog

## 1.15.6
### Changed
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be

## Unreleased
### Added

## 1.15.6
### Changed
- Added WhatsApp as a token method through Twilio
- Added the ability to modify the token input size
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think modifying the token input size should be a part of this PR

@ryaustin
Copy link
Author

Appreciate the feedback, i'll make the changes as soon as I can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Enable Whatsapp as a method
2 participants