From be1f919dee015bb07770a5fa12a84ce6c466c3eb Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Mon, 17 Aug 2026 12:33:08 +0800 Subject: [PATCH 1/3] Track role assignments per authentication origin RoleService.addUserToRole resolved an existing assignment with findByUserAndRole, which ignores origin. A user who already held a role from one origin therefore never received it from another: the lookup returned the existing row and inserted nothing, so LoginService.syncRoles kept re-adding the role on every login because getRolesByOrigin still did not see it. Concurrent logins racing the same check-then-insert also left duplicate rows, and once duplicated the Optional-returning query threw IncorrectResultSizeDataAccessException, which broke role synchronisation and made DELETE /role/{roleId}/users/{userId} return 500. Look assignments up by (user, role, origin), and remove every matching assignment rather than a single arbitrary one. findFirst keeps the lookup tolerant of databases that already contain duplicates; a migration collapses those rows. --- .../webapi/security/authz/RoleService.java | 32 ++--- .../security/authz/UserRoleRepository.java | 7 +- .../V2.99.0010__dedupe_sec_user_role.sql | 13 ++ .../security/authz/UserRoleOriginTest.java | 119 ++++++++++++++++++ 4 files changed, 156 insertions(+), 15 deletions(-) create mode 100644 src/main/resources/db/migration/postgresql/V2.99.0010__dedupe_sec_user_role.sql create mode 100644 src/test/java/org/ohdsi/webapi/security/authz/UserRoleOriginTest.java 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..89c58d309 100644 --- a/src/main/java/org/ohdsi/webapi/security/authz/RoleService.java +++ b/src/main/java/org/ohdsi/webapi/security/authz/RoleService.java @@ -205,12 +205,14 @@ public void addUserToRole(String login, String roleName, UserOrigin userOrigin) 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; @@ -229,24 +231,26 @@ 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()); + } } 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/UserRoleOriginTest.java b/src/test/java/org/ohdsi/webapi/security/authz/UserRoleOriginTest.java new file mode 100644 index 000000000..8a09e9731 --- /dev/null +++ b/src/test/java/org/ohdsi/webapi/security/authz/UserRoleOriginTest.java @@ -0,0 +1,119 @@ +/* + * Copyright 2024 cknoll1. + * + * 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)); + } +} From 28f0b6bff24a9b0ed57385d55af91860d421eb6e Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Mon, 17 Aug 2026 13:11:57 +0800 Subject: [PATCH 2/3] Let concurrent first logins register the same user sec_user.login is unique, and so is the personal role name derived from it, so two first logins for the same principal cannot both register it. Registration ran in the caller's transaction, so the loser marked that transaction rollback-only and its login failed with a constraint violation. Deployments that authenticate per request rather than per session hit this whenever a new user's first page load fans out. Serialise registration on a transaction scoped advisory lock, taken on the connection the transaction already holds, then look the user up again. Whoever waits observes the winner's row once that transaction commits, so the conflicting insert never happens. Registering in a nested REQUIRES_NEW transaction was tried first and rejected: it holds a second pooled connection for every in-flight first login while the caller's transaction still holds the first, so once simultaneous first logins reach the pool size none can obtain the second connection and none can release the first. They then fail together after the connection timeout instead of individually and immediately. Also document that role assignment is tracked per origin, and that removing a user from a role spans every origin. --- .../security/authz/AuthorizationService.java | 25 +++++- .../webapi/security/authz/RoleService.java | 37 ++++++++ .../authz/UserRegistrationRaceTest.java | 87 +++++++++++++++++++ .../security/authz/UserRoleOriginTest.java | 2 +- 4 files changed, 149 insertions(+), 2 deletions(-) create mode 100644 src/test/java/org/ohdsi/webapi/security/authz/UserRegistrationRaceTest.java 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..d856da97a 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,12 @@ public class AuthorizationService { private final EntityAccessService entityAccessService; private final SourceRepository sourceRepository; + // Advisory lock namespace, so user registration cannot collide with other advisory locks. + private static final int USER_REGISTRATION_LOCK_NAMESPACE = 0x55534552; + + @PersistenceContext + private EntityManager entityManager; + public AuthorizationService( AuthorizationCacheService authorizationCacheService, UserService userService, @@ -321,9 +329,24 @@ 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. The lock is + // held on this transaction's own connection and released when the transaction ends, so the + // waiting logins observe the registration once it has been committed. + entityManager.createNativeQuery("SELECT pg_advisory_xact_lock(?1, ?2)") + .setParameter(1, USER_REGISTRATION_LOCK_NAMESPACE) + .setParameter(2, login.hashCode()) + .getSingleResult(); + 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))); } /** 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 89c58d309..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,6 +203,22 @@ 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) { final UserOrigin origin = userOrigin != null ? userOrigin : UserOrigin.SYSTEM; @@ -221,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"); @@ -241,6 +267,17 @@ public void removeUserFromRole(String login, String roleName, UserOrigin origin) } } + /** + * 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); 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..41bd2a85b --- /dev/null +++ b/src/test/java/org/ohdsi/webapi/security/authz/UserRegistrationRaceTest.java @@ -0,0 +1,87 @@ +/* + * 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.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.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; + + private static final String LOGIN = "race_test_user"; + 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 = ?)", LOGIN); + jdbcTemplate.update("DELETE FROM " + ohdsiSchema + ".sec_user WHERE login = ?", LOGIN); + jdbcTemplate.update("DELETE FROM " + ohdsiSchema + ".sec_role WHERE name = ?", LOGIN); + } + + @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)); + } +} diff --git a/src/test/java/org/ohdsi/webapi/security/authz/UserRoleOriginTest.java b/src/test/java/org/ohdsi/webapi/security/authz/UserRoleOriginTest.java index 8a09e9731..77e0da547 100644 --- a/src/test/java/org/ohdsi/webapi/security/authz/UserRoleOriginTest.java +++ b/src/test/java/org/ohdsi/webapi/security/authz/UserRoleOriginTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2024 cknoll1. + * 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. From 7e0a306a7458c1b12b3b832d655738f7fe796765 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Mon, 17 Aug 2026 13:27:51 +0800 Subject: [PATCH 3/3] Stop concurrent logins from duplicating 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. Serialise the logins that have roles to add 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. Locking once per login rather than around each assignment keeps the acquisition order fixed. A first login holds both this lock and the registration one, but onSuccess always registers before it syncs and each lock is taken in a single place, so no two transactions can hold them in opposite orders. --- .../webapi/security/authc/LoginService.java | 23 ++++--- .../security/authz/AuthorizationService.java | 41 ++++++++++--- .../authz/UserRegistrationRaceTest.java | 61 ++++++++++++++++++- 3 files changed, 107 insertions(+), 18 deletions(-) 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 d856da97a..d80f06c1b 100644 --- a/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java +++ b/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java @@ -37,8 +37,10 @@ public class AuthorizationService { private final EntityAccessService entityAccessService; private final SourceRepository sourceRepository; - // Advisory lock namespace, so user registration cannot collide with other advisory locks. + // 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; @@ -335,13 +337,8 @@ public User ensureUserExists(String login, String name, UserOrigin origin, List< } // 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. The lock is - // held on this transaction's own connection and released when the transaction ends, so the - // waiting logins observe the registration once it has been committed. - entityManager.createNativeQuery("SELECT pg_advisory_xact_lock(?1, ?2)") - .setParameter(1, USER_REGISTRATION_LOCK_NAMESPACE) - .setParameter(2, login.hashCode()) - .getSingleResult(); + // 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)) @@ -349,6 +346,34 @@ public User ensureUserExists(String login, String name, UserOrigin origin, List< 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(); + } + /** * Registers a user by creating the suer (and personal role) and assinging any default roles * Will result in an exception of personal role already exists (because that indicates some data issue) diff --git a/src/test/java/org/ohdsi/webapi/security/authz/UserRegistrationRaceTest.java b/src/test/java/org/ohdsi/webapi/security/authz/UserRegistrationRaceTest.java index 41bd2a85b..2a0e71a93 100644 --- a/src/test/java/org/ohdsi/webapi/security/authz/UserRegistrationRaceTest.java +++ b/src/test/java/org/ohdsi/webapi/security/authz/UserRegistrationRaceTest.java @@ -16,6 +16,7 @@ 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; @@ -28,6 +29,8 @@ 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; @@ -43,15 +46,21 @@ 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 = ?)", LOGIN); + + "(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 = ?", LOGIN); + jdbcTemplate.update("DELETE FROM " + ohdsiSchema + ".sec_role WHERE name IN (?, ?)", LOGIN, ROLE_NAME); } @Test @@ -84,4 +93,52 @@ public void testConcurrentFirstLoginsAllSucceed() throws Exception { (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)); + } }