From 6e4e3e138cf8c9dba7d0522a5ac10e0174ddb72d Mon Sep 17 00:00:00 2001 From: Anthony Stirling <77850077+Frooodle@users.noreply.github.com> Date: Sat, 11 Jul 2026 23:53:33 +0100 Subject: [PATCH] Fix per-key attribution on JWT path; scrub stale comments; add tests --- .../api/PortalApiKeysController.java | 17 ++-- .../filter/JwtAuthenticationFilter.java | 17 +++- .../filter/UserAuthenticationFilter.java | 9 +- .../security/service/UserService.java | 6 +- .../UserBasedRateLimitingFilterTest.java | 95 +++++++++++++++++++ .../service/ApiKeyUsageRecorderTest.java | 88 +++++++++++++++++ .../service/PortalDocumentsServiceTest.java | 14 +++ .../public/locales/en-US/translation.toml | 2 +- 8 files changed, 227 insertions(+), 21 deletions(-) create mode 100644 app/proprietary/src/test/java/stirling/software/proprietary/security/filter/UserBasedRateLimitingFilterTest.java create mode 100644 app/proprietary/src/test/java/stirling/software/proprietary/security/service/ApiKeyUsageRecorderTest.java diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/controller/api/PortalApiKeysController.java b/app/proprietary/src/main/java/stirling/software/proprietary/controller/api/PortalApiKeysController.java index bc623d708f..af0f4d055a 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/controller/api/PortalApiKeysController.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/controller/api/PortalApiKeysController.java @@ -19,9 +19,9 @@ import stirling.software.proprietary.model.api.apikey.PortalApiKeysResponse; import stirling.software.proprietary.security.service.ApiKeyManagementService; /** - * Real backing for the portal Infrastructure → API Keys tab: list/create/revoke named keys, scoped - * personal or to the caller's team. Replaces the former portal-only mock endpoint. Not gated behind - * an Enterprise license - API keys are a core auth feature available on every self-hosted instance. + * Real backing for the portal Infrastructure → API Keys tab: list/create/revoke named, personal API + * keys. Replaces the former portal-only mock endpoint. Not gated behind an Enterprise license - API + * keys are a core auth feature available on every self-hosted instance. */ @ProprietaryUiDataApi @RequiredArgsConstructor @@ -31,10 +31,7 @@ public class PortalApiKeysController { // tier accepted for endpoint symmetry with the other infra tabs; ignored here. @GetMapping("/infrastructure/api-keys") - @Operation( - summary = "List API keys", - description = - "Keys the caller may see: their personal keys plus any team keys in scope.") + @Operation(summary = "List API keys", description = "The caller's personal API keys.") public ResponseEntity list( @RequestParam(value = "tier", required = false) String tier) { return ResponseEntity.ok(apiKeyManagementService.listVisibleKeys()); @@ -43,15 +40,13 @@ public class PortalApiKeysController { @PostMapping("/infrastructure/api-keys") @Operation( summary = "Create an API key", - description = - "Mints a personal or team-scoped key and returns its one-time secret. Team keys" - + " require team-leader rights.") + description = "Mints a personal key and returns its one-time secret.") public ResponseEntity create(@RequestBody CreateApiKeyRequest request) { return ResponseEntity.ok(apiKeyManagementService.createKey(request)); } @DeleteMapping("/infrastructure/api-keys/{id}") - @Operation(summary = "Revoke an API key", description = "Disables a key the caller manages.") + @Operation(summary = "Revoke an API key", description = "Disables a key the caller owns.") public ResponseEntity revoke(@PathVariable("id") Long id) { apiKeyManagementService.revokeKey(id); return ResponseEntity.noContent().build(); diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/filter/JwtAuthenticationFilter.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/filter/JwtAuthenticationFilter.java index 3ceb850fda..ed8001df5a 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/filter/JwtAuthenticationFilter.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/filter/JwtAuthenticationFilter.java @@ -11,6 +11,7 @@ import java.sql.SQLException; import java.util.Map; import java.util.Optional; +import org.slf4j.MDC; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.Authentication; import org.springframework.security.core.AuthenticationException; @@ -55,6 +56,9 @@ public class JwtAuthenticationFilter extends OncePerRequestFilter { protected void doFilterInternal( HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { + // Start clean so a pooled thread can't inherit a prior request's key label. This filter + // runs before UserAuthenticationFilter, so in JWT mode it owns the API-key label lifecycle. + MDC.remove(ApiKeyAuthenticationService.AUDIT_LABEL_MDC_KEY); if (!jwtService.isJwtEnabled()) { filterChain.doFilter(request, response); return; @@ -133,10 +137,10 @@ public class JwtAuthenticationFilter extends OncePerRequestFilter { if (apiKey != null && !apiKey.isBlank()) { try { - // Resolve through the shared service so a team/processing key gets its capped - // authorities and access flag - resolving via the owner here would hand a - // shared - // key the owner's full (admin) rights and bypass the processing-scope boundary. + // Resolve through the shared service so the multi-key table (then the legacy + // per-user key) is consulted and per-key usage is recorded; the key runs as its + // owner. It also yields a per-key label for the processor's document + // attribution. Optional resolved = apiKeyAuthenticationService.authenticate(apiKey); @@ -152,6 +156,11 @@ public class JwtAuthenticationFilter extends OncePerRequestFilter { new ApiKeyAuthenticationToken( resolved.get().user(), apiKey, resolved.get().authorities()); SecurityContextHolder.getContext().setAuthentication(authentication); + if (resolved.get().auditLabel() != null) { + MDC.put( + ApiKeyAuthenticationService.AUDIT_LABEL_MDC_KEY, + resolved.get().auditLabel()); + } return true; } catch (AuthenticationException e) { handleAuthenticationFailure( diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/filter/UserAuthenticationFilter.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/filter/UserAuthenticationFilter.java index fe1940dc23..8fe351ccde 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/filter/UserAuthenticationFilter.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/filter/UserAuthenticationFilter.java @@ -71,8 +71,13 @@ public class UserAuthenticationFilter extends OncePerRequestFilter { HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { - // Start each request clean so a pooled thread can't inherit a prior request's key label. - MDC.remove(API_KEY_LABEL_MDC); + // Start each request clean so a pooled thread can't inherit a prior request's key label - + // but keep a label an upstream filter (JwtAuthenticationFilter) already set for a request + // it API-key-authenticated, otherwise per-key attribution is lost on the JWT path. + if (!(SecurityContextHolder.getContext().getAuthentication() + instanceof ApiKeyAuthenticationToken)) { + MDC.remove(API_KEY_LABEL_MDC); + } if (!loginEnabledValue) { // If login is not enabled, just pass all requests without authentication diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/service/UserService.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/service/UserService.java index ad4a5bcf8e..0cb4653ef1 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/service/UserService.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/service/UserService.java @@ -148,8 +148,8 @@ public class UserService implements UserServiceInterface { } public Authentication getAuthentication(String apiKey) { - // Resolve through the shared service so a processing/team key authenticates with its - // capped authorities (never the owner's full admin rights), matching the auth filters. + // Resolve through the shared service (multi-key table, then the legacy per-user column). + // The key runs as its owner with the owner's authorities. var resolved = apiKeyAuthenticationService .authenticate(apiKey) @@ -157,7 +157,7 @@ public class UserService implements UserServiceInterface { return new UsernamePasswordAuthenticationToken( resolved.user(), // principal null, // credentials (we don't expose the password or API key here) - resolved.authorities()); // capped authorities honouring the key's access level + resolved.authorities()); // the owner's authorities } private Collection getAuthorities(User user) { diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/security/filter/UserBasedRateLimitingFilterTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/security/filter/UserBasedRateLimitingFilterTest.java new file mode 100644 index 0000000000..69bc910701 --- /dev/null +++ b/app/proprietary/src/test/java/stirling/software/proprietary/security/filter/UserBasedRateLimitingFilterTest.java @@ -0,0 +1,95 @@ +package stirling.software.proprietary.security.filter; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.List; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.mock.web.MockFilterChain; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.security.core.context.SecurityContextHolder; + +import stirling.software.common.model.enumeration.Role; +import stirling.software.proprietary.security.model.ApiKeyAuthenticationToken; +import stirling.software.proprietary.security.model.User; + +@ExtendWith(MockitoExtension.class) +@DisplayName("UserBasedRateLimitingFilter") +class UserBasedRateLimitingFilterTest { + + @AfterEach + void clear() { + SecurityContextHolder.clearContext(); + } + + private void authenticateAs(String username) { + User u = new User(); + u.setUsername(username); + u.setEnabled(true); + SecurityContextHolder.getContext() + .setAuthentication( + new ApiKeyAuthenticationToken( + u, + "irrelevant", + List.of(new SimpleGrantedAuthority(Role.USER.getRoleId())))); + } + + private long remainingAfterApiPost(UserBasedRateLimitingFilter filter, String apiKey) + throws Exception { + MockHttpServletRequest req = new MockHttpServletRequest("POST", "/api/v1/general/x"); + req.addHeader("X-API-KEY", apiKey); + MockHttpServletResponse res = new MockHttpServletResponse(); + filter.doFilter(req, res, new MockFilterChain()); + return Long.parseLong(res.getHeader("X-Rate-Limit-Remaining")); + } + + @Test + @DisplayName("all of a user's keys share ONE bucket - minting keys can't multiply the quota") + void keysShareOnePerUserBucket() throws Exception { + UserBasedRateLimitingFilter filter = new UserBasedRateLimitingFilter(true); + authenticateAs("alice"); + + long afterKeyA = remainingAfterApiPost(filter, "key-A"); + long afterKeyB = remainingAfterApiPost(filter, "key-B"); // different key, same user + + // The second (different) key drew from the SAME per-user bucket, so remaining fell by one. + // If it were keyed per-API-key, both would report the same remaining. + assertThat(afterKeyB).isEqualTo(afterKeyA - 1); + } + + @Test + @DisplayName("non-POST requests are not rate limited") + void nonPostPassesThrough() throws Exception { + UserBasedRateLimitingFilter filter = new UserBasedRateLimitingFilter(true); + authenticateAs("alice"); + MockHttpServletRequest req = new MockHttpServletRequest("GET", "/api/v1/general/x"); + req.addHeader("X-API-KEY", "key-A"); + MockHttpServletResponse res = new MockHttpServletResponse(); + MockFilterChain chain = new MockFilterChain(); + + filter.doFilter(req, res, chain); + + assertThat(res.getHeader("X-Rate-Limit-Remaining")).isNull(); + assertThat(chain.getRequest()).isNotNull(); // passed down the chain + } + + @Test + @DisplayName("rate limiting disabled: passes through untouched") + void disabledPassesThrough() throws Exception { + UserBasedRateLimitingFilter filter = new UserBasedRateLimitingFilter(false); + authenticateAs("alice"); + MockHttpServletRequest req = new MockHttpServletRequest("POST", "/api/v1/general/x"); + req.addHeader("X-API-KEY", "key-A"); + MockHttpServletResponse res = new MockHttpServletResponse(); + + filter.doFilter(req, res, new MockFilterChain()); + + assertThat(res.getHeader("X-Rate-Limit-Remaining")).isNull(); + } +} diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/security/service/ApiKeyUsageRecorderTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/security/service/ApiKeyUsageRecorderTest.java new file mode 100644 index 0000000000..6a03414585 --- /dev/null +++ b/app/proprietary/src/test/java/stirling/software/proprietary/security/service/ApiKeyUsageRecorderTest.java @@ -0,0 +1,88 @@ +package stirling.software.proprietary.security.service; + +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +/** + * Unit tests for the increment/insert/increment race protocol. {@code @Async} has no proxy in a + * plain Mockito test, so {@code record()} runs inline and is directly testable. + */ +@ExtendWith(MockitoExtension.class) +@DisplayName("ApiKeyUsageRecorder") +class ApiKeyUsageRecorderTest { + + private static final long KEY = 7L; + + @Mock private ApiKeyUsageWriter writer; + @InjectMocks private ApiKeyUsageRecorder recorder; + + @Test + @DisplayName("a null key id is a no-op") + void nullIdIsNoOp() { + recorder.record(null); + verifyNoInteractions(writer); + } + + @Test + @DisplayName("row already exists: one increment, never inserts") + void rowExistsFastPath() { + when(writer.increment(eq(KEY), anyLong())).thenReturn(1); + + recorder.record(KEY); + + verify(writer, times(1)).increment(eq(KEY), anyLong()); + verify(writer, never()).tryInsertFirstUse(anyLong(), anyLong()); + verify(writer).stampLastUsed(KEY); + } + + @Test + @DisplayName("first writer of the day: increment misses, insert wins, no second increment") + void firstWriterInserts() { + when(writer.increment(eq(KEY), anyLong())).thenReturn(0); + when(writer.tryInsertFirstUse(eq(KEY), anyLong())).thenReturn(true); + + recorder.record(KEY); + + verify(writer, times(1)).increment(eq(KEY), anyLong()); + verify(writer).tryInsertFirstUse(eq(KEY), anyLong()); + verify(writer).stampLastUsed(KEY); + } + + @Test + @DisplayName( + "lost the insert race: falls back to a second increment so the count is not dropped") + void lostInsertRaceReincrements() { + when(writer.increment(eq(KEY), anyLong())).thenReturn(0); + when(writer.tryInsertFirstUse(eq(KEY), anyLong())).thenReturn(false); + + recorder.record(KEY); + + verify(writer, times(2)).increment(eq(KEY), anyLong()); + verify(writer).stampLastUsed(KEY); + } + + @Test + @DisplayName("insert throws (rollback-only commit): still re-increments, count not dropped") + void insertThrowsStillReincrements() { + when(writer.increment(eq(KEY), anyLong())).thenReturn(0); + when(writer.tryInsertFirstUse(eq(KEY), anyLong())) + .thenThrow(new RuntimeException("UnexpectedRollbackException")); + + recorder.record(KEY); + + verify(writer, times(2)).increment(eq(KEY), anyLong()); + verify(writer).stampLastUsed(KEY); + } +} diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/service/PortalDocumentsServiceTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/service/PortalDocumentsServiceTest.java index 1afc0af4a4..f02d919ba3 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/service/PortalDocumentsServiceTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/service/PortalDocumentsServiceTest.java @@ -70,4 +70,18 @@ class PortalDocumentsServiceTest { assertThat(doc.getProduct()).isEqualTo("API"); assertThat(doc.getSource()).isEqualTo("API integration"); } + + @Test + void apiDocumentIsAttributedToItsNamedKey() { + PortalReviewDocumentDto doc = + onlyDoc( + "{\"path\":\"/api/v1/misc/compress-pdf\",\"__origin\":\"API\"," + + "\"__apiKeyLabel\":\"Production ingest (sk_demo0000)\"," + + "\"files\":[{\"name\":\"a.pdf\",\"type\":\"application/pdf\"}]," + + "\"statusCode\":200}"); + + // The specific key label surfaces as the source; product stays "API". + assertThat(doc.getProduct()).isEqualTo("API"); + assertThat(doc.getSource()).isEqualTo("API key · Production ingest (sk_demo0000)"); + } } diff --git a/frontend/editor/public/locales/en-US/translation.toml b/frontend/editor/public/locales/en-US/translation.toml index 4fcab851c2..bbd1ab66b1 100644 --- a/frontend/editor/public/locales/en-US/translation.toml +++ b/frontend/editor/public/locales/en-US/translation.toml @@ -6895,7 +6895,7 @@ title = "Infrastructure" [portal.infrastructure.apiKeys] createKey = "Create key" heading = "API keys" -subheading = "Personal and team-scoped credentials, each with its own usage tracking." +subheading = "Personal credentials, each with its own usage tracking." [portal.infrastructure.apiKeys.card] created = "Created"