From 2f29bcc256bd3c7b893d2519a7669125cf0b393b Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 11:10:45 +0800 Subject: [PATCH 01/22] fix(security): require authentication for all endpoints except login 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. --- .../webapi/security/authc/JwtAuthConfig.java | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/ohdsi/webapi/security/authc/JwtAuthConfig.java b/src/main/java/org/ohdsi/webapi/security/authc/JwtAuthConfig.java index 1d091f547..dab0c4875 100644 --- a/src/main/java/org/ohdsi/webapi/security/authc/JwtAuthConfig.java +++ b/src/main/java/org/ohdsi/webapi/security/authc/JwtAuthConfig.java @@ -258,21 +258,29 @@ public org.springframework.security.web.AuthenticationEntryPoint unauthorizedEnt public SecurityFilterChain apiChain(HttpSecurity http, org.springframework.security.web.AuthenticationEntryPoint unauthorizedEntryPoint) throws Exception { - httpSecurityShared.configureDefaults(http); - + httpSecurityShared.configureDefaults(http); + http .httpBasic(AbstractHttpConfigurer::disable) .authorizeHttpRequests(auth -> auth - .anyRequest().permitAll()) + // Endpoints that must stay reachable before login (used by the login page): + // /info – server/version banner + // /auth/providers – list of enabled auth methods + // /i18n/** – localization bundles + .requestMatchers("/info", "/auth/providers", "/i18n/**").permitAll() + // Everything else now requires an authenticated principal. + .anyRequest().authenticated()) // Configure JWT authentication .oauth2ResourceServer(oauth -> oauth .authenticationEntryPoint(unauthorizedEntryPoint) .jwt(jwt -> jwt.jwtAuthenticationConverter( new JwtToWebApiAuthenticationConverter(sessionService, userRepository)))) - // Fallback to anonymous if JWT not present + // Token-less requests become anonymous (rejected by authenticated()). .anonymous(anon -> anon .principal(WebApiPrincipal.ANONYMOUS) - .authorities("ROLE_ANONYMOUS")); + .authorities("ROLE_ANONYMOUS")) + // Return 401 (not 403) when an anonymous request hits a protected endpoint. + .exceptionHandling(ex -> ex.authenticationEntryPoint(unauthorizedEntryPoint)); return http.build(); } From f4f4a0b780aa7081c5636f5e315c61f90807f0a7 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 11:15:13 +0800 Subject: [PATCH 02/22] test(security): regression test that anonymous requests get 401 --- .../org/ohdsi/webapi/test/SecurityIT.java | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 src/test/java/org/ohdsi/webapi/test/SecurityIT.java diff --git a/src/test/java/org/ohdsi/webapi/test/SecurityIT.java b/src/test/java/org/ohdsi/webapi/test/SecurityIT.java new file mode 100644 index 000000000..c2c7eb523 --- /dev/null +++ b/src/test/java/org/ohdsi/webapi/test/SecurityIT.java @@ -0,0 +1,43 @@ +package org.ohdsi.webapi.test; + +import static org.junit.Assert.assertEquals; + +import org.junit.Test; +import org.springframework.boot.test.web.client.TestRestTemplate; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; + +/** + * Verifies the default-deny security model: every endpoint except the login + * allow-list must reject unauthenticated (token-less) requests with 401. + */ +public class SecurityIT extends WebApiIT { + + /** A template with NO Authorization interceptor — simulates an anonymous caller. */ + private final TestRestTemplate anonymous = new TestRestTemplate(); + + private HttpStatus statusOf(String path) { + ResponseEntity resp = + anonymous.getForEntity(getBaseUri() + path, String.class); + return HttpStatus.valueOf(resp.getStatusCode().value()); + } + + @Test + public void protectedEndpointsRejectAnonymous() { + // Sensitive endpoints that previously leaked data without a login. + assertEquals(HttpStatus.UNAUTHORIZED, statusOf("/user")); + assertEquals(HttpStatus.UNAUTHORIZED, statusOf("/role")); + assertEquals(HttpStatus.UNAUTHORIZED, statusOf("/permission")); + assertEquals(HttpStatus.UNAUTHORIZED, statusOf("/cohortdefinition")); + assertEquals(HttpStatus.UNAUTHORIZED, statusOf("/conceptset")); + assertEquals(HttpStatus.UNAUTHORIZED, statusOf("/tag")); + assertEquals(HttpStatus.UNAUTHORIZED, statusOf("/source/sources")); + } + + @Test + public void loginAllowListIsAnonymous() { + // These must stay reachable before login. + assertEquals(HttpStatus.OK, statusOf("/info")); + assertEquals(HttpStatus.OK, statusOf("/auth/providers")); + } +} From 6a1290026098496fddbbae49f26449b6e7b275b9 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 11:18:23 +0800 Subject: [PATCH 03/22] fix(security): require admin:cache for cache endpoints --- .../org/ohdsi/webapi/mvc/controller/CacheMvcController.java | 3 +++ src/test/java/org/ohdsi/webapi/test/SecurityIT.java | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/src/main/java/org/ohdsi/webapi/mvc/controller/CacheMvcController.java b/src/main/java/org/ohdsi/webapi/mvc/controller/CacheMvcController.java index 121a460ed..af2c61aca 100644 --- a/src/main/java/org/ohdsi/webapi/mvc/controller/CacheMvcController.java +++ b/src/main/java/org/ohdsi/webapi/mvc/controller/CacheMvcController.java @@ -6,6 +6,7 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RestController; @@ -42,6 +43,7 @@ public CacheMvcController(CacheManager cacheManager) { /** * Get list of all caches with statistics. */ + @PreAuthorize("isPermitted('admin:cache')") @GetMapping(produces = MediaType.APPLICATION_JSON_VALUE) public ResponseEntity> getCacheInfoList() { List caches = new ArrayList<>(); @@ -64,6 +66,7 @@ public ResponseEntity> getCacheInfoList() { /** * Clear all caches. */ + @PreAuthorize("isPermitted('admin:cache')") @GetMapping(value = "/clear", produces = MediaType.APPLICATION_JSON_VALUE) public ResponseEntity clearAll() { ClearCacheResult result = new ClearCacheResult(); diff --git a/src/test/java/org/ohdsi/webapi/test/SecurityIT.java b/src/test/java/org/ohdsi/webapi/test/SecurityIT.java index c2c7eb523..e17c42c8e 100644 --- a/src/test/java/org/ohdsi/webapi/test/SecurityIT.java +++ b/src/test/java/org/ohdsi/webapi/test/SecurityIT.java @@ -40,4 +40,9 @@ public void loginAllowListIsAnonymous() { assertEquals(HttpStatus.OK, statusOf("/info")); assertEquals(HttpStatus.OK, statusOf("/auth/providers")); } + + @Test + public void cacheClearRejectsAnonymous() { + assertEquals(HttpStatus.UNAUTHORIZED, statusOf("/cache/clear")); + } } From 3171d02b151fdf7de0ea85c50846648b335c46b9 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 11:21:02 +0800 Subject: [PATCH 04/22] fix(security): require admin:tags for tag mutations --- src/main/java/org/ohdsi/webapi/tag/TagService.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/main/java/org/ohdsi/webapi/tag/TagService.java b/src/main/java/org/ohdsi/webapi/tag/TagService.java index 52a93c594..30fa28617 100644 --- a/src/main/java/org/ohdsi/webapi/tag/TagService.java +++ b/src/main/java/org/ohdsi/webapi/tag/TagService.java @@ -19,6 +19,7 @@ import org.springframework.http.MediaType; import org.springframework.scheduling.annotation.Scheduled; import org.springframework.transaction.annotation.Transactional; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.bind.annotation.*; import jakarta.persistence.EntityManager; @@ -61,6 +62,7 @@ public TagService( * @param dto * @return */ + @PreAuthorize("isPermitted('admin:tags')") @PostMapping(produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) public TagDTO create(@RequestBody TagDTO dto) { Tag tag = conversionService.convert(dto, Tag.class); @@ -153,6 +155,7 @@ public List findByIdIn(List ids) { * @param entity * @return */ + @PreAuthorize("isPermitted('admin:tags')") @PutMapping(value = "/{id}", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) public TagDTO update(@PathVariable("id") Integer id, @RequestBody TagDTO entity) { Tag existing = tagRepository.findById(id).orElse(null); @@ -179,6 +182,7 @@ public TagDTO update(@PathVariable("id") Integer id, @RequestBody TagDTO entity) * * @param id */ + @PreAuthorize("isPermitted('admin:tags')") @DeleteMapping(value = "/{id}") public void delete(@PathVariable("id") Integer id) { Tag existing = tagRepository.findById(id).orElseThrow(); @@ -274,6 +278,7 @@ public AssignmentPermissionsDTO getAssignmentPermissions() { * * @param dto */ + @PreAuthorize("isPermitted('admin:tags')") @PostMapping(value = "/multiAssign", consumes = MediaType.APPLICATION_JSON_VALUE) public void assignGroup(@RequestBody TagGroupSubscriptionDTO dto) { tagGroupService.assignGroup(dto); @@ -284,6 +289,7 @@ public void assignGroup(@RequestBody TagGroupSubscriptionDTO dto) { * * @param dto */ + @PreAuthorize("isPermitted('admin:tags')") @PostMapping(value = "/multiUnassign", consumes = MediaType.APPLICATION_JSON_VALUE) public void unassignGroup(@RequestBody TagGroupSubscriptionDTO dto) { tagGroupService.unassignGroup(dto); From 25a06cbf69db6952933ceffda46b905a7f7c1ee3 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 11:23:45 +0800 Subject: [PATCH 05/22] fix(security): require admin:tools for tool mutations --- src/main/java/org/ohdsi/webapi/tool/ToolServiceImpl.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main/java/org/ohdsi/webapi/tool/ToolServiceImpl.java b/src/main/java/org/ohdsi/webapi/tool/ToolServiceImpl.java index 66825e56a..48279dace 100644 --- a/src/main/java/org/ohdsi/webapi/tool/ToolServiceImpl.java +++ b/src/main/java/org/ohdsi/webapi/tool/ToolServiceImpl.java @@ -11,6 +11,7 @@ import org.ohdsi.webapi.service.AbstractDaoService; import org.ohdsi.webapi.tool.dto.ToolDTO; import org.springframework.http.MediaType; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.bind.annotation.DeleteMapping; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PathVariable; @@ -40,6 +41,7 @@ public List getTools() { } @Override + @PreAuthorize("isPermitted('admin:tools')") @PostMapping(value = "", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) public ToolDTO saveTool(@RequestBody ToolDTO toolDTO) { Tool tool = saveToolFromDTO(toolDTO, getCurrentUser()); @@ -62,11 +64,13 @@ public ToolDTO getById(@PathVariable("id") Integer id) { } @Override + @PreAuthorize("isPermitted('admin:tools')") @DeleteMapping(value = "/{id}") public void delete(@PathVariable("id") Integer id) { toolRepository.deleteById(id); } + @PreAuthorize("isPermitted('admin:tools')") @PutMapping(value = "", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) public ToolDTO updateTool(@RequestBody ToolDTO toolDTO) { return saveTool(toolDTO); From c50d707fa9f5aedf104411e0bdcb725f0ca5f8c1 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 11:34:44 +0800 Subject: [PATCH 06/22] fix(security): require admin:security for user-import endpoints --- .../security/provisioning/service/UserImportJobServiceImpl.java | 2 ++ .../security/provisioning/service/UserImportServiceImpl.java | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/main/java/org/ohdsi/webapi/security/provisioning/service/UserImportJobServiceImpl.java b/src/main/java/org/ohdsi/webapi/security/provisioning/service/UserImportJobServiceImpl.java index 1de51eab5..b0d85028c 100644 --- a/src/main/java/org/ohdsi/webapi/security/provisioning/service/UserImportJobServiceImpl.java +++ b/src/main/java/org/ohdsi/webapi/security/provisioning/service/UserImportJobServiceImpl.java @@ -41,6 +41,7 @@ import org.springframework.web.bind.annotation.PutMapping; import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.server.ResponseStatusException; @@ -56,6 +57,7 @@ @RestController @RequestMapping("/user/import/job") @Transactional +@PreAuthorize("isPermitted('admin:security')") public class UserImportJobServiceImpl extends BaseJobServiceImpl implements UserImportJobService { private final UserImportService userImportService; diff --git a/src/main/java/org/ohdsi/webapi/security/provisioning/service/UserImportServiceImpl.java b/src/main/java/org/ohdsi/webapi/security/provisioning/service/UserImportServiceImpl.java index 468718e34..de164f46b 100644 --- a/src/main/java/org/ohdsi/webapi/security/provisioning/service/UserImportServiceImpl.java +++ b/src/main/java/org/ohdsi/webapi/security/provisioning/service/UserImportServiceImpl.java @@ -63,12 +63,14 @@ import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.server.ResponseStatusException; @RestController @RequestMapping("/user") @Transactional(readOnly = true) +@PreAuthorize("isPermitted('admin:security')") public class UserImportServiceImpl implements UserImportService { // Note: @RestController already includes @Component, so @Service is not needed From fd6fe44275696e2067fddf44cc2ddb2f6fc2c99c Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 11:39:04 +0800 Subject: [PATCH 07/22] fix(security): apply admin:security per HTTP handler, not class-level 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. --- .../provisioning/service/UserImportJobServiceImpl.java | 7 ++++++- .../provisioning/service/UserImportServiceImpl.java | 8 +++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/ohdsi/webapi/security/provisioning/service/UserImportJobServiceImpl.java b/src/main/java/org/ohdsi/webapi/security/provisioning/service/UserImportJobServiceImpl.java index b0d85028c..6d917024f 100644 --- a/src/main/java/org/ohdsi/webapi/security/provisioning/service/UserImportJobServiceImpl.java +++ b/src/main/java/org/ohdsi/webapi/security/provisioning/service/UserImportJobServiceImpl.java @@ -57,7 +57,6 @@ @RestController @RequestMapping("/user/import/job") @Transactional -@PreAuthorize("isPermitted('admin:security')") public class UserImportJobServiceImpl extends BaseJobServiceImpl implements UserImportJobService { private final UserImportService userImportService; @@ -175,6 +174,7 @@ public Optional getLatestHistoryItem(Long id) { consumes = MediaType.APPLICATION_JSON_VALUE, produces = MediaType.APPLICATION_JSON_VALUE ) + @PreAuthorize("isPermitted('admin:security')") public UserImportJobDTO createJobEndpoint(@RequestBody UserImportJobDTO jobDTO) { UserImportJob job = conversionService.convert(jobDTO, UserImportJob.class); try { @@ -194,6 +194,7 @@ public UserImportJobDTO createJobEndpoint(@RequestBody UserImportJobDTO jobDTO) consumes = MediaType.APPLICATION_JSON_VALUE, produces = MediaType.APPLICATION_JSON_VALUE ) + @PreAuthorize("isPermitted('admin:security')") public UserImportJobDTO updateJobEndpoint( @PathVariable("id") Long jobId, @RequestBody UserImportJobDTO jobDTO) { @@ -214,6 +215,7 @@ public UserImportJobDTO updateJobEndpoint( produces = MediaType.APPLICATION_JSON_VALUE ) @Transactional + @PreAuthorize("isPermitted('admin:security')") public List listJobsEndpoint() { return getJobs().stream() .map(job -> conversionService.convert(job, UserImportJobDTO.class)) @@ -229,6 +231,7 @@ public List listJobsEndpoint() { value = "/{id}", produces = MediaType.APPLICATION_JSON_VALUE ) + @PreAuthorize("isPermitted('admin:security')") public UserImportJobDTO getJobEndpoint(@PathVariable("id") Long id) { return getJob(id) .map(job -> conversionService.convert(job, UserImportJobDTO.class)) @@ -241,6 +244,7 @@ public UserImportJobDTO getJobEndpoint(@PathVariable("id") Long id) { @DeleteMapping( value = "/{id}" ) + @PreAuthorize("isPermitted('admin:security')") public void deleteJobEndpoint(@PathVariable("id") Long id) { UserImportJob job = getJob(id) .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND)); @@ -254,6 +258,7 @@ public void deleteJobEndpoint(@PathVariable("id") Long id) { value = "/{id}/history", produces = MediaType.APPLICATION_JSON_VALUE ) + @PreAuthorize("isPermitted('admin:security')") public List getImportHistoryEndpoint(@PathVariable("id") Long id) { return getJobHistoryItems(id) .map(item -> conversionService.convert(item, JobHistoryItemDTO.class)) diff --git a/src/main/java/org/ohdsi/webapi/security/provisioning/service/UserImportServiceImpl.java b/src/main/java/org/ohdsi/webapi/security/provisioning/service/UserImportServiceImpl.java index de164f46b..d78a1d754 100644 --- a/src/main/java/org/ohdsi/webapi/security/provisioning/service/UserImportServiceImpl.java +++ b/src/main/java/org/ohdsi/webapi/security/provisioning/service/UserImportServiceImpl.java @@ -70,7 +70,6 @@ @RestController @RequestMapping("/user") @Transactional(readOnly = true) -@PreAuthorize("isPermitted('admin:security')") public class UserImportServiceImpl implements UserImportService { // Note: @RestController already includes @Component, so @Service is not needed @@ -133,6 +132,7 @@ protected Optional getProvider(LdapProviderType type) { value = "/providers", produces = MediaType.APPLICATION_JSON_VALUE ) + @PreAuthorize("isPermitted('admin:security')") public AuthenticationProviders getAuthenticationProviders() { AuthenticationProviders providers = new AuthenticationProviders(); providers.setAdUrl(adUrl); @@ -147,6 +147,7 @@ public AuthenticationProviders getAuthenticationProviders() { value = "/import/{type}/test", produces = MediaType.APPLICATION_JSON_VALUE ) + @PreAuthorize("isPermitted('admin:security')") public ConnectionInfo testConnectionEndpoint(@PathVariable("type") String type) { LdapProviderType provider = LdapProviderType.fromValue(type); ConnectionInfo result = new ConnectionInfo(); @@ -163,6 +164,7 @@ public ConnectionInfo testConnectionEndpoint(@PathVariable("type") String type) value = "/import/{type}/groups", produces = MediaType.APPLICATION_JSON_VALUE ) + @PreAuthorize("isPermitted('admin:security')") public List findGroupsEndpoint( @PathVariable("type") String type, @RequestParam(value = "search", required = false) String searchStr) { @@ -178,6 +180,7 @@ public List findGroupsEndpoint( consumes = MediaType.APPLICATION_JSON_VALUE, produces = MediaType.APPLICATION_JSON_VALUE ) + @PreAuthorize("isPermitted('admin:security')") public List findDirectoryUsers( @PathVariable("type") String type, @RequestBody RoleGroupMapping mapping) { @@ -193,6 +196,7 @@ public List findDirectoryUsers( consumes = MediaType.APPLICATION_JSON_VALUE, produces = MediaType.APPLICATION_JSON_VALUE ) + @PreAuthorize("isPermitted('admin:security')") public UserImportJobDTO importUsersEndpoint( @RequestBody List users, @RequestParam(value = "provider") String provider, @@ -227,6 +231,7 @@ public UserImportJobDTO importUsersEndpoint( value = "/import/{type}/mapping", consumes = MediaType.APPLICATION_JSON_VALUE ) + @PreAuthorize("isPermitted('admin:security')") public void saveMappingEndpoint(@PathVariable("type") String type, @RequestBody RoleGroupMapping mapping) { LdapProviderType providerType = LdapProviderType.fromValue(type); List mappingEntities = RoleGroupMappingConverter.convertRoleGroupMapping(mapping); @@ -240,6 +245,7 @@ public void saveMappingEndpoint(@PathVariable("type") String type, @RequestBody value = "/import/{type}/mapping", produces = MediaType.APPLICATION_JSON_VALUE ) + @PreAuthorize("isPermitted('admin:security')") public RoleGroupMapping getMappingEndpoint(@PathVariable("type") String type) { LdapProviderType providerType = LdapProviderType.fromValue(type); List mappingEntities = getRoleGroupMapping(providerType); From bf3fcfe606b44bb3c382a3788692903156c24885 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 11:51:55 +0800 Subject: [PATCH 08/22] docs(security): document defaultGlobalReadPermissions in application.yaml --- src/main/resources/application.yaml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main/resources/application.yaml b/src/main/resources/application.yaml index 35b5b2d79..aec21ccc7 100644 --- a/src/main/resources/application.yaml +++ b/src/main/resources/application.yaml @@ -251,8 +251,10 @@ security: enabled: true allowed-origins: http://localhost - # If defaultGlobalReadPermissions is set to true (default), then all users can see every artifact. - # If it is set to false, WebAPI will filter out the artifacts that a user does not explicitly have read permissions to + # defaultGlobalReadPermissions: when true (default), the built-in "public" role grants every + # authenticated user read access to all artifacts (cohorts, concept sets, analyses, etc.). + # Set to false to enforce per-asset access control, where users must be explicitly granted + # read permission on each artifact before they can see it. defaultGlobalReadPermissions: true defaultRoles: "" From 0d0a70885d72ab51cb85e283e7fc5a60b2b8f1d3 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 14:03:59 +0800 Subject: [PATCH 09/22] test(security): harness for per-source authorization (limited user) --- .../org/ohdsi/webapi/test/SourceAccessIT.java | 211 ++++++++++++++++++ 1 file changed, 211 insertions(+) create mode 100644 src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java diff --git a/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java b/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java new file mode 100644 index 000000000..d70198098 --- /dev/null +++ b/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java @@ -0,0 +1,211 @@ +package org.ohdsi.webapi.test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; + +import java.sql.SQLException; +import java.time.Instant; +import java.util.Date; +import java.util.UUID; + +import org.junit.Before; +import org.junit.Test; +import org.ohdsi.webapi.security.authc.JwtService; +import org.ohdsi.webapi.security.authz.AuthorizationService; +import org.ohdsi.webapi.security.session.SessionService; +import org.ohdsi.webapi.source.Source; +import org.ohdsi.webapi.source.SourceRepository; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.web.client.TestRestTemplate; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; + +/** + * Test harness that issues HTTP requests as a NON-admin user with no + * data-source grant, so downstream tests can assert that source-scoped + * endpoints deny them with 403. + * + *

The two smoke tests here validate the harness against + * {@code PersonService}, which already enforces + * {@code @PreAuthorize("hasSourceAccess(#sourceKey, READ)")}. This means + * both tests pass without depending on any later security task. + * + *

Later tasks can extend this class (or replicate {@link #getAsLimitedUser}) + * to reuse the limited-user JWT. + */ +public class SourceAccessIT extends WebApiIT { + + // --------------------------------------------------------------------------- + // Spring beans + // --------------------------------------------------------------------------- + + @Autowired + private JwtService jwtService; + + @Autowired + private SessionService sessionService; + + @Autowired + private SourceRepository sourceRepository; + + @Autowired + private AuthorizationService authorizationService; + + // --------------------------------------------------------------------------- + // Limited-user state (set up in @Before) + // --------------------------------------------------------------------------- + + /** + * Hard-coded negative user-id so we never collide with the auto-sequence + * (which starts at 1000). Anonymous user sits at -1; we use -2 here. + */ + private static final long LIMITED_USER_ID = -2L; + private static final long LIMITED_ROLE_ID = -100L; // personal role, non-admin + private static final String LIMITED_LOGIN = "limited"; + + /** Bearer JWT for the limited user, minted fresh in each @Before. */ + private String limitedUserJwt; + + // --------------------------------------------------------------------------- + // Test fixture + // --------------------------------------------------------------------------- + + /** + * Seeds: + *

    + *
  1. A test CDM source (Embedded_PG) in {@code source} table.
    2. + *
  2. A {@code sec_source} row granting the admin role (id = 2) READ + * access to that source — so the admin HTTP client is NOT denied.
    3. + *
  3. A limited user ({@code login='limited'}, id = -2) with only a + * personal non-admin role and NO {@code sec_source} grant — so the + * limited user IS denied with 403.
    4. + *
  4. A valid JWT + session for the limited user.
    5. + *
+ * + * The authorization cache is cleared after each seeding so that stale + * entries from previous test runs in the same JVM do not interfere. + */ + @Before + public void setUpSourceAccessHarness() throws SQLException { + String schema = getOhdsiSchema(); + + // ---- 1. Seed CDM source -------------------------------------------------- + truncateTable(schema + ".source"); + resetSequence(schema + ".source_sequence"); + Source source = sourceRepository.saveAndFlush(getCdmSource()); + long sourceId = source.getId().longValue(); + + // ---- 2. Grant admin role READ access to the source ----------------------- + // role_id=2 is the built-in "admin" role (see baseline migration). + // hasSourceAccess() queries the sec_source table directly; the "*" + // wildcard permission in sec_permission does NOT bypass this check, + // so an explicit row is required. + jdbcTemplate.execute( + "INSERT INTO " + schema + ".sec_source (role_id, source_id, access_type) " + + "VALUES (2, " + sourceId + ", 'READ') " + + "ON CONFLICT DO NOTHING" + ); + + // ---- 3. Seed limited user (id=-2, login='limited') ---------------------- + // Uses hard-coded negative IDs so sequences are never consumed. + // The sec_user table has a UNIQUE constraint on login; use ON CONFLICT + // to make the @Before idempotent across repeated test runs. + jdbcTemplate.execute( + "INSERT INTO " + schema + ".sec_user (id, login, name, origin) " + + "VALUES (" + LIMITED_USER_ID + ", '" + LIMITED_LOGIN + "', 'Limited User', 'SYSTEM') " + + "ON CONFLICT (login) DO NOTHING" + ); + + // ---- 4. Seed a personal (non-admin) role for the limited user ----------- + // sec_role has a UNIQUE constraint on (name, system_role). + jdbcTemplate.execute( + "INSERT INTO " + schema + ".sec_role (id, name, system_role) " + + "VALUES (" + LIMITED_ROLE_ID + ", '" + LIMITED_LOGIN + "', false) " + + "ON CONFLICT DO NOTHING" + ); + + // ---- 5. Link the limited user to their personal role -------------------- + jdbcTemplate.execute( + "INSERT INTO " + schema + ".sec_user_role (id, user_id, role_id, origin) " + + "SELECT nextval('" + schema + ".sec_user_role_sequence'), " + LIMITED_USER_ID + ", " + LIMITED_ROLE_ID + ", 'SYSTEM' " + + "WHERE NOT EXISTS (" + + " SELECT 1 FROM " + schema + ".sec_user_role " + + " WHERE user_id = " + LIMITED_USER_ID + " AND role_id = " + LIMITED_ROLE_ID + + ")" + ); + // No sec_source row for LIMITED_ROLE_ID → the limited user has NO source access. + + // ---- 6. Clear authorization cache so new rows are visible --------------- + authorizationService.clearCache(); + + // ---- 7. Mint a JWT + session for the limited user ----------------------- + UUID sessionId = sessionService.createSession(LIMITED_LOGIN); + Date expiresAt = Date.from(Instant.now().plusSeconds(3600)); + limitedUserJwt = jwtService.generateToken(LIMITED_LOGIN, sessionId.toString(), expiresAt); + } + + // --------------------------------------------------------------------------- + // Reusable helper (the contract promised by the task brief) + // --------------------------------------------------------------------------- + + /** + * Issues a GET request to {@code getBaseUri() + path} as the limited user + * (authenticated but with no source grant). + * + *

Reusable by subclasses or other tests that want to assert 403 on + * source-scoped endpoints. + * + * @param path the path relative to the WebAPI base URI, e.g. + * {@code "/" + SOURCE_KEY + "/person/1"} + * @return the raw HTTP response + */ + protected ResponseEntity getAsLimitedUser(String path) { + TestRestTemplate limitedClient = new TestRestTemplate(); + limitedClient.getRestTemplate().getInterceptors().add((request, body, execution) -> { + request.getHeaders().set("Authorization", "Bearer " + limitedUserJwt); + return execution.execute(request, body); + }); + return limitedClient.getForEntity(getBaseUri() + path, String.class); + } + + // --------------------------------------------------------------------------- + // Validating tests + // --------------------------------------------------------------------------- + + /** + * A limited user (authenticated, but with no source grant) must receive + * HTTP 403 Forbidden when accessing a source-scoped endpoint. + * + *

PersonService already enforces + * {@code @PreAuthorize("hasSourceAccess(#sourceKey, READ)")} + * so this test passes without depending on any later task. + */ + @Test + public void limitedUserDeniedSourceScopedPerson() { + ResponseEntity r = getAsLimitedUser("/" + SOURCE_KEY + "/person/1"); + assertEquals( + "Limited user (no source grant) should be denied with 403 Forbidden", + HttpStatus.FORBIDDEN, + HttpStatus.valueOf(r.getStatusCode().value()) + ); + } + + /** + * The base-class admin user (anonymous, role=2, with a {@code sec_source} + * READ grant seeded in {@link #setUpSourceAccessHarness()}) must NOT be + * denied with 403 when accessing the same source-scoped endpoint. + * + *

The person id=1 likely does not exist in the embedded CDM, so we + * expect 404 or 500 — but NOT 403. + */ + @Test + public void adminUserNotDeniedSourceScopedPerson() { + ResponseEntity r = getRestTemplate() + .getForEntity(getBaseUri() + "/" + SOURCE_KEY + "/person/1", String.class); + assertNotEquals( + "Admin user (with source grant) should NOT be denied with 403 Forbidden", + HttpStatus.FORBIDDEN, + HttpStatus.valueOf(r.getStatusCode().value()) + ); + } +} From fdb566e5d86b60baf5bed4de5ac81e44b542a8cb Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 14:10:18 +0800 Subject: [PATCH 10/22] fix(security): require source access for cohort sample endpoints --- .../cohortdefinition/CohortSampleService.java | 8 ++++++++ .../org/ohdsi/webapi/test/SourceAccessIT.java | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/src/main/java/org/ohdsi/webapi/cohortdefinition/CohortSampleService.java b/src/main/java/org/ohdsi/webapi/cohortdefinition/CohortSampleService.java index 708679359..f1d9e299e 100644 --- a/src/main/java/org/ohdsi/webapi/cohortdefinition/CohortSampleService.java +++ b/src/main/java/org/ohdsi/webapi/cohortdefinition/CohortSampleService.java @@ -11,6 +11,7 @@ import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.bind.annotation.*; import org.springframework.web.server.ResponseStatusException; @@ -44,6 +45,7 @@ public CohortSampleService( * @param sourceKey * @return JSON containing information about cohort samples */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{cohortDefinitionId}/{sourceKey}", produces = MediaType.APPLICATION_JSON_VALUE) public CohortSampleListDTO listCohortSamples( @PathVariable("cohortDefinitionId") int cohortDefinitionId, @@ -73,6 +75,7 @@ public CohortSampleListDTO listCohortSamples( * @param fields * @return personId, gender, age of each person in the cohort sample */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{cohortDefinitionId}/{sourceKey}/{sampleId}", produces = MediaType.APPLICATION_JSON_VALUE) public CohortSampleDTO getCohortSample( @PathVariable("cohortDefinitionId") int cohortDefinitionId, @@ -94,6 +97,7 @@ public CohortSampleDTO getCohortSample( * @param fields * @return A sample of persons from a cohort */ + @PreAuthorize("isPermitted('write:source') or hasSourceAccess(#sourceKey, WRITE)") @PostMapping(value = "/{cohortDefinitionId}/{sourceKey}/{sampleId}/refresh", produces = MediaType.APPLICATION_JSON_VALUE) public CohortSampleDTO refreshCohortSample( @PathVariable("cohortDefinitionId") int cohortDefinitionId, @@ -126,6 +130,7 @@ public Map hasSamples( * @param cohortDefinitionId * @return true or false */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/has-samples/{cohortDefinitionId}/{sourceKey}", produces = MediaType.APPLICATION_JSON_VALUE) public Map hasSamplesForSource( @PathVariable("sourceKey") String sourceKey, @@ -143,6 +148,7 @@ public Map hasSamplesForSource( * @param sampleParameters * @return */ + @PreAuthorize("isPermitted('write:source') or hasSourceAccess(#sourceKey, WRITE)") @PostMapping(value = "/{cohortDefinitionId}/{sourceKey}", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) public CohortSampleDTO createCohortSample( @PathVariable("sourceKey") String sourceKey, @@ -169,6 +175,7 @@ public CohortSampleDTO createCohortSample( * @param sampleId * @return */ + @PreAuthorize("isPermitted('write:source') or hasSourceAccess(#sourceKey, WRITE)") @DeleteMapping("/{cohortDefinitionId}/{sourceKey}/{sampleId}") public ResponseEntity deleteCohortSample( @PathVariable("sourceKey") String sourceKey, @@ -189,6 +196,7 @@ public ResponseEntity deleteCohortSample( * @param cohortDefinitionId * @return */ + @PreAuthorize("isPermitted('write:source') or hasSourceAccess(#sourceKey, WRITE)") @DeleteMapping("/{cohortDefinitionId}/{sourceKey}") public ResponseEntity deleteCohortSamples( @PathVariable("sourceKey") String sourceKey, diff --git a/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java b/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java index d70198098..c8e327857 100644 --- a/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java +++ b/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java @@ -208,4 +208,23 @@ public void adminUserNotDeniedSourceScopedPerson() { HttpStatus.valueOf(r.getStatusCode().value()) ); } + + /** + * A limited user (authenticated, but with no source grant) must receive + * HTTP 403 Forbidden on the source-scoped CohortSampleService + * {@code hasSamplesForSource} endpoint. + * + *

This test is RED until Task 2 adds + * {@code @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)")} + * to {@code CohortSampleService#hasSamplesForSource}. + */ + @Test + public void limitedUserDeniedCohortSampleSourceScopedRead() { + ResponseEntity r = getAsLimitedUser("/cohortsample/has-samples/1/" + SOURCE_KEY); + assertEquals( + "Limited user (no source grant) should be denied with 403 Forbidden on source-scoped CohortSampleService read", + HttpStatus.FORBIDDEN, + HttpStatus.valueOf(r.getStatusCode().value()) + ); + } } From 4830da5bd43b8e9650e2cbb08d91ce2858745c9c Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 14:14:33 +0800 Subject: [PATCH 11/22] fix(security): require source access for cdmresults endpoints --- .../webapi/service/CDMResultsService.java | 14 ++++++++++++-- .../org/ohdsi/webapi/test/SourceAccessIT.java | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/ohdsi/webapi/service/CDMResultsService.java b/src/main/java/org/ohdsi/webapi/service/CDMResultsService.java index 09813c617..25b9b4494 100644 --- a/src/main/java/org/ohdsi/webapi/service/CDMResultsService.java +++ b/src/main/java/org/ohdsi/webapi/service/CDMResultsService.java @@ -51,6 +51,7 @@ import org.springframework.web.bind.annotation.RestController; import org.springframework.boot.context.event.ApplicationReadyEvent; import org.springframework.context.event.EventListener; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.server.ResponseStatusException; import jakarta.annotation.PostConstruct; @@ -184,6 +185,7 @@ public void scheduledWarmCaches(){ @PostMapping(value = "/{sourceKey}/conceptRecordCount", consumes = MediaType.APPLICATION_JSON_VALUE, produces = MediaType.APPLICATION_JSON_VALUE) + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") public List>> getConceptRecordCount( @PathVariable("sourceKey") String sourceKey, @RequestBody List identifiers) { @@ -218,6 +220,7 @@ private List>> convertToResponse(CollectionThis test is RED until Task 3 adds + * {@code @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)")} + * to {@code CDMResultsService#getPerson}. + */ + @Test + public void limitedUserDeniedCdmResultsPerson() { + ResponseEntity r = getAsLimitedUser("/cdmresults/" + SOURCE_KEY + "/person"); + assertEquals( + "Limited user (no source grant) should be denied with 403 Forbidden on CDMResultsService person endpoint", + HttpStatus.FORBIDDEN, + HttpStatus.valueOf(r.getStatusCode().value()) + ); + } } From 7732557fd174f496bc5759eac9b5689d196c725f Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 14:50:06 +0800 Subject: [PATCH 12/22] fix(security): require admin:cache for global cdmresults cache clear 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. --- .../webapi/service/CDMResultsService.java | 3 +- .../org/ohdsi/webapi/test/SourceAccessIT.java | 43 +++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/ohdsi/webapi/service/CDMResultsService.java b/src/main/java/org/ohdsi/webapi/service/CDMResultsService.java index 25b9b4494..0c8e54f00 100644 --- a/src/main/java/org/ohdsi/webapi/service/CDMResultsService.java +++ b/src/main/java/org/ohdsi/webapi/service/CDMResultsService.java @@ -303,12 +303,11 @@ public void clearCacheForSource(@PathVariable("sourceKey") final String sourceKe * Clear the cdm_cache and achilles_cache for all sources * * @summary Clear the cdm_cache and achilles_cache for all sources - * @throws ResponseStatusException if the user is not an admin */ @PostMapping(value = "/clearCache") @Transactional() + @PreAuthorize("isPermitted('admin:cache')") public void clearCache() { - //TODO: Add admin permission check cacheService.clearCache(); cdmCacheService.clearCache(); } diff --git a/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java b/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java index 2bfa972d6..4ff82de81 100644 --- a/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java +++ b/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java @@ -8,6 +8,10 @@ import java.util.Date; import java.util.UUID; +import org.springframework.http.HttpEntity; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; + import org.junit.Before; import org.junit.Test; import org.ohdsi.webapi.security.authc.JwtService; @@ -168,6 +172,25 @@ protected ResponseEntity getAsLimitedUser(String path) { return limitedClient.getForEntity(getBaseUri() + path, String.class); } + /** + * Issues a POST request to {@code getBaseUri() + path} as the limited user + * (authenticated but with no source grant or admin permissions), with an + * empty body. + * + * @param path the path relative to the WebAPI base URI + * @return the raw HTTP response + */ + protected ResponseEntity postAsLimitedUser(String path) { + TestRestTemplate limitedClient = new TestRestTemplate(); + limitedClient.getRestTemplate().getInterceptors().add((request, body, execution) -> { + request.getHeaders().set("Authorization", "Bearer " + limitedUserJwt); + return execution.execute(request, body); + }); + HttpHeaders headers = new HttpHeaders(); + HttpEntity requestEntity = new HttpEntity<>(null, headers); + return limitedClient.exchange(getBaseUri() + path, HttpMethod.POST, requestEntity, String.class); + } + // --------------------------------------------------------------------------- // Validating tests // --------------------------------------------------------------------------- @@ -245,4 +268,24 @@ public void limitedUserDeniedCdmResultsPerson() { HttpStatus.valueOf(r.getStatusCode().value()) ); } + + /** + * A limited user (authenticated, but without admin:cache permission) must + * receive HTTP 403 Forbidden on the global CDMResultsService cache-clear + * endpoint ({@code POST /cdmresults/clearCache}). + * + *

This test is RED until {@code CDMResultsService#clearCache} is + * annotated with + * {@code @PreAuthorize("isPermitted('admin:cache')")} + * and GREEN once that annotation is in place. + */ + @Test + public void limitedUserDeniedGlobalCdmResultsCacheClear() { + ResponseEntity r = postAsLimitedUser("/cdmresults/clearCache"); + assertEquals( + "Limited user (no admin:cache permission) should be denied with 403 Forbidden on global CDMResults cache clear", + HttpStatus.FORBIDDEN, + HttpStatus.valueOf(r.getStatusCode().value()) + ); + } } From 0ed259626971f0eef410ac56e999f5420c41762b Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 14:55:56 +0800 Subject: [PATCH 13/22] fix(security): require source access for vocabulary endpoints --- .../webapi/service/VocabularyService.java | 26 +++++++++++++++++++ .../org/ohdsi/webapi/test/SourceAccessIT.java | 18 +++++++++++++ 2 files changed, 44 insertions(+) diff --git a/src/main/java/org/ohdsi/webapi/service/VocabularyService.java b/src/main/java/org/ohdsi/webapi/service/VocabularyService.java index 1d33ade01..8a6ab78f5 100644 --- a/src/main/java/org/ohdsi/webapi/service/VocabularyService.java +++ b/src/main/java/org/ohdsi/webapi/service/VocabularyService.java @@ -69,6 +69,7 @@ import org.springframework.jdbc.core.RowCallbackHandler; import org.springframework.jdbc.core.RowMapper; import org.springframework.stereotype.Component; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.bind.annotation.*; import org.springframework.web.server.ResponseStatusException; import org.ohdsi.webapi.vocabulary.MappedRelatedConcept; @@ -195,6 +196,7 @@ public ConceptSetExport exportConceptSet(ConceptSet conceptSet, SourceInfo vocab * @param ids Concepts identifiers from concept set * @return A map of the form: {id -> List} */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/lookup/identifiers/ancestors", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) @@ -270,6 +272,7 @@ protected PreparedStatementRenderer prepareAscendantsCalculating(Long[] identifi * @param identifiers an array of concept identifiers * @return A collection of concepts */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/lookup/identifiers", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) @@ -353,6 +356,7 @@ public Collection executeIncludedConceptLookup(String sourceKey, Concep * @param sourcecodes array of source codes * @return A collection of concepts */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/lookup/sourcecodes", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) @@ -409,6 +413,7 @@ public Collection executeSourcecodeLookup(@RequestBody String[] sourcec * @param identifiers an array of concept identifiers * @return A collection of concepts */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/lookup/mapped", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) @@ -492,6 +497,7 @@ public Collection executeMappedLookup(String sourceKey, ConceptSetExpre * @param search The ConceptSearch parameters * @return A collection of concepts */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/search", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) @@ -673,6 +679,7 @@ public Collection executeSearch(@RequestBody ConceptSearch search) { * @param query The query to use to search for concepts * @return A collection of concepts */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/search/{query}", produces = MediaType.APPLICATION_JSON_VALUE) public Collection executeSearch( @@ -691,6 +698,7 @@ public Collection executeSearch( * @param rows The number of rows to return. * @return A collection of concepts */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/search", produces = MediaType.APPLICATION_JSON_VALUE, params = "query") @@ -758,6 +766,7 @@ public Collection executeSearch(@PathVariable("query") String query) { * @param id The concept ID to find * @return The concept details */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/concept/{id}", produces = MediaType.APPLICATION_JSON_VALUE) @Cacheable(cacheNames = CachingSetup.CONCEPT_DETAIL_CACHE, key = "#sourceKey.concat('/').concat(#id)") @@ -810,6 +819,7 @@ public Concept getConcept(@PathVariable("id") final long id) { * @param id The concept ID to find * @return A collection of related concepts */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/concept/{id}/related", produces = MediaType.APPLICATION_JSON_VALUE) @Cacheable(cacheNames = CachingSetup.CONCEPT_RELATED_CACHE, key = "#sourceKey.concat('/').concat(#id)") @@ -829,6 +839,7 @@ public Collection getRelatedConcepts( return concepts.values(); } + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/related-standard", produces = MediaType.APPLICATION_JSON_VALUE) public Collection getRelatedStandardMappedConcepts( @@ -907,6 +918,7 @@ void enrichResultCombinedMappedConcepts(Map resultCo * @param id The concept ID * @return A collection of related concepts */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/concept/{id}/ancestorAndDescendant", produces = MediaType.APPLICATION_JSON_VALUE) @Cacheable(cacheNames = CachingSetup.CONCEPT_HIERARCHY_CACHE, key = "#sourceKey.concat('/').concat(#id)") @@ -955,6 +967,7 @@ public Collection getRelatedConcepts(@PathVariable("id") final L * @param identifiers An array of concept identifiers * @return A collection of related concepts */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/commonAncestors", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) @@ -1015,6 +1028,7 @@ public Collection getCommonAncestors(@RequestBody Object[] ident * @param conceptSetExpression A concept set expression * @return A collection of concept identifiers */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/resolveConceptSetExpression", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) @@ -1063,6 +1077,7 @@ public Collection resolveConceptSetExpression(@RequestBody ConceptSetExpre * @param conceptSetExpression A concept set expression * @return A count of included concepts */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/included-concepts/count", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) @@ -1123,6 +1138,7 @@ public String getConceptSetExpressionSQL(@RequestBody ConceptSetExpression conce * @param id The concept identifier * @return A collection of concepts */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/concept/{id}/descendants", produces = MediaType.APPLICATION_JSON_VALUE) public Collection getDescendantConcepts( @@ -1171,6 +1187,7 @@ public Collection getDescendantConcepts(@PathVariable("id") fina * @param sourceKey The source containing the vocabulary * @return A collection of domains */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/domains", produces = MediaType.APPLICATION_JSON_VALUE) public Collection getDomains(@PathVariable("sourceKey") String sourceKey) { @@ -1217,6 +1234,7 @@ public Collection getDomains() { * @param sourceKey The source containing the vocabulary * @return A collection of vocabularies */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/vocabularies", produces = MediaType.APPLICATION_JSON_VALUE) public Collection getVocabularies(@PathVariable("sourceKey") String sourceKey) { @@ -1294,6 +1312,7 @@ private void addRelationships(final Map concepts, final Re * @param sourceKey The source containing the vocabulary * @return The vocabulary info */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/info", produces = MediaType.APPLICATION_JSON_VALUE) public VocabularyInfo getInfo(@PathVariable("sourceKey") String sourceKey) { @@ -1345,6 +1364,7 @@ public void clearCaches() { * @param search The descendant of ancestor search object * @return A collection of concepts */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/descendantofancestor", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) @@ -1399,6 +1419,7 @@ public Collection getDescendantOfAncestorConcepts(@RequestBody Descende * @param search The concept identifiers of interest * @return A collection of concepts */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/relatedconcepts", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) @@ -1464,6 +1485,7 @@ public Collection getRelatedConcepts(@RequestBody RelatedConceptSearch * @param conceptList The list of concept identifiers * @return A collection of concepts */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/conceptlist/descendants", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) @@ -1520,6 +1542,7 @@ protected PreparedStatementRenderer prepareGetDescendantConceptsByList(String[] * @param conceptList The list of concept identifiers * @return A collection of recommended concepts */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/lookup/recommended", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) @@ -1580,6 +1603,7 @@ private void addRecommended(Map concepts, ResultSet re * @param conceptSetExpressionList Expects a list of exactly 2 concept set expressions * @return A collection of concept set comparisons */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/compare", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) @@ -1611,6 +1635,7 @@ public Collection compareConceptSets( return returnVal; } + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/compare-arbitrary", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) @@ -1693,6 +1718,7 @@ public ConceptSetOptimizationResult optimizeConceptSet( * @param conceptSetExpression The concept set expression to optimize * @return A concept set optimization */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/optimize", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) diff --git a/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java b/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java index 4ff82de81..955ff235d 100644 --- a/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java +++ b/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java @@ -269,6 +269,24 @@ public void limitedUserDeniedCdmResultsPerson() { ); } + /** + * A limited user (authenticated, but with no source grant) must receive + * HTTP 403 Forbidden when accessing a source-scoped VocabularyService endpoint. + * + *

This test is RED until Task 4 adds + * {@code @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)")} + * to source-scoped handlers in {@code VocabularyService}. + */ + @Test + public void limitedUserDeniedVocabularySourceScopedConcept() { + ResponseEntity r = getAsLimitedUser("/vocabulary/" + SOURCE_KEY + "/concept/1"); + assertEquals( + "Limited user (no source grant) should be denied with 403 Forbidden on source-scoped VocabularyService endpoint", + HttpStatus.FORBIDDEN, + HttpStatus.valueOf(r.getStatusCode().value()) + ); + } + /** * A limited user (authenticated, but without admin:cache permission) must * receive HTTP 403 Forbidden on the global CDMResultsService cache-clear From eea48f24f80c218437d69b70adbd03899777d526 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 15:01:49 +0800 Subject: [PATCH 14/22] fix(security): require source access for evidence endpoints --- .../ohdsi/webapi/service/EvidenceService.java | 17 ++++++++++++++++ .../org/ohdsi/webapi/test/SourceAccessIT.java | 20 +++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/src/main/java/org/ohdsi/webapi/service/EvidenceService.java b/src/main/java/org/ohdsi/webapi/service/EvidenceService.java index 0ac772914..8dda86f25 100644 --- a/src/main/java/org/ohdsi/webapi/service/EvidenceService.java +++ b/src/main/java/org/ohdsi/webapi/service/EvidenceService.java @@ -61,6 +61,7 @@ import org.springframework.http.ResponseEntity; import org.springframework.jdbc.core.JdbcTemplate; import org.springframework.jdbc.core.RowMapper; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.bind.annotation.*; /** @@ -220,6 +221,7 @@ public Collection searchDrugLabels(@PathVariable("searchTerm") String * @param sourceKey The source key containing the CEM daimon * @return A collection of evidence information stored in CEM */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/info", produces = MediaType.APPLICATION_JSON_VALUE) public Collection getInfo(@PathVariable("sourceKey") String sourceKey) { Source source = getSourceRepository().findBySourceKey(sourceKey); @@ -251,6 +253,7 @@ public Collection getInfo(@PathVariable("sourceKey") String source * @param searchParams * @return */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/drugconditionpairs", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) public Collection getDrugConditionPairs(@PathVariable("sourceKey") String sourceKey, @RequestBody DrugConditionSourceSearchParams searchParams) { Source source = getSourceRepository().findBySourceKey(sourceKey); @@ -285,6 +288,7 @@ public Collection getDrugConditionPairs(@PathVariable("sourceKe * @param id - An RxNorm Drug Concept Id * @return A list of evidence */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/drug/{id}", produces = MediaType.APPLICATION_JSON_VALUE) public Collection getDrugEvidence(@PathVariable("sourceKey") String sourceKey, @PathVariable("id") final Long id) { Source source = getSourceRepository().findBySourceKey(sourceKey); @@ -322,6 +326,7 @@ public Collection getDrugEvidence(@PathVariable("sourceKey") Strin * @param id The conceptId for the health outcome of interest * @return A list of evidence */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/hoi/{id}", produces = MediaType.APPLICATION_JSON_VALUE) public Collection getHoiEvidence(@PathVariable("sourceKey") String sourceKey, @PathVariable("id") final Long id) { Source source = getSourceRepository().findBySourceKey(sourceKey); @@ -359,6 +364,7 @@ public Collection getHoiEvidence(@PathVariable("sourceKey") String * @param identifiers The list of RxNorm Ingredients concepts or ancestors * @return A list of evidence for the drug and HOI */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/druglabel", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) public Collection getDrugIngredientLabel(@PathVariable("sourceKey") String sourceKey, @RequestBody long[] identifiers) { Source source = getSourceRepository().findBySourceKey(sourceKey); @@ -374,6 +380,7 @@ public Collection getDrugIngredientLabel(@PathVariable("sourceKey * @param key The key must be structured as {drugConceptId}-{hoiConceptId} * @return A list of evidence for the drug and HOI */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/drughoi/{key}", produces = MediaType.APPLICATION_JSON_VALUE) public List getDrugHoiEvidence(@PathVariable("sourceKey") String sourceKey, @PathVariable("key") final String key) { Source source = getSourceRepository().findBySourceKey(sourceKey); @@ -417,6 +424,7 @@ public List getDrugHoiEvidence(@PathVariable("sourceKey") Strin * drug, branded drug) * @return A list of evidence rolled up */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/drugrollup/{filter}/{id}", produces = MediaType.APPLICATION_JSON_VALUE) public ResponseEntity> getDrugRollupIngredientEvidence(@PathVariable("sourceKey") String sourceKey, @PathVariable("id") final Long id, @PathVariable("filter") final String filter) { String warningMessage = "This method will be deprecated in the next release. Instead, please use the new REST endpoint: evidence/{sourceKey}/drug/{id}"; @@ -435,6 +443,7 @@ public ResponseEntity> getDrugRollupIngredientEvid * @param id The conceptId of interest * @return A list of evidence matching the conceptId of interest */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/{id}", produces = MediaType.APPLICATION_JSON_VALUE) public Collection getEvidence(@PathVariable("sourceKey") String sourceKey, @PathVariable("id") final Long id) { Source source = getSourceRepository().findBySourceKey(sourceKey); @@ -478,6 +487,7 @@ public Collection getEvidence(@PathVariable("sourceKey") String source * @param evidenceGroup The evidence group * @return A summary of evidence */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/evidencesummary", produces = MediaType.APPLICATION_JSON_VALUE) public ResponseEntity> getEvidenceSummaryBySource(@PathVariable("sourceKey") String sourceKey, @RequestParam(required = false) String conditionID, @RequestParam(required = false) String drugID, @RequestParam(required = false) String evidenceGroup) { String warningMessage = "This method will be deprecated in the next release. Instead, please use the new REST endpoint: evidence/{sourceKey}/drug/{id}"; @@ -500,6 +510,7 @@ public ResponseEntity> getEvidenceSummaryBySource(@Pa * @throws org.codehaus.jettison.json.JSONException * @throws java.io.IOException */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/evidencedetails", produces = MediaType.APPLICATION_JSON_VALUE) public ResponseEntity> getEvidenceDetails(@PathVariable("sourceKey") String sourceKey, @RequestParam(required = false) String conditionID, @@ -524,6 +535,7 @@ public ResponseEntity> getEvidenceDetails(@PathVariab * @throws JSONException * @throws IOException */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/spontaneousreports", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) public ResponseEntity> getSpontaneousReports(@PathVariable("sourceKey") String sourceKey, @RequestBody EvidenceSearch search) throws JSONException, IOException { String warningMessage = "This method will be deprecated in the next release."; @@ -544,6 +556,7 @@ public ResponseEntity> getSpontaneousReports(@PathV * @throws JSONException * @throws IOException */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/evidencesearch", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) public ResponseEntity> evidenceSearch(@PathVariable("sourceKey") String sourceKey, @RequestBody EvidenceSearch search) throws JSONException, IOException { String warningMessage = "This method will be deprecated in the next release."; @@ -564,6 +577,7 @@ public ResponseEntity> evidenceSearch(@PathVariable( * @throws JSONException * @throws IOException */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @PostMapping(value = "/{sourceKey}/labelevidence", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) public ResponseEntity> labelEvidence(@PathVariable("sourceKey") String sourceKey, @RequestBody EvidenceSearch search) throws JSONException, IOException { String warningMessage = "This method will be deprecated in the next release."; @@ -583,6 +597,7 @@ public ResponseEntity> labelEvidence(@PathVariable(" * @return information about the negative control job * @throws Exception */ + @PreAuthorize("isPermitted('write:source') or hasSourceAccess(#sourceKey, WRITE)") @PostMapping(value = "/{sourceKey}/negativecontrols", produces = MediaType.APPLICATION_JSON_VALUE, consumes = MediaType.APPLICATION_JSON_VALUE) public JobExecutionResource queueNegativeControlsJob(@PathVariable("sourceKey") String sourceKey, @RequestBody NegativeControlTaskParameters task) throws Exception { if (task == null) { @@ -681,6 +696,7 @@ public JobExecutionResource queueNegativeControlsJob(@PathVariable("sourceKey") * @param conceptSetId The concept set id * @return The list of negative controls */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/negativecontrols/{conceptsetid}", produces = MediaType.APPLICATION_JSON_VALUE) public Collection getNegativeControls(@PathVariable("sourceKey") String sourceKey, @PathVariable("conceptsetid") int conceptSetId) throws Exception { Source source = getSourceRepository().findBySourceKey(sourceKey); @@ -696,6 +712,7 @@ public Collection getNegativeControls(@PathVariable("sourceK * @param sourceKey The source key of the CEM daimon * @return The list of negative controls */ + @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)") @GetMapping(value = "/{sourceKey}/negativecontrols/sql", produces = MediaType.TEXT_PLAIN_VALUE) public String getNegativeControlsSqlStatement(@PathVariable("sourceKey") String sourceKey, @RequestParam(defaultValue = "CONDITION") String conceptDomain, diff --git a/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java b/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java index 955ff235d..44ed3e599 100644 --- a/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java +++ b/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java @@ -306,4 +306,24 @@ public void limitedUserDeniedGlobalCdmResultsCacheClear() { HttpStatus.valueOf(r.getStatusCode().value()) ); } + + /** + * A limited user (authenticated, but with no source grant) must receive + * HTTP 403 Forbidden when accessing the source-scoped EvidenceService info + * endpoint ({@code GET /evidence/{sourceKey}/info}). + * + *

This test is RED until Task 5 adds + * {@code @PreAuthorize("isAnyPermitted(anyOf('read:source','write:source')) or hasSourceAccess(#sourceKey, READ)")} + * to {@code EvidenceService#getInfo} (and all other source-scoped evidence + * handlers) and GREEN once those annotations are in place. + */ + @Test + public void limitedUserDeniedEvidenceSourceScopedInfo() { + ResponseEntity r = getAsLimitedUser("/evidence/" + SOURCE_KEY + "/info"); + assertEquals( + "Limited user (no source grant) should be denied with 403 Forbidden on source-scoped EvidenceService info endpoint", + HttpStatus.FORBIDDEN, + HttpStatus.valueOf(r.getStatusCode().value()) + ); + } } From 632d41d1eff224208a083edbd8939eee0c3f924a Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 15:07:14 +0800 Subject: [PATCH 15/22] fix(security): require admin for statistic endpoints --- .../statistic/service/StatisticService.java | 3 ++ .../org/ohdsi/webapi/test/SourceAccessIT.java | 45 +++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/src/main/java/org/ohdsi/webapi/statistic/service/StatisticService.java b/src/main/java/org/ohdsi/webapi/statistic/service/StatisticService.java index f7c884951..4d912d7f1 100644 --- a/src/main/java/org/ohdsi/webapi/statistic/service/StatisticService.java +++ b/src/main/java/org/ohdsi/webapi/statistic/service/StatisticService.java @@ -16,6 +16,7 @@ import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestMapping; @@ -269,6 +270,7 @@ private LocalDate getFileDate(Path path) { * @param executionStatisticsRequest filter settings for statistics * @return execution statistics in JSON or CSV format */ + @PreAuthorize("isPermitted('admin')") @PostMapping( value = "/executions", produces = MediaType.APPLICATION_JSON_VALUE, @@ -301,6 +303,7 @@ public ResponseEntity executionStatistics(@RequestBody ExecutionStatisticsReq * @param accessTrendsStatisticsRequest filter settings for statistics * @return access trends statistics in JSON or CSV format */ + @PreAuthorize("isPermitted('admin')") @PostMapping( value = "/accesstrends", produces = MediaType.APPLICATION_JSON_VALUE, diff --git a/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java b/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java index 44ed3e599..2787dd7c5 100644 --- a/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java +++ b/src/test/java/org/ohdsi/webapi/test/SourceAccessIT.java @@ -191,6 +191,28 @@ protected ResponseEntity postAsLimitedUser(String path) { return limitedClient.exchange(getBaseUri() + path, HttpMethod.POST, requestEntity, String.class); } + /** + * Issues a POST request with {@code Content-Type: application/json} and an + * empty JSON object body ({@code {}}) as the limited user. Use this variant + * for endpoints that require a JSON content type so that Spring's dispatcher + * routes the request to the handler method — allowing {@code @PreAuthorize} + * to fire — rather than rejecting it with 415 before security runs. + * + * @param path the path relative to the WebAPI base URI + * @return the raw HTTP response + */ + protected ResponseEntity postAsLimitedUserJson(String path) { + TestRestTemplate limitedClient = new TestRestTemplate(); + limitedClient.getRestTemplate().getInterceptors().add((request, body, execution) -> { + request.getHeaders().set("Authorization", "Bearer " + limitedUserJwt); + return execution.execute(request, body); + }); + HttpHeaders headers = new HttpHeaders(); + headers.setContentType(org.springframework.http.MediaType.APPLICATION_JSON); + HttpEntity requestEntity = new HttpEntity<>("{}", headers); + return limitedClient.exchange(getBaseUri() + path, HttpMethod.POST, requestEntity, String.class); + } + // --------------------------------------------------------------------------- // Validating tests // --------------------------------------------------------------------------- @@ -326,4 +348,27 @@ public void limitedUserDeniedEvidenceSourceScopedInfo() { HttpStatus.valueOf(r.getStatusCode().value()) ); } + + /** + * A limited user (authenticated, but without admin permission) must receive + * HTTP 403 Forbidden on the StatisticService executions endpoint + * ({@code POST /statistic/executions}). + * + *

This test is RED until Task 6 adds + * {@code @PreAuthorize("isPermitted('admin')")} to all 3 handlers in + * {@code StatisticService} and GREEN once those annotations are in place. + * + *

Note: an empty POST body may yield 400 before annotation (body binding + * runs before security); after annotation it must be 403 because the + * security gate runs first. + */ + @Test + public void limitedUserDeniedStatisticExecutions() { + ResponseEntity r = postAsLimitedUserJson("/statistic/executions"); + assertEquals( + "Limited user (no admin permission) should be denied with 403 Forbidden on StatisticService executions endpoint", + HttpStatus.FORBIDDEN, + HttpStatus.valueOf(r.getStatusCode().value()) + ); + } } From a68d4f07b168f25760e56648f4427b53e8d24927 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 15:24:25 +0800 Subject: [PATCH 16/22] fix(security): route internal callers to ungated vocabulary/treemap methods 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. --- .../org/ohdsi/webapi/cdmresults/AchillesCacheTasklet.java | 3 ++- .../java/org/ohdsi/webapi/conceptset/ConceptSetService.java | 2 +- .../java/org/ohdsi/webapi/service/VocabularyService.java | 6 ++++++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/ohdsi/webapi/cdmresults/AchillesCacheTasklet.java b/src/main/java/org/ohdsi/webapi/cdmresults/AchillesCacheTasklet.java index 1ec30b3b4..70466482e 100644 --- a/src/main/java/org/ohdsi/webapi/cdmresults/AchillesCacheTasklet.java +++ b/src/main/java/org/ohdsi/webapi/cdmresults/AchillesCacheTasklet.java @@ -145,7 +145,8 @@ private void cacheDrilldown(String domain) { } private List getConceptIds(String domain) { - ArrayNode treeMap = service.getTreemap(domain, source.getSourceKey()); + // call ungated raw method: this runs in a batch job with no security context + ArrayNode treeMap = service.getRawTreeMap(domain, source.getSourceKey()); Stream nodes = IntStream.range(0, treeMap.size()).mapToObj(treeMap::get); return nodes.map(node -> node.get("conceptId").intValue()) .distinct() diff --git a/src/main/java/org/ohdsi/webapi/conceptset/ConceptSetService.java b/src/main/java/org/ohdsi/webapi/conceptset/ConceptSetService.java index 7d9ffeff0..a92542fd1 100644 --- a/src/main/java/org/ohdsi/webapi/conceptset/ConceptSetService.java +++ b/src/main/java/org/ohdsi/webapi/conceptset/ConceptSetService.java @@ -333,7 +333,7 @@ private ConceptSetExpression getConceptSetExpressionInternal(int id, Integer ver sourceKey = sourceInfo.sourceKey; } - Collection concepts = vocabService.executeIdentifierLookup(sourceKey, identifiers); + Collection concepts = vocabService.executeIdentifierLookupInternal(sourceKey, identifiers); if (concepts.size() != identifiers.length) { String ids = Arrays.stream(identifiers).boxed() .filter(identifier -> concepts.stream().noneMatch(c -> c.conceptId.equals(identifier))) diff --git a/src/main/java/org/ohdsi/webapi/service/VocabularyService.java b/src/main/java/org/ohdsi/webapi/service/VocabularyService.java index 8a6ab78f5..4e7b336e4 100644 --- a/src/main/java/org/ohdsi/webapi/service/VocabularyService.java +++ b/src/main/java/org/ohdsi/webapi/service/VocabularyService.java @@ -283,6 +283,12 @@ public Collection executeIdentifierLookup( return executeIdentifierLookup(source, identifiers); } + // Ungated variant for internal (non-HTTP) callers; HTTP access goes through the @PreAuthorize-gated overload. + public Collection executeIdentifierLookupInternal(String sourceKey, long[] identifiers) { + Source source = getSourceRepository().findBySourceKey(sourceKey); + return executeIdentifierLookup(source, identifiers); + } + protected Collection executeIdentifierLookup(Source source, long[] identifiers) { Collection concepts = new ArrayList<>(); if (identifiers.length == 0) { From 3e3d9b8054740d9dabe2c1f8db419ef270a562f7 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 15:39:37 +0800 Subject: [PATCH 17/22] feat(security): register GraalVM reflection hints for method-security 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. --- .../security/spring/SpringSecurityConfig.java | 2 ++ .../spring/WebApiSecurityRuntimeHints.java | 30 +++++++++++++++++++ 2 files changed, 32 insertions(+) create mode 100644 src/main/java/org/ohdsi/webapi/security/spring/WebApiSecurityRuntimeHints.java diff --git a/src/main/java/org/ohdsi/webapi/security/spring/SpringSecurityConfig.java b/src/main/java/org/ohdsi/webapi/security/spring/SpringSecurityConfig.java index 4c489d014..76e2964b8 100644 --- a/src/main/java/org/ohdsi/webapi/security/spring/SpringSecurityConfig.java +++ b/src/main/java/org/ohdsi/webapi/security/spring/SpringSecurityConfig.java @@ -3,6 +3,7 @@ import org.ohdsi.webapi.security.authz.AuthorizationService; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.ImportRuntimeHints; import org.springframework.security.access.expression.method.MethodSecurityExpressionHandler; import org.springframework.security.config.annotation.method.configuration.EnableMethodSecurity; import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; @@ -10,6 +11,7 @@ @Configuration @EnableWebSecurity @EnableMethodSecurity +@ImportRuntimeHints(WebApiSecurityRuntimeHints.class) public class SpringSecurityConfig { @Bean diff --git a/src/main/java/org/ohdsi/webapi/security/spring/WebApiSecurityRuntimeHints.java b/src/main/java/org/ohdsi/webapi/security/spring/WebApiSecurityRuntimeHints.java new file mode 100644 index 000000000..4b30fe2c4 --- /dev/null +++ b/src/main/java/org/ohdsi/webapi/security/spring/WebApiSecurityRuntimeHints.java @@ -0,0 +1,30 @@ +package org.ohdsi.webapi.security.spring; + +import org.ohdsi.webapi.security.authz.access.AccessType; +import org.ohdsi.webapi.security.authz.access.EntityType; +import org.springframework.aot.hint.MemberCategory; +import org.springframework.aot.hint.RuntimeHints; +import org.springframework.aot.hint.RuntimeHintsRegistrar; + +/** + * GraalVM native-image reflection hints for method-security SpEL evaluation. + * + *

{@code @PreAuthorize} expressions reference public fields on + * {@link WebApiSecurityExpressionRoot} (e.g. {@code READ}, {@code WRITE}, + * {@code SOURCE}, {@code CONCEPT_SET}) and the {@link AccessType}/{@link EntityType} + * enums. SpEL resolves these reflectively at evaluation time, so the types — including + * their fields — must be reflection-reachable in a native image, otherwise expression + * evaluation fails at runtime. + * + *

A no-op on a regular JVM; consumed only by GraalVM AOT processing. + */ +public class WebApiSecurityRuntimeHints implements RuntimeHintsRegistrar { + + @Override + public void registerHints(RuntimeHints hints, ClassLoader classLoader) { + hints.reflection() + .registerType(WebApiSecurityExpressionRoot.class, MemberCategory.values()) + .registerType(AccessType.class, MemberCategory.values()) + .registerType(EntityType.class, MemberCategory.values()); + } +} From d2b6836dc45a4d4a9b21ce4906ac37fee71120ad Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 19:45:11 +0800 Subject: [PATCH 18/22] test(security): guard that no endpoint is reachable without authentication --- .../webapi/test/EndpointAuthCoverageIT.java | 104 ++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 src/test/java/org/ohdsi/webapi/test/EndpointAuthCoverageIT.java diff --git a/src/test/java/org/ohdsi/webapi/test/EndpointAuthCoverageIT.java b/src/test/java/org/ohdsi/webapi/test/EndpointAuthCoverageIT.java new file mode 100644 index 000000000..36f50b1d3 --- /dev/null +++ b/src/test/java/org/ohdsi/webapi/test/EndpointAuthCoverageIT.java @@ -0,0 +1,104 @@ +package org.ohdsi.webapi.test; + +import static org.junit.Assert.assertTrue; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import org.junit.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.boot.test.web.client.TestRestTemplate; +import org.springframework.http.HttpEntity; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; +import org.springframework.http.MediaType; +import org.springframework.http.ResponseEntity; +import org.springframework.web.bind.annotation.RequestMethod; +import org.springframework.web.method.HandlerMethod; +import org.springframework.web.servlet.mvc.method.RequestMappingInfo; +import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping; + +/** + * Codebase-wide guards: no org.ohdsi.webapi endpoint may be reachable without + * authentication (except a pinned allow-list), and every source-scoped handler + * must carry @PreAuthorize. Driven by the live handler set, so future endpoints + * are covered automatically. + */ +public class EndpointAuthCoverageIT extends WebApiIT { + + // Reachable before login by design. Adding to this list is a deliberate, + // reviewable change — keep it minimal. + static final List ANONYMOUS_ALLOW_LIST = List.of( + "/info", "/auth/providers", "/i18n", "/user/login", "/user/oauth/callback"); + + @Autowired + @Qualifier("requestMappingHandlerMapping") + private RequestMappingHandlerMapping handlerMapping; + + private final TestRestTemplate anonymous = new TestRestTemplate(); + + @Test + public void everyEndpointRejectsAnonymousExceptAllowList() { + List leaks = new ArrayList<>(); + for (Map.Entry e : handlerMapping.getHandlerMethods().entrySet()) { + HandlerMethod hm = e.getValue(); + if (!hm.getBeanType().getName().startsWith("org.ohdsi.webapi")) { + continue; + } + HttpMethod method = pickMethod(e.getKey()); + for (String pattern : patternsOf(e.getKey())) { + if (isAllowListed(pattern)) { + continue; + } + String url = getBaseUri() + substitutePathVars(pattern); + ResponseEntity resp = anonymous.exchange( + url, method, new HttpEntity<>(jsonHeaders()), String.class); + int sc = resp.getStatusCode().value(); + if (sc != 401 && sc != 403) { + leaks.add(method + " " + pattern + " -> " + sc); + } + } + } + assertTrue( + "Endpoints reachable without authentication (expected 401/403):\n " + + String.join("\n ", leaks), + leaks.isEmpty()); + } + + static Set patternsOf(RequestMappingInfo info) { + if (info.getPathPatternsCondition() != null) { + return info.getPathPatternsCondition().getPatternValues(); + } + return info.getPatternsCondition().getPatterns(); + } + + static boolean isAllowListed(String pattern) { + for (String p : ANONYMOUS_ALLOW_LIST) { + if (pattern.equals(p) || pattern.startsWith(p + "/")) { + return true; + } + } + return false; + } + + static String substitutePathVars(String pattern) { + return pattern.replaceAll("\\{[^/}]+\\}", "x").replace("/**", "/x").replace("/*", "/x"); + } + + static HttpMethod pickMethod(RequestMappingInfo info) { + Set methods = info.getMethodsCondition().getMethods(); + if (methods.isEmpty()) { + return HttpMethod.GET; + } + return HttpMethod.valueOf(methods.iterator().next().name()); + } + + static HttpHeaders jsonHeaders() { + HttpHeaders h = new HttpHeaders(); + h.setContentType(MediaType.APPLICATION_JSON); + return h; + } +} From 187189bbc626dd4ff4c1ea0f0f15f227aa96d8b7 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Fri, 19 Jun 2026 19:50:12 +0800 Subject: [PATCH 19/22] test(security): guard that every source-scoped handler requires authorization --- .../webapi/test/EndpointAuthCoverageIT.java | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/test/java/org/ohdsi/webapi/test/EndpointAuthCoverageIT.java b/src/test/java/org/ohdsi/webapi/test/EndpointAuthCoverageIT.java index 36f50b1d3..8b3628fbf 100644 --- a/src/test/java/org/ohdsi/webapi/test/EndpointAuthCoverageIT.java +++ b/src/test/java/org/ohdsi/webapi/test/EndpointAuthCoverageIT.java @@ -11,11 +11,14 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.boot.test.web.client.TestRestTemplate; +import org.springframework.core.MethodParameter; import org.springframework.http.HttpEntity; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpMethod; import org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; +import org.springframework.security.access.prepost.PreAuthorize; +import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.method.HandlerMethod; import org.springframework.web.servlet.mvc.method.RequestMappingInfo; @@ -101,4 +104,36 @@ static HttpHeaders jsonHeaders() { h.setContentType(MediaType.APPLICATION_JSON); return h; } + + @Test + public void everySourceScopedHandlerHasAuthorization() { + List unguarded = new ArrayList<>(); + for (HandlerMethod hm : handlerMapping.getHandlerMethods().values()) { + if (!hm.getBeanType().getName().startsWith("org.ohdsi.webapi")) { + continue; + } + if (!hasSourceKeyPathVariable(hm)) { + continue; + } + boolean guarded = hm.getMethod().isAnnotationPresent(PreAuthorize.class) + || hm.getBeanType().isAnnotationPresent(PreAuthorize.class); + if (!guarded) { + unguarded.add(hm.getBeanType().getSimpleName() + "#" + hm.getMethod().getName()); + } + } + assertTrue( + "Source-scoped handlers (@PathVariable(\"sourceKey\")) missing @PreAuthorize:\n " + + String.join("\n ", unguarded), + unguarded.isEmpty()); + } + + private static boolean hasSourceKeyPathVariable(HandlerMethod hm) { + for (MethodParameter p : hm.getMethodParameters()) { + PathVariable pv = p.getParameterAnnotation(PathVariable.class); + if (pv != null && ("sourceKey".equals(pv.value()) || "sourceKey".equals(pv.name()))) { + return true; + } + } + return false; + } } From e024b785a694880f891273f9817423cea27f72ba Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Sat, 20 Jun 2026 17:59:52 +0800 Subject: [PATCH 20/22] ci: run security authorization guardrail tests 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. --- .github/workflows/ci.yaml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 1c0c0b427..304d20896 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -42,6 +42,12 @@ jobs: - name: Test run: mvn -B test + # Security authorization guardrails (embedded PostgreSQL, no external services): + # - EndpointAuthCoverageIT: every endpoint rejects anonymous (401/403) except the allow-list + # - SecurityIT / SourceAccessIT: default-deny + per-source authorization + - name: Security integration tests + run: mvn -B -P webapi-postgresql -DskipUnitTests=true -Dit.test=EndpointAuthCoverageIT,SecurityIT,SourceAccessIT verify + # Check that the docker image builds correctly # Push to ghcr.io for commits on master or webapi-3.0. docker: From a469c9db4b6c932d36954278944c8a0e607928c8 Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Sat, 20 Jun 2026 18:12:25 +0800 Subject: [PATCH 21/22] test(security): static @PreAuthorize coverage guard; gate missed IR endpoints 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. --- .../webapi/ircalc/IRAnalysisService.java | 5 + .../webapi/test/EndpointAuthCoverageIT.java | 109 ++++++++---------- 2 files changed, 55 insertions(+), 59 deletions(-) diff --git a/src/main/java/org/ohdsi/webapi/ircalc/IRAnalysisService.java b/src/main/java/org/ohdsi/webapi/ircalc/IRAnalysisService.java index 967cb6369..2f182ffdd 100644 --- a/src/main/java/org/ohdsi/webapi/ircalc/IRAnalysisService.java +++ b/src/main/java/org/ohdsi/webapi/ircalc/IRAnalysisService.java @@ -321,6 +321,7 @@ private String getStrataTreemapData(int analysisId, int targetId, int outcomeId, } @Override + @PreAuthorize("isAnyPermitted(anyOf('read:incidence','write:incidence'))") public List getIRAnalysisList() { return getTransactionTemplate().execute(transactionStatus -> { Iterable analysisList = this.irAnalysisRepository.findAll(); @@ -338,6 +339,7 @@ public List getIRAnalysisList() { @Override @Transactional + @PreAuthorize("isOwner(#id, INCIDENCE_RATE) or isPermitted('read:incidence') or isPermitted('write:incidence') or hasEntityAccess(#id, INCIDENCE_RATE, READ)") public int getCountIRWithSameName(final int id, String name) { return irAnalysisRepository.getCountIRWithSameName(id, name); } @@ -610,6 +612,7 @@ public AnalysisReport getAnalysisReport(final int id, final String sourceKey, fi } @Override + @PreAuthorize("isAnyPermitted(anyOf('read:incidence','write:incidence'))") public GenerateSqlResult generateSql(GenerateSqlRequest request) { IRAnalysisQueryBuilder.BuildExpressionQueryOptions options = request.options; GenerateSqlResult result = new GenerateSqlResult(); @@ -623,6 +626,7 @@ public GenerateSqlResult generateSql(GenerateSqlRequest request) { } @Override + @PreAuthorize("isAnyPermitted(anyOf('read:incidence','write:incidence'))") public CheckResult runDiagnostics(IRAnalysisDTO irAnalysisDTO){ return new CheckResult(checker.check(irAnalysisDTO)); @@ -876,6 +880,7 @@ public IRAnalysisDTO copyAssetFromVersion(int id, int version) { @Override @Transactional + @PreAuthorize("isAnyPermitted(anyOf('read:incidence','write:incidence'))") public List listByTags(TagNameListRequestDTO requestDTO) { if (requestDTO == null || requestDTO.getNames() == null || requestDTO.getNames().isEmpty()) { return Collections.emptyList(); diff --git a/src/test/java/org/ohdsi/webapi/test/EndpointAuthCoverageIT.java b/src/test/java/org/ohdsi/webapi/test/EndpointAuthCoverageIT.java index 8b3628fbf..ba73b872d 100644 --- a/src/test/java/org/ohdsi/webapi/test/EndpointAuthCoverageIT.java +++ b/src/test/java/org/ohdsi/webapi/test/EndpointAuthCoverageIT.java @@ -10,13 +10,8 @@ import org.junit.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Qualifier; -import org.springframework.boot.test.web.client.TestRestTemplate; import org.springframework.core.MethodParameter; -import org.springframework.http.HttpEntity; -import org.springframework.http.HttpHeaders; import org.springframework.http.HttpMethod; -import org.springframework.http.MediaType; -import org.springframework.http.ResponseEntity; import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.RequestMethod; @@ -25,50 +20,78 @@ import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping; /** - * Codebase-wide guards: no org.ohdsi.webapi endpoint may be reachable without - * authentication (except a pinned allow-list), and every source-scoped handler - * must carry @PreAuthorize. Driven by the live handler set, so future endpoints - * are covered automatically. + * Codebase-wide authorization guards driven by the live handler set, so future + * endpoints are covered automatically: + * - every org.ohdsi.webapi endpoint must carry method security (@PreAuthorize on + * the handler or its controller), except a pinned login/bootstrap allow-list; + * - every source-scoped handler (@PathVariable("sourceKey")) must carry @PreAuthorize. + * + *

These are static annotation-presence checks rather than HTTP probes: with + * {@code security.anonymousAccess.enabled=true} the filter chain admits token-less + * requests and per-endpoint @PreAuthorize is the gate, so a missing annotation — + * not the filter — is what would leave an endpoint open. (Runtime denial of + * anonymous requests is exercised by {@link SecurityIT} and {@link SourceAccessIT}.) */ public class EndpointAuthCoverageIT extends WebApiIT { // Reachable before login by design. Adding to this list is a deliberate, // reviewable change — keep it minimal. static final List ANONYMOUS_ALLOW_LIST = List.of( - "/info", "/auth/providers", "/i18n", "/user/login", "/user/oauth/callback"); + "/info", "/auth/providers", "/i18n", + "/user/login", "/user/refresh", "/user/logout", "/user/oauth/callback"); @Autowired @Qualifier("requestMappingHandlerMapping") private RequestMappingHandlerMapping handlerMapping; - private final TestRestTemplate anonymous = new TestRestTemplate(); - @Test - public void everyEndpointRejectsAnonymousExceptAllowList() { - List leaks = new ArrayList<>(); + public void everyEndpointRequiresAuthorizationOrIsAllowListed() { + List unguarded = new ArrayList<>(); for (Map.Entry e : handlerMapping.getHandlerMethods().entrySet()) { HandlerMethod hm = e.getValue(); if (!hm.getBeanType().getName().startsWith("org.ohdsi.webapi")) { continue; } - HttpMethod method = pickMethod(e.getKey()); + if (isGuarded(hm)) { + continue; + } for (String pattern : patternsOf(e.getKey())) { - if (isAllowListed(pattern)) { - continue; - } - String url = getBaseUri() + substitutePathVars(pattern); - ResponseEntity resp = anonymous.exchange( - url, method, new HttpEntity<>(jsonHeaders()), String.class); - int sc = resp.getStatusCode().value(); - if (sc != 401 && sc != 403) { - leaks.add(method + " " + pattern + " -> " + sc); + if (!isAllowListed(pattern)) { + unguarded.add(pickMethod(e.getKey()) + " " + pattern + + " (" + hm.getBeanType().getSimpleName() + "#" + hm.getMethod().getName() + ")"); } } } assertTrue( - "Endpoints reachable without authentication (expected 401/403):\n " - + String.join("\n ", leaks), - leaks.isEmpty()); + "Endpoints without @PreAuthorize and not in the anonymous allow-list " + + "(each must be method-secured or explicitly allow-listed):\n " + + String.join("\n ", unguarded), + unguarded.isEmpty()); + } + + @Test + public void everySourceScopedHandlerHasAuthorization() { + List unguarded = new ArrayList<>(); + for (HandlerMethod hm : handlerMapping.getHandlerMethods().values()) { + if (!hm.getBeanType().getName().startsWith("org.ohdsi.webapi")) { + continue; + } + if (!hasSourceKeyPathVariable(hm)) { + continue; + } + if (!isGuarded(hm)) { + unguarded.add(hm.getBeanType().getSimpleName() + "#" + hm.getMethod().getName()); + } + } + assertTrue( + "Source-scoped handlers (@PathVariable(\"sourceKey\")) missing @PreAuthorize:\n " + + String.join("\n ", unguarded), + unguarded.isEmpty()); + } + + private static boolean isGuarded(HandlerMethod hm) { + return hm.getMethod().isAnnotationPresent(PreAuthorize.class) + || hm.getBeanType().isAnnotationPresent(PreAuthorize.class); } static Set patternsOf(RequestMappingInfo info) { @@ -87,10 +110,6 @@ static boolean isAllowListed(String pattern) { return false; } - static String substitutePathVars(String pattern) { - return pattern.replaceAll("\\{[^/}]+\\}", "x").replace("/**", "/x").replace("/*", "/x"); - } - static HttpMethod pickMethod(RequestMappingInfo info) { Set methods = info.getMethodsCondition().getMethods(); if (methods.isEmpty()) { @@ -99,34 +118,6 @@ static HttpMethod pickMethod(RequestMappingInfo info) { return HttpMethod.valueOf(methods.iterator().next().name()); } - static HttpHeaders jsonHeaders() { - HttpHeaders h = new HttpHeaders(); - h.setContentType(MediaType.APPLICATION_JSON); - return h; - } - - @Test - public void everySourceScopedHandlerHasAuthorization() { - List unguarded = new ArrayList<>(); - for (HandlerMethod hm : handlerMapping.getHandlerMethods().values()) { - if (!hm.getBeanType().getName().startsWith("org.ohdsi.webapi")) { - continue; - } - if (!hasSourceKeyPathVariable(hm)) { - continue; - } - boolean guarded = hm.getMethod().isAnnotationPresent(PreAuthorize.class) - || hm.getBeanType().isAnnotationPresent(PreAuthorize.class); - if (!guarded) { - unguarded.add(hm.getBeanType().getSimpleName() + "#" + hm.getMethod().getName()); - } - } - assertTrue( - "Source-scoped handlers (@PathVariable(\"sourceKey\")) missing @PreAuthorize:\n " - + String.join("\n ", unguarded), - unguarded.isEmpty()); - } - private static boolean hasSourceKeyPathVariable(HandlerMethod hm) { for (MethodParameter p : hm.getMethodParameters()) { PathVariable pv = p.getParameterAnnotation(PathVariable.class); From 8fdcb91b7b10c5cad52abdde00a018858a537f2f Mon Sep 17 00:00:00 2001 From: Peter Hoffmann <954078+p-hoffmann@users.noreply.github.com> Date: Sat, 20 Jun 2026 18:21:37 +0800 Subject: [PATCH 22/22] test(security): add positive-path and permission-string-validity guards 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. --- .github/workflows/ci.yaml | 2 +- .../webapi/test/AuthorizationAccessIT.java | 119 ++++++++++++++++++ .../AuthorizationPermissionStringsIT.java | 86 +++++++++++++ 3 files changed, 206 insertions(+), 1 deletion(-) create mode 100644 src/test/java/org/ohdsi/webapi/test/AuthorizationAccessIT.java create mode 100644 src/test/java/org/ohdsi/webapi/test/AuthorizationPermissionStringsIT.java diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 304d20896..9753a3f19 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -46,7 +46,7 @@ jobs: # - EndpointAuthCoverageIT: every endpoint rejects anonymous (401/403) except the allow-list # - SecurityIT / SourceAccessIT: default-deny + per-source authorization - name: Security integration tests - run: mvn -B -P webapi-postgresql -DskipUnitTests=true -Dit.test=EndpointAuthCoverageIT,SecurityIT,SourceAccessIT verify + run: mvn -B -P webapi-postgresql -DskipUnitTests=true -Dit.test=EndpointAuthCoverageIT,SecurityIT,SourceAccessIT,AuthorizationAccessIT,AuthorizationPermissionStringsIT verify # Check that the docker image builds correctly # Push to ghcr.io for commits on master or webapi-3.0. diff --git a/src/test/java/org/ohdsi/webapi/test/AuthorizationAccessIT.java b/src/test/java/org/ohdsi/webapi/test/AuthorizationAccessIT.java new file mode 100644 index 000000000..220e3bc0d --- /dev/null +++ b/src/test/java/org/ohdsi/webapi/test/AuthorizationAccessIT.java @@ -0,0 +1,119 @@ +package org.ohdsi.webapi.test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import java.time.Instant; +import java.util.Date; +import java.util.UUID; + +import org.junit.Before; +import org.junit.Test; +import org.ohdsi.webapi.security.authc.JwtService; +import org.ohdsi.webapi.security.authz.AuthorizationService; +import org.ohdsi.webapi.security.session.SessionService; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.web.client.TestRestTemplate; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; + +/** + * Positive-path authorization: a user who holds the matching permission must be + * ALLOWED (not 403). Complements {@link SourceAccessIT} / {@link SecurityIT}, + * which only prove that access is denied — they cannot catch an over-strict + * {@code @PreAuthorize} that wrongly denies a legitimate user. + * + *

The "reader" user is granted the generic {@code read} permission, which + * wildcard-implies every {@code read:}; so it must reach the domain + * read/list endpoints, but must still be denied admin operations. + */ +public class AuthorizationAccessIT extends WebApiIT { + + @Autowired + private JwtService jwtService; + @Autowired + private SessionService sessionService; + @Autowired + private AuthorizationService authorizationService; + + private static final long READER_USER_ID = -3L; + private static final long READER_ROLE_ID = -101L; + private static final String READER_LOGIN = "reader"; + + private String readerJwt; + + @Before + public void setUpReader() { + String schema = getOhdsiSchema(); + + jdbcTemplate.execute( + "INSERT INTO " + schema + ".sec_user (id, login, name, origin) " + + "VALUES (" + READER_USER_ID + ", '" + READER_LOGIN + "', 'Reader User', 'SYSTEM') " + + "ON CONFLICT (login) DO NOTHING"); + jdbcTemplate.execute( + "INSERT INTO " + schema + ".sec_role (id, name, system_role) " + + "VALUES (" + READER_ROLE_ID + ", '" + READER_LOGIN + "', false) " + + "ON CONFLICT DO NOTHING"); + jdbcTemplate.execute( + "INSERT INTO " + schema + ".sec_user_role (id, user_id, role_id, origin) " + + "SELECT nextval('" + schema + ".sec_user_role_sequence'), " + READER_USER_ID + ", " + READER_ROLE_ID + ", 'SYSTEM' " + + "WHERE NOT EXISTS (SELECT 1 FROM " + schema + ".sec_user_role WHERE user_id = " + READER_USER_ID + " AND role_id = " + READER_ROLE_ID + ")"); + // Grant the generic 'read' permission to the reader's role. + jdbcTemplate.execute( + "INSERT INTO " + schema + ".sec_role_permission (id, role_id, permission_id) " + + "SELECT nextval('" + schema + ".sec_role_permission_sequence'), " + READER_ROLE_ID + ", p.id " + + "FROM " + schema + ".sec_permission p WHERE p.value = 'read' " + + "AND NOT EXISTS (SELECT 1 FROM " + schema + ".sec_role_permission rp WHERE rp.role_id = " + READER_ROLE_ID + " AND rp.permission_id = p.id)"); + + authorizationService.clearCache(); + + UUID sessionId = sessionService.createSession(READER_LOGIN); + Date expiresAt = Date.from(Instant.now().plusSeconds(3600)); + readerJwt = jwtService.generateToken(READER_LOGIN, sessionId.toString(), expiresAt); + } + + private ResponseEntity getAsReader(String path) { + TestRestTemplate client = new TestRestTemplate(); + client.getRestTemplate().getInterceptors().add((request, body, execution) -> { + request.getHeaders().set("Authorization", "Bearer " + readerJwt); + return execution.execute(request, body); + }); + return client.getForEntity(getBaseUri() + path, String.class); + } + + private void assertReaderAllowed(String path) { + ResponseEntity r = getAsReader(path); + int sc = r.getStatusCode().value(); + assertTrue( + "Reader (granted 'read') must be allowed on " + path + " — got " + sc + + " (a 401/403 here means the endpoint is over-restricted beyond a read permission)", + sc >= 200 && sc < 300); + } + + @Test + public void readerAllowedCohortDefinitionList() { + assertReaderAllowed("/cohortdefinition"); + } + + @Test + public void readerAllowedConceptSetList() { + assertReaderAllowed("/conceptset"); + } + + @Test + public void readerAllowedIncidenceRateList() { + // Also guards the IR list endpoint (interface-based controller) against + // over-restriction beyond read:incidence. + assertReaderAllowed("/ir"); + } + + @Test + public void readerDeniedAdminRoleManagement() { + // 'read' must NOT grant admin:security — listing roles stays forbidden. + ResponseEntity r = getAsReader("/role"); + assertEquals( + "Reader (granted only 'read') must be denied admin role management", + HttpStatus.FORBIDDEN, + HttpStatus.valueOf(r.getStatusCode().value())); + } +} diff --git a/src/test/java/org/ohdsi/webapi/test/AuthorizationPermissionStringsIT.java b/src/test/java/org/ohdsi/webapi/test/AuthorizationPermissionStringsIT.java new file mode 100644 index 000000000..5dd579750 --- /dev/null +++ b/src/test/java/org/ohdsi/webapi/test/AuthorizationPermissionStringsIT.java @@ -0,0 +1,86 @@ +package org.ohdsi.webapi.test; + +import static org.junit.Assert.assertTrue; + +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; +import java.util.TreeSet; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import org.junit.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.core.annotation.AnnotatedElementUtils; +import org.springframework.security.access.prepost.PreAuthorize; +import org.springframework.web.method.HandlerMethod; +import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping; + +/** + * Guards that every permission literal referenced in a {@code @PreAuthorize} + * expression (e.g. {@code isPermitted('read:incidence')}, + * {@code anyOf('read:source','write:source')}) is an actual permission seeded in + * {@code sec_permission}. A typo such as {@code read:cohortdefinition} (vs + * {@code read:cohort-definition}) would otherwise silently deny everyone while + * passing every other test. + * + *

Single-quoted string literals in these SpEL expressions are always + * permission names — entity types ({@code INCIDENCE_RATE}) and access types + * ({@code READ}/{@code WRITE}) are unquoted identifiers, so the quote-based + * extraction picks up permissions only. + */ +public class AuthorizationPermissionStringsIT extends WebApiIT { + + private static final Pattern QUOTED = Pattern.compile("'([^']*)'"); + + @Autowired + @Qualifier("requestMappingHandlerMapping") + private RequestMappingHandlerMapping handlerMapping; + + @Test + public void everyPreAuthorizePermissionIsSeeded() { + Set seeded = new TreeSet<>( + jdbcTemplate.queryForList("SELECT value FROM " + getOhdsiSchema() + ".sec_permission", String.class)); + + Set unknown = new TreeSet<>(); + for (HandlerMethod hm : handlerMapping.getHandlerMethods().values()) { + if (!hm.getBeanType().getName().startsWith("org.ohdsi.webapi")) { + continue; + } + for (String expr : preAuthorizeExpressions(hm)) { + for (String permission : permissionLiterals(expr)) { + if (!seeded.contains(permission)) { + unknown.add(permission + " (in: " + expr + ")"); + } + } + } + } + assertTrue( + "@PreAuthorize references permissions not present in sec_permission " + + "(typo or missing migration?):\n " + String.join("\n ", unknown), + unknown.isEmpty()); + } + + private static Set preAuthorizeExpressions(HandlerMethod hm) { + Set exprs = new LinkedHashSet<>(); + PreAuthorize method = AnnotatedElementUtils.findMergedAnnotation(hm.getMethod(), PreAuthorize.class); + if (method != null) { + exprs.add(method.value()); + } + PreAuthorize type = AnnotatedElementUtils.findMergedAnnotation(hm.getBeanType(), PreAuthorize.class); + if (type != null) { + exprs.add(type.value()); + } + return exprs; + } + + private static List permissionLiterals(String expression) { + List permissions = new java.util.ArrayList<>(); + Matcher m = QUOTED.matcher(expression); + while (m.find()) { + permissions.add(m.group(1)); + } + return permissions; + } +}