Fix per-key attribution on JWT path; scrub stale comments; add tests
This commit is contained in:
+6
-11
@@ -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<PortalApiKeysResponse> 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<CreatedApiKeyDto> 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<Void> revoke(@PathVariable("id") Long id) {
|
||||
apiKeyManagementService.revokeKey(id);
|
||||
return ResponseEntity.noContent().build();
|
||||
|
||||
+13
-4
@@ -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<ApiKeyAuthentication> 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(
|
||||
|
||||
+7
-2
@@ -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
|
||||
|
||||
+3
-3
@@ -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<? extends GrantedAuthority> getAuthorities(User user) {
|
||||
|
||||
+95
@@ -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();
|
||||
}
|
||||
}
|
||||
+88
@@ -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);
|
||||
}
|
||||
}
|
||||
+14
@@ -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)");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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"
|
||||
|
||||
Reference in New Issue
Block a user