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 6edb245bd..1c622aaa6 100644 --- a/src/main/java/org/ohdsi/webapi/security/authc/LoginService.java +++ b/src/main/java/org/ohdsi/webapi/security/authc/LoginService.java @@ -1,9 +1,11 @@ package org.ohdsi.webapi.security.authc; import java.time.Instant; +import java.util.Arrays; import java.util.Date; import java.util.HashSet; import java.util.List; +import java.util.Locale; import java.util.Set; import java.util.UUID; @@ -37,8 +39,10 @@ public record Result( private final SessionProperties sessionProps; private final AuthorizationService authorizationService; private final List defaultRoles; + private final Set adminLogins; private static final Logger log = LoggerFactory.getLogger(LoginService.class); + private static final String ADMIN_ROLE = "admin"; public final static Result NO_SESSION = new Result(null, null, null, "No session."); public LoginService( @@ -46,12 +50,17 @@ public LoginService( AuthorizationService authorizationService, JwtService jwtService, @Value("${security.defaultRoles}") List defaultRoles, + @Value("${security.admin-login:}") String[] adminLogins, SessionProperties sessionProps) { this.sessionService = sessionService; this.authorizationService = authorizationService; this.jwtService = jwtService; this.sessionProps = sessionProps; this.defaultRoles = defaultRoles.stream().filter(s -> !s.isBlank()).toList(); + this.adminLogins = Arrays.stream(adminLogins) + .filter(login -> login != null && !login.isBlank()) + .map(login -> login.trim().toLowerCase(Locale.ROOT)) + .collect(java.util.stream.Collectors.toUnmodifiableSet()); } /** @@ -68,7 +77,7 @@ public LoginService( */ @Transactional public Result onSuccess(AuthenticatedLogin authenticatedLogin) { - String login = authenticatedLogin.getLogin().toLowerCase(); + String login = authenticatedLogin.getLogin().toLowerCase(Locale.ROOT); String name = authenticatedLogin.getName(); UserOrigin origin = authenticatedLogin.getOrigin(); Set targetRoles = authenticatedLogin.getRoles(); @@ -81,6 +90,13 @@ public Result onSuccess(AuthenticatedLogin authenticatedLogin) { // Sync roles: align database roles with target roles from this authentication source syncRoles(login, origin, targetRoles); + // This is intentionally a one-way grant. Clearing or changing admin-login does not + // revoke an existing assignment; that must be done explicitly by an administrator. + if (adminLogins.contains(login) + && authorizationService.ensureUserHasRole(ADMIN_ROLE, login, UserOrigin.SYSTEM)) { + log.info("SECURITY_AUDIT: admin-login granted role '{}' to user '{}'", ADMIN_ROLE, login); + } + // Create session UUID sessionId = sessionService.createSession(login); Instant expiresAt = Instant.now().plus(sessionProps.getExpiration()); 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 d80f06c1b..7a7db2fd9 100644 --- a/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java +++ b/src/main/java/org/ohdsi/webapi/security/authz/AuthorizationService.java @@ -238,6 +238,27 @@ public void addUserToRole(String roleName, String login, UserOrigin origin) { this.roleService.addUserToRole(login, roleName, origin); } + /** + * Idempotently grants a role from one origin. The per-login advisory lock prevents + * concurrent logins from creating duplicate assignments. + * + * @return {@code true} only when this call created the assignment + */ + @Transactional + public boolean ensureUserHasRole(String roleName, String login, UserOrigin origin) { + if (getRolesByOrigin(login, origin).contains(roleName)) { + return false; + } + + lockRoleSync(login); + if (getRolesByOrigin(login, origin).contains(roleName)) { + return false; + } + + roleService.addUserToRole(login, roleName, origin); + return true; + } + // ------------------------- // Permission & Entity Access Facade // ------------------------- diff --git a/src/main/resources/application.yaml b/src/main/resources/application.yaml index 6b83da8d6..4129e720b 100644 --- a/src/main/resources/application.yaml +++ b/src/main/resources/application.yaml @@ -269,6 +269,10 @@ security: defaultRoles: "" + # Comma-separated logins to grant the built-in admin role at authentication. + # Removing a login does not revoke a role already granted. + admin-login: "" + duration: increment: 10 initial: 10 diff --git a/src/test/java/org/ohdsi/webapi/security/authc/LoginServiceAdminLoginTest.java b/src/test/java/org/ohdsi/webapi/security/authc/LoginServiceAdminLoginTest.java new file mode 100644 index 000000000..b2d2a11a2 --- /dev/null +++ b/src/test/java/org/ohdsi/webapi/security/authc/LoginServiceAdminLoginTest.java @@ -0,0 +1,109 @@ +package org.ohdsi.webapi.security.authc; + +import static org.junit.Assert.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.Collections; +import java.util.Date; +import java.util.List; +import java.util.Set; +import java.util.UUID; +import java.util.Arrays; + +import org.junit.Before; +import org.junit.Test; +import org.ohdsi.webapi.security.authz.AuthorizationService; +import org.ohdsi.webapi.security.session.SessionProperties; +import org.ohdsi.webapi.security.session.SessionService; +import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; + +public class LoginServiceAdminLoginTest { + + @Configuration + static class AdminLoginTestConfiguration { + @Bean + List configuredAdminLogins(@Value("${security.admin-login:}") String[] adminLogins) { + return Arrays.asList(adminLogins); + } + } + + private SessionService sessionService; + private AuthorizationService authorizationService; + private JwtService jwtService; + private SessionProperties sessionProperties; + + @Before + public void setUp() { + sessionService = mock(SessionService.class); + authorizationService = mock(AuthorizationService.class); + jwtService = mock(JwtService.class); + sessionProperties = new SessionProperties(); + + when(authorizationService.getRolesByOrigin(anyString(), any(UserOrigin.class))) + .thenReturn(Collections.emptyList()); + when(authorizationService.getUserRoles(anyString())).thenReturn(Collections.emptyList()); + when(sessionService.createSession(anyString())).thenReturn(UUID.randomUUID()); + when(jwtService.generateToken(anyString(), anyString(), any(Date.class))).thenReturn("jwt"); + } + + @Test + public void matchingLoginReceivesSystemAdminRole() { + LoginService service = createService(new String[]{"other@example.com", " ADMIN@EXAMPLE.COM "}); + when(authorizationService.ensureUserHasRole("admin", "admin@example.com", UserOrigin.SYSTEM)) + .thenReturn(true); + + service.onSuccess(login("Admin@Example.com")); + + verify(authorizationService).ensureUserHasRole("admin", "admin@example.com", UserOrigin.SYSTEM); + } + + @Test + public void commaSeparatedPropertyBindsAllLogins() { + new ApplicationContextRunner() + .withUserConfiguration(AdminLoginTestConfiguration.class) + .withPropertyValues("security.admin-login=first@example.com,second@example.com") + .run(context -> assertEquals(List.of("first@example.com", "second@example.com"), + context.getBean("configuredAdminLogins", List.class))); + } + + @Test + public void nonMatchingLoginDoesNotReceiveAdminRole() { + LoginService service = createService(new String[]{"admin@example.com", "another@example.com"}); + + service.onSuccess(login("other@example.com")); + + verify(authorizationService, never()).ensureUserHasRole(eq("admin"), anyString(), eq(UserOrigin.SYSTEM)); + } + + @Test + public void emptyConfigurationDisablesAdminGrant() { + LoginService service = createService(new String[]{""}); + + service.onSuccess(login("admin@example.com")); + + verify(authorizationService, never()).ensureUserHasRole(eq("admin"), anyString(), eq(UserOrigin.SYSTEM)); + } + + private LoginService createService(String[] adminLogins) { + return new LoginService(sessionService, authorizationService, jwtService, List.of(), adminLogins, + sessionProperties); + } + + private AuthenticatedLogin login(String login) { + return AuthenticatedLogin.builder() + .login(login) + .name(login) + .origin(UserOrigin.OIDC) + .roles(Set.of()) + .build(); + } +} 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 77e0da547..d26e2409f 100644 --- a/src/test/java/org/ohdsi/webapi/security/authz/UserRoleOriginTest.java +++ b/src/test/java/org/ohdsi/webapi/security/authz/UserRoleOriginTest.java @@ -36,6 +36,9 @@ public class UserRoleOriginTest extends AbstractDatabaseTest { @Autowired private UserService userService; + @Autowired + private AuthorizationService authorizationService; + private static final Long USER_ID = 51001L; private static final Long ROLE_ID = 51002L; private static final String LOGIN = "origin_test_user"; @@ -116,4 +119,13 @@ public void testDuplicateRowsDoNotBreakAssignment() { roleService.removeUser(USER_ID, ROLE_ID); assertEquals("Removal should clear duplicates too", 0, countAssignments(null)); } + + @Test + public void testEnsureUserHasRoleIsIdempotent() { + assertEquals("First grant should create an assignment", true, + authorizationService.ensureUserHasRole(ROLE_NAME, LOGIN, UserOrigin.SYSTEM)); + assertEquals("Repeated grant should reuse the assignment", false, + authorizationService.ensureUserHasRole(ROLE_NAME, LOGIN, UserOrigin.SYSTEM)); + assertEquals("Only one assignment should exist", 1, countAssignments("SYSTEM")); + } }