feat(secure_storage): remove items on re-install for iOS & MacOS #2118
Conversation
|
|
||
| if (Platform.isIOS || Platform.isMacOS) { | ||
| final userDefaults = NSUserDefaultsAPI(); | ||
| final key = '$scopeStoragePrefix.${config.scope}.isKeychainConfigured'; |
There was a problem hiding this comment.
@harsh62 fyi - here is where the key name for NSUserDefaults is being created. scopeStoragePrefix is a static const definied in this class.
Codecov Report
@@ Coverage Diff @@
## next #2118 +/- ##
==========================================
+ Coverage 38.21% 42.98% +4.76%
==========================================
Files 111 115 +4
Lines 6652 7503 +851
==========================================
+ Hits 2542 3225 +683
- Misses 4110 4278 +168
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Jordan-Nelson
force-pushed
the
feat/package-uninstall-ios
branch
from
294ab8f
to
3e85c1b
Compare
| @@ -34,7 +34,7 @@ class AmplifyAuthCognito extends AmplifyAuthCognitoDart with AWSDebuggable { | |||
| /// | |||
| /// To change the default behavior of credential storage, | |||
| /// provide a [credentialStorage] value. If no value is provided, | |||
| /// [AmplifySecureStorage] will be used with a `scope` of "auth". | |||
| /// [AmplifySecureStorage] will be used with a `scope` of "awsCognitoAuthPlugin". | |||
There was a problem hiding this comment.
While discussing re-install behavior with @harsh62 we agreed that the scope name should be specific to Cognito.
| flutter format --fix lib/src/messages.cupertino.g.dart | ||
| # TODO(Jordan-Nelson): Remove when swift codegen in pigeon is no longer experimental. | ||
| # See: https://github.com/flutter/flutter/issues/73738#issuecomment-903927725 | ||
| sed -i '' -e 's/\<Flutter\/Flutter\.h\>/\<FlutterMacOS\/FlutterMacOS\.h\>/' macos/Classes/Messages.m |
There was a problem hiding this comment.
Note: Pigeon doesn't officially support using objective-c for MacOS because flutter doesn't officially support using it for MacOS plugins. However, it works just fine with this one import update. Pigeon recently added support for swift codegen for MacOS, but swift codgen in pigeon is considered experimental.
I think it makes sense to stick with objective-c until the swift code generator is no longer "experimental". At that point we can switch iOS and MacOS over to using swift.
| /// or more Keychain Access Groups in Xcode. For more info, see the | ||
| /// [Platform Setup docs](https://docs.amplify.aws/lib/project-setup/platform-setup/q/platform/flutter/). | ||
| factory MacOSSecureStorageOptions({ | ||
| bool useDataProtection = true, | ||
| @visibleForTesting bool useDataProtection = true, |
There was a problem hiding this comment.
If it's visiblefortesting only should it even be documented publicly?
I've been thinking about this generally, too. What about bool useDataProtection = zReleaseMode which would allow easier onboarding since even testing Auth apps locally is a pain having to configure Xcode and remember not to commit those changes.
There was a problem hiding this comment.
Re: should it be documented - Maybe not. I think it would be valid for a customer to use it in a test if they wanted, but not sure we need to document it.
It would allow easier onboarding, but potentially lead to confusion down the line. I could see a customer not realizing there is a difference in release build and being pretty confused why their first release build has an issue. I have also stumbled upon some odd differences in keychain behavior when this is true/false (See this comment). That is why I added the @visibleForTesting. I want to really discourage customers from using it given the differences in keychain behavior. I actually want to update our tests to have this set to true. I have been running the tests locally with it set to true, but would like to get that in CI given the differences in behavior I have seen.
There was a problem hiding this comment.
Bummer, okay. That makes sense.
There was a problem hiding this comment.
I think eventually, then, we should consider removing the parameter altogether.
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.