diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 601bc03..1760526 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -17,12 +17,6 @@ updates: patterns: - "*" ignore: - # Spotless 8.x's google-java-format integration intermittently fails to load Guava / GJF - # classes (NoClassDefFoundError on Newlines / ImmutableList$*) on Gradle 9 / JDK 21. Pin - # to 7.x until upstream fixes. The Gradle plugin marker artifact uses the plugin id, not - # the implementation artifact id, so this is the right Dependabot dependency-name. - - dependency-name: "com.diffplug.spotless:com.diffplug.spotless.gradle.plugin" - versions: [">=8"] # Micronaut 5.x requires JVM 25; the project targets Java 21 (brief §3). Stay on the 4.x # line until the JDK baseline moves. - dependency-name: "io.micronaut:*" diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 2be7644..0bb0330 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -10,9 +10,11 @@ java = "21" # Build / quality -# Spotless pinned to the 7.x line: Spotless 8's google-java-format integration intermittently -# fails class loading on Gradle 9 / JDK 21. Dependabot ignore in .github/dependabot.yml keeps -# this from getting bumped back. Revisit when Spotless 8 ships a fix. +# Spotless tracks the 8.x line (currently 8.7.0) and is kept current by Dependabot's +# dev-dependencies group. Historical note: Spotless 8's google-java-format integration could +# intermittently fail class loading on Gradle 9 / JDK 21 under a *cached* build, which is why the +# Gradle build cache stays disabled (see gradle.properties) — `./gradlew check` itself runs clean +# on 8.x. The earlier 7.x pin (and its dependabot ignore) has been retired. spotless = "8.7.0" errorprone-plugin = "5.1.0" errorprone-core = "2.50.0" diff --git a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationService.java b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationService.java index d6d976d..219e687 100644 --- a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationService.java +++ b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationService.java @@ -229,12 +229,20 @@ public StartRegistrationResult startRegistration( byte[] challenge = challengeGenerator.generate(); ChallengeId challengeId = ChallengeId.random(); + // Resolve the user-verification requirement (per-request override, else ceremony default) and + // persist it on the challenge record so finishRegistration can enforce it server-side. Without + // this, a per-request REQUIRED (e.g. step-up) is advisory only — finish would fall back to the + // global config and accept flagUV=false. + UserVerificationRequirement uv = + req.userVerification() == null ? ceremonyConfig.userVerification() : req.userVerification(); + challengeStore.put( challengeId, new ChallengeRecord( challenge, ChallengeRecord.Purpose.REGISTRATION, userHandle, + uv, clockProvider.now().plus(ceremonyConfig.challengeTtl())), ceremonyConfig.challengeTtl()); @@ -244,9 +252,6 @@ public StartRegistrationResult startRegistration( List excludeCredentials = credentialRepository.findByUserHandle(userHandle).stream().map(this::toDescriptor).toList(); - UserVerificationRequirement uv = - req.userVerification() == null ? ceremonyConfig.userVerification() : req.userVerification(); - AuthenticatorSelectionCriteria selection = new AuthenticatorSelectionCriteria(null, ceremonyConfig.residentKey(), null, uv); @@ -351,13 +356,31 @@ private RegistrationData verifyRegistrationWithW4j( new RegistrationParameters( serverProperty, WebAuthn4JConverters.pubKeyCredParams(ceremonyConfig.acceptedAlgorithms()), - WebAuthn4JConverters.userVerificationRequired(ceremonyConfig.userVerification()), + effectiveUserVerificationRequired(challengeRecord), /* userPresenceRequired */ true); // Attestation signature verification is intentionally non-strict in the default manager; // BadSignatureException is not thrown here (see finding #41 / #3 for strict-mode opt-in). return webAuthnManager.verify(w4jRequest, w4jParams); } + /** + * Effective user-verification requirement for a finish step: {@code true} (UV required) if EITHER + * the global {@link CeremonyConfig#userVerification()} OR the per-request requirement resolved at + * start (persisted on {@link ChallengeRecord#userVerification()}) is {@code REQUIRED} — i.e. the + * stricter of the two. This enforces a per-request step-up {@code REQUIRED} server-side even when + * the global default is relaxed to {@code PREFERRED}/{@code DISCOURAGED}, and never weakens the + * global config. A {@code null} recorded requirement (legacy record) contributes nothing, so the + * global config still applies. + */ + private boolean effectiveUserVerificationRequired(ChallengeRecord challengeRecord) { + boolean configRequired = + WebAuthn4JConverters.userVerificationRequired(ceremonyConfig.userVerification()); + boolean perRequestRequired = + challengeRecord.userVerification() != null + && WebAuthn4JConverters.userVerificationRequired(challengeRecord.userVerification()); + return configRequired || perRequestRequired; + } + /** * Validates the attested credential data against the configured {@link AttestationTrustPolicy} * and rejects duplicates already stored. Returns the failure result to emit, or {@code null} when @@ -399,7 +422,13 @@ private RegistrationResult persistRegistration( var authData = data.getAttestationObject().getAuthenticatorData(); byte[] coseBytes = WebAuthn4JConverters.serializeCoseKey(acd.getCOSEKey(), objectConverter); - UUID aaguidUuid = AAGUID.ZERO.equals(acd.getAaguid()) ? null : acd.getAaguid().getValue(); + // Null-guard the AAGUID to match evaluateAttestation: AAGUID.ZERO.equals(null) is false, so + // without the explicit null check a null AAGUID would NPE on getValue() and escape as a 500 + // instead of a sealed RegistrationResult. + UUID aaguidUuid = + acd.getAaguid() == null || AAGUID.ZERO.equals(acd.getAaguid()) + ? null + : acd.getAaguid().getValue(); Set transports = EnumSet.noneOf(Transport.class); if (data.getTransports() != null) { @@ -480,18 +509,21 @@ public StartAuthenticationResult startAuthentication( byte[] challenge = challengeGenerator.generate(); ChallengeId challengeId = ChallengeId.random(); + // Resolve UV (per-request override, else ceremony default) and persist it on the record so + // finishAuthentication enforces it server-side (see startRegistration for rationale). + UserVerificationRequirement uv = + req.userVerification() == null ? ceremonyConfig.userVerification() : req.userVerification(); + challengeStore.put( challengeId, new ChallengeRecord( challenge, ChallengeRecord.Purpose.AUTHENTICATION, resolvedHandle, + uv, clockProvider.now().plus(ceremonyConfig.challengeTtl())), ceremonyConfig.challengeTtl()); - UserVerificationRequirement uv = - req.userVerification() == null ? ceremonyConfig.userVerification() : req.userVerification(); - PublicKeyCredentialRequestOptionsJson options = new PublicKeyCredentialRequestOptionsJson( challenge, DEFAULT_TIMEOUT_MS, rpConfig.id(), allowCredentials, uv, null); @@ -648,7 +680,7 @@ private AuthenticationData verifyAssertionWithW4j( serverProperty, w4jCred, /* allowCredentials */ null, - WebAuthn4JConverters.userVerificationRequired(ceremonyConfig.userVerification()), + effectiveUserVerificationRequired(challengeRecord), /* userPresenceRequired */ true); return webAuthnManager.verify(w4jRequest, w4jParams); } diff --git a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/spi/ChallengeRecord.java b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/spi/ChallengeRecord.java index 089199f..d7eb1ea 100644 --- a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/spi/ChallengeRecord.java +++ b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/spi/ChallengeRecord.java @@ -2,6 +2,7 @@ package com.codeheadsystems.pkauth.spi; import com.codeheadsystems.pkauth.api.UserHandle; +import com.codeheadsystems.pkauth.api.UserVerificationRequirement; import java.time.Instant; import java.util.Arrays; import java.util.Objects; @@ -14,11 +15,21 @@ * @param purpose whether this challenge belongs to a registration or authentication ceremony * @param userHandle the user this challenge is bound to; nullable for usernameless flows where the * user is only known at finish + * @param userVerification the user-verification requirement resolved at start (the per-request + * override if the caller supplied one, otherwise the ceremony default); persisted so the finish + * step can enforce it server-side rather than letting a per-request {@code REQUIRED} be + * silently downgraded. {@code null} means "no resolved requirement recorded" (legacy records / + * direct constructions), in which case the finish step enforces only the global ceremony + * config. * @param expiresAt absolute expiration; consumers should treat past-due records as missing * @since 0.9.0 */ public record ChallengeRecord( - byte[] challenge, Purpose purpose, @Nullable UserHandle userHandle, Instant expiresAt) { + byte[] challenge, + Purpose purpose, + @Nullable UserHandle userHandle, + @Nullable UserVerificationRequirement userVerification, + Instant expiresAt) { public ChallengeRecord { Objects.requireNonNull(challenge, "challenge"); @@ -30,6 +41,17 @@ public record ChallengeRecord( challenge = challenge.clone(); } + /** + * Back-compatible constructor for callers that don't carry a resolved user-verification + * requirement; equivalent to passing {@code null} for {@code userVerification}. + * + * @since 2.1.0 + */ + public ChallengeRecord( + byte[] challenge, Purpose purpose, @Nullable UserHandle userHandle, Instant expiresAt) { + this(challenge, purpose, userHandle, null, expiresAt); + } + @Override public byte[] challenge() { return challenge.clone(); @@ -41,12 +63,14 @@ public boolean equals(Object o) { && Arrays.equals(this.challenge, other.challenge) && this.purpose == other.purpose && Objects.equals(this.userHandle, other.userHandle) + && this.userVerification == other.userVerification && this.expiresAt.equals(other.expiresAt); } @Override public int hashCode() { - return Objects.hash(Arrays.hashCode(challenge), purpose, userHandle, expiresAt); + return Objects.hash( + Arrays.hashCode(challenge), purpose, userHandle, userVerification, expiresAt); } /** diff --git a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/spi/OtpRepository.java b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/spi/OtpRepository.java index d474ecd..02016e7 100644 --- a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/spi/OtpRepository.java +++ b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/spi/OtpRepository.java @@ -57,10 +57,16 @@ record StoredOtp( void save(StoredOtp otp); /** - * Returns the most recently issued, non-consumed, non-expired OTP for the given user + phone, if - * any. Used by {@code OtpService.verify} as the candidate for matching. + * Returns the most recently issued, non-consumed, and non-expired ({@code expiresAt > now}) OTP + * for the given user + phone, if any. Used by {@code OtpService.verify} as the candidate for + * matching. + * + * @param userHandle owning user + * @param phoneE164 destination phone in E.164 format + * @param now the caller's current instant (from the host ClockProvider) + * @since 2.1.0 */ - Optional findLatestActive(UserHandle userHandle, String phoneE164); + Optional findLatestActive(UserHandle userHandle, String phoneE164, Instant now); /** * Atomically increments the attempts counter for the supplied OTP id and returns the new count. diff --git a/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/lifecycle/UserDeletionServiceTest.java b/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/lifecycle/UserDeletionServiceTest.java index 2706b91..2073efd 100644 --- a/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/lifecycle/UserDeletionServiceTest.java +++ b/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/lifecycle/UserDeletionServiceTest.java @@ -172,7 +172,8 @@ private static final class StubOtpRepository implements OtpRepository { public void save(StoredOtp otp) {} @Override - public Optional findLatestActive(UserHandle userHandle, String phoneE164) { + public Optional findLatestActive( + UserHandle userHandle, String phoneE164, Instant now) { return Optional.empty(); } diff --git a/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/spi/ChallengeRecordTest.java b/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/spi/ChallengeRecordTest.java index 5e272d9..750ae75 100644 --- a/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/spi/ChallengeRecordTest.java +++ b/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/spi/ChallengeRecordTest.java @@ -5,6 +5,7 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import com.codeheadsystems.pkauth.api.UserHandle; +import com.codeheadsystems.pkauth.api.UserVerificationRequirement; import java.time.Instant; import org.junit.jupiter.api.Test; @@ -24,6 +25,29 @@ void buildsAndCopiesChallenge() { assertThat(rec.expiresAt()).isEqualTo(exp); } + @Test + void carriesUserVerificationAndDefaultsToNullViaCompatConstructor() { + Instant exp = Instant.parse("2024-01-01T00:05:00Z"); + UserHandle uh = UserHandle.random(); + + ChallengeRecord withUv = + new ChallengeRecord( + new byte[] {1}, + ChallengeRecord.Purpose.AUTHENTICATION, + uh, + UserVerificationRequirement.REQUIRED, + exp); + assertThat(withUv.userVerification()).isEqualTo(UserVerificationRequirement.REQUIRED); + + // The back-compat 4-arg constructor records no resolved requirement. + ChallengeRecord legacy = + new ChallengeRecord(new byte[] {1}, ChallengeRecord.Purpose.AUTHENTICATION, uh, exp); + assertThat(legacy.userVerification()).isNull(); + + // userVerification participates in equality. + assertThat(withUv).isNotEqualTo(legacy); + } + @Test void equalsAndHashCode() { Instant exp = Instant.parse("2024-01-01T00:05:00Z"); 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 0a5be29..767e6ba 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 @@ -8,8 +8,8 @@ import com.codeheadsystems.pkauth.backupcodes.BackupCodeService; import com.codeheadsystems.pkauth.dropwizard.admin.PkAuthAdminResource; import com.codeheadsystems.pkauth.dropwizard.config.PkAuthConfig; -import com.codeheadsystems.pkauth.jwt.PkAuthJwtIssuer; -import com.codeheadsystems.pkauth.jwt.PkAuthJwtValidator; +import com.codeheadsystems.pkauth.jwt.JwtConfig; +import com.codeheadsystems.pkauth.jwt.JwtKeyset; import com.codeheadsystems.pkauth.lifecycle.BackupCodeRepositoryDeletionListener; import com.codeheadsystems.pkauth.lifecycle.OtpRepositoryDeletionListener; import com.codeheadsystems.pkauth.lifecycle.UserDeletionListener; @@ -156,8 +156,8 @@ BackupCodeService provideBackupCodeService(BackupCodeRepository repo, ClockProvi @Provides @Singleton MagicLinkService provideMagicLinkService( - PkAuthJwtIssuer issuer, - PkAuthJwtValidator validator, + JwtConfig jwtConfig, + JwtKeyset keyset, EmailSender emailSender, UserLookup userLookup, ClockProvider clock, @@ -168,8 +168,11 @@ MagicLinkService provideMagicLinkService( "pkAuth.magicLink configuration block is required when alt-flow auto-wiring is enabled" + " (set magicLink.baseUrl)."); } + // Dedicated magic-link issuer/validator scoped to MagicLinkService.DEFAULT_AUDIENCE so the + // resource-server validator (application audience) rejects magic-link tokens as bearer tokens. return MagicLinkService.create( - MagicLinkService.Dependencies.of(issuer, validator, emailSender, userLookup, clock), + MagicLinkService.Dependencies.ofDedicatedAudience( + keyset, jwtConfig.issuer(), emailSender, userLookup, clock), ml.baseUrl()); } diff --git a/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/JwtKeyset.java b/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/JwtKeyset.java index c8265cc..bf0840b 100644 --- a/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/JwtKeyset.java +++ b/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/JwtKeyset.java @@ -64,10 +64,13 @@ public static JwtKeyset hs256(byte[] secret) { public static JwtKeyset es256(ECKey current, ECKey... retired) { Objects.requireNonNull(current, "current"); List all = new ArrayList<>(); - all.add(current); + // Store ONLY public halves in the verification set. verificationSource() is public and a host + // may publish it as a JWKS endpoint; including the private `d` parameter here would leak the + // signing key. The full `current` (with the private key) is retained separately for signer(). + all.add(current.toPublicJWK()); for (ECKey r : retired) { Objects.requireNonNull(r, "retired key"); - all.add(r); + all.add(r.toPublicJWK()); } return new JwtKeyset(current, JWSAlgorithm.ES256, all); } diff --git a/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/PkAuthJwtIssuer.java b/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/PkAuthJwtIssuer.java index fed1c86..5328a5d 100644 --- a/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/PkAuthJwtIssuer.java +++ b/pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/PkAuthJwtIssuer.java @@ -7,6 +7,7 @@ import com.nimbusds.jose.JWSHeader; import com.nimbusds.jwt.JWTClaimsSet; import com.nimbusds.jwt.SignedJWT; +import java.time.Duration; import java.time.Instant; import java.util.Date; import java.util.List; @@ -14,6 +15,7 @@ import java.util.Objects; import java.util.Optional; import java.util.UUID; +import org.jspecify.annotations.Nullable; /** * Issues pk-auth JWTs. Maps {@link JwtClaims} onto the standard {@code iss/sub/aud/iat/nbf/exp/jti} @@ -78,11 +80,30 @@ public PkAuthJwtIssuer( * to the caller — the host must retry or surface the failure upstream. */ public String issue(JwtClaims claims) { + return issue(claims, null); + } + + /** + * Issues a signed JWT whose lifetime is {@code ttlOverride} instead of the per-audience + * access-token TTL resolved from {@link JwtConfig#ttlPolicy()}. Use this for short-lived, + * single-purpose tokens (e.g. magic links) that must NOT inherit the full access-token validity + * window — a magic link minted at the 1-hour access TTL stays redeemable as a bearer token (and + * replayable past its single-use JTI retention) far longer than intended. A {@code null} {@code + * ttlOverride} falls back to the audience's access TTL, making this exactly equivalent to {@link + * #issue(JwtClaims)}. + * + * @param claims the claims to sign + * @param ttlOverride the token lifetime, or {@code null} to use the audience access TTL + * @return the serialized, signed JWT + * @since 2.1.0 + */ + public String issue(JwtClaims claims, @Nullable Duration ttlOverride) { Objects.requireNonNull(claims, "claims"); Instant now = clockProvider.now(); Instant nbf = now.minus(config.notBeforeSkew()); String audience = claims.audience() != null ? claims.audience() : config.defaultAudience(); - Instant exp = now.plus(config.ttlPolicy().accessTtl(audience)); + Instant exp = + now.plus(ttlOverride != null ? ttlOverride : config.ttlPolicy().accessTtl(audience)); String jti = UUID.randomUUID().toString(); JWTClaimsSet.Builder body = 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 3d7aae2..7da9469 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 @@ -4,6 +4,8 @@ import com.codeheadsystems.pkauth.api.UserHandle; import com.codeheadsystems.pkauth.jwt.AuthMethod; import com.codeheadsystems.pkauth.jwt.JwtClaims; +import com.codeheadsystems.pkauth.jwt.JwtConfig; +import com.codeheadsystems.pkauth.jwt.JwtKeyset; import com.codeheadsystems.pkauth.jwt.JwtVerificationResult; import com.codeheadsystems.pkauth.jwt.PkAuthJwtIssuer; import com.codeheadsystems.pkauth.jwt.PkAuthJwtValidator; @@ -80,6 +82,18 @@ public final class MagicLinkService { /** Default rate-limit window. */ public static final Duration DEFAULT_RATE_WINDOW = Duration.ofHours(1); + /** + * Dedicated audience for magic-link JWTs. Magic-link tokens are minted with this audience (not + * the application's resource-server audience) so that the host's ordinary {@link + * PkAuthJwtValidator} — which only accepts the application audience — rejects them. This is what + * prevents a magic-link token (which sits in an email inbox / proxy log) from being replayed as + * an API bearer/access token. Wire the service via {@link Dependencies#ofDedicatedAudience} so + * the magic-link issuer and validator are both scoped to this audience. + * + * @since 2.1.0 + */ + public static final String DEFAULT_AUDIENCE = "pkauth:magic-link"; + /** Result of a send attempt. */ public sealed interface SendResult { /** Email was dispatched. */ @@ -139,6 +153,7 @@ public interface MagicLinkRateLimiter { private final int rateLimit; private final MagicLinkRateLimiter rateLimiter; private final ConsumedJtiStore consumedJtiStore; + private final Duration tokenTtl; private final Duration consumedJtiTtl; private final MessageFormatter messageFormatter; @@ -153,6 +168,7 @@ private MagicLinkService(Dependencies deps, Config config) { this.baseUrl = config.baseUrl(); this.rateLimit = config.rateLimit(); this.rateLimiter = config.rateLimiter(); + this.tokenTtl = config.tokenTtl(); this.consumedJtiTtl = config.consumedJtiTtl(); if (consumedJtiStore instanceof InMemoryConsumedJtiStore) { LOG.warn( @@ -250,6 +266,15 @@ public SendResult startEmailVerification(UserHandle user, String email) { * dominates and varies — hosts that need timing-side-channel resistance should front this with a * uniform-latency wrapper or rate-limit and monitor for enumeration probing. * + *

