Crypho / cordova-plugin-secure-storage

Secure storage plugin for Apache Cordova
MIT License
278 stars 269 forks source link

ios: multiple calls at the same, only the last one's callback is executed #30

Closed Alex-Sessler closed 8 years ago

Alex-Sessler commented 8 years ago

I've found this behaviour in my main app and have also managed to reproduce it in a minimal cordova app. All of this is executed after the deviceReady event.

Now these calls are supposed to fail, because there is no data in the keychain yet:

 window.secureStorage = new cordova.plugins.SecureStorage(
     function() {
         console.log('Securestorage init success.');

         console.log('securestorage get 1');
         window.secureStorage.get(function success(value) {
             console.log('secure get success', value);
         }, function failure(err) {
             console.log('Secure get failed', err);
         }, 'test');

         console.log('securestorage get 2');
         window.secureStorage.get(function success(value) {
             console.log('secure get 2 success', value);
         }, function failure(err) {
             console.log('Secure get 2 failed', err);
         }, 'test2');

         console.log('securestorage get 3');
         window.secureStorage.get(function success(value) {
             console.log('secure get 3 success', value);
         }, function failure(err) {
             console.log('Secure get 3 failed', err);
         }, 'test3');
     },
     function(error) {
         console.log('Securestorage init error ', error);

     },
     'demo');

The output I receive however, is:

[Log] Securestorage init success. (console-via-logger.js, line 173)
[Log] securestorage get 1 (console-via-logger.js, line 173)
[Log] securestorage get 2 (console-via-logger.js, line 173)
[Log] securestorage get 3 (console-via-logger.js, line 173)
[Log] Secure get 3 failed – "Failure in SecureStorage.get() - errSecItemNotFound" (console-via-logger.js, line 173)

Now let's say I want to store some values in the keychain, which should succeed:

 window.secureStorage = new cordova.plugins.SecureStorage(
     function() {
         console.log('Securestorage init success.');

         console.log('securestorage set 1');
         window.secureStorage.set(function success(value) {
             console.log('secure set success', value);
         }, function failure(err) {
             console.log('Secure set failed', err);
         }, 'test', 'value1');

         console.log('securestorage set 2');
         window.secureStorage.set(function success(value) {
             console.log('secure set 2 success', value);
         }, function failure(err) {
             console.log('Secure set 2 failed', err);
         }, 'test2', 'value2');

         console.log('securestorage set 3');
         window.secureStorage.set(function success(value) {
             console.log('secure set 3 success', value);
         }, function failure(err) {
             console.log('Secure set 3 failed', err);
         }, 'test3', 'value2');
     },
     function(error) {
         console.log('Securestorage init error ', error);

     },
     'demo');

The calls succeed, but the pattern is the same as before, the success callback is only executed for the last one:

[Log] Securestorage init success. (console-via-logger.js, line 173)
[Log] securestorage set 1 (console-via-logger.js, line 173)
[Log] securestorage set 2 (console-via-logger.js, line 173)
[Log] securestorage set 3 (console-via-logger.js, line 173)
[Log] secure set 3 success – "test" (console-via-logger.js, line 173)

There must be some problems with processing those requests in separate threads.

ggozad commented 8 years ago

Ouch! That's pretty bad, will try to fix this week. Would it be possible to make a failing test case for the above?

Alex-Sessler commented 8 years ago

Hm, what do you mean with "a failing test case"? In my first example, the get call fails and in the second example the set call succeeds. Since all functions use the same helper functions on error/succeed I though that should be good enough.

ggozad commented 8 years ago

I mean there is a small test suite in the package. If you could write a test case that fails (copy pasting the above and asserting that all callbacks are called properly) that would save me some trouble :)

ggozad commented 8 years ago

I've added a branch with a test case that demonstrates this in FIX-thread-callbacks. Seems that since callbackId is kept in self, this is overwritten every time. @goshakkk would you please be able to fix this?

ggozad commented 8 years ago

Seems I have fixed this in FIX-thread-callbacks. Tests now all pass including multiple calls to securestorage. @Alex-Sessler could you please verify with your app running the branch?

Alex-Sessler commented 8 years ago

I tested the branch version and it seems to fix my issues... Awesome, thanks!