Added support to configure credentials manager for iOS and Android [DO NOT MERGE] #576
Conversation
| class IOSCredentialsConfiguration { | ||
| String storeKey; | ||
| String accessGroup; | ||
| Accessibility accessibility; |
There was a problem hiding this comment.
Can storeKey , accessGroup and accessibility be optional ?
There was a problem hiding this comment.
storeKey has a default value on the Credentials manager (https://auth0.github.io/Auth0.swift/documentation/auth0/credentialsmanager/init(authentication:storekey:storage:)).
accessGroup defaults to nil on SimpleKeychain, and accesibility defaults to .afterFirstUnlock (https://auth0.github.io/SimpleKeychain/documentation/simplekeychain/simplekeychain/init(service:accessgroup:accessibility:accesscontrolflags:context:synchronizable:attributes:)).
Seeing that all of them have default values and thus are not mandatory, they should be optional.
There was a problem hiding this comment.
BTW, it would be good to mention the default values in the doc comments.
| @@ -0,0 +1,46 @@ | |||
| // | |||
| class CredentialsManagerConfiguration { | |||
There was a problem hiding this comment.
Add comments for all classes
| var instance = CredentialsManagerHandler.credentialsManager ?? CredentialsManager(authentication: apiClient) | ||
|
|
||
|
|
||
| var createCredentialManager: (_ apiClient: Authentication, _ arguments: [String: Any]) -> CredentialsManager = { apiClient, arguments in |
There was a problem hiding this comment.
I'd suggest moving createCredentialManager out of credentialsManagerProvider. The point of credentialsManagerProvider being a var is that you can mock it out for tests.
I think it makes sense for createCredentialManager to be swappable as well, but that only makes sense as long as there are tests. Otherwise there is no point in having it be a var.
There was a problem hiding this comment.
I understand... I had initially created this as a function but compiler kept throwing me error
Cannot use instance member 'createCredentialManager' within property initializer; property initializers run before 'self' is available
Will check it again
|
The iOS tests are missing. These should go here: https://github.com/auth0/auth0-flutter/blob/main/auth0_flutter/example/ios/Tests/CredentialsManager/CredentialsManagerHandlerTests.swift |
| <resources> | ||
| <string name="com_auth0_domain">YOUR_AUTH0_DOMAIN</string> | ||
| <string name="com_auth0_scheme">demo</string> | ||
| </resources> |
There was a problem hiding this comment.
Isn't this file needed for the Android example, given that strings.xml is not committed?
There was a problem hiding this comment.
This was accidentally pushed. Will revert
| /// Use [CredentialsManagerConfiguration] to configure platform specific | ||
| /// configuration of the CredentialsManager |
There was a problem hiding this comment.
| /// Use [CredentialsManagerConfiguration] to configure platform specific | |
| /// configuration of the CredentialsManager | |
| /// Use [CredentialsManagerConfiguration] to set platform-specific | |
| /// configuration for the default CredentialsManager. |
|
There should also be tests on the Dart layer. These should go here: https://github.com/auth0/auth0-flutter/blob/main/auth0_flutter_platform_interface/test/method_channel_credentials_manager_test.dart |
📋 Changes
This PR adds support to configure the CredentialsManager class on both Android and iOS platforms. This change allows user to set the
sharedpreferenceNamein Android andsecretKey,accessGroupandaccessibilityon iOS while setting up the CredentialsManager inauth0_flutter📎 References
#553