Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: 3656 - privacy compliance for cropped new images #3673

Merged
merged 3 commits into from
Feb 5, 2023

Conversation

monsieurtanuki
Copy link
Contributor

Impacted files:

  • background_task_image.dart: now we crop here the image before sending it
  • new_crop_page.dart: now we either let the server crop (old image) or let the background task crop (brand new image)
  • rotated_crop_controller.dart: refactored adding a static method getCroppedBitmap

What

  • There were privacy issues with image cropping, as cropped images were sent as full images with cropped parameters. That meant that areas beyond the crop were still visible, which can be problematic and is not a standard in apps.
  • Above that, there are performance issues about cropping locally on "small" smartphones like @alexgarel's.
  • @M123-dev suggested that we make a clear difference between brand new pictures (just taken by the users) and "repository" pictures (taken a while ago and allegedly privacy compliant).
  • For brand new pictures, we send only the cropped area, and we compute it locally, inside the background task in order to avoid waiting too long after clicking on the "Send!" button.
  • For "old" pictures, we let the server crop the already existing image - there's no change in the code in that case.
  • An isolate could help with performances for local cropping, if stuttering UI is detected.

Fixes bug(s)

Impacted files:
* `background_task_image.dart`: now we crop here the image before sending it
* `new_crop_page.dart`: now we either let the server crop (old image) or let the background task crop (brand new image)
* `rotated_crop_controller.dart`: refactored adding a static method `getCroppedBitmap`
@codecov-commenter
Copy link

codecov-commenter commented Feb 5, 2023

Codecov Report

Merging #3673 (b311cca) into develop (bf1fe91) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           develop   #3673      +/-   ##
==========================================
- Coverage     9.45%   9.42%   -0.03%     
==========================================
  Files          269     269              
  Lines        13576   13611      +35     
==========================================
  Hits          1283    1283              
- Misses       12293   12328      +35     
Impacted Files Coverage Δ
...ooth_app/lib/background/background_task_image.dart 0.00% <0.00%> (ø)
...th_app/lib/pages/image/uploaded_image_gallery.dart 0.00% <ø> (ø)
packages/smooth_app/lib/pages/image_crop_page.dart 0.00% <ø> (ø)
...th_app/lib/pages/product/product_image_viewer.dart 0.00% <ø> (ø)
...s/smooth_app/lib/tmp_crop_image/new_crop_page.dart 0.00% <0.00%> (ø)
...pp/lib/tmp_crop_image/rotated_crop_controller.dart 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…new images

Impacted files:
* `image_crop_page.dart`: minor refactoring
* `new_crop_page.dart`: minor refactoring
* `product_image_viewer.dart`: minor refactoring
* `uploaded_image_gallery.dart`: bug fix - we should consider that it's a new image
Copy link
Member

@M123-dev M123-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks @monsieurtanuki

try {
File(_getCroppedPath()).deleteSync();
} catch (e) {
// possible, but let's not spoil the task for that either.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we still log this, in case something happens (same a few lines above)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@M123-dev For this particular case (_getCroppedPath()) that wouldn't be such a good idea as sometimes - when the crop is actually the "full image" - we don't create this path/File. We would then have to handle a special flag for that.
Regarding the other cases, I wouldn't say it would be completely pointless, but almost: if it fails the rest of the app will work OK, including the next uploads.
And there's already an open issue regarding background task init+garbage collecting, that would get rid of those files anyway.
One day we may log this, but today that looks like a "just in case" that we don't really need.
Anyway sentry is overcrowded with camera issues that make it useless.

@monsieurtanuki monsieurtanuki merged commit c9935da into openfoodfacts:develop Feb 5, 2023
@monsieurtanuki
Copy link
Contributor Author

Thank you @M123-dev for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Privacy review of server-side cropping
4 participants