Repository navigation
fix: preserve GPS metadata in uploaded photos and videos - #234
Conversation
Since Android 10 the system redacts location EXIF from media read through the media store or SAF unless the app holds ACCESS_MEDIA_LOCATION and asks for the original with MediaStore.setRequireOriginal(). The app never declared the permission, so automatic uploads (and uploads of shared or picked media) lost their GPS tags while the rest of the EXIF survived. - declare ACCESS_MEDIA_LOCATION in the manifest - request it only from the picture/video automatic-uploads settings, when uploads are enabled (not on app start), with a snackbar on denial - add MediaLocationUtils to resolve a content URI to its unredacted original (media store URIs directly, SAF document URIs via MediaStore.getMediaUri) and use it where upload sources are read: UploadFileFromContentUriWorker and CopyAndUploadContentUrisTask; any failure falls back to the previous behaviour - Robolectric tests for MediaLocationUtils Fixes opencloud-eu#218 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@bluecasita We will definitely look at this, no worries... |
kulmann
left a comment
kulmann
left a comment
There was a problem hiding this comment.
Thanks, good fix overall. I'm requesting changes for three things:
- The recalculated checksum is computed over the stripped source, not the cached original, so uploads can keep failing on retry.
- The permission prompt and the "denied" snackbar come back on every visit to the settings screen.
- The fallback and the SAF URI path have no tests.
Details and some smaller suggestions are inline. integration-test is also red. Is that related?
- hash the cached copy, not the content URI, when the TUS checksum has to be recomputed. The cache now holds the unredacted original while a plain read of the content URI is redacted, so the recomputed SHA1 did not match the uploaded bytes and every retry failed - move the ACCESS_MEDIA_LOCATION request of the picture and video settings into one shared MediaLocationPermissionRequester - remember in the preferences that the permission was requested: later visits to the screen no longer prompt, and the request is only launched when Android really shows its dialog (never asked, or the user enables uploads while a rationale is still reported), so the "denied" snackbar only follows an answer the user gave - ask after the "Important" dialog is dismissed instead of on top of it - MediaLocationUtils: check the Android version once, API 29 code moved into @RequiresApi helpers - tests: SAF document URIs through MediaStore.getMediaUri, getMediaUri throwing or returning null, openInputStreamPreservingLocation falling back when the original throws or returns null, and the request decision Co-Authored-By: Claude <noreply@anthropic.com>
|
@kulmann Thanks for the careful review. All points are addressed in 9e224d8 (I also merged current main into the branch): Checksum: ensureOriginalTusChecksum() now hashes cachePath, so the recomputed SHA1 always matches the bytes that are uploaded. Permission prompt: the “requested” state is now stored in the preferences. Opening the screen only asks if the user was never asked. Enabling uploads asks again only while shouldShowRequestPermissionRationale is true. Once Android auto-denies, nothing is launched, so the snackbar only follows an answer the user actually gave. Tests: added SAF document URIs through getMediaUri, getMediaUri throwing / returning null, and openInputStreamPreservingLocation falling back when the original throws or returns null, plus a small test for the request decision. Shared helper: both fragments now use one MediaLocationPermissionRequester. Nit: one version check, API 29 code moved to private @RequiresApi(Q) helpers. Unit tests and detekt pass locally. |
kulmann
left a comment
kulmann
left a comment
There was a problem hiding this comment.
Nice, thank you for your contribution! Click-tested it in the simulator and everything seems to work. Code looks good to me as well.
github-project-automation
Bot
moved this from Needs Review
to Done
in OpenCloud Team Board
|
@bluecasita we've built a test apk, which can be installed in parallel to the official release. Are you willing to test it? https://github.com/opencloud-eu/android/releases/tag/build-tester-apk-13 |
|
Yes, I've installed it and testing now... |
|
@kulmann I've tested and it works successfully. Thanks! When will this be available in an official release? |
|
Version 1.3.1 is available for Open Testing: https://play.google.com/apps/testing/eu.opencloud.android |
Fixes #218
Problem
Since Android 10, the system redacts location metadata (the GPS EXIF block) from photos and videos an app reads through the media store or the Storage Access Framework, unless the app holds
ACCESS_MEDIA_LOCATIONand asks for the original viaMediaStore.setRequireOriginal(). The app never declared that permission, so every automatic upload (and every upload of a shared/picked media file) arrived on the server with all EXIF intact except the GPS tags. Details and reproduction in #218; Nextcloud's client had the identical bug (nextcloud/android#12188).Fix
ACCESS_MEDIA_LOCATION.MediaLocationUtilsresolves the content URI to its unredacted original when possible — a media store URI directly, a SAF document URI (what automatic uploads use) viaMediaStore.getMediaUri()— and appliessetRequireOriginal(). Used at the two places upload sources are read:UploadFileFromContentUriWorker(automatic uploads) andCopyAndUploadContentUrisTask(files shared to / picked in the app). Any failure to open the original falls back to the previous behaviour, so an upload can never fail because of this change; below Android 10 or without the permission the code path is a no-op.Testing
MediaLocationUtils(Robolectric): implicit grant below API 29, no-op without permission,requireOriginalapplied to media store URIs, non-media URIs untouched.DCIM/Camera): the permission prompt appears once in the picture-uploads settings screen; a photo taken afterwards was uploaded automatically and the server copy retains the complete GPS block (GPSLatitude,GPSLongitude,GPSAltitude,GPSDateStamp, checked with exiftool) with the rest of the EXIF unchanged — the same setup that reproduced GPS metadata stripped from uploaded photos #218 before the fix. Denying the permission keeps uploads working (location stripped, as before).Notes for review
MediaStore.getMediaUri()throws for non-media documents and for providers other than the external storage / media documents providers — that is caught and the plain URI is used.