From 8de145fa3a4c19d3d0a15588e25b313689000bc4 Mon Sep 17 00:00:00 2001 From: visvk Date: Sat, 28 Oct 2017 23:00:51 +0200 Subject: [PATCH 1/2] PKCE - implementation --- lib/grant-types/abstract-grant-type.js | 1 + .../authorization-code-grant-type.js | 31 +++++ lib/handlers/authorize-handler.js | 63 ++++++++-- lib/handlers/token-handler.js | 15 ++- lib/server.js | 6 +- lib/utils/string-util.js | 14 +++ package.json | 33 +++-- .../authorization-code-grant-type_test.js | 106 ++++++++++++++++ .../handlers/authorize-handler_test.js | 116 ++++++++++++++++++ .../handlers/token-handler_test.js | 75 ++++++++++- test/unit/handlers/authorize-handler_test.js | 4 +- 11 files changed, 439 insertions(+), 25 deletions(-) create mode 100644 lib/utils/string-util.js diff --git a/lib/grant-types/abstract-grant-type.js b/lib/grant-types/abstract-grant-type.js index be4259dec..3e78418f8 100644 --- a/lib/grant-types/abstract-grant-type.js +++ b/lib/grant-types/abstract-grant-type.js @@ -30,6 +30,7 @@ function AbstractGrantType(options) { this.model = options.model; this.refreshTokenLifetime = options.refreshTokenLifetime; this.alwaysIssueNewRefreshToken = options.alwaysIssueNewRefreshToken; + this.PKCEEnabled = options.PKCEEnabled; } /** diff --git a/lib/grant-types/authorization-code-grant-type.js b/lib/grant-types/authorization-code-grant-type.js index 7eae70f8f..6f302918b 100644 --- a/lib/grant-types/authorization-code-grant-type.js +++ b/lib/grant-types/authorization-code-grant-type.js @@ -9,10 +9,12 @@ var InvalidArgumentError = require('../errors/invalid-argument-error'); var InvalidGrantError = require('../errors/invalid-grant-error'); var InvalidRequestError = require('../errors/invalid-request-error'); var Promise = require('bluebird'); +var crypto = require('crypto'); var promisify = require('promisify-any').use(Promise); var ServerError = require('../errors/server-error'); var is = require('../validator/is'); var util = require('util'); +var stringUtil = require('../utils/string-util'); /** * Constructor. @@ -88,7 +90,9 @@ AuthorizationCodeGrantType.prototype.getAuthorizationCode = function(request, cl if (!is.vschar(request.body.code)) { throw new InvalidRequestError('Invalid parameter: `code`'); } + return promisify(this.model.getAuthorizationCode, 1).call(this.model, request.body.code) + .bind(this) .then(function(code) { if (!code) { throw new InvalidGrantError('Invalid grant: authorization code is invalid'); @@ -118,6 +122,33 @@ AuthorizationCodeGrantType.prototype.getAuthorizationCode = function(request, cl throw new InvalidGrantError('Invalid grant: `redirect_uri` is not a valid URI'); } + if (this.PKCEEnabled && client.isPublic) { + if (!code.codeChallenge) { + throw new InvalidGrantError('Invalid grant: code challenge is invalid'); + } + + /** + * The "code_challenge_method" is bound to the Authorization Code when + * the Authorization Code is issued. That is the method that the token + * endpoint MUST use to verify the "code_verifier". + */ + if (!code.codeChallengeMethod) { + throw new ServerError('Server error: `getAuthorizationCode()` did not return a `codeChallengeMethod` property'); + } + + if (code.codeChallengeMethod === 'plain' && code.codeChallenge !== request.body.code_verifier) { + throw new InvalidGrantError('Invalid grant: code verifier is invalid'); + } + + if (code.codeChallengeMethod === 'S256') { + var hash = stringUtil.base64URLEncode(crypto.createHash('sha256').update(request.body.code_verifier).digest()); + + if (code.codeChallenge !== hash) { + throw new InvalidGrantError('Invalid grant: code verifier is invalid'); + } + } + } + return code; }); }; diff --git a/lib/handlers/authorize-handler.js b/lib/handlers/authorize-handler.js index 984136a8d..6a40fa674 100644 --- a/lib/handlers/authorize-handler.js +++ b/lib/handlers/authorize-handler.js @@ -62,6 +62,7 @@ function AuthorizeHandler(options) { this.allowEmptyState = options.allowEmptyState; this.authenticateHandler = options.authenticateHandler || new AuthenticateHandler(options); this.authorizationCodeLifetime = options.authorizationCodeLifetime; + this.PKCEEnabled = options.PKCEEnabled; this.model = options.model; } @@ -95,18 +96,25 @@ AuthorizeHandler.prototype.handle = function(request, response) { var scope; var state; var ResponseType; + var codeChallenge; + var codeChallengeMethod; return Promise.bind(this) - .then(function() { + .then(function() { scope = this.getScope(request); + codeChallenge = this.getCodeChallenge(request, client); - return this.generateAuthorizationCode(client, user, scope); - }) + if (codeChallenge) { + codeChallengeMethod = this.getCodeChallengeMethod(request); + } + + return this.generateAuthorizationCode(client, user, scope); + }) .then(function(authorizationCode) { state = this.getState(request); ResponseType = this.getResponseType(request); - return this.saveAuthorizationCode(authorizationCode, expiresAt, scope, client, uri, user); + return this.saveAuthorizationCode(authorizationCode, expiresAt, scope, client, uri, user, codeChallenge, codeChallengeMethod); }) .then(function(code) { var responseType = new ResponseType(code.authorizationCode); @@ -135,11 +143,39 @@ AuthorizeHandler.prototype.handle = function(request, response) { AuthorizeHandler.prototype.generateAuthorizationCode = function(client, user, scope) { if (this.model.generateAuthorizationCode) { - return promisify(this.model.generateAuthorizationCode).call(this.model, client, user, scope); + return promisify(this.model.generateAuthorizationCode, 3).call(this.model, client, user, scope); } return tokenUtil.generateRandomToken(); }; +AuthorizeHandler.prototype.getCodeChallenge = function(request, client) { + var codeChallenge = request.body.code_challenge || request.query.code_challenge; + + /** + * If the server requires Proof Key for Code Exchange (PKCE) by OAuth + * public clients and the client does not send the "code_challenge" in + * the request, the authorization endpoint MUST return the authorization + * error response with the "error" value set to "invalid_request". The + * "error_description" or the response of "error_uri" SHOULD explain the + * nature of error, e.g., code challenge required. + */ + if (this.PKCEEnabled && client.isPublic && _.isEmpty(codeChallenge)) { + throw new InvalidRequestError('Missing parameter: `code_challenge`. Public clients must include a code_challenge'); + } + + return codeChallenge; +}; + +AuthorizeHandler.prototype.getCodeChallengeMethod = function(request) { + var codeChallengeMethod = request.body.code_challenge_method || request.query.code_challenge_method || 'plain'; + + if (!_.includes(['S256', 'plain'], codeChallengeMethod)) { + throw new InvalidRequestError('Invalid parameter: `code_challenge_method`, use S256 instead'); + } + + return codeChallengeMethod; +}; + /** * Get authorization code lifetime. */ @@ -172,6 +208,7 @@ AuthorizeHandler.prototype.getClient = function(request) { throw new InvalidRequestError('Invalid request: `redirect_uri` is not a valid URI'); } return promisify(this.model.getClient, 2).call(this.model, clientId, null) + .bind(this) .then(function(client) { if (!client) { throw new InvalidClientError('Invalid client: client credentials are invalid'); @@ -192,6 +229,16 @@ AuthorizeHandler.prototype.getClient = function(request) { if (redirectUri && !_.includes(client.redirectUris, redirectUri)) { throw new InvalidClientError('Invalid client: `redirect_uri` does not match client value'); } + + if (this.PKCEEnabled) { + if (client.isPublic === undefined) { + throw new InvalidClientError('Invalid client: missing client `isPublic`'); + } + + if (typeof client.isPublic !== 'boolean') { + throw new InvalidClientError('Invalid client: `isPublic` must be a boolean'); + } + } return client; }); }; @@ -257,12 +304,14 @@ AuthorizeHandler.prototype.getRedirectUri = function(request, client) { * Save authorization code. */ -AuthorizeHandler.prototype.saveAuthorizationCode = function(authorizationCode, expiresAt, scope, client, redirectUri, user) { +AuthorizeHandler.prototype.saveAuthorizationCode = function(authorizationCode, expiresAt, scope, client, redirectUri, user, codeChallenge, codeChallengeMethod) { var code = { authorizationCode: authorizationCode, expiresAt: expiresAt, redirectUri: redirectUri, - scope: scope + scope: scope, + codeChallenge: codeChallenge, + codeChallengeMethod: codeChallengeMethod }; return promisify(this.model.saveAuthorizationCode, 3).call(this.model, code, client, user); }; diff --git a/lib/handlers/token-handler.js b/lib/handlers/token-handler.js index feaad3f54..d19fd6a99 100644 --- a/lib/handlers/token-handler.js +++ b/lib/handlers/token-handler.js @@ -62,6 +62,7 @@ function TokenHandler(options) { this.allowExtendedTokenAttributes = options.allowExtendedTokenAttributes; this.requireClientAuthentication = options.requireClientAuthentication || {}; this.alwaysIssueNewRefreshToken = options.alwaysIssueNewRefreshToken !== false; + this.PKCEEnabled = options.PKCEEnabled; } /** @@ -133,6 +134,7 @@ TokenHandler.prototype.getClient = function(request, response) { } return promisify(this.model.getClient, 2).call(this.model, credentials.clientId, credentials.clientSecret) + .bind(this) .then(function(client) { if (!client) { throw new InvalidClientError('Invalid client: client is invalid'); @@ -146,6 +148,16 @@ TokenHandler.prototype.getClient = function(request, response) { throw new ServerError('Server error: `grants` must be an array'); } + if (this.PKCEEnabled) { + if (client.isPublic === undefined) { + throw new ServerError('Server error: missing client `isPublic`'); + } + + if (typeof client.isPublic !== 'boolean') { + throw new ServerError('Server error: invalid client, `isPublic` must be a boolean'); + } + } + return client; }) .catch(function(e) { @@ -224,7 +236,8 @@ TokenHandler.prototype.handleGrantType = function(request, client) { accessTokenLifetime: accessTokenLifetime, model: this.model, refreshTokenLifetime: refreshTokenLifetime, - alwaysIssueNewRefreshToken: this.alwaysIssueNewRefreshToken + alwaysIssueNewRefreshToken: this.alwaysIssueNewRefreshToken, + PKCEenabled: this.PKCEenabled }; return new Type(options) diff --git a/lib/server.js b/lib/server.js index fba9ccf81..cd52b6003 100644 --- a/lib/server.js +++ b/lib/server.js @@ -51,7 +51,8 @@ OAuth2Server.prototype.authenticate = function(request, response, options, callb OAuth2Server.prototype.authorize = function(request, response, options, callback) { options = _.assign({ allowEmptyState: false, - authorizationCodeLifetime: 5 * 60 // 5 minutes. + authorizationCodeLifetime: 5 * 60, // 5 minutes. + PKCEEnabled: false }, this.options, options); return new AuthorizeHandler(options) @@ -68,7 +69,8 @@ OAuth2Server.prototype.token = function(request, response, options, callback) { accessTokenLifetime: 60 * 60, // 1 hour. refreshTokenLifetime: 60 * 60 * 24 * 14, // 2 weeks. allowExtendedTokenAttributes: false, - requireClientAuthentication: {} // defaults to true for all grant types + requireClientAuthentication: {}, // defaults to true for all grant types + PKCEEnabled: false }, this.options, options); return new TokenHandler(options) diff --git a/lib/utils/string-util.js b/lib/utils/string-util.js new file mode 100644 index 000000000..1ad8045ba --- /dev/null +++ b/lib/utils/string-util.js @@ -0,0 +1,14 @@ +'use strict'; + +/** + * Export `StringUtil`. + */ + +module.exports = { + base64URLEncode: function(str) { + return str.toString('base64') + .replace(/\+/g, '-') + .replace(/\//g, '_') + .replace(/=/g, ''); + } +}; diff --git a/package.json b/package.json index 6992ef15e..5805cb171 100644 --- a/package.json +++ b/package.json @@ -7,13 +7,32 @@ "oauth2" ], "contributors": [ - { "name": "Thom Seddon", "email": "thom@seddonmedia.co.uk" }, - { "name": "Lars F. Karlström" , "email": "lars@lfk.io" }, - { "name": "Rui Marinho", "email": "ruipmarinho@gmail.com" }, - { "name" : "Tiago Ribeiro", "email": "tiago.ribeiro@gmail.com" }, - { "name": "Michael Salinger", "email": "mjsalinger@gmail.com" }, - { "name": "Nuno Sousa" }, - { "name": "Max Truxa" } + { + "name": "Thom Seddon", + "email": "thom@seddonmedia.co.uk" + }, + { + "name": "Lars F. Karlström", + "email": "lars@lfk.io" + }, + { + "name": "Rui Marinho", + "email": "ruipmarinho@gmail.com" + }, + { + "name": "Tiago Ribeiro", + "email": "tiago.ribeiro@gmail.com" + }, + { + "name": "Michael Salinger", + "email": "mjsalinger@gmail.com" + }, + { + "name": "Nuno Sousa" + }, + { + "name": "Max Truxa" + } ], "main": "index.js", "dependencies": { diff --git a/test/integration/grant-types/authorization-code-grant-type_test.js b/test/integration/grant-types/authorization-code-grant-type_test.js index 7f84e3443..a3fb344fc 100644 --- a/test/integration/grant-types/authorization-code-grant-type_test.js +++ b/test/integration/grant-types/authorization-code-grant-type_test.js @@ -9,8 +9,10 @@ var InvalidArgumentError = require('../../../lib/errors/invalid-argument-error') var InvalidGrantError = require('../../../lib/errors/invalid-grant-error'); var InvalidRequestError = require('../../../lib/errors/invalid-request-error'); var Promise = require('bluebird'); +var crypto = require('crypto'); var Request = require('../../../lib/request'); var ServerError = require('../../../lib/errors/server-error'); +var stringUtil = require('../../../lib/utils/string-util'); var should = require('should'); /** @@ -361,6 +363,110 @@ describe('AuthorizationCodeGrantType integration', function() { }); }); + it('should throw an error if the `code_verifier` is invalid', function() { + var codeVerifier = stringUtil.base64URLEncode(crypto.randomBytes(32)); + var authorizationCode = { + authorizationCode: 12345, + client: { id: 'foobar' }, + expiresAt: new Date(new Date() * 2), + user: {}, + codeChallengeMethod: 'S256', + codeChallenge: stringUtil.base64URLEncode(crypto.createHash('sha256').update(codeVerifier).digest()) + }; + var client = { id: 'foobar', isPublic: true }; + var model = { + getAuthorizationCode: function() { return authorizationCode; }, + revokeAuthorizationCode: function() {}, + saveToken: function() {} + }; + var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model, PKCEEnabled: true }); + var request = new Request({ body: { code: 12345, code_verifier: 'foo' }, headers: {}, method: {}, query: {} }); + + return grantType.getAuthorizationCode(request, client) + .then(should.fail) + .catch(function(e) { + e.should.be.an.instanceOf(InvalidGrantError); + e.message.should.equal('Invalid grant: code verifier is invalid'); + }); + }); + + it('should throw an error if the `code_verifier` is invalid', function() { + var authorizationCode = { + authorizationCode: 12345, + client: { id: 'foobar' }, + expiresAt: new Date(new Date() * 2), + user: {}, + codeChallengeMethod: 'plain', + codeChallenge: 'baz' + }; + var client = { id: 'foobar', isPublic: true }; + var model = { + getAuthorizationCode: function() { return authorizationCode; }, + revokeAuthorizationCode: function() {}, + saveToken: function() {} + }; + var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model, PKCEEnabled: true }); + var request = new Request({ body: { code: 12345, code_verifier: 'foo' }, headers: {}, method: {}, query: {} }); + + return grantType.getAuthorizationCode(request, client) + .then(should.fail) + .catch(function(e) { + e.should.be.an.instanceOf(InvalidGrantError); + e.message.should.equal('Invalid grant: code verifier is invalid'); + }); + }); + + it('should return an auth code when `code_verifier` is valid', function() { + var codeVerifier = stringUtil.base64URLEncode(crypto.randomBytes(32)); + var authorizationCode = { + authorizationCode: 12345, + client: { id: 'foobar', isPublic: true }, + expiresAt: new Date(new Date() * 2), + user: {}, + codeChallengeMethod: 'S256', + codeChallenge: stringUtil.base64URLEncode(crypto.createHash('sha256').update(codeVerifier).digest()) + }; + var client = { id: 'foobar', isPublic: true }; + var model = { + getAuthorizationCode: function() { return authorizationCode; }, + revokeAuthorizationCode: function() {}, + saveToken: function() {} + }; + var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model, PKCEEnabled: true }); + var request = new Request({ body: { code: 12345, code_verifier: codeVerifier }, headers: {}, method: {}, query: {} }); + + return grantType.getAuthorizationCode(request, client) + .then(function(data) { + data.should.equal(authorizationCode); + }) + .catch(should.fail); + }); + + it('should return an auth code when `code_verifier` is valid', function() { + var authorizationCode = { + authorizationCode: 12345, + client: { id: 'foobar' }, + expiresAt: new Date(new Date() * 2), + user: {}, + codeChallengeMethod: 'plain', + codeChallenge: 'baz' + }; + var client = { id: 'foobar', isPublic: true }; + var model = { + getAuthorizationCode: function() { return authorizationCode; }, + revokeAuthorizationCode: function() {}, + saveToken: function() {} + }; + var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model, PKCEEnabled: true }); + var request = new Request({ body: { code: 12345, code_verifier: 'baz' }, headers: {}, method: {}, query: {} }); + + return grantType.getAuthorizationCode(request, client) + .then(function(data) { + data.should.equal(authorizationCode); + }) + .catch(should.fail); + }); + it('should return an auth code', function() { var authorizationCode = { authorizationCode: 12345, client: { id: 'foobar' }, expiresAt: new Date(new Date() * 2), user: {} }; var client = { id: 'foobar' }; diff --git a/test/integration/handlers/authorize-handler_test.js b/test/integration/handlers/authorize-handler_test.js index 0d1aa333b..2fbe7d471 100644 --- a/test/integration/handlers/authorize-handler_test.js +++ b/test/integration/handlers/authorize-handler_test.js @@ -694,6 +694,44 @@ describe('AuthorizeHandler integration', function() { e.message.should.equal('Invalid client: `redirect_uri` does not match client value'); }); }); + describe('with PKCEEnabled', function() { + it('should throw an error if `client.isPublic` is missing`', function() { + var model = { + getAccessToken: function() {}, + getClient: function() { + return { grants: ['authorization_code'], redirectUris: ['https://example.com'] }; + }, + saveAuthorizationCode: function() {} + }; + var handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model, PKCEEnabled: true }); + var request = new Request({ body: { client_id: 12345, response_type: 'code', redirect_uri: 'https://example.com' }, headers: {}, method: {}, query: {} }); + + return handler.getClient(request) + .then(should.fail) + .catch(function(e) { + e.should.be.an.instanceOf(InvalidClientError); + e.message.should.equal('Invalid client: missing client `isPublic`'); + }); + }); + it('should throw an error if `client.isPublic` is invalid`', function() { + var model = { + getAccessToken: function() {}, + getClient: function() { + return { grants: ['authorization_code'], redirectUris: ['https://example.com'], isPublic: 'foo' }; + }, + saveAuthorizationCode: function() {} + }; + var handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model, PKCEEnabled: true }); + var request = new Request({ body: { client_id: 12345, response_type: 'code', redirect_uri: 'https://example.com' }, headers: {}, method: {}, query: {} }); + + return handler.getClient(request) + .then(should.fail) + .catch(function(e) { + e.should.be.an.instanceOf(InvalidClientError); + e.message.should.equal('Invalid client: `isPublic` must be a boolean'); + }); + }); + }); it('should support promises', function() { var model = { @@ -824,6 +862,84 @@ describe('AuthorizeHandler integration', function() { }); }); + describe('getCodeChallenge()', function() { + describe('with `code_challenge` in the request body', function() { + it('should return the code_challenge', function() { + var model = { + getAccessToken: function() {}, + getClient: function() {}, + saveAuthorizationCode: function() {} + }; + var handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model }); + var request = new Request({ body: { code_challenge: 'foo' }, headers: {}, method: {}, query: {} }); + + handler.getCodeChallenge(request).should.equal('foo'); + }); + }); + + describe('with `code_challenge` in the request query', function() { + it('should return the code_challenge', function() { + var model = { + getAccessToken: function() {}, + getClient: function() {}, + saveAuthorizationCode: function() {} + }; + var handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model }); + var request = new Request({ body: {}, headers: {}, method: {}, query: { code_challenge: 'foo' } }); + + handler.getCodeChallenge(request).should.equal('foo'); + }); + }); + }); + + describe('getCodeChallengeMethod()', function() { + it('should throw an error if `scope` is invalid', function() { + var model = { + getAccessToken: function() {}, + getClient: function() {}, + saveAuthorizationCode: function() {} + }; + var handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model }); + var request = new Request({ body: { code_challenge_method: 'foo' }, headers: {}, method: {}, query: {} }); + + try { + handler.getCodeChallengeMethod(request); + + should.fail(); + } catch (e) { + e.should.be.an.instanceOf(InvalidRequestError); + e.message.should.equal('Invalid parameter: `code_challenge_method`, use S256 instead'); + } + }); + describe('with `code_challenge_method` in the request body', function() { + it('should return the code_challenge_method', function() { + var model = { + getAccessToken: function() {}, + getClient: function() {}, + saveAuthorizationCode: function() {} + }; + var handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model }); + var request = new Request({ body: { code_challenge_method: 'plain' }, headers: {}, method: {}, query: {} }); + + handler.getCodeChallengeMethod(request).should.equal('plain'); + }); + }); + + describe('with `code_challenge_method` in the request query', function() { + it('should return the code_challenge_method', function() { + var model = { + getAccessToken: function() {}, + getClient: function() {}, + saveAuthorizationCode: function() {} + }; + var handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model }); + var request = new Request({ body: {}, headers: {}, method: {}, query: { code_challenge_method: 'S256' } }); + + handler.getCodeChallengeMethod(request).should.equal('S256'); + }); + }); + }); + describe('getState()', function() { it('should throw an error if `allowEmptyState` is false and `state` is missing', function() { var model = { diff --git a/test/integration/handlers/token-handler_test.js b/test/integration/handlers/token-handler_test.js index 50277c113..881767bb8 100644 --- a/test/integration/handlers/token-handler_test.js +++ b/test/integration/handlers/token-handler_test.js @@ -114,6 +114,16 @@ describe('TokenHandler integration', function() { handler.alwaysIssueNewRefreshToken.should.equal(true); }); + it('should set the `PKCEEnabled` to true', function() { + var model = { + getClient: function() {}, + saveToken: function() {} + }; + var handler = new TokenHandler({ accessTokenLifetime: 123, model: model, refreshTokenLifetime: 120, PKCEEnabled: true }); + + handler.PKCEEnabled.should.equal(true); + }); + it('should set the `extendedGrantTypes`', function() { var extendedGrantTypes = { foo: 'bar' }; var model = { @@ -569,13 +579,13 @@ describe('TokenHandler integration', function() { requireClientAuthentication: { password: false } - }); + }); var request = new Request({ - body: { grant_type: 'password'}, - headers: { 'authorization': util.format('Basic %s', new Buffer('blah:').toString('base64')) }, - method: {}, - query: {} - }); + body: { grant_type: 'password' }, + headers: { 'authorization': util.format('Basic %s', new Buffer('blah:').toString('base64')) }, + method: {}, + query: {} + }); return handler.getClient(request) .then(function(data) { @@ -584,6 +594,59 @@ describe('TokenHandler integration', function() { .catch(should.fail); }); }); + describe('with PKCE enabled', function() { + it('should return a client with `isPublic` parameter', function() { + var client = { id: 12345, grants: [], isPublic: true }; + var model = { + getClient: function() { return client; }, + saveToken: function() {} + }; + + var handler = new TokenHandler({ + accessTokenLifetime: 120, + model: model, + refreshTokenLifetime: 120, + PKCEEnabled: true + }); + var request = new Request({ body: { client_id: 'foo', client_secret: 'baz', grant_type: 'authorization_code' }, headers: {}, method: {}, query: {} }); + + return handler.getClient(request) + .then(function(data) { + data.should.equal(client); + }) + .catch(should.fail); + }); + it('should throw an error if `client.isPublic` is invalid', function() { + var model = { + getClient: function() { return { id: 123, grants: [], isPublic: 'foo' }; }, + saveToken: function() {} + }; + var handler = new TokenHandler({ accessTokenLifetime: 120, model: model, refreshTokenLifetime: 120, PKCEEnabled: true }); + var request = new Request({ body: { client_id: 12345, client_secret: 'secret' }, headers: {}, method: {}, query: {} }); + + return handler.getClient(request) + .then(should.fail) + .catch(function(e) { + e.should.be.an.instanceOf(ServerError); + e.message.should.equal('Server error: invalid client, `isPublic` must be a boolean'); + }); + }); + it('should throw an error if `client.isPublic` is missing', function() { + var model = { + getClient: function() { return { id: 123, grants: [] }; }, + saveToken: function() {} + }; + var handler = new TokenHandler({ accessTokenLifetime: 120, model: model, refreshTokenLifetime: 120, PKCEEnabled: true }); + var request = new Request({ body: { client_id: 12345, client_secret: 'secret' }, headers: {}, method: {}, query: {} }); + + return handler.getClient(request) + .then(should.fail) + .catch(function(e) { + e.should.be.an.instanceOf(ServerError); + e.message.should.equal('Server error: missing client `isPublic`'); + }); + }); + }); it('should support promises', function() { var model = { diff --git a/test/unit/handlers/authorize-handler_test.js b/test/unit/handlers/authorize-handler_test.js index fe9b6b1d7..30e3111d7 100644 --- a/test/unit/handlers/authorize-handler_test.js +++ b/test/unit/handlers/authorize-handler_test.js @@ -87,11 +87,11 @@ describe('AuthorizeHandler', function() { }; var handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model }); - return handler.saveAuthorizationCode('foo', 'bar', 'qux', 'biz', 'baz', 'boz') + return handler.saveAuthorizationCode('foo', 'bar', 'qux', 'biz', 'baz', 'boz', 'buz', 'bez') .then(function() { model.saveAuthorizationCode.callCount.should.equal(1); model.saveAuthorizationCode.firstCall.args.should.have.length(3); - model.saveAuthorizationCode.firstCall.args[0].should.eql({ authorizationCode: 'foo', expiresAt: 'bar', redirectUri: 'baz', scope: 'qux' }); + model.saveAuthorizationCode.firstCall.args[0].should.eql({ authorizationCode: 'foo', expiresAt: 'bar', redirectUri: 'baz', scope: 'qux', codeChallenge: 'buz', codeChallengeMethod: 'bez' }); model.saveAuthorizationCode.firstCall.args[1].should.equal('biz'); model.saveAuthorizationCode.firstCall.args[2].should.equal('boz'); model.saveAuthorizationCode.firstCall.thisValue.should.equal(model); From 9c94bba77fe017eb99af0943934c75b11685e31b Mon Sep 17 00:00:00 2001 From: viktor sincak Date: Fri, 1 Jun 2018 10:51:45 +0200 Subject: [PATCH 2/2] prevent empty code challenge and undo refactor --- lib/grant-types/abstract-grant-type.js | 1 - .../authorization-code-grant-type.js | 16 ++--- lib/handlers/authorize-handler.js | 39 +++--------- lib/handlers/token-handler.js | 15 +---- lib/server.js | 6 +- .../authorization-code-grant-type_test.js | 8 +-- .../handlers/authorize-handler_test.js | 38 ----------- .../handlers/token-handler_test.js | 63 ------------------- 8 files changed, 21 insertions(+), 165 deletions(-) diff --git a/lib/grant-types/abstract-grant-type.js b/lib/grant-types/abstract-grant-type.js index 6c5f20de9..224a473e3 100644 --- a/lib/grant-types/abstract-grant-type.js +++ b/lib/grant-types/abstract-grant-type.js @@ -30,7 +30,6 @@ function AbstractGrantType(options) { this.model = options.model; this.refreshTokenLifetime = options.refreshTokenLifetime; this.alwaysIssueNewRefreshToken = options.alwaysIssueNewRefreshToken; - this.PKCEEnabled = options.PKCEEnabled; } /** diff --git a/lib/grant-types/authorization-code-grant-type.js b/lib/grant-types/authorization-code-grant-type.js index 6f302918b..787dfabff 100644 --- a/lib/grant-types/authorization-code-grant-type.js +++ b/lib/grant-types/authorization-code-grant-type.js @@ -92,7 +92,6 @@ AuthorizationCodeGrantType.prototype.getAuthorizationCode = function(request, cl } return promisify(this.model.getAuthorizationCode, 1).call(this.model, request.body.code) - .bind(this) .then(function(code) { if (!code) { throw new InvalidGrantError('Invalid grant: authorization code is invalid'); @@ -122,20 +121,15 @@ AuthorizationCodeGrantType.prototype.getAuthorizationCode = function(request, cl throw new InvalidGrantError('Invalid grant: `redirect_uri` is not a valid URI'); } - if (this.PKCEEnabled && client.isPublic) { - if (!code.codeChallenge) { - throw new InvalidGrantError('Invalid grant: code challenge is invalid'); - } - - /** - * The "code_challenge_method" is bound to the Authorization Code when - * the Authorization Code is issued. That is the method that the token - * endpoint MUST use to verify the "code_verifier". - */ + if (code.codeChallenge) { if (!code.codeChallengeMethod) { throw new ServerError('Server error: `getAuthorizationCode()` did not return a `codeChallengeMethod` property'); } + if (!request.body.code_verifier) { + throw new InvalidGrantError('Missing parameter: `code_verifier`'); + } + if (code.codeChallengeMethod === 'plain' && code.codeChallenge !== request.body.code_verifier) { throw new InvalidGrantError('Invalid grant: code verifier is invalid'); } diff --git a/lib/handlers/authorize-handler.js b/lib/handlers/authorize-handler.js index a63703bfe..e2e767359 100644 --- a/lib/handlers/authorize-handler.js +++ b/lib/handlers/authorize-handler.js @@ -62,7 +62,6 @@ function AuthorizeHandler(options) { this.allowEmptyState = options.allowEmptyState; this.authenticateHandler = options.authenticateHandler || new AuthenticateHandler(options); this.authorizationCodeLifetime = options.authorizationCodeLifetime; - this.PKCEEnabled = options.PKCEEnabled; this.model = options.model; } @@ -99,17 +98,11 @@ AuthorizeHandler.prototype.handle = function(request, response) { var scope; var state; var ResponseType; - var codeChallenge; - var codeChallengeMethod; + var codeChallenge = this.getCodeChallenge(request); + var codeChallengeMethod = codeChallenge && this.getCodeChallengeMethod(request); return Promise.bind(this) .then(function() { - codeChallenge = this.getCodeChallenge(request, client); - - if (codeChallenge) { - codeChallengeMethod = this.getCodeChallengeMethod(request); - } - var requestedScope = this.getScope(request); return this.validateScope(user, client, requestedScope); @@ -157,19 +150,15 @@ AuthorizeHandler.prototype.generateAuthorizationCode = function(client, user, sc return tokenUtil.generateRandomToken(); }; -AuthorizeHandler.prototype.getCodeChallenge = function(request, client) { +AuthorizeHandler.prototype.getCodeChallenge = function(request) { var codeChallenge = request.body.code_challenge || request.query.code_challenge; - /** - * If the server requires Proof Key for Code Exchange (PKCE) by OAuth - * public clients and the client does not send the "code_challenge" in - * the request, the authorization endpoint MUST return the authorization - * error response with the "error" value set to "invalid_request". The - * "error_description" or the response of "error_uri" SHOULD explain the - * nature of error, e.g., code challenge required. - */ - if (this.PKCEEnabled && client.isPublic && _.isEmpty(codeChallenge)) { - throw new InvalidRequestError('Missing parameter: `code_challenge`. Public clients must include a code_challenge'); + if (!codeChallenge || codeChallenge === '') { + return; + } + + if (!is.vschar(codeChallenge)) { + throw new InvalidRequestError('Invalid parameter: `code_challenge`'); } return codeChallenge; @@ -217,7 +206,6 @@ AuthorizeHandler.prototype.getClient = function(request) { throw new InvalidRequestError('Invalid request: `redirect_uri` is not a valid URI'); } return promisify(this.model.getClient, 2).call(this.model, clientId, null) - .bind(this) .then(function(client) { if (!client) { throw new InvalidClientError('Invalid client: client credentials are invalid'); @@ -239,15 +227,6 @@ AuthorizeHandler.prototype.getClient = function(request) { throw new InvalidClientError('Invalid client: `redirect_uri` does not match client value'); } - if (this.PKCEEnabled) { - if (client.isPublic === undefined) { - throw new InvalidClientError('Invalid client: missing client `isPublic`'); - } - - if (typeof client.isPublic !== 'boolean') { - throw new InvalidClientError('Invalid client: `isPublic` must be a boolean'); - } - } return client; }); }; diff --git a/lib/handlers/token-handler.js b/lib/handlers/token-handler.js index f83b87ba4..af162ca38 100644 --- a/lib/handlers/token-handler.js +++ b/lib/handlers/token-handler.js @@ -62,7 +62,6 @@ function TokenHandler(options) { this.allowExtendedTokenAttributes = options.allowExtendedTokenAttributes; this.requireClientAuthentication = options.requireClientAuthentication || {}; this.alwaysIssueNewRefreshToken = options.alwaysIssueNewRefreshToken !== false; - this.PKCEEnabled = options.PKCEEnabled; } /** @@ -137,7 +136,6 @@ TokenHandler.prototype.getClient = function(request, response) { } return promisify(this.model.getClient, 2).call(this.model, credentials.clientId, credentials.clientSecret) - .bind(this) .then(function(client) { if (!client) { throw new InvalidClientError('Invalid client: client is invalid'); @@ -151,16 +149,6 @@ TokenHandler.prototype.getClient = function(request, response) { throw new ServerError('Server error: `grants` must be an array'); } - if (this.PKCEEnabled) { - if (client.isPublic === undefined) { - throw new ServerError('Server error: missing client `isPublic`'); - } - - if (typeof client.isPublic !== 'boolean') { - throw new ServerError('Server error: invalid client, `isPublic` must be a boolean'); - } - } - return client; }) .catch(function(e) { @@ -239,8 +227,7 @@ TokenHandler.prototype.handleGrantType = function(request, client) { accessTokenLifetime: accessTokenLifetime, model: this.model, refreshTokenLifetime: refreshTokenLifetime, - alwaysIssueNewRefreshToken: this.alwaysIssueNewRefreshToken, - PKCEenabled: this.PKCEenabled + alwaysIssueNewRefreshToken: this.alwaysIssueNewRefreshToken }; return new Type(options) diff --git a/lib/server.js b/lib/server.js index cd52b6003..fba9ccf81 100644 --- a/lib/server.js +++ b/lib/server.js @@ -51,8 +51,7 @@ OAuth2Server.prototype.authenticate = function(request, response, options, callb OAuth2Server.prototype.authorize = function(request, response, options, callback) { options = _.assign({ allowEmptyState: false, - authorizationCodeLifetime: 5 * 60, // 5 minutes. - PKCEEnabled: false + authorizationCodeLifetime: 5 * 60 // 5 minutes. }, this.options, options); return new AuthorizeHandler(options) @@ -69,8 +68,7 @@ OAuth2Server.prototype.token = function(request, response, options, callback) { accessTokenLifetime: 60 * 60, // 1 hour. refreshTokenLifetime: 60 * 60 * 24 * 14, // 2 weeks. allowExtendedTokenAttributes: false, - requireClientAuthentication: {}, // defaults to true for all grant types - PKCEEnabled: false + requireClientAuthentication: {} // defaults to true for all grant types }, this.options, options); return new TokenHandler(options) diff --git a/test/integration/grant-types/authorization-code-grant-type_test.js b/test/integration/grant-types/authorization-code-grant-type_test.js index a3fb344fc..31be214d6 100644 --- a/test/integration/grant-types/authorization-code-grant-type_test.js +++ b/test/integration/grant-types/authorization-code-grant-type_test.js @@ -379,7 +379,7 @@ describe('AuthorizationCodeGrantType integration', function() { revokeAuthorizationCode: function() {}, saveToken: function() {} }; - var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model, PKCEEnabled: true }); + var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model }); var request = new Request({ body: { code: 12345, code_verifier: 'foo' }, headers: {}, method: {}, query: {} }); return grantType.getAuthorizationCode(request, client) @@ -405,7 +405,7 @@ describe('AuthorizationCodeGrantType integration', function() { revokeAuthorizationCode: function() {}, saveToken: function() {} }; - var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model, PKCEEnabled: true }); + var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model }); var request = new Request({ body: { code: 12345, code_verifier: 'foo' }, headers: {}, method: {}, query: {} }); return grantType.getAuthorizationCode(request, client) @@ -432,7 +432,7 @@ describe('AuthorizationCodeGrantType integration', function() { revokeAuthorizationCode: function() {}, saveToken: function() {} }; - var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model, PKCEEnabled: true }); + var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model }); var request = new Request({ body: { code: 12345, code_verifier: codeVerifier }, headers: {}, method: {}, query: {} }); return grantType.getAuthorizationCode(request, client) @@ -457,7 +457,7 @@ describe('AuthorizationCodeGrantType integration', function() { revokeAuthorizationCode: function() {}, saveToken: function() {} }; - var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model, PKCEEnabled: true }); + var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model }); var request = new Request({ body: { code: 12345, code_verifier: 'baz' }, headers: {}, method: {}, query: {} }); return grantType.getAuthorizationCode(request, client) diff --git a/test/integration/handlers/authorize-handler_test.js b/test/integration/handlers/authorize-handler_test.js index 9d052b5de..3e1b3bc86 100644 --- a/test/integration/handlers/authorize-handler_test.js +++ b/test/integration/handlers/authorize-handler_test.js @@ -783,44 +783,6 @@ describe('AuthorizeHandler integration', function() { e.message.should.equal('Invalid client: `redirect_uri` does not match client value'); }); }); - describe('with PKCEEnabled', function() { - it('should throw an error if `client.isPublic` is missing`', function() { - var model = { - getAccessToken: function() {}, - getClient: function() { - return { grants: ['authorization_code'], redirectUris: ['https://example.com'] }; - }, - saveAuthorizationCode: function() {} - }; - var handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model, PKCEEnabled: true }); - var request = new Request({ body: { client_id: 12345, response_type: 'code', redirect_uri: 'https://example.com' }, headers: {}, method: {}, query: {} }); - - return handler.getClient(request) - .then(should.fail) - .catch(function(e) { - e.should.be.an.instanceOf(InvalidClientError); - e.message.should.equal('Invalid client: missing client `isPublic`'); - }); - }); - it('should throw an error if `client.isPublic` is invalid`', function() { - var model = { - getAccessToken: function() {}, - getClient: function() { - return { grants: ['authorization_code'], redirectUris: ['https://example.com'], isPublic: 'foo' }; - }, - saveAuthorizationCode: function() {} - }; - var handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model, PKCEEnabled: true }); - var request = new Request({ body: { client_id: 12345, response_type: 'code', redirect_uri: 'https://example.com' }, headers: {}, method: {}, query: {} }); - - return handler.getClient(request) - .then(should.fail) - .catch(function(e) { - e.should.be.an.instanceOf(InvalidClientError); - e.message.should.equal('Invalid client: `isPublic` must be a boolean'); - }); - }); - }); it('should support promises', function() { var model = { diff --git a/test/integration/handlers/token-handler_test.js b/test/integration/handlers/token-handler_test.js index 41d7bd7dc..e9eb4d846 100644 --- a/test/integration/handlers/token-handler_test.js +++ b/test/integration/handlers/token-handler_test.js @@ -114,16 +114,6 @@ describe('TokenHandler integration', function() { handler.alwaysIssueNewRefreshToken.should.equal(true); }); - it('should set the `PKCEEnabled` to true', function() { - var model = { - getClient: function() {}, - saveToken: function() {} - }; - var handler = new TokenHandler({ accessTokenLifetime: 123, model: model, refreshTokenLifetime: 120, PKCEEnabled: true }); - - handler.PKCEEnabled.should.equal(true); - }); - it('should set the `extendedGrantTypes`', function() { var extendedGrantTypes = { foo: 'bar' }; var model = { @@ -595,59 +585,6 @@ describe('TokenHandler integration', function() { .catch(should.fail); }); }); - describe('with PKCE enabled', function() { - it('should return a client with `isPublic` parameter', function() { - var client = { id: 12345, grants: [], isPublic: true }; - var model = { - getClient: function() { return client; }, - saveToken: function() {} - }; - - var handler = new TokenHandler({ - accessTokenLifetime: 120, - model: model, - refreshTokenLifetime: 120, - PKCEEnabled: true - }); - var request = new Request({ body: { client_id: 'foo', client_secret: 'baz', grant_type: 'authorization_code' }, headers: {}, method: {}, query: {} }); - - return handler.getClient(request) - .then(function(data) { - data.should.equal(client); - }) - .catch(should.fail); - }); - it('should throw an error if `client.isPublic` is invalid', function() { - var model = { - getClient: function() { return { id: 123, grants: [], isPublic: 'foo' }; }, - saveToken: function() {} - }; - var handler = new TokenHandler({ accessTokenLifetime: 120, model: model, refreshTokenLifetime: 120, PKCEEnabled: true }); - var request = new Request({ body: { client_id: 12345, client_secret: 'secret' }, headers: {}, method: {}, query: {} }); - - return handler.getClient(request) - .then(should.fail) - .catch(function(e) { - e.should.be.an.instanceOf(ServerError); - e.message.should.equal('Server error: invalid client, `isPublic` must be a boolean'); - }); - }); - it('should throw an error if `client.isPublic` is missing', function() { - var model = { - getClient: function() { return { id: 123, grants: [] }; }, - saveToken: function() {} - }; - var handler = new TokenHandler({ accessTokenLifetime: 120, model: model, refreshTokenLifetime: 120, PKCEEnabled: true }); - var request = new Request({ body: { client_id: 12345, client_secret: 'secret' }, headers: {}, method: {}, query: {} }); - - return handler.getClient(request) - .then(should.fail) - .catch(function(e) { - e.should.be.an.instanceOf(ServerError); - e.message.should.equal('Server error: missing client `isPublic`'); - }); - }); - }); it('should support promises', function() { var model = {