diff --git a/CLAUDE.md b/CLAUDE.md index b00cc4c..9f9a31a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -31,14 +31,14 @@ JDK 21 is required (Gradle's toolchain fetches one if absent). Node ≥ 20 + npm Dependency arrows always point **inward**. Adapters depend on core; core depends on no adapter, no framework, no servlet/HTTP API, no JDBC/DynamoDB. 1. **`pk-auth-core`** — framework- and persistence-neutral. Knows WebAuthn (WebAuthn4J), the wire contract, and declares the SPIs. `PasskeyAuthenticationService` is the ceremony entry point. The exported packages are `api`, `ceremony`, `config`, `credential`, `error`, `json`, `lifecycle`, `metrics`, and `spi` (enforced via `module-info.java`); everything else is module-internal. -2. **SPIs (ports)** — narrow interfaces the host implements: `UserLookup`, `CredentialRepository`, `ChallengeStore` (required); `BackupCodeRepository`, `OtpRepository`, `EmailSender`, `SmsSender`, `RefreshTokenRepository`, `AccessTokenStore`, `TokenTtlPolicy`, `UserDeletionListener`, `AttestationTrustPolicy`, `OriginValidator`, `ClockProvider` (optional / feature-gated). See `DESIGN.md` §6 for the required-vs-optional table. +2. **SPIs (ports)** — narrow interfaces the host implements: `UserLookup`, `CredentialRepository`, `ChallengeStore` (required); `BackupCodeRepository`, `OtpRepository`, `EmailSender`, `SmsSender`, `RefreshTokenRepository`, `AccessTokenStore`, `TokenTtlPolicy`, `RevocationCheck`, `UserDeletionListener`, `AttestationTrustPolicy`, `OriginValidator`, `ClockProvider`, `ConsumedJtiStore`, `CeremonyRateLimiter`, `MessageFormatter` (optional / feature-gated). See `DESIGN.md` §6 for the required-vs-optional table. 3. **Adapters** — `pk-auth-spring-boot-starter` (Spring Boot 4 / Security 7 autoconfigure), `pk-auth-dropwizard` (Dropwizard 5 `ConfiguredBundle` + Dagger 2), `pk-auth-micronaut` (Micronaut 4 `@Factory` + `@Filter`, deliberately **not** Micronaut Security). Each mounts the same `/auth/**` JSON contract and pattern-matches the core's sealed result sums into HTTP status codes. -Feature modules (`pk-auth-backup-codes`, `pk-auth-magic-link`, `pk-auth-otp`, `pk-auth-refresh-tokens`) and persistence modules (`pk-auth-persistence-jdbi`, `pk-auth-persistence-dynamodb`, in-memory `pk-auth-testkit`) implement core-declared SPIs and are wired in by the host. +Feature modules (`pk-auth-backup-codes`, `pk-auth-magic-link`, `pk-auth-otp`, `pk-auth-refresh-tokens`, `pk-auth-admin-api`) and persistence modules (`pk-auth-persistence-jdbi`, `pk-auth-persistence-dynamodb`, in-memory `pk-auth-testkit`) implement core-declared SPIs and are wired in by the host. (`pk-auth-admin-api` hosts `AdminService` / `AdminResult` and the account/credential/backup-code/email/phone admin operations mounted by all three adapters at `/auth/admin/**`.) ### Things that bite if you don't know them -- **Sealed result sums, not exceptions.** Ceremony and admin operations return sealed interfaces — `AdminResult` (`Success | NotFound | Forbidden | ValidationFailed | Conflict | RateLimited`), `RegistrationResult`, `AssertionResult`, `RotateResult`, `JwtVerificationResult`. Adapters map these to HTTP; never throw across that boundary. When you add a variant, every adapter's `*ResultMapper` must handle it. +- **Sealed result sums, not exceptions.** Ceremony and admin operations return sealed interfaces — `AdminResult` (`Success | NotFound | Forbidden | ValidationFailed | Conflict | RateLimited`, declared in `pk-auth-admin-api`), `RegistrationResult`, `AssertionResult` (core), `RotateResult` (`pk-auth-refresh-tokens`), `JwtVerificationResult` (`pk-auth-jwt`). Adapters map these to HTTP; never throw across that boundary. When you add a variant, every adapter's `*ResultMapper` must handle it. - **Wire bytes are base64url, no padding** (RFC 4648 §5). Jackson 3 adapters get this from `PkAuthObjectMappers.pkAuthModule()`; the Dropwizard adapter is still on Jackson 2 and uses the `PkAuthJacksonBridge`. - **`finish` endpoints are not idempotent** — challenges are single-use via `ChallengeStore.takeOnce`. There is **no shared transaction across SPIs**: `takeOnce` is consumed before `CredentialRepository.save`; a failed save forces a ceremony restart. This is intentional — see [`docs/transactional-semantics.md`](./docs/transactional-semantics.md). - **All three adapters mount identical `/auth/**` paths** (`/auth/passkeys/**`, `/auth/refresh`, `/auth/admin/**`) — Dropwizard's Jersey resources use the same `@Path("/auth/passkeys")` etc. as Spring/Micronaut, so the TS SDK targets one path scheme everywhere (no per-client path override). diff --git a/examples/dropwizard-demo/build.gradle.kts b/examples/dropwizard-demo/build.gradle.kts index 1d68d1c..25b05c3 100644 --- a/examples/dropwizard-demo/build.gradle.kts +++ b/examples/dropwizard-demo/build.gradle.kts @@ -47,8 +47,8 @@ dependencies { implementation(libs.slf4j.api) runtimeOnly(libs.logback.classic) - compileOnly("com.google.errorprone:error_prone_annotations:2.50.0") - testCompileOnly("com.google.errorprone:error_prone_annotations:2.50.0") + compileOnly(libs.build.errorprone.annotations) + testCompileOnly(libs.build.errorprone.annotations) testImplementation(libs.dropwizard.testing) } diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 1bc8ad9..2be7644 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -157,6 +157,7 @@ h2 = { module = "com.h2database:h2", version.ref = "h2" } build-spotless-plugin = { module = "com.diffplug.spotless:spotless-plugin-gradle", version.ref = "spotless" } build-errorprone-plugin = { module = "net.ltgt.gradle:gradle-errorprone-plugin", version.ref = "errorprone-plugin" } build-errorprone-core = { module = "com.google.errorprone:error_prone_core", version.ref = "errorprone-core" } +build-errorprone-annotations = { module = "com.google.errorprone:error_prone_annotations", version.ref = "errorprone-core" } # Test bundle ingredients junit-jupiter = { module = "org.junit.jupiter:junit-jupiter", version.ref = "junit-jupiter" } diff --git a/pk-auth-admin-api/src/main/java/com/codeheadsystems/pkauth/admin/DefaultAdminService.java b/pk-auth-admin-api/src/main/java/com/codeheadsystems/pkauth/admin/DefaultAdminService.java index 34ce89f..36e5394 100644 --- a/pk-auth-admin-api/src/main/java/com/codeheadsystems/pkauth/admin/DefaultAdminService.java +++ b/pk-auth-admin-api/src/main/java/com/codeheadsystems/pkauth/admin/DefaultAdminService.java @@ -206,7 +206,12 @@ public AdminResult finishEmailVerification(String token) { return new AdminResult.ValidationFailed<>("token must be non-blank"); } if (magicLinkService == null) return notConfigured("email verification"); - ConsumeResult result = magicLinkService.finishVerification(token); + // Demand the email-verify purpose: a login-flow magic-link token must not be replayable here to + // mark an address verified (cross-purpose token confusion). MagicLinkService checks the purpose + // before consuming the single-use JTI, so a rejected cross-purpose token stays usable for + // login. + ConsumeResult result = + magicLinkService.finishVerification(token, MagicLinkService.PURPOSE_EMAIL_VERIFY); if (result instanceof ConsumeResult.Success success) { // Host apps own the users table; we report success and let the adapter persist the // emailVerified flag via UserLookup's host-app-specific update path (out of scope here). @@ -216,6 +221,9 @@ public AdminResult finishEmailVerification(String token) { if (result instanceof ConsumeResult.AlreadyConsumed) { return new AdminResult.Conflict<>("token already consumed"); } + if (result instanceof ConsumeResult.WrongPurpose) { + return new AdminResult.ValidationFailed<>("token is not an email-verification token"); + } return new AdminResult.ValidationFailed<>("invalid token"); } @@ -249,6 +257,15 @@ public AdminResult finishPhoneVerification( } if (otpService == null) return notConfigured("phone verification"); OtpService.VerifyResult result = otpService.finishVerification(target, phoneE164, code); + // AttemptsExceeded is a brute-force lockout signal, not a "soft" verification outcome, so it + // must surface at the HTTP layer as 429 (RateLimited) rather than a 200 the failure of which is + // buried in the body — otherwise a client/proxy keying off status (the documented contract for + // these sums) can't see that guessing has been throttled. A new OTP must be requested. + // Ordinary mismatch/expiry stay as a typed 200 body on purpose: the privacy-neutral + // remainingAttempts is part of the verification UX, not an error. + if (result instanceof OtpService.VerifyResult.AttemptsExceeded) { + return new AdminResult.RateLimited<>(Duration.ofMinutes(15)); + } return new AdminResult.Success<>( switch (result) { case OtpService.VerifyResult.Success s -> new PhoneVerificationResult.Verified(); diff --git a/pk-auth-admin-api/src/test/java/com/codeheadsystems/pkauth/admin/DefaultAdminServiceTest.java b/pk-auth-admin-api/src/test/java/com/codeheadsystems/pkauth/admin/DefaultAdminServiceTest.java index 8dfb84b..3849080 100644 --- a/pk-auth-admin-api/src/test/java/com/codeheadsystems/pkauth/admin/DefaultAdminServiceTest.java +++ b/pk-auth-admin-api/src/test/java/com/codeheadsystems/pkauth/admin/DefaultAdminServiceTest.java @@ -330,6 +330,20 @@ void finishPhoneVerificationFlow() { s -> assertThat(s.value()).isInstanceOf(PhoneVerificationResult.Expired.class)); } + @Test + void finishPhoneVerificationExhaustedAttemptsMapsToRateLimited() { + // maxAttempts == 3: three wrong guesses are compared (soft 200 mismatch), and the fourth is + // refused. A brute-force lockout must surface as RateLimited (429), not a 200 whose failure is + // hidden in the body, so a client/proxy can see guessing was throttled. + admin.startPhoneVerification(alice, alice, "+15551234567"); + for (int i = 0; i < 3; i++) { + assertThat(admin.finishPhoneVerification(alice, alice, "+15551234567", "000000")) + .isInstanceOf(AdminResult.Success.class); + } + assertThat(admin.finishPhoneVerification(alice, alice, "+15551234567", "000000")) + .isInstanceOf(AdminResult.RateLimited.class); + } + // -- Authorizer override -- @Test diff --git a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/api/CeremonyWireMapper.java b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/api/CeremonyWireMapper.java index af18254..147c818 100644 --- a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/api/CeremonyWireMapper.java +++ b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/api/CeremonyWireMapper.java @@ -37,10 +37,51 @@ public final class CeremonyWireMapper { private CeremonyWireMapper() {} - /** Carries a wire-format response: HTTP status code + a JSON-serializable body. */ - public record CeremonyResponse(int status, Map body) { + /** + * {@code Retry-After} hint (seconds) emitted on every ceremony 429, mirroring the {@code + * Retry-After} the admin endpoints already send on their rate-limit refusals. The value is a + * conservative constant matching {@code InMemoryCeremonyRateLimiter.DEFAULT_WINDOW} (1 minute): + * the {@link com.codeheadsystems.pkauth.spi.CeremonyRateLimiter} SPI is boolean-only and exposes + * no per-call window, so an exact remaining-time cannot be computed here. A host that tightens or + * widens its limiter window may therefore serve a slightly stale hint, which {@code Retry-After} + * explicitly permits (it is advisory). Hosts wanting an exact value can override the header. + */ + private static final Map RATE_LIMIT_HEADERS = Map.of("Retry-After", "60"); + + /** + * Canonical 429 ceremony refusal: {@code {"outcome":"rate_limited"}} body + {@code Retry-After}. + */ + private static CeremonyResponse rateLimitedResponse() { + return new CeremonyResponse(429, Map.of("outcome", "rate_limited"), RATE_LIMIT_HEADERS); + } + + /** + * Carries a wire-format response: HTTP status code, a JSON-serializable body, and response + * headers. Adapters MUST copy {@link #headers()} onto the native HTTP response (e.g. {@code + * Retry-After} on a 429); a body-only adapter silently drops them. + * + *

The {@code headers} component was added in 2.1.0; the two-arg constructor preserves the + * prior {@code (status, body)} call sites with no headers. + * + * @param status the HTTP status code + * @param body the JSON-serializable response body + * @param headers response headers to copy onto the native HTTP response (since 2.1.0) + */ + public record CeremonyResponse( + int status, Map body, Map headers) { public CeremonyResponse { body = Map.copyOf(body); + headers = Map.copyOf(headers); + } + + /** + * Convenience constructor for a response with no extra headers. + * + * @param status the HTTP status code + * @param body the JSON-serializable response body + */ + public CeremonyResponse(int status, Map body) { + this(status, body, Map.of()); } } @@ -68,8 +109,7 @@ public static CeremonyResponse forRegistration(RegistrationResult result) { Base64Url.encode(dc.credentialId().value()))); case RegistrationResult.InvalidPayload ip -> new CeremonyResponse(400, errorBody("invalid_payload", "detail", ip.detail())); - case RegistrationResult.RateLimited rl -> - new CeremonyResponse(429, Map.of("outcome", "rate_limited")); + case RegistrationResult.RateLimited rl -> rateLimitedResponse(); }; } @@ -78,11 +118,12 @@ public static CeremonyResponse forRegistration(RegistrationResult result) { * controllers use this for the {@code RateLimited} variant of {@code StartRegistrationResult} / * {@code StartAuthenticationResult}. * - * @return canonical rate-limited response (HTTP 429, body {@code {"outcome": "rate_limited"}}) + * @return canonical rate-limited response (HTTP 429, body {@code {"outcome": "rate_limited"}}, + * plus a {@code Retry-After} header since 2.1.0) * @since 0.9.1 */ public static CeremonyResponse rateLimited() { - return new CeremonyResponse(429, Map.of("outcome", "rate_limited")); + return rateLimitedResponse(); } /** @@ -137,8 +178,7 @@ public static CeremonyResponse forAssertionError(AssertionResult result) { new CeremonyResponse(401, Map.of("outcome", "user_verification_required")); case AssertionResult.InvalidSignature is -> new CeremonyResponse(401, Map.of("outcome", "invalid_signature")); - case AssertionResult.RateLimited rl -> - new CeremonyResponse(429, Map.of("outcome", "rate_limited")); + case AssertionResult.RateLimited rl -> rateLimitedResponse(); }; } diff --git a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/lifecycle/package-info.java b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/lifecycle/package-info.java index 05d7005..780eba7 100644 --- a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/lifecycle/package-info.java +++ b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/lifecycle/package-info.java @@ -9,4 +9,5 @@ * * @since 1.1.0 */ +@org.jspecify.annotations.NullMarked package com.codeheadsystems.pkauth.lifecycle; diff --git a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/ratelimit/InMemoryWindowCounter.java b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/ratelimit/InMemoryWindowCounter.java index 6667502..af87d2c 100644 --- a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/ratelimit/InMemoryWindowCounter.java +++ b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/ratelimit/InMemoryWindowCounter.java @@ -10,7 +10,12 @@ import java.util.concurrent.atomic.AtomicInteger; /** - * Caffeine-backed sliding-window counter shared by the backup-codes and magic-link rate limiters. + * Caffeine-backed fixed-window counter shared by the backup-codes and magic-link rate limiters. + * + *

The window is fixed from a key's first increment, not sliding: {@code expireAfterWrite} starts + * the clock when the key's counter is created, and the in-place {@link AtomicInteger} increments + * that follow do not reset it, so the whole window expires at once {@code window} after the first + * increment (the next increment then starts a fresh window at {@code 1}). * *

Each rate-limiter SPI in this project (e.g. {@code BackupCodeRateLimiter}, {@code * MagicLinkRateLimiter}, {@code CeremonyRateLimiter}) defines its own key signature. The diff --git a/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/api/CeremonyWireMapperTest.java b/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/api/CeremonyWireMapperTest.java index a113c96..bdfa646 100644 --- a/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/api/CeremonyWireMapperTest.java +++ b/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/api/CeremonyWireMapperTest.java @@ -111,6 +111,7 @@ void registrationRateLimitedIs429() { CeremonyWireMapper.forRegistration(new RegistrationResult.RateLimited("register")); assertThat(r.status()).isEqualTo(429); assertThat(r.body()).containsEntry("outcome", "rate_limited"); + assertThat(r.headers()).containsEntry("Retry-After", "60"); } @Test @@ -185,6 +186,7 @@ void assertionRateLimitedIs429() { CeremonyWireMapper.forAssertionError(new AssertionResult.RateLimited("authenticate")); assertThat(r.status()).isEqualTo(429); assertThat(r.body()).containsEntry("outcome", "rate_limited"); + assertThat(r.headers()).containsEntry("Retry-After", "60"); } @Test @@ -192,6 +194,7 @@ void startCeremonyRateLimitedIs429() { CeremonyResponse r = CeremonyWireMapper.rateLimited(); assertThat(r.status()).isEqualTo(429); assertThat(r.body()).containsEntry("outcome", "rate_limited"); + assertThat(r.headers()).containsEntry("Retry-After", "60"); } @Test diff --git a/pk-auth-dropwizard/build.gradle.kts b/pk-auth-dropwizard/build.gradle.kts index f8e4644..3ac0d0f 100644 --- a/pk-auth-dropwizard/build.gradle.kts +++ b/pk-auth-dropwizard/build.gradle.kts @@ -50,8 +50,8 @@ dependencies { // Dropwizard's transitive Jersey/Jakarta dependencies surface annotations whose enclosing // packages reference com.google.errorprone.annotations.* — same pattern as the JDBI module. - compileOnly("com.google.errorprone:error_prone_annotations:2.50.0") - testCompileOnly("com.google.errorprone:error_prone_annotations:2.50.0") + compileOnly(libs.build.errorprone.annotations) + testCompileOnly(libs.build.errorprone.annotations) implementation(libs.slf4j.api) diff --git a/pk-auth-dropwizard/src/main/java/com/codeheadsystems/pkauth/dropwizard/dagger/AltFlowsModule.java b/pk-auth-dropwizard/src/main/java/com/codeheadsystems/pkauth/dropwizard/dagger/AltFlowsModule.java index f43c00f..0a5be29 100644 --- a/pk-auth-dropwizard/src/main/java/com/codeheadsystems/pkauth/dropwizard/dagger/AltFlowsModule.java +++ b/pk-auth-dropwizard/src/main/java/com/codeheadsystems/pkauth/dropwizard/dagger/AltFlowsModule.java @@ -43,11 +43,10 @@ * configuration shape. This module is used only by {@link PkAuthFullComponent}; the slim {@link * PkAuthComponent} (passkey-ceremony-only) does not include it. * - *

Fail-fast policy (matches the maintainer decision pinned in the TODO): no adapter-level - * defaults. The host must hand in the OTP pepper, the magic-link base URL, and an explicit {@link - * EmailSender} / {@link SmsSender} (or accept the {@link LoggingEmailSender} / {@link - * LoggingSmsSender} dev-only fallback by passing {@code null} senders, which only activates when - * the {@link AltFlowOptions#devMode()} flag is true). + *

Fail-fast policy: no adapter-level defaults. The host must hand in the OTP pepper, the + * magic-link base URL, and an explicit {@link EmailSender} / {@link SmsSender} (or accept the + * {@link LoggingEmailSender} / {@link LoggingSmsSender} dev-only fallback by passing {@code null} + * senders, which only activates when the {@link AltFlowOptions#devMode()} flag is true). * * @since 0.9.1 */ diff --git a/pk-auth-dropwizard/src/main/java/com/codeheadsystems/pkauth/dropwizard/resource/PkAuthCeremonyResource.java b/pk-auth-dropwizard/src/main/java/com/codeheadsystems/pkauth/dropwizard/resource/PkAuthCeremonyResource.java index 0ed2987..772cecb 100644 --- a/pk-auth-dropwizard/src/main/java/com/codeheadsystems/pkauth/dropwizard/resource/PkAuthCeremonyResource.java +++ b/pk-auth-dropwizard/src/main/java/com/codeheadsystems/pkauth/dropwizard/resource/PkAuthCeremonyResource.java @@ -76,7 +76,9 @@ public Response finishAuthentication( } private static Response toResponse(CeremonyResponse wire) { - return Response.status(wire.status()).entity(wire.body()).build(); + Response.ResponseBuilder builder = Response.status(wire.status()).entity(wire.body()); + wire.headers().forEach(builder::header); + return builder.build(); } private static String clientIp(HttpServletRequest httpRequest) { diff --git a/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/JwtClaims.java b/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/JwtClaims.java index 1d29a91..d8af840 100644 --- a/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/JwtClaims.java +++ b/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/JwtClaims.java @@ -7,6 +7,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; import org.jspecify.annotations.Nullable; /** @@ -29,10 +30,41 @@ public record JwtClaims( @Nullable Map additionalClaims, @Nullable String audience) { + /** + * Claim names the issuer sets itself and that {@link #additionalClaims} must never carry. Because + * the issuer merges {@code additionalClaims} into the same JWT body after writing these, allowing + * a caller to supply one would let it silently overwrite a security-critical value (e.g. a future + * {@code exp}, an impersonating {@code sub}, or a forged {@code aud}/{@code iss}). The RFC 7519 + * registered set plus the {@code pkauth.*} private claims mirror {@code + * PkAuthJwtValidator.removeKnownClaims}. + * + * @since 2.1.0 + */ + private static final Set RESERVED_CLAIM_NAMES = + Set.of( + "iss", + "sub", + "aud", + "iat", + "nbf", + "exp", + "jti", + "pkauth.method", + "pkauth.cred", + "pkauth.amr"); + public JwtClaims { Objects.requireNonNull(userHandle, "userHandle"); Objects.requireNonNull(method, "method"); Objects.requireNonNull(amr, "amr"); + if (additionalClaims != null) { + for (String key : additionalClaims.keySet()) { + if (RESERVED_CLAIM_NAMES.contains(key)) { + throw new IllegalArgumentException( + "additionalClaims must not contain the reserved claim '" + key + "'"); + } + } + } if (method == AuthMethod.PASSKEY && credentialId == null) { throw new IllegalArgumentException("credentialId is required when method == PASSKEY"); } diff --git a/pk-auth-magic-link/src/main/java/com/codeheadsystems/pkauth/magiclink/MagicLinkService.java b/pk-auth-magic-link/src/main/java/com/codeheadsystems/pkauth/magiclink/MagicLinkService.java index e41547f..3d7aae2 100644 --- a/pk-auth-magic-link/src/main/java/com/codeheadsystems/pkauth/magiclink/MagicLinkService.java +++ b/pk-auth-magic-link/src/main/java/com/codeheadsystems/pkauth/magiclink/MagicLinkService.java @@ -110,6 +110,16 @@ record Invalid(JwtVerificationResult reason) implements ConsumeResult {} /** Token already consumed earlier. */ record AlreadyConsumed() implements ConsumeResult {} + + /** + * Token is otherwise valid but was minted for a different {@code pkauth.purpose} than the + * endpoint requires (e.g. a {@code login} token presented to the email-verification flow). The + * single-use JTI is deliberately not consumed in this case, so the token stays usable + * at its intended endpoint. + * + * @since 2.1.0 + */ + record WrongPurpose(String expectedPurpose, String actualPurpose) implements ConsumeResult {} } /** Pluggable rate limiter — defaults to an in-process Caffeine counter. */ @@ -272,6 +282,24 @@ public SendResult startLogin(String username, String email) { * @since 0.9.1 */ public ConsumeResult finishVerification(String token) { + return finishVerification(token, null); + } + + /** + * Verifies and consumes a magic-link token, additionally enforcing that its {@code + * pkauth.purpose} claim equals {@code requiredPurpose}. This closes a cross-purpose token + * confusion: without it, a token minted for one flow (e.g. {@link #PURPOSE_LOGIN}) satisfies + * another flow's consume check (e.g. {@link #PURPOSE_EMAIL_VERIFY}), letting one ceremony's token + * stand in for another. The purpose is checked before the single-use JTI is consumed, so + * a cross-purpose attempt does not burn the token at its legitimate endpoint. + * + * @param token the magic-link JWT + * @param requiredPurpose the purpose the caller demands, or {@code null} to accept any purpose + * (the caller then inspects {@link ConsumeResult.Success#purpose()} itself) + * @return the consume outcome + * @since 2.1.0 + */ + public ConsumeResult finishVerification(String token, @Nullable String requiredPurpose) { Objects.requireNonNull(token, "token"); JwtVerificationResult verification = validator.validate(token); if (!(verification instanceof JwtVerificationResult.Success success)) { @@ -282,6 +310,14 @@ public ConsumeResult finishVerification(String token) { if (purpose == null) { return new ConsumeResult.Invalid(new JwtVerificationResult.MissingClaim(CLAIM_PURPOSE)); } + if (requiredPurpose != null && !requiredPurpose.equals(purpose)) { + LOG.warn( + "magiclink.verify wrong-purpose user={} expected={} actual={}", + claims.userHandle(), + requiredPurpose, + purpose); + return new ConsumeResult.WrongPurpose(requiredPurpose, purpose); + } String email = stringClaim(claims.additionalClaims(), CLAIM_EMAIL); // Single-use: the JTI is opaque; we don't have direct access to it through JwtClaims, so we @@ -383,6 +419,14 @@ public static Dependencies of( * SINGLE-INSTANCE ONLY. Multi-replica deployments MUST replace it with a shared (Redis/DB-backed) * implementation. * + *

{@code consumedJtiTtl} must be at least as long as the magic-link JWT's + * TTL. Single-use is enforced by retaining each consumed JTI for {@code consumedJtiTtl}; + * if that retention is shorter than the token's own validity, a still-unexpired token becomes + * redeemable again once its JTI entry is evicted, defeating single-use. The defaults are safe + * (30m retention vs the 15m default token TTL); keep this invariant if you tune either value. + * This is not enforced at construction because the token TTL is owned by the JWT issuer (and may + * vary per audience), not by this config. + * * @since 0.9.1 */ public record Config( diff --git a/pk-auth-magic-link/src/test/java/com/codeheadsystems/pkauth/magiclink/MagicLinkServiceTest.java b/pk-auth-magic-link/src/test/java/com/codeheadsystems/pkauth/magiclink/MagicLinkServiceTest.java index 12087d4..882fb4b 100644 --- a/pk-auth-magic-link/src/test/java/com/codeheadsystems/pkauth/magiclink/MagicLinkServiceTest.java +++ b/pk-auth-magic-link/src/test/java/com/codeheadsystems/pkauth/magiclink/MagicLinkServiceTest.java @@ -111,6 +111,32 @@ void loginEmailRoundTrip() { }); } + @Test + void finishVerificationRejectsCrossPurposeTokenWithoutConsumingIt() { + UserHandle user = users.register("alice", "Alice"); + + // A login-purpose token must not satisfy the email-verify consume check. + MagicLinkService.SendResult send = service.startLogin("alice", "alice@example.com"); + String token = ((MagicLinkService.SendResult.Sent) send).tokenJti(); + + MagicLinkService.ConsumeResult wrong = + service.finishVerification(token, MagicLinkService.PURPOSE_EMAIL_VERIFY); + assertThat(wrong) + .isInstanceOfSatisfying( + MagicLinkService.ConsumeResult.WrongPurpose.class, + w -> { + assertThat(w.expectedPurpose()).isEqualTo(MagicLinkService.PURPOSE_EMAIL_VERIFY); + assertThat(w.actualPurpose()).isEqualTo(MagicLinkService.PURPOSE_LOGIN); + }); + + // The rejected cross-purpose attempt did NOT burn the single-use JTI: the token still works at + // its intended endpoint. + assertThat(service.finishVerification(token, MagicLinkService.PURPOSE_LOGIN)) + .isInstanceOfSatisfying( + MagicLinkService.ConsumeResult.Success.class, + s -> assertThat(s.userHandle()).isEqualTo(user)); + } + @Test void loginUnknownUserReturnsSentToPreventEnumeration() { // Privacy invariant: startLogin must return Sent even when the username does not exist, diff --git a/pk-auth-micronaut/build.gradle.kts b/pk-auth-micronaut/build.gradle.kts index 4b8c8c0..1710f86 100644 --- a/pk-auth-micronaut/build.gradle.kts +++ b/pk-auth-micronaut/build.gradle.kts @@ -46,8 +46,8 @@ dependencies { // Micronaut's @Inject site references the JDBI errorprone @GuardedBy via the persistence // modules. Same trick as pk-auth-persistence-jdbi. - compileOnly("com.google.errorprone:error_prone_annotations:2.50.0") - testCompileOnly("com.google.errorprone:error_prone_annotations:2.50.0") + compileOnly(libs.build.errorprone.annotations) + testCompileOnly(libs.build.errorprone.annotations) annotationProcessor(libs.micronaut.inject.java) diff --git a/pk-auth-micronaut/src/main/java/com/codeheadsystems/pkauth/micronaut/PkAuthCeremonyController.java b/pk-auth-micronaut/src/main/java/com/codeheadsystems/pkauth/micronaut/PkAuthCeremonyController.java index 2669220..0cd7fc8 100644 --- a/pk-auth-micronaut/src/main/java/com/codeheadsystems/pkauth/micronaut/PkAuthCeremonyController.java +++ b/pk-auth-micronaut/src/main/java/com/codeheadsystems/pkauth/micronaut/PkAuthCeremonyController.java @@ -13,6 +13,7 @@ import io.micronaut.http.HttpResponse; import io.micronaut.http.HttpStatus; import io.micronaut.http.MediaType; +import io.micronaut.http.MutableHttpResponse; import io.micronaut.http.annotation.Body; import io.micronaut.http.annotation.Controller; import io.micronaut.http.annotation.Post; @@ -84,8 +85,11 @@ public HttpResponse> finishAuthentication( } private static HttpResponse> toResponse(CeremonyResponse wire) { - return HttpResponse.>status(HttpStatus.valueOf(wire.status())) - .body(wire.body()); + MutableHttpResponse> response = + HttpResponse.>status(HttpStatus.valueOf(wire.status())) + .body(wire.body()); + wire.headers().forEach(response::header); + return response; } private static @Nullable String clientIp(HttpRequest request) { diff --git a/pk-auth-otp/src/main/java/com/codeheadsystems/pkauth/otp/OtpService.java b/pk-auth-otp/src/main/java/com/codeheadsystems/pkauth/otp/OtpService.java index fa4da34..c162364 100644 --- a/pk-auth-otp/src/main/java/com/codeheadsystems/pkauth/otp/OtpService.java +++ b/pk-auth-otp/src/main/java/com/codeheadsystems/pkauth/otp/OtpService.java @@ -197,18 +197,21 @@ public VerifyResult finishVerification(UserHandle user, String phoneE164, String return new VerifyResult.Expired(); } - // Increment first to close the TOCTOU window: the returned count is authoritative. Use `>=` - // so the attempt that reaches the cap is itself rejected — paired with the JDBI repo now - // incrementing unconditionally, this closes the prior bypass where the guarded UPDATE was a - // no-op once attempts == max_attempts. An empty result means the row has vanished between - // findLatestActive and incrementAttempts (e.g. concurrent admin delete) — treat as no-op + // Increment first to close the TOCTOU window: the returned (1-based) count is authoritative. + // The repo increments unconditionally, which closes the prior bypass where the guarded UPDATE + // was a no-op once attempts == max_attempts. Reject with `>` (not `>=`) so the caller gets + // exactly maxAttempts code comparisons: the submission whose post-increment count equals the + // cap is still checked, and only the first submission *beyond* the cap is refused. Using `>=` + // wasted the final attempt (only maxAttempts-1 codes were ever compared), contradicting the + // documented "max N attempts" contract. An empty result means the row vanished between + // findLatestActive and incrementAttempts (e.g. concurrent admin delete) — treat as a no-op // mismatch via NoActiveOtp so we don't pretend a phantom verify succeeded. OptionalInt incremented = repository.incrementAttempts(user, active.otpId()); if (incremented.isEmpty()) { return new VerifyResult.NoActiveOtp(); } int newAttempts = incremented.getAsInt(); - if (newAttempts >= active.maxAttempts()) { + if (newAttempts > active.maxAttempts()) { return new VerifyResult.AttemptsExceeded(); } diff --git a/pk-auth-otp/src/test/java/com/codeheadsystems/pkauth/otp/OtpServiceTest.java b/pk-auth-otp/src/test/java/com/codeheadsystems/pkauth/otp/OtpServiceTest.java index 70dbddc..d5bf3fa 100644 --- a/pk-auth-otp/src/test/java/com/codeheadsystems/pkauth/otp/OtpServiceTest.java +++ b/pk-auth-otp/src/test/java/com/codeheadsystems/pkauth/otp/OtpServiceTest.java @@ -76,18 +76,40 @@ void verifyAcceptsMatchingCodeOnce() { @Test void verifyMismatchDecrementsRemainingAttempts() { + // maxAttempts == 3: the caller gets exactly three code comparisons (remaining 2, 1, 0); only + // the fourth submission is refused as AttemptsExceeded. Guards against the prior off-by-one + // (`>=`) where the third, in-budget attempt was wrongly rejected without comparing the code. service.startVerification(USER, PHONE); - OtpService.VerifyResult first = service.finishVerification(USER, PHONE, "000000"); - assertThat(first) + assertThat(service.finishVerification(USER, PHONE, "000000")) .isInstanceOfSatisfying( OtpService.VerifyResult.CodeMismatch.class, m -> assertThat(m.remainingAttempts()).isEqualTo(2)); - service.finishVerification(USER, PHONE, "000000"); - service.finishVerification(USER, PHONE, "000000"); + assertThat(service.finishVerification(USER, PHONE, "000000")) + .isInstanceOfSatisfying( + OtpService.VerifyResult.CodeMismatch.class, + m -> assertThat(m.remainingAttempts()).isEqualTo(1)); + assertThat(service.finishVerification(USER, PHONE, "000000")) + .isInstanceOfSatisfying( + OtpService.VerifyResult.CodeMismatch.class, + m -> assertThat(m.remainingAttempts()).isEqualTo(0)); assertThat(service.finishVerification(USER, PHONE, "000000")) .isInstanceOf(OtpService.VerifyResult.AttemptsExceeded.class); } + @Test + void verifyAcceptsCorrectCodeOnFinalAllowedAttempt() { + // The maxAttempts-th submission must still be compared: a correct code on the last allowed + // attempt succeeds. Two prior mismatches exhaust attempts 1 and 2, leaving the 3rd (final). + service.startVerification(USER, PHONE); + String code = sms.lastCode(); + assertThat(service.finishVerification(USER, PHONE, "000000")) + .isInstanceOf(OtpService.VerifyResult.CodeMismatch.class); + assertThat(service.finishVerification(USER, PHONE, "000000")) + .isInstanceOf(OtpService.VerifyResult.CodeMismatch.class); + assertThat(service.finishVerification(USER, PHONE, code)) + .isInstanceOf(OtpService.VerifyResult.Success.class); + } + @Test void sendRateLimits() { service.startVerification(USER, PHONE); diff --git a/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbOtpRepository.java b/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbOtpRepository.java index dd62270..2d37a08 100644 --- a/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbOtpRepository.java +++ b/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbOtpRepository.java @@ -65,7 +65,11 @@ public Optional findLatestActive(UserHandle userHandle, String phoneE .flatMap(page -> page.items().stream()) .filter(i -> phoneE164.equals(i.getPhoneE164())) .filter(i -> !i.isConsumed()) - .max(Comparator.comparing(OtpItem::getCreatedAt)) + // Compare parsed Instants, not the raw ISO strings: createdAt is stored via + // Instant.toString(), which is variable-precision (it drops the fractional-seconds + // field when zero), so "...:00Z" sorts after "...:00.000001Z" lexicographically and + // would pick the wrong "latest" for two OTPs minted in the same wall-clock second. + .max(Comparator.comparing(i -> DynamoDbSupport.parseInstant(i.getCreatedAt()))) .map(OtpItem::toRecord); }); } diff --git a/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbRefreshTokenRepository.java b/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbRefreshTokenRepository.java index 7cef66e..ab343d1 100644 --- a/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbRefreshTokenRepository.java +++ b/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbRefreshTokenRepository.java @@ -27,6 +27,7 @@ import software.amazon.awssdk.enhanced.dynamodb.model.TransactPutItemEnhancedRequest; import software.amazon.awssdk.enhanced.dynamodb.model.TransactUpdateItemEnhancedRequest; import software.amazon.awssdk.enhanced.dynamodb.model.TransactWriteItemsEnhancedRequest; +import software.amazon.awssdk.enhanced.dynamodb.model.UpdateItemEnhancedRequest; import software.amazon.awssdk.services.dynamodb.model.AttributeValue; import software.amazon.awssdk.services.dynamodb.model.CancellationReason; import software.amazon.awssdk.services.dynamodb.model.ConditionalCheckFailedException; @@ -226,8 +227,8 @@ public int revokeFamily(String familyId, Instant now, RevokeReason reason) { return DynamoDbSupport.wrap( "refresh_tokens.revokeFamily", () -> { - // Query the family-index for every member, then mutate the primary item of each (the - // primary is the authority on revoked_at). + // Query the family-index for every member, then mark the primary item of each revoked + // (the primary is the authority on revoked_at). int[] revoked = {0}; String nowIso = DynamoDbSupport.encodeInstant(now); table @@ -239,35 +240,7 @@ public int revokeFamily(String familyId, Instant now, RevokeReason reason) { .build())) .stream() .flatMap(p -> p.items().stream()) - .forEach( - indexItem -> { - RefreshTokenItem primary = - table.getItem( - Key.builder() - .partitionValue(PRIMARY_PK_PREFIX + indexItem.getRefreshId()) - .sortValue(PRIMARY_PK_PREFIX + indexItem.getRefreshId()) - .build()); - if (primary == null || primary.getRevokedAtIso() != null) { - return; - } - primary.setRevokedAtIso(nowIso); - primary.setRevokedReason(reason.name()); - // Conditional put: only set revoked_at if it's still null (idempotent under - // concurrent revokers). - try { - table.putItem( - PutItemEnhancedRequest.builder(RefreshTokenItem.class) - .item(primary) - .conditionExpression( - Expression.builder() - .expression("attribute_not_exists(revokedAtIso)") - .build()) - .build()); - revoked[0]++; - } catch (ConditionalCheckFailedException raceLost) { - // Another revoker won; revokedAt is now set. Nothing to do. - } - }); + .forEach(indexItem -> revoked[0] += markRevoked(indexItem, nowIso, reason)); return revoked[0]; }); } @@ -289,37 +262,47 @@ public int revokeAllForUser(UserHandle userHandle, Instant now, RevokeReason rea .build())) .stream() .flatMap(p -> p.items().stream()) - .forEach( - indexItem -> { - RefreshTokenItem primary = - table.getItem( - Key.builder() - .partitionValue(PRIMARY_PK_PREFIX + indexItem.getRefreshId()) - .sortValue(PRIMARY_PK_PREFIX + indexItem.getRefreshId()) - .build()); - if (primary == null || primary.getRevokedAtIso() != null) { - return; - } - primary.setRevokedAtIso(nowIso); - primary.setRevokedReason(reason.name()); - try { - table.putItem( - PutItemEnhancedRequest.builder(RefreshTokenItem.class) - .item(primary) - .conditionExpression( - Expression.builder() - .expression("attribute_not_exists(revokedAtIso)") - .build()) - .build()); - revoked[0]++; - } catch (ConditionalCheckFailedException raceLost) { - // Lost race; revoked by someone else. - } - }); + .forEach(indexItem -> revoked[0] += markRevoked(indexItem, nowIso, reason)); return revoked[0]; }); } + /** + * Marks one family/user-index member's primary item revoked via a conditional, scalar-only {@code + * UpdateItem} that writes ONLY {@code revokedAtIso}/{@code revokedReason} — never a + * read-modify-write of the whole item. A full-item {@code putItem} (the prior approach) would + * carry a stale snapshot and could silently revert a {@code usedAtIso} mark set concurrently by + * {@code rotateAtomically}, corrupting the rotation/audit trail. {@code attribute_exists(pk)} + * prevents resurrecting a primary that was already pruned (an index row can outlive it), and + * {@code attribute_not_exists(revokedAtIso)} keeps the operation idempotent under concurrent + * revokers. A failed condition (already revoked, or primary gone) means "nothing to do" and + * returns 0. + * + * @return 1 if this call set revoked_at, 0 if there was nothing to revoke + */ + private int markRevoked(RefreshTokenItem indexItem, String nowIso, RevokeReason reason) { + RefreshTokenItem mark = new RefreshTokenItem(); + mark.setPk(PRIMARY_PK_PREFIX + indexItem.getRefreshId()); + mark.setSk(PRIMARY_PK_PREFIX + indexItem.getRefreshId()); + mark.setRevokedAtIso(nowIso); + mark.setRevokedReason(reason.name()); + try { + table.updateItem( + UpdateItemEnhancedRequest.builder(RefreshTokenItem.class) + .item(mark) + .ignoreNullsMode(IgnoreNullsMode.SCALAR_ONLY) + .conditionExpression( + Expression.builder() + .expression("attribute_exists(pk) AND attribute_not_exists(revokedAtIso)") + .build()) + .build()); + return 1; + } catch (ConditionalCheckFailedException raceLost) { + // Already revoked by another revoker, or the primary was pruned — nothing to do. + return 0; + } + } + @Override public List findByUserHandle(UserHandle userHandle) { return DynamoDbSupport.wrap( diff --git a/pk-auth-persistence-jdbi/build.gradle.kts b/pk-auth-persistence-jdbi/build.gradle.kts index 295189b..24838c2 100644 --- a/pk-auth-persistence-jdbi/build.gradle.kts +++ b/pk-auth-persistence-jdbi/build.gradle.kts @@ -23,8 +23,8 @@ dependencies { // Without errorprone-annotations on the compile classpath, javac emits an annotation-not-found // warning that -Werror turns fatal. Pulled in compileOnly so we don't broadcast it as a // runtime dependency. - compileOnly("com.google.errorprone:error_prone_annotations:2.50.0") - testCompileOnly("com.google.errorprone:error_prone_annotations:2.50.0") + compileOnly(libs.build.errorprone.annotations) + testCompileOnly(libs.build.errorprone.annotations) implementation(libs.flyway.core) runtimeOnly(libs.flyway.postgres) runtimeOnly(libs.postgresql) diff --git a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiBackupCodeRepository.java b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiBackupCodeRepository.java index c9634b6..0c55a36 100644 --- a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiBackupCodeRepository.java +++ b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiBackupCodeRepository.java @@ -197,18 +197,19 @@ private void insertAuditEvent( String eventType, byte[] userHandle, String subjectId, String detail) { jdbi.useHandle( h -> { - Update update = + try (Update update = h.createUpdate( "INSERT INTO pkauth_audit_events" + " (event_type, user_handle, subject_id, detail)" + " VALUES (:eventType, :userHandle, :subjectId, :detail)") .bind("eventType", eventType) .bind("subjectId", subjectId) - .bind("detail", detail); - // user_handle is BYTEA and nullable (unknown attacker). Untyped-null default - // (Types.VARCHAR) is rejected against BYTEA — force Types.BINARY on the null branch. - bindNullable(update, "userHandle", userHandle, Types.BINARY); - update.execute(); + .bind("detail", detail)) { + // user_handle is BYTEA and nullable (unknown attacker). Untyped-null default + // (Types.VARCHAR) is rejected against BYTEA — force Types.BINARY on the null branch. + bindNullable(update, "userHandle", userHandle, Types.BINARY); + update.execute(); + } }); } diff --git a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiCredentialRepository.java b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiCredentialRepository.java index 2fc4151..478658e 100644 --- a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiCredentialRepository.java +++ b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiCredentialRepository.java @@ -57,7 +57,7 @@ public void save(CredentialRecord record) { () -> jdbi.withHandle( h -> { - Update update = + try (Update update = h.createUpdate( "INSERT INTO credentials (credential_id, user_handle," + " public_key_cose, sign_count, label, aaguid, transports," @@ -75,20 +75,22 @@ public void save(CredentialRecord record) { .bind("bs", record.backupState()) .bind( "createdAt", - OffsetDateTime.ofInstant(record.createdAt(), ZoneOffset.UTC)); - // aaguid is null for platform authenticators / attestation=none. The default - // untyped-null binding uses Types.VARCHAR, which Postgres rejects against a - // UUID column ("column ... is of type uuid but expression is of type - // character varying"). Force Types.OTHER for the null case. - bindNullable(update, "aaguid", record.aaguid(), Types.OTHER); - bindNullable( - update, - "lastUsedAt", - record.lastUsedAt() == null - ? null - : OffsetDateTime.ofInstant(record.lastUsedAt(), ZoneOffset.UTC), - Types.TIMESTAMP_WITH_TIMEZONE); - return update.execute(); + OffsetDateTime.ofInstant(record.createdAt(), ZoneOffset.UTC))) { + // aaguid is null for platform authenticators / attestation=none. The + // default + // untyped-null binding uses Types.VARCHAR, which Postgres rejects against a + // UUID column ("column ... is of type uuid but expression is of type + // character varying"). Force Types.OTHER for the null case. + bindNullable(update, "aaguid", record.aaguid(), Types.OTHER); + bindNullable( + update, + "lastUsedAt", + record.lastUsedAt() == null + ? null + : OffsetDateTime.ofInstant(record.lastUsedAt(), ZoneOffset.UTC), + Types.TIMESTAMP_WITH_TIMEZONE); + return update.execute(); + } })); if (inserted == 0) { throw new DuplicateCredentialException( diff --git a/pk-auth-spring-boot-starter/src/main/java/com/codeheadsystems/pkauth/spring/web/PkAuthCeremonyController.java b/pk-auth-spring-boot-starter/src/main/java/com/codeheadsystems/pkauth/spring/web/PkAuthCeremonyController.java index 2e13dae..f0b82d9 100644 --- a/pk-auth-spring-boot-starter/src/main/java/com/codeheadsystems/pkauth/spring/web/PkAuthCeremonyController.java +++ b/pk-auth-spring-boot-starter/src/main/java/com/codeheadsystems/pkauth/spring/web/PkAuthCeremonyController.java @@ -75,7 +75,9 @@ public ResponseEntity finishAuthentication( } private static ResponseEntity toResponseEntity(CeremonyResponse wire) { - return ResponseEntity.status(wire.status()).body(wire.body()); + ResponseEntity.BodyBuilder builder = ResponseEntity.status(wire.status()); + wire.headers().forEach(builder::header); + return builder.body(wire.body()); } private static @Nullable String clientIp(HttpServletRequest request) { diff --git a/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/InMemoryRefreshTokenRepository.java b/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/InMemoryRefreshTokenRepository.java index 4bc7284..22f40b5 100644 --- a/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/InMemoryRefreshTokenRepository.java +++ b/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/InMemoryRefreshTokenRepository.java @@ -15,8 +15,9 @@ /** * In-memory {@link RefreshTokenRepository}. Backed by a {@link ConcurrentHashMap}; the load-bearing - * {@link #rotateAtomically} primitive uses {@link java.util.concurrent.ConcurrentMap#compute} so - * the mark-then-insert pair is atomic against concurrent rotators. + * {@link #rotateAtomically} primitive speculatively inserts the successor, then serializes the + * parent's mark-used via {@link java.util.concurrent.ConcurrentMap#compute}, rolling the successor + * back if the parent was not fresh. Exactly one of N concurrent rotators of the same parent wins. * * @since 1.1.0 */ @@ -41,11 +42,17 @@ public Optional findByRefreshId(String refreshId) { @Override public boolean rotateAtomically( String parentRefreshId, Instant now, RefreshTokenRecord successor) { - // compute() on the parent key serializes concurrent rotators on the same parent. Inside the - // block we (a) check the freshness predicate on the parent, (b) if fresh, flip used_at and - // ALSO insert the successor under its own key — both atomic w.r.t. any other compute on - // either key (a ConcurrentHashMap-wide guarantee in practice for distinct keys; we additionally - // use putIfAbsent so we don't accept duplicate successor IDs). + // Insert the successor under its own key FIRST, then serialize the parent's mark-used via + // compute(). The successor write MUST happen outside compute(): ConcurrentHashMap.compute + // forbids updating any other mapping of the same map from within the remapping function (risk + // of lockup / undefined behavior), so the prior nested putIfAbsent was illegal. A successor id + // collision aborts without disturbing the existing entry; if the parent turns out non-fresh + // (already used/revoked/expired, or missing) we roll our speculative successor back out so a + // lost rotation leaves no orphan. compute() on the parent still serializes concurrent rotators, + // so exactly one flips used_at and returns true. + if (byRefreshId.putIfAbsent(successor.refreshId(), successor) != null) { + return false; // duplicate successor id → abort, never half-commit + } boolean[] rotated = {false}; byRefreshId.compute( parentRefreshId, @@ -58,13 +65,13 @@ public boolean rotateAtomically( || !existing.expiresAt().isAfter(now)) { return existing; // not fresh → no rotation } - // Insert successor first. If it collides, abort — never half-commit a rotation. - if (byRefreshId.putIfAbsent(successor.refreshId(), successor) != null) { - return existing; - } rotated[0] = true; return markUsed(existing, now); }); + if (!rotated[0]) { + // Roll back the speculative insert; remove(key, value) only deletes our own successor. + byRefreshId.remove(successor.refreshId(), successor); + } return rotated[0]; }