support data sharing between applications on Android

[ajayambre]

PR for #151

[ggozad]

Thanks @ajayambre. Please give me some time to look through this, bit of a busy period atm, but should be ok to do next week. @demetris-manikas can you please also review this?

[demetris-manikas]

Sure.

[ajayambre]

@ggozad @demetris-manikas Example code for creating a new instance with the additional optional option.

var ss = new cordova.plugins.SecureStorage(     function() {         console.log("Success");     },     function(error) {         console.log("Error " + error);     },     "my_app",     {         "android": {             "packageName": "com.example.myApp1"         }     } );

The packageName value is the package name of the application that we want to access data of. The property is specific to android platform and is ignored for other platforms.

It allows for adding any new platform specific options (may be for ios or windows) in future.

[demetris-manikas]

@ajayambre Thanks for your contribution. Would it be possible to add some tests also?

[ajayambre]

@demetris-manikas Thanks for reviewing the code changes. I have done and committed the suggested changes.

Can I add the tests in the defineManualTests function? This feature requires that both apps be installed and have the same android:sharedUsedId. Any suggestions/expectations about tests would be helpful.

[demetris-manikas]

@ajayambre Well done! Sorry for I should have seen this before but I 'll have to ask you for some more changes.

By replacing String packageName = options.getString("packageName"); with String packageName = options.optString("packageName", getContext().getPackageName()); the code will be greatly simplified.

The null checking in getPackageContext can go away and also the default options in SecureStorage.prototype become obsolete.

Therefore the _merge_options is no longer needed and the code could go like

    var platformId = cordova.platformId;
    var opts = options && options[platformId] ? options[platformId] : {};

    this.service = service;
    try {
        _executeNativeMethod(success, error, 'init', [this.service, opts]);
    } catch (e) {
        error(e);
    }

[demetris-manikas]

Concerning the tests it so seems that a second app is needed so as to check this feature and I guess it will take some effort to automate it. The only thing I could come up with is to create a project for the tests that manipulates two cordova apps. Let us see what @ggozad has to say about it too.

[ajayambre]

@demetris-manikas Thank you. I agree that it would simplify the code.

Therefore the _merge_options is no longer needed and the code could go like

_merge_options checked for invalid properties and would throw an error if an unsupported property was passed as an option. I tried to maintain that behaviour from the earlier code. Do you think that validation is no longer required? This would mean the caller could pass any properties as options and they would be ignored without throwing any error.

Please confirm so that I can make the suggested changes.

[demetris-manikas]

Confirmed.

[ajayambre]

@demetris-manikas Thank you. Request you to please review the changes.

[demetris-manikas]

@ajayambre Looks good. :) The README should also be updated but lets wait until @ggozad reviews.

[ggozad]

Hey, sorry for the delay, it's been very busy lately. Can we have a manual test please? I guess the simplest would be if there was a minimal second app that would write one value with some package name. That app could run once during the manual setup (please provide instructions to make it clear). Then the test would try to read the value. I can't see how it can be simplified, but perhaps it can?

[ajayambre]

@ggozad Thanks. Will work on a test for this.

[srinathKrishna]

Hi Ajay. This is the same i am also looking for. I tried with 2 apps but my second app could not access the data saved by first app. I did have same android:shareduserId and i gave the app1 packagename while creating securestorage. Can you please help.

Thanks, Srinath

[ajayambre]

@ggozad Have added a test to verify the data sharing between apps on Android.

This test adds 2 new buttons in the Manual test. One button will set a key from the first app and the other button will read that key from the second app.

I created 2 Cordova apps to test this. One with a package name io.cordova.hellocordova and the other as io.cordova.hellocordova2

Modified the config.xml for both the apps with below changes. 1) Added xmlns:android="http://schemas.android.com/apk/res/android" in the initial widget tag.

2) Added the below tag in <platform name="android"> <edit-config file="AndroidManifest.xml" mode="merge" target="/manifest">     <manifest android:sharedUserId="io.cordova.myUser" /> </edit-config>
This is required as both the apps should have the same android:sharedUserId

The test passed and the second app was able to access a key set by the first app.

I also tried running the test by setting a different android:sharedUserId and the test for second app failed with an error Error: Key [sharedKey] not found

