Add support for MFA using OIDC conformant endpoints #146
Conversation
lbalmaceda
force-pushed
the
update-mfa
branch
from
3df6d64
to
265b6ae
Compare
|
Can you list Public API changes/additions in the first post please. If you are not convinced of throwing an exception why do it? What is the best experience for the end user who may experience this scenario? |
| * @return a request to configure and start that will yield {@link Credentials} | ||
| */ | ||
| @SuppressWarnings("WeakerAccess") | ||
| public AuthenticationRequest loginWithOTP(@NonNull String mfaToken, @NonNull String otp) { |
There was a problem hiding this comment.
I can't think of a better name right now but it would be good to be consistent on platforms, In swift the counterpart might be defined as login(withOTP:
There was a problem hiding this comment.
Ideally we can have a single "login" method that just sets the client_id and the path /oauth/token. But then users will need to know for each grant type what parameters are accepted. That's the reason we now have a new login "with otp" method that helps to construct this request.
So in swift what are you doing, is that "withOTP" like a flag?
There was a problem hiding this comment.
No, so in Swift there are 2 login methods one is /ro the other grant password-realm. In this case if I had login(withOTP otp: String, mfaToken: String)
The auto-complete would show 3 logins and it's based on the params, in Swift the guideline is to create methods that have meaninful names. You could have login(otp: String, token: String) which isn't as informative.
When the user calls this method they would write login(withOTP: "12356", mfaToken: "blah") the otp is used inside the method.
| } | ||
|
|
||
| /// When the MFA Token received on the login request has expired | ||
| public boolean isMultifactorTokenExpired() { |
There was a problem hiding this comment.
Why not the more generic isMultifactorTokenInvalid as an expired token is invalid, does it matter how? You also have {"error":"invalid_grant","error_description":"Malformed mfa_token"}
There was a problem hiding this comment.
I didn't catch that error response, probably because I tested it on Lock directly and the token is handled from one endpoint to the other as it is. Yeah it makes sense to handle both cases under the same method, and I agree that in that case it should be named like that -> Invalid.
|
@lbalmaceda Should we mention that you need to enable the |
|
@cocojoe Totally. I've updated the PR adding those changes. |
| } | ||
|
|
||
| /// When MFA is required and the user is not enrolled | ||
| public boolean isMultifactorEnrollRequired() { | ||
| return "a0.mfa_registration_required".equals(code); | ||
| return "a0.mfa_registration_required".equals(code) || "unsupported_challenge_type".equals(code); |
There was a problem hiding this comment.
The error is the specific challenge type is not expected vs enrolment in general. Should we clarify this? As it could be they don't support OTP but do OOB?
There was a problem hiding this comment.
association_required will be the new not-enrolled error code once they release the new API, currently this is unsupported_challenge_type. On the other side, "unsupported_challenge_type" does mean that the challenge is not supported. Because we're not going to support other challenge type than OTP, I don't think that's an error we can provide an alternate path to the user to continue the authentication. I tried to find without success, but I think somewhere in the docs they mention that OTP must be supported at least.
…all MFA endpoints
lbalmaceda
force-pushed
the
update-mfa
branch
2 times, most recently
from
7850d36
to
fdc2bd9
Compare
|
@lbalmaceda what's codecov complaining about here? Overall project looks fine. |
|
Can probably just lower the patch threshold to say 50%, the project is the most valuable one for me. |
This PR adds support for the new MFA OTP flow which is mainly based on https://github.com/jaredhanson/draft-oauth-mfa/blob/master/Draft-1.0.txt.
It's still missing theIt also adds javadoc fixes.association_requirederror handling (we're blocked) for when the user has not yet enrolled.The only thing I'm not convinced of is throwing an exception when the new "login with otp" method is called on a non-oidc conformant client. Maybe it's better if we let the request to the server fail and return that error response to the user, WDYT?
Also to be reviewed: Name of the login method, currently
loginWithOTP(mfaToken, otp).Public API changes/additions:
loginWithOTP(@NonNull String mfaToken, @NonNull String otp)to log in sending the mfa-otp grant and the mfa token + otp.isMultifactorTokenInvalid()to evaluate if the error instance corresponds to a "mfa token expired" error.EDIT: 04/24/18. The
association_requirederror code is meant for the challenge endpoint only, which this SDK is not going to implement.