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

fix: remove camera permission #14162

Merged
merged 1 commit into from
Aug 18, 2023
Merged

Conversation

BrayanDSO
Copy link
Member

@BrayanDSO BrayanDSO commented Jul 27, 2023

if only the image taken by the camera app is necessary, there is no need for the permission

Purpose / Description

thought about removing it after seeing the play console message about less than 1% of the similar apps having the permission

Approach

simply remove the permission. The current code didn't need it

How Has This Been Tested?

In a SDK 23 and a 33 emulators, in a Android 29 phone

  1. Press the FAB >> Add
  2. Tap the attachment clip icon > Select image > Camera
  3. Take a picture
  4. See if it is added to a card

Learning

https://stackoverflow.com/questions/32462725/capture-image-without-permission-with-android-6-0

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

if only the image taken by the camera app is necessary, there is no need for the permission
@BrayanDSO
Copy link
Member Author

BrayanDSO commented Jul 27, 2023

Tested in SDK 23, not 21, because it is going to be the new minSdkVersion in 2.17

P.S.: probably this means that the amazon flavor can be removed?

Copy link
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

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

Ok, much better to have fewer permissions requested.

However, if we move to API 23 as a minimum we might as well delete the lines below as they are related to the camera permission code:

// Until Android API22 you must manually handle permissions for image capture w/FileProvider
// This can be removed once minSDK is >= 22
// https://medium.com/@quiro91/sharing-files-through-intents-part-2-fixing-the-permissions-before-lollipop-ceb9bb0eec3a
if (CompatHelper.sdkVersion < Build.VERSION_CODES.LOLLIPOP_MR1) {
cameraIntent.clipData = ClipData.newRawUri("", imageUri)
cameraIntent.addFlags(Intent.FLAG_GRANT_WRITE_URI_PERMISSION or Intent.FLAG_GRANT_READ_URI_PERMISSION)
}

@lukstbit lukstbit added Needs Author Reply Waiting for a reply from the original author Needs Second Approval Has one approval, one more approval to merge labels Jul 30, 2023
@lukstbit
Copy link
Member

I was thinking, if we merge this we can't create releases on 2.16.x without introducing crashes for users below < 23(unless we create a separate branch and duplicate fixes).

@BrayanDSO
Copy link
Member Author

AFAIU based on what Mike said in Discord, he hasn’t created a 2.16 branch yet because he is traveling, but can do it later and cherry pick only the commits that should be in the stable branch

@BrayanDSO
Copy link
Member Author

And idk if this doesn’t work with API 21. I haven’t tested because I believed this would not be in 2.16, and 2.17 will have a minSdk upgrade (see Damien’s 2.1.66 PR).

Gonna mark this as blocked by dependency since the minSdk upgrade commit should remove this block

@BrayanDSO BrayanDSO added Blocked by dependency Currently blocked by some other dependent / related change and removed Needs Author Reply Waiting for a reply from the original author labels Jul 31, 2023
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

I tested this on API21 and it works fine, nice one! I'm okay with this for 2.16

I don't believe we can remove the amazon flavor because it was triggered simply by the declaration of the camera in uses-feature, perhaps that can be removed as well? I haven't looked back far enough to see if that is possible (can be resolved I hope by referring to the error messages in related issues/PRs for amazon to see exactly what they said - assuming I pasted the exact text in there...)

    <uses-feature android:name="android.hardware.camera" android:required="false" />
    <uses-feature android:name="android.hardware.camera.any" android:required="false" />

This appears to successfully remove the permission though, which is a nice "fix" yes, and really cool that you made the leap from seeing the Play Store message now that you have access and to the concrete possibility of this change 🧠

@mikehardy mikehardy merged commit cf5624d into ankidroid:main Aug 18, 2023
@github-actions github-actions bot added this to the 2.16 release milestone Aug 18, 2023
@github-actions github-actions bot removed Blocked by dependency Currently blocked by some other dependent / related change Needs Second Approval Has one approval, one more approval to merge labels Aug 18, 2023
@mikehardy
Copy link
Member

