facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
118.19k stars 24.21k forks source link

[Android] Removing unused permissions added at build time #5886

Closed lucidrains closed 5 years ago

lucidrains commented 8 years ago

edit: see https://medium.com/@applification/fixing-react-native-android-permissions-9e78996e9865 for a workaround -@hramos

It seems like Read/Write external storage and Read phone state permissions are automatically added to the manifest on building the android apk. Are these necessary for all React Native android apps? Is there any way to remove these permissions?

android:uses-permission#android.permission.READ_EXTERNAL_STORAGE android:uses-permission#android.permission.WRITE_EXTERNAL_STORAGE

It feels weird that I'll see these permissions being requested if I install the app from the Play store if I'm not using them.

facebook-github-bot commented 8 years ago

Hey lucidrains, thanks for reporting this issue!

React Native, as you've probably heard, is getting really popular and truth is we're getting a bit overwhelmed by the activity surrounding it. There are just too many issues for us to manage properly.

mkonicek commented 8 years ago

Just remove them from your AndroidManifest.xml if you don't need them. I believe these are for AsyncStorage.

mkonicek commented 8 years ago

There's an awesome place to post code / ask question first before filing an issue: StackOverflow. It's the best system for Q&A. Many people from the community hang out there and will be able to see and answer your question. This lets us use the github bug tracker for bugs.

I'm posting this here because github issues haven't been working very well for us and because StackOverflow is so much better. Thanks for reading! :)

satya164 commented 8 years ago

@mkonicek aren't they added at build time? In that case it's not easy to remove them

kevinejohn commented 8 years ago

+1

Even when you don't have the READ and WRITE permissions in your AndroidManifest they still show up on build. Do you just have to remove AsyncStorage completely?

vshy108 commented 8 years ago

I have similar problem when I generated release apk.

Before install, it asked for permissions:

<uses-permission android:name="android.permission.INTERNET" />

in my AndroidManifest

Bhullnatik commented 8 years ago

@mkonicek @satya164 @bestander Could this issue be re-opened please? I digged around a bit, I couldn't find where those permissions were added. So far there are :

<android:uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"/>
<android:uses-permission android:name="android.permission.READ_PHONE_STATE"/>
<android:uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE"/>

Which I'm not sure what they do, but I don't think there are actually used because it wouldn't work on Android 6.+ (run-time permissions). Also it seems strange to force permissions to use a module since in NetInfoModule on Android, React-native requires YOU to add the needed permission to your manifest (See NetInfoModule.java#L38). Anyway it's not for AsyncStorage as it uses a SQLite DB under the hood, which doesn't require any permission. Even if it used another method of storage like SharedPreferences or the internal storage to store files, it shouldn't require any permission (See Storage Options).

I'm sure this is coming from React-native, I tried on a fresh project with no other permission, and there were here once the APK generated. I also looked at every Android app in the showcase and every app has those permissions, except one which doesn't have READ_PHONE_STATE at least : Facebook Ads manager (See Permissions: View details at the bottom).

Also not completely related, but this is a permission too:

<uses-permission android:name="android.permission.SYSTEM_ALERT_WINDOW"/>

This permission (Draw over other apps) is needed in debug mode to show the error redbox, but is still present in release mode, which is quite scary for the user (even though it is not used). I could probably submit a PR for that (have 2 different manifests for debug and release, only uses the permission in the debug one).

Sorry for the long post, but my company is in the process of releasing our new Android app, and we would really like to ship it without un-needed permissions since that's not some the Android community really likes.

Thanks for your attention.

bestander commented 8 years ago

Maybe gradle does that? I can't find any string with READ_PHONE_STATE in the code. I don't mind reopening this but could you help us investigating this?

bestander commented 8 years ago

@facebook-github-bot reopen

facebook-github-bot commented 8 years ago

Okay, reopening this issue.

Bhullnatik commented 8 years ago

@bestander Yeah I think it's added automatically by Android/gradle since I couldn't find any mention of them in the code, but I'm really not sure why. I'll try to investigate quickly and get back to you/submit a PR as soon as I find something.

Bhullnatik commented 8 years ago

I forgot to check the manifest merger log... Turns out the problem is quite obvious:

