Skip to content

Added multi-factor authentication for account login#4618

Closed
mschwager wants to merge 1 commit intopypi:masterfrom
mschwager:multi-factor-authentication
Closed

Added multi-factor authentication for account login#4618
mschwager wants to merge 1 commit intopypi:masterfrom
mschwager:multi-factor-authentication

Conversation

@mschwager
Copy link
Copy Markdown

@mschwager mschwager commented Aug 24, 2018 •
edited
Loading

Fixes #996.

Hey all,

I'm hoping to get some early feedback on this MFA work. Here's the current workflow:

Visit the account management page and configure MFA

2018-08-24-092346_1162x733_scrot

Then, when logging in, you're presented with the same primary authentication page

2018-08-24-092404_982x516_scrot

If MFA is enabled for that user then you'll be asked to input an authentication token

2018-08-24-092417_1076x613_scrot

This workflow is similar to what we were discussing in IRC. There's undoubtedly some UI/UX improvements to be made, but I was hoping to get some eyes (@dstufft, @di, @sergeykolosov, others?) on this as soon as possible. Addtionally, there's some things that need to be done, and probably some that should be done, before this is ready for prime time:

  • Pass username/password along to MFA form when performing secondary authentication
  • Allow disabling of MFA in account management
  • Add additional MFA tests for the accounts views page
  • Spot check UI/UX/verbiage

I'm about to take off for the weekend and was hoping to get this up before then :)

@mschwager
Copy link
Copy Markdown
Author

mschwager commented Aug 27, 2018

Okay, finally appeased the linter and maintained 100% test coverage. Not sure what's up with the Dependencies job, it's been consistently failing on every run for reasons that aren't immediately obvious.

@di
Copy link
Copy Markdown
Member

di commented Aug 27, 2018

@mschwager The dependencies job is failing because you've added pyotp to requirements/main.txt, but it's not in requirements/main.in.

@mschwager
Copy link
Copy Markdown
Author

mschwager commented Aug 27, 2018

Wooo! Everything passed - thanks @di!

@di
Copy link
Copy Markdown
Member

di commented Aug 27, 2018

No problem! Thanks for this work. Bear with us as it might take us a bit to fully review this.

mschwager
mschwager commented Aug 28, 2018
View reviewed changes
warehouse/accounts/views.py
with_mfa = False

# Upgrade to MFA form
if with_mfa and _form_class == LoginForm:
Copy link
Copy Markdown
Author

@mschwager mschwager Aug 28, 2018

Choose a reason for hiding this comment

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

This check is a bit awkward. Basically, we have to check if _form_class is the default value set in the keyword argument. If we don't then testing becomes difficult because passing in a stubbed form_class will be overwritten when we set it to LoginWithMfaForm.

I typically don't like writing code that's been contorted to make tests pass, but I couldn't see a good way around it without a more significant refactor. I'm open to ideas though!

@di di self-assigned this Oct 8, 2018
@mschwager
Copy link
Copy Markdown
Author

mschwager commented Oct 27, 2018

I just rebased on top of master.

Looks like the Dependencies CI job is complaining again:

echo "$DEPCHECKER" | python - /tmp/tmp.xiuuNpsu8m/main.txt requirements/main.txt
+ future
make[1]: *** [deps] Error 1
make[1]: Leaving directory `/home/travis/build/pypa/warehouse'
make: *** [travis-deps] Error 2

It looks like it's complaining that future is in requirements/main.txt but not requirements/main.in. That's how it is on master, though, so I'm not sure how CI is succeeding there.

@di
Copy link
Copy Markdown
Member

di commented Oct 27, 2018

@mschwager It's due to a recent release of readme_renderer which removes the future dependency. That will be fixed in #4965.

@mschwager
Copy link
Copy Markdown
Author

mschwager commented Oct 27, 2018

Ahhh - thanks for the heads up! I rebased on master and it looks like it's fixed!

@mschwager
Copy link
Copy Markdown
Author

mschwager commented Feb 15, 2019

I've rebased on top of the latest code in master. Still hoping to get a review on this.

@brainwane
Copy link
Copy Markdown
Contributor

brainwane commented Mar 14, 2019

As @woodruffw said about a related PR:

I've restarted work on this in master...trailofbits:tob/996-add-2fa-support. Will open a PR shortly so that others can review progress in real time.

His PR is #5567.

(Context: this grant-funded project.)

@brainwane
Copy link
Copy Markdown
Contributor

brainwane commented Mar 14, 2019

@mschwager Your work is one of the sources Will is learning from here -- thank you so much for it!

@woodruffw woodruffw mentioned this pull request Mar 21, 2019
Merged
woodruffw added a commit to trail-of-forks/warehouse that referenced this pull request Mar 22, 2019
@woodruffw
warehouse: TOTP QR generation
438409f 
Cribbed largely from pypi#4618,
with some accessibility improvements.
woodruffw added a commit to trail-of-forks/warehouse that referenced this pull request Apr 26, 2019
@woodruffw
warehouse: TOTP QR generation
35fa10c 
Cribbed largely from pypi#4618,
with some accessibility improvements.
woodruffw added a commit to trail-of-forks/warehouse that referenced this pull request Apr 26, 2019
@woodruffw
warehouse: TOTP QR generation
1f3644d 
Cribbed largely from pypi#4618,
with some accessibility improvements.
@di
Copy link
Copy Markdown
Member

di commented May 4, 2019

Closing in favor of #5567, but thank you for your contribution!

@di di closed this May 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@di di

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add support for two-factor authentication

3 participants

@mschwager @di @brainwane