Add authorization guardrail tests and run them in CI

.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:

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<IRAnalysisShortDTO> getIRAnalysisList() {
     return getTransactionTemplate().execute(transactionStatus -> {
       Iterable<IncidenceRateAnalysis> analysisList = this.irAnalysisRepository.findAll();
@@ -338,6 +339,7 @@ public List<IRAnalysisShortDTO> 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<IRAnalysisDTO> listByTags(TagNameListRequestDTO requestDTO) {
     if (requestDTO == null || requestDTO.getNames() == null || requestDTO.getNames().isEmpty()) {
       return Collections.emptyList();

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.
+ *
+ * <p>The "reader" user is granted the generic {@code read} permission, which
+ * wildcard-implies every {@code read:<domain>}; 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<String> 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<String> 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<String> r = getAsReader("/role");
+        assertEquals(
+            "Reader (granted only 'read') must be denied admin role management",
+            HttpStatus.FORBIDDEN,
+            HttpStatus.valueOf(r.getStatusCode().value()));
+    }
+}

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.
+ *
+ * <p>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<String> seeded = new TreeSet<>(
+            jdbcTemplate.queryForList("SELECT value FROM " + getOhdsiSchema() + ".sec_permission", String.class));
+
+        Set<String> 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<String> preAuthorizeExpressions(HandlerMethod hm) {
+        Set<String> 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<String> permissionLiterals(String expression) {
+        List<String> permissions = new java.util.ArrayList<>();
+        Matcher m = QUOTED.matcher(expression);
+        while (m.find()) {
+            permissions.add(m.group(1));
+        }
+        return permissions;
+    }
+}

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.
+ *
+ * <p>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<String> 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<String> unguarded = new ArrayList<>();
+        for (Map.Entry<RequestMappingInfo, HandlerMethod> 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<String> 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<String> 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<RequestMethod> 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;
+    }
+}