Merge pull request #267 from node-oauth/compliance/fix-scope

Compliance/fix scope

Author: jankapunkt

lib/handlers/authenticate-handler.js

@@ -13,6 +13,7 @@ const Request = require('../request');
 const Response = require('../response');
 const ServerError = require('../errors/server-error');
 const UnauthorizedRequestError = require('../errors/unauthorized-request-error');
+const { parseScope } = require('../utils/scope-util');
 
 /**
  * Constructor.
@@ -46,7 +47,7 @@ class AuthenticateHandler {
     this.addAuthorizedScopesHeader = options.addAuthorizedScopesHeader;
     this.allowBearerTokensInQueryString = options.allowBearerTokensInQueryString;
     this.model = options.model;
-    this.scope = options.scope;
+    this.scope = parseScope(options.scope);
   }
 
   /**

lib/handlers/token-handler.js

@@ -266,6 +266,12 @@ class TokenHandler {
   updateSuccessResponse (response, tokenType) {
     response.body = tokenType.valueOf();
 
+    // for compliance reasons we rebuild the internal scope to be a string
+    // https://datatracker.ietf.org/doc/html/rfc6749.html#section-5.1
+    if (response.body.scope) {
+      response.body.scope = response.body.scope.join(' ');
+    }
+
     response.set('Cache-Control', 'no-store');
     response.set('Pragma', 'no-cache');
   }

lib/utils/scope-util.js

@@ -1,16 +1,25 @@
 const isFormat = require('@node-oauth/formats');
 const InvalidScopeError = require('../errors/invalid-scope-error');
+const whiteSpace = /\s+/g;
 
 module.exports = {
   parseScope: function (requestedScope) {
-    if (!isFormat.nqschar(requestedScope)) {
+    if (requestedScope == null) {
+      return undefined;
+    }
+
+    if (typeof requestedScope !== 'string') {
       throw new InvalidScopeError('Invalid parameter: `scope`');
     }
 
-    if (requestedScope == null) {
-      return undefined;
+    // XXX: this prevents spaced-only strings to become
+    // treated as valid nqchar by making them empty strings
+    requestedScope = requestedScope.trim();
+
+    if(!isFormat.nqschar(requestedScope)) {
+      throw new InvalidScopeError('Invalid parameter: `scope`');
     }
 
-    return requestedScope.split(' ');
+    return requestedScope.split(whiteSpace);
   }
 };

lib/validator/is.js

@@ -90,7 +90,7 @@ describe('ClientCredentials Workflow Compliance (4.4)', function () {
       response.body.token_type.should.equal('Bearer');
       response.body.access_token.should.equal(token.accessToken);
       response.body.expires_in.should.be.a('number');
-      response.body.scope.should.eql(['read', 'write']);
+      response.body.scope.should.eql('read write');
       ('refresh_token' in response.body).should.equal(false);
 
       token.accessToken.should.be.a('string');

test/compliance/client-credential-workflow_test.js

@@ -101,7 +101,7 @@ describe('PasswordGrantType Compliance', function () {
       response.body.access_token.should.equal(token.accessToken);
       response.body.refresh_token.should.equal(token.refreshToken);
       response.body.expires_in.should.be.a('number');
-      response.body.scope.should.eql(['read', 'write']);
+      response.body.scope.should.eql('read write');
 
       token.accessToken.should.be.a('string');
       token.refreshToken.should.be.a('string');

test/compliance/password-grant-type_test.js

@@ -124,7 +124,7 @@ describe('RefreshTokenGrantType Compliance', function () {
       refreshResponse.body.access_token.should.equal(token.accessToken);
       refreshResponse.body.refresh_token.should.equal(token.refreshToken);
       refreshResponse.body.expires_in.should.be.a('number');
-      refreshResponse.body.scope.should.eql(['read', 'write']);
+      refreshResponse.body.scope.should.eql('read write');
 
       token.accessToken.should.be.a('string');
       token.refreshToken.should.be.a('string');
@@ -223,7 +223,7 @@ describe('RefreshTokenGrantType Compliance', function () {
       refreshResponse.body.access_token.should.equal(token.accessToken);
       refreshResponse.body.refresh_token.should.equal(token.refreshToken);
       refreshResponse.body.expires_in.should.be.a('number');
-      refreshResponse.body.scope.should.eql(['read']);
+      refreshResponse.body.scope.should.eql('read');
     });
   });
 });

test/compliance/refresh-token-grant-type_test.js

@@ -10,6 +10,9 @@ function createModel (db) {
   }
 
   async function saveToken (token, client, user) {
+    if (token.scope && !Array.isArray(token.scope)) {
+      throw new Error('Scope should internally be an array');
+    }
     const meta = {
       clientId: client.id,
       userId: user.id,
@@ -38,7 +41,9 @@ function createModel (db) {
     if (!meta) {
       return false;
     }
-
+    if (meta.scope && !Array.isArray(meta.scope)) {
+      throw new Error('Scope should internally be an array');
+    }
     return {
       accessToken,
       accessTokenExpiresAt: meta.accessTokenExpiresAt,
@@ -54,7 +59,9 @@ function createModel (db) {
     if (!meta) {
       return false;
     }
-
+    if (meta.scope && !Array.isArray(meta.scope)) {
+      throw new Error('Scope should internally be an array');
+    }
     return {
       refreshToken,
       refreshTokenExpiresAt: meta.refreshTokenExpiresAt,
@@ -71,6 +78,9 @@ function createModel (db) {
   }
 
   async function verifyScope (token, scope) {
+    if (!Array.isArray(scope)) {
+      throw new Error('Scope should internally be an array');
+    }
     return scope.every(s => scopes.includes(s));
   }
 

test/helpers/model.js

@@ -93,7 +93,7 @@ describe('AuthenticateHandler integration', function() {
         addAcceptedScopesHeader: true,
         addAuthorizedScopesHeader: true,
         model: model,
-        scope: ['foobar']
+        scope: 'foobar'
       });
 
       grantType.scope.should.eql(['foobar']);
@@ -254,7 +254,7 @@ describe('AuthenticateHandler integration', function() {
           return true;
         }
       };
-      const handler = new AuthenticateHandler({ addAcceptedScopesHeader: true, addAuthorizedScopesHeader: true, model: model, scope: ['foo'] });
+      const handler = new AuthenticateHandler({ addAcceptedScopesHeader: true, addAuthorizedScopesHeader: true, model: model, scope: 'foo' });
       const request = new Request({
         body: {},
         headers: { 'Authorization': 'Bearer foo' },
@@ -522,7 +522,7 @@ describe('AuthenticateHandler integration', function() {
           return false;
         }
       };
-      const handler = new AuthenticateHandler({ addAcceptedScopesHeader: true, addAuthorizedScopesHeader: true, model: model, scope: ['foo'] });
+      const handler = new AuthenticateHandler({ addAcceptedScopesHeader: true, addAuthorizedScopesHeader: true, model: model, scope: 'foo' });
 
       return handler.verifyScope(['foo'])
         .then(should.fail)
@@ -539,7 +539,7 @@ describe('AuthenticateHandler integration', function() {
           return true;
         }
       };
-      const handler = new AuthenticateHandler({ addAcceptedScopesHeader: true, addAuthorizedScopesHeader: true, model: model, scope: ['foo'] });
+      const handler = new AuthenticateHandler({ addAcceptedScopesHeader: true, addAuthorizedScopesHeader: true, model: model, scope: 'foo' });
 
       handler.verifyScope(['foo']).should.be.an.instanceOf(Promise);
     });
@@ -551,7 +551,7 @@ describe('AuthenticateHandler integration', function() {
           return true;
         }
       };
-      const handler = new AuthenticateHandler({ addAcceptedScopesHeader: true, addAuthorizedScopesHeader: true, model: model, scope: ['foo'] });
+      const handler = new AuthenticateHandler({ addAcceptedScopesHeader: true, addAuthorizedScopesHeader: true, model: model, scope: 'foo' });
 
       handler.verifyScope(['foo']).should.be.an.instanceOf(Promise);
     });
@@ -576,7 +576,7 @@ describe('AuthenticateHandler integration', function() {
         getAccessToken: function() {},
         verifyScope: function() {}
       };
-      const handler = new AuthenticateHandler({ addAcceptedScopesHeader: true, addAuthorizedScopesHeader: false, model: model, scope: ['foo', 'bar'] });
+      const handler = new AuthenticateHandler({ addAcceptedScopesHeader: true, addAuthorizedScopesHeader: false, model: model, scope: 'foo bar' });
       const response = new Response({ body: {}, headers: {} });
 
       handler.updateResponse(response, { scope: ['foo', 'biz'] });
@@ -602,7 +602,7 @@ describe('AuthenticateHandler integration', function() {
         getAccessToken: function() {},
         verifyScope: function() {}
       };
-      const handler = new AuthenticateHandler({ addAcceptedScopesHeader: false, addAuthorizedScopesHeader: true, model: model, scope: ['foo', 'bar'] });
+      const handler = new AuthenticateHandler({ addAcceptedScopesHeader: false, addAuthorizedScopesHeader: true, model: model, scope: 'foo bar' });
       const response = new Response({ body: {}, headers: {} });
 
       handler.updateResponse(response, { scope: ['foo', 'biz'] });

test/integration/handlers/authenticate-handler_test.js

@@ -329,7 +329,7 @@ describe('TokenHandler integration', function() {
     });
 
     it('should not return custom attributes in a bearer token if the allowExtendedTokenAttributes is not set', function() {
-      const token = { accessToken: 'foo', client: {}, refreshToken: 'bar', scope: ['foobar'], user: {}, foo: 'bar' };
+      const token = { accessToken: 'foo', client: {}, refreshToken: 'bar', scope: ['baz'], user: {}, foo: 'bar' };
       const model = {
         getClient: function() { return { grants: ['password'] }; },
         getUser: function() { return {}; },
@@ -357,14 +357,14 @@ describe('TokenHandler integration', function() {
           should.exist(response.body.access_token);
           should.exist(response.body.refresh_token);
           should.exist(response.body.token_type);
-          should.exist(response.body.scope);
+          response.body.scope.should.eql('baz');
           should.not.exist(response.body.foo);
         })
         .catch(should.fail);
     });
 
     it('should return custom attributes in a bearer token if the allowExtendedTokenAttributes is set', function() {
-      const token = { accessToken: 'foo', client: {}, refreshToken: 'bar', scope: ['foobar'], user: {}, foo: 'bar' };
+      const token = { accessToken: 'foo', client: {}, refreshToken: 'bar', scope: ['baz'], user: {}, foo: 'bar' };
       const model = {
         getClient: function() { return { grants: ['password'] }; },
         getUser: function() { return {}; },
@@ -392,7 +392,7 @@ describe('TokenHandler integration', function() {
           should.exist(response.body.access_token);
           should.exist(response.body.refresh_token);
           should.exist(response.body.token_type);
-          should.exist(response.body.scope);
+          response.body.scope.should.eql('baz');
           should.exist(response.body.foo);
         })
         .catch(should.fail);

test/integration/handlers/token-handler_test.js

@@ -166,7 +166,7 @@ describe('AuthenticateHandler', function() {
         getAccessToken: function() {},
         verifyScope: sinon.stub().returns(true)
       };
-      const handler = new AuthenticateHandler({ addAcceptedScopesHeader: true, addAuthorizedScopesHeader: true, model: model, scope: ['bar'] });
+      const handler = new AuthenticateHandler({ addAcceptedScopesHeader: true, addAuthorizedScopesHeader: true, model: model, scope: 'bar' });
 
       return handler.verifyScope(['foo'])
         .then(function() {

test/unit/handlers/authenticate-handler_test.js

@@ -0,0 +1,45 @@
+const { parseScope } = require('../../../lib/utils/scope-util');
+const should = require('chai').should();
+
+describe(parseScope.name, () => {
+  it('should return undefined on nullish values', () => {
+    const values = [undefined, null];
+    values.forEach(str => {
+      const compare = parseScope(str) === undefined;
+      compare.should.equal(true);
+    });
+  });
+  it('should throw on non-string values', () => {
+    const invalid = [1, -1, true, false, {}, ['foo'], [], () => {}, Symbol('foo')];
+    invalid.forEach(str => {
+      try {
+        parseScope(str);
+        should.fail();
+      } catch (e) {
+        e.message.should.eql('Invalid parameter: `scope`');
+      }
+    });
+  });
+  it('should throw on empty strings', () => {
+    const invalid = ['', ' ', '      ', '\n', '\t', '\r'];
+    invalid.forEach(str => {
+      try {
+        parseScope(str);
+        should.fail();
+      } catch (e) {
+        e.message.should.eql('Invalid parameter: `scope`');
+      }
+    });
+  });
+  it('should split space-delimited strings into arrays', () => {
+    const values = [
+      ['foo', ['foo']],
+      ['foo bar', ['foo', 'bar']],
+      ['foo       bar', ['foo', 'bar']],
+    ];
+    values.forEach(([str, compare]) => {
+      const parsed = parseScope(str);
+      parsed.should.deep.equal(compare);
+    });
+  });
+});