Image EXIF/XMP metadata removal routine #2863

VitalyVPinchuk · 2019-04-05T10:45:40Z

Description (required)
Updating pull request #1712 to recent app version.

Fixes #1712 [WIP] Implements of in-app EXIF and location anonymization
In response to #2671.

What changes did you make and why?
Uses @ilgazer's code of branch master.

Method redactExifTags() is called from FileProcessor.processFileCoordinates(), which removes selected EXIF tags from file being uploaded.

Tests performed (required)
Tested betaDebug on emulator with API level 28.
Instrumental tests are broken for me now.

Screenshots showing what changed (optional - for UI changes)

…locationAccuracy)

…upport Set<String> data type used in preferences (EXIF tags)

…oordinates in FileProcessor, GPSExtractor

codecov-io · 2019-04-05T11:00:36Z

Codecov Report

Merging #2863 into master will increase coverage by <.01%.
The diff coverage is 3.84%.

@@            Coverage Diff            @@
##           master   #2863      +/-   ##
=========================================
+ Coverage    3.69%   3.69%   +<.01%     
=========================================
  Files         247     249       +2     
  Lines       12217   12268      +51     
  Branches     1083    1088       +5     
=========================================
+ Hits          451     453       +2     
- Misses      11732   11780      +48     
- Partials       34      35       +1

