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 significant enhancements to the CommCare application, focusing on user invitations to the CommCareHQ project domain. Key modifications include the addition of a new Possibly related PRs
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.
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.
app/src/org/commcare/activities/connect/HQUserInviteActivity.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/activities/connect/HQUserInviteActivity.java
Outdated
Show resolved
Hide resolved
| <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
There was a problem hiding this comment.
I agree that this is a concern and think we should probably improve both the intent filter and the data validation
There was a problem hiding this comment.
@OrangeAndGreen Here are specific recommendations for both areas:
- Intent Filter Improvements:
<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" />
<!-- Remove HTTP, use only HTTPS -->
<data android:scheme="https" />
<data android:host="connectid.dimagi.com" />
<!-- Add specific paths -->
<data android:pathPattern="/users/confirm_hq_invite" />
</intent-filter>
</activity>- Data Validation Improvements:
private void validateAndProcessIntent() {
Intent intent = getIntent();
Uri data = intent.getData();
if (data == null) {
setErrorMessage("Invalid invitation link");
return;
}
// Validate callback URL
callBackURL = data.getQueryParameter("callback_url");
if (callBackURL == null || !isValidCallbackUrl(callBackURL)) {
setErrorMessage("Invalid callback URL");
return;
}
// Validate required parameters
username = data.getQueryParameter("hq_username");
inviteCode = data.getQueryParameter("invite_code");
domain = data.getQueryParameter("hq_domain");
connectUserName = data.getQueryParameter("connect_username");
if (username == null || inviteCode == null ||
domain == null || connectUserName == null) {
setErrorMessage("Missing required parameters");
return;
}
handleButtons();
}
private boolean isValidCallbackUrl(String url) {
try {
Uri uri = Uri.parse(url);
String host = uri.getHost();
return host != null &&
(host.equals("commcare.dimagi.com") ||
host.endsWith(".commcare.dimagi.com"));
} catch (Exception e) {
return false;
}
}- Additional Security Measures:
- Add HTTPS validation for callback URL
- Implement rate limiting for invitation attempts
- Add logging for security-related events
- Consider adding signature verification for the invite parameters
Would you like me to provide more detailed implementation for any of these areas?
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check current implementation of HQUserInviteActivity
fd -e java -e kt "HQUserInviteActivity" --exec cat {}
# Look for any existing URL validation utilities
rg -l "validateUrl|validateUri|isValidUrl|isValidUri"
# Check if there are any similar activities with URL validation
rg -A 10 "android.intent.action.VIEW.*connectid.dimagi.com"
# Check for any existing security utilities
fd -e java -e kt "security|util|validation" --exec rg -l "validate|verify|check"
Length of output: 7592
✏️ Learnings added
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2927
File: app/AndroidManifest.xml:151-165
Timestamp: 2025-02-04T19:06:14.404Z
Learning: The HQUserInviteActivity should validate all deep link parameters (callback_url, hq_username, invite_code, hq_domain, connect_username) before processing the invitation. The callback URL should be restricted to trusted domains (commcare.dimagi.com).
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2927
File: app/AndroidManifest.xml:151-165
Timestamp: 2025-02-04T19:06:14.404Z
Learning: Deep link intent filters for connectid.dimagi.com should use HTTPS scheme only and specify exact pathPatterns to prevent malicious deep link handling.
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.
There was a problem hiding this comment.
I do agree with the suggestions here.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@j13panchal Would you be able to take a pass on coderabbit comments here to save time on manual review. You can reply to on those threads if you think the comments are not necessarily applicable here. Can we also add a feature demo in Product Description section of PR template to showcase what's being achieved in this PR. |
… into jp/CCCT-469-HQ-user-invite-connectId
… into jp/CCCT-469-HQ-user-invite-connectId
|
@damagatchi retest this please |
Some code cleanup.
|
@coderabbitai review |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/org/commcare/activities/connect/HQUserInviteActivity.java (1)
96-146: Enhance error message clarity.The error messages could be more user-friendly. For example,
"Error requesting SMS code(%d)"doesn't provide clear guidance to users.Consider using more descriptive error messages:
- setErrorMessage("Error requesting SMS code" + message); + setErrorMessage(getString(R.string.error_invitation_failed, message));Add to
strings.xml:<string name="error_invitation_failed">Failed to process invitation. Please try again. Error: %s</string>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/AndroidManifest.xml(3 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(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/res/layout/activity_hquser_invite.xml
- app/src/org/commcare/connect/network/ApiConnectId.java
- app/res/values-fr/strings.xml
- app/res/values-pt/strings.xml
- app/res/values/strings.xml
🧰 Additional context used
📓 Learnings (2)
app/src/org/commcare/activities/connect/HQUserInviteActivity.java (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2927
File: app/src/org/commcare/activities/connect/HQUserInviteActivity.java:0-0
Timestamp: 2025-02-04T15:29:12.993Z
Learning: In HQUserInviteActivity, parameters from Intent data (domain, inviteCode, username, etc.) are required and the code is designed to fail immediately if they're missing, rather than adding null checks. This helps catch integration issues early.
app/AndroidManifest.xml (1)
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#2927
File: app/src/org/commcare/activities/connect/HQUserInviteActivity.java:0-0
Timestamp: 2025-02-04T15:29:12.993Z
Learning: In HQUserInviteActivity, parameters from Intent data (domain, inviteCode, username, etc.) are required and the code is designed to fail immediately if they're missing, rather than adding null checks. This helps catch integration issues early.
🔇 Additional comments (5)
app/src/org/commcare/activities/connect/HQUserInviteActivity.java (2)
34-52: LGTM! Fail-fast parameter handling.The implementation correctly follows the fail-fast approach for required parameters, as confirmed by the learnings.
54-74: LGTM! Clear button handling logic.The implementation correctly manages button states based on token presence and user verification.
app/AndroidManifest.xml (3)
30-32: LGTM! Required permissions for Android 13+.The new permissions are correctly added to support:
- Media access (replacing READ_EXTERNAL_STORAGE)
- Notification posting
Also applies to: 40-40
587-593: LGTM! Secure Firebase service configuration.The service is correctly configured:
- Non-exported for security
- Proper intent filter for Firebase messaging
151-165: 🛠️ Refactor suggestionRemove HTTP scheme for secure deep linking.
The activity allows both HTTP and HTTPS schemes. For security:
- Remove the HTTP scheme to enforce secure communications
- Add path patterns to restrict accepted URLs
Apply this diff:
<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"/> + <data android:pathPattern="/hq/invite/.*"/> </intent-filter>⛔ Skipped due to learnings
Learnt from: OrangeAndGreen PR: dimagi/commcare-android#2927 File: app/src/org/commcare/activities/connect/HQUserInviteActivity.java:0-0 Timestamp: 2025-02-04T15:29:12.993Z Learning: In HQUserInviteActivity, parameters from Intent data (domain, inviteCode, username, etc.) are required and the code is designed to fail immediately if they're missing, rather than adding null checks. This helps catch integration issues early.
| <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.
I agree that this is a concern and think we should probably improve both the intent filter and the data validation
| try { | ||
| String responseAsString = new String(StreamsUtil.inputStreamToByteArray(responseData)); | ||
| if (responseAsString.length() > 0) { | ||
| startActivity(new Intent(HQUserInviteActivity.this, DispatchActivity.class)); |
There was a problem hiding this comment.
What should happen when the user accepts the invitation? Seems like ideally we would take them to install an app next, but currently this just launches DispatchActivity and lets it make the call (login page).
There was a problem hiding this comment.
I would think user should land in some sort of connect workflow that initiates the app install if needed (I would think the install should happen only when they claim an opportunity). Would you be able to describe the user story here more in the PR description. For eg. It's not entirely clear to me when this feature will get used as a whole or when will someone invite a connect user to a project space and what we want to achive with it ?
Added some missing string translations.
| @@ -22,6 +22,7 @@ | |||
| <string name="ConnectClaimJobURL">https://%s/api/opportunity/%d/claim</string> | |||
| <string name="ConnectDeliveriesURL">https://%s/api/opportunity/%d/delivery_progress</string> | |||
| <string name="ConnectPaymentConfirmationURL">https://%s/api/payment/%s/confirm</string> | |||
| <string name="ConnectConfirmUserInvitation">https://connectid.dimagi.com/users/confirm_hq_invite</string> | |||
There was a problem hiding this comment.
Is it possible to define only the relative url to host ? That makes the code more felxible in event we want to have configurable hosts or test environments.
| private void setButtonVisibility(boolean visible) { | ||
| binding.btnAcceptInvitation.setVisibility(visible ? View.VISIBLE : View.GONE); | ||
| binding.btnDeniedInvitation.setVisibility(visible ? View.VISIBLE : View.GONE); | ||
| binding.btnGoToRecovery.setVisibility(visible ? View.GONE : View.VISIBLE); |
There was a problem hiding this comment.
we want to show the recovery button even when the user is incorrect ?
| } | ||
|
|
||
| private void handleInvitation(String callBackUrl, String inviteCode) { | ||
| Logger.log("HQInvite", "User accepted invitation"); |
There was a problem hiding this comment.
Use constants for Log Types otherwise we risk having a lot of different scattered categories in logs. You can add a connect specific class to signify logtypes like ConnectLogTypes if it's easier.
| try { | ||
| String responseAsString = new String(StreamsUtil.inputStreamToByteArray(responseData)); | ||
| if (responseAsString.length() > 0) { | ||
| startActivity(new Intent(HQUserInviteActivity.this, DispatchActivity.class)); |
There was a problem hiding this comment.
I would think user should land in some sort of connect workflow that initiates the app install if needed (I would think the install should happen only when they claim an opportunity). Would you be able to describe the user story here more in the PR description. For eg. It's not entirely clear to me when this feature will get used as a whole or when will someone invite a connect user to a project space and what we want to achive with it ?
| }; | ||
| ConnectUserRecord user = ConnectManager.getUser(this); | ||
| binding.progressBar.setVisibility(View.VISIBLE); | ||
| boolean isBusy = !ApiConnectId.hqUserInvitation(HQUserInviteActivity.this,user.getUserId(),user.getPassword(), callBackUrl, inviteCode, callback); |
|
|
||
| public static void reportHQInvitationResponse(String hqDomain, boolean accepted, String method) { | ||
| reportEvent(CCAnalyticsEvent.HQ_INVITE_LINK_RESPONSE, | ||
| new String[]{CCAnalyticsParam.CCHQ_DOMAIN, CCAnalyticsParam.ACCEPTED, |
There was a problem hiding this comment.
nit: better to use FirebaseAnalytics.Param.VALUE to signify success as 0 or 1 . That lets us do a simple AVG() operation to get the success rate across events.
| CCAnalyticsParam.CCHQ_DOMAIN, hqDomain); | ||
| } | ||
|
|
||
| public static void reportHQInvitationResponse(String hqDomain, boolean accepted, String method) { |
There was a problem hiding this comment.
I see that we are sending errors for the method parameter. Is that intentional ? If so we should rename the parameter as reason to signify the fail reason.
| 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)); | ||
| } |
There was a problem hiding this comment.
should not we be logging reportHQInvitationResponse here as well ?
| @Override | ||
| public void processFailure(int responseCode, IOException e) { | ||
| FirebaseAnalyticsUtil.reportHQInvitationResponse(domain, false, "API error"); | ||
| Logger.log("HQInvite", "Acceptance failed"); |
There was a problem hiding this comment.
probably log the IOException as Logger.exception() instead if it's not happening in the network layer itself.
|
|
||
| @Override | ||
| public void processFailure(int responseCode, IOException e) { | ||
| FirebaseAnalyticsUtil.reportHQInvitationResponse(domain, false, "API error"); |
There was a problem hiding this comment.
Instead of using a loose string "API Error", we should use constants for different possible failure causes, This should also log the response code if available.
https://dimagi.atlassian.net/browse/CCCT-469
Summary
Introduces deep linking for SMS messages that invite ConnectID users to join an HQ domain. The SMS message contains a link with the required info, and when the user clicks on the link the CommCare app is configured to override the action and open the app on a special page instead of opening the link in a web browser.
The new "HQ User Invite" page shows the user info about the HQ domain and allows them to accept or decline the invitation.
cross-request: dimagi/commcare-core#1455
Release Note:
ConnectID users can now receive and accept invitations to HQ domains sent to the user's phone via SMS.
Feature Flag
Connect
Product Description
SMS invitation to HQ domain contains a link with relevant information.
Clicking on link opens CommCare to a page showing the invitation.
The user is provided options to accept or decline the invitation.
PR Checklist
Automated test coverage
No automated tests for Connect yet.
Safety story
Possible error situations that need to be handled: