VSB-TUO/fix: embargo synchronisation left ORIGINAL bitstreams with zero resource policies (HTTP 401) - #1415
Merged
Conversation
Running `dspace itemupdate -a dc.date.embargoend` with an embargo end date
that has already passed left the ORIGINAL bitstreams of the item with zero
resource policies. Every download answered HTTP 401 and the item was shown
as `accessStatus = restricted` even though its metadata said
`dc.rights.access = openAccess`. Confirmed on dspace7-test.vsb.cz on item
4dde91f3-7078-4241-9938-9b8488623bb1: 401 on all 18 bitstreams.
Cause
-----
`ItemUpdate.syncEmbargoPolicies()` deleted before it validated.
`clearExistingSafEmbargoPolicies()` ran as the very first statement and
removed every dated Anonymous/READ policy on the ORIGINAL bitstreams; only
afterwards did the method look at the date, find it in the past, print a
warning and return without creating a replacement. Because the previous run
had already removed the collection's undated default policy
(`removeImmediateAnonymousReadPolicies`), that dated policy was the only
one left, so the file ended up with none at all. The same wipe happened for
an end date of today (the "is it past" test ran before the "+1 day"), for a
removed, empty or unparseable `dc.date.embargoend`, and regardless of
`dc.rights.access`. In every case the exit code was 0.
Fix
---
`syncEmbargoPolicies()` is now ordered objections -> validation -> mutation,
and nothing is deleted before its replacement is stored:
* Refuses to act on a withdrawn item, on an item that is not archived, and
on any `dc.rights.access` value outside {openAccess, embargoedAccess} -
a single unknown or restrictive value blocks the whole item.
* Validates `dc.date.embargoend` (strict `LocalDate.parse`; `DCDate` rolls
2026-02-30 over into 2026-03-02) before touching a policy.
* Computes the start date as `embargoend + 1 day` at midnight UTC, so the
day boundary no longer depends on the server time zone, and an embargo
ending today keeps the file closed today.
* An expired embargo is a publication, not a deletion: the real, already
passed start date is written and the file becomes readable.
* Instead of delete + create, the existing Anonymous/READ policy is mutated
in place ("survivor", the dated one with the oldest start date), then
normalised to `TYPE_CUSTOM` / rpName `embargo`, then the duplicates are
removed. Survivors are found by (group, action) and not by rpName, so the
legacy "Standard Embargo" / "Special Case Embargo" rows already in the
customer database are adopted and normalised.
* A bitstream without an Anonymous/READ policy never gets one invented -
that would widen access instead of re-dating it. It is reported, together
with a pointer to `dspace bulk-access-control`.
* Removing `dc.date.embargoend` lifts the embargo (`startDate = null`),
which is how an operator is supposed to re-open a file.
* `ItemUpdate` gained a real log4j2 logger and `prWarn`/`prErr`, and every
refusal increments `embargoSyncFailures`, which makes `main()` exit 1 -
the problems used to be invisible to a script.
`ItemImportServiceImpl` carried three latent defects on the neighbouring
import path, fixed here as well:
* rpName "Special Case Embargo - No access rights metadata" is 48
characters and the column is `varchar(30)`; on PostgreSQL this aborts the
whole import. Both scenarios now write `embargo`, the access condition
name from access-conditions.xml.
* The past-date guard ran before the "+1 day", so an embargo ending today
was dropped. Replaced with the same UTC calendar-day arithmetic.
* The created policy had no `rpType`, and `processEmbargoMetadata` ran on
the workflow branch too, i.e. before the submission was approved. It is
now `TYPE_CUSTOM` and only applied when `!useWorkflow`.
The import path still creates the policy - there is no Anonymous/READ
policy on a bitstream before `installItem` - so the survivor rule of
`ItemUpdate` is deliberately not shared with it.
Tests
-----
New: EmbargoPastDateIT (reproduces the customer 401), EmbargoDateBoundaryIT,
EmbargoSafetyIT, EmbargoLifecycleIT - 28 integration tests covering the
boundary days, the "never zero READ policies" invariant, withdrawn and
non-archived items, unknown access rights and the legacy rpNames.
Changed: six ItemUpdateIT tests and EmbargoImportIT.testPastEmbargoDateNoPolicy
asserted only the absence of a policy named "Standard Embargo" or the absence
of a start date. A bitstream stripped of every policy passes those assertions
just as well as a correct one, which is precisely why the bug survived them.
They now assert the identity of the surviving policy and whether an anonymous
visitor can actually read the file.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n embargoes The review of f0c93ab found two ways this branch could itself publish an embargoed file. Both are closed here. Leak 1 - approving a workflow import published the files -------------------------------------------------------- f0c93ab moved `processEmbargoMetadata()` out of the common path of `ItemImportServiceImpl.addItem()` into the `!useWorkflow` branch, so `dspace import -a -w` created no embargo policy at all. On approval, `XmlWorkflowServiceImpl.archive` -> `installItem` -> `inheritCollectionDefaultPolicies(.., false)` -> `adjustBundleBitstreamPolicies` -> `removeAllPoliciesAndAddDefault` -> `addDefaultPoliciesNotInPlace` finds a bitstream with no Anonymous READ policy and clones the collection's *undated* `DEFAULT_BITSTREAM_READ` onto it. An item carrying `dc.rights.access=embargoedAccess` and a future `dc.date.embargoend` was therefore public from the second it was approved. The call is back on the common path. What kept `installItem` from cloning that default before the move was never the `rpType`, as the comment claimed, but `addDefaultPoliciesNotInPlace` -> `AuthorizeServiceImpl.isAnIdenticalPolicyAlreadyInPlace`, which matches on `(dso, group, action)` alone and therefore already sees the embargo policy; the comment says so now. `TYPE_CUSTOM` - the other half of f0c93ab - is what makes creating the policy early safe: `AuthorizeServiceImpl` skips custom policies on a bitstream that belongs to no installed item (DS-2614), so an item waiting for approval discloses nothing. `EmbargoImportIT.testWorkflowEmbargoSurvivesApproval` imports with `-w`, claims and approves the review task, then asserts that the file is not anonymously readable and that exactly one dated Anonymous READ policy remains. Moving the call back into the `!useWorkflow` branch fails it on precisely that assertion. Leak 2 - a missing dc.date.embargoend lifted every embargo in the batch ----------------------------------------------------------------------- Row 5 of the specification made an absent `dc.date.embargoend` clear the start date of the surviving policy. Since the survivor is located by `(group=Anonymous, action=READ)` and not by `rpName`, that also cleared embargoes `itemupdate` never set: the submission access condition and `dspace bulk-access-control` write precisely the same policy (Anonymous/READ, `TYPE_CUSTOM`, rpName `embargo`, future start date). `syncEmbargoPolicies` runs for every item of a batch as soon as `-a`/`-d` names an embargo field - the documented VSB command - so one SAF package without the field would have published every embargoed ORIGINAL bitstream in that batch. An absent `dc.date.embargoend` is now "no instruction": nothing is touched, the set of policy ids stays identical, the exit code stays 0, and an INFO line says so. The only way to open a file is a `dc.date.embargoend` in the past, which is the path that was already implemented and tested. The two "lift" tests are rewritten and renamed (`EmbargoLifecycleIT.removingEmbargoMetadataLeavesPoliciesUntouched`, `ItemUpdateIT.processArchiveUpdateRemovingEmbargoMetadataLeavesPoliciesUntouched`), and `EmbargoLifecycleIT.foreignEmbargoIsNeverLifted` covers the finding directly: it puts the submission's own policy on a bitstream and then runs a batch that does not mention the embargo. Exit code --------- Rows 6, 7, 8 and 11 require a non-zero exit code, but every test threw its `ItemUpdate` instance away, so `embargoSyncFailures` and its use in `main()` were completely uncovered. The mapping now lives in `ItemUpdate.exitStatus(status, failures)`, which `main()` calls and `ItemUpdateIT.embargoSyncFailuresDecideTheExitCode` asserts directly, and every `runItemUpdate` helper keeps its instance: 1 reported problem for rows 6/7/8/11, 0 for every run the tool is supposed to carry out. Also ---- * `EmbargoSafetyIT.withdrawnItemIsNeverRepublished` was vacuous with respect to its own subject: `ItemServiceImpl.withdraw()` also clears `archived`, so the `!isArchived` guard satisfied every assertion and the test passed with the withdrawal guard deleted. It now asserts that the console says "is withdrawn". * `EmbargoImportIT.testStandardEmbargoImport` and `testMultipleBitstreamsEmbargo` picked the embargo policy with `findFirst()`, so a second, undated policy next to it - exactly what leak 1 produces - passed unnoticed. They now count the Anonymous READ policies and ask the authorisation system whether an anonymous visitor can download the file. * Three `ItemUpdateIT` fixtures left the collection's undated default policy next to the embargo policy, so the file was readable throughout and their "nothing changed" assertions could not have detected a leak. The default is removed now and the fixtures assert that they really are embargoed. * The import-path "special case" branch (`dc.date.embargoend` without `dc.rights.access=embargoedAccess`) had no test at all - which is why nobody noticed it wrote a 48 character rpName into a `varchar(30)` column. `EmbargoImportIT.testEmbargoEndWithoutAccessRightsStillEmbargoes` covers it. * `EMBARGO_POLICY_NAME` was declared twice. Both tools now read `org.dspace.app.util.SafEmbargoConstants.EMBARGO_POLICY_NAME`: import writes the rpName, itemupdate later adopts it, and the two must not drift apart. 49 integration tests, 0 failures; checkstyle 0 violations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…goed item The review of f26f648 found one more way this branch can publish a closed file, and it is the branch's own doing. Both commits replaced `new DCDate(str)` with a strict `LocalDate.parse(str)`. On the `itemupdate` path that is fail-closed - the item is refused, no policy is touched, exit code 1. On the import path the very same parse failure was a log line and a `return`: catch (DateTimeParseException e) { logError(...); return; } no embargo policy is created, `addItem` archives the item anyway, `installItem` -> `addDefaultPoliciesNotInPlace` clones the collection's *undated* DEFAULT_BITSTREAM_READ onto the bitstream, and the file is public although its own metadata says `dc.rights.access=embargoedAccess`. Exit code 0. That half was fail-open before this branch too, but the branch widened the set of inputs that fall into it: `DCDate` accepted `yyyy`, `yyyy-MM` and four full ISO timestamp shapes, `LocalDate.parse` accepts `yyyy-MM-dd` and nothing else. A package with `dc.date.embargoend=2099` used to produce a dated policy on `origin/customer/vsb-tuo` and produced none here. Reproduced on the branch code before touching it, with a SAF package carrying `embargoedAccess` + `dc.date.embargoend=invalid-date-format`: item=05f0fee9-37c7-4443-94ea-21caeda8553d archived=true bitstream=e8f96c7f-826d-4b08-9e4e-86a30b85f375 anonymousCanRead=true id=257 group=Anonymous rpType=TYPE_INHERITED rpName=null start=null Backwards compatible parsing ---------------------------- `org.dspace.app.util.SafEmbargoDateParser` reads the shapes `DCDate` read, and maps each one to the day `DCDate.toDate()` mapped it to - verified against `DCDate` itself, not from memory: * `yyyy-MM-dd`, and `yyyy-M-d` unpadded * `yyyy-MM-dd'T'HH[:mm[:ss[.fff]]]['Z']` - the UTC day of the instant, the time of day is dropped * `yyyy-MM` -> the *first* day of that month * `yyyy` -> *1 January* of that year, not 31 December. `DCDate` keeps a granularity but `toDate()` returns the first instant of the period, and the old code used that `Date` as the embargo end, so `2099` has always meant "closed until 1 January 2099, open on the 2nd". Widening it to the end of the year would extend embargoes the operators already live with. Deliberately not kept from `DCDate`: the lenient roll-over (`2026-02-30` became 2 March, i.e. a typo became a real embargo date), trailing garbage (`SimpleDateFormat` read `2099garbage` as the year 2099), and a numeric UTC offset, which `DCDate` mis-read as UTC anyway. All three now throw, and a throw means "refuse the package", never "no embargo". `ItemUpdate` uses the same parser, or the same SAF package would mean two different days in the two tools; on that path an unreadable date keeps its existing behaviour (policies untouched, `embargoSyncFailures`, exit 1). Fail closed on the import path ------------------------------ Every early return of `processEmbargoMetadata` and `applyEmbargoToItemBitstreams` was audited by asking one question: what happens to an item that says `dc.rights.access=embargoedAccess`? Five of them answered "archived and public", and those now throw `EmbargoMetadataException`, which `ItemImport.internalRun()` turns into `context.abort()` and exit 1: * `embargoedAccess` without any `dc.date.embargoend` * `dc.date.embargoend` present but empty * `dc.date.embargoend` that no format accepts * the `Anonymous` group not found * the two blanket `catch (Exception e) { logError(...) }` blocks and the per-bitstream one, which swallowed every failure of the policy write - the bitstream then reached `installItem` without an Anonymous READ policy, which is exactly the leak above The other three returns stay as they are and are documented: no embargo metadata at all (the collection defaults decide), an embargo that has already expired (a publication, not a failure - the branch's rule, covered by `EmbargoPastDateIT` and `testPastEmbargoDateNoPolicy`), and no ORIGINAL bundle (no file exists, so no file can be disclosed). Removing the outer `catch (Exception)` exposed what it had been hiding: `dspace import --test` creates no item, so `processEmbargoMetadata` was called with `null` and logged an NPE as "ERROR: Failed to process embargo metadata" on every package. It now returns before touching anything. Multiple `dc.date.embargoend` values keep the specified behaviour (first value wins) and now produce the same operator warning as `itemupdate`. Tests ----- `EmbargoImportIT` grows from 7 to 15 tests; the 9 changed or added ones all fail on the code of f26f648: * `testInvalidEmbargoDateFormat` asserted only "no embargo policy", which a wide open bitstream satisfies just as well - it is the empty assertion that let the leak through. It now asserts that the operator is told *and* that no file of the package is anonymously readable. * `testLenientRollOverEmbargoDateIsRefused`, `testBlankEmbargoEndIsRefused`, `testEmbargoedAccessWithoutEndDateIsRefused` - the other refusal paths. * `testYearOnlyEmbargoEndIsFirstOfJanuary`, `testYearMonthEmbargoEndIsFirstOfMonth`, `testIsoTimestampEmbargoEndIsTruncatedToUtcDay` - the `DCDate` shapes produce a policy again, with the start date `DCDate` would have produced. * `testFailureToWriteThePolicyIsNotSwallowed`, `testMissingAnonymousGroupIsNotSwallowed` inject the failure into a hand-wired service instance instead of breaking the test database, and assert that it reaches the caller. 57 integration tests, 0 failures; checkstyle 0 violations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The DCDate backwards compatibility of SafEmbargoDateParser was only covered on the import side. Reducing parseEmbargoEndDay to a bare LocalDate.parse failed 3 tests in EmbargoImportIT and none of the 42 itemupdate ones, although ItemUpdate.syncEmbargoPolicies uses the same parser and aligning the two tools was the point of the change. EmbargoDateBoundaryIT now pins the itemupdate path down as well: * legacyEmbargoEndShapesKeepTheirDcDateDay - a bare year, a bare month and an ISO timestamp close the file until exactly the day DCDate mapped them to (1 January, the 1st of the month, the UTC day of the instant). * legacyPastEmbargoEndPublishesOnPurpose - a legacy value whose day lies in the past publishes the file. That is a deliberate reading of the metadata and no longer an accident of strict parsing: "2020" says the embargo ended in 2020. Before the DCDate shapes were read again such a value threw, the item was refused and the run exited 1; that refusal was a side effect, not a decision, so the new behaviour is asserted rather than left to happen silently. * unparseableLegacyLookalikeLeavesPoliciesUntouched - trailing garbage after a year and a numeric UTC offset, both of which DCDate used to swallow, are refused: identical policy ids, unchanged start dates, one counted failure. A failing embargo synchronisation is no longer reported as a successful run. processArchive caught every per-item exception, printed it and left the exit code at 0, but syncEmbargoPolicies re-dates the surviving Anonymous READ policy of a bitstream before deleting the duplicates, so an exception in that last step left context.complete() committing a published file while the run claimed success. The call now counts an embargo failure before the exception propagates, covered by EmbargoSafetyIT.embargoSyncThatDiesHalfWayIsNotReportedAsSuccess. Known limitation, unchanged and now correctly commented in ItemImportServiceImpl.applyEmbargoToItemBitstreams: a package with no ORIGINAL bundle is a no-op for the embargo code, which is not the same as "no file is disclosed". A SAF contents file can route its files into another bundle with the bundle:<name> marker, and those bitstreams are archived with the collection default READ policy however loudly dc.rights.access claims an embargo. Both SAF tools declare a scope of ORIGINAL bitstreams only; the comment used to claim a security property that scope does not give.
There was a problem hiding this comment.
Pull request overview
Fixes embargo synchronization and SAF import paths to avoid policy loss, unintended publication, and silent failures.
Changes:
- Validates embargo metadata before policy mutation and preserves policy identity.
- Adds shared strict date parsing and normalized embargo policy constants.
- Expands integration coverage for imports, workflows, legacy dates, and access safety.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
ItemUpdate.java |
Reworks embargo synchronization and failure reporting. |
ItemImportServiceImpl.java |
Makes SAF embargo imports fail closed. |
SafEmbargoDateParser.java |
Parses supported embargo date formats strictly. |
SafEmbargoConstants.java |
Defines the canonical embargo policy name. |
EmbargoMetadataException.java |
Represents fatal import embargo errors. |
ItemUpdateIT.java |
Strengthens synchronization assertions. |
EmbargoSafetyIT.java |
Tests refusal and failure scenarios. |
EmbargoPastDateIT.java |
Reproduces the reported policy-loss regression. |
EmbargoLifecycleIT.java |
Tests policy lifecycle and legacy adoption. |
EmbargoDateBoundaryIT.java |
Tests date boundaries and legacy formats. |
EmbargoImportIT.java |
Tests import, workflow, and fail-closed behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ACCEPTED * An Anonymous READ policy that carries an END date is no longer synchronised (ItemUpdate.applyEmbargoToItemBitstreams). Such a policy is a "lease" - the access condition of access-conditions.xml with groupName=Anonymous and hasEndDate=true, written by the submission UI, by a REST access condition patch and by dspace bulk-access-control - and it means "public now, closed again on that day". dc.date.embargoend says nothing about that day, so both ways of synchronising it decide access on the operator's behalf: keeping the end date next to a fresh start date can close the file for good (measured on the mutant: start=2027-08-20 end=2027-02-19), and losing it publishes for ever a file that was to close itself. The bitstream is therefore left untouched, the offending policy is named on the console together with the bulk-access-control hint, and embargoSyncFailures is incremented so the run exits 1. Adjustment to the proposal: EVERY Anonymous READ policy of the bitstream is examined, not only the survivor. A lease has no start date, so selectSurvivorPolicy prefers a dated embargo policy next to it and the lease falls into the deletion loop - deleting it is exactly what drops the end date. The mutant proves it: policy ids went from [202, 203] to [202]. Copilot's own suggestion (normalise the end date, i.e. set it to null) was NOT implemented: clearing an end date widens access, which this branch forbids. * EmbargoLifecycleIT class javadoc corrected. It claimed the life cycle ends by "lifting it again by dropping dc.date.embargoend", the exact opposite of the contract and of what removingEmbargoMetadataLeavesPoliciesUntouched() asserts. The absence of the field is no instruction; an embargo is ended by a dc.date.embargoend in the past. Grepped the other embargo ITs and ItemUpdate itself for the same stale claim - no other occurrence. ADJUSTED * @TeMPOraL(DATE) / policy start date (ItemUpdate:808, ItemImportServiceImpl:2641) - production code deliberately unchanged, documentation and test added instead. Copilot is right that resourcepolicy.start_date is a DATE column behind a @TeMPOraL(DATE) field and that only the calendar day survives; on PostgreSQL in a JVM zone behind UTC the stored day even shifts back one day. But the value written here is byte-for-byte what core DSpace writes for the same day (DCDate.toDate() parses date-only values in UTC; the REST and submission layer goes through TimeHelpers.toMidnightUTC), so the defect belongs to the mapping and hits every DSpace embargo path identically. Changing only these two lines to local midnight would desynchronise itemupdate/itemimport from REST, from bulk-access-control and from the core embargo lifter on the same repository, and it would be measurably worse here: with atStartOfDay(ZoneId.systemDefault()) the IT suite stores the day BEFORE the intended one (expected 2026-08-20, stored 2026-08-19 - eight failures in EmbargoDateBoundaryIT), i.e. every embargo would open a day early. The customer instance runs Europe/Prague, where the stored day is correct either way. hibernate.jdbc.time_zone cannot help: it is ignored for @TeMPOraL(DATE). Both call sites now carry a comment recording why midnight UTC is used, that only the calendar day is stored, and that the negative-offset day shift is an upstream limitation - so this is not re-litigated in a sixth review round. startDateIsUtcMidnightNotServerZone was rewritten as startDateSurvivesTheDatabaseAsTheExpectedCalendarDay. It no longer stops at the in-session instant (which no reload ever sees): it commits, drops the Hibernate session, reads the policy back out of the DATE column and asserts the stored calendar day, that the policy is not date-valid while that day is ahead, and that it IS date-valid - and the file readable - once the day has arrived. The in-session midnight-UTC assertion is kept as the encoding contract. Its javadoc records the blind spot: the harness pins H2 to TIME ZONE=UTC, so no test in this class can reproduce the PostgreSQL day shift. * Atomicity of syncEmbargoPolicies (ItemUpdate:513) - behaviour unchanged, the misleading comment that invited the finding corrected. The premise does not hold. A DB error cannot commit a half-synchronised bitstream: Hibernate marks the transaction rollback-only for every RuntimeException that passes through a session call (SessionImpl.fireDelete / doFlush -> ExceptionConverterImpl.markForRollbackOnly), and HibernateDBConnection.commit() refuses to commit a MARKED_ROLLBACK transaction, so context.complete() writes the whole batch or nothing. What remains is a non-DB exception between the survivor re-dating and the duplicate deletion, and that state is never more open than the state the tool found - the only widening step is the survivor re-date, it runs first, and it writes what the item's own metadata instructs. embargoSyncFailures + exit 1 reports it, and the synchronisation is idempotent, so re-running the same SAF package is the repair. The old comment asserted unconditionally that "context.complete() commits that state"; it now says what actually happens. REJECTED * Aborting the whole run on a per-item embargo failure. There is no per-item rollback primitive in this DSpace (no savepoint support anywhere), Context is one transaction, and an abort would discard 499 correct items to protect against a state that is not a disclosure. It would also create a footgun: the undo archive and undo_*_command.sh are written to disk BEFORE the sync, and because the undo command carries -a dc.date.embargoend it re-enables embargo syncing - replaying it after an abort would push stale embargo dates onto items that were never touched. * Clearing the survivor's end date (see ACCEPTED, first item). IMPORT PATH Checked and deliberately not changed. ItemImportServiceImpl creates a brand new policy and never sets an end date; at import time the bitstream has no Anonymous READ policy to adopt, and installItem's addDefaultPoliciesNotInPlace skips the collection default because isAnIdenticalPolicyAlreadyInPlace matches on (dso, group, action) alone and already sees the created policy. So no lease can be adopted or deleted there. TESTS * EmbargoSafetyIT.leasedAnonymousReadPolicyIsUntouched - a bitstream whose only Anonymous READ policy has an end date, run with a future and with an expired dc.date.embargoend: policy ids and every policy value unchanged, embargoSyncFailures == 1 per run, console names bulk-access-control. * EmbargoSafetyIT.leaseNextToADatedEmbargoPolicyIsNotDeleted - lease next to a dated embargo policy: nothing deleted, nothing mutated, embargoSyncFailures == 1. * EmbargoDateBoundaryIT.startDateSurvivesTheDatabaseAsTheExpectedCalendarDay replaces startDateIsUtcMidnightNotServerZone (see above). Every new assertion was mutation-verified: with the end-date guard disabled both EmbargoSafetyIT tests fail, and with atStartOfDay(ZoneId.systemDefault()) the rewritten boundary test fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
milanmajchrak
changed the title
Cut the design essays and the review narration the previous commits left in the embargo code down to short notes about why the code does what it does. Javadoc is shortened in place, never removed. Phase banners, internal call chain traces and issue references are gone. Assertion messages are untouched: they are the diagnostics a failing test prints. No executable code changed. Verified with a Java lexer that strips comments and compares the token streams: all eleven files are token-identical to the previous commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
itemupdate synchronised the Anonymous READ policy of the ORIGINAL bundle only, so an embargoed item kept a public thumbnail and, worse, a public TEXT bitstream holding the full text extracted from the embargoed file. The whole content of an embargoed thesis was therefore downloadable for the entire embargo period. applyEmbargoToItemBitstreams now walks ORIGINAL, TEXT and THUMBNAIL. The embargo is a property of the item, so all three get the same start date, and the per-bitstream rules are unchanged: mutate the surviving Anonymous READ policy, never create one where none exists, delete duplicates only after the survivor is stored, leave a policy with an end date alone, count every skipped bitstream in embargoSyncFailures. The bundle list lives in SafEmbargoConstants so the two SAF tools cannot drift apart. LICENSE, CC-LICENSE and METADATA stay out of scope, the licence text has to remain readable. Reports now name the bundle a bitstream belongs to, and the hint at the end of a failure report depends on it: bulk-access-control walks the ORIGINAL bundle only, so recommending it for a TEXT or THUMBNAIL bitstream would send the operator down a dead end. MediaFilterService.updatePoliciesOfDerivativeBitstreams was deliberately not reused. It returns immediately unless setFilterClasses() was called, which only MediaFilterScript does, so it is a no-op for every other caller; and when primed it re-publishes thumbnails listed in filter.*.publicPermission, which is the opposite of what an embargo needs. EmbargoLifecycleIT.derivativeBundlesAreNotTouchedDirectly asserted the old behaviour - that TEXT and THUMBNAIL policies are byte-identical before and after a run - which is exactly the leak. It is rewritten as derivativeBundlesFollowTheEmbargo and now pins the new rule, keeping its assertions on the ORIGINAL bundle object: the fix works on bitstreams, bundle-level policies stay untouched. EmbargoDerivativesIT covers the closing and re-opening of derivatives, in-place mutation, the untouched licence bundles, a derivative without an Anonymous READ policy, and the withdrawn/restrictedAccess guards. The import path needs no change: at import time the derivatives do not exist yet, they are created later by filter-media, which derives their policies itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The six ItemUpdate embargo test classes each carried their own copy of the same setup: the collection whose bitstreams inherit an undated Anonymous READ policy, the metadata field registration, the SAF archive writer that drives a run, and the policy inspection helpers. AbstractEmbargoIT now holds that fixture and each class keeps only the scenario it pins down. No test case is removed and no assertion changes. Three helpers had drifted apart between the copies and are unified on the version that was already documented as the correct one: - replaceAnonymousReadPolicies deletes policies one by one; the bulk removePoliciesActionFilter leaves the in-memory collection stale - dublinCore has one implementation instead of five - a run tees the console, so the output still reaches the failsafe report
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
dspace-api/src/main/java/org/dspace/app/itemupdate/ItemUpdate.java:796
- This success message is false for supported refusal cases. For example, a bitstream with no Anonymous/READ policy or with a lease remains unreadable, yet the tool first says all bitstreams are public and only afterward reports the synchronization failure. Phrase this as an attempted synchronization, or emit success only after every bitstream is updated.
if (!accessStartDay.isAfter(LocalDate.now(ZoneOffset.UTC))) {
// An expired embargo is a publication, not a deletion: the start date that has already passed
// makes the policy effective immediately.
pr("Embargo of item " + itemLabel(item) + " already expired on " + embargoEndDay
+ ", its bitstreams are public since " + accessStartDay + ".");
dspace-api/src/main/java/org/dspace/app/util/SafEmbargoDateParser.java:94
- The new parser does not preserve all valid legacy values accepted by
DCDate, despite the compatibility contract above.DCDateusedSimpleDateFormat("yyyy-MM"), which accepts an unpadded value such as2027-2, butYearMonth.parserequires2027-02; the import now rejects such an existing SAF package. Add a strict unpadded year-month formatter (and corresponding compatibility test), as was already done for unpadded full dates.
try {
// DCDate.toDate() reported a bare month as its first day, so it keeps meaning that day
return YearMonth.parse(trimmed).atDay(1);
} catch (DateTimeParseException notAYearMonth) {
// ditto
…s refused The batch import path completes its context in a finally block, so a workspace item left behind by a refused package was committed as an orphan submission with its bitstreams. The command line path aborts its context and was never affected. Mirrors the cleanup the install failure path in the same method already does.
A SAF contents file can route its payload into a bundle of its own with the "bundle:<name>" marker. The import path only looked at ORIGINAL, so such a package was archived with public files although its own metadata declared it closed, and itemupdate left that bundle public for the whole embargo. Both tools now cover every bundle except LICENSE, CC-LICENSE and METADATA, the same three DefaultEmbargoSetter leaves world readable. Two smaller fixes on the same paths: - SafEmbargoDateParser accepts an unpadded year-month such as 2027-2, which SimpleDateFormat read and existing SAF packages therefore contain - itemupdate reports an expired embargo as published only after every bitstream is synchronised, so a bitstream it had to refuse is not announced as public
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
dspace-api/src/main/java/org/dspace/app/itemupdate/ItemUpdate.java:806
- This Javadoc link targets a constant that does not exist, so generated API documentation cannot resolve it. The implementation uses the inverse deny-list
NON_EMBARGOED_BUNDLE_NAMES; link that field and describe the exclusion instead.
* {@link SafEmbargoConstants#EMBARGOED_BUNDLE_NAMES}, leaving exactly one such policy per bitstream, as a
dspace-api/src/main/java/org/dspace/app/util/SafEmbargoDateParser.java:45
- This formatter is stricter in width, not only in calendar validity:
ISO_LOCAL_DATEand the fixed-width hour/minute/second fields reject values such as2027-5-9T1:2:3Z, while the previousDCDate/SimpleDateFormatparser accepted them. Since the parser explicitly preserves unpadded legacy date forms below, existing SAF packages using the same unpadded form with a time component will now fail import/update. Use variable-width numeric fields for the legacy timestamp (while retainingResolverStyle.STRICT) and add that timestamp shape to the compatibility tests.
private static final DateTimeFormatter LEGACY_TIMESTAMP = new DateTimeFormatterBuilder()
.append(DateTimeFormatter.ISO_LOCAL_DATE)
.appendLiteral('T')
.appendValue(ChronoField.HOUR_OF_DAY, 2)
.optionalStart().appendLiteral(':').appendValue(ChronoField.MINUTE_OF_HOUR, 2)
.optionalStart().appendLiteral(':').appendValue(ChronoField.SECOND_OF_MINUTE, 2)
.optionalStart().appendFraction(ChronoField.NANO_OF_SECOND, 1, 9, true)
.optionalEnd().optionalEnd().optionalEnd()
.optionalStart().appendLiteral('Z').optionalEnd()
.toFormatter().withResolverStyle(ResolverStyle.STRICT);
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
References
Reported by VSB-TUO on
dspace7-test.vsb.cz, item4dde91f3-7078-4241-9938-9b8488623bb1.Description
dspace itemupdatewith adc.date.embargoendin the past removed everyAnonymous/READpolicy from the item's ORIGINAL bitstreams, so the files answered HTTP 401 and the item showedaccessStatus = restrictedwhile its metadata saiddc.rights.access = openAccess. An embargo that has run out now publishes the files instead of hiding them.Instructions for Reviewers
ItemUpdate.syncEmbargoPolicies()deleted the existing embargo policies as its first statement and only then looked at the date. A past date printed a warning and returned without writing a replacement, leaving the bitstream with no policy at all. The run still exited 0.List of changes in this PR:
syncEmbargoPolicies()is ordered objections, validation, mutation. Nothing is deleted before its replacement is stored.+1 dayis applied before the past/future test, so an embargo ending today keeps the file closed today.Anonymous/READpolicy is re-dated in place. It is located by(group, action), so policies written by earlier versions of the tool (Standard Embargo,Special Case Embargo) are adopted and normalised torpType=TYPE_CUSTOM,rpName=embargo.Anonymous/READpolicy does not get one created, and a policy carrying anendDate(a lease) is left alone.filter.*.publicPermissionis ignored. LICENSE, CC-LICENSE and METADATA are never touched.dc.date.embargoendis not an instruction to open the files. An embargo is ended by setting a date in the past.dc.rights.accessother thanopenAccessorembargoedAccessare skipped.SafEmbargoDateParser, which keeps the shapesDCDateaccepted (yyyy,yyyy-MM, ISO timestamps). A value it cannot read fails the item's import instead of archiving it publicly.embargoSyncFailures, so the tool exits 1 instead of reporting success.embargo. The previous 48-character name did not fitresourcepolicy.rpname varchar(30)and aborted the import on PostgreSQL.DSpace already has an embargo subsystem (
org.dspace.embargo.EmbargoServicewith a pluggableEmbargoSetter, driven byembargo.field.terms), and it is not used here on purpose. It is invoked fromInstallItemServiceImplwhen an item is installed, which covers a fresh import but not the case this tool exists for: re-synchronising the policies of an item that is already archived, when its embargo metadata changes. The bundle scope does follow that subsystem —DefaultEmbargoSetterembargoes every bundle except LICENSE, CC-LICENSE and METADATA, and does not consultfilter.*.publicPermissioneither.To reproduce the original bug, put an item with an ORIGINAL bitstream in a collection that grants
DEFAULT_BITSTREAM_READto Anonymous, runitemupdatewith a futuredc.date.embargoend, then run it again with one in the past. Before this PR the bitstream ends up with zero policies andGET /server/api/core/bitstreams/<uuid>/contentreturns 401; after it, the policy is re-dated into the past and the file is readable.EmbargoPastDateITcovers exactly this sequence.69 integration tests across
EmbargoPastDateIT,EmbargoDateBoundaryIT,EmbargoSafetyIT,EmbargoLifecycleIT,EmbargoDerivativesIT,ItemUpdateITandEmbargoImportIT.One thing this PR does not do: items already stripped of their policies are not repaired, because the code never creates a policy that was not there. Use
dspace bulk-access-controlwithmode: addfor those, and note that it restores the ORIGINAL bundle only — it walksitem.getBundles(CONTENT_BUNDLE_NAME)and itsupdatePoliciesOfDerivativeBitstreamscall is a no-op untilsetFilterClasses()has run, which onlyMediaFilterScriptdoes. Derivatives are restored withdspace filter-media -f.Checklist
pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.