Fix role assignment across authentication origins and under concurrent logins

src/main/java/org/ohdsi/webapi/security/authc/LoginService.java

@@ -117,14 +117,21 @@ private void syncRoles(String login, UserOrigin origin, Set<String> targetRoles)
       return;
     }
 
-    // Add roles present in target but not in current
-    for (String roleName : targetRoles) {
-      if (!currentOriginRoles.contains(roleName)) {
-        try {
-          authorizationService.addUserToRole(roleName, login, origin);
-          log.info("Sync roles: added role '{}' to user '{}' (origin: {})", roleName, login, origin);
-        } catch (Exception e) {
-          log.warn("Sync roles: could not add role '{}' to user '{}': {}", roleName, login, e.getMessage());
+    // Add roles present in target but not in current. Concurrent logins would otherwise both
+    // find a role missing and both add it, so the ones with work to do are serialised and then
+    // re-read what the winner committed.
+    if (!currentOriginRoles.containsAll(targetRoles)) {
+      authorizationService.lockRoleSync(login);
+      currentOriginRoles = authorizationService.getRolesByOrigin(login, origin);
+
+      for (String roleName : targetRoles) {
+        if (!currentOriginRoles.contains(roleName)) {
+          try {
+            authorizationService.addUserToRole(roleName, login, origin);
+            log.info("Sync roles: added role '{}' to user '{}' (origin: {})", roleName, login, origin);
+          } catch (Exception e) {
+            log.warn("Sync roles: could not add role '{}' to user '{}': {}", roleName, login, e.getMessage());
+          }
         }
       }
     }

src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java

@@ -8,6 +8,8 @@
 import org.ohdsi.webapi.security.identity.WebApiPrincipal;
 import org.ohdsi.webapi.source.Source;
 import org.ohdsi.webapi.source.SourceRepository;
+import jakarta.persistence.EntityManager;
+import jakarta.persistence.PersistenceContext;
 import org.springframework.jdbc.core.JdbcTemplate;
 import org.springframework.security.core.context.SecurityContextHolder;
 import org.springframework.stereotype.Service;
@@ -35,6 +37,14 @@ public class AuthorizationService {
   private final EntityAccessService entityAccessService;
   private final SourceRepository sourceRepository;
 
+  // Advisory lock namespaces, so these locks cannot collide with each other or with any
+  // other advisory lock taken against this database.
+  private static final int USER_REGISTRATION_LOCK_NAMESPACE = 0x55534552;
+  private static final int ROLE_SYNC_LOCK_NAMESPACE = 0x524f4c45;
+
+  @PersistenceContext
+  private EntityManager entityManager;
+
   public AuthorizationService(
       AuthorizationCacheService authorizationCacheService,
       UserService userService,
@@ -321,9 +331,47 @@ public void revokeEntityAccess(EntityType entityType, Long entityId, Long roleId
    */
   @Transactional
   public User ensureUserExists(String login, String name, UserOrigin origin, List<String> defaultRoles) {
+    Optional<UserEntity> existing = userService.getUserByLogin(login);
+    if (existing.isPresent()) {
+      return updateIfNeeded(existing.get(), name, origin);
+    }
+
+    // Concurrent first logins for one principal would otherwise race the unique sec_user.login,
+    // and the loser would abort the caller's transaction. Serialise them instead.
+    lockLogin(USER_REGISTRATION_LOCK_NAMESPACE, login);
+
     return userService.getUserByLogin(login)
         .map(entity -> updateIfNeeded(entity, name, origin))
-        .orElseGet(() -> registerUser(login, name, origin, new HashSet<>(defaultRoles == null ? List.of() : defaultRoles)));
+        .orElseGet(() -> registerUser(login, name, origin,
+            new HashSet<>(defaultRoles == null ? List.of() : defaultRoles)));
+  }
+
+  /**
+   * Serialise the callers that are about to grant this login the roles an origin asserts.
+   *
+   * Role assignment is a lookup followed by an insert, so without this two logins can both
+   * find a role missing and both add it. Held only by the logins that actually have
+   * something to add, and released when the transaction ends.
+   *
+   * @param login the login whose role assignments are being changed
+   */
+  @Transactional
+  public void lockRoleSync(String login) {
+    lockLogin(ROLE_SYNC_LOCK_NAMESPACE, login);
+  }
+
+  /**
+   * Take a transaction scoped advisory lock keyed on a login.
+   *
+   * Runs through the EntityManager so that it is taken on the connection this transaction
+   * already holds; a JdbcTemplate would take a second one and lock in a different
+   * transaction. Requires an active transaction, or the lock is released immediately.
+   */
+  private void lockLogin(int namespace, String login) {
+    entityManager.createNativeQuery("SELECT pg_advisory_xact_lock(?1, ?2)")
+        .setParameter(1, namespace)
+        .setParameter(2, login.hashCode())
+        .getSingleResult();
   }
 
   /**

src/main/java/org/ohdsi/webapi/security/authz/RoleService.java

@@ -203,14 +203,32 @@ public void addUserToRole(String login, String roleName, UserOrigin userOrigin)
     this.addUserToRole(user, role, userOrigin);
   }
 
+  /**
+   * Grant a role to a user on behalf of one authentication origin.
+   *
+   * The same role may be held from several origins at once, so an existing grant from
+   * another origin does not satisfy this one. Callers may pass a null origin, which is
+   * recorded as SYSTEM.
+   *
+   * The lookup and the insert are not atomic, so concurrent callers can still create a
+   * duplicate assignment. Duplicates are tolerated rather than prevented; removing that
+   * race needs an upsert and a unique constraint on (user, role, origin).
+   *
+   * @param user the user to grant the role to
+   * @param role the role to grant
+   * @param userOrigin the authentication origin making the grant, null for SYSTEM
+   * @return the existing or newly created assignment
+   */
   public UserRoleEntity addUserToRole(final UserEntity user, final RoleEntity role,
       final UserOrigin userOrigin) {
-    UserRoleEntity relation = this.userRoleRepository.findByUserAndRole(user, role)
+    final UserOrigin origin = userOrigin != null ? userOrigin : UserOrigin.SYSTEM;
+
+    UserRoleEntity relation = this.userRoleRepository.findFirstByUserAndRoleAndOrigin(user, role, origin)
         .orElseGet(() -> {
           UserRoleEntity newRelation = new UserRoleEntity();
           newRelation.setUser(user);
           newRelation.setRole(role);
-          newRelation.setOrigin(userOrigin != null ? userOrigin : UserOrigin.SYSTEM);
+          newRelation.setOrigin(origin);
           newRelation = this.userRoleRepository.save(newRelation);
           authCacheService.evictUser(user.getId());
           return newRelation;
@@ -219,6 +237,16 @@ public UserRoleEntity addUserToRole(final UserEntity user, final RoleEntity role
     return relation;
   }
 
+  /**
+   * Revoke a role from a user, for one authentication origin or for all of them.
+   *
+   * Every assignment matching the origin is removed, so grants recorded more than once
+   * do not survive the call. Grants from other origins are left untouched.
+   *
+   * @param login the user to revoke the role from
+   * @param roleName the role to revoke
+   * @param origin the authentication origin to revoke for, null for every origin
+   */
   public void removeUserFromRole(String login, String roleName, UserOrigin origin) {
     Assert.hasLength(roleName, "roleName can not be empty.");
     Assert.hasLength(login, "login can not be empty");
@@ -229,24 +257,37 @@ public void removeUserFromRole(String login, String roleName, UserOrigin origin)
     RoleEntity role = this.getSystemRoleByName(roleName).orElseThrow(() -> new RuntimeException("Role not found."));
     UserEntity user = userService.getUserByLogin(login).orElseThrow(() -> new RuntimeException("Login not found."));
 
-    this.userRoleRepository.findByUserAndRole(user, role)
-        .ifPresent((userRole) -> {
-          if (origin == null || origin.equals(userRole.getOrigin())) {
-            this.userRoleRepository.delete(userRole);
-            authCacheService.evictUser(user.getId());
-          }
-        });
+    List<UserRoleEntity> assignments = this.userRoleRepository.findAllByUserAndRole(user, role).stream()
+        .filter(userRole -> origin == null || origin.equals(userRole.getOrigin()))
+        .toList();
+
+    if (!assignments.isEmpty()) {
+      this.userRoleRepository.deleteAll(assignments);
+      authCacheService.evictUser(user.getId());
+    }
   }
 
+  /**
+   * Revoke a role from a user across every authentication origin.
+   *
+   * This spans all origins so that the result matches what {@link #getRoleUsers(Long)}
+   * reports, which is not origin-scoped: leaving another origin's grant in place would
+   * keep the user listed in the role after being removed from it. An origin that still
+   * asserts the role re-grants it on the user's next login.
+   *
+   * @param userId the user to revoke the role from
+   * @param roleId the role to revoke
+   */
   public void removeUser(Long userId, Long roleId) {
     UserEntity user = userService.getUserById(userId);
     RoleEntity role = this.getRole(roleId);
 
-    this.userRoleRepository.findByUserAndRole(user, role)
-      .ifPresent((userRole) -> {
-        this.userRoleRepository.delete(userRole);
-        authCacheService.evictUser(user.getId());
-      });
+    List<UserRoleEntity> assignments = this.userRoleRepository.findAllByUserAndRole(user, role);
+
+    if (!assignments.isEmpty()) {
+      this.userRoleRepository.deleteAll(assignments);
+      authCacheService.evictUser(user.getId());
+    }
   }
 
   public Set<RoleEntity> getUserRoles(Long userId) {

src/main/java/org/ohdsi/webapi/security/authz/UserRoleRepository.java

@@ -16,7 +16,12 @@ public interface UserRoleRepository extends CrudRepository<UserRoleEntity, Long>
 
   public List<UserRoleEntity> findByUser(UserEntity user);
 
-  public Optional<UserRoleEntity> findByUserAndRole(UserEntity user, RoleEntity role);
+  // findFirst, not a plain Optional query: databases predating the dedupe migration
+  // can still hold duplicate rows, which would raise IncorrectResultSizeDataAccessException.
+  public Optional<UserRoleEntity> findFirstByUserAndRoleAndOrigin(UserEntity user, RoleEntity role,
+      UserOrigin origin);
+
+  public List<UserRoleEntity> findAllByUserAndRole(UserEntity user, RoleEntity role);
 
 @Query("""
     select ur.user.id

src/main/resources/db/migration/postgresql/V2.99.0010__dedupe_sec_user_role.sql

@@ -0,0 +1,13 @@
+-- Collapse duplicate role assignments left by the pre-origin-aware addUserToRole,
+-- keeping the lowest id of each (user_id, role_id, origin) group.
+
+DELETE FROM ${ohdsiSchema}.sec_user_role
+WHERE id IN (
+    SELECT id
+    FROM (
+        SELECT id,
+               row_number() OVER (PARTITION BY user_id, role_id, origin ORDER BY id) AS rn
+        FROM ${ohdsiSchema}.sec_user_role
+    ) ranked
+    WHERE ranked.rn > 1
+);

src/test/java/org/ohdsi/webapi/security/authz/UserRegistrationRaceTest.java

@@ -0,0 +1,144 @@
+/*
+ * Copyright 2026 p-hoffmann.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.ohdsi.webapi.security.authz;
+
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+import org.junit.After;
+import org.junit.Test;
+import org.ohdsi.webapi.AbstractDatabaseTest;
+import org.ohdsi.webapi.security.authc.AuthenticatedLogin;
+import org.ohdsi.webapi.security.authc.LoginService;
+import org.ohdsi.webapi.security.authc.UserOrigin;
+import org.springframework.beans.factory.annotation.Autowired;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+/**
+ * Concurrent first logins for the same principal must all succeed. sec_user.login is
+ * unique, so only one of them can insert the user and the rest have to fall back to it.
+ */
+public class UserRegistrationRaceTest extends AbstractDatabaseTest {
+
+  @Autowired
+  private AuthorizationService authorizationService;
+
+  @Autowired
+  private LoginService loginService;
+
+  private static final String LOGIN = "race_test_user";
+  private static final String ROLE_NAME = "RaceTestRole";
+  private static final Long ROLE_ID = 51003L;
+  private static final int THREADS = 16;
+
+  @After
+  public void deleteFixture() {
+    jdbcTemplate.update("DELETE FROM " + ohdsiSchema + ".sec_user_role WHERE user_id IN "
+        + "(SELECT id FROM " + ohdsiSchema + ".sec_user WHERE login = ?) OR role_id = ?", LOGIN, ROLE_ID);
+    jdbcTemplate.update("DELETE FROM " + ohdsiSchema + ".sec_session WHERE login = ?", LOGIN);
+    jdbcTemplate.update("DELETE FROM " + ohdsiSchema + ".sec_user WHERE login = ?", LOGIN);
+    jdbcTemplate.update("DELETE FROM " + ohdsiSchema + ".sec_role WHERE name IN (?, ?)", LOGIN, ROLE_NAME);
+  }
+
+  @Test
+  public void testConcurrentFirstLoginsAllSucceed() throws Exception {
+    CyclicBarrier startTogether = new CyclicBarrier(THREADS);
+    ExecutorService pool = Executors.newFixedThreadPool(THREADS);
+
+    try {
+      List<Callable<Object>> logins = IntStream.range(0, THREADS)
+          .<Callable<Object>>mapToObj(i -> () -> {
+            startTogether.await(30, TimeUnit.SECONDS);
+            return authorizationService.ensureUserExists(LOGIN, LOGIN, UserOrigin.OIDC, List.of());
+          })
+          .collect(Collectors.toList());
+
+      List<Future<Object>> results = pool.invokeAll(logins, 60, TimeUnit.SECONDS);
+
+      for (Future<Object> result : results) {
+        try {
+          result.get();
+        } catch (Exception e) {
+          fail("Concurrent first login failed: " + e.getCause());
+        }
+      }
+    } finally {
+      pool.shutdownNow();
+    }
+
+    assertEquals("Exactly one user should have been registered", 1,
+        (int) jdbcTemplate.queryForObject(
+            "SELECT count(*) FROM " + ohdsiSchema + ".sec_user WHERE login = ?", Integer.class, LOGIN));
+  }
+
+  @Test
+  public void testConcurrentLoginsDoNotDuplicateRoleAssignments() throws Exception {
+    jdbcTemplate.update("INSERT INTO " + ohdsiSchema + ".sec_role (id, name, system_role) VALUES (?, ?, true)",
+        ROLE_ID, ROLE_NAME);
+
+    // Register first, so the concurrent logins below race role assignment rather than
+    // queueing on the registration lock.
+    loginService.onSuccess(AuthenticatedLogin.builder()
+        .login(LOGIN).name(LOGIN).origin(UserOrigin.OIDC).roles(Set.of()).originAuthentication(null).build());
+
+    AuthenticatedLogin authenticated = AuthenticatedLogin.builder()
+        .login(LOGIN)
+        .name(LOGIN)
+        .origin(UserOrigin.OIDC)
+        .roles(Set.of(ROLE_NAME))
+        .originAuthentication(null)
+        .build();
+
+    CyclicBarrier startTogether = new CyclicBarrier(THREADS);
+    ExecutorService pool = Executors.newFixedThreadPool(THREADS);
+
+    try {
+      List<Callable<Object>> logins = IntStream.range(0, THREADS)
+          .<Callable<Object>>mapToObj(i -> () -> {
+            startTogether.await(30, TimeUnit.SECONDS);
+            return loginService.onSuccess(authenticated);
+          })
+          .collect(Collectors.toList());
+
+      for (Future<Object> result : pool.invokeAll(logins, 60, TimeUnit.SECONDS)) {
+        try {
+          result.get();
+        } catch (Exception e) {
+          fail("Concurrent login failed: " + e.getCause());
+        }
+      }
+    } finally {
+      pool.shutdownNow();
+    }
+
+    assertEquals("The role should be assigned exactly once", 1,
+        (int) jdbcTemplate.queryForObject(
+            "SELECT count(*) FROM " + ohdsiSchema + ".sec_user_role ur "
+                + "JOIN " + ohdsiSchema + ".sec_user u ON u.id = ur.user_id "
+                + "JOIN " + ohdsiSchema + ".sec_role r ON r.id = ur.role_id "
+                + "WHERE u.login = ? AND r.name = ?", Integer.class, LOGIN, ROLE_NAME));
+  }
+}