From 5d5d7a381b133cef989733dd51de601a72428e08 Mon Sep 17 00:00:00 2001 From: Valentin Laurin Date: Thu, 13 Mar 2025 18:57:07 +0000 Subject: [PATCH] Combine prefixed scopes and roles as authorities Removal of roles from authorities in 081b5bb was a mistake, this change reverts authorities to being the concatenation of both scopes and roles. However, it is not correct for roles and scopes to not distinguishable in authorities. Spring addresses this by prefixing scopes with `SCOPE_` and roles with `ROLE_`. This change follows this practice. --- .../QuickcaseClientAuthentication.java | 18 ++++---- .../AccessTokenAuthenticationConverter.java | 9 ++-- .../QuickcaseAuthenticationConverter.java | 28 +++++++++++- .../UserInfoAuthenticationConverter.java | 7 ++- .../QuickcaseClientAuthenticationTest.java | 5 ++- ...ccessTokenAuthenticationConverterTest.java | 43 +++++++++++++++---- .../UserInfoAuthenticationConverterTest.java | 35 ++++++++++++--- 7 files changed, 112 insertions(+), 33 deletions(-) diff --git a/api/src/main/java/app/quickcase/spring/oidc/authentication/QuickcaseClientAuthentication.java b/api/src/main/java/app/quickcase/spring/oidc/authentication/QuickcaseClientAuthentication.java index 7593985..63ee44d 100644 --- a/api/src/main/java/app/quickcase/spring/oidc/authentication/QuickcaseClientAuthentication.java +++ b/api/src/main/java/app/quickcase/spring/oidc/authentication/QuickcaseClientAuthentication.java @@ -3,7 +3,6 @@ import java.util.Collection; import java.util.Optional; import java.util.Set; -import java.util.stream.Collectors; import app.quickcase.spring.oidc.AccessLevel; import app.quickcase.spring.oidc.SecurityClassification; @@ -23,12 +22,17 @@ private static OrganisationProfile clientProfile() { } private final String clientId; - - public QuickcaseClientAuthentication(String accessToken, - String clientId, - Collection authorities) { + private final Set roles; + + public QuickcaseClientAuthentication( + String accessToken, + String clientId, + Collection authorities, + Set roles + ) { super(authorities, accessToken); this.clientId = clientId; + this.roles = roles; this.setAuthenticated(true); } @@ -54,9 +58,7 @@ public String getName() { @Override public Set getRoles() { - return getAuthorities().stream() - .map(GrantedAuthority::getAuthority) - .collect(Collectors.toSet()); + return roles; } @Override diff --git a/api/src/main/java/app/quickcase/spring/oidc/authentication/converter/AccessTokenAuthenticationConverter.java b/api/src/main/java/app/quickcase/spring/oidc/authentication/converter/AccessTokenAuthenticationConverter.java index 3983016..10bd34c 100644 --- a/api/src/main/java/app/quickcase/spring/oidc/authentication/converter/AccessTokenAuthenticationConverter.java +++ b/api/src/main/java/app/quickcase/spring/oidc/authentication/converter/AccessTokenAuthenticationConverter.java @@ -7,11 +7,11 @@ import app.quickcase.spring.oidc.authentication.QuickcaseUserAuthentication; import app.quickcase.spring.oidc.claims.ClaimsParser; import app.quickcase.spring.oidc.claims.JwtClaimsParser; +import app.quickcase.spring.oidc.userinfo.UserInfo; import app.quickcase.spring.oidc.userinfo.UserInfoExtractor; - import org.springframework.security.oauth2.jwt.Jwt; -import static app.quickcase.spring.oidc.utils.StringUtils.authorities; +import static app.quickcase.spring.oidc.authentication.converter.QuickcaseAuthenticationConverter.authorities; import static app.quickcase.spring.oidc.utils.StringUtils.fromSpaceSeparated; /** @@ -48,12 +48,13 @@ private QuickcaseAuthentication clientAuthentication(Jwt source) { final String accessToken = source.getTokenValue(); final String subject = source.getSubject(); final Set scopes = fromSpaceSeparated(source.getClaimAsString("scope")); - return new QuickcaseClientAuthentication(accessToken, subject, authorities(scopes)); + return new QuickcaseClientAuthentication(accessToken, subject, authorities(scopes), scopes); } private QuickcaseAuthentication userAuthentication(Jwt source) { final ClaimsParser claims = new JwtClaimsParser(source.getClaims()); final Set scopes = fromSpaceSeparated(source.getClaimAsString("scope")); - return new QuickcaseUserAuthentication(source.getTokenValue(), authorities(scopes), userInfoExtractor.extract(claims)); + final UserInfo userInfo = userInfoExtractor.extract(claims); + return new QuickcaseUserAuthentication(source.getTokenValue(), authorities(scopes, userInfo.getRoles()), userInfo); } } diff --git a/api/src/main/java/app/quickcase/spring/oidc/authentication/converter/QuickcaseAuthenticationConverter.java b/api/src/main/java/app/quickcase/spring/oidc/authentication/converter/QuickcaseAuthenticationConverter.java index 181718b..82deda9 100644 --- a/api/src/main/java/app/quickcase/spring/oidc/authentication/converter/QuickcaseAuthenticationConverter.java +++ b/api/src/main/java/app/quickcase/spring/oidc/authentication/converter/QuickcaseAuthenticationConverter.java @@ -1,9 +1,35 @@ package app.quickcase.spring.oidc.authentication.converter; -import app.quickcase.spring.oidc.authentication.QuickcaseAuthentication; +import java.util.Set; +import java.util.stream.Stream; +import app.quickcase.spring.oidc.authentication.QuickcaseAuthentication; import org.springframework.core.convert.converter.Converter; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.SimpleGrantedAuthority; import org.springframework.security.oauth2.jwt.Jwt; +import static java.util.stream.Collectors.toSet; + public interface QuickcaseAuthenticationConverter extends Converter { + static String prefixScope(String scope) { + return "SCOPE_" + scope; + } + + static String prefixRole(String role) { + return "ROLE_" + role; + } + + static Set authorities(Set scopes) { + return authorities(scopes, Set.of()); + } + + static Set authorities(Set scopes, Set roles) { + return Stream.concat( + scopes.stream().map(QuickcaseAuthenticationConverter::prefixScope), + roles.stream().map(QuickcaseAuthenticationConverter::prefixRole) + ) + .map(SimpleGrantedAuthority::new) + .collect(toSet()); + } } diff --git a/api/src/main/java/app/quickcase/spring/oidc/authentication/converter/UserInfoAuthenticationConverter.java b/api/src/main/java/app/quickcase/spring/oidc/authentication/converter/UserInfoAuthenticationConverter.java index 779535d..ca020ee 100644 --- a/api/src/main/java/app/quickcase/spring/oidc/authentication/converter/UserInfoAuthenticationConverter.java +++ b/api/src/main/java/app/quickcase/spring/oidc/authentication/converter/UserInfoAuthenticationConverter.java @@ -7,10 +7,9 @@ import app.quickcase.spring.oidc.authentication.QuickcaseUserAuthentication; import app.quickcase.spring.oidc.userinfo.UserInfo; import app.quickcase.spring.oidc.userinfo.UserInfoService; - import org.springframework.security.oauth2.jwt.Jwt; -import static app.quickcase.spring.oidc.utils.StringUtils.authorities; +import static app.quickcase.spring.oidc.authentication.converter.QuickcaseAuthenticationConverter.authorities; import static app.quickcase.spring.oidc.utils.StringUtils.fromSpaceSeparated; public class UserInfoAuthenticationConverter implements QuickcaseAuthenticationConverter { @@ -43,7 +42,7 @@ private QuickcaseAuthentication clientAuthentication(Jwt source) { final String accessToken = source.getTokenValue(); final String subject = source.getSubject(); final Set scopes = fromSpaceSeparated(source.getClaimAsString("scope")); - return new QuickcaseClientAuthentication(accessToken, subject, authorities(scopes)); + return new QuickcaseClientAuthentication(accessToken, subject, authorities(scopes), scopes); } private QuickcaseUserAuthentication userAuthentication(Jwt source) { @@ -52,6 +51,6 @@ private QuickcaseUserAuthentication userAuthentication(Jwt source) { final Set scopes = fromSpaceSeparated(source.getClaimAsString("scope")); final UserInfo userInfo = userInfoService.loadUserInfo(subject, accessToken); - return new QuickcaseUserAuthentication(accessToken, authorities(scopes), userInfo); + return new QuickcaseUserAuthentication(accessToken, authorities(scopes, userInfo.getRoles()), userInfo); } } diff --git a/api/src/test/java/app/quickcase/spring/oidc/authentication/QuickcaseClientAuthenticationTest.java b/api/src/test/java/app/quickcase/spring/oidc/authentication/QuickcaseClientAuthenticationTest.java index f70b9fa..88d59f3 100644 --- a/api/src/test/java/app/quickcase/spring/oidc/authentication/QuickcaseClientAuthenticationTest.java +++ b/api/src/test/java/app/quickcase/spring/oidc/authentication/QuickcaseClientAuthenticationTest.java @@ -111,7 +111,8 @@ void getOrganisationProfile() { } private QuickcaseAuthentication clientAuthentication() { - final Set authorities = StringUtils.authorities("ROLE-1", "ROLE-2"); - return new QuickcaseClientAuthentication(ACCESS_TOKEN, CLIENT_ID, authorities); + final Set scopes = Set.of("ROLE-1", "ROLE-2"); + final Set authorities = StringUtils.authorities(scopes); + return new QuickcaseClientAuthentication(ACCESS_TOKEN, CLIENT_ID, authorities, scopes); } } \ No newline at end of file diff --git a/api/src/test/java/app/quickcase/spring/oidc/authentication/converter/AccessTokenAuthenticationConverterTest.java b/api/src/test/java/app/quickcase/spring/oidc/authentication/converter/AccessTokenAuthenticationConverterTest.java index 7407757..edce115 100644 --- a/api/src/test/java/app/quickcase/spring/oidc/authentication/converter/AccessTokenAuthenticationConverterTest.java +++ b/api/src/test/java/app/quickcase/spring/oidc/authentication/converter/AccessTokenAuthenticationConverterTest.java @@ -8,7 +8,6 @@ import app.quickcase.spring.oidc.userinfo.UserInfo; import app.quickcase.spring.oidc.userinfo.UserInfoExtractor; import app.quickcase.spring.oidc.userinfo.UserPreferences; - import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; @@ -18,9 +17,7 @@ import org.springframework.security.oauth2.jwt.Jwt; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.containsInAnyOrder; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.*; class AccessTokenAuthenticationConverterTest { private static final String ACCESS_TOKEN = "token123"; @@ -84,12 +81,22 @@ void shouldGetIdFromClient() { } @Test - @DisplayName("should use scopes as authorities") + @DisplayName("should use scopes as prefixed authorities") void shouldUseScopesAsAuthorities() { final QuickcaseAuthentication authentication = clientAuthentication(); - final GrantedAuthority[] scopes = authorities(SCOPE_1, SCOPE_2); - assertThat(authentication.getAuthorities(), containsInAnyOrder(scopes)); + assertThat(authentication.getAuthorities(), containsInAnyOrder(authorities( + "SCOPE_" + SCOPE_1, + "SCOPE_" + SCOPE_2 + ))); + } + + @Test + @DisplayName("should use scopes as roles") + void shouldUseScopesAsRoles() { + final QuickcaseAuthentication authentication = clientAuthentication(); + + assertThat(authentication.getRoles(), containsInAnyOrder(SCOPE_1, SCOPE_2)); } @Test @@ -131,6 +138,27 @@ private QuickcaseAuthentication userAuthentication(String openidScope) { return converter.convert(jwt); } + @Test + @DisplayName("should combine prefixed scopes and roles as authorities") + void shouldUseRolesAsAuthorities() { + final QuickcaseAuthentication authentication = userAuthentication(); + + assertThat(authentication.getAuthorities(), containsInAnyOrder(authorities( + "SCOPE_openid", + "SCOPE_" + SCOPE_2, + "ROLE_" + ROLE_1, + "ROLE_" + ROLE_2 + ))); + } + + @Test + @DisplayName("should expose user roles") + void shouldUseScopesAsRoles() { + final QuickcaseAuthentication authentication = userAuthentication(); + + assertThat(authentication.getRoles(), containsInAnyOrder(ROLE_1, ROLE_2)); + } + @Test @DisplayName("should populate user authentication from access token") void shouldGetIdFromUser() { @@ -141,7 +169,6 @@ void shouldGetIdFromUser() { assertThat(authentication.getId(), equalTo(USER_ID)); assertThat(authentication.getName(), equalTo(USER_NAME)); assertThat(authentication.getEmail().get(), equalTo(USER_EMAIL)); - assertThat(authentication.getAuthorities(), containsInAnyOrder(authorities(AccessTokenAuthenticationConverter.OPENID_SCOPE, SCOPE_2))); assertThat(authentication.getRoles(), containsInAnyOrder(ROLE_1, ROLE_2)); assertThat(authentication.getUserInfo() .get() diff --git a/api/src/test/java/app/quickcase/spring/oidc/authentication/converter/UserInfoAuthenticationConverterTest.java b/api/src/test/java/app/quickcase/spring/oidc/authentication/converter/UserInfoAuthenticationConverterTest.java index 02ccbdd..5ee6829 100644 --- a/api/src/test/java/app/quickcase/spring/oidc/authentication/converter/UserInfoAuthenticationConverterTest.java +++ b/api/src/test/java/app/quickcase/spring/oidc/authentication/converter/UserInfoAuthenticationConverterTest.java @@ -48,7 +48,7 @@ void setUp() { userInfoServiceStub = (sub, token) -> UserInfo.builder(sub) .name(USER_NAME) .email(USER_EMAIL) - .authorities(ROLE_1, ROLE_2) + .roles(ROLE_1, ROLE_2) .organisationProfile("org-a", orgA) .build(); @@ -79,12 +79,22 @@ void shouldGetIdFromClient() { } @Test - @DisplayName("should use scopes as authorities") + @DisplayName("should use scopes as prefixed authorities") void shouldUseScopesAsAuthorities() { final QuickcaseAuthentication authentication = clientAuthentication(); - final GrantedAuthority[] scopes = authorities(SCOPE_1, SCOPE_2); - assertThat(authentication.getAuthorities(), containsInAnyOrder(scopes)); + assertThat(authentication.getAuthorities(), containsInAnyOrder(authorities( + "SCOPE_" + SCOPE_1, + "SCOPE_" + SCOPE_2 + ))); + } + + @Test + @DisplayName("should use scopes as roles") + void shouldUseScopesAsRoles() { + final QuickcaseAuthentication authentication = clientAuthentication(); + + assertThat(authentication.getRoles(), containsInAnyOrder(SCOPE_1, SCOPE_2)); } @Test @@ -158,11 +168,24 @@ void shouldHaveUserInfo() { } @Test - @DisplayName("should use scopes as authorities") + @DisplayName("should combine prefixed scopes and roles as authorities") void shouldUseRolesAsAuthorities() { final QuickcaseAuthentication authentication = userAuthentication(); - assertThat(authentication.getAuthorities(), containsInAnyOrder(authorities("openid", SCOPE_2))); + assertThat(authentication.getAuthorities(), containsInAnyOrder(authorities( + "SCOPE_openid", + "SCOPE_" + SCOPE_2, + "ROLE_" + ROLE_1, + "ROLE_" + ROLE_2 + ))); + } + + @Test + @DisplayName("should expose user roles") + void shouldUseScopesAsRoles() { + final QuickcaseAuthentication authentication = userAuthentication(); + + assertThat(authentication.getRoles(), containsInAnyOrder(ROLE_1, ROLE_2)); } @Test