[wip] identity linking by kosarko · Pull Request #1382 · ufal/clarin-dspace

kosarko · 2026-07-09T07:17:04Z

Introduces ClarinIdentityService as the single login-lookup path backed by an eperson_netid_alias table (eperson.netid becomes a denormalized display column). When an allowlisted identity proxy (e.g. e-INFRA CZ/Perun) asserts voperson_external_id, ClarinShibAuthentication auto-links the login to the matching existing EPerson instead of falling through to email matching or auto-registration, avoiding duplicate accounts across IdP paths. Admins can attach an alias manually via POST /api/clarin/identity/link.

Note

This PR originally also contained the EPerson merge tooling; that was split out into its own stacked PR to keep the review manageable.

Problem description

Analysis

(Write here, if there is needed describe some specific problem. Erase it, when it is not needed.)

Problems

(Write here, if some unexpected problems occur during solving issues. Erase it, when it is not needed.)

Manual Testing (if applicable)

Added to testing scenarios

Copilot review

Requested review from Copilot

Copilot

Pull request overview

This PR introduces CLARIN-specific identity linking backed by a new eperson_netid_alias table (making netid resolution consistent across login paths), and adds an admin eperson-merge tool to safely merge duplicate EPerson accounts while preserving an audit trail.

Changes:

Add ClarinIdentityService + alias table mappings/migrations, and route Shibboleth netid resolution through it (including proxy-driven auto-linking via voperson_external_id for allowlisted proxies).
Add EPersonMergeService + CLI script to re-point key foreign keys, dedupe memberships/policies/subscriptions, tombstone the source account, and record a JSON audit record in eperson_merge_audit.
Add REST admin endpoint for manual alias linking, plus supporting service/DAO beans and integration tests.

Reviewed changes

