test(e2e): cover auth_flow TTL regression boundary - #124
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
🚅 Deployed to the ePDS-pr-124 environment in ePDS
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds end-to-end test steps and a feature scenario that simulate the OAuth auth_flow expiring (>60 minutes) by calling an internal expire hook, intercepting the ChangesOAuth Flow Expiry Test Coverage
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant InternalHook
participant Browser
participant Server
TestRunner->>InternalHook: POST /_internal/test/expire-auth-flow (backdate auth_flow)
InternalHook-->>TestRunner: { rowsUpdated: N }
TestRunner->>Browser: set up page.route("**/auth/ping")
Browser->>Server: auth/ping (request)
opt first ping after route set
Browser->>Browser: route.fetch() -> raw Response
Browser->>TestRunner: captured ping JSON (stored in pendingPingBody)
Browser-->>Server: fulfilled response with same raw body
end
Browser->>Server: navigation triggers auth/complete or auth/abort
Server-->>Browser: auth/abort (redirect)
TestRunner->>Browser: assert pendingPingBody.reason == "flow_expired" and auth/abort UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
73c5c4e to
ee1c28e
Compare
Coverage Report for CI Build 25394910354Coverage remained the same at 55.53%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
ee1c28e to
0421a95
Compare
There was a problem hiding this comment.
Pull request overview
Adds an additional e2e regression boundary for passwordless OAuth flows to ensure the auth_flow + epds_auth_flow cookie TTL (60 minutes) is enforced independently of OTP validity, asserting that /auth/complete reports an expired authentication session once the flow TTL elapses.
Changes:
- Added a new
@otp-expiryscenario asserting auth flow expiry after >60 minutes (even with a still-valid OTP). - Added Cucumber step definitions to expire the auth flow via an internal test hook, clear the flow cookie, and assert the
/auth/completeerror rendering.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| features/passwordless-authentication.feature | Adds a new auth_flow TTL regression scenario (but currently includes an unresolved merge conflict marker). |
| e2e/step-definitions/auth.steps.ts | Adds steps to expire the auth_flow + clear cookie, and a new assertion step for /auth/complete error content. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/step-definitions/auth.steps.ts`:
- Around line 491-500: The test fails because the expected string in the step
definition (the Then step using page.locator('p.error') in auth.steps.ts) does
not match the actual message rendered by the auth completion flow (complete.ts
-> "Your sign-in took too long to complete. Please start sign-in again."); fix
this by reconciling the two: either update the feature expectation to "Your
sign-in took too long to complete. Please start sign-in again." (so the Then
step continues to call expect(page.locator('p.error')).toContainText(expected))
or change the auth completion rendering in complete.ts (and any helper in
render-error.ts) to return "Authentication session expired" so the existing
feature text remains correct—pick one approach and apply the text change
consistently across the feature, auth.steps.ts expectation, and the
complete.ts/render-error.ts implementation.
In `@features/passwordless-authentication.feature`:
- Line 275: Remove the leftover Git conflict marker "<<<<<<< HEAD" from the
feature file (it appears as a raw line in the Gherkin text), ensure there are no
other conflict markers (like "=======" or ">>>>>>>") remaining, and keep the
existing content that follows intact so the Gherkin parser can parse the
feature; verify by running the Cucumber/Gherkin parser or test suite to confirm
the file loads cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b78b98b-80d8-46d6-bc58-4ebc36d3dbd2
📒 Files selected for processing (2)
e2e/step-definitions/auth.steps.tsfeatures/passwordless-authentication.feature
0421a95 to
f0a11ab
Compare
f0a11ab to
0831858
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0831858 to
650c2cb
Compare
Adds a second @otp-expiry scenario that backdates the auth_flow row via the existing /_internal/test/expire-auth-flow hook, then submits a still-valid OTP. After PR #154's reactive abort gate the OTP form pings /auth/ping before submitting; with the auth_flow row dead the ping reports `flow_expired`, the gate navigates to /auth/abort, and cleanExit serves its Tier-2 styled "Sign-in session expired" fallback page (the OAuth client redirect path needs the dead row's clientId, which is exactly what's missing here). The scenario asserts both signals — the ping reason (proving auth_flow specifically tripped, not PAR) and the abort fallback page — so a regression that, say, swaps which timer the gate honours would still be caught. Without this guardrail nothing in CI would notice if AUTH_FLOW_TTL_MS were quietly shortened back to the OTP TTL; the existing scenario only proves the 10-min-and-resend path works, not that the 60-min boundary is enforced. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
650c2cb to
c2ca73a
Compare
|
Summary
Stacks on PR #122. Adds a second
@otp-expiryscenario that asserts the auth_flow + cookie 60-minute TTL is actually enforced — backdates the auth_flow row + drops the cookie via the/_internal/test/expire-auth-flowhook (already shipped in #122), then submits a still-valid OTP./auth/completemust show "Authentication session expired" because the OAuth ticket has aged out independently of the OTP.Without this guardrail, nothing in CI would catch a future change quietly shortening
AUTH_FLOW_TTL_MSback to the OTP TTL — the existing scenario in #122 only proves the 10-min-and-resend path works, not that the 60-min boundary exists.Reuses every existing step except two new ones:
When more than 60 minutes pass before the user submits the OTP— backdates auth_flow + clears the cookie, leaves OTP untouched.Then the auth-complete page shows an "Authentication session expired" error— asserts on the rendered error page at/auth/complete.Test plan
pnpm format:checkcleanpnpm lintcleanpnpm typecheckcleanpnpm test— 812 unit tests pass (no new tests; only e2e changes)@otp-expiryscenarios pass (29 steps, 18.7s) against docker-compose stackmainonce test(e2e): cover OTP expiry without 10-minute wait #122 merges🤖 Generated with Claude Code
Summary by CodeRabbit