Validate docs/ADRs against code + fix DynamoDB refresh-token forensic-retention TTL#37
Merged
Conversation
…ity claims Validated README.md, docs/operator-guide.md, and docs/transactional-semantics.md against the code. Fixes for claims that did not match the implementation: - DynamoDB persistence uses TWO tables (PkAuthCore + PkAuthUsers per ADR 0008 and PkAuthDynamoTables), not a single physical table. The operator guide told operators to provision one table; corrected to provision both. - DynamoDB-native TTL attribute is `ttl` (epoch seconds), not `expiresAt`, and is set on Challenge / OneTimePasscode (and the 1.1.0 token rows) — there is no MagicLink item; magic-link tokens are never persisted. - OTP codes are hashed with HMAC-SHA256(pepper, code), not Argon2id; relabel the secrets-table row accordingly (backup codes do use Argon2id, with no pepper). - Operator-guide migration list now covers V8/V9/V10 (access_tokens, refresh_tokens, refresh_tokens amr column), not just V1-V7. - transactional-semantics: backup/OTP `consume` atomicity is the id-keyed mark-used step (the hash compare is a deliberately racy service-layer step); clarified which contracts have concurrency parity tests; fixed "Two surfaces" -> "Three surfaces". Verified-correct and left unchanged: JWT HS256/ES256 support, 32-byte secret hard-fail at boot, TokenTtlPolicy, AccessTokenStore, ceremony-finish ordering, rotateAtomically across all three backends, the concurrentRotationExactlyOneSucceedsFamilyRevoked parity test, UserDeletionService fan-out, and the admin-module wiring matrix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e TTL DynamoDbRefreshTokenRepository set the native `ttl` attribute to expiresAt.epochSecond, so DynamoDB's background sweep pruned used/revoked refresh rows at expiry — ~30 days earlier than the JDBI backend, which keeps them for the documented cleanupRetention window. RefreshTokenItem's own Javadoc already claimed ttl = expiresAt + cleanupRetention, so the code contradicted both the comment and JDBI parity. The `ttl` attribute was also doing double duty as the rotateAtomically freshness check (`#ttl > :nowEpoch`), so it could not simply be extended without treating expired tokens as fresh (a replay-defense regression). Split the two concerns: - New `expiresAtEpoch` attribute (= expiresAt.epochSecond) drives the freshness condition; `ttl` now carries expiresAt + cleanupRetention for native pruning. - deleteExpiredBefore filters on `expiresAtEpoch` to keep `expires_at < cutoff` semantics matching the JDBI cleanup SQL. - Add a backward-compatible constructor overload taking the retention Duration; the existing 2-arg constructor delegates with the documented 30-day default. Pre-release (1.1.0-SNAPSHOT), so no item-schema migration concern. Verified against DynamoDB Local: all 10 refresh-token integration tests pass, including the concurrent-rotation race and expiry tests, plus a new nativeTtlExtendsExpiryByCleanupRetention assertion. operator-guide.md updated to describe the per-type ttl semantics. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Validated all 16 ADRs against the current code. Decisions all still hold; fixed the claims that no longer match: - 0003: runtime deps are JDBI 3 + Postgres driver + HikariCP + slf4j-api, not "JDBI 3 + Postgres driver only". - 0004: PersistenceBindings is a class (not a record); module types are PkAuthCeremonyResource / PkAuthDropwizardAuthenticator (not Passkey*); PkAuthComponent now has six provision methods (added userDeletionService, refreshHandler); admin is now Dagger-wired via PkAuthFullComponent (PkAuthModule + AltFlowsModule) with PkAuthAdminResource, direct instantiation only for a host-supplied AdminService. - 0005: token revocation and refresh tokens, listed as deferred non-goals, shipped in 1.1.0 (AccessTokenStore / ADR 0015, pk-auth-refresh-tokens / ADR 0013); the stateless-JWT default still stands. Annotated and marked resolved. - 0008: "Three attribute definitions" -> "Four" (pk, sk, gsi1pk, gsi1sk). - 0009: Jackson databind 3.1.3 -> 3.1.4; annotations 2.21 -> 2.22. - 0010: Dropwizard 5.0.1 -> 5.0.2 (current catalog pin). - 0012: Micronaut 4.10.23 -> 4.10.25 (current catalog pin). - 0015: AccessTokenStore.delete(String) -> delete(UserHandle, String) (IDOR defense), in both the interface snippet and the logout example. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cosmetic line-wrapping only (no semantic change) so :check's spotlessJavaCheck passes. Follows up the TTL fix in 23da547. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validated the project docs and all 16 ADRs against the actual code, fixed the inaccuracies, and corrected one latent behavior bug uncovered during validation. Most of the diff is documentation; the only production code change is in the DynamoDB refresh-token repository.
./gradlew checkpasses (full Spotless lint + tests + JaCoCo coverage gates). A focused security review of the code change vs v1.3.1 found no security regression.Documentation fixes
PkAuthCore+PkAuthUsers), not one; native TTL attribute isttl(notexpiresAt) and there is no MagicLink item (magic links are never persisted); OTP hashing is HMAC-SHA256(pepper, code), not Argon2id; migration list extended through V10; backup/OTPconsumeatomicity wording corrected (id-keyed mark-used; the hash compare is a deliberately racy service-layer step).PkAuthFullComponent); 0005 revocation + refresh tokens shipped in 1.1.0 (cross-ref 0013/0015); 0008 "Three"→"Four" attribute definitions; 0009/0010/0012 version drift; 0015delete(UserHandle, String)signature.Code fix — DynamoDB refresh-token forensic-retention TTL
DynamoDbRefreshTokenRepositoryset the nativettlto plainexpiresAt, so DynamoDB's background sweep pruned used/revoked refresh rows at expiry — ~30 days earlier than the JDBI backend and contradicting bothRefreshTokenItem's Javadoc and the documentedcleanupRetentionwindow.ttlwas also doing double duty as therotateAtomicallyfreshness/replay check, so it couldn't simply be extended without risking acceptance of expired tokens. The fix separates the two concerns:expiresAtEpochattribute (=expiresAt.epochSecond, the value the oldttlcarried) drives the freshness/expiry check — semantics unchanged.ttlnow carriesexpiresAt + cleanupRetentionfor native pruning only, restoring JDBI parity and making the Javadoc true.deleteExpiredBeforefilters onexpiresAtEpochto keepexpires_at < cutoffsemantics.Duration; the existing 2-arg constructor delegates with the documented 30-day default.Pre-existing rows lacking
expiresAtEpochfail the freshness check (fail-closed → family scorch → re-auth); no migration concern beyond a one-time re-auth window.Testing
nativeTtlExtendsExpiryByCleanupRetentionassertion../gradlew checkgreen across all modules.Security review
Reviewed the code diff vs v1.3.1. The expiry/replay guarantees are preserved exactly (freshness check compares the same value it always did); the only behavioral change is that native TTL prunes 30 days later, which keeps used/revoked rows around longer — fail-safe for replay detection and consistent with the JDBI backend. No findings.
🤖 Generated with Claude Code