Repository navigation
refactor: use Spring Security's resource server - #175
Merged
Merged
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
Reviewer's GuideData structuresReview the authenticated-principal representation and its conversion into request authentication, which preserve
APIReview the resource-server filter chain and the authentication entry point together: they define protected and public routes and the externally visible 401 response contract.
Uses of the APICheck that task handling continues to obtain the
ImplementationReview the user-info introspector's endpoint validation, timeouts, refusal handling, and construction of the authenticated user and authorities.
Review service-client token validation as an opaque-token introspector, including its delegation to user-info validation and client-principal construction.
Check that user and group extraction retain the existing authorization semantics while supplying the resource-server introspector.
ConfigurationReview removal of the legacy Spring Security OAuth dependencies and addition of the Spring Security resource-server dependency.
DocumentationCheck the access-control documentation against token extraction, validation, timeout, and 401-response behavior.
Review the API status-code documentation's link to the detailed unauthorized-response contract.
Check the configuration documentation for the required user-info endpoint setting and its retained environment-variable name.
Review upgrade notes for intentional client-visible changes and endpoint requirements.
TestsReview integration coverage for unauthenticated requests, malformed or invalid tokens, user-info calls, and the 401 response contract.
Check unit coverage for authenticated-principal behavior and the 401 entry-point response.
Review tests for service-client introspection and user-info endpoint validation, failures, logging, and timeouts.
Check the controller test's resource-server setup and that CSRF remains enabled in that test configuration.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #175 +/- ##
==========================================
+ Coverage 75.81% 76.82% +1.01%
==========================================
Files 71 73 +2
Lines 3345 3439 +94
Branches 402 412 +10
==========================================
+ Hits 2536 2642 +106
+ Misses 651 640 -11
+ Partials 158 157 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 <noreply@anthropic.com>
uniqueg
marked this pull request as ready for review
November 14, 2026 00:30
Linked issuesChecked at commit f1f1a44 #165 refactor: use Spring Security's resource server · 6 of 7 met
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/main/java/uk/ac/ebi/tsc/tesk/config/security/ResourceServerConfiguration.java" line_range="61" />
<code_context>
@Value("${openapi.taskExecutionService.base-path:/ga4gh/tes/v1}") String baseUrl,
@Value("${tesk.api.service-info.public:true}") boolean serviceInfoPublic,
- ObjectProvider<ResourceServerTokenServices> tokenServices) {
+ @Value("${security.oauth2.resource.user-info-uri:}") String userInfoUri) {
this.errorAttributes = errorAttributes;
</code_context>
<issue_to_address>
**Environment override is ignored**
When a deployment configures a custom provider using the documented `SECURITY_OAUTH2_RESOURCE_USER_INFO_URI` environment variable, `@Value` looks up the dashed property directly rather than binding it as a `@ConfigurationProperties` value, so `SECURITY_OAUTH2_RESOURCE_USER_INFO_URI` is not resolved as `security.oauth2.resource.user-info-uri`; the bundled default then remains active and requests for a deployment's custom provider are refused.
Bind the setting through `@ConfigurationProperties` or explicitly resolve the documented environment-variable name, and add a test that sets it as an environment property.
</issue_to_address>
### Comment 2
<location path="src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java" line_range="114" />
<code_context>
+ 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);
</code_context>
<issue_to_address>
**Malformed subjects authenticate**
When the user-info endpoint returns an empty or non-string `sub`, the introspector rejects only a null `sub`; an empty or non-string value passes, and `ElixirPrincipalExtractor` converts it with `toString()` into a user ID. The request is authenticated with an identity that does not satisfy OpenID Connect's subject requirements.
Require `sub` to be a non-empty string before constructing the user.
</issue_to_address>
### Comment 3
<location path="src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java" line_range="97" />
<code_context>
+ this.userInfoUri = URI.create(userInfoUri.trim());
+ String scheme = this.userInfoUri.getScheme();
+ if (!("https".equalsIgnoreCase(scheme) || "http".equalsIgnoreCase(scheme))
+ || this.userInfoUri.getRawAuthority() == null) {
+ throw new IllegalStateException(
+ "security.oauth2.resource.user-info-uri (SECURITY_OAUTH2_RESOURCE_USER_INFO_URI)"
</code_context>
<issue_to_address>
**Invalid user-info hosts pass startup**
When the configured user-info URI has an HTTP(S) scheme and a non-null authority but no host, the raw-authority check accepts the URI, so the application starts and user-info calls fail; `userInfo()` then treats valid tokens as invalid, and authenticated requests receive 401 responses instead of a startup configuration error.
Validate that the URI has a host, not merely a non-null raw authority.
Also at `docs/upgrading.md:58`.
</issue_to_address>
### Comment 4
<location path="src/main/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPoint.java" line_range="129-130" />
<code_context>
+ }
+ MediaType.sortBySpecificityAndQuality(accepted);
+ for (MediaType mediaType : accepted) {
+ for (HttpMessageConverter<?> converter : this.messageConverters) {
+ if (converter.canWrite(body.getClass(), mediaType)) {
+ ((HttpMessageConverter<Object>) converter)
+ .write(body, mediaType, new ServletServerHttpResponse(response));
</code_context>
<issue_to_address>
**Rejected JSON still gets an error body**
When the request's `Accept` header gives JSON quality zero, the media-type loop still considers the JSON entry, and `converter.canWrite` checks compatibility without excluding quality-zero media types, so the entry point writes a JSON error body despite the client's rejection.
Skip media types with quality zero before checking converters.
</issue_to_address>
### Comment 5
<location path="src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java" line_range="78" />
<code_context>
+ */
+ static RestTemplate restTemplate() {
+ SimpleClientHttpRequestFactory requestFactory = new SimpleClientHttpRequestFactory();
+ requestFactory.setConnectTimeout(CONNECT_TIMEOUT_MS);
+ requestFactory.setReadTimeout(READ_TIMEOUT_MS);
+ return new RestTemplate(requestFactory);
</code_context>
<issue_to_address>
**DNS stalls bypass the timeout**
When the configured endpoint hostname's DNS lookup stalls, `SimpleClientHttpRequestFactory`'s connect timeout does not bound a blocking JVM DNS lookup, so `RestTemplate.exchange` can hold request threads past the documented 10-second limit while resolving the user-info host; callers keep waiting instead of receiving a refusal.
Bound DNS resolution or run endpoint calls behind a separately enforced overall deadline.
</issue_to_address>
### Comment 6
<location path="src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java" line_range="79" />
<code_context>
+ static RestTemplate restTemplate() {
+ SimpleClientHttpRequestFactory requestFactory = new SimpleClientHttpRequestFactory();
+ requestFactory.setConnectTimeout(CONNECT_TIMEOUT_MS);
+ requestFactory.setReadTimeout(READ_TIMEOUT_MS);
+ return new RestTemplate(requestFactory);
+ }
</code_context>
<issue_to_address>
**Slow responses exceed the read limit**
When the endpoint sends response data often enough to avoid each individual read timeout but never completes, `setReadTimeout` limits individual blocking reads rather than the total response duration, so `RestTemplate.exchange` keeps the request thread occupied beyond the documented limit.
Enforce a total response deadline in addition to the per-read timeout.
Also at `src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java:80`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 6 findings to address first, and the resource-server migration now decides which bearer tokens are accepted and which protected API requests proceed; a validation or filter-chain mistake could expose task data or permit protected actions. Reverting stops further impact, but cannot undo data already exposed or actions already performed.
Blocking findings: src/main/java/uk/ac/ebi/tsc/tesk/config/security/ResourceServerConfiguration.java:61, src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java:114, src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java:97, src/main/java/uk/ac/ebi/tsc/tesk/config/security/CustomAuthenticationEntryPoint.java:130, src/main/java/uk/ac/ebi/tsc/tesk/config/security/UserInfoTokenIntrospector.java:78, and 1 more
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 <noreply@anthropic.com>
uniqueg
added a commit
that referenced
this pull request
Nov 14, 2026
…lient Resolves pom.xml: #175 removed spring-security-oauth2 and its autoconfigure, so the Bouncy Castle exclusion goes with them; the removed pins stay removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
uniqueg
added a commit
that referenced
this pull request
Nov 14, 2026
spring-security-jwt left with #175, so there are no older Bouncy Castle jars left to keep out. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
uniqueg
added a commit
that referenced
this pull request
Nov 14, 2026
Brings in #161, #174, #175, #176, #178, #181, #183 and #184. The conflict in docs/upgrading.md is resolved by merging both lists under "To 1.3.0": the upgrade order for filer labels and Java 21 come first, and the TES 1.1 fields item no longer says that ignore_error: true is refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
uniqueg
added a commit
that referenced
this pull request
Nov 14, 2026
Lists the changes merged into dev since the notes were written: Java 21 (#161), ignore_error (#174), the resource server (#175), output file logs (#176), the Kubernetes client (#178), Dependabot labels (#181), the executor job selector (#183) and full test coverage (#184). Adds a Breaking changes section, as in the 1.2.0 notes, and sets date-released to the planned tag date, 2026-11-16. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the end-of-life Spring Security OAuth (
spring-security-oauth2,spring-security-oauth2-autoconfigure,@EnableResourceServer) with the resource server of Spring Security 5.8, still on Spring Boot 2.7 (child 4 of #66).ResourceServerConfigurationdeclares aSecurityFilterChainwithoauth2ResourceServer().opaqueToken(). It is stateless, has no CSRF, and keeps the same public paths: service-info unlessTESK_API_SERVICE_INFO_PUBLIC=false, the API description and the Swagger UI.UserInfoTokenIntrospectorvalidates tokens at the user info endpoint, asUserInfoTokenServicesdid. It builds the sameUser(ElixirPrincipalExtractor) and the same group authorities (GroupNamesSecurityExtractor).ServiceClientTokenServices(feat(auth): accept tokens of allow-listed clients #154) is now anOpaqueTokenIntrospector. Its checks, messages and logger name are unchanged; the class keeps its name because operators configure its logger by that name.AuthenticatedUserand anOpaqueTokenAuthenticationConverterkeep theUseras the principal of the request. This matters forTesService's@PreAuthorizerules, which testprincipal instanceof User.CustomAuthenticationEntryPointkeeps the 401 answers: a JSON body witherror/error_description,WWW-Authenticate: Bearer realm="oauth", error=…, error_description=…, andCache-Control: no-store.security.oauth2.resource.user-info-uri/SECURITY_OAUTH2_RESOURCE_USER_INFO_URIkeeps working under the same name. No new key was added. Both Spring Security OAuth artifacts and thespring-security-oauth2pin are removed;spring-security-oauth2-resource-servertakes their place (no Nimbus or JOSE dependencies).The existing auth integration tests are the contract and are unchanged:
AuthIT,ClaimAuthIT,ServiceClientAuthIT,NoAuthIT,ServiceInfoTokenRequiredIT,SwaggerUi*AuthITandLegacyUserLabelsIT. A newAuthErrorResponsesITpins the 401 answers. I captured them ondevfirst. Four of its cases fail ondevand pass here; they are the deliberate changes below.Deliberate changes, all listed in
docs/upgrading.md:error_description/messageof an invalid token no longer repeats the token. It is nowInvalid access token.access_tokenparameter in the URL or a form is ignored. The token is read only from the header (RFC 6750).invalid_tokenwithout a call to the user info endpoint. Before, it was passed on to the endpoint, and if the endpoint answered, the request was authenticated.subnow gives 401 instead of 500.Closes #165
Verification
./mvnw clean verifyon Java 21 passes locally (last run on 8841552): over 18,400 unit tests and 109 integration tests, including all auth ITs unchanged plusAuthErrorResponsesIT../mvnw spotless:check checkstyle:checkon Java 8 (the CI Lint setup) reports 0 violations.mkdocs build --strictpasses.AuthErrorResponsesITwas run againstdev. The no-token cases and the user info request headers matched, which confirms the old answers are kept. The token echo, query-parameter, malformed-header and missing-subcases failed ondev, which confirms they are the intended changes.CI passed, Test, Lint, Docs, Smoke test, Scan image, codecov patch 100% and project). The image smoke test (401 without a token, public service-info) runs in CI only.java/spring-disabled-csrf-protection) onResourceServerConfiguration. It is a false positive: this is a stateless bearer-token API with no session and no cookies. The old@EnableResourceServeralso disabled CSRF, just out of CodeQL's sight, and keeping CSRF on would turn the 401 for a POST without a token into 403, which breaksAuthIT. The alert needs dismissing as a false positive (drafted for the maintainer). The alert in the test configuration is fixed: CSRF stays on there.Declined review findings
ServiceClientTokenServices: declined.docs/access-control.mddocuments its logger name, so renaming it would break operators' logging configuration. The class Javadoc now explains why the name stays.Cache-Control/Pragmaheaders: declined. Both are already the behaviour ondev:public_serviceInfo_withInvalidToken_isUnauthorizedcovers the first, and the headers match what I captured ondev.subis refused like a missing one; the user info URL must have a host; a client that refuses JSON (q=0) gets the 401 without a body.@Valueignores the environment variable: declined. Relaxed binding mapsSECURITY_OAUTH2_RESOURCE_USER_INFO_URIto the key, as forTESK_API_SERVICE_INFO_PUBLIC.sub), listed indocs/upgrading.md.Open questions
None.
Overlaps
ignore_error) also changesdocs/upgrading.md(same "To the next release" list),docs/api.mdandTasksApiControllerTest.java(here, only the test's security config at the top). Whichever merges second needs a small merge of the upgrade notes.docs/upgrading.md,docs/configuration.mdandpom.xml. Not touched here.🤖 Generated with Claude Code
Summary by Sourcery
Migrate TESK API authentication to Spring Security's opaque-token resource server while preserving existing authorization contracts and documenting the resulting token-validation behavior.
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: