Added OIDC MFA Support #528
Conversation
Capture MFA Token Added Route for MFA with Token
cocojoe
force-pushed
the
added-oidc-mfa
branch
from
eb02288 to
0e282bd
Compare
| @@ -118,6 +118,10 @@ class DatabasePresenter: Presentable, Loggable { | |||
| } | |||
| if case CredentialAuthError.multifactorRequired = error { | |||
| self.navigator.navigate(.multifactor) | |||
| return | |||
| } else if case CredentialAuthError.multifactorTokenRequired(let token) = error { | |||
There was a problem hiding this comment.
what's this syntax doing? I'm confused with the let there. Sounds like you're declaring a function instead of calling one
There was a problem hiding this comment.
Traditionally you would need to do a switch statement, this is a quick way to check a single case value, if the case matches and I want to get the value of the param that was passed with multifactorTokenRequired Then I need to extract it from the case. Here were do let token so I can extract the token that was passed and use it inside the if block.
| @@ -72,6 +73,8 @@ func == (lhs: Route, rhs: Route) -> Bool { | |||
| return lhsConnection.name == rhsConnection.name && lhsDomain == rhsDomain | |||
| case (.unrecoverableError(let lhsError), .unrecoverableError(let rhsError)): | |||
| return lhsError == rhsError | |||
| case (.multifactorWithToken(let lhsToken), .multifactorWithToken(let rhsToken)): | |||
| return lhsToken == rhsToken | |||
There was a problem hiding this comment.
what is this naming? lhs / rhs
There was a problem hiding this comment.
left hand side, right hand side.
| @@ -690,13 +691,13 @@ class DatabaseInteractorSpec: QuickSpec { | |||
| } | |||
| } | |||
|
|
|||
| it("should indicate that mfa is required") { | |||
There was a problem hiding this comment.
isn't this still the case for non-oidc clients? This test should be kept
There was a problem hiding this comment.
yes the description just changed and in OIDC it will always be the with token path.
| } | ||
| } | ||
|
|
||
| it("should notify when code is invalid") { |
There was a problem hiding this comment.
code or token? because you're passing "invalid token" in mfaToken
| } | ||
|
|
||
| it("should yield error on failure") { | ||
| interactor = MultifactorInteractor(user: user, authentication: Auth0.authentication(clientId: clientId, domain: domain), connection: connection, options: options, dispatcher: dispatcher, mfaToken: nil) |
There was a problem hiding this comment.
what is the precondition for this test to fail?
There was a problem hiding this comment.
I'll update description, if token is not present,
| } | ||
|
|
||
| it("should yield error when input is not valid") { | ||
| waitUntil(timeout: 2) { done in |
There was a problem hiding this comment.
what is the precondition for this test to fail?
| @@ -658,6 +666,15 @@ class DatabasePresenterSpec: QuickSpec { | |||
| view.primaryButton?.onPress(view.primaryButton!) | |||
| expect(navigator.route).toEventually(equal(Route.multifactor)) | |||
| } | |||
|
|
|||
| it("should navigate to multifactor token required screen") { | |||
There was a problem hiding this comment.
this looks like a dupe of a test added 200 lines above, but the test name is not exactly the same. That aside, why is this added twice? (file too long to check 🚎 )
There was a problem hiding this comment.
It is but it's part of the sign up logic flow that will try to login after sign up.
| var json = ["error": code, "error_description": description] | ||
| json["name"] = name | ||
| json += jsonExtra |
There was a problem hiding this comment.
this is overwriting the json map with the keys in jsonExtra, right?
Changes
This PR adds support for the OIDC MFA endpoint.
https://github.com/auth0/Auth0.swift/blob/master/Auth0/Auth0Authentication.swift#L75
References
Internal 282
Testing
Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.
Checklist