Skip to content

Commit 20fb20d

Browse files
wolpertclaude
andcommitted
feat: complete the two deferred review items (magic-link audience, OTP expiry filter)
Both were noted as follow-up in the prior commit; they land here with the full `./gradlew check` gate green (Spotless + tests incl. Testcontainers + JaCoCo). magic-link: dedicated audience (token-confusion defense) - MagicLinkService.DEFAULT_AUDIENCE ("pkauth:magic-link") + a new Dependencies.ofDedicatedAudience(keyset, issuerName, ...) factory that builds a magic-link-scoped JWT issuer + validator (no-op AccessTokenStore, so magic-link jtis never enter the access-token store). Because magic-link tokens now carry the magic-link audience, the host's resource-server validator (application audience) rejects them — a magic-link token can no longer be replayed as an API bearer/access token. - All three adapters (Spring autoconfigure, Dropwizard Dagger, Micronaut factory) wire the magic-link service via ofDedicatedAudience using the shared keyset + the resource-server issuer name. - Adds a test proving a magic-link token is rejected by a resource-server validator yet still consumed by its own service. otp: in-store expiry filtering - OtpRepository.findLatestActive gains an `Instant now` parameter so each backend can filter `expiresAt > now` against the HOST clock (not the DB wall clock, which would be a second, uncontrollable clock source and broke fixed-clock tests). OtpService passes clockProvider.now(); the existing service-level expiry re-check stays as defense-in-depth. - JDBI (`AND expires_at > :now`), DynamoDB (stream filter on parsed expiresAt), and the in-memory testkit all filter expiry now. - OtpServiceTest.expiredOtpIsRejected now drives the service-level re-check via a non-filtering repository decorator, preserving that branch's coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent a5e15bb commit 20fb20d

16 files changed

Lines changed: 208 additions & 48 deletions

File tree

