Automatic encryption key retrieval for messaging channels #2971
Conversation
…ssage is received for that channel and the key hasn't been retrieved yet.
📝 WalkthroughWalkthroughThe changes modify the processing of encryption keys for messaging channels. In the Sequence Diagram(s)sequenceDiagram
participant MM as MessageManager
participant Api as ApiConnectId
participant DB as ConnectDatabaseHelper
Note over MM: Message received
MM->>MM: Check if encryption key is null/empty
alt Key is missing
MM->>Api: retrieveChannelEncryptionKeySync(context, channel)
Api->>Api: retrieveConnectIdTokenSync
Api->>Api: Construct parameter map & send POST request
Api->>Api: Evaluate response (200-299)
Api->>Api: handleReceivedEncryptionKey(context, stream, channel)
Api->>DB: storeMessagingChannel(channel)
Api-->>MM: Encryption key updated
else Key is present
Note over MM: Process message normally
end
MM->>MM: Continue message processing or ignore if key still missing
Suggested labels
Suggested reviewers
✨ 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
🧹 Nitpick comments (2)
app/src/org/commcare/connect/network/ApiConnectId.java (2)
514-527: Synchronous key retrieval implementation looks good but needs better error handlingThe implementation of
retrieveChannelEncryptionKeySyncis well structured but lacks error handling for network failures.Consider adding error logging if the response code indicates failure:
if(result.responseCode >= 200 && result.responseCode < 300) { handleReceivedEncryptionKey(context, result.responseStream, channel); +} else { + Logger.log(LogTypes.TYPE_WARNING, "Failed to retrieve encryption key: " + result.responseCode); }Also, verify the key URL is not empty before attempting the request:
if(auth != null) { HashMap<String, Object> params = new HashMap<>(); params.put("channel_id", channel.getChannelId()); + + if (Strings.isNullOrEmpty(channel.getKeyUrl())) { + Logger.log(LogTypes.TYPE_WARNING, "Missing key URL for channel: " + channel.getChannelId()); + return; + } ConnectNetworkHelper.PostResult result = ConnectNetworkHelper.postSync(context, channel.getKeyUrl(), null, auth, params, true, true);
540-553: Improve JSON handling in encryption key processingThe method properly extracts the encryption key from the response, but could use improved validation.
Add explicit checking for the existence of the "key" field in the JSON response:
if(responseAsString.length() > 0) { JSONObject json = new JSONObject(responseAsString); - channel.setKey(json.getString("key")); - ConnectDatabaseHelper.storeMessagingChannel(context, channel); + if(json.has("key")) { + channel.setKey(json.getString("key")); + ConnectDatabaseHelper.storeMessagingChannel(context, channel); + } else { + Logger.log(LogTypes.TYPE_WARNING, "Encryption key response missing 'key' field"); + } }Also, consider adding specific exception handling for JSON parsing:
try { String responseAsString = new String( StreamsUtil.inputStreamToByteArray(stream)); if(responseAsString.length() > 0) { JSONObject json = new JSONObject(responseAsString); channel.setKey(json.getString("key")); ConnectDatabaseHelper.storeMessagingChannel(context, channel); } -} catch(Exception e) { +} catch(JSONException e) { + Logger.exception("Error parsing encryption key JSON", e); +} catch(IOException e) { Logger.exception("Messaging channel encryption key", e); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/org/commcare/connect/MessageManager.java(3 hunks)app/src/org/commcare/connect/network/ApiConnectId.java(3 hunks)
🔇 Additional comments (4)
app/src/org/commcare/connect/MessageManager.java (4)
7-7: Appropriate import additionThe addition of
com.google.common.base.Stringsis appropriate for using theisNullOrEmptyutility method.
51-52: Good fallback comment explaining behaviorThe comment clearly documents the fallback behavior that if we still don't have a key after the retrieval attempt, the message will be ignored. This aligns with the safety assurance mentioned in the PR objectives.
214-219: Code improvement: Better encapsulation of encryption key handlingGood refactoring to extract the encryption key handling logic into a common method. This improves code maintainability by centralizing the logic for processing encryption keys.
46-50:Details
✅ Verification successful
Automatic encryption key retrieval looks good but consider potential blocking
The implementation to automatically retrieve missing encryption keys when a message is received is well designed. This makes the code more robust by ensuring messages can be properly processed even if keys weren't previously available.
One thing to verify is whether this method is called on a background thread, as synchronous network operations could potentially block the UI thread:
🏁 Script executed:
#!/bin/bash # Find where handleReceivedMessage is called to verify it's not called directly from UI operations rg -B 5 -A 2 "handleReceivedMessage" --type javaLength of output: 1602
Encryption key retrieval verified against UI blocking concerns
The automatic retrieval logic is robust. I verified via the shell script that the
handleReceivedMessagemethod is invoked fromCommCareFirebaseMessagingService, which operates on a background thread. As a result, the synchronous call to retrieve the encryption key should not block the UI thread.
- Location:
app/src/org/commcare/connect/MessageManager.java(Lines 46-50)if(Strings.isNullOrEmpty(channel.getKey())) { //Attempt to get the encryption key now if we don't have it yet ApiConnectId.retrieveChannelEncryptionKeySync(context, channel); }- Invocation Verified In:
app/src/org/commcare/services/CommCareFirebaseMessagingService.javaPlease keep an eye on any similar synchronous network calls elsewhere to maintain non-blocking UI performance.
| if(result.responseCode >= 200 && result.responseCode < 300) { | ||
| handleReceivedEncryptionKey(context, result.responseStream, channel); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential resource leak in response stream handling
The response stream might not be closed if an exception occurs during processing.
Ensure the response stream is properly closed:
if(result.responseCode >= 200 && result.responseCode < 300) {
- handleReceivedEncryptionKey(context, result.responseStream, channel);
+ try {
+ handleReceivedEncryptionKey(context, result.responseStream, channel);
+ } finally {
+ if(result.responseStream != null) {
+ try {
+ result.responseStream.close();
+ } catch(IOException e) {
+ Logger.exception("Error closing response stream", 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.
| if(result.responseCode >= 200 && result.responseCode < 300) { | |
| handleReceivedEncryptionKey(context, result.responseStream, channel); | |
| } | |
| if(result.responseCode >= 200 && result.responseCode < 300) { | |
| try { | |
| handleReceivedEncryptionKey(context, result.responseStream, channel); | |
| } finally { | |
| if(result.responseStream != null) { | |
| try { | |
| result.responseStream.close(); | |
| } catch(IOException e) { | |
| Logger.exception("Error closing response stream", e); | |
| } | |
| } | |
| } | |
| } |
| } catch(Exception e) { | ||
| Logger.exception("Messaging channel encryption key", e); | ||
| } |
There was a problem hiding this comment.
We must move away from practice of catching generalised exceptions in Connect unless there is strong reason to do so.
…ing encryption key.
| channel.setKey(json.getString("key")); | ||
| ConnectDatabaseHelper.storeMessagingChannel(context, channel); | ||
| } | ||
| } catch(IOException | JSONException e) { |
There was a problem hiding this comment.
Given a JSONException here signifies a coding error here, I think this needs to hard crash the application.
There was a problem hiding this comment.
Ah, okay, that makes sense to me too. The compiler won't let me leave those exceptions uncaught so I converted to a RuntimeException (b2b465f), is that the best way to handle this?
There was a problem hiding this comment.
yes, that's what we want but only for JSONException and not IOException as IOException can happen for external reasons (network/disk I/O etc) out of our control.
There was a problem hiding this comment.
The call to StreamsUtil.inputStreamToByteArray requires that we handle IOException somehow. Would it be better to silently log an exception in that case? I can also bubble up the exception but then it just needs to be handled by the two callers instead.
There was a problem hiding this comment.
Looking into inputStreamFromByteArray a bit more, I see Connect code using it quite a bit so whatever we decide to do will need to be applied in other spots as well. But that's probably work for a new ticket...
When I look at existing usage in the legacy code I see a few ways this has been handled:
There was a problem hiding this comment.
Here's the change for silently logging the IOException if that's acceptable. 0caff08
There was a problem hiding this comment.
thanks, yeah that's acceptable for IOException as they don't necessarily signify a code error on our end but think it's better for them to bubble up to UI level method calls and show a related error on UI corresponding to the error message.
https://dimagi.atlassian.net/browse/CCCT-789
Product Description
Not user visible
Technical Summary
Attempting to retrieve encryption key for consented channel when a message is received for that channel and the key hasn't been retrieved yet. To support this, I added code to synchronously retrieve the encryption key, and extracted the common code for handling the received key to a helper method.
Feature Flag
ConnectID Messaging
Safety Assurance
Safety story
Low risk, leverages existing code to attempt to retrieve the encryption key on the fly. If it fails, the fallback behavior is to ignore the message.
Automated test coverage
No automated tests for ConnectID yet.
QA Plan
Create an auto-consented channel for a Connect opportunity that a mobile worker is part of.
Before ever accessing the channel in the messaging channel list, send a message to the device on that channel.
The message should be received, decrypted, and notified to the user so they can view it.
Labels and Review