From 681484367e79a8fe09d91e91e262f4a1aafc4f16 Mon Sep 17 00:00:00 2001 From: Milan Kuchtiak Date: Mon, 29 Jun 2026 13:47:13 +0200 Subject: [PATCH] Issue 1373: obtain special groups from user context when new token is generated (on token refresh) (ufal/clarin-dspace#1378) * 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. --------- Co-authored-by: Ondrej Kosarko (cherry picked from commit 4c294b24585a8581361aeb59aa2fb160ca9801c6) --- .../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 ba5d8cd65bfa..ef0b9e4b0a82 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 e721deff5e71..3f615d4ebfb2 100644 --- a/dspace-api/src/main/java/org/dspace/core/Context.java +++ b/dspace-api/src/main/java/org/dspace/core/Context.java @@ -714,7 +714,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;