I also tried running the test by uninstalling the first app and the test for second app failed with an error Error: Application package io.cordova.hellocordova not found

Request you to please review the test. Thanks.

[ajayambre]

Hi Ajay. This is the same i am also looking for. I tried with 2 apps but my second app could not access the data saved by first app. I did have same android:shareduserId and i gave the app1 packagename while creating securestorage. Can you please help.

Thanks, Srinath

@srinathKrishna The code changes for this PR have not been merged in master yet. Please test by manually making these changes in your local or wait for this PR to be merged. :)

[ggozad]

@ajayambre thanks for this. I will test it out next week, couple of notes: Could you please also update the README and add android specific instructions on how android users can achieve sharing the storage? I haven't tested it yet, but if I understand correctly the tests assume that somehow the user will create the second app. If this is the case could you please create a folder under test where this app is already setup?

[ajayambre]

@ggozad Sorry for the late response.

Adding the second test app setup under the test folder was causing a cyclic dependency when running the cordova plugin add cordova-plugin-secure-storage command. So I have created a separate repo here and added 2 test apps and instructions for demonstrating data sharing for Android.

I have also updated the README.md with the instructions to achieve data sharing on Android.

Request you to please review the changes. Thank you.

[ajayambre]

@ggozad @demetris-manikas Request you to please review the committed changes.

[demetris-manikas]

Hi @ajayambre. Sorry for the delay but I am pretty busy for the time being. I 'll probably have time to take a look into it next week. At first glimpse I have some comments to make. Firstly the name of the repo is misleading. One would think that the tests for this plugin lie in your repo. Secondly I would expect one of the two apps to be the test app of this plugin. One extra app should be enough. Finally please consider using cordova-plugin-app-launcher to launch the second app from within the manual tests suite of this plugin. Thanks for your efforts.

[ajayambre]

@demetris-manikas Thank you for your feedback. Will work on the suggested changes.

[demetris-manikas]

@ajayambre Hi!. Any chance of finishing this up in the near future?

[ajayambre]

@demetris-manikas Hi. Sorry for the delay.

The main tests for my code changes are added in the tests/tests.js file.

I am not really sure of how to set up and simulate tests from 2 Cordova apps. Can you please help in setting up the test apps?

[demetris-manikas]

@ajayambre Could you please change the android section of the tests/plugin.xml to read

<platform name="android">
        <edit-config file="AndroidManifest.xml" mode="merge" target="/manifest">
            <manifest android:sharedUserId="com.crypho.SecureStorageTestsUser" />
        </edit-config>
        <dependency id="cordova-plugin-device" version="*" />
        <dependency id="cordova-plugin-app-launcher" version="*" />
</platform> 

and remove all tests. We 'll merge and create the tests on a later stage. Sorry for the delay but both me and @ggozad are very busy lately.

[ajayambre]

@demetris-manikas Thank you. Will work on this and commit the suggested changes over this weekend.

[ajayambre]

@demetris-manikas I have made the required changes to tests/plugin.xml and have removed all the tests that I added. I have also deleted the separate repo that I created to simulate 2 apps.

Request you to please review the changes.

[demetris-manikas]

@ajayambre Great! Thanks a lot. I have created a test app but needs some refinements. As soon as I am finished with it I 'll let you know.

@ggozad The code is fine and can be merged.

[demetris-manikas]

@ajayambre You forgot to add yourself to the contributors list!

[ajayambre]

@demetris-manikas Do you want me to update the contributors list in package.json? :blush:

[demetris-manikas]

Yeap

Στις Τετ, 10 Απρ 2019, 2:21 μ.μ. ο χρήστης Ajay Ambre < notifications@github.com> έγραψε:

@demetris-manikas https://github.com/demetris-manikas Do you want me to update the contributors list in package.json? 😊

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Crypho/cordova-plugin-secure-storage/pull/155#issuecomment-481649982, or mute the thread https://github.com/notifications/unsubscribe-auth/AKpIrxSN0BLMB6QYehBUu5nNkKSejD0iks5vfclQgaJpZM4YVHbm .

[ajayambre]

@demetris-manikas Done. Thank you!

[ajayambre]

@demetris-manikas @ggozad Thank you for merging this PR and releasing a new version for this. I would like to thank both of you for your reviews, guidance, patience and support. Keep up the great work!