The login link is delivered ONLY to the address bound to the resolved user + * via {@link UserLookup#emailFor(UserHandle)} — never to the caller-supplied {@code email}. + * Sending to a caller-supplied address would let an attacker request a login token for any + * account by username and have it delivered to an address they control (account takeover). When + * the host has not implemented {@code emailFor} (no trusted destination exists), the send is + * skipped and the same enumeration-resistant {@link SendResult.Sent} shape is returned. The + * {@code email} parameter is retained for source/back-compatibility but is not used as the + * delivery address. + * * @since 0.9.1 */ public SendResult startLogin(String username, String email) { @@ -263,6 +288,18 @@ public SendResult startLogin(String username, String email) { return new SendResult.Sent(""); } UserHandle user = resolved.get(); + // SECURITY: resolve the delivery address from the binding, not the caller-supplied parameter. + Optional bound = userLookup.emailFor(user); + if (bound.isEmpty()) { + LOG.warn( + "magiclink.login no-bound-email user={} — UserLookup#emailFor returned empty; refusing" + + " to send a login link to a caller-supplied address. Implement UserLookup#emailFor" + + " so a trusted destination can be established.", + user); + // Enumeration-resistant: same Sent shape as the not-found / success paths. + return new SendResult.Sent(""); + } + String deliveryEmail = bound.get(); int count = rateLimiter.countAndIncrement(user, PURPOSE_LOGIN, clockProvider.now()); if (count > rateLimit) { return new SendResult.RateLimited(count); @@ -270,8 +307,8 @@ public SendResult startLogin(String username, String email) { String token = issue(user, PURPOSE_LOGIN, Map.of()); String url = baseUrl + "?t=" + URLEncoder.encode(token, StandardCharsets.UTF_8); MagicLinkMessage message = - messageFormatter.format(new MagicLinkContext(user, email, url, PURPOSE_LOGIN)); - emailSender.send(email, message.subject(), message.body()); + messageFormatter.format(new MagicLinkContext(user, deliveryEmail, url, PURPOSE_LOGIN)); + emailSender.send(deliveryEmail, message.subject(), message.body()); return new SendResult.Sent(token); } @@ -335,7 +372,10 @@ private String issue(UserHandle user, String purpose, Map extras Map additional = new HashMap<>(extras); additional.put(CLAIM_PURPOSE, purpose); JwtClaims claims = new JwtClaims(user, AuthMethod.MAGIC_LINK, null, List.of("eml"), additional); - return issuer.issue(claims); + // Issue with the magic-link tokenTtl (default 15m), NOT the issuer's per-audience access TTL + // (default 1h). Inheriting the access TTL would leave the link redeemable — and replayable + // once its single-use JTI is evicted at consumedJtiTtl — for far longer than intended. + return issuer.issue(claims, tokenTtl); } private static @Nullable String stringClaim(@Nullable Map map, String name) { @@ -406,6 +446,52 @@ public static Dependencies of( new InMemoryConsumedJtiStore(DEFAULT_CONSUMED_JTI_TTL), new DefaultMagicLinkFormatter()); } + + /** + * Builds {@link Dependencies} with a dedicated magic-link JWT issuer and + * validator, both scoped to {@link MagicLinkService#DEFAULT_AUDIENCE} and derived from the + * host's {@code keyset} and {@code issuerName} (pass the resource-server issuer so the {@code + * iss} claim is consistent). This is the recommended wiring: because magic-link tokens carry + * the magic-link audience rather than the application audience, the host's resource-server + * {@link PkAuthJwtValidator} rejects them, so a magic-link token cannot be replayed as an API + * bearer/access token (token-confusion defense). The dedicated issuer also uses a no-op + * access-token store, so magic-link jtis never pollute the host's {@code AccessTokenStore}. + * + *

Prefer this over {@link #of} for production wiring. Token lifetime is controlled by {@link + * MagicLinkService} (the {@link Config#tokenTtl()}), not the issuer's access-token TTL, so the + * audience config's TTL policy is irrelevant here. + * + * @param keyset the host's signing keyset (shared with the resource-server issuer) + * @param issuerName the {@code iss} claim value (the resource-server issuer name) + * @param emailSender the email transport + * @param userLookup the user lookup SPI + * @param clockProvider the clock + * @return dependencies wired to a magic-link-scoped issuer + validator + * @since 2.1.0 + */ + public static Dependencies ofDedicatedAudience( + JwtKeyset keyset, + String issuerName, + EmailSender emailSender, + UserLookup userLookup, + ClockProvider clockProvider) { + Objects.requireNonNull(keyset, "keyset"); + Objects.requireNonNull(issuerName, "issuerName"); + JwtConfig audienceConfig = JwtConfig.defaults(issuerName, DEFAULT_AUDIENCE); + // No-op AccessTokenStore on purpose: magic-link jtis must not be recorded as live access + // tokens. Single-use is enforced separately via the ConsumedJtiStore. + PkAuthJwtIssuer dedicatedIssuer = new PkAuthJwtIssuer(audienceConfig, keyset, clockProvider); + PkAuthJwtValidator dedicatedValidator = + new PkAuthJwtValidator(audienceConfig, keyset, clockProvider); + return new Dependencies( + dedicatedIssuer, + dedicatedValidator, + emailSender, + userLookup, + clockProvider, + new InMemoryConsumedJtiStore(DEFAULT_CONSUMED_JTI_TTL), + new DefaultMagicLinkFormatter()); + } } /** @@ -419,35 +505,65 @@ 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. + *

{@code consumedJtiTtl} must be at least as long as {@code tokenTtl}. + * 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. This service now OWNS the token TTL + * ({@code tokenTtl}, default {@link #DEFAULT_TTL} = 15m) and issues magic links with it + * explicitly (rather than inheriting the JWT issuer's 1h access TTL), so the invariant is + * enforced at construction: the compact constructor rejects {@code consumedJtiTtl < tokenTtl}. + * The defaults are safe (30m retention vs 15m token TTL). * * @since 0.9.1 */ public record Config( - String baseUrl, int rateLimit, MagicLinkRateLimiter rateLimiter, Duration consumedJtiTtl) { + String baseUrl, + int rateLimit, + MagicLinkRateLimiter rateLimiter, + Duration tokenTtl, + Duration consumedJtiTtl) { /** - * Compact constructor — enforces non-null on every field, and rejects a {@code baseUrl} that - * isn't an http(s) URL or that carries whitespace / CRLF (which would enable header-splitting - * if the value flowed into a response header). Hosts running in dev mode may pass {@code - * http://}; production deployments are expected to pass {@code https://}. + * Compact constructor — enforces non-null on every field, rejects a {@code baseUrl} that isn't + * an http(s) URL or that carries whitespace / CRLF (which would enable header-splitting if the + * value flowed into a response header), and enforces {@code consumedJtiTtl >= tokenTtl} so + * single-use cannot be defeated by JTI eviction while a token is still valid. Hosts running in + * dev mode may pass {@code http://}; production deployments are expected to pass {@code + * https://}. */ public Config { Objects.requireNonNull(baseUrl, "baseUrl"); Objects.requireNonNull(rateLimiter, "rateLimiter"); + Objects.requireNonNull(tokenTtl, "tokenTtl"); Objects.requireNonNull(consumedJtiTtl, "consumedJtiTtl"); validateBaseUrl(baseUrl); if (rateLimit < 1) { throw new IllegalArgumentException("rateLimit must be at least 1"); } + if (tokenTtl.isZero() || tokenTtl.isNegative()) { + throw new IllegalArgumentException("tokenTtl must be strictly positive"); + } if (consumedJtiTtl.isZero() || consumedJtiTtl.isNegative()) { throw new IllegalArgumentException("consumedJtiTtl must be strictly positive"); } + if (consumedJtiTtl.compareTo(tokenTtl) < 0) { + throw new IllegalArgumentException( + "consumedJtiTtl (" + + consumedJtiTtl + + ") must be >= tokenTtl (" + + tokenTtl + + ") so a consumed magic link cannot be replayed after its single-use JTI is" + + " evicted but before the token itself expires"); + } + } + + /** + * Back-compatible constructor that defaults {@code tokenTtl} to {@link #DEFAULT_TTL} (15m). + * + * @since 2.1.0 + */ + public Config( + String baseUrl, int rateLimit, MagicLinkRateLimiter rateLimiter, Duration consumedJtiTtl) { + this(baseUrl, rateLimit, rateLimiter, DEFAULT_TTL, consumedJtiTtl); } private static void validateBaseUrl(String baseUrl) { @@ -474,6 +590,7 @@ public static Config defaults(String baseUrl) { baseUrl, DEFAULT_RATE_LIMIT, new InMemoryRateLimiter(DEFAULT_RATE_WINDOW), + DEFAULT_TTL, DEFAULT_CONSUMED_JTI_TTL); } } 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 882fb4b..452fc2c 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 @@ -7,6 +7,7 @@ import com.codeheadsystems.pkauth.api.UserHandle; import com.codeheadsystems.pkauth.jwt.JwtConfig; import com.codeheadsystems.pkauth.jwt.JwtKeyset; +import com.codeheadsystems.pkauth.jwt.JwtVerificationResult; import com.codeheadsystems.pkauth.jwt.PkAuthJwtIssuer; import com.codeheadsystems.pkauth.jwt.PkAuthJwtValidator; import com.codeheadsystems.pkauth.spi.ClockProvider; @@ -95,11 +96,14 @@ void verificationEmailDispatchesAndTokenConsumesOnce() { @Test void loginEmailRoundTrip() { - UserHandle user = users.register("alice", "Alice"); + // startLogin delivers only to the address bound to the resolved user (never the caller-supplied + // one), so the fixture must bind alice's email. + UserHandle user = users.register("alice", "Alice", "alice@example.com"); MagicLinkService.SendResult send = service.startLogin("alice", "alice@example.com"); String token = ((MagicLinkService.SendResult.Sent) send).tokenJti(); assertThat(emails.sent.get(0).subject).isEqualTo("Sign in"); + assertThat(emails.sent.get(0).to).isEqualTo("alice@example.com"); MagicLinkService.ConsumeResult consumed = service.finishVerification(token); assertThat(consumed) @@ -113,7 +117,7 @@ void loginEmailRoundTrip() { @Test void finishVerificationRejectsCrossPurposeTokenWithoutConsumingIt() { - UserHandle user = users.register("alice", "Alice"); + UserHandle user = users.register("alice", "Alice", "alice@example.com"); // A login-purpose token must not satisfy the email-verify consume check. MagicLinkService.SendResult send = service.startLogin("alice", "alice@example.com"); @@ -137,6 +141,35 @@ void finishVerificationRejectsCrossPurposeTokenWithoutConsumingIt() { s -> assertThat(s.userHandle()).isEqualTo(user)); } + @Test + void magicLinkTokenIsRejectedByResourceServerValidatorButConsumedByItsOwnService() { + byte[] secret = new byte[32]; + new SecureRandom().nextBytes(secret); + JwtKeyset keyset = JwtKeyset.hs256(secret); + ClockProvider clock = ClockProvider.fromClock(Clock.fixed(NOW, ZoneOffset.UTC)); + users.register("alice", "Alice", "alice@example.com"); + + // Service wired with a dedicated magic-link audience. + MagicLinkService dedicated = + MagicLinkService.create( + MagicLinkService.Dependencies.ofDedicatedAudience(keyset, ISSUER, emails, users, clock), + BASE_URL); + String token = + ((MagicLinkService.SendResult.Sent) dedicated.startLogin("alice", "alice@example.com")) + .tokenJti(); + + // A resource-server validator scoped to the application audience MUST reject the magic-link + // token (wrong audience) — it cannot be replayed as an API bearer/access token. + PkAuthJwtValidator resourceServer = + new PkAuthJwtValidator(JwtConfig.defaults(ISSUER, AUDIENCE), keyset, clock); + assertThat(resourceServer.validate(token)) + .isInstanceOf(JwtVerificationResult.WrongAudience.class); + + // The magic-link service still validates and consumes its own token. + assertThat(dedicated.finishVerification(token, MagicLinkService.PURPOSE_LOGIN)) + .isInstanceOf(MagicLinkService.ConsumeResult.Success.class); + } + @Test void loginUnknownUserReturnsSentToPreventEnumeration() { // Privacy invariant: startLogin must return Sent even when the username does not exist, diff --git a/pk-auth-micronaut/src/main/java/com/codeheadsystems/pkauth/micronaut/PkAuthFactory.java b/pk-auth-micronaut/src/main/java/com/codeheadsystems/pkauth/micronaut/PkAuthFactory.java index 2092dfc..0b9b03d 100644 --- a/pk-auth-micronaut/src/main/java/com/codeheadsystems/pkauth/micronaut/PkAuthFactory.java +++ b/pk-auth-micronaut/src/main/java/com/codeheadsystems/pkauth/micronaut/PkAuthFactory.java @@ -249,13 +249,16 @@ SmsSender smsSender() { @Singleton MagicLinkService magicLinkService( - PkAuthJwtIssuer issuer, - PkAuthJwtValidator validator, + JwtConfig jwtConfig, + JwtKeyset keyset, EmailSender emailSender, UserLookup userLookup, ClockProvider clock) { + // Dedicated magic-link issuer/validator scoped to MagicLinkService.DEFAULT_AUDIENCE so the + // resource-server validator (application audience) rejects magic-link tokens as bearer tokens. return MagicLinkService.create( - MagicLinkService.Dependencies.of(issuer, validator, emailSender, userLookup, clock), + MagicLinkService.Dependencies.ofDedicatedAudience( + keyset, jwtConfig.issuer(), emailSender, userLookup, clock), "http://localhost:8080/auth/magic"); } 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 c162364..84e353f 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 @@ -181,7 +181,8 @@ public VerifyResult finishVerification(UserHandle user, String phoneE164, String Objects.requireNonNull(phoneE164, "phoneE164"); Objects.requireNonNull(candidate, "candidate"); - Optional activeOpt = repository.findLatestActive(user, phoneE164); + Instant now = clockProvider.now(); + Optional activeOpt = repository.findLatestActive(user, phoneE164, now); if (activeOpt.isEmpty()) { // Run a throwaway HMAC so the no-active-OTP branch takes approximately the same wall-clock // time as the matching/mismatching branches below — the HMAC dominates the cost, so the @@ -192,7 +193,6 @@ public VerifyResult finishVerification(UserHandle user, String phoneE164, String return new VerifyResult.NoActiveOtp(); } StoredOtp active = activeOpt.get(); - Instant now = clockProvider.now(); if (now.isAfter(active.expiresAt())) { return new VerifyResult.Expired(); } diff --git a/pk-auth-otp/src/main/java/com/codeheadsystems/pkauth/otp/TwilioSmsSender.java b/pk-auth-otp/src/main/java/com/codeheadsystems/pkauth/otp/TwilioSmsSender.java index 22bcb9c..5356dbb 100644 --- a/pk-auth-otp/src/main/java/com/codeheadsystems/pkauth/otp/TwilioSmsSender.java +++ b/pk-auth-otp/src/main/java/com/codeheadsystems/pkauth/otp/TwilioSmsSender.java @@ -17,8 +17,16 @@ */ public final class TwilioSmsSender implements SmsSender { + // Retained to document the canonical Twilio constructor shape (account SID, auth token, sender + // number) — the whole point of this naming-anchor skeleton. A real implementation reads them in + // send(); until then they are intentionally unread and MUST NOT be echoed into logs/exceptions. + @SuppressWarnings("unused") private final String accountSid; + + @SuppressWarnings("unused") private final String authToken; + + @SuppressWarnings("unused") private final String fromNumber; public TwilioSmsSender(String accountSid, String authToken, String fromNumber) { @@ -34,16 +42,11 @@ public TwilioSmsSender(String accountSid, String authToken, String fromNumber) { */ @Override public void send(String phoneE164, String body) { + // Deliberately fail loud, but DO NOT echo credential material (account SID, sender number, + // auth-token length) into the exception message — it would land in logs / stack traces if a + // host ever wired this skeleton unmodified. throw new UnsupportedOperationException( - "TwilioSmsSender is a skeleton; host applications must wire the Twilio SDK. accountSid=" - + accountSid - + " fromNumber=" - + fromNumber - + " (authToken length=" - + authToken.length() - + ") phone=" - + phoneE164 - + " bodyLength=" - + body.length()); + "TwilioSmsSender is a skeleton; host applications must add the Twilio SDK dependency and" + + " replace send() with the real REST call before sending SMS."); } } 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 d5bf3fa..1c71e6b 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 @@ -6,6 +6,7 @@ import com.codeheadsystems.pkauth.api.UserHandle; import com.codeheadsystems.pkauth.spi.ClockProvider; +import com.codeheadsystems.pkauth.spi.OtpRepository; import com.codeheadsystems.pkauth.testkit.InMemoryOtpRepository; import java.security.SecureRandom; import java.time.Clock; @@ -14,6 +15,8 @@ import java.time.ZoneOffset; import java.util.ArrayList; import java.util.List; +import java.util.Optional; +import java.util.OptionalInt; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -125,10 +128,14 @@ void sendRateLimits() { void expiredOtpIsRejected() { service.startVerification(USER, PHONE); String code = sms.lastCode(); + // Wrap the repository so it does NOT filter expiry — this exercises the service's own expiry + // re-check (defense-in-depth). A conforming repository filters expired rows itself (verified by + // the persistence integration tests); this asserts the service still rejects an expired row if + // a host repository hands one back. OtpService advanced = OtpService.create( OtpService.Dependencies.of( - repository, + new NonFilteringOtpRepository(repository), sms, ClockProvider.fromClock( Clock.fixed(NOW.plus(Duration.ofMinutes(10)), ZoneOffset.UTC))), @@ -143,6 +150,50 @@ void expiredOtpIsRejected() { .isInstanceOf(OtpService.VerifyResult.Expired.class); } + /** + * {@link OtpRepository} decorator that ignores the {@code now} expiry filter (delegates with + * {@link Instant#MIN}), modelling a host repository that does not filter expired rows in-store — + * so the service-level expiry re-check is the thing under test. + */ + private static final class NonFilteringOtpRepository implements OtpRepository { + private final OtpRepository delegate; + + NonFilteringOtpRepository(OtpRepository delegate) { + this.delegate = delegate; + } + + @Override + public void save(StoredOtp otp) { + delegate.save(otp); + } + + @Override + public Optional findLatestActive( + UserHandle userHandle, String phoneE164, Instant now) { + return delegate.findLatestActive(userHandle, phoneE164, Instant.MIN); + } + + @Override + public OptionalInt incrementAttempts(UserHandle userHandle, String otpId) { + return delegate.incrementAttempts(userHandle, otpId); + } + + @Override + public boolean consume(UserHandle userHandle, String otpId) { + return delegate.consume(userHandle, otpId); + } + + @Override + public int countSince(UserHandle userHandle, String phoneE164, Instant since) { + return delegate.countSince(userHandle, phoneE164, since); + } + + @Override + public int deleteByUserHandle(UserHandle userHandle) { + return delegate.deleteByUserHandle(userHandle); + } + } + @Test void maskPhoneKeepsCountryPrefixAndLast4() { // Normal E.164 numbers: keep '+' + first country digit + '***' + last 4 digits. diff --git a/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/ChallengeItem.java b/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/ChallengeItem.java index 04c61bd..a93cb35 100644 --- a/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/ChallengeItem.java +++ b/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/ChallengeItem.java @@ -3,6 +3,7 @@ import com.codeheadsystems.pkauth.api.ChallengeId; import com.codeheadsystems.pkauth.api.UserHandle; +import com.codeheadsystems.pkauth.api.UserVerificationRequirement; import com.codeheadsystems.pkauth.json.Base64Url; import com.codeheadsystems.pkauth.spi.ChallengeRecord; import java.time.Instant; @@ -21,6 +22,7 @@ public final class ChallengeItem { private String challenge; private String purpose; private String userHandle; + private String userVerification; private Long expiresAt; @DynamoDbPartitionKey @@ -81,6 +83,14 @@ public void setUserHandle(String userHandle) { this.userHandle = userHandle; } + public String getUserVerification() { + return userVerification; + } + + public void setUserVerification(String userVerification) { + this.userVerification = userVerification; + } + public Long getExpiresAt() { return expiresAt; } @@ -100,6 +110,8 @@ public static ChallengeItem build(ChallengeId id, ChallengeRecord record) { item.setPurpose(record.purpose().name()); item.setUserHandle( record.userHandle() == null ? null : Base64Url.encode(record.userHandle().value())); + item.setUserVerification( + record.userVerification() == null ? null : record.userVerification().name()); item.setExpiresAt(record.expiresAt().toEpochMilli()); return item; } @@ -110,6 +122,7 @@ public ChallengeRecord toRecord() { Base64Url.decode(challenge), ChallengeRecord.Purpose.valueOf(purpose), userHandle == null ? null : UserHandle.of(Base64Url.decode(userHandle)), + userVerification == null ? null : UserVerificationRequirement.valueOf(userVerification), Instant.ofEpochMilli(expiresAt)); } } diff --git a/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbChallengeStore.java b/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbChallengeStore.java index 8c685af..abeb81b 100644 --- a/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbChallengeStore.java +++ b/pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbChallengeStore.java @@ -53,6 +53,9 @@ public void put(ChallengeId id, ChallengeRecord record, Duration ttl) { if (item.getUserHandle() != null) { values.put("userHandle", AttributeValue.fromS(item.getUserHandle())); } + if (item.getUserVerification() != null) { + values.put("userVerification", AttributeValue.fromS(item.getUserVerification())); + } values.put("expiresAt", AttributeValue.fromN(Long.toString(item.getExpiresAt()))); client.putItem(PutItemRequest.builder().tableName(tableName).item(values).build()); return null; @@ -106,6 +109,9 @@ public Optional takeOnce(ChallengeId id) { if (attrs.containsKey("userHandle")) { item.setUserHandle(attrs.get("userHandle").s()); } + if (attrs.containsKey("userVerification")) { + item.setUserVerification(attrs.get("userVerification").s()); + } item.setExpiresAt(Long.parseLong(attrs.get("expiresAt").n())); return Optional.of(item.toRecord()); }); 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 2d37a08..a45f9f3 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 @@ -49,7 +49,8 @@ public void save(StoredOtp otp) { } @Override - public Optional findLatestActive(UserHandle userHandle, String phoneE164) { + public Optional findLatestActive( + UserHandle userHandle, String phoneE164, Instant now) { return DynamoDbSupport.wrap( "otp.findLatestActive", () -> { @@ -65,6 +66,9 @@ public Optional findLatestActive(UserHandle userHandle, String phoneE .flatMap(page -> page.items().stream()) .filter(i -> phoneE164.equals(i.getPhoneE164())) .filter(i -> !i.isConsumed()) + // Filter expiry against the caller-supplied instant (the host ClockProvider's "now"), + // not the DynamoDB wall clock, so a single authoritative clock governs expiry. + .filter(i -> DynamoDbSupport.parseInstant(i.getExpiresAt()).isAfter(now)) // 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 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 ab343d1..f44749e 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 @@ -8,6 +8,7 @@ import com.codeheadsystems.pkauth.refresh.RevokeReason; import com.codeheadsystems.pkauth.refresh.spi.Amr; import com.codeheadsystems.pkauth.refresh.spi.RefreshTokenRepository; +import com.codeheadsystems.pkauth.spi.PkAuthPersistenceException; import java.time.Duration; import java.time.Instant; import java.util.LinkedHashMap; @@ -21,7 +22,6 @@ import software.amazon.awssdk.enhanced.dynamodb.Key; import software.amazon.awssdk.enhanced.dynamodb.TableSchema; import software.amazon.awssdk.enhanced.dynamodb.model.IgnoreNullsMode; -import software.amazon.awssdk.enhanced.dynamodb.model.PutItemEnhancedRequest; import software.amazon.awssdk.enhanced.dynamodb.model.QueryConditional; import software.amazon.awssdk.enhanced.dynamodb.model.ScanEnhancedRequest; import software.amazon.awssdk.enhanced.dynamodb.model.TransactPutItemEnhancedRequest; @@ -94,7 +94,56 @@ public void create(RefreshTokenRecord record) { DynamoDbSupport.wrap( "refresh_tokens.create", () -> { - putAllItems(record, /*requirePrimaryAbsent*/ true); + // Write the primary item plus its user-index and family-index pointers as a single atomic + // TransactWriteItems — the same idiom rotateAtomically uses. If any of the three puts + // fails + // (throttle/crash) none commit, so a breach-response revokeFamily/revokeAllForUser (which + // scan the index pointers) can never miss an orphaned primary whose index entries were + // never written. The primary carries an attribute_not_exists(pk) guard so a duplicate + // refreshId is rejected; the two index items are unconditional puts but ride the same + // transaction. + String userB64 = Base64Url.encode(record.userHandle().value()); + RefreshTokenItem primary = + toItem( + record, + PRIMARY_PK_PREFIX + record.refreshId(), + PRIMARY_PK_PREFIX + record.refreshId()); + RefreshTokenItem userIndex = + toItem(record, USER_PK_PREFIX + userB64, INDEX_SK_PREFIX + record.refreshId()); + RefreshTokenItem familyIndex = + toItem( + record, + FAMILY_PK_PREFIX + record.familyId(), + INDEX_SK_PREFIX + record.refreshId()); + try { + enhanced.transactWriteItems( + TransactWriteItemsEnhancedRequest.builder() + .addPutItem( + table, + TransactPutItemEnhancedRequest.builder(RefreshTokenItem.class) + .item(primary) + .conditionExpression( + Expression.builder().expression("attribute_not_exists(pk)").build()) + .build()) + .addPutItem(table, userIndex) + .addPutItem(table, familyIndex) + .build()); + } catch (TransactionCanceledException cancelled) { + // The primary's attribute_not_exists(pk) guard is the first action added (index 0); a + // ConditionalCheckFailed there means the refreshId already exists. Surface it as the + // same + // duplicate signal create() has always produced, but routed through + // DynamoDbSupport.wrap + // as a PkAuthPersistenceException like every other failure in this class — rather than + // as + // a raw IllegalStateException that bypassed the wrapper. Any other cancellation + // (throughput, transaction conflict, validation) is rethrown for the wrapper to map. + if (isPrimaryDuplicate(cancelled)) { + throw new PkAuthPersistenceException( + "refresh_tokens.create", "duplicate refreshId: " + record.refreshId(), cancelled); + } + throw cancelled; + } return null; }); } @@ -222,6 +271,22 @@ private static boolean isParentFreshnessFailure(TransactionCanceledException can return CONDITIONAL_CHECK_FAILED.equals(reasons.get(0).code()); } + /** + * True only when the {@code create} transaction was cancelled because the primary item's {@code + * attribute_not_exists(pk)} guard failed — i.e. the refreshId already exists. The primary {@code + * Put} is the first action added to the transaction, so its reason is at index 0; a {@code + * ConditionalCheckFailed} code there is the duplicate signal. Any other cancellation reason + * (throughput, transaction conflict, validation) is transient and is rethrown by the caller for + * {@code DynamoDbSupport.wrap} to map. + */ + private static boolean isPrimaryDuplicate(TransactionCanceledException cancelled) { + List reasons = cancelled.cancellationReasons(); + if (reasons == null || reasons.isEmpty()) { + return false; + } + return CONDITIONAL_CHECK_FAILED.equals(reasons.get(0).code()); + } + @Override public int revokeFamily(String familyId, Instant now, RevokeReason reason) { return DynamoDbSupport.wrap( @@ -408,29 +473,6 @@ public int deleteExpiredBefore(Instant cutoff) { // -- Internals -------------------------------------------------------------------------- - private void putAllItems(RefreshTokenRecord record, boolean requirePrimaryAbsent) { - String userB64 = Base64Url.encode(record.userHandle().value()); - RefreshTokenItem primary = - toItem( - record, PRIMARY_PK_PREFIX + record.refreshId(), PRIMARY_PK_PREFIX + record.refreshId()); - PutItemEnhancedRequest.Builder primaryReq = - PutItemEnhancedRequest.builder(RefreshTokenItem.class).item(primary); - if (requirePrimaryAbsent) { - primaryReq.conditionExpression( - Expression.builder().expression("attribute_not_exists(pk)").build()); - } - try { - table.putItem(primaryReq.build()); - } catch (ConditionalCheckFailedException duplicate) { - throw new IllegalStateException("duplicate refreshId: " + record.refreshId(), duplicate); - } - // User-index pointer (best-effort; not load-bearing for correctness). - table.putItem(toItem(record, USER_PK_PREFIX + userB64, INDEX_SK_PREFIX + record.refreshId())); - // Family-index pointer (best-effort; not load-bearing for correctness). - table.putItem( - toItem(record, FAMILY_PK_PREFIX + record.familyId(), INDEX_SK_PREFIX + record.refreshId())); - } - private void deleteAllItems(RefreshTokenItem primary) { String refreshId = primary.getRefreshId(); table.deleteItem( diff --git a/pk-auth-persistence-dynamodb/src/test/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbAltFlowsIntegrationTest.java b/pk-auth-persistence-dynamodb/src/test/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbAltFlowsIntegrationTest.java index 5a1a4c3..978c59d 100644 --- a/pk-auth-persistence-dynamodb/src/test/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbAltFlowsIntegrationTest.java +++ b/pk-auth-persistence-dynamodb/src/test/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbAltFlowsIntegrationTest.java @@ -92,8 +92,8 @@ void otpDeleteByUserHandleRemovesEveryRowForTheUser() { int removed = otp.deleteByUserHandle(user); assertThat(removed).isEqualTo(2); - assertThat(otp.findLatestActive(user, "+15551110000")).isEmpty(); - assertThat(otp.findLatestActive(other, "+15552220000")).isPresent(); + assertThat(otp.findLatestActive(user, "+15551110000", t0)).isEmpty(); + assertThat(otp.findLatestActive(other, "+15552220000", t0)).isPresent(); } @Test @@ -115,17 +115,17 @@ void otpRoundTripAndCountSince() { t0.plusSeconds(60), t0.plusSeconds(360))); - var active = otp.findLatestActive(user, "+15551234567").orElseThrow(); + var active = otp.findLatestActive(user, "+15551234567", t0).orElseThrow(); assertThat(active.otpId()).isEqualTo("o2"); otp.incrementAttempts(user, "o2"); - var refreshed = otp.findLatestActive(user, "+15551234567").orElseThrow(); + var refreshed = otp.findLatestActive(user, "+15551234567", t0).orElseThrow(); assertThat(refreshed.attempts()).isEqualTo(1); assertThat(otp.countSince(user, "+15551234567", t0.minusSeconds(10))).isEqualTo(2); otp.consume(user, "o2"); - assertThat(otp.findLatestActive(user, "+15551234567")) + assertThat(otp.findLatestActive(user, "+15551234567", t0)) .hasValueSatisfying(o -> assertThat(o.otpId()).isEqualTo("o1")); } diff --git a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiChallengeStore.java b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiChallengeStore.java index de48919..3b545ae 100644 --- a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiChallengeStore.java +++ b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiChallengeStore.java @@ -3,6 +3,7 @@ import com.codeheadsystems.pkauth.api.ChallengeId; import com.codeheadsystems.pkauth.api.UserHandle; +import com.codeheadsystems.pkauth.api.UserVerificationRequirement; import com.codeheadsystems.pkauth.spi.ChallengeRecord; import com.codeheadsystems.pkauth.spi.ChallengeStore; import java.sql.ResultSet; @@ -42,15 +43,21 @@ public void put(ChallengeId id, ChallengeRecord record, Duration ttl) { h -> h.createUpdate( "INSERT INTO challenges (id, challenge, purpose, user_handle," - + " expires_at)" - + " VALUES (:id, :challenge, :purpose, :uh, :expiresAt)" + + " user_verification, expires_at)" + + " VALUES (:id, :challenge, :purpose, :uh, :uv, :expiresAt)" + " ON CONFLICT (id) DO UPDATE SET challenge = EXCLUDED.challenge," + " purpose = EXCLUDED.purpose, user_handle = EXCLUDED.user_handle," + + " user_verification = EXCLUDED.user_verification," + " expires_at = EXCLUDED.expires_at") .bind("id", id.value()) .bind("challenge", record.challenge()) .bind("purpose", record.purpose().name()) .bind("uh", record.userHandle() == null ? null : record.userHandle().value()) + .bind( + "uv", + record.userVerification() == null + ? null + : record.userVerification().name()) .bind( "expiresAt", OffsetDateTime.ofInstant(record.expiresAt(), ZoneOffset.UTC)) .execute()); @@ -67,7 +74,8 @@ public Optional takeOnce(ChallengeId id) { h -> h.createQuery( "DELETE FROM challenges WHERE id = :id AND expires_at > NOW()" - + " RETURNING challenge, purpose, user_handle, expires_at") + + " RETURNING challenge, purpose, user_handle, user_verification," + + " expires_at") .bind("id", id.value()) .map(MAPPER) .findFirst())); @@ -80,7 +88,11 @@ private static ChallengeRecord readRow(ResultSet rs) throws SQLException { ChallengeRecord.Purpose purpose = ChallengeRecord.Purpose.valueOf(rs.getString("purpose")); byte[] userHandleBytes = rs.getBytes("user_handle"); UserHandle userHandle = userHandleBytes == null ? null : UserHandle.of(userHandleBytes); + String uvName = rs.getString("user_verification"); + UserVerificationRequirement userVerification = + uvName == null ? null : UserVerificationRequirement.valueOf(uvName); OffsetDateTime expiresAt = rs.getObject("expires_at", OffsetDateTime.class); - return new ChallengeRecord(challenge, purpose, userHandle, expiresAt.toInstant()); + return new ChallengeRecord( + challenge, purpose, userHandle, userVerification, expiresAt.toInstant()); } } 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 478658e..b7ea4cd 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 @@ -130,8 +130,12 @@ public List findByUserHandle(UserHandle userHandle) { @Override public void updateSignCount(CredentialId credentialId, long newCount, Instant lastUsedAt) { // Guard against concurrent racing assertions overwriting a higher stored counter with a - // lower one — that would silently defeat WebAuthn's clone-detection invariant. Only advance - // the counter when the new value strictly exceeds the stored one. + // lower one — that would silently defeat WebAuthn's clone-detection invariant. Refuse only a + // strict regression (stored > new); allow the equal case through so last_used_at is still + // recorded. Most modern/synced passkeys (iCloud Keychain, Google Password Manager, many FIDO + // keys) always report sign_count = 0, so a strict `<` guard would never fire for them and + // last_used_at would stay NULL forever — see CredentialRepository#updateSignCount, which + // documents this as an unconditional last-writer-wins of both fields. JdbiSupport.wrap( "credentials.updateSignCount", () -> { @@ -139,7 +143,7 @@ public void updateSignCount(CredentialId credentialId, long newCount, Instant la h -> h.createUpdate( "UPDATE credentials SET sign_count = :sc, last_used_at = :lua" - + " WHERE credential_id = :cid AND sign_count < :sc") + + " WHERE credential_id = :cid AND sign_count <= :sc") .bind("sc", newCount) .bind("lua", OffsetDateTime.ofInstant(lastUsedAt, ZoneOffset.UTC)) .bind("cid", credentialId.value()) diff --git a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiOtpRepository.java b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiOtpRepository.java index 10797a7..a4483da 100644 --- a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiOtpRepository.java +++ b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiOtpRepository.java @@ -50,18 +50,23 @@ public void save(StoredOtp otp) { } @Override - public Optional findLatestActive(UserHandle userHandle, String phoneE164) { + public Optional findLatestActive( + UserHandle userHandle, String phoneE164, Instant now) { return JdbiSupport.wrap( "otp.findLatestActive", () -> jdbi.withHandle( h -> h.createQuery( + // Expiry is filtered against the caller-supplied instant (the host + // ClockProvider's "now"), not the database wall clock, so fixed-clock + // tests stay deterministic and there is a single authoritative clock. "SELECT * FROM otp_codes WHERE user_handle = :uh AND phone_e164 =" - + " :phone AND consumed = FALSE" + + " :phone AND consumed = FALSE AND expires_at > :now" + " ORDER BY created_at DESC LIMIT 1") .bind("uh", userHandle.value()) .bind("phone", phoneE164) + .bind("now", OffsetDateTime.ofInstant(now, ZoneOffset.UTC)) .map(MAPPER) .findFirst())); } diff --git a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/PkAuthJdbiSchema.java b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/PkAuthJdbiSchema.java index 4c75200..e45e747 100644 --- a/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/PkAuthJdbiSchema.java +++ b/pk-auth-persistence-jdbi/src/main/java/com/codeheadsystems/pkauth/persistence/jdbi/PkAuthJdbiSchema.java @@ -22,7 +22,7 @@ public final class PkAuthJdbiSchema { * #migrateForDevelopment(DataSource)} pins Flyway's {@code target} to this value so that * unreleased migrations on the classpath are never applied accidentally. */ - public static final String CURRENT_SCHEMA_VERSION = "10"; + public static final String CURRENT_SCHEMA_VERSION = "11"; private PkAuthJdbiSchema() {} diff --git a/pk-auth-persistence-jdbi/src/main/resources/db/migration/V11__challenges_user_verification.sql b/pk-auth-persistence-jdbi/src/main/resources/db/migration/V11__challenges_user_verification.sql new file mode 100644 index 0000000..8eca43c --- /dev/null +++ b/pk-auth-persistence-jdbi/src/main/resources/db/migration/V11__challenges_user_verification.sql @@ -0,0 +1,10 @@ +-- SPDX-License-Identifier: MIT +-- +-- Persist the user-verification requirement resolved at ceremony start so the finish step can +-- enforce a per-request REQUIRED server-side (rather than silently falling back to the global +-- ceremony config). Nullable: legacy/in-flight rows and callers that record no resolved +-- requirement leave it NULL, in which case the finish step enforces only the global config. + +ALTER TABLE challenges + ADD COLUMN user_verification TEXT + CHECK (user_verification IN ('REQUIRED', 'PREFERRED', 'DISCOURAGED')); diff --git a/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiAltFlowsIntegrationTest.java b/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiAltFlowsIntegrationTest.java index 0e08eda..dab5a31 100644 --- a/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiAltFlowsIntegrationTest.java +++ b/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiAltFlowsIntegrationTest.java @@ -101,8 +101,8 @@ void otpDeleteByUserHandleRemovesEveryRowForTheUser() { int removed = otp.deleteByUserHandle(user); assertThat(removed).isEqualTo(1); - assertThat(otp.findLatestActive(user, "+15551110000")).isEmpty(); - assertThat(otp.findLatestActive(other, "+15552220000")).isPresent(); + assertThat(otp.findLatestActive(user, "+15551110000", t0)).isEmpty(); + assertThat(otp.findLatestActive(other, "+15552220000", t0)).isPresent(); } @Test @@ -120,7 +120,7 @@ void incrementAttempts_advancesPastCap() { // (A prior cap-guarded UPDATE made the verifier a no-op past the cap, which allowed unlimited // verify attempts within the TTL.) assertThat(result).hasValue(4); - var stored = otp.findLatestActive(user, "+15559990000").orElseThrow(); + var stored = otp.findLatestActive(user, "+15559990000", t0).orElseThrow(); assertThat(stored.attempts()).isEqualTo(4); } @@ -135,7 +135,7 @@ void incrementAttempts_advancesWhenBelowCap() { var result = otp.incrementAttempts(user, "o-inc"); assertThat(result).hasValue(3); - var stored = otp.findLatestActive(user, "+15558880000").orElseThrow(); + var stored = otp.findLatestActive(user, "+15558880000", t0).orElseThrow(); assertThat(stored.attempts()).isEqualTo(3); } @@ -158,19 +158,19 @@ void otpRoundTripAndCountSince() { t0.plusSeconds(60), t0.plusSeconds(360))); - var active = otp.findLatestActive(user, "+15551234567").orElseThrow(); + var active = otp.findLatestActive(user, "+15551234567", t0).orElseThrow(); assertThat(active.otpId()).isEqualTo("o2"); otp.incrementAttempts(user, "o2"); otp.incrementAttempts(user, "o2"); - var refreshed = otp.findLatestActive(user, "+15551234567").orElseThrow(); + var refreshed = otp.findLatestActive(user, "+15551234567", t0).orElseThrow(); assertThat(refreshed.attempts()).isEqualTo(2); assertThat(otp.countSince(user, "+15551234567", t0.minusSeconds(10))).isEqualTo(2); assertThat(otp.countSince(user, "+15551234567", t0.plusSeconds(120))).isZero(); otp.consume(user, "o2"); - var noActive = otp.findLatestActive(user, "+15551234567"); + var noActive = otp.findLatestActive(user, "+15551234567", t0); assertThat(noActive).hasValueSatisfying(o -> assertThat(o.otpId()).isEqualTo("o1")); } diff --git a/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiRepositoryExceptionWrappingTest.java b/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiRepositoryExceptionWrappingTest.java index 6555a66..ddd7274 100644 --- a/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiRepositoryExceptionWrappingTest.java +++ b/pk-auth-persistence-jdbi/src/test/java/com/codeheadsystems/pkauth/persistence/jdbi/JdbiRepositoryExceptionWrappingTest.java @@ -96,7 +96,9 @@ void otpRepoWrapsConnectionFailure() { .isInstanceOf(PkAuthPersistenceException.class) .extracting("operation") .isEqualTo("otp.save"); - assertThatThrownBy(() -> repo.findLatestActive(user, "+15551112222")) + assertThatThrownBy( + () -> + repo.findLatestActive(user, "+15551112222", Instant.parse("2026-05-15T00:00:00Z"))) .isInstanceOf(PkAuthPersistenceException.class); assertThatThrownBy(() -> repo.incrementAttempts(user, "o1")) .isInstanceOf(PkAuthPersistenceException.class); diff --git a/pk-auth-spring-boot-starter/src/main/java/com/codeheadsystems/pkauth/spring/autoconfigure/PkAuthAutoConfiguration.java b/pk-auth-spring-boot-starter/src/main/java/com/codeheadsystems/pkauth/spring/autoconfigure/PkAuthAutoConfiguration.java index 52cb740..563e773 100644 --- a/pk-auth-spring-boot-starter/src/main/java/com/codeheadsystems/pkauth/spring/autoconfigure/PkAuthAutoConfiguration.java +++ b/pk-auth-spring-boot-starter/src/main/java/com/codeheadsystems/pkauth/spring/autoconfigure/PkAuthAutoConfiguration.java @@ -402,8 +402,8 @@ public BackupCodeService pkAuthBackupCodeService( @ConditionalOnMissingBean @ConditionalOnBean(EmailSender.class) public MagicLinkService pkAuthMagicLinkService( - PkAuthJwtIssuer issuer, - PkAuthJwtValidator validator, + JwtConfig jwtConfig, + JwtKeyset keyset, EmailSender emailSender, UserLookup userLookup, ClockProvider clockProvider, @@ -411,8 +411,11 @@ public MagicLinkService pkAuthMagicLinkService( // baseUrl for the magic link uses the configured RP origin's first entry. Demo apps that // serve at a different path override the service bean explicitly. String baseUrl = props.relyingParty().origins().iterator().next(); + // Dedicated magic-link issuer/validator scoped to MagicLinkService.DEFAULT_AUDIENCE so the + // resource-server validator (application audience) rejects magic-link tokens as bearer tokens. return MagicLinkService.create( - MagicLinkService.Dependencies.of(issuer, validator, emailSender, userLookup, clockProvider), + MagicLinkService.Dependencies.ofDedicatedAudience( + keyset, jwtConfig.issuer(), emailSender, userLookup, clockProvider), baseUrl); } diff --git a/pk-auth-spring-boot-starter/src/main/java/com/codeheadsystems/pkauth/spring/autoconfigure/PkAuthWebAutoConfiguration.java b/pk-auth-spring-boot-starter/src/main/java/com/codeheadsystems/pkauth/spring/autoconfigure/PkAuthWebAutoConfiguration.java index 544512a..e4f6edb 100644 --- a/pk-auth-spring-boot-starter/src/main/java/com/codeheadsystems/pkauth/spring/autoconfigure/PkAuthWebAutoConfiguration.java +++ b/pk-auth-spring-boot-starter/src/main/java/com/codeheadsystems/pkauth/spring/autoconfigure/PkAuthWebAutoConfiguration.java @@ -20,8 +20,11 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; +import org.springframework.boot.web.servlet.FilterRegistrationBean; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Import; +import org.springframework.core.Ordered; +import org.springframework.core.annotation.Order; import org.springframework.http.HttpStatus; import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.security.config.annotation.web.configurers.AbstractHttpConfigurer; @@ -74,7 +77,40 @@ public PkAuthJwtAuthenticationFilter pkAuthJwtAuthenticationFilter(PkAuthJwtVali return new PkAuthJwtAuthenticationFilter(validator); } + /** + * Keep the JWT filter out of the global servlet filter chain. Because {@link + * #pkAuthJwtAuthenticationFilter} is exposed as a top-level {@link + * org.springframework.web.filter.OncePerRequestFilter} bean, Spring Boot would otherwise + * auto-register it with the servlet container so it runs on every request path — meaning + * a valid pk-auth JWT would populate the {@code SecurityContext} application-wide, even outside + * {@code /auth/**}. Disabling the registration confines the filter to where we wire it + * explicitly: inside {@link #pkAuthSecurityFilterChain} via {@code addFilterBefore}. + * + * @param filter the JWT authentication filter bean to suppress from global registration + * @return a disabled registration so the container does not mount the filter globally + * @since 2.1.0 + */ + @Bean + @ConditionalOnMissingBean(name = "pkAuthJwtAuthenticationFilterRegistration") + public FilterRegistrationBean + pkAuthJwtAuthenticationFilterRegistration(PkAuthJwtAuthenticationFilter filter) { + FilterRegistrationBean registration = + new FilterRegistrationBean<>(filter); + registration.setEnabled(false); + return registration; + } + + /** + * Pin the pk-auth {@code /auth/**} chain to an early order. Spring Security consults {@link + * SecurityFilterChain} beans in {@link Order} sequence and the first whose matcher matches wins + * exclusively. A host app that defines its own chain — especially a broad catch-all with no + * {@code securityMatcher}, which matches everything — at the default (lowest) precedence would + * otherwise swallow {@code /auth/**} before this chain is consulted, silently disabling pk-auth's + * rules on the public ceremony/refresh endpoints. Running near the highest precedence ensures our + * narrow {@code /auth/**} matcher is evaluated first. + */ @Bean + @Order(Ordered.HIGHEST_PRECEDENCE + 10) @ConditionalOnMissingBean(name = "pkAuthSecurityFilterChain") public SecurityFilterChain pkAuthSecurityFilterChain( HttpSecurity http, PkAuthJwtAuthenticationFilter filter) throws Exception { diff --git a/pk-auth-spring-boot-starter/src/test/java/com/codeheadsystems/pkauth/spring/autoconfigure/PkAuthWebAutoConfigurationUnitTest.java b/pk-auth-spring-boot-starter/src/test/java/com/codeheadsystems/pkauth/spring/autoconfigure/PkAuthWebAutoConfigurationUnitTest.java new file mode 100644 index 0000000..2606da9 --- /dev/null +++ b/pk-auth-spring-boot-starter/src/test/java/com/codeheadsystems/pkauth/spring/autoconfigure/PkAuthWebAutoConfigurationUnitTest.java @@ -0,0 +1,49 @@ +// SPDX-License-Identifier: MIT +package com.codeheadsystems.pkauth.spring.autoconfigure; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +import com.codeheadsystems.pkauth.jwt.PkAuthJwtValidator; +import com.codeheadsystems.pkauth.spring.security.PkAuthJwtAuthenticationFilter; +import java.lang.reflect.Method; +import org.junit.jupiter.api.Test; +import org.springframework.boot.web.servlet.FilterRegistrationBean; +import org.springframework.core.Ordered; +import org.springframework.core.annotation.Order; + +/** + * Unit-tests the filter-wiring guarantees of {@link PkAuthWebAutoConfiguration}: the JWT filter + * must not be auto-registered as a global servlet filter, and the {@code /auth/**} security chain + * must carry an early {@link Order} so a host's broad catch-all chain cannot intercept the public + * ceremony/refresh endpoints first. + */ +class PkAuthWebAutoConfigurationUnitTest { + + private final PkAuthWebAutoConfiguration autoConfig = new PkAuthWebAutoConfiguration(); + + @Test + void jwtFilterRegistrationIsDisabledSoItDoesNotRunGlobally() { + PkAuthJwtAuthenticationFilter filter = + new PkAuthJwtAuthenticationFilter(mock(PkAuthJwtValidator.class)); + + FilterRegistrationBean registration = + autoConfig.pkAuthJwtAuthenticationFilterRegistration(filter); + + assertThat(registration.getFilter()).isSameAs(filter); + assertThat(registration.isEnabled()).isFalse(); + } + + @Test + void securityFilterChainHasEarlyOrder() throws NoSuchMethodException { + Method chainMethod = + PkAuthWebAutoConfiguration.class.getMethod( + "pkAuthSecurityFilterChain", + org.springframework.security.config.annotation.web.builders.HttpSecurity.class, + PkAuthJwtAuthenticationFilter.class); + + Order order = chainMethod.getAnnotation(Order.class); + assertThat(order).isNotNull(); + assertThat(order.value()).isEqualTo(Ordered.HIGHEST_PRECEDENCE + 10); + } +} diff --git a/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/InMemoryOtpRepository.java b/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/InMemoryOtpRepository.java index 8380c8e..fe12b08 100644 --- a/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/InMemoryOtpRepository.java +++ b/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/InMemoryOtpRepository.java @@ -23,11 +23,13 @@ public void save(StoredOtp otp) { } @Override - public Optional findLatestActive(UserHandle userHandle, String phoneE164) { + public Optional findLatestActive( + UserHandle userHandle, String phoneE164, Instant now) { return byId.values().stream() .filter(o -> o.userHandle().equals(userHandle)) .filter(o -> o.phoneE164().equals(phoneE164)) .filter(o -> !o.consumed()) + .filter(o -> o.expiresAt().isAfter(now)) .max(Comparator.comparing(StoredOtp::createdAt)); } diff --git a/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/InMemoryUserLookup.java b/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/InMemoryUserLookup.java index ca8e0d1..7fae5f4 100644 --- a/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/InMemoryUserLookup.java +++ b/pk-auth-testkit/src/main/java/com/codeheadsystems/pkauth/testkit/InMemoryUserLookup.java @@ -14,6 +14,7 @@ public final class InMemoryUserLookup implements UserLookup { private final Map byUsername = new ConcurrentHashMap<>(); private final Map byHandle = new ConcurrentHashMap<>(); + private final Map emailByHandle = new ConcurrentHashMap<>(); public InMemoryUserLookup() {} @@ -23,6 +24,11 @@ public Optional findHandleByUsername(String username) { return u == null ? Optional.empty() : Optional.of(u.handle()); } + @Override + public Optional emailFor(UserHandle handle) { + return Optional.ofNullable(emailByHandle.get(handle)); + } + @Override public Optional findViewByHandle(UserHandle handle) { return Optional.ofNullable(byHandle.get(handle)); @@ -54,4 +60,17 @@ public UserHandle register(String username, String displayName) { byHandle.put(handle, view); return handle; } + + /** + * Test helper that pre-registers a user with a bound email, exposed via {@link + * #emailFor(UserHandle)}. Useful for flows (e.g. magic-link login) that deliver only to the + * address bound to the resolved user. + * + * @since 2.1.0 + */ + public UserHandle register(String username, String displayName, String email) { + UserHandle handle = register(username, displayName); + emailByHandle.put(handle, email); + return handle; + } } diff --git a/pk-auth-testkit/src/test/java/com/codeheadsystems/pkauth/testkit/InMemoryAltFlowRepositoriesTest.java b/pk-auth-testkit/src/test/java/com/codeheadsystems/pkauth/testkit/InMemoryAltFlowRepositoriesTest.java index fb39535..8d6ad70 100644 --- a/pk-auth-testkit/src/test/java/com/codeheadsystems/pkauth/testkit/InMemoryAltFlowRepositoriesTest.java +++ b/pk-auth-testkit/src/test/java/com/codeheadsystems/pkauth/testkit/InMemoryAltFlowRepositoriesTest.java @@ -38,19 +38,19 @@ void otpRepositoryCrudAndCountSince() { new StoredOtp( "o2", user, "+15551234567", "h", 0, 5, false, t0.plusSeconds(60), t0.plusSeconds(360))); - assertThat(repo.findLatestActive(user, "+15551234567")) + assertThat(repo.findLatestActive(user, "+15551234567", t0)) .hasValueSatisfying(o -> assertThat(o.otpId()).isEqualTo("o2")); repo.incrementAttempts(user, "o2"); repo.incrementAttempts(user, "o2"); - assertThat(repo.findLatestActive(user, "+15551234567")) + assertThat(repo.findLatestActive(user, "+15551234567", t0)) .hasValueSatisfying(o -> assertThat(o.attempts()).isEqualTo(2)); assertThat(repo.countSince(user, "+15551234567", t0.minusSeconds(10))).isEqualTo(2); assertThat(repo.countSince(user, "+15551234567", t0.plusSeconds(120))).isZero(); repo.consume(user, "o2"); - assertThat(repo.findLatestActive(user, "+15551234567")) + assertThat(repo.findLatestActive(user, "+15551234567", t0)) .hasValueSatisfying(o -> assertThat(o.otpId()).isEqualTo("o1")); } }