feat: disallow management actions without a verified primary email #14126
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
di
reviewed
Jul 14, 2023
miketheman
force-pushed
the
miketheman/require-verified-email-to-manage
branch
from
24f43b0 to
1fe699c
Compare
As a way to continue on the path of making user accounts more secured, require a user to have verified an email address prior to performing any other management actions. Preserves their ability to manage their own account. Signed-off-by: Mike Fiedler <[email protected]>
Signed-off-by: Mike Fiedler <[email protected]>
miketheman
force-pushed
the
miketheman/require-verified-email-to-manage
branch
from
1fe699c to
16369e8
Compare
miketheman
changed the title
di
reviewed
Jul 19, 2023
| @@ -134,6 +134,22 @@ def forbidden(exc, request): | |||
|
|
|||
| # Check if the error has a "result" attribute and if it is a WarehouseDenied | |||
| if hasattr(exc, "result") and isinstance(exc.result, WarehouseDenied): | |||
| # If the forbidden error is because the user does not have a verified | |||
| # email address, redirect them to their account page for email verification. | |||
| if exc.result.reason == "unverified_email": | |||
There was a problem hiding this comment.
I realize you're just following the pattern already established here, but I'm wondering if these should be subclasses of WarehouseDenied that we check with isinstance instead of comparing the reason here.
There was a problem hiding this comment.
It's a good thought - I tinkered around with that, but didn't understand exactly what I was doing, since these aren't exceptions, rather subclasses of pyramid.security.Denied. I'll leave this as is for now, especially since there's other work in play about restructuring the ACLs and separating authentication from authorization a little better.
miketheman
added a commit
to miketheman/warehouse
that referenced
this pull request
Aug 7, 2023
Introduced in pypi#14126, check was only applied for the SessionSecurityPolicy. Migrating it into the common helper applies to both Session and BasicAuth policies. It was implicitly being checked before with BasicAuth via `_check_mfa` which, if enabled and mandated, would have required a verified email anyhow. Moving it into the helper allows us to have a consistent requirement across both policies. Signed-off-by: Mike Fiedler <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As a way to continue on the path of making user accounts more secured,
require a user to have verified the primary email address prior to performing any
other management actions.
Preserves their ability to manage their own account.
Signed-off-by: Mike Fiedler [email protected]