Pm retrofit #2890
Pm retrofit #2890
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive network communication overhaul for the application, focusing on modernizing API interactions. The changes primarily involve integrating Retrofit and OkHttp for more robust and efficient network operations. Three new dependencies are added to the A new network architecture is established with the introduction of several classes: Sequence DiagramsequenceDiagram
participant App
participant ApiService
participant Server
App->>ApiClient: Initialize Retrofit
ApiClient-->>App: Retrofit Instance
App->>ApiService: Make API Request
ApiService->>Server: Send HTTP Request
Server-->>ApiService: Respond with Data
ApiService-->>App: Return Response
The sequence diagram illustrates the high-level flow of network requests using the new Retrofit-based architecture, demonstrating how the application now interacts with the server through a more structured and standardized approach. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (29)
app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragmnet.java (3)
Line range hint
1-1: Fix typo in filename: "Fragmnet" should be "Fragment"The class name and file name contain a typo: "Fragmnet" should be "Fragment". This could cause confusion and should be corrected for better code maintainability.
Rename the file and class to:
-ConnectIdPhoneVerificationFragmnet.java +ConnectIdPhoneVerificationFragment.java
367-369: Remove commented codeThe commented code related to the busy state handling should be removed as it's no longer needed with the new Retrofit implementation.
-// if (isBusy) { -// Toast.makeText(requireActivity(), R.string.busy_message, Toast.LENGTH_SHORT).show(); -// }
457-467: Consider extracting API method selection logicThe switch statements for selecting API methods in both
requestSmsCode()andverifySmsCode()follow similar patterns. Consider extracting this logic into a separate method to improve maintainability and reduce duplication.+private ApiConnectId.ApiMethod getApiMethod(boolean isVerification) { + return switch (method) { + case MethodRecoveryPrimary -> isVerification ? + ApiConnectId.ApiMethod.CONFIRM_RECOVERY_PRIMARY : + ApiConnectId.ApiMethod.REQUEST_RECOVERY_PRIMARY; + case MethodRecoveryAlternate -> isVerification ? + ApiConnectId.ApiMethod.CONFIRM_RECOVERY_SECONDARY : + ApiConnectId.ApiMethod.REQUEST_RECOVERY_SECONDARY; + case MethodVerifyAlternate -> isVerification ? + ApiConnectId.ApiMethod.CONFIRM_VERIFICATION_SECONDARY : + ApiConnectId.ApiMethod.REQUEST_VERIFICATION_SECONDARY; + default -> isVerification ? + ApiConnectId.ApiMethod.CONFIRM_REGISTRATION_PRIMARY : + ApiConnectId.ApiMethod.REQUEST_REGISTRATION_PRIMARY; + }; +}Then use it in both methods:
-switch (method) { - case MethodRecoveryPrimary -> { - ApiConnectId.requestRecoveryOtpPrimary(requireActivity(), username, callback); - } - // ... other cases -} +ApiConnectId.executeMethod(getApiMethod(false), requireActivity(), username, password, callback);Also applies to: 354-364
app/src/org/commcare/network/connectId/network/ApiService.java (2)
1-7: Consider adding JavaDoc comments for clarity.Since this interface contains multiple endpoints that handle critical user data, adding JavaDoc comments at the class and method levels would improve readability and help other developers quickly understand each endpoint’s purpose and expected parameters.
8-63: Use strongly typed request/response models instead of raw Maps.You are currently passing request bodies as
Map<String, String>and receivingResponseBody. While this is convenient, it greatly reduces type safety and can make the code more prone to errors. Consider introducing data classes (POJOs) for request and response bodies, which would leverage Retrofit’s automatic serialization/deserialization and make the code more maintainable.app/src/org/commcare/connect/network/ApiConnectId.java (16)
193-203: Address UI lifecycle concerns withshowProgressDialog.Displaying a progress dialog inside a static method that references
CommCareActivitycan lead to lifecycle issues (e.g., activity is destroyed mid-call). Consider using an activity or lifecycle-aware approach to avoid potential memory leaks or crashes.
206-213: Consistency withshowProgressDialog.This matches the design in
showProgressDialog, but confirm you won’t dismiss the dialog after the activity is destroyed or recreated. You might need a lifecycle check to avoid calling UI operations on a destroyed context.
214-243: Avoid potential static re-entrancy issues incallApi.This method always calls
showProgressDialogbefore enqueueing a request. If multiple requests are triggered, there could be overlapping progress dialogs or race conditions. Consider a consistent queue or reference count approach to track how many requests are in flight.
257-266: Security note forresetPassword.When updating a user’s password, ensure the call is made over HTTPS only, considering the potential sensitivity of user credentials. Adding explicit SSL pinning or certificate checks can further strengthen security.
269-279: Use a single consistent naming scheme incheckPin.The request map key uses
"recovery_pin", but the function is namedcheckPin. Ensure naming consistency across the codebase, or add clarifying comments so developers are aware of the meaning behind “recovery_pin.”
282-293: Re-useApiServiceinstance instead of reassigning it inchangePin.Repeatedly calling
ApiClient.getClient().create(ApiService.class)can lead to confusion about whether a single or multipleApiServiceinstances exist. Consider a single-lazy loaded instance or a dependency-injection approach to ensure consistent usage.
302-312: Include validation forregisterUser.While you gather user data (username, password, name, phone_number, fcm_token), ensure that the client or server side enforces format or presence validations. This helps avoid incomplete or malformed registrations.
329-345: Consolidate repeated code forupdateUserProfile.
updateProfile()is very similar in structure to many other methods in this file, including building a HashMap, retrieving a token, and callingcallApi. Consider refactoring or centralizing repeated logic to improve maintainability.
348-355: Name clarity inrequestRegistrationOtpPrimary.This method calls
validatePhoneon the API. The naming might be confusing, as “requestRegistrationOtpPrimary” and “validatePhone” are not obviously synonymous. Either rename the method or add clarifying comments for future maintainers.
358-363: Implement retry/backoff logic forrequestRecoveryOtpPrimary.Network calls can fail sporadically, especially when the user has poor connectivity. Adding a retry or exponential backoff could help improve UX for password recovery flows on suboptimal networks.
366-373: Keep track of two-phase flows inrequestRecoveryOtpSecondary.Recovery flows that require multiple steps are prone to user confusion. Provide robust logging or user feedback if a secondary phone recovery step fails or times out.
386-395: Distinguish param names inconfirmRegistrationOtpPrimary.You name your request parameter “token,” while also building an HTTP authorization header with “basicToken.” To reduce confusion, consider a more descriptive name, such as “otpCode” or “confirmationCode.”
398-406: Edge case handling inconfirmRecoveryOtpPrimary.After passing “phone,” “secret_key,” and “token,” if the user is partially registered or locked out, confirm the server’s error codes are handled. Potentially return more user-friendly errors inside
processFailure.
420-428: Prevent accidental override inconfirmVerificationOtpSecondary.You store “token” in a local var called
token1. A consistent naming approach across the code would reduce confusion while ensuring you don’t inadvertently overwrite or drop the more critical connect token.
431-437: Add user feedback forrequestInitiateAccountDeactivation.Deactivation is a major action. Consider a confirmation prompt or UI feedback that explains the next steps clearly if the request is successful or fails. This helps the user not end up in a locked-out state unknowingly.
452-468: Leverage a logging framework inhandleApiError.Right now, you print messages to stdout. Using Android’s
Logor a custom logger allows better filtering and integration with crash reporting. That can also help track usage in production and quickly identify issues.app/src/org/commcare/network/connectId/network/ApiClient.java (2)
18-18: Remove or implement the///Todo retry part of the api fails.If retry functionality is required for critical flows, consider implementing a retry interceptor or using a library that gracefully handles transient network failures and exponential backoff. Otherwise, remove the comment to maintain clarity.
26-55: Confirm concurrency and interceptors ingetClient.Your OkHttp client includes a logging interceptor and a custom header interceptor, which is good for debugging and header management. If you expect frequent concurrent requests, ensure the client is thread-safe and that you do not replicate interceptors for each new call. Reusing the same Retrofit instance is recommended.
app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java (1)
166-169: Refactor phone update calls.Lines 166 and 169 call
ApiConnectId.updateUserProfilevschangePhone. The code branches onMETHOD_CHANGE_ALTERNATE. Consider consolidating or naming these methods consistently to avoid confusion, since functionally both update a phone number.app/src/org/commcare/fragments/connectId/ConnectIdPhoneFragment.java (2)
312-312: Remove or clarify commented-out encoding code.
If the server requires%2binstead of+, reintroduce or refactor this logic to ensure correctness. Otherwise, remove the commented-out line.
315-315: Check for duplicate phone availability requests.
Repeated calls toApiConnectId.checkPhoneAvailablemay overload the server if triggered too frequently (e.g., by text watchers). Consider debouncing or caching results.app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java (1)
246-246: Evaluate error handling forcheckPin().
Add more descriptive failure messages (e.g., invalid PIN vs. server error) to help users troubleshoot easily.app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java (2)
275-275: Remove or revisit commented-out line for phone formatting.
If phone encoding is no longer needed by the server, removing the commented line helps maintain clarity.
279-279: Beware of rapid repeated phone availability checks.
As with other fragments, consider adding a brief debounce or single flight mechanism to avoid excessive calls from text watchers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/build.gradle(1 hunks)app/src/org/commcare/connect/network/ApiConnectId.java(5 hunks)app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java(3 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPhoneFragment.java(2 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragmnet.java(2 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java(2 hunks)app/src/org/commcare/network/connectId/network/ApiClient.java(1 hunks)app/src/org/commcare/network/connectId/network/ApiEndPoints.java(1 hunks)app/src/org/commcare/network/connectId/network/ApiService.java(1 hunks)
🔇 Additional comments (15)
app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragmnet.java (2)
354-364: Verify API endpoint implementationsThe API calls have been updated to use Retrofit, but we should verify that all endpoints are properly implemented in the Retrofit service interface.
✅ Verification successful
Let me gather more information about the API implementation to verify if these endpoints are properly implemented in the Retrofit service.
Let me search for the ApiService interface to verify the endpoint implementations.
Let me check the complete ApiService interface and the endpoint constants.
All API endpoints are properly implemented in the Retrofit service
After reviewing the implementation, I can confirm that all the required API endpoints are properly implemented in the Retrofit service interface:
requestRegistrationOtpPrimaryusesvalidatePhoneendpointrequestRecoveryOtpPrimaryusesrequestOTPPrimaryendpointrequestRecoveryOtpSecondaryusesrecoverSecondaryendpointrequestVerificationOtpSecondaryusesvalidateSecondaryPhoneendpointThe ApiService interface contains all the necessary Retrofit method declarations with proper annotations (@post, @get), headers, and request body mappings. The endpoints are well-defined in the ApiEndPoints class and correctly mapped in both the interface and the ApiConnectId implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Retrofit service interface definitions echo "Searching for API interface definitions..." rg -l "interface.*Api.*Connect.*Id" # Search for these specific endpoint methods in the interface echo "Searching for endpoint implementations..." rg "requestRecoveryOtpPrimary|requestRecoveryOtpSecondary|requestVerificationOtpSecondary|requestRegistrationOtpPrimary"Length of output: 1721
Script:
#!/bin/bash # Let's examine the ApiConnectId.java file to see the implementation details echo "Checking ApiConnectId implementation:" cat app/src/org/commcare/connect/network/ApiConnectId.java # Let's also search for any Retrofit service interfaces that might be used by ApiConnectId echo -e "\nSearching for Retrofit service interfaces:" rg "interface.*Service.*Connect" -A 10Length of output: 22389
Script:
#!/bin/bash # Search for the ApiService interface definition echo "Searching for ApiService interface:" rg -p "interface ApiService" -A 20 --type javaLength of output: 1161
Script:
#!/bin/bash # Get the complete ApiService interface echo "Complete ApiService interface:" cat app/src/org/commcare/network/connectId/network/ApiService.java # Check the API endpoints constants echo -e "\nAPI Endpoints constants:" rg "class ApiEndPoints" -A 20Length of output: 6102
457-467: Verify API endpoint implementations for verification methodsThe verification API calls have been updated to use Retrofit. Let's verify the implementation of these endpoints.
✅ Verification successful
Let me check the implementation details of these API endpoints.
Let me check the API service interface to verify the endpoint definitions.
Let me try a different approach to find the API service interface.
Let me try one final search to find the API interface definition.
API endpoints implementation verified with consistent Retrofit patterns
Based on the verification results, I can confirm that:
- All four verification methods (
confirmRegistrationOtpPrimary,confirmRecoveryOtpPrimary,confirmRecoveryOtpSecondary, andconfirmVerificationOtpSecondary) are properly implemented inApiConnectId.java- Each method follows consistent patterns:
- Uses proper parameter handling with
HashMap- Creates API service instance using
ApiClient.getClient()- Makes appropriate Retrofit calls with correct authentication where needed
- Uses common error handling through
callApimethodWhile we couldn't locate the interface definition file, the implementation shows proper Retrofit integration with consistent patterns across all verification endpoints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for verification endpoint implementations echo "Searching for verification endpoint implementations..." rg "confirmRecoveryOtpPrimary|confirmRecoveryOtpSecondary|confirmVerificationOtpSecondary|confirmRegistrationOtpPrimary"Length of output: 1641
Script:
#!/bin/bash # Get the implementation details of the verification endpoints echo "Checking API endpoint implementations in ApiConnectId.java..." rg -A 20 "confirmRegistrationOtpPrimary|confirmRecoveryOtpPrimary|confirmRecoveryOtpSecondary|confirmVerificationOtpSecondary" "app/src/org/commcare/connect/network/ApiConnectId.java"Length of output: 3239
Script:
#!/bin/bash # Check the API service interface definitions echo "Checking API service interface..." rg -A 10 "interface ApiService" "app/src/org/commcare/connect/network/"Length of output: 148
Script:
#!/bin/bash # Search for API endpoint definitions using the method names from the implementation echo "Searching for API endpoint definitions..." rg "confirmOTP|recoverConfirmOTP|recoverConfirmOTPSecondary|confirmOTPSecondary" "app/src/org/commcare/connect/network/"Length of output: 744
Script:
#!/bin/bash # Find all interface definitions in the network package echo "Searching for API interface definitions..." fd -e java . app/src/org/commcare/connect/network/ --exec cat {} | grep -A 15 "interface.*Service"Length of output: 197
app/src/org/commcare/connect/network/ApiConnectId.java (7)
Line range hint
130-137: Verify null-handling before callingmakeHeartbeatRequestSync.You are conditionally adding the
fcm_tokenparameter iftokenis not null, but ensure that subsequent logic (like reading frompostResult.responseStream) gracefully handles potential null or unsuccessful responses.
246-255: Ensure tokens are properly validated incheckPassword.You are retrieving a credential for standard usage. While passing parameters in a Map is flexible, confirm that only the known keys are placed in the final request to avoid potential injection or tampering from upstream calls.
296-299: Validate phone number string incheckPhoneAvailable.Before making the request, consider trimming or sanitizing phone input to reduce the risk of accidental trailing spaces or invalid characters that might miss the server-side validation.
316-326: Double-check multi-step logic inchangePhone.You build a token from a user’s credentials and then pass
old_phone_numberandnew_phone_numberin the payload. If the phone change requires more security, confirm that the token is indeed a valid proof of identity on the server side.
376-383: Validate empty param sets inrequestVerificationOtpSecondary.This method posts an empty
HashMap<String, String>aside from the token. Confirm that the server logic expects an empty request or that no further parameters are required here.
409-417: Confirm phone format inconfirmRecoveryOtpSecondary.Similar to
confirmRecoveryOtpPrimary, ensure that “phone” is well-formatted when merging user input. Logging or additional checks can prevent confusion around invalid requests to the server.
440-450: Don’t swallow exceptions inconfirmUserDeactivation.If the call fails or the server returns unexpected data, your callback logs or toasts an error. Confirm that we surface the error to the user or developer logs. This helps with debugging in production.
app/src/org/commcare/network/connectId/network/ApiEndPoints.java (1)
9-9: Discuss the trailing question mark inphoneAvailable.The endpoint is listed as
"/users/phone_available?"; confirm if this question mark is required by backend logic. If not, removing it can prevent confusion or routing errors.app/src/org/commcare/fragments/connectId/ConnectIdPhoneFragment.java (2)
280-280: Ensure robust error handling forupdateUserProfile.
The callback handles specific failures, but consider broader HTTP response code checks (e.g., 4xx vs. 5xx) and user feedback for edge cases.
283-283: Confirm existing phone number usage inchangePhone.
existingis used as the old phone parameter. If the user has multiple stored phone fields, verify thatexistingis indeed the correct field to pass.app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java (1)
219-219: Consider secure handling of PIN data inchangePin().
Storing or logging the plain-text PIN can be a security risk. Evaluate secure storage or cryptographic hashing, if feasible.app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java (1)
180-180: Avoid storing or logging raw passwords incheckPassword().
Ensure no logs or memory traces (e.g., debug statements) contain sensitive information.app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java (1)
297-302: Refine user feedback for response code 406.
Currently, the block sets the error text to “Network is outdated”. Verify that 406 status always indicates an outdated network scenario or if more granular messaging is possible.
| public static void linkHqWorker(Context context, String hqUsername, String hqPassword, String connectToken) { | ||
| String seatedAppId = CommCareApplication.instance().getCurrentApp().getUniqueId(); | ||
| ConnectLinkedAppRecord appRecord = ConnectDatabaseHelper.getAppData(context, seatedAppId, hqUsername); | ||
| if (appRecord != null && !appRecord.getWorkerLinked()) { | ||
| HashMap<String, String> params = new HashMap<>(); | ||
| params.put("token", connectToken); | ||
|
|
||
| String url = ServerUrls.getKeyServer().replace("phone/keys/", | ||
| "settings/users/commcare/link_connectid_user/"); | ||
|
|
||
| try { | ||
| ConnectNetworkHelper.PostResult postResult = ConnectNetworkHelper.postSync(context, url, | ||
| API_VERSION_NONE, new AuthInfo.ProvidedAuth(hqUsername, appRecord.getPassword()), params, true, false); | ||
| if (postResult.e == null && postResult.responseCode == 200) { | ||
| postResult.responseStream.close(); | ||
|
|
||
| //Remember that we linked the user successfully | ||
| appRecord.setWorkerLinked(true); | ||
| ConnectDatabaseHelper.storeApp(context, appRecord); | ||
| //Remember that we linked the user successfully | ||
| appRecord.setWorkerLinked(true); | ||
| ConnectDatabaseHelper.storeApp(context, appRecord); | ||
| } | ||
| } catch (IOException e) { | ||
| Logger.exception("Linking HQ worker", e); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent error-handling in linkHqWorker.
This function mixes synchronous network calls (ConnectNetworkHelper.postSync) with try/catch blocks. If the call fails, you log the exception but don’t surface the error to the caller beyond logging. Ensure you handle or propagate exceptions uniformly throughout the code so calling methods can properly respond.
| private static void handleNetworkError(Throwable t) { | ||
| if (t instanceof IOException) { | ||
| // IOException is usually a network error (no internet, timeout, etc.) | ||
| System.out.println("Network Error: " + t.getMessage()); | ||
| } else if (t instanceof HttpException) { | ||
| // Handle HTTP exceptions separately if needed | ||
| System.out.println("HTTP Error: " + t.getMessage()); | ||
| } else { | ||
| System.out.println("Unexpected Error: " + t.getMessage()); | ||
| } |
There was a problem hiding this comment.
Avoid printing sensitive errors with handleNetworkError.
Network errors can occasionally reveal tokens or credentials. Ensure no sensitive info is printed in the error messages. Mask or sanitize any user data that might appear in logs.
| public static final String resetPassword = "/users/recover/reset_password"; | ||
| public static final String changePassword = "/users/change_password"; | ||
| public static final String checkPassword = "/users/recover/confirm_password"; | ||
| public static final String confirmPassword = "//users/recover/confirm_password"; |
There was a problem hiding this comment.
Potential typo for confirmPassword.
It’s defined as "//users/recover/confirm_password". The double slash might break the endpoint. Please confirm if "/users/recover/confirm_password" is intended instead.
- public static final String confirmPassword = "//users/recover/confirm_password";
+ public static final String confirmPassword = "/users/recover/confirm_password";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static final String confirmPassword = "//users/recover/confirm_password"; | |
| public static final String confirmPassword = "/users/recover/confirm_password"; |
| @@ -357,7 +361,7 @@ public void createAccount() { | |||
| binding.nameTextValue.getText().toString(), ""); | |||
|
|
|||
| final Context context = getActivity(); | |||
| boolean isBusy = !ApiConnectId.registerUser(requireActivity(), tempUser.getUserId(), tempUser.getPassword(), | |||
| ApiConnectId.registerUser(requireActivity(), tempUser.getUserId(), tempUser.getPassword(), | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate user credentials upon registration.
Consider adding server-side validations for name, phone format, or password strength. This avoids storing incomplete or invalid user records.
| implementation 'com.squareup.retrofit2:retrofit:2.9.0' | ||
| implementation 'com.squareup.okhttp3:logging-interceptor:4.11.0' | ||
| implementation 'com.squareup.retrofit2:converter-gson:2.9.0' |
There was a problem hiding this comment.
💡 Codebase verification
Android SDK 35 is not yet released, update required
The verification confirms that compileSdk 35 is indeed used in both app/build.gradle and commcare-support-library/build.gradle. As of January 2025, Android 14 (API 34) is the latest stable release, making SDK 35 an invalid configuration that will cause build failures.
app/build.gradle: Line containingcompileSdk 35commcare-support-library/build.gradle: Line containingcompileSdk 35
Both files need to be updated to use compileSdk 34 to ensure compatibility with the latest stable Android SDK.
🔗 Analysis chain
Verify Android SDK compatibility.
The project is configured to use compileSdk 35, but Android 14 (API 34) is the latest release. This might cause build issues or unexpected behavior.
Consider updating the compileSdk version in the build configuration:
android {
- compileSdk 35
+ compileSdk 34
}Let me verify the Android SDK version configuration in the build files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for compileSdk configuration in Gradle build files
rg "compileSdk" -A 2 --type gradle
Length of output: 302
💡 Codebase verification
The logging interceptor is currently unconditionally enabled with BODY level logging, which could expose sensitive information in production. The implementation in app/src/org/commcare/network/connectId/network/ApiClient.java needs to be updated to enable logging only in debug builds.
- Update
ApiClient.javato conditionally enable logging:
HttpLoggingInterceptor logging = new HttpLoggingInterceptor();
if (BuildConfig.DEBUG) {
logging.setLevel(HttpLoggingInterceptor.Level.BODY);
} else {
logging.setLevel(HttpLoggingInterceptor.Level.NONE);
}🔗 Analysis chain
LGTM with a security consideration.
The Retrofit dependencies are appropriate for implementing type-safe HTTP client functionality. However, ensure that the OkHttp logging interceptor is only enabled in debug builds to prevent potential sensitive information leakage in production.
Run this script to verify if logging interceptor configuration exists in the codebase:
If no configuration is found, consider adding a debug-only configuration for the logging interceptor.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if logging interceptor is properly configured for debug/release builds
rg -l "LoggingInterceptor"
Length of output: 88
Script:
#!/bin/bash
# Check the implementation of LoggingInterceptor in ApiClient.java
rg -A 5 "LoggingInterceptor" app/src/org/commcare/network/connectId/network/ApiClient.java
Length of output: 655
Script:
#!/bin/bash
# Check if there's any build variant or debug configuration check before enabling logging
rg -B 5 -A 2 "HttpLoggingInterceptor" app/src/org/commcare/network/connectId/network/ApiClient.java
Length of output: 800
|
@damagatchi retest this please |
There was a problem hiding this comment.
Seeing a couple issues on further testing that I think we're going to need to address.
- ApiConnectId doesn't go through ConnectNetworkHelper for most of its calls anymore, but in the process that means we lost the ability to do "foreground" API calls, i.e. calls that block the user and show a wait dialog until they are complete. This mechanism prevents the user from pressing the button multiple times while waiting for the first API call to return, and needs to be maintained.
- Logging of errors (via CrashUtil.reportException) also happened through ConnectNetworkHelper and needs to be preserved in the new code.
| callApi(context,call,callback); | ||
| } | ||
|
|
||
| private static void handleApiError(Response<?> response) { |
There was a problem hiding this comment.
Errors should be reported via CrashUtil.reportException so we can observe them from Crashlytics.
retrofit implementation of the api netwrok call
Safety Assurance
Automated test coverage
Safety story