PKCE refactor: add option PKCEEnabled, require isPublic client parameter

visvk · visvk · commit 009671b2d81a · 2018-05-29T12:08:28.000+02:00
diff --git 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
@@ -90,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');
@@ -120,7 +122,11 @@ AuthorizationCodeGrantType.prototype.getAuthorizationCode = function(request, cl
         throw new InvalidGrantError('Invalid grant: `redirect_uri` is not a valid URI');
       }
 
-      if (code.codeChallenge) {
+      if (this.PKCEEnabled && client.isPublic) {
+        if (!code.codeChallenge) {
+          throw new ServerError('Server error: `getAuthorizationCode()` did not return a `codeChallenge` property');
+        }
+
         if (!code.codeChallengeMethod) {
           throw new ServerError('Server error: `getAuthorizationCode()` did not return a `codeChallengeMethod` property');
         }
diff --git 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;
 }
 
@@ -101,13 +102,13 @@ AuthorizeHandler.prototype.handle = function(request, response) {
       return Promise.bind(this)
         .then(function() {
           scope = this.getScope(request);
-          codeChallenge = this.getCodeChallenge(request);
+          codeChallenge = this.getCodeChallenge(request, client);
 
           if (codeChallenge) {
-            codeChallengeMethod = this.getCodeChallengeMethod(request) || 'plain';
+            codeChallengeMethod = this.getCodeChallengeMethod(request);
           }
 
-          return this.generateAuthorizationCode(client, user, scope, codeChallenge, codeChallengeMethod);
+          return this.generateAuthorizationCode(client, user, scope);
         })
         .then(function(authorizationCode) {
           state = this.getState(request);
@@ -140,23 +141,27 @@ AuthorizeHandler.prototype.handle = function(request, response) {
  * Generate authorization code.
  */
 
-AuthorizeHandler.prototype.generateAuthorizationCode = function(client, user, scope, codeChallenge, codeChallengeMethod) {
+AuthorizeHandler.prototype.generateAuthorizationCode = function(client, user, scope) {
   if (this.model.generateAuthorizationCode) {
-    return promisify(this.model.generateAuthorizationCode, 5).call(this.model, client, user, scope, codeChallenge, codeChallengeMethod);
+    return promisify(this.model.generateAuthorizationCode, 3).call(this.model, client, user, scope);
   }
   return tokenUtil.generateRandomToken();
 };
 
-AuthorizeHandler.prototype.getCodeChallenge = function(request) {
+AuthorizeHandler.prototype.getCodeChallenge = function(request, client) {
   var codeChallenge = request.body.code_challenge || request.query.code_challenge;
 
+  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;
+  var codeChallengeMethod = request.body.code_challenge_method || request.query.code_challenge_method || 'plain';
 
-  if (codeChallengeMethod && !_.includes(['S256', 'plain'], codeChallengeMethod)) {
+  if (!_.includes(['S256', 'plain'], codeChallengeMethod)) {
     throw new InvalidRequestError('Invalid parameter: `code_challenge_method`');
   }
 
@@ -195,6 +200,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');
@@ -215,6 +221,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;
     });
 };
diff --git 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
@@ -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/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
@@ -373,13 +373,13 @@ describe('AuthorizationCodeGrantType integration', function() {
         codeChallengeMethod: 'S256',
         codeChallenge: stringUtil.base64URLEncode(crypto.createHash('sha256').update(codeVerifier).digest())
       };
-      var client = { id: 'foobar' };
+      var client = { id: 'foobar', isPublic: true };
       var model = {
         getAuthorizationCode: function() { return authorizationCode; },
         revokeAuthorizationCode: function() {},
         saveToken: function() {}
       };
-      var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model });
+      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)
@@ -399,13 +399,13 @@ describe('AuthorizationCodeGrantType integration', function() {
         codeChallengeMethod: 'plain',
         codeChallenge: 'baz'
       };
-      var client = { id: 'foobar' };
+      var client = { id: 'foobar', isPublic: true };
       var model = {
         getAuthorizationCode: function() { return authorizationCode; },
         revokeAuthorizationCode: function() {},
         saveToken: function() {}
       };
-      var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model });
+      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)
@@ -420,19 +420,19 @@ describe('AuthorizationCodeGrantType integration', function() {
       var codeVerifier = stringUtil.base64URLEncode(crypto.randomBytes(32));
       var authorizationCode = {
         authorizationCode: 12345,
-        client: { id: 'foobar' },
+        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' };
+      var client = { id: 'foobar', isPublic: true };
       var model = {
         getAuthorizationCode: function() { return authorizationCode; },
         revokeAuthorizationCode: function() {},
         saveToken: function() {}
       };
-      var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model });
+      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)
@@ -451,13 +451,13 @@ describe('AuthorizationCodeGrantType integration', function() {
         codeChallengeMethod: 'plain',
         codeChallenge: 'baz'
       };
-      var client = { id: 'foobar' };
+      var client = { id: 'foobar', isPublic: true };
       var model = {
         getAuthorizationCode: function() { return authorizationCode; },
         revokeAuthorizationCode: function() {},
         saveToken: function() {}
       };
-      var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model });
+      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)
diff --git 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 = {
diff --git a/test/integration/handlers/token-handler_test.js b/test/integration/handlers/token-handler_test.js

Original file line number	Diff line number	Diff line change
`@@ -30,6 +30,7 @@ function AbstractGrantType(options) {`
`30`	`30`	`this.model = options.model;`
`31`	`31`	`this.refreshTokenLifetime = options.refreshTokenLifetime;`
`32`	`32`	`this.alwaysIssueNewRefreshToken = options.alwaysIssueNewRefreshToken;`
	`33`	`+ this.PKCEEnabled = options.PKCEEnabled;`
`33`	`34`	`}`
`34`	`35`
`35`	`36`	`/**`