6357: Fix java.lang.SecurityException for multi-uploads #6402
Conversation
…to issue-6357-Fix-security-exception-on-open-document
|
Called the function for other upload methods as well. Cases to be tested:
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a SecurityException that occurs during multi-file uploads by properly managing persistable URI permissions for multiple selected files. The issue was that the app wasn't taking persistable permissions for all URIs when multiple files were selected, causing security exceptions during upload processing.
- Enhanced URI permission handling to support multiple selected files in clip data
- Added proper cleanup of persistable URI permissions after successful uploads
- Updated permission flags to only request read permissions instead of read/write
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| FilePicker.kt | Enhanced takePersistableUriPermissions to handle multiple URIs from clip data and added permission taking for gallery selections |
| UploadWorker.kt | Added cleanup logic to release persistable URI permissions after successful uploads |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
✅ Generated APK variants! |
| showSuccessNotification(contribution) | ||
| if (appContext.contentResolver.persistedUriPermissions.any { | ||
| it.uri == contribution.contentUri }) { | ||
| appContext.contentResolver.releasePersistableUriPermission( |
There was a problem hiding this comment.
Max persistence limit is 512, so clean up is necessary after a successful upload. Ref: https://android.googlesource.com/platform/frameworks/base/+/master/services/core/java/com/android/server/uri/UriGrantsManagerService.java#126
|
Thanks @misaochan for reviewing the code. @nicolas-raoul was testing these changes and would be able to confirm if they helped fix the crash he'd been experiencing.
Unfortunately, the CI has been failing since 2023 if I remember correctly 😞 The instrumented tests need a major overhaul as things have changed significantly in the last two years. We have an issue for the same to begin in an incremental way, let me look into this once the update for this month goes out. |
Indeed. The CI failing since long time has always been a bummer. Would it possibly be fine to disable the instrumentation tests alone until we get around to properly fixing it? That way I think we could at least ensure the unit tests pass successfully 🤔 |
Description (required)
Fixes #6357
What changes did you make and why?
Handled java.lang.SecurityException for a multi upload by taking persistable permission for all the URIs.
Tests performed (required)
Tested prodDebug on moto g34 5G with API level 34.
Screenshots (for UI changes only)
Need help? See https://support.google.com/android/answer/9075928
Note: Please ensure that you have read CONTRIBUTING.md if this is your first pull request.