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

Add support for new passwordless endpoint #556

Merged
merged 13 commits into from
Dec 7, 2020

Conversation

nbandarchi
Copy link
Contributor

Changes

Fixing PasswordlessAuthenticator.signIn()

  • Changed grant type to http://auth0.com/oauth/grant-type/passwordless/otp
  • Mapping connection to realm and password to otp
  • Changing default type from ro to token in OAuthAuthenticator.signIn()
  • Allowing type to be specified in options

References

Passwordless signin process is attempting to use oauth/ro endpoint which no longer exists:

Testing

  • This change adds unit test coverage
  • This change adds integration test coverage

Checklist

@nbandarchi nbandarchi requested a review from a team as a code owner November 27, 2020 20:26
Copy link
Contributor

@davidpatrick davidpatrick left a comment

Choose a reason for hiding this comment

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

@nbandarchi thanks for raising the PR and addressing this issue. Unfortunately oauth/ro does still exist when a customer has an allow legacy grant type flag enabled on their account. So in order to accept this PR without cutting a major we will need keep the default behavior of the passwordless signIn function.

We can add a deprecation notice to that function and that it will move to legacySignIn on the next major. And in order to handle the functionality of the new endpoint, we can add the otp and realm parameter and when either is detected switch the other parameters to match the new endpoint.

Nick Bandarchi added 3 commits December 3, 2020 10:01
@nbandarchi
Copy link
Contributor Author

Updated so that it's not defaulting to token. if passwordless.signIn() is called with otp and optionally realm it will change the grant type to http://auth0.com/oauth/grant-type/passwordless/otp and pass in token as an option to oauth.signIn(). Realm will default to sms the same way that connection currently does.

Copy link
Contributor

@davidpatrick davidpatrick left a comment

Choose a reason for hiding this comment

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

@nbandarchi thanks for the quick turnaround! Can you please document the otp and realm parameters in the jsdoc area for that function? Also we'll need some new tests in passwordless.tests.js for #signIn to cover the new otp usage.

Thanks 🙏

@nbandarchi
Copy link
Contributor Author

No problem! Added tests and documentation. The tests are just a duplicated and updated copy of the current tests, but still should show that both paths can coexist.

@davidpatrick davidpatrick changed the title Fix passwordless signin Add support for new passwordless authenticate Dec 4, 2020
@davidpatrick davidpatrick changed the title Add support for new passwordless authenticate Add support for new passwordless endpoint Dec 7, 2020
Copy link
Contributor

@davidpatrick davidpatrick left a comment

Choose a reason for hiding this comment

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

Changes are great! Thanks for the work here

@davidpatrick davidpatrick merged commit a21787a into auth0:master Dec 7, 2020
@davidpatrick davidpatrick added this to the v2.31.0 milestone Dec 8, 2020
@davidpatrick davidpatrick mentioned this pull request Dec 8, 2020
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