Fix/project role permission grant (#8084) #8132
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
## Background In #6380 we fixed a privilege escalation bug that allowed members of a project that had permission to add users to the project with roles that had a higher permission set than themselves. The PR linked essentially constricts you only be able to assign users to roles that you possess yourself if you are not an Admin or Project owner. This fix broke expectations for another customer who needed to have a project owner without the DELETE_PROJECT permission. The fix above made it so that their custom project owner role only was able to assign users to the project with the role that they posessed. ## Fix Instead of looking directly at which role the role granter has, this PR addresses the issue by making the assessment based on the permission sets of the user and the roles to be granted. If the granter has all the permissions of the role being granted, the granter is permitted to assign the role. ## Other considerations The endpoint to get roles was changed in this PR. It previously only retrieved the roles that the user had in the project. This no-longer makes sense because the user should be able to see other project roles than the one they themselves hold when assigning users to the project. The drawback of returning all project roles is that there may be a project role in the list that the user does not have access to assign, because they do not hold all the permissions required of the role. This was discussed internally and we decided that it's an acceptable trade-off for now because the complexities of returning a role list based on comparing permissions set is not trivial. We would have to retrieve each project role with permissions from the database, and run the same in-memory check against the users permission to determine which roles to return from this endpoint. Instead we opted for returning all project roles and display an error if you try to assign a role that you do not have access to. ## Follow up When this is merged, there's no longer need for the frontend logic that filters out roles in the role assignment form. I deliberately left this out of the scope for this PR because I couldn't wrap my head around everything that was going on there and I thought it was better to pair on this with @chriswk or @nunogois in order to make sure we get this right as the logic for this filtering seemed quite complex and was touching multiple different components. --------- Co-authored-by: Fredrik Strand Oseberg <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
FredrikOseberg
requested review from
ivarconr,
chriswk,
sjaanus and
kwasniew
chriswk
approved these changes
Sep 10, 2024
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.
Cherry pick the fix merged in #8084