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

set permission GET_ACCOUNTS required only upto android 5.1 #5863

Merged

Conversation

parneet-guraya
Copy link
Contributor

App showed permission Contact which we never ask and the app do not even have the use of it. This PR aims to fix that.

Fixes #4740

What changes did you make and why?

Issue:

We had Contacts permission showing in the permission settings our app. Though we don't ask it at runtime but still it is sensitive permission and could be concerning for user.

We don't have this permission defined in the manifest but it still shows why? because android maps permissions to their permission-group. So, every permission comes under a defined permission group. Now, in the settings page of applications android shows the permission by their group's name.

We can check what permissions comes under a certain group using getPlatformPermissionsForGroup() . You can see below the permission we defined in our manifest android.permission.GET_ACCOUNTS was the reason we had this Contacts group showing in settings.

Screenshot from 2024-10-17 16-56-43

Solution:

We know the reason is this android.permission.GET_ACCOUNTS permission. Fortunately there's an easy fix. In the docs of this permission there's a note

Note: Beginning with Android 6.0 (API level 23), if an app shares the signature of the authenticator that manages an account, it does not need "GET_ACCOUNTS" permission to read information about that account. On Android 5.1 and lower, all apps need "GET_ACCOUNTS" permission to read information about any account.

Since, our authenticator implementation is only for our own defined account (wikimedia) so they share the same signature with the app itself. That means we have full control over an account (onwards Android 6) without the need for permission as long as signature is same.

Permission is only required below Android 5.1. That is why I set the permission to be used on Android 5.1 and below. Now, we won't see Contacts permission in these versions because permission are only categorized by groups from Android 6 onwards.

link

It's important to understand that revoking a single permission may not be reflected in the settings UI, which treats permissions by group.

So, we see something like this on Android 5.1 and below.

Screenshot from 2024-10-16 03-13-00

Also, it should be noted that we were not asking permission at runtime anyways. This also shows that application runs fine without this permission. On Android 5.1 and below there was no concept of runtime permission but they were granted while installation so we didn't see any crashes due to permission not being granted. We can safely remove this permission onwards Android 6

Tests performed (required)

Tested: Login, Logout works fine.

Emulator (Android 5.1 & 5) (permission required)

Emulator (Android 7.1, 13, 14) (without permission)
Oneplus 9RT 5G (Android 14)

@parneet-guraya parneet-guraya changed the title set permission required only upto android 5.1 set permission GET_ACCOUNTS required only upto android 5.1 Oct 17, 2024
Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Very elegant change and great explanation in the pull request description!

For readability by future developers, would you mind adding an XML comment after android:maxSdkVersion="22"/> saying something like <!-- Permission needed up to Android 5.1, see https://github.com/commons-app/apps-android-commons/pull/5863 -->

Thanks a lot!

@parneet-guraya
Copy link
Contributor Author

@nicolas-raoul , I added the requested change 👍 . Also, thanks, appreciate your kind words. I try to do my best work to my best abilities :)

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Sorry I did not realize that the comment would not fit on the same line.
In that case, would you mind moving the comment line to just above the <uses-permission android:name="android.permission.GET_ACCOUNTS" line?
Thanks a lot!

@parneet-guraya
Copy link
Contributor Author

Done 👍

@nicolas-raoul nicolas-raoul merged commit 817e07b into commons-app:main Oct 18, 2024
1 check passed
@parneet-guraya parneet-guraya deleted the remove-contact-permission branch October 18, 2024 01:20
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.

Commons app info lists "Contacts" permission, in "Not allowed" state but it should not be shown at all
2 participants