IMPLIED from /Users/mayouk/Dev/tmp/AwesomeProject/android/app/src/main/AndroidManifest.xml:1:1-23:12 reason: org.webkit.android_jsc has a targetSdkVersion < 4
android:uses-permission#android.permission.READ_PHONE_STATE
IMPLIED from /Users/mayouk/Dev/tmp/AwesomeProject/android/app/src/main/AndroidManifest.xml:1:1-23:12 reason: org.webkit.android_jsc has a targetSdkVersion < 4
android:uses-permission#android.permission.READ_EXTERNAL_STORAGE
IMPLIED from /Users/mayouk/Dev/tmp/AwesomeProject/android/app/src/main/AndroidManifest.xml:1:1-23:12 reason: org.webkit.android_jsc requested WRITE_EXTERNAL_STORAGE

Because android-jsc doesn't provide a targetSdkVersion, the manifest merger added the permissions for various reasons (See Implicit permissions). Apparently @astuetz already fixed this in https://github.com/facebook/android-jsc/pull/12 but either the new version of android-jsc wasn't published or it isn't used in React-native currently. If you can contact the maintainers of android-jsc to solve this, that would be fantastic!

bestander commented 8 years ago

@brentvatne ping, is it possible to publish new android-jsc?

ghost commented 8 years ago

@Bhullnatik what we're doing until a new version of android-jsc will be released is just stripping away the unwanted permissions in our apps' manifest like this:

<uses-permission
  android:name="android.permission.READ_PHONE_STATE"
  tools:node="remove"/>
Bhullnatik commented 8 years ago

@astuetz I was looking at the manifest merger on how to fix this until it gets solved, thanks for the heads-up πŸ˜„

brentvatne commented 8 years ago

@bestander - sure I can do that, we might also want to update to r189384 as well @kmagiera?

mouneyrac commented 8 years ago

@astuetz thanks, it helped me, justto add to this info I had to declare the tools attribut in

 <manifest xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:tools="http://schemas.android.com/tools"
    package="com.bepaw.esportninja">. 

By default there is only xmlns:android, no xmlns:tools

Bhullnatik commented 8 years ago

@bestander @brentvatne Hey! Any update on this?

danleveille commented 8 years ago

πŸ‘ +1 for this.

marcshilling commented 8 years ago

I feel like the removal of SYSTEM_ALERT_WINDOW from release builds is far more critical than the other permissions being discussed.

Bhullnatik commented 8 years ago

@marcshilling It's all equally important to remove them in my opinion, but this one is a little more tricky. The easiest way to remove it is to remove it in the release AndroidManifest.xml, but the configuration is a little bit heavy to include in every React Native project. The other way would be to remove the usage, thus not needing the permission anymore. For that I'm not sure where/for what it is used, but I didn't see any particular need that couldn't be fulfilled with something else. I could be wrong though.

marcshilling commented 8 years ago

For those who want to exclude SYSTEM_ALERT_WINDOW from release builds without separate Manifest files:

In your manifest:

<uses-permission android:name="android.permission.SYSTEM_ALERT_WINDOW" tools:remove="${excludeSystemAlertWindowPermission}"/>

In app/build.gradle:

    buildTypes {
        debug {
            manifestPlaceholders = [excludeSystemAlertWindowPermission: "false"]
        }
        release {
            manifestPlaceholders = [excludeSystemAlertWindowPermission: "true"]
        }
    }

Seems to work well.

danleveille commented 8 years ago

@marcshilling Perfect! Thank you! It's been kind of embarrassing to have to explain that to users because I couldn't figure out how to remove it. πŸ˜•

jbrodriguez commented 8 years ago

Hi, I've been alerted about READ_PHONE_STATE by a user of one of my apps.

I'm getting the same reason in the build log

reason: org.webkit.android_jsc has a targetSdkVersion < 4

Hopefully it gets solved in a future release.

For the time being, I'm using the permission removal workarounds mentioned here astuetz - READ_PHONE_STATE - https://github.com/facebook/react-native/issues/5886#issuecomment-200869879 marcshilling - SYSTEM_ALERT_WINDOW - https://github.com/facebook/react-native/issues/5886#issuecomment-224397883

