Recovery code login flow UI - #151
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-151/ This page is automatically updated on each push to this PR. |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-151/ This page is automatically updated on each push to this PR. |
romanetar
changed the title
Close the remaining gaps in the recovery-code MFA login mode. - onBackToOtp / onUseRecovery clear recoveryCode and errors.recovery along with the mode switch, so an abandoned attempt is not re-shown when the user toggles back into recovery mode. - Recovery field drops autoComplete="one-time-code": that hint makes the OS offer the e-mailed OTP in the recovery field, which is the wrong credential and the OTP/recovery confusion risk called out in the ticket. Added a format hint and copy that distinguishes it from the e-mailed code. Tests: - New recovery-code-form.test.js: autocomplete, format hint, disabled states, inline error, submit, back vs cancel, raw value handed to parent. - login.mfa.test.js: mode switching + state cleanup, input normalization, empty/in-flight submits, success redirect, low-codes warning (and its already-dismissed variant), invalid/used code, mfa_session_expired, mfa_rate_limit. - E2E TS-005 fixed (it filled a 16-char code that can never exist) and now asserts the dash never reaches the endpoint; new TS-009 back-to-OTP, TS-010 invalid/used code, TS-011 recovery rate limit, TS-012 recovery session expiry. CI seeds mfa-ts-009..012 for them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The MFA e2e suite gives every TS-* test its own account because a real login burns that user's own OTP rate-limit window. TS-009..TS-012 were added with the seed loop widened only in pull_request_frontend_tests.yml, so "Front End Tests On Push" logged in as users that do not exist and the four new tests timed out waiting for the password step. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5c70578 to
415c3a6
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-151/ This page is automatically updated on each push to this PR. |
romanetar
changed the title
verifyRecoveryCode() posted through postRawRequest(), which copies every param onto the query string in addition to the body (base_actions.js:71). That writes the recovery code - a credential that completes a login on its own - into any access log along the path. #146 hit the same trap with current_password and added the body-only postRawRequestFull() for it; switch this call to it as well. TS-005 asserted the code off the query string, which encoded the leak as the expected contract. It now asserts the body carries the code and the query string does not, so the fix cannot silently regress. Also from review: - the "all 8 tests" comment in the MFA spec had gone stale at 12 tests; reworded so it does not track a count, and it now states the seed loop lives in BOTH workflow files (the divergence that broke push CI). - onBackToOtp()'s comment justified its reset with a clean-field-on-reentry guarantee that onUseRecovery() already provides; state the real reason, which is not holding an unspent credential in component state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-151/ This page is automatically updated on each push to this PR. |
ref https://app.clickup.com/t/86ba2zp4f
Context
The recovery-code login flow was already almost fully implemented by #126 (merged into #125's branch, commit
68a6c8dc). This PR closes the remaining gaps against the ticket's acceptance criteria and adds the missing test coverage.Changes
UI
onBackToOtp/onUseRecoverynow clearrecoveryCodeanderrors.recoveryalong with the mode switch. Previously, going back to OTP and re-entering recovery mode re-showed the half-typed code and its error.autoComplete="one-time-code". That hint makes the OS offer the e-mailed OTP in the wrong field — exactly the OTP/recovery confusion risk called out in the ticket. Added a format hint and copy that distinguishes it from the e-mailed code.Input normalization is unchanged: it still accepts both
XXXXXXXXandXXXX-XXXX, and displays the normalized value (no dash). The backend normalizes the same way inAbstractMFAChallengeStrategy.Tests
tests/js/login/components/recovery-code-form.test.js(8 cases).login.mfa.test.js(+13 cases): mode switching with state cleanup, input normalization, empty / in-flight submits, success →redirect_url, low-codes warning and its already-dismissed variant, invalid or used code,mfa_session_expired,mfa_rate_limit.mfa-ts-009..012.Verification
npx jest --testPathPattern=tests/js→ 66/66 across 12 suites.Review notes
helperTexthardcodes "8 characters", which comes fromauth.recovery_codes.length(default 8) and is not exposed to the front-end. If that config ever changes, the copy and theXXXX-XXXXformat drift apart.🤖 Generated with Claude Code