fix(auth): wallet password validation in creation and update flows#2620
fix(auth): wallet password validation in creation and update flows#2620
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 You can disable this status message by setting the WalkthroughThis update introduces a detailed password validation system with granular error messaging and Unicode-aware rules, adds a user-controllable setting to allow weak passwords, and integrates this setting throughout authentication, settings, and wallet creation flows. The UI is updated to let users toggle weak password allowance, and comprehensive tests are added for the new validation logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsPage
participant SettingsBloc
participant SettingsRepository
User->>SettingsPage: Toggle "Allow weak password"
SettingsPage->>SettingsBloc: Dispatch WeakPasswordsAllowedChanged
SettingsBloc->>SettingsRepository: Update weakPasswordsAllowed
SettingsBloc->>SettingsPage: Emit updated state
sequenceDiagram
participant User
participant PasswordField
participant SettingsBloc
User->>PasswordField: Enter password
PasswordField->>SettingsBloc: Read weakPasswordsAllowed
alt weakPasswordsAllowed = true
PasswordField->>PasswordField: Use legacy validator
else weakPasswordsAllowed = false
PasswordField->>PasswordField: Use strict validator
end
PasswordField->>User: Show error or accept password
Suggested labels
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Visit the preview URL for this PR (updated for commit ea63fd7): https://walletrc--pull-2620-merge-6lbch9db.web.app (expires Thu, 29 May 2025 12:32:58 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes wallet password validation in both creation and update flows by updating password requirements and integrating a weak password toggle. Key changes include:
- Updating password validation functions to distinguish between legacy and new validations based on settings.
- Adding UI components and settings management for the weak password option.
- Adjusting the authentication flow to pass the weak password setting when authenticating.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test_units/main.dart | Updated test call for password validation to invoke the new testing function. |
| lib/views/wallets_manager/widgets/creation_password_fields.dart | Modified password validator to conditionally use legacy validation based on the weak password setting. |
| lib/views/settings/widgets/security_settings/password_update_page.dart | Updated password validation in the password update flow using the new setting. |
| lib/views/settings/widgets/general_settings/settings_manage_weak_passwords.dart | Introduced UI for managing the weak password toggle. |
| lib/views/settings/widgets/general_settings/general_settings.dart | Added weak password management section in general settings. |
| lib/shared/utils/validators.dart | Extended and updated password validation logic and error messaging. |
| lib/model/stored_settings.dart | Augmented the settings model to include the weakPasswordsAllowed field. |
| lib/main.dart | Modified AuthBloc instantiation to pass a SettingsRepository instance. |
| lib/bloc/settings/settings_state.dart, settings_event.dart, settings_bloc.dart | Incorporated the weakPasswordsAllowed field into settings state management. |
| lib/bloc/auth_bloc/auth_bloc.dart | Updated authentication flows to retrieve and use the weak password setting during sign-in, registration, and restore flows. |
Comments suppressed due to low confidence (1)
test_units/main.dart:52
- [nitpick] Consider renaming the test function to 'testValidatePasswordRequirements' for consistency with existing naming conventions and clarity on its purpose.
testcheckPasswordRequirements();
| return MultiBlocProvider( | ||
| providers: [ | ||
| BlocProvider<AuthBloc>( | ||
| create: (_) => AuthBloc(komodoDefiSdk, walletsRepository), | ||
| create: (_) => | ||
| AuthBloc(komodoDefiSdk, walletsRepository, SettingsRepository()), |
There was a problem hiding this comment.
Rather than instantiating SettingsRepository inline, consider injecting it via dependency injection to maintain consistency across the codebase.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
lib/bloc/auth_bloc/auth_bloc.dart (2)
151-152: Same SDK parameter issue in _onRegister methodThe
allowWeakPasswordparameter is also used here but doesn't exist in the current SDK version.Also applies to: 160-160
🧰 Tools
🪛 GitHub Actions: takenagain is running unit tests on PR 🚀
[error] Flutter build failed with exit code 1 due to compilation errors related to invalid named parameters.
🪛 GitHub Actions: takenagain is deploying a preview build to Firebase Hosting 🚀
[error] Flutter build failed due to compilation errors related to invalid constructor parameters. Command exited with status 1.
199-200: Same SDK parameter issue in _onRestore methodThe
allowWeakPasswordparameter is also used here but doesn't exist in the current SDK version.Also applies to: 209-209
🧰 Tools
🪛 GitHub Actions: takenagain is running unit tests on PR 🚀
[error] Flutter build failed with exit code 1 due to compilation errors related to invalid named parameters.
🪛 GitHub Actions: takenagain is deploying a preview build to Firebase Hosting 🚀
[error] Flutter build failed due to compilation errors related to invalid constructor parameters. Command exited with status 1.
🧹 Nitpick comments (5)
test_units/main.dart (1)
52-52: Fix function name casing inconsistencyThe function name
testcheckPasswordRequirements()uses inconsistent camel case. It should betestCheckPasswordRequirements()with a capital 'C' in "Check" to follow proper camelCase naming convention.- testcheckPasswordRequirements(); + testCheckPasswordRequirements();lib/shared/utils/validators.dart (2)
71-75: Simplify “contains password” check & avoid redundant flagsYou already lowercase the string, so the
caseSensitive: falseflag is no longer needed.
Moreover, usingRegExp('password')provides no advantage over a plaincontains('password')and incurs unnecessary regex compilation on every call.- if (password - .toLowerCase() - .contains(RegExp('password', caseSensitive: false, unicode: true))) { + if (password.toLowerCase().contains('password')) { return PasswordValidationError.containsPassword; }
98-104: Micro-optimisation: avoid full list allocationConverting the whole string to
charactersListallocates an intermediate list.
A simple sliding counter while iteratingCharactersdirectly avoids this extra memory:- final charactersList = password.characters.toList(); - for (int i = 0; i < charactersList.length - 2; i++) { - if (charactersList[i] == charactersList[i + 1] && - charactersList[i] == charactersList[i + 2]) { + final it = password.characters.iterator; + String? prevPrev, prev; + while (it.moveNext()) { + final curr = it.current; + if (curr == prev && curr == prevPrev) { return PasswordValidationError.consecutiveCharacters; } + prevPrev = prev; + prev = curr; }This keeps the check O(n) but eliminates the extra list.
test_units/tests/password/validate_password_test.dart (2)
83-100: Coupling tests to internal error-priority orderThe “multiple validation errors” assertions assume that
checkPasswordRequirementswill always report containsPassword before consecutiveCharacters, etc.
If the implementation order changes, perfectly valid behaviour might break tests.Instead of relying on priority, assert that the returned error is one of the expected ones, or split inputs so that they trigger a single rule each.
468-507: Fuzzy testing currently provides no assertionsThe random-password loop only calls
checkPasswordRequirementswithout asserting anything.
If the function accidentally throws or returns an unexpected enum in the future, the loop will still pass.Add at least a sanity assertion, e.g. “result is not null” (it never is) or “does not throw” using the
expectLatersyntax.expect(() => checkPasswordRequirements(passwordBuffer.toString()), returnsNormally);If the goal is crash-detection only, a comment explaining that intent would help future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
lib/generated/codegen_loader.g.dartis excluded by!**/generated/**
📒 Files selected for processing (14)
assets/translations/en.json(2 hunks)lib/bloc/auth_bloc/auth_bloc.dart(8 hunks)lib/bloc/settings/settings_bloc.dart(2 hunks)lib/bloc/settings/settings_event.dart(1 hunks)lib/bloc/settings/settings_state.dart(1 hunks)lib/main.dart(1 hunks)lib/model/stored_settings.dart(4 hunks)lib/shared/utils/validators.dart(1 hunks)lib/views/settings/widgets/general_settings/general_settings.dart(2 hunks)lib/views/settings/widgets/general_settings/settings_manage_weak_passwords.dart(1 hunks)lib/views/settings/widgets/security_settings/password_update_page.dart(2 hunks)lib/views/wallets_manager/widgets/creation_password_fields.dart(2 hunks)test_units/main.dart(1 hunks)test_units/tests/password/validate_password_test.dart(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Building mobile apps 📱
lib/bloc/auth_bloc/auth_bloc.dart
[error] 90-90: Error: No named parameter with the name 'allowWeakPassword'. This parameter is used but not defined in the constructor of AuthOptions.
[error] 160-160: Error: No named parameter with the name 'allowWeakPassword'. This parameter is used but not defined in the constructor of AuthOptions.
[error] 209-209: Error: No named parameter with the name 'allowWeakPassword'. This parameter is used but not defined in the constructor of AuthOptions.
🪛 GitHub Actions: Building desktop apps 🖥️
lib/bloc/auth_bloc/auth_bloc.dart
[error] 90-90: No named parameter with the name 'allowWeakPassword'.
[error] 160-160: No named parameter with the name 'allowWeakPassword'.
[error] 209-209: No named parameter with the name 'allowWeakPassword'.
🪛 GitHub Actions: takenagain is running UI tests on PR 🚀
lib/bloc/auth_bloc/auth_bloc.dart
[error] 90-90: No named parameter with the name 'allowWeakPassword'. This parameter is used but not defined in the constructor of AuthOptions.
[error] 160-160: No named parameter with the name 'allowWeakPassword'. This parameter is used but not defined in the constructor of AuthOptions.
[error] 209-209: No named parameter with the name 'allowWeakPassword'. This parameter is used but not defined in the constructor of AuthOptions.
🪛 GitHub Actions: takenagain is running unit tests on PR 🚀
lib/bloc/auth_bloc/auth_bloc.dart
[error] 90-90: Error: No named parameter with the name 'allowWeakPassword'. This parameter is used but not defined in the constructor of AuthOptions.
[error] 160-160: Error: No named parameter with the name 'allowWeakPassword'. This parameter is used but not defined in the constructor of AuthOptions.
[error] 209-209: Error: No named parameter with the name 'allowWeakPassword'. This parameter is used but not defined in the constructor of AuthOptions.
[error] Flutter build failed with exit code 1 due to compilation errors related to invalid named parameters.
🪛 GitHub Actions: takenagain is validating code guidelines 🚀
lib/bloc/auth_bloc/auth_bloc.dart
[error] 90-90: Error: No named parameter with the name 'allowWeakPassword'. This parameter is used but not defined in the constructor of AuthOptions.
[error] 160-160: Error: No named parameter with the name 'allowWeakPassword'. This parameter is used but not defined in the constructor of AuthOptions.
[error] 209-209: Error: No named parameter with the name 'allowWeakPassword'. This parameter is used but not defined in the constructor of AuthOptions.
🪛 GitHub Actions: takenagain is deploying a preview build to Firebase Hosting 🚀
lib/bloc/auth_bloc/auth_bloc.dart
[error] 90-90: No named parameter with the name 'allowWeakPassword'. This parameter is used but not defined in the AuthOptions constructor.
[error] 160-160: No named parameter with the name 'allowWeakPassword'. This parameter is used but not defined in the AuthOptions constructor.
[error] 209-209: No named parameter with the name 'allowWeakPassword'. This parameter is used but not defined in the AuthOptions constructor.
[error] Flutter build failed due to compilation errors related to invalid constructor parameters. Command exited with status 1.
🔇 Additional comments (21)
lib/views/settings/widgets/general_settings/general_settings.dart (1)
11-11: LGTM!The addition of the
SettingsManageWeakPasswordswidget is properly integrated into the general settings UI with appropriate spacing. This will allow users to toggle whether weak passwords are allowed in the application.Also applies to: 32-33
lib/bloc/settings/settings_event.dart (1)
31-37: LGTM!The
WeakPasswordsAllowedChangedevent class is well-structured with appropriate constructor parameters and property overrides. This event will be dispatched when the user toggles the weak password allowance setting.lib/main.dart (1)
154-155:Details
✅ Verification successful
Verify settings repository behavior with stateless instantiation
Creating a new
SettingsRepositoryinstance directly in the provider might cause issues if the repository relies on initialization or requires state persistence. Consider using a singleton or repository instance from a provider.
🏁 Script executed:
#!/bin/bash # Check how SettingsRepository is used elsewhere in the codebase # and whether it's typically instantiated directly or provided # Look for other SettingsRepository instantiations echo "Looking for SettingsRepository instantiations:" rg "SettingsRepository\(\)" --type dart # Check if SettingsRepository has initialization methods echo -e "\nChecking for init methods in SettingsRepository:" rg "class SettingsRepository" -A 20 --type dartLength of output: 2529
No changes needed for SettingsRepository instantiation
After reviewing
SettingsRepositoryinlib/bloc/settings/settings_repository.dart, it’s a lightweight, stateless wrapper aroundBaseStorage(viagetStorage()) with no in-memory caching or async initialization. Every call toloadSettingsorupdateSettingsreads from or writes to the same underlying persistent store, so creating multiple instances has no adverse effects.• The class holds no internal state beyond its storage reference.
•getStorage()returns the underlying persistence layer, ensuring shared data across instances.
• Other parts of the app already instantiateSettingsRepository()inline without issues.No changes are required here; keep the existing direct instantiation.
lib/bloc/settings/settings_bloc.dart (2)
21-21: New event handler registered properlyGood addition of the event handler registration in the constructor, following the same pattern as other event handlers.
61-69: Event handler implementation is consistent with existing patternsThe implementation of
_onWeakPasswordsAllowedChangedfollows the same pattern as other event handlers in the file, properly updating the repository and emitting a new state.lib/views/settings/widgets/security_settings/password_update_page.dart (4)
12-12: Added necessary import for SettingsBlocGood addition of the SettingsBloc import to access the weak password setting.
282-282: Improved type safety with more specific function typeChanged from
Function(bool)tovoid Function(bool)for better type safety.
290-290: Password validation now uses settings-based conditional logicGood refactoring to use a private method that accesses the settings state.
297-310: Proper implementation of conditional password validationThe validation logic correctly checks the settings state and applies either the legacy validator or the new stricter validator based on user preferences.
lib/bloc/auth_bloc/auth_bloc.dart (3)
9-9: Added necessary import for SettingsRepositoryGood addition of the SettingsRepository import to access the weak password setting.
🧰 Tools
🪛 GitHub Actions: takenagain is running unit tests on PR 🚀
[error] Flutter build failed with exit code 1 due to compilation errors related to invalid named parameters.
🪛 GitHub Actions: takenagain is deploying a preview build to Firebase Hosting 🚀
[error] Flutter build failed due to compilation errors related to invalid constructor parameters. Command exited with status 1.
23-23: Constructor updated with new dependencyThe constructor properly adds the SettingsRepository as a dependency and stores it in a final field.
Also applies to: 37-37
🧰 Tools
🪛 GitHub Actions: takenagain is running unit tests on PR 🚀
[error] Flutter build failed with exit code 1 due to compilation errors related to invalid named parameters.
🪛 GitHub Actions: takenagain is deploying a preview build to Firebase Hosting 🚀
[error] Flutter build failed due to compilation errors related to invalid constructor parameters. Command exited with status 1.
47-50: Helper method to retrieve weak password settingGood implementation of a helper method to abstract the retrieval of the weak password setting.
🧰 Tools
🪛 GitHub Actions: takenagain is running unit tests on PR 🚀
[error] Flutter build failed with exit code 1 due to compilation errors related to invalid named parameters.
🪛 GitHub Actions: takenagain is deploying a preview build to Firebase Hosting 🚀
[error] Flutter build failed due to compilation errors related to invalid constructor parameters. Command exited with status 1.
assets/translations/en.json (3)
129-129: Comprehensive password requirements in error messageThe updated error message clearly explains all the password requirements, which is helpful for users.
369-375: Granular error messages for each password requirementGood addition of specific error messages for each password validation rule, allowing for more precise user feedback.
376-378: Clear labeling for the weak password settingThe added translations provide clear labels for the weak password setting UI elements, including a description that explains its purpose is for debugging only.
lib/views/wallets_manager/widgets/creation_password_fields.dart (1)
96-108: Secure implementation of conditional password validationThe changes correctly integrate the user's password policy preference by conditionally applying different validation logic based on the
weakPasswordsAllowedsetting. This provides good flexibility while maintaining security.Note that this approach properly reads from the SettingsBloc without subscribing to changes, which is appropriate for an on-demand validation method.
lib/bloc/settings/settings_state.dart (1)
11-11: State management integration is well-implementedThe
weakPasswordsAllowedproperty is correctly integrated throughout all parts of the state class - constructor, factory method, fields, equality comparison, and the copyWith method.Also applies to: 19-19, 26-26, 33-33, 40-40, 46-46
lib/views/settings/widgets/general_settings/settings_manage_weak_passwords.dart (2)
11-21: Clean UI component implementationThe
SettingsManageWeakPasswordswidget properly encapsulates the weak password settings UI using the existingSettingsSectioncomponent for consistent styling.
23-48: Well-structured BLoC integration for settings toggleThe
AllowWeakPasswordsSwitchercomponent correctly:
- Uses BlocBuilder to react to state changes
- Properly displays the current setting value
- Dispatches the appropriate event when toggled
This implementation follows good Flutter practices by separating the UI logic from the event handling.
lib/model/stored_settings.dart (1)
12-12: Security-conscious model implementationThe
weakPasswordsAllowedsetting is properly integrated into the model with a secure default value offalse, ensuring that stronger password validation is applied unless explicitly disabled by the user.The JSON serialization/deserialization and copyWith implementations are correct and consistent with the existing pattern.
Also applies to: 19-19, 27-27, 41-41, 51-51, 60-60, 68-68
test_units/tests/password/validate_password_test.dart (1)
6-7: Ensure the test group is executed
testcheckPasswordRequirements()is only defined here.
Iftest_units/main.dart(or another runner) forgets to invoke it, the entire suite will be silently skipped.
Consider renaming the function to the conventionalmain()or keep the current name but add a call in the same file:void main() => testcheckPasswordRequirements();
takenagain
requested review from
AndrewDelaney,
CharlVS,
ShantanuSharma9873 and
smk762
smk762
left a comment
smk762
left a comment
There was a problem hiding this comment.
KDF password constraints functioning as expected when allow weak password is disabled. ✔️
When allow weak password is enabled, password constraints are still enforced 🚫
Additionally, while allow weak password is enabled, all validation error messages are displayed when any of the constraints are not met.
vokoscreenNG-2025-05-13_12-58-13.mp4
do not perform any validation if allow_weak_passwords is toggled
Fixed in be23475. All validation checks are now skipped when |
smk762
left a comment
smk762
left a comment
There was a problem hiding this comment.
Confirmed functioning as expected on wallet create/import and password change.
AndrewDelaney
left a comment
AndrewDelaney
left a comment
There was a problem hiding this comment.
All seems to be working as intended
ShantanuSharma9873
left a comment
ShantanuSharma9873
left a comment
There was a problem hiding this comment.
The functionality is working as expected. Looks good to me !
Closes #2612
Test for wallet creation and settings > update password.
allow weak passwordavailable before loginallow weak passwordis enabled, above constraints not enforcedChanges
allow_weak_passwordconfig parameter for support/debugging in the General settings menu.95191579e721eaedfd536c747991e6aa7ff1f4dcSummary by CodeRabbit
New Features
Bug Fixes
Tests
Style