Handling CID unlock in ConnectActivity #2945
Conversation
Moved redirection code into unlock fragment (happens after user unlocks ConnectID).
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive update to the ConnectID feature in the CommCare application. A new Sequence DiagramsequenceDiagram
participant User
participant ConnectActivity
participant ConnectUnlockFragment
participant ConnectManager
participant ApiConnect
User->>ConnectActivity: Initiate Connect Flow
ConnectActivity->>ConnectUnlockFragment: Navigate with Arguments
ConnectUnlockFragment->>ConnectManager: Unlock Opportunities
ConnectManager->>ApiConnect: Retrieve Opportunities
ApiConnect-->>ConnectUnlockFragment: Return Opportunity Data
ConnectUnlockFragment->>ConnectUnlockFragment: Parse and Store Opportunities
ConnectUnlockFragment->>ConnectActivity: Navigate to Next Fragment
Possibly related PRs
Suggested Labels
✨ 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: 1
🔭 Outside diff range comments (1)
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
🚨 Security vulnerabilities found in payment notification handling
- PaymentAcknowledgeReceiver is exposed without permissions, allowing any app to forge payment acknowledgments
- Payment IDs are transmitted in plain text through FCM and Intents
- No user authentication validation before processing payment acknowledgments
Recommendations:
- Add custom permission requirement for PaymentAcknowledgeReceiver
- Encrypt sensitive payment data in Intents using existing encryption utilities
- Add user authentication check in PaymentAcknowledgeReceiver.onReceive()
🔗 Analysis chain
Line range hint
180-196: Verify security implications of payment notification handling.While the code correctly handles PendingIntent flags for Android S+, consider these security aspects:
- Ensure payment IDs in extras are properly encrypted or obfuscated
- Verify that only authorized users can trigger payment acknowledgments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential sensitive data exposure in payment handling # Look for encryption/security measures around payment data rg -A 5 "PAYMENT_ID|OPPORTUNITY_ID" --type java # Check for authentication checks in PaymentAcknowledgeReceiver ast-grep --pattern 'class PaymentAcknowledgeReceiver { $$$ onReceive($_, $_) { $$$ } }'Length of output: 5658
Script:
#!/bin/bash # Check Firebase security and broadcast receiver protection # Look for Firebase configuration and security rg -A 5 "FirebaseMessaging|setReceiveTimeout|validateMessage" --type java # Check for broadcast receiver permissions and protection rg -A 5 "permission|PERMISSION|<receiver" --type xml # Look for any encryption utilities rg -A 5 "encrypt|decrypt|cipher|security" --type javaLength of output: 121411
🧹 Nitpick comments (4)
app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java (2)
40-48: Consider using stronger naming conventions and limiting global scope.
Declaring theView view; String redirectionAction; String opportunityId;as class fields can sometimes lead to confusion or unintended side effects. It's often cleaner to encapsulate view references within local scopes or use properties with narrower visibility.
73-119: Ensure robust handling of large or malformed JSON responses.
The block for parsing JSON, storing jobs, and logging exceptions is good. However, large or unexpectedly structured responses might cause performance issues or produce subtle parsing bugs. Consider a partial save approach or advanced error handling if real-world usage involves large data sets.app/src/org/commcare/connect/ConnectConstants.java (1)
38-41: LGTM! Consider adding documentation for the new constants.The new navigation constants follow consistent naming patterns and are well-structured. Consider adding Javadoc comments to describe the purpose and usage of each constant, especially since they're used for navigation across different features.
+ /** Destination constant for the opportunity summary page */ public static final String CCC_DEST_OPPORTUNITY_SUMMARY_PAGE = "ccc_opportunity_summary_page"; + /** Destination constant for the learning progress page */ public static final String CCC_DEST_LEARN_PROGRESS = "ccc_learn_progress"; + /** Destination constant for the delivery progress page */ public static final String CCC_DEST_DELIVERY_PROGRESS = "ccc_delivery_progress"; + /** Destination constant for the payments page */ public static final String CCC_DEST_PAYMENTS = "ccc_payment";app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
Line range hint
180-196: Consider adding error handling for missing payment data.The code assumes payment-related data will be present in the payload. Add null checks for
OPPORTUNITY_IDandPAYMENT_IDto handle cases where they might be missing.if (action.equals(ConnectConstants.CCC_DEST_PAYMENTS)) { + String opportunityId = payloadData.get(OPPORTUNITY_ID); + String paymentId = payloadData.get(PAYMENT_ID); + + if (opportunityId == null || paymentId == null) { + Logger.log(LogTypes.TYPE_FCM, "Missing payment data in payload"); + return; + } + // Yes button intent with payment_id from payload Intent yesIntent = new Intent(this, PaymentAcknowledgeReceiver.class); - yesIntent.putExtra(OPPORTUNITY_ID,payloadData.get(OPPORTUNITY_ID)); - yesIntent.putExtra(PAYMENT_ID,payloadData.get(PAYMENT_ID)); + yesIntent.putExtra(OPPORTUNITY_ID, opportunityId); + yesIntent.putExtra(PAYMENT_ID, paymentId); yesIntent.putExtra(PAYMENT_STATUS,true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/res/layout/fragment_connect_unlock.xml(1 hunks)app/res/navigation/nav_graph_connect.xml(1 hunks)app/src/org/commcare/activities/connect/ConnectActivity.java(4 hunks)app/src/org/commcare/connect/ConnectConstants.java(1 hunks)app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java(1 hunks)app/src/org/commcare/services/CommCareFirebaseMessagingService.java(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/res/layout/fragment_connect_unlock.xml
🔇 Additional comments (6)
app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java (1)
121-164: Confirm correct navigation behavior after unlocking.
UsingpopBackStack()and thennavigate(...)withpopUpTo(...)is a valid approach, but can sometimes cause confusing back stack states. Thoroughly test flows involving repeated unlock attempts or quick user navigation.app/src/org/commcare/activities/connect/ConnectActivity.java (3)
59-65: Input sanitization consideration.
While defaulting a nullopportunityIdto an empty string is fine, consider validating the user’s input more thoroughly to avoid potential parsing exceptions or logic errors.
93-105: Good clarity on notification-based navigation.
The logic is straightforward for handling entry from a push notification: callingConnectManager.init(...)and then directing toconnect_unlock_fragment. This effectively ensures that no sensitive data is shown until the user is unlocked. Nicely done!
114-120: Leverage consistent result handling.
Handling verification results inonActivityResultand delegating toConnectManager.handleFinishedActivity(...)centralizes logic and improves maintainability. Good approach.app/res/navigation/nav_graph_connect.xml (1)
8-14: Appropriate addition of the unlock fragment.
Declaring a dedicated<fragment>forConnectUnlockFragmentneatly structures the navigation flow. Ensure the layoutfragment_connect_unlockremains minimal to avoid UI confusion if the user rapidly navigates away before unlocking completes.app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
20-20: LGTM! Clean import addition.The import of ConnectConstants is appropriate for accessing the new navigation constants.
app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java
Outdated
Show resolved
Hide resolved
|
@damagatchi retest this please |
Summary
This addresses an issue where the user would be directed to the Connect activity on clicking a push notification, and while unlocking ConnectID the opportunity list fragment would already appear in the background and begin updating. Now instead, a blank fragment is shown when the user enters the app this way while the user is prompted to unlock ConnectID. This also addresses a bug where the wait dialog (shown while updating the opportunity list from the API) would sometimes fail to dismiss when the call completed.
cross-request: dimagi/commcare-core#1455
Product Description
Users will not see the opportunity list behind the Connect biometric unlock popup. Instead, the screen behind will be blank during this time.
PR Checklist
Automated test coverage
No automated tests for Connect code yet...
Safety story
This increases safety slightly by not showing any information about current or available opportunities until the user has successfully unlocked ConnectID via their biometric.