[jr-rk]

References

dspace-customers issue 1083 - finding V7.

Description

For REST calls to bulk-access-control, BulkAccessControlScriptConfiguration.isAllowedToExecute collected the target uuids only from parameters named exactly -u, and then required isAdmin(context, dso) for each of them. Commons CLI's DefaultParser, which the script uses to parse the same arguments, also accepts --uuid <id>, --uuid=<id>, -u<id> and unambiguous long-option prefixes like --uu <id>. With any of those forms the uuid list in the check was empty, so the method returned true. The REST runnable does not check again (BulkAccessControl.isAuthorized returns true, because the REST layer has already checked). Starting a process only requires hasAuthority('AUTHENTICATED') (ScriptProcessesController, ScriptRestRepository). As a result, any logged-in user, not only community, collection or item admins, could change the resource policies of any community, collection or item in the repository. The CLI was not affected: BulkAccessControlCli.isAuthorized builds the parameters from the parsed command line, so they always reach the check as -u.

Before the fix, POST /api/system/scripts/bulk-access-control/processes with --uuid <community the user does not administer> returned 202 Accepted. Now it returns 403.

Instructions for Reviewers

List of changes in this PR (only BulkAccessControlScriptConfiguration changes in main code):

• The target uuids now come from parsing the parameters with the same DefaultParser and getOptions() that DSpaceRunnable uses. The arguments are built the same way as in ScriptRestRepository.constructArgs. So every spelling the script accepts is checked per object. ScriptRestRepository.startProcess is the only place that starts the REST runnable, and it always calls this check first.
• Fail closed: if parameters are given but no target uuid can be resolved (no -u, or arguments that do not parse, for example a missing required option, an unknown option, or -h only), only a site admin passes (isAdmin(context)). Before, any authenticated user passed this case. The broader isAdmin || isComColAdmin || isItemAdmin check is kept only for the parameterless call that lists and shows the script (ScriptServiceImpl.getScriptConfigurations, ScriptRestRepository.findOne). Side effect: a community, collection or item admin who sends only -h through REST now gets 403 instead of a help-only process. The UI does not send -h for this script.

BulkAccessControlScriptIT: 3 new tests, 1 changed.

• --uuid <id>, --uu <id> (commons-cli 1.9.0 DefaultParser matches long-option prefixes by default), --uuid=<id> and -u<id> on a community the user does not administer → 403 each.
• --uuid <id> on a community the user administers → 202, so the long form still works for legitimate users.
• No uuid, sent by a community admin → 403.
• Changed: bulkAccessScriptWithoutTargetUUIDParameterTest now runs as a site admin and still expects the 500 with "At least one target uuid must be provided". Before, it ran as a community admin, who is now rejected earlier.

How to test (the dspace module must be in the reactor, otherwise the test environment zip has no config-definition.xml):

mvn -pl .,dspace,dspace-api,dspace-server-webapp -am verify -DskipIntegrationTests=false -Dit.test=BulkAccessControlIT,BulkAccessControlScriptIT -Dsurefire.failIfNoSpecifiedTests=false -Dfailsafe.failIfNoSpecifiedTests=false -DfailIfNoTests=false

Results:

• With the fix (57f5c2090f): BulkAccessControlIT (CLI) 31/31 and BulkAccessControlScriptIT 10/10 pass. Checkstyle passes on dspace-api and dspace-server-webapp.
• Without the fix (BulkAccessControlScriptConfiguration from dtq-dev, new tests): bulkAccessScriptWithAlternativeUuidSpellingsOfNotAdministeredCommunityTest fails with Status expected:<403> but was:<202>, which confirms the bypass. bulkAccessScriptWithoutTargetUUIDParameterWithAdminUserOfCommunityTest fails with expected:<403> but was:<500>. The other 8 tests pass. In an earlier run on 5ec06cc0fc, --uuid=<id> and -u<id> each returned 202 on the old code too.

AI Disclosure (REQUIRED)

Were AI tools used in the creation of this PR?

• No (This PR does NOT include AI-generated or AI-assisted content that requires disclosure.)
• Yes (This PR includes AI-generated or AI-assisted content that requires disclosure.)

Claude Code wrote the patch and the tests. They were run as described above.

Checklist

