fix(dashspend): fix DashSpend login & token-refresh auth#1492
Open
AshFrancis wants to merge 4 commits into
Open
fix(dashspend): fix DashSpend login & token-refresh auth#1492AshFrancis wants to merge 4 commits into
AshFrancis wants to merge 4 commits into
Conversation
The DashSpend (CTXSpend) token refresh flow could log users out or hang because of how the refresh endpoint and token state were handled. - Build the /refresh-token API without an Authenticator or Authorization header, so a 401 on refresh no longer recurses into another refresh (infinite refresh loop / wedged auth). - Only drop the session when the refresh token is genuinely rejected (HTTP 401); transient failures (offline, 5xx) now keep the tokens so the user stays signed in. - Persist rotated tokens under NonCancellable, so a refresh that wins the network race is saved even if the caller's coroutine is cancelled. - Serialize all refreshes through a single process-wide lock and one TokenAuthenticator.refreshAccessToken() entry point shared by the OkHttp retry path and CTXSpendRepository.refreshToken(). Previously these used separate per-instance locks, so a losing refresh could clear tokens that a parallel refresh had just rotated. - checkToken() now verifies the refresh token exists and refreshes when the stored token is missing/expired instead of trusting the timestamp alone. - Centralize token persistence in CTXSpendConfig.saveTokenState() / clearTokenState(). Adds unit tests covering rotation-after-cancel, transient-failure preservation, 401 rejection clearing, and the no-network dedup path.
Contributor
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors CTXSpend token handling: token-only Retrofit clients omit Authorization, headers injection is optional, refresh is synchronized by a process-wide mutex, token state persistence centralized, repository wiring updated, UI verifyEmail simplified, and tests cover rotation, failure, and 401 paths. ChangesToken Refresh Architecture
Sequence Diagram(s)sequenceDiagram
participant Caller
participant TokenAuthenticator
participant Mutex
participant CTXSpendTokenApi
participant CTXSpendConfig
Caller->>TokenAuthenticator: refreshAccessToken(staleAccessToken)
TokenAuthenticator->>Mutex: acquire()
TokenAuthenticator->>CTXSpendConfig: get stored access/refresh token
alt token already rotated
CTXSpendConfig-->>TokenAuthenticator: new access token
TokenAuthenticator->>Caller: return new token
else token stale -> proceed to network
TokenAuthenticator->>CTXSpendTokenApi: refreshToken(refreshToken)
CTXSpendTokenApi-->>TokenAuthenticator: RefreshTokenResponse
TokenAuthenticator->>CTXSpendConfig: saveTokenState(newAccess, newRefresh)
TokenAuthenticator->>Caller: return new access token
else 401 rejection
TokenAuthenticator->>CTXSpendConfig: clearTokenState()
TokenAuthenticator->>Caller: return null
end
TokenAuthenticator->>Mutex: release()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt`:
- Around line 122-127: In verifyEmail (CTXSpendRepository) avoid using the
non-null assertions on response and its tokens: check that email (from
CTXSpendConfig.PREFS_KEY_CTX_PAY_EMAIL) is non-null, call api.verifyEmail and
verify the returned RefreshTokenResponse and its accessToken/refreshToken are
non-null before calling config.saveTokenState; if any are missing, handle
gracefully (return false or surface a clear error) instead of using !!. Use safe
calls and explicit null checks around api.verifyEmail, response.accessToken and
response.refreshToken so saveTokenState is only invoked with valid tokens and
the method returns a safe Boolean result.
🪄 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: 125d763c-0aac-4979-9317-454ab5aedf1c
📒 Files selected for processing (8)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/di/ExploreDashModule.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/RemoteDataSource.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/authenticator/TokenAuthenticator.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/interceptor/HeadersInterceptor.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/DashSpendRepositoryFactory.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/utils/CTXSpendConfig.ktfeatures/exploredash/src/test/java/org/dash/wallet/features/exploredash/CTXSpendRepositoryTokenTest.kt
verifyEmail used `response?.accessToken!!` / `response.refreshToken!!`, which crashes if the verify endpoint returns a 2xx with a null/partial body. Replace the non-null assertions with explicit null/blank checks that return false and persist nothing, so a malformed response fails gracefully instead of throwing on the auth-critical path. Addresses CodeRabbit review on dashpay#1492. Adds a unit test covering the token-less response case.
After a successful email verification (server returns 200 with tokens, which are parsed and stored), the OTP screen decided navigation from viewModel.selectedProvider. That is non-persisted, nav-graph-scoped ViewModel state which resets to null when the ViewModel is recreated (process death or "Don't keep activities" - routine while the user leaves the app to read the emailed code). A null provider hit `else -> error(...)`, which the catch block painted over as "invalid code" even though login had actually succeeded; the user then retried the now-consumed code and got real 400s. - Navigate using `provider` (the nav arg, which survives recreation), not viewModel.selectedProvider. - Restore viewModel.selectedProvider so downstream screens that read it (PurchaseGiftCardFragment/V2) don't crash next. - Drop the error() crash; the `when` over the GiftCardProviderType sealed class is exhaustive. - Stop masking every failure as a silent no-op / generic "invalid code": log the real cause and show an error on the unsuccessful path too.
AshFrancis
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/dashspend/DashSpendUserAuthFragment.kt`:
- Around line 257-267: The verify flow is collapsing server/transport exceptions
into the "invalid code" UX; change verifyEmail to return a richer result (e.g.,
sealed class or enum like VerifyResult { Success, InvalidCode,
ServerError(message) }) across CTXSpendRepository.verifyEmail and
DashSpendViewModel.verifyEmail, update the fragment's handling in
DashSpendUserAuthFragment to switch on that result and only call
showVerifyError(R.string.invaild_code) for InvalidCode while showing/logging
ServerError with the exception-derived message (or a localized server-failure
string) and preserve authUser()'s existing behavior for thrown exceptions; if
you cannot change signatures immediately, at minimum modify the catch block in
DashSpendUserAuthFragment to call showVerifyError(e.message ?:
getString(R.string.verification_failed)) and log the full exception instead of
mapping it to R.string.invaild_code.
🪄 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: 87377259-c7ac-4c45-82ab-879f9b03d5dd
📒 Files selected for processing (1)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/dashspend/DashSpendUserAuthFragment.kt
verifyEmail() routed every failure to R.string.invaild_code, so a 5xx, network drop, or backend anomaly was shown to the user as a wrong code. Distinguish a genuine code rejection (CTXSpendException with HTTP 400) from other failures: only the former shows "invalid code"; everything else shows a generic "An error occurred, try again" (R.string.loading_error) and is logged. The non-exception false path (200 with no tokens) is likewise a server anomaly, not a wrong code, so it uses the generic message too. Addresses CodeRabbit review on dashpay#1492.
Collaborator
|
Thank you @AshFrancis, we will verify this with QA testing in the coming week. |
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.
Issue being fixed or feature implemented
A set of related DashSpend (CTXSpend) authentication bugs that could prevent login or log users out unexpectedly.
1. First login failed even though the server returned 200 (most user-visible)
On a successful email verification — server returns
200withaccessToken/refreshToken, which the client parses and stores — the OTP screen decided navigation fromviewModel.selectedProvider. That is non-persisted, nav-graph-scoped ViewModel state that resets tonullwhen the ViewModel is recreated (process death or "Don't keep activities" — routine while the user leaves the app to read the emailed code). A null provider hitelse -> error(...), which the genericcatchpainted over as "invalid code" even though login had actually succeeded. The user then retried the now-consumed code and got real400s.Verified against the
spend-apisource:verify-emailreturns200with tokens only on a correct code (and clears the code on the user's first-ever login viaSetEmailVerifiedwhenLastLogin.IsZero()), which matches the observedlogin → verify(200) → verify → verify → loginserver log.Fix:
provider(the nav arg, which survives recreation), notviewModel.selectedProvider.viewModel.selectedProviderso downstream screens that read it (PurchaseGiftCardFragment/V2) don't crash next.error()crash (whenover theGiftCardProviderTypesealed class is exhaustive).2. Token refresh could loop, log users out, or clobber tokens
/refresh-tokenAPI was built with the standard client (it attached theTokenAuthenticatorand theAuthorizationheader), so a401on refresh recursed into another refresh — an infinite loop / wedged auth. It's now built with no authenticator and noAuthorizationheader.401rejection (confirmed:spend-apireturns401 ErrTokenInvalidfor a bad refresh token); transient failures keep the session.NonCancellable, so a refresh that completes is saved even if the caller is cancelled.TokenAuthenticator.refreshAccessToken()behind a process-wide lock (correct:CTXSpendConfigis a singleton). Previously the OkHttp retry path andCTXSpendRepository.refreshToken()used separate per-instance locks, so a losing refresh could clear tokens another caller had just rotated.checkToken()verifies the refresh token exists and refreshes when missing/expired instead of trusting the timestamp alone.CTXSpendConfig.saveTokenState()/clearTokenState().3.
verifyEmailNPE hardeningresponse?.accessToken!!/response.refreshToken!!could NPE on a 2xx with a null/partial body. Now returnsfalsegracefully without persisting. (Raised by CodeRabbit; predated this PR.)Related PR's and Dependencies
None.
Screenshots / Videos
N/A — networking/auth + navigation logic.
How Has This Been Tested?
CTXSpendRepositoryTokenTest(Robolectric): rotation persisted after caller cancels, tokens preserved on transient failure, tokens cleared on401, no-network dedup path, andverifyEmailreturningfalseon a token-less response../gradlew :features:exploredash:testDebugUnitTest→ all pass.compileDebugKotlin+ktlint(main & test) clean.spend-apihandlers + the client ViewModel scoping); recommend a QA pass of first login with the app backgrounded during the OTP step.Checklist:
Summary by CodeRabbit
Bug Fixes
Refactor
Tests