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

(android) catch RecoverableSecurityException #896

Conversation

byronaltice
Copy link

@byronaltice byronaltice commented Aug 13, 2024

Platforms affected

Android

Motivation and Context

#781 (forked this PR and added additional changes)
#679
#660
#656
Closes #679
Closes #660
Closes #679

Description

This is just to resolve the issues with #781 - there were formattting issues requested to be resolved, and fork needed synced with latest. That is the only thing additional changes with this PR over #781. Original PR text included below:

Catch the RecoverableSecurityException and Requests permission to delete the media from the media store. see https://developer.android.com/training/data-storage/shared/media#update-other-apps-files

Testing

Using a

  • Ulefone Armor 8, Android 10, api 29
  • S21+ Ultra, Android 14, API 34
  • Motorola Moto G Stylus (2022), Android 12, Api 32

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@byronaltice byronaltice marked this pull request as ready for review August 13, 2024 19:37
@cfrank19
Copy link

cfrank19 commented Oct 15, 2024

I believe my team is running into this issue (extremely challenging to reproduce across different hardware) - any way we can get a release with this update to eliminate it as an option?
@erisu @breautek

@breautek breautek added this to the 8.0.0 milestone Oct 17, 2024
@breautek
Copy link
Contributor

@byronaltice

Looking for some opinions... I have drafted #907 (242cd45 for the actual changes specific to that PR) which completely removes checkForDuplicateImage and tracking the numpics. The details are in #907 but I'll summarise the rationale here.

The code obviously is attempting to determine if capturing an image is resulting in a duplicate entry in the gallery. I'm assuming this is if the user is also using saveToPhotoAlbum option on image captures, but the camera intent also stores the captured image in the gallery. It's also ancient, this was in the code base since the initial commit, so it's hard to say for certain. I can say in my testing, I'm not seeing any duplicates.

Overall the code works is it queries the media store and counts how many images are available when you start capturing a picture. And then it does it again at the end of processing the captured photo to determine if there was a duplicate.

The problem I have though is if you think about it logically, any app at any time can insert images or media into the gallery. If this occurs, it will obviously trip up these count checks. I don't see how this guarantees that it is even selecting the "duplicate" image to delete. I think it is possible that the camera could be attempting to delete a completely unrelated photo. Even if some camera apps insert the captured photo themselves, which would cause a duplicate if saveToPhotoAlbum is used, the way this handling that "duplicate" is unsafe, in my opinion.

Wondering if you have some time to analyse and see if you can come to the same conclusion?

@breautek
Copy link
Contributor

This is made obsolete by #907 which removed the code that required handling RecoverablySecurityException

@breautek breautek closed this Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants