From 6a0dfe7704bfe41409b41b7e05446d7df9a1e2fc Mon Sep 17 00:00:00 2001 From: Alex Kanitz Date: Sat, 14 Nov 2026 00:50:09 +0100 Subject: [PATCH 1/6] refactor: use Spring Security's resource server Replace Spring Security OAuth (spring-security-oauth2 and spring-security-oauth2-autoconfigure, end of life, not on Spring Security 6) with the resource server of Spring Security 5.8: - ResourceServerConfiguration declares a SecurityFilterChain with oauth2ResourceServer().opaqueToken(), stateless and without CSRF, with the same public paths. - UserInfoTokenIntrospector validates tokens at the user info endpoint (security.oauth2.resource.user-info-uri, unchanged), as UserInfoTokenServices did, and builds the User and its group authorities with ElixirPrincipalExtractor and GroupNamesSecurityExtractor. - ServiceClientTokenServices is an OpaqueTokenIntrospector now, with the same checks, messages and logger name. - AuthenticatedUser carries the User; the authentication of a request keeps the User as principal, as the authorisation rules expect. - CustomAuthenticationEntryPoint writes the same 401 answers: JSON body with error and error_description, WWW-Authenticate with realm "oauth". Deliberate changes, covered by AuthErrorResponsesIT: the description of an invalid token no longer repeats the token; tokens in an access_token parameter are ignored; malformed bearer headers are refused without a call to the user info endpoint; a user info without sub gives 401 instead of 500. The existing auth integration tests pass unchanged. Co-Authored-By: Claude Opus 5.5 --- pom.xml | 11 +- .../ac/ebi/tsc/tesk/AuthErrorResponsesIT.java | 200 ++++++++++++++++ .../config/security/AuthenticatedUser.java | 74 ++++++ .../CustomAuthenticationEntryPoint.java | 151 +++++++----- .../security/ElixirPrincipalExtractor.java | 16 +- .../security/GroupNamesSecurityExtractor.java | 12 +- .../security/ResourceServerConfiguration.java | 119 +++++----- .../security/ServiceClientTokenServices.java | 72 +++--- .../security/UserInfoTokenIntrospector.java | 116 ++++++++++ .../tsc/tesk/tes/api/TasksApiController.java | 11 +- .../CustomAuthenticationEntryPointTest.java | 91 ++++++++ .../ServiceClientTokenServicesTest.java | 74 +++--- .../UserInfoTokenIntrospectorTest.java | 214 ++++++++++++++++++ .../tesk/tes/api/TasksApiControllerTest.java | 76 +++---- 14 files changed, 978 insertions(+), 259 deletions(-) create mode 100644 src/integration-test/java/uk/ac/ebi/tsc/tesk/AuthErrorResponsesIT.java create mode 100644 src/main/java/uk/ac/ebi/tsc/tesk/config/security/AuthenticatedUser.java create mode 100644 src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java create mode 100644 src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java create mode 100644 src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java diff --git a/pom.xml b/pom.xml index 81e454c1..a07e4efe 100644 --- a/pom.xml +++ b/pom.xml @@ -45,11 +45,6 @@ - - org.springframework.security.oauth - spring-security-oauth2 - 2.5.2.RELEASE - com.google.protobuf protobuf-java @@ -113,10 +108,10 @@ org.springframework.boot spring-boot-starter-security + - org.springframework.security.oauth.boot - spring-security-oauth2-autoconfigure - 2.6.8 + org.springframework.security + spring-security-oauth2-resource-server diff --git a/src/integration-test/java/uk/ac/ebi/tsc/tesk/AuthErrorResponsesIT.java b/src/integration-test/java/uk/ac/ebi/tsc/tesk/AuthErrorResponsesIT.java new file mode 100644 index 00000000..4bd7131f --- /dev/null +++ b/src/integration-test/java/uk/ac/ebi/tsc/tesk/AuthErrorResponsesIT.java @@ -0,0 +1,200 @@ +package uk.ac.ebi.tsc.tesk; + +import static com.github.tomakehurst.wiremock.client.WireMock.*; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.head; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; +import static uk.ac.ebi.tsc.tesk.UrlConstants.TASK_URL; + +import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.junit.WireMockRule; +import io.kubernetes.client.openapi.ApiClient; +import io.kubernetes.client.util.Config; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.context.TestConfiguration; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Primary; +import org.springframework.http.MediaType; +import org.springframework.test.context.TestPropertySource; +import org.springframework.test.context.junit4.SpringRunner; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.ResultActions; + +/** + * Integration testing of the answers to requests without a valid access token (auth profile): the + * status, the headers ({@code WWW-Authenticate}) and the JSON body. The user info endpoint is + * WireMocked. + */ +@RunWith(SpringRunner.class) +@SpringBootTest +@AutoConfigureMockMvc +@TestPropertySource( + locations = {"classpath:application.properties"}, + properties = { + "security.oauth2.resource.user-info-uri = http://localhost:8090", + "spring.profiles.active=auth" + }) +public class AuthErrorResponsesIT { + + private static final String TOKEN = "TOKEN-THAT-MUST-NOT-BE-ECHOED"; + + private static final String NO_TOKEN_MESSAGE = + "Full authentication is required to access this resource"; + + private static final String INVALID_TOKEN_MESSAGE = "Invalid access token"; + + @Autowired private MockMvc mvc; + + @Rule public WireMockRule mockUserInfo = new WireMockRule(8090); + + @TestConfiguration + static class KubernetesClientMock { + @Bean + @Primary + public ApiClient kubernetesApiClient() { + return Config.fromUrl("http://localhost:9000", false); + } + } + + /** The answer to a request without a usable token: as before the resource server moved */ + private static void expectUnauthorized(ResultActions result, String error, String description) + throws Exception { + result.andExpect(status().isUnauthorized()) + .andExpect( + header().string( + "WWW-Authenticate", + "Bearer realm=\"oauth\", error=\"" + + error + + "\", error_description=\"" + + description + + "\"")) + .andExpect(header().string("Cache-Control", "no-store")) + .andExpect(header().string("Pragma", "no-cache")) + .andExpect(content().contentTypeCompatibleWith(MediaType.APPLICATION_JSON)) + .andExpect(header().string("WWW-Authenticate", not(containsString(TOKEN)))) + .andExpect(content().string(not(containsString(TOKEN)))); + } + + private static void expectBody(ResultActions result, String error, String description) + throws Exception { + result.andExpect(jsonPath("$.status").value(401)) + .andExpect(jsonPath("$.error").value(error)) + .andExpect(jsonPath("$.message").value(description)) + .andExpect(jsonPath("$.error_description").value(description)) + .andExpect(jsonPath("$.path").value(TASK_URL)) + .andExpect(jsonPath("$.timestamp").exists()); + } + + @Test + public void withoutToken_isUnauthorized() throws Exception { + ResultActions result = this.mvc.perform(get(TASK_URL)); + expectUnauthorized(result, "unauthorized", NO_TOKEN_MESSAGE); + expectBody(result, "unauthorized", NO_TOKEN_MESSAGE); + expectUnauthorized(this.mvc.perform(head(TASK_URL)), "unauthorized", NO_TOKEN_MESSAGE); + mockUserInfo.verify(0, anyRequestedFor(anyUrl())); + } + + @Test + public void otherScheme_isUnauthorized() throws Exception { + ResultActions result = + this.mvc.perform(get(TASK_URL).header("Authorization", "Basic Zm9vOmJhcg==")); + expectUnauthorized(result, "unauthorized", NO_TOKEN_MESSAGE); + expectBody(result, "unauthorized", NO_TOKEN_MESSAGE); + mockUserInfo.verify(0, anyRequestedFor(anyUrl())); + } + + /** The user info endpoint gets the token as a bearer token, and is asked for JSON */ + @Test + public void validToken_isSentToUserInfo() throws Exception { + mockUserInfo.givenThat( + WireMock.get("/") + .willReturn(okJson("{\"sub\":\"123\",\"eduperson_entitlement\":[]}"))); + this.mvc + .perform(get(TASK_URL).header("Authorization", "Bearer " + TOKEN)) + .andExpect(status().isForbidden()); + mockUserInfo.verify( + 1, + getRequestedFor(urlEqualTo("/")) + .withHeader("Authorization", equalTo("Bearer " + TOKEN)) + .withHeader("Accept", containing("application/json"))); + } + + private void expectInvalidToken() throws Exception { + ResultActions result = + this.mvc.perform(get(TASK_URL).header("Authorization", "Bearer " + TOKEN)); + expectUnauthorized(result, "invalid_token", INVALID_TOKEN_MESSAGE); + expectBody(result, "invalid_token", INVALID_TOKEN_MESSAGE); + } + + @Test + public void tokenRefusedByUserInfo_isUnauthorized() throws Exception { + mockUserInfo.givenThat(WireMock.get("/").willReturn(unauthorized())); + expectInvalidToken(); + } + + @Test + public void userInfoFails_isUnauthorized() throws Exception { + mockUserInfo.givenThat(WireMock.get("/").willReturn(serverError())); + expectInvalidToken(); + } + + @Test + public void userInfoWithError_isUnauthorized() throws Exception { + mockUserInfo.givenThat( + WireMock.get("/").willReturn(okJson("{\"error\":\"invalid_token\"}"))); + expectInvalidToken(); + } + + @Test + public void userInfoWithoutSub_isUnauthorized() throws Exception { + mockUserInfo.givenThat(WireMock.get("/").willReturn(okJson("{\"name\":\"x\"}"))); + expectInvalidToken(); + } + + @Test + public void userInfoNotJson_isUnauthorized() throws Exception { + mockUserInfo.givenThat( + WireMock.get("/") + .willReturn( + aResponse() + .withHeader("Content-Type", "application/json") + .withBody("[\"sub\"]"))); + expectInvalidToken(); + } + + /** Malformed bearer tokens are refused without asking the user info endpoint */ + @Test + public void malformedToken_isUnauthorized() throws Exception { + mockUserInfo.givenThat( + WireMock.get("/") + .willReturn(okJson("{\"sub\":\"123\",\"eduperson_entitlement\":[]}"))); + for (String value : new String[] {"Bearer ", "Bearer a b", "Bearer a\"b"}) { + this.mvc + .perform(get(TASK_URL).header("Authorization", value)) + .andExpect(status().isUnauthorized()) + .andExpect(jsonPath("$.error").value("invalid_token")); + } + mockUserInfo.verify(0, anyRequestedFor(anyUrl())); + } + + /** Tokens are only taken from the Authorization header, not from the URL */ + @Test + public void tokenInQuery_isIgnored() throws Exception { + mockUserInfo.givenThat( + WireMock.get("/") + .willReturn(okJson("{\"sub\":\"123\",\"eduperson_entitlement\":[]}"))); + ResultActions result = this.mvc.perform(get(TASK_URL).param("access_token", TOKEN)); + expectUnauthorized(result, "unauthorized", NO_TOKEN_MESSAGE); + mockUserInfo.verify(0, anyRequestedFor(anyUrl())); + } +} diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/AuthenticatedUser.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/AuthenticatedUser.java new file mode 100644 index 00000000..22e0377c --- /dev/null +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/AuthenticatedUser.java @@ -0,0 +1,74 @@ +package uk.ac.ebi.tsc.tesk.config.security; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.oauth2.core.OAuth2AuthenticatedPrincipal; +import org.springframework.security.oauth2.server.resource.InvalidBearerTokenException; + +/** + * The result of validating an access token ({@link UserInfoTokenIntrospector}, {@link + * ServiceClientTokenServices}): the {@link User} (a user or a service client), its authorities (the + * group names) and the claims it was built from. {@link #toAuthentication} turns it into the + * authentication of the request, whose principal is the {@link User}, as the authorisation rules + * (e.g. of {@code TesService}) expect. + */ +public final class AuthenticatedUser implements OAuth2AuthenticatedPrincipal { + + private final User user; + private final Map attributes; + private final List authorities; + + AuthenticatedUser( + User user, + Map attributes, + Collection authorities) { + this.user = Objects.requireNonNull(user); + this.attributes = Collections.unmodifiableMap(attributes); + this.authorities = Collections.unmodifiableList(new ArrayList<>(authorities)); + } + + /** The user or service client */ + public User getUser() { + return this.user; + } + + /** The claims (of the user info, or of a service client's token) */ + @Override + public Map getAttributes() { + return this.attributes; + } + + @Override + public Collection getAuthorities() { + return this.authorities; + } + + /** The ID of the user (the {@code sub} of the user info) or of the service client */ + @Override + public String getName() { + return this.user.getUsername(); + } + + /** + * The authentication of a request with a valid access token (an {@code + * OpaqueTokenAuthenticationConverter}): the {@link User} as principal, with the authorities; + * the token isn't kept in it + */ + static Authentication toAuthentication( + String accessToken, OAuth2AuthenticatedPrincipal principal) { + if (!(principal instanceof AuthenticatedUser)) { + // not expected: both introspectors return an AuthenticatedUser + throw new InvalidBearerTokenException("Invalid access token"); + } + AuthenticatedUser authenticated = (AuthenticatedUser) principal; + return new UsernamePasswordAuthenticationToken( + authenticated.getUser(), "N/A", authenticated.getAuthorities()); + } +} diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPoint.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPoint.java index d69f7fd9..73a09fe2 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPoint.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPoint.java @@ -4,48 +4,60 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; -import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.boot.autoconfigure.http.HttpMessageConverters; import org.springframework.boot.web.error.ErrorAttributeOptions; import org.springframework.boot.web.servlet.error.ErrorAttributes; -import org.springframework.http.ResponseEntity; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpStatus; +import org.springframework.http.MediaType; import org.springframework.http.converter.HttpMessageConverter; +import org.springframework.http.server.ServletServerHttpResponse; import org.springframework.security.core.AuthenticationException; -import org.springframework.security.oauth2.common.exceptions.OAuth2Exception; -import org.springframework.security.oauth2.provider.error.DefaultOAuth2ExceptionRenderer; -import org.springframework.security.oauth2.provider.error.DefaultWebResponseExceptionTranslator; -import org.springframework.security.oauth2.provider.error.OAuth2AuthenticationEntryPoint; -import org.springframework.security.oauth2.provider.error.WebResponseExceptionTranslator; +import org.springframework.security.oauth2.core.OAuth2AuthenticationException; +import org.springframework.security.oauth2.core.OAuth2Error; +import org.springframework.security.web.AuthenticationEntryPoint; +import org.springframework.util.StringUtils; import org.springframework.web.context.request.ServletWebRequest; -import org.springframework.web.servlet.HandlerExceptionResolver; -import org.springframework.web.servlet.mvc.support.DefaultHandlerExceptionResolver; import org.springframework.web.util.WebUtils; /** - * Entrypoint for OAuth Errors. Inspired by {@link OAuth2AuthenticationEntryPoint} - sadly needed - * some repetition. Adds more data to response body object, to conform to ErrorAttributes format - * plus uses externally supllied messageConverters (Autoconfigured HttpMessageConverters from - * context are used - {@link ResourceServerConfiguration} + * Answers requests without a valid access token with {@code 401 Unauthorized}, in our usual error + * format (Spring Boot's error attributes, written with the autoconfigured message converters) plus + * the OAuth 2.0 error code ({@code error}) and description ({@code error_description}), and a + * {@code WWW-Authenticate} header with both (RFC 6750, section 3). These are the answers of the + * resource server of the former Spring Security OAuth: {@code unauthorized} without a token, {@code + * invalid_token} for a token that isn't valid. * * @author Ania Niewielska */ -public class CustomAuthenticationEntryPoint extends OAuth2AuthenticationEntryPoint { +public class CustomAuthenticationEntryPoint implements AuthenticationEntryPoint { + + private static final Logger logger = + LoggerFactory.getLogger(CustomAuthenticationEntryPoint.class); + + /** The realm in {@code WWW-Authenticate}, as before */ + static final String REALM = "oauth"; + + /** The error code without an OAuth 2.0 error, e.g. without a token */ + static final String UNAUTHORIZED = "unauthorized"; + + /** Name of the OAuth 2.0 error code (RFC 6750, section 3) */ + static final String ERROR = "error"; + + /** Name of the OAuth 2.0 error description (RFC 6750, section 3) */ + static final String DESCRIPTION = "error_description"; - private WebResponseExceptionTranslator exceptionTranslator = - new DefaultWebResponseExceptionTranslator(); - private HandlerExceptionResolver handlerExceptionResolver = - new DefaultHandlerExceptionResolver(); - private DefaultOAuth2ExceptionRenderer exceptionRenderer = new DefaultOAuth2ExceptionRenderer(); private final ErrorAttributes errorAttributes; + private final List> messageConverters; CustomAuthenticationEntryPoint( ErrorAttributes errorAttributes, HttpMessageConverters messageConverters) { this.errorAttributes = errorAttributes; - List> converters = new ArrayList<>(); - converters.addAll(messageConverters.getConverters()); - this.exceptionRenderer.setMessageConverters(converters); + this.messageConverters = new ArrayList<>(messageConverters.getConverters()); } @Override @@ -53,39 +65,72 @@ public void commence( HttpServletRequest request, HttpServletResponse response, AuthenticationException authException) - throws IOException, ServletException { + throws IOException { + String error = UNAUTHORIZED; + String description = authException.getMessage(); + if (authException instanceof OAuth2AuthenticationException) { + OAuth2Error oauth2Error = ((OAuth2AuthenticationException) authException).getError(); + error = oauth2Error.getErrorCode(); + if (StringUtils.hasText(oauth2Error.getDescription())) { + description = oauth2Error.getDescription(); + } + } + WebUtils.exposeErrorRequestAttributes(request, authException, null); + request.setAttribute(WebUtils.ERROR_STATUS_CODE_ATTRIBUTE, HttpStatus.UNAUTHORIZED.value()); + Map body = + this.errorAttributes.getErrorAttributes( + new ServletWebRequest(request), + ErrorAttributeOptions.defaults() + .including(ErrorAttributeOptions.Include.MESSAGE)); + body.put(ERROR, error); + body.put(DESCRIPTION, description); + + response.setStatus(HttpStatus.UNAUTHORIZED.value()); + response.setHeader(HttpHeaders.CACHE_CONTROL, "no-store"); + response.setHeader(HttpHeaders.PRAGMA, "no-cache"); + response.setHeader(HttpHeaders.WWW_AUTHENTICATE, wwwAuthenticate(error, description)); + write(body, request, response); + response.flushBuffer(); + } + + /** The {@code WWW-Authenticate} header, with quotes and backslashes in the values escaped */ + static String wwwAuthenticate(String error, String description) { + return String.format( + "Bearer realm=\"%s\", %s=\"%s\", %s=\"%s\"", + REALM, ERROR, quote(error), DESCRIPTION, quote(description)); + } + + private static String quote(String value) { + return String.valueOf(value).replace("\\", "\\\\").replace("\"", "\\\""); + } + + /** + * Writes the body with the first converter that can write it in a media type that the request + * accepts (most specific first), as before; without one, the answer has no body + */ + @SuppressWarnings("unchecked") + private void write( + Map body, HttpServletRequest request, HttpServletResponse response) + throws IOException { + List accepted; try { - ResponseEntity result = - enhanceResponse(exceptionTranslator.translate(authException), authException); - OAuth2Exception oauth2Exception = (OAuth2Exception) result.getBody(); - WebUtils.exposeErrorRequestAttributes(request, authException, null); - request.setAttribute( - WebUtils.ERROR_STATUS_CODE_ATTRIBUTE, result.getStatusCode().value()); - Map errorBody = - errorAttributes.getErrorAttributes( - new ServletWebRequest(request), - ErrorAttributeOptions.defaults() - .including(ErrorAttributeOptions.Include.MESSAGE)); - errorBody.put(OAuth2Exception.ERROR, oauth2Exception.getOAuth2ErrorCode()); - errorBody.put(OAuth2Exception.DESCRIPTION, oauth2Exception.getMessage()); - ResponseEntity> newResult = - new ResponseEntity<>(errorBody, result.getHeaders(), result.getStatusCode()); - exceptionRenderer.handleHttpEntityResponse( - newResult, new ServletWebRequest(request, response)); - response.flushBuffer(); - } catch (ServletException e) { - // Re-use some of the default Spring dispatcher behaviour - the exception came from the - // filter chain and - // not from an MVC handler so it won't be caught by the dispatcher (even if there is - // one) - if (handlerExceptionResolver.resolveException(request, response, this, e) == null) { - throw e; + accepted = new ArrayList<>(MediaType.parseMediaTypes(request.getHeader("Accept"))); + } catch (IllegalArgumentException e) { + accepted = new ArrayList<>(); + } + if (accepted.isEmpty()) { + accepted.add(MediaType.ALL); + } + MediaType.sortBySpecificityAndQuality(accepted); + for (MediaType mediaType : accepted) { + for (HttpMessageConverter converter : this.messageConverters) { + if (converter.canWrite(body.getClass(), mediaType)) { + ((HttpMessageConverter) converter) + .write(body, mediaType, new ServletServerHttpResponse(response)); + return; + } } - } catch (IOException | RuntimeException e) { - throw e; - } catch (Exception e) { - // Wrap other Exceptions. These are not expected to happen - throw new RuntimeException(e); } + logger.debug("No message converter writes the error body in an accepted media type"); } } diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ElixirPrincipalExtractor.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ElixirPrincipalExtractor.java index 466dc6a0..44566b9f 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ElixirPrincipalExtractor.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ElixirPrincipalExtractor.java @@ -8,19 +8,18 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; -import org.springframework.boot.autoconfigure.security.oauth2.resource.AuthoritiesExtractor; -import org.springframework.boot.autoconfigure.security.oauth2.resource.PrincipalExtractor; import org.springframework.security.core.GrantedAuthority; import org.springframework.util.StringUtils; /** - * Custom principal extractor (puts all of the info from userinfo to custom {@link User} object) + * Custom principal extractor (puts all of the info from userinfo to custom {@link User} object; + * used by {@link UserInfoTokenIntrospector}) * * @author Ania Niewielska */ -public class ElixirPrincipalExtractor implements PrincipalExtractor { +public class ElixirPrincipalExtractor { - private final AuthoritiesExtractor groupsExtractor; + private final GroupNamesSecurityExtractor groupsExtractor; private final AuthorisationProperties authorisationProperties; private final Pattern MEMBERED_GROUP; private final Pattern MANAGED_GROUP; @@ -30,7 +29,8 @@ public class ElixirPrincipalExtractor implements PrincipalExtractor { private final Set claimValues; ElixirPrincipalExtractor( - AuthoritiesExtractor groupsExtractor, AuthorisationProperties authorisationProperties) { + GroupNamesSecurityExtractor groupsExtractor, + AuthorisationProperties authorisationProperties) { this.groupsExtractor = groupsExtractor; this.authorisationProperties = authorisationProperties; // the configured parts are text, matched literally; only the team name is a pattern @@ -94,8 +94,8 @@ private boolean isClaimAuthorised(Map map) { .anyMatch(value -> value instanceof String && this.claimValues.contains(value)); } - @Override - public Object extractPrincipal(Map map) { + /** The user of the user info, with its groups and rights */ + public User extractPrincipal(Map map) { User.UserBuilder builder = User.builder(map.get("sub").toString()) .preferredUsername( diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/GroupNamesSecurityExtractor.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/GroupNamesSecurityExtractor.java index 14bff0f1..3e635f9d 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/GroupNamesSecurityExtractor.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/GroupNamesSecurityExtractor.java @@ -4,7 +4,6 @@ import java.util.Collection; import java.util.List; import java.util.Map; -import org.springframework.boot.autoconfigure.security.oauth2.resource.AuthoritiesExtractor; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.AuthorityUtils; import org.springframework.util.ObjectUtils; @@ -12,12 +11,12 @@ /** * Custom authorities extractor, that looks for groups element and transforms it into authorities. - * Heavily inspired by {@link - * org.springframework.boot.autoconfigure.security.oauth2.resource.FixedAuthoritiesExtractor} + * Heavily inspired by {@code FixedAuthoritiesExtractor} of Spring Security OAuth (used by {@link + * UserInfoTokenIntrospector}) * * @author Ania Niewielska */ -public class GroupNamesSecurityExtractor implements AuthoritiesExtractor { +public class GroupNamesSecurityExtractor { public GroupNamesSecurityExtractor(String authoritiesClaim, String groupPrefix) { this.authoritiesClaim = authoritiesClaim; @@ -27,7 +26,10 @@ public GroupNamesSecurityExtractor(String authoritiesClaim, String groupPrefix) private final String authoritiesClaim; private final String groupPrefix; - @Override + /** + * The group names in the groups claim of the user info, as authorities; without that claim, the + * authority {@code :USER} + */ public List extractAuthorities(Map map) { String authorities = this.groupPrefix + ":USER"; if (map.containsKey(this.authoritiesClaim)) { diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ResourceServerConfiguration.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ResourceServerConfiguration.java index ba35fc80..a6ccc1d6 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ResourceServerConfiguration.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ResourceServerConfiguration.java @@ -3,11 +3,8 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; -import org.springframework.beans.factory.ObjectProvider; import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.http.HttpMessageConverters; -import org.springframework.boot.autoconfigure.security.oauth2.resource.AuthoritiesExtractor; -import org.springframework.boot.autoconfigure.security.oauth2.resource.PrincipalExtractor; import org.springframework.boot.web.servlet.error.ErrorAttributes; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -16,32 +13,29 @@ import org.springframework.security.config.annotation.method.configuration.EnableGlobalMethodSecurity; import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.security.config.core.GrantedAuthorityDefaults; +import org.springframework.security.config.http.SessionCreationPolicy; import org.springframework.security.core.userdetails.UserDetailsService; -import org.springframework.security.oauth2.config.annotation.web.configuration.EnableResourceServer; -import org.springframework.security.oauth2.config.annotation.web.configuration.ResourceServerConfigurerAdapter; -import org.springframework.security.oauth2.config.annotation.web.configurers.ResourceServerSecurityConfigurer; -import org.springframework.security.oauth2.provider.token.ResourceServerTokenServices; +import org.springframework.security.oauth2.server.resource.introspection.OpaqueTokenIntrospector; +import org.springframework.security.web.AuthenticationEntryPoint; +import org.springframework.security.web.SecurityFilterChain; /** - * This class (specifically the @EnableResourceServer annotation) + 2 dependencies: - * spring-boot-starter-security + spring-security-oauth2-autoconfigure + - * security.oauth2.resource.user-info-uri property (which points to user info endpoint of the - * authorisation server) switch on protection of (all minus Swagger) the endpoints via OAuth2 token + * With the auth profile, protects (all but the public) endpoints with OAuth2 bearer tokens, using + * the resource server of Spring Security: tokens are validated at the user info endpoint of the + * authorisation server ({@code security.oauth2.resource.user-info-uri}; {@link + * UserInfoTokenIntrospector}), or, with tesk.api.authorisation.service-clients enabled, the tokens + * of the listed service clients locally ({@link ServiceClientTokenServices}). * * @author Ania Niewielska */ @Configuration -@EnableResourceServer @EnableGlobalMethodSecurity(prePostEnabled = true) @Profile("auth") -public class ResourceServerConfiguration extends ResourceServerConfigurerAdapter { +public class ResourceServerConfiguration { - private final ErrorAttributes errorAttributes; - private final HttpMessageConverters messageConverters; - private final AuthorisationProperties authorisationProperties; // The API description, and the Swagger UI paths, which are only mapped (else 404) with // tesk.api.swagger-ui.enabled - private final String[] ALLOWED_URLS = { + private static final String[] ALLOWED_URLS = { "/", "/v2/api-docs", "/configuration/ui", @@ -50,13 +44,13 @@ public class ResourceServerConfiguration extends ResourceServerConfigurerAdapter "/swagger-ui/**", "/webjars/**" }; + + private final ErrorAttributes errorAttributes; + private final HttpMessageConverters messageConverters; + private final AuthorisationProperties authorisationProperties; private final String baseUrl; private final boolean serviceInfoPublic; - /** - * The token services of the user info endpoint (auto-configured by Spring Boot), looked up only - * when the resource server is configured, as they need the extractors defined here - */ - private final ObjectProvider tokenServices; + private final String userInfoUri; public ResourceServerConfiguration( ErrorAttributes errorAttributes, @@ -64,53 +58,77 @@ public ResourceServerConfiguration( AuthorisationProperties authorisationProperties, @Value("${openapi.taskExecutionService.base-path:/ga4gh/tes/v1}") String baseUrl, @Value("${tesk.api.service-info.public:true}") boolean serviceInfoPublic, - ObjectProvider tokenServices) { + @Value("${security.oauth2.resource.user-info-uri:}") String userInfoUri) { this.errorAttributes = errorAttributes; this.messageConverters = messageConverters; this.authorisationProperties = authorisationProperties; this.baseUrl = baseUrl; this.serviceInfoPublic = serviceInfoPublic; - this.tokenServices = tokenServices; + this.userInfoUri = userInfoUri; } /** - * We leave the sensible defaults only letting swagger UI requests in, and service info unless a - * token is required for it (tesk.api.service-info.public) + * Lets only the Swagger UI requests in without a token, and service info unless a token is + * required for it (tesk.api.service-info.public). A request with a token has it validated also + * on these. Requests without a valid token get the JSON error responses and {@code + * WWW-Authenticate} headers of {@link CustomAuthenticationEntryPoint}. Stateless and without + * CSRF protection (no cookies are used), as the resource server of Spring Security OAuth was. */ - @Override - public void configure(HttpSecurity http) throws Exception { - List allowedUrls = new ArrayList<>(); - allowedUrls.addAll(Arrays.asList(ALLOWED_URLS)); + @Bean + public SecurityFilterChain resourceServerFilterChain( + HttpSecurity http, OpaqueTokenIntrospector introspector) throws Exception { + List allowedUrls = new ArrayList<>(Arrays.asList(ALLOWED_URLS)); if (serviceInfoPublic) { allowedUrls.add(baseUrl + "/service-info"); } String[] allowedUrlsArray = allowedUrls.toArray(new String[0]); - http.authorizeRequests() + AuthenticationEntryPoint entryPoint = + new CustomAuthenticationEntryPoint(errorAttributes, messageConverters); + http.csrf() + .disable() + .sessionManagement() + .sessionCreationPolicy(SessionCreationPolicy.STATELESS) + .and() + .authorizeRequests() .antMatchers(HttpMethod.GET, allowedUrlsArray) .permitAll() .antMatchers(HttpMethod.HEAD, allowedUrlsArray) .permitAll() .anyRequest() - .authenticated(); + .authenticated() + .and() + .exceptionHandling() + .authenticationEntryPoint(entryPoint) + .and() + .oauth2ResourceServer() + .authenticationEntryPoint(entryPoint) + .opaqueToken() + .introspector(introspector) + .authenticationConverter(AuthenticatedUser::toAuthentication); + return http.build(); } /** - * Sets a custom entryPoint that returns json in our usual error format with the use of standard - * Boot's errorAttributes and autoconfigured messageConverters (specifically with autoconfigured - * objectMapper for JacksonConverter). With tesk.api.authorisation.service-clients enabled, - * tokens of the listed service clients are checked locally, all others at the user info - * endpoint as before ({@link ServiceClientTokenServices}). + * Validates the tokens at the user info endpoint, and, with tesk.api.authorisation.service- + * clients enabled, the tokens of the listed service clients locally ({@link + * ServiceClientTokenServices}) */ - @Override - public void configure(ResourceServerSecurityConfigurer resources) { - resources.authenticationEntryPoint( - new CustomAuthenticationEntryPoint(errorAttributes, messageConverters)); + @Bean + public OpaqueTokenIntrospector tokenIntrospector() { + GroupNamesSecurityExtractor authoritiesExtractor = + new GroupNamesSecurityExtractor( + authorisationProperties.getGroupsClaim(), + authorisationProperties.getGroupPrefix()); + OpaqueTokenIntrospector userInfoIntrospector = + new UserInfoTokenIntrospector( + userInfoUri, + new ElixirPrincipalExtractor(authoritiesExtractor, authorisationProperties), + authoritiesExtractor); if (authorisationProperties.getServiceClients().isEnabled()) { - resources.tokenServices( - new ServiceClientTokenServices( - tokenServices.getObject(), authorisationProperties)); + return new ServiceClientTokenServices(userInfoIntrospector, authorisationProperties); } + return userInfoIntrospector; } /** Magic, that changes default role prefix from ROLE_ to (...)elixir: */ @@ -119,19 +137,6 @@ public GrantedAuthorityDefaults grantedAuthorityDefaults() { return new GrantedAuthorityDefaults(this.authorisationProperties.getGroupPrefix()); } - /** Custom authorities extractor (we use Elixir roles as authorities) */ - @Bean - public AuthoritiesExtractor authoritiesExtractor() { - return new GroupNamesSecurityExtractor( - authorisationProperties.getGroupsClaim(), authorisationProperties.getGroupPrefix()); - } - - /** Custom principal extractor (we use the whole userinfo response object as Principal */ - @Bean - public PrincipalExtractor principalExtractor() { - return new ElixirPrincipalExtractor(authoritiesExtractor(), authorisationProperties); - } - /** * Switches off default in memory user store containing one user with auto-generated password */ diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServices.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServices.java index 1163373b..61c571bd 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServices.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServices.java @@ -29,20 +29,16 @@ import org.jose4j.lang.JoseException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; -import org.springframework.security.core.AuthenticationException; import org.springframework.security.core.authority.AuthorityUtils; -import org.springframework.security.oauth2.common.OAuth2AccessToken; -import org.springframework.security.oauth2.common.exceptions.InvalidTokenException; -import org.springframework.security.oauth2.provider.OAuth2Authentication; -import org.springframework.security.oauth2.provider.OAuth2Request; -import org.springframework.security.oauth2.provider.token.ResourceServerTokenServices; +import org.springframework.security.oauth2.core.OAuth2AuthenticatedPrincipal; +import org.springframework.security.oauth2.server.resource.introspection.BadOpaqueTokenException; +import org.springframework.security.oauth2.server.resource.introspection.OpaqueTokenIntrospector; import org.springframework.util.StringUtils; /** * Accepts access tokens of allow-listed service clients (issue #153; settings in {@link - * AuthorisationProperties.ServiceClients}), and passes all other tokens on to the token services of - * the user info endpoint. + * AuthorisationProperties.ServiceClients}), and passes all other tokens on to the user info + * endpoint ({@link UserInfoTokenIntrospector}). * *

A token is a service client's if it is a signed JWT whose {@code iss} is the configured * issuer, whose client claim (by default {@code client_id}) names a listed client, and (unless that @@ -59,7 +55,7 @@ * *

Logs the client's ID for accepted and refused tokens, never the token. */ -public class ServiceClientTokenServices implements ResourceServerTokenServices { +public class ServiceClientTokenServices implements OpaqueTokenIntrospector { private static final Logger logger = LoggerFactory.getLogger(ServiceClientTokenServices.class); @@ -95,7 +91,7 @@ public class ServiceClientTokenServices implements ResourceServerTokenServices { /** How long the keys are kept if fetching them again fails, in seconds */ private static final long RETAIN_KEYS_ON_ERROR_SECONDS = 15 * 60; - private final ResourceServerTokenServices userInfoTokenServices; + private final OpaqueTokenIntrospector userInfoIntrospector; private final String issuer; private final String jwksUri; private final String clientClaim; @@ -125,19 +121,18 @@ public class ServiceClientTokenServices implements ResourceServerTokenServices { private final ReentrantLock buildLock = new ReentrantLock(); /** - * @param userInfoTokenServices the token services of the user info endpoint, for all other - * tokens + * @param userInfoIntrospector validates all other tokens, at the user info endpoint * @param properties the settings; {@code service-clients} must be enabled * @throws IllegalStateException if the settings lack the issuer or a client ID */ public ServiceClientTokenServices( - ResourceServerTokenServices userInfoTokenServices, AuthorisationProperties properties) { - this(userInfoTokenServices, properties, JWKS_REFRESH_INTERVAL_MS); + OpaqueTokenIntrospector userInfoIntrospector, AuthorisationProperties properties) { + this(userInfoIntrospector, properties, JWKS_REFRESH_INTERVAL_MS); } /** As above, with the least time between two fetches of the key set (for tests) */ ServiceClientTokenServices( - ResourceServerTokenServices userInfoTokenServices, + OpaqueTokenIntrospector userInfoIntrospector, AuthorisationProperties properties, long jwksRefreshIntervalMs) { AuthorisationProperties.ServiceClients settings = properties.getServiceClients(); @@ -162,7 +157,7 @@ public ServiceClientTokenServices( "tesk.api.authorisation.service-clients.clock-skew-seconds must not be" + " negative"); } - this.userInfoTokenServices = Objects.requireNonNull(userInfoTokenServices); + this.userInfoIntrospector = Objects.requireNonNull(userInfoIntrospector); this.issuer = settings.getIssuer().trim(); this.jwksUri = StringUtils.hasText(settings.getJwksUri()) ? settings.getJwksUri().trim() : null; @@ -204,47 +199,40 @@ static boolean isSecure(String url) { } @Override - public OAuth2Authentication loadAuthentication(String accessToken) - throws AuthenticationException, InvalidTokenException { + public OAuth2AuthenticatedPrincipal introspect(String accessToken) { String clientId = listedClient(accessToken); if (clientId == null) { - OAuth2Authentication authentication = - this.userInfoTokenServices.loadAuthentication(accessToken); - Object principal = authentication == null ? null : authentication.getPrincipal(); - if (principal instanceof User - && this.clientIds.contains(((User) principal).getUsername())) { + OAuth2AuthenticatedPrincipal principal = + this.userInfoIntrospector.introspect(accessToken); + if (principal != null && this.clientIds.contains(principal.getName())) { // a user with a listed client's ID would share the client's tasks logger.warn( "Refused user token: its sub is the ID of service client {}", - ((User) principal).getUsername()); - throw new InvalidTokenException("User ID of a service client"); + principal.getName()); + throw new BadOpaqueTokenException("User ID of a service client"); } - return authentication; + return principal; } + JwtClaims claims; try { - verifier().process(accessToken); + claims = verifier().process(accessToken).getJwtClaims(); } catch (InvalidJwtException e) { logger.warn("Refused token of service client {}: {}", clientId, reasons(e)); - throw new InvalidTokenException("Invalid access token of a service client"); + throw new BadOpaqueTokenException("Invalid access token of a service client"); } catch (IOException | JoseException e) { // the discovery document can't be read or has no JWKS URI logger.warn( "Refused token of service client {}: keys of the issuer unavailable ({})", clientId, e.getMessage()); - throw new InvalidTokenException("Invalid access token of a service client"); + throw new BadOpaqueTokenException("Invalid access token of a service client"); } catch (RuntimeException e) { // unexpected: logged with its stack trace (which doesn't hold the token) logger.warn("Refused token of service client {}: unexpected error", clientId, e); - throw new InvalidTokenException("Invalid access token of a service client"); + throw new BadOpaqueTokenException("Invalid access token of a service client"); } logger.info("Accepted token of service client {}", clientId); - return authentication(clientId); - } - - @Override - public OAuth2AccessToken readAccessToken(String accessToken) { - return this.userInfoTokenServices.readAccessToken(accessToken); + return principal(clientId, claims); } /** @@ -276,19 +264,15 @@ private String listedClient(String accessToken) { return (String) client; } - /** The authentication of the client, as a principal of its own without groups */ - private OAuth2Authentication authentication(String clientId) { + /** The client, as a principal of its own without groups, with the claims of its token */ + private static OAuth2AuthenticatedPrincipal principal(String clientId, JwtClaims claims) { User user = User.builder(clientId) .allGroups(Collections.emptySet()) .serviceClient(true) .legacyUserLabels(false) .build(); - UsernamePasswordAuthenticationToken userAuthentication = - new UsernamePasswordAuthenticationToken(user, "N/A", AuthorityUtils.NO_AUTHORITIES); - OAuth2Request request = - new OAuth2Request(null, clientId, null, true, null, null, null, null, null); - return new OAuth2Authentication(request, userAuthentication); + return new AuthenticatedUser(user, claims.getClaimsMap(), AuthorityUtils.NO_AUTHORITIES); } /** diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java new file mode 100644 index 00000000..3f7f1119 --- /dev/null +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java @@ -0,0 +1,116 @@ +package uk.ac.ebi.tsc.tesk.config.security; + +import java.net.URI; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.core.ParameterizedTypeReference; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; +import org.springframework.http.MediaType; +import org.springframework.http.RequestEntity; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.oauth2.core.OAuth2AuthenticatedPrincipal; +import org.springframework.security.oauth2.server.resource.introspection.BadOpaqueTokenException; +import org.springframework.security.oauth2.server.resource.introspection.OpaqueTokenIntrospector; +import org.springframework.util.StringUtils; +import org.springframework.web.client.RestOperations; +import org.springframework.web.client.RestTemplate; + +/** + * Validates access tokens at the user info endpoint of the OpenID Connect provider ({@code + * security.oauth2.resource.user-info-uri}), on each request, as Spring Security OAuth's {@code + * UserInfoTokenServices} did before: a token is valid if the endpoint answers it with a JSON object + * that has a {@code sub} and no {@code error}. The user ({@link ElixirPrincipalExtractor}) and its + * authorities, the group names ({@link GroupNamesSecurityExtractor}), are taken from that answer. + * + *

Logs why a token was refused, never the token. + */ +public class UserInfoTokenIntrospector implements OpaqueTokenIntrospector { + + private static final Logger logger = LoggerFactory.getLogger(UserInfoTokenIntrospector.class); + + /** Description of the error for all tokens that the user info endpoint doesn't accept */ + static final String INVALID_TOKEN = "Invalid access token"; + + private static final ParameterizedTypeReference> USER_INFO = + new ParameterizedTypeReference>() {}; + + private final URI userInfoUri; + private final ElixirPrincipalExtractor principalExtractor; + private final GroupNamesSecurityExtractor authoritiesExtractor; + private final RestOperations restOperations; + + /** + * @param userInfoUri the user info endpoint + * @param principalExtractor builds the user from the user info + * @param authoritiesExtractor takes the authorities (group names) from the user info + * @throws IllegalStateException if the user info endpoint isn't set + * @throws IllegalArgumentException if it isn't a URI + */ + public UserInfoTokenIntrospector( + String userInfoUri, + ElixirPrincipalExtractor principalExtractor, + GroupNamesSecurityExtractor authoritiesExtractor) { + this(userInfoUri, principalExtractor, authoritiesExtractor, new RestTemplate()); + } + + /** As above, with the client of the user info endpoint (for tests) */ + UserInfoTokenIntrospector( + String userInfoUri, + ElixirPrincipalExtractor principalExtractor, + GroupNamesSecurityExtractor authoritiesExtractor, + RestOperations restOperations) { + if (!StringUtils.hasText(userInfoUri)) { + throw new IllegalStateException( + "The auth profile needs security.oauth2.resource.user-info-uri" + + " (SECURITY_OAUTH2_RESOURCE_USER_INFO_URI)"); + } + this.userInfoUri = URI.create(userInfoUri.trim()); + this.principalExtractor = Objects.requireNonNull(principalExtractor); + this.authoritiesExtractor = Objects.requireNonNull(authoritiesExtractor); + this.restOperations = Objects.requireNonNull(restOperations); + } + + @Override + public OAuth2AuthenticatedPrincipal introspect(String token) { + Map userInfo = userInfo(token); + if (userInfo.containsKey("error")) { + logger.warn("Refused token: the user info has an error"); + throw new BadOpaqueTokenException(INVALID_TOKEN); + } + if (userInfo.get("sub") == null) { + logger.warn("Refused token: the user info has no sub"); + throw new BadOpaqueTokenException(INVALID_TOKEN); + } + User user = this.principalExtractor.extractPrincipal(userInfo); + List authorities = this.authoritiesExtractor.extractAuthorities(userInfo); + return new AuthenticatedUser(user, userInfo, authorities); + } + + /** The user info of the token's owner; any failure means that the token isn't valid */ + private Map userInfo(String token) { + HttpHeaders headers = new HttpHeaders(); + headers.setBearerAuth(token); + headers.setAccept(Collections.singletonList(MediaType.APPLICATION_JSON)); + RequestEntity request = + new RequestEntity<>(headers, HttpMethod.GET, this.userInfoUri); + Map body; + try { + body = this.restOperations.exchange(request, USER_INFO).getBody(); + } catch (RuntimeException e) { + // the message of a client error can hold the answer, but never the token + logger.warn("Could not fetch user details: {}, {}", e.getClass(), e.getMessage()); + throw new BadOpaqueTokenException(INVALID_TOKEN); + } + if (body == null) { + logger.warn("Refused token: the user info is empty"); + throw new BadOpaqueTokenException(INVALID_TOKEN); + } + return new LinkedHashMap<>(body); + } +} diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiController.java b/src/main/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiController.java index a222196d..718b7e4a 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiController.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiController.java @@ -11,7 +11,6 @@ import org.springframework.http.ResponseEntity; import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContextHolder; -import org.springframework.security.oauth2.provider.OAuth2Authentication; import org.springframework.stereotype.Controller; import org.springframework.web.bind.WebDataBinder; import org.springframework.web.bind.annotation.InitBinder; @@ -151,12 +150,14 @@ private static List asList(String[] values) { return values == null ? null : Arrays.asList(values); } + /** + * The authenticated user (with the auth profile), or a user without groups named after the + * anonymous principal (noauth) + */ private User getUser() { Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); - OAuth2Authentication oauth = null; - if (authentication instanceof OAuth2Authentication) { - oauth = (OAuth2Authentication) authentication; - return (User) oauth.getPrincipal(); + if (authentication.getPrincipal() instanceof User) { + return (User) authentication.getPrincipal(); } return User.builder(authentication.getName()).build(); } diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java new file mode 100644 index 00000000..b82f183c --- /dev/null +++ b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java @@ -0,0 +1,91 @@ +package uk.ac.ebi.tsc.tesk.config.security; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import java.util.Collections; +import org.junit.Test; +import org.springframework.boot.autoconfigure.http.HttpMessageConverters; +import org.springframework.boot.web.servlet.error.DefaultErrorAttributes; +import org.springframework.http.converter.json.MappingJackson2HttpMessageConverter; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.security.authentication.InsufficientAuthenticationException; +import org.springframework.security.core.AuthenticationException; +import org.springframework.security.oauth2.server.resource.InvalidBearerTokenException; + +/** The answers to requests without a valid token */ +public class CustomAuthenticationEntryPointTest { + + private final CustomAuthenticationEntryPoint entryPoint = + new CustomAuthenticationEntryPoint( + new DefaultErrorAttributes(), + new HttpMessageConverters( + false, + Collections.singletonList(new MappingJackson2HttpMessageConverter()))); + + private MockHttpServletResponse commence(String accept, AuthenticationException e) + throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/tasks"); + if (accept != null) { + request.addHeader("Accept", accept); + } + MockHttpServletResponse response = new MockHttpServletResponse(); + entryPoint.commence(request, response, e); + return response; + } + + @Test + public void wwwAuthenticate_escapesQuotes() { + assertEquals( + "Bearer realm=\"oauth\", error=\"invalid_token\"," + + " error_description=\"a \\\"b\\\" \\\\c\"", + CustomAuthenticationEntryPoint.wwwAuthenticate("invalid_token", "a \"b\" \\c")); + } + + @Test + public void invalidToken_isDescribed() throws Exception { + MockHttpServletResponse response = + commence(null, new InvalidBearerTokenException("Invalid access token")); + assertEquals(401, response.getStatus()); + assertEquals( + "Bearer realm=\"oauth\", error=\"invalid_token\"," + + " error_description=\"Invalid access token\"", + response.getHeader("WWW-Authenticate")); + assertTrue(response.getContentAsString().contains("\"error\":\"invalid_token\"")); + assertTrue( + response.getContentAsString() + .contains("\"error_description\":\"Invalid access token\"")); + } + + @Test + public void jsonAccepted_isWritten() throws Exception { + MockHttpServletResponse response = + commence( + "text/html;q=0.5, application/json", + new InsufficientAuthenticationException("no token")); + assertEquals(401, response.getStatus()); + assertTrue(response.getContentType().startsWith("application/json")); + assertTrue(response.getContentAsString().contains("\"error\":\"unauthorized\"")); + } + + /** Without a converter for an accepted type, the answer has the status and headers only */ + @Test + public void nothingAccepted_hasNoBody() throws Exception { + MockHttpServletResponse response = + commence("text/html", new InsufficientAuthenticationException("no token")); + assertEquals(401, response.getStatus()); + assertEquals("", response.getContentAsString()); + assertEquals( + "Bearer realm=\"oauth\", error=\"unauthorized\", error_description=\"no token\"", + response.getHeader("WWW-Authenticate")); + } + + @Test + public void invalidAccept_isTakenAsAll() throws Exception { + MockHttpServletResponse response = + commence("not a media type", new InsufficientAuthenticationException("no token")); + assertEquals(401, response.getStatus()); + assertTrue(response.getContentAsString().contains("\"error\":\"unauthorized\"")); + } +} diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServicesTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServicesTest.java index d2f9f265..dfcab192 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServicesTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServicesTest.java @@ -41,9 +41,9 @@ import org.junit.Rule; import org.junit.Test; import org.springframework.boot.test.system.OutputCaptureRule; -import org.springframework.security.oauth2.common.exceptions.InvalidTokenException; -import org.springframework.security.oauth2.provider.OAuth2Authentication; -import org.springframework.security.oauth2.provider.token.ResourceServerTokenServices; +import org.springframework.security.oauth2.core.OAuth2AuthenticatedPrincipal; +import org.springframework.security.oauth2.server.resource.introspection.BadOpaqueTokenException; +import org.springframework.security.oauth2.server.resource.introspection.OpaqueTokenIntrospector; import uk.ac.ebi.tsc.tesk.k8s.convert.CreatorLabels; /** @@ -61,8 +61,8 @@ public class ServiceClientTokenServicesTest { @Rule public WireMockRule idp = new WireMockRule(options().dynamicPort()); @Rule public OutputCaptureRule output = new OutputCaptureRule(); - private ResourceServerTokenServices userInfo; - private OAuth2Authentication userAuthentication; + private OpaqueTokenIntrospector userInfo; + private OAuth2AuthenticatedPrincipal userPrincipal; private AuthorisationProperties properties; @BeforeClass @@ -75,9 +75,9 @@ public static void generateKeys() throws JoseException { @Before public void setUp() { - userInfo = mock(ResourceServerTokenServices.class); - userAuthentication = mock(OAuth2Authentication.class); - when(userInfo.loadAuthentication(anyString())).thenReturn(userAuthentication); + userInfo = mock(OpaqueTokenIntrospector.class); + userPrincipal = mock(OAuth2AuthenticatedPrincipal.class); + when(userInfo.introspect(anyString())).thenReturn(userPrincipal); properties = new AuthorisationProperties(); AuthorisationProperties.ServiceClients settings = properties.getServiceClients(); settings.setEnabled(true); @@ -143,20 +143,22 @@ private static String sign(JwtClaims claims) throws JoseException { } private User accepted(String token) { - OAuth2Authentication authentication = services().loadAuthentication(token); - verify(userInfo, never()).loadAuthentication(anyString()); - return (User) authentication.getPrincipal(); + OAuth2AuthenticatedPrincipal principal = services().introspect(token); + verify(userInfo, never()).introspect(anyString()); + assertEquals(principal.getName(), ((AuthenticatedUser) principal).getUser().getUsername()); + assertTrue(principal.getAuthorities().isEmpty()); + return ((AuthenticatedUser) principal).getUser(); } private void refused(String token) { ServiceClientTokenServices services = services(); - assertThrows(InvalidTokenException.class, () -> services.loadAuthentication(token)); - verify(userInfo, never()).loadAuthentication(anyString()); + assertThrows(BadOpaqueTokenException.class, () -> services.introspect(token)); + verify(userInfo, never()).introspect(anyString()); } private void passedOn(String token) { - assertSame(userAuthentication, services().loadAuthentication(token)); - verify(userInfo).loadAuthentication(token); + assertSame(userPrincipal, services().introspect(token)); + verify(userInfo).introspect(token); } @Test @@ -287,15 +289,14 @@ public void symmetricSignature_isRefused() throws Exception { public void unknownKeyId_refreshesTheKeys() throws Exception { ServiceClientTokenServices services = new ServiceClientTokenServices(userInfo, properties, 0); - services.loadAuthentication(sign(claims(CLIENT))); + services.introspect(sign(claims(CLIENT))); // the issuer rotates its keys jwks(key, otherKey); - OAuth2Authentication authentication = - services.loadAuthentication( - sign(claims(CLIENT), otherKey, otherKey.getKeyId(), "at+jwt")); + OAuth2AuthenticatedPrincipal principal = + services.introspect(sign(claims(CLIENT), otherKey, otherKey.getKeyId(), "at+jwt")); - assertEquals(CLIENT, ((User) authentication.getPrincipal()).getUsername()); + assertEquals(CLIENT, ((AuthenticatedUser) principal).getUser().getUsername()); idp.verify(2, getRequestedFor(urlEqualTo("/oidc/jwk"))); // the discovery document is read once idp.verify( @@ -312,10 +313,10 @@ public void keyIdNotInTheKeySet_isRefused() throws Exception { @Test public void unknownKeyIds_refreshTheKeysAtMostOncePerInterval() throws Exception { ServiceClientTokenServices services = services(); - services.loadAuthentication(sign(claims(CLIENT))); + services.introspect(sign(claims(CLIENT))); String unknown = sign(claims(CLIENT), otherKey, otherKey.getKeyId(), "at+jwt"); for (int i = 0; i < 3; i++) { - assertThrows(InvalidTokenException.class, () -> services.loadAuthentication(unknown)); + assertThrows(BadOpaqueTokenException.class, () -> services.introspect(unknown)); } idp.verify(1, getRequestedFor(urlEqualTo("/oidc/jwk"))); } @@ -394,12 +395,6 @@ public void discoveryUnavailable_isRefused() throws Exception { refused(sign(claims(CLIENT))); } - @Test - public void readAccessToken_isPassedOn() { - services().readAccessToken("token"); - verify(userInfo).readAccessToken("token"); - } - @Test public void settingsWithoutIssuer_failAtStartup() { properties.getServiceClients().setIssuer(" "); @@ -438,7 +433,7 @@ public void failedDiscovery_isNotRetriedWithinTheInterval() throws Exception { ServiceClientTokenServices services = services(); String token = sign(claims(CLIENT)); for (int i = 0; i < 3; i++) { - assertThrows(InvalidTokenException.class, () -> services.loadAuthentication(token)); + assertThrows(BadOpaqueTokenException.class, () -> services.introspect(token)); } idp.verify( 1, @@ -454,10 +449,10 @@ public void failedDiscovery_isRetriedAfterTheInterval() throws Exception { ServiceClientTokenServices services = new ServiceClientTokenServices(userInfo, properties, 0); String token = sign(claims(CLIENT)); - assertThrows(InvalidTokenException.class, () -> services.loadAuthentication(token)); + assertThrows(BadOpaqueTokenException.class, () -> services.introspect(token)); discovery(issuer()); - services.loadAuthentication(token); + services.introspect(token); } @Test @@ -474,17 +469,17 @@ public void emptyKeySet_isRefused() throws Exception { public void failingRefresh_isThrottled() throws Exception { ServiceClientTokenServices services = new ServiceClientTokenServices(userInfo, properties, 300); - services.loadAuthentication(sign(claims(CLIENT))); + services.introspect(sign(claims(CLIENT))); Thread.sleep(400); idp.givenThat(get("/oidc/jwk").willReturn(aResponse().withStatus(503))); String unknown = sign(claims(CLIENT), otherKey, otherKey.getKeyId(), "at+jwt"); for (int i = 0; i < 3; i++) { - assertThrows(InvalidTokenException.class, () -> services.loadAuthentication(unknown)); + assertThrows(BadOpaqueTokenException.class, () -> services.introspect(unknown)); } // the first fetch, and one refresh for the unknown key IDs idp.verify(2, getRequestedFor(urlEqualTo("/oidc/jwk"))); // the known key still works - services.loadAuthentication(sign(claims(CLIENT))); + services.introspect(sign(claims(CLIENT))); } /** A failed or empty key set isn't fetched again within the interval, also without discovery */ @@ -501,7 +496,7 @@ public void failedKeySet_isNotRefetchedWithinTheInterval() throws Exception { } ServiceClientTokenServices services = services(); for (int i = 0; i < 3; i++) { - assertThrows(InvalidTokenException.class, () -> services.loadAuthentication(token)); + assertThrows(BadOpaqueTokenException.class, () -> services.introspect(token)); } idp.verify(1, getRequestedFor(urlEqualTo("/oidc/jwk"))); } @@ -534,17 +529,16 @@ public void clientClaimNotAString_isPassedOn() throws Exception { /** A user whose sub is a listed client's ID would share its tasks, so is refused */ @Test public void userWithClientId_isRefused() { - when(userAuthentication.getPrincipal()).thenReturn(User.builder(CLIENT).build()); + when(userPrincipal.getName()).thenReturn(CLIENT); ServiceClientTokenServices services = services(); - assertThrows(InvalidTokenException.class, () -> services.loadAuthentication("opaque")); - verify(userInfo).loadAuthentication("opaque"); + assertThrows(BadOpaqueTokenException.class, () -> services.introspect("opaque")); + verify(userInfo).introspect("opaque"); } @Test public void otherUser_isPassedOn() { - when(userAuthentication.getPrincipal()) - .thenReturn(User.builder("user@example.org").build()); + when(userPrincipal.getName()).thenReturn("user@example.org"); passedOn("opaque"); } diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java new file mode 100644 index 00000000..c4c76636 --- /dev/null +++ b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java @@ -0,0 +1,214 @@ +package uk.ac.ebi.tsc.tesk.config.security; + +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.equalTo; +import static com.github.tomakehurst.wiremock.client.WireMock.get; +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.okJson; +import static com.github.tomakehurst.wiremock.client.WireMock.serverError; +import static com.github.tomakehurst.wiremock.client.WireMock.unauthorized; +import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import com.github.tomakehurst.wiremock.junit.WireMockRule; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.springframework.boot.test.system.OutputCaptureRule; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.oauth2.core.DefaultOAuth2AuthenticatedPrincipal; +import org.springframework.security.oauth2.core.OAuth2AuthenticatedPrincipal; +import org.springframework.security.oauth2.server.resource.InvalidBearerTokenException; +import org.springframework.security.oauth2.server.resource.introspection.BadOpaqueTokenException; + +/** Validation of tokens at the user info endpoint (WireMocked) */ +public class UserInfoTokenIntrospectorTest { + + private static final String TOKEN = "secret-token"; + private static final String PREFIX = "urn:example:group:"; + + @Rule public WireMockRule userInfo = new WireMockRule(options().dynamicPort()); + @Rule public OutputCaptureRule output = new OutputCaptureRule(); + + private UserInfoTokenIntrospector introspector; + + @Before + public void setUp() { + AuthorisationProperties properties = new AuthorisationProperties(); + properties.setBaseGroupPrefix(PREFIX + "BASE:"); + properties.setAdminSubgroup("ADMIN"); + properties.setAdminGroupFull(PREFIX + "BASE:ADMIN"); + GroupNamesSecurityExtractor groups = new GroupNamesSecurityExtractor("groups", PREFIX); + introspector = + new UserInfoTokenIntrospector( + "http://localhost:" + userInfo.port() + "/userinfo", + new ElixirPrincipalExtractor(groups, properties), + groups); + } + + private void answer(String json) { + userInfo.givenThat(get("/userinfo").willReturn(okJson(json))); + } + + private void refused() { + assertThrows(BadOpaqueTokenException.class, () -> introspector.introspect(TOKEN)); + // the refusal is logged, without the token (other loggers, e.g. of WireMock, may log it) + List logged = + Arrays.stream(output.getOut().split("\n")) + .filter( + line -> + line.contains( + UserInfoTokenIntrospector.class.getSimpleName())) + .collect(Collectors.toList()); + assertFalse(logged.isEmpty()); + assertThat(String.join("\n", logged), not(containsString(TOKEN))); + } + + @Test + public void validToken_givesTheUserAndGroups() { + answer( + "{\"sub\":\"user@example.org\",\"name\":\"A User\",\"groups\":[\"" + + PREFIX + + "BASE:TEAM\",\"" + + PREFIX + + "BASE:ADMIN\"]}"); + + OAuth2AuthenticatedPrincipal principal = introspector.introspect(TOKEN); + + User user = ((AuthenticatedUser) principal).getUser(); + assertEquals("user@example.org", user.getUsername()); + assertEquals("user@example.org", principal.getName()); + assertEquals("A User", user.getName()); + assertTrue(user.isTeskAdmin()); + assertTrue(user.isGroupMember("TEAM")); + assertThat( + principal.getAuthorities().stream() + .map(GrantedAuthority::getAuthority) + .collect(Collectors.toList()), + containsInAnyOrder(PREFIX + "BASE:TEAM", PREFIX + "BASE:ADMIN")); + assertEquals("A User", principal.getAttribute("name")); + userInfo.verify( + getRequestedFor(urlEqualTo("/userinfo")) + .withHeader("Authorization", equalTo("Bearer " + TOKEN)) + .withHeader("Accept", equalTo("application/json"))); + } + + /** Without the groups claim, the user has the authority of a user without groups */ + @Test + public void noGroups_givesTheUserAuthority() { + answer("{\"sub\":\"123\"}"); + + OAuth2AuthenticatedPrincipal principal = introspector.introspect(TOKEN); + + assertEquals(PREFIX + ":USER", principal.getAuthorities().iterator().next().getAuthority()); + assertFalse(((AuthenticatedUser) principal).getUser().isMember()); + } + + /** The authentication of the request has the user as principal, without the token */ + @Test + public void authentication_hasTheUser() { + answer("{\"sub\":\"123\",\"groups\":[\"" + PREFIX + "BASE\"]}"); + OAuth2AuthenticatedPrincipal principal = introspector.introspect(TOKEN); + + Authentication authentication = AuthenticatedUser.toAuthentication(TOKEN, principal); + + assertTrue(authentication.isAuthenticated()); + assertEquals(((AuthenticatedUser) principal).getUser(), authentication.getPrincipal()); + assertEquals("123", authentication.getName()); + assertEquals(principal.getAuthorities(), authentication.getAuthorities()); + assertFalse(String.valueOf(authentication.getCredentials()).contains(TOKEN)); + } + + @Test + public void authentication_ofAnotherPrincipal_isRefused() { + OAuth2AuthenticatedPrincipal other = + new DefaultOAuth2AuthenticatedPrincipal( + Collections.singletonMap("sub", "123"), Collections.emptyList()); + assertThrows( + InvalidBearerTokenException.class, + () -> AuthenticatedUser.toAuthentication(TOKEN, other)); + } + + @Test + public void refusedByUserInfo_isRefused() { + userInfo.givenThat(get("/userinfo").willReturn(unauthorized())); + refused(); + } + + @Test + public void userInfoFails_isRefused() { + userInfo.givenThat(get("/userinfo").willReturn(serverError())); + refused(); + } + + @Test + public void userInfoUnreachable_isRefused() { + userInfo.stop(); + refused(); + } + + @Test + public void userInfoWithError_isRefused() { + answer("{\"sub\":\"123\",\"error\":\"invalid_token\"}"); + refused(); + } + + @Test + public void userInfoWithoutSub_isRefused() { + answer("{\"name\":\"A User\"}"); + refused(); + } + + @Test + public void userInfoNotAnObject_isRefused() { + answer("[\"sub\"]"); + refused(); + } + + @Test + public void userInfoEmpty_isRefused() { + userInfo.givenThat( + get("/userinfo") + .willReturn( + aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json"))); + refused(); + } + + @Test + public void userInfoNotJson_isRefused() { + userInfo.givenThat( + get("/userinfo") + .willReturn( + aResponse() + .withHeader("Content-Type", "text/html") + .withBody(""))); + refused(); + } + + @Test + public void withoutUserInfoUri_doesNotStart() { + GroupNamesSecurityExtractor groups = new GroupNamesSecurityExtractor("groups", PREFIX); + ElixirPrincipalExtractor extractor = + new ElixirPrincipalExtractor(groups, new AuthorisationProperties()); + IllegalStateException e = + assertThrows( + IllegalStateException.class, + () -> new UserInfoTokenIntrospector(" ", extractor, groups)); + assertThat(e.getMessage(), containsString("security.oauth2.resource.user-info-uri")); + } +} diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiControllerTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiControllerTest.java index c26e47ec..aba45e1b 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiControllerTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiControllerTest.java @@ -35,19 +35,14 @@ import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest; import org.springframework.boot.test.context.TestConfiguration; import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.context.annotation.Bean; import org.springframework.http.MediaType; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; -import org.springframework.security.core.AuthenticationException; -import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.security.core.authority.AuthorityUtils; -import org.springframework.security.oauth2.common.OAuth2AccessToken; -import org.springframework.security.oauth2.common.exceptions.InvalidTokenException; -import org.springframework.security.oauth2.config.annotation.web.configuration.EnableResourceServer; -import org.springframework.security.oauth2.config.annotation.web.configuration.ResourceServerConfigurerAdapter; -import org.springframework.security.oauth2.config.annotation.web.configurers.ResourceServerSecurityConfigurer; -import org.springframework.security.oauth2.provider.OAuth2Authentication; -import org.springframework.security.oauth2.provider.OAuth2Request; -import org.springframework.security.oauth2.provider.token.ResourceServerTokenServices; +import org.springframework.security.oauth2.core.DefaultOAuth2AuthenticatedPrincipal; +import org.springframework.security.web.SecurityFilterChain; +import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.junit4.SpringRunner; import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.ResultActions; @@ -68,38 +63,41 @@ @RunWith(SpringRunner.class) @WebMvcTest(TasksApiController.class) +// the auth profile, as the security configuration below stands for it (and the noauth one, which +// application.properties sets, has a security configuration of its own) +@ActiveProfiles("auth") public class TasksApiControllerTest { + /** + * Accepts any bearer token, as the user admin, with the token as comma-separated authorities + */ @TestConfiguration - @EnableResourceServer - static class ResourceServerConfiguration extends ResourceServerConfigurerAdapter { - - @Override - public void configure(ResourceServerSecurityConfigurer security) { - security.tokenServices(tokenServices()); - } - - ResourceServerTokenServices tokenServices() { - return new ResourceServerTokenServices() { - - @Override - public OAuth2Authentication loadAuthentication(String accessToken) - throws AuthenticationException, InvalidTokenException { - Object principal = User.builder("admin").build(); - List authorities = - AuthorityUtils.commaSeparatedStringToAuthorityList(accessToken); - OAuth2Request request = - new OAuth2Request(null, null, null, true, null, null, null, null, null); - UsernamePasswordAuthenticationToken token = - new UsernamePasswordAuthenticationToken(principal, "N/A", authorities); - return new OAuth2Authentication(request, token); - } - - @Override - public OAuth2AccessToken readAccessToken(String accessToken) { - return null; - } - }; + static class ResourceServerConfiguration { + + @Bean + SecurityFilterChain filterChain(HttpSecurity http) throws Exception { + http.csrf() + .disable() + .authorizeRequests() + .anyRequest() + .authenticated() + .and() + .oauth2ResourceServer() + .opaqueToken() + .introspector( + token -> + new DefaultOAuth2AuthenticatedPrincipal( + "admin", + Collections.singletonMap("sub", "admin"), + AuthorityUtils.commaSeparatedStringToAuthorityList( + token))) + .authenticationConverter( + (token, principal) -> + new UsernamePasswordAuthenticationToken( + User.builder("admin").build(), + "N/A", + principal.getAuthorities())); + return http.build(); } } From a84c6996d121dab61c1f9d01f249bbd381761450 Mon Sep 17 00:00:00 2001 From: Alex Kanitz Date: Sat, 14 Nov 2026 00:50:33 +0100 Subject: [PATCH 2/6] docs: describe token validation answers Describe how tokens are taken and validated with the resource server of Spring Security, the 401 answers (WWW-Authenticate and body), that SECURITY_OAUTH2_RESOURCE_USER_INFO_URI keeps its name, and the changes that clients may notice in the upgrade notes. Co-Authored-By: Claude Opus 5.5 --- docs/access-control.md | 22 ++++++++++++++++++++-- docs/api.md | 2 +- docs/configuration.md | 2 +- docs/upgrading.md | 19 +++++++++++++++++++ 4 files changed, 41 insertions(+), 4 deletions(-) diff --git a/docs/access-control.md b/docs/access-control.md index b78b692a..14638211 100644 --- a/docs/access-control.md +++ b/docs/access-control.md @@ -45,10 +45,28 @@ with the authorisation code grant, and with the redirect URI provider must issue tokens to it without a client secret and without PKCE. ### Token validation -TESK API validates the token by hitting Elixir AAI `user_info` endpoint on each request and extracting information returned from that endpoint. If user info is returned without error, token is considered valid. Invalid token or lack of thereof in `authorisation: Bearer` header will result in `HTTP 401 Unauthorized` error. Tokens of allow-listed services are checked locally instead, if that is on (see [Tokens of services](#tokens-of-services)). +TESK API validates the token by hitting Elixir AAI `user_info` endpoint on each request and extracting information returned from that endpoint, using the resource server of Spring Security. A token is valid if the endpoint answers it (`200 OK`) with a JSON object that has a `sub` and no `error`. An invalid token, or lack of thereof in the `Authorization: Bearer` header, will result in `HTTP 401 Unauthorized` error. Tokens of allow-listed services are checked locally instead, if that is on (see [Tokens of services](#tokens-of-services)). + +TESK API takes the token from the `Authorization` header only (`Bearer `, as in [RFC 6750, section 2.1](https://datatracker.ietf.org/doc/html/rfc6750#section-2.1)), not from an `access_token` parameter in the URL or in a form. A header whose token is empty or has characters that tokens can't have (e.g. spaces) is refused without a call to the user info endpoint. + +The answer `401 Unauthorized` has a `WWW-Authenticate` header and a JSON body with the OAuth 2.0 error code and description: + +* without a token: `WWW-Authenticate: Bearer realm="oauth", error="unauthorized", error_description="Full authentication is required to access this resource"`; +* with an invalid token: `error="invalid_token"`, with the description `Invalid access token` (`Bearer token is malformed` for a header as above; `Invalid access token of a service client` or `User ID of a service client` for [tokens of services](#tokens-of-services)). The token itself is never part of the answer. + +```json +{ + "timestamp" : "2026-11-14T10:00:00.000+00:00", + "status" : 401, + "error" : "invalid_token", + "message" : "Invalid access token", + "path" : "/ga4gh/tes/v1/tasks", + "error_description" : "Invalid access token" +} +``` ### Integrating with a different OIDC provider -We have not tested TESK API integration with a different OIDC provider than Elixir AAI, but since standard Spring OAuth library is used, it should be possible (just point `security.oauth2.resource.user-info-uri`, or `SECURITY_OAUTH2_RESOURCE_USER_INFO_URI`, to a proper URL), provided that the other Authorization Server provides information about the user in the same format that Elixir does. Otherwise changes to [GroupNamesSecurityExtractor](https://github.com/elixir-cloud-aai/tesk-api/blob/dev/src/main/java/uk/ac/ebi/tsc/tesk/config/security/GroupNamesSecurityExtractor.java) and [ElixirPrincipalExtractor](https://github.com/elixir-cloud-aai/tesk-api/blob/dev/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ElixirPrincipalExtractor.java) will be necessary. +We have not tested TESK API integration with a different OIDC provider than Elixir AAI, but since tokens are validated at the standard user info endpoint of OpenID Connect, it should be possible (just point `security.oauth2.resource.user-info-uri`, or `SECURITY_OAUTH2_RESOURCE_USER_INFO_URI`, to a proper URL), provided that the other Authorization Server provides information about the user in the same format that Elixir does. Otherwise changes to [GroupNamesSecurityExtractor](https://github.com/elixir-cloud-aai/tesk-api/blob/dev/src/main/java/uk/ac/ebi/tsc/tesk/config/security/GroupNamesSecurityExtractor.java) and [ElixirPrincipalExtractor](https://github.com/elixir-cloud-aai/tesk-api/blob/dev/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ElixirPrincipalExtractor.java) will be necessary. ## Authorisation TESK API uses Elixir AAI group membership to authorise users (and optionally a claim, see [Authorisation without groups](#authorisation-without-groups)). Elixir recognises group managers and members, but only includes information about the latter in `user_info` endpoint response. Therefore a nested structure of groups is used by us to reflect different roles of users. diff --git a/docs/api.md b/docs/api.md index c2c94052..0ef4fd15 100644 --- a/docs/api.md +++ b/docs/api.md @@ -296,7 +296,7 @@ Status code | Meaning --- | --- 200 | OK: successful request 400 | Bad request: validation error, or cancellation of a task that doesn't run -401 | Unauthorized: no access token in the request, or an invalid token +401 | Unauthorized: no access token in the request, or an invalid token (see [Token validation](access-control.md#token-validation) for the `WWW-Authenticate` header and the body) 403 | Forbidden: the user may not do this (usually, not in the required group, or without the required claim) 404 | Not found: no task with this ID 500 | Something else went wrong (possibly the configuration) diff --git a/docs/configuration.md b/docs/configuration.md index ad43ee23..38b99fe4 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -28,7 +28,7 @@ Environment variable | Meaning `TESK_API_TASKMASTER_EXECUTOR_SECRET_MOUNT_PATH` | Where that secret is mounted. Default: `/secret`. `TESK_API_SERVICE_INFO_LOCATION` | The YAML file of [service info](#service-info). Default: the built-in `classpath:service-info.yaml`. `TESK_API_SERVICE_INFO_PUBLIC` | With the `auth` profile: `true` (default) lets `GET` and `HEAD /service-info` answer without a token; `false` makes them need a valid token (no group membership). No effect with `noauth`, where all endpoints are open. Since 1.2.0. -`SECURITY_OAUTH2_RESOURCE_USER_INFO_URI` | User info endpoint of the OpenID Connect provider, which validates tokens; see [Access control](access-control.md#token-validation). Default: ELIXIR AAI. +`SECURITY_OAUTH2_RESOURCE_USER_INFO_URI` | User info endpoint of the OpenID Connect provider, which validates tokens; see [Access control](access-control.md#token-validation). Required with the `auth` profile (TESK API doesn't start if it is blank). The name comes from Spring Security OAuth, which TESK API no longer uses; it is the only one of its `SECURITY_OAUTH2_*` settings that TESK API reads. Default: ELIXIR AAI. `TESK_API_AUTHORISATION_*` | Authorisation by group membership: `TESK_API_AUTHORISATION_GROUPS_CLAIM` (default `eduperson_entitlement`), `…_GROUP_PREFIX`, `…_GROUP_SUFFIX`, `…_DELIMITER`, `…_PARENT_GROUP`, `…_ENV_SUBGROUP`, `…_BASE_GROUP` and `…_ADMIN_SUBGROUP`; see [Access control](access-control.md#configuration) for their meanings and defaults. `TESK_API_AUTHORISATION_LEGACY_USER_LABELS` | `true`: tasks created by earlier versions, which have only the old creator label, belong to their creators again; `false` (default): only team admins and super admins see them. See [Access control](access-control.md#task-creators). In the Helm chart of TESK: `auth.legacyUserLabels` (from chart 0.3.0). `TESK_API_AUTHORISATION_CLAIM_*` | Authorisation without groups, by a claim of the user info; off by default. Turning it on lets everyone with one of the configured values run tasks; see [Access control](access-control.md#authorisation-without-groups) for the settings and their security implications. diff --git a/docs/upgrading.md b/docs/upgrading.md index 263acdaa..aa85e917 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -29,6 +29,25 @@ notice changed answers: are more (see [Listing tasks](api.md#listing-tasks)). Before, it returned all tasks at once. Clients that don't pass `page_size` must follow `next_page_token`, or pass a `page_size` of up to 2048. +- **Token validation.** TESK API validates access tokens with the resource + server of Spring Security instead of Spring Security OAuth, which is end + of life. Deployments need no change: `SECURITY_OAUTH2_RESOURCE_USER_INFO_URI` + keeps its name and meaning (and is required with the `auth` profile, as + before). TESK API reads none of the other `security.oauth2.*` settings of + Spring Security OAuth (e.g. `security.oauth2.resource.token-info-uri`), + which it never documented. Clients may notice (see [Token + validation](access-control.md#token-validation)): + - The token is taken from the `Authorization: Bearer` header only; an + `access_token` parameter in the URL or in a form is ignored, so such + requests get `401 Unauthorized`. Send the token in the header. + - For an invalid token, `error_description` and `message` of the + `401` say `Invalid access token` instead of repeating the token. + - A header with an empty token, or with characters that tokens can't + have (e.g. spaces), is refused with `401` (`invalid_token`, + `Bearer token is malformed`) without a call to the user info + endpoint. Before, it was passed on to the endpoint. + - A token whose user info has no `sub` is refused with `401` instead + of failing with `500 Internal Server Error`. ## To 1.2.0 From d0e1b77292e49d64adbec282ad8bf8ad0391e21b Mon Sep 17 00:00:00 2001 From: Alex Kanitz Date: Sat, 14 Nov 2026 01:08:58 +0100 Subject: [PATCH 3/6] fix: harden user info calls after review Findings of the pre-push review: - Time out calls to the user info endpoint (10 s connect, 30 s read), so that a hanging endpoint can't hold request threads; a timeout refuses the token. - Refuse to start unless the user info URI is an absolute http(s) URL. - Log the status of a refused user info call, not its answer, and only the exception class for unreadable answers. - Test AuthenticatedUser on its own, parse the JSON bodies in the entry point tests, and test a blank message. - Javadoc that describes the current behaviour; why ServiceClientTokenServices keeps its name (the documented logger). - Docs: allowed token characters, timeouts, URL check, and the 401 without a body for requests that accept no JSON. Co-Authored-By: Claude Opus 5.5 --- docs/access-control.md | 4 +- docs/configuration.md | 2 +- docs/upgrading.md | 8 ++ .../config/security/AuthenticatedUser.java | 9 +- .../security/ElixirPrincipalExtractor.java | 4 +- .../security/GroupNamesSecurityExtractor.java | 4 +- .../security/ResourceServerConfiguration.java | 2 +- .../security/ServiceClientTokenServices.java | 3 + .../security/UserInfoTokenIntrospector.java | 54 ++++++++++-- .../tsc/tesk/tes/api/TasksApiController.java | 4 +- .../security/AuthenticatedUserTest.java | 82 +++++++++++++++++++ .../CustomAuthenticationEntryPointTest.java | 47 +++++++++-- .../UserInfoTokenIntrospectorTest.java | 44 ++++------ 13 files changed, 210 insertions(+), 57 deletions(-) create mode 100644 src/test/java/uk/ac/ebi/tsc/tesk/config/security/AuthenticatedUserTest.java diff --git a/docs/access-control.md b/docs/access-control.md index 14638211..fc9dc56b 100644 --- a/docs/access-control.md +++ b/docs/access-control.md @@ -45,9 +45,9 @@ with the authorisation code grant, and with the redirect URI provider must issue tokens to it without a client secret and without PKCE. ### Token validation -TESK API validates the token by hitting Elixir AAI `user_info` endpoint on each request and extracting information returned from that endpoint, using the resource server of Spring Security. A token is valid if the endpoint answers it (`200 OK`) with a JSON object that has a `sub` and no `error`. An invalid token, or lack of thereof in the `Authorization: Bearer` header, will result in `HTTP 401 Unauthorized` error. Tokens of allow-listed services are checked locally instead, if that is on (see [Tokens of services](#tokens-of-services)). +TESK API validates the token by hitting Elixir AAI `user_info` endpoint on each request and extracting information returned from that endpoint, using the resource server of Spring Security. A token is valid if the endpoint answers it (`200 OK`) with a JSON object that has a `sub` and no `error`. TESK API waits at most 10 seconds to connect to the endpoint and 30 seconds for its answer; if the endpoint can't be reached or doesn't answer in time, the token is refused like an invalid one. An invalid token, or lack of thereof in the `Authorization: Bearer` header, will result in `HTTP 401 Unauthorized` error. Tokens of allow-listed services are checked locally instead, if that is on (see [Tokens of services](#tokens-of-services)). -TESK API takes the token from the `Authorization` header only (`Bearer `, as in [RFC 6750, section 2.1](https://datatracker.ietf.org/doc/html/rfc6750#section-2.1)), not from an `access_token` parameter in the URL or in a form. A header whose token is empty or has characters that tokens can't have (e.g. spaces) is refused without a call to the user info endpoint. +TESK API takes the token from the `Authorization` header only (`Bearer `, as in [RFC 6750, section 2.1](https://datatracker.ietf.org/doc/html/rfc6750#section-2.1)), not from an `access_token` parameter in the URL or in a form. A header whose token is empty or has characters that bearer tokens can't have (anything but letters, digits and `-._~+/`, with `=` only at the end; e.g. spaces) is refused without a call to the user info endpoint. The answer `401 Unauthorized` has a `WWW-Authenticate` header and a JSON body with the OAuth 2.0 error code and description: diff --git a/docs/configuration.md b/docs/configuration.md index 38b99fe4..cfd75924 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -28,7 +28,7 @@ Environment variable | Meaning `TESK_API_TASKMASTER_EXECUTOR_SECRET_MOUNT_PATH` | Where that secret is mounted. Default: `/secret`. `TESK_API_SERVICE_INFO_LOCATION` | The YAML file of [service info](#service-info). Default: the built-in `classpath:service-info.yaml`. `TESK_API_SERVICE_INFO_PUBLIC` | With the `auth` profile: `true` (default) lets `GET` and `HEAD /service-info` answer without a token; `false` makes them need a valid token (no group membership). No effect with `noauth`, where all endpoints are open. Since 1.2.0. -`SECURITY_OAUTH2_RESOURCE_USER_INFO_URI` | User info endpoint of the OpenID Connect provider, which validates tokens; see [Access control](access-control.md#token-validation). Required with the `auth` profile (TESK API doesn't start if it is blank). The name comes from Spring Security OAuth, which TESK API no longer uses; it is the only one of its `SECURITY_OAUTH2_*` settings that TESK API reads. Default: ELIXIR AAI. +`SECURITY_OAUTH2_RESOURCE_USER_INFO_URI` | User info endpoint of the OpenID Connect provider, which validates tokens; see [Access control](access-control.md#token-validation). Required with the `auth` profile: TESK API doesn't start unless it is an absolute `http` or `https` URL. The name comes from Spring Security OAuth, which TESK API no longer uses; it is the only one of its `SECURITY_OAUTH2_*` settings that TESK API reads. Default: ELIXIR AAI. `TESK_API_AUTHORISATION_*` | Authorisation by group membership: `TESK_API_AUTHORISATION_GROUPS_CLAIM` (default `eduperson_entitlement`), `…_GROUP_PREFIX`, `…_GROUP_SUFFIX`, `…_DELIMITER`, `…_PARENT_GROUP`, `…_ENV_SUBGROUP`, `…_BASE_GROUP` and `…_ADMIN_SUBGROUP`; see [Access control](access-control.md#configuration) for their meanings and defaults. `TESK_API_AUTHORISATION_LEGACY_USER_LABELS` | `true`: tasks created by earlier versions, which have only the old creator label, belong to their creators again; `false` (default): only team admins and super admins see them. See [Access control](access-control.md#task-creators). In the Helm chart of TESK: `auth.legacyUserLabels` (from chart 0.3.0). `TESK_API_AUTHORISATION_CLAIM_*` | Authorisation without groups, by a claim of the user info; off by default. Turning it on lets everyone with one of the configured values run tasks; see [Access control](access-control.md#authorisation-without-groups) for the settings and their security implications. diff --git a/docs/upgrading.md b/docs/upgrading.md index aa85e917..764391d7 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -48,6 +48,14 @@ notice changed answers: endpoint. Before, it was passed on to the endpoint. - A token whose user info has no `sub` is refused with `401` instead of failing with `500 Internal Server Error`. + - Without a token, a request that accepts no JSON (e.g. `Accept: + text/plain`) gets `401` with the `WWW-Authenticate` header and no + body, instead of another error status (e.g. `406 Not Acceptable`). + Operators may notice that TESK API waits at most 10 seconds to connect to + the user info endpoint and 30 seconds for its answer, and then refuses + the token (before, it waited without limit), and that it doesn't start + if `SECURITY_OAUTH2_RESOURCE_USER_INFO_URI` isn't an absolute `http` or + `https` URL. ## To 1.2.0 diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/AuthenticatedUser.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/AuthenticatedUser.java index 22e0377c..309e453b 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/AuthenticatedUser.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/AuthenticatedUser.java @@ -58,14 +58,17 @@ public String getName() { /** * The authentication of a request with a valid access token (an {@code - * OpaqueTokenAuthenticationConverter}): the {@link User} as principal, with the authorities; - * the token isn't kept in it + * OpaqueTokenAuthenticationConverter}): the {@link User} as principal, with the authorities + * + * @param accessToken the token, which that interface passes; deliberately not kept in the + * authentication + * @param principal the result of the introspector */ static Authentication toAuthentication( String accessToken, OAuth2AuthenticatedPrincipal principal) { if (!(principal instanceof AuthenticatedUser)) { // not expected: both introspectors return an AuthenticatedUser - throw new InvalidBearerTokenException("Invalid access token"); + throw new InvalidBearerTokenException(UserInfoTokenIntrospector.INVALID_TOKEN); } AuthenticatedUser authenticated = (AuthenticatedUser) principal; return new UsernamePasswordAuthenticationToken( diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ElixirPrincipalExtractor.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ElixirPrincipalExtractor.java index 44566b9f..9f3310c4 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ElixirPrincipalExtractor.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ElixirPrincipalExtractor.java @@ -12,8 +12,8 @@ import org.springframework.util.StringUtils; /** - * Custom principal extractor (puts all of the info from userinfo to custom {@link User} object; - * used by {@link UserInfoTokenIntrospector}) + * Custom principal extractor (puts all of the info from userinfo to custom {@link User} object). + * Used by {@link UserInfoTokenIntrospector}. * * @author Ania Niewielska */ diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/GroupNamesSecurityExtractor.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/GroupNamesSecurityExtractor.java index 3e635f9d..dd39af4e 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/GroupNamesSecurityExtractor.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/GroupNamesSecurityExtractor.java @@ -11,8 +11,8 @@ /** * Custom authorities extractor, that looks for groups element and transforms it into authorities. - * Heavily inspired by {@code FixedAuthoritiesExtractor} of Spring Security OAuth (used by {@link - * UserInfoTokenIntrospector}) + * Inspired by {@code FixedAuthoritiesExtractor} of Spring Security OAuth. Used by {@link + * UserInfoTokenIntrospector}. * * @author Ania Niewielska */ diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ResourceServerConfiguration.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ResourceServerConfiguration.java index a6ccc1d6..ef32f0f8 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ResourceServerConfiguration.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ResourceServerConfiguration.java @@ -73,7 +73,7 @@ public ResourceServerConfiguration( * required for it (tesk.api.service-info.public). A request with a token has it validated also * on these. Requests without a valid token get the JSON error responses and {@code * WWW-Authenticate} headers of {@link CustomAuthenticationEntryPoint}. Stateless and without - * CSRF protection (no cookies are used), as the resource server of Spring Security OAuth was. + * CSRF protection, as no cookies are used. */ @Bean public SecurityFilterChain resourceServerFilterChain( diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServices.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServices.java index 61c571bd..97c5ce74 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServices.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServices.java @@ -54,6 +54,9 @@ * User} of its own (the client's ID, no groups, {@link User#isServiceClient()}). * *

Logs the client's ID for accepted and refused tokens, never the token. + * + *

An {@link OpaqueTokenIntrospector}; the name stays from before, so that the documented logger + * name ({@code uk.ac.ebi.tsc.tesk.config.security.ServiceClientTokenServices}) keeps working. */ public class ServiceClientTokenServices implements OpaqueTokenIntrospector { diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java index 3f7f1119..c47c099b 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java @@ -13,22 +13,26 @@ import org.springframework.http.HttpMethod; import org.springframework.http.MediaType; import org.springframework.http.RequestEntity; +import org.springframework.http.client.SimpleClientHttpRequestFactory; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.oauth2.core.OAuth2AuthenticatedPrincipal; import org.springframework.security.oauth2.server.resource.introspection.BadOpaqueTokenException; import org.springframework.security.oauth2.server.resource.introspection.OpaqueTokenIntrospector; import org.springframework.util.StringUtils; +import org.springframework.web.client.ResourceAccessException; +import org.springframework.web.client.RestClientResponseException; import org.springframework.web.client.RestOperations; import org.springframework.web.client.RestTemplate; /** * Validates access tokens at the user info endpoint of the OpenID Connect provider ({@code - * security.oauth2.resource.user-info-uri}), on each request, as Spring Security OAuth's {@code - * UserInfoTokenServices} did before: a token is valid if the endpoint answers it with a JSON object - * that has a {@code sub} and no {@code error}. The user ({@link ElixirPrincipalExtractor}) and its - * authorities, the group names ({@link GroupNamesSecurityExtractor}), are taken from that answer. + * security.oauth2.resource.user-info-uri}), on each request: a token is valid if the endpoint + * answers it with a JSON object that has a {@code sub} and no {@code error}. Any failure (an error + * status, an unreachable endpoint, an answer that isn't such an object) refuses the token. The user + * ({@link ElixirPrincipalExtractor}) and its authorities, the group names ({@link + * GroupNamesSecurityExtractor}), are taken from that answer. * - *

Logs why a token was refused, never the token. + *

Logs why a token was refused, never the token or the answer of the endpoint. */ public class UserInfoTokenIntrospector implements OpaqueTokenIntrospector { @@ -37,6 +41,12 @@ public class UserInfoTokenIntrospector implements OpaqueTokenIntrospector { /** Description of the error for all tokens that the user info endpoint doesn't accept */ static final String INVALID_TOKEN = "Invalid access token"; + /** Timeout of connections to the user info endpoint, in milliseconds */ + static final int CONNECT_TIMEOUT_MS = 10_000; + + /** Timeout of reading the answer of the user info endpoint, in milliseconds */ + static final int READ_TIMEOUT_MS = 30_000; + private static final ParameterizedTypeReference> USER_INFO = new ParameterizedTypeReference>() {}; @@ -49,14 +59,25 @@ public class UserInfoTokenIntrospector implements OpaqueTokenIntrospector { * @param userInfoUri the user info endpoint * @param principalExtractor builds the user from the user info * @param authoritiesExtractor takes the authorities (group names) from the user info - * @throws IllegalStateException if the user info endpoint isn't set + * @throws IllegalStateException if the user info endpoint isn't an absolute http(s) URL * @throws IllegalArgumentException if it isn't a URI */ public UserInfoTokenIntrospector( String userInfoUri, ElixirPrincipalExtractor principalExtractor, GroupNamesSecurityExtractor authoritiesExtractor) { - this(userInfoUri, principalExtractor, authoritiesExtractor, new RestTemplate()); + this(userInfoUri, principalExtractor, authoritiesExtractor, restTemplate()); + } + + /** + * The client of the user info endpoint, with timeouts, so that a hanging endpoint doesn't hold + * request threads (a timeout refuses the token) + */ + private static RestTemplate restTemplate() { + SimpleClientHttpRequestFactory requestFactory = new SimpleClientHttpRequestFactory(); + requestFactory.setConnectTimeout(CONNECT_TIMEOUT_MS); + requestFactory.setReadTimeout(READ_TIMEOUT_MS); + return new RestTemplate(requestFactory); } /** As above, with the client of the user info endpoint (for tests) */ @@ -71,6 +92,13 @@ public UserInfoTokenIntrospector( + " (SECURITY_OAUTH2_RESOURCE_USER_INFO_URI)"); } this.userInfoUri = URI.create(userInfoUri.trim()); + String scheme = this.userInfoUri.getScheme(); + if (!("https".equalsIgnoreCase(scheme) || "http".equalsIgnoreCase(scheme)) + || this.userInfoUri.getHost() == null) { + throw new IllegalStateException( + "security.oauth2.resource.user-info-uri (SECURITY_OAUTH2_RESOURCE_USER_INFO_URI)" + + " must be an absolute http(s) URL"); + } this.principalExtractor = Objects.requireNonNull(principalExtractor); this.authoritiesExtractor = Objects.requireNonNull(authoritiesExtractor); this.restOperations = Objects.requireNonNull(restOperations); @@ -102,9 +130,17 @@ private Map userInfo(String token) { Map body; try { body = this.restOperations.exchange(request, USER_INFO).getBody(); + } catch (RestClientResponseException e) { + // its message holds the answer, which may have personal data: the status only + logger.warn("Could not fetch user details: status {}", e.getRawStatusCode()); + throw new BadOpaqueTokenException(INVALID_TOKEN); + } catch (ResourceAccessException e) { + // e.g. connection refused + logger.warn("Could not fetch user details: {}", e.getMessage()); + throw new BadOpaqueTokenException(INVALID_TOKEN); } catch (RuntimeException e) { - // the message of a client error can hold the answer, but never the token - logger.warn("Could not fetch user details: {}, {}", e.getClass(), e.getMessage()); + // e.g. an answer that isn't a JSON object: its message may quote the answer + logger.warn("Could not read user details: {}", e.getClass().getName()); throw new BadOpaqueTokenException(INVALID_TOKEN); } if (body == null) { diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiController.java b/src/main/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiController.java index 718b7e4a..cd60318d 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiController.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiController.java @@ -151,8 +151,8 @@ private static List asList(String[] values) { } /** - * The authenticated user (with the auth profile), or a user without groups named after the - * anonymous principal (noauth) + * The {@link User} principal of the request, or, without one (noauth), a user without groups + * named after the authentication */ private User getUser() { Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/AuthenticatedUserTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/AuthenticatedUserTest.java new file mode 100644 index 00000000..dc5b433a --- /dev/null +++ b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/AuthenticatedUserTest.java @@ -0,0 +1,82 @@ +package uk.ac.ebi.tsc.tesk.config.security; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.junit.Test; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.AuthorityUtils; +import org.springframework.security.oauth2.core.DefaultOAuth2AuthenticatedPrincipal; +import org.springframework.security.oauth2.core.OAuth2AuthenticatedPrincipal; +import org.springframework.security.oauth2.server.resource.InvalidBearerTokenException; + +/** The principal of a validated token, and the authentication made from it */ +public class AuthenticatedUserTest { + + private static final String TOKEN = "secret-token"; + + private final User user = User.builder("user@example.org").name("A User").build(); + private final List authorities = + AuthorityUtils.createAuthorityList("group:A", "group:B"); + + private AuthenticatedUser authenticatedUser() { + Map claims = new HashMap<>(); + claims.put("sub", "user@example.org"); + return new AuthenticatedUser(user, claims, authorities); + } + + @Test + public void name_isTheUserId() { + AuthenticatedUser principal = authenticatedUser(); + assertEquals("user@example.org", principal.getName()); + assertSame(user, principal.getUser()); + assertEquals("user@example.org", principal.getAttribute("sub")); + assertEquals(authorities, principal.getAuthorities()); + } + + @Test + public void attributesAndAuthorities_areUnmodifiable() { + AuthenticatedUser principal = authenticatedUser(); + assertThrows( + UnsupportedOperationException.class, + () -> principal.getAttributes().put("sub", "other")); + assertThrows(UnsupportedOperationException.class, () -> principal.getAuthorities().clear()); + } + + @Test + public void withoutUser_isRefused() { + assertThrows( + NullPointerException.class, + () -> new AuthenticatedUser(null, Collections.emptyMap(), Collections.emptyList())); + } + + /** The authentication has the user as principal, the authorities, and not the token */ + @Test + public void authentication_hasTheUser() { + Authentication authentication = + AuthenticatedUser.toAuthentication(TOKEN, authenticatedUser()); + + assertTrue(authentication.isAuthenticated()); + assertSame(user, authentication.getPrincipal()); + assertEquals("user@example.org", authentication.getName()); + assertEquals(authorities, authentication.getAuthorities()); + assertEquals("N/A", authentication.getCredentials()); + } + + @Test + public void authentication_ofAnotherPrincipal_isRefused() { + OAuth2AuthenticatedPrincipal other = + new DefaultOAuth2AuthenticatedPrincipal( + Collections.singletonMap("sub", "123"), Collections.emptyList()); + assertThrows( + InvalidBearerTokenException.class, + () -> AuthenticatedUser.toAuthentication(TOKEN, other)); + } +} diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java index b82f183c..5aa10205 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java @@ -3,7 +3,12 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.ObjectMapper; +import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; +import java.util.Map; import org.junit.Test; import org.springframework.boot.autoconfigure.http.HttpMessageConverters; import org.springframework.boot.web.servlet.error.DefaultErrorAttributes; @@ -35,6 +40,24 @@ private MockHttpServletResponse commence(String accept, AuthenticationException return response; } + private static Map body(MockHttpServletResponse response) throws Exception { + return new ObjectMapper() + .readValue( + response.getContentAsString(), new TypeReference>() {}); + } + + /** Without an OAuth 2.0 error or a message, the description is the message (null) */ + @Test + public void blankMessage_isUnauthorized() throws Exception { + MockHttpServletResponse response = + commence(null, new InsufficientAuthenticationException(null)); + assertEquals(401, response.getStatus()); + assertEquals( + "Bearer realm=\"oauth\", error=\"unauthorized\", error_description=\"null\"", + response.getHeader("WWW-Authenticate")); + assertEquals("unauthorized", body(response).get("error")); + } + @Test public void wwwAuthenticate_escapesQuotes() { assertEquals( @@ -52,10 +75,22 @@ public void invalidToken_isDescribed() throws Exception { "Bearer realm=\"oauth\", error=\"invalid_token\"," + " error_description=\"Invalid access token\"", response.getHeader("WWW-Authenticate")); - assertTrue(response.getContentAsString().contains("\"error\":\"invalid_token\"")); - assertTrue( - response.getContentAsString() - .contains("\"error_description\":\"Invalid access token\"")); + Map body = body(response); + assertEquals( + new HashSet<>( + Arrays.asList( + "timestamp", + "status", + "error", + "message", + "path", + "error_description")), + body.keySet()); + assertEquals(401, body.get("status")); + assertEquals("invalid_token", body.get("error")); + assertEquals("Invalid access token", body.get("message")); + assertEquals("Invalid access token", body.get("error_description")); + assertEquals("/tasks", body.get("path")); } @Test @@ -66,7 +101,7 @@ public void jsonAccepted_isWritten() throws Exception { new InsufficientAuthenticationException("no token")); assertEquals(401, response.getStatus()); assertTrue(response.getContentType().startsWith("application/json")); - assertTrue(response.getContentAsString().contains("\"error\":\"unauthorized\"")); + assertEquals("unauthorized", body(response).get("error")); } /** Without a converter for an accepted type, the answer has the status and headers only */ @@ -86,6 +121,6 @@ public void invalidAccept_isTakenAsAll() throws Exception { MockHttpServletResponse response = commence("not a media type", new InsufficientAuthenticationException("no token")); assertEquals(401, response.getStatus()); - assertTrue(response.getContentAsString().contains("\"error\":\"unauthorized\"")); + assertEquals("unauthorized", body(response).get("error")); } } diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java index c4c76636..08c416ed 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java @@ -20,18 +20,14 @@ import com.github.tomakehurst.wiremock.junit.WireMockRule; import java.util.Arrays; -import java.util.Collections; import java.util.List; import java.util.stream.Collectors; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.springframework.boot.test.system.OutputCaptureRule; -import org.springframework.security.core.Authentication; import org.springframework.security.core.GrantedAuthority; -import org.springframework.security.oauth2.core.DefaultOAuth2AuthenticatedPrincipal; import org.springframework.security.oauth2.core.OAuth2AuthenticatedPrincipal; -import org.springframework.security.oauth2.server.resource.InvalidBearerTokenException; import org.springframework.security.oauth2.server.resource.introspection.BadOpaqueTokenException; /** Validation of tokens at the user info endpoint (WireMocked) */ @@ -117,31 +113,6 @@ public void noGroups_givesTheUserAuthority() { assertFalse(((AuthenticatedUser) principal).getUser().isMember()); } - /** The authentication of the request has the user as principal, without the token */ - @Test - public void authentication_hasTheUser() { - answer("{\"sub\":\"123\",\"groups\":[\"" + PREFIX + "BASE\"]}"); - OAuth2AuthenticatedPrincipal principal = introspector.introspect(TOKEN); - - Authentication authentication = AuthenticatedUser.toAuthentication(TOKEN, principal); - - assertTrue(authentication.isAuthenticated()); - assertEquals(((AuthenticatedUser) principal).getUser(), authentication.getPrincipal()); - assertEquals("123", authentication.getName()); - assertEquals(principal.getAuthorities(), authentication.getAuthorities()); - assertFalse(String.valueOf(authentication.getCredentials()).contains(TOKEN)); - } - - @Test - public void authentication_ofAnotherPrincipal_isRefused() { - OAuth2AuthenticatedPrincipal other = - new DefaultOAuth2AuthenticatedPrincipal( - Collections.singletonMap("sub", "123"), Collections.emptyList()); - assertThrows( - InvalidBearerTokenException.class, - () -> AuthenticatedUser.toAuthentication(TOKEN, other)); - } - @Test public void refusedByUserInfo_isRefused() { userInfo.givenThat(get("/userinfo").willReturn(unauthorized())); @@ -211,4 +182,19 @@ public void withoutUserInfoUri_doesNotStart() { () -> new UserInfoTokenIntrospector(" ", extractor, groups)); assertThat(e.getMessage(), containsString("security.oauth2.resource.user-info-uri")); } + + @Test + public void userInfoUriNotAbsoluteHttp_doesNotStart() { + GroupNamesSecurityExtractor groups = new GroupNamesSecurityExtractor("groups", PREFIX); + ElixirPrincipalExtractor extractor = + new ElixirPrincipalExtractor(groups, new AuthorisationProperties()); + for (String uri : + new String[] {"login.example.org/oidc/userinfo", "ftp://login.example.org/"}) { + IllegalStateException e = + assertThrows( + IllegalStateException.class, + () -> new UserInfoTokenIntrospector(uri, extractor, groups)); + assertThat(e.getMessage(), containsString("absolute http(s) URL")); + } + } } From 9cbe9a495be9fa0fcf46ec1632b8c7e5418d915e Mon Sep 17 00:00:00 2001 From: Alex Kanitz Date: Sat, 14 Nov 2026 01:16:19 +0100 Subject: [PATCH 4/6] fix: address second review round - Accept user info URLs whose host URI doesn't parse (e.g. with "_", as in Docker Compose): check for an authority, not a host. - Log the cause of an unreachable endpoint, not the message with the URI, which may hold credentials. - Describe a 401 without a message as "Unauthorized", not "null". - Test the timeouts, a slow and a reset endpoint, and that refused calls log the status but not the answer. - Upgrade notes: own bullet for the user info endpoint; no claim about the former answer to requests that accept no JSON. Co-Authored-By: Claude Opus 5.5 --- docs/upgrading.md | 21 ++--- .../CustomAuthenticationEntryPoint.java | 3 + .../security/UserInfoTokenIntrospector.java | 12 ++- .../CustomAuthenticationEntryPointTest.java | 22 ++--- .../UserInfoTokenIntrospectorTest.java | 81 ++++++++++++++++--- 5 files changed, 107 insertions(+), 32 deletions(-) diff --git a/docs/upgrading.md b/docs/upgrading.md index 764391d7..24951d43 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -31,9 +31,9 @@ notice changed answers: `next_page_token`, or pass a `page_size` of up to 2048. - **Token validation.** TESK API validates access tokens with the resource server of Spring Security instead of Spring Security OAuth, which is end - of life. Deployments need no change: `SECURITY_OAUTH2_RESOURCE_USER_INFO_URI` - keeps its name and meaning (and is required with the `auth` profile, as - before). TESK API reads none of the other `security.oauth2.*` settings of + of life. Deployments need no change if + `SECURITY_OAUTH2_RESOURCE_USER_INFO_URI` is an absolute `http` or + `https` URL: it keeps its name and meaning. TESK API reads none of the other `security.oauth2.*` settings of Spring Security OAuth (e.g. `security.oauth2.resource.token-info-uri`), which it never documented. Clients may notice (see [Token validation](access-control.md#token-validation)): @@ -48,14 +48,15 @@ notice changed answers: endpoint. Before, it was passed on to the endpoint. - A token whose user info has no `sub` is refused with `401` instead of failing with `500 Internal Server Error`. - - Without a token, a request that accepts no JSON (e.g. `Accept: - text/plain`) gets `401` with the `WWW-Authenticate` header and no - body, instead of another error status (e.g. `406 Not Acceptable`). - Operators may notice that TESK API waits at most 10 seconds to connect to + - Without a valid token, a request that accepts no JSON (e.g. + `Accept: text/plain`) gets `401` with the `WWW-Authenticate` + header and no body. +- **User info endpoint.** TESK API waits at most 10 seconds to connect to the user info endpoint and 30 seconds for its answer, and then refuses - the token (before, it waited without limit), and that it doesn't start - if `SECURITY_OAUTH2_RESOURCE_USER_INFO_URI` isn't an absolute `http` or - `https` URL. + the token (before, it waited without limit). It doesn't start if + `SECURITY_OAUTH2_RESOURCE_USER_INFO_URI` isn't an absolute `http` or + `https` URL with a host; before, it started and refused every + token. ## To 1.2.0 diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPoint.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPoint.java index 73a09fe2..7228ba5c 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPoint.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPoint.java @@ -75,6 +75,9 @@ public void commence( description = oauth2Error.getDescription(); } } + if (!StringUtils.hasText(description)) { + description = HttpStatus.UNAUTHORIZED.getReasonPhrase(); + } WebUtils.exposeErrorRequestAttributes(request, authException, null); request.setAttribute(WebUtils.ERROR_STATUS_CODE_ATTRIBUTE, HttpStatus.UNAUTHORIZED.value()); Map body = diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java index c47c099b..a577711f 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java @@ -73,7 +73,7 @@ public UserInfoTokenIntrospector( * The client of the user info endpoint, with timeouts, so that a hanging endpoint doesn't hold * request threads (a timeout refuses the token) */ - private static RestTemplate restTemplate() { + static RestTemplate restTemplate() { SimpleClientHttpRequestFactory requestFactory = new SimpleClientHttpRequestFactory(); requestFactory.setConnectTimeout(CONNECT_TIMEOUT_MS); requestFactory.setReadTimeout(READ_TIMEOUT_MS); @@ -94,7 +94,7 @@ private static RestTemplate restTemplate() { this.userInfoUri = URI.create(userInfoUri.trim()); String scheme = this.userInfoUri.getScheme(); if (!("https".equalsIgnoreCase(scheme) || "http".equalsIgnoreCase(scheme)) - || this.userInfoUri.getHost() == null) { + || this.userInfoUri.getRawAuthority() == null) { throw new IllegalStateException( "security.oauth2.resource.user-info-uri (SECURITY_OAUTH2_RESOURCE_USER_INFO_URI)" + " must be an absolute http(s) URL"); @@ -135,8 +135,12 @@ private Map userInfo(String token) { logger.warn("Could not fetch user details: status {}", e.getRawStatusCode()); throw new BadOpaqueTokenException(INVALID_TOKEN); } catch (ResourceAccessException e) { - // e.g. connection refused - logger.warn("Could not fetch user details: {}", e.getMessage()); + // e.g. connection refused; its own message has the URI, which may hold credentials + Throwable cause = e.getCause() != null ? e.getCause() : e; + logger.warn( + "Could not fetch user details: {}: {}", + cause.getClass().getSimpleName(), + cause.getMessage()); throw new BadOpaqueTokenException(INVALID_TOKEN); } catch (RuntimeException e) { // e.g. an answer that isn't a JSON object: its message may quote the answer diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java index 5aa10205..9b1e18dc 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java @@ -46,16 +46,20 @@ private static Map body(MockHttpServletResponse response) throws response.getContentAsString(), new TypeReference>() {}); } - /** Without an OAuth 2.0 error or a message, the description is the message (null) */ + /** Without a message (null or blank), the description is the reason phrase of 401 */ @Test - public void blankMessage_isUnauthorized() throws Exception { - MockHttpServletResponse response = - commence(null, new InsufficientAuthenticationException(null)); - assertEquals(401, response.getStatus()); - assertEquals( - "Bearer realm=\"oauth\", error=\"unauthorized\", error_description=\"null\"", - response.getHeader("WWW-Authenticate")); - assertEquals("unauthorized", body(response).get("error")); + public void blankMessage_isDescribedAsUnauthorized() throws Exception { + for (String message : new String[] {null, "", " "}) { + MockHttpServletResponse response = + commence(null, new InsufficientAuthenticationException(message)); + assertEquals(401, response.getStatus()); + assertEquals( + "Bearer realm=\"oauth\", error=\"unauthorized\"," + + " error_description=\"Unauthorized\"", + response.getHeader("WWW-Authenticate")); + assertEquals("unauthorized", body(response).get("error")); + assertEquals("Unauthorized", body(response).get("error_description")); + } } @Test diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java index 08c416ed..a2f0216c 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java @@ -18,17 +18,20 @@ import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; +import com.github.tomakehurst.wiremock.http.Fault; import com.github.tomakehurst.wiremock.junit.WireMockRule; import java.util.Arrays; -import java.util.List; import java.util.stream.Collectors; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.springframework.boot.test.system.OutputCaptureRule; +import org.springframework.http.client.SimpleClientHttpRequestFactory; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.oauth2.core.OAuth2AuthenticatedPrincipal; import org.springframework.security.oauth2.server.resource.introspection.BadOpaqueTokenException; +import org.springframework.test.util.ReflectionTestUtils; +import org.springframework.web.client.RestTemplate; /** Validation of tokens at the user info endpoint (WireMocked) */ public class UserInfoTokenIntrospectorTest { @@ -59,18 +62,19 @@ private void answer(String json) { userInfo.givenThat(get("/userinfo").willReturn(okJson(json))); } + /** What this class logged (other loggers, e.g. of WireMock, log requests and answers) */ + private String logged() { + return Arrays.stream(output.getOut().split("\n")) + .filter(line -> line.contains(UserInfoTokenIntrospector.class.getSimpleName())) + .collect(Collectors.joining("\n")); + } + private void refused() { assertThrows(BadOpaqueTokenException.class, () -> introspector.introspect(TOKEN)); // the refusal is logged, without the token (other loggers, e.g. of WireMock, may log it) - List logged = - Arrays.stream(output.getOut().split("\n")) - .filter( - line -> - line.contains( - UserInfoTokenIntrospector.class.getSimpleName())) - .collect(Collectors.toList()); + String logged = logged(); assertFalse(logged.isEmpty()); - assertThat(String.join("\n", logged), not(containsString(TOKEN))); + assertThat(logged, not(containsString(TOKEN))); } @Test @@ -183,6 +187,65 @@ public void withoutUserInfoUri_doesNotStart() { assertThat(e.getMessage(), containsString("security.oauth2.resource.user-info-uri")); } + /** A refused call is logged with its status, without the answer (which may be personal) */ + @Test + public void refusedCall_logsStatusNotAnswer() { + userInfo.givenThat( + get("/userinfo").willReturn(aResponse().withStatus(500).withBody("ANSWER-MARKER"))); + refused(); + String logged = logged(); + assertThat(logged, containsString("Could not fetch user details: status 500")); + assertThat(logged, not(containsString("ANSWER-MARKER"))); + } + + @Test + public void connectionReset_isRefused() { + userInfo.givenThat( + get("/userinfo").willReturn(aResponse().withFault(Fault.CONNECTION_RESET_BY_PEER))); + refused(); + } + + /** A user info endpoint that doesn't answer in time refuses the token */ + @Test + public void slowUserInfo_isRefused() { + userInfo.givenThat( + get("/userinfo").willReturn(okJson("{\"sub\":\"123\"}").withFixedDelay(2_000))); + SimpleClientHttpRequestFactory factory = new SimpleClientHttpRequestFactory(); + factory.setReadTimeout(200); + GroupNamesSecurityExtractor groups = new GroupNamesSecurityExtractor("groups", PREFIX); + introspector = + new UserInfoTokenIntrospector( + "http://localhost:" + userInfo.port() + "/userinfo", + new ElixirPrincipalExtractor(groups, new AuthorisationProperties()), + groups, + new RestTemplate(factory)); + refused(); + } + + /** The client of the user info endpoint has timeouts */ + @Test + public void restTemplate_hasTimeouts() { + SimpleClientHttpRequestFactory factory = + (SimpleClientHttpRequestFactory) + UserInfoTokenIntrospector.restTemplate().getRequestFactory(); + assertEquals( + UserInfoTokenIntrospector.CONNECT_TIMEOUT_MS, + ReflectionTestUtils.getField(factory, "connectTimeout")); + assertEquals( + UserInfoTokenIntrospector.READ_TIMEOUT_MS, + ReflectionTestUtils.getField(factory, "readTimeout")); + } + + /** Host names that URI doesn't parse as hosts (e.g. with _, as in Docker Compose) work */ + @Test + public void userInfoUriWithUnderscore_starts() { + GroupNamesSecurityExtractor groups = new GroupNamesSecurityExtractor("groups", PREFIX); + new UserInfoTokenIntrospector( + "http://my_idp:8080/userinfo", + new ElixirPrincipalExtractor(groups, new AuthorisationProperties()), + groups); + } + @Test public void userInfoUriNotAbsoluteHttp_doesNotStart() { GroupNamesSecurityExtractor groups = new GroupNamesSecurityExtractor("groups", PREFIX); From bbc3ad3ae55d75ac010652c14b8c93ffa0df3c86 Mon Sep 17 00:00:00 2001 From: Alex Kanitz Date: Sat, 14 Nov 2026 01:27:16 +0100 Subject: [PATCH 5/6] test: cover every changed line Codecov requires full coverage of the patch: - Test an OAuth 2.0 error without a description, a user info URL without an authority, and an unexpected error while checking a service client's token. - Drop the null check of the introspector's principal (introspectors never return null) and log the most specific cause of an unreachable endpoint. - Keep CSRF protection on in TasksApiControllerTest: the resource server exempts requests with a bearer token (CodeQL alert). Co-Authored-By: Claude Opus 5.5 --- .../security/ServiceClientTokenServices.java | 2 +- .../security/UserInfoTokenIntrospector.java | 2 +- .../CustomAuthenticationEntryPointTest.java | 15 +++++++++++++++ .../security/ServiceClientTokenServicesTest.java | 15 +++++++++++++++ .../security/UserInfoTokenIntrospectorTest.java | 4 +++- .../tsc/tesk/tes/api/TasksApiControllerTest.java | 5 ++--- 6 files changed, 37 insertions(+), 6 deletions(-) diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServices.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServices.java index 97c5ce74..a1e05dae 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServices.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServices.java @@ -207,7 +207,7 @@ public OAuth2AuthenticatedPrincipal introspect(String accessToken) { if (clientId == null) { OAuth2AuthenticatedPrincipal principal = this.userInfoIntrospector.introspect(accessToken); - if (principal != null && this.clientIds.contains(principal.getName())) { + if (this.clientIds.contains(principal.getName())) { // a user with a listed client's ID would share the client's tasks logger.warn( "Refused user token: its sub is the ID of service client {}", diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java index a577711f..d6ecfddf 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java @@ -136,7 +136,7 @@ private Map userInfo(String token) { throw new BadOpaqueTokenException(INVALID_TOKEN); } catch (ResourceAccessException e) { // e.g. connection refused; its own message has the URI, which may hold credentials - Throwable cause = e.getCause() != null ? e.getCause() : e; + Throwable cause = e.getMostSpecificCause(); logger.warn( "Could not fetch user details: {}: {}", cause.getClass().getSimpleName(), diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java index 9b1e18dc..c1039068 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java @@ -17,6 +17,8 @@ import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.authentication.InsufficientAuthenticationException; import org.springframework.security.core.AuthenticationException; +import org.springframework.security.oauth2.core.OAuth2AuthenticationException; +import org.springframework.security.oauth2.core.OAuth2Error; import org.springframework.security.oauth2.server.resource.InvalidBearerTokenException; /** The answers to requests without a valid token */ @@ -62,6 +64,19 @@ public void blankMessage_isDescribedAsUnauthorized() throws Exception { } } + /** An OAuth 2.0 error without a description is described by the exception's message */ + @Test + public void oauth2ErrorWithoutDescription_hasTheMessage() throws Exception { + MockHttpServletResponse response = + commence( + null, + new OAuth2AuthenticationException( + new OAuth2Error("invalid_token"), "Token refused")); + assertEquals( + "Bearer realm=\"oauth\", error=\"invalid_token\", error_description=\"Token refused\"", + response.getHeader("WWW-Authenticate")); + } + @Test public void wwwAuthenticate_escapesQuotes() { assertEquals( diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServicesTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServicesTest.java index dfcab192..8a6906e6 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServicesTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/ServiceClientTokenServicesTest.java @@ -33,6 +33,7 @@ import org.jose4j.jws.JsonWebSignature; import org.jose4j.jwt.JwtClaims; import org.jose4j.jwt.NumericDate; +import org.jose4j.jwt.consumer.JwtConsumer; import org.jose4j.keys.EllipticCurves; import org.jose4j.keys.HmacKey; import org.jose4j.lang.JoseException; @@ -44,6 +45,7 @@ import org.springframework.security.oauth2.core.OAuth2AuthenticatedPrincipal; import org.springframework.security.oauth2.server.resource.introspection.BadOpaqueTokenException; import org.springframework.security.oauth2.server.resource.introspection.OpaqueTokenIntrospector; +import org.springframework.test.util.ReflectionTestUtils; import uk.ac.ebi.tsc.tesk.k8s.convert.CreatorLabels; /** @@ -542,6 +544,19 @@ public void otherUser_isPassedOn() { passedOn("opaque"); } + /** An unexpected error while checking a service client's token refuses it */ + @Test + public void unexpectedError_isRefused() throws Exception { + JwtConsumer failing = mock(JwtConsumer.class); + when(failing.process(anyString())).thenThrow(new IllegalStateException("unexpected")); + ServiceClientTokenServices services = services(); + ReflectionTestUtils.setField(services, "verifier", failing); + + assertThrows( + BadOpaqueTokenException.class, () -> services.introspect(sign(claims(CLIENT)))); + verify(userInfo, never()).introspect(anyString()); + } + @Test public void defaults() { AuthorisationProperties.ServiceClients defaults = diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java index a2f0216c..c19040da 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java @@ -252,7 +252,9 @@ public void userInfoUriNotAbsoluteHttp_doesNotStart() { ElixirPrincipalExtractor extractor = new ElixirPrincipalExtractor(groups, new AuthorisationProperties()); for (String uri : - new String[] {"login.example.org/oidc/userinfo", "ftp://login.example.org/"}) { + new String[] { + "login.example.org/oidc/userinfo", "ftp://login.example.org/", "http:userinfo" + }) { IllegalStateException e = assertThrows( IllegalStateException.class, diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiControllerTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiControllerTest.java index aba45e1b..7e4b40c2 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiControllerTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/tes/api/TasksApiControllerTest.java @@ -74,11 +74,10 @@ public class TasksApiControllerTest { @TestConfiguration static class ResourceServerConfiguration { + // CSRF protection stays on: the resource server exempts requests with a bearer token @Bean SecurityFilterChain filterChain(HttpSecurity http) throws Exception { - http.csrf() - .disable() - .authorizeRequests() + http.authorizeRequests() .anyRequest() .authenticated() .and() From 8841552520ffb78b81a6c98add02b8d3db42e41c Mon Sep 17 00:00:00 2001 From: Alex Kanitz Date: Sat, 14 Nov 2026 01:41:04 +0100 Subject: [PATCH 6/6] fix: address automated review findings Refuse user info answers with a blank sub, require a host in the user info URL, and send no body when the client refuses JSON (q=0). Document that the timeouts are per read. Co-Authored-By: Claude Opus 5.5 --- docs/access-control.md | 2 +- docs/upgrading.md | 4 ++-- .../CustomAuthenticationEntryPoint.java | 2 ++ .../security/UserInfoTokenIntrospector.java | 18 +++++++++++++-- .../CustomAuthenticationEntryPointTest.java | 11 +++++++++ .../UserInfoTokenIntrospectorTest.java | 23 ++++++++++++++++++- 6 files changed, 54 insertions(+), 6 deletions(-) diff --git a/docs/access-control.md b/docs/access-control.md index fc9dc56b..db288c07 100644 --- a/docs/access-control.md +++ b/docs/access-control.md @@ -45,7 +45,7 @@ with the authorisation code grant, and with the redirect URI provider must issue tokens to it without a client secret and without PKCE. ### Token validation -TESK API validates the token by hitting Elixir AAI `user_info` endpoint on each request and extracting information returned from that endpoint, using the resource server of Spring Security. A token is valid if the endpoint answers it (`200 OK`) with a JSON object that has a `sub` and no `error`. TESK API waits at most 10 seconds to connect to the endpoint and 30 seconds for its answer; if the endpoint can't be reached or doesn't answer in time, the token is refused like an invalid one. An invalid token, or lack of thereof in the `Authorization: Bearer` header, will result in `HTTP 401 Unauthorized` error. Tokens of allow-listed services are checked locally instead, if that is on (see [Tokens of services](#tokens-of-services)). +TESK API validates the token by hitting Elixir AAI `user_info` endpoint on each request and extracting information returned from that endpoint, using the resource server of Spring Security. A token is valid if the endpoint answers it (`200 OK`) with a JSON object that has a `sub` (not blank) and no `error`. TESK API waits at most 10 seconds to connect to the endpoint and 30 seconds for each read of its answer (these limits don't cover the name lookup of its host, nor the answer as a whole); if the endpoint can't be reached or doesn't answer in time, the token is refused like an invalid one. An invalid token, or lack of thereof in the `Authorization: Bearer` header, will result in `HTTP 401 Unauthorized` error. Tokens of allow-listed services are checked locally instead, if that is on (see [Tokens of services](#tokens-of-services)). TESK API takes the token from the `Authorization` header only (`Bearer `, as in [RFC 6750, section 2.1](https://datatracker.ietf.org/doc/html/rfc6750#section-2.1)), not from an `access_token` parameter in the URL or in a form. A header whose token is empty or has characters that bearer tokens can't have (anything but letters, digits and `-._~+/`, with `=` only at the end; e.g. spaces) is refused without a call to the user info endpoint. diff --git a/docs/upgrading.md b/docs/upgrading.md index 24951d43..56d29c7f 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -46,13 +46,13 @@ notice changed answers: have (e.g. spaces), is refused with `401` (`invalid_token`, `Bearer token is malformed`) without a call to the user info endpoint. Before, it was passed on to the endpoint. - - A token whose user info has no `sub` is refused with `401` instead + - A token whose user info has no `sub` (or a blank one) is refused with `401` instead of failing with `500 Internal Server Error`. - Without a valid token, a request that accepts no JSON (e.g. `Accept: text/plain`) gets `401` with the `WWW-Authenticate` header and no body. - **User info endpoint.** TESK API waits at most 10 seconds to connect to - the user info endpoint and 30 seconds for its answer, and then refuses + the user info endpoint and 30 seconds for each read of its answer, and then refuses the token (before, it waited without limit). It doesn't start if `SECURITY_OAUTH2_RESOURCE_USER_INFO_URI` isn't an absolute `http` or `https` URL with a host; before, it started and refused every diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPoint.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPoint.java index 7228ba5c..4e024204 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPoint.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPoint.java @@ -124,6 +124,8 @@ private void write( if (accepted.isEmpty()) { accepted.add(MediaType.ALL); } + // q=0: not acceptable + accepted.removeIf(mediaType -> mediaType.getQualityValue() == 0); MediaType.sortBySpecificityAndQuality(accepted); for (MediaType mediaType : accepted) { for (HttpMessageConverter converter : this.messageConverters) { diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java index d6ecfddf..fa7c7973 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java @@ -1,5 +1,6 @@ package uk.ac.ebi.tsc.tesk.config.security; +import java.net.MalformedURLException; import java.net.URI; import java.util.Collections; import java.util.LinkedHashMap; @@ -80,6 +81,18 @@ static RestTemplate restTemplate() { return new RestTemplate(requestFactory); } + /** + * Whether the URL names a host, as the HTTP client reads it (also names that {@link URI} + * doesn't parse as hosts, e.g. with {@code _}, as in Docker Compose) + */ + static boolean hasHost(URI uri) { + try { + return StringUtils.hasText(uri.toURL().getHost()); + } catch (MalformedURLException | IllegalArgumentException e) { + return false; + } + } + /** As above, with the client of the user info endpoint (for tests) */ UserInfoTokenIntrospector( String userInfoUri, @@ -94,7 +107,7 @@ static RestTemplate restTemplate() { this.userInfoUri = URI.create(userInfoUri.trim()); String scheme = this.userInfoUri.getScheme(); if (!("https".equalsIgnoreCase(scheme) || "http".equalsIgnoreCase(scheme)) - || this.userInfoUri.getRawAuthority() == null) { + || !hasHost(this.userInfoUri)) { throw new IllegalStateException( "security.oauth2.resource.user-info-uri (SECURITY_OAUTH2_RESOURCE_USER_INFO_URI)" + " must be an absolute http(s) URL"); @@ -111,7 +124,8 @@ public OAuth2AuthenticatedPrincipal introspect(String token) { logger.warn("Refused token: the user info has an error"); throw new BadOpaqueTokenException(INVALID_TOKEN); } - if (userInfo.get("sub") == null) { + Object sub = userInfo.get("sub"); + if (sub == null || !StringUtils.hasText(sub.toString())) { logger.warn("Refused token: the user info has no sub"); throw new BadOpaqueTokenException(INVALID_TOKEN); } diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java index c1039068..2817eea2 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPointTest.java @@ -135,6 +135,17 @@ public void nothingAccepted_hasNoBody() throws Exception { response.getHeader("WWW-Authenticate")); } + /** A media type with quality 0 is not acceptable */ + @Test + public void refusedJson_hasNoBody() throws Exception { + MockHttpServletResponse response = + commence( + "application/json;q=0, text/html", + new InsufficientAuthenticationException("no token")); + assertEquals(401, response.getStatus()); + assertEquals("", response.getContentAsString()); + } + @Test public void invalidAccept_isTakenAsAll() throws Exception { MockHttpServletResponse response = diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java index c19040da..75b3f449 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospectorTest.java @@ -20,6 +20,7 @@ import com.github.tomakehurst.wiremock.http.Fault; import com.github.tomakehurst.wiremock.junit.WireMockRule; +import java.net.URI; import java.util.Arrays; import java.util.stream.Collectors; import org.junit.Before; @@ -147,6 +148,12 @@ public void userInfoWithoutSub_isRefused() { refused(); } + @Test + public void userInfoWithBlankSub_isRefused() { + answer("{\"sub\":\" \"}"); + refused(); + } + @Test public void userInfoNotAnObject_isRefused() { answer("[\"sub\"]"); @@ -246,6 +253,16 @@ public void userInfoUriWithUnderscore_starts() { groups); } + @Test + public void hasHost() { + assertTrue(UserInfoTokenIntrospector.hasHost(URI.create("https://login.example.org/"))); + assertTrue(UserInfoTokenIntrospector.hasHost(URI.create("http://my_idp:8080/"))); + assertFalse(UserInfoTokenIntrospector.hasHost(URI.create("http://:8080/"))); + // not a URL that Java can open + assertFalse(UserInfoTokenIntrospector.hasHost(URI.create("unknown://host/"))); + assertFalse(UserInfoTokenIntrospector.hasHost(URI.create("relative/path"))); + } + @Test public void userInfoUriNotAbsoluteHttp_doesNotStart() { GroupNamesSecurityExtractor groups = new GroupNamesSecurityExtractor("groups", PREFIX); @@ -253,7 +270,11 @@ public void userInfoUriNotAbsoluteHttp_doesNotStart() { new ElixirPrincipalExtractor(groups, new AuthorisationProperties()); for (String uri : new String[] { - "login.example.org/oidc/userinfo", "ftp://login.example.org/", "http:userinfo" + "login.example.org/oidc/userinfo", + "ftp://login.example.org/", + "http:userinfo", + "http://:8080/userinfo", + "http://user@/userinfo" }) { IllegalStateException e = assertThrows(