-
Notifications
You must be signed in to change notification settings - Fork 983
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
Add ExpiryCaveat
#11122
Add ExpiryCaveat
#11122
Conversation
This needs more testing, but the basic idea:
|
Marking as ready for review since this is in a stable state, but see comments above about the current approach. |
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 good, but I didn't give it a super deep review. I had one high level question: how will this be backwards-compatible for macaroons created before this change? The migration is just dropping one column and adding a new one without doing a data migration.
Yeah, this was an oversight on my part. This will break the UI for pre-existing macaroons, at least in the form they're recorded in the DB. The migration should probably just rename the column rather than dropping it and inserting a new one, since interior data is the same. I'll fix that now. |
Blocked on: #11157. |
* warehouse, tests: pick DB changes from #11122 * warehouse: `make translations` * manage/views: remove outdated note * warehouse, tests: `Macaroon.permissions -> Macaroon.permissions_caveat` Emphasizes that this is the entire caveat, and not just the permissions body. * warehouse: `make translations` * warehouse/templates: handle stale event caveats Prior to these changes, the `caveats` field in API token events was a dictionary, not a list. * warehouse: `make translations`
#11157 has been merged! |
Excellent! I'll de-conflict here. |
71339b3
to
1f65cff
Compare
Edit: Done. |
I also confirmed locally that this does not break verification of current tokens (i.e., ones missing an expiry caveat). |
Add tests for success/failure conditions in the top-level verifier.
This was a misdesign.
What is the deployment policy of pypi.org? Is everything on main automatically deployed? If not, where can I see what version is deployed? In other words, how can I see that changes like this are available on pypi.org? |
@fschulze Everything is automatically deployed within 5-10 minutes after merge. |
But also, note that these changes aren't user-facing yet! We don't currently allow users to create API tokens with expiry restrictions. |
@woodruffw doesn't matter for my use case. I want to derive unique upload tokens for pushing releases with devpi. The upload is done on the server side and the server currently sees the token/password. With the expiration and the project caveat I can create unique tokens on the client for use by the server. |
* warehouse, tests: pick DB changes from pypi#11122 * warehouse: `make translations` * manage/views: remove outdated note * warehouse, tests: `Macaroon.permissions -> Macaroon.permissions_caveat` Emphasizes that this is the entire caveat, and not just the permissions body. * warehouse: `make translations` * warehouse/templates: handle stale event caveats Prior to these changes, the `caveats` field in API token events was a dictionary, not a list. * warehouse: `make translations`
* warehouse, tests: add and test `ExpiryCaveat` * warehouse, tests: refactor macaroon failure handling Add tests for success/failure conditions in the top-level verifier. * tests: lint and format * macaroons/caveats: remove `_fail` API This was a misdesign. Co-authored-by: Ee Durbin <ewdurbin@gmail.com>
Closes #10792.
Closes #6255.