-
Notifications
You must be signed in to change notification settings - Fork 995
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
warehouse: Require a verified email for 2FA modifications #5889
warehouse: Require a verified email for 2FA modifications #5889
Conversation
The method within the User model does the work here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its not clear to me under what condition we would ever flag an account as being prohibited from using 2FA, but I suppose it's nice to have the option.
Overall this is ready to go as far as I can tell. My only concern is that we send blind 403s without explanation. I suppose we could flash a notification to help people understand why they're getting a 403?
Yeah, that state/option is vestigial from the TOTP rollout; I can take it out in this PR or a future one, if desired 🙂
Yeah, I can add this. I'm also about to push some changes that ensure that we only reject 2FA provisioning requests and not authentication (to prevent the de-verified email scenario discussed out-of-band). |
We should never disable 2FA for authentication just because a user's primary email has changed to unverified.
Alright, warning flashes added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks ready to go. @di, @dstufft any concerns?
@brainwane when this merges TOTP goes live, do we need to coordinate launch?
To be clear: I'd like active approval by @di before we can merge this PR and thus launch TOTP as a feature for all PyPI.org and test.pypi.org users, but I'm ok with going forward without @dstufft's +1. If Dustin, Ernest, or Donald objects and thinks Donald's +1 is a necessary prerequisite, please override me. :-) Re: launch coordination (also, followup to #5661): I will write up a blog post for the PSF blog to be published within the next few days, and I think we should link to it on https://status.python.org/ . Therefore, my request is that we merge after Wednesday May 29th 2pm Eastern Time, to give me time to draft it. |
@di is reviewing this now! @ewdurbin https://pad.sfconservancy.org/p/2fa-pypi-launch-post is what I think could be posted at https://pyfound.blogspot.com/ , https://blog.python.org/ , and status.python.org. Take a look, and please feel free to edit. Also, I need to step away from my laptop for the afternoon and evening, so please feel free to go ahead and publish that post once we have merged this PR. |
@brainwane It'd be best to also link to the pypi-announce mailing list at the end, when mentioning it: https://mail.python.org/mailman3/lists/pypi-announce.python.org/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree w/ @ewdurbin, I don't see a reason why we'd use two_factor_prohibited
. In the interest of keeping things simple here, let's take it out in this PR.
Prevents users from modifying their 2FA methods without a primary, verified email. Already enabled 2FA methods will continue to work, even if the user's email lapses into an unverified state.
Inverts the
two_factor_allowed
column intotwo_factor_prohibited
, with a new property onUser
testingtwo_factor_prohibited
plus whether the user has both a primary email and whether that email is verified.Closes #5866.