Skip to content

Commit 669ffec

Browse files
milanmajchrakclaudekosarkokuchtiak-ufal
authored
ZCU-DATA/fix: shibboleth special groups lost in short-lived and refreshed tokens - 403 on download (backport #1347) (#1375)
* ZCU-DATA/test: shibboleth special groups must survive short-lived and refreshed tokens Replicates #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 <noreply@anthropic.com> * UFAL/Obtain special groups from user context when new token is generated (on token refresh) (ufal#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#1373/ufal#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#1373/ufal#1378. --------- (cherry picked from commit 4c294b2) Co-authored-by: Milan Kuchtiak <kuchtiak@ufal.mff.cuni.cz> (cherry picked from commit 74f5862) * ZCU-DATA/test: fail cleanly on a missing Authorization header, avoid a raw-type read Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ZCU-DATA/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 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Ondřej Košarko <ko_ok@centrum.cz> Co-authored-by: Milan Kuchtiak <kuchtiak@ufal.mff.cuni.cz>
1 parent 2e43782 commit 669ffec

3 files changed

Lines changed: 204 additions & 25 deletions

File tree

‎dspace-api/src/main/java/org/dspace/authenticate/clarin/ClarinShibAuthentication.java‎

Lines changed: 17 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,7 @@ public int authenticate(Context context, String username, String password,
267267

268268
// Step 4: Log the user in.
269269
context.setCurrentUser(eperson);
270-
request.getSession().setAttribute("shib.authenticated", true);
270+
request.setAttribute("shib.authenticated", true);
271271
AuthenticateServiceFactory.getInstance().getAuthenticationService().initEPerson(context, request, eperson);
272272

273273
log.info(eperson.getEmail() + " has been authenticated via shibboleth.");
@@ -320,42 +320,35 @@ public int authenticate(Context context, String username, String password,
320320
@Override
321321
public List<Group> getSpecialGroups(Context context, HttpServletRequest request) {
322322
try {
323-
// User has not successfuly authenticated via shibboleth.
324-
if (request == null ||
325-
context.getCurrentUser() == null ||
326-
request.getSession().getAttribute("shib.authenticated") == null) {
327-
return Collections.EMPTY_LIST;
323+
// User has not successfully authenticated via shibboleth.
324+
if (request == null || context.getCurrentUser() == null) {
325+
return Collections.emptyList();
328326
}
329327

330-
// If we have already calculated the special groups then return them.
331-
if (request.getSession().getAttribute("shib.specialgroup") != null) {
332-
log.debug("Returning cached special groups.");
333-
List<UUID> sessionGroupIds = (List<UUID>) request.getSession().getAttribute("shib.specialgroup");
334-
List<Group> result = new ArrayList<>();
335-
for (UUID uuid : sessionGroupIds) {
336-
result.add(groupService.find(context, uuid));
337-
}
338-
return result;
328+
List<Group> specialGroups = context.getSpecialGroups();
329+
if (!specialGroups.isEmpty()) {
330+
log.debug("Returning special groups from context.");
331+
return specialGroups;
339332
}
340333

334+
if (request.getAttribute("shib.authenticated") == null) {
335+
log.debug("User has not been authenticated via shibboleth, returning empty list of special groups.");
336+
return Collections.emptyList();
337+
}
341338

342339
List<UUID> groupIds = new ShibGroup(new ShibHeaders(request), context).get();
343-
// Cache the special groups, so we don't have to recalculate them again
344-
// for this session.
345-
request.getSession().setAttribute("shib.specialgroup", groupIds);
346340

347341
List<Group> groups = new ArrayList<>();
348342
for (UUID uuid : groupIds) {
349343
Group foundGroup = groupService.find(context, uuid);
350-
if (Objects.isNull(foundGroup)) {
351-
continue;
344+
if (foundGroup != null) {
345+
groups.add(foundGroup);
352346
}
353-
groups.add(foundGroup);
354347
}
355348
return groups;
356349
} catch (Throwable t) {
357-
log.error("Unable to validate any sepcial groups this user may belong too because of an exception.", t);
358-
return Collections.EMPTY_LIST;
350+
log.error("Unable to validate any special groups this user may belong to because of an exception.", t);
351+
return Collections.emptyList();
359352
}
360353
}
361354

@@ -1315,7 +1308,7 @@ private String getShibURL(HttpServletRequest request) {
13151308
public boolean isUsed(final Context context, final HttpServletRequest request) {
13161309
if (request != null &&
13171310
context.getCurrentUser() != null &&
1318-
request.getSession().getAttribute("shib.authenticated") != null) {
1311+
request.getAttribute("shib.authenticated") != null) {
13191312
return true;
13201313
}
13211314
return false;

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -686,7 +686,12 @@ public boolean inSpecialGroup(UUID groupID) {
686686
public List<Group> getSpecialGroups() throws SQLException {
687687
List<Group> myGroups = new ArrayList<>();
688688
for (UUID groupId : specialGroups) {
689-
myGroups.add(EPersonServiceFactory.getInstance().getGroupService().find(this, groupId));
689+
Group group = EPersonServiceFactory.getInstance().getGroupService().find(this, groupId);
690+
// A special group UUID may reference a group that has since been deleted; skip nulls
691+
// so callers never receive a list containing null (avoids NPE downstream).
692+
if (group != null) {
693+
myGroups.add(group);
694+
}
690695
}
691696

692697
return myGroups;

‎dspace-server-webapp/src/test/java/org/dspace/app/rest/security/ClarinShibbolethSpecialGroupsIT.java‎

Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
/**
2+
* The contents of this file are subject to the license and copyright
3+
* detailed in the LICENSE and NOTICE files at the root of the source
4+
* tree and available online at
5+
*
6+
* http://www.dspace.org/license/
7+
*/
8+
package org.dspace.app.rest.security;
9+
10+
import static org.junit.Assert.assertNotNull;
11+
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
12+
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
13+
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
14+
15+
import java.io.InputStream;
16+
17+
import com.fasterxml.jackson.databind.JsonNode;
18+
import com.fasterxml.jackson.databind.ObjectMapper;
19+
import org.apache.commons.codec.CharEncoding;
20+
import org.apache.commons.io.IOUtils;
21+
import org.dspace.app.rest.test.AbstractControllerIntegrationTest;
22+
import org.dspace.app.util.Util;
23+
import org.dspace.builder.BitstreamBuilder;
24+
import org.dspace.builder.CollectionBuilder;
25+
import org.dspace.builder.CommunityBuilder;
26+
import org.dspace.builder.EPersonBuilder;
27+
import org.dspace.builder.GroupBuilder;
28+
import org.dspace.builder.ItemBuilder;
29+
import org.dspace.content.Bitstream;
30+
import org.dspace.content.Collection;
31+
import org.dspace.content.Community;
32+
import org.dspace.content.Item;
33+
import org.dspace.core.I18nUtil;
34+
import org.dspace.eperson.EPerson;
35+
import org.dspace.eperson.Group;
36+
import org.dspace.services.ConfigurationService;
37+
import org.junit.Before;
38+
import org.junit.Test;
39+
import org.springframework.beans.factory.annotation.Autowired;
40+
import org.springframework.test.web.servlet.MvcResult;
41+
42+
/**
43+
* Integration test verifying that the Shibboleth special groups (e.g. the default `Authenticated` group)
44+
* survive into tokens which are minted on stateless REST requests after the login:
45+
* the short-lived token used for bitstream downloads and the refreshed login token.
46+
*
47+
* Replicates https://github.com/dataquest-dev/DSpace/issues/900 - a bitstream restricted to the
48+
* `Authenticated` group is visible after the Shibboleth login, but its download returns 403,
49+
* because the special groups are lost when the short-lived token is generated
50+
* (see ufal/clarin-dspace#1373).
51+
*
52+
* @author Milan Majchrak (milan.majchrak at dataquest.sk)
53+
*/
54+
public class ClarinShibbolethSpecialGroupsIT extends AbstractControllerIntegrationTest {
55+
56+
public static final String[] SHIB_ONLY = {"org.dspace.authenticate.clarin.ClarinShibAuthentication"};
57+
private static final String NET_ID_TEST_EPERSON = "123456789";
58+
private static final String IDP_TEST_EPERSON = "Test Idp";
59+
60+
private EPerson clarinEperson;
61+
private Bitstream restrictedBitstream;
62+
63+
@Autowired
64+
ConfigurationService configurationService;
65+
66+
@Before
67+
public void setup() throws Exception {
68+
super.setUp();
69+
70+
// Enable Shibboleth login for all tests
71+
configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", SHIB_ONLY);
72+
73+
context.turnOffAuthorisationSystem();
74+
75+
// Create an eperson with netID - that means the user already exists in the database
76+
clarinEperson = EPersonBuilder.createEPerson(context)
77+
.withCanLogin(false)
78+
.withEmail("clarin@email.com")
79+
.withNameInMetadata("first", "last")
80+
.withLanguage(I18nUtil.getDefaultLocale().getLanguage())
81+
.withNetId(Util.formatNetId(NET_ID_TEST_EPERSON, IDP_TEST_EPERSON))
82+
.build();
83+
84+
// The group every shibboleth-authenticated user is implicitly added to (as a special group)
85+
String defaultGroupName = configurationService.getProperty("authentication-shibboleth.default.auth.group");
86+
Group authenticatedGroup = GroupBuilder.createGroup(context)
87+
.withName(defaultGroupName)
88+
.build();
89+
90+
// A bitstream readable only by the shibboleth default special group
91+
Community community = CommunityBuilder.createCommunity(context)
92+
.withName("Community")
93+
.build();
94+
Collection collection = CollectionBuilder.createCollection(context, community)
95+
.withName("Collection")
96+
.build();
97+
Item item = ItemBuilder.createItem(context, collection)
98+
.withTitle("Item with a restricted bitstream")
99+
.build();
100+
try (InputStream is = IOUtils.toInputStream("Restricted content", CharEncoding.UTF_8)) {
101+
restrictedBitstream = BitstreamBuilder.createBitstream(context, item, is)
102+
.withName("restricted.txt")
103+
.withMimeType("text/plain")
104+
.withReaderGroup(authenticatedGroup)
105+
.build();
106+
}
107+
108+
context.restoreAuthSystemState();
109+
}
110+
111+
/**
112+
* Replication of the issue #900:
113+
* 1. Sign in via Shibboleth - the user is implicitly added into the `Authenticated` special group.
114+
* 2. The bitstream restricted to the `Authenticated` group is readable with the login token.
115+
* 3. The UI downloads the bitstream with a short-lived token minted on a separate stateless request
116+
* - the download must succeed too.
117+
*/
118+
@Test
119+
public void shouldDownloadRestrictedBitstreamWithShortLivedTokenAfterShibLogin() throws Exception {
120+
String loginToken = shibLogin();
121+
122+
// Sanity check: the login token keeps the special groups (its `sg` claim was computed
123+
// during the shibboleth login request), so the restricted bitstream is readable.
124+
getClient(loginToken).perform(get("/api/core/bitstreams/" + restrictedBitstream.getID() + "/content"))
125+
.andExpect(status().isOk());
126+
127+
// The short-lived token is minted on a stateless request - the special groups must be
128+
// obtained from the user context (restored from the login token), not from the session.
129+
String shortLivedToken = getShortLivedToken(loginToken);
130+
getClient().perform(get("/api/core/bitstreams/" + restrictedBitstream.getID()
131+
+ "/content?authentication-token=" + shortLivedToken))
132+
.andExpect(status().isOk());
133+
}
134+
135+
/**
136+
* The refreshed login token (POST /api/authn/login with the Bearer token, no shibboleth headers)
137+
* must keep the special groups too, otherwise the user loses the access after the first token refresh
138+
* (see ufal/clarin-dspace#1373).
139+
*/
140+
@Test
141+
public void shouldKeepSpecialGroupsAfterLoginTokenRefresh() throws Exception {
142+
String loginToken = shibLogin();
143+
144+
// Sanity check: the restricted bitstream is readable with the login token
145+
getClient(loginToken).perform(get("/api/core/bitstreams/" + restrictedBitstream.getID() + "/content"))
146+
.andExpect(status().isOk());
147+
148+
// Refresh the login token on a stateless request (no shibboleth session/headers)
149+
String refreshedAuthHeader = getClient(loginToken).perform(post("/api/authn/login"))
150+
.andExpect(status().isOk())
151+
.andReturn().getResponse().getHeader(AUTHORIZATION_HEADER);
152+
assertNotNull("The token refresh must return the Authorization header", refreshedAuthHeader);
153+
String refreshedToken = refreshedAuthHeader.replace(AUTHORIZATION_TYPE, "");
154+
155+
// The restricted bitstream must still be readable with the refreshed token
156+
getClient(refreshedToken).perform(get("/api/core/bitstreams/" + restrictedBitstream.getID() + "/content"))
157+
.andExpect(status().isOk());
158+
}
159+
160+
private String shibLogin() throws Exception {
161+
String authHeader = getClient().perform(get("/api/authn/shibboleth")
162+
.header("SHIB-MAIL", clarinEperson.getEmail())
163+
.header("Shib-Identity-Provider", IDP_TEST_EPERSON)
164+
.header("SHIB-NETID", NET_ID_TEST_EPERSON))
165+
.andExpect(status().is3xxRedirection())
166+
.andReturn().getResponse().getHeader(AUTHORIZATION_HEADER);
167+
assertNotNull("The shibboleth login must return the Authorization header", authHeader);
168+
return authHeader.replace(AUTHORIZATION_TYPE, "");
169+
}
170+
171+
private String getShortLivedToken(String loginToken) throws Exception {
172+
ObjectMapper mapper = new ObjectMapper();
173+
MvcResult mvcResult = getClient(loginToken).perform(post("/api/authn/shortlivedtokens"))
174+
.andExpect(status().isOk())
175+
.andReturn();
176+
String content = mvcResult.getResponse().getContentAsString();
177+
JsonNode token = mapper.readTree(content).get("token");
178+
assertNotNull("The shortlivedtokens response must contain the token field", token);
179+
return token.asText();
180+
}
181+
}

0 commit comments

Comments
 (0)