Cleaned up local DB passphrase code #3354
Conversation
-All passphrases assumed to be local -Removed obsolete code for retrieving remote passphrase and re-keying DB. -Removed legacy support for DBs using the old passphrase system (with different handling when getting DB handle). -Deprecated the isLocal flag in ConnectKeyRecord
📝 WalkthroughWalkthroughThis PR removes all remote Connect DB passphrase fetching and dual local/remote key handling. It deletes the string resource ConnectFetchDbKeyURL and the API method ApiPersonalId.fetchDbPassphrase. PersonalIdManager no longer requests or applies a remote DB passphrase during init. ConnectDatabaseHelper simplifies passphrase handling and DB open logic to a single local key path. ConnectDatabaseUtils drops isLocal-related APIs, consolidating to a single local key record and updated method signatures. ConnectKeyRecord removes the isLocal constructor parameter, deprecates the isLocal field (default true), and removes getIsLocal(). Sequence Diagram(s)sequenceDiagram
autonumber
actor User as App Start
participant PIM as PersonalIdManager
participant CDU as ConnectDatabaseUtils
participant CDH as ConnectDatabaseHelper
participant DB as Encrypted DB
rect rgb(245,248,255)
note over PIM,CDH: New flow (remote fetch removed)
User->>PIM: init(context)
PIM->>CDU: getConnectDbPassphrase(context)
CDU-->>PIM: passphrase bytes
PIM->>CDH: openConnectDb(context, passphrase)
CDH->>DB: open with key(passphrase)
DB-->>CDH: opened/exception
CDH-->>PIM: result
end
sequenceDiagram
autonumber
actor User as App Start
participant PIM as PersonalIdManager
participant API as ApiPersonalId
participant CDU as ConnectDatabaseUtils
participant CDH as ConnectDatabaseHelper
participant DB as Encrypted DB
rect rgb(255,245,245)
note over PIM,API: Previous flow (removed)
User->>PIM: init(context)
PIM-x API: fetchDbPassphrase(...) [deleted]
API--x PIM: callback with remote key [deleted]
PIM-x CDU: storeConnectDbPassphrase(context, key, isLocal=false) [removed]
end
rect rgb(245,248,255)
note over PIM,CDU: Current
PIM->>CDU: getConnectDbPassphrase(context)
CDU-->>PIM: local key only
PIM->>CDH: open with local key
CDH->>DB: open
DB-->>CDH: result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (1)
33-35: Consider renaming for clarity.The method name
handleReceivedDbPassphrasesuggests receiving a passphrase from a remote source, but the implementation now only stores locally. Consider renaming tostoreDbPassphraseor similar to reflect the simplified behavior.app/src/org/commcare/android/database/global/models/ConnectKeyRecord.java (1)
20-23: Consider adding deprecation documentation.The
@Deprecatedannotation should include a brief explanation (e.g.,@deprecated No longer used; passphrase is always local as of v2.61) to guide developers away from this field.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/res/values/strings.xml(0 hunks)app/src/org/commcare/android/database/global/models/ConnectKeyRecord.java(2 hunks)app/src/org/commcare/connect/PersonalIdManager.java(0 hunks)app/src/org/commcare/connect/database/ConnectDatabaseHelper.java(2 hunks)app/src/org/commcare/connect/database/ConnectDatabaseUtils.java(3 hunks)app/src/org/commcare/connect/network/ApiPersonalId.java(0 hunks)
💤 Files with no reviewable changes (3)
- app/src/org/commcare/connect/network/ApiPersonalId.java
- app/res/values/strings.xml
- app/src/org/commcare/connect/PersonalIdManager.java
👮 Files not reviewed due to content moderation or server errors (3)
- app/src/org/commcare/android/database/global/models/ConnectKeyRecord.java
- app/src/org/commcare/connect/database/ConnectDatabaseHelper.java
- app/src/org/commcare/connect/database/ConnectDatabaseUtils.java
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/org/commcare/connect/database/ConnectDatabaseUtils.java (1)
app/src/org/commcare/CommCareApplication.java (1)
CommCareApplication(157-1279)
app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (1)
app/src/org/commcare/connect/database/ConnectDatabaseUtils.java (1)
ConnectDatabaseUtils(16-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (27)
app/src/org/commcare/android/database/global/models/ConnectKeyRecord.java (8)
20-23: LGTM! Correct deprecation strategy.The deprecated
isLocalfield retains its persistence annotations and defaults totrue, ensuring backward compatibility with existing database records while signaling obsolescence to developers.
28-30: LGTM! Constructor correctly simplified.The constructor now only accepts the encrypted passphrase, relying on the default
isLocal = true. This aligns with the broader simplification removing local vs. remote passphrase branching.
39-41: LGTM! Migration method correctly updated.The
fromV6migration method now uses the simplified constructor signature, and the migrated record will correctly defaultisLocaltotrue.
28-30: Constructor simplification is correct.The removal of the
isLocalparameter aligns with the PR objective to eliminate dual local/remote passphrase handling. All new records will default toisLocal=true.
39-41: Migration correctly drops obsolete isLocal information.The
fromV6method now uses the simplified constructor, and migrated records will default toisLocal=true. This is intentional and safe since the PR removes all remote passphrase handling.
20-23: No explicitisLocalreferences detected; verify DB migration
Grep checks forgetIsLocal(),.isLocal =, and old two-arg constructors returned no matches—confirm existing ConnectKeyRecord instances deserialize correctly withisLocal = true.
39-41: LGTM!The migration from V6 correctly uses the new constructor signature, and the
isLocalfield will default totrue, which aligns with the simplified "always local" passphrase handling.
28-30: No remaining two-argumentConnectKeyRecordconstructor usages. Verified codebase search shows only single-argument instantiations.app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (5)
33-35: LGTM! Correctly simplified passphrase handling.The method now only stores the passphrase without rekey logic, aligning with the removal of remote vs. local passphrase branching. This is appropriate given the migration period has ended.
52-58: LGTM! DB opening flow correctly streamlined.The passphrase retrieval now uses the simplified
getConnectDbPassphrase(context)API (without theisLocalparameter), and the DB is opened directly with the encoded key. The error handling path is preserved.
52-58: DB opening logic correctly simplified.The streamlined passphrase retrieval and DB initialization align with the removal of dual local/remote passphrase handling. Thread safety and error handling are preserved.
52-58: LGTM!The DB opening path has been correctly simplified to use the single local passphrase retrieval method and construct the database helper with the encoded key. Error handling is preserved.
33-35: Simplification is safe; callers verified
handleReceivedDbPassphrase is still invoked in PersonalIdPhotoCaptureFragment (line 134) and PersonalIdBackupCodeFragment (line 227), so the refactor poses no risk.app/src/org/commcare/connect/database/ConnectDatabaseUtils.java (14)
19-19: LGTM! Public API correctly simplified.The
isLocalparameter has been removed from the method signature, aligning with the shift to always-local passphrase semantics.
30-32: LGTM! Record creation and retrieval correctly updated.The code now uses the simplified
getKeyRecord()signature and the updatedConnectKeyRecord(String)constructor. New records will default toisLocal = true.
43-49: LGTM! Simplified signature with hardcoded local filter.The method signature no longer accepts an
isLocalparameter and always queries for records whereIS_LOCAL = true, ensuring only local passphrases are retrieved. This works correctly for both legacy records (explicitly marked) and new records (defaulted).
51-58: LGTM! String overload correctly simplified.The method signature removes the
isLocalparameter and correctly delegates to the updatedstoreConnectDbPassphrase(Context, byte[])method. Base64 decoding and error handling are preserved.
60-77: LGTM! Passphrase retrieval correctly simplified.The method signature removes the
isLocalparameter and now returns abyte[]directly. The implementation uses the simplifiedgetKeyRecord()and preserves all decryption and error handling logic.
51-58: String overload correctly simplified.The method signature change is consistent with the byte[] overload simplification and properly delegates without the
isLocalparameter.
60-77: Passphrase retrieval correctly simplified.The method now always retrieves the local passphrase record. The decryption logic and error handling are preserved.
19-41: Approved: No references to old signature detected
Search returned no usages ofstoreConnectDbPassphrasewith the obsoleteisLocalparameter, confirming the signature change is safe.
43-49: Ignore isLocal filter warning. TheisLocalfield in ConnectKeyRecord.java is deprecated, defaults to true, and no code path ever sets it to false—so filtering onIS_LOCAL=truecannot omit any existing records.Likely an incorrect or invalid review comment.
30-35: LGTM!The
ConnectKeyRecordinstantiation correctly uses the new single-argument constructor. TheisLocalfield will default totrue, which is consistent with the simplified passphrase handling.
51-58: LGTM!The String-based overload correctly delegates to the simplified byte[] method. The Base64 decoding and error handling are preserved.
60-77: All callers updated to new signature and return type
Verified that the only usage in ConnectDatabaseHelper now callsgetConnectDbPassphrase(context)and handles thebyte[]return; no remaining breaking-call sites.
19-41: No callers use the removedisLocalsignature. All existing invocations target the new two-argument overloads.
43-49: No callers use the removed boolean parameter; the change to a parameterlessgetKeyRecord()is safe.
| @Persisting(2) | ||
| @MetaField(IS_LOCAL) | ||
| boolean isLocal; | ||
| boolean isLocal = true; |
There was a problem hiding this comment.
Thoughts on setting it to false given there is no local passphrase anymore ? (everything comes from server)
There was a problem hiding this comment.
Ah, good idea. I had in my head that we needed to keep checking for the local passphrase to support users upgrading with a working database, but I tested it with isLocal defaulted to false and the query simply returning the first item and it works fine as well. 34bbc35
| Vector<ConnectKeyRecord> records = CommCareApplication.instance() | ||
| .getGlobalStorage(ConnectKeyRecord.class) | ||
| .getRecordsForValue(ConnectKeyRecord.IS_LOCAL, local); | ||
| .getRecordsForValue(ConnectKeyRecord.IS_LOCAL, true); |
There was a problem hiding this comment.
can we not query for local field anymore and just get the first entry in this table ?
| //Rekey the DB if the remote passphrase is different than local | ||
| String localPassphrase = ConnectDatabaseUtils.getConnectDbEncodedPassphrase(context, true); | ||
| if (connectDatabase != null && connectDatabase.isOpen() && !remotePassphrase.equals(localPassphrase)) { | ||
| DatabaseConnectOpenHelper.rekeyDB(connectDatabase, remotePassphrase); |
There was a problem hiding this comment.
can we also clear out the rekeyDB from DatabaseConnectOpenHelper or is it still used elsewhere ?
Taking first ConnectKeyRecord entry from DB when retrieving (regardless of isLocal flag).
| if (records.iterator().hasNext()) { | ||
| return records.iterator().next(); | ||
| } |
There was a problem hiding this comment.
@OrangeAndGreen I was thinking that app should ask for local specifically to check that all users are migrated?
There was a problem hiding this comment.
I worried the same but tested and the false case is working fine through an app upgrade
|
Found one more obsolete bit of code to remove! 70c859c |
https://dimagi.atlassian.net/browse/CCCT-1574
Product Description
No user-facing changes
Technical Summary
This PR cleans up the migration code that was introduced earlier this year when we changed the Connect DB to start using a passphrase managed by PersonalID for encryption, instead of a locally-generated passphrase.
Brief description of the migration flow:
-Added an isLocal flag to ConnectKeyRecord
-All existing passphrases became "local" passphrases (isLocal=true) during the upgrade
-At app startup, we would issue a special API call if we detected the user didn't have the remote passphrase yet (isLocal=false)
-Once the remote passphrase was received, we would:
-Store it as remote
-Rekey the database if necessary (local != remote)
-Store it as local
One additional caveat:
During the change described above we realized Connect was using the wrong overload for the DatabaseConnectOpenHelper constructor, and passing in a byte[] instead of an encoded string. So while supporting the upgrade/migration, we kept the ability for the app to keep accessing the DB via the legacy method until the migration was complete. The code supporting the legacy byte[] method has also been removed in this PR.
-Connect legacy used Base64.encodeToString
-Common CC code uses UserSandboxUtils.getSqlCipherEncodedKey
Changes to the code:
-Removed obsolete code for retrieving remote passphrase and re-keying DB.
-Removed legacy support for DBs using the old passphrase system (with different handling when getting DB handle).
-Deprecated the isLocal flag in ConnectKeyRecord
Feature Flag
PersonalID
Safety Assurance
Safety story
Dev tested:
For all 4 tests, "things work as expected" simply means that PersonalID is functional. This can be tested by:
-Viewing the side nav menu and verifying the PersonalID user info is displayed
-Account does not automatically get de-configured
-No crashes
Automated test coverage
None
QA Plan
See the dev tests conducted above with success criteria