CCCT-469 || HQ User Invite Connect #2927
Conversation
Working on HQ user Invitation API integration
Set deep-linking code
Update update invitation logic
Update API logic
Add connect-ID not configure scenario
Set connect user name from url
Set get data from query params instead of URL
Solved user not found issue
Working on API issue
Set loader while API call
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive enhancement to the CommCare application's user invitation and connection process. The changes primarily focus on implementing a new The modifications span multiple files across the Android project, including the The core functionality is implemented in These changes enhance the application's user management capabilities, providing a more robust and localized approach to handling project domain invitations. Suggested reviewers
Possibly related PRs
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.
Actionable comments posted: 4
🧹 Nitpick comments (7)
app/src/org/commcare/activities/connect/HQUserInviteActivity.java (2)
86-98: Use descriptive listener method naming
When associating click listeners, consider using clearer method names or lambda expressions with descriptive comments to clarify each button's purpose. This promotes readability and maintainability.
148-155: Potential for centralized error-handling
This method sets an error message UI element. If multiple error states occur in the activity, you might centralize error handling logic (e.g., a dedicated utility method or a shared handler) to streamline user-facing messages and reduce code duplication.app/res/layout/activity_hquser_invite.xml (2)
1-8: Consider adding accessibility attributes
Ensure that components in this ConstraintLayout have content descriptions or talkback hints for screen readers, especially for any images or interactive elements.
100-112: Progress indicators
Since the ProgressBar is shown after the user clicks "Accept Invitation" or triggers an API call, ensure it’s also hidden if the user navigates back or if an error occurs before the API response. This helps avoid stale loading indicators.app/res/values-fr/strings.xml (1)
308-313: Review correctness of newly added strings
These invitation-related strings appear clear and contextually correct. Ensure styling is consistent for language-specific formatting, including punctuation and spacing (e.g., “dabord” vs. “d’abord”). Minor grammar improvements can enhance user clarity.app/AndroidManifest.xml (1)
Line range hint
426-433: Consider adding exported attribute to PaymentAcknowledgeReceiverThe PaymentAcknowledgeReceiver should explicitly declare its exported status for security clarity.
-<receiver android:name="org.commcare.services.PaymentAcknowledgeReceiver" /> +<receiver + android:name="org.commcare.services.PaymentAcknowledgeReceiver" + android:exported="false" />app/res/values/strings.xml (1)
45-45: Consider making the URL configurable per environmentThe hardcoded URL might cause issues when switching between environments (staging/production).
Consider moving this to a configuration file or making it environment-aware:
-<string name="ConnectConfirmUserInvitation">https://connectid.dimagi.com/users/confirm_hq_invite</string> +<string name="ConnectConfirmUserInvitation">@string/connect_base_url/users/confirm_hq_invite</string>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/AndroidManifest.xml(2 hunks)app/res/layout/activity_hquser_invite.xml(1 hunks)app/res/values-fr/strings.xml(1 hunks)app/res/values-pt/strings.xml(1 hunks)app/res/values/strings.xml(2 hunks)app/src/org/commcare/activities/connect/HQUserInviteActivity.java(1 hunks)app/src/org/commcare/connect/network/ApiConnectId.java(9 hunks)
🔇 Additional comments (6)
app/src/org/commcare/activities/connect/HQUserInviteActivity.java (2)
52-72: Validate user identity logic
When verifying the current user (lines 57-60), consider providing a user-friendly message/logging if an unexpected user mismatch occurs, especially if user records in local storage could be outdated or corrupted.
157-169: Finalize activity result workflows
Currently, onActivityResult immediately finishes the activity (lines 166-167). This approach may not notify upstream logic about the outcome if external callbacks or post-processing are needed. Confirm that finishing immediately is the desired flow.
app/res/layout/activity_hquser_invite.xml (1)
33-48: Maintain consistent text sizing
The “roundButtonTextSize” attribute uses a relatively small “6sp”. Confirm that this is legible on all devices. If not, consider sizing up to improve readability.
app/AndroidManifest.xml (1)
Line range hint 25-32: LGTM: Permission changes follow Android best practices
The permission changes properly handle:
- Media permissions for Android 13+ (READ_MEDIA_*)
- Notification permission for Android 13+
- Storage permissions correctly scoped with maxSdkVersion=32
app/res/values-pt/strings.xml (1)
575-582: LGTM: Portuguese translations are complete and accurate
All new strings for the HQ user invitation feature are properly translated to Portuguese.
app/res/values/strings.xml (1)
879-885: LGTM: Invitation strings are complete
The new strings for HQ user invitation provide clear messaging for all user scenarios.
| public class HQUserInviteActivity extends CommCareActivity<HQUserInviteActivity> { | ||
|
|
||
| private ActivityHquserInviteBinding binding; | ||
| String domain; | ||
| String inviteCode; | ||
| String username; | ||
| String callBackURL; | ||
| String connectUserName; | ||
| boolean isProgressVisible = false; | ||
|
|
||
| @Override | ||
| protected void onCreate(Bundle savedInstanceState) { | ||
| super.onCreate(savedInstanceState); | ||
| binding = ActivityHquserInviteBinding.inflate(getLayoutInflater()); | ||
| setContentView(binding.getRoot()); | ||
| Intent intent = getIntent(); | ||
| Uri data = intent.getData(); | ||
| if (data != null) { | ||
| callBackURL = data.getQueryParameter("callback_url"); | ||
| username = data.getQueryParameter("hq_username"); | ||
| inviteCode = data.getQueryParameter("invite_code"); | ||
| domain = data.getQueryParameter("hq_domain"); | ||
| connectUserName = data.getQueryParameter("connect_username"); | ||
| } | ||
| handleButtons(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure domain & parameter validation in onCreate()
This block relies on Intent data to populate fields like domain, inviteCode, etc. It's safer to handle malformed or missing query parameters by checking for null or empty strings here to avoid potential NullPointerExceptions or incorrect state.
| private void handleInvitation(String callBackUrl, String inviteCode) { | ||
| IApiCallback callback = new IApiCallback() { | ||
| @Override | ||
| public void processSuccess(int responseCode, InputStream responseData) { | ||
| binding.progressBar.setVisibility(View.GONE); | ||
| try { | ||
| String responseAsString = new String(StreamsUtil.inputStreamToByteArray(responseData)); | ||
| if (responseAsString.length() > 0) { | ||
| startActivity(new Intent(HQUserInviteActivity.this, DispatchActivity.class)); | ||
| finish(); | ||
| } | ||
| } catch (IOException e) { | ||
| Logger.exception("Parsing return from OTP request", e); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void processFailure(int responseCode, IOException e) { | ||
| binding.progressBar.setVisibility(View.GONE); | ||
| String message = ""; | ||
| if (responseCode > 0) { | ||
| message = String.format(Locale.getDefault(), "(%d)", responseCode); | ||
| } else if (e != null) { | ||
| message = e.toString(); | ||
| } | ||
| setErrorMessage("Error requesting SMS code" + message); | ||
| } | ||
|
|
||
| @Override | ||
| public void processNetworkFailure() { | ||
| binding.progressBar.setVisibility(View.GONE); | ||
| setErrorMessage(getString(R.string.recovery_network_unavailable)); | ||
| } | ||
|
|
||
| @Override | ||
| public void processOldApiError() { | ||
| binding.progressBar.setVisibility(View.GONE); | ||
| setErrorMessage(getString(R.string.recovery_network_outdated)); | ||
| } | ||
| }; | ||
| ConnectUserRecord user = ConnectManager.getUser(this); | ||
| binding.progressBar.setVisibility(View.VISIBLE); | ||
| boolean isBusy = !ApiConnectId.hqUserInvitation(HQUserInviteActivity.this,user.getUserId(),user.getPassword(), callBackUrl, inviteCode, callback); | ||
| if (isBusy) { | ||
| Toast.makeText(HQUserInviteActivity.this, R.string.busy_message, Toast.LENGTH_SHORT).show(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error messaging & handle potential null connectIdToken
Before calling connectIdToken.toString() (line 432), verify that connectIdToken is not null to avoid a NullPointerException. Also, consider providing more user-friendly or localized error messages when setErrorMessage is called, to help guide users on how to recover from invitation errors.
| public static boolean hqUserInvitation(Context context,String username,String password, String callBackUrl, String invitationCode, IApiCallback callback) { | ||
| int urlId = R.string.ConnectConfirmUserInvitation; | ||
| AuthInfo.TokenAuth connectIdToken = retrieveConnectIdTokenSync(context); | ||
| HashMap<String, String> params = new HashMap<>(); | ||
| params.put("callback_url", callBackUrl); | ||
| params.put("invite_code", invitationCode); | ||
| params.put("user_token", connectIdToken.toString()); | ||
| return ConnectNetworkHelper.post(context, context.getString(urlId), API_VERSION_CONNECT_ID, new AuthInfo.ProvidedAuth(username, password, false), params, false, false, callback); | ||
| } |
There was a problem hiding this comment.
Check for valid token & domain edge cases
In hqUserInvitation(), retrieving connectIdToken may return null if ConnectManager isn’t properly initialized or if the user record is stale. Consider verifying the token before proceeding with the POST request.
Proposed fix in a diff block:
+if (connectIdToken == null) {
+ Logger.log(Log.ERROR, "ConnectID token is null; cannot proceed with invitation");
+ callback.processFailure(-1, new IOException("Token is null"));
+ return false;
+}Committable suggestion skipped: line range outside the PR's diff.
| <activity | ||
| android:name="org.commcare.activities.connect.HQUserInviteActivity" | ||
| android:exported="true"> | ||
| <intent-filter android:autoVerify="true"> | ||
| <action android:name="android.intent.action.VIEW" /> | ||
|
|
||
| <category android:name="android.intent.category.DEFAULT" /> | ||
| <category android:name="android.intent.category.BROWSABLE" /> | ||
|
|
||
| <data android:scheme="https"/> | ||
| <data android:host="connectid.dimagi.com"/> | ||
| <data android:scheme="http"/> | ||
| <data android:host="connectid.dimagi.com"/> | ||
| </intent-filter> | ||
| </activity> |
There was a problem hiding this comment.
💡 Codebase verification
Additional security measures recommended for deep link handling
Based on the code analysis, while the activity implements basic deep link handling, several security improvements are needed:
- The activity accepts and processes deep link parameters without any validation of the
callback_urlparameter, which could lead to potential security vulnerabilities - No null checks are performed on required parameters before processing the invitation
- The activity accepts both HTTP and HTTPS schemes without enforcing HTTPS-only communication
- No validation exists for the domain of callback URLs to ensure they match expected patterns
Recommended actions:
- Add URL validation for the callback_url parameter to ensure it points to trusted domains
- Implement null/empty checks for all required parameters (callback_url, invite_code, etc.)
- Consider removing the HTTP scheme from the intent filter to enforce secure communications
- Add path patterns to the intent filter to restrict the accepted URLs to specific endpoints
🔗 Analysis chain
Verify security measures for exported activity handling deep links
The exported activity with deep link handling requires careful security review:
- Ensure proper validation of incoming deep link data
- Verify that
android:autoVerify="true"is intended as it allows automatic verification of deep links - Consider adding intent filters for specific paths rather than allowing all paths
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for URL validation in HQUserInviteActivity
ast-grep --pattern 'class HQUserInviteActivity {
$$$
getIntent() {
$$$
}
$$$
}'
Length of output: 93
Script:
#!/bin/bash
# Search for HQUserInviteActivity implementation
fd -e java -e kt "HQUserInviteActivity" --exec cat {}
# Search for URL/URI validation patterns in the activity
rg -l "HQUserInviteActivity" | xargs rg -A 5 "getIntent|getData|parse.*Uri|validate"
Length of output: 7692
Script:
#!/bin/bash
# Check if ConnectManager performs any validation
rg -A 10 "class ConnectManager"
# Check ApiConnectId implementation for security measures
rg -A 10 "hqUserInvitation.*callback"
# Look for any URL validation utilities
rg -l "validateUrl|validateUri|isValidUrl|isValidUri"
Length of output: 3910
Summary
https://dimagi.atlassian.net/browse/CCCT-469
Feature Flag
Product Description
PR Checklist
Automated test coverage
Safety story