Support id_token response type #62

cocojoe · 2016-11-29T11:19:22Z

Chaining method for response types
Auto set implicit grant
Credentials support for id_token only
Chaining method for nonce
Nonce required for id_token response type
Tests added

Chaining method for response types Auto set implicit grant Credentials support for id_token only Chaining method for nonce Nonce required for id_token response type Tests added

hzalaz · 2016-11-29T11:51:14Z

Auth0/ResponseType.swift

+
+import Foundation
+
+public enum ResponseType: String {


The OptionType is a best choice for this

hzalaz · 2016-11-29T11:52:42Z

Auth0/SafariWebAuth.swift

+        if !responseType.contains(.code) {
+            _ = self.usingImplicitGrant()
+        }
+        let response = responseType.reduce(String()) { $0 + $1.rawValue + " "}


Try to use literals whenever possible not String()

Also you could use

responseType.joined(separator: " ")

and avoid the dropLast()

It's not as easy as that due to it being an enum and requiring rawValue, perhaps with the suggestion of using OptionSet it will flow more easily. I'll look into it.

responseTypes.map { $0.rawValue }.joined(separator: " ")

hzalaz · 2016-11-29T11:54:21Z

Auth0/SafariWebAuth.swift

@@ -88,6 +102,11 @@ class SafariWebAuth: WebAuth {
            else {
                return callback(Result.failure(error: WebAuthError.noBundleIdentifierFound))
        }
+        if let response = self.parameters["response_type"], response.contains(ResponseType.id_token.rawValue) {


response_type is mandatory and we should combine the if and guard in a single guard, it serves no purpose having them separated

hzalaz · 2016-11-29T11:55:15Z

Auth0/WebAuthError.swift

@@ -63,6 +65,11 @@ public enum WebAuthError: CustomNSError {
                NSLocalizedDescriptionKey: message,
                WebAuthError.infoKey: self
            ]
+        case .noNonceProvided:
+            return [
+                NSLocalizedDescriptionKey: "You must provide a nonce value for the id_token response type",


A nonce value must be supplied when response_type includes id_token in order to prevent replay attacks

hzalaz · 2016-11-29T11:56:48Z

Auth0/Credentials.swift

    public let idToken: String?
    public let refreshToken: String?
+    public var hasToken: Bool {


Don't see the why we need this method, the hasToken can only be known to the one who starts the auth or if we pass along the response_type and validate. Thing we should do before yielding the credentials to the caller

hzalaz · 2016-11-29T11:57:32Z

Auth0/Credentials.swift


-    required public init(accessToken: String, tokenType: String, idToken: String? = nil, refreshToken: String? = nil) {
+    required public init(accessToken: String? = nil, tokenType: String? = nil, idToken: String? = nil, refreshToken: String? = nil) {


This is sort of a breaking change so I'd need to figure out a way to communicate this, not a big one but still a breaking one

hzalaz · 2016-11-29T11:58:22Z

Auth0/OAuth2Grant.swift

@@ -33,7 +33,7 @@ struct ImplicitGrant: OAuth2Grant {
    let defaults: [String : String] = ["response_type": "token"]

    func credentials(from values: [String : String], callback: @escaping (Result<Credentials>) -> ()) {
-        guard let credentials = Credentials(json: values as [String : Any]) else {
+        guard let credentials = Credentials(json: values as [String : Any]), credentials.hasToken else {


We need to validate that was requested was returned otherwise we need to fail.

hzalaz · 2016-11-29T12:04:06Z

Auth0/SafariWebAuth.swift

@@ -76,6 +76,20 @@ class SafariWebAuth: WebAuth {
        return self
    }

+    func responseType(_ responseType: [ResponseType]) -> Self {


We need to enforce that a response_type is picked here and by default should include code

Updated Carthage libs in line with Lock v2 JWTDecode added for nonce check in id_token ResponseType Enum changed to OptionSet Response Error enum added

Shuffled code around to be inline with builder expectations

Reduced and merged response errors into webauth errors Separate idtoken validation code

hzalaz · 2016-12-01T01:13:28Z

Auth0/OAuth2Grant.swift


    func credentials(from values: [String : String], callback: @escaping (Result<Credentials>) -> ()) {
+        // id token reponse expectations and JWT validation, nonce validation
+        if response.contains(.id_token) {


why the if-guard? just use guard

Multiple responseType are processed in this func, guard would exit if the responseType was not .id_token, even .id_token would then fail in the next proposed guard as it wouldn't contain .token
This applies to all my usage of if then guard.

hzalaz · 2016-12-01T01:13:37Z

Auth0/OAuth2Grant.swift

+        }
+
+        // token response expectations
+        if response.contains(.token) {


Same as above

hzalaz · 2016-12-01T01:16:33Z

Auth0/OAuth2Grant.swift

@@ -97,3 +124,16 @@ struct PKCE: OAuth2Grant {
        return items
    }
 }
+
+private func validate(token: String, nonce: String) -> Bool {
+    do {


guard let jwt = try? decode(jwt: token), let nonce = jwt.claim(name: "nonce") else { return false } return nonce.string == nonce

If we are not handling the error is useless to catch it

jwt.claim is non optional, agree it can be streamlined.

hzalaz · 2016-12-01T01:18:11Z

Auth0/SafariWebAuth.swift

-    func usingImplicitGrant() -> Self {
-        self.usePKCE = false
+    func response(_ response: [ResponseOptions]) -> Self {
+        if !response.contains(.code) { self.usePKCE = false }


do we need the if?

We can probably retire self.userPKCE and replace with a check to see if response contains .code

hzalaz · 2016-12-01T01:18:43Z

Auth0/SafariWebAuth.swift

    func start(_ callback: @escaping (Result<Credentials>) -> ()) {
        guard
            let redirectURL = self.redirectURL
            , !redirectURL.absoluteString.hasPrefix(SafariWebAuth.NoBundleIdentifier)
            else {
                return callback(Result.failure(error: WebAuthError.noBundleIdentifierFound))
        }
+        if self.response.contains(.id_token) {


if-guard again, it's better to have just guard

hzalaz · 2016-12-01T01:19:37Z

Auth0/ResponseOptions.swift

+
+public struct ResponseOptions: OptionSet {
+    public let rawValue: Int
+    public let label: String?


I assume this is part of the hack to use label right?

If we use an enum? it may be better, we might need to provide a computed property name but it could be less hackish

We can do this anyway, originally I had this but removed it for some reason. However in retrospect I think it's fine.

var label: String? { switch self.rawValue { case ResponseOptions.token.rawValue: return "token" case ResponseOptions.id_token.rawValue: return "id_token" case ResponseOptions.code.rawValue: return "code" default: return nil } }

hzalaz · 2016-12-01T01:22:58Z

Auth0/WebAuth.swift

+    /// Setup the response types to be used for authentcation
+    ///
+    /// - Parameter response: Array of ResponseOptions
+    /// - Returns: the same OAuth2 instance to allow method chaining


OAuth2 -> WebAuth

All existing documentation in this file references 'OAuth2', I'll rename them all then.

hzalaz · 2016-12-01T01:23:24Z

Auth0/WebAuth.swift

+    ///
+    /// - Parameter response: Array of ResponseOptions
+    /// - Returns: the same OAuth2 instance to allow method chaining
+    func response(_ response: [ResponseOptions]) -> Self


Let's stick with responseType

hzalaz · 2016-12-01T01:23:54Z

Auth0/WebAuthError.swift

@@ -29,12 +29,17 @@ import Foundation
 - CannotDismissWebAuthController: When trying to dismiss WebAuth controller, no presenter controller could be found.
 - UserCancelled:                  User cancelled the web-based authentication, e.g. tapped the "Done" button in SFSafariViewController
 - PKCENotAllowed:                 PKCE for the supplied Auth0 ClientId was not allowed. You need to set the `Token Endpoint Authentication Method` to `None` in your Auth0 Dashboard
+ - noNonceProvided:                A nonce value must be provided to use the response option of id_token


missing the last errors here

These were added.

hzalaz · 2016-12-01T01:25:09Z

Auth0Tests/CredentialsSpec.swift

-                let credentials = Credentials(json: ["access_token": AccessToken])
-                expect(credentials).to(beNil())
-            }
+//            it("should return nil when missing access_token") {


what about these?

ResponseOptions computed label Retired usePKCE, use ResponseType Updated Tests Updated Docsheaders

hzalaz · 2016-12-01T13:01:56Z

Auth0/OAuth2Grant.swift

+
+        // token response expectations
+        if responseType.contains(.token) {
+            guard values["access_token"] != nil && values["token_type"] != nil else {


guard !responseType.contains(.token) || values["access_token"] != nil else {...}

No need to account for tokenType

hzalaz · 2016-12-01T13:03:03Z

Auth0/OAuth2Grant.swift


    func credentials(from values: [String : String], callback: @escaping (Result<Credentials>) -> ()) {
+        // id token reponse expectations and JWT validation, nonce validation
+        if responseType.contains(.id_token) {


Move all logic of nonce validation to he validate method and then you can join guard and if into a guard.

hzalaz · 2016-12-01T13:04:33Z

Auth0/Credentials.swift

    public let idToken: String?
    public let refreshToken: String?

-    required public init(accessToken: String, tokenType: String, idToken: String? = nil, refreshToken: String? = nil) {
+    required public init(accessToken: String? = nil, tokenType: String? = nil, idToken: String? = nil, refreshToken: String? = nil) {
        self.accessToken = accessToken
        self.tokenType = tokenType
        self.idToken = idToken
        self.refreshToken = refreshToken
    }

    convenience required public init?(json: [String: Any]) {


We are no longer returning nil so the init is not required to be a failable one

hzalaz · 2016-12-01T13:06:01Z

Auth0/ResponseOptions.swift

+
+import Foundation
+
+public struct ResponseOptions: OptionSet {


Let's make it an enum so we can avoid the dev adding strange values as response types

Also ResponseType might be more suitable as a name (plus missing docs)

So after originally being an enum and saying to change it to OptionSet, you want to go back to enum again?

Forgot that enum won't be able to conform OptionSet protocol

hzalaz · 2016-12-01T13:06:53Z

Auth0/ResponseOptions.swift

+    }
+
+    public static let token     = ResponseOptions(rawValue: 1 << 0)
+    public static let id_token  = ResponseOptions(rawValue: 1 << 1)


Let's keep with camelCase

hzalaz · 2016-12-01T13:08:07Z

Auth0/WebAuthError.swift

+    case noNonceProvided
+    case missingResponseParam(String)
+    case idTokenValidationFailed
+    case tokenValidationFailed


missingAccessToken

hzalaz · 2016-12-01T13:08:39Z

Auth0/WebAuthError.swift

 */
 public enum WebAuthError: CustomNSError {
    case noBundleIdentifierFound
    case cannotDismissWebAuthController
    case userCancelled
    case pkceNotAllowed(String)
+    case noNonceProvided
+    case missingResponseParam(String)
+    case idTokenValidationFailed


invalidIdTokenNonce

Support id_token

11c8279

Chaining method for response types Auto set implicit grant Credentials support for id_token only Chaining method for nonce Nonce required for id_token response type Tests added

hzalaz suggested changes Nov 29, 2016

View reviewed changes

cocojoe added 3 commits November 30, 2016 13:31

Response type validation logic added

bdc8831

Updated Carthage libs in line with Lock v2 JWTDecode added for nonce check in id_token ResponseType Enum changed to OptionSet Response Error enum added

Added Unit Tests

537923b

Shuffled code around to be inline with builder expectations

Code tidy

5ea5a35

Reduced and merged response errors into webauth errors Separate idtoken validation code

cocojoe force-pushed the response_type_id_token branch from 53153cf to 5ea5a35 Compare November 30, 2016 23:07

hzalaz suggested changes Dec 1, 2016

View reviewed changes

Response renamed ResponseType

e5897c1

ResponseOptions computed label Retired usePKCE, use ResponseType Updated Tests Updated Docsheaders

hzalaz suggested changes Dec 1, 2016

View reviewed changes

cocojoe and others added 3 commits December 1, 2016 15:32

Code review revisions

235f57f

Updated unit tests

f107f1c

Use xcode8.1 image

fb43036

hzalaz approved these changes Dec 2, 2016

View reviewed changes

hzalaz added the CH: Added label Dec 2, 2016

hzalaz changed the title ~~Support id_token~~ Support id_token response type Dec 2, 2016

hzalaz added this to the 1.1.0 milestone Dec 2, 2016

hzalaz merged commit aa0e751 into master Dec 2, 2016

hzalaz deleted the response_type_id_token branch January 29, 2017 14:04


		required public init(accessToken: String, tokenType: String, idToken: String? = nil, refreshToken: String? = nil) {
		required public init(accessToken: String? = nil, tokenType: String? = nil, idToken: String? = nil, refreshToken: String? = nil) {

Support id_token response type #62

Support id_token response type #62

Conversation

cocojoe commented Nov 29, 2016

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

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

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

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 Dec 1, 2016 • 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

cocojoe Dec 1, 2016 •

edited

Loading