diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 1c0c0b427..9753a3f19 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,AuthorizationAccessIT,AuthorizationPermissionStringsIT verify + # Check that the docker image builds correctly # Push to ghcr.io for commits on master or webapi-3.0. docker: 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/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; + } +} 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..ba73b872d --- /dev/null +++ b/src/test/java/org/ohdsi/webapi/test/EndpointAuthCoverageIT.java @@ -0,0 +1,130 @@ +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.core.MethodParameter; +import org.springframework.http.HttpMethod; +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; +import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping; + +/** + * 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/refresh", "/user/logout", "/user/oauth/callback"); + + @Autowired + @Qualifier("requestMappingHandlerMapping") + private RequestMappingHandlerMapping handlerMapping; + + @Test + 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; + } + if (isGuarded(hm)) { + continue; + } + for (String pattern : patternsOf(e.getKey())) { + if (!isAllowListed(pattern)) { + unguarded.add(pickMethod(e.getKey()) + " " + pattern + + " (" + hm.getBeanType().getSimpleName() + "#" + hm.getMethod().getName() + ")"); + } + } + } + assertTrue( + "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) { + 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 HttpMethod pickMethod(RequestMappingInfo info) { + Set methods = info.getMethodsCondition().getMethods(); + if (methods.isEmpty()) { + return HttpMethod.GET; + } + return HttpMethod.valueOf(methods.iterator().next().name()); + } + + 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; + } +}