assign roles without privilege escalation #10342
Merged
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 this PR does / why we need it:
This PR changes what permissions are required to assign a role.
Previously, only "Manage{Dataset,Dataverse,File}Permissions" was required. However, this allows giving yourself or someone else permissions you don't already have.
E.g. if you have the "ManageXPermissions" permission, you could use it to make yourself admin. This issue has been pointed out before, e.g. here https://groups.google.com/g/dataverse-community/c/wZfSTBiJuPQ/m/N_WXj6nPAAAJ
A possible solution was suggested here: #7252 (comment) When assigning a role, we could compare the permission bits of the assigning user and the assigned role. If the assigned role has any permission bit the assigning user doesn't have, the role assignment is forbidden.
This PR basically implements this fix. For assigning a role, the assigning user must have all of the permissions that the assigned role has.
Which issue(s) this PR closes:
Closes #9358
Special notes for your reviewer:
/
Suggestions on how to test this:
I extended this test:
mvn test -Dtest="DatasetsIT#testAddRoles"
Without the changes in AssignRoleCommand, the extended test fails:
With the changes, it passes.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
/
Is there a release notes update needed for this change?:
not sure
Additional documentation:
/
cc @johannes-darms