From 255549bd35981912edb56623e7750d0b55f4953f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Ko=C5=A1arko?= Date: Wed, 1 Jul 2026 16:49:33 +0200 Subject: [PATCH 1/3] UFAL/fix: DOI Organizer creates duplicate dc.identifier.doi metadata (ufal/clarin-dspace#1368) (#1350) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (cherry picked from commit e9392ae191 on dtq-dev) v9 adaptations: - DOIIdentifierProviderTest: the two new tests' 'new Date().getTime()' rewritten to 'Instant.now().toEpochMilli()' — v9-base's Date->Instant migration removed the java.util.Date import, so the clean cherry-pick would not compile (known tripwire, sync plan card e9392ae191). Fulfils CLARIN_V9_POST_SNAPSHOT_SYNC_ACCEPTANCE.md §5 / e9392ae191 (BE-1, Vlna 1). --- .../ctask/general/ItemMetadataQAChecker.java | 1 + .../identifier/DOIIdentifierProvider.java | 25 ++++-- .../curate/ItemMetadataQACheckerIT.java | 22 ++++++ .../identifier/DOIIdentifierProviderTest.java | 77 +++++++++++++++++++ 4 files changed, 119 insertions(+), 6 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/ctask/general/ItemMetadataQAChecker.java b/dspace-api/src/main/java/org/dspace/ctask/general/ItemMetadataQAChecker.java index bcd00454aeb8..06fbbe3d928b 100644 --- a/dspace-api/src/main/java/org/dspace/ctask/general/ItemMetadataQAChecker.java +++ b/dspace-api/src/main/java/org/dspace/ctask/general/ItemMetadataQAChecker.java @@ -87,6 +87,7 @@ public void init(Curator curator, String taskId) throws IOException { "dc.rights.label", "dc.date.available", "dc.source.uri", + "dc.identifier.doi", "metashare.ResourceInfo#DistributionInfo#LicenseInfo.license" }); strangeMetadata = configurationService.getArrayProperty("lr.curation.metadata.strange", new String[]{ diff --git a/dspace-api/src/main/java/org/dspace/identifier/DOIIdentifierProvider.java b/dspace-api/src/main/java/org/dspace/identifier/DOIIdentifierProvider.java index c6e6f8ba79b3..2ffc683bf9c4 100644 --- a/dspace-api/src/main/java/org/dspace/identifier/DOIIdentifierProvider.java +++ b/dspace-api/src/main/java/org/dspace/identifier/DOIIdentifierProvider.java @@ -1069,13 +1069,26 @@ protected void saveDOIToObject(Context context, DSpaceObject dso, String doi) } Item item = (Item) dso; - itemService.addMetadata(context, item, MD_SCHEMA, DOI_ELEMENT, DOI_QUALIFIER, null, - doiService.DOIToExternalForm(doi)); - try { - itemService.update(context, item); - } catch (SQLException | AuthorizeException ex) { - throw ex; + String doiURL = doiService.DOIToExternalForm(doi); + + // Add the DOI to the metadata only if this exact value is not present yet. This keeps the operation + // idempotent (re-registration does not create duplicate values) without ever deleting metadata: a + // pre-existing, different DOI is left untouched. This method is called after the DOI has already been + // registered with the external agency, so destroying metadata here would be lossy and irreversible. + // Items that end up with more than one dc.identifier.doi value are surfaced by the ItemMetadataQAChecker + // curation task for manual review. + List existing = itemService.getMetadata(item, MD_SCHEMA, DOI_ELEMENT, DOI_QUALIFIER, Item.ANY); + boolean alreadyPresent = existing.stream() + .anyMatch(metadataValue -> doiURL.equals(metadataValue.getValue())); + + if (alreadyPresent) { + log.debug("The DOI {} is already part of the metadata of Item {}. Not adding it again.", + doi, item.getID()); + return; } + + itemService.addMetadata(context, item, MD_SCHEMA, DOI_ELEMENT, DOI_QUALIFIER, null, doiURL); + itemService.update(context, item); } /** diff --git a/dspace-api/src/test/java/org/dspace/curate/ItemMetadataQACheckerIT.java b/dspace-api/src/test/java/org/dspace/curate/ItemMetadataQACheckerIT.java index d6680648de56..9d5c9acdf4ed 100644 --- a/dspace-api/src/test/java/org/dspace/curate/ItemMetadataQACheckerIT.java +++ b/dspace-api/src/test/java/org/dspace/curate/ItemMetadataQACheckerIT.java @@ -63,6 +63,7 @@ public class ItemMetadataQACheckerIT extends AbstractIntegrationTestWithDatabase Item itemWithIncorrectLanguageName; Item itemWithTwoAvailableDates; Item itemWithTwoAvailableDatesAndLang; + Item itemWithTwoDois; Item itemVersion1; Item itemVersion2; Item itemVersion3; @@ -146,6 +147,13 @@ public void setUp() throws Exception { itemService.addMetadata(context, itemWithTwoAvailableDatesAndLang,"dc", "date", "available", "en_US", "2021-01-01"); + itemWithTwoDois = ItemBuilder.createItem(context, collection) + .withTitle("Item With Two DOIs") + .withMetadata("dc", "type", null, "corpus") + .withMetadata("dc", "identifier", "doi", "https://doi.org/10.5072/test-1") + .withMetadata("dc", "identifier", "doi", "https://doi.org/10.5072/test-2") + .build(); + itemVersion1 = ItemBuilder.createItem(context, collection) .withTitle("Item Version 1") .withMetadata("dc", "type", null, "corpus") @@ -233,6 +241,20 @@ public void testItemWithTwoAvailableDatesAndLang() throws IOException { assertTrue("Result should mention multiple dc.date.available", result.contains("dc.date.available")); } + @Test + public void testItemWithTwoDois() throws IOException { + Curator curator = new Curator(); + curator.addTask(TASK_NAME); + context.setCurrentUser(admin); + + // Run curator task for item with two dc.identifier.doi - should fail + curator.curate(context, itemWithTwoDois.getHandle()); + int status = curator.getStatus(TASK_NAME); + assertEquals("Curation should fail for item with two dc.identifier.doi", Curator.CURATE_FAIL, status); + String result = curator.getResult(TASK_NAME); + assertTrue("Result should mention multiple dc.identifier.doi", result.contains("dc.identifier.doi")); + } + @Test public void testValidItem() throws IOException { Curator curator = new Curator(); diff --git a/dspace-api/src/test/java/org/dspace/identifier/DOIIdentifierProviderTest.java b/dspace-api/src/test/java/org/dspace/identifier/DOIIdentifierProviderTest.java index 6d2fd796c471..a5e6a08194cb 100644 --- a/dspace-api/src/test/java/org/dspace/identifier/DOIIdentifierProviderTest.java +++ b/dspace-api/src/test/java/org/dspace/identifier/DOIIdentifierProviderTest.java @@ -23,6 +23,7 @@ import java.util.ArrayList; import java.util.List; import java.util.Random; +import java.util.stream.Collectors; import org.apache.commons.collections4.CollectionUtils; import org.apache.commons.lang3.ObjectUtils; @@ -332,6 +333,52 @@ public void testStore_DOI_as_item_metadata() assertTrue("Cannot store DOI as item metadata value.", result); } + @Test + public void testStore_DOI_keeps_existing_different_doi_metadata() throws SQLException, AuthorizeException, + IOException, IdentifierException, IllegalAccessException, WorkflowException { + Item item = newItem(); + + // this checks that the method does not fail if there is already a *different* DOI in the metadata, + // here we verify that the existing DOI is preserved (not deleted) and the new one is added alongside it. + // Items with more than one DOI are reported by the ItemMetadataQAChecker curation task, not silently + // cleaned up here. + String oldDoi = DOI.SCHEME + PREFIX + "/" + NAMESPACE_SEPARATOR + "1234"; + String newDoi = DOI.SCHEME + PREFIX + "/" + NAMESPACE_SEPARATOR + + Long.toHexString(Instant.now().toEpochMilli()); + + context.turnOffAuthorisationSystem(); + itemService.addMetadata(context, item, DOIIdentifierProvider.MD_SCHEMA, + DOIIdentifierProvider.DOI_ELEMENT, + DOIIdentifierProvider.DOI_QUALIFIER, + null, + doiService.DOIToExternalForm(oldDoi)); + provider.saveDOIToObject(context, item, newDoi); + context.restoreAuthSystemState(); + + checkDoiMetadata(item, oldDoi, newDoi); + } + + @Test + public void testStore_DOI_check_single_doi_metadata() throws SQLException, AuthorizeException, IOException, + IdentifierException, IllegalAccessException, WorkflowException { + Item item = newItem(); + + // this checks that the method does not fail if there is already a DOI in the metadata, + // here we check if DOI metadata are not duplicated + String doi = DOI.SCHEME + PREFIX + "/" + NAMESPACE_SEPARATOR + Long.toHexString(Instant.now().toEpochMilli()); + + context.turnOffAuthorisationSystem(); + itemService.addMetadata(context, item, DOIIdentifierProvider.MD_SCHEMA, + DOIIdentifierProvider.DOI_ELEMENT, + DOIIdentifierProvider.DOI_QUALIFIER, + null, + doiService.DOIToExternalForm(doi)); + provider.saveDOIToObject(context, item, doi); + context.restoreAuthSystemState(); + + checkSingleDoiMetadata(item, doi); + } + @Test public void testGet_DOI_out_of_item_metadata() throws SQLException, AuthorizeException, IOException, IdentifierException, IllegalAccessException, @@ -868,4 +915,34 @@ public void testLoadOrCreateDOIReturnsMintedStatus() // registerOnline // reserveOnline + private void checkSingleDoiMetadata(Item item, String doi) throws IdentifierException { + List metadata = itemService.getMetadata(item, DOIIdentifierProvider.MD_SCHEMA, + DOIIdentifierProvider.DOI_ELEMENT, + DOIIdentifierProvider.DOI_QUALIFIER, + Item.ANY); + boolean result = false; + if (metadata.size() == 1 && metadata.get(0).getValue().equals(doiService.DOIToExternalForm(doi))) { + result = true; + } + assertTrue("Invalid or duplicate 'dc.identifier.doi' metadata value(s).", result); + } + + private void checkDoiMetadata(Item item, String... dois) throws IdentifierException { + List values = itemService.getMetadata(item, DOIIdentifierProvider.MD_SCHEMA, + DOIIdentifierProvider.DOI_ELEMENT, + DOIIdentifierProvider.DOI_QUALIFIER, + Item.ANY) + .stream() + .map(MetadataValue::getValue) + .collect(Collectors.toList()); + + List expected = new ArrayList<>(); + for (String doi : dois) { + expected.add(doiService.DOIToExternalForm(doi)); + } + + assertEquals("Unexpected number of 'dc.identifier.doi' metadata values.", expected.size(), values.size()); + assertTrue("Expected 'dc.identifier.doi' metadata values are missing.", values.containsAll(expected)); + } + } From 7415b4cbf2f56eaac34876fb27bb9ba79888c0b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Ko=C5=A1arko?= Date: Wed, 1 Jul 2026 16:43:28 +0200 Subject: [PATCH 2/3] UFAL/Obtain special groups from user context when new token is generated (on token refresh) (ufal/clarin-dspace#1378) (#1347) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (cherry picked from commit 74f5862748 on dtq-dev) v9 adaptations / additions: - ClarinShibbolethSpecialGroupsIT.java added verbatim from the zcu backport branch head b7d3c786ac (origin/zcu-pub/backport-1347-shib-special-groups) — the regression IT exists only on the 7.6 customer backport branches, not on dtq-dev; fix sourced from 74f5862748 (canonical), test from b7d3c786ac per the sync plan card. - No code adaptations: the pick applied conflict-free (method bodies only, jakarta imports untouched); Context.getSpecialGroups null-guard included. Fulfils CLARIN_V9_POST_SNAPSHOT_SYNC_ACCEPTANCE.md §5 / 74f5862748 (BE-1, Vlna 1). --- .../clarin/ClarinShibAuthentication.java | 41 ++-- .../main/java/org/dspace/core/Context.java | 7 +- .../ClarinShibbolethSpecialGroupsIT.java | 181 ++++++++++++++++++ 3 files changed, 204 insertions(+), 25 deletions(-) create mode 100644 dspace-server-webapp/src/test/java/org/dspace/app/rest/security/ClarinShibbolethSpecialGroupsIT.java diff --git a/dspace-api/src/main/java/org/dspace/authenticate/clarin/ClarinShibAuthentication.java b/dspace-api/src/main/java/org/dspace/authenticate/clarin/ClarinShibAuthentication.java index aeaf4c6e3a52..67bfa767ce3a 100644 --- a/dspace-api/src/main/java/org/dspace/authenticate/clarin/ClarinShibAuthentication.java +++ b/dspace-api/src/main/java/org/dspace/authenticate/clarin/ClarinShibAuthentication.java @@ -277,7 +277,7 @@ public int authenticate(Context context, String username, String password, // Step 4: Log the user in. context.setCurrentUser(eperson); - request.getSession().setAttribute("shib.authenticated", true); + request.setAttribute("shib.authenticated", true); AuthenticateServiceFactory.getInstance().getAuthenticationService().initEPerson(context, request, eperson); log.info(eperson.getEmail() + " has been authenticated via shibboleth."); @@ -330,42 +330,35 @@ public int authenticate(Context context, String username, String password, @Override public List getSpecialGroups(Context context, HttpServletRequest request) { try { - // User has not successfuly authenticated via shibboleth. - if (request == null || - context.getCurrentUser() == null || - request.getSession().getAttribute("shib.authenticated") == null) { - return Collections.EMPTY_LIST; + // User has not successfully authenticated via shibboleth. + if (request == null || context.getCurrentUser() == null) { + return Collections.emptyList(); } - // If we have already calculated the special groups then return them. - if (request.getSession().getAttribute("shib.specialgroup") != null) { - log.debug("Returning cached special groups."); - List sessionGroupIds = (List) request.getSession().getAttribute("shib.specialgroup"); - List result = new ArrayList<>(); - for (UUID uuid : sessionGroupIds) { - result.add(groupService.find(context, uuid)); - } - return result; + List specialGroups = context.getSpecialGroups(); + if (!specialGroups.isEmpty()) { + log.debug("Returning special groups from context."); + return specialGroups; } + if (request.getAttribute("shib.authenticated") == null) { + log.debug("User has not been authenticated via shibboleth, returning empty list of special groups."); + return Collections.emptyList(); + } List groupIds = new ShibGroup(new ShibHeaders(request), context).get(); - // Cache the special groups, so we don't have to recalculate them again - // for this session. - request.getSession().setAttribute("shib.specialgroup", groupIds); List groups = new ArrayList<>(); for (UUID uuid : groupIds) { Group foundGroup = groupService.find(context, uuid); - if (Objects.isNull(foundGroup)) { - continue; + if (foundGroup != null) { + groups.add(foundGroup); } - groups.add(foundGroup); } return groups; } catch (Throwable t) { - log.error("Unable to validate any sepcial groups this user may belong too because of an exception.", t); - return Collections.EMPTY_LIST; + log.error("Unable to validate any special groups this user may belong to because of an exception.", t); + return Collections.emptyList(); } } @@ -1291,7 +1284,7 @@ private String getShibURL(HttpServletRequest request) { public boolean isUsed(final Context context, final HttpServletRequest request) { if (request != null && context.getCurrentUser() != null && - request.getSession().getAttribute("shib.authenticated") != null) { + request.getAttribute("shib.authenticated") != null) { return true; } return false; diff --git a/dspace-api/src/main/java/org/dspace/core/Context.java b/dspace-api/src/main/java/org/dspace/core/Context.java index 1b0ebd43d5fe..89b79b35b266 100644 --- a/dspace-api/src/main/java/org/dspace/core/Context.java +++ b/dspace-api/src/main/java/org/dspace/core/Context.java @@ -686,7 +686,12 @@ public boolean inSpecialGroup(UUID groupID) { public List getSpecialGroups() throws SQLException { List myGroups = new ArrayList<>(); for (UUID groupId : specialGroups) { - myGroups.add(EPersonServiceFactory.getInstance().getGroupService().find(this, groupId)); + Group group = EPersonServiceFactory.getInstance().getGroupService().find(this, groupId); + // A special group UUID may reference a group that has since been deleted; skip nulls + // so callers never receive a list containing null (avoids NPE downstream). + if (group != null) { + myGroups.add(group); + } } return myGroups; diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/security/ClarinShibbolethSpecialGroupsIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/security/ClarinShibbolethSpecialGroupsIT.java new file mode 100644 index 000000000000..079eef92905e --- /dev/null +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/security/ClarinShibbolethSpecialGroupsIT.java @@ -0,0 +1,181 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +package org.dspace.app.rest.security; + +import static org.junit.Assert.assertNotNull; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import java.io.InputStream; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.commons.codec.CharEncoding; +import org.apache.commons.io.IOUtils; +import org.dspace.app.rest.test.AbstractControllerIntegrationTest; +import org.dspace.app.util.Util; +import org.dspace.builder.BitstreamBuilder; +import org.dspace.builder.CollectionBuilder; +import org.dspace.builder.CommunityBuilder; +import org.dspace.builder.EPersonBuilder; +import org.dspace.builder.GroupBuilder; +import org.dspace.builder.ItemBuilder; +import org.dspace.content.Bitstream; +import org.dspace.content.Collection; +import org.dspace.content.Community; +import org.dspace.content.Item; +import org.dspace.core.I18nUtil; +import org.dspace.eperson.EPerson; +import org.dspace.eperson.Group; +import org.dspace.services.ConfigurationService; +import org.junit.Before; +import org.junit.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.test.web.servlet.MvcResult; + +/** + * Integration test verifying that the Shibboleth special groups (e.g. the default `Authenticated` group) + * survive into tokens which are minted on stateless REST requests after the login: + * the short-lived token used for bitstream downloads and the refreshed login token. + * + * Replicates https://github.com/dataquest-dev/DSpace/issues/900 - a bitstream restricted to the + * `Authenticated` group is visible after the Shibboleth login, but its download returns 403, + * because the special groups are lost when the short-lived token is generated + * (see ufal/clarin-dspace#1373). + * + * @author Milan Majchrak (milan.majchrak at dataquest.sk) + */ +public class ClarinShibbolethSpecialGroupsIT extends AbstractControllerIntegrationTest { + + public static final String[] SHIB_ONLY = {"org.dspace.authenticate.clarin.ClarinShibAuthentication"}; + private static final String NET_ID_TEST_EPERSON = "123456789"; + private static final String IDP_TEST_EPERSON = "Test Idp"; + + private EPerson clarinEperson; + private Bitstream restrictedBitstream; + + @Autowired + ConfigurationService configurationService; + + @Before + public void setup() throws Exception { + super.setUp(); + + // Enable Shibboleth login for all tests + configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY); + + context.turnOffAuthorisationSystem(); + + // Create an eperson with netID - that means the user already exists in the database + clarinEperson = EPersonBuilder.createEPerson(context) + .withCanLogin(false) + .withEmail("clarin@email.com") + .withNameInMetadata("first", "last") + .withLanguage(I18nUtil.getDefaultLocale().getLanguage()) + .withNetId(Util.formatNetId(NET_ID_TEST_EPERSON, IDP_TEST_EPERSON)) + .build(); + + // The group every shibboleth-authenticated user is implicitly added to (as a special group) + String defaultGroupName = configurationService.getProperty("authentication-shibboleth.default.auth.group"); + Group authenticatedGroup = GroupBuilder.createGroup(context) + .withName(defaultGroupName) + .build(); + + // A bitstream readable only by the shibboleth default special group + Community community = CommunityBuilder.createCommunity(context) + .withName("Community") + .build(); + Collection collection = CollectionBuilder.createCollection(context, community) + .withName("Collection") + .build(); + Item item = ItemBuilder.createItem(context, collection) + .withTitle("Item with a restricted bitstream") + .build(); + try (InputStream is = IOUtils.toInputStream("Restricted content", CharEncoding.UTF_8)) { + restrictedBitstream = BitstreamBuilder.createBitstream(context, item, is) + .withName("restricted.txt") + .withMimeType("text/plain") + .withReaderGroup(authenticatedGroup) + .build(); + } + + context.restoreAuthSystemState(); + } + + /** + * Replication of the issue #900: + * 1. Sign in via Shibboleth - the user is implicitly added into the `Authenticated` special group. + * 2. The bitstream restricted to the `Authenticated` group is readable with the login token. + * 3. The UI downloads the bitstream with a short-lived token minted on a separate stateless request + * - the download must succeed too. + */ + @Test + public void shouldDownloadRestrictedBitstreamWithShortLivedTokenAfterShibLogin() throws Exception { + String loginToken = shibLogin(); + + // Sanity check: the login token keeps the special groups (its `sg` claim was computed + // during the shibboleth login request), so the restricted bitstream is readable. + getClient(loginToken).perform(get("/api/core/bitstreams/" + restrictedBitstream.getID() + "/content")) + .andExpect(status().isOk()); + + // The short-lived token is minted on a stateless request - the special groups must be + // obtained from the user context (restored from the login token), not from the session. + String shortLivedToken = getShortLivedToken(loginToken); + getClient().perform(get("/api/core/bitstreams/" + restrictedBitstream.getID() + + "/content?authentication-token=" + shortLivedToken)) + .andExpect(status().isOk()); + } + + /** + * The refreshed login token (POST /api/authn/login with the Bearer token, no shibboleth headers) + * must keep the special groups too, otherwise the user loses the access after the first token refresh + * (see ufal/clarin-dspace#1373). + */ + @Test + public void shouldKeepSpecialGroupsAfterLoginTokenRefresh() throws Exception { + String loginToken = shibLogin(); + + // Sanity check: the restricted bitstream is readable with the login token + getClient(loginToken).perform(get("/api/core/bitstreams/" + restrictedBitstream.getID() + "/content")) + .andExpect(status().isOk()); + + // Refresh the login token on a stateless request (no shibboleth session/headers) + String refreshedAuthHeader = getClient(loginToken).perform(post("/api/authn/login")) + .andExpect(status().isOk()) + .andReturn().getResponse().getHeader(AUTHORIZATION_HEADER); + assertNotNull("The token refresh must return the Authorization header", refreshedAuthHeader); + String refreshedToken = refreshedAuthHeader.replace(AUTHORIZATION_TYPE, ""); + + // The restricted bitstream must still be readable with the refreshed token + getClient(refreshedToken).perform(get("/api/core/bitstreams/" + restrictedBitstream.getID() + "/content")) + .andExpect(status().isOk()); + } + + private String shibLogin() throws Exception { + String authHeader = getClient().perform(get("/api/authn/shibboleth") + .header("SHIB-MAIL", clarinEperson.getEmail()) + .header("Shib-Identity-Provider", IDP_TEST_EPERSON) + .header("SHIB-NETID", NET_ID_TEST_EPERSON)) + .andExpect(status().is3xxRedirection()) + .andReturn().getResponse().getHeader(AUTHORIZATION_HEADER); + assertNotNull("The shibboleth login must return the Authorization header", authHeader); + return authHeader.replace(AUTHORIZATION_TYPE, ""); + } + + private String getShortLivedToken(String loginToken) throws Exception { + ObjectMapper mapper = new ObjectMapper(); + MvcResult mvcResult = getClient(loginToken).perform(post("/api/authn/shortlivedtokens")) + .andExpect(status().isOk()) + .andReturn(); + String content = mvcResult.getResponse().getContentAsString(); + JsonNode token = mapper.readTree(content).get("token"); + assertNotNull("The shortlivedtokens response must contain the token field", token); + return token.asText(); + } +} From 59387ebd6beea4da85bf8ff4fde224ab9ec97eb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Ko=C5=A1arko?= Date: Tue, 23 Jun 2026 11:09:29 +0200 Subject: [PATCH 3/3] [Port to dtq-dev] Issue 1364: tgz file preview fix (#1338) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (cherry picked from commit 15b296aa2a on dtq-dev) v9 adaptations / conflict resolution: - FilePreviewIT.java: single conflict resolved toward the v9-base deletion of testPreviewWithSyncStorage + SyncBitstreamStorageServiceImpl import/SYNC_STORE_NUMBER (that class does not exist on v9-base); the commit's modernization hunk for that test dropped, everything else applied (testUnauthorizedPassword removed, -p args dropped, checkHandlerMessages helper + testPreviewWithForce added). Resulting file contains exactly 6 tests. - Intentional semantics change carried from the fork commit: the file-preview CLI no longer requires -p/--password; EPerson resolved from context or -e email, consistent with other DSpace CLI scripts. Admin-only operations stay guarded server-side. Fulfils CLARIN_V9_POST_SNAPSHOT_SYNC_ACCEPTANCE.md §5 / 15b296aa2a (BE-1, Vlna 1). --- .../content/PreviewContentServiceImpl.java | 2 +- .../scripts/filepreview/FilePreview.java | 64 ++++++++-------- .../filepreview/FilePreviewConfiguration.java | 10 +-- .../scripts/filepreview/FilePreviewIT.java | 74 +++++++++++++------ .../app/rest/PreviewContentServiceImplIT.java | 26 ++++++- 5 files changed, 112 insertions(+), 64 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/content/PreviewContentServiceImpl.java b/dspace-api/src/main/java/org/dspace/content/PreviewContentServiceImpl.java index a29e05e8aefe..0abdf603b631 100644 --- a/dspace-api/src/main/java/org/dspace/content/PreviewContentServiceImpl.java +++ b/dspace-api/src/main/java/org/dspace/content/PreviewContentServiceImpl.java @@ -369,7 +369,7 @@ private void processGzipFile(List filePaths, File file, Bitstream bitstr if (fileName == null) { logBitstreamNameIsNull(); } else { - if (fileName.toLowerCase().endsWith("tar.gz")) { + if (fileName.toLowerCase().endsWith(".tar.gz") || fileName.toLowerCase().endsWith(".tgz")) { processTarGzipFile(filePaths, file, bitstream); } else { try (InputStream is = new GzipCompressorInputStream(new FileInputStream(file))) { diff --git a/dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java b/dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java index ba0b0ef0bf12..38e10361e9f9 100644 --- a/dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java +++ b/dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreview.java @@ -16,12 +16,10 @@ import org.apache.commons.cli.ParseException; import org.apache.commons.lang3.StringUtils; -import org.dspace.authenticate.AuthenticationMethod; -import org.dspace.authenticate.factory.AuthenticateServiceFactory; -import org.dspace.authenticate.service.AuthenticationService; import org.dspace.content.Bitstream; import org.dspace.content.Bundle; import org.dspace.content.Item; +import org.dspace.content.PreviewContent; import org.dspace.content.factory.ContentServiceFactory; import org.dspace.content.service.ItemService; import org.dspace.content.service.PreviewContentService; @@ -46,13 +44,12 @@ public class FilePreview extends DSpaceRunnable { ContentServiceFactory.getInstance().getPreviewContentService(); private EPersonService ePersonService = EPersonServiceFactory.getInstance() .getEPersonService(); - private AuthenticationService authenticateService = AuthenticateServiceFactory.getInstance() - .getAuthenticationService(); /** * `-i`: Info, show help information. */ private boolean info = false; + private boolean force = false; /** * `-u`: UUID of the Item for which to create a preview of its bitstreams. @@ -60,7 +57,6 @@ public class FilePreview extends DSpaceRunnable { private String specificItemUUID = null; private String epersonMail = null; - private String epersonPassword = null; @Override public FilePreviewConfiguration getScriptConfiguration() { @@ -84,11 +80,14 @@ public void setup() throws ParseException { specificItemUUID); } + if (commandLine.hasOption('f')) { + force = true; + } + epersonMail = commandLine.getOptionValue('e'); - epersonPassword = commandLine.getOptionValue('p'); - if (getEpersonIdentifier() == null && (epersonMail == null || epersonPassword == null)) { - throw new ParseException("Provide both -e/--email and -p/--password when no eperson is supplied."); + if (getEpersonIdentifier() == null && epersonMail == null) { + throw new ParseException("Provide -e/--email when no eperson is supplied."); } } @@ -101,7 +100,7 @@ public void internalRun() throws Exception { Context context = new Context(); try { - context.setCurrentUser(getAuthenticatedEperson((context))); + context.setCurrentUser(getEperson(context)); handler.logInfo("Authentication by user: " + context.getCurrentUser().getEmail()); if (StringUtils.isNotBlank(specificItemUUID)) { // Generate the preview only for a specific item @@ -152,7 +151,17 @@ private void generateItemFilePreviews(Context context, UUID itemUUID) throws Exc } // Generate new content if we didn't find any if (previewContentService.hasPreview(context, bitstream)) { - continue; + if (force) { + List previewContents = previewContentService + .findByBitstream(context, bitstream.getID()); + for (PreviewContent content : previewContents) { + handler.logInfo("Deleting existing preview content: '" + content.getName() + + "', for bitstream: '" + bitstream.getName() + "'"); + previewContentService.delete(context, content); + } + } else { + continue; + } } List fileInfos = previewContentService.getFilePreviewContent(context, bitstream); @@ -162,6 +171,7 @@ private void generateItemFilePreviews(Context context, UUID itemUUID) throws Exc continue; } + handler.logInfo("Generating file preview for bitstream: " + bitstream.getName()); for (FileInfo fi : fileInfos) { previewContentService.createPreviewContent(context, bitstream, fi); } @@ -176,38 +186,30 @@ public void printHelp() { "You can choose from these available options:\n" + " -i, --info Show help information\n" + " -u, --uuid The UUID of the ITEM for which to create a preview of its bitstreams\n" + - " -e, --email Email for authentication\n" + - " -p, --password Password for authentication\n"); + " -f, --force Force to create preview, even when the preview exists\n" + + " -e, --email Email of the eperson to run the script as\n"); } /** - * Retrieves an EPerson object either by its identifier or by performing an email-based lookup. - * It then authenticates the EPerson using the provided email and password. - * If the authentication is successful, it returns the EPerson object; otherwise, - * it throws an AuthenticationException. + * Resolves the EPerson the script runs as: the eperson supplied by the launching context + * (e.g. the logged-in user when started from the admin UI) if present, otherwise the eperson + * looked up by the {@code -e}/--email option. Like other CLI scripts, command-line invocation + * is trusted (shell access implies full server access), so no password is verified here; + * admin-only operations remain guarded by authorization checks in the service layer. * * @param context The Context object used for interacting with the DSpace database and service layer. - * @return The authenticated EPerson object corresponding to the provided email, - * if authentication is successful. - * @throws SQLException If a database error occurs while retrieving or interacting with the EPerson data. - * @throws AuthenticationException If no EPerson is found for the provided email - * or if the authentication fails. + * @return The EPerson the script should run as. + * @throws SQLException If a database error occurs while retrieving the EPerson data. + * @throws AuthenticationException If no EPerson is found for the provided email. */ - private EPerson getAuthenticatedEperson(Context context) throws SQLException, AuthenticationException { + private EPerson getEperson(Context context) throws SQLException, AuthenticationException { if (getEpersonIdentifier() != null) { return ePersonService.find(context, getEpersonIdentifier()); } - String msg; EPerson ePerson = ePersonService.findByEmail(context, epersonMail); if (ePerson == null) { - msg = "No EPerson found for this email: " + epersonMail; - handler.logError(msg); - throw new AuthenticationException(msg); - } - int authenticated = authenticateService.authenticate(context, epersonMail, epersonPassword, null, null); - if (AuthenticationMethod.SUCCESS != authenticated) { - msg = "Authentication failed for email: " + epersonMail; + String msg = "No EPerson found for this email: " + epersonMail; handler.logError(msg); throw new AuthenticationException(msg); } diff --git a/dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreviewConfiguration.java b/dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreviewConfiguration.java index 610691434060..a46b4fbe4eff 100644 --- a/dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreviewConfiguration.java +++ b/dspace-api/src/main/java/org/dspace/scripts/filepreview/FilePreviewConfiguration.java @@ -39,15 +39,11 @@ public Options getOptions() { options.getOption("u").setType(String.class); options.getOption("u").setRequired(false); + options.addOption("f", "force", false, "Force to create preview, even when the preview exists."); + options.addOption("e", "email", true, - "Email for authentication."); + "Email of the eperson to run the script as."); options.getOption("e").setType(String.class); - options.getOption("e").setRequired(true); - - options.addOption("p", "password", true, - "Password for authentication."); - options.getOption("p").setType(String.class); - options.getOption("p").setRequired(true); super.options = options; } diff --git a/dspace-api/src/test/java/org/dspace/scripts/filepreview/FilePreviewIT.java b/dspace-api/src/test/java/org/dspace/scripts/filepreview/FilePreviewIT.java index 27330cdd8f60..5e6e89460e32 100644 --- a/dspace-api/src/test/java/org/dspace/scripts/filepreview/FilePreviewIT.java +++ b/dspace-api/src/test/java/org/dspace/scripts/filepreview/FilePreviewIT.java @@ -102,21 +102,11 @@ public void testUnauthorizedEmail() throws Exception { assertEquals(1, run); // Since a ParseException was caught, expect return code 1 } - @Test - public void testUnauthorizedPassword() throws Exception { - // Run the script - TestDSpaceRunnableHandler testDSpaceRunnableHandler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "file-preview", "-e", ePerson.getEmail()}; - int run = ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), - testDSpaceRunnableHandler, kernelImpl); - assertEquals(1, run); // Since a ParseException was caught, expect return code 1 - } - @Test public void testWhenNoFilesRun() throws Exception { TestDSpaceRunnableHandler testDSpaceRunnableHandler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "file-preview", "-e", ePerson.getEmail(), "-p", PASSWORD }; + String[] args = new String[] { "file-preview", "-e", ePerson.getEmail() }; int run = ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), testDSpaceRunnableHandler, kernelImpl); assertEquals(0, run); @@ -127,7 +117,8 @@ public void testWhenNoFilesRun() throws Exception { public void testForSpecificItem() throws Exception { Item item2 = createOtherWorkspaceItemWithBitstream(ePerson, 0); // Run the script - runScriptForItemWithBitstreams(item2, ePerson, PASSWORD); + TestDSpaceRunnableHandler testHandler = runScriptForItemWithBitstreams(item2, ePerson); + checkHandlerMessages(testHandler, ePerson, item2, "logos.tgz", true); Bitstream b = bitstreamService.findAll(context).stream() .filter(bitstream -> bitstream.getName().equals("logos.tgz")) @@ -145,7 +136,8 @@ public void testForSpecificItem() throws Exception { public void testWhenScriptCannotCreateFilePreview() throws Exception { Item item2 = createOtherWorkspaceItemWithBitstream(eperson, 0); // Run the script as another user, without admin rights - runScriptForItemWithBitstreams(item2, ePerson, PASSWORD); + TestDSpaceRunnableHandler testHandler = runScriptForItemWithBitstreams(item2, ePerson); + checkHandlerMessages(testHandler, ePerson, item2, null, false); Bitstream b = bitstreamService.findAll(context).stream() .filter(bitstream -> bitstream.getName().equals("logos.tgz")) @@ -159,7 +151,8 @@ public void testWhenScriptCannotCreateFilePreview() throws Exception { assertFalse("Expects preview content not created.", previewContentService.hasPreview(context, b)); // Run the script as admin user - runScriptForItemWithBitstreams(item2, admin, password); + testHandler = runScriptForItemWithBitstreams(item2, admin); + checkHandlerMessages(testHandler, admin, item2, "logos.tgz", true); // now the preview content was created since the script was run by admin user assertTrue("Expects preview content created.", previewContentService.hasPreview(context, b)); @@ -170,7 +163,7 @@ public void testWhenScriptCannotCreateFilePreview() throws Exception { public void testForAllItem() throws Exception { // Run the script TestDSpaceRunnableHandler testDSpaceRunnableHandler = new TestDSpaceRunnableHandler(); - String[] args = new String[] { "file-preview", "-e", ePerson.getEmail(), "-p", PASSWORD}; + String[] args = new String[] { "file-preview", "-e", ePerson.getEmail()}; int run = ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), testDSpaceRunnableHandler, kernelImpl); assertEquals(0, run); @@ -178,29 +171,68 @@ public void testForAllItem() throws Exception { checkNoError(testDSpaceRunnableHandler); } + @Test + public void testPreviewWithForce() throws Exception { + Item item2 = createOtherWorkspaceItemWithBitstream(ePerson, 0); + // Run the script + TestDSpaceRunnableHandler testHandler1 = runScriptForItemWithBitstreams(item2, ePerson); + checkHandlerMessages(testHandler1, ePerson, item2, "logos.tgz", true); + + // run again with force option, the existing preview content should be deleted and new one created + TestDSpaceRunnableHandler testHandler2 = new TestDSpaceRunnableHandler(); + String[] args = new String[] { "file-preview", "-u", item2.getID().toString(), + "-e", admin.getEmail(), "-f"}; + int run = ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), testHandler2, kernelImpl); + assertEquals(0, run); + checkNoError(testHandler2); + + List messages = testHandler2.getInfoMessages(); + assertThat(messages, hasSize(7)); + + assertThat(messages, hasItem(containsString("Deleting existing preview content:"))); + + Bitstream b = bitstreamService.findAll(context).stream() + .filter(bitstream -> bitstream.getName().equals("logos.tgz")) + .findFirst().orElse(null); + + assertTrue("Expects preview content created.", previewContentService.hasPreview(context, b)); + assertEquals(2, previewContentService.getPreview(context, b).size()); + } + private void checkNoError(TestDSpaceRunnableHandler testDSpaceRunnableHandler) { assertThat(testDSpaceRunnableHandler.getErrorMessages(), empty()); assertThat(testDSpaceRunnableHandler.getWarningMessages(), empty()); } - private void runScriptForItemWithBitstreams(Item item, EPerson user, String password) throws Exception { + private TestDSpaceRunnableHandler runScriptForItemWithBitstreams(Item item, EPerson user) + throws Exception { // Run the script TestDSpaceRunnableHandler testDSpaceRunnableHandler = new TestDSpaceRunnableHandler(); String[] args = new String[] { "file-preview", "-u", item.getID().toString(), - "-e", user.getEmail(), "-p", password}; + "-e", user.getEmail()}; int run = ScriptLauncher.handleScript(args, ScriptLauncher.getConfig(kernelImpl), testDSpaceRunnableHandler, kernelImpl); assertEquals(0, run); // There should be no errors or warnings checkNoError(testDSpaceRunnableHandler); - // There should be an info message about generating the file previews for the specified item + return testDSpaceRunnableHandler; + } + + private void checkHandlerMessages(TestDSpaceRunnableHandler testDSpaceRunnableHandler, + EPerson user, + Item item, + String fileName, + boolean previewGenerationExpected) { List messages = testDSpaceRunnableHandler.getInfoMessages(); - assertThat(messages, hasSize(2)); + assertThat(messages, hasSize(previewGenerationExpected ? 3 : 2)); assertThat(messages, hasItem(containsString("Generate the file previews for the specified item with " + "the given UUID: " + item.getID()))); - assertThat(messages, - hasItem(containsString("Authentication by user: " + user.getEmail()))); + assertThat(messages, hasItem(containsString("Authentication by user: " + user.getEmail()))); + if (previewGenerationExpected) { + // There should be an info message about generating the file previews for the specified bitstream + assertThat(messages, hasItem(containsString("Generating file preview for bitstream: " + fileName))); + } } private Item createOtherWorkspaceItemWithBitstream(EPerson user, int storageNumber) throws Exception { diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/PreviewContentServiceImplIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/PreviewContentServiceImplIT.java index 1249a029845a..47f7564b8eb3 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/PreviewContentServiceImplIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/PreviewContentServiceImplIT.java @@ -66,6 +66,7 @@ public class PreviewContentServiceImplIT extends AbstractControllerIntegrationTe Bitstream gzFile; Bitstream tarXzFile; Bitstream xzFile; + Bitstream tgzFileWithGzipMimeType; Bitstream tarGzFileWithWrongExtension; Bitstream tarXzFileWithIncorrectMimeType; @@ -133,6 +134,15 @@ public void setup() throws SQLException, AuthorizeException, IOException { .build(); } + try (InputStream is = getClass().getResourceAsStream("assetstore/logos.tgz")) { + tgzFileWithGzipMimeType = BitstreamBuilder. + createBitstream(context, bundle1, is) + .withName("logos.tgz") + .withDescription("tar.gz compressed file with tgz extension") + .withMimeType("application/x-gzip") + .build(); + } + try (InputStream is = getClass().getResourceAsStream("assetstore/logos.tgz")) { tgzFile = BitstreamBuilder. createBitstream(context, bundle1, is) @@ -235,18 +245,21 @@ public void destroy() throws Exception { BitstreamBuilder.deleteBitstream(tarGzFile.getID()); BitstreamFormat customMimeTypeFormat = tarXGzipFile.getFormat(context); - BitstreamBuilder.deleteBitstream(tarXGzipFile.getID()); - if (customMimeTypeFormat != null) { - bitstreamFormatService.delete(context, customMimeTypeFormat); - } + BitstreamBuilder.deleteBitstream(tarXGzipFile.getID()); BitstreamBuilder.deleteBitstream(tgzFile.getID()); BitstreamBuilder.deleteBitstream(gzFile.getID()); BitstreamBuilder.deleteBitstream(tarXzFile.getID()); BitstreamBuilder.deleteBitstream(xzFile.getID()); + BitstreamBuilder.deleteBitstream(tgzFileWithGzipMimeType.getID()); BitstreamBuilder.deleteBitstream(tarGzFileWithWrongExtension.getID()); BitstreamBuilder.deleteBitstream(tarXzFileWithIncorrectMimeType.getID()); + // removing custom mime type format created for tarXGzipFile and tgzFileWithGzipMimeType files + if (customMimeTypeFormat != null) { + bitstreamFormatService.delete(context, customMimeTypeFormat); + } + super.destroy(); } @@ -338,6 +351,11 @@ public void testXzContent() throws Exception { assertFileInfo(xzFile, "logos", 24); } + @Test + public void testTgzContentWithGzipMimetype() throws Exception { + assertFileInfos(tgzFileWithGzipMimeType); + } + @Test public void testGzContentForFileWithWrongExtension() throws Exception { assertFileInfo(tarGzFileWithWrongExtension, "TAR GZ File", 24);