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

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
92f1465
Add transaction manager to passwordlessLogin
luisrudge Apr 4, 2018
e1c8a4a
Add transaction manager to login
luisrudge Apr 4, 2018
905a6a5
Merge branch 'master' into fix-passwordlessLogin-transaction-manager
luisrudge Apr 6, 2018
f6e2227
adding nonce test
luisrudge Apr 13, 2018
f581f88
fix pr
luisrudge Apr 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 39 additions & 9 deletions src/web-auth/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,13 +628,28 @@ WebAuth.prototype.signupAndAuthorize = 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.login = function(options, cb) {
var params = objectHelper
.merge(this.baseOptions, [
'clientID',
'responseType',
'redirectUri',
'scope',
'audience',
'_csrf',
'state',
'_intstate',
'nonce'
])
.with(options);
params = this.transactionManager.process(params);

var isHostedLoginPage = windowHelper.getWindow().location.host === this.baseOptions.domain;
if (isHostedLoginPage) {
options.connection = options.realm;
delete options.realm;
this._universalLogin.login(options, cb);
params.connection = params.realm;
delete params.realm;
this._universalLogin.login(params, cb);
} else {
this.crossOriginAuthentication.login(options, cb);
this.crossOriginAuthentication.login(params, cb);
}
};

Expand All @@ -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

.merge(this.baseOptions, [
'clientID',
'responseType',
'redirectUri',
'scope',
'audience',
'_csrf',
'state',
'_intstate',
'nonce'
])
.with(options);
params = this.transactionManager.process(params);

var isHostedLoginPage = windowHelper.getWindow().location.host === this.baseOptions.domain;
if (isHostedLoginPage) {
this.passwordlessVerify(options, cb);
this.passwordlessVerify(params, cb);
} else {
var crossOriginOptions = objectHelper.extend(
{
credentialType: 'http://auth0.com/oauth/grant-type/passwordless/otp',
realm: options.connection,
username: options.email || options.phoneNumber,
otp: options.verificationCode
realm: params.connection,
username: params.email || params.phoneNumber,
otp: params.verificationCode
},
objectHelper.blacklist(options, ['connection', 'email', 'phoneNumber', 'verificationCode'])
objectHelper.blacklist(params, ['connection', 'email', 'phoneNumber', 'verificationCode'])
);
this.crossOriginAuthentication.login(crossOriginOptions, cb);
}
Expand Down
47 changes: 40 additions & 7 deletions test/web-auth/web-auth.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

});
});
context('when outside of the universal login page', function() {
Expand Down Expand Up @@ -1782,7 +1783,12 @@ 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?

realm: 'sms',
username: '+55165134',
otp: '123456'
otp: '123456',
clientID: '...',
responseType: 'code',
redirectUri: 'http://page.com/callback',
state: 'randomState',
nonce: 'the-nonce'
};
stub(CrossOriginAuthentication.prototype, 'login', function(options, cb) {
expect(options).to.be.eql(expectedOptions);
Expand All @@ -1806,7 +1812,12 @@ 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

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

responseType: 'code',
redirectUri: 'http://page.com/callback',
state: 'randomState',
nonce: 'the-nonce'
};
stub(CrossOriginAuthentication.prototype, 'login', function(options, cb) {
expect(options).to.be.eql(expectedOptions);
Expand Down Expand Up @@ -1842,9 +1853,14 @@ describe('auth0.WebAuth', function() {
});
it('should call `webauth.passwordlessVerify` with phoneNumber', function(done) {
var expectedOptions = {
clientID: '...',
responseType: 'code',
redirectUri: 'http://page.com/callback',
connection: 'sms',
phoneNumber: '+55165134',
verificationCode: '123456'
verificationCode: '123456',
state: 'randomState',
nonce: 'the-nonce'
};
stub(this.auth0, 'passwordlessVerify', function(options, cb) {
expect(options).to.be.eql(expectedOptions);
Expand All @@ -1865,9 +1881,14 @@ describe('auth0.WebAuth', function() {
});
it('should call `webauth.passwordlessVerify` with email', function(done) {
var expectedOptions = {
clientID: '...',
responseType: 'code',
redirectUri: 'http://page.com/callback',
connection: 'email',
email: 'the@email.com',
verificationCode: '123456'
verificationCode: '123456',
state: 'randomState',
nonce: 'the-nonce'
};
stub(this.auth0, 'passwordlessVerify', function(options, cb) {
expect(options).to.be.eql(expectedOptions);
Expand Down Expand Up @@ -2157,7 +2178,13 @@ describe('auth0.WebAuth', function() {
});

it('should call CrossOriginAuthentication.login', function(done) {
var expectedOptions = { foo: 'bar' };
var expectedOptions = {
clientID: '...',
responseType: 'token',
redirectUri: 'http://page.com/callback',
foo: 'bar',
state: 'randomState'
};
stub(CrossOriginAuthentication.prototype, 'login', function(options, cb) {
expect(options).to.be.eql(expectedOptions);
expect(cb()).to.be('cb');
Expand Down Expand Up @@ -2191,7 +2218,13 @@ describe('auth0.WebAuth', function() {
windowHelper.getWindow.restore();
});
it('calls _hostedPages.login mapping the connection parameter', function(done) {
var expectedOptions = { connection: 'bar' };
var expectedOptions = {
clientID: '...',
responseType: 'token',
redirectUri: 'http://page.com/callback',
state: 'randomState',
connection: 'bar'
};
stub(HostedPages.prototype, 'login', function(options, cb) {
expect(options).to.be.eql(expectedOptions);
expect(cb()).to.be('cb');
Expand Down