• My PR is created against the main branch of code (unless it is a backport or is fixing an issue specific to an older branch).
• My PR is small in size (e.g. less than 1,000 lines of code, not including comments & integration tests). Exceptions may be made if previously agreed upon.
• My PR follows all coding best practices based on the Code Conventions Guide.
• My PR passes Checkstyle validation based on the Code Style Guide.
• My PR includes Javadoc for all new (or modified) public methods and classes. It also includes Javadoc for large or complex private methods.
• My PR passes all tests and includes new/updated Unit or Integration Tests based on the Code Testing Guide.
• My PR includes details on how to test it. I've provided clear instructions to reviewers on how to successfully test this fix or feature.
• If my PR includes new libraries/dependencies (in any pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
• If my PR modifies REST API endpoints, I've opened a separate REST Contract PR related to this change.
• If my PR includes new configurations, I've provided basic technical documentation in the PR itself.
• If my PR fixes an issue ticket, I've linked them together.

Written on dtq-dev (c1b0423240). Cherry-picks cleanly onto customer/lindat.

[Copilot]

🔵 Needs a closer look

It changes an authorization check on a script that modifies resource policies, and the fallback for unparseable input needs a human decision before merge.

2 open issues

• Clarify handling of unparseable arguments · New
• Add coverage for abbreviated UUID option prefix matching · New

What changed in this PR

This PR closes an authorization bypass in the bulk access control script. The REST pre-check only looked at parameters named exactly -u, so forms such as --uuid <id>, --uuid=<id>, -u<id> and --uu <id> skipped the per-object admin check. The fix parses the arguments with the same parser the script uses, so every accepted spelling is checked.

Changes:

• The pre-check in BulkAccessControlScriptConfiguration now reads target UUIDs from the parsed command line. If parsing fails, it uses the general admin check, and the script then rejects the same input during initialization.
• BulkAccessControl.isAuthorized re-checks permissions at run time against the parsed command line. The command-line subclass's duplicate override was removed, so its behavior is unchanged.
• BulkAccessControlScriptIT gains three tests: the long --uuid form and the inline --uuid= and -u forms on a community the user does not administer, plus the long form on an administered community.

File	Description
dspace-server-webapp/​src/​test/​java/​org/​dspace/​app/​bulkaccesscontrol/​BulkAccessControlScriptIT.java	Adds REST tests for the new UUID spellings, both denied and allowed cases.
dspace-api/​src/​main/​java/​org/​dspace/​app/​bulkaccesscontrol/​BulkAccessControlScriptConfiguration.java	Parses target UUIDs with the script's own parser so the pre-check covers every accepted spelling.
dspace-api/​src/​main/​java/​org/​dspace/​app/​bulkaccesscontrol/​BulkAccessControlCli.java	Removes the isAuthorized override that now lives in the parent class. Behavior is unchanged.
dspace-api/​src/​main/​java/​org/​dspace/​app/​bulkaccesscontrol/​BulkAccessControl.java	Adds the run-time permission check against the parsed command line as a second layer.

_{Review effort: Lite}

[Copilot]

🔵 Needs a closer look

It changes authorization for a script that edits resource policies, so a human security review should confirm the behaviour change.

No open issues 🎉

2 resolved since last review

• Clarify handling of unparseable arguments
• Add coverage for abbreviated UUID option prefix matching

_{Review effort: Lite}

[Copilot]

🔵 Needs a closer look

The fix looks correct and is well tested, but it changes authorization for a script that modifies resource policies, so it needs human sign-off.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Rename test to clarify community admin access is forbidden

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

The name suggests a success case, but the test expects 403 for a community admin who sends no target uuid. Rename it so the expected denial is clear, for example to bulkAccessScriptWithoutTargetUUIDParameterForbiddenForCommunityAdminTest.

_{Review effort: Lite}

[milanmajchrak]

How --uuid skipped the check

The pre-check and the script read the uuid parameters differently. Now the check parses them the same way as the script.

flowchart TD
  R["REST call<br/>--uuid X"] --> C{"isAllowedToExecute"}
  C -->|before| B["sees only -u,<br/>finds no target,<br/>allows"]
  B --> S["script reads<br/>--uuid as -u,<br/>changes policies<br/>under X"]
  C -->|after| A["parses like the<br/>script, finds X"]
  A --> F["403 unless<br/>admin of X"]

Loading

Who	Sends	Before	After
Any logged-in user	--uuid X, --uuid=X, -uX or --uu X	202, policies under X change	403
Same user	-u X	403	403
Admin of community A, via UI	-u A	202	202
Admin of community A	--uuid A	202	202
Admin of community A	no uuid, or only -h	passes the check, then 500 or help	403

"Under X" means the items the caller can find in search. The script looks them up in Solr with the caller's rights.