-
-
Notifications
You must be signed in to change notification settings - Fork 45
Perform Integrity check and report when prompted by HQ #3240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Created IntegrityReporter. Added code for new PersonalID report_integrity API call. Added code for detecting requested integrity check in HeartbeatRequester.
📝 WalkthroughWalkthroughThis change introduces a new integrity reporting workflow in the application. A new 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
Suggested labels
Suggested reviewers
✨ 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 (
|
There was a problem hiding this 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
📒 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
lifecycleOwnernullable 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
IntegrityReporterworker 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
resultCodeproperty 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
reportIntegritymethod correctly follows the team's established conventions:
- Uses
Map<String, String>for request body andResponseBodyfor 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
reportPersonalIdIntegritySubmissionmethod 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
reportEventmethod for consistent handlingapp/src/org/commcare/heartbeat/HeartbeatRequester.java (3)
29-29: LGTM! Clean import addition.The import for
IntegrityReporteris appropriately added to support the new integrity reporting functionality.
100-100: LGTM! Proper integration with heartbeat response flow.The call to
checkForIntegrityRequestis appropriately placed after the existing response checks, maintaining the logical flow of theparseResponsemethod.
121-126: LGTM! Well-implemented integrity request detection.The
checkForIntegrityRequestmethod follows the established pattern used by other response checks in this class:
- Uses
optStringwith a safe default value- Properly launches the
IntegrityReporterwith 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
ReportIntegrityResponseParseris correctly added to support the new integrity reporting functionality.
55-62: LGTM! Method follows established API handler patterns.The
makeIntegrityReportCallmethod correctly follows the established patterns used by other methods in this class:
- Creates a new
PersonalIdSessionDatainstance for session state- Uses the appropriate
ApiPersonalIdmethod for the API call- Properly utilizes
createCallbackwith the corresponding response parser- Uses
Contextinstead ofActivity, which is appropriate for background operationsThe 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
reportIntegritymethod 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
optStringwith 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
suspendCoroutineto integrate the callback-based PersonalIdApiHandler with coroutines. The null safety handling with fallback values ensures robust error reporting.
app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.kt
Outdated
Show resolved
Hide resolved
app/src/org/commcare/connect/network/connectId/parser/ReportIntegrityResponseParser.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java
Outdated
Show resolved
Hide resolved
| val storedId = preferences.getString(HiddenPreferences.INTEGRITY_REQUEST_ID, null) | ||
| if (storedId == requestId) { | ||
| // Already processed this request | ||
| return Result.success() | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
nullinstead 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
ExceptionTypeas the failure as well and mark the integrity test as done in preferences.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app/src/org/commcare/android/integrity/IntegrityTokenApiRequestHelper.kt
Outdated
Show resolved
Hide resolved
app/src/org/commcare/android/integrity/IntegrityTokenApiRequestHelper.kt
Outdated
Show resolved
Hide resolved
…rm the logic anyway).
…ve on device. Recording analytics event for same.
…are-android into dv/integrity_report
…ach ID is stored separately).
…eating the request ID)
…nto dv/integrity_report
| } | ||
|
|
||
| val preferences = PreferenceManager.getDefaultSharedPreferences(context) | ||
| val storedId = preferences.getString(getIntegrityRequestIdKey(requestId), null) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| preferences.edit { | ||
| putBoolean(getIntegrityRequestIdKey(requestId), true) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.