API Update global role + fixed issue with GUI custom role edition

doc/release-notes/8808-10575-update-global-role.md

@@ -0,0 +1,11 @@
+## Release Highlights
+
+### Update a Global Role
+
+A new API endpoint has been added that allows a global role to be updated. See [Native API Guide > Update Global Role](https://guides.dataverse.org/en/6.3/api/native-api.html#update-global-role) (#10612)
+
+## Bug fixes
+
+### Edition of custom role fixed
+
+It is now possible to edit a custom role with the same alias (reported in #8808)

doc/sphinx-guides/source/api/native-api.rst

@@ -4461,12 +4461,12 @@ The JSON representation of a role (``roles.json``) looks like this::
 
   {
     "alias": "sys1",
-    "name": “Restricted System Role”,
-    "description": “A person who may only add datasets.”,
+    "name": "Restricted System Role",
+    "description": "A person who may only add datasets.",
     "permissions": [
       "AddDataset"
     ]
-  } 
+  }
 
 .. note:: alias is constrained to a length of 16 characters
 
@@ -5593,22 +5593,43 @@ Creates a global role in the Dataverse installation. The data POSTed are assumed
 .. code-block:: bash
 
   export API_TOKEN=xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
-  export SERVER_URL=https://demo.dataverse.org
-  export ID=root
+  export SERVER_URL=http://localhost:8080
+
+  curl -H "Content-Type: application/json" -H "X-Dataverse-key:$API_TOKEN" -X POST "$SERVER_URL/api/admin/roles" --upload-file roles.json
+
+``roles.json`` see :ref:`json-representation-of-a-role`
+
+Update Global Role
+~~~~~~~~~~~~~~~~~~
+
+Update a global role in the Dataverse installation. The PUTed data is assumed to be a complete JSON role as it will overwrite the existing role. ::
+
+    PUT http://$SERVER/api/admin/roles/$ID
 
-  curl -H "X-Dataverse-key:$API_TOKEN" -X POST "$SERVER_URL/api/admin/roles" --upload-file roles.json
+A curl example using an ``ID``
+
+.. code-block:: bash
+
+  export SERVER_URL=http://localhost:8080
+  export ID=24
+
+  curl -H "Content-Type: application/json" -X PUT "$SERVER_URL/api/admin/roles/$ID" --upload-file roles.json
 
 ``roles.json`` see :ref:`json-representation-of-a-role`
 
 Delete Global Role
 ~~~~~~~~~~~~~~~~~~
 
+Deletes an ``DataverseRole`` whose ``id``  is passed. ::
+
+    DELETE http://$SERVER/api/admin/roles/$ID
+
 A curl example using an ``ID``
 
 .. code-block:: bash
 
   export API_TOKEN=xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
-  export SERVER_URL=https://demo.dataverse.org
+  export SERVER_URL=http://localhost:8080
   export ID=24
 
   curl -H "X-Dataverse-key:$API_TOKEN" -X DELETE "$SERVER_URL/api/admin/roles/$ID"

src/main/java/edu/harvard/iq/dataverse/DataverseRoleServiceBean.java

@@ -23,7 +23,6 @@
 import jakarta.persistence.EntityManager;
 import jakarta.persistence.PersistenceContext;
 import jakarta.persistence.TypedQuery;
-//import jakarta.validation.constraints.NotNull;
 
 /**
  *
@@ -40,6 +39,9 @@ public class DataverseRoleServiceBean implements java.io.Serializable {
 
     @EJB
     RoleAssigneeServiceBean roleAssigneeService;
+
+    @EJB
+    DataverseServiceBean dataverseService;
     @EJB
     IndexServiceBean indexService;
     @EJB
@@ -48,22 +50,23 @@ public class DataverseRoleServiceBean implements java.io.Serializable {
     IndexAsync indexAsync;
 
     public DataverseRole save(DataverseRole aRole) {
-        if (aRole.getId() == null) {
+        if (aRole.getId() == null) { // persist a new Role
             em.persist(aRole);
-            /**
-             * @todo Why would getId be null? Should we call
-             * indexDefinitionPoint here too? A: it's null for new roles.
-             */
-            return aRole;
-        } else {
-            DataverseRole merged = em.merge(aRole);
-            /**
-             * @todo update permissionModificationTime here.
-             */
-            IndexResponse indexDefinitionPountResult = indexDefinitionPoint(merged.getOwner());
-            logger.info("aRole getId was not null. Indexing result: " + indexDefinitionPountResult);
-            return merged;
+        } else { // update an existing Role
+            aRole = em.merge(aRole);
+        }
+
+        DvObject owner = aRole.getOwner();
+        if(owner == null) { // Builtin Role
+            owner = dataverseService.findByAlias("root");
+        }
+
+        if(owner != null) { // owner may be null if a role is created before the root collection as in setup-all.sh
+            IndexResponse indexDefinitionPointResult = indexDefinitionPoint(owner);
+            logger.info("Indexing result: " + indexDefinitionPointResult);
         }
