Credentials support NSSecureCoding, CredentialsManager Utility, KeyChain Storage #113

cocojoe · 2017-05-02T08:45:32Z

Although I was very tempted to add SimpleKeyChain to this repo, I felt we should leave it up to the user. NS*Coding is how you generally expect to serialize information in iOS. I don't feel we need to add any convenience wrappers here to NSKeyedArchiver/NSKeyedUnArchiver as they are essentially one liners and is yet another public API.

lbalmaceda · 2017-05-05T17:08:23Z

Auth0/Authentication.swift

+        guard let refreshToken = credentials.refreshToken else {
+            return callback(AuthenticationError(string: "missing refresh_token", statusCode: 400), nil)
+        }
+        guard let expiresIn = credentials.expiresIn, expiresIn < Date(timeIntervalSinceNow: -60) else { return callback(nil, credentials) }


What does the -60 means? Now that I see this, it reminds me that some tokens might be deemed invalid wrongly depending on the device's clock. Remember the leeway thing we support in our jwt libs? Maybe we should open a method to customize it too for this case. It should not be an issue for most of the users but some of them might have it.

I added the 60 although it should perhaps be longer based like 5 minutes. As it would be if a token was not renewed as it was close to expiry and then immediately expires when the user goes to use it a few seconds later.
I've not looked at this in the JWT Libs, although I feel if a user's clock is wrong this isn't really our issue.

The token should be used right after the user asks for it, so it's up to him if he takes too long and the server rejects it.. Maybe not 5 minutes, but 2 min already is too much. Remember that the problem here is the server, because we can still decode a jwt payload if needed even if expired.. Maybe it's best to check the server "allowed leeway" and pick a smaller value just to be sure. Anyway, it's not too hard to log in again 😛. I'll approve this pr and let you decide if a change it's needed.

We should have a leeway eventually just in case to avoid issues with customers. Having 5 minutes or a big one is not that a big deal since the RS will properly validate the AT and reject it on API call

hzalaz · 2017-05-08T23:48:45Z

Auth0/Authentication.swift

