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

assign roles without privilege escalation #10342

Merged

Conversation

vera
Copy link
Contributor

@vera vera commented Feb 27, 2024

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:

[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   DatasetsIT.testAddRoles:1688 expected: <401> but was: <200>
[INFO] 
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0

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

@coveralls
Copy link

coveralls commented Feb 27, 2024

Coverage Status

coverage: 20.278% (+0.003%) from 20.275%
when pulling c0c247e on vera:assign-roles-without-privilege-escalation
into de45c13 on IQSS:develop.

@qqmyers qqmyers added the Size: 3 A percentage of a sprint. 2.1 hours. label Feb 27, 2024
Copy link
Member

@qqmyers qqmyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good. Can you add a note in the Guides about how this works and add a ~one line release note?

@vera
Copy link
Contributor Author

vera commented Feb 27, 2024

Thanks for reviewing! Sure, just did.

@sekmiller sekmiller self-assigned this Mar 1, 2024
@sekmiller sekmiller merged commit c6568ca into IQSS:develop Mar 4, 2024
12 checks passed
@poikilotherm
Copy link
Contributor

poikilotherm commented Mar 4, 2024

Such an "easy" fix! I ❤️ it @vera ! Great job in figuring this one out, much appreciated!

@pdurbin pdurbin added this to the 6.2 milestone Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: 3 A percentage of a sprint. 2.1 hours.
Projects
Status: Done 🧹
Development

Successfully merging this pull request may close these issues.

Everything but publishing Role
6 participants