(side note: I'm willing to take the 2.16 risk partially because API21 usage is astonishingly low. Almost no one still uses API21 and I'll apologize to them if I blew them up...)

@github-actions
Copy link
Contributor

Hi there @BrayanDSO! This is the OpenCollective Notice for PRs merged from 2023-08-01 through 2023-08-31

If you are interested in compensation for this work, the process with details is here:

https://github.com/ankidroid/Anki-Android/wiki/OpenCollective-Payment-Process#how-to-get-paid

We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month

Please note that GSoC contributions are okay for this process. Our philosophy is that our users have donated to AnkiDroid for all contributions. The only PRs that will not go through the OpenCollective process are ones directly related to am accepted GSoC project from a selected participant, since those receive a stipend from GSoC itself.

Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding.

Thanks!

@BrayanDSO BrayanDSO deleted the rem_camera branch February 17, 2024 22:52
criticalAY added a commit to criticalAY/Anki-Android that referenced this pull request Aug 8, 2024
* We previously removed the camera permission from the manifest file (commit: fix: remove camera permission ankidroid#14162, by Brayan). However, the code now includes a check for this permission that was introduced in commit 6a8ede0. Since th permission is not declared, this check fails, and the camera cannot be launched.

* The camera feature is declared in the manifest file as <uses-feature android:name="android.hardware.camera" /> rather than using <uses-permission android:name="android.permission.CAMERA" />. This omission causes the current permission
check to always return false, preventing users from accessing the camera options. This PR corrects that issue by removing that permission check.
criticalAY added a commit to criticalAY/Anki-Android that referenced this pull request Aug 19, 2024
* We previously removed the camera permission from the manifest file (commit: fix: remove camera permission ankidroid#14162, by Brayan). However, the code now includes a check for this permission that was introduced in commit 6a8ede0. Since th permission is not declared, this check fails, and the camera cannot be launched.

* The camera feature is declared in the manifest file as <uses-feature android:name="android.hardware.camera" /> rather than using <uses-permission android:name="android.permission.CAMERA" />. This omission causes the current permission
check to always return false, preventing users from accessing the camera options. This PR corrects that issue by removing that permission check.
criticalAY added a commit to criticalAY/Anki-Android that referenced this pull request Aug 19, 2024
* We previously removed the camera permission from the manifest file (commit: fix: remove camera permission ankidroid#14162, by Brayan). However, the code now includes a check for this permission that was introduced in commit 6a8ede0. Since th permission is not declared, this check fails, and the camera cannot be launched.

* The camera feature is declared in the manifest file as <uses-feature android:name="android.hardware.camera" /> rather than using <uses-permission android:name="android.permission.CAMERA" />. This omission causes the current permission
check to always return false, preventing users from accessing the camera options. This PR corrects that issue by removing that permission check.
github-merge-queue bot pushed a commit that referenced this pull request Aug 21, 2024
* We previously removed the camera permission from the manifest file (commit: fix: remove camera permission #14162, by Brayan). However, the code now includes a check for this permission that was introduced in commit 6a8ede0. Since th permission is not declared, this check fails, and the camera cannot be launched.

* The camera feature is declared in the manifest file as <uses-feature android:name="android.hardware.camera" /> rather than using <uses-permission android:name="android.permission.CAMERA" />. This omission causes the current permission
check to always return false, preventing users from accessing the camera options. This PR corrects that issue by removing that permission check.
xenonnn4w pushed a commit to xenonnn4w/Anki-Android that referenced this pull request Aug 26, 2024
* We previously removed the camera permission from the manifest file (commit: fix: remove camera permission ankidroid#14162, by Brayan). However, the code now includes a check for this permission that was introduced in commit 6a8ede0. Since th permission is not declared, this check fails, and the camera cannot be launched.

* The camera feature is declared in the manifest file as <uses-feature android:name="android.hardware.camera" /> rather than using <uses-permission android:name="android.permission.CAMERA" />. This omission causes the current permission
check to always return false, preventing users from accessing the camera options. This PR corrects that issue by removing that permission check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants