Skip to content

Commit

Permalink
feat: passport strategy automatically checks response REQUIRED params
Browse files Browse the repository at this point in the history
  • Loading branch information
@panva
panva committed Aug 11, 2018
1 parent 6e0ac57 commit 902eeed
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 19 deletions.
24 changes: 14 additions & 10 deletions lib/passport_strategy.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ OpenIDConnectStrategy.prototype.authenticate = function authenticate(req, option
params.nonce = uuid();
}

req.session[sessionKey] = _.pick(params, 'nonce', 'state', 'max_age');
req.session[sessionKey] = _.pick(params, 'nonce', 'state', 'max_age', 'response_type');

if (this._usePKCE) {
const verifier = uuid();
Expand All @@ -111,11 +111,19 @@ OpenIDConnectStrategy.prototype.authenticate = function authenticate(req, option
/* end authentication request */

/* start authentication response */

const session = req.session[sessionKey];
const state = _.get(session, 'state');
const maxAge = _.get(session, 'max_age');
const nonce = _.get(session, 'nonce');
const codeVerifier = _.get(session, 'code_verifier');
if (_.isEmpty(session)) {
this.error(new Error(util.format(
`did not find expected authorization request details in session, req.session["${sessionKey}"] is %j`,
session
)));
return;
}

const {
state, nonce, max_age: maxAge, code_verifier: codeVerifier, response_type: responseType,
} = session;

try {
delete req.session[sessionKey];
Expand All @@ -130,6 +138,7 @@ OpenIDConnectStrategy.prototype.authenticate = function authenticate(req, option
nonce,
max_age: maxAge,
code_verifier: codeVerifier,
response_type: responseType,
};

let callback = client.authorizationCallback(opts.redirect_uri, reqParams, checks)
Expand Down Expand Up @@ -167,11 +176,6 @@ OpenIDConnectStrategy.prototype.authenticate = function authenticate(req, option
&& error.error !== 'server_error'
&& !error.error.startsWith('invalid')) {
this.fail(error);
} else if (error.message === 'state mismatch' && !state) {
this.error(new Error(util.format(
'state mismatch, could not find a state in the session, this is likely an environment setup issue, loaded session: %j',
session
)));
} else {
this.error(error);
}
Expand Down
41 changes: 32 additions & 9 deletions test/passport/passport_strategy.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ const { Issuer, Strategy } = require('../../lib');
expect(target).to.include('redirect_uri=');
expect(target).to.include('scope=');
expect(req.session).to.have.property('oidc:op.example.com');
expect(req.session['oidc:op.example.com']).to.have.keys('state');
expect(req.session['oidc:op.example.com']).to.have.keys('state', 'response_type');
});

it('starts authentication requests for POSTs', function () {
Expand All @@ -89,7 +89,7 @@ const { Issuer, Strategy } = require('../../lib');
expect(target).to.include('redirect_uri=');
expect(target).to.include('scope=');
expect(req.session).to.have.property('oidc:op.example.com');
expect(req.session['oidc:op.example.com']).to.have.keys('state');
expect(req.session['oidc:op.example.com']).to.have.keys('state', 'response_type');
});

it('can have redirect_uri and scope specified', function () {
Expand Down Expand Up @@ -135,7 +135,7 @@ const { Issuer, Strategy } = require('../../lib');
expect(target).to.include('nonce=');
expect(target).to.include('response_mode=form_post');
expect(req.session).to.have.property('oidc:op.example.com');
expect(req.session['oidc:op.example.com']).to.have.keys('state', 'nonce');
expect(req.session['oidc:op.example.com']).to.have.keys('state', 'nonce', 'response_type');
});

describe('use pkce', () => {
Expand Down Expand Up @@ -244,7 +244,7 @@ const { Issuer, Strategy } = require('../../lib');
strategy.authenticate(req);

expect(req.session).to.have.property('oidc:op.example.com:foo');
expect(req.session['oidc:op.example.com:foo']).to.have.keys('state');
expect(req.session['oidc:op.example.com:foo']).to.have.keys('state', 'response_type');
});
});