Copilot reviewed 37 out of 37 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
dspace/config/spring/api/scripts.xml	Registers `eperson-merge` CLI script.
dspace/config/spring/api/core-services.xml	Wires new identity + merge services.
dspace/config/spring/api/core-dao-services.xml	Wires new alias + audit DAOs.
dspace/config/modules/authentication-shibboleth.cfg	Adds config for `voperson_external_id` header.
dspace/config/hibernate.cfg.xml	Registers Hibernate mappings for new entities.
dspace/config/clarin-dspace.cfg	Adds `identity.auto-link.proxy-allowlist` configuration.
dspace-server-webapp/src/main/java/org/dspace/app/rest/security/clarin/ClarinShibbolethLoginFilter.java	Switches netid lookup to use `ClarinIdentityService`.
dspace-server-webapp/src/main/java/org/dspace/app/rest/ClarinIdentityLinkController.java	Adds admin REST endpoint for manual alias attachment.
dspace-api/src/test/java/org/dspace/eperson/clarin/EPersonMergeServiceTest.java	Integration tests for merge behavior (FK moves, dedupe, tombstone, audit).
dspace-api/src/test/java/org/dspace/eperson/clarin/ClarinIdentityServiceTest.java	Tests for alias resolution, attach semantics, and auto-link matrix.
dspace-api/src/main/resources/org/dspace/storage/rdbms/sqlmigration/postgres/V7.6_2026.07.08__eperson_identity_linking.sql	Postgres migration for alias + audit tables and seeding.
dspace-api/src/main/resources/org/dspace/storage/rdbms/sqlmigration/h2/V7.6_2026.07.08__eperson_identity_linking.sql	H2 migration for alias + audit tables and seeding.
dspace-api/src/main/java/org/dspace/versioning/VersioningServiceImpl.java	Adds version lookup by `EPerson` for merge support.
dspace-api/src/main/java/org/dspace/versioning/service/VersioningService.java	Declares `findByEPerson()` API for merge.
dspace-api/src/main/java/org/dspace/versioning/dao/VersionDAO.java	Declares DAO query for versions by `EPerson`.
dspace-api/src/main/java/org/dspace/versioning/dao/impl/VersionDAOImpl.java	Implements criteria query for versions by `EPerson`.
dspace-api/src/main/java/org/dspace/eperson/service/clarin/EPersonMergeService.java	New merge service contract.
dspace-api/src/main/java/org/dspace/eperson/service/clarin/EPersonMergeAuditService.java	New audit-recording service contract.
dspace-api/src/main/java/org/dspace/eperson/service/clarin/ClarinIdentityService.java	New identity resolution/alias + auto-link service contract.
dspace-api/src/main/java/org/dspace/eperson/factory/EPersonServiceFactoryImpl.java	Exposes new services via factory.
dspace-api/src/main/java/org/dspace/eperson/factory/EPersonServiceFactory.java	Adds factory accessors for new services.
dspace-api/src/main/java/org/dspace/eperson/dao/impl/clarin/EPersonNetidAliasDAOImpl.java	Implements alias lookups (exact + value-prefix).
dspace-api/src/main/java/org/dspace/eperson/dao/impl/clarin/EPersonMergeAuditDAOImpl.java	DAO impl for merge audit entity.
dspace-api/src/main/java/org/dspace/eperson/dao/clarin/EPersonNetidAliasDAO.java	DAO API for alias operations.
dspace-api/src/main/java/org/dspace/eperson/dao/clarin/EPersonMergeAuditDAO.java	DAO API for audit persistence.
dspace-api/src/main/java/org/dspace/eperson/clarin/EPersonNetidAlias.java	New alias entity mapping.
dspace-api/src/main/java/org/dspace/eperson/clarin/EPersonMergeServiceImpl.java	Implements merge workflow (re-pointing, dedupe, tombstone, audit).
dspace-api/src/main/java/org/dspace/eperson/clarin/EPersonMergeAuditServiceImpl.java	Serializes + stores merge audit detail JSON.
dspace-api/src/main/java/org/dspace/eperson/clarin/EPersonMergeAudit.java	New audit entity mapping.
dspace-api/src/main/java/org/dspace/eperson/clarin/ClarinIdentityServiceImpl.java	Implements alias resolution/attach + auto-linking.
dspace-api/src/main/java/org/dspace/content/service/clarin/MatomoReportSubscriptionService.java	Adds reassign API used by merge.
dspace-api/src/main/java/org/dspace/content/clarin/MatomoReportSubscriptionServiceImpl.java	Implements Matomo subscription reassignment with dedupe.
dspace-api/src/main/java/org/dspace/authenticate/clarin/ClarinShibAuthentication.java	Routes netid login via alias table + adds proxy-driven auto-linking.
dspace-api/src/main/java/org/dspace/administer/EPersonMergeConfiguration.java	Script configuration + options for `eperson-merge`.
dspace-api/src/main/java/org/dspace/administer/EPersonMergeCliConfiguration.java	CLI configuration class for `eperson-merge`.
dspace-api/src/main/java/org/dspace/administer/EPersonMergeCli.java	CLI runnable wrapper for merge script.
dspace-api/src/main/java/org/dspace/administer/EPersonMerge.java	Merge CLI script implementation and audit logging.

kosarko · 2026-07-09T07:59:48Z

