PM-connect id initial pr commit #2949
Conversation
|
@coderabbitai full review |
📝 WalkthroughWalkthroughThe pull request introduces extensive updates that integrate ConnectID functionality into the application. Changes span multiple layers including the Android manifest, build configuration, UI resources, layouts, and business logic. A new SMS broadcast receiver and ConnectIdActivity are added to support SMS retrieval and user connection flows. Several new fragments, activities, and custom views (such as CustomOtpView, ConnectEditText, and various ConnectTextViews) have been introduced to manage user signup, login, biometric, PIN, and OTP verification processes. Additional enhancements include network API calls via ApiConnect and SSO token retrieval through ConnectSsoHelper, expanded analytics tracking, and support for maintaining developer preferences for ConnectID. Overall, these modifications refine the user authentication and recovery process through a structured ConnectID workflow. Sequence Diagram(s)sequenceDiagram
participant U as User
participant A as ConnectIdActivity
participant N as NavController
participant F as ConnectID Fragment
participant SM as SMSBroadcastReceiver
participant CM as ConnectManager
participant API as ApiConnect/ConnectSsoHelper
U->>A: Launch ConnectID flow
A->>N: Process intent & handle redirection
N->>F: Navigate to signup/verification fragment
U->>F: Enter phone number / OTP / PIN
alt SMS retrieval needed
SM->>CM: Receive SMS OTP event
CM->>F: Notify OTP received
end
F->>API: Verify credentials/OTP via API call
API-->>F: Return verification response
F->>A: Finish process & update UI
Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (56)
app/src/org/commcare/views/connect/connecttextview/ConnectBoldTextView.java (1)
28-34: 🛠️ Refactor suggestion
Improve error handling and logging for font initialization.
The current implementation has several areas for improvement:
- Exception handling only prints stack trace
- No fallback mechanism if font loading fails
- Missing logging for debugging/analytics
Consider this improved implementation:
private void init(Context context) { try { setTypeface(ResourcesCompat.getFont(context, R.font.roboto_bold)); } catch (Exception e) { - e.printStackTrace(); + Logger.error(TAG, "Failed to load Roboto Bold font", e); + // Fallback to system bold font + setTypeface(Typeface.DEFAULT_BOLD); } }Add this constant at class level:
private static final String TAG = ConnectBoldTextView.class.getSimpleName();app/src/org/commcare/views/connect/connecttextview/ConnectMediumTextView.java (1)
28-34: 🛠️ Refactor suggestion
Enhance error handling and logging for font initialization.
The current implementation has several areas for improvement:
- The catch block is too broad - consider catching specific exceptions like
Resources.NotFoundException.- Simple stack trace printing may not be sufficient for production monitoring.
- No fallback strategy when font loading fails.
Consider this improved implementation:
private void init(Context context) { try { setTypeface(ResourcesCompat.getFont(context, R.font.roboto_medium)); - } catch (Exception e) { - e.printStackTrace(); + } catch (Resources.NotFoundException e) { + Log.e("ConnectMediumTextView", "Failed to load Roboto Medium font", e); + // Fallback to system font + setTypeface(Typeface.DEFAULT_BOLD); } }Don't forget to add the import:
import android.util.Log; import android.graphics.Typeface;app/src/org/commcare/connect/ConnectConstants.java (1)
9-11: 🛠️ Refactor suggestion
Maintain consistent naming convention for constants.
The constant
ConnectIdTaskIdOffsetuses camel case while other constants use uppercase with underscores. This inconsistency makes the code harder to maintain.Apply this diff to maintain consistency:
- public static final int ConnectIdTaskIdOffset = 1000; + public static final int CONNECT_ID_TASK_ID_OFFSET = 1000;Remember to update all references to this constant throughout the file.
📝 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 int CONNECT_ID_TASK_ID_OFFSET = 1000; public final static int CREDENTIAL_PICKER_REQUEST = 2000; public static final int CONNECTID_REQUEST_CODE = 1034;app/src/org/commcare/views/connect/connecttextview/connecttextview/ConnectItalicTextView.java (1)
28-34: 🛠️ Refactor suggestion
Improve font loading error handling.
The current implementation has several areas for improvement:
- Implement fallback behavior when font loading fails
- Use specific exception types
- Add proper error logging for debugging
private void init(Context context) { try { setTypeface(ResourcesCompat.getFont(context, R.font.roboto_italic)); - } catch (Exception e) { - e.printStackTrace(); + } catch (Resources.NotFoundException e) { + Log.e("ConnectItalicTextView", "Failed to load Roboto Italic font", e); + // Fallback to system italic font + setTypeface(Typeface.defaultFromStyle(Typeface.ITALIC)); } }📝 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.private void init(Context context) { try { setTypeface(ResourcesCompat.getFont(context, R.font.roboto_italic)); } catch (Resources.NotFoundException e) { Log.e("ConnectItalicTextView", "Failed to load Roboto Italic font", e); // Fallback to system italic font setTypeface(Typeface.defaultFromStyle(Typeface.ITALIC)); } }app/src/org/commcare/utils/BiometricsHelper.java (1)
99-125: 🛠️ Refactor suggestion
Refactor usage of a static callback holder and remove commented-out code.
- Storing
BiometricPrompt.AuthenticationCallbackin a static field can cause concurrency issues or memory leaks if multiple authentication flows overlap or if the user leaves the activity unexpectedly. Consider a non-static approach or a more lifecycle-aware solution.- The large block of commented code implies either observed partial functionality or unresolved issues. It may be clearer to remove it or provide additional context (via documentation) if needed.
app/src/org/commcare/fragments/SelectInstallModeFragment.java (1)
200-211: 🛠️ Refactor suggestion
Add documentation and improve null safety.
The new public method needs documentation and additional null checks for robustness.
Apply these improvements:
+ /** + * Updates the visibility and click listener of the Connect button and related UI elements. + * @param connectEnabled Whether the connect feature should be enabled + * @param listener Click listener to be set when the button is enabled + */ public void updateConnectButton(boolean connectEnabled, View.OnClickListener listener) { - if(mConnectButton != null) { + if (mConnectButton != null && mOrText != null) { boolean enabled = connectEnabled && ConnectManager.shouldShowConnectButton(); if (enabled) { + if (listener != null) { mConnectButton.setOnClickListener(listener); + } } mConnectButton.setVisibility(enabled ? View.VISIBLE : View.GONE); mOrText.setVisibility(enabled ? View.VISIBLE : View.GONE); } }📝 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./** * Updates the visibility and click listener of the Connect button and related UI elements. * @param connectEnabled Whether the connect feature should be enabled * @param listener Click listener to be set when the button is enabled */ public void updateConnectButton(boolean connectEnabled, View.OnClickListener listener) { if (mConnectButton != null && mOrText != null) { boolean enabled = connectEnabled && ConnectManager.shouldShowConnectButton(); if (enabled) { if (listener != null) { mConnectButton.setOnClickListener(listener); } } mConnectButton.setVisibility(enabled ? View.VISIBLE : View.GONE); mOrText.setVisibility(enabled ? View.VISIBLE : View.GONE); } }app/src/org/commcare/views/connect/connecttextview/connecttextview/ConnectMediumTextView.java (1)
28-34: 🛠️ Refactor suggestion
Improve error handling and add safety checks.
The current implementation has several areas for improvement:
- The catch block uses a generic Exception and only prints the stack trace
- Missing null check for the context parameter
- No fallback handling when font loading fails
Consider this improved implementation:
private void init(Context context) { + if (context == null) { + return; + } try { setTypeface(ResourcesCompat.getFont(context, R.font.roboto_medium)); - } catch (Exception e) { - e.printStackTrace(); + } catch (Resources.NotFoundException e) { + Log.e("ConnectMediumTextView", "Failed to load Roboto Medium font", e); + // Fall back to system default medium font + setTypeface(Typeface.DEFAULT_BOLD); } }📝 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.private void init(Context context) { + if (context == null) { + return; + } try { setTypeface(ResourcesCompat.getFont(context, R.font.roboto_medium)); - } catch (Exception e) { - e.printStackTrace(); + } catch (Resources.NotFoundException e) { + Log.e("ConnectMediumTextView", "Failed to load Roboto Medium font", e); + // Fall back to system default medium font + setTypeface(Typeface.DEFAULT_BOLD); } }app/src/org/commcare/views/connect/CustomOtpView.java (1)
103-109: 🛠️ Refactor suggestion
Replace magic number with constant or parameter.
The hardcoded value '5' for index checking appears to be tied to a specific use case. This could cause issues if
digitCountis changed.- if(index == 5) { + if(index == digitCount - 1) {📝 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.if(index == digitCount - 1) { //Check for special case where we need to delete last digit and stay in the cell EditText last = (EditText)getChildAt(index); if(last.getText().length() > 0) { edtIndex++; } }app/src/org/commcare/connect/workers/ConnectHeartbeatWorker.kt (1)
14-30: 🛠️ Refactor suggestion
Add error handling and logging for failure scenarios.
The current implementation lacks error handling and logging, which could make it difficult to diagnose issues in production.
Consider adding try-catch block and logging:
override suspend fun doWork(): Result { return withContext(Dispatchers.IO) { - if (!ConnectManager.isConnectIdConfigured()) { - return@withContext Result.failure() - } + try { + if (!ConnectManager.isConnectIdConfigured()) { + Log.w(TAG, "Connect ID not configured, skipping heartbeat") + return@withContext Result.failure() + } - val result = ApiConnectId.makeHeartbeatRequestSync(applicationContext); - return@withContext if (result.responseCode in 200..299) Result.success() else Result.failure() + val result = ApiConnectId.makeHeartbeatRequestSync(applicationContext) + return@withContext if (result.responseCode in 200..299) { + Log.d(TAG, "Heartbeat successful") + Result.success() + } else { + Log.e(TAG, "Heartbeat failed with response code: ${result.responseCode}") + Result.failure() + } + } catch (e: Exception) { + Log.e(TAG, "Error during heartbeat", e) + return@withContext Result.failure() + } } } + +private companion object { + private const val TAG = "ConnectHeartbeatWorker" +}📝 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.override suspend fun doWork(): Result { return withContext(Dispatchers.IO) { try { if (!ConnectManager.isConnectIdConfigured()) { Log.w(TAG, "Connect ID not configured, skipping heartbeat") return@withContext Result.failure() } //NOTE: Using trad'l code route until we can get the commented code to work //val connectNetworkService = ConnectNetworkServiceFactory.createConnectIdNetworkSerive() //val fcmToken = FirebaseMessagingUtil.getFCMToken(); //val requestBody = HeartBeatBody(fcmToken) //val response = connectNetworkService.makeHeartbeatRequest(requestBody)!!.execute() //return@withContext if (response.isSuccessful) Result.success() else Result.failure() val result = ApiConnectId.makeHeartbeatRequestSync(applicationContext) return@withContext if (result.responseCode in 200..299) { Log.d(TAG, "Heartbeat successful") Result.success() } else { Log.e(TAG, "Heartbeat failed with response code: ${result.responseCode}") Result.failure() } } catch (e: Exception) { Log.e(TAG, "Error during heartbeat", e) return@withContext Result.failure() } } } private companion object { private const val TAG = "ConnectHeartbeatWorker" }app/src/org/commcare/connect/network/ConnectSsoHelper.java (2)
24-52: 🛠️ Refactor suggestion
Add error handling and null checks for robustness.
While the implementation is thread-safe, it could be more robust:
- Add null check for Context in doInBackground
- Add error handling for token retrieval failures
Consider this implementation:
@Override protected AuthInfo.TokenAuth doInBackground(Void... voids) { Context context = weakContext.get(); + if (context == null) { + return null; + } if(hqUsername == null) { - return ApiConnectId.retrieveConnectIdTokenSync(context); + try { + return ApiConnectId.retrieveConnectIdTokenSync(context); + } catch (Exception e) { + // Log error + return null; + } } - return retrieveHqSsoTokenSync(context, hqUsername, linkHqUser); + try { + return retrieveHqSsoTokenSync(context, hqUsername, linkHqUser); + } catch (Exception e) { + // Log error + return null; + } }📝 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.//Used for aynchronously retrieving HQ or SSO token private static class TokenTask extends AsyncTask<Void, Void, AuthInfo.TokenAuth> { private final WeakReference<Context> weakContext; private final String hqUsername; //null for ConnectId private final boolean linkHqUser; final TokenCallback callback; TokenTask(Context context, String hqUsername, boolean linkHqUser, TokenCallback callback) { super(); this.weakContext = new WeakReference<>(context); this.hqUsername = hqUsername; this.linkHqUser = linkHqUser; this.callback = callback; } @Override protected AuthInfo.TokenAuth doInBackground(Void... voids) { Context context = weakContext.get(); + if (context == null) { + return null; + } if (hqUsername == null) { - return ApiConnectId.retrieveConnectIdTokenSync(context); + try { + return ApiConnectId.retrieveConnectIdTokenSync(context); + } catch (Exception e) { + // Log error + return null; + } } - return retrieveHqSsoTokenSync(context, hqUsername, linkHqUser); + try { + return retrieveHqSsoTokenSync(context, hqUsername, linkHqUser); + } catch (Exception e) { + // Log error + return null; + } } @Override protected void onPostExecute(AuthInfo.TokenAuth token) { callback.tokenRetrieved(token); } }
60-91:
⚠️ Potential issueFix potential NullPointerException in token handling.
There's a bug in the linkHqWorker call where hqTokenAuth might be null:
-ApiConnectId.linkHqWorker(context, hqUsername, hqTokenAuth.password, connectIdToken.bearerToken); +// Get password through secure means or handle null case +String password = getSecurePassword(hqUsername); // Implement this method +if (password != null) { + ApiConnectId.linkHqWorker(context, hqUsername, password, connectIdToken.bearerToken); +}Committable suggestion skipped: line range outside the PR's diff.
app/src/org/commcare/views/connect/connecttextview/connecttextview/ConnectBoldTextView.java (1)
1-1: 🛠️ Refactor suggestion
Fix the redundant package name segment.
The package name contains a duplicated "connecttextview" segment which should be removed for better organization.
-package org.commcare.views.connect.connecttextview.connecttextview; +package org.commcare.views.connect.connecttextview;📝 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.package org.commcare.views.connect.connecttextview;app/src/org/commcare/activities/connect/ConnectIdActivity.java (2)
42-45:
⚠️ Potential issueFix the request code vs. result code mismatch.
UsingrequestCode == RESULT_OKis likely unintended. Typically, you compare theresultCodetoRESULT_OK.-if (requestCode == RESULT_OK) { +if (resultCode == RESULT_OK) { finish(); }📝 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.if (resultCode == RESULT_OK) { finish(); } }
37-39:
⚠️ Potential issueGuard against potential NullPointerException when retrieving the current fragment.
getCurrentFragment()might returnnullif the current fragment is not an instance ofConnectIdBiometricConfigFragment, leading to a crash on.onActivityResult(...).You could add a null check before using the fragment reference, for instance:
if (requestCode == ConnectConstants.CONNECT_UNLOCK_PIN) { - getCurrentFragment().onActivityResult(requestCode, resultCode, data); + ConnectIdBiometricConfigFragment fragment = getCurrentFragment(); + if (fragment != null) { + fragment.onActivityResult(requestCode, resultCode, data); + } }📝 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.if (requestCode == ConnectConstants.CONNECT_UNLOCK_PIN) { - getCurrentFragment().onActivityResult(requestCode, resultCode, data); + ConnectIdBiometricConfigFragment fragment = getCurrentFragment(); + if (fragment != null) { + fragment.onActivityResult(requestCode, resultCode, data); + } } else if (requestCode == ConnectConstants.CONNECTID_REQUEST_CODE) {app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java (1)
264-270: 🛠️ Refactor suggestion
Stop the handler when leaving this fragment or after errors.
The handler is only stopped on successful verification. If the user navigates back or verification fails multiple times, the handler continues to run. Stopping it inonPause()or upon failures prevents potential resource leaks.app/src/org/commcare/fragments/connectId/ConnectIdPhoneFragment.java (2)
105-121:
⚠️ Potential issueGracefully handle null data in
phoneNumberHintLaunchercallback.
Ifdataor the parsed phone number isnull, the code immediately throws aRuntimeException(line 116). Consider logging an error or displaying a warning message to the user rather than throwing an unhandled exception.- throw new RuntimeException(e); + Logger.exception("Retrieved phone hint but encountered an ApiException", e); + Toast.makeText(getContext(), R.string.error_occured, Toast.LENGTH_SHORT).show();📝 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.PhoneNumberHelper.phoneNumberHintLauncher = registerForActivityResult( new ActivityResultContracts.StartIntentSenderForResult(), result -> { if (result.getResultCode() == Activity.RESULT_OK && result.getData() != null) { Intent data = result.getData(); String phoneNumber; try { phoneNumber = Identity.getSignInClient(requireActivity()).getPhoneNumberFromIntent(data); displayNumber(phoneNumber); } catch (ApiException e) { Toast.makeText(getContext(), R.string.error_occured, Toast.LENGTH_SHORT).show(); Logger.exception("Retrieved phone hint but encountered an ApiException", e); Toast.makeText(getContext(), R.string.error_occured, Toast.LENGTH_SHORT).show(); } } } );
130-140:
⚠️ Potential issueAdd null checks for the retrieved
userobject.
Whenuserisnull, calls likeuser.getPrimaryPhone()might throw NullPointerExceptions. Consider returning early or safely handling the null object.- ConnectUserRecord user = ConnectManager.getUser(getActivity()); - String existing = user != null ? user.getPrimaryPhone() : existingPhone; + ConnectUserRecord user = ConnectManager.getUser(getActivity()); + if (user == null && existingPhone == null) { + // handle scenario gracefully, e.g. show an error or skip certain logic + } + String existing = user != null ? user.getPrimaryPhone() : existingPhone;📝 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.ConnectUserRecord user = ConnectManager.getUser(getActivity()); String title = getString(R.string.connect_phone_title_primary); String message = getString(R.string.connect_phone_message_primary); if (user == null && existingPhone == null) { // handle scenario gracefully, e.g. show an error or skip certain logic } String existing = user != null ? user.getPrimaryPhone() : existingPhone; binding.connectPrimaryPhoneTitle.setText(title); binding.connectPrimaryPhoneMessage.setText(message); displayNumber(existing); requireActivity().setTitle(R.string.connect_phone_title_primary); return binding.getRoot(); }app/src/org/commcare/activities/LoginActivityUIController.java (1)
544-553:
⚠️ Potential issueHandle missing or partial user data when updating the welcome message.
InupdateConnectLoginState(), callingConnectUserDatabaseUtil.getUser(activity).getName()assumes the user record always exists and contains a valid name. Guard against null or empty strings to avoid potential crashes or misleading UI strings.+ ConnectUserRecord user = ConnectUserDatabaseUtil.getUser(activity); + if (user == null || user.getName() == null) { + welcomeMessage.setText(getString(R.string.default_welcome_message)); + return; + } String welcomeText = activity.getString(R.string.login_welcome_connect_signed_in, - ConnectUserDatabaseUtil.getUser(activity).getName()); + user.getName()); welcomeMessage.setText(welcomeText);Committable suggestion skipped: line range outside the PR's diff.
app/src/org/commcare/connect/ConnectManager.java (2)
998-1008:
⚠️ Potential issueUse a secure random generator for password creation.
Currently, the code uses
new Random()to generate passwords, which is not cryptographically secure. Usejava.security.SecureRandomto generate strong, less predictable passwords.- StringBuilder password = new StringBuilder(); - for (int i = 0; i < passwordLength; i++) { - password.append(charSet.charAt(new Random().nextInt(charSet.length()))); - } + SecureRandom secureRandom = new SecureRandom(); + StringBuilder password = new StringBuilder(passwordLength); + for (int i = 0; i < passwordLength; i++) { + password.append(charSet.charAt(secureRandom.nextInt(charSet.length()))); + }📝 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 String generatePassword() { int passwordLength = 20; String charSet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_!.?"; SecureRandom secureRandom = new SecureRandom(); StringBuilder password = new StringBuilder(passwordLength); for (int i = 0; i < passwordLength; i++) { password.append(charSet.charAt(secureRandom.nextInt(charSet.length()))); } return password.toString(); }
222-224:
⚠️ Potential issueAvoid storing an Activity context in a static manager.
Storing the
parentActivity(which might be an Activity context) in a static field risks memory leaks. Consider using only anApplicationcontext for this purpose or removing the direct context storage altogether if not strictly needed.- public static void setParent(Context parent) { - getInstance().parentActivity = parent; - } + /** + * Store only application context or remove if context usage is not necessary + * for ConnectManager's logic. + */Committable suggestion skipped: line range outside the PR's diff.
app/src/org/commcare/connect/SMSBroadcastReceiver.java (3)
1-43:
⚠️ Potential issueAdd SMSListener interface definition.
The SMSListener interface is missing from the codebase.
Add this interface to handle SMS events:
public interface SMSListener { void onSuccess(Intent messageIntent); void onError(String error); }
19-19: 🛠️ Refactor suggestion
Make smsListener private and provide setter method.
The public field exposes internal implementation details and breaks encapsulation.
- public SMSListener smsListener; + private SMSListener smsListener; + + public void setSmsListener(SMSListener listener) { + this.smsListener = listener; + }📝 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.private SMSListener smsListener; public void setSmsListener(SMSListener listener) { this.smsListener = listener; }
32-37:
⚠️ Potential issueAdd null check and error handling.
The code needs to handle potential null values from
getParcelableand status failures.- Intent messageIntent = extras.getParcelable(SmsRetriever.EXTRA_CONSENT_INTENT); - // Extract one-time code from the message and complete verification - // by sending the code back to your server. - if (smsListener != null) - smsListener.onSuccess(messageIntent); + Intent messageIntent = extras.getParcelable(SmsRetriever.EXTRA_CONSENT_INTENT); + if (messageIntent != null && smsListener != null) { + smsListener.onSuccess(messageIntent); + } + } else { + if (smsListener != null) { + smsListener.onError("Failed to retrieve SMS: " + status.getStatusMessage()); + }📝 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.Intent messageIntent = extras.getParcelable(SmsRetriever.EXTRA_CONSENT_INTENT); if (messageIntent != null && smsListener != null) { smsListener.onSuccess(messageIntent); } } else { if (smsListener != null) { smsListener.onError("Failed to retrieve SMS: " + status.getStatusMessage()); }app/src/org/commcare/utils/CrashUtil.java (1)
59-63: 🛠️ Refactor suggestion
Add error handling for getUser call.
The
getUsercall could throw an exception if the user is not properly initialized.public static void registerConnectUser() { if (crashlyticsEnabled && ConnectManager.isConnectIdConfigured()) { - FirebaseCrashlytics.getInstance().setCustomKey(CCC_USER, ConnectManager.getUser(CommCareApplication.instance()).getUserId()); + try { + String userId = ConnectManager.getUser(CommCareApplication.instance()).getUserId(); + FirebaseCrashlytics.getInstance().setCustomKey(CCC_USER, userId); + } catch (Exception e) { + FirebaseCrashlytics.getInstance().recordException(e); + } } }📝 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 void registerConnectUser() { if (crashlyticsEnabled && ConnectManager.isConnectIdConfigured()) { try { String userId = ConnectManager.getUser(CommCareApplication.instance()).getUserId(); FirebaseCrashlytics.getInstance().setCustomKey(CCC_USER, userId); } catch (Exception e) { FirebaseCrashlytics.getInstance().recordException(e); } } }app/src/org/commcare/fragments/connectId/ConnectIdPhoneAvailableBottomSheet.java (2)
47-53:
⚠️ Potential issueAdd null safety for phone number.
The
setTextcall could throw NPE if phone number is null.back = (Button) view.findViewById(R.id.back_button); recover = (Button) view.findViewById(R.id.recover_button); phoneTextView = (ConnectMediumTextView) view.findViewById(R.id.phone_number); if (getArguments() != null) { phoneNumber = ConnectIdPhoneAvailableBottomSheetArgs.fromBundle(getArguments()).getPhone(); } - phoneTextView.setText(phoneNumber); + phoneTextView.setText(phoneNumber != null ? phoneNumber : "");📝 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.back = (Button) view.findViewById(R.id.back_button); recover = (Button) view.findViewById(R.id.recover_button); phoneTextView = (ConnectMediumTextView) view.findViewById(R.id.phone_number); if (getArguments() != null) { phoneNumber = ConnectIdPhoneAvailableBottomSheetArgs.fromBundle(getArguments()).getPhone(); } phoneTextView.setText(phoneNumber != null ? phoneNumber : "");
30-35: 🛠️ Refactor suggestion
Remove unused parameters from newInstance.
The
newInstancemethod accepts unused parameters.- public static ConnectIdPhoneAvailableBottomSheet newInstance(String param1, String param2) { + public static ConnectIdPhoneAvailableBottomSheet newInstance() { ConnectIdPhoneAvailableBottomSheet fragment = new ConnectIdPhoneAvailableBottomSheet(); - Bundle args = new Bundle(); - fragment.setArguments(args); return fragment; }📝 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 ConnectIdPhoneAvailableBottomSheet newInstance() { ConnectIdPhoneAvailableBottomSheet fragment = new ConnectIdPhoneAvailableBottomSheet(); return fragment; }app/src/org/commcare/views/connect/CircleProgressBar.java (4)
79-82: 🛠️ Refactor suggestion
Add progress value validation.
The progress value should be validated to ensure it stays within the 0-100 range.
public void setProgress(float progress) { + if (progress < 0) { + progress = 0; + } else if (progress > 100) { + progress = 100; + } this.progress = progress; invalidate(); }📝 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 void setProgress(float progress) { if (progress < 0) { progress = 0; } else if (progress > 100) { progress = 100; } this.progress = progress; invalidate(); }
79-82: 🛠️ Refactor suggestion
Add input validation for progress value.
The progress value should be validated to ensure it stays within the valid range (0-100).
public void setProgress(float progress) { + if (progress < 0f) { + progress = 0f; + } else if (progress > 100f) { + progress = 100f; + } this.progress = progress; invalidate(); }📝 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 void setProgress(float progress) { if (progress < 0f) { progress = 0f; } else if (progress > 100f) { progress = 100f; } this.progress = progress; invalidate(); }
79-82: 🛠️ Refactor suggestion
Add input validation for progress value.
The progress value should be validated to ensure it stays within the valid range (0-100).
public void setProgress(float progress) { + if (progress < 0f) { + progress = 0f; + } else if (progress > 100f) { + progress = 100f; + } this.progress = progress; invalidate(); }📝 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 void setProgress(float progress) { if (progress < 0f) { progress = 0f; } else if (progress > 100f) { progress = 100f; } this.progress = progress; invalidate(); }
98-99: 🛠️ Refactor suggestion
Replace error logging with debug logging.
Using
Log.efor non-error operations is not recommended as it pollutes error logs.- Log.e("CircleProgressBar", "Setting progress color to: " + color); + Log.d("CircleProgressBar", "Setting progress color to: " + color);📝 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.Log.d("CircleProgressBar", "Setting progress color to: " + color); this.progressColor = color;app/src/org/commcare/utils/PhoneNumberHelper.java (8)
115-128: 🛠️ Refactor suggestion
Handle ApiException properly.
The ApiException is silently ignored. Consider logging the error and providing appropriate feedback.
- } catch (ApiException ignored) { + } catch (ApiException e) { + Log.e("PhoneNumberHelper", "Failed to get phone number: " + e.getMessage(), e); + return null; }📝 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 String handlePhoneNumberPickerResult(int requestCode, int resultCode, Intent intent, Activity activity) { if (requestCode == ConnectConstants.CREDENTIAL_PICKER_REQUEST && resultCode == Activity.RESULT_OK) { SignInClient signInClient = Identity.getSignInClient(activity); String phoneNumber; try { phoneNumber = signInClient.getPhoneNumberFromIntent(intent); return phoneNumber; } catch (ApiException e) { Log.e("PhoneNumberHelper", "Failed to get phone number: " + e.getMessage(), e); return null; } } return ""; }
29-42: 🛠️ Refactor suggestion
Make PhoneNumberUtil thread-safe.
The static PhoneNumberUtil instance is not thread-safe. Consider using ThreadLocal or synchronization.
- private static PhoneNumberUtil utilStatic = null; + private static final ThreadLocal<PhoneNumberUtil> utilStatic = new ThreadLocal<>(); + private static PhoneNumberUtil getUtil(Context context) { - if (utilStatic == null) { - utilStatic = PhoneNumberUtil.createInstance(context); + PhoneNumberUtil util = utilStatic.get(); + if (util == null) { + util = PhoneNumberUtil.createInstance(context); + utilStatic.set(util); } - return utilStatic; + return util; }📝 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.private static final ThreadLocal<PhoneNumberUtil> utilStatic = new ThreadLocal<>(); public static ActivityResultLauncher<IntentSenderRequest> phoneNumberHintLauncher; //Private constructor, class should be used statically private PhoneNumberHelper() { } private static PhoneNumberUtil getUtil(Context context) { PhoneNumberUtil util = utilStatic.get(); if (util == null) { util = PhoneNumberUtil.createInstance(context); utilStatic.set(util); } return util; }
102-113: 🛠️ Refactor suggestion
Improve exception handling in requestPhoneNumberHint.
The generic Exception catch block swallows all exceptions. Consider handling specific exceptions and providing appropriate error feedback.
- } catch (Exception e) { + } catch (SecurityException e) { + Log.e("PhoneNumberHelper", "Security permission denied", e); + } catch (IllegalStateException e) { + Log.e("PhoneNumberHelper", "Activity was destroyed", e); + } catch (Exception e) { e.printStackTrace(); }📝 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 void requestPhoneNumberHint(Activity activity) { GetPhoneNumberHintIntentRequest hintRequest = GetPhoneNumberHintIntentRequest.builder().build(); Identity.getSignInClient(activity).getPhoneNumberHintIntent(hintRequest) .addOnSuccessListener(pendingIntent -> { try { IntentSenderRequest intentSenderRequest = new IntentSenderRequest.Builder(pendingIntent).build(); phoneNumberHintLauncher.launch(intentSenderRequest); } catch (SecurityException e) { Log.e("PhoneNumberHelper", "Security permission denied", e); } catch (IllegalStateException e) { Log.e("PhoneNumberHelper", "Activity was destroyed", e); } catch (Exception e) { e.printStackTrace(); } }); }
57-59: 🛠️ Refactor suggestion
Improve exception handling.
Empty catch blocks with only comments should be avoided. Consider logging the error or notifying the caller.
} catch (NumberParseException e) { - //Error parsing number means it isn't valid, fall-through to return false + Log.d("PhoneNumberHelper", "Failed to parse number: " + e.getMessage()); }📝 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.} catch (NumberParseException e) { Log.d("PhoneNumberHelper", "Failed to parse number: " + e.getMessage()); }
109-111: 🛠️ Refactor suggestion
Improve error handling in requestPhoneNumberHint.
Catching generic Exception and only printing stack trace is not ideal. Consider handling specific exceptions and notifying the user.
- } catch (Exception e) { + } catch (SecurityException | IllegalStateException e) { e.printStackTrace(); + Log.e("PhoneNumberHelper", "Failed to launch phone number hint: " + e.getMessage()); + // Consider adding a callback to notify the caller of the failure }📝 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.} catch (SecurityException | IllegalStateException e) { e.printStackTrace(); Log.e("PhoneNumberHelper", "Failed to launch phone number hint: " + e.getMessage()); // Consider adding a callback to notify the caller of the failure }
109-111: 🛠️ Refactor suggestion
Improve exception handling in requestPhoneNumberHint.
Generic exception catching and stack trace printing should be replaced with specific exception handling.
- } catch (Exception e) { - e.printStackTrace(); + } catch (RuntimeException e) { + Log.e("PhoneNumberHelper", "Failed to launch phone number hint picker", e); + // Consider notifying the caller about the failure📝 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.} catch (RuntimeException e) { Log.e("PhoneNumberHelper", "Failed to launch phone number hint picker", e); // Consider notifying the caller about the failure }
123-124: 🛠️ Refactor suggestion
Handle ApiException properly in handlePhoneNumberPickerResult.
The ApiException is silently ignored, which could hide important error information.
- } catch (ApiException ignored) { + } catch (ApiException e) { + Log.e("PhoneNumberHelper", "Failed to get phone number from intent", e); + // Consider passing the error to the caller📝 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.} catch (ApiException e) { Log.e("PhoneNumberHelper", "Failed to get phone number from intent", e); // Consider passing the error to the caller }
30-30: 🛠️ Refactor suggestion
Encapsulate phoneNumberHintLauncher field.
The field should be private with a setter method to maintain encapsulation.
- public static ActivityResultLauncher<IntentSenderRequest> phoneNumberHintLauncher; + private static ActivityResultLauncher<IntentSenderRequest> phoneNumberHintLauncher; + + public static void setPhoneNumberHintLauncher(ActivityResultLauncher<IntentSenderRequest> launcher) { + phoneNumberHintLauncher = launcher; + }📝 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.private static ActivityResultLauncher<IntentSenderRequest> phoneNumberHintLauncher; public static void setPhoneNumberHintLauncher(ActivityResultLauncher<IntentSenderRequest> launcher) { phoneNumberHintLauncher = launcher; }app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java (2)
171-205:
⚠️ Potential issueReplace assertion with null check.
The assertion on line 203 could crash in production. Replace it with a null check and appropriate error handling.
Apply this diff to improve error handling:
- assert directions != null; + if (directions == null) { + Toast.makeText(getContext(), R.string.connect_navigation_error, Toast.LENGTH_SHORT).show(); + return; + } Navigation.findNavController(binding.continueButton).navigate(directions);📝 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 void finish(boolean success, String phone) { NavDirections directions = null; ConnectUserRecord user = ConnectUserDatabaseUtil.getUser(getActivity()); switch (callingClass) { case ConnectConstants.CONNECT_REGISTRATION_ALTERNATE_PHONE -> { if (success) { user.setAlternatePhone(phone); ConnectUserDatabaseUtil.storeUser(getActivity(), user); ConnectDatabaseHelper.setRegistrationPhase(getActivity(), ConnectConstants.CONNECT_REGISTRATION_CONFIRM_PIN); directions = ConnectIDSecondaryPhoneNumberDirections.actionConnectidSecondaryPhoneFragmentToConnectidPin(ConnectConstants.CONNECT_REGISTRATION_CONFIRM_PIN, phone, "").setRecover(false).setChange(false); } else { directions = ConnectIDSecondaryPhoneNumberDirections.actionConnectidSecondaryPhoneFragmentToConnectidPin(ConnectConstants.CONNECT_REGISTRATION_CONFIGURE_PIN, phone, "").setRecover(false).setChange(true); } } case ConnectConstants.CONNECT_UNLOCK_ALT_PHONE_CHANGE -> { directions = ConnectIDSecondaryPhoneNumberDirections.actionConnectidSecondaryPhoneFragmentToConnectidPhoneVerify(ConnectConstants.CONNECT_UNLOCK_VERIFY_ALT_PHONE, String.format(Locale.getDefault(), "%d", ConnectIdPhoneVerificationFragmnet.MethodVerifyAlternate), null, user.getUserId(), user.getPassword(), null, false).setAllowChange(false); } case ConnectConstants.CONNECT_VERIFY_ALT_PHONE_CHANGE -> { if (success) { directions = ConnectIDSecondaryPhoneNumberDirections.actionConnectidSecondaryPhoneFragmentToConnectidPhoneVerify(ConnectConstants.CONNECT_VERIFY_ALT_PHONE, String.format(Locale.getDefault(), "%d", ConnectIdPhoneVerificationFragmnet.MethodVerifyAlternate), null, user.getUserId(), user.getPassword(), null, false).setAllowChange(false); } else { directions = ConnectIDSecondaryPhoneNumberDirections.actionConnectidSecondaryPhoneFragmentToConnectidMessage(getString(R.string.connect_recovery_alt_title), getString(R.string.connect_recovery_alt_message), ConnectConstants.CONNECT_VERIFY_ALT_PHONE_MESSAGE, getString(R.string.connect_password_fail_button), getString(R.string.connect_recovery_alt_change_button), null, null); } } default -> { } } if (directions == null) { Toast.makeText(getContext(), R.string.connect_navigation_error, Toast.LENGTH_SHORT).show(); return; } Navigation.findNavController(binding.continueButton).navigate(directions); }
120-169: 🛠️ Refactor suggestion
Add loading state and null checks.
Consider the following improvements:
- Add a loading state during server calls to prevent multiple submissions.
- Add null checks for context before making server calls.
Apply this diff to improve error handling and user experience:
public void handleButtonPress() { + if (getContext() == null) { + return; + } + binding.continueButton.setEnabled(false); String phone = PhoneNumberHelper.buildPhoneNumber(binding.countryCode.getText().toString(), binding.connectPrimaryPhoneInput.getText().toString()); ConnectUserRecord user = ConnectManager.getUser(getContext()); String existing = user != null ? user.getPrimaryPhone() : existingPhone; if (method.equals(ConnectConstants.METHOD_CHANGE_ALTERNATE)) { existing = user != null ? user.getAlternatePhone() : null; } if (user != null && existing != null && !existing.equals(phone)) { IApiCallback callback = new IApiCallback() { @Override public void processSuccess(int responseCode, InputStream responseData) { skipPhoneNumberCheck = false; + binding.continueButton.setEnabled(true); finish(true, phone); } @Override public void processFailure(int responseCode, IOException e) { skipPhoneNumberCheck = false; + binding.continueButton.setEnabled(true); Toast.makeText(getContext(), getString(R.string.connect_phone_change_error), Toast.LENGTH_SHORT).show(); } @Override public void processNetworkFailure() { skipPhoneNumberCheck = false; + binding.continueButton.setEnabled(true); ConnectNetworkHelper.showNetworkError(getContext()); } @Override public void processOldApiError() { skipPhoneNumberCheck = false; + binding.continueButton.setEnabled(true); ConnectNetworkHelper.showOutdatedApiError(getContext()); } };📝 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 void handleButtonPress() { if (getContext() == null) { return; } binding.continueButton.setEnabled(false); String phone = PhoneNumberHelper.buildPhoneNumber(binding.countryCode.getText().toString(), binding.connectPrimaryPhoneInput.getText().toString()); ConnectUserRecord user = ConnectManager.getUser(getContext()); String existing = user != null ? user.getPrimaryPhone() : existingPhone; if (method.equals(ConnectConstants.METHOD_CHANGE_ALTERNATE)) { existing = user != null ? user.getAlternatePhone() : null; } if (user != null && existing != null && !existing.equals(phone)) { IApiCallback callback = new IApiCallback() { @Override public void processSuccess(int responseCode, InputStream responseData) { skipPhoneNumberCheck = false; binding.continueButton.setEnabled(true); finish(true, phone); } @Override public void processFailure(int responseCode, IOException e) { skipPhoneNumberCheck = false; binding.continueButton.setEnabled(true); Toast.makeText(getContext(), getString(R.string.connect_phone_change_error), Toast.LENGTH_SHORT).show(); } @Override public void processNetworkFailure() { skipPhoneNumberCheck = false; binding.continueButton.setEnabled(true); ConnectNetworkHelper.showNetworkError(getContext()); } @Override public void processOldApiError() { skipPhoneNumberCheck = false; binding.continueButton.setEnabled(true); ConnectNetworkHelper.showOutdatedApiError(getContext()); } }; //Update the phone number with the server boolean isBusy; if (method.equals(ConnectConstants.METHOD_CHANGE_ALTERNATE)) { ApiConnectId.updateUserProfile(getContext(), user.getUserId(), user.getPassword(), null, phone, callback); } else { ApiConnectId.changePhone(getContext(), user.getUserId(), user.getPassword(), existing, phone, callback); } } else { finish(true, phone); } }app/src/org/commcare/views/connect/ConnectEditText.java (1)
159-184: 🛠️ Refactor suggestion
Add bounds check and tint compatibility check.
- Add validation for drawable dimensions.
- Check if tinting is supported before applying it.
Apply this diff to improve drawable handling:
private void setDrawables( Drawable drawableStart, Drawable drawableEnd, boolean drawableStartVisible, boolean drawableEndVisible, int drawableWidth, int drawableHeight, int tintColor, int paddingLeft, int paddingRight, int drawablePadding ) { + if (drawableWidth <= 0 || drawableHeight <= 0) { + return; + } if (drawableStart != null && drawableStartVisible) { drawableStart.setBounds(paddingLeft, 0, drawableWidth + paddingLeft, drawableHeight); - drawableStart.setTint(tintColor); + if (drawableStart.mutate().getConstantState() != null) { + drawableStart.setTint(tintColor); + } } if (drawableEnd != null && drawableEndVisible) { drawableEnd.setBounds(-paddingRight, 0, drawableWidth - paddingRight, drawableHeight); - drawableEnd.setTint(tintColor); + if (drawableEnd.mutate().getConstantState() != null) { + drawableEnd.setTint(tintColor); + } }📝 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.private void setDrawables( Drawable drawableStart, Drawable drawableEnd, boolean drawableStartVisible, boolean drawableEndVisible, int drawableWidth, int drawableHeight, int tintColor, int paddingLeft, int paddingRight, int drawablePadding ) { if (drawableWidth <= 0 || drawableHeight <= 0) { return; } if (drawableStart != null && drawableStartVisible) { drawableStart.setBounds(paddingLeft, 0, drawableWidth + paddingLeft, drawableHeight); if (drawableStart.mutate().getConstantState() != null) { drawableStart.setTint(tintColor); } } if (drawableEnd != null && drawableEndVisible) { drawableEnd.setBounds(-paddingRight, 0, drawableWidth - paddingRight, drawableHeight); if (drawableEnd.mutate().getConstantState() != null) { drawableEnd.setTint(tintColor); } } setCompoundDrawables(drawableStartVisible ? drawableStart : null, null, drawableEndVisible ? drawableEnd : null, null); setCompoundDrawablePadding(dpToPx(drawablePadding)); }app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java (2)
172-213: 🛠️ Refactor suggestion
Use ConnectUserResponseParser for consistent handling.
Based on previous feedback, server responses containing user data should be parsed using ConnectUserResponseParser to ensure consistent handling of payment information and secondary phone verification across all authentication flows.
Apply this diff to use the standard parser:
- String username = null; - String name = null; - try { - String responseAsString = new String( - StreamsUtil.inputStreamToByteArray(responseData)); - if (responseAsString.length() > 0) { - JSONObject json = new JSONObject(responseAsString); - String key = ConnectConstants.CONNECT_KEY_USERNAME; - if (json.has(key)) { - username = json.getString(key); - } - - key = ConnectConstants.CONNECT_KEY_NAME; - if (json.has(key)) { - name = json.getString(key); - } - - key = ConnectConstants.CONNECT_KEY_DB_KEY; - if (json.has(key)) { - ConnectDatabaseHelper.handleReceivedDbPassphrase(context, json.getString(key)); - } - - ConnectUserRecord user = new ConnectUserRecord(phone, username, - password, name, ""); - - key = ConnectConstants.CONNECT_KEY_VALIDATE_SECONDARY_PHONE_BY; - user.setSecondaryPhoneVerified(!json.has(key) || json.isNull(key)); - if (!user.getSecondaryPhoneVerified()) { - user.setSecondaryPhoneVerifyByDate(DateUtils.parseDate(json.getString(key))); - } + try { + ConnectUserRecord user = ConnectUserResponseParser.parseUserResponse(responseData, phone, password); + if (user != null) { ConnectUserDatabaseUtil.storeUser(context, user); }📝 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 void processSuccess(int responseCode, InputStream responseData) { try { ConnectUserRecord user = ConnectUserResponseParser.parseUserResponse(responseData, phone, password); if (user != null) { ConnectUserDatabaseUtil.storeUser(context, user); } } catch (IOException | JSONException e) { Logger.exception("Parsing return from OTP request", e); } logRecoveryResult(true); finish(true, false); }
157-231:
⚠️ Potential issueUse secure password comparison and add rate limiting.
- Replace
equalswith a constant-time comparison to prevent timing attacks.- Add rate limiting to prevent brute force attempts.
Apply this diff to improve security:
+ private static boolean secureEquals(String a, String b) { + if (a == null || b == null) { + return false; + } + if (a.length() != b.length()) { + return false; + } + int result = 0; + for (int i = 0; i < a.length(); i++) { + result |= a.charAt(i) ^ b.charAt(i); + } + return result == 0; + } + public void handleButtonPress() { String password = Objects.requireNonNull(binding.connectPasswordVerifyInput.getText()).toString(); ConnectUserRecord user = ConnectUserDatabaseUtil.getUser(requireActivity()); if (user != null) { + long lastAttemptTime = ConnectDatabaseHelper.getLastPasswordAttemptTime(requireActivity()); + long currentTime = System.currentTimeMillis(); + if (currentTime - lastAttemptTime < 1000) { // 1 second delay between attempts + Toast.makeText(requireActivity(), R.string.connect_password_rate_limit, Toast.LENGTH_SHORT).show(); + return; + } + ConnectDatabaseHelper.setLastPasswordAttemptTime(requireActivity(), currentTime); //If we have the password stored locally, no need for network call - if (password.equals(user.getPassword())) { + if (secureEquals(password, user.getPassword())) {📝 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.private static boolean secureEquals(String a, String b) { if (a == null || b == null) { return false; } if (a.length() != b.length()) { return false; } int result = 0; for (int i = 0; i < a.length(); i++) { result |= a.charAt(i) ^ b.charAt(i); } return result == 0; } public void handleButtonPress() { String password = Objects.requireNonNull(binding.connectPasswordVerifyInput.getText()).toString(); ConnectUserRecord user = ConnectUserDatabaseUtil.getUser(requireActivity()); if (user != null) { long lastAttemptTime = ConnectDatabaseHelper.getLastPasswordAttemptTime(requireActivity()); long currentTime = System.currentTimeMillis(); if (currentTime - lastAttemptTime < 1000) { // 1 second delay between attempts Toast.makeText(requireActivity(), R.string.connect_password_rate_limit, Toast.LENGTH_SHORT).show(); return; } ConnectDatabaseHelper.setLastPasswordAttemptTime(requireActivity(), currentTime); //If we have the password stored locally, no need for network call if (secureEquals(password, user.getPassword())) { logRecoveryResult(true); finish(true, false); } else { handleWrongPassword(); } } else { final Context context = requireActivity(); ApiConnectId.checkPassword(requireActivity(), phone, secretKey, password, new IApiCallback() { @Override public void processSuccess(int responseCode, InputStream responseData) { String username = null; String name = null; try { String responseAsString = new String( StreamsUtil.inputStreamToByteArray(responseData)); if (responseAsString.length() > 0) { JSONObject json = new JSONObject(responseAsString); String key = ConnectConstants.CONNECT_KEY_USERNAME; if (json.has(key)) { username = json.getString(key); } key = ConnectConstants.CONNECT_KEY_NAME; if (json.has(key)) { name = json.getString(key); } key = ConnectConstants.CONNECT_KEY_DB_KEY; if (json.has(key)) { ConnectDatabaseHelper.handleReceivedDbPassphrase(context, json.getString(key)); } ConnectUserRecord user = new ConnectUserRecord(phone, username, password, name, ""); key = ConnectConstants.CONNECT_KEY_VALIDATE_SECONDARY_PHONE_BY; user.setSecondaryPhoneVerified(!json.has(key) || json.isNull(key)); if (!user.getSecondaryPhoneVerified()) { user.setSecondaryPhoneVerifyByDate(DateUtils.parseDate(json.getString(key))); } //TODO: Need to get secondary phone from server ConnectUserDatabaseUtil.storeUser(context, user); } } catch (IOException | JSONException e) { Logger.exception("Parsing return from OTP request", e); } logRecoveryResult(true); finish(true, false); } @Override public void processFailure(int responseCode, IOException e) { handleWrongPassword(); } @Override public void processNetworkFailure() { ConnectNetworkHelper.showOutdatedApiError(requireActivity().getApplicationContext()); } @Override public void processOldApiError() { ConnectNetworkHelper.showOutdatedApiError(requireActivity().getApplicationContext()); } }); } }app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java (1)
95-131:
⚠️ Potential issueConsider security implications of error handling.
The error handling in the biometric callback reveals too much information about the authentication state. The error message "Authentication failed" could help attackers determine if a fingerprint was partially matched.
Apply this diff to make the error message more generic:
- Toast.makeText(requireActivity().getApplicationContext(), "Authentication failed", + Toast.makeText(requireActivity().getApplicationContext(), R.string.connect_biometric_error, Toast.LENGTH_SHORT) .show();📝 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.private BiometricPrompt.AuthenticationCallback preparePromptCallbacks() { final Context context = requireActivity(); return new BiometricPrompt.AuthenticationCallback() { @Override public void onAuthenticationError(int errorCode, @NonNull CharSequence errString) { super.onAuthenticationError(errorCode, errString); if (attemptingFingerprint) { attemptingFingerprint = false; if (BiometricsHelper.isPinConfigured(context, biometricManager) && allowPassword) { //Automatically try password, it's the only option performPinUnlock(); } else { //Automatically try password, it's the only option performPasswordUnlock(); } } } @Override public void onAuthenticationSucceeded( @NonNull BiometricPrompt.AuthenticationResult result) { super.onAuthenticationSucceeded(result); logSuccess(); finish(true, false); } @Override public void onAuthenticationFailed() { super.onAuthenticationFailed(); - Toast.makeText(requireActivity().getApplicationContext(), "Authentication failed", + Toast.makeText(requireActivity().getApplicationContext(), R.string.connect_biometric_error, Toast.LENGTH_SHORT) .show(); } }; }app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java (2)
206-228: 🛠️ Refactor suggestion
Improve state management of skipPhoneNumberCheck flag.
The
skipPhoneNumberCheckflag is toggled multiple times within the method, which could lead to race conditions or unexpected behavior if the method is called concurrently.Consider using a more robust state management approach or removing the flag entirely if possible.
411-421:
⚠️ Potential issueImprove random ID generation security.
The current implementation creates a new Random instance for each character, which is inefficient and potentially less secure. Consider using SecureRandom and reusing the instance.
Apply this diff:
- String charSet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; - StringBuilder userId = new StringBuilder(); - for (int i = 0; i < idLength; i++) { - userId.append(charSet.charAt(new Random().nextInt(charSet.length()))); - } + String charSet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + StringBuilder userId = new StringBuilder(); + SecureRandom secureRandom = new SecureRandom(); + for (int i = 0; i < idLength; i++) { + userId.append(charSet.charAt(secureRandom.nextInt(charSet.length()))); + }📝 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.private String generateUserId() { int idLength = 20; String charSet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; StringBuilder userId = new StringBuilder(); SecureRandom secureRandom = new SecureRandom(); for (int i = 0; i < idLength; i++) { userId.append(charSet.charAt(secureRandom.nextInt(charSet.length()))); } return userId.toString(); }app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java (2)
182-200: 🛠️ Refactor suggestion
Add PIN complexity requirements.
The current implementation only checks PIN length but doesn't enforce any complexity requirements (e.g., no repeated digits, no sequential numbers).
Consider adding PIN complexity validation to enhance security.
343-349:
⚠️ Potential issueAdd rate limiting for PIN attempts.
The current implementation tracks failure attempts but doesn't implement any cooldown period after multiple failures, which could make it vulnerable to brute force attacks.
Consider adding exponential backoff for repeated failures:
private void handleWrongPin() { int failures = ConnectManager.getFailureAttempt() + 1; ConnectManager.setFailureAttempt(failures); logRecoveryResult(false); clearPin(); // Add exponential backoff if (failures >= 3) { long delayMillis = (long) Math.pow(2, failures - 3) * 1000; // Start with 1s, then 2s, 4s, etc. binding.connectPinButton.setEnabled(false); new Handler().postDelayed(() -> binding.connectPinButton.setEnabled(true), delayMillis); } finish(false, failures >= MaxFailures, null); }app/src/org/commcare/activities/LoginActivity.java (1)
801-831: 🛠️ Refactor suggestion
Add null checks and simplify complex conditional logic.
The credential check method has complex nested conditions and potential NPE risks when accessing appIdDropdownList.
Consider refactoring to:
private void checkForSavedCredentials() { boolean loginWithConnectIDVisible = false; if (ConnectManager.isConnectIdConfigured()) { if (appLaunchedFromConnect && !connectLaunchPerformed) { handleConnectLaunch(); } else { - int selectorIndex = uiController.getSelectedAppIndex(); - if(selectorIndex > 0) { - String selectedAppId = appIdDropdownList.size() > 0 ? appIdDropdownList.get(selectorIndex) : ""; + handleNormalLaunch(); } } uiController.setConnectIdLoginState(loginWithConnectIDVisible); } +private void handleConnectLaunch() { + loginWithConnectIDVisible = true; + uiController.setConnectButtonVisible(false); + uiController.setUsername(getString(R.string.login_input_auto)); + uiController.setPasswordOrPin(getString(R.string.login_input_auto)); + if (presetAppId != null && !presetAppId.equals(CommCareApplication.instance().getCurrentApp().getUniqueId())) { + connectLaunchPerformed = true; + initiateLoginAttempt(uiController.isRestoreSessionChecked()); + } +} +private void handleNormalLaunch() { + int selectorIndex = uiController.getSelectedAppIndex(); + if (selectorIndex > 0 && appIdDropdownList != null && !appIdDropdownList.isEmpty()) { + String selectedAppId = appIdDropdownList.get(selectorIndex); + String seatedAppId = CommCareApplication.instance().getCurrentApp().getUniqueId(); + if (!uiController.isAppSelectorVisible() || selectedAppId.equals(seatedAppId)) { + loginWithConnectIDVisible = ConnectManager.isLoginManagedByConnectId( + seatedAppId, + uiController.getEnteredUsername() + ); + } + } else { + loginWithConnectIDVisible = true; + } +}Committable suggestion skipped: line range outside the PR's diff.
app/src/org/commcare/activities/CommCareSetupActivity.java (1)
644-650:
⚠️ Potential issueRemove commented code and verify implementation completeness.
The updateConnectButton method contains commented code that suggests incomplete implementation.
Either implement the commented functionality or remove it:
private void updateConnectButton() { installFragment.updateConnectButton(!fromManager && !fromExternal && ConnectManager.isConnectIdConfigured(), v -> { ConnectManager.unlockConnect(this, success -> { - //ConnectManager.goToConnectJobsList(this); + // TODO: Implement navigation to jobs list }); }); }📝 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.private void updateConnectButton() { installFragment.updateConnectButton(!fromManager && !fromExternal && ConnectManager.isConnectIdConfigured(), v -> { ConnectManager.unlockConnect(this, success -> { // TODO: Implement navigation to jobs list }); }); }app/res/layout/fragment_recovery_code.xml (1)
29-39:
⚠️ Potential issueTypo in Recovery Code Title ID
The ID@+id/recovery_code_tilteappears to contain a typo. Renaming it to@+id/recovery_code_titlewould improve clarity and maintain consistency.app/res/navigation/nav_graph_connectid.xml (1)
47-51:
⚠️ Potential issueTypo in Fragment Class Name
The fragment declared with ID
connectid_phone_verify(line 49) uses the class
org.commcare.fragments.connectId.ConnectIdPhoneVerificationFragmnet
which appears to have a typo ("Fragmnet" instead of "Fragment"). Please verify and correct the class name to ensure proper instantiation.Proposed diff:
- android:name="org.commcare.fragments.connectId.ConnectIdPhoneVerificationFragmnet" + android:name="org.commcare.fragments.connectId.ConnectIdPhoneVerificationFragment"📝 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.<fragment android:id="@+id/connectid_phone_verify" android:name="org.commcare.fragments.connectId.ConnectIdPhoneVerificationFragment" android:label="fragment_connectid_phone_verify" tools:layout="@layout/screen_connect_phone_verify">app/src/org/commcare/connect/network/ApiConnect.java (3)
22-31: 🛠️ Refactor suggestion
Improve error handling for token retrieval failure.
The current implementation silently fails when token retrieval fails. The callback should be notified of the failure.
ConnectSsoHelper.retrieveConnectTokenAsync(context, token -> { if(token == null) { + handler.onError("Failed to retrieve token"); return; }📝 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.ConnectSsoHelper.retrieveConnectTokenAsync(context, token -> { if(token == null) { handler.onError("Failed to retrieve token"); return; } String url = context.getString(R.string.ConnectOpportunitiesURL, BuildConfig.CCC_HOST); Multimap<String, String> params = ArrayListMultimap.create(); ConnectNetworkHelper.get(context, url, API_VERSION_CONNECT, token, params, false, handler); });
22-31: 🛠️ Refactor suggestion
Add timeout handling for token retrieval.
The token retrieval callback could potentially never complete, leaving the API call hanging indefinitely.
Consider adding a timeout mechanism to the token retrieval process:
+ private static final long TOKEN_RETRIEVAL_TIMEOUT_MS = 30000; // 30 seconds + ConnectSsoHelper.retrieveConnectTokenAsync(context, token -> { if(token == null) { return; } // ... rest of the code - }); + }, TOKEN_RETRIEVAL_TIMEOUT_MS);📝 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.private static final long TOKEN_RETRIEVAL_TIMEOUT_MS = 30000; // 30 seconds ConnectSsoHelper.retrieveConnectTokenAsync(context, token -> { if(token == null) { return; } String url = context.getString(R.string.ConnectOpportunitiesURL, BuildConfig.CCC_HOST); Multimap<String, String> params = ArrayListMultimap.create(); ConnectNetworkHelper.get(context, url, API_VERSION_CONNECT, token, params, false, handler); }, TOKEN_RETRIEVAL_TIMEOUT_MS);
17-34: 🛠️ Refactor suggestion
Add null context validation.
The method should validate that the context parameter is not null to prevent NullPointerException.
public static boolean getConnectOpportunities(Context context, IApiCallback handler) { + if (context == null) { + throw new IllegalArgumentException("Context cannot be null"); + } if (ConnectNetworkHelper.isBusy()) { return false; } // ... rest of the code }📝 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 boolean getConnectOpportunities(Context context, IApiCallback handler) { if (context == null) { throw new IllegalArgumentException("Context cannot be null"); } if (ConnectNetworkHelper.isBusy()) { return false; } ConnectSsoHelper.retrieveConnectTokenAsync(context, token -> { if(token == null) { return; } String url = context.getString(R.string.ConnectOpportunitiesURL, BuildConfig.CCC_HOST); Multimap<String, String> params = ArrayListMultimap.create(); ConnectNetworkHelper.get(context, url, API_VERSION_CONNECT, token, params, false, handler); }); return true; }app/src/org/commcare/views/connect/connecttextview/connecttextview/ConnectRegularTextView.java (1)
1-1:
⚠️ Potential issueFix duplicated package segment.
The package name contains a duplicated "connecttextview" segment which should be removed.
-package org.commcare.views.connect.connecttextview.connecttextview; +package org.commcare.views.connect.connecttextview;📝 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.-package org.commcare.views.connect.connecttextview.connecttextview; +package org.commcare.views.connect.connecttextview;
|
|
@coderabbitai I pushed a fix in commit 1f6f083, please review it. |
|
@pm-dimagi I've reviewed the changes in commit 1f6f083. The fixes look good and include several important improvements:
The changes improve the overall stability, security and maintainability of the code. Nice work! 🐰 happy to see such comprehensive improvements 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 65588 ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 28
🔭 Outside diff range comments (2)
app/src/org/commcare/connect/database/ConnectDatabaseUtils.java (2)
53-64: Fix inconsistency in passphrase retrieval logic.The method accepts a
localparameter but doesn't use it in the internal call togetConnectDbPassphrase, which hardcodeslocal=true. This means remote passphrases will never be retrieved.Apply this diff to fix the issue:
- public static String getConnectDbEncodedPassphrase(Context context,boolean local) { + public static String getConnectDbEncodedPassphrase(Context context, boolean local) { try { - byte[] passBytes = getConnectDbPassphrase(context); + ConnectKeyRecord record = ConnectDatabaseUtils.getKeyRecord(local); + if (record != null) { + byte[] passBytes = EncryptionUtils.decryptFromBase64String(context, record.getEncryptedPassphrase()); if (passBytes != null) { return Base64.encode(passBytes); } + } } catch (Exception e) { Logger.exception("Getting DB passphrase", e); } return null; }
66-79: Update error handling and call sites for getConnectDbPassphraseThe current implementation still uses the old signature without the boolean parameter, which means:
- Typo & Redundant logging: The error message still contains "paraphrase" (instead of "passphrase") and logs an error via
CrashUtil.logbefore throwing an exception.- Hardcoded Parameter: The method internally calls
ConnectDatabaseUtils.getKeyRecord(true)instead of using a parameter value.- Call Sites Needing Update: Our search shows that both in
ConnectDatabaseHelper.java:60and withinConnectDatabaseUtils.java:55, calls togetConnectDbPassphrasedo not pass a boolean value. This confirms that if the method signature is updated to include aboolean local, then these call sites must also be updated to pass an appropriate value (likelytrue).These issues support the original review comment's suggestions to standardize the error messages, remove redundant logging, and refactor the method signature along with its callers.
🔗 Analysis chain
Improve error handling and consistency.
Several issues in the error handling:
- Typo in the error message ("paraphrase").
- Inconsistent error messages between logging and exception.
- Redundant logging before throwing exception.
- Hardcoded
local=trueis inconsistent withgetConnectDbEncodedPassphrase.Apply this diff to improve the error handling:
- public static byte[] getConnectDbPassphrase(Context context) { + public static byte[] getConnectDbPassphrase(Context context, boolean local) { try { - ConnectKeyRecord record = ConnectDatabaseUtils.getKeyRecord(true); + ConnectKeyRecord record = ConnectDatabaseUtils.getKeyRecord(local); if (record != null) { return EncryptionUtils.decryptFromBase64String(context, record.getEncryptedPassphrase()); - }else{ - CrashUtil.log("We dont find paraphrase in db"); - throw new RuntimeException("We dont find a record in db to get passphrase"); + } else { + throw new RuntimeException("No passphrase record found in database"); } } catch (Exception e) { Logger.exception("Getting DB passphrase", e); throw new RuntimeException(e); } }This change will require updating all callers to provide the
localparameter.Let's verify the callers that need to be updated:
I'll run another shell script using
rgto search for calls togetConnectDbPassphraseacross the entire repository so that we can verify if there are any callers that need updating.Once we see the output from this command, we can determine whether any callers need to be updated with the new boolean parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to getConnectDbPassphrase ast-grep --pattern 'getConnectDbPassphrase($context)'Length of output: 55
Script:
#!/bin/bash # Search for calls to getConnectDbPassphrase in the repository rg "getConnectDbPassphrase\\(" -n .Length of output: 477
🧹 Nitpick comments (57)
app/src/org/commcare/views/connect/CircleProgressBar.java (4)
14-25: Add support for XML attributes.Consider adding support for XML attributes to allow customization through layout files. This would improve the reusability of the component.
Add the following attributes to
attrs.xml:<declare-styleable name="CircleProgressBar"> <attr name="progress" format="float" /> <attr name="strokeWidth" format="dimension" /> <attr name="startAngle" format="integer" /> <attr name="progressColor" format="color" /> <attr name="circleBackgroundColor" format="color" /> </declare-styleable>Then update the constructor to handle these attributes:
public CircleProgressBar(Context context, AttributeSet attrs) { super(context, attrs); TypedArray a = context.obtainStyledAttributes(attrs, R.styleable.CircleProgressBar); try { progress = a.getFloat(R.styleable.CircleProgressBar_progress, 0f); strokeWidth = a.getDimension(R.styleable.CircleProgressBar_strokeWidth, 10f); startAngle = a.getInteger(R.styleable.CircleProgressBar_startAngle, 270); progressColor = a.getColor(R.styleable.CircleProgressBar_progressColor, Color.BLUE); backgroundColor = a.getColor(R.styleable.CircleProgressBar_circleBackgroundColor, Color.LTGRAY); } finally { a.recycle(); } init(); }
41-54: Consider adding layer type optimization.For better performance, consider setting the layer type to LAYER_TYPE_HARDWARE when the progress bar is animating.
Add the following method:
public void setAnimating(boolean animating) { setLayerType(animating ? LAYER_TYPE_HARDWARE : LAYER_TYPE_NONE, null); }
85-85: Fix formatting: Add spaces around operator.Add spaces around the assignment operator.
- this.progress=progress; + this.progress = progress;
103-103: Remove debug logging.Consider removing the debug log statement or using a more appropriate log level for production code.
- Log.d("CircleProgressBar", "Setting progress color to: " + color);app/src/org/commcare/views/connect/ConnectEditText.java (2)
148-151: Avoid reassigning text size insidesetHintAttributes.Currently,
setTextSize(size)insetHintAttributesapplies the same size for both text and hint. If you need uniform sizing, keep it explicit. Otherwise, create a distinct parameter or logic for hint text size vs. actual text size.
196-217: Prevent overwriting custom touch listeners.
setupDrawableClickListenersunconditionally sets anOnTouchListener, potentially overriding any externalOnTouchListenerthat the consumer code might set. This may break event handling from external callers.Consider a pattern that chains or merges with an existing listener if one is already assigned.
app/src/org/commcare/activities/LoginActivityUIController.java (4)
96-99: Add @NotNull annotations to UI elements.Based on the codebase conventions, UI elements that are required for the functionality should be annotated with @NotNull to prevent null pointer exceptions.
- @UiElement(value = R.id.connect_login_button) - private Button connectLoginButton; - @UiElement(value = R.id.login_or) - private ConnectMediumTextView orLabel; + @UiElement(value = R.id.connect_login_button) + @NotNull + private Button connectLoginButton; + @UiElement(value = R.id.login_or) + @NotNull + private ConnectMediumTextView orLabel;
207-210: Simplify complex condition and improve readability.The condition for showing login inputs is complex and could be simplified for better readability.
- boolean noApps = readyApps.isEmpty(); - setLoginInputsVisibility(!noApps); - if (!ConnectManager.isConnectIdConfigured() && readyApps.size() == 1 || presetAppRecord != null) { - setLoginInputsVisibility(true); + boolean hasApps = !readyApps.isEmpty(); + boolean isSingleAppMode = readyApps.size() == 1; + boolean shouldShowLoginInputs = hasApps && (!ConnectManager.isConnectIdConfigured() && isSingleAppMode || presetAppRecord != null); + setLoginInputsVisibility(shouldShowLoginInputs); + if (shouldShowLoginInputs) {
515-518: Improve comment clarity and formatting.The comment about language changes could be clearer and should follow proper JavaDoc format.
- /** - * included these lines because when a user changes the language from the system settings while in the app, - * the strings should be translate correctly when they return to the app. That's why I added this code. - */ + // Ensure correct translation when user changes system language settings while in the app
500-527: Refactor setConnectIdLoginState for better separation of concerns.The method handles multiple responsibilities: state management, UI updates, and localization. Consider breaking it down into smaller, focused methods.
+ private void updateLoginButtonText(boolean useConnectId) { + String text = useConnectId + ? activity.getString(R.string.login_button_connectid) + : Localization.get("login.button"); + loginButton.setText(text); + } + + private void updatePasswordFieldState(boolean useConnectId) { + passwordOrPin.setBackgroundColor(getResources().getColor( + useConnectId ? R.color.grey_light : R.color.white)); + passwordOrPin.setInputType(useConnectId + ? InputType.TYPE_CLASS_TEXT + : (InputType.TYPE_CLASS_TEXT | InputType.TYPE_TEXT_VARIATION_PASSWORD)); + if (useConnectId) { + passwordOrPin.setText(R.string.login_password_by_connect); + passwordOrPin.clearFocus(); + } + } + protected void setConnectIdLoginState(boolean useConnectId) { if (!useConnectId && loginManagedByConnectId) { setPasswordOrPin(""); } loginManagedByConnectId = useConnectId; - String text; - if (useConnectId) { - text = activity.getString(R.string.login_button_connectid); - } else { - text = Localization.get("login.button"); - } - loginButton.setText(text); + updateLoginButtonText(useConnectId); connectLoginButton.setText(activity.getString(R.string.connect_button_logged_in)); - passwordOrPin.setBackgroundColor(getResources().getColor(useConnectId ? R.color.grey_light : R.color.white)); - if (useConnectId) { - passwordOrPin.setText(R.string.login_password_by_connect); - passwordOrPin.clearFocus(); - } - passwordOrPin.setInputType(useConnectId ? InputType.TYPE_CLASS_TEXT : (InputType.TYPE_CLASS_TEXT | InputType.TYPE_TEXT_VARIATION_PASSWORD)); + updatePasswordFieldState(useConnectId); }app/src/org/commcare/connect/network/ConnectSsoHelper.java (6)
24-26: Add documentation for the TokenCallback interface.Consider adding Javadoc to explain:
- The purpose of the interface
- When the callback is invoked
- What null token means
+/** + * Callback interface for asynchronous token retrieval operations. + */ public interface TokenCallback { + /** + * Called when token retrieval is complete. + * @param token The retrieved token, or null if retrieval failed + */ void tokenRetrieved(AuthInfo.TokenAuth token); }
28-29: Fix typo in comment.- //Used for aynchronously retrieving HQ or SSO token + //Used for asynchronously retrieving HQ or SSO token
33-33: Fix inconsistent spacing around operators.- private final String TAG=TokenTask.class.getSimpleName(); + private final String TAG = TokenTask.class.getSimpleName();
51-55: Improve error handling and logging.The current error handling logs the error but doesn't provide context about what operation failed. Consider:
- Using more specific exception types
- Adding context to the error message
- Logging at appropriate levels (ERROR for unexpected failures, DEBUG for expected scenarios)
return ApiConnectId.retrieveConnectIdTokenSync(context); - }catch (Exception e){ - Log.e(TAG,e.toString()); + } catch (Exception e) { + Log.e(TAG, "Failed to retrieve ConnectID token: " + e.getMessage(), e); return null; }
71-75: Add documentation for public methods.Consider adding Javadoc to explain the purpose, parameters, and return values of public methods.
+ /** + * Asynchronously retrieves an HQ SSO token. + * @param context The application context + * @param hqUsername The HQ username to retrieve token for + * @param linkHqUser Whether to link the HQ user + * @param callback Callback to receive the token + */ public static void retrieveHqSsoTokenAsync(Context context, String hqUsername, boolean linkHqUser, TokenCallback callback) { + /** + * Asynchronously retrieves a Connect token. + * @param context The application context + * @param callback Callback to receive the token + */ public static void retrieveConnectTokenAsync(Context context, TokenCallback callback) {Also applies to: 115-119
77-113: Consider breaking down the sync method for better readability.The
retrieveHqSsoTokenSyncmethod is quite long and handles multiple responsibilities. Consider extracting the token retrieval logic into a separate method.public static AuthInfo.TokenAuth retrieveHqSsoTokenSync(Context context, String hqUsername, boolean performLink) { if (!ConnectManager.isConnectIdConfigured()) { return null; } String seatedAppId = CommCareApplication.instance().getCurrentApp().getUniqueId(); ConnectLinkedAppRecord appRecord = ConnectAppDatabaseUtil.getAppData(context, seatedAppId, hqUsername); if (appRecord == null) { return null; } AuthInfo.TokenAuth hqTokenAuth = ConnectManager.getTokenCredentialsForApp(seatedAppId, hqUsername); - if (hqTokenAuth == null && (performLink || appRecord.getWorkerLinked())) { - AuthInfo.TokenAuth connectIdToken = ApiConnectId.retrieveConnectIdTokenSync(context); - - if (connectIdToken != null) { - if(!appRecord.getWorkerLinked()) { - ApiConnectId.linkHqWorker(context, hqUsername, appRecord, connectIdToken.bearerToken); - } - - try { - hqTokenAuth = ApiConnectId.retrieveHqTokenApi(context, hqUsername, connectIdToken.bearerToken); - } catch (MalformedURLException e) { - Logger.log(LogTypes.TYPE_EXCEPTION,e.toString()); - throw new RuntimeException(e); - } - } + if (hqTokenAuth == null && (performLink || appRecord.getWorkerLinked())) { + hqTokenAuth = retrieveAndLinkHqToken(context, hqUsername, appRecord); } return hqTokenAuth; } + + private static AuthInfo.TokenAuth retrieveAndLinkHqToken(Context context, String hqUsername, ConnectLinkedAppRecord appRecord) { + AuthInfo.TokenAuth connectIdToken = ApiConnectId.retrieveConnectIdTokenSync(context); + if (connectIdToken == null) { + return null; + } + + if (!appRecord.getWorkerLinked()) { + ApiConnectId.linkHqWorker(context, hqUsername, appRecord, connectIdToken.bearerToken); + } + + try { + return ApiConnectId.retrieveHqTokenApi(context, hqUsername, connectIdToken.bearerToken); + } catch (MalformedURLException e) { + Logger.log(LogTypes.TYPE_EXCEPTION, "Invalid HQ URL: " + e.getMessage(), e); + return null; + } + }app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (1)
64-65: Make boolean parameters more explicit using constants.The boolean parameters
trueandfalseare magic values that could be made more explicit to improve code readability and maintainability.Consider defining constants and using them instead:
+ private static final boolean IS_LOCAL_PASSPHRASE = true; + private static final boolean IS_REMOTE_PASSPHRASE = false; - String remotePassphrase = ConnectDatabaseUtils.getConnectDbEncodedPassphrase(context,false); - String localPassphrase = ConnectDatabaseUtils.getConnectDbEncodedPassphrase(context,true); + String remotePassphrase = ConnectDatabaseUtils.getConnectDbEncodedPassphrase(context, IS_REMOTE_PASSPHRASE); + String localPassphrase = ConnectDatabaseUtils.getConnectDbEncodedPassphrase(context, IS_LOCAL_PASSPHRASE);app/res/values/strings.xml (4)
462-472: Consistency in Consent & Registration Strings:
The new ConnectID registration and consent strings (lines 462–472) are comprehensive and clear. One minor nitpick is the inconsistent use of apostrophes: notice that line 470 uses a straight apostrophe (Dimagi's) whereas later (e.g. in similar strings like line 771) a curly apostrophe (Dimagi’s) is used. Standardizing the apostrophe style will help maintain consistency across the UI.
491-492: Improve Clarity of Alternate Phone Error Messages:
The messages on lines 491–492 could be made clearer. For example, instead of:
- "Can not use your alt. number" (line 491)
- "Alt. number must be different than primary" (line 492)
consider revising them to:
- "Cannot use your alternate number"
- "Alternate number must be different from the primary phone number"
This change improves readability and consistency.
518-518: Clarify Password Verification Prompt:
The string on line 518,"Don't have or forgot your password?", could be rephrased for clarity. A suggestion would be:
- "Don't have a password or forgot it?"
This makes the message easier to read and understand.
573-574: Standardize Negative Constructions in Error Messages:
For a smoother tone and consistency, consider using "cannot" instead of "can not" in the error messages. For instance, change:
"ID can not be empty"(line 573)"Name can not be empty"(line 574)to:
"ID cannot be empty""Name cannot be empty"
This small change helps maintain a uniform style across your error messages.app/src/org/commcare/utils/BiometricsHelper.java (3)
35-36: Prefer standard UPPER_CASE naming for final constants.Currently, the constants
StrongBiometricandPinBiometricdeviate from typical Java naming conventions forstatic finalfields. Adopting uppercase (e.g.,STRONG_BIOMETRIC,PIN_BIOMETRIC) can help maintain consistency and clarity in the code.
114-120: Avoid passingnulltoonAuthenticationSucceeded(...).Passing
nullcould lead to potential NullPointerExceptions if the callback attempts to use the result parameter. Return a properAuthenticationResultinstance or handle it carefully within the callback.if (resultCode == Activity.RESULT_OK) { - biometricPromptCallbackHolder.onAuthenticationSucceeded(null); + BiometricPrompt.AuthenticationResult dummyResult = + new BiometricPrompt.AuthenticationResult(null); + biometricPromptCallbackHolder.onAuthenticationSucceeded(dummyResult); } else { biometricPromptCallbackHolder.onAuthenticationFailed(); }
157-174: Use a dedicated request code instead ofIntentIntegrator.REQUEST_CODE.Reusing the ZXing scanning request code might cause collisions or confusion if the enrollment flow and scanning flow occur in the same activity. It’s safer to define a unique request code for biometric enrollment.
- activity.startActivityForResult(enrollIntent, IntentIntegrator.REQUEST_CODE); + private static final int BIOMETRIC_ENROLL_REQUEST_CODE = 6001; + activity.startActivityForResult(enrollIntent, BIOMETRIC_ENROLL_REQUEST_CODE);app/src/org/commcare/fragments/connectId/ConnectIdPhoneFragment.java (6)
55-68: Consider debouncing the phone number validation calls.
Frequent calls tocheckPhoneNumber()on every keystroke could flood the server with requests, especially if network latency is involved. To enhance efficiency and user experience, consider using a short delay or a debounced approach to reduce redundant validation calls.
70-79: Remove or utilize the unused Bundle innewInstance().
Currently, aBundleis created but never populated or used. This adds unnecessary overhead and confusion. If no arguments are expected, consider removing theBundleentirely.public static ConnectIdPhoneFragment newInstance() { ConnectIdPhoneFragment fragment = new ConnectIdPhoneFragment(); - Bundle args = new Bundle(); - fragment.setArguments(args); return fragment; }
81-85: Avoid overridingonCreateif no additional behavior is involved.
This method is empty and can simply be removed to reduce clutter.-@Override -public void onCreate(Bundle savedInstanceState) { - super.onCreate(savedInstanceState); -}
153-160: Unify the oldonActivityResultapproach with the new activity result API.
It's recommended to stick to one modern mechanism (e.g., ActivityResultContracts) throughout the codebase to avoid confusion and potential maintenance headaches.
212-235: Revisit the toggling ofskipPhoneNumberCheck.
Manually togglingskipPhoneNumberCheckindisplayNumber()can cause confusion since it’s set totrueandfalsemultiple times in quick succession. Consider a more self-documenting approach (e.g., local flags or separate methods) to make the control flow clearer.
370-374: Remove or implement logic inonViewCreated.
Empty lifecycle methods add noise. If there is no specific logic to implement here, consider removing this override.-@Override -public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceState) { - super.onViewCreated(view, savedInstanceState); -}app/src/org/commcare/utils/PhoneNumberHelper.java (2)
51-57: Handle potential null inputs or unexpected formatting inbuildPhoneNumber().
IfcountryCodeorphoneis null or contains unexpected characters, the current replacement logic may yield incomplete strings or cause runtime exceptions. Consider validating input parameters or throwing an early exception.
126-141: Unify return values inhandlePhoneNumberPickerResult().
This method returnsnullon parsing errors but returns an empty string ("") if the request code doesn't match. Consider returningnullin both cases or an optional-like structure for clarity.- return null; ... - return ""; + return null;app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java (3)
113-114: Remove duplicateaddTextChangedListener()calls.
Both lines 113 and 114 attach the samewatchertobinding.connectPinInput. This is likely unintentional duplication.binding.connectPinInput.addTextChangedListener(watcher); -binding.connectPinInput.addTextChangedListener(watcher);
278-283: Implement toast or error UI for unhandled error paths.
TheTODOmentions showing a toast on error. Ignoring these errors might cause silent failures. Consider completing this implementation or removing theTODO.Would you like me to open a new issue or offer a quick fix snippet to implement a basic toast?
310-317: Retrieve secondary phone from server as indicated by theTODO.
If the process flow depends on a secondary phone, the incomplete retrieval might break the user recovery flow. Completing this step can ensure consistent user experience.app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragment.java (1)
179-225: Fix naming of broadcast-related methods and variables.
registerBrodcastReciever()has two spelling errors: "broadcast" and "receiver" are misspelled. This might confuse maintainers and hamper text searches.-public void registerBrodcastReciever() { +public void registerBroadcastReceiver() {app/src/org/commcare/connect/SMSBroadcastReceiver.java (1)
18-48: Add method to unregister the SMS listener.To prevent memory leaks, add a method to unregister the listener when it's no longer needed.
+ public void clearSmsListener() { + this.smsListener = null; + }app/src/org/commcare/fragments/connectId/ConnectIdPhoneAvailableBottomSheet.java (1)
21-24: Use view binding consistently for view references.Instead of declaring individual view fields, leverage view binding throughout the class for better null safety and maintainability.
- ConnectMediumTextView phoneTextView; - Button recover; - Button back; - String phoneNumber; + private FragmentPhoneAvailableBottomSheetBinding binding; + private String phoneNumber;app/src/org/commcare/utils/CrashUtil.java (1)
59-68: Improve error handling in registerConnectUser method.The catch block should log more details about the exception to help with debugging.
public static void registerConnectUser() { if (crashlyticsEnabled && ConnectManager.isConnectIdConfigured()) { try{ + if (CommCareApplication.instance() == null) { + return; + } String userId=ConnectManager.getUser(CommCareApplication.instance()).getUserId(); + if (userId == null) { + return; + } FirebaseCrashlytics.getInstance().setCustomKey(CCC_USER, userId); }catch (Exception e){ + log("Failed to register Connect user: " + e.getMessage()); FirebaseCrashlytics.getInstance().recordException(e); } } }app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java (2)
179-214: Refactor complex navigation logic using a navigation map.The switch statement for navigation is complex and could be simplified using a navigation map.
+ private static final Map<Integer, Function<Boolean, NavDirections>> NAVIGATION_MAP = new HashMap<>(); + static { + NAVIGATION_MAP.put(ConnectConstants.CONNECT_REGISTRATION_ALTERNATE_PHONE, + success -> success ? + ConnectIDSecondaryPhoneNumberDirections.actionConnectidSecondaryPhoneFragmentToConnectidPin( + ConnectConstants.CONNECT_REGISTRATION_CONFIRM_PIN, phone, "").setRecover(false).setChange(false) : + ConnectIDSecondaryPhoneNumberDirections.actionConnectidSecondaryPhoneFragmentToConnectidPin( + ConnectConstants.CONNECT_REGISTRATION_CONFIGURE_PIN, phone, "").setRecover(false).setChange(true)); + // Add other navigation mappings + }
133-172: Extract network callback to a separate class.The network callback implementation is complex and should be extracted to improve readability and maintainability.
Consider creating a separate class
PhoneUpdateCallbackthat implementsIApiCallbackto handle the network responses.app/src/org/commcare/fragments/connectId/ConnectIdMessageFragment.java (4)
100-229: Extract navigation logic into separate methods.The
finishmethod is quite long and handles multiple navigation scenarios. Consider extracting each case into a separate method for better readability and maintainability.Example refactor:
+private NavDirections handleRegistrationSuccess(boolean success, boolean secondButton) { + if (success) { + ConnectManager.setStatus(ConnectManager.ConnectIdStatus.LoggedIn); + ConnectDatabaseHelper.setRegistrationPhase(getActivity(), ConnectConstants.CONNECT_NO_ACTIVITY); + requireActivity().setResult(RESULT_OK); + requireActivity().finish(); + } + return null; +} +private NavDirections handleRecoveryAltPhoneMessage(boolean success, boolean secondButton) { + if (success) { + if (secondButton) { + return ConnectIdMessageFragmentDirections.actionConnectidMessageSelf( + getString(R.string.connect_deactivate_account), + getString(R.string.connect_deactivate_account_message), + ConnectConstants.CONNECT_USER_DEACTIVATE_CONFIRMATION, + getString(R.string.connect_deactivate_account_delete), + getString(R.string.connect_deactivate_account_go_back), + userName, + password); + } else { + return ConnectIdMessageFragmentDirections.actionConnectidMessageToConnectidPhoneVerify( + ConnectConstants.CONNECT_RECOVERY_VERIFY_ALT_PHONE, + String.format(Locale.getDefault(), "%d", ConnectIdPhoneVerificationFragment.MethodRecoveryAlternate), + null, + ConnectIdActivity.recoverPhone, + ConnectIdActivity.recoverSecret, + ConnectIdActivity.recoveryAltPhone, + false + ).setAllowChange(false); + } + } + return null; +} public void finish(boolean success, boolean secondButton) { NavDirections directions = null; ConnectUserRecord user = ConnectUserDatabaseUtil.getUser(getActivity()); switch (callingClass) { case ConnectConstants.CONNECT_REGISTRATION_SUCCESS: directions = handleRegistrationSuccess(success, secondButton); break; case ConnectConstants.CONNECT_RECOVERY_ALT_PHONE_MESSAGE: directions = handleRecoveryAltPhoneMessage(success, secondButton); break; // ... extract other cases similarly } if (success && directions != null) { NavHostFragment.findNavController(this).navigate(directions); } }
88-94: Add accessibility support.The button handling code lacks accessibility considerations.
Add accessibility improvements:
public void setButton2Text(String buttonText) { boolean show = buttonText != null; binding.connectMessageButton2.setVisibility(show ? View.VISIBLE : View.GONE); if (show) { binding.connectMessageButton2.setText(buttonText); + binding.connectMessageButton2.setContentDescription(buttonText); + binding.connectMessageButton2.setImportantForAccessibility(View.IMPORTANT_FOR_ACCESSIBILITY_YES); + binding.connectMessageButton2.setAccessibilityTraversalBefore(R.id.connect_message_button); } }
35-41: Improve state management.The current state management uses mutable variables without proper encapsulation or validation.
Consider using a state object pattern:
+ private static class MessageState { + private final String title; + private final String message; + private final String buttonText; + private final String button2Text; + private final String userName; + private final String password; + private final int callingClass; + + private MessageState(Builder builder) { + this.title = builder.title; + this.message = builder.message; + this.buttonText = builder.buttonText; + this.button2Text = builder.button2Text; + this.userName = builder.userName; + this.password = builder.password; + this.callingClass = builder.callingClass; + } + + public static class Builder { + // ... builder pattern implementation + } + } + - private String title; - private String message; - private String buttonText; - private String button2Text; - private String userName; - private String password; - private int callingClass; + private MessageState state;
100-229: Consider refactoring the navigation logic.The switch statement is quite long and complex, making it hard to maintain. Consider:
- Extracting navigation logic into separate methods for each case
- Using a navigation map to reduce complexity
Example refactor:
private Map<Integer, NavigationHandler> navigationMap; private void initNavigationMap() { navigationMap = new HashMap<>(); navigationMap.put(ConnectConstants.CONNECT_REGISTRATION_SUCCESS, this::handleRegistrationSuccess); navigationMap.put(ConnectConstants.CONNECT_RECOVERY_ALT_PHONE_MESSAGE, this::handleRecoveryAltPhone); // Add other cases... } private NavDirections handleRegistrationSuccess(boolean success, boolean secondButton) { if (success) { ConnectManager.setStatus(ConnectManager.ConnectIdStatus.LoggedIn); ConnectDatabaseHelper.setRegistrationPhase(getActivity(), ConnectConstants.CONNECT_NO_ACTIVITY); requireActivity().setResult(RESULT_OK); requireActivity().finish(); } return null; } // Add other handler methods...app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java (3)
248-248: Remove commented out code.Remove the commented out code as it's not providing any value and can be confusing.
Apply this diff to remove the commented out code:
-// finish(true, true);
248-248: Remove or document commented code.The commented line
//finish(true, true);suggests uncertainty in the implementation. Either remove it if it's no longer needed or document why it's kept for future reference.-// finish(true, true);
248-248: Remove commented out code.Remove the commented out code as it adds noise to the codebase.
Apply this diff:
-// finish(true, true);app/src/org/commcare/connect/ConnectManager.java (4)
768-848: Remove commented out code blocks.Large blocks of commented out code should be removed as they add noise to the codebase. If this code is needed in the future, it can be retrieved from version control.
Remove the commented out code blocks at:
- Lines 768-848 (updateJobProgress and updateLearningProgress methods)
- Lines 295-314 (updateSecondaryPhoneConfirmationTile method)
Also applies to: 295-314
937-957: Improve network error handling.The current error handling can be enhanced with:
- More specific error messages.
- Retry mechanism for transient failures.
- Consistent error reporting.
Apply these improvements:
+ private static final int MAX_RETRIES = 3; + private static final long RETRY_DELAY_MS = 1000; + + private void retryWithBackoff(int retryCount, ConnectActivityCompleteListener listener) { + if (retryCount < MAX_RETRIES) { + new Handler().postDelayed(() -> { + updateDeliveryProgress(context, job, listener); + }, RETRY_DELAY_MS * (1 << retryCount)); + } else { + listener.connectActivityComplete(false); + } + } + @Override public void processFailure(int responseCode, IOException e) { - Logger.log("ERROR", String.format(Locale.getDefault(), "Delivery progress call failed: %d", responseCode)); + String errorMessage = getErrorMessageForCode(responseCode); + Logger.log("ERROR", String.format(Locale.getDefault(), + "Delivery progress call failed: %d - %s", responseCode, errorMessage)); reportApiCall(false); - listener.connectActivityComplete(false); + + if (isTransientError(responseCode)) { + retryWithBackoff(retryCount + 1, listener); + } else { + listener.connectActivityComplete(false); + } } @Override public void processNetworkFailure() { - Logger.log("ERROR", "Failed (network)"); + Logger.log("ERROR", "Network failure: " + getNetworkErrorDetails()); reportApiCall(false); - listener.connectActivityComplete(false); + retryWithBackoff(retryCount + 1, listener); }
999-1010: Use SecureRandom for password generation.The password generation method uses SecureRandom, which is the correct choice for generating secure passwords. However, consider adding additional security measures:
- Ensure minimum character type requirements
- Add entropy validation
Example enhancement:
public static String generatePassword() { int passwordLength = 20; String upperCase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; String lowerCase = "abcdefghijklmnopqrstuvwxyz"; String numbers = "0123456789"; String specialChars = "_!.?"; SecureRandom secureRandom = new SecureRandom(); StringBuilder password = new StringBuilder(passwordLength); // Ensure at least one of each character type password.append(upperCase.charAt(secureRandom.nextInt(upperCase.length()))); password.append(lowerCase.charAt(secureRandom.nextInt(lowerCase.length()))); password.append(numbers.charAt(secureRandom.nextInt(numbers.length()))); password.append(specialChars.charAt(secureRandom.nextInt(specialChars.length()))); // Fill remaining length with random chars String allChars = upperCase + lowerCase + numbers + specialChars; for (int i = 4; i < passwordLength; i++) { password.append(allChars.charAt(secureRandom.nextInt(allChars.length()))); } // Shuffle the password char[] passwordArray = password.toString().toCharArray(); for (int i = passwordArray.length - 1; i > 0; i--) { int index = secureRandom.nextInt(i + 1); char temp = passwordArray[index]; passwordArray[index] = passwordArray[i]; passwordArray[i] = temp; } return new String(passwordArray); }
295-314: Remove commented out code blocks.Remove large blocks of commented out code as they add noise to the codebase. If this code is needed in the future, it can be retrieved from version control.
Remove the commented out code blocks for:
- updateSecondaryPhoneConfirmationTile
- getConnectToken
- updateJobProgress and updateLearningProgress
Also applies to: 586-595, 768-848
app/res/navigation/nav_graph_connectid.xml (3)
7-22: Dialog "connectid_phone_not_available" SetupThe dialog for "connectid_phone_not_available" is properly defined with a clear ID, name, and layout reference. The inclusion of an action and the two arguments (
phoneandcallingClass) is appropriate. Consider reviewing the label text ("fragment_connectid_phone_available_fragment") to ensure it accurately reflects the purpose of this dialog.
258-290: Fragment "connectid_signup_fragment" and Action ID NamingThe signup fragment is correctly established with default argument values (e.g.,
callingClassset to 1002 and a default empty string forphone). However, note that the action IDs within this fragment use the prefixaction_connectid_phone_fragment_...even though the fragment in which they are defined is the signup fragment. For consistency and readability, consider renaming these action IDs (for example, to useaction_connectid_signup_fragment_...instead).Suggested diff snippet:
- <action - android:id="@+id/action_connectid_phone_fragment_to_connectid_phone_not_available" - app:destination="@id/connectid_phone_not_available" /> + <action + android:id="@+id/action_connectid_signup_fragment_to_connectid_phone_not_available" + app:destination="@id/connectid_phone_not_available" />Repeat a similar renaming for the subsequent actions in this fragment.
292-317: Fragment "connectid_secondary_phone_fragment" ConfigurationThe secondary phone fragment is structured with clear navigation actions leading to phone verification, message, and PIN screens. The arguments are generally well-defined. One minor suggestion: if the
methodargument is optional, consider explicitly addingapp:nullable="true"for clarity.app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java (2)
204-204: Address TODO for secondary phone retrieval.The TODO comment indicates that secondary phone retrieval from the server is not implemented yet.
Would you like me to help implement the secondary phone retrieval functionality? I can:
- Update the ConnectUserResponseParser to handle secondary phone.
- Add the necessary fields to the ConnectUserRecord model.
- Implement the server API integration.
47-48: Extract max failures to a constant.The max failures value is hardcoded. Consider extracting it to a constant for better maintainability.
Apply this diff to extract the constant:
- private static final int MaxFailures = 3; + private static final int MAX_FAILURES = 3;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
app/res/navigation/nav_graph_connectid.xml(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/activities/LoginActivityUIController.java(6 hunks)app/src/org/commcare/connect/ConnectConstants.java(2 hunks)app/src/org/commcare/connect/ConnectManager.java(1 hunks)app/src/org/commcare/connect/SMSBroadcastReceiver.java(1 hunks)app/src/org/commcare/connect/database/ConnectDatabaseHelper.java(1 hunks)app/src/org/commcare/connect/database/ConnectDatabaseUtils.java(1 hunks)app/src/org/commcare/connect/network/ConnectSsoHelper.java(1 hunks)app/src/org/commcare/connect/workers/ConnectHeartbeatWorker.kt(1 hunks)app/src/org/commcare/fragments/SelectInstallModeFragment.java(5 hunks)app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdMessageFragment.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPhoneAvailableBottomSheet.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPhoneFragment.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragment.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java(1 hunks)app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java(1 hunks)app/src/org/commcare/utils/BiometricsHelper.java(1 hunks)app/src/org/commcare/utils/CrashUtil.java(3 hunks)app/src/org/commcare/utils/PhoneNumberHelper.java(1 hunks)app/src/org/commcare/views/connect/CircleProgressBar.java(1 hunks)app/src/org/commcare/views/connect/ConnectEditText.java(1 hunks)app/src/org/commcare/views/connect/connecttextview/ConnectBoldTextView.java(1 hunks)app/src/org/commcare/views/connect/connecttextview/ConnectItalicTextView.java(1 hunks)app/src/org/commcare/views/connect/connecttextview/ConnectMediumTextView.java(1 hunks)app/src/org/commcare/views/connect/connecttextview/ConnectRegularTextView.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- app/src/org/commcare/views/connect/connecttextview/ConnectItalicTextView.java
- app/src/org/commcare/views/connect/connecttextview/ConnectRegularTextView.java
- app/src/org/commcare/views/connect/connecttextview/ConnectBoldTextView.java
- app/src/org/commcare/connect/workers/ConnectHeartbeatWorker.kt
- app/src/org/commcare/views/connect/connecttextview/ConnectMediumTextView.java
- app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java
- app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java
- app/src/org/commcare/connect/ConnectConstants.java
👮 Files not reviewed due to content moderation or server errors (4)
- app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java
- app/src/org/commcare/fragments/connectId/ConnectIdMessageFragment.java
- app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java
- app/src/org/commcare/connect/ConnectManager.java
🧰 Additional context used
📓 Learnings (3)
app/src/org/commcare/activities/LoginActivityUIController.java (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:81-89
Timestamp: 2025-01-21T17:27:58.754Z
Learning: In the CommCare Android codebase, use org.jetbrains.annotations.NotNull for null-safety annotations. Empty strings are acceptable for unspecified values, but null values should be prevented using @NotNull.
app/src/org/commcare/fragments/connectId/ConnectIdPhoneVerificationFragment.java (2)
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#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/fragments/connectId/ConnectIdPasswordVerificationFragment.java (2)
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#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.
🔇 Additional comments (38)
app/src/org/commcare/views/connect/CircleProgressBar.java (1)
26-40: LGTM!The constructors follow Android View best practices by implementing all three standard constructors.
app/src/org/commcare/views/connect/ConnectEditText.java (1)
182-182: Verify negative left coordinate forsetBounds.
drawableEnd.setBounds(-paddingRight, 0, drawableWidth - paddingRight, drawableHeight)uses a negative left coordinate, which may cause unexpected drawable placement. Double-check that it achieves the intended layout in all Android versions and screen densities.app/src/org/commcare/fragments/SelectInstallModeFragment.java (5)
12-12: LGTM!The new imports are correctly added and used within the class.
Also applies to: 25-25
53-54: LGTM!The member variables follow Android naming conventions and have appropriate access modifiers.
200-204: LGTM!The method is well-documented with clear parameter descriptions.
81-82: Verify layout resource IDs exist.The view initialization follows Android best practices.
Run this script to verify the layout resource IDs exist:
✅ Verification successful
Layout resource IDs verified.
The output confirms that both
connect_login_buttonandlogin_orIDs exist in the expected layout files, includingselect_install_mode_fragment.xmlandscreen_login.xml. The view initialization inSelectInstallModeFragment.javais correctly referencing existing IDs.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the required view IDs are defined in layout XML files # Search for connect_login_button and login_or IDs in layout files rg -A 2 'android:id="@\+?id/(connect_login_button|login_or)"' --type xmlLength of output: 1303
205-216: 🛠️ Refactor suggestionAdd null check and consider memory leak prevention.
The method has the following issues:
- Missing null check for
mOrTextbefore accessing it- Potential memory leak with the click listener if the fragment is destroyed
Apply this diff to address the issues:
public void updateConnectButton(boolean connectEnabled, View.OnClickListener listener) { - if(mConnectButton != null) { + if(mConnectButton != null && mOrText != null) { boolean enabled = connectEnabled && ConnectManager.shouldShowConnectButton(); if (enabled && listener!=null) { mConnectButton.setOnClickListener(listener); + } else { + mConnectButton.setOnClickListener(null); } mConnectButton.setVisibility(enabled ? View.VISIBLE : View.GONE); mOrText.setVisibility(enabled ? View.VISIBLE : View.GONE); } }Since this is part of the Connect ID feature and no automated tests have been written yet, consider adding unit tests. Would you like me to help generate test cases for this method?
✅ Verification successful
Action Required: Address Null Pointer and Listener Leak Concerns
The proposed changes correctly add a null check for
mOrTextand clear the click listener when the button isn’t enabled, which mitigates the potential memory leak when the fragment is destroyed. Adding unit tests for this behavior is also a good idea.
- The updated condition now checks that both
mConnectButtonandmOrTextare non-null.- Setting the listener to
nullin theelsebranch ensures that any lingering reference is removed, helping to avoid memory leaks.- Consider adding unit tests for both states (enabled and disabled) to verify that the correct listener is set or cleared and that the visibility flags are applied as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts or web queries found
app/src/org/commcare/connect/network/ConnectSsoHelper.java (1)
1-23: LGTM! Well-structured class with proper documentation.The class structure follows best practices with clear documentation and appropriate imports.
app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (1)
70-72: Verify the legacy code path.The comment indicates this is a legacy code path for opening the database using a byte array instead of a String. Consider if this legacy support is still needed or if it can be cleaned up.
Run the following script to check for other legacy code paths and their usage:
app/res/values/strings.xml (2)
462-830: Overall Assessment of ConnectID String Integrations:
The extensive set of new string resources (lines 462–830) effectively supports the added ConnectID functionality for registration, recovery, and related interactions. The messages are generally well structured and make good use of placeholders (e.g.,%s,%d) and inline HTML where appropriate. Just ensure that:• All typographical and grammatical details (such as the suggestions above) are addressed.
• Consistency is maintained in naming conventions—especially regarding the use of “ConnectID” versus “connectid” and apostrophe styling.
• HTML formatting and escaped characters work as intended in the UI.Once these minor revisions are made, the resources should be in excellent shape.
792-792:⚠️ Potential issueFix Typographical Error in Deactivation Dialog:
The deactivation dialog description on line 792 contains a typo:
- Current:
"You want to deactivate you account permanently?"- Proposed:
"Do you want to deactivate your account permanently?"Apply the following diff to correct it:
- <string name="connect_deactivate_dialog_description">You want to deactivate you account permanently?</string> + <string name="connect_deactivate_dialog_description">Do you want to deactivate your account permanently?</string>Likely invalid or redundant comment.
app/src/org/commcare/utils/BiometricsHelper.java (1)
50-58: Verify fallback logic for fingerprint authentication on newer devices.On Android versions above Q, the code falls back to PIN authentication instead of showing the fingerprint prompt. Verify if this is the intended user experience, as some users might expect a fingerprint prompt even on newer Android versions.
app/src/org/commcare/fragments/connectId/ConnectIdPhoneFragment.java (1)
130-135: Provide robust handling for nulluserandexistingPhone.
Merely logging a "Null Exception" case leaves the state ambiguous. Consider showing a user-facing message or navigational fallback if both references are null.app/src/org/commcare/utils/PhoneNumberHelper.java (1)
30-32: Ensure proper lifecycle management forPhoneNumberUtilinstance.
Using aThreadLocal<PhoneNumberUtil>in an Android environment might be unnecessary overhead or cause subtle concurrency issues, as most UI operations run on the main thread. Confirm that this approach doesn't inadvertently tie the lifecycle ofPhoneNumberUtilto a thread that outlives theActivityorContext.Would you like me to generate a small script to search for multiple creations or uses of this
PhoneNumberUtilacross the codebase for potential concurrency pitfalls?app/src/org/commcare/connect/SMSBroadcastReceiver.java (1)
40-42: 🛠️ Refactor suggestionAdd null check for messageIntent before accessing it.
The null check should be performed before accessing messageIntent to prevent potential NPE.
- if (messageIntent!=null && smsListener != null) - smsListener.onSuccess(messageIntent); + if (messageIntent != null && smsListener != null) { + smsListener.onSuccess(messageIntent); + }Likely invalid or redundant comment.
app/src/org/commcare/fragments/connectId/ConnectIdPhoneAvailableBottomSheet.java (1)
44-50: 🛠️ Refactor suggestionAdd null safety checks for view binding.
Ensure view binding is properly initialized and cleaned up to prevent memory leaks.
+ @Override + public void onDestroyView() { + super.onDestroyView(); + binding = null; + }Likely invalid or redundant comment.
app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java (4)
204-204: Reminder: Implement secondary phone retrieval from server.The TODO comment indicates that secondary phone retrieval from the server is missing.
Do you want me to generate the implementation for retrieving the secondary phone from the server?
89-229: LGTM!The recovery and unlocking flow is well-implemented with proper navigation, analytics tracking, and state management.
1-81: LGTM! Fragment initialization and UI setup looks good.The fragment follows Android best practices:
- Uses data binding for UI management
- Safely retrieves arguments using the navigation component
- Properly handles lifecycle methods
207-229: LGTM! Error handling and logging looks good.The fragment properly handles:
- Network failures
- JSON parsing errors
- Error logging with appropriate context
app/src/org/commcare/fragments/connectId/ConnectIdMessageFragment.java (2)
77-98: LGTM!The UI update and button handling logic is well-implemented with proper visibility management.
1-94: LGTM! Fragment initialization and UI setup looks good.The fragment follows Android best practices:
- Uses data binding for UI management
- Safely retrieves arguments using the navigation component
- Properly handles button visibility
app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java (3)
139-193: LGTM!The UI state management is well-implemented with proper handling of different biometric configurations.
1-93: LGTM! Fragment initialization and UI setup looks good.The fragment follows Android best practices:
- Uses data binding for UI management
- Properly initializes BiometricManager
- Correctly checks biometric and PIN status
133-194: LGTM! UI state management looks good.The fragment properly:
- Updates UI based on biometric and PIN status
- Handles button visibility
- Updates button texts
app/src/org/commcare/connect/ConnectManager.java (4)
86-185: LGTM!The class initialization and status management is well-implemented with proper thread safety and heartbeat scheduling.
186-607: LGTM!The authentication and token management is well-implemented with proper security measures, including the use of SecureRandom for password generation.
186-260: LGTM! Biometric authentication looks good.The class properly:
- Handles fingerprint and PIN authentication
- Manages BiometricManager instance
- Handles authentication callbacks
850-997: LGTM! Job and payment management looks good.The class properly:
- Handles job progress updates
- Manages payment confirmation
- Handles delivery progress
app/res/navigation/nav_graph_connectid.xml (9)
1-6: Navigation Graph InitializationThe navigation header is well-defined with the appropriate XML declaration and namespace configurations. The start destination is clearly set to
@id/connectid_signup_fragment, which aligns with the intended entry point for the Connect ID flow.
24-45: Fragment "connectid_biometric_config" ConfigurationThis fragment is configured with clear navigation actions to both the phone verification and message flows. The use of
app:popUpToensures proper back stack handling. Additionally, the arguments (callingClassandallowPassword) are correctly defined. It may be worthwhile to verify that the default value forallowPassword(set to false) is consistent with the business logic.
47-112: Fragment "connectid_phone_verify" Navigation FlowThe "connectid_phone_verify" fragment contains multiple navigation actions covering transitions to PIN, biometric configuration, password, message, and even self-navigations. The arguments are well laid out to support various parameters such as
callingClass,method,primaryPhone, and others. This comprehensive setup is in line with expected user flows.
114-173: Dialog "connectid_message" Detailed ActionsThis dialog is structured with a rich set of navigation actions, allowing transitions to biometric config, PIN, password, phone verification, and secondary actions. All required arguments (e.g.,
title,message,callingClass,buttonText, etc.) are clearly defined. While the variety of navigation actions is powerful, please verify that these transitions align with the overall user experience and that any self-navigation (refresh) is intentional.
175-222: Fragment "connectid_pin" for PIN HandlingThe fragment dedicated to PIN input is well-implemented. It includes actions for navigating back to the phone verification screen, moving forward to messages, and even self-navigation. The boolean arguments (
recoverandchange) with their default values are appropriately set for the expected behaviors.
224-256: Fragment "connectid_password" for Password VerificationThe "connectid_password" fragment is configured to route users to the appropriate next steps (message, phone verification, PIN, or phone screens) based on the flow. The arguments (
phone,secret, andcallingClass) are correctly specified.
319-351: Fragment "connectid_phoneNo" for Primary Phone ScreenThis fragment defines the primary phone screen with its associated navigation actions to PIN, phone verification, biometric configuration, and message dialogs. The arguments (
method,phone, andcallingClass) are clearly delineated, ensuring consistent flow control.
353-392: Fragment "connectid_user_deactivate_otp_verify" for OTP VerificationThe OTP verification fragment for user deactivation is well-laid out with a clear navigation action directing to the message dialog. The arguments support necessary parameters for the action, such as
callingClass,method,primaryPhone, and others. Everything appears correctly specified for this critical flow.
1-393: Overall Navigation Graph Design and ConsistencyThe comprehensive navigation graph effectively maps the Connect ID registration and recovery flows by integrating dialogs and multiple fragments with clear navigation actions and argument definitions. The use of
app:popUpToenhances back stack management, and the overall structure aligns with the new Connect ID functionality. Ensure that all action IDs and argument nullability settings are consistent with the intended business logic. Minor renaming improvements (as noted in the signup fragment) could enhance maintainability.
| @Override | ||
| protected void onDraw(Canvas canvas) { | ||
| super.onDraw(canvas); | ||
|
|
||
| int width = getWidth(); | ||
| int height = getHeight(); | ||
| int radius = Math.min(width, height) / 2 - (int) strokeWidth; | ||
| int centerX = width / 2; | ||
| int centerY = height / 2; | ||
|
|
||
| canvas.drawCircle(centerX, centerY, radius, backgroundPaint); | ||
| RectF rectF = new RectF(centerX - radius, centerY - radius, centerX + radius, centerY + radius); | ||
| float sweepAngle = progress * 360f / 100f; | ||
| if (isGradient) { | ||
| Shader shader = new SweepGradient(centerX, centerY, gradientColors, null); | ||
| progressPaint.setShader(shader); | ||
| } else { | ||
| progressPaint.setShader(null); | ||
| progressPaint.setColor(progressColor); | ||
| } | ||
| canvas.drawArc(rectF, startAngle, sweepAngle, false, progressPaint); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add state saving support.
The view should implement state saving to handle configuration changes properly.
Add the following methods:
@Override
protected Parcelable onSaveInstanceState() {
Bundle bundle = new Bundle();
bundle.putParcelable("superState", super.onSaveInstanceState());
bundle.putFloat("progress", progress);
bundle.putFloat("strokeWidth", strokeWidth);
bundle.putInt("startAngle", startAngle);
bundle.putInt("progressColor", progressColor);
bundle.putInt("backgroundColor", backgroundColor);
bundle.putBoolean("isGradient", isGradient);
bundle.putIntArray("gradientColors", gradientColors);
return bundle;
}
@Override
protected void onRestoreInstanceState(Parcelable state) {
if (state instanceof Bundle) {
Bundle bundle = (Bundle) state;
progress = bundle.getFloat("progress");
strokeWidth = bundle.getFloat("strokeWidth");
startAngle = bundle.getInt("startAngle");
progressColor = bundle.getInt("progressColor");
backgroundColor = bundle.getInt("backgroundColor");
isGradient = bundle.getBoolean("isGradient");
gradientColors = bundle.getIntArray("gradientColors");
state = bundle.getParcelable("superState");
}
super.onRestoreInstanceState(state);
}| public void setHintTextSize(float size) { | ||
| setTextSize(size); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename or refactor method to accurately reflect behavior.
The method setHintTextSize(float size) calls setTextSize(size), which changes the text size for the entire EditText, not just the hint. This could be misleading for maintainers.
Consider renaming the method or actually setting the hint text size independently from the primary text size. If you do wish to allow separate hint text sizing, one approach is to apply a distinct TextAppearanceSpan to just the hint.
| if (ConnectManager.isConnectIdConfigured()) { | ||
| ConnectUserRecord user = ConnectUserDatabaseUtil.getUser(activity); | ||
| if (user == null || user.getName() == null) { | ||
| welcomeMessage.setText(activity.getString(R.string.default_welcome_message)); | ||
| return; | ||
| } | ||
| String welcomeText = activity.getString(R.string.login_welcome_connect_signed_in, | ||
| user.getName()); | ||
|
|
||
| welcomeMessage.setText(welcomeText); | ||
| } |
There was a problem hiding this comment.
Add null checks to prevent potential NPE.
The updateConnectLoginState method should handle the case where activity is null.
if (ConnectManager.isConnectIdConfigured()) {
+ if (activity == null) {
+ return;
+ }
ConnectUserRecord user = ConnectUserDatabaseUtil.getUser(activity);
if (user == null || user.getName() == null) {
welcomeMessage.setText(activity.getString(R.string.default_welcome_message));
return;
}
String welcomeText = activity.getString(R.string.login_welcome_connect_signed_in,
user.getName());
welcomeMessage.setText(welcomeText);
}📝 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.
| if (ConnectManager.isConnectIdConfigured()) { | |
| ConnectUserRecord user = ConnectUserDatabaseUtil.getUser(activity); | |
| if (user == null || user.getName() == null) { | |
| welcomeMessage.setText(activity.getString(R.string.default_welcome_message)); | |
| return; | |
| } | |
| String welcomeText = activity.getString(R.string.login_welcome_connect_signed_in, | |
| user.getName()); | |
| welcomeMessage.setText(welcomeText); | |
| } | |
| if (ConnectManager.isConnectIdConfigured()) { | |
| if (activity == null) { | |
| return; | |
| } | |
| ConnectUserRecord user = ConnectUserDatabaseUtil.getUser(activity); | |
| if (user == null || user.getName() == null) { | |
| welcomeMessage.setText(activity.getString(R.string.default_welcome_message)); | |
| return; | |
| } | |
| String welcomeText = activity.getString(R.string.login_welcome_connect_signed_in, | |
| user.getName()); | |
| welcomeMessage.setText(welcomeText); | |
| } |
| } catch (MalformedURLException e) { | ||
| Logger.log(LogTypes.TYPE_EXCEPTION,e.toString()); | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid throwing RuntimeException for checked exceptions.
Converting MalformedURLException (checked) to RuntimeException (unchecked) is not a good practice as it hides the fact that this method can fail due to invalid URLs from the caller.
Consider:
- Either declare MalformedURLException in the method signature
- Or handle it gracefully by returning null and logging the error
- } catch (MalformedURLException e) {
- Logger.log(LogTypes.TYPE_EXCEPTION,e.toString());
- throw new RuntimeException(e);
+ } catch (MalformedURLException e) {
+ Logger.log(LogTypes.TYPE_EXCEPTION, "Invalid HQ URL: " + e.getMessage(), e);
+ return null;
}📝 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.
| } catch (MalformedURLException e) { | |
| Logger.log(LogTypes.TYPE_EXCEPTION,e.toString()); | |
| throw new RuntimeException(e); | |
| } | |
| } catch (MalformedURLException e) { | |
| Logger.log(LogTypes.TYPE_EXCEPTION, "Invalid HQ URL: " + e.getMessage(), e); | |
| return null; | |
| } |
| private static BiometricPrompt.AuthenticationCallback biometricPromptCallbackHolder; | ||
|
|
||
| public static void authenticatePin(FragmentActivity activity, BiometricManager biometricManager, | ||
| BiometricPrompt.AuthenticationCallback biometricPromptCallback) { | ||
| if (BiometricsHelper.isPinConfigured(activity, biometricManager)) { | ||
| biometricPromptCallbackHolder = biometricPromptCallback; | ||
| KeyguardManager manager = (KeyguardManager)activity.getSystemService(Context.KEYGUARD_SERVICE); | ||
| activity.startActivityForResult( | ||
| manager.createConfirmDeviceCredentialIntent( | ||
| activity.getString(R.string.connect_unlock_title), | ||
| activity.getString(R.string.connect_unlock_message)), | ||
| ConnectConstants.CONNECT_UNLOCK_PIN); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid storing callback in a static field to prevent memory leaks and concurrency issues.
Storing BiometricPrompt.AuthenticationCallback in biometricPromptCallbackHolder as a static field can lead to memory leaks (if the activity is destroyed) and thread-safety issues (if accessed concurrently). It's safer to keep this callback in an instance-scoped context, such as within the FragmentActivity or a non-static helper instance.
Below is a possible refactor using a request-based model, removing the static callback holder:
- private static BiometricPrompt.AuthenticationCallback biometricPromptCallbackHolder;
public static void authenticatePin(FragmentActivity activity,
BiometricManager biometricManager,
- BiometricPrompt.AuthenticationCallback biometricPromptCallback) {
+ BiometricPrompt.AuthenticationCallback callback) {
if (BiometricsHelper.isPinConfigured(activity, biometricManager)) {
- biometricPromptCallbackHolder = biometricPromptCallback;
KeyguardManager manager = (KeyguardManager)activity.getSystemService(Context.KEYGUARD_SERVICE);
activity.startActivityForResult(
manager.createConfirmDeviceCredentialIntent(
activity.getString(R.string.connect_unlock_title),
activity.getString(R.string.connect_unlock_message)),
ConnectConstants.CONNECT_UNLOCK_PIN
);
}
}
// Store the callback in the activity's field or a retained fragment instead.📝 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.
| private static BiometricPrompt.AuthenticationCallback biometricPromptCallbackHolder; | |
| public static void authenticatePin(FragmentActivity activity, BiometricManager biometricManager, | |
| BiometricPrompt.AuthenticationCallback biometricPromptCallback) { | |
| if (BiometricsHelper.isPinConfigured(activity, biometricManager)) { | |
| biometricPromptCallbackHolder = biometricPromptCallback; | |
| KeyguardManager manager = (KeyguardManager)activity.getSystemService(Context.KEYGUARD_SERVICE); | |
| activity.startActivityForResult( | |
| manager.createConfirmDeviceCredentialIntent( | |
| activity.getString(R.string.connect_unlock_title), | |
| activity.getString(R.string.connect_unlock_message)), | |
| ConnectConstants.CONNECT_UNLOCK_PIN); | |
| } | |
| } | |
| public static void authenticatePin(FragmentActivity activity, | |
| BiometricManager biometricManager, | |
| BiometricPrompt.AuthenticationCallback callback) { | |
| if (BiometricsHelper.isPinConfigured(activity, biometricManager)) { | |
| KeyguardManager manager = (KeyguardManager) activity.getSystemService(Context.KEYGUARD_SERVICE); | |
| activity.startActivityForResult( | |
| manager.createConfirmDeviceCredentialIntent( | |
| activity.getString(R.string.connect_unlock_title), | |
| activity.getString(R.string.connect_unlock_message)), | |
| ConnectConstants.CONNECT_UNLOCK_PIN); | |
| } | |
| } |
| ConnectManager.setStatus(ConnectManager.ConnectIdStatus.LoggedIn); | ||
| ConnectDatabaseHelper.setRegistrationPhase(getActivity(), ConnectConstants.CONNECT_NO_ACTIVITY); | ||
| requireActivity().setResult(RESULT_OK); | ||
| requireActivity().finish(); | ||
| break; | ||
| case ConnectConstants.CONNECT_BIOMETRIC_ENROLL_FAIL: | ||
| if (success) { | ||
| SettingsHelper.launchSecuritySettings(requireActivity()); | ||
| } else { | ||
| directions = ConnectIdMessageFragmentDirections.actionConnectidMessageToConnectidBiometricConfig(ConnectConstants.CONNECT_REGISTRATION_CONFIGURE_BIOMETRICS); | ||
| } | ||
| break; | ||
| case ConnectConstants.CONNECT_RECOVERY_VERIFY_PASSWORD: | ||
| if (success) { | ||
| if (ConnectIdActivity.forgotPassword) { | ||
| directions = ConnectIdMessageFragmentDirections.actionConnectidMessageSelf(getString(R.string.connect_recovery_alt_title), getString(R.string.connect_recovery_alt_message), ConnectConstants.CONNECT_RECOVERY_ALT_PHONE_MESSAGE, getString(R.string.connect_recovery_alt_button), null, userName, password); | ||
| } else { | ||
| directions = ConnectIdMessageFragmentDirections.actionConnectidMessageToConnectidPin(ConnectConstants.CONNECT_RECOVERY_CHANGE_PIN, ConnectIdActivity.recoverPhone, ConnectIdActivity.recoverSecret).setRecover(true).setChange(true); | ||
| } | ||
| } else { | ||
| directions = ConnectIdMessageFragmentDirections.actionConnectidMessageToConnectidPhoneVerify(ConnectConstants.CONNECT_RECOVERY_VERIFY_PRIMARY_PHONE, String.format(Locale.getDefault(), "%d", | ||
| ConnectIdPhoneVerificationFragment.MethodRecoveryPrimary), ConnectIdActivity.recoverPhone, ConnectIdActivity.recoverPhone, null, null,false).setAllowChange(false); | ||
| } | ||
| break; | ||
| case ConnectConstants.CONNECT_RECOVERY_WRONG_PASSWORD: | ||
| if (success) { | ||
| directions = ConnectIdMessageFragmentDirections.actionConnectidMessageToConnectidPassword(ConnectIdActivity.recoverPhone, ConnectIdActivity.recoverSecret, ConnectConstants.CONNECT_RECOVERY_VERIFY_PASSWORD); | ||
| } | ||
| break; | ||
| case ConnectConstants.CONNECT_UNLOCK_ALT_PHONE_MESSAGE: | ||
| if (success) { | ||
| if (secondButton) { | ||
| directions = ConnectIdMessageFragmentDirections.actionConnectidMessageToConnectidSecondaryPhoneFragment(ConnectConstants.CONNECT_UNLOCK_ALT_PHONE_CHANGE, ConnectConstants.METHOD_CHANGE_ALTERNATE, null); | ||
| } else { | ||
| directions = ConnectIdMessageFragmentDirections.actionConnectidMessageToConnectidPhoneVerify(ConnectConstants.CONNECT_UNLOCK_VERIFY_ALT_PHONE, String.format(Locale.getDefault(), "%d", | ||
| ConnectIdPhoneVerificationFragment.MethodVerifyAlternate), null, user.getUserId(), user.getPassword(), null,false).setAllowChange(false); | ||
| } | ||
| } | ||
| break; | ||
| //CONNECT_RECOVERY_WRONG_PIN | ||
| case ConnectConstants.CONNECT_RECOVERY_WRONG_PIN: | ||
| if (success) { | ||
| if (ConnectManager.getFailureAttempt() > 2) { | ||
| directions = ConnectIdMessageFragmentDirections.actionConnectidMessageToConnectidPhoneVerify(ConnectConstants.CONNECT_RECOVERY_VERIFY_ALT_PHONE, String.format(Locale.getDefault(), "%d", | ||
| ConnectIdPhoneVerificationFragment.MethodRecoveryAlternate), null, ConnectIdActivity.recoverPhone, ConnectIdActivity.recoverSecret, ConnectIdActivity.recoveryAltPhone,false).setAllowChange(false); | ||
| ConnectManager.setFailureAttempt(0); | ||
| } else { | ||
| directions = ConnectIdMessageFragmentDirections.actionConnectidMessageToConnectidPin(ConnectConstants.CONNECT_RECOVERY_VERIFY_PIN, ConnectIdActivity.recoverPhone, ConnectIdActivity.recoverSecret).setChange(false).setRecover(true); | ||
| } | ||
| } | ||
| break; | ||
| case ConnectConstants.CONNECT_REGISTRATION_WRONG_PIN: | ||
| if (success) { | ||
| if (ConnectManager.getFailureAttempt() > 2) { | ||
| directions = ConnectIdMessageFragmentDirections.actionConnectidMessageToConnectidPin(ConnectConstants.CONNECT_REGISTRATION_CHANGE_PIN, user.getPrimaryPhone(), "").setChange(true).setRecover(false); | ||
| } else { | ||
| directions = ConnectIdMessageFragmentDirections.actionConnectidMessageToConnectidPin(ConnectConstants.CONNECT_REGISTRATION_CONFIRM_PIN, user.getPrimaryPhone(), "").setChange(false).setRecover(false); | ||
| } | ||
| } | ||
| break; | ||
| case ConnectConstants.CONNECT_VERIFY_ALT_PHONE_MESSAGE: | ||
| if (success) { | ||
| if (secondButton) { | ||
| directions = ConnectIdMessageFragmentDirections.actionConnectidMessageToConnectidSecondaryPhoneFragment(ConnectConstants.CONNECT_VERIFY_ALT_PHONE_CHANGE, ConnectConstants.METHOD_CHANGE_ALTERNATE, null); | ||
| } else { | ||
| directions = ConnectIdMessageFragmentDirections.actionConnectidMessageToConnectidPhoneVerify(ConnectConstants.CONNECT_VERIFY_ALT_PHONE, String.format(Locale.getDefault(), "%d", | ||
| ConnectIdPhoneVerificationFragment.MethodVerifyAlternate), null, user.getUserId(), user.getPassword(), null,false).setAllowChange(false); | ||
| } | ||
| } | ||
| break; | ||
| case ConnectConstants.CONNECT_USER_DEACTIVATE_CONFIRMATION: | ||
| if (success) { | ||
| if (!secondButton) { | ||
| directions = ConnectIdMessageFragmentDirections.actionConnectidMessageToConnectidUserDeactivateOtpVerify( | ||
| ConnectConstants.CONNECT_VERIFY_USER_DEACTIVATE, | ||
| String.format(Locale.getDefault(), "%d", ConnectIdPhoneVerificationFragment.MethodUserDeactivate), | ||
| ConnectIdActivity.recoverPhone, | ||
| userName, | ||
| password, | ||
| null,true).setAllowChange(false); | ||
| } else { | ||
| NavHostFragment.findNavController(this).popBackStack(); | ||
| } | ||
| } | ||
| break; | ||
| case ConnectConstants.CONNECT_USER_DEACTIVATE_SUCCESS: | ||
| if (success) { | ||
| if (!secondButton) { | ||
| ConnectManager.forgetUser("Account deactivation"); | ||
| requireActivity().finish(); | ||
| } | ||
| } | ||
| break; | ||
| } | ||
| if (success) { | ||
| if (directions != null) { | ||
| NavHostFragment.findNavController(this).navigate(directions); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor complex navigation logic.
The finish() method is overly complex with multiple responsibilities and duplicate navigation logic.
Consider extracting navigation logic into separate methods:
+ private enum NavigationType {
+ REGISTRATION, RECOVERY, BIOMETRIC, UNLOCK, DEACTIVATE
+ }
+
+ private NavDirections handleRegistrationNavigation(boolean success, boolean secondButton) {
+ if (success) {
+ ConnectManager.setStatus(ConnectManager.ConnectIdStatus.LoggedIn);
+ ConnectDatabaseHelper.setRegistrationPhase(getActivity(), ConnectConstants.CONNECT_NO_ACTIVITY);
+ requireActivity().setResult(RESULT_OK);
+ requireActivity().finish();
+ }
+ return null;
+ }
+
public void finish(boolean success, boolean secondButton) {
NavDirections directions = null;
ConnectUserRecord user = ConnectUserDatabaseUtil.getUser(getActivity());
+ NavigationType type = getNavigationType(callingClass);
+
+ switch (type) {
+ case REGISTRATION:
+ directions = handleRegistrationNavigation(success, secondButton);
+ break;
+ case RECOVERY:
+ directions = handleRecoveryNavigation(success, secondButton, user);
+ break;
+ // ... other cases
+ }
- switch (callingClass) {
- case ConnectConstants.CONNECT_REGISTRATION_SUCCESS:
- if (success) {
- ConnectManager.setStatus(ConnectManager.ConnectIdStatus.LoggedIn);
- ConnectDatabaseHelper.setRegistrationPhase(getActivity(), ConnectConstants.CONNECT_NO_ACTIVITY);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@pm-dimagi Understood, I'll skip the refactoring suggestion.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| private BiometricPrompt.AuthenticationCallback preparePromptCallbacks() { | ||
| final Context context = requireActivity(); | ||
| return new BiometricPrompt.AuthenticationCallback() { | ||
| @Override | ||
| public void onAuthenticationError(int errorCode, | ||
| @NonNull CharSequence errString) { | ||
| super.onAuthenticationError(errorCode, errString); | ||
| if (attemptingFingerprint) { | ||
| attemptingFingerprint = false; | ||
| if (BiometricsHelper.isPinConfigured(context, biometricManager) && | ||
| allowPassword) { | ||
| //Automatically try password, it's the only option | ||
| performPinUnlock(); | ||
| } else { | ||
| //Automatically try password, it's the only option | ||
| performPasswordUnlock(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void onAuthenticationSucceeded( | ||
| @NonNull BiometricPrompt.AuthenticationResult result) { | ||
| super.onAuthenticationSucceeded(result); | ||
| logSuccess(); | ||
| finish(true, false); | ||
| } | ||
|
|
||
| @Override | ||
| public void onAuthenticationFailed() { | ||
| super.onAuthenticationFailed(); | ||
| Toast.makeText(requireActivity().getApplicationContext(), R.string.connect_biometric_error, | ||
| Toast.LENGTH_SHORT) | ||
| .show(); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Enhance biometric authentication security.
The current biometric authentication implementation needs security improvements:
- No strong biometric authentication requirement.
- Missing specific error handling.
- No fallback mechanism.
Apply these security improvements:
+ private static final int ERROR_LOCKOUT = BiometricPrompt.ERROR_LOCKOUT;
+ private static final int ERROR_PERMANENT_LOCKOUT = BiometricPrompt.ERROR_LOCKOUT_PERMANENT;
+ private static final int ERROR_HW_NOT_PRESENT = BiometricPrompt.ERROR_HW_NOT_PRESENT;
+
private BiometricPrompt.AuthenticationCallback preparePromptCallbacks() {
final Context context = requireActivity();
return new BiometricPrompt.AuthenticationCallback() {
@Override
public void onAuthenticationError(int errorCode,
@NonNull CharSequence errString) {
super.onAuthenticationError(errorCode, errString);
+ switch (errorCode) {
+ case ERROR_LOCKOUT:
+ case ERROR_PERMANENT_LOCKOUT:
+ // Handle biometric lockout
+ handleBiometricLockout(errorCode == ERROR_PERMANENT_LOCKOUT);
+ break;
+ case ERROR_HW_NOT_PRESENT:
+ // Fallback to PIN
+ if (BiometricsHelper.isPinConfigured(context, biometricManager)) {
+ performPinUnlock();
+ } else {
+ handleBiometricHardwareError();
+ }
+ break;
+ default:
+ // Handle other errors
+ handleBiometricError(errorCode, errString);
+ }
if (attemptingFingerprint) {
attemptingFingerprint = false;Also, update the biometric prompt configuration to require strong authentication:
public void performFingerprintUnlock() {
attemptingFingerprint = true;
boolean allowOtherOptions = BiometricsHelper.isPinConfigured(requireActivity(), biometricManager) ||
allowPassword;
- BiometricsHelper.authenticateFingerprint(requireActivity(), biometricManager, allowOtherOptions, biometricPromptCallbacks);
+ BiometricPrompt.PromptInfo promptInfo = new BiometricPrompt.PromptInfo.Builder()
+ .setTitle(getString(R.string.biometric_prompt_title))
+ .setSubtitle(getString(R.string.biometric_prompt_subtitle))
+ .setAllowedAuthenticators(BiometricManager.Authenticators.BIOMETRIC_STRONG)
+ .setNegativeButtonText(getString(android.R.string.cancel))
+ .build();
+ BiometricsHelper.authenticateFingerprint(requireActivity(), biometricManager, promptInfo, allowOtherOptions, biometricPromptCallbacks);
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@pm-dimagi Understood, I'll skip the biometric authentication security review.
✏️ Learnings added
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.880Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| private static ConnectManager manager = null; | ||
| private ConnectIdStatus connectStatus = ConnectIdStatus.NotIntroduced; | ||
| private Context parentActivity; | ||
|
|
||
| private String primedAppIdForAutoLogin = null; | ||
|
|
||
| private int pendingAction = PENDING_ACTION_NONE; | ||
|
|
||
| //Singleton, private constructor | ||
| private ConnectManager() { | ||
| } | ||
|
|
||
| private static ConnectManager getInstance() { | ||
| if (manager == null) { | ||
| manager = new ConnectManager(); | ||
| } | ||
|
|
||
| return manager; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve singleton pattern implementation.
The current singleton implementation is not thread-safe and lacks protection against reflection and serialization attacks.
Apply these improvements:
- private static ConnectManager manager = null;
+ private static volatile ConnectManager manager;
private ConnectIdStatus connectStatus = ConnectIdStatus.NotIntroduced;
private Context parentActivity;
//Singleton, private constructor
private ConnectManager() {
+ // Protect against reflection
+ if (manager != null) {
+ throw new IllegalStateException("Already initialized.");
+ }
}
+ // Protect against serialization
+ private Object readResolve() {
+ return manager;
+ }
private static ConnectManager getInstance() {
if (manager == null) {
- manager = new ConnectManager();
+ synchronized (ConnectManager.class) {
+ if (manager == null) {
+ manager = new ConnectManager();
+ }
+ }
}
return manager;
}📝 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.
| private static ConnectManager manager = null; | |
| private ConnectIdStatus connectStatus = ConnectIdStatus.NotIntroduced; | |
| private Context parentActivity; | |
| private String primedAppIdForAutoLogin = null; | |
| private int pendingAction = PENDING_ACTION_NONE; | |
| //Singleton, private constructor | |
| private ConnectManager() { | |
| } | |
| private static ConnectManager getInstance() { | |
| if (manager == null) { | |
| manager = new ConnectManager(); | |
| } | |
| return manager; | |
| } | |
| private static volatile ConnectManager manager; | |
| private ConnectIdStatus connectStatus = ConnectIdStatus.NotIntroduced; | |
| private Context parentActivity; | |
| private String primedAppIdForAutoLogin = null; | |
| private int pendingAction = PENDING_ACTION_NONE; | |
| //Singleton, private constructor | |
| private ConnectManager() { | |
| // Protect against reflection | |
| if (manager != null) { | |
| throw new IllegalStateException("Already initialized."); | |
| } | |
| } | |
| // Protect against serialization | |
| private Object readResolve() { | |
| return manager; | |
| } | |
| private static ConnectManager getInstance() { | |
| if (manager == null) { | |
| synchronized (ConnectManager.class) { | |
| if (manager == null) { | |
| manager = new ConnectManager(); | |
| } | |
| } | |
| } | |
| return manager; | |
| } |
| public static String generatePassword() { | ||
| int passwordLength = 20; | ||
|
|
||
| String charSet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_!.?"; | ||
| SecureRandom secureRandom = new SecureRandom(); | ||
| StringBuilder password = new StringBuilder(passwordLength); | ||
| for (int i = 0; i < passwordLength; i++) { | ||
| password.append(charSet.charAt(secureRandom.nextInt(charSet.length()))); | ||
| } | ||
|
|
||
| return password.toString(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance password generation security.
The current password generation can be improved:
- Limited special characters in character set.
- No validation of password strength.
- No guarantee of character type inclusion.
Improve password generation:
public static String generatePassword() {
- int passwordLength = 20;
+ final int MIN_LENGTH = 20;
+ final int MIN_DIGITS = 2;
+ final int MIN_SPECIAL = 2;
+ final int MIN_UPPERCASE = 2;
+ final int MIN_LOWERCASE = 2;
+
+ final String UPPERCASE = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
+ final String LOWERCASE = "abcdefghijklmnopqrstuvwxyz";
+ final String DIGITS = "0123456789";
+ final String SPECIAL = "!@#$%^&*()_+-=[]{}|;:,.<>?";
- String charSet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_!.?";
SecureRandom secureRandom = new SecureRandom();
- StringBuilder password = new StringBuilder(passwordLength);
- for (int i = 0; i < passwordLength; i++) {
- password.append(charSet.charAt(secureRandom.nextInt(charSet.length())));
+ StringBuilder password = new StringBuilder(MIN_LENGTH);
+
+ // Ensure minimum requirements
+ for (int i = 0; i < MIN_UPPERCASE; i++) {
+ password.append(UPPERCASE.charAt(secureRandom.nextInt(UPPERCASE.length())));
}
+ for (int i = 0; i < MIN_LOWERCASE; i++) {
+ password.append(LOWERCASE.charAt(secureRandom.nextInt(LOWERCASE.length())));
+ }
+ for (int i = 0; i < MIN_DIGITS; i++) {
+ password.append(DIGITS.charAt(secureRandom.nextInt(DIGITS.length())));
+ }
+ for (int i = 0; i < MIN_SPECIAL; i++) {
+ password.append(SPECIAL.charAt(secureRandom.nextInt(SPECIAL.length())));
+ }
+
+ // Fill remaining length with random chars
+ String allChars = UPPERCASE + LOWERCASE + DIGITS + SPECIAL;
+ while (password.length() < MIN_LENGTH) {
+ password.append(allChars.charAt(secureRandom.nextInt(allChars.length())));
+ }
+
+ // Shuffle the password
+ char[] passwordArray = password.toString().toCharArray();
+ for (int i = passwordArray.length - 1; i > 0; i--) {
+ int j = secureRandom.nextInt(i + 1);
+ char temp = passwordArray[i];
+ passwordArray[i] = passwordArray[j];
+ passwordArray[j] = temp;
+ }
- return password.toString();
+ return new String(passwordArray);
}📝 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 String generatePassword() { | |
| int passwordLength = 20; | |
| String charSet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_!.?"; | |
| SecureRandom secureRandom = new SecureRandom(); | |
| StringBuilder password = new StringBuilder(passwordLength); | |
| for (int i = 0; i < passwordLength; i++) { | |
| password.append(charSet.charAt(secureRandom.nextInt(charSet.length()))); | |
| } | |
| return password.toString(); | |
| } | |
| public static String generatePassword() { | |
| final int MIN_LENGTH = 20; | |
| final int MIN_DIGITS = 2; | |
| final int MIN_SPECIAL = 2; | |
| final int MIN_UPPERCASE = 2; | |
| final int MIN_LOWERCASE = 2; | |
| final String UPPERCASE = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; | |
| final String LOWERCASE = "abcdefghijklmnopqrstuvwxyz"; | |
| final String DIGITS = "0123456789"; | |
| final String SPECIAL = "!@#$%^&*()_+-=[]{}|;:,.<>?"; | |
| SecureRandom secureRandom = new SecureRandom(); | |
| StringBuilder password = new StringBuilder(MIN_LENGTH); | |
| // Ensure minimum requirements | |
| for (int i = 0; i < MIN_UPPERCASE; i++) { | |
| password.append(UPPERCASE.charAt(secureRandom.nextInt(UPPERCASE.length()))); | |
| } | |
| for (int i = 0; i < MIN_LOWERCASE; i++) { | |
| password.append(LOWERCASE.charAt(secureRandom.nextInt(LOWERCASE.length()))); | |
| } | |
| for (int i = 0; i < MIN_DIGITS; i++) { | |
| password.append(DIGITS.charAt(secureRandom.nextInt(DIGITS.length()))); | |
| } | |
| for (int i = 0; i < MIN_SPECIAL; i++) { | |
| password.append(SPECIAL.charAt(secureRandom.nextInt(SPECIAL.length()))); | |
| } | |
| // Fill remaining length with random chars | |
| String allChars = UPPERCASE + LOWERCASE + DIGITS + SPECIAL; | |
| while (password.length() < MIN_LENGTH) { | |
| password.append(allChars.charAt(secureRandom.nextInt(allChars.length()))); | |
| } | |
| // Shuffle the password | |
| char[] passwordArray = password.toString().toCharArray(); | |
| for (int i = passwordArray.length - 1; i > 0; i--) { | |
| int j = secureRandom.nextInt(i + 1); | |
| char temp = passwordArray[i]; | |
| passwordArray[i] = passwordArray[j]; | |
| passwordArray[j] = temp; | |
| } | |
| return new String(passwordArray); | |
| } |
There was a problem hiding this comment.
@pm-dimagi Understood, I'll skip the password generation improvements.
| try { | ||
| String responseAsString = new String( | ||
| StreamsUtil.inputStreamToByteArray(responseData)); | ||
| if (responseAsString.length() > 0) { | ||
| JSONObject json = new JSONObject(responseAsString); | ||
| String key = ConnectConstants.CONNECT_KEY_USERNAME; | ||
| if (json.has(key)) { | ||
| username = json.getString(key); | ||
| } | ||
|
|
||
| key = ConnectConstants.CONNECT_KEY_NAME; | ||
| if (json.has(key)) { | ||
| name = json.getString(key); | ||
| } | ||
|
|
||
| key = ConnectConstants.CONNECT_KEY_DB_KEY; | ||
| if (json.has(key)) { | ||
| ConnectDatabaseHelper.handleReceivedDbPassphrase(context, json.getString(key)); | ||
| } | ||
|
|
||
| ConnectUserRecord user = new ConnectUserRecord(phone, username, | ||
| password, name, ""); | ||
|
|
||
| key = ConnectConstants.CONNECT_KEY_VALIDATE_SECONDARY_PHONE_BY; | ||
| user.setSecondaryPhoneVerified(!json.has(key) || json.isNull(key)); | ||
| if (!user.getSecondaryPhoneVerified()) { | ||
| user.setSecondaryPhoneVerifyByDate(DateUtils.parseDate(json.getString(key))); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use ConnectUserResponseParser for parsing server responses.
Based on the retrieved learnings, server responses containing user data should be parsed using ConnectUserResponseParser to ensure consistent handling of payment information and secondary phone verification across all authentication flows.
Apply this diff to use ConnectUserResponseParser:
- String key = ConnectConstants.CONNECT_KEY_USERNAME;
- if (json.has(key)) {
- username = json.getString(key);
- }
-
- key = ConnectConstants.CONNECT_KEY_NAME;
- if (json.has(key)) {
- name = json.getString(key);
- }
+ ConnectUserRecord user = ConnectUserResponseParser.parseUserResponse(json);
+ username = user.getUserId();
+ name = user.getName();Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@pm-dimagi Understood, I'll skip this suggestion.
🐰
(•́ . •̀ )
🧠 Learnings used
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.
|
@coderabbitai I pushed a fix in commit 1f6f083, please review it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 19173 @pm-dimagi I've reviewed the changes in commit ece3cef. The fixes look good and address several important areas:
However, there's one minor concern in ConnectEditText: public void setHintTextSize(float size) {
setHintTextSize(size); // This causes infinite recursion
}This should probably be: public void setHintTextSize(float size) {
setTextSize(size);
}Would you like me to create an issue for this? If you found this review helpful, would you consider giving us a shout-out on X? |
Summary
Connect id code merge to the Foundation branch
Feature Flag
Connect ID registration and recovery flow
Product Description
This feature is used to to create the connectID and its recovery so it can be single source of login and user can login without id pass
PR Checklist
Automated test coverage
no test cases written
Safety story
Locally tested all the flows of the feature