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 the possibility for the user to change the double authentication #380

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

Conversation

lmarion-source
Copy link

@lmarion-source lmarion-source commented Aug 25, 2020

Add the possibility for the user to change the double authentication

Description

From the profile view, the user can now switch between Google authenticator/ sms or call method for double authentication.

Motivation and Context

This solves the issue reported in #347

How Has This Been Tested?

No new test was added, however the example repository has been updated to take the change into account. This can then be tested locally by using the example repository

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [ X ] 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.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • [ X ] All existing tests passed.

@lmarion-source lmarion-source force-pushed the change_double_authentication_method branch from 8fdb088 to 81efb63 Compare August 25, 2020 10:50
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.

That's quite a lot of templates you've added to the example app and I'm not quite sure why they're necessary (and why those changes aren't just in two_factor itself if they are necessary).

Also tests are not optional!

key = random_hex_str(20)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why all these random newlines? I don't see how they are related to your proposed change.

session_key_name = 'django_two_factor-qr_secret_key'

form_list = (
('generator', TOTPDeviceForm),
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's only one form here, what about yubikey and phone methods?

Copy link
Author

Choose a reason for hiding this comment

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

This is exactly the point: this view is used when you already have a phone as double authentication method and want to go to the generator.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What if I want to change to another method?

Copy link
Author

Choose a reason for hiding this comment

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

Well then you have the other view that allows you to go from the token generator to the SMS/call. You can only have one method for double authentication at a time. If you previously were using the google authenticator, you'll arrive on the view to use SMS/call. If you previously were using the SMS/call, you'll arrive on the view to use google authenticator. If you want to add extra backup tokens or phone etc nothing changes. All of this assumes you already have enable the double authentication method. If not, then you are still pointing to the initial setup view.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And what about yubikey?

Copy link
Author

Choose a reason for hiding this comment

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

I will add this one. My bad.

Copy link
Author

Choose a reason for hiding this comment

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

tests are added, the case of yhe use of a yubikey is also considered

@lmarion-source
Copy link
Author

lmarion-source commented Aug 25, 2020

I added a bunch of templates because the example was not working initially....a bunch of templates were actually missing to make it work due to a change in the settings for the repository of templates (or I missed something?). I'll write tests.

@lmarion-source lmarion-source force-pushed the change_double_authentication_method branch 5 times, most recently from 7328022 to 59f4bc7 Compare August 27, 2020 07:46
@lmarion-source lmarion-source force-pushed the change_double_authentication_method branch from 59f4bc7 to c24cedf Compare August 27, 2020 08:14
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.

2 participants