Delete SSO token storage when token auth fails #2958
Conversation
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
The code here looks good to me but I wonder if we should trigger the process to get a new token when we get a auth error ? Correct me if I am wrong but it seems like the request would just fail without us retrying to do the request with a new token ?
📝 WalkthroughWalkthroughThis PR removes several unused import statements across multiple classes in the Connect module, including those related to Sequence Diagram(s)sequenceDiagram
participant Client
participant RequestGenerator
participant ExternalService
participant SsoHelper
Client->>RequestGenerator: Send HTTP request with AuthInfo
RequestGenerator->>ExternalService: Forward request
ExternalService-->>RequestGenerator: Return response (e.g., 200-299 or 401)
alt Response is 401 and using TokenAuth
RequestGenerator->>SsoHelper: discardTokens(username)
SsoHelper-->>RequestGenerator: Tokens cleared
RequestGenerator-->>Client: Return error response
else Successful response
RequestGenerator-->>Client: Return valid response
end
sequenceDiagram
participant App
participant NetworkHelper
participant ExternalService
participant SsoHelper
App->>NetworkHelper: Call getInternal/postInternal (with token auth)
NetworkHelper->>ExternalService: Send network request
ExternalService-->>NetworkHelper: Return response code
alt Using TokenAuth and response is 401
NetworkHelper->>SsoHelper: discardTokens(username)
SsoHelper-->>NetworkHelper: Acknowledge token clearance
NetworkHelper-->>App: Return error response
else
NetworkHelper-->>App: Return successful response
end
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
app/src/org/commcare/android/database/connect/models/ConnectJobAssessmentRecord.java(0 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecord.java(0 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobLearningRecord.java(0 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobPaymentRecord.java(0 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobPaymentRecordV3.java(0 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java(0 hunks)app/src/org/commcare/android/database/connect/models/ConnectLinkedAppRecord.java(1 hunks)app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java(1 hunks)app/src/org/commcare/connect/ConnectManager.java(0 hunks)app/src/org/commcare/connect/network/ApiConnectId.java(2 hunks)app/src/org/commcare/connect/network/ConnectNetworkHelper.java(3 hunks)app/src/org/commcare/connect/network/ConnectSsoHelper.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java(0 hunks)app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java(0 hunks)app/src/org/commcare/network/CommcareRequestGenerator.java(6 hunks)
💤 Files with no reviewable changes (9)
- app/src/org/commcare/android/database/connect/models/ConnectJobAssessmentRecord.java
- app/src/org/commcare/android/database/connect/models/ConnectJobLearningRecord.java
- app/src/org/commcare/android/database/connect/models/ConnectJobPaymentRecord.java
- app/src/org/commcare/android/database/connect/models/ConnectJobPaymentRecordV3.java
- app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java
- app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java
- app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java
- app/src/org/commcare/connect/ConnectManager.java
- app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecord.java
🧰 Additional context used
🧠 Learnings (1)
app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java (1)
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecordV5.java:72-76
Timestamp: 2025-01-26T14:32:09.266Z
Learning: In ConnectUserRecordV5, initializing lastPasswordDate and connectTokenExpiration to new Date() in the constructor is intentional.
🔇 Additional comments (15)
app/src/org/commcare/android/database/connect/models/ConnectLinkedAppRecord.java (1)
120-123: Good implementation for clearing the HQ token.The method correctly implements token deletion by setting it to null and resetting the expiration date to the current date, effectively marking it as expired. This aligns well with the PR objective of handling token authentication failures.
app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java (1)
197-200: Good implementation for clearing the Connect token.The method follows the same pattern as
clearHqToken()and properly invalidates the token by setting it to null and updating the expiration date. This is consistent with the existing patterns in the codebase, including the initialization behavior in the constructor.app/src/org/commcare/network/CommcareRequestGenerator.java (5)
153-164: Good refactoring to support token error checking.Storing the Auth object in a variable allows it to be used both for the request and for error checking, which is a good pattern.
215-220: Good implementation of token error handling.This method correctly identifies token authentication failures (401 responses with TokenAuth) and invokes the token discard mechanism, which aligns with the PR's objective. The error logging provides good visibility for debugging.
233-244: Good consistency in token error handling.The same pattern of storing the auth info and checking for token errors is applied here, maintaining consistency across the codebase.
287-302: Good consistency in token error handling for multipart requests.The pattern is consistently applied to multipart POST requests as well.
315-325: Good consistency in token error handling for simple GET requests.The pattern is consistently applied to simple GET requests, ensuring comprehensive coverage of token error handling across all network operations.
app/src/org/commcare/connect/network/ApiConnectId.java (2)
80-80: Good improvement to handle a range of success status codes.Expanding the success check from a specific 200 code to the range of 200-299 is a better practice and aligns with HTTP standards.
104-107: Good implementation of token invalidation for 401 responses.This handling of unauthorized responses for ConnectID tokens is consistent with the pattern implemented elsewhere and properly discards invalid tokens.
app/src/org/commcare/connect/network/ConnectSsoHelper.java (2)
7-7: Appropriate import additions.The new imports support the functionality being added for token management.
Also applies to: 12-13
102-120: Solid implementation of token cleanup mechanism.The new
discardTokensmethod properly handles the cleanup of both HQ and Connect tokens when authentication fails. The implementation follows good practices by:
- Logging maintenance activity
- Handling null username cases properly
- Checking for null records before attempting to clear tokens
- Persisting changes back to the database
This implementation directly addresses the PR objective of deleting SSO token storage when token authentication fails.
app/src/org/commcare/connect/network/ConnectNetworkHelper.java (4)
201-202: Good token auth detection in the postInternal method.The change correctly detects when token authentication is being used and passes this information to the response processor.
328-329: Consistent token auth detection in the getInternal method.This change mirrors the implementation in the postInternal method, ensuring consistent handling of token authentication across HTTP methods.
335-336: Appropriate method signature update.The method signature has been updated to include the new
usingTokenAuthparameter, which is essential for the added token handling functionality.
356-360: Robust token invalidation for 401 responses.This implementation correctly:
- Checks for 401 Unauthorized responses
- Verifies that token authentication was used
- Logs the failure appropriately
- Calls the new discardTokens method to clean up invalid tokens
This is the core implementation of the PR objective to handle token authentication failures, particularly in cases of clock errors mentioned in the PR description.
|
Per discussion with Clayton, this seems like something that should happen rarely enough that we can just fail the call and try again later with new tokens rather than trying to support the complex logic of nesting one API call in several more. Although Jignesh mentioned that for Retrofit calls (so to Connect, not HQ) we can configure an interceptor to handle the 401 and automatically attempt to get a new token, so I think that would be another good addition to schedule soon. |
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-666
When an API call fails due to a 401 error while using TokenAuth, we should delete the token from local storage.
This is most likely to occur due to a clock error (drift or time zone mismatch) on the device.
For whatever reason the token failed, it should be discarded so a new token is retrieved the next time an API call is made.
This code addresses token failures for both HQ and ConnectID tokens in several places:
Feature Flag
ConnectID
Safety Assurance
Safety story
Tested by performing a successful HQ API call with an app managed by ConnectID, to get valid ConnectID and HQ tokens.
Then waited for the tokens to expire
Manually set date on the device to the day before so it thought the tokens were still valid
Tried to make a Connect API, observed the 401 call and discarded token, observed retry on the call succeeding (including new token retrieval).
Repeated the test from the beginning (new tokens, then expired), then attempted a managed HQ API call and observed the same results as previous test.
The main idea here is that tokens are pretty cheap, so if a token isn't good when we thought it would be we should just go get a new one and expect things to work at that point.
In normal usage where tokens are managed correctly (no clock errors resulting in using expired tokens), there will be no effect from this new change.
Automated test coverage
No automated tests for ConnectID yet.
QA Plan
QA should attempt similar steps for testing to the ones described above, i.e. manually altering the clock to trick the device into attempting an API call with one or both bad tokens.
Labels and Review