feat(untrusted-clients): restrict scopes that untrusted clients can r…

…equest

Fixes #243.

config/test.json

@@ -35,6 +35,15 @@
       "redirectUri": "",
       "whitelisted": true,
       "canGrant": false
+    },
+    {
+      "name": "Untrusted",
+      "id": "ea3ca969f8c6bb0e",
+      "hashedSecret": "0000000000000000000000000000000000000000000000000000000000000000",
+      "imageUri": "",
+      "redirectUri": "https://example.domain/return?foo=bar",
+      "whitelisted": false,
+      "canGrant": false
     }
   ],
   "logging": {

docs/api.md

@@ -46,6 +46,7 @@ The currently-defined error responses are:
 | 401 | 111 | unauthorized |
 | 403 | 112 | forbidden |
 | 415 | 113 | invalid content type |
+| 400 | 114 | invalid scopes |
 | 500 | 999 | internal server error |
 
 ## API Endpoints
@@ -470,4 +471,4 @@ A valid request will return JSON with these properties:
 [token]: #post-v1token
 [delete]: #post-v1destroy
 [verify]: #post-v1verify
-[developer-activate]: #post-v1developeractivate
+[developer-activate]: #post-v1developeractivate

lib/error.js

@@ -212,4 +212,15 @@ AppError.invalidContentType = function invalidContentType() {
   });
 };
 
+AppError.invalidScopes = function invalidScopes(scopes) {
+  return new AppError({
+    code: 400,
+    error: 'Invalid scopes',
+    errno: 114,
+    message: 'Invalid scopes'
+  }, {
+    invalidScopes: scopes
+  });
+};
+
 module.exports = AppError;

lib/routes/authorization.js

@@ -4,7 +4,6 @@
 
 const buf = require('buf').hex;
 const hex = require('buf').to.hex;
-const Hapi = require('hapi');
 const Joi = require('joi');
 const URI = require('URIjs');
 
@@ -19,6 +18,12 @@ const verify = require('../browserid');
 const CODE = 'code';
 const TOKEN = 'token';
 
+const UNTRUSTED_CLIENT_ALLOWED_SCOPES = [
+  'profile:uid',
+  'profile:email',
+  'profile:display_name'
+];
+
 /*jshint camelcase: false*/
 
 function set(arr) {
@@ -29,15 +34,30 @@ function set(arr) {
   return Object.keys(obj);
 }
 
-
+function scopeStringToSet(scopes) {
+  return typeof scopes === 'string' ?
+    set(scopes.split(/\s+/g)) :
+    scopes;
+}
 
 function isLocalHost(url) {
   var host = new URI(url).hostname();
   return host === 'localhost' || host === '127.0.0.1';
 }
 
-function generateCode(claims, client, scope, req) {
+function detectInvalidScopes(requestedScopes, validScopes) {
+  var invalidScopes = [];
+
+  requestedScopes.forEach(function(scope) {
+    if (validScopes.indexOf(scope) === -1) {
+      invalidScopes.push(scope);
+    }
+  });
+
+  return invalidScopes;
+}
 
+function generateCode(claims, client, scope, req) {
   return db.generateCode(
     client.id,
     buf(claims.uid),
@@ -127,6 +147,7 @@ module.exports = {
     var start = Date.now();
     var wantsGrant = req.payload.response_type === TOKEN;
     var exitEarly = false;
+    var scope = req.payload.scope ? scopeStringToSet(req.payload.scope) : [];
     P.all([
       verify(req.payload.assertion).then(function(claims) {
         logger.info('time.browserid_verify', { ms: Date.now() - start });
@@ -146,9 +167,12 @@ module.exports = {
           logger.debug('notFound', { id: req.payload.client_id });
           throw AppError.unknownClient(req.payload.client_id);
         } else if (!client.whitelisted) {
-          logger.error('notWhitelisted', { id: req.payload.client_id });
-          // TODO: implement external clients so we can remove this
-          throw Hapi.error.notImplemented();
+          var invalidScopes = detectInvalidScopes(scope,
+                                UNTRUSTED_CLIENT_ALLOWED_SCOPES);
+
+          if (invalidScopes.length) {
+            throw AppError.invalidScopes(invalidScopes);
+          }
         }
 
         var uri = req.payload.redirect_uri || client.redirectUri;
@@ -178,8 +202,7 @@ module.exports = {
 
         return client;
       }),
-      // make scope a set
-      req.payload.scope ? set(req.payload.scope.split(' ')) : [],
+      scope,
       req
     ])
     .spread(wantsGrant ? generateGrant : generateCode)

test/api.js

@@ -157,6 +157,12 @@ function getUniqueUserAndToken(cId) {
   });
 }
 
+function clientByName(name) {
+  return config.get('clients').reduce(function (client, lastClient) {
+    return client.name === name ? client : lastClient;
+  });
+}
+
 
 describe('/v1', function() {
   before(function(done) {
@@ -166,7 +172,7 @@ describe('/v1', function() {
         AN_ASSERTION = ass;
       }),
       db.ping().then(function() {
-        client = config.get('clients')[0];
+        client = clientByName('Mocha');
         clientId = client.id;
         assert.equal(encrypt.hash(secret).toString('hex'), client.hashedSecret);
         badSecret = Buffer(secret, 'hex').slice();
@@ -244,6 +250,40 @@ describe('/v1', function() {
       });
     });
 
+    describe('untrusted client scope', function() {
+      it('should fail if invalid scopes', function() {
+        var client = clientByName('Untrusted');
+        mockAssertion().reply(200, VERIFY_GOOD);
+        return Server.api.post({
+          url: '/authorization',
+          payload: authParams({
+            client_id: client.id,
+            scope: 'profile profile:write profile:uid'
+          })
+        }).then(function(res) {
+          assert.equal(res.statusCode, 400);
+          assert.equal(res.result.errno, 114);
+          assert.ok(res.result.invalidScopes.indexOf('profile') !== -1);
+          assert.ok(res.result.invalidScopes.indexOf('profile:write') !== -1);
+          assert.ok(res.result.invalidScopes.indexOf('profile:uid') === -1);
+        });
+      });
+
+      it('should succeed if valid scope', function() {
+        var client = clientByName('Untrusted');
+        mockAssertion().reply(200, VERIFY_GOOD);
+        return Server.api.post({
+          url: '/authorization',
+          payload: authParams({
+            client_id: client.id,
+            scope: 'profile:email profile:uid'
+          })
+        }).then(function(res) {
+          assert.equal(res.statusCode, 200);
+        });
+      });
+    });
+
     describe('?client_id', function() {
 
       it('is required', function(done) {
@@ -458,7 +498,7 @@ describe('/v1', function() {
       });
 
       describe('token', function() {
-        var client2 = config.get('clients')[1];
+        var client2 = clientByName('Admin');
         assert(client2.canGrant); //sanity check
 
         it('does not require state argument', function() {