Skip to content
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

Enable MFA support for OIDC conformant clients #451

Merged
merged 1 commit into from
Jul 19, 2018
Merged

Conversation

lbalmaceda
Copy link
Contributor

@lbalmaceda lbalmaceda commented Feb 20, 2018

OIDC conformant clients were not able to authenticate after an mfa challenge was requested. This PR adds support for them. I also enhanced the errors that we display in the widget.

For legacy mode:

  • Requiring MFA (when already enrolled) will take the user to the "input MFA code" form.
  • Requiring enrollment will now keep the user in the "username / password" form instead of taking them to the "input MFA code" form from which they wouldn't be able to do anything, as enrollment is not supported currently from Lock. Users will see a "THE USER HAS NOT ENROLLED MULTIFACTOR AUTHENTICATION" message.
  • An invalid/expired OTP value will show a "THE CODE IS INVALID OR HAS EXPIRED" message.

For OIDC mode:

  • Requiring MFA (doesn't care if the user is enrolled) will take the user to the "input MFA code" form.
  • Requiring enrollment will only be known once the user attempts to send the OTP code, so they are already on this form. This might change once the association_required error is introduced to the API. Users will see a "THE USER HAS NOT ENROLLED MULTIFACTOR AUTHENTICATION" message.
  • Invalid OTP will show a "THE CODE IS INVALID OR HAS EXPIRED" message.
  • mfa_token lasts 10 minutes. If it expires, the user needs to retry the authentication from the beginning. So an expired MFA token will take the user back to the "username / password" form and display a "THE CODE IS INVALID OR HAS EXPIRED" message.

General notes about this PR:

- It's based on a still not released library, so tests will fail 💃 -> auth0/Auth0.Android#146

  • Legacy MFA flow required a 2nd call to /ro sending again the email/username, password and mfa_code (the OTP) values.
  • The new flow requires a call to /oauth/token first with the password grant, which will fail due to "mfa_required" and will include an mfa_token value. A second call is required on /oauth/token this time with mfa-otp grant and the previous mfa_token value along with the otp code given by the user obtained from the 2FA/MFA app.
  • Method names and default english message resources are subject to review and change

@lbalmaceda lbalmaceda added this to the v2-Next milestone Feb 20, 2018
@lbalmaceda lbalmaceda requested a review from cocojoe February 20, 2018 21:15
@cocojoe
Copy link
Member

cocojoe commented Jul 19, 2018

"It's based on a still not released library, so tests will fail 💃" ? This old I presume as your tests all pass.

@cocojoe
Copy link
Member

cocojoe commented Jul 19, 2018

Also noticed no CodeCov in this repo?

@@ -169,6 +170,38 @@ public void shouldCallLegacyDatabaseLoginWithVerificationCode() throws Exception
assertThat(reqParams, hasEntry("mfa_code", "123456"));
}

@Test
public void shouldCallOIDCDatabaseLoginWithOTPCodeAndMFAToken() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one test? What about Login with OTP but no token etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those cases are already tested on the auth0.android SDK. There's no scenario where OTP is defined but the Token is not. If you're looking for the "legacy mfa" behavior check the test right above this one. https://github.com/auth0/Lock.Android/pull/451/files#diff-d3f9a8a9ba90477d9b0ffd039c4faf4cR152

Copy link
Member

@cocojoe cocojoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then LGTM

@lbalmaceda lbalmaceda merged commit 91d8fd7 into master Jul 19, 2018
@lbalmaceda lbalmaceda modified the milestones: v2-Next, 2.8.5 Jul 19, 2018
@lbalmaceda lbalmaceda deleted the use-mfa-oidc branch May 4, 2021 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants