feat: add APIs to list roles a user can assign for a dataset or dataverse - #11919
feat: add APIs to list roles a user can assign for a dataset or dataverse#11919vera wants to merge 11 commits into
Conversation
…asets and dataverses
# Conflicts: # src/main/java/edu/harvard/iq/dataverse/DataverseRoleServiceBean.java # src/main/java/edu/harvard/iq/dataverse/ManagePermissionsPage.java # src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java
|
@pdurbin Could you assign a ready for review label? |
|
@johannes-darms sure! Well, I put it in triage, for Triage Tuesday. 😅 @vera can you please resolve the merge conflicts? Thanks! ❤️ |
# Conflicts: # src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java # src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java
done :) |
jp-tosca
moved this from Ready for Triage
to Ready for Review ⏩
in IQSS Dataverse Project
cmbz
added
FY26 Sprint 23
stevenwinship
moved this from Ready for Review ⏩
to In Review 🔎
in IQSS Dataverse Project
I think adding this change is a good idea but I will not hold up this PR |
github-project-automation
Bot
moved this from In Review 🔎
to Ready for QA ⏩
in IQSS Dataverse Project
| @GET | ||
| @AuthRequired | ||
| @Path("{identifier}/assignments/userAssignableRoles") |
There was a problem hiding this comment.
For all APIs, please add new annotations described here:
There was a problem hiding this comment.
Done, also fixed the merge conflicts.
Vacuum score is back up to 98:
$ vacuum lint -r scripts/openapi/vacuum-recommended.yaml target/classes/META-INF/openapi.json
# Conflicts: # src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java
There was a problem hiding this comment.
I found two authorization mismatches that should be addressed before merge:
- availableRoles does not account for the required Manage*Permissions.
- The endpoints use User instead of the full DataverseRequest, which can miss request-dependent permissions.
I also noted a small test assertion typo. Requesting changes so the returned roles match the authorization enforced by AssignRoleCommand.
| // Filter roles assignable by given user | ||
| Set<Permission> granted = permissionService.permissionsFor(user, dvo); | ||
| roles = roles.stream() | ||
| .filter(role -> granted.containsAll(role.permissions())) |
There was a problem hiding this comment.
AssignRoleCommand requires the applicable Manage*Permissions in addition to the role's permissions, but this filter checks only role.permissions(). This can return roles the requester cannot actually assign. Could we align this calculation with AssignRoleCommand and add an integration test that assigns one returned role?
| description = "List of user-assignable roles", | ||
| content = @Content(mediaType = "application/json")) | ||
| public Response getAssignableRoles(@Context ContainerRequestContext crc, @Parameter(description = "Dataset id or persistent identifier.") @PathParam("identifier") String id) { | ||
| return response(req -> ok(jsonDataverseRoles(new ArrayList<>(dataverseRoleService.availableRoles(findDatasetOrDie(id), req.getUser())))), getRequestUser(crc)); |
There was a problem hiding this comment.
Could we pass the full DataverseRequest here, and in the equivalent Dataverses endpoint? permissionsFor(RoleAssignee, ...) excludes request-dependent groups such as IP and mail-domain groups, while AssignRoleCommand uses the full request context, so this list can omit roles the requester can actually assign.
| assertEquals(200, deleteUserResponse.getStatusCode()); | ||
|
|
||
| Response deleteUser2Response = UtilIT.deleteUser(username2); | ||
| assertEquals(200, deleteUserResponse.getStatusCode()); |
There was a problem hiding this comment.
This checks deleteUserResponse instead of deleteUser2Response. The same typo appears in DataversesIT.
9 participants
What this PR does / why we need it:
This PR adds two API endpoints that list the roles a user is allowed to assign to others for a given dataset or dataverse.
The endpoints return all roles that have no more permissions than the current user. Roles with higher permissions are excluded.
This is needed for our custom UI’s role assignment form, so that users only see roles they are actually allowed to assign. The same endpoints could also be used by the new SPA.
For reference, the current Dataverse UI lists all roles, even those the user cannot assign:
If a user tries to assign a role they are not allowed to assign, they currently see an error message:
With this PR, the UI could also be updated to show only assignable roles, preventing this error, by applying the following change in
ManagePermissionsPage.java:Which issue(s) this PR closes:
Not aware of an issue
Special notes for your reviewer:
/
Suggestions on how to test this:
I've added tests here:
mvn test -Dtest="DataversesIT#testListAssignableRoles"mvn test -Dtest="DatasetsIT#testListAssignableRoles"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?:
I've added a release note.
Additional documentation:
/
cc @johannes-darms