As well as adding the namespace as explained by mouneyrac (https://github.com/facebook/react-native/issues/5886#issuecomment-209218936)

jbrodriguez commented 8 years ago

So, I wasn't being able to remove SYSTEM_ALERT_WINDOW with the solution above.

Had to resort to a hack (after much prodding and poking).

in AndroidManifest.xml

    <uses-permission android:name="${permissionName}" tools:node="remove"/>

in app/build.gradle

    buildTypes {
        debug {
            manifestPlaceholders = [permissionName: "android.permission.ACCESS_FINE_LOCATION"]
            ...
        }    
        release {
            manifestPlaceholders = [permissionName: "android.permission.SYSTEM_ALERT_WINDOW"]
            ...
        }
    }

In debug mode, I remove ACCESS_FINE_LOCATION, since I don't use (neither in dev nor in prod). Any other non-essential permission could be used (BIND_TV_INPUT?).

In release mode, I remove SYSTEM_ALERT_WINDOW. If you don't forcibly remove it, it will get merged in.

Hacky, but effective πŸ˜‚

Jeiwan commented 8 years ago

If you use this solution https://github.com/facebook/react-native/issues/5886#issuecomment-224397883, don't forget to add xmlns:tools as described here: https://github.com/facebook/react-native/issues/5886#issuecomment-209218936

satya164 commented 8 years ago

@marcshilling I think this is something we can have in the generator template by default.

ghost commented 8 years ago

@marcshilling Your solution didn't work for me as well but the one from @jbrodriguez did.

One thing I noticed was that instead of using a real permission which will be removed, you can also use a non-existing permission name like REMOVE_ME.

tgaff commented 8 years ago

Quick comment: after updating from 0.30 to 0.32 it seems react-native now suddenly adds the permission com.google.android.c2dm.permission.RECEIVE I've seen this in two apps now. I doubt it needs this new permission either.

mikelambert commented 7 years ago

The use of @marcshilling 's tools:remove doesn't work for me either.

My solution to avoid unnecessary permissions (a mix of the above) is: In the app/src/main/AndroidManifest.xml, I added:

<uses-permission tools:node="remove" android:name="android.permission.READ_PHONE_STATE" />

And in a newly-created app/src/release/AndroidManifest.xml, I put:

<manifest
    xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:tools="http://schemas.android.com/tools"
    >

    <!-- These are added by React Native for debug mode, but actually aren't needed in releasemode -->
    <uses-permission tools:node="remove" android:name="android.permission.SYSTEM_ALERT_WINDOW" />
</manifest>

Variable substitution inside a tools:node doesn't work for me, so something like tools:node="${someManifestPlaceholder}" wouldn't work. So the above approach of multiple AndroidManifest.xml files that merge together seems like a better method of adding/removing certain permissions on a debug vs release basis (or the default main).

goldylucks commented 7 years ago

@marcshilling your solution didn't work for me, so I've used @jbrodriguez's combined with @astuetz's

u guys think it's worth creating a script to automate this?

maybe create a npm package that makes this on install and then remove itself? Never done something like that tho I believe it is possible.

@mikelambert, regarding your approach, is anything else needed to make it work?

does gradle automagically merges app/src/release/AndroidManifest.xml into app/src/main/AndroidManifest.xml?

that seems like the cleaner approach to me

Thoughts and comments appreciated! :)

mikelambert commented 7 years ago

Yes, it automatically merges the AndroidManifest.xml files. I believe it's all that was necessary to make it work for me, but let me know if I've missed anything!

freakinruben commented 7 years ago

@mikelambert also worked for me. I've got multiple release buildTypes though, so I needed release{X}/AndroidManifest.xml for each build-type in order to get it working.

Not sure how you could capture something like that in a npm package..

mikelambert commented 7 years ago

The proper fix (IMO) would be to set this up as part of the project when react-native init is run. ie, something similar to how this gets used in the initial project setup: https://github.com/facebook/react-native/blob/master/local-cli/templates/HelloWorld/android/app/src/main/AndroidManifest.xml

goldylucks commented 7 years ago

@freakinruben everything u do manually can b done in a script ;) the first version of the script would be basic, and just support release/debug versions

later we can prompt user for more configurations

goldylucks commented 7 years ago

@mikelambert for sure man, we r all hacking this thing here until a proper solution would b merged by the react-native team :)

Jaafar-abusair commented 7 years ago
<uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" tools:node="remove" />
<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" tools:node="remove" />
<uses-permission android:name="android.permission.READ_PHONE_STATE" tools:node="remove"/>
ferrannp commented 7 years ago

Hi all,

Be careful with letting RN add this permission for you android.permission.READ_PHONE_STATE. If you try to publish an app to the Google Play store with such permission (among others), it will warn you with:

Your app has an apk with version code X that requests the following permission(s): android.permission.READ_PHONE_STATE. Apps using these permissions in an APK are required to have a privacy policy set.

More info: https://play.google.com/intl/en-US_ALL/about/privacy-security/additional-requirements/. So yeah, be careful and ensure to delete this permission if you don't need it till this is fixed in RN. I feel this should be documented somewhere for now just so people with releasing intentions are aware.

ujwal-setlur commented 7 years ago

@mikelambert solution worked for me. Thank you.

alessiodionisi commented 7 years ago

