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

Approach for resolving #6232 #6353

Merged
merged 4 commits into from
Jan 16, 2020
Merged

Approach for resolving #6232 #6353

merged 4 commits into from
Jan 16, 2020

Conversation

ewdurbin
Copy link
Member

@ewdurbin ewdurbin commented Aug 2, 2019

Fixes #6232.

Resulting error messages:

Invalid Username/Password:

HTTPError: 403 Client Error: Invalid or non-existent authentication information. for url: http://localhost/legacy/

Invalid API Token:

HTTPError: 403 Client Error: Invalid or non-existent authentication information. for url: http://localhost/legacy/

Valid Username/Password that does not have permission:

HTTPError: 403 Client Error: The user '<USER_NAME>' isn't allowed to upload to project '< PROJECT_NAME >'. See http://localhost/help/#project-name for more information. for url: http://localhost/legacy/

Valid API Token missing scope for project:

HTTPError: 403 Client Error: Invalid API Token: project-scoped token is not valid for project '<PROJECT_NAME>' for url: http://localhost/legacy/

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, although we could possibly use the message passed in each Denied instance (and pull the underlying error out from V1Caveat) instead of using isinstance to test the class of allowed. That might be a little easier to test + allow more error message reuse, but this approach is also fine.

@ewdurbin
Copy link
Member Author

ewdurbin commented Aug 6, 2019

@dstufft do you have a preference for which approach to take here?

@ewdurbin
Copy link
Member Author

ewdurbin commented Aug 7, 2019

Offered another approach based on feedback from @dstufft, though I'm still feeling really icky about how much we're having to bubble through.

@brainwane
Copy link
Contributor

This item is a beta blocker https://github.com/pypa/warehouse/milestone/17 so it'd be nice to address this month so we can send out a pypi-announce email around Halloween (security is spooky!).

@brainwane
Copy link
Contributor

I would like to wrap up the beta of the API token feature and email out an announcement to get more people using the feature. Thus: @ewdurbin could you please rebase this, and then @dstufft could you review it, so we can finish getting this merged and then get more people uploading with API tokens?

Thanks.

@brainwane
Copy link
Contributor

IMO fixing this is still a blocker for coming out of beta #5661 (comment) -- @ewdurbin @di @woodruffw @yeraydiazdiaz could we finish this PR within the next few weeks so we can announce the feature on pypi-announce, increase adoption, and better secure PyPI packages?

Will need review from @dstufft and @woodruffw

If they approve... needs tests/etc
still unsure about the approach
@ewdurbin
Copy link
Member Author

I think this is ready for review.

@di di changed the title WIP: Approach for resolving #6232 Approach for resolving #6232 Jan 16, 2020
@ewdurbin ewdurbin merged commit 72db1a6 into master Jan 16, 2020
@ewdurbin ewdurbin deleted the approach_for_6232 branch January 16, 2020 19:45
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.

Improved error reporting for token permissions
4 participants