CCCT1397 - integrity error checks #3233
Conversation
📝 WalkthroughWalkthroughThis set of changes updates the integrity token handling and error management within the application. The Sequence Diagram(s)sequenceDiagram
participant User
participant PersonalIdPhoneFragment
participant IntegrityTokenViewModel
participant StandardIntegrityManager
participant API
User->>PersonalIdPhoneFragment: Initiate integrity token request
PersonalIdPhoneFragment->>IntegrityTokenViewModel: requestIntegrityToken()
IntegrityTokenViewModel->>StandardIntegrityManager: Request Token
StandardIntegrityManager-->>IntegrityTokenViewModel: Return StandardIntegrityToken
IntegrityTokenViewModel-->>PersonalIdPhoneFragment: onTokenReceived(token, hash, StandardIntegrityToken)
PersonalIdPhoneFragment->>API: makeStartConfigurationCall(token, hash, body, StandardIntegrityToken)
API-->>PersonalIdPhoneFragment: Response (may include INTEGRITY_ERROR)
alt INTEGRITY_ERROR
PersonalIdPhoneFragment->>PersonalIdPhoneFragment: handleIntegritySubError(StandardIntegrityToken, subError)
PersonalIdPhoneFragment->>StandardIntegrityToken: showDialog(activity, code)
end
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🧰 Additional context used🧠 Learnings (4)📓 Common learningsapp/src/org/commcare/connect/network/base/BaseApiCallback.kt (1)app/src/org/commcare/connect/network/base/BaseApiHandler.kt (1)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (8)🧬 Code Graph Analysis (1)app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (11)
✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
can we add video showing different resolution pathways and UX there.
| int codeType = switch (subError) { | ||
| case "UNLICENSED_APP_ERROR" -> 1; | ||
| case "APP_INTEGRITY_ERROR" -> 2; | ||
| case "DEVICE_INTEGRITY_ERROR" -> 3; | ||
| default -> 0; |
There was a problem hiding this comment.
@pm-dimagi From where do you get this mapping? Here it has some mention but not able to map the server error with code type.
| } else { | ||
| // User canceled or some issue occurred | ||
| showToastError(R.string.personalid_configuration_process_failed_unexpected_error); |
There was a problem hiding this comment.
@pm-dimagi We should show the device not eligible failure from message fragment?
| }).addOnFailureListener(e -> { | ||
| // Dialog failed to launch or some error occurred | ||
| showToastError(R.string.personalid_configuration_process_failed_unexpected_error); |
There was a problem hiding this comment.
@pm-dimagi Same, we should show the device not eligible failure from message fragment?
| HashMap<String, String> body) { | ||
| HashMap<String, String> body, | ||
| StandardIntegrityManager.@NotNull StandardIntegrityToken integrityTokenResponse) { | ||
| handleIntegritySubError(integrityTokenResponse,"UNLICENSED_APP_ERROR"); |
There was a problem hiding this comment.
@pm-dimagi I think you have used to test the flow, please remove it.
|
@Jignesh-dimagi @shubham1g5 i am moaking more changes to this as got addition info on this code types from zandre.. making this PR to draft for a time being |
|
|
||
| interface IntegrityTokenCallback { | ||
| fun onTokenReceived(token: String, requestHash: String) | ||
| fun onTokenReceived(token: String, requestHash: String, integrityTokeResponse: StandardIntegrityManager.StandardIntegrityToken) |
There was a problem hiding this comment.
no need to send the token separately if you are seding the whole response and implementations can get token directly from the response instead.
| Task<Integer> integrityDialogResponseCode = tokenResponse.showDialog(requireActivity(), 1); | ||
| integrityDialogResponseCode.addOnSuccessListener(result -> { | ||
| if (result == DIALOG_SUCCESSFUL) { | ||
| // Retry the integrity token check | ||
| enableContinueButton(true); | ||
| } else { | ||
| // User canceled or some issue occurred | ||
| callOnConfigurationFailure(); | ||
| } | ||
| }).addOnFailureListener(e -> { | ||
| // Dialog failed to launch or some error occurred | ||
| callOnConfigurationFailure(); | ||
| }); |
There was a problem hiding this comment.
looks like this should not wait for the startConfiguration call but we should be able to parse the tokenResponse even before we make the startConfiguration call.
There was a problem hiding this comment.
We get the error codes in the configuartion api only according to this doc and we cannot parse the tokenresponse at android level server can only do that
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
Outdated
Show resolved
Hide resolved
- handle failure subcodes
|
@shubham1g5 just need a suggestion in above commit i have passed session data in BaseApiCallback which is making it tightly coupled , instead a suggestion can we send failure code in baseApiHandler.onFailure( |
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Just a typo to fix
|
|
||
| interface IntegrityTokenCallback { | ||
| fun onTokenReceived(token: String, requestHash: String) | ||
| fun onTokenReceived(requestHash: String, integrityTokeResponse: StandardIntegrityManager.StandardIntegrityToken) |
There was a problem hiding this comment.
Check spelling, missing an 'n' in integrityTokeResponse
| } | ||
| }).addOnFailureListener(e -> { | ||
| // Dialog failed to launch or some error occurred | ||
| Logger.log(LogTypes.TYPE_MAINTENANCE, "Integrity Dialog Fialed to launch " + e.getMessage()); |
| } else if (errorCode.equals("INTEGRITY_ERROR", ignoreCase = true)) { | ||
| if (json.has("sub_code")) { | ||
| Logger.log(LogTypes.TYPE_MAINTENANCE, "Integrity error with subcode " + json.optString("sub_code")) | ||
| sessionData?.sessionFailureSubcode = json.optString("sub_code") |
There was a problem hiding this comment.
can sessionData be null here ?
There was a problem hiding this comment.
yes earlier it can be due to credentials api which dosent need session data but now this field is removed
| * Returns true if the error was handled, otherwise false. | ||
| */ | ||
| private fun handleErrorCodeIfPresent(errorResponse: InputStream?): Boolean { | ||
| private fun handleErrorCodeIfPresent(errorResponse: InputStream?, sessionData: PersonalIdSessionData?): Boolean { |
There was a problem hiding this comment.
agree on not using sessionData in base callback but we should instead override this in createCallback and do all error code processing there instead.
| break; | ||
|
|
||
| case INTEGRITY_ERROR: | ||
| handleIntegritySubError(integrityTokenResponse, Objects.requireNonNull( |
There was a problem hiding this comment.
- do we always get a subcode in integrity error from server ?
- Also the requireNonNull call should be inside the
handleIntegritySubErrorinstead.
There was a problem hiding this comment.
If there is integrity error there will always be a subcode only in error code INTEGRITY_SERVICE_UNAVAILABLE sub code will not be available
| getString(R.string.personalid_configuration_process_failed_subtitle) | ||
| ); | ||
| } else { | ||
| Task<Integer> integrityDialogResponseCode = tokenResponse.showDialog(requireActivity(), 1); |
There was a problem hiding this comment.
this should only happen when ther errors are of type listed in here and nothing else. I will reverse the if/else logic here to only make the call when the error code is of type where we can do a resolution and otherwise default to calling onConfigurationFailure
There was a problem hiding this comment.
The logic what i take here is INTEGRITY_ERROR code will always have teh sub error which are as follows : UNLICENSED_APP_ERROR,APP_INTEGRITY_ERROR, INTEGRITY_REQUEST_ERROR, DEVICE_INTEGRITY_ERROR
for first three we have to show the dialog type 1 because there is mismatch in the package name so it would allow the user to download the app from the play store ... and in only 1 case DEVICE_INTEGRITY we have to show onConfigurationFailure thats why i have put reverse condition
There was a problem hiding this comment.
for first three we have to show the dialog type 1 because there is mismatch in the package name
What makes you say they are all due to mismatch in package name ?
Also we should add dialog type 2 and 3 as well from here - https://developer.android.com/google/play/integrity/remediation and have server send that info if it's not already.
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdPhoneFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java
Outdated
Show resolved
Hide resolved
| * Checks for "error_code" in the API error response and handles known cases. | ||
| * Returns true if the error was handled, otherwise false. | ||
| */ | ||
| private fun handleErrorCodeIfPresent(errorResponse: InputStream?): Boolean { |
There was a problem hiding this comment.
@pm-dimagi Ideally, we need this function asBaseApiCallback is used by personalId configuration api call, PersonalId other api calls, and connect api calls. We might need to grab sub_code in any of these calls. Instead we need some mechanism to pass the sub_code to calling fragment/activity, which will be of great help going forward for many api calls which might be requiring sub_code
The easiest way to handle sub_code I can see is that declare
var subErrorparams = mapOf<String,Any>() in PersonalIdOrConnectApiErrorCodes
Use this in BaseApiCallback like
val integrityError= PersonalIdOrConnectApiErrorCodes.INTEGRITY_ERROR
integrityError.subErrorparams = mapOf("sub_code" to "UNLICENSED_APP_ERROR")
return onFailure(PersonalIdOrConnectApiErrorCodes.INTEGRITY_ERROR, null);
In calling Fragment, check for PersonalIdOrConnectApiErrorCodes.INTEGRITY_ERROR, and its subErrorparams.
For this case, In PersonalIdPhoneFragment fragment and onFailure method of it can have access this using failureCode.subErrorparams
This method has currently limitation if 2 calls are made simultaneously and both same error, their sub codes can mix. But I guess that possibility is very rare to come.
@shubham1g5 @OrangeAndGreen any thoughts?
There was a problem hiding this comment.
@Jignesh-dimagi I am not sure I understand, why would we check for integrity errors in a general purpose callback ?
There was a problem hiding this comment.
@shubham1g5 so that personalId configuration apis, personalId apis and connect apis can use the concept of sub_code for all future work.
There was a problem hiding this comment.
yeah that concept makes sense to me, just not sure on what code change you are suggesting to maintain that. Are you suggesting that we maintain knowledge of all possible error codes across all APIs in BaseApiCallback ?
There was a problem hiding this comment.
It's actually map object which we are returning, so yeah as and when required, we should keep adding the error codes in BaseApiCallback. This will make this code single source of truth for error code instead of defining at each ApiHandler.
There was a problem hiding this comment.
Hmm lets revisit it later if required, but I think the error codes across personal ID and Connect would be quite different to make sense to maintain them together.
There was a problem hiding this comment.
@shubham1g5 Agree and correct but we can keep adding knowledge for error_code, fragment/activity can use whichever is required on their side.
| } else { | ||
| return false; | ||
| } |
Product Description
https://dimagi.atlassian.net/browse/CCCT-1397
To show the proper dialogs based out of integrity error checks send by the server based on these subcodes
Screen_Recording_20250708_170002_CommCare.Debug.mp4
Screen_Recording_20250708_170227_CommCare.Debug.mp4
UNLICENSED_APP_ERROR
APP_INTEGRITY_ERROR
INTEGRITY_REQUEST_ERROR
these error will redirect to the play store to find the exact matched apps
DEVICE_INTEGRITY_ERROR will redirect to the message fragment as device is not comply the checks
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review