Stopped fallback to PIN when configured for biometric #3150
Conversation
Camera permission before starting camera
📝 WalkthroughWalkthroughThis change introduces explicit runtime camera permission handling to the Sequence Diagram(s)sequenceDiagram
participant User
participant MicroImageActivity
participant Android OS
participant Camera
User->>MicroImageActivity: Launch activity
MicroImageActivity->>Android OS: Check camera permission
alt Permission granted
MicroImageActivity->>Camera: Start camera
else Permission not granted
MicroImageActivity->>Android OS: Request camera permission
Android OS-->>User: Show permission dialog
User-->>Android OS: Grant or deny permission
Android OS-->>MicroImageActivity: Return permission result
alt User granted permission
MicroImageActivity->>Camera: Start camera
else User denied permission
MicroImageActivity->>MicroImageActivity: Log error, finish activity
end
end
sequenceDiagram
participant User
participant PersonalIdBiometricConfigFragment
participant BiometricsHelper
participant BiometricPrompt
User->>PersonalIdBiometricConfigFragment: Click "Authenticate"
PersonalIdBiometricConfigFragment->>BiometricsHelper: authenticateFingerprint(..., allowOtherOptions)
BiometricsHelper->>BiometricPrompt: Show prompt (biometric only or with PIN fallback)
User-->>BiometricPrompt: Authenticate (biometric or PIN)
BiometricPrompt-->>BiometricsHelper: Return result
BiometricsHelper-->>PersonalIdBiometricConfigFragment: Callback with result
PersonalIdBiometricConfigFragment->>PersonalIdBiometricConfigFragment: Handle success or error
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (1)
35-36: New imports added but not actively used in visible code.The error code imports
ERROR_NEGATIVE_BUTTONandERROR_USER_CANCELEDare added but don't appear to be used in the current implementation. This might indicate they're intended for future use or were part of removed logic.If these imports are not needed, consider removing them to keep the code clean:
-import static androidx.biometric.BiometricPrompt.ERROR_NEGATIVE_BUTTON; -import static androidx.biometric.BiometricPrompt.ERROR_USER_CANCELED;app/src/org/commcare/fragments/MicroImageActivity.java (3)
74-83: Fix formatting and improve error handling in permission launcher.The ActivityResultLauncher implementation is functionally correct but has minor issues:
Apply this diff to fix the formatting and improve readability:
- }else{ + } else {The permission flow logic is sound - granting permission starts the camera, while denial logs an error and exits appropriately.
302-302: Fix typo in method name.There's a typo in the method name that should be corrected for consistency and readability.
Apply this diff to fix the typo:
- finishWithResul(finalImageAsBase64); + finishWithResult(finalImageAsBase64);Note: This will also require updating the method declaration at line 319.
319-319: Update method declaration to match the corrected method name.The method declaration needs to be updated to match the corrected method name.
Apply this diff to fix the method declaration:
- private void finishWithResul(String finalImageAsBase64) { + private void finishWithResult(String finalImageAsBase64) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/assets/locales/android_translatable_strings.txt(1 hunks)app/res/values/strings.xml(1 hunks)app/src/org/commcare/connect/PersonalIdManager.java(1 hunks)app/src/org/commcare/fragments/MicroImageActivity.java(5 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java(4 hunks)app/src/org/commcare/utils/BiometricsHelper.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/org/commcare/connect/PersonalIdManager.java (1)
app/src/org/commcare/utils/BiometricsHelper.java (1)
BiometricsHelper(31-257)
🔇 Additional comments (13)
app/src/org/commcare/connect/PersonalIdManager.java (1)
198-199: LGTM! Clear logic for controlling authentication fallback options.The implementation correctly determines whether to allow PIN fallback during fingerprint authentication based on PIN availability. This aligns well with the PR objective to prevent fallback to PIN when configured for biometric-only authentication.
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (3)
107-107: Improved authentication method detection.The change from tracking fingerprint attempts via
isAttemptingFingerprintto determining the method directly fromBiometricPrompt.AuthenticationResultis a cleaner approach. It eliminates state management complexity and provides more accurate authentication method reporting.
119-121: Method signature updated to accept explicit authentication method.The
reportAuthSuccessmethod now takes an explicitmethodparameter instead of relying on implicit state tracking. This makes the method more predictable and easier to test.
202-202:Details
❓ Verification inconclusive
Verify the authentication flow logic for biometric-only requirements.
The boolean expression
!BIOMETRIC_TYPE.equals(personalIdSessionDataViewModel.getPersonalIdSessionData().getRequiredLock())determines whether other options are allowed. This means:
- If required lock is BIOMETRIC_TYPE →
allowOtherOptions = false(biometric only)- If required lock is PIN →
allowOtherOptions = true(allow PIN fallback)This correctly implements the intended behavior to prevent PIN fallback when biometric authentication is specifically required.
Let me verify this logic by checking the session data constants:
🏁 Script executed:
#!/bin/bash # Description: Verify the PersonalIdSessionData constants to confirm the lock type values # Search for the lock type constants ast-grep --pattern 'class PersonalIdSessionData { $$$ public static final String BIOMETRIC_TYPE = $_; $$$ public static final String PIN = $_; $$$ }' # Also search for any other references to these constants rg -A 2 -B 2 "BIOMETRIC_TYPE|PIN.*=" --type javaLength of output: 13538
Verify lock-type constant definitions in PersonalIdSessionData
Before approving, please confirm that the string constants and null-safety align with your logic:
• In app/src/org/commcare/android/database/connect/models/PersonalIdSessionData.java, ensure
– BIOMETRIC_TYPE is defined as the exact value you expect for biometric-only flows
– PIN is defined as the exact fallback value
•getRequiredLock()never returns null (or add a null check) so that
!BIOMETRIC_TYPE.equals(getRequiredLock())behaves as intendedThe call in
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (line 202)BiometricsHelper.authenticateFingerprint( requireActivity(), biometricManager, biometricCallback, !BIOMETRIC_TYPE.equals( personalIdSessionDataViewModel .getPersonalIdSessionData() .getRequiredLock() ) );correctly toggles “allowOtherOptions” based on the required lock.
app/src/org/commcare/utils/BiometricsHelper.java (4)
80-82: Method signature updated to support authentication flow control.The addition of the
allowOtherOptionsparameter enables callers to control whether PIN fallback is available during fingerprint authentication. The parameter is correctly passed through to the underlying implementation.
126-127: PIN authentication correctly allows all options.The
authenticatePinmethod correctly passestrueforallowOtherOptions, which makes sense since PIN authentication should naturally allow device credentials.
130-130: Core method signature updated with new parameter.The
authenticatePinOrBiometricmethod now accepts theallowOtherOptionsparameter, enabling conditional prompt configuration based on authentication requirements.
136-148:Details
✅ Verification successful
Well-implemented conditional biometric prompt configuration.
The implementation correctly handles both authentication scenarios:
When
allowOtherOptions = true:
- Allows device credentials (PIN) + strong/weak biometrics
- System handles the UI for multiple options
When
allowOtherOptions = false:
- Restricts to biometric authentication only
- Provides explicit "Cancel" button for user control
This design properly implements the requirement to prevent PIN fallback when biometric-only authentication is required.
Let me verify that the string resources are properly defined:
🏁 Script executed:
#!/bin/bash # Description: Verify that the required string resources exist # Check for the title and message strings rg "connect_unlock_title|connect_unlock_message|cancel" app/res/values/strings.xml # Also check if these strings exist in any other resource files fd "strings.xml" --exec grep -l "connect_unlock_title\|connect_unlock_message\|cancel" {}Length of output: 1082
Conditional Biometric Prompt and String Resources Verified
The biometric prompt logic in app/src/org/commcare/utils/BiometricsHelper.java (lines 136–148) correctly handles both scenarios:
allowOtherOptions = true: allows device credentials + strong/weak biometricsallowOtherOptions = false: restricts to biometrics only and provides a “Cancel” buttonAll referenced string resources (
connect_unlock_title,connect_unlock_message,cancel) are defined in app/res/values/strings.xml and present in each locale folder.No further changes needed—approving these updates.
app/assets/locales/android_translatable_strings.txt (1)
1024-1025:Details
❌ Incorrect review comment
New locale key for camera permission denial
The entrymicroimage.camera.permission.deniedclearly informs users when camera permission is denied. Please verify that the code references this exact key and that the XML resource (see above) uses the matching name.To confirm, run:
🏁 Script executed:
#!/bin/bash # Verify the denial string key is referenced in code rg -n "microimage_camera_permission_denied" -A3 app/srcLength of output: 55
🏁 Script executed:
#!/bin/bash # Check that the Android string resource is defined with the correct key rg -n '<string name="microimage_camera_permission_denied"' -g 'app/src/**/*.xml' # Check that the code actually references this string resource in Java/Kotlin rg -n "microimage_camera_permission_denied" -g 'app/src/**/*.{java,kt}'Length of output: 303
🏁 Script executed:
#!/bin/bash # Search for XML resource definition across entire repo rg -n '<string name="microimage_camera_permission_denied"' --type xml # Search Java/Kotlin references to the generated resource rg -n 'R\.string\.microimage_camera_permission_denied' -g '*.{java,kt}' # Search direct dotted key usage in code rg -n 'microimage\.camera\.permission\.denied' -g '*.{java,kt}'Length of output: 388
Ignore Android XML resource suggestion
Themicroimage.camera.permission.deniedkey is defined in your runtime locale file and is correctly referenced in code—no Android XML<string>entry is required here.• Locale definition:
app/assets/locales/android_translatable_strings.txtcontainsmicroimage.camera.permission.denied=Go to settings and enable camera permission to use this feature• Code reference:
app/src/org/commcare/fragments/MicroImageActivity.java:80logErrorAndExit("Error acquiring camera permission", "microimage.camera.permission.denied", null);CommCare uses these asset-based locale files at runtime rather than Android XML resources for translatable strings. You can safely disregard the XML‐resource check.
Likely an incorrect or invalid review comment.
app/src/org/commcare/fragments/MicroImageActivity.java (4)
3-3: LGTM! Proper imports and interface implementation for permission handling.The new imports correctly include necessary classes for runtime permission handling (
Manifest,RuntimePermissionRequester,Permissions, dialog helpers, andActivityResultLauncher). The class now properly implementsRuntimePermissionRequesterinterface to handle permission requests in a standardized way.Also applies to: 27-27, 33-33, 35-36, 44-45, 60-60
106-107: Excellent security improvement - proper permission checking before camera access.Replacing the direct
startCamera()call withcheckForCameraPermission()ensures the app follows Android's runtime permission model, preventing potential crashes or permission denial issues.
109-124: Well-implemented permission checking logic with proper UX flow.The permission checking method correctly:
- Checks for missing camera permission using utility methods
- Shows rationale dialog when appropriate (follows Android UX guidelines)
- Requests permission directly when rationale isn't needed
- Falls back to starting camera when permission is already granted
The implementation follows Android best practices for runtime permissions and provides good user experience.
126-129: Correct implementation of RuntimePermissionRequester interface.The
requestNeededPermissionsmethod properly launches the camera permission request via the ActivityResultLauncher, following the modern Android permission request pattern.
| <string name="camera_permission_title">Permission for camera</string> | ||
| <string name="camera_permission_msg">In order to take a picture, CommCare needs permission to use your device camera.</string> |
There was a problem hiding this comment.
Add missing denial string resource
You’ve added a translation key microimage.camera.permission.denied in the locale bundle, but there’s no corresponding <string> in strings.xml. Please add an entry, for example:
+ <string name="microimage_camera_permission_denied">
+ Go to settings and enable camera permission to use this feature
+ </string>This ensures the denial message can be resolved at runtime.
📝 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 name="camera_permission_title">Permission for camera</string> | |
| <string name="camera_permission_msg">In order to take a picture, CommCare needs permission to use your device camera.</string> | |
| <string name="camera_permission_title">Permission for camera</string> | |
| <string name="camera_permission_msg">In order to take a picture, CommCare needs permission to use your device camera.</string> | |
| + <string name="microimage_camera_permission_denied"> | |
| + Go to settings and enable camera permission to use this feature | |
| + </string> |
🤖 Prompt for AI Agents
In app/res/values/strings.xml around lines 935 to 936, you need to add a missing
string resource for the key microimage.camera.permission.denied. Add a new
<string> entry with this name and a suitable denial message to ensure the app
can resolve this string at runtime without errors.
🛠️ Refactor suggestion
Consolidate camera permission strings
The newly added camera_permission_title and camera_permission_msg are functionally identical to the existing permission.camera.title and permission.camera.message resources elsewhere in this file. To avoid duplication and ensure consistency, please reuse the existing permission keys in MicroImageActivity (or remove the duplicates).
🤖 Prompt for AI Agents
In app/res/values/strings.xml around lines 935 to 936, the new strings
camera_permission_title and camera_permission_msg duplicate existing keys
permission.camera.title and permission.camera.message. Remove these duplicate
strings and update MicroImageActivity to reference the existing
permission.camera.title and permission.camera.message keys to maintain
consistency and avoid redundancy.
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
All looks good so far, but as discussed today we should add user storage for the required unlock level (specified by the server) and enforce it during unlocks after successful device configuration.
| new ActivityResultContracts.RequestPermission(), | ||
| isGranted -> { | ||
| if (isGranted) { | ||
| startCamera(); |
There was a problem hiding this comment.
code formatting is off here
| getString(R.string.camera_permission_msg)); | ||
| dialog.showNonPersistentDialog(this); | ||
| } else { | ||
| requestNeededPermissions(-1); |
There was a problem hiding this comment.
can we take constant value for this -1 looks odd
There was a problem hiding this comment.
-1 as not used anymore due to permission / activity launcher
| if(allowOtherOptions){ | ||
| builder.setAllowedAuthenticators(BiometricManager.Authenticators.DEVICE_CREDENTIAL | | ||
| BiometricManager.Authenticators.BIOMETRIC_STRONG | BiometricManager.Authenticators.BIOMETRIC_WEAK); | ||
| }else{ |
There was a problem hiding this comment.
code formatting is off and secondly BiometricManager.Authenticators.BIOMETRIC_WEAK is not required because it allows face unlock also and i dont think we need that BIOMETRIC_STRONG is enough
There was a problem hiding this comment.
Code format done
When Biometric from server, app can have face or fingerprint so we can leave upto Android OS to decide.
Connect Unlock always checks for required lock security
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Code looks good to me and nice job figuring out the db migrations, one suggestion for future work - it's generally better to not have composite PRs like this touching multiple things. This I think should have been 2 separate PRs atleast - one for camera permission change and another for biometric changes.
| private boolean isDemo; | ||
|
|
||
| @Persisting(value = 15) | ||
| private String required_lock = PersonalIdSessionData.PIN; |
There was a problem hiding this comment.
use camel case here - requiredLock
| builder.setAllowedAuthenticators(BiometricManager.Authenticators.DEVICE_CREDENTIAL | | ||
| BiometricManager.Authenticators.BIOMETRIC_STRONG | BiometricManager.Authenticators.BIOMETRIC_WEAK); | ||
| } else { | ||
| builder.setAllowedAuthenticators(BiometricManager.Authenticators.BIOMETRIC_STRONG | BiometricManager.Authenticators.BIOMETRIC_WEAK); |
There was a problem hiding this comment.
nit:code formattting seems off
|
@shubham1g5 Completely agree for single PR for each feature/bug but this time had created a common ticket so I was stuck. In future, separate ticket -> separate PR. Thanks for pointing out. |
Camera permission before starting camera
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-1304
Labels and Review