Additional logging for failed HQ token auth in ConnectID-managed apps. #2984
Conversation
OrangeAndGreen
requested review from
Jignesh-dimagi,
avazirna,
pm-dimagi and
shubham1g5
📝 WalkthroughWalkthroughThis pull request revises the error handling workflow for authentication failures related to ConnectID managed applications. In the Sequence Diagram(s)Authentication Failure Handling in LoginActivity and SyncCapableCommCareActivitysequenceDiagram
participant User
participant Activity
participant ConnectManager
participant Logger
User->>Activity: Initiate Login
Activity->>Activity: Execute handlePullTaskResult
Activity->>ConnectManager: checkForFailedConnectIdAuth(seatedAppId, username)
ConnectManager-->>Activity: Boolean result
alt Token auth failure detected
Activity->>Logger: Log exception ("token authentication failed for ConnectID managed app")
else
Activity->>Activity: Proceed with standard bad credentials handling
end
Authentication Flow in CommcareRequestGeneratorsequenceDiagram
participant Component
participant RequestGenerator
participant ConnectManager
participant Logger
Component->>RequestGenerator: buildAuth() call
RequestGenerator->>ConnectManager: checkForFailedConnectIdAuth(seatedAppId, username)
ConnectManager-->>RequestGenerator: Boolean result
alt Token auth failure detected
RequestGenerator->>Logger: Log exception ("token authentication error for connect-managed app")
else
RequestGenerator->>Component: Return user session authentication
end
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🔇 Additional comments (5)
✨ Finishing Touches
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:
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 (
|
| case AUTH_FAILED: | ||
| String seatedAppId = CommCareApplication.instance().getCurrentApp().getUniqueId(); | ||
| String username = CommCareApplication.instance().getRecordForCurrentUser().getUsername(); | ||
| ConnectManager.forgetAppCredentials(seatedAppId, username); |
There was a problem hiding this comment.
Yes, getting rid of that on purpose. It was added with ConnectID-managed traditional apps in mind. But now that we know the token auth could fail for other reasons, we don't want to take the unnecessary step of forcing the app credentials to be setup again. The improved messaging is a better reaction to this error.
| break; | ||
| case AUTH_FAILED: | ||
| String seatedAppId = CommCareApplication.instance().getCurrentApp().getUniqueId(); | ||
| if(ConnectManager.checkForFailedConnectIdAuth(seatedAppId, uiController.getEnteredUsername())) { |
There was a problem hiding this comment.
we can move the seatedAppId eval in checkForFailedConnectIdAuth itself
https://dimagi.atlassian.net/browse/CCCT-668
cross-request: dimagi/commcare-core#1455
Technical Summary
Some additional logging to help capture the failed token auths related to ConnectID-managed apps.
We're already logging the case where we're unable to get an HQ token just before making an HQ call.
But currently we still make the call anyway, which is guaranteed to fail.
Even once that's fixed, we still may get surprise failures due to tokens we thought were good actually not being good.
So this PR adds logging after HQ calls fail (key record or data pull requests) due to bad authorization when they should be managed by ConnectID via tokens.
Feature Flag
ConnectID
Safety Assurance
Safety story
Simple addition that silently logs a condition, and checks happen in a try-catch in case something goes wrong.
Automated test coverage
No automated tests for ConnectID yet
QA Plan
No extra QA plan necessary, this is for dev tracking.
Labels and Review