-
Notifications
You must be signed in to change notification settings - Fork 232
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.
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
Added OIDC MFA EndPoint #189
Conversation
Need to add a few more tests |
Public API Addition - login(withOTP otp: String, mfaToken: String) Added MFA Errors Public API New Error - isMultifactorTokenInvalid Added Error Tests Added Endpoint Tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would this "new login type" method be called? Since I see you already have methods named login
. Can you please share 2 calls in a snippet?
@@ -67,6 +68,17 @@ struct Auth0Authentication: Authentication { | |||
return Request(session: session, url: resourceOwner, method: "POST", handle: authenticationObject, payload: payload, logger: self.logger, telemetry: self.telemetry) | |||
} | |||
|
|||
func login(withOTP otp: String, mfaToken: String) -> Request<Credentials, AuthenticationError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
android has the parameters in the opposite order. Would you be open to swap them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH this was commented in your Android PR, if you say loginWithOTP
you would expect that the otp
would be the first parameter. auth0/Auth0.Android#146 (comment)
Auth0/Authentication.swift
Outdated
|
||
``` | ||
Auth0. | ||
.authentication(clientId: clientId, domain: "samples.auth0.com") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing indentation
Auth0/Authentication.swift
Outdated
} | ||
``` | ||
|
||
- parameter otp: One time password supplied by MFA Authenticator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the method below adds indentation after the :
. Should this do the same?
@@ -275,6 +320,7 @@ class AuthenticationErrorSpecSharedExamplesConfiguration: QuickConfiguration { | |||
it("should not match any known error") { | |||
expect(error.isMultifactorCodeInvalid).to(beFalse(), description: "should not match mfa invalid") | |||
expect(error.isMultifactorEnrollRequired).to(beFalse(), description: "should not match mfa enroll") | |||
expect(error.isMultifactorCodeInvalid).to(beFalse(), description: "should not match code invalid") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about isMultifactorTokenInvalid
?
} | ||
} | ||
|
||
it("should fail login with bad otp") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one is a dupe of the above (i can't spot any diff)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have replaced with invalid mfa token test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -13,7 +13,7 @@ | |||
// all copies or substantial portions of the Software. | |||
// | |||
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | |||
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | |||
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,u |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
u what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is my keyboard, it has issues :(
expect(error.isMultifactorTokenInvalid) == true | ||
} | ||
|
||
it("should detect invalid malformed mfa token") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should detect invalid malformed mfa token -> should detect malformed mfa token
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🌮
Public API
Methods Added:
login(withOTP otp: String, mfaToken: String)
Errors Added
isMultifactorTokenInvalid
Tests
Added Error Tests
Added Endpoint Tests