Recognize and inform user when login invalidated due to login on another device #3513
Conversation
…oring in TokenDeniedException.
Using exception bubble-up to CommCareExceptionHandler to handle global error storage, DB cleanup, and redirect to login. Helper method to detect LOGIN_FROM_DIFFERENT_DEVICE error, used in two relevant places (for async and sync calls). Added new GlobalErrors value to represent login from second device. Added error message for second device login with translations. Filled in missing translations for the generic error message when login gets invalidated.
…nto ccct-2025/lost_config_reason Handled conflicts in: # app/src/org/commcare/connect/PersonalIdManager.java # app/src/org/commcare/connect/network/ApiPersonalId.java # app/src/org/commcare/connect/network/ConnectNetworkHelper.java # app/src/org/commcare/connect/network/ConnectSsoHelper.java
OrangeAndGreen
requested review from
Jignesh-dimagi,
conroy-ricketts and
shubham1g5
📝 WalkthroughWalkthroughThis PR introduces a refactored error handling mechanism for PersonalID authentication failures. It renames Sequence DiagramsequenceDiagram
participant Client as HTTP Client
participant BaseApi as BaseApiCallback
participant Helper as ConnectNetworkHelper
participant DbHelper as ConnectDatabaseHelper
participant Handler as CommCareExceptionHandler
Client->>BaseApi: HTTP 400 Bad Request
BaseApi->>Helper: checkForLoginFromDifferentDevice(errorBody)
Helper-->>BaseApi: true (if LOGIN_FROM_DIFFERENT_DEVICE code)
alt Login from Different Device Detected
BaseApi->>DbHelper: triggerGlobalError(PERSONALID_LOGIN_FROM_DIFFERENT_DEVICE_ERROR)
DbHelper->>DbHelper: throw LoginInvalidatedException(reason)
else Token Request Denied
BaseApi->>DbHelper: triggerGlobalError(PERSONALID_LOST_CONFIGURATION_ERROR)
DbHelper->>DbHelper: throw LoginInvalidatedException(reason)
end
DbHelper-->>Handler: uncaught LoginInvalidatedException
Handler->>Handler: findRootLoginInvalidatedException(cause chain)
Handler->>Handler: Derive crash reason from exception
Handler->>DbHelper: Store crash record
Handler->>Handler: startDispatchActivity()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/org/commcare/network/HttpCalloutTask.java (1)
79-110: Don’t swallow LoginInvalidatedException in callout handling.
SinceLoginInvalidatedExceptionnow extendsIOException, it will be mapped toNetworkFailurehere, preventing the global invalidation flow from triggering. Add an explicit catch before the genericIOExceptionbranch and let it bubble (wrapped) for the global handler.🛠 Proposed fix
import org.commcare.connect.network.TokenUnavailableException; +import org.commcare.connect.network.LoginInvalidatedException; ... } catch (CaptivePortalRedirectException e) { e.printStackTrace(); outcome = HttpCalloutOutcomes.CaptivePortal; + } catch (LoginInvalidatedException e) { + throw new RuntimeException(e); } catch (TokenUnavailableException e) { e.printStackTrace(); outcome = HttpCalloutOutcomes.TokenUnavailable; } catch (IOException e) {app/src/org/commcare/tasks/DataPullTask.java (1)
296-321: Let LoginInvalidatedException escape to the global handler.
With the new exception type, the broadIOExceptioncatch will mask login invalidation. Handle it explicitly and rethrow (wrapped) so the global recovery path can run.🛠 Proposed fix
} catch (TokenUnavailableException e) { responseError = PullTaskResult.TOKEN_UNAVAILABLE; + } catch (LoginInvalidatedException e) { + throw new RuntimeException(e); } catch (IOException e) { e.printStackTrace(); Logger.log(LogTypes.TYPE_WARNING_NETWORK, "Couldn't sync due to IO Error|" + e.getMessage()); } catch (UnknownSyncError e) {
🤖 Fix all issues with AI agents
In `@app/src/org/commcare/utils/CommCareExceptionHandler.java`:
- Around line 47-54: The call to CrashUtil.reportException currently passes
ex.getCause(), which can be an intermediate wrapper and may omit the full
context; update the reporting to pass either the full exception (ex) for
complete stack trace or the resolved root LoginInvalidatedException
(loginInvalidated) if you only want that specific error; locate the logic in
CommCareExceptionHandler where findRootLoginInvalidatedException(ex) is used and
replace CrashUtil.reportException(ex.getCause()) with
CrashUtil.reportException(ex) or CrashUtil.reportException(loginInvalidated) as
appropriate for the desired diagnostics.
🧹 Nitpick comments (2)
app/src/org/commcare/connect/PersonalIdManager.java (1)
544-552: Potential nullappRecordif database state changes between checks.The
isSeatedAppLinkedToPersonalId(username)check at line 544 queries the database, butgetConnectLinkedAppRecordat line 548-549 queries it again. In a rare race condition, the record could be deleted between these calls, resulting inappRecordbeing null when passed toretrieveHqSsoTokenSync.Consider adding a null check or reusing the record from the validation check:
♻️ Suggested defensive check
ConnectLinkedAppRecord appRecord = ConnectAppDatabaseUtil.getConnectLinkedAppRecord(manager.parentActivity, seatedAppId, username); + if (appRecord == null) { + return null; + } + return ConnectSsoSyncHelper.INSTANCE.retrieveHqSsoTokenSync(CommCareApplication.instance(), user, appRecord, username, false);app/src/org/commcare/connect/network/LoginInvalidatedException.kt (1)
15-17: Consider makingreasonimmutable.The
reasonfield is declared asvar, making it mutable. For an exception class, immutability is generally preferred to ensure the exception state remains consistent after creation.Suggested fix
-class LoginInvalidatedException(`@JvmField` var reason: GlobalErrors?) : IOException() { +class LoginInvalidatedException(`@JvmField` val reason: GlobalErrors?) : IOException() {
| LoginInvalidatedException loginInvalidated = findRootLoginInvalidatedException(ex); | ||
| if(loginInvalidated != null) { | ||
| CrashUtil.reportException(ex.getCause()); | ||
| GlobalErrors reason = loginInvalidated.reason != null ? loginInvalidated.reason : | ||
| GlobalErrors.PERSONALID_LOST_CONFIGURATION_ERROR; | ||
| ConnectDatabaseHelper.crashDb(reason); | ||
| startDispatchActivity(); | ||
| System.exit(0); |
There was a problem hiding this comment.
Potential issue: ex.getCause() may not report the intended exception.
On line 49, CrashUtil.reportException(ex.getCause()) reports the immediate cause of the original exception. However, LoginInvalidatedException could be nested several levels deep in the exception chain. This may result in reporting an intermediate wrapper exception rather than the full context or the LoginInvalidatedException itself.
Consider reporting the full exception ex for complete stack trace context, or loginInvalidated if only the root cause is desired.
Suggested fix
if(loginInvalidated != null) {
- CrashUtil.reportException(ex.getCause());
+ CrashUtil.reportException(ex);
GlobalErrors reason = loginInvalidated.reason != null ? loginInvalidated.reason :
GlobalErrors.PERSONALID_LOST_CONFIGURATION_ERROR;🤖 Prompt for AI Agents
In `@app/src/org/commcare/utils/CommCareExceptionHandler.java` around lines 47 -
54, The call to CrashUtil.reportException currently passes ex.getCause(), which
can be an intermediate wrapper and may omit the full context; update the
reporting to pass either the full exception (ex) for complete stack trace or the
resolved root LoginInvalidatedException (loginInvalidated) if you only want that
specific error; locate the logic in CommCareExceptionHandler where
findRootLoginInvalidatedException(ex) is used and replace
CrashUtil.reportException(ex.getCause()) with CrashUtil.reportException(ex) or
CrashUtil.reportException(loginInvalidated) as appropriate for the desired
diagnostics.
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Looking quite good overall, left some minor questions/suggestions.
| public static void crashDb(GlobalErrors error) { | ||
| crashDb(error, null); | ||
| public static void triggerGlobalError(GlobalErrors error) { | ||
| throw new RuntimeException(new LoginInvalidatedException(error)); |
There was a problem hiding this comment.
I would imagine that you can instead extend LoginInvalidatedException from RuntimeException and throw it directly instead of wrapping it
There was a problem hiding this comment.
Ah I like that, makes it a bit simpler. I think in previous code having the exception be an IOException helped with routing it correctly, but no need for that now. f28de33
app/src/org/commcare/connect/database/ConnectDatabaseHelper.java
Outdated
Show resolved
Hide resolved
| * Corrective action is to recover the ConnectID account on the device. | ||
| */ | ||
| class TokenDeniedException: IOException() { | ||
| class LoginInvalidatedException(@JvmField var reason: GlobalErrors?) : IOException() { |
There was a problem hiding this comment.
Little curious what is @JvmField meant to achieve here, also should reason be defined as non null instead ?
There was a problem hiding this comment.
That makes the field accessible from Java code, i.e. loginInvalidated.reason in CommCareExceptionHandler.
There was a problem hiding this comment.
Got it, think Java way to do that would be loginInvalidated.getReason() without needing to add a JvmField
app/src/org/commcare/connect/database/ConnectDatabaseHelper.java
Outdated
Show resolved
Hide resolved
…on PR comments. Not throwing exception if error body isn't JSON format. Changed LoginInvalidatedExceptoin to a RuntimeException (to avoid having to wrap when throwing) and made reason non-nullable.
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
LGTM! The biggest thing is a clarifying question I had in ConnectSsoHelper.java.
app/res/values/strings.xml
Outdated
| <string name="recovery_network_outdated">This app is outdated. Please update the app on the Google Play Store in order to proceed.</string> | ||
| <string name="recovery_network_token_unavailable">There was a network issue. Please try again.</string> | ||
| <string name="recovery_network_token_request_rejected">Lost PersonalID configuration with server, please recover your Personal ID account and retry</string> | ||
| <string name="recovery_network_token_request_rejected">Lost PersonalID configuration with server, please recover your PersonalID account and retry</string> |
There was a problem hiding this comment.
Side note: is "PersonalID" the correct branding? For some reason I always thought it was "Personal ID" with a space
There was a problem hiding this comment.
Oh wow, this is a great point to bring up! I've always gone with PersonalID (no space) but we should make sure we're using the right one and then be consistent. Fortunately I currently only see five instances of UI strings using "Personal ID", while the majority don't include a space.
There was a problem hiding this comment.
@OrangeAndGreen Judging from Slack, it looks like the consensus is that the correct branding is "PersonalID". Do you want to include that as part of this PR? I noticed that there a few code comments that also say "Personal ID". I don't mind making a quick PR to get rid of the spaces everywhere in the code
| return doTaskBackground(params); | ||
| } catch (Exception e) { | ||
| if (!(e instanceof UserCausedRuntimeException)) { | ||
| if (e.getCause() instanceof LoginInvalidatedException) { |
There was a problem hiding this comment.
should this now change change to if (e instanceof LoginInvalidatedException) {
There was a problem hiding this comment.
Even better! Now that LoginInvalidatedException is a RuntimeException, there's less handling required (i.e. wrapping, catching and rethrowing). Here, the unchecked exception isn't handled by the AsyncTask so now bubbles up directly to CommCareExceptionHandler. 71fc6e3
| if (RecoveryMeasuresHelper.recoveryMeasuresPending()) { | ||
| LoginInvalidatedException loginInvalidated = findRootLoginInvalidatedException(ex); | ||
| if (loginInvalidated != null) { | ||
| CrashUtil.reportException(ex.getCause()); |
There was a problem hiding this comment.
similarly, should this just be CrashUtil.reportException(ex);
There was a problem hiding this comment.
This improves too thanks to LoginInvalidatedException becoming a RuntimeException. We can just check the main ex exception here and cast it if appropriate (no recursive looping into getCause to search for it). 71fc6e3
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
LGTM, and I'm also curious to see what the Connect team says on the "Personal ID" vs "PersonalID" point, but I don't think that's blocking
…'s a RuntimeException.
|
The last thing to do here is avoid bumping the user out of form entry (good catch @shubham1g5)! I'm looking for a reliable way to detect when the user is in that state, then we won't throw |
https://dimagi.atlassian.net/browse/CCCT-2025
Product Description
If a user attempts to access PersonalID on a device after logging into the same account on a different device, the following will happen:
Technical Summary
This change modifies the existing TokenDeniedException and GlobalErrors handling.
First, TokenDeniedException was renamed to LoginInvalidatedException, and I added an optional GlobalErrors input to provide a reason. The exception gets thrown when the base API callback sees a 400 error with the LOGIN_FROM_DIFFERENT_DEVICE error code.
In a change to how these errors are handled, we now let the exception bubble all the way up to the last-resort CommCareExceptionHandler, where it is caught and handled as described in the user-facing steps above (delete DB, navigate to landing page, show error message). Note that achieving this required the async CommCareTask to catch and rethrow the exception. Also, the handling code in CommCareExceptionHandler checks for the LoginInvalidatedException to be wrapped (possibly multiple times) in other exceptions (i.e. due to the original wrapping in a RuntimeException so calling code doesn't have to handle it explicitly, and possible additional wrapping from the async task rethrow).
I also noticed the translations for the existing generic logout error were missing so I went ahead and added those.
Feature Flag
None
Safety Assurance
Safety story
The server isn't sending the LOGIN_FROM_DIFFERENT_DEVICE error code yet, so for testing I modified the code temporarily to interpret any 400 response on a PersonalID token request to be treated as that error.
In order to speed up testing, I also disabled both HQ and PersonalID token caching (in PersonalIdManager) so the code always requested fresh tokens when attempting relevant API calls.
Test procedure:
Spots tested:
Automated test coverage
None
QA Plan
Once the server functionality is ready, follow the tests described above and verify that in every case the user is logged out of PersonalID and shown the error message about a new device logging into the account.