From 996a9b9978c7c6ade0a8b7bd325a749b7c9f6300 Mon Sep 17 00:00:00 2001 From: Mark Stosberg Date: Fri, 2 Apr 2021 11:02:07 -0400 Subject: [PATCH 1/2] chore: upgrade deps This addresses several deprecations and vulns in the dependency chain. --- package.json | 20 ++++++++++---------- test/integration/express/app.spec.js | 26 ++++++++++---------------- 2 files changed, 20 insertions(+), 26 deletions(-) diff --git a/package.json b/package.json index 4b00bb3..83d9420 100644 --- a/package.json +++ b/package.json @@ -29,15 +29,15 @@ "url": "https://github.com/tschaub/authorized/issues" }, "devDependencies": { - "chai": "1.5.0", - "mocha": "1.9.0", - "grunt": "0.4.1", - "grunt-contrib-jshint": "0.4.3", - "grunt-cafe-mocha": "0.1.2", - "grunt-contrib-watch": "0.3.1", - "grunt-cli": "0.1.7", - "express": "3.2.0", - "chai-http": "0.3.0" + "chai": "^1.5.0", + "chai-http": "^4.3.0", + "express": "^4.17.1", + "grunt": "^1.3.0", + "grunt-cafe-mocha": "^0.1.2", + "grunt-cli": "^1.4.1", + "grunt-contrib-jshint": "^3.0.0", + "grunt-contrib-watch": "^1.1.0", + "mocha": "^8.3.2" }, "main": "./lib/authorized", "scripts": { @@ -45,7 +45,7 @@ "watch": "grunt watch" }, "dependencies": { - "async": "0.9.0", + "async": "^0.9.0", "pause": "0.0.1" } } diff --git a/test/integration/express/app.spec.js b/test/integration/express/app.spec.js index 77c3c92..b3639fc 100644 --- a/test/integration/express/app.spec.js +++ b/test/integration/express/app.spec.js @@ -56,7 +56,8 @@ describe('Usage in Express app', function() { express.json(), function(req, res, next) { var view = auth.view(req); - res.send(202, { + res.status(202) + .send({ roles: view.roles, entities: view.entities, actions: view.actions @@ -65,7 +66,8 @@ describe('Usage in Express app', function() { app.use(function(err, req, res, next) { if (err instanceof UnauthorizedError) { - res.send(401, 'Unauthorized'); + res.status(401) + .send('Unauthorized'); } else { next(err); } @@ -77,10 +79,8 @@ describe('Usage in Express app', function() { chai.request(app) .post('/organizations/org1/members') - .req(function(req) { - req.set('x-fake-user-id', 'user.1'); - }) - .res(function(res) { + .set('x-fake-user-id', 'user.1') + .end(function(err, res) { assert.strictEqual(res.status, 202); var body = res.body; assert.isFalse(body.roles.admin, 'not admin'); @@ -95,16 +95,10 @@ describe('Usage in Express app', function() { chai.request(app) .post('/organizations/org1/members') - .req(function(req) { - req.set('x-fake-user-id', 'user.2'); - }) - .res(function(res) { - assert.strictEqual(res.status, 401); - }); - + .set('x-fake-user-id', 'user.2') + .end(function(err, res) { + assert.strictEqual(res.status, 401); + }); }); - }); - - }); From e7e2768448685e9c6333f6ad5840aa59d50d31a0 Mon Sep 17 00:00:00 2001 From: Mark Stosberg Date: Fri, 2 Apr 2021 11:38:09 -0400 Subject: [PATCH 2/2] Breaking change: throw when attempting to re-define actions, roles or entities. This prevents accidentally overwriting authorization rules with weaker rules. Such an accident becomes more likely as apps grow in size and could lead to an security vulnerability. Fixes #11 --- Gruntfile.js | 1 + README.md | 6 +++--- lib/manager.js | 15 +++++++++++++++ package.json | 3 +++ test/lib/manager.spec.js | 39 +++++++++++++++++++++++++++++++++------ 5 files changed, 55 insertions(+), 9 deletions(-) diff --git a/Gruntfile.js b/Gruntfile.js index f220ec5..888661a 100644 --- a/Gruntfile.js +++ b/Gruntfile.js @@ -11,6 +11,7 @@ module.exports = function(grunt) { var testSrc = 'test/**/*.js'; var lintOptions = { + esversion: 6, curly: true, eqeqeq: true, indent: 2, diff --git a/README.md b/README.md index 073c71e..4979210 100644 --- a/README.md +++ b/README.md @@ -172,7 +172,7 @@ Register a getter for a role. If the role is a string of the form `entity.relation`, a getter for the entity must be registered with the [`entity`](#manager.entity) method. Roles without `.` are "simple" roles (e.g. `"admin"`) and no entity is looked up. Throws [`ConfigError`](#configerror) if -called with an invalid role name. +called with an invalid or duplicate role name. #### `entity(type, getter)` @@ -183,7 +183,7 @@ called with an invalid role name. generated while getting the entity and the second is the target entity. Register a getter for an entity. Throws [`ConfigError`](#configerror) if called -with invalid arguments. +with invalid arguments or duplicate entity name. #### `action(name, roles)` @@ -194,7 +194,7 @@ with invalid arguments. Specify the roles that a user must have to perform the named action. Throws [`ConfigError`](#configerror) if the provided roles have not yet been registered -with the [`role`](#manager.role) method. +with the [`role`](#manager.role) method or if the action name is a duplicate. #### `can(action)` diff --git a/lib/manager.js b/lib/manager.js index 363b4f8..a162bef 100644 --- a/lib/manager.js +++ b/lib/manager.js @@ -73,6 +73,11 @@ Manager.prototype.action = function(name, roles) { } return role; }); + + if (name in this.actionDefs_) { + throw new errors.ConfigError(`action already registered: ${name}`); + } + this.actionDefs_[name] = roles; }; @@ -181,6 +186,11 @@ Manager.prototype.entity = function(type, getter) { throw new errors.ConfigError( 'Entity getter must be a function that takes two arguments'); } + + if (type in this.entityGetters_) { + throw new errors.ConfigError(`entity already registered: ${type}`); + } + this.entityGetters_[type] = getter; }; @@ -294,6 +304,11 @@ Manager.prototype.role = function(role, getter) { throw new errors.ConfigError( 'Getters for simple roles (without entities) take two arguments'); } + + if (role.name in this.roleGetters_) { + throw new errors.ConfigError(`role already registered: ${role.name}`); + } + this.roleGetters_[role.name] = getter; }; diff --git a/package.json b/package.json index 83d9420..3a3e4fc 100644 --- a/package.json +++ b/package.json @@ -47,5 +47,8 @@ "dependencies": { "async": "^0.9.0", "pause": "0.0.1" + }, + "engines": { + "node": ">=10" } } diff --git a/test/lib/manager.spec.js b/test/lib/manager.spec.js index 698ab4f..a147a0b 100644 --- a/test/lib/manager.spec.js +++ b/test/lib/manager.spec.js @@ -107,16 +107,25 @@ describe('Manager', function() { // pretend nobody is admin done(null, false); }); - auth.role('page.author', function(page, req, done) { - // pretend everybody is author - done(null, true); - }); assert.throws(function() { auth.action('can edit page', ['admin', 'page.author']); }); }); + it('throws when attempting to define action twice', function() { + auth.role('admin', function(req, done) { + done(null, false); + }); + + auth.action('can edit page', ['admin']); + + assert.throws(function() { + auth.action('can edit page', ['admin']); + }); + }); + + }); describe('#can()', function() { @@ -318,6 +327,16 @@ describe('Manager', function() { }, ConfigError); }); + it('throws when attempting to define entity twice', function() { + assert.doesNotThrow(function() { + auth.entity('page', (req, done) => done(null, {})); + }); + assert.throws(() => { + auth.entity('page', (req, done) => done(null, {})); + }); + }); + + }); describe('#role()', function() { @@ -405,7 +424,15 @@ describe('Manager', function() { }, ConfigError); }); - }); - + it('throws when attempting to define a role twice', function() { + auth.role('admin', function(req, done) { + // pretend nobody is admin + done(null, false); + }); + assert.throws(() => { + auth.role('admin', (req, done) => done(null, false)); + }); + }); + }); });