Skip to content

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.


# 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.

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.

from two_factor.gateways import make_call, send_sms
from two_factor.gateways import make_call, send_sms, send_whatsapp

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.

'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.

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

# 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