Expand All @@ -267,6 +267,7 @@ const { Issuer, Strategy } = require('../../lib');
'oidc:op.example.com': {
nonce: 'nonce',
state: 'state',
response_type: 'code',
},
};

Expand All @@ -276,8 +277,14 @@ const { Issuer, Strategy } = require('../../lib');
it('triggers the error function when server_error is encountered', function (next) {
const strategy = new Strategy({ client: this.client }, () => {});

const req = new MockRequest('GET', '/login/oidc/callback?error=server_error');
req.session = {};
const req = new MockRequest('GET', '/login/oidc/callback?error=server_error&state=state');
req.session = {
'oidc:op.example.com': {
nonce: 'nonce',
state: 'state',
response_type: 'code',
},
};

strategy.error = (error) => {
try {
Expand All @@ -299,7 +306,7 @@ const { Issuer, Strategy } = require('../../lib');

strategy.error = (error) => {
try {
expect(error.message).to.match(/^state mismatch, could not find a state in the session/);
expect(error.message).to.eql('did not find expected authorization request details in session, req.session["oidc:op.example.com"] is undefined');
next();
} catch (err) {
next(err);
Expand All @@ -316,8 +323,14 @@ const { Issuer, Strategy } = require('../../lib');
return Promise.reject(new Error('callback error'));
});

const req = new MockRequest('GET', '/login/oidc/callback?code=code');
req.session = {};
const req = new MockRequest('GET', '/login/oidc/callback?code=code&state=state');
req.session = {
'oidc:op.example.com': {
nonce: 'nonce',
state: 'state',
response_type: 'code',
},
};

strategy.error = (error) => {
try {
Expand All @@ -337,7 +350,9 @@ const { Issuer, Strategy } = require('../../lib');
const req = new MockRequest('GET', '/login/oidc/callback?error=login_required&state=state');
req.session = {
'oidc:op.example.com': {
nonce: 'nonce',
state: 'state',
response_type: 'code',
},
};

Expand Down Expand Up @@ -366,7 +381,9 @@ const { Issuer, Strategy } = require('../../lib');
const req = new MockRequest('GET', '/login/oidc/callback?code=foo&state=state');
req.session = {
'oidc:op.example.com': {
nonce: 'nonce',
state: 'state',
response_type: 'code',
},
};

Expand Down Expand Up @@ -396,6 +413,7 @@ const { Issuer, Strategy } = require('../../lib');
req.session = {
'oidc:op.example.com': {
nonce: 'nonce',
response_type: 'code',
state: 'state',
},
};
Expand Down Expand Up @@ -431,6 +449,7 @@ const { Issuer, Strategy } = require('../../lib');
req.session = {
'oidc:op.example.com': {
nonce: 'nonce',
response_type: 'code',
state: 'state',
},
};
Expand Down Expand Up @@ -462,6 +481,7 @@ const { Issuer, Strategy } = require('../../lib');
req.session = {
'oidc:op.example.com': {
nonce: 'nonce',
response_type: 'code',
state: 'state',
},
};
Expand Down Expand Up @@ -496,6 +516,7 @@ const { Issuer, Strategy } = require('../../lib');
req.session = {
'oidc:op.example.com': {
nonce: 'nonce',
response_type: 'code',
state: 'state',
},
};
Expand Down Expand Up @@ -535,6 +556,7 @@ const { Issuer, Strategy } = require('../../lib');
req.session = {
'oidc:op.example.com': {
nonce: 'nonce',
response_type: 'code',
state: 'state',
},
};
Expand Down Expand Up @@ -570,6 +592,7 @@ const { Issuer, Strategy } = require('../../lib');
req.session = {
'oidc:op.example.com': {
nonce: 'nonce',
response_type: 'code',
state: 'state',
},
};
Expand Down

0 comments on commit 902eeed

Please sign in to comment.