From 5ec06cc0fc1da6c8e497ede9300ec6ed48bb7cc5 Mon Sep 17 00:00:00 2001 From: Juraj Roka <95219754+jr-rk@users.noreply.github.com> Date: Tue, 27 Oct 2026 14:12:00 +0100 Subject: [PATCH 1/3] UFAL/Fix dspace-customers#1083 V7: check bulk-access-control target uuids however they are spelled BulkAccessControlScriptConfiguration.isAllowedToExecute only collected target uuids from parameters named exactly "-u". Commons CLI also accepts "--uuid", "--uuid=", "-u" and unambiguous prefixes like "--uu", so a community, collection or item admin could pass any of those forms, skip the per-object admin check, and change resource policies of objects they do not administer. - Resolve the target uuids by parsing the parameters with the same DefaultParser and options the runnable uses. Unparseable arguments fall back to the general eligibility check; they fail the same way when the script initializes, so nothing is modified. - Re-check authorization at runtime in BulkAccessControl (REST runnable) against the parsed command line, as the CLI subclass already did; the CLI override is now the inherited default. - Add REST ITs for "--uuid", "--uuid=" and "-u" on a not administered community (403) and for "--uuid" on an administered one (202). --- .../bulkaccesscontrol/BulkAccessControl.java | 16 +++- .../BulkAccessControlCli.java | 17 ---- .../BulkAccessControlScriptConfiguration.java | 41 ++++++++-- .../BulkAccessControlScriptIT.java | 82 +++++++++++++++++++ 4 files changed, 130 insertions(+), 26 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControl.java b/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControl.java index 333aa995e9a6..c8e55d0f7c52 100644 --- a/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControl.java +++ b/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControl.java @@ -64,6 +64,7 @@ import org.dspace.eperson.EPerson; import org.dspace.eperson.factory.EPersonServiceFactory; import org.dspace.eperson.service.EPersonService; +import org.dspace.scripts.DSpaceCommandLineParameter; import org.dspace.scripts.DSpaceRunnable; import org.dspace.services.ConfigurationService; import org.dspace.services.factory.DSpaceServicesFactory; @@ -680,8 +681,21 @@ private boolean isAppendModeEnabled() { return configurationService.getBooleanProperty("core.authorization.installitem.inheritance-read.append-mode"); } + /** + * Re-check the permissions against the parsed command line, so the target uuids are exactly the ones this + * run will modify, regardless of how the parameters were spelled when the process was requested. + */ protected boolean isAuthorized(Context context) { - return true; + + if (context.getCurrentUser() == null) { + return false; + } + + return getScriptConfiguration().isAllowedToExecute(context, + Arrays.stream(commandLine.getOptions()) + .map(option -> + new DSpaceCommandLineParameter("-" + option.getOpt(), option.getValue())) + .collect(Collectors.toList())); } @Override diff --git a/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlCli.java b/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlCli.java index 4e8cfe480eeb..6faf81bf141c 100644 --- a/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlCli.java +++ b/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlCli.java @@ -8,14 +8,11 @@ package org.dspace.app.bulkaccesscontrol; import java.sql.SQLException; -import java.util.Arrays; import java.util.UUID; -import java.util.stream.Collectors; import org.apache.commons.lang3.StringUtils; import org.dspace.core.Context; import org.dspace.eperson.EPerson; -import org.dspace.scripts.DSpaceCommandLineParameter; /** * Extension of {@link BulkAccessControl} for CLI. @@ -49,18 +46,4 @@ protected void setEPerson(Context context) throws SQLException { context.setCurrentUser(myEPerson); } - - @Override - protected boolean isAuthorized(Context context) { - - if (context.getCurrentUser() == null) { - return false; - } - - return getScriptConfiguration().isAllowedToExecute(context, - Arrays.stream(commandLine.getOptions()) - .map(option -> - new DSpaceCommandLineParameter("-" + option.getOpt(), option.getValue())) - .collect(Collectors.toList())); - } } diff --git a/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptConfiguration.java index 5196247f94cb..813d19ff17cc 100644 --- a/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptConfiguration.java +++ b/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; @@ -38,16 +40,11 @@ public class BulkAccessControlScriptConfiguration e public boolean isAllowedToExecute(Context context, List commandLineParameters) { try { - if (Objects.isNull(commandLineParameters)) { + String[] dspaceObjectIDs = getTargetUuids(commandLineParameters); + if (Objects.isNull(dspaceObjectIDs)) { return authorizeService.isAdmin(context) || authorizeService.isComColAdmin(context) || authorizeService.isItemAdmin(context); } else { - List dspaceObjectIDs = - commandLineParameters.stream() - .filter(parameter -> "-u".equals(parameter.getName())) - .map(DSpaceCommandLineParameter::getValue) - .collect(Collectors.toList()); - DSpaceObjectUtils dSpaceObjectUtils = new DSpace().getServiceManager().getServiceByName( DSpaceObjectUtilsImpl.class.getName(), DSpaceObjectUtilsImpl.class); @@ -71,6 +68,34 @@ public boolean isAllowedToExecute(Context context, List commandLineParameters) { + if (Objects.isNull(commandLineParameters)) { + return null; + } + + List 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) { diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptIT.java index da0f90ca97c7..45bb8ab40fae 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptIT.java +++ b/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}. @@ -499,4 +500,85 @@ public void bulkAccessScriptWithNormalUserTest() throws Exception { .andExpect(status().isForbidden()); } + @Test + public void bulkAccessScriptWithLongUuidOptionOfNotAdministeredCommunityTest() throws Exception { + context.turnOffAuthorisationSystem(); + CommunityBuilder.createCommunity(context) + .withName("Administered Community") + .withAdminGroup(eperson) + .build(); + parentCommunity = CommunityBuilder.createCommunity(context) + .withName("Other Community") + .build(); + context.restoreAuthSystemState(); + + performBulkAccessScript(eperson.getEmail(), + new DSpaceCommandLineParameter("--uuid", parentCommunity.getID().toString())) + .andExpect(status().isForbidden()); + } + + @Test + public void bulkAccessScriptWithAttachedUuidOptionOfNotAdministeredCommunityTest() throws Exception { + context.turnOffAuthorisationSystem(); + CommunityBuilder.createCommunity(context) + .withName("Administered Community") + .withAdminGroup(eperson) + .build(); + parentCommunity = CommunityBuilder.createCommunity(context) + .withName("Other Community") + .build(); + context.restoreAuthSystemState(); + + performBulkAccessScript(eperson.getEmail(), + new DSpaceCommandLineParameter("--uuid=" + parentCommunity.getID(), null)) + .andExpect(status().isForbidden()); + + performBulkAccessScript(eperson.getEmail(), + new DSpaceCommandLineParameter("-u" + parentCommunity.getID(), null)) + .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 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))); + } + } From f7ff7fa0523138a36a7adf2fbd3f6fb439e9c997 Mon Sep 17 00:00:00 2001 From: Juraj Roka <95219754+jr-rk@users.noreply.github.com> Date: Tue, 27 Oct 2026 15:17:25 +0100 Subject: [PATCH 2/3] UFAL/Fix dspace-customers#1083 V7: fail closed when no bulk-access-control target uuid resolves Address review: when parameters are given but no target uuid can be resolved (no -u, or arguments that do not parse), only a site admin passes the check now. Before, any community, collection or item admin passed and the safety relied on the script failing the same parse during initialize. The broader admin check stays only for the parameterless call used to list and show the script. - bulkAccessScriptWithoutTargetUUIDParameterTest runs as a site admin and still expects the "At least one target uuid" error. - New: a community admin without a uuid gets 403. - New: the abbreviated long option "--uu " on a not administered community gets 403. --- .../BulkAccessControlScriptConfiguration.java | 17 +++++---- .../BulkAccessControlScriptIT.java | 37 +++++++++++++++++-- 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptConfiguration.java b/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptConfiguration.java index 813d19ff17cc..443cc8ad8206 100644 --- a/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptConfiguration.java @@ -40,10 +40,16 @@ public class BulkAccessControlScriptConfiguration e public boolean isAllowedToExecute(Context context, List commandLineParameters) { try { - String[] dspaceObjectIDs = getTargetUuids(commandLineParameters); - if (Objects.isNull(dspaceObjectIDs)) { + if (Objects.isNull(commandLineParameters)) { return authorizeService.isAdmin(context) || authorizeService.isComColAdmin(context) || authorizeService.isItemAdmin(context); + } + + 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); @@ -73,14 +79,9 @@ public boolean isAllowedToExecute(Context context, List commandLineParameters) { - if (Objects.isNull(commandLineParameters)) { - return null; - } - List args = new ArrayList<>(); for (DSpaceCommandLineParameter parameter : commandLineParameters) { args.add(parameter.getName()); diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptIT.java index 45bb8ab40fae..efd856105788 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptIT.java @@ -432,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) @@ -448,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 @@ -517,6 +529,23 @@ public void bulkAccessScriptWithLongUuidOptionOfNotAdministeredCommunityTest() t .andExpect(status().isForbidden()); } + @Test + public void bulkAccessScriptWithAbbreviatedUuidOptionOfNotAdministeredCommunityTest() throws Exception { + context.turnOffAuthorisationSystem(); + CommunityBuilder.createCommunity(context) + .withName("Administered Community") + .withAdminGroup(eperson) + .build(); + parentCommunity = CommunityBuilder.createCommunity(context) + .withName("Other Community") + .build(); + context.restoreAuthSystemState(); + + performBulkAccessScript(eperson.getEmail(), + new DSpaceCommandLineParameter("--uu", parentCommunity.getID().toString())) + .andExpect(status().isForbidden()); + } + @Test public void bulkAccessScriptWithAttachedUuidOptionOfNotAdministeredCommunityTest() throws Exception { context.turnOffAuthorisationSystem(); From 57f5c2090f6cdf6c13c47957b4cde9153602f323 Mon Sep 17 00:00:00 2001 From: Juraj Roka <95219754+jr-rk@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:38:56 +0100 Subject: [PATCH 3/3] UFAL/Fix dspace-customers#1083 V7: keep the fix in the REST pre-check only The REST runnable is started only by ScriptRestRepository.startProcess, which always runs BulkAccessControlScriptConfiguration.isAllowedToExecute on the same arguments that the runnable parses. Now that the pre-check parses them with the same parser and options, re-checking at run time adds nothing. Restore BulkAccessControl.isAuthorized and the BulkAccessControlCli override as they were. Merge the four tests for alternative uuid spellings (--uuid, --uu, --uuid=, -u) into one test with the same assertions. --- .../bulkaccesscontrol/BulkAccessControl.java | 16 +----- .../BulkAccessControlCli.java | 17 ++++++ .../BulkAccessControlScriptIT.java | 52 ++++--------------- 3 files changed, 28 insertions(+), 57 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControl.java b/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControl.java index c8e55d0f7c52..333aa995e9a6 100644 --- a/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControl.java +++ b/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControl.java @@ -64,7 +64,6 @@ import org.dspace.eperson.EPerson; import org.dspace.eperson.factory.EPersonServiceFactory; import org.dspace.eperson.service.EPersonService; -import org.dspace.scripts.DSpaceCommandLineParameter; import org.dspace.scripts.DSpaceRunnable; import org.dspace.services.ConfigurationService; import org.dspace.services.factory.DSpaceServicesFactory; @@ -681,21 +680,8 @@ private boolean isAppendModeEnabled() { return configurationService.getBooleanProperty("core.authorization.installitem.inheritance-read.append-mode"); } - /** - * Re-check the permissions against the parsed command line, so the target uuids are exactly the ones this - * run will modify, regardless of how the parameters were spelled when the process was requested. - */ protected boolean isAuthorized(Context context) { - - if (context.getCurrentUser() == null) { - return false; - } - - return getScriptConfiguration().isAllowedToExecute(context, - Arrays.stream(commandLine.getOptions()) - .map(option -> - new DSpaceCommandLineParameter("-" + option.getOpt(), option.getValue())) - .collect(Collectors.toList())); + return true; } @Override diff --git a/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlCli.java b/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlCli.java index 6faf81bf141c..4e8cfe480eeb 100644 --- a/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlCli.java +++ b/dspace-api/src/main/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlCli.java @@ -8,11 +8,14 @@ package org.dspace.app.bulkaccesscontrol; import java.sql.SQLException; +import java.util.Arrays; import java.util.UUID; +import java.util.stream.Collectors; import org.apache.commons.lang3.StringUtils; import org.dspace.core.Context; import org.dspace.eperson.EPerson; +import org.dspace.scripts.DSpaceCommandLineParameter; /** * Extension of {@link BulkAccessControl} for CLI. @@ -46,4 +49,18 @@ protected void setEPerson(Context context) throws SQLException { context.setCurrentUser(myEPerson); } + + @Override + protected boolean isAuthorized(Context context) { + + if (context.getCurrentUser() == null) { + return false; + } + + return getScriptConfiguration().isAllowedToExecute(context, + Arrays.stream(commandLine.getOptions()) + .map(option -> + new DSpaceCommandLineParameter("-" + option.getOpt(), option.getValue())) + .collect(Collectors.toList())); + } } diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptIT.java index efd856105788..f328f25b1877 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/bulkaccesscontrol/BulkAccessControlScriptIT.java @@ -513,7 +513,7 @@ public void bulkAccessScriptWithNormalUserTest() throws Exception { } @Test - public void bulkAccessScriptWithLongUuidOptionOfNotAdministeredCommunityTest() throws Exception { + public void bulkAccessScriptWithAlternativeUuidSpellingsOfNotAdministeredCommunityTest() throws Exception { context.turnOffAuthorisationSystem(); CommunityBuilder.createCommunity(context) .withName("Administered Community") @@ -524,47 +524,15 @@ public void bulkAccessScriptWithLongUuidOptionOfNotAdministeredCommunityTest() t .build(); context.restoreAuthSystemState(); - performBulkAccessScript(eperson.getEmail(), - new DSpaceCommandLineParameter("--uuid", parentCommunity.getID().toString())) - .andExpect(status().isForbidden()); - } - - @Test - public void bulkAccessScriptWithAbbreviatedUuidOptionOfNotAdministeredCommunityTest() throws Exception { - context.turnOffAuthorisationSystem(); - CommunityBuilder.createCommunity(context) - .withName("Administered Community") - .withAdminGroup(eperson) - .build(); - parentCommunity = CommunityBuilder.createCommunity(context) - .withName("Other Community") - .build(); - context.restoreAuthSystemState(); - - performBulkAccessScript(eperson.getEmail(), - new DSpaceCommandLineParameter("--uu", parentCommunity.getID().toString())) - .andExpect(status().isForbidden()); - } - - @Test - public void bulkAccessScriptWithAttachedUuidOptionOfNotAdministeredCommunityTest() throws Exception { - context.turnOffAuthorisationSystem(); - CommunityBuilder.createCommunity(context) - .withName("Administered Community") - .withAdminGroup(eperson) - .build(); - parentCommunity = CommunityBuilder.createCommunity(context) - .withName("Other Community") - .build(); - context.restoreAuthSystemState(); - - performBulkAccessScript(eperson.getEmail(), - new DSpaceCommandLineParameter("--uuid=" + parentCommunity.getID(), null)) - .andExpect(status().isForbidden()); - - performBulkAccessScript(eperson.getEmail(), - new DSpaceCommandLineParameter("-u" + parentCommunity.getID(), null)) - .andExpect(status().isForbidden()); + 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