Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added support for custom Keychain key in Credentials Manager #208

Merged
merged 2 commits into from
Jul 26, 2018
Merged

Added support for custom Keychain key in Credentials Manager #208

merged 2 commits into from
Jul 26, 2018

Conversation

danielphillips
Copy link
Contributor

Solution for support ticket 45174.

This allows us to use multiple CredentialsManager instances in order to support multiple accounts in one app.

cocojoe
cocojoe reviewed Jul 24, 2018
View reviewed changes
Auth0/CredentialsManager.swift Outdated
@@ -40,7 +40,8 @@ public struct CredentialsManager {
///
/// - Parameters:
/// - authentication: Auth0 authentication instance
public init(authentication: Authentication) {
public init(authentication: Authentication, storeKey: String = "credentials") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add Doc Header

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

cocojoe
cocojoe requested changes Jul 24, 2018
View reviewed changes
Copy link
Member

@cocojoe cocojoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this in general, please add some test(s). For example using two instances and ensuring storage is isolated.

@danielphillips
Copy link
Contributor Author

I've added two tests proving that:

  1. Two instances using a different store key store to different places in the keychain and don't interfere with each other.
  2. Two instances using the same store key experience shared credentials.

I'm not best pleased with how I've laid out the tests, any feedback on this would be nice.

@cocojoe cocojoe changed the title Move storeKey to init, allowing usage to point to anywhere in keychain. Added support for custom Keychain key in Credentials Manager Jul 24, 2018
cocojoe
cocojoe reviewed Jul 24, 2018
View reviewed changes
Auth0/CredentialsManager.swift Outdated
@@ -40,7 +40,9 @@ public struct CredentialsManager {
///
/// - Parameters:
/// - authentication: Auth0 authentication instance
public init(authentication: Authentication) {
/// - storeKey: String key to store user credentials in the keychain, defaults to "credentials"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to specify the type in the description String -> Key to store...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just this to address and I'll approve, thx

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I've amended that now.

cocojoe
cocojoe requested changes Jul 24, 2018
View reviewed changes
Auth0Tests/CredentialsManagerSpec.swift

}

describe("multi instances of credentials manager") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not as bothered about tests as long as they work. However, they would look cleaner to use closure shorthand like https://github.com/danielphillips/Auth0.swift/blob/a78a4018722c9202ee5d1a75c2608f5c00f855ac/Auth0Tests/CredentialsManagerSpec.swift#L263

credentialsManager.credentials { 
   expect($1?.accessToken) == AccessToken
   expect($1?.idToken) == IdToken
  done()
}

Cut down on the extra params.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point thank you. Done that now.

@cocojoe
Copy link
Member

cocojoe commented Jul 24, 2018

@danielphillips just merged another PR into master, can you rebase this please.

@danielphillips danielphillips force-pushed the feature/45174 branch 3 times, most recently from d408607 to 69094ae Compare July 24, 2018 21:16
@danielphillips
Copy link
Contributor Author

I'm rebased and I've addressed the feedback.

@danielphillips
Copy link
Contributor Author

I've addressed the doc header showing the type.

cocojoe
cocojoe approved these changes Jul 26, 2018
View reviewed changes
Copy link
Member

@cocojoe cocojoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cocojoe cocojoe merged commit 1c76432 into auth0:master Jul 26, 2018
@cocojoe cocojoe modified the milestones: 1.9.2, vNext Jul 26, 2018
@cocojoe cocojoe mentioned this pull request Jul 26, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cocojoe cocojoe cocojoe approved these changes

Assignees
No one assigned
Labels
CH: Added
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.
2 participants
@danielphillips @cocojoe