Skip to content
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import org.dspace.eperson.factory.EPersonServiceFactory;
import org.dspace.eperson.service.EPersonService;
import org.dspace.eperson.service.GroupService;
import org.dspace.eperson.service.clarin.ClarinIdentityService;
import org.dspace.services.ConfigurationService;
import org.dspace.services.factory.DSpaceServicesFactory;

Expand Down Expand Up @@ -124,6 +125,7 @@ public class ClarinShibAuthentication implements AuthenticationMethod {
ClarinServiceFactory.getInstance().getClarinUserRegistration();
protected ClarinVerificationTokenService clarinVerificationTokenService = ClarinServiceFactory.getInstance()
.getClarinVerificationTokenService();
protected ClarinIdentityService identityService = EPersonServiceFactory.getInstance().getClarinIdentityService();

/**
* Authenticate the given or implicit credentials. This is the heart of the
Expand Down Expand Up @@ -558,7 +560,18 @@ protected EPerson findEPerson(Context context, HttpServletRequest request, Strin

// 1) First, look for a netid header.
if (netidHeaders != null) {
eperson = findEpersonByNetId(netidHeaders, shibheaders, ePersonService, context, true);
eperson = findEpersonByNetId(netidHeaders, shibheaders, identityService, context, true);
if (eperson != null) {
foundNetID = true;
}
}

// 1b) CLARIN: an allowlisted identity proxy (e.g. e-INFRA CZ/Perun) may release
// voperson_external_id, listing this user's other registered external identities.
// If exactly one existing EPerson already holds one of them, auto-link this login
// to it instead of falling through to email matching / auto-registration.
if (eperson == null && netidHeaders != null) {
eperson = tryAutoLink(context, netidHeaders);
if (eperson != null) {
foundNetID = true;
}
Expand Down Expand Up @@ -643,6 +656,37 @@ protected EPerson findEPerson(Context context, HttpServletRequest request, Strin
return eperson;
}

/**
* Try to auto-link this login to an existing EPerson via the allowlisted proxy's
* {@code voperson_external_id} attribute (Perun's "merging by all registered external
* identities"). See {@link ClarinIdentityService#autoLink}.
*
* @return the EPerson to log into, or null if auto-linking does not apply/match.
*/
protected EPerson tryAutoLink(Context context, String[] netidHeaders) throws SQLException {
String idp = shibheaders.get_idp();
if (!identityService.isAllowlistedProxy(idp)) {
return null;
}

String vopersonExternalIdHeader = configurationService
.getProperty("authentication-shibboleth.voperson-external-id-header");
if (vopersonExternalIdHeader == null) {
return null;
}
List<String> releasedEppns = shibheaders.get(vopersonExternalIdHeader);
Comment thread
kosarko marked this conversation as resolved.
Outdated
if (releasedEppns == null || releasedEppns.isEmpty()) {
return null;
}

String proxyNetid = getFirstNetId(netidHeaders);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (proxyNetid == null) {
return null;
}

return identityService.autoLink(context, releasedEppns, idp, proxyNetid);
}

/**
* Register a new eperson object. This method is called when no existing user was
* found for the NetID or Email and autoregister is enabled. When these conditions
Expand Down Expand Up @@ -1310,9 +1354,13 @@ public String getEmailAcceptedOrNull(String email) {

/**
* 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#resolve}, which checks the {@code eperson_netid_alias}
* table first and falls back to the legacy {@code EPerson.netid} column for accounts that have no alias row
* (netids written directly at login are not backfilled into the alias table).
*/
Comment on lines 1370 to 1375
public static EPerson findEpersonByNetId(String[] netidHeaders, ShibHeaders shibheaders,
EPersonService ePersonService, Context context, boolean logAllowed)
ClarinIdentityService identityService, Context context,
boolean logAllowed)
throws SQLException {
// Go through all the netid headers and try to find a user. It could be e.g., `eppn`, `persistent-id`,..
for (String netidHeader : netidHeaders) {
Expand All @@ -1322,7 +1370,7 @@ public static EPerson findEpersonByNetId(String[] netidHeaders, ShibHeaders shib
continue;
}

EPerson eperson = ePersonService.findByNetid(context, netid);
EPerson eperson = identityService.resolve(context, netid);

if (eperson == null && logAllowed) {
log.info(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
/**
* The contents of this file are subject to the license and copyright
* detailed in the LICENSE and NOTICE files at the root of the source
* tree and available online at
*
* http://www.dspace.org/license/
*/
package org.dspace.eperson.clarin;

import java.sql.SQLException;
import java.util.ArrayList;
import java.util.Date;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;

import org.apache.commons.lang3.ArrayUtils;
import org.apache.commons.lang3.StringUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.dspace.core.Context;
import org.dspace.eperson.EPerson;
import org.dspace.eperson.dao.clarin.EPersonNetidAliasDAO;
import org.dspace.eperson.service.EPersonService;
import org.dspace.eperson.service.clarin.ClarinIdentityService;
import org.dspace.services.ConfigurationService;
import org.springframework.beans.factory.annotation.Autowired;

/**
* @see ClarinIdentityService
*
* @author Ondrej Kosarko
*/
public class ClarinIdentityServiceImpl implements ClarinIdentityService {

private static final Logger log = LogManager.getLogger(ClarinIdentityServiceImpl.class);

@Autowired
private EPersonNetidAliasDAO ePersonNetidAliasDAO;

@Autowired
private ConfigurationService configurationService;

@Autowired
private EPersonService ePersonService;

@Override
public EPerson resolve(Context context, String netid) throws SQLException {
if (StringUtils.isBlank(netid)) {
return null;
}
EPersonNetidAlias alias = ePersonNetidAliasDAO.findByNetid(context, netid);
if (alias != null) {
return alias.getEPerson();
}
// Fall back to the legacy denormalized eperson.netid column: EPersons whose netid was
// 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return ePersonService.findByNetid(context, netid);
}

@Override
public EPersonNetidAlias attach(Context context, EPerson ePerson, String netid, String source, EPerson createdBy)
throws SQLException {
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 (" +
Comment on lines +69 to +73
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);
Comment on lines +69 to +85

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}

@Override
public List<EPersonNetidAlias> findAliases(Context context, EPerson ePerson) throws SQLException {
return ePersonNetidAliasDAO.findByEPerson(context, ePerson);
}

@Override
public void detach(Context context, EPersonNetidAlias alias) throws SQLException {
ePersonNetidAliasDAO.delete(context, alias);
}

@Override
public boolean isAllowlistedProxy(String authority) {
if (StringUtils.isBlank(authority)) {
return false;
}
String[] allowlist = configurationService.getArrayProperty("identity.auto-link.proxy-allowlist");
return ArrayUtils.contains(allowlist, authority);
}

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!isAllowlistedProxy(proxyAuthority)) {
log.warn("Auto-link via proxy '{}' refused: proxy is not on the identity.auto-link.proxy-allowlist.",
proxyAuthority);
return null;
}

// Already linked (e.g. concurrent/repeat login racing this method) - nothing to do.
EPerson alreadyLinked = resolve(context, proxyNetid);
if (alreadyLinked != null) {
return alreadyLinked;
}

Set<EPerson> matches = new LinkedHashSet<>();
List<String> matchedOn = new ArrayList<>();
for (String eppn : releasedEppns) {
if (StringUtils.isBlank(eppn)) {
continue;
}
for (EPersonNetidAlias alias : ePersonNetidAliasDAO.findByValuePrefix(context, eppn)) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why by value prefix and not the whole value?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (matches.add(alias.getEPerson())) {
matchedOn.add(eppn);
}
}
}

if (matches.isEmpty()) {
return null;
}
if (matches.size() > 1) {
log.warn("Auto-link via proxy '{}' matched {} different EPersons for released identities {} - " +
"refusing to guess, flag for admin merge (proxy netid '{}').",
proxyAuthority, matches.size(), matchedOn, proxyNetid);
return null;
Comment thread
kosarko marked this conversation as resolved.
Outdated
}

EPerson matched = matches.iterator().next();
Comment thread
kosarko marked this conversation as resolved.
attach(context, matched, proxyNetid, SOURCE_AUTO_VOPERSON, null);
log.info("Auto-linked netid '{}' to EPerson {} via proxy '{}' voperson_external_id match.",
proxyNetid, matched.getID(), proxyAuthority);
return matched;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
/**
* The contents of this file are subject to the license and copyright
* detailed in the LICENSE and NOTICE files at the root of the source
* tree and available online at
*
* http://www.dspace.org/license/
*/
package org.dspace.eperson.clarin;

import java.util.Date;
import javax.persistence.Column;
import javax.persistence.Entity;
import javax.persistence.FetchType;
import javax.persistence.GeneratedValue;
import javax.persistence.GenerationType;
import javax.persistence.Id;
import javax.persistence.JoinColumn;
import javax.persistence.ManyToOne;
import javax.persistence.SequenceGenerator;
import javax.persistence.Table;
import javax.persistence.Temporal;
import javax.persistence.TemporalType;

import org.dspace.core.ReloadableEntity;
import org.dspace.eperson.EPerson;

/**
* An alias binding one identity value (a formatted "value[authority]" netid,
* where authority is a SAML IdP entityID or OIDC issuer URL) to an EPerson.
*
* This table is the primary source netid-based login resolution consults;
* {@link EPerson#getNetid()} is otherwise a denormalized display field, but
* is still consulted as a fallback by
* {@link org.dspace.eperson.service.clarin.ClarinIdentityService#resolve}
* for EPersons that do not (yet) have an alias row of their own.
*
* @author Ondrej Kosarko
*/
@Entity
@Table(name = "eperson_netid_alias")
public class EPersonNetidAlias implements ReloadableEntity<Integer> {

@Id
@Column(name = "eperson_netid_alias_id")
@GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "eperson_netid_alias_id_seq")
@SequenceGenerator(name = "eperson_netid_alias_id_seq", sequenceName = "eperson_netid_alias_id_seq",
allocationSize = 1)
private Integer id;

@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "eperson_id", nullable = false)
private EPerson ePerson;

/**
* Formatted "value[authority]" identity, e.g. "novak@cuni.cz[https://cas.cuni.cz/idp/shibboleth]".
*/
@Column(name = "netid", nullable = false, unique = true)
private String netid;
Comment thread
Copilot marked this conversation as resolved.
Outdated

/**
* How this alias was created: 'migration' | 'auto-voperson' | 'admin' | 'merge'.
*/
@Column(name = "source", nullable = false)
private String source;
Comment thread
Copilot marked this conversation as resolved.
Outdated

@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "created_by")
private EPerson createdBy;

@Column(name = "created_date", nullable = false)
@Temporal(TemporalType.TIMESTAMP)
private Date createdDate;

protected EPersonNetidAlias() {
}

@Override
public Integer getID() {
return id;
}

public EPerson getEPerson() {
return ePerson;
}

public void setEPerson(EPerson ePerson) {
this.ePerson = ePerson;
}

public String getNetid() {
return netid;
}

public void setNetid(String netid) {
this.netid = netid;
}

public String getSource() {
return source;
}

public void setSource(String source) {
this.source = source;
}

public EPerson getCreatedBy() {
return createdBy;
}

public void setCreatedBy(EPerson createdBy) {
this.createdBy = createdBy;
}

public Date getCreatedDate() {
return createdDate;
}

public void setCreatedDate(Date createdDate) {
this.createdDate = createdDate;
}
}
Loading
Loading