feat: Separate api token roles #4019
Merged
Conversation
This file contains 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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
nunogois
added a commit
that referenced
this pull request
Jun 21, 2023
#4019 introduced a bug where API token filtering was not taking into account ADMIN permissions, which means the API tokens were not being displayed on the UI.
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.
What
As part of the move to enable custom-root-roles, our permissions model was found to not be granular enough to allow service accounts to only be allowed to create read-only tokens (client, frontend), but not be allowed to create admin tokens to avoid opening up a path for privilege escalation.
How
This PR adds 12 new roles, a CRUD set for each of the three token types (admin, client, frontend). To access the
/api/admin/api-tokensendpoints you will still need the existing permission (CREATE_API_TOKEN, DELETE_API_TOKEN, READ_API_TOKEN, UPDATE_API_TOKEN). Once this PR has been merged the token type you're modifying will also be checked, so if you're trying to create a CLIENT api-token, you will needCREATE_API_TOKENandCREATE_CLIENT_API_TOKENpermissions. If the user performing the create call does not have these two permissions or theADMINpermission, the creation will be rejected with a403 - FORBIDDENstatus.Discussion points
The test suite tests all operations using a token with operation_CLIENT_API_TOKEN permission and verifies that it fails trying to do any of the operations against FRONTEND and ADMIN tokens. During development the operation_FRONTEND_API_TOKEN and operation_ADMIN_API_TOKEN permission has also been tested in the same way. I wonder if it's worth it to re-add these tests in order to verify that the permission checker works for all operations, or if this is enough. Since we're running them using e2e tests, I've removed them for now, to avoid hogging too much processing time.