Harden authentication flow with login throttling and safer credential handling#2679
Open
Divyateja2709 wants to merge 1 commit into
Open
Harden authentication flow with login throttling and safer credential handling#2679Divyateja2709 wants to merge 1 commit into
Divyateja2709 wants to merge 1 commit into
Conversation
Summary by CodeRabbit
WalkthroughThis PR implements temporary account lockout after repeated failed login attempts, improves password security by not persisting credentials, and refactors login result handling to use consistent failure/success callbacks throughout the authentication flow. ChangesLogin Attempt Lockout & Password Security
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (2 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/auth/src/commonMain/kotlin/com/mifos/feature/auth/login/LoginViewModel.kt (2)
79-105: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAsync collection and reducer/state transitions are still mixed.
In LoginViewModel.kt,
collectdirectly mutates_loginUiStateacross branches instead of dispatching internal actions and reducing in one place. This is explicitly against the project’s ViewModel MVI rule.As per coding guidelines: "Repository/network/database results MUST be converted into internal actions using
sendAction(...)" and "handleAction(...)must act as the primary reducer responsible for state mutation."🤖 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 `@feature/auth/src/commonMain/kotlin/com/mifos/feature/auth/login/LoginViewModel.kt` around lines 79 - 105, The collector in LoginViewModel is directly mutating _loginUiState inside loginUseCase(...).collect and must instead translate DataState branches into internal actions and let handleAction(...) perform all state updates; replace direct assignments and Logger calls in the collect block with sendAction(...) calls (e.g., LoginAction.LoginLoading, LoginAction.LoginSuccess(authData, username), LoginAction.LoginError(errorPayload)) and include any error/result payloads needed, then implement handling in handleAction(...) to set _loginUiState (ShowProgress, ShowError, resetLoginFailureState, onLoginSuccessful/onLoginFailure) so LoginViewModel only dispatches actions from the collect block and handleAction is the single reducer that mutates _loginUiState.
114-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard nullable auth response fields before persisting user data.
PostAuthenticationResponsedefinesuserId,officeId, andpermissionsas nullable. The force-unwraps at lines 116–121 will crash the login success path if the backend returns incomplete payloads.Add explicit null checks before persisting user data, failing gracefully with a
ShowErrorstate:Proposed patch
viewModelScope.launch { + val userId = user.userId + val officeId = user.officeId + val permissions = user.permissions + if (userId == null || officeId == null || permissions == null) { + _loginUiState.value = LoginUiState.ShowError(Res.string.feature_auth_error_login_failed) + Logger.d("@@@", Throwable("login: missing required auth fields in response")) + return@launch + } + prefManager.updateUser( User( username = username, password = null, - userId = user.userId!!, + userId = userId, base64EncodedAuthenticationKey = user.base64EncodedAuthenticationKey, isAuthenticated = user.authenticated ?: false, - officeId = user.officeId!!, + officeId = officeId, officeName = user.officeName, - permissions = user.permissions!!, + permissions = permissions, ), ) }Per coding guidelines: avoid
!!operator and handle null cases explicitly.🤖 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 `@feature/auth/src/commonMain/kotlin/com/mifos/feature/auth/login/LoginViewModel.kt` around lines 114 - 123, The code is force-unwrapping nullable fields from PostAuthenticationResponse (user.userId, user.officeId, user.permissions) before calling prefManager.updateUser, which can crash; update LoginViewModel's success handling to explicitly guard those fields on the received `user` (from PostAuthenticationResponse) - check that user.userId, user.officeId and user.permissions are non-null (and any other nullable fields used) and if any are null emit the ShowError state (or appropriate failure state) and abort persisting; only call prefManager.updateUser(User(...)) with the unwrapped values after these null checks, removing all `!!` uses. Ensure you reference the existing prefManager.updateUser call and the User construction in LoginViewModel to apply the checks.
🤖 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
`@core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoginRepositoryImp.kt`:
- Around line 24-25: The login method in LoginRepositoryImp returns the network
DTO PostAuthenticationResponse directly; change it to call
dataManagerAuth.login(...) and map the returned PostAuthenticationResponse into
a domain model (or appropriate entity) using the existing mapper, then wrap the
result in a DataState before returning; update the signature/return flow of
LoginRepositoryImp.login to return DataState<YourDomainAuthModel> (or
DataState<Unit/SuccessType> as appropriate), use the mapper to translate
PostAuthenticationResponse -> Domain type, and propagate errors into DataState
as per repository conventions.
In
`@feature/auth/src/commonMain/kotlin/com/mifos/feature/auth/login/LoginViewModel.kt`:
- Around line 81-85: The failure handling currently calls onLoginFailure() but
doesn't know if lockout was triggered, so update onLoginFailure to return a
Boolean (true when lockout threshold reached) and change all callers (the
DataState.Error branches in LoginViewModel where Logger.d and _loginUiState are
set, e.g., the blocks around the existing handlers at the mentioned locations)
to capture the returned value and choose
LoginUiState.ShowLockout(Res.string.feature_auth_error_login_locked_out) when
true, otherwise
LoginUiState.ShowError(Res.string.feature_auth_error_login_failed); also update
the onLoginFailure implementation to compute and return the lockout flag and
propagate any necessary state changes.
---
Outside diff comments:
In
`@feature/auth/src/commonMain/kotlin/com/mifos/feature/auth/login/LoginViewModel.kt`:
- Around line 79-105: The collector in LoginViewModel is directly mutating
_loginUiState inside loginUseCase(...).collect and must instead translate
DataState branches into internal actions and let handleAction(...) perform all
state updates; replace direct assignments and Logger calls in the collect block
with sendAction(...) calls (e.g., LoginAction.LoginLoading,
LoginAction.LoginSuccess(authData, username),
LoginAction.LoginError(errorPayload)) and include any error/result payloads
needed, then implement handling in handleAction(...) to set _loginUiState
(ShowProgress, ShowError, resetLoginFailureState,
onLoginSuccessful/onLoginFailure) so LoginViewModel only dispatches actions from
the collect block and handleAction is the single reducer that mutates
_loginUiState.
- Around line 114-123: The code is force-unwrapping nullable fields from
PostAuthenticationResponse (user.userId, user.officeId, user.permissions) before
calling prefManager.updateUser, which can crash; update LoginViewModel's success
handling to explicitly guard those fields on the received `user` (from
PostAuthenticationResponse) - check that user.userId, user.officeId and
user.permissions are non-null (and any other nullable fields used) and if any
are null emit the ShowError state (or appropriate failure state) and abort
persisting; only call prefManager.updateUser(User(...)) with the unwrapped
values after these null checks, removing all `!!` uses. Ensure you reference the
existing prefManager.updateUser call and the User construction in LoginViewModel
to apply the checks.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86dbf3af-e1ef-4604-a40c-8db819fd003b
📒 Files selected for processing (3)
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoginRepositoryImp.ktfeature/auth/src/commonMain/composeResources/values/strings.xmlfeature/auth/src/commonMain/kotlin/com/mifos/feature/auth/login/LoginViewModel.kt
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/data/repositoryImp/**/*.kt
⚙️ CodeRabbit configuration file
**/data/repositoryImp/**/*.kt: Repository Implementation Guidelines:
- MUST convert
T->DataState<T>for one-shot APIs, andFlow<T>->Flow<DataState<T>>using.asDataStateFlow().- MUST perform network availability checks, handle exceptions, and apply appropriate dispatchers.
- Repositories act as the boundary translator. They MUST use mappers to convert DTOs (from network) and Entities (from database) into Domain Models (from :core:model) before returning data.
- Flag: If a repository implementation returns a raw DTO or Entity directly to the caller instead of a Domain model.
Files:
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoginRepositoryImp.kt
**/{core-base,core}/**/*.kt
⚙️ CodeRabbit configuration file
**/{core-base,core}/**/*.kt: Critical Module Change Detection:
- Changes in
core-basemodule must be treated as high-impact.- Flag any PR that modifies files inside
core-basefor careful review.- Verify that modifications in
core-baseare necessary and minimal.
Output:- Clearly highlight that
core-baseis a shared foundational module and requires extra review attention.
Files:
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoginRepositoryImp.kt
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: Additional Code Review Guidelines:
- Null Safety & Stability
- Avoid using
!!operator- Handle null cases explicitly using safe calls or proper state handling
- Do not assume values are always non-null without guarantees
- Architecture Boundaries
- ViewModel must not depend on specific network/library implementations
- Ensure proper separation between data, domain, and presentation layers
- Do not format data (currency, dates, calculations) inside the UI layer
- All formatting must be handled in the ViewModel and exposed via state (e.g., StateFlow)
- Performance Considerations
- Avoid unnecessary recompositions in Compose
- Do not attach heavy logic to frequently changing states (e.g., scrollState)
- Prefer lifting state up instead of recomputing in child composables
- Compose & Navigation Best Practices
- NEVER trigger navigation functions or side-effects directly during composition
- Always wrap navigation calls inside
LaunchedEffectorEventsEffectto avoid repeated execution on recomposition- Avoid triggering intensive side-effects during recomposition
- Navigation routes must be type-safe.
- Ensure all route classes or objects used for navigation are annotated with
@Serializable.
- UI Structure
- Dialogs must be separated into their own composables
- Do not embed dialogs inline within complex main screens
- Localization Consistency
- Ensure all supported languages are updated consistently across modules
- Verify translations exist for newly added UI strings
- Code Cleanliness
- Avoid unnecessary inline comments unless critical
- Remove leftover debug or commented code
- Focus on correctness, readability, and maintainability over cosmetic nitpicks.
- Avoid reviewing README, config, or asset files.
- Prioritize identifying bugs, performance issues, and architectural concerns.
- Naming & Intent Rules:
- Follow the official Kotlin Coding Conventions:
https://kotlinlang.org/docs/coding-conventions.html- Use self-explanatory ...
Files:
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoginRepositoryImp.ktfeature/auth/src/commonMain/kotlin/com/mifos/feature/auth/login/LoginViewModel.kt
**/composeResources/values*/strings.xml
⚙️ CodeRabbit configuration file
**/composeResources/values*/strings.xml: String resource conventions:Naming:
- All keys must follow:
feature_{feature_name}_{ui_text_in_snake_case}Examples:
"In Advance" → feature_loan_in_advance
"Outstanding" → feature_loan_outstanding
- Avoid generic names like: title, text1, label
- The suffix should be a short, readable representation of the UI text
- Avoid multiple keys representing the same UI text
- Keys must be lowercase and use snake_case
Flag:
- Incorrect naming pattern
- Generic or unclear key names
- Duplicate keys for same UI text
Files:
feature/auth/src/commonMain/composeResources/values/strings.xml
**/*ViewModel.kt
⚙️ CodeRabbit configuration file
**/*ViewModel.kt: MVI architecture rules:
- All new features must follow MVI
- ViewModel must extend BaseViewModel
- The ViewModel MUST maintain a single UI state (e.g., a single StateFlow)
instead of multiple separate state variables.Required:
- Use *State, *Event, *Action
- Naming must be consistent (FeatureViewModel, FeatureState, FeatureEvent, FeatureAction)
- Follow unidirectional flow:
Action → ViewModel → State → UIInternal reducer/action architecture rules:
Async operations MUST NOT directly mutate UI state repeatedly inside
Flow collectors, suspend callbacks, or repository result handlers.Repository/network/database results MUST be converted into internal
actions usingsendAction(...).
handleAction(...)must act as the primary reducer responsible for:
- state mutation
- reducer-style state transitions
- triggering follow-up actions
Large async methods must be split into:
- async collection layer
- internal action dispatching
- reducer/state handling
Avoid directly calling another business/data-loading method from
repository collectors or async callbacks.
Prefer dispatching follow-up internal actions instead.Preferred pattern:
repository result
-> sendAction(...)
-> handleAction(...)
-> mutableStateFlow.update { ... }Anti-pattern examples:
mutableStateFlow.update { ... } inside collect { }
fetchX() -> directly calls fetchY() inside async collector
large methods mixing:
- collection
- state mutation
- navigation
- business chainingFlag:
- Multiple
mutableStateFlow.update {}calls inside collect { }- Direct state mutation inside async repository callbacks
- Async methods performing both collection and reducer logic
- Direct business-flow chaining from async collectors
- Missing internal reducer actions for async results
- Missing *State / *Event / *Action
- ViewModel not...
Files:
feature/auth/src/commonMain/kotlin/com/mifos/feature/auth/login/LoginViewModel.kt
🧠 Learnings (2)
📚 Learning: 2026-02-06T13:15:16.968Z
Learnt from: sahilshivekar
Repo: openMF/android-client PR: 2603
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsViewModel.kt:43-106
Timestamp: 2026-02-06T13:15:16.968Z
Learning: Guideline: When a Kotlin function parameter is nullable (e.g., balance: Double?, currencyCode: String?, maximumFractionDigits: Int?) and downstream calls require a non-null value, add null-safety handling in all implementations. Specifically, avoid calling Currency.getInstance(currencyCode) with a possibly null currencyCode; provide a safe default (e.g., currencyCode ?: "$") or validate before use. Ensure all platform targets (Android/Desktop/Native) follow consistent null handling, and consider centralizing currencyCode normalization in the common layer if feasible. Add tests covering null currencyCode to prevent NPEs.
Applied to files:
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoginRepositoryImp.ktfeature/auth/src/commonMain/kotlin/com/mifos/feature/auth/login/LoginViewModel.kt
📚 Learning: 2026-04-01T05:03:14.323Z
Learnt from: kartikey004
Repo: openMF/mifos-x-field-officer-app PR: 2659
File: feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanDisbursement/LoanAccountDisbursementScreen.kt:190-195
Timestamp: 2026-04-01T05:03:14.323Z
Learning: In this repo, existing `SelectableDates` / `SelectableDates.isSelectableDate(utcTimeMillis: Long)` implementations use `Clock.System.now().toEpochMilliseconds()` (UTC epoch millis) for date-boundary checks. During PR reviews, do not flag these checks for not using a timezone-aware `LocalDate` start-of-day approach; treat it as an established project-wide pattern. If a change is desired, handle it as a coordinated project-wide improvement rather than as a per-PR review issue.
Applied to files:
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoginRepositoryImp.ktfeature/auth/src/commonMain/kotlin/com/mifos/feature/auth/login/LoginViewModel.kt
🔇 Additional comments (1)
feature/auth/src/commonMain/composeResources/values/strings.xml (1)
21-21: String key and copy look good.In strings.xml,
feature_auth_error_too_many_attemptsis well-named and the message is user-friendly.
Comment on lines
24
to
+25
| override suspend fun login(username: String, password: String): PostAuthenticationResponse { | ||
| return dataManagerAuth.login(username.trim(), password.trim()) | ||
| return dataManagerAuth.login(username.trim(), password) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Repository boundary still leaks a network DTO.
In LoginRepositoryImp.kt, login (Line 24) returns PostAuthenticationResponse directly. This bypasses the repository translation boundary and couples callers to transport models.
As per coding guidelines: "MUST convert T -> DataState<T> ... Repositories ... MUST use mappers ... Flag ... returns a raw DTO or Entity directly."
🤖 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
`@core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoginRepositoryImp.kt`
around lines 24 - 25, The login method in LoginRepositoryImp returns the network
DTO PostAuthenticationResponse directly; change it to call
dataManagerAuth.login(...) and map the returned PostAuthenticationResponse into
a domain model (or appropriate entity) using the existing mapper, then wrap the
result in a DataState before returning; update the signature/return flow of
LoginRepositoryImp.login to return DataState<YourDomainAuthModel> (or
DataState<Unit/SuccessType> as appropriate), use the mapper to translate
PostAuthenticationResponse -> Domain type, and propagate errors into DataState
as per repository conventions.
Comment on lines
81
to
85
| is DataState.Error -> { | ||
| onLoginFailure() | ||
| _loginUiState.value = | ||
| LoginUiState.ShowError(Res.string.feature_auth_error_login_failed) | ||
| Logger.d("@@@", Throwable("login: ${result.data}")) |
There was a problem hiding this comment.
Show lockout error immediately on the threshold failure.
In LoginViewModel.kt, when the 5th failure occurs, lockout is activated but UI still shows generic login failure; lockout text appears only on the next attempt.
Short fix: make onLoginFailure() return whether lockout was triggered and map the error string accordingly.
Proposed patch
- private fun onLoginFailure() {
+ private fun onLoginFailure(): Boolean {
failedLoginAttempts += 1
if (failedLoginAttempts >= MAX_FAILED_ATTEMPTS) {
lockoutExpiresAtMillis = Clock.System.now().toEpochMilliseconds() + LOCKOUT_DURATION_MILLIS
failedLoginAttempts = 0
+ return true
}
+ return false
}- is DataState.Error -> {
- onLoginFailure()
- _loginUiState.value =
- LoginUiState.ShowError(Res.string.feature_auth_error_login_failed)
+ is DataState.Error -> {
+ val isLocked = onLoginFailure()
+ _loginUiState.value = LoginUiState.ShowError(
+ if (isLocked) Res.string.feature_auth_error_too_many_attempts
+ else Res.string.feature_auth_error_login_failed,
+ )
Logger.d("@@@", Throwable("login: ${result.data}"))
}- } else {
- onLoginFailure()
- _loginUiState.value =
- LoginUiState.ShowError(Res.string.feature_auth_error_login_failed)
+ } else {
+ val isLocked = onLoginFailure()
+ _loginUiState.value = LoginUiState.ShowError(
+ if (isLocked) Res.string.feature_auth_error_too_many_attempts
+ else Res.string.feature_auth_error_login_failed,
+ )Also applies to: 97-100, 135-140
🤖 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
`@feature/auth/src/commonMain/kotlin/com/mifos/feature/auth/login/LoginViewModel.kt`
around lines 81 - 85, The failure handling currently calls onLoginFailure() but
doesn't know if lockout was triggered, so update onLoginFailure to return a
Boolean (true when lockout threshold reached) and change all callers (the
DataState.Error branches in LoginViewModel where Logger.d and _loginUiState are
set, e.g., the blocks around the existing handlers at the mentioned locations)
to capture the returned value and choose
LoginUiState.ShowLockout(Res.string.feature_auth_error_login_locked_out) when
true, otherwise
LoginUiState.ShowError(Res.string.feature_auth_error_login_failed); also update
the onLoginFailure implementation to compute and return the lockout flag and
propagate any necessary state changes.
IOhacker
requested changes
May 11, 2026
IOhacker
left a comment
IOhacker
left a comment
Contributor
There was a problem hiding this comment.
The Max failed Attempts is managed by the back end not by the front end. It should be removed.
IOhacker
reviewed
May 11, 2026
| _loginUiState.value = LoginUiState.PassCodeActivityIntent | ||
| } | ||
|
|
||
| private fun isLoginTemporarilyLocked(): Boolean { |
Contributor
There was a problem hiding this comment.
The Max failed Attempts is managed by the back end not by the front end. It should be removed.
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
2 participants
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.
hi @therajanmaurya
This PR introduces a focused security remediation in the authentication flow, aligned with Cipher 2026 hardening goals.
It adds temporary login throttling after repeated failed attempts, avoids persisting plaintext passwords in local user state, and preserves password fidelity by removing accidental trimming before auth requests.
Changes included
Add failed-attempt lockout logic in login flow (5 failures -> 1 minute lockout).
Add localized error message for temporary lockout.
Stop storing raw password in persisted User object.
Keep username normalization (trim) but do not trim password in repository login call.
Why this matters
Reduces risk of brute-force behavior on client login attempts.
Minimizes sensitive data exposure in local persisted state.
Prevents authentication edge cases caused by password mutation.
Validation
Static lints on touched files: no issues.
Build verification was blocked locally because JAVA_HOME is not set in the environment.