From 972790c949b13a3f4cfffbcd65f6cc5837655777 Mon Sep 17 00:00:00 2001 From: Emmanuel Durin Date: Thu, 23 Nov 2023 10:11:03 +0100 Subject: [PATCH 1/3] ugly fix just to see if it works --- .../admin/CreateAccountUserCustomizer.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/gateway/src/main/java/org/georchestra/gateway/accounts/admin/CreateAccountUserCustomizer.java b/gateway/src/main/java/org/georchestra/gateway/accounts/admin/CreateAccountUserCustomizer.java index 04d8bd5a..162af17f 100644 --- a/gateway/src/main/java/org/georchestra/gateway/accounts/admin/CreateAccountUserCustomizer.java +++ b/gateway/src/main/java/org/georchestra/gateway/accounts/admin/CreateAccountUserCustomizer.java @@ -18,13 +18,20 @@ */ package org.georchestra.gateway.accounts.admin; +import java.lang.reflect.Field; +import java.util.Collections; import java.util.Objects; +import java.util.stream.Collectors; import org.georchestra.gateway.security.GeorchestraUserCustomizerExtension; import org.georchestra.security.model.GeorchestraUser; import org.springframework.core.Ordered; +import org.springframework.security.authentication.AbstractAuthenticationToken; import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.SimpleGrantedAuthority; import org.springframework.security.oauth2.client.authentication.OAuth2AuthenticationToken; +import org.springframework.security.oauth2.client.registration.ClientRegistration; import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken; import lombok.NonNull; @@ -62,6 +69,18 @@ public class CreateAccountUserCustomizer implements GeorchestraUserCustomizerExt final boolean isPreAuth = auth instanceof PreAuthenticatedAuthenticationToken; if (isOauth2) { Objects.requireNonNull(mappedUser.getOAuth2ProviderId(), "GeorchestraUser.oAuth2ProviderId is null"); + GeorchestraUser user = accounts.getOrCreate(mappedUser); + + try { +// ((OAuth2AuthenticationToken) auth).setDetails(); + Field field = AbstractAuthenticationToken.class.getDeclaredField("authorities"); + field.setAccessible(true); + field.set(auth, + user.getRoles().stream().map(r -> new SimpleGrantedAuthority(r)).collect(Collectors.toList())); + } catch (NoSuchFieldException e) { + } catch (IllegalAccessException e) { + } + } if (isPreAuth) { Objects.requireNonNull(mappedUser.getUsername(), "GeorchestraUser.username is null"); From d07caea7417a37e42500d2b464be057dd42afe24 Mon Sep 17 00:00:00 2001 From: Emmanuel Durin Date: Tue, 12 Dec 2023 20:57:49 +0100 Subject: [PATCH 2/3] Revert "ugly fix just to see if it works" This reverts commit 972790c949b13a3f4cfffbcd65f6cc5837655777. --- .../admin/CreateAccountUserCustomizer.java | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/gateway/src/main/java/org/georchestra/gateway/accounts/admin/CreateAccountUserCustomizer.java b/gateway/src/main/java/org/georchestra/gateway/accounts/admin/CreateAccountUserCustomizer.java index 162af17f..04d8bd5a 100644 --- a/gateway/src/main/java/org/georchestra/gateway/accounts/admin/CreateAccountUserCustomizer.java +++ b/gateway/src/main/java/org/georchestra/gateway/accounts/admin/CreateAccountUserCustomizer.java @@ -18,20 +18,13 @@ */ package org.georchestra.gateway.accounts.admin; -import java.lang.reflect.Field; -import java.util.Collections; import java.util.Objects; -import java.util.stream.Collectors; import org.georchestra.gateway.security.GeorchestraUserCustomizerExtension; import org.georchestra.security.model.GeorchestraUser; import org.springframework.core.Ordered; -import org.springframework.security.authentication.AbstractAuthenticationToken; import org.springframework.security.core.Authentication; -import org.springframework.security.core.GrantedAuthority; -import org.springframework.security.core.authority.SimpleGrantedAuthority; import org.springframework.security.oauth2.client.authentication.OAuth2AuthenticationToken; -import org.springframework.security.oauth2.client.registration.ClientRegistration; import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken; import lombok.NonNull; @@ -69,18 +62,6 @@ public class CreateAccountUserCustomizer implements GeorchestraUserCustomizerExt final boolean isPreAuth = auth instanceof PreAuthenticatedAuthenticationToken; if (isOauth2) { Objects.requireNonNull(mappedUser.getOAuth2ProviderId(), "GeorchestraUser.oAuth2ProviderId is null"); - GeorchestraUser user = accounts.getOrCreate(mappedUser); - - try { -// ((OAuth2AuthenticationToken) auth).setDetails(); - Field field = AbstractAuthenticationToken.class.getDeclaredField("authorities"); - field.setAccessible(true); - field.set(auth, - user.getRoles().stream().map(r -> new SimpleGrantedAuthority(r)).collect(Collectors.toList())); - } catch (NoSuchFieldException e) { - } catch (IllegalAccessException e) { - } - } if (isPreAuth) { Objects.requireNonNull(mappedUser.getUsername(), "GeorchestraUser.username is null"); From 1ce982281b4e1df3ac376f2a3d7fa70f1800f7ec Mon Sep 17 00:00:00 2001 From: Gabriel Roldan Date: Fri, 8 Sep 2023 11:35:00 -0300 Subject: [PATCH 3/3] Fix Gateway authorization does not work for derived roles Use a custom `ReactiveAuthorizationManager` to authorize requests using both the `Authentication` object's granted authorities and the `GeorchestraUser`'s (possibly) derived role names. --- .../accessrules/AccessRulesConfiguration.java | 6 +- .../accessrules/AccessRulesCustomizer.java | 8 +- ...rchestraUserRolesAuthorizationManager.java | 159 ++++++++++++++++++ .../AccessRulesCustomizerTest.java | 7 +- ...straUserRolesAuthorizationManagerTest.java | 101 +++++++++++ 5 files changed, 276 insertions(+), 5 deletions(-) create mode 100644 gateway/src/main/java/org/georchestra/gateway/security/accessrules/GeorchestraUserRolesAuthorizationManager.java create mode 100644 gateway/src/test/java/org/georchestra/gateway/security/accessrules/GeorchestraUserRolesAuthorizationManagerTest.java diff --git a/gateway/src/main/java/org/georchestra/gateway/security/accessrules/AccessRulesConfiguration.java b/gateway/src/main/java/org/georchestra/gateway/security/accessrules/AccessRulesConfiguration.java index ec4c74ac..1063147b 100644 --- a/gateway/src/main/java/org/georchestra/gateway/security/accessrules/AccessRulesConfiguration.java +++ b/gateway/src/main/java/org/georchestra/gateway/security/accessrules/AccessRulesConfiguration.java @@ -19,6 +19,7 @@ package org.georchestra.gateway.security.accessrules; import org.georchestra.gateway.model.GatewayConfigProperties; +import org.georchestra.gateway.security.GeorchestraUserMapper; import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -28,7 +29,8 @@ public class AccessRulesConfiguration { @Bean - AccessRulesCustomizer georchestraAccessRulesCustomizer(GatewayConfigProperties config) { - return new AccessRulesCustomizer(config); + AccessRulesCustomizer georchestraAccessRulesCustomizer(GatewayConfigProperties config, + GeorchestraUserMapper userMapper) { + return new AccessRulesCustomizer(config, userMapper); } } diff --git a/gateway/src/main/java/org/georchestra/gateway/security/accessrules/AccessRulesCustomizer.java b/gateway/src/main/java/org/georchestra/gateway/security/accessrules/AccessRulesCustomizer.java index 119e3107..0b3b8d54 100644 --- a/gateway/src/main/java/org/georchestra/gateway/security/accessrules/AccessRulesCustomizer.java +++ b/gateway/src/main/java/org/georchestra/gateway/security/accessrules/AccessRulesCustomizer.java @@ -25,6 +25,7 @@ import org.georchestra.gateway.model.GatewayConfigProperties; import org.georchestra.gateway.model.RoleBasedAccessRule; import org.georchestra.gateway.model.Service; +import org.georchestra.gateway.security.GeorchestraUserMapper; import org.georchestra.gateway.security.ServerHttpSecurityCustomizer; import org.springframework.security.config.web.server.ServerHttpSecurity; import org.springframework.security.config.web.server.ServerHttpSecurity.AuthorizeExchangeSpec; @@ -57,6 +58,7 @@ public class AccessRulesCustomizer implements ServerHttpSecurityCustomizer { private final @NonNull GatewayConfigProperties config; + private final @NonNull GeorchestraUserMapper userMapper; @Override public void customize(ServerHttpSecurity http) { @@ -145,7 +147,11 @@ void requireAuthenticatedUser(Access access) { @VisibleForTesting void hasAnyAuthority(Access access, List roles) { - access.hasAnyAuthority(roles.toArray(String[]::new)); + // Checks against the effective set of rules (both provided by the Authorization + // service and derived from roles mappings) + access.access( + GeorchestraUserRolesAuthorizationManager.hasAnyAuthority(userMapper, roles.toArray(String[]::new))); + // access.hasAnyAuthority(roles.toArray(String[]::new)); } @VisibleForTesting diff --git a/gateway/src/main/java/org/georchestra/gateway/security/accessrules/GeorchestraUserRolesAuthorizationManager.java b/gateway/src/main/java/org/georchestra/gateway/security/accessrules/GeorchestraUserRolesAuthorizationManager.java new file mode 100644 index 00000000..95ecac78 --- /dev/null +++ b/gateway/src/main/java/org/georchestra/gateway/security/accessrules/GeorchestraUserRolesAuthorizationManager.java @@ -0,0 +1,159 @@ +/* + * Copyright (C) 2022 by the geOrchestra PSC + * + * This file is part of geOrchestra. + * + * geOrchestra is free software: you can redistribute it and/or modify it under + * the terms of the GNU General Public License as published by the Free + * Software Foundation, either version 3 of the License, or (at your option) + * any later version. + * + * geOrchestra is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * geOrchestra. If not, see . + */ +package org.georchestra.gateway.security.accessrules; + +import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Stream; + +import org.georchestra.gateway.security.GeorchestraUserMapper; +import org.georchestra.security.model.GeorchestraUser; +import org.springframework.security.authorization.AuthorityAuthorizationDecision; +import org.springframework.security.authorization.AuthorityReactiveAuthorizationManager; +import org.springframework.security.authorization.AuthorizationDecision; +import org.springframework.security.authorization.ReactiveAuthorizationManager; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.AuthorityUtils; +import org.springframework.util.Assert; + +import com.google.common.annotations.VisibleForTesting; + +import reactor.core.publisher.Mono; + +/** + * Variant of {@link AuthorityReactiveAuthorizationManager} that + * {@link #check(Mono, Object) checks} access based on the effectively resolved + * set of role names in a {@link GeorchestraUser}, as opposed to only on the + * {@link Authentication#getAuthorities() Authenticateion authorities}. + *

+ * This is so because the authorization provider (e.g. OAuth2/OIDC) returns an + * {@link Authentication} object from which {@link GeorchestraUserMapper} will + * derive additional roles to be sent to the downstream services as the + * {@code sec-roles} header, and we need to also account for those derived role + * names when granting access to an URI (see {@link AccessRulesCustomizer#apply} + * and {@link AccessRulesCustomizer#hasAnyAuthority}). + */ +class GeorchestraUserRolesAuthorizationManager implements ReactiveAuthorizationManager { + + private final GeorchestraUserMapper userMapper; + private final List authorities; + private final Set authorityFilter; + private final AuthorityAuthorizationDecision unauthorized; + + GeorchestraUserRolesAuthorizationManager(GeorchestraUserMapper userMapper, String... authorities) { + this.userMapper = userMapper; + this.authorities = AuthorityUtils.createAuthorityList(authorities); + this.authorityFilter = Set.of(authorities); + this.unauthorized = new AuthorityAuthorizationDecision(false, this.authorities); + } + + @Override + public Mono check(Mono authentication, T object) { + return authentication.map(this::authorize).map( + (granted) -> ((AuthorizationDecision) new AuthorityAuthorizationDecision(granted, this.authorities))) + .defaultIfEmpty(unauthorized); + } + + @VisibleForTesting + boolean authorize(Authentication authentication) { + if (!authentication.isAuthenticated()) { + return false; + } + Optional user = userMapper.resolve(authentication); + Stream effectiveRoles = user.map(GeorchestraUser::getRoles).map(List::stream).orElse(Stream.empty()); + Stream grandtedAuthorities = authentication.getAuthorities().stream() + .map(GrantedAuthority::getAuthority); + + return Stream.concat(effectiveRoles, grandtedAuthorities).sorted().distinct() + .anyMatch(authorityFilter::contains); + } + + /** + * Creates an instance of {@link GeorchestraUserRolesAuthorizationManager} with + * the provided authority. + * + * @param authority the authority to check for + * @param the type of object being authorized + * @return the new instance + */ + public static GeorchestraUserRolesAuthorizationManager hasAuthority(GeorchestraUserMapper userMapper, + String authority) { + Assert.notNull(authority, "authority cannot be null"); + return new GeorchestraUserRolesAuthorizationManager<>(userMapper, authority); + } + + /** + * Creates an instance of {@link GeorchestraUserRolesAuthorizationManager} with + * the provided authorities. + * + * @param authorities the authorities to check for + * @param the type of object being authorized + * @return the new instance + */ + public static GeorchestraUserRolesAuthorizationManager hasAnyAuthority(GeorchestraUserMapper userMapper, + String... authorities) { + Assert.notNull(authorities, "authorities cannot be null"); + for (String authority : authorities) { + Assert.notNull(authority, "authority cannot be null"); + } + return new GeorchestraUserRolesAuthorizationManager<>(userMapper, authorities); + } + + /** + * Creates an instance of {@link GeorchestraUserRolesAuthorizationManager} with + * the provided authority. + * + * @param role the authority to check for prefixed with "ROLE_" + * @param the type of object being authorized + * @return the new instance + */ + public static GeorchestraUserRolesAuthorizationManager hasRole(GeorchestraUserMapper userMapper, + String role) { + Assert.notNull(role, "role cannot be null"); + return hasAuthority(userMapper, "ROLE_" + role); + } + + /** + * Creates an instance of {@link GeorchestraUserRolesAuthorizationManager} with + * the provided authorities. + * + * @param roles the authorities to check for prefixed with "ROLE_" + * @param the type of object being authorized + * @return the new instance + */ + public static GeorchestraUserRolesAuthorizationManager hasAnyRole(GeorchestraUserMapper userMapper, + String... roles) { + Assert.notNull(roles, "roles cannot be null"); + for (String role : roles) { + Assert.notNull(role, "role cannot be null"); + } + return hasAnyAuthority(userMapper, toNamedRolesArray(roles)); + } + + private static String[] toNamedRolesArray(String... roles) { + String[] result = new String[roles.length]; + for (int i = 0; i < roles.length; i++) { + result[i] = "ROLE_" + roles[i]; + } + return result; + } + +} diff --git a/gateway/src/test/java/org/georchestra/gateway/security/accessrules/AccessRulesCustomizerTest.java b/gateway/src/test/java/org/georchestra/gateway/security/accessrules/AccessRulesCustomizerTest.java index 6ca35035..1727aaeb 100644 --- a/gateway/src/test/java/org/georchestra/gateway/security/accessrules/AccessRulesCustomizerTest.java +++ b/gateway/src/test/java/org/georchestra/gateway/security/accessrules/AccessRulesCustomizerTest.java @@ -39,6 +39,7 @@ import org.georchestra.gateway.model.GatewayConfigProperties; import org.georchestra.gateway.model.RoleBasedAccessRule; import org.georchestra.gateway.model.Service; +import org.georchestra.gateway.security.GeorchestraUserMapper; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; @@ -58,14 +59,16 @@ class AccessRulesCustomizerTest { @BeforeEach void setUp() throws Exception { config = new GatewayConfigProperties(); - customizer = new AccessRulesCustomizer(config); + customizer = new AccessRulesCustomizer(config, new GeorchestraUserMapper(List.of(), List.of())); http = spy(new ServerHttpSecurity() { }); } @Test void testConstructorDoesNotAcceptNullConfig() { - assertThrows(NullPointerException.class, () -> new AccessRulesCustomizer(null)); + assertThrows(NullPointerException.class, + () -> new AccessRulesCustomizer(null, new GeorchestraUserMapper(List.of(), List.of()))); + assertThrows(NullPointerException.class, () -> new AccessRulesCustomizer(config, null)); } @Test diff --git a/gateway/src/test/java/org/georchestra/gateway/security/accessrules/GeorchestraUserRolesAuthorizationManagerTest.java b/gateway/src/test/java/org/georchestra/gateway/security/accessrules/GeorchestraUserRolesAuthorizationManagerTest.java new file mode 100644 index 00000000..75a9fa6e --- /dev/null +++ b/gateway/src/test/java/org/georchestra/gateway/security/accessrules/GeorchestraUserRolesAuthorizationManagerTest.java @@ -0,0 +1,101 @@ +/* + * Copyright (C) 2022 by the geOrchestra PSC + * + * This file is part of geOrchestra. + * + * geOrchestra is free software: you can redistribute it and/or modify it under + * the terms of the GNU General Public License as published by the Free + * Software Foundation, either version 3 of the License, or (at your option) + * any later version. + * + * geOrchestra is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * geOrchestra. If not, see . + */ +package org.georchestra.gateway.security.accessrules; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.List; +import java.util.Optional; + +import org.georchestra.gateway.security.GeorchestraUserMapper; +import org.georchestra.security.model.GeorchestraUser; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.security.authentication.TestingAuthenticationToken; + +class GeorchestraUserRolesAuthorizationManagerTest { + + GeorchestraUserMapper userMapper; + GeorchestraUser user; + GeorchestraUserRolesAuthorizationManager authManager; + + @BeforeEach + void setup() { + userMapper = mock(GeorchestraUserMapper.class); + user = new GeorchestraUser(); + when(userMapper.resolve(any())).thenReturn(Optional.of(user)); + + authManager = GeorchestraUserRolesAuthorizationManager.hasAnyAuthority(userMapper, "GDI_ADMIN", "SUPERUSER", + "ROLE_ADMIN"); + } + + private TestingAuthenticationToken authentication(String... roles) { + return new TestingAuthenticationToken("gabe", null, roles); + } + + @Test + void hasAnyAuthority_notAuthenticated() { + TestingAuthenticationToken authentication = authentication(); + authentication.setAuthenticated(false); + assertThat(authManager.authorize(authentication)).isFalse(); + } + + @Test + void hasAnyAuthority() { + TestingAuthenticationToken authentication = authentication("ROLE_USER"); + user.setRoles(List.of("ROLE_USER", "GDI_ADMIN")); + assertThat(authManager.authorize(authentication)).isTrue(); + + user.setRoles(List.of("ROLE_USER", "SUPERUSER")); + assertThat(authManager.authorize(authentication)).isTrue(); + + user.setRoles(List.of("ROLE_USER", "ROLE_ADMIN")); + assertThat(authManager.authorize(authentication)).isTrue(); + + user.setRoles(List.of("ROLE_USER")); + assertThat(authManager.authorize(authentication)).isFalse(); + } + + @Test + void hasAnyAuthority_joins_user_and_authentication_authorities() { + TestingAuthenticationToken authentication = authentication("GDI_ADMIN"); + user.setRoles(List.of("ROLE_USER")); + assertThat(authManager.authorize(authentication)).isTrue(); + } + + @Test + void hasAnyAuthority_noResolvedUser_nor_grantedAuthorities() { + TestingAuthenticationToken authentication = authentication(); + when(userMapper.resolve(any())).thenReturn(Optional.empty()); + + assertThat(authManager.authorize(authentication)).isFalse(); + } + + @Test + void hasAnyAuthority_noResolvedUser_resolved_grantedAuthorities() { + TestingAuthenticationToken authentication = authentication("GDI_ADMIN"); + when(userMapper.resolve(any())).thenReturn(Optional.empty()); + + assertThat(authManager.authorize(authentication)).isTrue(); + } + +}