Android 14 update #2894
Android 14 update #2894
Conversation
This reverts commit 000fef6.
| <receiver | ||
| android:exported="false" | ||
| android:name="org.commcare.views.notifications.NotificationClearReceiver"> |
There was a problem hiding this comment.
we should either add a qa note to test clearning of notifications or test it out ourselves and mention it in safety story of the PR
There was a problem hiding this comment.
I tested this and it worked fine, I will add a note to the PR.
| @@ -1,2 +1,2 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <manifest package="org.commcare.commcaresupportlibrary" /> | |||
There was a problem hiding this comment.
Think we should be bumping the Android sdk version in support lib build.gradle as well to go along with this change.
avazirna
added
QA Note
High Risk
|
Added a QA note. |
|
@coderabbitai full review |
📝 Walkthrough📝 WalkthroughWalkthroughThe recent changes encompass modifications across multiple files in the Android application, focusing on enhancing security, updating SDK versions, and refining task management. The Key changes include the introduction of conditional checks based on Android API levels for registering broadcast receivers, ensuring compliance with newer security models. This is evident in classes such as Sequence Diagram(s)sequenceDiagram
participant User
participant CommCareActivity
participant BroadcastReceiver
User->>CommCareActivity: onResume()
CommCareActivity->>CommCareActivity: Check Android Version
alt Android Version >= Tiramisu
CommCareActivity->>BroadcastReceiver: Register with RECEIVER_NOT_EXPORTED
else Android Version < Tiramisu
CommCareActivity->>BroadcastReceiver: Register normally
end
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: 2
🧹 Outside diff range and nitpick comments (2)
commcare-support-library/build.gradle (1)
Update androidx dependencies to latest stable versions for Android 14 compatibility
The androidx dependencies are significantly outdated and should be updated to the latest stable versions:
- appcompat 1.0.0 → 1.7.0
- constraintlayout 1.1.3 → 2.2.0
Apply these changes:
- implementation 'androidx.appcompat:appcompat:1.0.0' - implementation 'androidx.constraintlayout:constraintlayout:1.1.3' + implementation 'androidx.appcompat:appcompat:1.7.0' + implementation 'androidx.constraintlayout:constraintlayout:2.2.0'🔗 Analysis chain
Line range hint
63-65: Update outdated androidx dependencies.The androidx dependencies are significantly outdated and should be updated for Android 14 compatibility:
- appcompat 1.0.0 → 1.6.1
- constraintlayout 1.1.3 → 2.1.4
Apply these changes:
- implementation 'androidx.appcompat:appcompat:1.0.0' - implementation 'androidx.constraintlayout:constraintlayout:1.1.3' + implementation 'androidx.appcompat:appcompat:1.6.1' + implementation 'androidx.constraintlayout:constraintlayout:2.1.4'Let's check for any other outdated dependencies:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for latest versions of androidx dependencies # Check latest versions from Google's Maven repository curl -s https://dl.google.com/dl/android/maven2/androidx/appcompat/appcompat/maven-metadata.xml | grep '<version>' | tail -1 curl -s https://dl.google.com/dl/android/maven2/androidx/constraintlayout/constraintlayout/maven-metadata.xml | grep '<version>' | tail -1Length of output: 329
app/src/org/commcare/services/CommCareSessionService.java (1)
709-713: LGTM: Improved PendingIntent flag handlingGood refactoring to consolidate the flag logic and ensure security with FLAG_IMMUTABLE.
Consider extracting this common PendingIntent flag logic into a utility method since it's likely used in other places:
- int pendingIntentFlags = PendingIntent.FLAG_UPDATE_CURRENT; - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { - pendingIntentFlags = pendingIntentFlags | PendingIntent.FLAG_IMMUTABLE; - } - PendingIntent contentIntent = PendingIntent.getActivity(this, 0, callable, pendingIntentFlags); + PendingIntent contentIntent = PendingIntent.getActivity(this, 0, callable, + CommCareUtils.getPendingIntentFlags(PendingIntent.FLAG_UPDATE_CURRENT));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
app/AndroidManifest.xml(5 hunks)app/build.gradle(7 hunks)app/src/org/commcare/activities/CommCareActivity.java(2 hunks)app/src/org/commcare/activities/FormEntryActivity.java(2 hunks)app/src/org/commcare/services/CommCareFirebaseMessagingService.java(1 hunks)app/src/org/commcare/services/CommCareSessionService.java(3 hunks)app/src/org/commcare/utils/SessionRegistrationHelper.java(2 hunks)build.gradle(1 hunks)commcare-support-library/build.gradle(1 hunks)commcare-support-library/src/main/AndroidManifest.xml(1 hunks)
🔇 Additional comments (16)
app/src/org/commcare/activities/CommCareActivity.java (2)
306-313: LGTM! Secure broadcast receiver registration implemented correctly.
The conditional registration of the data sync receiver with RECEIVER_NOT_EXPORTED flag for Android Oreo and above follows security best practices.
Line range hint 1004-1008: LGTM! Secure broadcast receiver registration implemented correctly.
The conditional registration of the pending sync alert receiver with RECEIVER_NOT_EXPORTED flag for Android Oreo and above follows security best practices.
app/src/org/commcare/activities/FormEntryActivity.java (1)
300-304: LGTM! Secure broadcast receiver registration implemented correctly.
The conditional registration of the location service issue receiver with RECEIVER_NOT_EXPORTED flag for Android Oreo and above follows security best practices.
commcare-support-library/src/main/AndroidManifest.xml (1)
2-2: LGTM! Package attribute removal aligns with AGP 8.x best practices.
The removal of the package attribute from manifest is correct as it's now properly declared via the namespace in build.gradle.
Let's verify the namespace declaration:
✅ Verification successful
✓ Package namespace correctly declared in build.gradle
The verification confirms that the namespace "org.commcare.commcaresupportlibrary" is properly declared in build.gradle, matching the package that was removed from AndroidManifest.xml. This change follows the recommended AGP 8.x practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify namespace declaration in build.gradle matches the removed package
# Check if namespace is properly declared in build.gradle
rg "namespace.*org.commcare.commcaresupportlibrary" commcare-support-library/build.gradle
Length of output: 143
build.gradle (1)
9-9: Verify AGP 8.1.1 compatibility with other project dependencies.
The upgrade to AGP 8.1.1 is necessary for Android 14 support. However, we should ensure compatibility with other key dependencies.
Let's check for potential compatibility issues:
✅ Verification successful
The previous results showed Kotlin 1.8.20 is used, and AGP 8.1.1 is consistently referenced. Let's verify the Gradle and Java version requirements to ensure full compatibility matrix.
AGP 8.1.1 upgrade is compatible with project dependencies
The compatibility matrix has been verified:
- Gradle wrapper version 8.1.1 matches AGP version 8.1.1
- Kotlin version 1.8.20 is compatible with AGP 8.1.1
- Java 17 configuration is supported by AGP 8.1.1
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check compatibility between AGP and other dependencies
# Check Kotlin compatibility requirements
rg "kotlin_version.*=.*'.*'" build.gradle
# Check if any other build.gradle files specify different AGP versions
rg "com.android.tools.build:gradle:" -g "*.gradle"
Length of output: 204
Script:
#!/bin/bash
# Check Gradle version requirements
rg "distributionUrl.*gradle-.*-bin.zip" -g "gradle/wrapper/gradle-wrapper.properties"
# Check for any explicit Gradle version constraints
rg "gradle.*version" -g "*.gradle*" -i
# Check for any explicit JDK/Java version requirements
rg "sourceCompatibility|targetCompatibility|JavaVersion|jvmTarget" -g "*.gradle*"
Length of output: 629
commcare-support-library/build.gradle (1)
46-46: LGTM! Correct targetSdkVersion for Android 14.
Setting targetSdkVersion to 34 is appropriate for Android 14 compatibility.
app/AndroidManifest.xml (4)
40-40: LGTM: Required permission for Android 14 foreground services.
The addition of android.permission.FOREGROUND_SERVICE_SPECIAL_USE permission is necessary for Android 14 compatibility, as it's required for services that declare foregroundServiceType="specialUse".
309-313: LGTM: Proper foreground service declaration for Android 14.
The changes to CommCareSessionService correctly implement Android 14's requirements for foreground services:
- Declares
foregroundServiceType="specialUse" - Provides a clear description of the special use case via the
PROPERTY_SPECIAL_USE_FGS_SUBTYPEproperty
377-379: LGTM: Security improvement for notification receiver.
Setting android:exported="false" for NotificationClearReceiver improves security by explicitly preventing external access.
567-568: LGTM: Proper handling of problematic receiver.
Marking KujakuNetworkChangeReceiver as not exported and removing it via tools:node="remove" addresses the IllegalStateException issue with map downloader service.
app/build.gradle (3)
225-225: LGTM: Appropriate SDK version updates for Android 14.
The update of compileSdk and targetSdkVersion to 34 is necessary for Android 14 compatibility.
Also applies to: 252-252
127-127: LGTM: Updated Play Core dependency.
Replacing play-core with app-update:2.1.0 ensures compatibility with newer Android versions.
187-187: LGTM: Modernized task declarations.
The transition from task to tasks.register and tasks.whenTaskAdded to tasks.configureEach follows modern Gradle best practices.
Also applies to: 495-495, 525-525, 555-555
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
95-99: LGTM: Secure PendingIntent flag handling.
The implementation correctly:
- Sets base flags with
FLAG_UPDATE_CURRENT - Adds
FLAG_IMMUTABLEfor Android 6.0 (API 23) and above - Maintains backward compatibility
app/src/org/commcare/services/CommCareSessionService.java (2)
12-12: LGTM: Required import for Android 14 foreground service type
The addition of ServiceInfo import is necessary for specifying the foreground service type in Android 14.
215-219: Verify Android 14 foreground service declaration in manifest
The foreground service type is correctly set for Android 14, but this requires corresponding changes in the manifest.
✅ Verification successful
Android 14 foreground service declaration is properly configured
The manifest correctly declares both the required permission and service configuration for Android 14:
- Permission:
<uses-permission android:name="android.permission.FOREGROUND_SERVICE_SPECIAL_USE" /> - Service declaration with type and subtype:
<service
android:enabled="true"
android:foregroundServiceType="specialUse"
android:name="org.commcare.services.CommCareSessionService">
<property
android:name="android.app.PROPERTY_SPECIAL_USE_FGS_SUBTYPE"
android:value="Service that maintains an encryption key in memory to securely submit data to the server" />
</service>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the service is properly declared in AndroidManifest.xml
rg -A 2 "CommCareSessionService" --type xml | grep -i "foregroundServiceType"
Length of output: 75
Script:
#!/bin/bash
# Let's first find the AndroidManifest.xml file and check its content for the service declaration
fd "AndroidManifest.xml" --type f --exec cat {}
Length of output: 28271
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
| activity.registerReceiver(userSessionExpiredReceiver, expirationFilter, Context.RECEIVER_NOT_EXPORTED); | ||
| } else { | ||
| activity.registerReceiver(userSessionExpiredReceiver, expirationFilter); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Broadcast receiver security improvements needed in several files
The verification revealed additional broadcast receiver registrations that need security improvements:
CommCareProviderLocationController.kt: Missing SDK version check and RECEIVER_NOT_EXPORTED flag for provider location changesCommCareWiFiDirectActivity.java: Missing SDK version check and RECEIVER_NOT_EXPORTED flag for WiFi Direct receiver- Test files can be excluded as they use LocalBroadcastManager which is already secure
🔗 Analysis chain
LGTM! Secure broadcast receiver registration implemented correctly.
The conditional registration of the session expiration receiver with RECEIVER_NOT_EXPORTED flag for Android Oreo and above follows security best practices.
Let's verify that all broadcast receiver registrations in the codebase follow this pattern:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potentially insecure broadcast receiver registrations
# Look for registerReceiver calls that might need the RECEIVER_NOT_EXPORTED flag
# Search for registerReceiver calls without RECEIVER_NOT_EXPORTED
rg "registerReceiver\([^,]+,[^,]+" -A 2
Length of output: 2983
There was a problem hiding this comment.
@avazirna can you verify this please.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I pushed a fix in commit 38a4dc9, please review it.
|
@coderabbitai review |
|
I pushed a fix in commit 38a4dc9, please review it. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/src/org/commcare/location/CommCareProviderLocationController.kt (1)
Line range hint
1-108: Consider Android 14 location improvementsAndroid 14 introduces several location-related improvements that could enhance this implementation:
- Foreground Service Types for location: Consider declaring
foregroundServiceType="location"in the manifest for better transparency- Predictive Back Gestures: Ensure the location UI handles the new back navigation
- Grammatical Inflection API: Consider using it for location permission dialogs
These improvements would make the app more compliant with Android 14's features and user experience guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
app/src/org/commcare/activities/CommCareActivity.java(2 hunks)app/src/org/commcare/activities/CommCareWiFiDirectActivity.java(1 hunks)app/src/org/commcare/activities/FormEntryActivity.java(2 hunks)app/src/org/commcare/location/CommCareProviderLocationController.kt(2 hunks)app/src/org/commcare/utils/SessionRegistrationHelper.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/org/commcare/activities/CommCareActivity.java
👮 Files not reviewed due to content moderation or server errors (2)
- app/src/org/commcare/activities/CommCareWiFiDirectActivity.java
- app/src/org/commcare/activities/FormEntryActivity.java
🔇 Additional comments (5)
app/src/org/commcare/utils/SessionRegistrationHelper.java (3)
8-9: LGTM! Import added for Android version check.
The addition of the Build import is necessary for the version check implementation.
57-61: LGTM! Secure broadcast receiver registration implemented correctly.
The conditional registration with RECEIVER_NOT_EXPORTED flag for Android 13 (Tiramisu) and above follows security best practices.
57-58: Consider using UPSIDE_DOWN_CAKE for Android 14 compatibility.
Since this PR is specifically for Android 14 support, consider if there are any Android 14-specific considerations that need to be handled differently. While the current implementation using TIRAMISU (Android 13) is secure, you might want to:
- Document why TIRAMISU is sufficient for Android 14
- Or use Build.VERSION_CODES.UPSIDE_DOWN_CAKE if there are Android 14-specific requirements
Let's check if there are any Android 14-specific broadcast receiver changes:
✅ Verification successful
Using TIRAMISU is sufficient for Android 14 broadcast receiver security
The current implementation using TIRAMISU (Android 13) for broadcast receiver security is correct and sufficient for Android 14. The RECEIVER_NOT_EXPORTED flag was introduced in Android 13 and continues to be the recommended approach for securing broadcast receivers in Android 14. Looking at the codebase:
- The implementation consistently uses
RECEIVER_NOT_EXPORTEDfor security-sensitive receivers - The only usage of
UPSIDE_DOWN_CAKEin the codebase is for foreground service changes, not related to broadcast receivers - There are no Android 14-specific changes required for broadcast receiver registration security
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for Android 14 specific broadcast receiver changes in the codebase
# Look for any Android 14 (UPSIDE_DOWN_CAKE) specific handling
# Search for UPSIDE_DOWN_CAKE version checks
rg "VERSION_CODES\.UPSIDE_DOWN_CAKE|VERSION\.SDK_INT.*34" -A 2
# Search for Android 14 related comments
rg "Android 14|Android\s+14|API\s+34" -A 2
Length of output: 506
Script:
#!/bin/bash
# Let's check for broadcast receiver registrations and their security considerations
# This will help understand the context better
# Search for broadcast receiver registrations
rg "registerReceiver.*Context\.RECEIVER" -A 2
# Also check for any security-related comments or documentation around broadcast receivers
rg "broadcast.*security|receiver.*security" -i -A 2
# Look for any documentation about Android version compatibility
fd "README|CHANGELOG" --exec cat {} \;
Length of output: 17287
app/src/org/commcare/location/CommCareProviderLocationController.kt (2)
10-10: LGTM: Required imports for Android API level checks
The added imports support the conditional broadcast receiver registration based on API level.
Also applies to: 12-12
52-57:
Security concern: Verify if RECEIVER_EXPORTED is necessary
The receiver is being registered with RECEIVER_EXPORTED flag on Android 13+, which makes it accessible to external applications. This could pose a security risk if not intentional.
Consider using RECEIVER_NOT_EXPORTED instead, as seen in other files mentioned in the AI summary (CommCareActivity.java, FormEntryActivity.java), unless there's a specific requirement for external apps to trigger location updates.
Let's verify the receiver usage pattern across the codebase:
Update test libs and work manager
|
@damagatchi retest this please |
avazirna
force-pushed
the
android-14-update
branch
2 times, most recently
from
9fb7a39 to
afa3da3
Compare
|
@damagatchi retest this please |
avazirna
force-pushed
the
android-14-update
branch
4 times, most recently
from
d74937c to
5f8099f
Compare
Espress 3.5.1 and higher is causing our instrumentation tests to not run in BrowserStack.
avazirna
force-pushed
the
android-14-update
branch
from
5f8099f to
86ef226
Compare
|
@shubham1g5 I had to revert most of the test lib updates as they were causing instrumentation tests to not run in BrowserStack and I have created an issue to investigate this. |
Summary
This PR implements the changes required to support Android 14.
Feature Flag
Product Description
Safety Assurance
Automated test coverage
Safety story
QA Note: Do all release testing on Android 14 device unless explicitly specified by test case.