Impacted Files	Coverage Δ
.../main/java/fr/free/nrw/commons/settings/Prefs.java	`0% <ø> (ø)`	⬆️
...java/fr/free/nrw/commons/upload/FileProcessor.java	`3.96% <0%> (-1.38%)`	⬇️
...fr/free/nrw/commons/settings/SettingsFragment.java	`0% <0%> (ø)`	⬆️
...n/java/fr/free/nrw/commons/upload/UploadModel.java	`34.25% <0%> (ø)`	⬆️
...references/LongTitleMultiSelectListPreference.java	`0% <0%> (ø)`
.../fr/free/nrw/commons/upload/FileMetadataUtils.java	`18.18% <18.18%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d827e4...2fbecab. Read the comment docs.

neslihanturan

Seems like a clean PR

neslihanturan · 2019-04-10T10:45:09Z

I aprroved it by reading the code, however it needs some tests. Does anyone have some tests photos which has location EXIF?

ilgazer · 2019-04-10T12:59:21Z

I should have some but can't write the tests. I can post them here though.

…

On Apr 10, 2019 1:45 PM, "neslihanturan" ***@***.***> wrote: I aprroved it by reading the code, however it needs some tests. Does anyone have some tests photos which has location EXIF? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2863 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHRLoyJ-cgLD6AsChhd_vihN_oPGlmCYks5vfcC2gaJpZM4cepyC> .

misaochan · 2019-04-10T14:11:34Z

Hello @VitalyVPinchuk , thanks for taking over this PR. It seems that the location accuracy feature has been carried over - unfortunately this is very contentious, as per the comments starting #1712 (comment) . Until we can reach a consensus on how to do this without adding erroneous information to Commons, I would strongly suggest removing that part from the PR entirely, and keeping the rest of the anonymization. Then we can go ahead with merging this PR if the tests succeed.

misaochan

Pending removal of feature that submits deliberately imprecise coordinates

VitalyVPinchuk · 2019-04-10T14:35:49Z

Hello @misaochan
I've got no objections over your remark. I should've read the thread more carefully.

Should I manage lat/long tags as the rest of the other exif tags? That is, just provide a user an option to keep them or remove them?

misaochan · 2019-04-10T14:53:57Z

Should I manage lat/long tags as the rest of the other exif tags? That is, just provide a user an option to keep them or remove them?

Yes, this sounds good, thanks! :)

VitalyVPinchuk · 2019-04-12T15:51:17Z

Pending removal of feature that submits deliberately imprecise coordinates

Updated.

neslihanturan · 2019-04-17T12:15:34Z

Hi @VitalyVPinchuk I have tested for camera model and it works. @ilgazer it would be nice using your test images if they include geotag etc. @misaochan which aspects of this PR did you test?

misaochan · 2019-04-18T13:51:55Z

@neslihanturan I have not tested this, sorry. I have only looked at the code to check that the location accuracy feature was removed. :)

maskaravivek · 2019-04-18T14:44:03Z

@VitalyVPinchuk Can you rename the PR title to make it more descriptive.

maskaravivek · 2019-04-18T15:09:35Z

@VitalyVPinchuk Can you also add unit test cases for the changes made in this PR.

app/src/main/java/fr/free/nrw/commons/kvstore/BasicKvStore.java

app/src/main/java/fr/free/nrw/commons/kvstore/KeyValueStore.java

maskaravivek · 2019-04-19T04:10:00Z

app/src/main/java/fr/free/nrw/commons/upload/FileMetadataUtils.java

+
+import static androidx.exifinterface.media.ExifInterface.*;
+
+public class FileMetadataUtils {


Please add unit test cases.

app/src/main/java/fr/free/nrw/commons/upload/FileMetadataUtils.java

app/src/main/java/fr/free/nrw/commons/upload/FileProcessor.java

VitalyVPinchuk · 2019-04-19T04:24:02Z

Thanks for the thorough analysis, I'll try to fix all of your remarks.

ilgazer · 2019-04-19T05:05:28Z

THese are my test images, I hope they are useful to you as well:

There is also this image from commons for both EXIF and XMP https://commons.wikimedia.org/wiki/File:Metadata_test_file_-_includes_data_in_IIM,_XMP,_and_Exif.jpg , and these for EXIF data https://github.com/ianare/exif-samples

By the way, the XMP code vas very much a work in progress and wasn't working if I remember correctly. Has anyone tested it? Looking back, starting work on XMP support feels like scope creep. Maybe we should add in the EXIF stuff and put the XMP stuff in a seperate PR?

… JsonKVStore already has it.

…IF redaction fails.

neslihanturan

I think all reviewers should be happy by now. Thanks for the great job both @VitalyVPinchuk and @ilgazer .

neslihanturan

Oh no, I thought this PR is ready to be merged but Travis fails...

VitalyVPinchuk · 2019-04-30T09:29:26Z

Oh no, I thought this PR is ready to be merged but Travis fails...

I can't understand what causes the issue. Can it be due to adding a folder with sample image?

neslihanturan · 2019-04-30T09:38:22Z

app/src/test/kotlin/fr/free/nrw/commons/upload/FileProcessorTest.kt

+        val filePathTmp: String? = "" + System.getProperty("java.io.tmpdir") + "exif_redact_sample_tmp.jpg"
+
+        val inStream = FileInputStream(filePathRef)
+        val outStream = FileOutputStream(filePathTmp)


See the details of the test results, it says:

fr.free.nrw.commons.upload.FileProcessorTest > redactExifTags FAILED java.io.FileNotFoundException at FileProcessorTest.kt:53

I see. I removed it. Help needed. Sorry.

Sure @VitalyVPinchuk , let me investigate. Thanks for your fast responses.

Hi @neslihanturan
Any luck investigating the problem?

Hi @VitalyVPinchuk sorry for the delay. Currently I have another task, I am planning to finalize this one at May 18. Thanks for keeping the pr up to date.

Hi @VitalyVPinchuk , I am still busy with my current refactor task. Didn't forget about this one though:)

neslihanturan

Since this issue is sent before our test requirement and there is so much effort on this thread, we decided to merge this. Thanks @VitalyVPinchuk , I will create an issue to add tests for this PR.

ashishkumar468 · 2019-06-27T10:03:25Z

Hi @VitalyVPinchuk Sorry to bother you after the PR is closed, but this merge has introduced a new bug which prunes out GPS information from the Exif data.
Here is the relevant function

`public static void redactExifTags(ExifInterface exifInterface, Set<String> redactTags) {
        if(redactTags.isEmpty()) return;

         Disposable disposable = Observable.fromIterable(redactTags)
                 .flatMap(tag -> Observable.fromArray(FileMetadataUtils.getTagsFromPref(tag)))
                 .forEach(tag -> {
                     Timber.d("Checking for tag: %s", tag);
                     String oldValue = exifInterface.getAttribute(tag);
                     if (oldValue != null && !oldValue.isEmpty()) {
                         Timber.d("Exif tag %s with value %s redacted.", tag, oldValue);
                         exifInterface.setAttribute(tag, null);
                     }
                 });
         CompositeDisposable disposables = new CompositeDisposable();
         disposables.add(disposable);
         disposables.clear();

         try {
             exifInterface.saveAttributes();
        } catch (IOException e) {
            Timber.w("EXIF redaction failed: %s", e.toString());
        }
    }`

Line Number 125, FileProcessor.Java, current master. I could not exactly understand the utility of this function. Can you explain me what exactly is the purpose of this function?

VitalyVPinchuk · 2019-06-27T10:13:25Z

Hello @ashishkumar468 !
This function is intended to remove EXIF tags (that could be specified via settings activity) such as GPS Latitude/Longitude, Camera/Lens model and so on. As far as I understand this feature is implemented for privacy concerns.

ashishkumar468 · 2019-06-27T10:16:27Z

Thanks for quick response @VitalyVPinchuk . Does it mean that the attributes which are unticked in Privacy->Manage Exif Tags will not be sent/used ?

VitalyVPinchuk · 2019-06-27T10:24:07Z

Yes it does. Though it doesn't seem for me really straightforward but the community wants it this way. I figured that out from the discussion thread of the initial author of PR that I used.

ashishkumar468 · 2019-06-27T10:26:39Z

IMO, It should have been like by default all the attributes should have been checked, and pruned only when the user explicitly unchecks it. It is very unlikely that the user would go and enable these attributes from the settings on their own and as a result of which we are not using certain exif attributes which we actually need to (for example the GPS coordinated for showing categories), even though the user has not himself actually asked for it?

misaochan · 2019-06-27T12:11:17Z

I apologize for not reading the code thoroughly before this was merged. I was assuming that the user would be given the option of stripping their EXIF information (as per the wording of #70 ), not that it would be stripped by default. FTR, even the web-based Upload Wizard (the default uploader) does not silently strip EXIF, although it allows users the option of removing some of them (i.e. location) during the upload. Check https://commons.wikimedia.org/wiki/File:Sprungli_Confiserie.jpg for instance, which I just uploaded using the default uploader.

I personally feel like silently stripping EXIF by default would be a bad move for us. Users will not be aware of it and will believe that the app is malfunctioning. In order to release with this feature, I think we should at the very least make the default option work the same way as the app always has - with the tags present.

A beneficial addition would be to have a separate upload field that shows the user which EXIF tags are being taken from the image, with a link to the Settings to disable them - but this will likely be a future enhancement.

@neslihanturan @maskaravivek @nicolas-raoul @VojtechDostal thoughts are most welcome.

neslihanturan · 2019-06-27T12:18:48Z

I am okay with the either way, but in either options we should emphasize user can change settings "now" and link them to settings page. Otherwise this feature may be forgotten or unrecognized even by privacy concerned people.

misaochan · 2019-06-27T12:21:14Z

in either options we should emphasize user can change settings "now" and link them to settings page.

Can we do this in time for this release? Otherwise we may have to revert and bump this feature for the next release.

I personally think releasing without the emphasis is fine, we can always add the emphasis in the next release. It is still an improvement and would not require a revert.

neslihanturan · 2019-06-27T12:23:00Z

Sure, not necessary for this release considering we have short time.

VitalyVPinchuk · 2019-06-27T12:31:23Z

So setting all the attributes checked by default seems to a be a good decision.
If I got it right then I could fix it.

misaochan · 2019-06-27T12:32:30Z

Thank you @VitalyVPinchuk . :) That's my thoughts on it, yes, but let's give @nicolas-raoul a bit of time to weigh in since he wrote the original issue.

nicolas-raoul · 2019-06-28T02:54:44Z

Yes it should be off by default. Thanks!

maskaravivek · 2019-06-28T05:03:42Z

Yes it should be off by default. Thanks!

@nicolas-raoul Just to clarify, you mean the EXIF removal should be off by default, right?

nicolas-raoul · 2019-06-28T05:08:04Z

@maskaravivek Yes exactly.

VitalyVPinchuk added 7 commits April 4, 2019 11:17

[WIP] Added preferences for EXIF tags

63a09d6

[WIP] Added arrays, keys, strings to support EXIF preferences

32d981b

[WIP] Updated SettingsFragment to setup summary of added preferences(…

15dbe90

…locationAccuracy)

[WIP] Added methods getStringSet()in BasicKvStore, KeyValueStore to s…

08da024

…upport Set<String> data type used in preferences (EXIF tags)

[WIP] Added methods for removing EXIF tags and anonimyzing location c…

eb5ae30

…oordinates in FileProcessor, GPSExtractor

[WIP] Fixed errors in preferences EXIF tags, added XMP removal routine

9ee28b9

Merge branch 'master' into pr-1712

11aee92

neslihanturan approved these changes Apr 10, 2019

View reviewed changes

misaochan requested changes Apr 10, 2019

View reviewed changes

VitalyVPinchuk added 3 commits April 11, 2019 12:48

[WIP] Removed erroneous location accuracy handling

8cf6cfd

Merge branch 'master' into pr-1712

d654110

[WIP] Fixed mistyped GPS Tags

65afb07

misaochan approved these changes Apr 13, 2019

View reviewed changes

VitalyVPinchuk changed the title ~~Continuing PR #1712~~ Image EXIF/XMP metadata removal routine Apr 18, 2019

maskaravivek requested changes Apr 19, 2019

View reviewed changes

VitalyVPinchuk added 2 commits April 19, 2019 23:19

Reverted BasicKvStore. Removed Set<String> support in BasicKvStore as…

ff6606c

… JsonKVStore already has it.

FileProcessor: Replaced throwing runtime exception with warning if EX…

4516704

…IF redaction fails.

Merge branch 'master' into pr-1712

06f2bad

neslihanturan approved these changes Apr 30, 2019

View reviewed changes

neslihanturan requested changes Apr 30, 2019

View reviewed changes

neslihanturan requested a review from maskaravivek April 30, 2019 09:08

neslihanturan reviewed Apr 30, 2019

View reviewed changes

VitalyVPinchuk added 3 commits April 30, 2019 12:42

[WIP] FileProcessorTest: redactExifTags() doesn't work properly

6ab1bff

Merge branch 'master' into pr-1712

6d040fd

Merge branch 'master' into pr-1712

2fbecab

neslihanturan approved these changes Jun 4, 2019

View reviewed changes

neslihanturan mentioned this pull request Jun 4, 2019

Add tests for Image EXIF/XMP metadata removal routine #2988

Open

neslihanturan merged commit cc0b059 into commons-app:master Jun 4, 2019

VitalyVPinchuk deleted the pr-1712 branch June 4, 2019 14:04

VitalyVPinchuk mentioned this pull request Jun 28, 2019

EXIF removal set to off by default #3043

Closed

VitalyVPinchuk mentioned this pull request Jul 13, 2019

EXIF removal set to off by default (base 2.11-release) #3068

Merged

ilgazer mentioned this pull request Jul 29, 2019

Remove from EXIF: copyright field (name) #212

Closed


		import static androidx.exifinterface.media.ExifInterface.*;

		public class FileMetadataUtils {

Image EXIF/XMP metadata removal routine #2863

Image EXIF/XMP metadata removal routine #2863

Conversation

VitalyVPinchuk commented Apr 5, 2019 • edited Loading

codecov-io commented Apr 5, 2019 • edited Loading

Codecov Report

neslihanturan left a comment

Choose a reason for hiding this comment

neslihanturan commented Apr 10, 2019

ilgazer commented Apr 10, 2019 via email

misaochan commented Apr 10, 2019 • edited Loading

misaochan left a comment

Choose a reason for hiding this comment

VitalyVPinchuk commented Apr 10, 2019

misaochan commented Apr 10, 2019

VitalyVPinchuk commented Apr 12, 2019

neslihanturan commented Apr 17, 2019

misaochan commented Apr 18, 2019

maskaravivek commented Apr 18, 2019

maskaravivek commented Apr 18, 2019

maskaravivek Apr 19, 2019

Choose a reason for hiding this comment

VitalyVPinchuk commented Apr 19, 2019

ilgazer commented Apr 19, 2019

neslihanturan left a comment

Choose a reason for hiding this comment

neslihanturan left a comment

Choose a reason for hiding this comment

VitalyVPinchuk commented Apr 30, 2019

neslihanturan Apr 30, 2019

Choose a reason for hiding this comment

VitalyVPinchuk Apr 30, 2019

Choose a reason for hiding this comment

neslihanturan Apr 30, 2019

Choose a reason for hiding this comment

VitalyVPinchuk May 14, 2019

Choose a reason for hiding this comment

neslihanturan May 14, 2019

Choose a reason for hiding this comment

neslihanturan May 24, 2019

Choose a reason for hiding this comment

neslihanturan left a comment

Choose a reason for hiding this comment

ashishkumar468 commented Jun 27, 2019 • edited Loading

VitalyVPinchuk commented Jun 27, 2019

ashishkumar468 commented Jun 27, 2019

VitalyVPinchuk commented Jun 27, 2019

ashishkumar468 commented Jun 27, 2019 • edited Loading

misaochan commented Jun 27, 2019 • edited Loading

neslihanturan commented Jun 27, 2019

misaochan commented Jun 27, 2019

neslihanturan commented Jun 27, 2019

VitalyVPinchuk commented Jun 27, 2019

misaochan commented Jun 27, 2019

nicolas-raoul commented Jun 28, 2019

maskaravivek commented Jun 28, 2019

nicolas-raoul commented Jun 28, 2019

VitalyVPinchuk commented Apr 5, 2019 •

edited

Loading

codecov-io commented Apr 5, 2019 •

edited

Loading

misaochan commented Apr 10, 2019 •

edited

Loading

ashishkumar468 commented Jun 27, 2019 •

edited

Loading

ashishkumar468 commented Jun 27, 2019 •

edited

Loading

misaochan commented Jun 27, 2019 •

edited

Loading