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

chore: remove most of basic auth code #15182

Merged

Conversation

miketheman
Copy link
Member

Now that the active policies are Session (for web) and Macaroon (for upload), remove the bulk of the BasicAuth policy.

Move the logic farther up the chain, so that there's no extra work put in before telling the user that they should migrate to API Tokens.

Resolves #13770

Now that the active policies are Session (for web) and Macaroon (for
upload), remove the bulk of the BasicAuth policy.

Move the logic farther up the chain, so that there's no extra work put
in before telling the user that they should migrate to API Tokens.

Resolves pypi#13770

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
@miketheman miketheman added the security Security-related issues and pull requests label Jan 10, 2024
@miketheman miketheman requested a review from a team as a code owner January 10, 2024 22:12
@miketheman
Copy link
Member Author

P.S. We can potentially also remove commit_veto, since nothing can raise the BasicAuth errors any longer.

def commit_veto(request, response):
# By default pyramid_tm will veto the commit anytime request.exc_info is not None,
# we are going to copy that logic with one difference, we are still going to commit
# if the exception was for a BasicAuthFailedPassword or BreachedPassword.
# TODO: We should probably use a registry or something instead of hardcoded.
allowed_types = (BasicAuthBreachedPassword, BasicAuthFailedPassword)
try:
return not isinstance(request.exc_info[1], allowed_types)
except (AttributeError, TypeError):
return False

I elected to defer until this is reviewed and approved, in case there's some other tricky reason to keep the commit_veto.

@miketheman miketheman requested a review from woodruffw January 10, 2024 22:21
@miketheman
Copy link
Member Author

P.P.S. There's also some basic auth email sending code in the legacy upload endpoint that would be another deferral.

Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM! Negative diffs are the best diffs 🙂

@di
Copy link
Member

di commented Jan 11, 2024

I think we can also remove this:

# Check if the user has 2FA and used basic auth
# NOTE: We don't need to guard request.user here because basic auth
# can only be used with user identities.
if (
request.authentication_method == AuthenticationMethod.BASIC_AUTH
and request.user.has_two_factor
):
send_basic_auth_with_two_factor_email(
request, request.user, project_name=project.name
)
raise _exc_with_message(
BasicAuthTwoFactorEnabled,
(
f"User { request.user.username } has two factor auth enabled, "
"an API Token or Trusted Publisher must be used to upload "
"in place of password."
),
)

@miketheman
Copy link
Member Author

I think we can also remove this:

Agreed - I noted that I'm deferring that #15182 (comment)

@di
Copy link
Member

di commented Jan 11, 2024

Sorry, missed your P.P.S. 😂

@miketheman miketheman merged commit ad08a81 into pypi:main Jan 11, 2024
17 checks passed
@miketheman miketheman deleted the miketheman/simplify-basic-auth-policy branch January 11, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Security-related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove support for passwords in upload API
3 participants