Fix role assignment across authentication origins and under concurrent logins

[p-hoffmann]

Three defects in the login path, surfacing as roles silently failing to apply, as duplicated role assignments, or as a failed login.

1. Role assignments ignored their origin

sec_user_role.origin records which authentication source granted an assignment, and LoginService.syncRoles uses it to reconcile only the roles asserted by the origin the user just logged in from. RoleService.addUserToRole did not honour that: it resolved an existing assignment with findByUserAndRole, which ignores origin.

Effects:

1. A user who already held a role from one origin never received it from another. The lookup returned the existing row and inserted nothing, so the grant silently never took effect — while the caller still logged it as added. getRolesByOrigin continued not to see it, so it was retried on every subsequent login.
2. Concurrent logins racing the same check-then-insert left duplicate rows; there is no unique constraint on the table.
3. Once duplicated, the Optional-returning query threw IncorrectResultSizeDataAccessException. That broke role synchronisation for the affected role permanently, and made DELETE /role/{roleId}/users/{userId} return 500, so the duplicates could not be cleared through the API either.

Changes:

• addUserToRole resolves the assignment by (user, role, origin).
• removeUserFromRole and removeUser delete every matching assignment instead of one arbitrary row. removeUser (the admin endpoint) intentionally spans all origins, so that it stays consistent with getRoleUsers, which is not origin-scoped; anything the identity provider still asserts is re-added on the next login.
• The repository query uses findFirst so databases already holding duplicates do not throw.
• V2.99.0010__dedupe_sec_user_role.sql collapses existing duplicates, keeping the lowest id per (user_id, role_id, origin).

2. Concurrent first logins could not both register the user

ensureUserExists registered the user inside the caller's transaction. sec_user.login is unique, and so is the personal role name derived from it, so when two first logins for the same principal ran at once the loser marked the caller's transaction rollback-only and the login failed outright with duplicate key value violates unique constraint "sec_user_login_unique".

Registration is now serialised on a transaction scoped advisory lock, taken on the connection the transaction already holds, after which the user is looked up again. Whoever waits observes the winner's row once that transaction commits, so the conflicting insert never happens.

Serialising was chosen over registering in a nested REQUIRES_NEW transaction. That alternative works for a single login, but it holds a second pooled connection for the duration of every in-flight first login while the caller's transaction still holds the first. Once simultaneous first logins reach the pool size (10 by default), none of them can obtain the second connection and none can release the first, so they all fail together after spring.datasource.hikari.connection-timeout rather than immediately. UserRegistrationRaceTest runs more threads than the default pool holds and reproduces exactly that.

This uses pg_advisory_xact_lock, which is Postgres specific. This branch ships Postgres migrations only, but say the word if a portable form is preferred.

Deployments that authenticate on every request rather than once per session hit the underlying race reliably.

3. Concurrent logins duplicated role assignments

Granting a role is a lookup followed by an insert and sec_user_role has no unique constraint, so two logins that both found a role missing both added it. A deployment that authenticates per request accumulated a row per concurrent request; the test below produces ten.

The logins that have roles to add now serialise on a transaction scoped advisory lock and re-read the assignments once it is held. Logins with nothing to add, which is all of them after the first, take no lock.

The lock is taken once per login rather than once per assignment deliberately. Locking per assignment, or adding a unique constraint and letting the insert conflict, would make a transaction hold several of these locks at once, and syncRoles iterates an unordered set — so two logins could take the same locks in opposite orders and deadlock.

A first login does hold two of these locks, the registration one and this one, but onSuccess always registers before it syncs and each lock is taken in exactly one place, so the acquisition order is the same in every transaction and no cycle is possible.

A unique constraint on (user_id, role_id, origin) would be a reasonable follow-up now that duplicates can no longer be created, but it is not needed to close this and is left out to keep the migration additive.

Two related things are deliberately left alone. addUserToRole remains unsynchronised for its non-login callers, the admin endpoint and the bulk user import; those grant at SYSTEM origin while login sync grants at the origin that authenticated, so they write different rows rather than duplicates. And ExternalRoleMapService.syncUserRoles repeats the pattern this fixes but has no callers at all — worth deleting or routing through the same lock before anything starts using it.

Testing

UserRoleOriginTest covers a grant from a second origin, per-origin removal, and tolerance of pre-existing duplicates. UserRegistrationRaceTest releases sixteen concurrent logins for the same principal from a barrier — more than the default connection pool holds — and asserts that all succeed, that exactly one user is registered, and that a role asserted by all of them is assigned exactly once.

Each fails on the unmodified branch with the corresponding production symptom and passes with this change.

ExternalRoleMappingTest and ExternalRoleMappingCsvTest fail identically with and without this change on webapi-3.0.

[chrisknoll]

Only comment i have on this is the handling of the dupes:

• The repository query uses findFirst so databases already holding duplicates do not throw.
• V2.99.0010__dedupe_sec_user_role.sql collapses existing duplicates, keeping the lowest id per (user_id, role_id, origin).

Since the migration script de-dupes, we can be assured that we won't have that duplication, so we could change the code to not handle this (making it appear that it may be a valid data arrangement). It is a valid data arrangement, however, to have a user have the same roles assigned more than once (from different origins) so want to be clear about if findFirst would still be needed in that case in the event of a double-role-assignment. But if it's just to account for a dupe of user-role-origin, then we probably won't need it after we de-dupe and apply a unique consraint.