fix(a11y): TOTP modal validation and error handling#37049
fix(a11y): TOTP modal validation and error handling#37049dionisio-bot[bot] merged 7 commits intoRocketChat:developfrom
Conversation
|
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
WalkthroughShifts two-factor login flow to Promise-based handling, awaiting code submission. Updates invalid 2FA UI message key. Exposes and converts password+TOTP login to an exported Promise-returning function. Adjusts error propagation paths to resolve/reject Promises instead of relying solely on callbacks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant M as TwoFactor Modal
participant C as Client 2FA Flow (process2faReturn)
participant L as onCode Handler
participant A as loginWithPasswordAndTOTP
participant S as Server
U->>M: Enter TOTP code
M->>C: submit(code)
C->>L: await onCode(code, method)
rect rgba(230,240,255,0.5)
note right of L: Promise-based handling
L->>A: return A(user,password,code)
A->>S: Accounts.callLoginMethod
S-->>A: success or error
A-->>L: Promise resolved (success or error passthrough via callback)
end
alt Success
L-->>C: resolve
C-->>M: close / continue login
else Invalid code
L-->>C: reject (after toast i18n)
C-->>M: show "Invalid_two_factor_code"
else Max attempts reached
L-->>C: resolve (handled case)
C-->>M: proceed with handled state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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
🧹 Nitpick comments (6)
apps/meteor/client/meteor/overrides/login/password.ts (2)
23-29: Avoid parameter reassignment; normalize into a local variableReassigning function parameters harms readability and can confuse TS inference. Normalize into a local and use
includes:- if (typeof userDescriptor === 'string') { - if (userDescriptor.indexOf('@') === -1) { - userDescriptor = { username: userDescriptor }; - } else { - userDescriptor = { email: userDescriptor }; - } - } + const normalizedUser = + typeof userDescriptor === 'string' + ? userDescriptor.includes('@') + ? { email: userDescriptor } + : { username: userDescriptor } + : userDescriptor;And update the usage below:
- user: userDescriptor, + user: normalizedUser,
17-22: Exported API semantics changed — document or align behaviorNow that
loginWithPasswordAndTOTPis exported and Promise-returning, ensure downstream consumers know it rejects on errors (see suggested fix above) or document that it only signals via the callback. Silent resolution on error will cause hard-to-debug flows.apps/meteor/client/lib/2fa/process2faReturn.ts (2)
66-68: Type onCode to allow PromisesSince you now
await onCode(...), broaden the type to reflect Promise-returning handlers:- onCode: (code: string, method: string) => void; + onCode: (code: string, method: string) => void | Promise<void>;
84-92: Return the recursive call to preserve Promise chainAvoid a dangling Promise when you re-enter the flow:
- process2faReturn({ + return process2faReturn({ error: error as globalThis.Error | Meteor.Error | Meteor.TypedError | undefined, result, originalCallback, onCode, emailOrUsername, });apps/meteor/client/lib/2fa/overrideLoginMethod.ts (2)
16-16: Remove no-op return inside callback
return error;inside the callback is ignored by the caller and has no effect.- callback?.(error); - return error; + callback?.(error); + return;
86-89: Confirm intent: Swallowing invalid-code error in Promise flowHere, invalid-code shows a toast and calls
callback?.(undefined), which signals success to the consumer. Is this intended to only clear loading states without marking failure?If not, consider invoking
callback?.(error)to propagate failure, or re-enter the 2FA flow similarly to the callback-based path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/meteor/client/components/TwoFactorModal/TwoFactorTotpModal.tsx(1 hunks)apps/meteor/client/lib/2fa/overrideLoginMethod.ts(2 hunks)apps/meteor/client/lib/2fa/process2faReturn.ts(1 hunks)apps/meteor/client/meteor/overrides/login/password.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/client/meteor/overrides/login/password.ts (1)
apps/meteor/client/lib/2fa/overrideLoginMethod.ts (1)
LoginCallback(5-5)
apps/meteor/client/lib/2fa/overrideLoginMethod.ts (1)
apps/meteor/client/lib/toast.ts (1)
dispatchToastMessage(22-25)
🔇 Additional comments (3)
apps/meteor/client/lib/2fa/process2faReturn.ts (1)
83-83: Good: await onCode to drive modal lifecycleAwaiting
onCodeensures the modal closes and reopens correctly on failures.apps/meteor/client/lib/2fa/overrideLoginMethod.ts (1)
27-33: LGTM: Promise-wrapped TOTP path enables awaiting retriesWrapping the TOTP callback path in a Promise aligns with
process2faReturn’s awaited flow.apps/meteor/client/components/TwoFactorModal/TwoFactorTotpModal.tsx (1)
54-54: LGTM — i18n key verifiedThe translation key "Invalid_two_factor_code" exists in packages/i18n/src/locales/*.i18n.json and is already used elsewhere in the codebase; approve.
NightSkyHigh
changed the title
juliajforesti
force-pushed
the
fix/196-TOTP
branch
from
77b8553 to
77083dd
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37049 +/- ##
===========================================
- Coverage 70.27% 69.76% -0.51%
===========================================
Files 3284 3291 +7
Lines 117118 119102 +1984
Branches 20736 21456 +720
===========================================
+ Hits 82308 83095 +787
- Misses 31517 32699 +1182
- Partials 3293 3308 +15
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
juliajforesti
force-pushed
the
fix/196-TOTP
branch
from
77083dd to
4198dc7
Compare
juliajforesti
force-pushed
the
fix/196-TOTP
branch
from
4198dc7 to
b21b7f7
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 9 files
You’re at about 96% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/client/lib/2fa/overrideLoginMethod.ts">
<violation number="1" location="apps/meteor/client/lib/2fa/overrideLoginMethod.ts:44">
P2: Missing rejection handling on dynamic imports can leave `onCode` Promise pending indefinitely and stall 2FA modal completion.</violation>
</file>
<file name="apps/meteor/client/meteor/login/password.ts">
<violation number="1" location="apps/meteor/client/meteor/login/password.ts:52">
P2: Error handling mixes callback and Promise rejection, which can trigger unhandled rejections because the caller uses callback style and does not consume the returned Promise.</violation>
</file>
<file name="apps/meteor/client/lib/2fa/process2faReturn.ts">
<violation number="1" location="apps/meteor/client/lib/2fa/process2faReturn.ts:116">
P2: Truthiness check on `result` wrongly throws for valid falsy `onCode` return values.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
juliajforesti
changed the title
juliajforesti
changed the title
juliajforesti
force-pushed
the
fix/196-TOTP
branch
from
0115eef to
7571bef
Compare
dougfabris
changed the title
dougfabris
added
the
stat: QA assured
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
dougfabris
added
stat: QA assured
0d34fc1
6 participants
fix modal not closing when entering wrong code. fixed inline error to display correct text
WA-60
WA-33
Summary by CodeRabbit
Bug Fixes
Refactor