diff --git a/src/main/java/org/ohdsi/webapi/security/authc/LoginService.java b/src/main/java/org/ohdsi/webapi/security/authc/LoginService.java index 121f365e8..6edb245bd 100644 --- a/src/main/java/org/ohdsi/webapi/security/authc/LoginService.java +++ b/src/main/java/org/ohdsi/webapi/security/authc/LoginService.java @@ -117,14 +117,21 @@ private void syncRoles(String login, UserOrigin origin, Set 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()); + } } } } diff --git a/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java b/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java index 84945684a..d80f06c1b 100644 --- a/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java +++ b/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 defaultRoles) { + Optional 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(); } /** diff --git a/src/main/java/org/ohdsi/webapi/security/authz/RoleService.java b/src/main/java/org/ohdsi/webapi/security/authz/RoleService.java index 2d33a6bb1..42484e130 100644 --- a/src/main/java/org/ohdsi/webapi/security/authz/RoleService.java +++ b/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 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 assignments = this.userRoleRepository.findAllByUserAndRole(user, role); + + if (!assignments.isEmpty()) { + this.userRoleRepository.deleteAll(assignments); + authCacheService.evictUser(user.getId()); + } } public Set getUserRoles(Long userId) { diff --git a/src/main/java/org/ohdsi/webapi/security/authz/UserRoleRepository.java b/src/main/java/org/ohdsi/webapi/security/authz/UserRoleRepository.java index b39787271..aeaabf08c 100644 --- a/src/main/java/org/ohdsi/webapi/security/authz/UserRoleRepository.java +++ b/src/main/java/org/ohdsi/webapi/security/authz/UserRoleRepository.java @@ -16,7 +16,12 @@ public interface UserRoleRepository extends CrudRepository public List findByUser(UserEntity user); - public Optional 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 findFirstByUserAndRoleAndOrigin(UserEntity user, RoleEntity role, + UserOrigin origin); + + public List findAllByUserAndRole(UserEntity user, RoleEntity role); @Query(""" select ur.user.id diff --git a/src/main/resources/db/migration/postgresql/V2.99.0010__dedupe_sec_user_role.sql b/src/main/resources/db/migration/postgresql/V2.99.0010__dedupe_sec_user_role.sql new file mode 100644 index 000000000..66b3d1207 --- /dev/null +++ b/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 +); diff --git a/src/test/java/org/ohdsi/webapi/security/authz/UserRegistrationRaceTest.java b/src/test/java/org/ohdsi/webapi/security/authz/UserRegistrationRaceTest.java new file mode 100644 index 000000000..2a0e71a93 --- /dev/null +++ b/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> logins = IntStream.range(0, THREADS) + .>mapToObj(i -> () -> { + startTogether.await(30, TimeUnit.SECONDS); + return authorizationService.ensureUserExists(LOGIN, LOGIN, UserOrigin.OIDC, List.of()); + }) + .collect(Collectors.toList()); + + List> results = pool.invokeAll(logins, 60, TimeUnit.SECONDS); + + for (Future 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> logins = IntStream.range(0, THREADS) + .>mapToObj(i -> () -> { + startTogether.await(30, TimeUnit.SECONDS); + return loginService.onSuccess(authenticated); + }) + .collect(Collectors.toList()); + + for (Future 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)); + } +} diff --git a/src/test/java/org/ohdsi/webapi/security/authz/UserRoleOriginTest.java b/src/test/java/org/ohdsi/webapi/security/authz/UserRoleOriginTest.java new file mode 100644 index 000000000..77e0da547 --- /dev/null +++ b/src/test/java/org/ohdsi/webapi/security/authz/UserRoleOriginTest.java @@ -0,0 +1,119 @@ +/* + * 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 org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.ohdsi.webapi.AbstractDatabaseTest; +import org.ohdsi.webapi.security.authc.UserOrigin; +import org.springframework.beans.factory.annotation.Autowired; + +import static org.junit.Assert.assertEquals; + +/** + * Verifies that a role assignment is tracked per authentication origin, so a grant from + * one origin neither blocks nor is blocked by the same role granted from another. + */ +public class UserRoleOriginTest extends AbstractDatabaseTest { + + @Autowired + private RoleService roleService; + + @Autowired + private UserService userService; + + private static final Long USER_ID = 51001L; + private static final Long ROLE_ID = 51002L; + private static final String LOGIN = "origin_test_user"; + private static final String ROLE_NAME = "OriginTestRole"; + + @Before + public void insertFixture() { + deleteFixture(); + jdbcTemplate.update("INSERT INTO " + ohdsiSchema + ".sec_user (id, login, name, origin) VALUES (?, ?, ?, 'SYSTEM')", + USER_ID, LOGIN, LOGIN); + jdbcTemplate.update("INSERT INTO " + ohdsiSchema + ".sec_role (id, name, system_role) VALUES (?, ?, true)", + ROLE_ID, ROLE_NAME); + } + + @After + public void deleteFixture() { + jdbcTemplate.update("DELETE FROM " + ohdsiSchema + ".sec_user_role WHERE user_id = ? OR role_id = ?", + USER_ID, ROLE_ID); + jdbcTemplate.update("DELETE FROM " + ohdsiSchema + ".sec_user WHERE id = ?", USER_ID); + jdbcTemplate.update("DELETE FROM " + ohdsiSchema + ".sec_role WHERE id = ?", ROLE_ID); + } + + private int countAssignments(String origin) { + String sql = "SELECT count(*) FROM " + ohdsiSchema + ".sec_user_role WHERE user_id = ? AND role_id = ?" + + (origin == null ? "" : " AND origin = '" + origin + "'"); + return jdbcTemplate.queryForObject(sql, Integer.class, USER_ID, ROLE_ID); + } + + private void insertAssignment(String origin) { + jdbcTemplate.update("INSERT INTO " + ohdsiSchema + ".sec_user_role (id, user_id, role_id, origin) " + + "VALUES (nextval('" + ohdsiSchema + ".sec_user_role_sequence'), ?, ?, ?)", USER_ID, ROLE_ID, origin); + } + + @Test + public void testGrantFromSecondOriginIsNotShadowed() { + UserEntity user = userService.getUserById(USER_ID); + RoleEntity role = roleService.getRole(ROLE_ID); + + roleService.addUserToRole(user, role, UserOrigin.SYSTEM); + roleService.addUserToRole(user, role, UserOrigin.OIDC); + + assertEquals("SYSTEM grant should be recorded", 1, countAssignments("SYSTEM")); + assertEquals("OIDC grant must not be shadowed by the existing SYSTEM grant", 1, countAssignments("OIDC")); + + roleService.addUserToRole(user, role, UserOrigin.OIDC); + assertEquals("Re-granting the same origin should not duplicate", 1, countAssignments("OIDC")); + } + + @Test + public void testRemoveByOriginLeavesOtherOriginsIntact() { + UserEntity user = userService.getUserById(USER_ID); + RoleEntity role = roleService.getRole(ROLE_ID); + + roleService.addUserToRole(user, role, UserOrigin.SYSTEM); + roleService.addUserToRole(user, role, UserOrigin.OIDC); + + roleService.removeUserFromRole(LOGIN, ROLE_NAME, UserOrigin.OIDC); + + assertEquals("OIDC grant should be removed", 0, countAssignments("OIDC")); + assertEquals("SYSTEM grant should survive", 1, countAssignments("SYSTEM")); + + roleService.removeUser(USER_ID, ROLE_ID); + assertEquals("Removing the user from the role should clear every origin", 0, countAssignments(null)); + } + + @Test + public void testDuplicateRowsDoNotBreakAssignment() { + UserEntity user = userService.getUserById(USER_ID); + RoleEntity role = roleService.getRole(ROLE_ID); + + // Duplicates predating the dedupe migration must not make the lookup throw. + insertAssignment("SYSTEM"); + insertAssignment("SYSTEM"); + + roleService.addUserToRole(user, role, UserOrigin.SYSTEM); + assertEquals("Existing duplicates should be left alone, not added to", 2, countAssignments("SYSTEM")); + + roleService.removeUser(USER_ID, ROLE_ID); + assertEquals("Removal should clear duplicates too", 0, countAssignments(null)); + } +}