diff --git a/doc/release-notes/10342-assign-roles-without-privilege-escalation.md b/doc/release-notes/10342-assign-roles-without-privilege-escalation.md new file mode 100644 index 00000000000..a4ef743f50d --- /dev/null +++ b/doc/release-notes/10342-assign-roles-without-privilege-escalation.md @@ -0,0 +1 @@ +The permissions required to assign a role have been fixed. It is no longer possible to assign a role that includes permissions that the assigning user doesn't have. \ No newline at end of file diff --git a/doc/sphinx-guides/source/user/dataset-management.rst b/doc/sphinx-guides/source/user/dataset-management.rst index 708113f9a99..d3faf479deb 100755 --- a/doc/sphinx-guides/source/user/dataset-management.rst +++ b/doc/sphinx-guides/source/user/dataset-management.rst @@ -566,7 +566,7 @@ This is where you will enable a particular Guestbook for your dataset, which is Roles & Permissions =================== -Dataverse installation user accounts can be granted roles that define which actions they are allowed to take on specific Dataverse collections, datasets, and/or files. Each role comes with a set of permissions, which define the specific actions that users may take. +Dataverse installation user accounts can be granted roles that define which actions they are allowed to take on specific Dataverse collections, datasets, and/or files. Each role comes with a set of permissions, which define the specific actions that users may take. It is not possible to grant a role that comes with a permission that the granting user themselves does not have. Roles and permissions may also be granted to groups. Groups can be defined as a set of Dataverse user accounts, a collection of IP addresses (e.g. all users of a library's computers), or a collection of all users who log in using a particular institutional login (e.g. everyone who logs in with a particular university's account credentials). diff --git a/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/AssignRoleCommand.java b/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/AssignRoleCommand.java index 5577d541012..e4edb973cd9 100644 --- a/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/AssignRoleCommand.java +++ b/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/AssignRoleCommand.java @@ -19,6 +19,7 @@ import edu.harvard.iq.dataverse.engine.command.exception.CommandException; import edu.harvard.iq.dataverse.engine.command.exception.IllegalCommandException; import java.util.Collections; +import java.util.HashSet; import java.util.Map; import java.util.Set; @@ -75,9 +76,19 @@ public RoleAssignment execute(CommandContext ctxt) throws CommandException { @Override public Map> getRequiredPermissions() { // for data file check permission on owning dataset - return Collections.singletonMap("", - defPoint instanceof Dataverse ? Collections.singleton(Permission.ManageDataversePermissions) - : defPoint instanceof Dataset ? Collections.singleton(Permission.ManageDatasetPermissions) : Collections.singleton(Permission.ManageFilePermissions)); + Set requiredPermissions = new HashSet(); + + if (defPoint instanceof Dataverse) { + requiredPermissions.add(Permission.ManageDataversePermissions); + } else if (defPoint instanceof Dataset) { + requiredPermissions.add(Permission.ManageDatasetPermissions); + } else { + requiredPermissions.add(Permission.ManageFilePermissions); + } + + requiredPermissions.addAll(role.permissions()); + + return Collections.singletonMap("", requiredPermissions); } @Override diff --git a/src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java b/src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java index e1c4b901116..310bc2e5851 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java @@ -1670,6 +1670,26 @@ public void testAddRoles(){ giveRandoPermission = UtilIT.grantRoleOnDataset(datasetPersistentId, "fileDownloader", "@" + randomUsername, apiToken); giveRandoPermission.prettyPrint(); assertEquals(200, giveRandoPermission.getStatusCode()); + + // Create another random user to become curator: + + Response createCuratorUser = UtilIT.createRandomUser(); + createCuratorUser.prettyPrint(); + String curatorUsername = UtilIT.getUsernameFromResponse(createCuratorUser); + String curatorUserApiToken = UtilIT.getApiTokenFromResponse(createCuratorUser); + + Response giveCuratorPermission = UtilIT.grantRoleOnDataset(datasetPersistentId, "curator", "@" + curatorUsername, apiToken); + giveCuratorPermission.prettyPrint(); + assertEquals(200, giveCuratorPermission.getStatusCode()); + + // Test if privilege escalation is possible: curator should not be able to assign admin rights + Response giveTooMuchPermission = UtilIT.grantRoleOnDataset(datasetPersistentId, "admin", "@" + curatorUsername, curatorUserApiToken); + giveTooMuchPermission.prettyPrint(); + assertEquals(401, giveTooMuchPermission.getStatusCode()); + + giveTooMuchPermission = UtilIT.grantRoleOnDataset(datasetPersistentId, "admin", "@" + randomUsername, curatorUserApiToken); + giveTooMuchPermission.prettyPrint(); + assertEquals(401, giveTooMuchPermission.getStatusCode()); String idToDelete = JsonPath.from(giveRandoPermission.getBody().asString()).getString("data.id"); @@ -1692,7 +1712,7 @@ public void testAddRoles(){ deleteGrantedAccess.prettyPrint(); assertEquals(200, deleteGrantedAccess.getStatusCode()); - Response deleteDatasetResponse = UtilIT.deleteDatasetViaNativeApi(datasetId, apiToken); + Response deleteDatasetResponse = UtilIT.deleteDatasetViaNativeApi(datasetId, apiToken); deleteDatasetResponse.prettyPrint(); assertEquals(200, deleteDatasetResponse.getStatusCode()); @@ -1703,6 +1723,14 @@ public void testAddRoles(){ Response deleteUserResponse = UtilIT.deleteUser(username); deleteUserResponse.prettyPrint(); assertEquals(200, deleteUserResponse.getStatusCode()); + + deleteUserResponse = UtilIT.deleteUser(randomUsername); + deleteUserResponse.prettyPrint(); + assertEquals(200, deleteUserResponse.getStatusCode()); + + deleteUserResponse = UtilIT.deleteUser(curatorUsername); + deleteUserResponse.prettyPrint(); + assertEquals(200, deleteUserResponse.getStatusCode()); }