Skip to content

Commit

Permalink
feat: passport strategy will now use PKCE by default where applicable
Browse files Browse the repository at this point in the history 
BREAKING CHANGE: PKCE is now used by default in the passport strategy
  • Loading branch information
@panva
panva committed Sep 8, 2020
1 parent 4cb21a4 commit 56f9fe7
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 27 deletions.
27 changes: 14 additions & 13 deletions lib/passport_strategy.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ function OpenIDConnectStrategy({
params = {},
passReqToCallback = false,
sessionKey,
usePKCE = false,
usePKCE,
} = {}, verify) {
if (!(client instanceof BaseClient)) {
throw new TypeError('client must be an instance of openid-client Client');
Expand All @@ -51,27 +51,28 @@ function OpenIDConnectStrategy({
this._key = sessionKey || `oidc:${url.parse(this._issuer.issuer).hostname}`;
this._params = cloneDeep(params);

if (this._usePKCE === true) {
const supportedMethods = this._issuer.code_challenge_methods_supported;
if (!Array.isArray(supportedMethods)) {
throw new TypeError('code_challenge_methods_supported is not properly set on issuer');
}
if (supportedMethods.includes('S256')) {
if (!this._params.response_type) this._params.response_type = resolveResponseType.call(client);
if (!this._params.redirect_uri) this._params.redirect_uri = resolveRedirectUri.call(client);
if (!this._params.scope) this._params.scope = 'openid';

if (this._usePKCE === true || (typeof this._usePKCE === 'undefined' && this._params.response_type.includes('code'))) {
const supportedMethods = Array.isArray(this._issuer.code_challenge_methods_supported)
? this._issuer.code_challenge_methods_supported : false;

if (supportedMethods && supportedMethods.includes('S256')) {
this._usePKCE = 'S256';
} else if (supportedMethods.includes('plain')) {
} else if (supportedMethods && supportedMethods.includes('plain')) {
this._usePKCE = 'plain';
} else if (supportedMethods) {
throw new TypeError('neither code_challenge_method supported by the client is supported by the issuer');
} else {
throw new TypeError('neither supported code_challenge_method is supported by the issuer');
this._usePKCE = 'S256';
}
} else if (typeof this._usePKCE === 'string' && !['plain', 'S256'].includes(this._usePKCE)) {
throw new TypeError(`${this._usePKCE} is not valid/implemented PKCE code_challenge_method`);
}

this.name = url.parse(client.issuer.issuer).hostname;

if (!this._params.response_type) this._params.response_type = resolveResponseType.call(client);
if (!this._params.redirect_uri) this._params.redirect_uri = resolveRedirectUri.call(client);
if (!this._params.scope) this._params.scope = 'openid';
}

OpenIDConnectStrategy.prototype.authenticate = function authenticate(req, options) {
Expand Down
27 changes: 13 additions & 14 deletions test/passport/passport_strategy.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ describe('OpenIDConnectStrategy', () => {
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', 'response_type');
expect(req.session['oidc:op.example.com']).to.have.keys('state', 'response_type', 'code_verifier');
});

it('starts authentication requests for POSTs', function () {
Expand All @@ -104,7 +104,7 @@ describe('OpenIDConnectStrategy', () => {
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', 'response_type');
expect(req.session['oidc:op.example.com']).to.have.keys('state', 'response_type', 'code_verifier');
});

it('can have redirect_uri and scope specified', function () {
Expand Down Expand Up @@ -170,33 +170,32 @@ describe('OpenIDConnectStrategy', () => {
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', 'response_type');
expect(req.session['oidc:op.example.com']).to.have.keys('state', 'nonce', 'response_type', 'code_verifier');
});

describe('use pkce', () => {
it('can be set to use PKCE with boolean', function () {
instance(this.issuer).get('metadata').set('code_challenge_methods_supported', ['S256', 'plain']);
const s256 = new Strategy({ // eslint-disable-line no-new
const s256 = new Strategy({
client: this.client,
usePKCE: true,
}, () => {});
expect(s256).to.have.property('_usePKCE', 'S256');

instance(this.issuer).get('metadata').set('code_challenge_methods_supported', ['plain']);
const plain = new Strategy({ // eslint-disable-line no-new
const plain = new Strategy({
client: this.client,
usePKCE: true,
}, () => {});
expect(plain).to.have.property('_usePKCE', 'plain');

['foobar', undefined, false].forEach((invalidDiscoveryValue) => {
instance(this.issuer).get('metadata').set('code_challenge_methods_supported', invalidDiscoveryValue);
expect(() => {
new Strategy({ // eslint-disable-line no-new
client: this.client,
usePKCE: true,
}, () => {});
}).to.throw('code_challenge_methods_supported is not properly set on issuer');
const defaultS256 = new Strategy({
client: this.client,
usePKCE: true,
}, () => {});
expect(defaultS256).to.have.property('_usePKCE', 'S256');
});

instance(this.issuer).get('metadata').set('code_challenge_methods_supported', []);
Expand All @@ -205,15 +204,15 @@ describe('OpenIDConnectStrategy', () => {
client: this.client,
usePKCE: true,
}, () => {});
}).to.throw('neither supported code_challenge_method is supported by the issuer');
}).to.throw('neither code_challenge_method supported by the client is supported by the issuer');

instance(this.issuer).get('metadata').set('code_challenge_methods_supported', ['not supported']);
expect(() => {
new Strategy({ // eslint-disable-line no-new
client: this.client,
usePKCE: true,
}, () => {});
}).to.throw('neither supported code_challenge_method is supported by the issuer');
}).to.throw('neither code_challenge_method supported by the client is supported by the issuer');
});

it('will throw when explictly provided value is not supported', function () {
Expand Down Expand Up @@ -279,7 +278,7 @@ describe('OpenIDConnectStrategy', () => {
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', 'response_type');
expect(req.session['oidc:op.example.com:foo']).to.have.keys('state', 'response_type', 'code_verifier');
});
});

Expand Down

0 comments on commit 56f9fe7

Please sign in to comment.