pk-auth-core/src/main/java/com/codeheadsystems/pkauth/spi/OtpRepository.java

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,16 @@ record StoredOtp(
5757
void save(StoredOtp otp);
5858

5959
/**
60-
* Returns the most recently issued, non-consumed, non-expired OTP for the given user + phone, if
61-
* any. Used by {@code OtpService.verify} as the candidate for matching.
60+
* Returns the most recently issued, non-consumed, and non-expired ({@code expiresAt > now}) OTP
61+
* for the given user + phone, if any. Used by {@code OtpService.verify} as the candidate for
62+
* matching.
63+
*
64+
* @param userHandle owning user
65+
* @param phoneE164 destination phone in E.164 format
66+
* @param now the caller's current instant (from the host ClockProvider)
67+
* @since 2.1.0
6268
*/
63-
Optional<StoredOtp> findLatestActive(UserHandle userHandle, String phoneE164);
69+
Optional<StoredOtp> findLatestActive(UserHandle userHandle, String phoneE164, Instant now);
6470

6571
/**
6672
* Atomically increments the attempts counter for the supplied OTP id and returns the new count.

pk-auth-core/src/test/java/com/codeheadsystems/pkauth/lifecycle/UserDeletionServiceTest.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,8 @@ private static final class StubOtpRepository implements OtpRepository {
172172
public void save(StoredOtp otp) {}
173173

174174
@Override
175-
public Optional<StoredOtp> findLatestActive(UserHandle userHandle, String phoneE164) {
175+
public Optional<StoredOtp> findLatestActive(
176+
UserHandle userHandle, String phoneE164, Instant now) {
176177
return Optional.empty();
177178
}
178179

pk-auth-dropwizard/src/main/java/com/codeheadsystems/pkauth/dropwizard/dagger/AltFlowsModule.java

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@
88
import com.codeheadsystems.pkauth.backupcodes.BackupCodeService;
99
import com.codeheadsystems.pkauth.dropwizard.admin.PkAuthAdminResource;
1010
import com.codeheadsystems.pkauth.dropwizard.config.PkAuthConfig;
11-
import com.codeheadsystems.pkauth.jwt.PkAuthJwtIssuer;
12-
import com.codeheadsystems.pkauth.jwt.PkAuthJwtValidator;
11+
import com.codeheadsystems.pkauth.jwt.JwtConfig;
12+
import com.codeheadsystems.pkauth.jwt.JwtKeyset;
1313
import com.codeheadsystems.pkauth.lifecycle.BackupCodeRepositoryDeletionListener;
1414
import com.codeheadsystems.pkauth.lifecycle.OtpRepositoryDeletionListener;
1515
import com.codeheadsystems.pkauth.lifecycle.UserDeletionListener;
@@ -156,8 +156,8 @@ BackupCodeService provideBackupCodeService(BackupCodeRepository repo, ClockProvi
156156
@Provides
157157
@Singleton
158158
MagicLinkService provideMagicLinkService(
159-
PkAuthJwtIssuer issuer,
160-
PkAuthJwtValidator validator,
159+
JwtConfig jwtConfig,
160+
JwtKeyset keyset,
161161
EmailSender emailSender,
162162
UserLookup userLookup,
163163
ClockProvider clock,
@@ -168,8 +168,11 @@ MagicLinkService provideMagicLinkService(
168168
"pkAuth.magicLink configuration block is required when alt-flow auto-wiring is enabled"
169169
+ " (set magicLink.baseUrl).");
170170
}
171+
// Dedicated magic-link issuer/validator scoped to MagicLinkService.DEFAULT_AUDIENCE so the
172+
// resource-server validator (application audience) rejects magic-link tokens as bearer tokens.
171173
return MagicLinkService.create(
172-
MagicLinkService.Dependencies.of(issuer, validator, emailSender, userLookup, clock),
174+
MagicLinkService.Dependencies.ofDedicatedAudience(
175+
keyset, jwtConfig.issuer(), emailSender, userLookup, clock),
173176
ml.baseUrl());
174177
}
175178

pk-auth-magic-link/src/main/java/com/codeheadsystems/pkauth/magiclink/MagicLinkService.java

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
import com.codeheadsystems.pkauth.api.UserHandle;
55
import com.codeheadsystems.pkauth.jwt.AuthMethod;
66
import com.codeheadsystems.pkauth.jwt.JwtClaims;
7+
import com.codeheadsystems.pkauth.jwt.JwtConfig;
8+
import com.codeheadsystems.pkauth.jwt.JwtKeyset;
79
import com.codeheadsystems.pkauth.jwt.JwtVerificationResult;
810
import com.codeheadsystems.pkauth.jwt.PkAuthJwtIssuer;
911
import com.codeheadsystems.pkauth.jwt.PkAuthJwtValidator;
@@ -80,6 +82,18 @@ public final class MagicLinkService {
8082
/** Default rate-limit window. */
8183
public static final Duration DEFAULT_RATE_WINDOW = Duration.ofHours(1);
8284

85+
/**
86+
* Dedicated audience for magic-link JWTs. Magic-link tokens are minted with this audience (not
87+
* the application's resource-server audience) so that the host's ordinary {@link
88+
* PkAuthJwtValidator} — which only accepts the application audience — rejects them. This is what
89+
* prevents a magic-link token (which sits in an email inbox / proxy log) from being replayed as
90+
* an API bearer/access token. Wire the service via {@link Dependencies#ofDedicatedAudience} so
91+
* the magic-link issuer and validator are both scoped to this audience.
92+
*
93+
* @since 2.1.0
94+
*/
95+
public static final String DEFAULT_AUDIENCE = "pkauth:magic-link";
96+
8397
/** Result of a send attempt. */
8498
public sealed interface SendResult {
8599
/** Email was dispatched. */
@@ -432,6 +446,52 @@ public static Dependencies of(
432446
new InMemoryConsumedJtiStore(DEFAULT_CONSUMED_JTI_TTL),
433447
new DefaultMagicLinkFormatter());
434448
}
449+
450+
/**
451+
* Builds {@link Dependencies} with a <strong>dedicated</strong> magic-link JWT issuer and
452+
* validator, both scoped to {@link MagicLinkService#DEFAULT_AUDIENCE} and derived from the
453+
* host's {@code keyset} and {@code issuerName} (pass the resource-server issuer so the {@code
454+
* iss} claim is consistent). This is the recommended wiring: because magic-link tokens carry
455+
* the magic-link audience rather than the application audience, the host's resource-server
456+
* {@link PkAuthJwtValidator} rejects them, so a magic-link token cannot be replayed as an API
457+
* bearer/access token (token-confusion defense). The dedicated issuer also uses a no-op
458+
* access-token store, so magic-link jtis never pollute the host's {@code AccessTokenStore}.
459+
*
460+
* <p>Prefer this over {@link #of} for production wiring. Token lifetime is controlled by {@link
461+
* MagicLinkService} (the {@link Config#tokenTtl()}), not the issuer's access-token TTL, so the
462+
* audience config's TTL policy is irrelevant here.
463+
*
464+
* @param keyset the host's signing keyset (shared with the resource-server issuer)
465+
* @param issuerName the {@code iss} claim value (the resource-server issuer name)
466+
* @param emailSender the email transport
467+
* @param userLookup the user lookup SPI
468+
* @param clockProvider the clock
469+
* @return dependencies wired to a magic-link-scoped issuer + validator
470+
* @since 2.1.0
471+
*/
472+
public static Dependencies ofDedicatedAudience(
473+
JwtKeyset keyset,
474+
String issuerName,
475+
EmailSender emailSender,
476+
UserLookup userLookup,
477+
ClockProvider clockProvider) {
478+
Objects.requireNonNull(keyset, "keyset");
479+
Objects.requireNonNull(issuerName, "issuerName");
480+
JwtConfig audienceConfig = JwtConfig.defaults(issuerName, DEFAULT_AUDIENCE);
481+
// No-op AccessTokenStore on purpose: magic-link jtis must not be recorded as live access
482+
// tokens. Single-use is enforced separately via the ConsumedJtiStore.
483+
PkAuthJwtIssuer dedicatedIssuer = new PkAuthJwtIssuer(audienceConfig, keyset, clockProvider);
484+
PkAuthJwtValidator dedicatedValidator =
485+
new PkAuthJwtValidator(audienceConfig, keyset, clockProvider);
486+
return new Dependencies(
487+
dedicatedIssuer,
488+
dedicatedValidator,
489+
emailSender,
490+
userLookup,
491+
clockProvider,
492+
new InMemoryConsumedJtiStore(DEFAULT_CONSUMED_JTI_TTL),
493+
new DefaultMagicLinkFormatter());
494+
}
435495
}
436496

