Support two-factor authentication at login#201
Merged
Conversation
koel 9.9.0 lets a login require a second factor: POST me now answers
{two_factor: true, login_token} instead of the token pair when the user
has 2FA enabled. AuthProvider.login returns a TwoFactorChallenge in that
case (null otherwise), and completeTwoFactorChallenge posts the login
token plus the code to me/two-factor-challenge for the token pair.
The login screen pushes a TwoFactorChallengeScreen that accepts a TOTP
code or a recovery code and finishes the login on success.
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds two-factor authentication login support, a challenge screen with code entry and verification, segmented one-time-code input, and provider and widget tests covering the new flow. ChangesTwo-Factor Authentication Flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LoginScreen
participant AuthProvider
participant Server
participant TwoFactorChallengeScreen
participant DataLoadingScreen
User->>LoginScreen: submit host/email/password
LoginScreen->>AuthProvider: login(host, email, password)
AuthProvider->>Server: POST me
Server-->>AuthProvider: response(two_factor, login_token)
alt two_factor required
AuthProvider-->>LoginScreen: TwoFactorChallenge(loginToken)
LoginScreen->>TwoFactorChallengeScreen: navigate(host, email, loginToken)
User->>TwoFactorChallengeScreen: enter code, submit
TwoFactorChallengeScreen->>AuthProvider: completeTwoFactorChallenge(loginToken, code)
AuthProvider->>Server: POST me/two-factor-challenge
Server-->>AuthProvider: success or 401
alt success
TwoFactorChallengeScreen->>AuthProvider: tryGetAuthUser()
TwoFactorChallengeScreen->>DataLoadingScreen: replace route
else invalid code
TwoFactorChallengeScreen->>User: show error dialog
end
else no 2FA
AuthProvider-->>LoginScreen: null
LoginScreen->>LoginScreen: continue authenticated flow
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![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 (1)
lib/ui/screens/two_factor_challenge.dart (1)
149-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
widgetlambda parameter to avoid shadowingState.widget.The
.expand((widget) => ...)parameter shadows theState.widgetgetter that provides access toTwoFactorChallengeScreen. If someone referenceswidget.hostinside this lambda they'd get a compile error instead of the screen's property. Rename tochildto encode intent and avoid confusion.As per coding guidelines: "rename variables to encode intent."
♻️ Proposed refactor
- ].expand((widget) => [widget, const SizedBox(height: 12)]), + ].expand((child) => [child, const SizedBox(height: 12)]),🤖 Prompt for 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. In `@lib/ui/screens/two_factor_challenge.dart` at line 149, Rename the `.expand((widget) => ...)` callback parameter in `TwoFactorChallengeScreen` to a non-conflicting name like `child` to avoid shadowing `State.widget` and make the intent clearer. Update the lambda used in the list of widgets so any future access to the screen’s `widget` getter remains unambiguous, and keep the surrounding widget-building logic unchanged.Source: Coding guidelines
🤖 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 `@lib/ui/screens/login.dart`:
- Around line 98-104: The login flow in attemptLogin and attemptLoginWithOtp
resumes after async auth calls and then uses context to navigate or show
dialogs, so add a mounted guard immediately after each await before calling
_gotoTwoFactorChallenge, _gotoHomeScreen, or any dialog-related helper. Use the
State methods in login.dart to locate the affected async flows and ensure no UI
actions run if the widget has been disposed.
In `@lib/ui/screens/two_factor_challenge.dart`:
- Around line 76-94: The async flow in two_factor_challenge.dart needs mounted
checks after await gaps to avoid using a disposed State or stale context. In the
method that wraps completeTwoFactorChallenge and tryGetAuthUser, guard the
finally block before calling setState(() => _verifying = false), and also check
mounted again before the post-success navigation and preference updates. Use the
surrounding _verifying, showErrorDialog, and Navigator.of(context,
rootNavigator: true).pushReplacementNamed(DataLoadingScreen.routeName) logic as
the fix points.
---
Nitpick comments:
In `@lib/ui/screens/two_factor_challenge.dart`:
- Line 149: Rename the `.expand((widget) => ...)` callback parameter in
`TwoFactorChallengeScreen` to a non-conflicting name like `child` to avoid
shadowing `State.widget` and make the intent clearer. Update the lambda used in
the list of widgets so any future access to the screen’s `widget` getter remains
unambiguous, and keep the surrounding widget-building logic unchanged.
🪄 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: a85f7738-3967-40fd-b4cb-7ca0467ed280
📒 Files selected for processing (7)
lib/providers/auth_provider.dartlib/ui/screens/login.dartlib/ui/screens/screens.dartlib/ui/screens/two_factor_challenge.darttest/providers/auth_provider_test.darttest/ui/screens/two_factor_challenge_test.darttest/ui/screens/two_factor_challenge_test.mocks.dart
Mirror koel's web flow: default to six auto-advancing digit boxes for the authenticator code (auto-submitting once full), with a toggle to a recovery-code text field and back. "Use a recovery code" and "Back to login" sit side by side as ghost buttons below Verify.
Revert the side-by-side row; "Use a recovery code" and "Back to login" are borderless text buttons stacked below Verify.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/ui/screens/two_factor_challenge.dart (1)
78-105: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd
mountedchecks after async gaps to prevent post-dispose crashes.This was previously flagged but remains unaddressed. After the
awaitcalls on lines 79 and 83, the catch blocks call_resetTotpInput()(which triggerssetState) andshowErrorDialog(which usescontext), thefinallyblock callssetState, and the post-finally code usescontextforNavigator— all withoutmountedchecks.The previous fix suggestion placed
if (!mounted) returnbeforeshowErrorDialog, but the current code also calls_resetTotpInput()(which callssetState) beforeshowErrorDialogin both catch blocks. Themountedguard must precede_resetTotpInput()as well.🔒 Proposed fix
try { await _auth.completeTwoFactorChallenge( loginToken: widget.loginToken, code: code, ); await _auth.tryGetAuthUser(); successful = true; } on HttpResponseException catch (error) { + if (!mounted) return; _resetTotpInput(); await showErrorDialog( context, message: error.response.statusCode == 401 ? 'Invalid authentication code.' : null, ); } catch (error) { + if (!mounted) return; _resetTotpInput(); await showErrorDialog(context); } finally { - setState(() => _verifying = false); + if (mounted) setState(() => _verifying = false); } - if (successful) { + if (successful && mounted) { preferences.host = widget.host; preferences.userEmail = widget.email; Navigator.of(context, rootNavigator: true) .pushReplacementNamed(DataLoadingScreen.routeName); }🤖 Prompt for 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. In `@lib/ui/screens/two_factor_challenge.dart` around lines 78 - 105, The async flow in the two-factor submit handler can still touch `setState` and `context` after the widget is disposed. In the `TwoFactorChallengeScreen` callback, add `mounted` guards immediately after each `await` boundary before calling `_resetTotpInput()`, `showErrorDialog()`, the `finally` block’s `setState`, and the post-success `Navigator.of(context, rootNavigator: true)` path. Use the existing `_resetTotpInput`, `showErrorDialog`, and the submit method in `two_factor_challenge.dart` to place the checks so no state or context access happens when the widget is unmounted.
🤖 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 `@lib/ui/screens/two_factor_challenge.dart`:
- Around line 108-113: _resetTotpInput() only clears the TOTP state, but
verify() calls it for both TOTP and recovery flows, so the recovery TextField
keeps stale text while _code is reset. Update the reset helper in
two_factor_challenge.dart to handle both modes by adding a TextEditingController
for the recovery input, clearing that controller alongside _code and
_totpResetToken, and renaming _resetTotpInput() to _resetInput() to reflect its
broader use. Also update the recovery TextField to use the new controller, and
change both verify() catch-block call sites to use the renamed reset method.
---
Duplicate comments:
In `@lib/ui/screens/two_factor_challenge.dart`:
- Around line 78-105: The async flow in the two-factor submit handler can still
touch `setState` and `context` after the widget is disposed. In the
`TwoFactorChallengeScreen` callback, add `mounted` guards immediately after each
`await` boundary before calling `_resetTotpInput()`, `showErrorDialog()`, the
`finally` block’s `setState`, and the post-success `Navigator.of(context,
rootNavigator: true)` path. Use the existing `_resetTotpInput`,
`showErrorDialog`, and the submit method in `two_factor_challenge.dart` to place
the checks so no state or context access happens when the widget is unmounted.
🪄 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: 98ee5126-2705-405d-966a-5c32bb289a5e
📒 Files selected for processing (5)
lib/ui/screens/two_factor_challenge.dartlib/ui/widgets/one_time_code_input.dartlib/ui/widgets/widgets.darttest/ui/screens/two_factor_challenge_test.darttest/ui/widgets/oops_box_test.mocks.dart
✅ Files skipped from review due to trivial changes (1)
- lib/ui/widgets/widgets.dart
Address review: add mounted checks after the awaited auth calls in the login and two-factor screens so a widget disposed mid-request doesn't setState or use a stale context. Also rename an `.expand` lambda param that shadowed State.widget.
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.
Integrates the two-factor authentication added in koel 9.9.0.
Server contract
POST me{email, password}now returns{two_factor: true, login_token}instead of the token pair when the user has 2FA enabled (401 still means bad credentials).POST me/two-factor-challenge{login_token, code}returns the{token, audio-token}pair.codeaccepts either a TOTP code or a recovery code.Changes
AuthProvider.loginreturns aTwoFactorChallenge(carrying thelogin_token) when the server asks for a second factor, ornullwhen the credentials alone completed the login. Token storage is centralised in a_storeCompositeTokenhelper reused by the OTP path too.AuthProvider.completeTwoFactorChallengeposts the login token + code and stores the resulting token pair.TwoFactorChallengeScreenwhen a challenge is returned.Challenge UI (mirrors koel's web flow)
OneTimeCodeInput) for the authenticator code, auto-submitting once all six are filled; supports paste/autofill distribution and backspace-to-previous.Tests
auth_provider_test.dart— normal login stores tokens and returns null; atwo_factorresponse returns the challenge and stores no token; the challenge posts the right body and stores tokens; a 401 propagates.two_factor_challenge_test.dart— six boxes by default; entering all six auto-submits and advances; toggling to recovery mode and back; a recovery code is upper-cased and submitted; an invalid code shows an error and stays put.Summary by CodeRabbit