@mikelambert Thank you for the best solution.

kennethpdev commented 7 years ago

@ferrannp it was actually the reason why I got here. :) and the solutions worked fine.

mmazzarolo commented 7 years ago

Small recap since March 15 is approaching:

schermata 2017-03-13 alle 17 29 25
antoinerousseau commented 7 years ago

@mmazzarolo what happens on March 15 (tomorrow)?

mmazzarolo commented 7 years ago

@antoinerousseau: Google Play’s New Privacy Policy Rule Goes into Effect on March 15th, and READ_PHONE_STATE is one of the permissions that will force you to publish a Privacy Policies document in both the Play Store and app itself.

iegik commented 7 years ago

build.gradle

        classpath 'com.android.tools.build:gradle:1.2.3' // For Android 5.1

Following rule craches on 5.1:

    <uses-permission android:name="android.permission.SYSTEM_ALERT_WINDOW" tools:node="remove"/>
06-02 15:06:54.102: E/AndroidRuntime(18353): android.view.WindowManager$BadTokenException: Unable to add window android.view.ViewRootImpl$W@f03a0a2 -- permission denied for this window type
06-02 15:06:54.102: E/AndroidRuntime(18353):    at android.view.ViewRootImpl.setView(ViewRootImpl.java:704)
06-02 15:06:54.102: E/AndroidRuntime(18353):    at android.view.WindowManagerGlobal.addView(WindowManagerGlobal.java:289)
06-02 15:06:54.102: E/AndroidRuntime(18353):    at android.view.WindowManagerImpl.addView(WindowManagerImpl.java:85)
06-02 15:06:54.102: E/AndroidRuntime(18353):    at android.app.Dialog.show(Dialog.java:311)
06-02 15:06:54.102: E/AndroidRuntime(18353):    at com.facebook.react.devsupport.DevSupportManagerImpl.showProgressDialog(DevSupportManagerImpl.java:762)
06-02 15:06:54.102: E/AndroidRuntime(18353):    at com.facebook.react.devsupport.DevSupportManagerImpl.reloadJSFromServer(DevSupportManagerImpl.java:767)
06-02 15:06:54.102: E/AndroidRuntime(18353):    at com.facebook.react.devsupport.DevSupportManagerImpl.handleReloadJS(DevSupportManagerImpl.java:658)
06-02 15:06:54.102: E/AndroidRuntime(18353):    at com.facebook.react.XReactInstanceManagerImpl$3$1.run(XReactInstanceManagerImpl.java:412)
06-02 15:06:54.102: E/AndroidRuntime(18353):    at android.os.Handler.handleCallback(Handler.java:815)
06-02 15:06:54.102: E/AndroidRuntime(18353):    at android.os.Handler.dispatchMessage(Handler.java:104)
06-02 15:06:54.102: E/AndroidRuntime(18353):    at android.os.Looper.loop(Looper.java:194)
06-02 15:06:54.102: E/AndroidRuntime(18353):    at android.app.ActivityThread.main(ActivityThread.java:5631)
06-02 15:06:54.102: E/AndroidRuntime(18353):    at java.lang.reflect.Method.invoke(Native Method)
06-02 15:06:54.102: E/AndroidRuntime(18353):    at java.lang.reflect.Method.invoke(Method.java:372)
06-02 15:06:54.102: E/AndroidRuntime(18353):    at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:959)
06-02 15:06:54.102: E/AndroidRuntime(18353):    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:754)

Solution is to remove:

tools:node="remove
mikelambert commented 7 years ago

@iegik , I'm not sure whose solution/code you were trying, but adding a SYSTEM_ALERT_WINDOW permission to release builds is not a solution I would recommend to others.

Please try my suggestion above, if you don't want to require that permission in your release builds.

joshfriend commented 7 years ago

@a-koka it's already been explained above several times

iegik commented 7 years ago

@mikelambert I have to request SYSTEM_ALERT_WINDOW permission from users, because our app crashes somewhere on getting permissions :(

mikelambert commented 7 years ago

@iegik From what I understand, normal release-mode apps shouldn't need SYSTEM_ALERT_WINDOW, and it's only useful for development-mode apps. Do you have a log of said crash, that show where this "somewhere" is?

Of course, you're welcome to do that if it suits your needs and isn't worth debugging for you... I just didn't want future readers of this thread to think they need to request that permission too, since this thread is about how to not request unnecessary permissions. :)

npaez commented 7 years ago

Adding this line, makes my app crash. <uses-permission android:name="android.permission.READ_PHONE_STATE" tools:node="remove" />