[COR-343] auth-go refresh tier: e2e harness (mock OAuth server + cross-process tests)#12
Open
khaong wants to merge 12 commits into
Open
[COR-343] auth-go refresh tier: e2e harness (mock OAuth server + cross-process tests)#12khaong wants to merge 12 commits into
khaong wants to merge 12 commits into
Conversation
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Test-only helper: produces three-segment JWTs (EdDSA header, JSON payload, invalid sig) that round-trip through tokens.ParseClaims. First slice of the e2e harness; family/Server build on it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Refresh-token family state machine for the e2e mock OAuth server. Implements RFC 9700 rotation with reuse detection: an active RT rotates to a successor; a replay within the idempotency window returns the already-issued successor; a replay outside the window (or any consume on a revoked family) revokes and returns invalid-grant-shaped output. Goroutine-safe under concurrent Consume; a contention test asserts exactly one rotation wins under N parallel callers. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… (COR-343) Compose the Registry + MintUnsignedJWT into an httptest-backed mock authorization server. Routes /oauth/token by grant_type (refresh_token, token-exchange, device_code) and /oauth/device_authorization. Helpers: SeedFamily for bypassing device-flow login in test setup, ApproveDeviceCode for driving the device flow, ForceNextRefresh for one-shot failure injection (FailInvalidGrant landed; FailNetworkError deferred to Task 4). Per-category grant counters for assertion. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Honours ForceNextRefresh(FailNetworkError) by hijacking the TCP connection in the refresh handler and closing it before writing any response. Client surfaces a transport error from http.Client.Do, which lets e2e tests validate that tokenmanager.doRefresh does not misclassify network failures as ErrReauthRequired. The override is still one-shot: the next un-forced refresh succeeds with the same RT (the forced failure does not consume it). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
In-process composed-flow tests using the real internal/testoauth mock server: refresh-on-expiry + exchange composition, two-cycle silent refresh, goroutine coalescing under real HTTP, rotation reuse-detection revoking the family then surfacing as ErrReauthRequired, idempotency window absorbing a replay, network-failure-not-misclassified-as-reauth. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…R-343) One-shot stall on the next refresh_token request, released by the returned func. Cross-process tests use this to hold a refresh in-flight in subprocess A while subprocess B attempts a concurrent mutation, then release the stall and assert the serialisation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Standard Go subprocess-test pattern: TestMain dispatches helper modes when AUTHGO_E2E_HELPER is set, parent tests spawn the binary via exec.Command(os.Args[0], -test.run=...) with env-passed config. Four scenarios: - Cross-process single-flight: two refresh subprocesses against a shared file-backed store + lockDir + server → exactly one server grant; both processes return the same access token. - Logout wins: subprocess A holds a stalled refresh; B's DeleteCoreToken blocks on the lock, then deletes; final store empty. - Re-login wins: same shape with SaveCoreToken; final store has the new identity. - Proclock mutual exclusion (no OAuth): two subprocesses contend on the same lock with a shared counter file; the counter ends at the expected total, proving the lock held under cross-process load. Subprocess tests are gated by testing.Short(). A file-backed tokenstore (fileStore, sha256-keyed filenames) enables cross-process token-state sharing for the single-flight coalescing test. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Remove dead Config.RefreshTokenTTL (never wired; wire expires_in correctly uses loginJWTTTL per RFC 6749 §5.1). - Claims.Extra: typed fields now win over Extra on key collision, matching sts.buildForm convention. Doc updated. - GrantCount godoc now accurately describes what is counted (handler invocations, including hijacked failures). - ApproveDeviceCode panics on unknown device_code (was a silent no-op that produced a 600s hang on the polling side). - TestServer_DeviceFlowApprovedYieldsTokens now round-trips the minted access token through tokens.ParseClaims — closes the one-grant-type gap. - Spec doc synced with implementation: removed RefreshTokenTTL + Issuer fields from Config, added Subject/Audience to SeededLogin, corrected test names, dropped never-implemented refresh-blocking helper mode, fixed locking-model description and spawnHelper signature. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Logout/relogin subprocess tests now prove lock-serialisation rigorously: subprocess-side elapsed_ms (immune to parent-side startup jitter) plus srv.RefreshGrantCount()==1 after both finish (proves the refresh actually completed before the mutation acquired the lock). Removes the previous parent-side timing measurement that was a tautology (always passed because it included the parent's own pre-release sleep). Pre-release sleep bumped 200ms→300ms to give subprocess startup headroom so elapsed_ms reliably clears the 200ms threshold. - fileStore.SaveTokens rejects empty AccessToken per the Store contract. - TestE2E_RefreshOnExpiredJWTThenExchange now asserts the returned token has aud == [resource], catching a mock-bug that returned a wrong-aud token. - readCounter and lineCaptureWriter no longer silently swallow errors / residual bytes; failures surface via the helperResult protocol or t.Log. - Replace bytesIndexByte with bytes.IndexByte (already imported). - Remove dead SetNowForTest call + dead AUTHGO_E2E_SERVER env var. - os.IsNotExist → errors.Is(err, fs.ErrNotExist) (Go 1.16+ idiom). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The subprocess Manager runs in a separate process and uses real time.Now(); pinning the server clock to a fixed UTC date while the Manager doesn't created a time-of-day-dependent flake. Once real wall time passed the pinned-exp (server.now + LoginJWTTTL), the second subprocess in TestE2ESub_CrossProcessSingleFlight saw the persisted JWT as already expired and refreshed again, breaking the single-flight assertion (RefreshGrantCount would be 2 instead of 1). Same hazard affected the logout/relogin stall tests. Use time.Now() on the server so the refreshed JWT's exp tracks real wall time. mintExpiredJWT's exp is hard-coded to year 2000 so the seeded "expired" JWT stays expired regardless of clock pinning. Addresses Cursor Bugbot finding on PR #12. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5bb057c. Configure here.
The 75ms threshold on r.took was racing the parent's 75ms wall-clock timer. The goroutine's time.Since(start) is measured from AFTER the goroutine is scheduled, several ms after the parent kicks off its time.After(75ms), so r.took is consistently ~5-10ms below 75ms — the test passed 50/50 iterations but failed 8/200 (~4% flake) in a high- iteration sweep. Lower the threshold to 50ms (still meaningful as a sanity check that the stall engaged) and document why the offset exists. The first select case above already proves "request did not return before release"; this assertion is just a sanity guard against the stall mechanism failing entirely. Verified stable under go test -race -count=200 (17s, no failures). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit da0ccbc. Configure here.
5 tasks
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
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
Closes the test gaps that COR-314's unit/component tests can't, against the v0.4.0 refresh tier. Pure test infrastructure — no production code is modified. 3,247 insertions across 9 files in 10 commits.
What lands
internal/testoauth/— a tiny RFC-conformant mock OAuth server:MintUnsignedJWT(alg: EdDSA, deliberately invalid sig) round-trips throughtokens.ParseClaims.Registry+Family— refresh-token state machine implementing RFC 6749 §6 rotation with RFC 9700 reuse-detection and an optional idempotent-successor window. Concurrent-rotation test pins single-rotation-wins under contention.Server(httptest-backed) routes/oauth/tokenby grant_type:refresh_token,urn:…:token-exchange,urn:…:device_code, plus/oauth/device_authorization. Per-grant counters andFamilyRevoked()for assertions.ForceNextRefresh(FailInvalidGrant|FailNetworkError)(the latter via TCP connection hijack),StallNextRefresh()for holding refreshes in-flight across processes.tokenmanager/e2e_test.go— 6 in-process composed-flow tests against the real mock: refresh-on-expiry + exchange composition, two-cycle silent refresh, goroutine coalescing under real HTTP, rotation reuse-detection revoking the family and surfacing asErrReauthRequired, idempotency window absorbing replay, network-failure-not-misclassified-as-reauth (validates the bug Codex's adversarial review caught on PR [COR-314] auth-go refresh tier: persist refresh + refresh_token grant + single-flight #10).tokenmanager/e2e_subprocess_test.go— 4 cross-process tests via the standard Goos.Args[0]self-re-exec pattern:TestE2ESub_CrossProcessSingleFlight— two subprocesses + sharedfileStore+ same LockDir → server sees exactly 1 refresh grant; both processes return the same access token (proves cross-process token sharing via the lock + persist).TestE2ESub_LogoutWinsOverInFlightRefresh/_ReloginWinsOverInFlightRefresh— subprocess A holds a stalled refresh, subprocess B'sDeleteCoreToken/SaveCoreTokenblocks on the cross-process flock, then runs. Asserts subprocess-sideelapsed_ms >= stall_windowANDRefreshGrantCount == 1after both complete — rigorously proves serialisation rather than passing on a lucky write-race. This validates the fix Codex's adversarial review flagged on PR [COR-314] auth-go refresh tier: persist refresh + refresh_token grant + single-flight #10 for the first time at the OS-process layer.TestE2ESub_ProclockMutualExclusion— pure proclock, two subprocesses, shared counter file with deliberate race window; counter must end at exactly2 × rounds.testing.Short().Process
Standard subagent-driven dev (7 implementer tasks + multi-agent PR review). The PR-review-toolkit found 5 substantive items worth fixing pre-merge — all closed in commits
6d35709+7ef645d. Highlights of what review caught:helperResult.ElapsedMs+RefreshGrantCount == 1.Claims.Extraordering was inverse ofsts.buildForm's convention — flipped to match (typed fields win on collision).Config.RefreshTokenTTLwas documented but unused — removed.fileStore.SaveTokensnow honours theStorecontract's empty-token rejection.Verification
mise run check— gofmt clean, golangci-lint 0 issues, govulncheck clean, all tests pass with-racego test ./... -race -count=3— no flakiness over 3 runsGOOS=windows go build ./...— cleango test -short— subprocess tests correctly skippedinternal/testoauth83.7%,tokenmanager92.9% (up from 92.5% post-v0.4.0)Non-goals
🤖 Generated with Claude Code
Note
Low Risk
Changes are limited to internal test code and documentation; production credential handling is unchanged, though the tests exercise sensitive refresh and lock behavior.
Overview
Adds test-only end-to-end coverage for the refresh tier (COR-343): no production
tokenmanageror auth paths are modified.A new
internal/testoauthpackage provides anhttptestmock OAuth server with refresh rotation, RFC 9700-style family revocation on replay, optional idempotent-successor window, token exchange, device flow, and hooks (ForceNextRefresh,StallNextRefresh, grant counters).tokenmanager/e2e_test.goruns six in-process scenarios (refresh + exchange, two refresh cycles, goroutine single-flight, reuse →ErrReauthRequiredwithout deleting creds, idempotency, network error ≠ reauth).tokenmanager/e2e_subprocess_test.gore-execs the test binary viaAUTHGO_E2E_HELPERfor cross-process single-flight, logout/relogin vs stalled refresh (with subprocesselapsed_msassertions), andproclockmutual exclusion; subprocess tests skip under-short. An approved design spec documents the harness.Reviewed by Cursor Bugbot for commit da0ccbc. Configure here.