From b9e35e314b81e350a9069733a8cc860ea3d0694f Mon Sep 17 00:00:00 2001 From: jr-rk <95219754+jr-rk@users.noreply.github.com> Date: Tue, 11 Aug 2026 16:06:11 +0200 Subject: [PATCH 1/3] fix(ldap): harden LDAPAuthentication (email fallback, groupmap guard, logging) Three isolated defects in LDAP login: - setEpersonAttributes now falls back to the login e-mail when LDAP provides none, so an EPerson is never created/updated with a null mail. - assignGroups guards a groupmap entry that has no ':' separator (previously an ArrayIndexOutOfBoundsException) -- it logs and skips the malformed entry. - The distinguished name is logged via LogHelper instead of System.out.println. Only the LDAPAuthentication.java hunks are ported; the customer's LDAP config (authentication-ldap.cfg, Dockerfile) stays on customer/vsb-tuo. The source's verbose per-iteration debug logging is intentionally omitted per reference/coding-standards.md. Port of dataquest-dev/dspace-customers#903 (item 2). Source: customer/vsb-tuo 40e30e6fdd. Co-Authored-By: Claude Fable 5 --- .../authenticate/LDAPAuthentication.java | 27 ++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/authenticate/LDAPAuthentication.java b/dspace-api/src/main/java/org/dspace/authenticate/LDAPAuthentication.java index da6a7092481..9a767789242 100644 --- a/dspace-api/src/main/java/org/dspace/authenticate/LDAPAuthentication.java +++ b/dspace-api/src/main/java/org/dspace/authenticate/LDAPAuthentication.java @@ -322,7 +322,7 @@ public int authenticate(Context context, log.info(LogHelper.getHeader(context, "type=ldap-login", "type=ldap_but_already_email")); context.turnOffAuthorisationSystem(); - setEpersonAttributes(context, eperson, ldap, Optional.of(netid)); + setEpersonAttributes(context, eperson, ldap, Optional.of(netid), email); ePersonService.update(context, eperson); context.dispatchEvents(); context.restoreAuthSystemState(); @@ -339,7 +339,7 @@ public int authenticate(Context context, try { context.turnOffAuthorisationSystem(); eperson = ePersonService.create(context); - setEpersonAttributes(context, eperson, ldap, Optional.of(netid)); + setEpersonAttributes(context, eperson, ldap, Optional.of(netid), email); eperson.setCanLogIn(true); authenticationService.initEPerson(context, request, eperson); ePersonService.update(context, eperson); @@ -382,9 +382,21 @@ public int authenticate(Context context, */ private void setEpersonAttributes(Context context, EPerson eperson, SpeakerToLDAP ldap, Optional netid) throws SQLException { + setEpersonAttributes(context, eperson, ldap, netid, null); + } + + /** + * Update eperson's attributes, falling back to the supplied login e-mail when LDAP has none. + */ + private void setEpersonAttributes(Context context, EPerson eperson, SpeakerToLDAP ldap, + Optional netid, String email) throws SQLException { + // Set the e-mail: prefer the LDAP-provided address, otherwise the one the user logged in with, + // so an EPerson is never persisted with a null e-mail. if (StringUtils.isNotEmpty(ldap.ldapEmail)) { eperson.setEmail(ldap.ldapEmail); + } else if (StringUtils.isNotEmpty(email)) { + eperson.setEmail(email); } if (StringUtils.isNotEmpty(ldap.ldapGivenName)) { eperson.setFirstName(context, ldap.ldapGivenName); @@ -734,7 +746,7 @@ public String getName() { */ private void assignGroups(String dn, ArrayList group, Context context) { if (StringUtils.isNotBlank(dn)) { - System.out.println("dn:" + dn); + log.info(LogHelper.getHeader(context, "assignGroups", "dn=" + dn)); int groupmapIndex = 1; String groupMap = configurationService.getProperty("authentication-ldap.login.groupmap." + groupmapIndex); boolean cmp; @@ -744,6 +756,15 @@ private void assignGroups(String dn, ArrayList group, Context context) { // outer loop with the DSpace groups while (groupMap != null) { String t[] = groupMap.split(":"); + if (t.length < 2) { + log.error(LogHelper.getHeader(context, "assignGroups", + "malformed groupmap entry at index " + groupmapIndex + ": " + groupMap + + " - missing ':' separator")); + groupMap = configurationService.getProperty( + "authentication-ldap.login.groupmap." + ++groupmapIndex); + continue; + } + String ldapSearchString = t[0]; String dspaceGroupName = t[1]; From 640f46bd8814ee52720c81393c914d2e76cc498f Mon Sep 17 00:00:00 2001 From: jr-rk <95219754+jr-rk@users.noreply.github.com> Date: Wed, 12 Aug 2026 16:18:45 +0200 Subject: [PATCH 2/3] =?UTF-8?q?fix(ldap):=20address=20review=20=E2=80=94?= =?UTF-8?q?=20DN=20log=20level=20+=20comment=20wording?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Log the distinguished name at DEBUG (not INFO) in assignGroups, matching the existing "got DN" DEBUG line and keeping a semi-sensitive DN off the default log level. - Reword the setEpersonAttributes e-mail comment: the method leaves the e-mail unchanged when neither LDAP nor the login address supplies one; the non-null guarantee for the create paths lives in the caller, not here. Addresses Copilot review on dataquest-dev/DSpace#1408. No behavior change. Co-Authored-By: Claude Opus 4.8 --- .../java/org/dspace/authenticate/LDAPAuthentication.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/authenticate/LDAPAuthentication.java b/dspace-api/src/main/java/org/dspace/authenticate/LDAPAuthentication.java index 9a767789242..2cc01ad402e 100644 --- a/dspace-api/src/main/java/org/dspace/authenticate/LDAPAuthentication.java +++ b/dspace-api/src/main/java/org/dspace/authenticate/LDAPAuthentication.java @@ -391,8 +391,9 @@ private void setEpersonAttributes(Context context, EPerson eperson, SpeakerToLDA private void setEpersonAttributes(Context context, EPerson eperson, SpeakerToLDAP ldap, Optional netid, String email) throws SQLException { - // Set the e-mail: prefer the LDAP-provided address, otherwise the one the user logged in with, - // so an EPerson is never persisted with a null e-mail. + // Set the e-mail: prefer the LDAP-provided address, otherwise fall back to the + // login e-mail when one was supplied. If neither is available, the existing + // e-mail is left unchanged. if (StringUtils.isNotEmpty(ldap.ldapEmail)) { eperson.setEmail(ldap.ldapEmail); } else if (StringUtils.isNotEmpty(email)) { @@ -746,7 +747,7 @@ public String getName() { */ private void assignGroups(String dn, ArrayList group, Context context) { if (StringUtils.isNotBlank(dn)) { - log.info(LogHelper.getHeader(context, "assignGroups", "dn=" + dn)); + log.debug(LogHelper.getHeader(context, "assignGroups", "dn=" + dn)); int groupmapIndex = 1; String groupMap = configurationService.getProperty("authentication-ldap.login.groupmap." + groupmapIndex); boolean cmp; From d5caebdeb4b31d680dc99885fce13ff23f631adc Mon Sep 17 00:00:00 2001 From: jr-rk <95219754+jr-rk@users.noreply.github.com> Date: Thu, 13 Aug 2026 10:38:45 +0200 Subject: [PATCH 3/3] fix(ldap): reject groupmap entries with a blank search or group part The malformed-entry guard in assignGroups only rejected a missing ':'. An entry with an empty left part (e.g. ":admins") still passed, leaving an empty ldapSearchString; the subsequent containsIgnoreCase(dn, "" + ",") then matched essentially every DN and assigned the mapped group to all LDAP users. Tighten the guard to also reject blank left/right parts (StringUtils.isBlank), and parse with split(":", 2) so a colon inside a DSpace group name is preserved instead of truncated. Well-formed entries parse identically; only genuinely malformed lines (":group", "group:", ":") are now skipped. Addresses Copilot review on dataquest-dev/DSpace#1408. Co-Authored-By: Claude Opus 4.8 --- .../java/org/dspace/authenticate/LDAPAuthentication.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/authenticate/LDAPAuthentication.java b/dspace-api/src/main/java/org/dspace/authenticate/LDAPAuthentication.java index 2cc01ad402e..dc50e39bf09 100644 --- a/dspace-api/src/main/java/org/dspace/authenticate/LDAPAuthentication.java +++ b/dspace-api/src/main/java/org/dspace/authenticate/LDAPAuthentication.java @@ -756,11 +756,11 @@ private void assignGroups(String dn, ArrayList group, Context context) { // groupmap contains the mapping of LDAP groups to DSpace groups // outer loop with the DSpace groups while (groupMap != null) { - String t[] = groupMap.split(":"); - if (t.length < 2) { + String t[] = groupMap.split(":", 2); + if (t.length < 2 || StringUtils.isBlank(t[0]) || StringUtils.isBlank(t[1])) { log.error(LogHelper.getHeader(context, "assignGroups", "malformed groupmap entry at index " + groupmapIndex + ": " + groupMap + - " - missing ':' separator")); + " - expected ':' with both parts non-empty")); groupMap = configurationService.getProperty( "authentication-ldap.login.groupmap." + ++groupmapIndex); continue;