From ed471e0f0e83df0439b7c26464027b010979c783 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Thu, 16 Jul 2026 15:28:09 +0200 Subject: [PATCH 1/4] ZCU-PUB/test: shibboleth special groups must survive short-lived and refreshed tokens Replicates dataquest-dev/DSpace#900: a bitstream restricted to the default shibboleth group (Authenticated) is readable with the login token, but the download via a short-lived token returns 403, because the special groups are recomputed from the (missing) servlet session instead of the user context when a new token is minted. Co-Authored-By: Claude Fable 5 --- .../ClarinShibbolethSpecialGroupsIT.java | 178 ++++++++++++++++++ 1 file changed, 178 insertions(+) create mode 100644 dspace-server-webapp/src/test/java/org/dspace/app/rest/security/ClarinShibbolethSpecialGroupsIT.java 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..00602fe3cbb9 --- /dev/null +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/security/ClarinShibbolethSpecialGroupsIT.java @@ -0,0 +1,178 @@ +/** + * 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.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 java.util.Map; + +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 static final String AUTHORIZATION_TYPE = "Bearer "; + + 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 refreshedToken = getClient(loginToken).perform(post("/api/authn/login")) + .andExpect(status().isOk()) + .andReturn().getResponse().getHeader("Authorization") + .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"); + 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(); + Map map = mapper.readValue(content, Map.class); + return String.valueOf(map.get("token")); + } +} From 9bb81d686ab8e3759362589830975405c667e474 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/4] UFAL/Obtain special groups from user context when new token is generated (on token refresh) (ufal/clarin-dspace#1378) (#1347) * Issue 1373: obtain special groups from user context when new token is generated (on token refresh) * resolve Copilot comments * resolve Copilot Comments: compute special groups only when when user is authenticated * Remove HttpSession dependency from ClarinShibAuthentication Use request-scoped attributes for shib.authenticated instead of HttpSession/JSESSIONID, aligning with upstream ShibAuthentication. Follow-up to ufal/clarin-dspace#1373/ufal/clarin-dspace#1378. * Guard against null special groups in Context.getSpecialGroups A special-group UUID may reference a Group that has since been deleted; GroupService.find returns null in that case. The list was built with an unconditional add, so it could contain null elements, which caused an NPE downstream (e.g. SpecialGroupClaimProvider.getValue maps group.getID() while generating the JWT sg claim on token refresh). Filter nulls once here so every caller is covered. Follow-up to ufal/clarin-dspace#1373/ufal/clarin-dspace#1378. --------- (cherry picked from commit 4c294b24585a8581361aeb59aa2fb160ca9801c6) Co-authored-by: Milan Kuchtiak (cherry picked from commit 74f5862748412bebb6bb7ab43952308871779bf9) --- .../clarin/ClarinShibAuthentication.java | 41 ++++++++----------- .../main/java/org/dspace/core/Context.java | 7 +++- 2 files changed, 23 insertions(+), 25 deletions(-) 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 822543d08c80..b73ef877145c 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 @@ -267,7 +267,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."); @@ -320,42 +320,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(); } } @@ -1315,7 +1308,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 02a3fee09f8a..34ab7491c8da 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; From 57ba86a2f24e535a453dfbb00764e9912a9a571e Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Thu, 16 Jul 2026 15:57:59 +0200 Subject: [PATCH 3/4] ZCU-PUB/test: fail cleanly on a missing Authorization header, avoid a raw-type read Co-Authored-By: Claude Fable 5 --- .../app/rest/security/ClarinShibbolethSpecialGroupsIT.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 index 00602fe3cbb9..004e35f4f12a 100644 --- 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 @@ -7,12 +7,12 @@ */ 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 java.util.Map; import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.commons.codec.CharEncoding; @@ -163,6 +163,7 @@ private String shibLogin() throws Exception { .header("SHIB-NETID", NET_ID_TEST_EPERSON)) .andExpect(status().is3xxRedirection()) .andReturn().getResponse().getHeader("Authorization"); + assertNotNull("The shibboleth login must return the Authorization header", authHeader); return authHeader.replace(AUTHORIZATION_TYPE, ""); } @@ -172,7 +173,6 @@ private String getShortLivedToken(String loginToken) throws Exception { .andExpect(status().isOk()) .andReturn(); String content = mvcResult.getResponse().getContentAsString(); - Map map = mapper.readValue(content, Map.class); - return String.valueOf(map.get("token")); + return mapper.readTree(content).get("token").asText(); } } From b7d3c786acee1781a093a7522f7b70ed66d639b2 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Thu, 16 Jul 2026 16:29:35 +0200 Subject: [PATCH 4/4] ZCU-PUB/test: resolve Copilot review comments - reuse AUTHORIZATION_HEADER/AUTHORIZATION_TYPE from AbstractControllerIntegrationTest - assert the Authorization header and the token field are present before using them Co-Authored-By: Claude Fable 5 --- .../security/ClarinShibbolethSpecialGroupsIT.java | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) 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 index 004e35f4f12a..079eef92905e 100644 --- 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 @@ -14,6 +14,7 @@ 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; @@ -55,7 +56,6 @@ public class ClarinShibbolethSpecialGroupsIT extends AbstractControllerIntegrati 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 static final String AUTHORIZATION_TYPE = "Bearer "; private EPerson clarinEperson; private Bitstream restrictedBitstream; @@ -146,10 +146,11 @@ public void shouldKeepSpecialGroupsAfterLoginTokenRefresh() throws Exception { .andExpect(status().isOk()); // Refresh the login token on a stateless request (no shibboleth session/headers) - String refreshedToken = getClient(loginToken).perform(post("/api/authn/login")) + String refreshedAuthHeader = getClient(loginToken).perform(post("/api/authn/login")) .andExpect(status().isOk()) - .andReturn().getResponse().getHeader("Authorization") - .replace(AUTHORIZATION_TYPE, ""); + .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")) @@ -162,7 +163,7 @@ private String shibLogin() throws Exception { .header("Shib-Identity-Provider", IDP_TEST_EPERSON) .header("SHIB-NETID", NET_ID_TEST_EPERSON)) .andExpect(status().is3xxRedirection()) - .andReturn().getResponse().getHeader("Authorization"); + .andReturn().getResponse().getHeader(AUTHORIZATION_HEADER); assertNotNull("The shibboleth login must return the Authorization header", authHeader); return authHeader.replace(AUTHORIZATION_TYPE, ""); } @@ -173,6 +174,8 @@ private String getShortLivedToken(String loginToken) throws Exception { .andExpect(status().isOk()) .andReturn(); String content = mvcResult.getResponse().getContentAsString(); - return mapper.readTree(content).get("token").asText(); + JsonNode token = mapper.readTree(content).get("token"); + assertNotNull("The shortlivedtokens response must contain the token field", token); + return token.asText(); } }