[Port to dtq-dev] Issue dspace-customers#903: harden LDAP auth (email fallback, groupmap guard, logging)

[jr-rk]

Problem

Three defects in LDAP authentication (LDAPAuthentication.java):

1. A first-time LDAP login whose LDAP record has no mail attribute creates/updates an EPerson with a null e-mail, even when the user typed a valid e-mail to log in.
2. A groupmap config entry missing its : separator throws ArrayIndexOutOfBoundsException and aborts group assignment.
3. The distinguished name is dumped to stdout via System.out.println("dn:" + dn) instead of the logger.

Port of dataquest-dev/dspace-customers#903 (item 2). Only the LDAPAuthentication.java hunks are ported; the customer's authentication-ldap.cfg / Dockerfile changes stay on customer/vsb-tuo.

Source provenance (verified by diff, not commit message)

The issue's item 2 cites b64b7859b4, 40e30e6fdd, 1047f4d787, but only one of those touches this file:

• e-mail fallback → customer/vsb-tuo 40e30e6fdd.
• groupmap guard + stdout→log → customer/vsb-tuo a097b030b1 ("Added logs to see more info about ldap error") — not listed in the issue; the two cited siblings b64b7859b4 (authentication.cfg) and 1047f4d787 (Dockerfile) are config-only and out of scope.

Root cause

1. setEpersonAttributes() set the e-mail only from ldap.ldapEmail; when that was empty there was no fallback to the address the user authenticated with.
2. assignGroups() did String t[] = groupMap.split(":"); … t[1] with no length check.
3. Debug leftover writing to stdout.

Change set

LDAPAuthentication.java only:

• Add an overload setEpersonAttributes(context, eperson, ldap, netid, email); when ldap.ldapEmail is empty, fall back to the supplied email. The effective fix is in the self-register/create branch (a freshly created EPerson otherwise persists with a null mail); the already-registered-by-mail branch also passes it (harmless — that record was just looked up by that e-mail). The original 4-arg signature delegates with null, so other callers are unchanged.
• In assignGroups(), guard the groupmap entry parse: split on the first : (split(":", 2)) and skip the entry (log an error naming the index, continue to the next groupmap.<n>) when it has fewer than 2 fields or either part is blank. This stops the original ArrayIndexOutOfBoundsException and also rejects an empty search fragment such as :group, which would otherwise match every DN and assign the mapped group to all LDAP users.
• Replace System.out.println("dn:" + dn) with log.debug(LogHelper.getHeader(context, "assignGroups", "dn=" + dn)) — DEBUG (not INFO) to match the existing got DN line and keep a semi-sensitive DN off the default log level.

Not ported: the source's verbose per-iteration log.info array-dump lines (debug scaffolding) — omitted per reference/coding-standards.md (no narration logging). Only the functional guard + a single dn log line are kept. This accounts for the smaller diff vs the customer commits.

Review addressed (commit 640f46bd88)

• DN log lowered INFO → DEBUG (Copilot: DN at INFO is semi-sensitive / verbose).
• Reworded the setEpersonAttributes e-mail comment to match the code — 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 (lines 302–318), not this method.
• Both Copilot review threads resolved. No behavior change.

Test evidence

$ mvn -pl dspace-api checkstyle:check   -> 0 violations, BUILD SUCCESS
$ mvn -pl dspace-api compile            -> BUILD SUCCESS

LDAP first-login (e-mail fallback path) is not reproducible in the local stack (no LDAP directory server), so per docker-setup §7 that part is stated, not faked. The groupmap guard is pure config-string parsing and is unit-testable without LDAP — a focused test for the malformed-entry case is a reasonable follow-up (see open point below).

Behaviour delta:

1. e-mail fallback fills a previously-null EPerson.mail in the create path;
2. a groupmap entry with no : now logs an error and is skipped instead of throwing ArrayIndexOutOfBoundsException;
3. the dn is logged via LogHelper at DEBUG, not printed to stdout.

Risk & rollback

Confined to LDAP login. The e-mail fallback only fills a value that was previously null; the groupmap guard only adds a skip on malformed config; the log line replaces stdout. Revert = drop the two commits on this branch.

Open points (non-blocking)

• No automated test lands with this port. The login/e-mail paths need a directory server, but the groupmap guard does not — a unit test for the malformed-groupmap case is the one gap worth closing (tracked as follow-up rather than expanding this PR's scope).
• Tracking issue is cross-repo (dspace-customers#903), so GitHub's development section cannot auto-link it; the reference above is the trace. A status/provenance note is posted on use dspace.url to dspace.server.url #903.

Notes / assumptions

LDAP directory config is customer-specific and intentionally left on customer/vsb-tuo.

[Copilot]

Pull request overview

This PR ports targeted hardening improvements to LDAPAuthentication to avoid bad/unsafe behavior during LDAP login (null EPerson email on first login, malformed groupmap entries crashing group assignment, and stdout debug output).

Changes:

• Pass the computed login email into setEpersonAttributes() and add an overload to fall back to that email when LDAP provides no mail attribute.
• Guard groupmap parsing in assignGroups() to skip (and log) malformed entries instead of throwing ArrayIndexOutOfBoundsException.
• Replace System.out.println() DN output with structured logging via LogHelper.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jr-rk]

Addressed the Copilot review in 640f46b (non-blocking polish, no behavior change):

• assignGroups: DN logged at DEBUG instead of INFO, matching the existing got DN line.
• Reworded the setEpersonAttributes e-mail comment to match what the method actually does.

Both threads resolved. checkstyle:check + compile on dspace-api pass.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

dspace-api/src/main/java/org/dspace/authenticate/LDAPAuthentication.java:764

• This new config-parsing behavior (skip malformed groupmap entries and log an error) is unit-testable without an LDAP server. Since this package already has JUnit tests, adding a focused test for a missing/invalid ':' (and blank-side cases, if handled) would help prevent regressions in auth hardening.

                if (t.length < 2) {
                    log.error(LogHelper.getHeader(context, "assignGroups",
                        "malformed groupmap entry at index " + groupmapIndex + ": " + groupMap +
                        " - missing ':' separator"));
                    groupMap = configurationService.getProperty(

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

dspace-api/src/main/java/org/dspace/authenticate/LDAPAuthentication.java:766

• The new groupmap parsing guard is a functional behavior change (skipping malformed entries instead of throwing) but there’s no automated test coverage for it. There are existing authentication unit tests in this module (e.g. ShibHeadersTest, IPMatcherTest), so adding a focused unit test for malformed authentication-ldap.login.groupmap.<n> values (missing ':', leading ':', trailing ':', and whitespace-only parts) would help prevent regressions without requiring an LDAP server.

            while (groupMap != null) {
                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 +
                        " - expected '<ldapSearchFragment>:<dspaceGroupName>' with both parts non-empty"));
                    groupMap = configurationService.getProperty(
                            "authentication-ldap.login.groupmap." + ++groupmapIndex);
                    continue;