+     - parameter callback: callback with the user's credentials or the cause of the error.
+     - important: This method only works for a refresh token obtained after auth with OAuth 2.0 API Authorization.
+     */
+    func renewExpired(_ credentials: Credentials, scope: String? = nil, callback: @escaping (Error?, Credentials?) -> Void) {


This sort of breaks all the convention of API with the request that the rest of the methods have. If we want to avoid that we can move this to a different type

Also the naming is a bit odd, since it won't renew expired all the time

If we add the keychain storing how the whole story will look like?

Regarding naming, well renewExpired implies only renew when expired, you will get the same credentials back if they have not expired.

Yes, I was conflicted over where to put it. I put it here as it requires Authentication so rather than passing an Authentication instance it, but agree the breaking of convention makes it not quite feel right from an API purity point of view.

I ran originally ran through a few scenarios, for example I liked the idea of the renew method in Credentials or in a Storage utility class.

public class Storage: A0SimpleKeychain { public func archive(object: NSSecureCoding, forKey key: String) -> Bool { return self.setData(NSKeyedArchiver.archivedData(withRootObject: object), forKey: key) } public func unarchive(objectWithKey key: String) -> Any? { guard let data = self.data(forKey:key) else { return nil } return NSKeyedUnarchiver.unarchiveObject(with: data) } }

If we want to avoid adding SimpleKeyChain this can become a protocol.

One thought:

let storedCredentials = storage.unarchive("credentials") as? Credentials // Class method in Credentials Credentials.renewExpired(credentials, authentication: authentication) { error, credentials in }

You could of course add a 1 call wrapper as well to unarchive and renew from storage, which people would use.

The inheritance there is not needed and tbh it exposes way too much info and API. Also you could have achieved the same thing with an extension.

I'd say we iterate the idea a bit more with a separate entity to handle the Auth state.

If I read it renewExpired seems to be imply that it might be called when I know the token is expired not to ensure I have a fresh/valid/nonexpired token

hzalaz · 2017-05-08T23:49:15Z

Auth0/Authentication.swift

+     */
+    func renewExpired(_ credentials: Credentials, scope: String? = nil, callback: @escaping (Error?, Credentials?) -> Void) {
+        guard let refreshToken = credentials.refreshToken else {
+            return callback(AuthenticationError(string: "missing refresh_token", statusCode: 400), nil)


We should use a different type of error for this case

I wanted to avoid creating another error type. WebAuthError is closer but as it deals with missing token cases or can create a new error type but it's pretty much one case.

Ok but put status code 0 not 400

cocojoe · 2017-05-09T21:52:39Z

@hzalaz after todays talks, I've added SimpleKeychain as it makes sense to have this here now. I've used my SimpleKeychain branch as it supports all platforms that Auth0.Swift does.

Turned this into a Credentials utility for easy, store, retrieve, renew. Sort of thing be nice to see in the Centralized QS.
I've not enforced expiry checking as I feel this is an education issue.

I can update tests and add docheaders in morning, wanted to check it was the right direction to take. Not 100% sure on name though.

cocojoe · 2017-05-10T20:36:32Z

I am still not sure on renewExpired, I like the idea of a best practice (as user probably not bother otherwise) method to retrieve -> check expiry -> (return or renew).
In last iteration this was an exercise for the user, retrieve -> check expiry yourself -> use or call renew.
Perhaps really it just needs a better name to imply retrieval + optional renew if expired.
retrieveCredentialsAutoRenew

hzalaz · 2017-05-14T23:42:50Z

Auth0/CredentialsManager.swift

+    /// Retrieve stored credentials instance from keychain
+    ///
+    /// - Returns: Optional Credentials instance
+    public func retrieve() -> Credentials? {


Lets remove this one in favor of the next one

hzalaz · 2017-05-14T23:43:57Z

Auth0/CredentialsManager.swift

+    ///
+    /// - Parameter credentials: credentials instance to store
+    /// - Returns: Bool outcome of success
+    public func store(_ credentials: Credentials) -> Bool {


Lets not use _

I don't mind either way, however isn't it clear inside a CredentialsManager that has a param type of Credentials that it is going to store a credentials instance?

more for the flow of the api manager.store(credentials: result) then in the future we could have manager.store(accessToken: token, expiresIn: date) to avoid passing all the credential object.

hzalaz · 2017-05-14T23:45:01Z

Auth0/CredentialsManager.swift

+    ///   - scope: scopes to request for the new tokens. By default is nil which will ask for the same ones requested during original Auth
+    ///   - callback: callback with the user's credentials or the cause of the error.
+    /// - Important: This method only works for a refresh token obtained after auth with OAuth 2.0 API Authorization.
+    public func retrieveCredentialsAutoRenew(withScope scope: String? = nil, callback: @escaping (CredentialsManagerError?, Credentials?) -> Void) {


Just call it either credentials(withScope:, callback:) or withCredentials(scope:, callback) the idea is to use this one to try to get credentials to be used. If it fails the user should login otherwise it will always yield a valid token

Thx, sometimes naming is the hardest part.

hzalaz · 2017-05-14T23:46:45Z

Auth0/CredentialsManagerError.swift

+
+import Foundation
+
+public enum CredentialsManagerError: Error {


I am not sure about this list of errors, few of them are actionable for the developer

I tend to add them for logging, could argue that expiresIn and refreshToken could be one guard and one error e.g. just .noRefreshToken

There should be 3 cases

Failed to refresh (api call or no expires In)

No credentials (no creds)

No refresh token (only raised when trying to renew)

hzalaz · 2017-05-16T18:50:22Z

Auth0/CredentialsManagerError.swift

+
+import Foundation
+
+public enum CredentialsManagerError: Error {


There should be 3 cases

Failed to refresh (api call or no expires In)

No credentials (no creds)

No refresh token (only raised when trying to renew)

Added CredentialsManager convenience utility

Update Tests

cocojoe mentioned this pull request May 5, 2017

Added Touch ID "Remeber me" with keychain storage and credentials renewal auth0/Lock.swift#435

Closed

lbalmaceda reviewed May 5, 2017

View reviewed changes

lbalmaceda previously approved these changes May 8, 2017

View reviewed changes

hzalaz suggested changes May 8, 2017

View reviewed changes

cocojoe force-pushed the added-credentials-securecoding branch from be9822a to 3eb9eb4 Compare May 10, 2017 12:18

cocojoe force-pushed the added-credentials-securecoding branch from f0c3ca6 to 476d3ac Compare May 11, 2017 08:47

cocojoe changed the title ~~NSSecureCoding conformance for Credentials, Renew expired tokens convenience~~ Credentials support NSSecureCoding, CredentialsManager Utility, KeyChain Storage May 11, 2017

hzalaz suggested changes May 14, 2017

View reviewed changes

hzalaz suggested changes May 16, 2017

View reviewed changes

cocojoe added 2 commits May 17, 2017 10:21

Added Credentials conformance to NSSecureCoding

e3d01a8

Added SimpleKeychain Lib (iOS Only)

8350850

Added CredentialsManager convenience utility

cocojoe force-pushed the added-credentials-securecoding branch 2 times, most recently from c56857e to 00903d6 Compare May 18, 2017 15:27

hzalaz previously approved these changes May 18, 2017

View reviewed changes

Streamline CredentialManagerErrors

5cceb8f

Update Tests

cocojoe dismissed stale reviews from hzalaz and lbalmaceda via 5cceb8f May 18, 2017 22:28

cocojoe force-pushed the added-credentials-securecoding branch from 00903d6 to 5cceb8f Compare May 18, 2017 22:28

hzalaz approved these changes May 18, 2017

View reviewed changes

hzalaz added the CH: Added label May 18, 2017

hzalaz added this to the v1-Next milestone May 18, 2017

hzalaz merged commit c21f67d into master May 18, 2017

hzalaz deleted the added-credentials-securecoding branch May 18, 2017 22:59

cocojoe mentioned this pull request Jun 6, 2017

Release 1.6.0 #119

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Credentials support NSSecureCoding, CredentialsManager Utility, KeyChain Storage #113

Credentials support NSSecureCoding, CredentialsManager Utility, KeyChain Storage #113

cocojoe commented May 2, 2017 •

edited

Loading

lbalmaceda May 5, 2017

cocojoe May 8, 2017

lbalmaceda May 8, 2017

hzalaz May 8, 2017

hzalaz May 8, 2017

hzalaz May 8, 2017

hzalaz May 8, 2017

cocojoe May 9, 2017 •

edited

Loading

cocojoe May 9, 2017 •

edited

Loading

cocojoe May 9, 2017

hzalaz May 9, 2017

hzalaz May 8, 2017

cocojoe May 9, 2017

hzalaz May 9, 2017

cocojoe commented May 9, 2017

cocojoe commented May 10, 2017

hzalaz May 14, 2017

hzalaz May 14, 2017

cocojoe May 15, 2017

hzalaz May 16, 2017

hzalaz May 14, 2017

cocojoe May 15, 2017

hzalaz May 14, 2017

cocojoe May 15, 2017

hzalaz May 16, 2017

hzalaz May 16, 2017


		import Foundation

		public enum CredentialsManagerError: Error {

Credentials support NSSecureCoding, CredentialsManager Utility, KeyChain Storage #113

Credentials support NSSecureCoding, CredentialsManager Utility, KeyChain Storage #113

Conversation

cocojoe commented May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocojoe May 9, 2017 • edited Loading

Choose a reason for hiding this comment

cocojoe May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocojoe commented May 9, 2017

cocojoe commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocojoe commented May 2, 2017 •

edited

Loading

cocojoe May 9, 2017 •

edited

Loading

cocojoe May 9, 2017 •

edited

Loading