+
+        return aRole;
     }
 
     public RoleAssignment save(RoleAssignment assignment) {

src/main/java/edu/harvard/iq/dataverse/api/Admin.java

@@ -1010,6 +1010,22 @@ public Response createNewBuiltinRole(RoleDTO roleDto) {
 			actionLogSvc.log(alr);
 		}
 	}
+	@Path("roles/{id}")
+	@PUT
+	public Response updateBuiltinRole(RoleDTO roleDto, @PathParam("id") long roleId) {
+		ActionLogRecord alr = new ActionLogRecord(ActionLogRecord.ActionType.Admin, "updateBuiltInRole")
+				.setInfo(roleDto.getAlias() + ":" + roleDto.getDescription());
+		try {
+			DataverseRole role = roleDto.updateRoleFromDTO(rolesSvc.find(roleId));
+			return ok(json(rolesSvc.save(role)));
+		} catch (Exception e) {
+			alr.setActionResult(ActionLogRecord.Result.InternalError);
+			alr.setInfo(alr.getInfo() + "// " + e.getMessage());
+			return error(Response.Status.INTERNAL_SERVER_ERROR, e.getMessage());
+		} finally {
+			actionLogSvc.log(alr);
+		}
+	}
 
 	@Path("roles")
 	@GET

src/main/java/edu/harvard/iq/dataverse/api/dto/RoleDTO.java

@@ -47,11 +47,11 @@ public void setPermissions(String[] permissions) {
 		this.permissions = permissions;
 	}
 
-	public DataverseRole asRole() {
-		DataverseRole r = new DataverseRole();
+	public DataverseRole updateRoleFromDTO(DataverseRole r) {
 		r.setAlias(alias);
 		r.setDescription(description);
 		r.setName(name);
+		r.clearPermissions();
 		if (permissions != null) {
 			if (permissions.length > 0) {
 				if (permissions[0].trim().toLowerCase().equals("all")) {
@@ -65,5 +65,9 @@ public DataverseRole asRole() {
 		}
 		return r;
 	}
+
+	public DataverseRole asRole() {
+		return updateRoleFromDTO(new DataverseRole());
+	}
 
 }

src/main/java/edu/harvard/iq/dataverse/engine/command/impl/CreateRoleCommand.java

@@ -22,12 +22,12 @@
 @RequiredPermissions(Permission.ManageDataversePermissions)
 public class CreateRoleCommand extends AbstractCommand<DataverseRole> {
 
-    private final DataverseRole created;
+    private final DataverseRole role;
     private final Dataverse dv;
 
     public CreateRoleCommand(DataverseRole aRole, DataverseRequest aRequest, Dataverse anAffectedDataverse) {
         super(aRequest, anAffectedDataverse);
-        created = aRole;
+        role = aRole;
         dv = anAffectedDataverse;
     }
 
@@ -41,16 +41,16 @@ public DataverseRole execute(CommandContext ctxt) throws CommandException {
         //Test to see if the role already exists in DB
         try {
             DataverseRole testRole = ctxt.em().createNamedQuery("DataverseRole.findDataverseRoleByAlias", DataverseRole.class)
-                    .setParameter("alias", created.getAlias())
+                    .setParameter("alias", role.getAlias())
                     .getSingleResult();
-            if (!(testRole == null)) {
+            if (testRole != null && !testRole.getId().equals(role.getId())) {
                 throw new IllegalCommandException(BundleUtil.getStringFromBundle("permission.role.not.created.alias.already.exists"), this);
             }
         } catch (NoResultException nre) {
-            //  we want no results because that meand we can create a role
+            //  we want no results because that meant we can create a role
         }
-        dv.addRole(created);
-        return ctxt.roles().save(created);
+        dv.addRole(role);
+        return ctxt.roles().save(role);
     }
 
 }

src/main/java/edu/harvard/iq/dataverse/search/SolrIndexServiceBean.java

@@ -375,6 +375,12 @@ public IndexResponse indexPermissionsOnSelfAndChildren(long definitionPointId) {
      * inheritance
      */
     public IndexResponse indexPermissionsOnSelfAndChildren(DvObject definitionPoint) {
+
+        if (definitionPoint == null) {
+            logger.log(Level.WARNING, "Cannot perform indexPermissionsOnSelfAndChildren with a definitionPoint null");
+            return null;
+        }
+
         List<DataFile> filesToReindexAsBatch = new ArrayList<>();
         /**
          * @todo Re-indexing the definition point itself seems to be necessary