437497
/**

pk-auth-magic-link/src/test/java/com/codeheadsystems/pkauth/magiclink/MagicLinkServiceTest.java

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import com.codeheadsystems.pkauth.api.UserHandle;
88
import com.codeheadsystems.pkauth.jwt.JwtConfig;
99
import com.codeheadsystems.pkauth.jwt.JwtKeyset;
10+
import com.codeheadsystems.pkauth.jwt.JwtVerificationResult;
1011
import com.codeheadsystems.pkauth.jwt.PkAuthJwtIssuer;
1112
import com.codeheadsystems.pkauth.jwt.PkAuthJwtValidator;
1213
import com.codeheadsystems.pkauth.spi.ClockProvider;
@@ -140,6 +141,35 @@ void finishVerificationRejectsCrossPurposeTokenWithoutConsumingIt() {
140141
s -> assertThat(s.userHandle()).isEqualTo(user));
141142
}
142143

144+
@Test
145+
void magicLinkTokenIsRejectedByResourceServerValidatorButConsumedByItsOwnService() {
146+
byte[] secret = new byte[32];
147+
new SecureRandom().nextBytes(secret);
148+
JwtKeyset keyset = JwtKeyset.hs256(secret);
149+
ClockProvider clock = ClockProvider.fromClock(Clock.fixed(NOW, ZoneOffset.UTC));
150+
users.register("alice", "Alice", "alice@example.com");
151+
152+
// Service wired with a dedicated magic-link audience.
153+
MagicLinkService dedicated =
154+
MagicLinkService.create(
155+
MagicLinkService.Dependencies.ofDedicatedAudience(keyset, ISSUER, emails, users, clock),
156+
BASE_URL);
157+
String token =
158+
((MagicLinkService.SendResult.Sent) dedicated.startLogin("alice", "alice@example.com"))
159+
.tokenJti();
160+
161+
// A resource-server validator scoped to the application audience MUST reject the magic-link
162+
// token (wrong audience) — it cannot be replayed as an API bearer/access token.
163+
PkAuthJwtValidator resourceServer =
164+
new PkAuthJwtValidator(JwtConfig.defaults(ISSUER, AUDIENCE), keyset, clock);
165+
assertThat(resourceServer.validate(token))
166+
.isInstanceOf(JwtVerificationResult.WrongAudience.class);
167+
168+
// The magic-link service still validates and consumes its own token.
169+
assertThat(dedicated.finishVerification(token, MagicLinkService.PURPOSE_LOGIN))
170+
.isInstanceOf(MagicLinkService.ConsumeResult.Success.class);
171+
}
172+
143173
@Test
144174
void loginUnknownUserReturnsSentToPreventEnumeration() {
145175
// Privacy invariant: startLogin must return Sent even when the username does not exist,

pk-auth-micronaut/src/main/java/com/codeheadsystems/pkauth/micronaut/PkAuthFactory.java

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -249,13 +249,16 @@ SmsSender smsSender() {
249249

250250
@Singleton
251251
MagicLinkService magicLinkService(
252-
PkAuthJwtIssuer issuer,
253-
PkAuthJwtValidator validator,
252+
JwtConfig jwtConfig,
253+
JwtKeyset keyset,
254254
EmailSender emailSender,
255255
UserLookup userLookup,
256256
ClockProvider clock) {
257+
// Dedicated magic-link issuer/validator scoped to MagicLinkService.DEFAULT_AUDIENCE so the
258+
// resource-server validator (application audience) rejects magic-link tokens as bearer tokens.
257259
return MagicLinkService.create(
258-
MagicLinkService.Dependencies.of(issuer, validator, emailSender, userLookup, clock),
260+
MagicLinkService.Dependencies.ofDedicatedAudience(
261+
keyset, jwtConfig.issuer(), emailSender, userLookup, clock),
259262
"http://localhost:8080/auth/magic");
260263
}
261264

pk-auth-otp/src/main/java/com/codeheadsystems/pkauth/otp/OtpService.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,8 @@ public VerifyResult finishVerification(UserHandle user, String phoneE164, String
181181
Objects.requireNonNull(phoneE164, "phoneE164");
182182
Objects.requireNonNull(candidate, "candidate");
183183

184-
Optional<StoredOtp> activeOpt = repository.findLatestActive(user, phoneE164);
184+
Instant now = clockProvider.now();
185+
Optional<StoredOtp> activeOpt = repository.findLatestActive(user, phoneE164, now);
185186
if (activeOpt.isEmpty()) {
186187
// Run a throwaway HMAC so the no-active-OTP branch takes approximately the same wall-clock
187188
// 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
192193
return new VerifyResult.NoActiveOtp();
193194
}
194195
StoredOtp active = activeOpt.get();
195-
Instant now = clockProvider.now();
196196
if (now.isAfter(active.expiresAt())) {
197197
return new VerifyResult.Expired();
198198
}

pk-auth-otp/src/test/java/com/codeheadsystems/pkauth/otp/OtpServiceTest.java

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
import com.codeheadsystems.pkauth.api.UserHandle;
88
import com.codeheadsystems.pkauth.spi.ClockProvider;
9+
import com.codeheadsystems.pkauth.spi.OtpRepository;
910
import com.codeheadsystems.pkauth.testkit.InMemoryOtpRepository;
1011
import java.security.SecureRandom;
1112
import java.time.Clock;
@@ -14,6 +15,8 @@
1415
import java.time.ZoneOffset;
1516
import java.util.ArrayList;
1617
import java.util.List;
18+
import java.util.Optional;
19+
import java.util.OptionalInt;
1720
import org.junit.jupiter.api.BeforeEach;
1821
import org.junit.jupiter.api.Test;
1922

@@ -125,10 +128,14 @@ void sendRateLimits() {
125128
void expiredOtpIsRejected() {
126129
service.startVerification(USER, PHONE);
127130
String code = sms.lastCode();
131+
// Wrap the repository so it does NOT filter expiry — this exercises the service's own expiry
132+
// re-check (defense-in-depth). A conforming repository filters expired rows itself (verified by
133+
// the persistence integration tests); this asserts the service still rejects an expired row if
134+
// a host repository hands one back.
128135
OtpService advanced =
129136
OtpService.create(
130137
OtpService.Dependencies.of(
131-
repository,
138+
new NonFilteringOtpRepository(repository),
132139
sms,
133140
ClockProvider.fromClock(
134141
Clock.fixed(NOW.plus(Duration.ofMinutes(10)), ZoneOffset.UTC))),
@@ -143,6 +150,50 @@ void expiredOtpIsRejected() {
143150
.isInstanceOf(OtpService.VerifyResult.Expired.class);
144151
}
145152

153+
/**
154+
* {@link OtpRepository} decorator that ignores the {@code now} expiry filter (delegates with
155+
* {@link Instant#MIN}), modelling a host repository that does not filter expired rows in-store —
156+
* so the service-level expiry re-check is the thing under test.
157+
*/
158+
private static final class NonFilteringOtpRepository implements OtpRepository {
159+
private final OtpRepository delegate;
160+
161+
NonFilteringOtpRepository(OtpRepository delegate) {
162+
this.delegate = delegate;
163+
}
164+
165+
@Override
166+
public void save(StoredOtp otp) {
167+
delegate.save(otp);
168+
}
169+
170+
@Override
171+
public Optional<StoredOtp> findLatestActive(
172+
UserHandle userHandle, String phoneE164, Instant now) {
173+
return delegate.findLatestActive(userHandle, phoneE164, Instant.MIN);
174+
}
175+
176+
@Override
177+
public OptionalInt incrementAttempts(UserHandle userHandle, String otpId) {
178+
return delegate.incrementAttempts(userHandle, otpId);
179+
}
180+
181+
@Override
182+
public boolean consume(UserHandle userHandle, String otpId) {
183+
return delegate.consume(userHandle, otpId);
184+
}
185+
186+
@Override
187+
public int countSince(UserHandle userHandle, String phoneE164, Instant since) {
188+
return delegate.countSince(userHandle, phoneE164, since);
189+
}
190+
191+
@Override
192+
public int deleteByUserHandle(UserHandle userHandle) {
193+
return delegate.deleteByUserHandle(userHandle);
194+
}
195+
}
196+
146197
@Test
147198
void maskPhoneKeepsCountryPrefixAndLast4() {
148199
// Normal E.164 numbers: keep '+' + first country digit + '***' + last 4 digits.

pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbOtpRepository.java

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,8 @@ public void save(StoredOtp otp) {
4949
}
5050

5151
@Override
52-
public Optional<StoredOtp> findLatestActive(UserHandle userHandle, String phoneE164) {
52+
public Optional<StoredOtp> findLatestActive(
53+
UserHandle userHandle, String phoneE164, Instant now) {
5354
return DynamoDbSupport.wrap(
5455
"otp.findLatestActive",
5556
() -> {
@@ -65,6 +66,9 @@ public Optional<StoredOtp> findLatestActive(UserHandle userHandle, String phoneE
6566
.flatMap(page -> page.items().stream())
6667
.filter(i -> phoneE164.equals(i.getPhoneE164()))
6768
.filter(i -> !i.isConsumed())
69+
// Filter expiry against the caller-supplied instant (the host ClockProvider's "now"),
70+
// not the DynamoDB wall clock, so a single authoritative clock governs expiry.
71+
.filter(i -> DynamoDbSupport.parseInstant(i.getExpiresAt()).isAfter(now))
6872
// Compare parsed Instants, not the raw ISO strings: createdAt is stored via
6973
// Instant.toString(), which is variable-precision (it drops the fractional-seconds
7074
// field when zero), so "...:00Z" sorts after "...:00.000001Z" lexicographically and

pk-auth-persistence-dynamodb/src/test/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbAltFlowsIntegrationTest.java

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -92,8 +92,8 @@ void otpDeleteByUserHandleRemovesEveryRowForTheUser() {
9292

9393
int removed = otp.deleteByUserHandle(user);
9494
assertThat(removed).isEqualTo(2);
95-
assertThat(otp.findLatestActive(user, "+15551110000")).isEmpty();
96-
assertThat(otp.findLatestActive(other, "+15552220000")).isPresent();
95+
assertThat(otp.findLatestActive(user, "+15551110000", t0)).isEmpty();
96+
assertThat(otp.findLatestActive(other, "+15552220000", t0)).isPresent();
9797
}
9898

9999
@Test
@@ -115,17 +115,17 @@ void otpRoundTripAndCountSince() {
115115
t0.plusSeconds(60),
116116
t0.plusSeconds(360)));
117117

118-
var active = otp.findLatestActive(user, "+15551234567").orElseThrow();
118+
var active = otp.findLatestActive(user, "+15551234567", t0).orElseThrow();
119119
assertThat(active.otpId()).isEqualTo("o2");
120120

121121
otp.incrementAttempts(user, "o2");
122-
var refreshed = otp.findLatestActive(user, "+15551234567").orElseThrow();
122+
var refreshed = otp.findLatestActive(user, "+15551234567", t0).orElseThrow();
123123
assertThat(refreshed.attempts()).isEqualTo(1);
124124

125125
assertThat(otp.countSince(user, "+15551234567", t0.minusSeconds(10))).isEqualTo(2);
126126

127127
otp.consume(user, "o2");
128-
assertThat(otp.findLatestActive(user, "+15551234567"))
128+
assertThat(otp.findLatestActive(user, "+15551234567", t0))
129129
.hasValueSatisfying(o -> assertThat(o.otpId()).isEqualTo("o1"));
130130
}
131131

0 commit comments

Comments
 (0)