Skipping biometric Keystore when biometric not configured #3186
Conversation
📝 WalkthroughWalkthroughThis update introduces significant refactoring and improvements to the PersonalId (formerly ConnectID) authentication and registration flows. It standardizes error handling by updating API error callbacks to include detailed error types and messages, and introduces a new OtpErrorType enum for granular OTP error reporting. Biometric secret key management is enhanced with methods to check key validity, handle invalidation, and delete keys, alongside analytics reporting for biometric invalidation events. The UI and string resources are extensively updated to replace "ConnectID" with "PersonalId" and to improve error message visibility and feedback throughout the registration and recovery processes. Several localization files are cleaned up, removing obsolete or redundant strings. Compatibility checks for device support are added, and context usage in database helpers is streamlined. The control flow for phone number entry, OTP verification, and backup code handling is refactored for better error handling and user experience. Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant PersonalIdManager
participant BiometricKeyStore
participant FirebaseAuthService
participant PersonalIdApiHandler
participant Analytics
User->>App: Initiate PersonalId registration/login
App->>PersonalIdManager: unlockConnect()
PersonalIdManager->>BiometricKeyStore: logBiometricInvalidations()
alt Biometric key invalid
BiometricKeyStore->>Analytics: reportBiometricInvalidated()
BiometricKeyStore->>BiometricKeyStore: deleteKey()
BiometricKeyStore->>BiometricKeyStore: generateNewKey()
end
App->>FirebaseAuthService: requestOtp(phone)
FirebaseAuthService->>User: Send OTP via SMS
User->>App: Enter OTP
App->>FirebaseAuthService: verifyOtp(code)
FirebaseAuthService->>App: onSuccess / onFailure(OtpErrorType, message)
alt onSuccess
App->>PersonalIdApiHandler: validateFirebaseIdToken()
PersonalIdApiHandler->>App: onSuccess / onFailure(ErrorCode, Throwable)
alt onFailure
App->>App: Show error message (from PersonalIdApiErrorHandler)
end
else onFailure
App->>App: Show OTP error message (based on OtpErrorType)
end
Possibly related PRs
Suggested reviewers
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 11
🔭 Outside diff range comments (4)
app/src/org/commcare/connect/network/ApiPersonalId.java (1)
233-240:⚠️ Potential issue
processFailureis invoked twice – leads to double callbacksInside the error branch you call
callback.processFailure(...)once when
errorBody != nulland again unconditionally afterwards. This causes UI
handlers to fire twice and can easily break state machines.- if (response.errorBody() != null){ - callback.processFailure(response.code(), response.errorBody().byteStream()); - } - callback.processFailure(response.code(), null); + if (response.errorBody() != null) { + callback.processFailure(response.code(), response.errorBody().byteStream()); + } else { + callback.processFailure(response.code(), null); + }app/src/org/commcare/utils/BiometricsHelper.java (1)
220-238:⚠️ Potential issue
crashWithInvalidSecurityTypeException()silently swallows the exception
crashWithInvalidSecurityTypeException()merely instantiatesRuntimeExceptionwithout throwing or logging it.
Consequently, the calling code continues execution with an invalidrequiredLock, very likely leading to obscure failures later.-private static void crashWithInvalidSecurityTypeException(Activity activity, String requiredLock) { - new RuntimeException(activity.getString(R.string.personalid_configuration_process_failed_server_msg, requiredLock)); -} +private static void crashWithInvalidSecurityTypeException(Activity activity, String requiredLock) { + throw new IllegalArgumentException( + activity.getString( + R.string.personalid_configuration_process_failed_server_msg, + requiredLock)); +}Fail fast and explicitly.
app/src/org/commcare/fragments/personalId/PersonalIdPhotoCaptureFragment.java (1)
103-110: 🛠️ Refactor suggestionGuard against blank messages when displaying API errors
handle()may legitimately return an empty string (see TOKEN_DENIED case).
CallingshowError()unconditionally then surfaces a blank message.- showError(PersonalIdApiErrorHandler.handle(requireActivity(), failureCode, t)); + String msg = PersonalIdApiErrorHandler.handle(requireActivity(), failureCode, t); + if (!android.text.TextUtils.isEmpty(msg)) { + showError(msg); + } else { + clearError(); + }(Requires
import android.text.TextUtils;)app/src/org/commcare/fragments/personalId/PersonalIdBackupCodeFragment.java (1)
191-197: 🛠️ Refactor suggestionSame empty-string corner-case as PhotoCaptureFragment
Blank strings lead to a visible but empty error label.
- showError(PersonalIdApiErrorHandler.handle(requireActivity(), failureCode, t)); + String msg = PersonalIdApiErrorHandler.handle(requireActivity(), failureCode, t); + if (!TextUtils.isEmpty(msg)) { + showError(msg); + } else { + clearError(); + }Add
import android.text.TextUtils;
🧹 Nitpick comments (20)
app/src/org/commcare/fragments/MicroImageActivity.java (1)
113-117: Hard-coding PersonalId-specific copy reduces re-usability
MicroImageActivityis a generic micro-photo capture component, yet the permission rationale now uses
personalid_camera_permission_*strings.
If the same activity is reused outside the PersonalId flow (e.g. another feature requiring camera), end-users will see irrelevant copy.Consider either
- passing the title/message via
Intentextras, or- reverting to generic camera-permission strings.
app/src/org/commcare/android/security/AndroidKeyStore.kt (1)
16-20: Expose failure information fromdeleteKey
KeyStore.deleteEntry()can throwKeyStoreException; silently swallowing it makes diagnosis impossible.fun deleteKey(alias: String) { if (instance.containsAlias(alias)) { - instance.deleteEntry(alias) + try { + instance.deleteEntry(alias) + } catch (e: Exception) { + Logger.exception("Unable to delete keystore entry: $alias", e) + throw e // or return false and let caller decide + } } }Returning a boolean or propagating the exception will let higher layers react (e.g., fall back or show error).
app/src/org/commcare/activities/LoginActivityUIController.java (1)
567-569: Connect button now hard-hidden for all states – confirm requirement
setConnectButtonVisible(false)is executed both when the PersonalId user is signed in and when they are not, effectively disabling the button altogether.
Given the preceding logic (and the TODO), this looks intentional but temporary. Please confirm that:
- No flows currently depend on the user being able to sign-out / switch PersonalId while on the login screen.
- Any analytics that relied on the button visibility are updated accordingly.
If this is only a stop-gap, consider guarding it behind a feature-flag rather than hard-coding.
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
529-532: Missing rate-limit / doc for newreportBiometricInvalidated()helperThe helper correctly delegates to
reportEvent, but unlike most other public wrappers it:
- Lacks a short KDoc/Javadoc explaining the trigger condition.
- Emits every invalidation with no sampling (many of the similar low-frequency events use
rateLimitReporting()defensively).If the invalidation can fire repeatedly on a mis-configured device, consider adding a small sample gate or dedupe guard.
app/src/org/commcare/android/security/KeyStoreHandler.kt (1)
7-9: Interface expansion OK – consider return value ondeleteKey()Adding
isKeyValid()anddeleteKey()brings welcome parity with the Android keystore APIs.
Minor: returningBoolean(success/failure) or throwing a typed exception fromdeleteKey()would let callers react to failure instead of treating it as best-effort.app/src/org/commcare/connect/network/ApiPersonalId.java (1)
219-223: Potential activity leak – keep only a weak reference tocontext
callApicaptures the passedContextin an anonymous Retrofit callback.
If thatContextis anActivity, it will be retained for the full lifetime of
the network request, surviving configuration changes and risking leaks.
Consider holding aWeakReference<Context>(or passing only the
Applicationcontext where possible) and guarding againstnullbefore UI
operations (show/dismissProgressDialog).app/src/org/commcare/utils/PhoneNumberHelper.java (1)
95-108: Method works but could bestaticand returnStringThe helper never uses instance state other than
phoneNumberUtil, so
getNationalNumbercan bestaticand delegate to
PhoneNumberHelper.getInstance(context)when needed.Also,
longcannot represent numbers ≥ 9 223 372 036 854 775 807; some
national numbers exceed this. ReturningStringavoids truncation.Optional refactor:
- public long getNationalNumber(String phone) { + public String getNationalNumber(String phone) { try { Phonenumber.PhoneNumber phoneNumber = phoneNumberUtil.parse(phone, null); if (phoneNumberUtil.isValidNumber(phoneNumber)) { - return phoneNumber.getNationalNumber(); + return String.valueOf(phoneNumber.getNationalNumber()); } } catch (NumberParseException ignored) { } - return -1; + return ""; }app/src/org/commcare/activities/CommCareSetupActivity.java (1)
508-512: Visibility logic is sound – minor naming nitpickThe additional
checkDeviceCompability()guard correctly hides the Connect ID sign-in option on unsupported devices.
However, the method name is miss-spelled (“Compability”). In long-lived code this kind of typo proliferates and hurts searchability and consistency.- && PersonalIdManager.getInstance().checkDeviceCompability()); + && PersonalIdManager.getInstance().checkDeviceCompatibility());Renaming in both
PersonalIdManagerand call-sites would be a quick clean-up.app/src/org/commcare/utils/EncryptionKeyProvider.java (1)
48-55: NewisKeyValid()/deleteKey()wrappers look correct – consider caching handlerBoth methods simply delegate to
getHandler(false); this re-evaluates device version and key-existence every call.
If these checks are performed frequently (e.g. inside a polling loop) you could cache the chosenKeyStoreHandleron first access:- private KeyStoreHandler getHandler(boolean isEncryptMode) { + private volatile KeyStoreHandler cachedDecryptHandler; // lazily initialised + + private KeyStoreHandler getHandler(boolean isEncryptMode) { + if (!isEncryptMode && cachedDecryptHandler != null) { + return cachedDecryptHandler; + } RsaKeyStoreHandler rsaKeystoreHandler = new RsaKeyStoreHandler(context, keyAlias, isEncryptMode); ... - return new AesKeyStoreHandler(keyAlias, needsUserAuth); + KeyStoreHandler handler = new AesKeyStoreHandler(keyAlias, needsUserAuth); + if (!isEncryptMode) { + cachedDecryptHandler = handler; + } + return handler;Not critical, but avoids redundant object creation.
app/src/org/commcare/utils/OtpVerificationCallback.java (1)
30-35: Javadoc & nullability are out of syncThe new signature introduces
errorTypeand marksmessageas nullable, but the Javadoc still documents a singlemessageparameter and omitserrorType.- * @param message A description of the error that occurred + * @param errorType Categorised reason for the failure + * @param message Optional human-readable detail (may be null)Keeping the Javadoc accurate avoids misunderstandings for implementers.
app/src/org/commcare/android/security/AesKeyStoreHandler.kt (1)
64-71: Parameter name shadows property – leads to fragile code
generateAesKey(alias: String, …)shadows the class propertyalias.
Inside the method both identifiers are indistinguishable; future refactors can easily introduce bugs.Rename the parameter for clarity or remove it altogether – the property is already available inside the class.
-private fun generateAesKey(alias: String, needsUserAuth: Boolean): SecretKey { +private fun generateAesKey(keyAlias: String, needsUserAuth: Boolean): SecretKey { val keyGenerator = KeyGenerator.getInstance( KeyProperties.KEY_ALGORITHM_AES, AndroidKeyStore.ANDROID_KEY_STORE_NAME ) - val builder = KeyGenParameterSpec.Builder( - alias, + val builder = KeyGenParameterSpec.Builder( + keyAlias,app/res/values-ti/strings.xml (1)
235-242: Resource IDs still use legacy “connect” prefix – creates confusionAlthough the feature has been re-branded to PersonalId, new strings continue to use IDs like
login_password_by_connectorconnect_registration_title.
Keeping historical IDs inflates cognitive load and hinders grep-based searches.Consider introducing
personalid_…IDs for all new/renamed strings and add a quick migration comment. This is a one-time hit that pays off in readability long-term.app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java (2)
17-27: Store iterator once to avoid double traversal
records.iterator()is called twice – once forhasNext()and again fornext().
Creating the iterator once avoids the second traversal and is marginally faster/readable.- if (records.iterator().hasNext()) { - return records.iterator().next(); + Iterator<ConnectUserRecord> it = records.iterator(); + if (it.hasNext()) { + return it.next(); }
50-66: Skip delete when DB is already absent to reduce noisy exceptions
DatabaseConnectOpenHelper.deleteDb()is invoked un-conditionally.
When the DB has already been removed (fairly common during logout / re-login loops) the helper can throwIllegalStateException, resulting in unnecessary error logging & exception wrapping.- DatabaseConnectOpenHelper.deleteDb(); + if (ConnectDatabaseHelper.dbExists()) { + DatabaseConnectOpenHelper.deleteDb(); + }app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (1)
260-269: Key-generation on UI thread may cause jank
EncryptionKeyProvider#getKeyForEncryption()touches the Android Keystore and
can take ~100 ms on some devices. Executing it insidenavigateForwardruns
on the main thread right before a navigation transition – risking UI hitches.Consider off-loading to a worker thread and continuing navigation in its
completion callback, e.g.:Executors.newSingleThreadExecutor().execute(() -> { new EncryptionKeyProvider(requireContext(), true, BIOMETRIC_INVALIDATION_KEY) .getKeyForEncryption(); });app/src/org/commcare/connect/network/PersonalIdApiHandler.java (2)
40-43: Consider adding INVALID_RESPONSE_ERROR to retry logicThe
INVALID_RESPONSE_ERRORis still used in the code (line 103) but is not included in theshouldAllowRetry()method. Consider whether this error should be retryable, especially for transient network issues that might cause invalid responses.
97-101: Improve error handling in catch blockWhen JSON parsing or IO exceptions occur while parsing the error response, you're calling
onFailurewithUNKNOWN_ERRORand then returning, which prevents the original error information from being passed. Consider preserving the original error context.- } catch (JSONException | IOException e) { - Logger.exception("Error parsing API error response", e); - onFailure(PersonalIdApiErrorCodes.UNKNOWN_ERROR, e); - return; + } catch (JSONException | IOException e) { + Logger.exception("Error parsing API error response", e); + // Continue with the original error information + info.append(" (Failed to parse error response)"); }app/res/values/strings.xml (1)
596-596: Remove unnecessary newline in stringThe string has a trailing newline character which might cause UI layout issues.
- <string name="personalid_login_via_connect">Ready to login!\n</string> + <string name="personalid_login_via_connect">Ready to login!</string>app/res/values-fr/strings.xml (1)
436-436: Remove trailing newline for consistencySimilar to the English version, remove the trailing newline.
- <string name="personalid_login_via_connect">Prêt à vous connecter !\n</string> + <string name="personalid_login_via_connect">Prêt à vous connecter !</string>app/src/org/commcare/utils/FirebaseAuthService.java (1)
86-99: Comprehensive error type mapping for better user feedback.The granular error handling provides specific error types that enable appropriate user messaging. Consider adding logging for debugging purposes.
Consider adding debug logging to help troubleshoot authentication issues:
private void handleFirebaseException(Exception e) { + Logger.log(LogTypes.TYPE_MAINTENANCE, "Firebase auth exception: " + e.getClass().getSimpleName()); if (e instanceof FirebaseAuthInvalidCredentialsException) { callback.onFailure(OtpErrorType.INVALID_CREDENTIAL, null);
🛑 Comments failed to post (11)
app/res/layout/fragment_recovery_code.xml (1)
185-189: 💡 Verification agent
🧩 Analysis chain
Verify availability of the new
personalid_this_is_not_mestring in all localesThe layout now references
@string/personalid_this_is_not_me.
If this key is missing from any localizedstrings.xml, the app will crash at runtime with aResources$NotFoundException.If the command prints any path, that locale is missing the translation.
🏁 Script executed:
#!/bin/bash # Verify that the string is defined in every strings.xml present fd strings.xml | xargs grep -L '<string name="personalid_this_is_not_me">'Length of output: 363
Add
personalid_this_is_not_meto all localized strings.xmlThe new key
personalid_this_is_not_meis missing in several resource files. If left undefined, referencing it will crash the app at runtime. Please add the string (or a placeholder) to each of the following:• app/res/values-es/strings.xml
• app/res/values-large/strings.xml
• app/res/values-lt/strings.xml
• app/res/values-no/strings.xml
• app/res/values-small/strings.xml
• app/res/values-sw720dp-land/strings.xml
• app/res/values-xlarge/strings.xml
• commcare-support-library/src/main/res/values/strings.xml🤖 Prompt for AI Agents
In app/res/layout/fragment_recovery_code.xml around lines 185 to 189, the layout references the string resource personalid_this_is_not_me, but this key is missing in multiple localized strings.xml files. To fix this, add the personalid_this_is_not_me string entry with appropriate translations or placeholders to all the listed localized strings.xml files, including app/res/values-es/strings.xml, app/res/values-large/strings.xml, app/res/values-lt/strings.xml, app/res/values-no/strings.xml, app/res/values-small/strings.xml, app/res/values-sw720dp-land/strings.xml, app/res/values-xlarge/strings.xml, and commcare-support-library/src/main/res/values/strings.xml to prevent runtime crashes.app/src/org/commcare/utils/OtpManager.java (1)
23-29: 🛠️ Refactor suggestion
Re-introduce basic input validation to prevent unnecessary backend calls
requestOtpandsubmitOtpnow forward arguments blindly.
Passingnull/blank values will bubble down toFirebaseAuthand return immediate failures, but that still triggers network/async overhead and clutters logs.public void requestOtp(String phoneNumber) { - authService.requestOtp(phoneNumber); + if (phoneNumber == null || phoneNumber.trim().isEmpty()) { + authService.handleClientSideError(OtpErrorType.INVALID_INPUT, + "Phone number cannot be empty"); + return; + } + authService.requestOtp(phoneNumber); } public void submitOtp(String code) { - authService.verifyOtp(code); + if (code == null || code.trim().isEmpty()) { + authService.handleClientSideError(OtpErrorType.INVALID_INPUT, + "OTP code cannot be empty"); + return; + } + authService.verifyOtp(code); }This keeps the public surface of
OtpManagerdefensive and avoids superfluous error callbacks.📝 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.public void requestOtp(String phoneNumber) { if (phoneNumber == null || phoneNumber.trim().isEmpty()) { authService.handleClientSideError( OtpErrorType.INVALID_INPUT, "Phone number cannot be empty" ); return; } authService.requestOtp(phoneNumber); } public void submitOtp(String code) { if (code == null || code.trim().isEmpty()) { authService.handleClientSideError( OtpErrorType.INVALID_INPUT, "OTP code cannot be empty" ); return; } authService.verifyOtp(code); }🤖 Prompt for AI Agents
In app/src/org/commcare/utils/OtpManager.java around lines 23 to 29, the methods requestOtp and submitOtp currently forward their arguments without validation, allowing null or blank inputs to trigger unnecessary backend calls and clutter logs. Add basic input validation to both methods to check if the phoneNumber or code is null or blank before calling authService methods. If the input is invalid, simply return early without making the backend call to keep the public API defensive and reduce superfluous error callbacks.app/src/org/commcare/AppUtils.java (1)
170-174: 💡 Verification agent
🧩 Analysis chain
Duplicate
ccvparameter & missingVERSION_NAME– verify placeholder order
Localization.get()now receives
{ccv, VERSION_CODE, ccv, buildNumber, buildDate, profileVersion}.
That means the CommCare version string is passed twice whileBuildConfig.VERSION_NAME
has disappeared.
Unless the translatable string was intentionally reordered to expect the extra copy, this
will surface as either (a) the same value shown twice in the UI, or (b) a
MissingFormatArgumentExceptionat runtime.- ccv, String.valueOf(BuildConfig.VERSION_CODE), ccv, buildNumber, buildDate, profileVersion}); + BuildConfig.VERSION_NAME, String.valueOf(BuildConfig.VERSION_CODE), + ccv, buildNumber, buildDate, profileVersion});Please cross-check
res/values/android_translatable_strings.txt(key
app_version_string) to ensure the argument indexes really match.
I’ll verify the placeholder order by inspecting both the extracted translations file and the XML string resource.
🏁 Script executed:
#!/bin/bash set -e echo "=== Locating android_translatable_strings.txt ===" fd android_translatable_strings.txt if file=$(fd android_translatable_strings.txt); then echo echo "=== Snippet around app_version_string in $file ===" grep -n "app_version_string" "$file" -B2 -A2 || true fi echo echo "=== Searching for app_version_string in XML ===" rg "<string name=\"app_version_string\"" -n -B2 -A2Length of output: 3179
I’ll pull in the actual template entries and inspect how
ccvis defined:
🏁 Script executed:
#!/bin/bash set -e LOCALES_FILE="app/assets/locales/android_translatable_strings.txt" echo "=== version.id.long entry in $LOCALES_FILE ===" grep -n "version.id.long" "$LOCALES_FILE" -B1 -A3 || true echo echo "=== version.id.short entry in $LOCALES_FILE ===" grep -n "version.id.short" "$LOCALES_FILE" -B1 -A3 || true echo echo "=== Context around ccv in AppUtils.java ===" sed -n '130,200p' app/src/org/commcare/AppUtils.javaLength of output: 4264
Fix placeholder mapping in getCurrentVersionString()
The parameter array currently passes the CommCare version (
ccv) twice and omits the app’s version name, causing placeholders${0}and${2}to be identical. It should passBuildConfig.VERSION_NAMEfirst, then version code, thenccv, followed by build number, build date, and profile version.Location:
- app/src/org/commcare/AppUtils.java, in the
getCurrentVersionString()methodRequired change:
- return Localization.get(application.getString(R.string.app_version_string), new String[]{ - ccv, String.valueOf(BuildConfig.VERSION_CODE), ccv, buildNumber, buildDate, profileVersion}); + return Localization.get(application.getString(R.string.app_version_string), new String[]{ + BuildConfig.VERSION_NAME, String.valueOf(BuildConfig.VERSION_CODE), + ccv, buildNumber, buildDate, profileVersion});📝 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.String buildNumber = BuildConfig.BUILD_NUMBER; return Localization.get(application.getString(R.string.app_version_string), new String[]{ BuildConfig.VERSION_NAME, String.valueOf(BuildConfig.VERSION_CODE), ccv, buildNumber, buildDate, profileVersion}); }🤖 Prompt for AI Agents
In app/src/org/commcare/AppUtils.java around lines 170 to 174, the getCurrentVersionString() method incorrectly passes the CommCare version string (ccv) twice and omits BuildConfig.VERSION_NAME in the parameter array for Localization.get(). To fix this, replace the second occurrence of ccv with BuildConfig.VERSION_NAME so the parameters are ordered as VERSION_NAME, VERSION_CODE, ccv, buildNumber, buildDate, and profileVersion, matching the expected placeholders in the translation string.app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java (1)
41-52: 🛠️ Refactor suggestion
Null-safety of
personalIdSessionData
personalIdSessionDatais fetched viaViewModelProviderbut never checked for null.
While unlikely, returningnull(e.g., view-model not initialised) would NPE at line 145.-personalIdSessionData = new ViewModelProvider(requireActivity()) - .get(PersonalIdSessionDataViewModel.class).getPersonalIdSessionData(); +personalIdSessionData = new ViewModelProvider(requireActivity()) + .get(PersonalIdSessionDataViewModel.class) + .getPersonalIdSessionData(); +if (personalIdSessionData == null) { + Logger.exception("PersonalIdSessionData unavailable", new IllegalStateException()); + dismiss(); // or alternative graceful handling +}Worth guarding.
📝 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.private PersonalIdSessionData personalIdSessionData; @Override public View onCreateView(@NonNull LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) { binding = ScreenPersonalidMessageBinding.inflate(inflater, container, false); View view = binding.getRoot(); loadSavedState(savedInstanceState); - personalIdSessionData = new ViewModelProvider(requireActivity()).get( - PersonalIdSessionDataViewModel.class).getPersonalIdSessionData(); + personalIdSessionData = new ViewModelProvider(requireActivity()) + .get(PersonalIdSessionDataViewModel.class) + .getPersonalIdSessionData(); + if (personalIdSessionData == null) { + Logger.exception("PersonalIdSessionData unavailable", new IllegalStateException()); + dismiss(); // or alternative graceful handling + } binding.connectMessageButton.setOnClickListener(v -> handleContinueButtonPress()); return view; }🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/personalId/PersonalIdMessageFragment.java around lines 41 to 52, the personalIdSessionData obtained from ViewModelProvider is not checked for null, which could cause a NullPointerException later. Add a null check after fetching personalIdSessionData and handle the null case appropriately, such as showing an error message or disabling related UI elements, to ensure null-safety and prevent crashes.app/src/org/commcare/android/security/AesKeyStoreHandler.kt (1)
38-47:
⚠️ Potential issue
isKeyValid()only handles one failure mode
Cipher.getInstance(..)/cipher.init()can throw several otherGeneralSecurityExceptionsubclasses (UnrecoverableKeyException,InvalidKeyException, etc.).
Catching onlyKeyPermanentlyInvalidatedExceptionrisks returningtruewhen the key is actually unusable for another reason, causing downstream cryptic failures.-} catch (_: KeyPermanentlyInvalidatedException) { - return false +} catch (e: GeneralSecurityException) { + // Any failure to init the cipher means the key is not usable + return false }📝 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.override fun isKeyValid(): Boolean { val key = getKeyIfExists() ?: return false try { val cipher = Cipher.getInstance(TRANSFORM); cipher.init(Cipher.ENCRYPT_MODE, key); return true } catch (e: GeneralSecurityException) { // Any failure to init the cipher means the key is not usable return false } }🤖 Prompt for AI Agents
In app/src/org/commcare/android/security/AesKeyStoreHandler.kt around lines 38 to 47, the isKeyValid() method currently only catches KeyPermanentlyInvalidatedException, but other GeneralSecurityException subclasses like UnrecoverableKeyException and InvalidKeyException can also be thrown. Modify the catch block to handle all relevant GeneralSecurityException subclasses by catching a broader exception such as GeneralSecurityException or multiple specific exceptions, and return false in those cases to ensure the method accurately reflects key validity.app/src/org/commcare/connect/network/PersonalIdApiErrorHandler.java (1)
31-47:
⚠️ Potential issueAvoid returning an empty string – UI ends up showing a blank error banner
Returning
""forTOKEN_DENIED_ERRORcauses callers such as
showError(message)to display a visible but emptyTextView.Either return a concrete, translatable message or make the contract explicit
( e.g. returnnull) so that the caller can skipshowError.- case TOKEN_DENIED_ERROR: - ConnectNetworkHelper.handleTokenDeniedException(); - return ""; + case TOKEN_DENIED_ERROR: + ConnectNetworkHelper.handleTokenDeniedException(); + return activity.getString(R.string.recovery_network_token_denied);📝 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.public static String handle(Activity activity, PersonalIdApiHandler.PersonalIdApiErrorCodes errorCode, Throwable t) { switch (errorCode) { case NETWORK_ERROR: return activity.getString(R.string.recovery_network_unavailable); case TOKEN_UNAVAILABLE_ERROR: return activity.getString(R.string.recovery_network_token_unavailable); case RATE_LIMIT_EXCEEDED_ERROR: return activity.getString(R.string.recovery_network_cooldown); case FAILED_AUTH_ERROR: return activity.getString(R.string.recovery_network_unauthorized); case SERVER_ERROR: return activity.getString(R.string.recovery_network_server_error); case TOKEN_DENIED_ERROR: ConnectNetworkHelper.handleTokenDeniedException(); return activity.getString(R.string.recovery_network_token_denied); case OLD_API_ERROR:🤖 Prompt for AI Agents
In app/src/org/commcare/connect/network/PersonalIdApiErrorHandler.java between lines 31 and 47, the method returns an empty string for the TOKEN_DENIED_ERROR case, which causes the UI to show a blank error banner. To fix this, either return a meaningful, translatable error message string for this case or return null explicitly so that the caller can detect it and avoid displaying an empty error message.app/src/org/commcare/connect/PersonalIdManager.java (2)
217-231:
⚠️ Potential issuePotential ClassCastException in biometric invalidation logging
The cast to
CommCareActivity<?>on line 218 could throw aClassCastExceptionifparentActivityis not an instance ofCommCareActivity. Consider adding an instanceof check:private void logBiometricInvalidations() { - CommCareActivity<?> activity = (CommCareActivity<?>) parentActivity; - if(BiometricsHelper.isFingerprintConfigured(parentActivity, - PersonalIdManager.getInstance().getBiometricManager(activity))) { + if (parentActivity instanceof CommCareActivity<?> activity && + BiometricsHelper.isFingerprintConfigured(parentActivity, + PersonalIdManager.getInstance().getBiometricManager(activity))) { EncryptionKeyProvider encryptionKeyProvider = new EncryptionKeyProvider(parentActivity, true, BIOMETRIC_INVALIDATION_KEY); if (!encryptionKeyProvider.isKeyValid()) { FirebaseAnalyticsUtil.reportBiometricInvalidated(); // reset key encryptionKeyProvider.deleteKey(); encryptionKeyProvider.getKeyForEncryption(); } } }📝 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.private void logBiometricInvalidations() { if (parentActivity instanceof CommCareActivity<?> activity && BiometricsHelper.isFingerprintConfigured(parentActivity, PersonalIdManager.getInstance().getBiometricManager(activity))) { EncryptionKeyProvider encryptionKeyProvider = new EncryptionKeyProvider(parentActivity, true, BIOMETRIC_INVALIDATION_KEY); if (!encryptionKeyProvider.isKeyValid()) { FirebaseAnalyticsUtil.reportBiometricInvalidated(); // reset key encryptionKeyProvider.deleteKey(); encryptionKeyProvider.getKeyForEncryption(); } } }🤖 Prompt for AI Agents
In app/src/org/commcare/connect/PersonalIdManager.java around lines 217 to 231, the cast of parentActivity to CommCareActivity<?> may cause a ClassCastException if parentActivity is not an instance of CommCareActivity. To fix this, add an instanceof check before casting parentActivity. Only perform the cast and subsequent operations if parentActivity is an instance of CommCareActivity; otherwise, safely skip or handle the case to avoid the exception.
686-688: 💡 Verification agent
🧩 Analysis chain
Verify API level requirement for PersonalId features.
The method checks for API 24 (Nougat), which is appropriate for the biometric and security features used.
Confirm that all PersonalId security features are compatible with API 24:
🌐 Web query:
What are the minimum Android API requirements for BiometricManager and Android Keystore biometric-bound keys?💡 Result:
Minimum Android API Requirements
BiometricManager
- The
BiometricManagerclass was introduced in Android 10 (API level 29)[2][6].- With the support library (
androidx.biometric), many biometric features—includingBiometricPrompt—are backported to Android 6.0 (API level 23) and above, but core platform capabilities forBiometricManagerare natively available from API 29 onward[6]. The compatibility library extends support for querying biometric enrollment and availability further back, but developers should target API level 29+ for consistentBiometricManagerfunctionality.Android Keystore Biometric-Bound Keys
- The Android Keystore system allows you to generate cryptographic keys that are bound to authentication using biometrics or device credentials.
- The ability to require user authentication for each use of a key (i.e., per-use authentication support, which is essential for secure biometric-bound keys) is fully available starting with Android 9 (API level 28)[8].
- From Android 9+, you can create keys in the Keystore that require biometric authentication for each operation by using parameters such as
setUserAuthenticationRequired(true)andsetUserAuthenticationValidityDurationSeconds(-1)[8][6].- Additional improvements, such as distinguishing between strong and weak biometrics, credential types, and advanced authenticator options, were introduced in Android 10 and Android 11 (API levels 29–30)[2][4].
Summary Table
Feature Minimum API Level Notes BiometricManager (core platform) 29 (Android 10) Support library gives backward compatibility to 23+. BiometricPrompt (with CryptoObject) 28 (Android 9) Required for secure per-use biometric-bound keys. Biometric-bound Keystore keys 28 (Android 9) Use of setUserAuthenticationRequiredand other params for per-use biometric enforcement.Key Points
- Use API level 28 (Android 9) or higher if your app needs to generate biometric-bound Keystore keys and enforce per-use biometric authentication[8][6].
BiometricManageris native from API 29 but compatibility libraries allow querying availability on older versions[6].- For advanced features (e.g., distinguishing between strong/weak biometrics, device credential as fallback), target API 29+ and integrate updates from the support library as needed[2][4][6].
This ensures your app supports biometric authentication securely and in line with modern Android security best practices.
Citations:
- 1: https://developer.android.com/jetpack/androidx/releases/biometric
- 2: https://source.android.com/docs/security/features/biometric
- 3: https://docs.onespan.com/docs/biometric-sensor-sdk-ig-overview-4-30-0
- 4: https://developer.android.com/reference/android/hardware/biometrics/BiometricManager.Authenticators
- 5: https://support.google.com/googleplay/android-developer/answer/11926878
- 6: https://mas.owasp.org/MASTG/0x05f-Testing-Local-Authentication/
- 7: https://android.googlesource.com/platform/prebuilts/fullsdk/sources/+/refs/heads/androidx-compose-integration-release/android-34/android/security/keystore/KeyProtection.java
- 8: https://developer.android.com/privacy-and-security/keystore
- 9: https://stytch.com/blog/android-keystore-pitfalls-and-best-practices/
- 10: https://android.googlesource.com/platform/frameworks/base/+/master/keystore/java/android/security/keystore/KeyProtection.java
Update minimum API requirement for biometric and keystore features
The current
checkDeviceCompatibility()enforces only API 24 (Nougat), but:
- Biometric-bound Keystore keys with per-use authentication require Android 9 (API 28+,
Build.VERSION_CODES.P).BiometricManageris natively available from Android 10 (API 29), though AndroidX Biometric backports only availability checks to API 23+.Please update
checkDeviceCompatibility()to at leastreturn Build.VERSION.SDK_INT >= Build.VERSION_CODES.P;or, better yet, perform a runtime feature check via
BiometricManager.from(context).canAuthenticate() == BiometricManager.BIOMETRIC_SUCCESS🤖 Prompt for AI Agents
In app/src/org/commcare/connect/PersonalIdManager.java around lines 686 to 688, the method checkDeviceCompability currently checks for API level 24, which is insufficient for biometric-bound keystore keys requiring per-use authentication that need at least API 28 (Android 9). Update the method to check for Build.VERSION_CODES.P (API 28) or higher. Alternatively, replace the version check with a runtime biometric feature check using BiometricManager.from(context).canAuthenticate() == BiometricManager.BIOMETRIC_SUCCESS to ensure device compatibility with biometric features.app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (2)
107-114:
⚠️ Potential issueFix syntax error in switch expression
There's a syntax error in the default case of the switch expression. The ternary operator needs proper parentheses:
- getString(R.string.personalid_otp_verification_failed_generic) + errorMessage != null ? errorMessage : "Unknown error"; + getString(R.string.personalid_otp_verification_failed_generic) + (errorMessage != null ? errorMessage : "Unknown error");📝 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.String userMessage = switch (errorType) { case INVALID_CREDENTIAL -> getString(R.string.personalid_incorrect_otp); case TOO_MANY_REQUESTS -> getString(R.string.personalid_too_many_otp_attempts); case MISSING_ACTIVITY -> getString(R.string.personalid_otp_missing_activity); case VERIFICATION_FAILED -> getString(R.string.personalid_otp_verification_failed); default -> getString(R.string.personalid_otp_verification_failed_generic) + (errorMessage != null ? errorMessage : "Unknown error"); };🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java around lines 107 to 114, the default case in the switch expression has a syntax error due to missing parentheses around the ternary operator. Fix this by enclosing the ternary expression in parentheses to ensure correct evaluation order, like wrapping (errorMessage != null ? errorMessage : "Unknown error") properly within the string concatenation.
107-114:
⚠️ Potential issueClean error message mapping with switch expression.
The switch expression provides clear mapping from error types to user messages. Note the potential issue on line 113.
Fix the conditional operator precedence issue:
- getString(R.string.personalid_otp_verification_failed_generic) + errorMessage != null ? errorMessage : "Unknown error"; + getString(R.string.personalid_otp_verification_failed_generic) + (errorMessage != null ? errorMessage : "Unknown error");The current code would concatenate the string resource with the boolean result of
errorMessage != nullinstead of the intended error message.📝 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.String userMessage = switch (errorType) { case INVALID_CREDENTIAL -> getString(R.string.personalid_incorrect_otp); case TOO_MANY_REQUESTS -> getString(R.string.personalid_too_many_otp_attempts); case MISSING_ACTIVITY -> getString(R.string.personalid_otp_missing_activity); case VERIFICATION_FAILED -> getString(R.string.personalid_otp_verification_failed); default -> getString(R.string.personalid_otp_verification_failed_generic) + (errorMessage != null ? errorMessage : "Unknown error"); };🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java around lines 107 to 114, the default case in the switch expression has a conditional operator precedence issue causing incorrect string concatenation. Fix this by adding parentheses around the conditional expression so that the concatenation happens with the actual errorMessage if it is not null, otherwise with "Unknown error".app/src/org/commcare/connect/network/PersonalIdApiHandler.java (1)
88-96:
⚠️ Potential issueRemove UI operations from API handler
The API handler should not display Toast messages directly. This violates the separation of concerns principle and could cause crashes if called from a background thread.
Remove the Toast and let the UI layer handle error display:
- info.append(": ").append(json.optString("error")); - Toast.makeText(CommCareApplication.instance(), json.optString("error"), - Toast.LENGTH_LONG).show(); + String errorMessage = json.optString("error"); + info.append(": ").append(errorMessage);Consider passing the error message through the exception:
- onFailure(PersonalIdApiErrorCodes.INVALID_RESPONSE_ERROR, new Exception(info.toString())); + String errorMessage = info.toString(); + if (json != null && json.has("error")) { + errorMessage = json.optString("error"); + } + onFailure(PersonalIdApiErrorCodes.INVALID_RESPONSE_ERROR, new Exception(errorMessage));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/network/PersonalIdApiHandler.java around lines 88 to 96, remove the Toast.makeText call that displays the error message directly in the API handler, as UI operations should not be performed here. Instead, extract the error message from the JSON response and pass it through the exception or return it to the UI layer, which will handle displaying the error message appropriately.
| CommCareActivity<?> activity = (CommCareActivity<?>) parentActivity; | ||
| if(BiometricsHelper.isFingerprintConfigured(parentActivity, | ||
| PersonalIdManager.getInstance().getBiometricManager(activity))) { |
There was a problem hiding this comment.
instead we can just check if key exists in the Keystore
Checking for invalidated biometric key when it exists (instead of based on user config).
Fixes a crash when the user configures their device with PIN/pattern instead of a biometric (only applicable to Connect users).
We can't store a Keystore entry linked to the biometric if one isn't configured.