Skip to content

Conversation

@OrangeAndGreen
Copy link
Contributor

Product Description

No user-facing changes

Technical Summary

This code supports HQ-driven integrity reports sent to the PersonalID server. When HQ includes a new flag in the heartbeat response, mobile will perform an integrity check and send the token to PersonalID to be evaluated. A result code will be returned by PersonalID, and mobile will send an analytics event to record the integrity report.

Changes:
-Detect new flag in HQ heartbeat response
-Trigger new IntegrityReporter on a separate thread
-Retrieve an integrity token and send it to PersonalID (new API endpoint)
-Record analytics event with the results

Feature Flag

PersonalID

Safety Assurance

Safety story

Manually triggered the event to see the code work (since the HQ functionality to make the request doesn't exist yet. I did this by calling IntegrityReporter.launch and passing in a fake "TEST" GUID. Everything worked as expected up to the API call, which failed since the API endpoint doesn't exist on PersonalID yet.

Automated test coverage

None

QA Plan

No way for QA to test this yet, pending HQ and PersonalID functionality.

Created IntegrityReporter.
Added code for new PersonalID report_integrity API call.
Added code for detecting requested integrity check in HeartbeatRequester.
@coderabbitai
Copy link

coderabbitai bot commented Jul 11, 2025

📝 Walkthrough

Walkthrough

This change introduces a new integrity reporting workflow in the application. A new IntegrityReporter background worker is added to handle integrity report requests, which are triggered by the server via the heartbeat mechanism. The workflow involves fetching an integrity token, constructing a request with device and request IDs, and submitting it to a new /users/report_integrity API endpoint. Supporting changes include new API methods and endpoints, response parsing, analytics event reporting, and a mechanism to avoid duplicate processing using shared preferences. The integrity report result code is now stored in the session data model for further use or analytics.

Sequence Diagram(s)

sequenceDiagram
    participant HeartbeatRequester
    participant IntegrityReporter
    participant IntegrityTokenApiRequestHelper
    participant PersonalIdApiHandler
    participant ApiPersonalId
    participant ApiService
    participant FirebaseAnalyticsUtil
    participant SharedPreferences

    HeartbeatRequester->>IntegrityReporter: launch(context, requestId)
    IntegrityReporter->>SharedPreferences: check for duplicate requestId
    alt Not duplicate
        IntegrityReporter->>IntegrityTokenApiRequestHelper: fetchIntegrityToken(requestHash)
        IntegrityTokenApiRequestHelper-->>IntegrityReporter: integrityToken
        IntegrityReporter->>PersonalIdApiHandler: makeIntegrityReportCall(context, body, integrityToken, requestHash)
        PersonalIdApiHandler->>ApiPersonalId: reportIntegrity(context, body, integrityToken, requestHash, callback)
        ApiPersonalId->>ApiService: reportIntegrity(headers, body)
        ApiService-->>ApiPersonalId: response (with result_code)
        ApiPersonalId-->>PersonalIdApiHandler: callback(response)
        PersonalIdApiHandler-->>IntegrityReporter: result_code
        IntegrityReporter->>FirebaseAnalyticsUtil: reportPersonalIdIntegritySubmission(requestId, result_code)
        IntegrityReporter->>SharedPreferences: store requestId as processed
    else Duplicate
        IntegrityReporter-->>HeartbeatRequester: Skip processing
    end
Loading

Suggested labels

skip-integration-tests

Suggested reviewers

  • pm-dimagi
  • shubham1g5
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dv/integrity_report

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai auto-generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ac18b48 and ac342cf.

📒 Files selected for processing (13)
  • app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt (1 hunks)
  • app/src/org/commcare/android/integrity/IntegrityReporter.kt (1 hunks)
  • app/src/org/commcare/android/integrity/IntegrityTokenApiRequestHelper.kt (3 hunks)
  • app/src/org/commcare/connect/network/ApiEndPoints.java (1 hunks)
  • app/src/org/commcare/connect/network/ApiPersonalId.java (1 hunks)
  • app/src/org/commcare/connect/network/ApiService.java (1 hunks)
  • app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (2 hunks)
  • app/src/org/commcare/connect/network/connectId/parser/ReportIntegrityResponseParser.java (1 hunks)
  • app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java (1 hunks)
  • app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (1 hunks)
  • app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1 hunks)
  • app/src/org/commcare/heartbeat/HeartbeatRequester.java (3 hunks)
  • app/src/org/commcare/preferences/HiddenPreferences.java (1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: shubham1g5
PR: dimagi/commcare-android#0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#0
File: :0-0
Timestamp: 2025-05-08T13:40:19.645Z
Learning: PR #3048 introduces a comprehensive messaging system in the Connect feature, implementing secure encryption using AES-GCM for message content, proper channel management with consent flows, and a well-designed UI separation between sent and received messages with real-time notification integration.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java:95-131
Timestamp: 2025-02-04T21:38:11.970Z
Learning: Biometric authentication security improvements were considered but skipped in PR #2949 as per user's request. The implementation remained with basic biometric authentication without additional security layers.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3108
File: app/src/org/commcare/activities/StandardHomeActivityUIController.java:0-0
Timestamp: 2025-06-20T13:21:20.908Z
Learning: User OrangeAndGreen prefers to handle code issues in separate PRs rather than immediately fixing them in the current PR when they are not directly related to the main changes.
app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3037
File: app/src/org/commcare/connect/ConnectConstants.java:11-15
Timestamp: 2025-04-21T18:48:08.330Z
Learning: Request codes used for startActivityForResult should be unique throughout the application, even if they're used in different activities. COMMCARE_SETUP_CONNECT_LAUNCH_REQUEST_CODE and STANDARD_HOME_CONNECT_LAUNCH_REQUEST_CODE should have different values.
app/src/org/commcare/connect/network/ApiEndPoints.java (1)
Learnt from: pm-dimagi
PR: dimagi/commcare-android#3133
File: app/src/org/commcare/connect/network/connectId/ApiService.java:55-55
Timestamp: 2025-05-28T11:30:37.998Z
Learning: In the CommCare Android codebase, API service method names in ApiService.java should match the format of the actual API endpoint names rather than using semantically meaningful names. For example, if the endpoint is "users/set_recovery_pin", the method name should follow that endpoint structure for consistency and maintainability.
app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt (3)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:66-71
Timestamp: 2025-01-21T17:28:09.007Z
Learning: The ConnectUserRecord class in CommCare Android uses @Persisting annotations with sequential indices for field persistence, and @MetaField annotations for fields that have corresponding META constants. Fields should be documented with Javadoc comments explaining their purpose, format requirements, and relationships with other fields.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2927
File: app/src/org/commcare/activities/connect/HQUserInviteActivity.java:0-0
Timestamp: 2025-02-04T15:29:12.993Z
Learning: In HQUserInviteActivity, parameters from Intent data (domain, inviteCode, username, etc.) are required and the code is designed to fail immediately if they're missing, rather than adding null checks. This helps catch integration issues early.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:214-215
Timestamp: 2025-01-21T18:19:05.799Z
Learning: In ConnectIdPasswordVerificationFragment, when creating a ConnectUserRecord, it's acceptable for payment information (paymentName and paymentPhone) to be empty strings if the server response doesn't include payment info in the CONNECT_PAYMENT_INFO field.
app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3037
File: app/src/org/commcare/connect/ConnectConstants.java:11-15
Timestamp: 2025-04-21T18:48:08.330Z
Learning: Request codes used for startActivityForResult should be unique throughout the application, even if they're used in different activities. COMMCARE_SETUP_CONNECT_LAUNCH_REQUEST_CODE and STANDARD_HOME_CONNECT_LAUNCH_REQUEST_CODE should have different values.
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (3)
Learnt from: shubham1g5
PR: dimagi/commcare-android#0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:206-213
Timestamp: 2025-01-21T18:20:43.883Z
Learning: In the Connect ID feature, server responses containing user data should be parsed using ConnectUserResponseParser to ensure consistent handling of payment information (owner_name and phone_number) and secondary phone verification across all authentication flows.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java:244-275
Timestamp: 2025-02-04T21:22:56.537Z
Learning: Direct JSONObject parsing is acceptable for handling user data responses in ConnectIdPinFragment, as decided by the team. No need to enforce ConnectUserResponseParser usage in this context.
app/src/org/commcare/connect/network/ApiService.java (3)
Learnt from: pm-dimagi
PR: dimagi/commcare-android#3133
File: app/src/org/commcare/connect/network/connectId/ApiService.java:55-55
Timestamp: 2025-05-28T11:30:37.998Z
Learning: In the CommCare Android codebase, API service method names in ApiService.java should match the format of the actual API endpoint names rather than using semantically meaningful names. For example, if the endpoint is "users/set_recovery_pin", the method name should follow that endpoint structure for consistency and maintainability.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/connect/network/connectId/ApiService.java:8-63
Timestamp: 2025-01-28T09:38:59.882Z
Learning: The ConnectID API service methods should use Map<String, String> for request bodies and ResponseBody for responses, as per team preference.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/connect/network/connectId/ApiService.java:10-62
Timestamp: 2025-01-28T10:48:49.236Z
Learning: In the ConnectID API service, Map<String, String> is intentionally used for request bodies and ResponseBody for responses instead of typed models.
app/src/org/commcare/heartbeat/HeartbeatRequester.java (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3040
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java:39-55
Timestamp: 2025-04-18T20:13:29.655Z
Learning: In the CommCare Android Connect feature, the JSON object passed to `ConnectJobDeliveryFlagRecord.fromJson()` method should never be null based on the implementation design.
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (2)
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2847
File: app/src/org/commcare/utils/EncryptionUtils.java:167-167
Timestamp: 2025-01-27T15:14:56.422Z
Learning: MD5 hashing is acceptable for non-security purposes like analytics tracking in FirebaseAnalyticsUtil.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3037
File: app/src/org/commcare/connect/ConnectConstants.java:11-15
Timestamp: 2025-04-21T18:48:08.330Z
Learning: Request codes used for startActivityForResult should be unique throughout the application, even if they're used in different activities. COMMCARE_SETUP_CONNECT_LAUNCH_REQUEST_CODE and STANDARD_HOME_CONNECT_LAUNCH_REQUEST_CODE should have different values.
app/src/org/commcare/connect/network/connectId/parser/ReportIntegrityResponseParser.java (4)
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java:244-275
Timestamp: 2025-02-04T21:22:56.537Z
Learning: Direct JSONObject parsing is acceptable for handling user data responses in ConnectIdPinFragment, as decided by the team. No need to enforce ConnectUserResponseParser usage in this context.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:206-213
Timestamp: 2025-01-21T18:20:43.883Z
Learning: In the Connect ID feature, server responses containing user data should be parsed using ConnectUserResponseParser to ensure consistent handling of payment information (owner_name and phone_number) and secondary phone verification across all authentication flows.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3108
File: app/src/org/commcare/connect/network/PersonalIdApiHandler.java:51-56
Timestamp: 2025-06-13T15:53:12.951Z
Learning: For `PersonalIdApiHandler`, the team’s convention is to propagate `JSONException` as an unchecked `RuntimeException` so the app crashes, signalling a contract/implementation bug rather than attempting a graceful retry.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3040
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java:39-55
Timestamp: 2025-04-18T20:13:29.655Z
Learning: In the CommCare Android Connect feature, the JSON object passed to `ConnectJobDeliveryFlagRecord.fromJson()` method should never be null based on the implementation design.
app/src/org/commcare/connect/network/ApiPersonalId.java (1)
Learnt from: pm-dimagi
PR: dimagi/commcare-android#3133
File: app/src/org/commcare/connect/network/connectId/ApiService.java:55-55
Timestamp: 2025-05-28T11:30:37.998Z
Learning: In the CommCare Android codebase, API service method names in ApiService.java should match the format of the actual API endpoint names rather than using semantically meaningful names. For example, if the endpoint is "users/set_recovery_pin", the method name should follow that endpoint structure for consistency and maintainability.
🧬 Code Graph Analysis (3)
app/src/org/commcare/connect/network/ApiService.java (1)
app/src/org/commcare/connect/network/ApiEndPoints.java (1)
  • ApiEndPoints (3-23)
app/src/org/commcare/heartbeat/HeartbeatRequester.java (2)
app/src/org/commcare/android/integrity/IntegrityReporter.kt (1)
  • launch (26-40)
app/src/org/commcare/CommCareApplication.java (1)
  • CommCareApplication (154-1246)
app/src/org/commcare/connect/network/ApiPersonalId.java (2)
app/src/org/commcare/connect/network/connectId/PersonalIdApiClient.java (1)
  • PersonalIdApiClient (10-30)
app/src/org/commcare/connect/network/ApiEndPoints.java (1)
  • ApiEndPoints (3-23)
🔇 Additional comments (17)
app/src/org/commcare/preferences/HiddenPreferences.java (1)

123-123: LGTM!

The new preference key follows the established naming conventions and is properly integrated with the existing constants.

app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (1)

27-28: LGTM!

The new analytics parameter constants follow the established naming patterns and are consistent with the existing codebase.

app/src/org/commcare/android/integrity/IntegrityTokenApiRequestHelper.kt (2)

22-22: Good refactoring to support background operations.

Making lifecycleOwner nullable and conditionally observing the provider state enables this helper to be used in background workers where no lifecycle owner is available. The lambda syntax is also cleaner and more idiomatic.

Also applies to: 32-48


121-171: Well-implemented coroutine support for background operations.

The suspend function is properly implemented with:

  • Correct use of suspendCancellableCoroutine
  • Proper handling of all provider states
  • Memory leak prevention by removing observers on cancellation
  • Result type for clean error handling

This provides a clean API for the IntegrityReporter worker to fetch tokens asynchronously.

app/src/org/commcare/connect/network/ApiEndPoints.java (1)

9-9: LGTM!

The new endpoint follows the established naming conventions and RESTful path structure consistent with other user endpoints.

app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java (1)

38-38: LGTM!

The new analytics event follows the established naming pattern for PersonalID events and is properly grouped with related events.

app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt (1)

25-25: LGTM! Clean addition of resultCode property.

The nullable resultCode property is appropriately added to capture the integrity report response code. The documentation clearly explains its purpose and it follows the established pattern of other nullable properties in the data class.

app/src/org/commcare/connect/network/ApiService.java (1)

11-14: LGTM! Method follows established API service patterns.

The reportIntegrity method correctly follows the team's established conventions:

  • Uses Map<String, String> for request body and ResponseBody for response as per team preference
  • Header names follow clear naming conventions
  • Method signature is consistent with other API methods in the interface
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)

402-407: LGTM! Analytics method follows established patterns.

The reportPersonalIdIntegritySubmission method correctly follows the established analytics reporting pattern:

  • Creates a Bundle with appropriate parameter keys
  • Uses descriptive parameter names consistent with the analytics constants
  • Calls the standard reportEvent method for consistent handling
app/src/org/commcare/heartbeat/HeartbeatRequester.java (3)

29-29: LGTM! Clean import addition.

The import for IntegrityReporter is appropriately added to support the new integrity reporting functionality.


100-100: LGTM! Proper integration with heartbeat response flow.

The call to checkForIntegrityRequest is appropriately placed after the existing response checks, maintaining the logical flow of the parseResponse method.


121-126: LGTM! Well-implemented integrity request detection.

The checkForIntegrityRequest method follows the established pattern used by other response checks in this class:

  • Uses optString with a safe default value
  • Properly launches the IntegrityReporter with the application context
  • Maintains consistency with the existing codebase patterns
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (2)

17-17: LGTM! Appropriate import addition.

The import for ReportIntegrityResponseParser is correctly added to support the new integrity reporting functionality.


55-62: LGTM! Method follows established API handler patterns.

The makeIntegrityReportCall method correctly follows the established patterns used by other methods in this class:

  • Creates a new PersonalIdSessionData instance for session state
  • Uses the appropriate ApiPersonalId method for the API call
  • Properly utilizes createCallback with the corresponding response parser
  • Uses Context instead of Activity, which is appropriate for background operations

The method signature is well-designed and consistent with the broader API handler architecture.

app/src/org/commcare/connect/network/ApiPersonalId.java (1)

272-277: Implementation follows established patterns correctly.

The new reportIntegrity method is consistent with other API methods in this class and properly delegates to the Retrofit service.

app/src/org/commcare/connect/network/connectId/parser/ReportIntegrityResponseParser.java (1)

7-14: Parser implementation is robust and handles missing fields gracefully.

Good use of optString with a fallback value to handle cases where the server doesn't return a result code.

app/src/org/commcare/android/integrity/IntegrityReporter.kt (1)

77-96: Well-implemented coroutine bridge for callback-based API.

Good use of suspendCoroutine to integrate the callback-based PersonalIdApiHandler with coroutines. The null safety handling with fallback values ensures robust error reporting.

Comment on lines 48 to 52
val storedId = preferences.getString(HiddenPreferences.INTEGRITY_REQUEST_ID, null)
if (storedId == requestId) {
// Already processed this request
return Result.success()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: If a user is logged into 2 apps with same username, will they get same request ids here or different ? If different, we might need to better support multiple request ids here. As per this code, users who get multiple request ids due to frequently switching apps will keep reporting integrity checks as they switch between the 2 apps and hence 2 request ids.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could check with the server team, but my guess is that two different HQ mobile workers on the same device could be requested to perform checks and would probably get different IDs. I'm thinking maybe it would be better to use the SharedPreferences for the current app to store the value instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want only one check across apps so global prefs does make sense for me here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But doesn't that create the issue you described above, where a user logging into two apps that were both requested to perform integrity checks will keep overwriting the value in global shared prefs and therefore keep repeating both integrity checks?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably instead store the pref as a boolean per guid and avoid it - reported_integrity_check_$guid:true, but fine to ignore that if server can send the guid consistently across apps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a chat with Clayton about his intentions here and he clarified that each request ID should be stored separately using unique keys in SharedPrefs, so that we don't need to worry about only remembering the most recently processed request ID.
5a5c8bf

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think this pref should store a boolean now on whether the check is complete or not

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

val requestHash = org.commcare.utils.HashUtils.computeHash(jsonBody, org.commcare.utils.HashUtils.HashAlgorithm.SHA256)
val tokenResult = fetchIntegrityToken(requestHash)
val (integrityToken, hash) = tokenResult.getOrElse {
makeReportIntegrityCall(context, it.message, "ERROR", body, requestId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • why do we want to make the APi call for failures ? And if so, we should be clear that we were not able to get the token by passing it as null instead of a random message. If we want to track the error on server side, we should be passing the exception type as well to the server.

  • Also we should store this in Analytics with ExceptionType as the failure as well and mark the integrity test as done in preferences.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the goal here is to make sure the PersonalID server is aware of the outcome of the integrity check even when that outcome is that the device couldn't retrieve a token. Passing details about the exception makes sense although somehow we need the server to easily be able to tell that we're sending a failure message instead of an integrity token. My thought was that using a single error string like "ERROR" would make that an easy check for the server. But maybe instead we should put it as a different field in the payload, i.e. leave the CC-Integrity-Token and hash headers blank and instead include a device_error field in the payload for these scenarios. Your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I think I would prefer using a separate filed instead of using token for it so that server is clear on the API contract here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fbbe947

Looks like all we get back when the device fails to retrieve the integrity token is a "message"? Including that in both the API call and analytics

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OrangeAndGreen the exception type here is more helpful than the message itself. Look at the code handling here for example , think we should pass a separate code to the server in respect to each of these errors ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@avazirna avazirna added this to the 2.58 milestone Jul 14, 2025
}

val preferences = PreferenceManager.getDefaultSharedPreferences(context)
val storedId = preferences.getString(getIntegrityRequestIdKey(requestId), null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not a string anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +84 to +86
preferences.edit {
putBoolean(getIntegrityRequestIdKey(requestId), true)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing .apply() call to commit the edit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Kotlin extension for edit() handles applying the changes automatically, more info at the top of this article

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooo, that's great

public static final String USER_DOMAIN_SERVER_URL_SUFFIX = ".commcarehq.org";
private static final String INTERRUPTED_FORM_INDEX = "interrupted-form-index";

public final static String INTEGRITY_REQUEST_ID = "integrity-request-id";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably remove it from here as we are not using this anywhere in this class unlike other prefs defined in the class. Alternative would be to expose all access to this pred through edit/get methods in this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OrangeAndGreen OrangeAndGreen requested a review from avazirna July 15, 2025 19:07
@shubham1g5 shubham1g5 merged commit db8ae12 into master Jul 16, 2025
4 of 9 checks passed
@shubham1g5 shubham1g5 deleted the dv/integrity_report branch July 16, 2025 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants