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.

Sign up for GitHub

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

Jump to bottom

Add transaction manager to passwordlessLogin and login #731

Merged
merged 5 commits into from
Apr 16, 2018
Merged

Add transaction manager to passwordlessLogin and login #731

merged 5 commits into from
Apr 16, 2018

Conversation

luisrudge
Copy link
Contributor

@luisrudge luisrudge commented Apr 4, 2018 •
edited
Loading

This fixes an issue that state/nonce were not being auto generated in popup mode.
fix #730

@luisrudge luisrudge changed the title Add transaction manager to passwordlessLogin Add transaction manager to passwordlessLogin and login Apr 4, 2018
@luisrudge luisrudge mentioned this pull request Apr 4, 2018
Closed
@Neaox Neaox mentioned this pull request Apr 4, 2018
Closed
@lbalmaceda lbalmaceda self-requested a review April 6, 2018 19:19
@Neaox
Copy link

Neaox commented Apr 12, 2018

Any updates as to when this might be released?

@luisrudge
Copy link
Contributor Author

We still have a few outstanding issues we want in the next release. It's either tomorrow or next week.

lbalmaceda
lbalmaceda reviewed Apr 13, 2018
View reviewed changes
src/web-auth/index.js
@@ -651,18 +666,33 @@ WebAuth.prototype.login = function(options, cb) {
* @param {crossOriginLoginCallback} cb Callback function called only when an authentication error, like invalid username or password, occurs. For other types of errors, there will be a redirect to the `redirectUri`.
*/
WebAuth.prototype.passwordlessLogin = function(options, cb) {
var params = objectHelper
Copy link
Contributor

Choose a reason for hiding this comment

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

You are doing the exact same thing on the WebAuth.prototype.login method. Maintaining this would be a PITA. Why not moving this to a helper method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use that a bunch of different places, so I feel this should be tackled in a bigger refactor on how we handle transactions. Tracking: #740

test/web-auth/web-auth.test.js
@@ -1782,7 +1782,11 @@ describe('auth0.WebAuth', function() {
credentialType: 'http://auth0.com/oauth/grant-type/passwordless/otp',
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't seem to find this http://auth0.com/oauth/grant-type/passwordless/otp value anywhere in docs. Can you please point me to a doc where this is mentioned?

test/web-auth/web-auth.test.js
@@ -1806,7 +1810,11 @@ describe('auth0.WebAuth', function() {
credentialType: 'http://auth0.com/oauth/grant-type/passwordless/otp',
realm: 'email',
username: 'the@email.com',
otp: '123456'
otp: '123456',
clientID: '...',
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading the PR description, what was missing originally was the state/nonce generated values.

  1. Why are clientID, responseType, redirectUri passed now then? If they are NOT required I see no point on passing them down.
  2. Like I mentioned, the PR talks about state and nonce, but tests only check that state is being set. You might want to expect a nonce=randomNonce somewhere too, right?

Copy link
Contributor Author

@luisrudge luisrudge Apr 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We allow people to override most of the options in all most of the methods, that's why I added all the options that you can pass to authorize, except responseMode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright with (1). Solve (2) and I'll approve.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

luisrudge
luisrudge commented Apr 13, 2018
View reviewed changes
test/web-auth/web-auth.test.js
@@ -1806,7 +1810,11 @@ describe('auth0.WebAuth', function() {
credentialType: 'http://auth0.com/oauth/grant-type/passwordless/otp',
realm: 'email',
username: 'the@email.com',
otp: '123456'
otp: '123456',
clientID: '...',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

test/web-auth/web-auth.test.js
@@ -1806,7 +1810,11 @@ describe('auth0.WebAuth', function() {
credentialType: 'http://auth0.com/oauth/grant-type/passwordless/otp',
realm: 'email',
username: 'the@email.com',
otp: '123456'
otp: '123456',
clientID: '...',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

lbalmaceda
lbalmaceda reviewed Apr 13, 2018
View reviewed changes
test/web-auth/web-auth.test.js Outdated
@@ -1754,7 +1754,8 @@ describe('auth0.WebAuth', function() {
clientID: '...',
redirectUri: 'http://page.com/callback',
responseType: 'code',
_sendTelemetry: false
_sendTelemetry: false,
nonce: 'the-nonce'
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't be setting the nonce value yourself. The PR description clearly states that what was broken is "state/nonce were not being auto generated". Please DO check that the nonce is present but don't manually set it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line is already doing some of that

auth0.js/test/web-auth/web-auth.test.js

Line 27 in f6e2227

return { state: state || 'randomState', nonce: nonce || 'randomNonce' };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right. I switched to id_token since it's the only case where it generates the nonce.

lbalmaceda
lbalmaceda approved these changes Apr 16, 2018
View reviewed changes
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

👍

@luisrudge luisrudge merged commit 9be008d into master Apr 16, 2018
@luisrudge luisrudge deleted the fix-passwordlessLogin-transaction-manager branch April 16, 2018 17:33
@luisrudge luisrudge added this to the v9.5.0 milestone Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lbalmaceda lbalmaceda lbalmaceda approved these changes

Assignees
No one assigned
Labels
CH: Added
Projects
None yet
Milestone
v9.5.0
Development

Successfully merging this pull request may close these issues.

validateAuthenticationResponse null transaction state, undefined response state
3 participants
@luisrudge @Neaox @lbalmaceda