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

Remove not needed permissions from cordova config / Split plugin #181

Closed
pke opened this issue Feb 21, 2017 · 18 comments
Closed

Remove not needed permissions from cordova config / Split plugin #181

pke opened this issue Feb 21, 2017 · 18 comments
Labels
feature request New feature or request

Comments

@pke
Copy link

pke commented Feb 21, 2017

This plugin drags in a lot of permissions that our app does not need. We just use this plugin for checking and requesting camera permissions.
However, location (back & foreground), calendar, microphone permissions are all in the apps manifest without justification.
Is there a way to remove those permissions from the resulting config.xml and the links to the frameworks to bundle the app with:

<framework src="CoreBluetooth.framework" />
<framework src="CoreMotion.framework" />
<framework src="AVFoundation.framework" />
<framework src="Photos.framework" />
<framework src="AddressBook.framework" />
<framework src="EventKit.framework" />
<framework src="Contacts.framework" />

I think a better approach to a finer grained permission and code usage would be to split this plugin into separate plugins for each domain: camera, photolib, addressbook, contacts, bluetooth.
cordova-plugin-diagnostic-camera etc

@pke pke changed the title Remove not needed permissions from cordova config Remove not needed permissions from cordova config / Split plugin Feb 21, 2017
@dpa99c
Copy link
Owner

dpa99c commented Feb 21, 2017

The options are pretty limited since Cordova provides no means of adding conditional logic to the plugin.xml: it would be great if the variables entered via the CLI could be used in XSSI-style conditionals such as:

cordova plugin add cordova.plugins.diagnostic --variable INCLUDE_BLUETOOTH=1

plugin.xml: <!--#if INCLUDE_BLUETOOTH --><framework src="CoreBluetooth.framework" /><!--#endif-->

But they can't :-(

So currently, the options are:

  • fragment the plugin into multiple plugins based on functional areas (camera, bluetooth, location, etc.). I'm really not keen to do this since it means currently shared common functionality will need to be duplicated into each micro-plugin, in addition to a greater maintenance overhead.

  • fork the plugin and comment out the bits you don't want/use

Neither is ideal, but given the current Cordova CLI tooling, I don't have a better solution.

@pke
Copy link
Author

pke commented Feb 21, 2017

Thanks for the swift response. I really like the micro-plugins in the eslint and webpack community. And since apps should only really ask for a minimal set of permissions in respect to the users privacy I'll fork and split the plugin and see how much functionality can be shared.

It could actually work like that the main plugin still provides the shared functionality and the functional plugins just hook into that. First I need to check which functions are actually shared.

@dpa99c
Copy link
Owner

dpa99c commented Feb 21, 2017

Having a main plugin containing common code then additional plugins which hook into it is probably the way forward. Thinking about it, it should be possible to share the native and JS code, so long as it's namespaced. The micro-plugins will need access to things like native permission requests on Android (which is common) and both native and JS constants that are defined for permission statuses, for example. It's entirely doable, but will need some reworking, such as exposing currently private functions in the main plugin so the micro-plugins can access them.

@pke
Copy link
Author

pke commented Feb 21, 2017

Yeah at first glance quickly at the code now I saw a bunch of private functions that would need to be made public. Do you see any other pitfalls?

@dpa99c
Copy link
Owner

dpa99c commented Feb 21, 2017

Do you see any other pitfalls?

Not off the top of my head, but there probably are some :-)

@pke
Copy link
Author

pke commented Feb 23, 2017

You know what? The confusion raised by the security audit that my app supposedly is using the mic, raised from the fact that the auditing software just scanned the plist of the ios app and saw the Microphone Usage Description. It never actually checked if the mic API is called or anything.

How about you remove all those default strings from the plugin.xml and instead let the user specify them using the cordova-custom-config like you also mention here for the Android permissions? I think that would be the best way for now. Since no permissions are actually requested, just the message dialog texts specified in the plist. And you wisely not added all the Android permissions to your plugin.xml for the very reason that users might not even use the mic in the first place.

What you think?

@dpa99c
Copy link
Owner

dpa99c commented Feb 23, 2017

I deliberately didn't allow the plugin to add <uses-permission/> entries to AndroidManifest.xml, because otherwise these permissions are asked for at installation.

But the reason I added the default iOS usage descriptions is a side effect on having to include every core framework in the plugin.xml.

The default usage descriptions are not actually used unless you actually request the permission at runtime. But if they are not present and the app includes the relevant framework, you app will get rejected by Apple.

For every core framework included in the iOS app, there must be a corresponding usage description, otherwise your uploaded app will get rejected once you upload it to iTunes Connect.