+    public EPerson autoLink(Context context, List<String> releasedEppns, String proxyAuthority, String proxyNetid)
+            throws SQLException {
+        if (releasedEppns == null || releasedEppns.isEmpty() || StringUtils.isBlank(proxyNetid)) {
+            return null;
+        }


Addressed in 190d5bc: autoLink() now checks isAllowlistedProxy(proxyAuthority) up front and returns null (with a warning log) for non-allowlisted proxies, before any alias lookup. The pre-existing ClarinIdentityServiceTest.autoLinkIgnoresProxiesNotOnTheAllowlist now passes.

kosarko · 2026-07-09T07:59:55Z

+        String originalEmail = from.getEmail();
+        String parkedEmail = StringUtils.isBlank(originalEmail) ? null
+                : "merged-" + from.getID().toString().substring(0, 8) + "+" + originalEmail;
+        if (parkedEmail != null) {


Addressed in 190d5bc: the parked email is now built via buildParkedEmail(), which truncates the local part and appends a short hash of the original address when merged-<8charId>+<email> would exceed the 64-char eperson.email column, with a hard truncation fallback for pathological domains. Covered by mergeTombstonesFromWithLongEmailWithoutExceedingColumnLength.

kosarko · 2026-07-09T07:59:58Z

+    private static String resourcePolicySignature(ResourcePolicy rp) {
+        return rp.getdSpaceObject().getID() + "|" + rp.getAction() + "|" + rp.getRpType();
+    }


Addressed in 190d5bc: resourcePolicySignature() now also includes start/end dates and rpName/rpDescription (matching the field set ResourcePolicy.equals() treats as significant), so e.g. a time-limited policy is moved rather than skipped when the target has an unlimited one for the same object/action/type. Covered by mergeMovesResourcePoliciesThatOnlyDifferByDates; the existing exact-duplicate test still passes.

Copilot

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.

    /**
     * Find an EPerson by a NetID header. The method will go through all the netid headers and try to find a user.
+     * Resolution goes through {@link ClarinIdentityService}'s alias table only - {@code EPerson.netid} is a
+     * denormalized display field that nothing at login reads.
     */


+            identityService.attach(context, ePerson, netid, ClarinIdentityServiceImpl.SOURCE_ADMIN,
+                    context.getCurrentUser());


+        EPersonNetidAlias existing = ePersonNetidAliasDAO.findByNetid(context, netid);
+        if (existing != null) {
+            if (!existing.getEPerson().getID().equals(ePerson.getID())) {
+                throw new IllegalStateException(
+                        "netid '" + netid + "' is already attached to a different EPerson (" +


…oved to the service interface, accurate netid-resolution javadoc

Copilot

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.

kosarko · 2026-07-17T07:55:09Z

+        EPersonNetidAlias existing = ePersonNetidAliasDAO.findByNetid(context, netid);
+        if (existing != null) {
+            if (!existing.getEPerson().getID().equals(ePerson.getID())) {
+                throw new IllegalStateException(
+                        "netid '" + netid + "' is already attached to a different EPerson (" +
+                                existing.getEPerson().getID() + "), refusing to reattach to " + ePerson.getID());
+            }
+            return existing;
+        }
+
+        EPersonNetidAlias alias = new EPersonNetidAlias();
+        alias.setEPerson(ePerson);
+        alias.setNetid(netid);
+        alias.setSource(source);
+        alias.setCreatedBy(createdBy);
+        alias.setCreatedDate(new Date());
+        return ePersonNetidAliasDAO.create(context, alias);


Acknowledged, and deliberately left as check-then-insert. The unique constraint on netid preserves integrity; the worst case under concurrent first logins with the same brand-new netid is that the losing request fails once at flush time and succeeds on retry. Catching the constraint violation in the same session is not a reliable recovery either — after a failed flush the Hibernate session is in an undefined state — so handling it in-place would add complexity without a real guarantee. Documented this decision in a code comment in attach() in 64b523b (which also addresses the other comments from this review).

…id aliases, controller exception type

kosarko

...

kosarko · 2026-07-17T11:46:20Z

+            return null;
+        }
+
+        String proxyNetid = getFirstNetId(netidHeaders);


Why just the first one? Don't we need the full fallback dance? Maybe not, we should be pretty sure of what the proxy releases

Deliberate: the alias must record the same netid registerNewEPerson/updateEPerson would lock the account to — both use getFirstNetId, so tryAutoLink uses it too. Attaching an alias for a netid header the rest of the login flow doesn't use would just create a dangling alias. The proxy is one known source releasing one primary identifier. Added a javadoc sentence spelling this out in 76f7b89.

kosarko · 2026-07-17T13:00:52Z

+        // set directly (e.g. ClarinShibAuthentication#updateEPerson locking a legacy account to
+        // its first-seen netid) never get an alias row of their own, only pre-existing netids
+        // captured by the migration backfill do. Without this fallback such accounts would
+        // silently stop resolving by netid once login moved to alias-only lookup.


Fair — rewritten in plain language in 76f7b89: alias rows exist only for netids that went through attach() or the migration backfill; a netid written straight to eperson.netid after the migration (updateEPerson locking a first login) has no alias row yet, hence the legacy-column fallback. Making the login flow write through attach() is the known follow-up.

kosarko · 2026-07-17T13:09:04Z

+                "SELECT a FROM EPersonNetidAlias a WHERE a.netid LIKE :pattern ESCAPE '\\'");
+        query.setParameter("pattern", escapeLike(valuePrefix) + "[%");


The "[%" was the start of the [authority] suffix that Util.formatNetId appends to every stored netid (value[authority]). Made self-explanatory in 76f7b89: extracted a NETID_AUTHORITY_START constant, renamed the method to findByValueAnyAuthority, and the javadoc now points at Util#formatNetId. The ESCAPE clause is there to neutralize %/_/\\ in the released values themselves.

kosarko · 2026-07-17T13:10:01Z

+            if (StringUtils.isBlank(eppn)) {
+                continue;
+            }
+            for (EPersonNetidAlias alias : ePersonNetidAliasDAO.findByValuePrefix(context, eppn)) {


Why by value prefix and not the whole value?

Because the released voperson_external_id values are bare (novak@cuni.cz) while aliases are stored formatted (novak@cuni.cz[https://cas.cuni.cz/idp/shibboleth]) — an exact match would never hit. The prefix pattern value[% matches that value under any authority without matching longer identities (the [ terminates the value part). Renamed to findByValueAnyAuthority in 76f7b89 so the name says exactly that.

Is there any benefit in keeping the authority value, should the migration drop it?

kosarko · 2026-07-17T13:21:31Z

+    public EPersonNetidAlias findByNetid(Context context, String netid) throws SQLException {
+        Query query = createQuery(context, "SELECT a FROM EPersonNetidAlias a WHERE a.netid = :netid");
+        query.setParameter("netid", netid);
+        query.setHint("org.hibernate.cacheable", Boolean.TRUE);


Maybe. Is it used anywhere else in the project?

Yes — the same org.hibernate.cacheable hint is used in EPersonDAOImpl, GroupDAOImpl, HandleDAOImpl, MetadataSchemaDAOImpl and the clarin DAOs (e.g. ClarinVerificationTokenDAOImpl). Kept for consistency with those hot single-row lookups.

Doesn't it need @Cacheable/@Cache though?

@RequestParam

…etid-matching code - autoLink now throws AmbiguousIdentityException when the released voperson_external_id values match more than one EPerson; tryAutoLink catches it and sets isDuplicateUser so no duplicate account is auto-registered (email matching still works). Covered by unit test + new ClarinShibbolethLoginFilterIT case. - Rename releasedEppns -> upstreamEppns everywhere. - Rename EPersonNetidAliasDAO.findByValuePrefix -> findByValueAnyAuthority, extract NETID_AUTHORITY_START constant, javadoc points at Util#formatNetId. - Rewrite the resolve() legacy-column fallback comment in plain language. - ClarinIdentityLinkController: params as @RequestParam (UUID binding), drop hand-rolled parseUuid/requireParameter, keep explicit blank check. - tryAutoLink javadoc explains why only the first netid header is aliased. - SOURCE_MIGRATION javadoc names the migration file.

kosarko · 2026-07-18T05:45:25Z

How the pieces of this PR fit together — the flow of identity information at login, and everything that writes the eperson_netid_alias table (as of the current head):

flowchart TD
    H["IdP/proxy request headers:<br/>Shib-Identity-Provider, netid headers (eppn, persistent-id, ...),<br/>SHIB-MAIL, voperson_external_id"] --> F["ClarinShibAuthentication.findEPerson"]

    ALIAS[("eperson_netid_alias<br/>netid = value#91;authority#93;")]
    LEGACY[("legacy eperson.netid column")]
    OK(["log in as matched EPerson"])
    FAIL(["login fails (NO_SUCH_USER)"])

    F -->|"1. netid headers, formatted value#91;authority#93;"| R["ClarinIdentityService.resolve"]
    R -->|"lookup"| ALIAS
    ALIAS -->|"alias hit"| OK
    R -->|"no alias row (netids written directly at login are not aliased yet)"| LEGACY
    LEGACY -->|"hit"| OK
    LEGACY -->|"miss"| AL{"1b. proxy on<br/>identity.auto-link.proxy-allowlist<br/>and voperson_external_id released?"}

    AL -->|"no"| EM["2. email header:<br/>ePersonService.findByEmail<br/>+ netid-lock guard"]
    AL -->|"yes"| M["autoLink: match each released value<br/>against aliases value#91;any authority#93;"]
    M -->|"exactly 1 EPerson"| AT["attach proxy netid<br/>source='auto-voperson'"]
    AT -->|"insert"| ALIAS
    AT --> OK
    M -->|"0 matches"| EM
    M -->|"&gt;1 EPersons: AmbiguousIdentityException"| BLOCK["block auto-registration<br/>(isDuplicateUser)"]
    BLOCK --> EM

    EM -->|"hit"| OK
    EM -->|"miss, autoregister on, not blocked"| REG["3. registerNewEPerson<br/>writes eperson.netid directly<br/>(no alias row - known follow-up)"]
    REG --> OK
    EM -->|"miss, registration blocked"| FAIL

    MIG["Flyway migration backfill<br/>(V7.6_2026.07.08, source='migration')"] -->|"seed from pre-existing eperson.netid"| ALIAS
    ADM["admin: POST /api/clarin/identity/link<br/>source='admin'"] -->|"insert"| ALIAS

Notes:

The alias table is the primary lookup; the legacy eperson.netid column is only a fallback for netids that never went through attach() (post-migration first logins lock the netid directly — making login write through attach() is the known follow-up).
An ambiguous voperson_external_id match (several existing accounts) now refuses to guess and blocks auto-registration for that login, so no duplicate account is created; an existing account can still be identified by email. The accounts need an admin merge/link first (the merge tooling is the stacked follow-up PR).

kosarko requested a review from Copilot July 9, 2026 07:17

Copilot started reviewing on behalf of kosarko July 9, 2026 07:18 View session

Copilot AI reviewed Jul 9, 2026

View reviewed changes

Add e-INFRA CZ AAI identity linking (netid aliases + voperson auto-link)

a1b38d0

kosarko force-pushed the identity-linking branch from 80e38e3 to a1b38d0 Compare July 16, 2026 08:09

kosarko changed the title ~~[wip] identity linking and EPerson merge tooling~~ [wip] identity linking Jul 16, 2026

kosarko mentioned this pull request Jul 16, 2026

[wip] EPerson merge tooling #1389

Draft

2 tasks

kosarko requested a review from Copilot July 16, 2026 08:34

Copilot started reviewing on behalf of kosarko July 16, 2026 08:34 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Address Copilot review: 404 for unknown EPerson, SOURCE_* constants m…

6bd3abf

…oved to the service interface, accurate netid-resolution javadoc

kosarko requested a review from Copilot July 16, 2026 12:45

Copilot started reviewing on behalf of kosarko July 16, 2026 12:45 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Address Copilot review: JPA column lengths, ON DELETE CASCADE for net…

64b523b

…id aliases, controller exception type

kosarko commented Jul 17, 2026

View reviewed changes

		identityService.attach(context, ePerson, netid, ClarinIdentityServiceImpl.SOURCE_ADMIN,
		context.getCurrentUser());

		"SELECT a FROM EPersonNetidAlias a WHERE a.netid LIKE :pattern ESCAPE '\\'");
		query.setParameter("pattern", escapeLike(valuePrefix) + "[%");

Uh oh!

Conversation

kosarko commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem description

Analysis

Problems

Manual Testing (if applicable)

Copilot review

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kosarko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kosarko commented Jul 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kosarko commented Jul 9, 2026 •

edited

Loading