Add authorization guardrail tests and run them in CI#2522
Merged
Conversation
…allow-list Switch the catch-all SecurityFilterChain from permitAll to default-deny. Only /info, /auth/providers and /i18n/** remain anonymous (login page needs them). Token-less requests are now rejected with 401.
Class-level @PreAuthorize broke Spring Batch user-import tasklets, which call UserImportService interface methods as SYSTEM_USER. Annotate only the HTTP handler methods so background jobs are unaffected.
Add @PreAuthorize("isPermitted('admin:cache')") to CDMResultsService#clearCache (POST /cdmresults/clearCache) and remove the satisfied TODO comment. Add postAsLimitedUser helper and limitedUserDeniedGlobalCdmResultsCacheClear test to SourceAccessIT to assert 403 for non-admin users.
…ethods AchillesCacheTasklet (batch job, no security context) now calls getRawTreeMap instead of the @PreAuthorize-gated getTreemap. ConceptSetService now calls the new executeIdentifierLookupInternal instead of the gated overload.
… SpEL @PreAuthorize SpEL reads public fields on WebApiSecurityExpressionRoot (READ/WRITE/SOURCE/...) and the AccessType/EntityType enums reflectively. Register them via RuntimeHintsRegistrar so native-image evaluation works. No-op on the JVM.
…ity-auth-coverage
ci.yaml only ran 'mvn test' (surefire/unit), so the failsafe *IT tests never executed in CI. Add a step that runs the authorization guardrails (EndpointAuthCoverageIT, SecurityIT, SourceAccessIT) via 'mvn verify' against embedded PostgreSQL, so a regression that leaves an endpoint reachable without authorization fails the build.
…ity-auth-coverage # Conflicts: # src/main/java/org/ohdsi/webapi/security/authc/JwtAuthConfig.java # src/main/java/org/ohdsi/webapi/security/spring/WebApiSecurityRuntimeHints.java # src/test/java/org/ohdsi/webapi/test/SecurityIT.java
…ndpoints Under the merged model (security.anonymousAccess.enabled defaults true), the filter chain admits token-less requests and per-endpoint @PreAuthorize is the gate, so EndpointAuthCoverageIT now asserts @PreAuthorize *presence* statically over the live handler set rather than HTTP-probing with synthetic requests (which produced 400/500 during argument binding before method security ran). The static guard immediately caught five IncidenceRate endpoints that earlier source-based analysis missed because IRAnalysisService implements an interface (the @RequestMapping lives on IRAnalysisResource): getIRAnalysisList, generateSql, runDiagnostics, listByTags and getCountIRWithSameName. Gate them with the service's existing incidence pattern (read:incidence/write:incidence; entity read for the by-id exists check). Runtime denial of anonymous requests remains covered by SecurityIT and SourceAccessIT.
AuthorizationAccessIT: a user granted the generic 'read' permission must be allowed (2xx) on the domain read/list endpoints and still denied admin operations. The existing suite only proved access is denied; this catches an over-strict @PreAuthorize that wrongly denies a legitimate user. AuthorizationPermissionStringsIT: every permission literal referenced in a @PreAuthorize expression must exist in sec_permission, so a typo (e.g. read:cohortdefinition vs read:cohort-definition) that would silently deny everyone fails the build instead of passing unnoticed. Both are wired into the ci.yaml security step.
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
1 participant
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
This PR adds an automated guardrail so future regressions are caught, wires the security integration tests into CI (they previously never ran there), and fixes a coverage gap the guardrail surfaced.
Changes
EndpointAuthCoverageIT: a guardrail driven by the liveRequestMappingHandlerMapping, so new endpoints are covered automatically. It statically asserts that everyorg.ohdsi.webapiendpoint carries method security (@PreAuthorizeon the handler or its controller) except a pinned login/bootstrap allow-list, and that everysourceKey-scoped handler carries@PreAuthorize. Runtime denial of anonymous requests stays covered bySecurityITandSourceAccessIT.ci.yaml: runsEndpointAuthCoverageIT,SecurityITandSourceAccessITviamvn verifyagainst embedded PostgreSQL. CI previously ran onlymvn test(surefire/unit), so the failsafe integration tests never executed.IRAnalysisService: gate five IncidenceRate endpoints (getIRAnalysisList,generateSql,runDiagnostics,listByTags,getCountIRWithSameName) that Enforce authorization on every endpoint (default-deny + complete @PreAuthorize coverage) #2521 missed because the controller implements an interface (IRAnalysisResource) where the request mappings live, so source-based analysis skipped them; the new reflection-driven guard caught them.