ZCU-PUB/fix: shibboleth special groups lost in short-lived and refreshed tokens - 403 on download (backport #1347)

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<Group> 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<UUID> sessionGroupIds = (List<UUID>) request.getSession().getAttribute("shib.specialgroup");
-                List<Group> result = new ArrayList<>();
-                for (UUID uuid : sessionGroupIds) {
-                    result.add(groupService.find(context, uuid));
-                }
-                return result;
+            List<Group> 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<UUID> 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<Group> 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;

dspace-api/src/main/java/org/dspace/core/Context.java

@@ -686,7 +686,12 @@ public boolean inSpecialGroup(UUID groupID) {
     public List<Group> getSpecialGroups() throws SQLException {
         List<Group> 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;

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.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.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");
+        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();
+        return mapper.readTree(content).get("token").asText();
+    }
+}