UFAL/Fix dspace-customers#1083 V7: check bulk-access-control target uuids however they are spelled

dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptConfiguration.java

@@ -9,12 +9,14 @@
 
 import java.io.InputStream;
 import java.sql.SQLException;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Objects;
 import java.util.UUID;
-import java.util.stream.Collectors;
 
+import org.apache.commons.cli.DefaultParser;
 import org.apache.commons.cli.Options;
+import org.apache.commons.cli.ParseException;
 import org.dspace.app.util.DSpaceObjectUtilsImpl;
 import org.dspace.app.util.service.DSpaceObjectUtils;
 import org.dspace.content.DSpaceObject;
@@ -41,13 +43,14 @@ public boolean isAllowedToExecute(Context context, List<DSpaceCommandLineParamet
             if (Objects.isNull(commandLineParameters)) {
                 return authorizeService.isAdmin(context) || authorizeService.isComColAdmin(context)
                     || authorizeService.isItemAdmin(context);
-            } else {
-                List<String> dspaceObjectIDs =
-                    commandLineParameters.stream()
-                                         .filter(parameter -> "-u".equals(parameter.getName()))
-                                         .map(DSpaceCommandLineParameter::getValue)
-                                         .collect(Collectors.toList());
+            }
 
+            String[] dspaceObjectIDs = getTargetUuids(commandLineParameters);
+            if (Objects.isNull(dspaceObjectIDs)) {
+                // Without target uuids there is nothing to check per object, so fail closed: only a site admin
+                // may go on. The run would fail before modifying anything, but this check must not rely on that.
+                return authorizeService.isAdmin(context);
+            } else {
                 DSpaceObjectUtils dSpaceObjectUtils = new DSpace().getServiceManager().getServiceByName(
                     DSpaceObjectUtilsImpl.class.getName(), DSpaceObjectUtilsImpl.class);
 
@@ -71,6 +74,29 @@ public boolean isAllowedToExecute(Context context, List<DSpaceCommandLineParamet
         return true;
     }
 
+    /**
+     * Resolve the target uuids exactly as the runnable will, by parsing the parameters with the same parser and
+     * options. Matching parameter names (e.g. only "-u") is not enough: "--uuid", "--uuid=X", "-uX" or an
+     * abbreviated "--uu" are all accepted by the parser and would otherwise skip the per-object check.
+     *
+     * @return the target uuids, or null when there is no uuid or the parameters cannot be parsed
+     */
+    private String[] getTargetUuids(List<DSpaceCommandLineParameter> commandLineParameters) {
+        List<String> args = new ArrayList<>();
+        for (DSpaceCommandLineParameter parameter : commandLineParameters) {
+            args.add(parameter.getName());
+            if (parameter.getValue() != null) {
+                args.add(parameter.getValue());
+            }
+        }
+
+        try {
+            return new DefaultParser().parse(getOptions(), args.toArray(new String[0])).getOptionValues('u');
+        } catch (ParseException e) {
+            return null;
+        }
+    }
+
     @Override
     public Options getOptions() {
         if (options == null) {

dspace-server-webapp/src/test/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptIT.java

@@ -50,6 +50,7 @@
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.http.MediaType;
 import org.springframework.mock.web.MockMultipartFile;
+import org.springframework.test.web.servlet.ResultActions;
 
 /**
  * Basic integration testing for the bulk access Import feature via UI {@link BulkAccessControl}.
@@ -431,6 +432,21 @@ public void bulkAccessScriptWithMultipleTargetUuidsWithAdminUserTest() throws Ex
 
     @Test
     public void bulkAccessScriptWithoutTargetUUIDParameterTest() throws Exception {
+        String token = getAuthToken(admin.getEmail(), password);
+
+        getClient(token)
+            .perform(
+                multipart(CURATE_SCRIPT_ENDPOINT)
+                    .param("properties", new ObjectMapper().writeValueAsString(List.of()))
+            )
+            .andExpect(status().isInternalServerError())
+            .andExpect(result -> assertTrue(result.getResolvedException()
+                                                  .getMessage()
+                                                  .contains("At least one target uuid must be provided")));
+    }
+
+    @Test
+    public void bulkAccessScriptWithoutTargetUUIDParameterWithAdminUserOfCommunityTest() throws Exception {
         context.turnOffAuthorisationSystem();
 
         parentCommunity = CommunityBuilder.createCommunity(context)
@@ -447,10 +463,7 @@ public void bulkAccessScriptWithoutTargetUUIDParameterTest() throws Exception {
                 multipart(CURATE_SCRIPT_ENDPOINT)
                     .param("properties", new ObjectMapper().writeValueAsString(List.of()))
             )
-            .andExpect(status().isInternalServerError())
-            .andExpect(result -> assertTrue(result.getResolvedException()
-                                                  .getMessage()
-                                                  .contains("At least one target uuid must be provided")));
+            .andExpect(status().isForbidden());
     }
 
     @Test
@@ -499,4 +512,70 @@ public void bulkAccessScriptWithNormalUserTest() throws Exception {
             .andExpect(status().isForbidden());
     }
 
+    @Test
+    public void bulkAccessScriptWithAlternativeUuidSpellingsOfNotAdministeredCommunityTest() throws Exception {
+        context.turnOffAuthorisationSystem();
+        CommunityBuilder.createCommunity(context)
+                        .withName("Administered Community")
+                        .withAdminGroup(eperson)
+                        .build();
+        parentCommunity = CommunityBuilder.createCommunity(context)
+                                          .withName("Other Community")
+                                          .build();
+        context.restoreAuthSystemState();
+
+        String uuid = parentCommunity.getID().toString();
+        for (DSpaceCommandLineParameter uuidParameter : List.of(
+            new DSpaceCommandLineParameter("--uuid", uuid),
+            new DSpaceCommandLineParameter("--uu", uuid),
+            new DSpaceCommandLineParameter("--uuid=" + uuid, null),
+            new DSpaceCommandLineParameter("-u" + uuid, null))) {
+            performBulkAccessScript(eperson.getEmail(), uuidParameter)
+                .andExpect(status().isForbidden());
+        }
+    }
+
+    @Test
+    public void bulkAccessScriptWithLongUuidOptionOfAdministeredCommunityTest() throws Exception {
+        context.turnOffAuthorisationSystem();
+        parentCommunity = CommunityBuilder.createCommunity(context)
+                                          .withName("Administered Community")
+                                          .withAdminGroup(eperson)
+                                          .build();
+        context.restoreAuthSystemState();
+
+        performBulkAccessScript(eperson.getEmail(),
+            new DSpaceCommandLineParameter("--uuid", parentCommunity.getID().toString()))
+            .andExpect(status().isAccepted());
+    }
+
+    private ResultActions performBulkAccessScript(String email, DSpaceCommandLineParameter uuidParameter)
+        throws Exception {
+        String json = "{ \"item\": {\n" +
+            "      \"mode\": \"replace\",\n" +
+            "      \"accessConditions\": [\n" +
+            "          {\n" +
+            "            \"name\": \"openaccess\"\n" +
+            "          }\n" +
+            "      ]\n" +
+            "   }}\n";
+
+        MockMultipartFile bitstreamFile = new MockMultipartFile("file", "test.json", MediaType.TEXT_PLAIN_VALUE,
+            new ByteArrayInputStream(json.getBytes(StandardCharsets.UTF_8)));
+
+        List<ParameterValueRest> list =
+            List.of(uuidParameter, new DSpaceCommandLineParameter("-f", "test.json")).stream()
+                .map(dSpaceCommandLineParameter -> dSpaceRunnableParameterConverter
+                    .convert(dSpaceCommandLineParameter, Projection.DEFAULT))
+                .collect(Collectors.toList());
+
+        String token = getAuthToken(email, password);
+
+        return getClient(token)
+            .perform(
+                multipart(CURATE_SCRIPT_ENDPOINT)
+                    .file(bitstreamFile)
+                    .param("properties", new ObjectMapper().writeValueAsString(list)));
+    }
+
 }