For example, if I remove NSPhotoLibraryUsageDescription, the plugin.xml still contains <framework src="Photos.framework" />. If you don't actually call requestCameraRollAuthorization() in your code, the message contained in `NSPhotoLibraryUsageDescription will never be displayed, but when you upload your app to iTunes Connected it will get automatically rejected with:

This app attempts to access privacy-sensitive data without a usage description. The app's Info.plist must contain an NSPhotoLibraryUsageDescription key with a string value explaining to the user how the app uses this data.

This requirement was introduced with iOS10/XCode 8. You can find plenty of related Q's on SO, e.g. this one.

I'd say the security audit software is being overzealous because unless there is a call to request permission at runtime, both the related framework and usage description message are never used.

@pke
Copy link
Author

pke commented Feb 23, 2017

Ah ok, understand now. Well then the original proposal still stands to split up the various sections of the plugin into sep. plugins. With them would go the framework.

Or we really need a pre-processor for cordova plugin.xml parsing ;)
Or the <framework> directive could be extended by a test attribute that is evaluated against config vars. Or any tag in the config.xml for that matter can have this test attribute and is treated as non-existent if the test eval is false.

After figuring out why the security audit software complained about mic access I was underwhelmed how bad its detection algo actually is.

@tverilytt
Copy link

Hi!
@pke @dpa99c I think it would be really useful to be able to specify which components to use.

I have tried to comment out, in plugin.xml, the framworks and plist entries I don't use / need.
But iTunes Connect reports problems with missing plist entries. So I guess they scan the code for actual usage.

Cheers
-jo

@pke
Copy link
Author

pke commented Feb 24, 2017

Yeah I think too the split would be helpful. If only to ship less code, reduce bundle sizes.

@dpa99c dpa99c added the feature request New feature or request label Mar 3, 2017
@pke
Copy link
Author

pke commented Mar 29, 2017

I feel more confident to work on the split now. I'll fork this repo and create a PR.

@peresbruno
Copy link

Hello @pke

You know what? The confusion raised by the security audit that my app supposedly is using the mic, raised from the fact that the auditing software just scanned the plist of the ios app and saw the Microphone Usage Description. It never actually checked if the mic API is called or anything.

I am having the same problem here. Have you a workaround for this? I have tried removing the info.plist key and the related framework from Linked Frameworks and Libraries, as you can see in this SO Question, but not worked.

Any suggestion? Thanks!

@spinninghamster
Copy link

Has anyone created a version that allows for selective permissions? I'm also running into this problem where iOS asks me why I want all these permissions even though I only need one..

@pke
Copy link
Author

pke commented Feb 5, 2018

@spinninghamster not yet. But it's still on my list of todos for this year.

@dpa99c
Copy link
Owner

dpa99c commented Feb 16, 2018

FYI I've started work on a major rework of the plugin which will divide up the plugin into logical areas of functionality (e.g Location, Bluetooth) which will be specified using plugin variables at install time.

My current strategy will be to retain a single plugin structure, but with the optional parts being included/excluded via plugin hook scripts at plugin install time based on the plugin variable settings in the config.xml

@dpa99c
Copy link
Owner

dpa99c commented Feb 26, 2018

I've pushed a proof-of-concept modules branch which demonstrates my proposed approach of using an npm post-install script to apply module preferences in a config.xml key at plugin install time.

You can try it out by installing direct from the branch:

cordova plugin add https://github.com/dpa99c/cordova-diagnostic-plugin#modules

There is currently only one optional module, Location, which implements modularity for Android, iOS and Windows 10.

So in your config.xml you would put <preference name="cordova.plugins.diagnostic.modules" value="LOCATION" /> to install the optional Location module at plugin install time. See the specifying modules notes for details.

Also note that the preference must be present in config.xml before the plugin is installed to the project. This is because the script runs on the npm install post-install hook. To update installed modules you must add/remove the plugin from your project. I would've like to use the before_plugin_install Cordova hook but it does not allow the dynamic manipulation of the plugin's plugin.xml configuration file that is required to make this single-plugin/multi-module approach work.

Any feedback/suggestions/bug reports on the modules branch is most welcome.

@dpa99c
Copy link
Owner

dpa99c commented Mar 1, 2018

This is resolved now that cordova.plugins.diagnostic@4 has been published, which splits the plugin into logical optional modules.

@dpa99c dpa99c closed this as completed Mar 1, 2018
@pke
Copy link
Author

pke commented Mar 1, 2018

That was quick. Will give it a try probably tomorrow. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants