search

mogol / flutter_secure_storage

A Flutter plugin to store data in secure storage
https://pub.dartlang.org/packages/flutter_secure_storage
BSD 3-Clause "New" or "Revised" License
1.09k stars 340 forks source link

fix: [iOS / macOS] pass accessibility to `readAll` and `deleteAll` #719

Closed techouse closed 1 month ago

techouse commented 1 month ago

As described in https://github.com/mogol/flutter_secure_storage/issues/709#issuecomment-2114743623 I was having issues getting values from my keychain using await secureStorage.readAll(), however, await secureStorage.read(key: 'foo') worked fine. Additionally, @feinstein reported similar issues with deleteAll in #720

It seems that somehow the accessibility for readAll and deleteAll was explicitly (possibly deliberately?) set to nil in #602

After I've passed accessibility to readAll and deleteAll just like it's passed in read and delete I was able to get data from my keychain using readAll and delete it using deleteAll.

Please see the tests mentioned in https://github.com/mogol/flutter_secure_storage/issues/709#issuecomment-2114743623 how to replicate / test this.

Further reading (Apple documentation)

techouse commented 1 month ago

If anyone wants to test this PR directly you can do so using

dependencies:
  flutter_secure_storage: ^9.2.1

dependency_overrides:
  flutter_secure_storage:
    git:
      url: https://github.com/techouse/flutter_secure_storage.git
      ref: fix/read-all
      path: flutter_secure_storage
  flutter_secure_storage_macos:
    git:
      url: https://github.com/techouse/flutter_secure_storage.git
      ref: fix/read-all
      path: flutter_secure_storage_macos
juliansteenbakker commented 1 month ago

Thank you for providing a fix for this issue. Should we also change this for the deleteAll function? Accessibility is still nill in that function.

    internal func deleteAll(groupId: String?, accountName: String?, synchronizable: Bool?) -> FlutterSecureStorageResponse {
        let keychainQuery = baseQuery(key: nil, groupId: groupId, accountName: accountName, synchronizable: synchronizable, accessibility: nil, returnData: nil)
techouse commented 1 month ago

It would be good to find out why @bierbaumtim set it to nil in #602 or what changed in the package and/or potentially Apple's side that broke this in v9.1.0+ but worked normally in v9.0.0 and below.

juliansteenbakker commented 1 month ago

possibly related to #720

techouse commented 1 month ago

possibly related to #720

@juliansteenbakker you were right!

deleteAll is/was broken just like described in #720 and adding the accessibility parameter fixes it.

juliansteenbakker commented 1 month ago

Awesome, thanks for the quick fix !

feinstein commented 1 month ago

Can we cut a new stable version with this fix, so I can add it to my app?

techouse commented 1 month ago

Can we cut a new stable version with this fix, so I can add it to my app?

I guess that's a question for @juliansteenbakker

For now I suggest you either stick with v9.0.0

dependencies:
  flutter_secure_storage: ^9.0.0
dependency_overrides:
  flutter_secure_storage: 9.0.0
  flutter_secure_storage_macos: 3.0.1
  flutter_secure_storage_platform_interface: 1.0.2

or use the dev branch

dependencies:
  flutter_secure_storage: ^9.2.1
dependency_overrides:
  flutter_secure_storage:
    git:
      url: https://github.com/mogol/flutter_secure_storage.git
      ref: develop
      path: flutter_secure_storage
  flutter_secure_storage_macos:
    git:
      url: https://github.com/mogol/flutter_secure_storage.git
      ref: develop
      path: flutter_secure_storage_macos
juliansteenbakker commented 1 month ago

You can expect an update in a few hours @feinstein

bierbaumtim commented 1 month ago

@techouse I set it to nil because it didn't make sense to me to read multiple values and only provide one parameter for accessibility. But I was wrong about that and it makes perfect sense.

So, sorry for that bug.

techouse commented 1 month ago

@bierbaumtim Don't be sorry, man! It's a bug, not a crime. 😄

Thanx for the explanation. 🙂 I was hoping it was something as simple as this and not yet another Apple caveat.

@juliansteenbakker looks like it's all good for the release then 🚀

feinstein commented 1 month ago

Maybe we need to explore the idea of adding integration tests to the library, so we can protect ourselves from these kinds of bugs?

A integration test that reads all and deletes all would have caught these bugs, and prevented a release.

juliansteenbakker commented 1 month ago

I have released v9.2.2 with this fix. @feinstein there already are integration tests, but Github Actions is only configured for Android right now.

feinstein commented 1 month ago

Thanks a lot!

Do you need help to spin up the ios tests?

techouse commented 1 month ago

Do you need help to spin up the ios tests?

@juliansteenbakker looks like the integration tests still use flutter_driver. If there's gonna be any work done on adding an iOS integration test we should probably also migrate to integration_test.

CC / @feinstein