diff --git a/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js b/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js index 5c9eedeb2a..19b70dd668 100644 --- a/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js +++ b/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js @@ -4,6 +4,7 @@ const async = require('async') const logger = require('logger-sharelatex') const { ObjectId } = require('mongojs') const Errors = require('../Errors/Errors') +const HttpErrors = require('@overleaf/o-error/http') const AuthenticationController = require('../Authentication/AuthenticationController') const TokenAccessHandler = require('../TokenAccess/TokenAccessHandler') @@ -180,7 +181,7 @@ module.exports = AuthorizationMiddleware = { { userId, projectId }, 'denying user admin access to project' ) - AuthorizationMiddleware.redirectToRestricted(req, res, next) + next(new HttpErrors.ForbiddenError({})) } ) }) diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsRouter.js b/services/web/app/src/Features/Collaborators/CollaboratorsRouter.js index 6b9889646d..a2176ab743 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsRouter.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsRouter.js @@ -25,6 +25,7 @@ module.exports = { webRouter.delete( '/project/:Project_id/users/:user_id', + AuthenticationController.requireLogin(), AuthorizationMiddleware.ensureUserCanAdminProject, CollaboratorsController.removeUserFromProject ) diff --git a/services/web/app/src/Features/Errors/HttpErrorController.js b/services/web/app/src/Features/Errors/HttpErrorController.js index 3ee5dcc5a7..f4e05b3063 100644 --- a/services/web/app/src/Features/Errors/HttpErrorController.js +++ b/services/web/app/src/Features/Errors/HttpErrorController.js @@ -8,7 +8,7 @@ function renderHTMLError(statusCode, publicInfo, res) { if (statusCode === 404) { res.render('general/404', { title: 'page_not_found' }) } else if (statusCode === 403) { - res.render('user/restricted') + res.render('user/restricted', { title: 'restricted' }) } else if (statusCode >= 400 && statusCode < 500) { res.render('general/500', { title: 'Client Error' }) } else { diff --git a/services/web/app/src/router.js b/services/web/app/src/router.js index 127bd9fa45..c6dd5a2bf3 100644 --- a/services/web/app/src/router.js +++ b/services/web/app/src/router.js @@ -308,6 +308,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { ) webRouter.post( '/project/:Project_id/settings/admin', + AuthenticationController.requireLogin(), AuthorizationMiddleware.ensureUserCanAdminProject, ProjectController.updateProjectAdminSettings ) @@ -462,11 +463,13 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.delete( '/Project/:Project_id', + AuthenticationController.requireLogin(), AuthorizationMiddleware.ensureUserCanAdminProject, ProjectController.deleteProject ) webRouter.post( '/Project/:Project_id/restore', + AuthenticationController.requireLogin(), AuthorizationMiddleware.ensureUserCanAdminProject, ProjectController.restoreProject ) @@ -478,6 +481,7 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { webRouter.post( '/project/:Project_id/rename', + AuthenticationController.requireLogin(), AuthorizationMiddleware.ensureUserCanAdminProject, ProjectController.renameProject ) diff --git a/services/web/test/acceptance/src/AuthorizationTests.js b/services/web/test/acceptance/src/AuthorizationTests.js index 4c2e5b92de..3d3d9de2c9 100644 --- a/services/web/test/acceptance/src/AuthorizationTests.js +++ b/services/web/test/acceptance/src/AuthorizationTests.js @@ -1,59 +1,46 @@ -/* eslint-disable - camelcase, - max-len, - no-undef, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const { expect } = require('chai') const async = require('async') const User = require('./helpers/User') const request = require('./helpers/request') const settings = require('settings-sharelatex') -const MockDocstoreApi = require('./helpers/MockDocstoreApi') -const MockDocUpdaterApi = require('./helpers/MockDocUpdaterApi') +require('./helpers/MockDocstoreApi') +require('./helpers/MockDocUpdaterApi') -const try_read_access = (user, project_id, test, callback) => +function tryReadAccess(user, projectId, test, callback) { async.series( [ cb => - user.request.get(`/project/${project_id}`, (error, response, body) => { + user.request.get(`/project/${projectId}`, (error, response, body) => { if (error != null) { return cb(error) } test(response, body) - return cb() + cb() }), cb => user.request.get( - `/project/${project_id}/download/zip`, + `/project/${projectId}/download/zip`, (error, response, body) => { if (error != null) { return cb(error) } test(response, body) - return cb() + cb() } ) ], callback ) +} -const try_settings_write_access = (user, project_id, test, callback) => +function trySettingsWriteAccess(user, projectId, test, callback) { async.series( [ cb => user.request.post( { - uri: `/project/${project_id}/settings`, + uri: `/project/${projectId}/settings`, json: { compiler: 'latex' } @@ -63,20 +50,21 @@ const try_settings_write_access = (user, project_id, test, callback) => return cb(error) } test(response, body) - return cb() + cb() } ) ], callback ) +} -const try_admin_access = (user, project_id, test, callback) => +function tryAdminAccess(user, projectId, test, callback) { async.series( [ cb => user.request.post( { - uri: `/project/${project_id}/rename`, + uri: `/project/${projectId}/rename`, json: { newProjectName: 'new-name' } @@ -86,13 +74,13 @@ const try_admin_access = (user, project_id, test, callback) => return cb(error) } test(response, body) - return cb() + cb() } ), cb => user.request.post( { - uri: `/project/${project_id}/settings/admin`, + uri: `/project/${projectId}/settings/admin`, json: { publicAccessLevel: 'private' } @@ -102,26 +90,27 @@ const try_admin_access = (user, project_id, test, callback) => return cb(error) } test(response, body) - return cb() + cb() } ) ], callback ) +} -const try_content_access = function(user, project_id, test, callback) { +function tryContentAccess(user, projectId, test, callback) { // The real-time service calls this end point to determine the user's // permissions. - let user_id + let userId if (user.id != null) { - user_id = user.id + userId = user.id } else { - user_id = 'anonymous-user' + userId = 'anonymous-user' } - return request.post( + request.post( { - url: `/project/${project_id}/join`, - qs: { user_id }, + url: `/project/${projectId}/join`, + qs: { user_id: userId }, auth: { user: settings.apis.web.user, pass: settings.apis.web.pass, @@ -135,26 +124,26 @@ const try_content_access = function(user, project_id, test, callback) { return callback(error) } test(response, body) - return callback() + callback() } ) } -const expect_read_access = (user, project_id, callback) => +function expectReadAccess(user, projectId, callback) { async.series( [ cb => - try_read_access( + tryReadAccess( user, - project_id, + projectId, (response, body) => expect(response.statusCode).to.be.oneOf([200, 204]), cb ), cb => - try_content_access( + tryContentAccess( user, - project_id, + projectId, (response, body) => expect(body.privilegeLevel).to.be.oneOf([ 'owner', @@ -166,92 +155,109 @@ const expect_read_access = (user, project_id, callback) => ], callback ) +} -const expect_content_write_access = (user, project_id, callback) => - try_content_access( +function expectContentWriteAccess(user, projectId, callback) { + tryContentAccess( user, - project_id, + projectId, (response, body) => expect(body.privilegeLevel).to.be.oneOf(['owner', 'readAndWrite']), callback ) +} -const expect_settings_write_access = (user, project_id, callback) => - try_settings_write_access( +function expectSettingsWriteAccess(user, projectId, callback) { + trySettingsWriteAccess( user, - project_id, + projectId, (response, body) => expect(response.statusCode).to.be.oneOf([200, 204]), callback ) +} -const expect_admin_access = (user, project_id, callback) => - try_admin_access( +function expectAdminAccess(user, projectId, callback) { + tryAdminAccess( user, - project_id, + projectId, (response, body) => expect(response.statusCode).to.be.oneOf([200, 204]), callback ) +} -const expect_no_read_access = (user, project_id, options, callback) => +function expectNoReadAccess(user, projectId, options, callback) { async.series( [ cb => - try_read_access( + tryReadAccess( user, - project_id, + projectId, (response, body) => { expect(response.statusCode).to.equal(302) - return expect(response.headers.location).to.match( + expect(response.headers.location).to.match( new RegExp(options.redirect_to) ) }, cb ), cb => - try_content_access( + tryContentAccess( user, - project_id, + projectId, (response, body) => expect(body.privilegeLevel).to.be.equal(false), cb ) ], callback ) +} -const expect_no_content_write_access = (user, project_id, callback) => - try_content_access( +function expectNoContentWriteAccess(user, projectId, callback) { + tryContentAccess( user, - project_id, + projectId, (response, body) => expect(body.privilegeLevel).to.be.oneOf([false, 'readOnly']), callback ) +} -const expect_no_settings_write_access = (user, project_id, options, callback) => - try_settings_write_access( +function expectNoSettingsWriteAccess(user, projectId, options, callback) { + trySettingsWriteAccess( user, - project_id, + projectId, (response, body) => { expect(response.statusCode).to.equal(302) - return expect(response.headers.location).to.match( + expect(response.headers.location).to.match( new RegExp(options.redirect_to) ) }, callback ) +} -const expect_no_admin_access = (user, project_id, options, callback) => - try_admin_access( +function expectNoAdminAccess(user, projectId, callback) { + tryAdminAccess( user, - project_id, + projectId, (response, body) => { - expect(response.statusCode).to.equal(302) - return expect(response.headers.location).to.match( - new RegExp(options.redirect_to) - ) + expect(response.statusCode).to.equal(403) }, callback ) +} + +function expectNoAnonymousAdminAccess(user, projectId, callback) { + tryAdminAccess( + user, + projectId, + (response, body) => { + expect(response.statusCode).to.equal(302) + expect(response.headers.location).to.match(/^\/login/) + }, + callback + ) +} describe('Authorization', function() { beforeEach(function(done) { @@ -261,18 +267,18 @@ describe('Authorization', function() { this.other2 = new User() this.anon = new User() this.site_admin = new User({ email: 'admin@example.com' }) - return async.parallel( + async.parallel( [ cb => this.owner.login(cb), cb => this.other1.login(cb), cb => this.other2.login(cb), cb => this.anon.getCsrfToken(cb), cb => { - return this.site_admin.login(err => { - if (typeof error !== 'undefined' && error !== null) { + this.site_admin.login(err => { + if (err != null) { return cb(err) } - return this.site_admin.ensure_admin(cb) + this.site_admin.ensure_admin(cb) }) } ], @@ -282,114 +288,97 @@ describe('Authorization', function() { describe('private project', function() { beforeEach(function(done) { - return this.owner.createProject( - 'private-project', - (error, project_id) => { - if (error != null) { - return done(error) - } - this.project_id = project_id - return done() + this.owner.createProject('private-project', (error, projectId) => { + if (error != null) { + return done(error) } - ) + this.projectId = projectId + done() + }) }) it('should allow the owner read access to it', function(done) { - return expect_read_access(this.owner, this.project_id, done) + expectReadAccess(this.owner, this.projectId, done) }) it('should allow the owner write access to its content', function(done) { - return expect_content_write_access(this.owner, this.project_id, done) + expectContentWriteAccess(this.owner, this.projectId, done) }) it('should allow the owner write access to its settings', function(done) { - return expect_settings_write_access(this.owner, this.project_id, done) + expectSettingsWriteAccess(this.owner, this.projectId, done) }) it('should allow the owner admin access to it', function(done) { - return expect_admin_access(this.owner, this.project_id, done) + expectAdminAccess(this.owner, this.projectId, done) }) it('should not allow another user read access to the project', function(done) { - return expect_no_read_access( + expectNoReadAccess( this.other1, - this.project_id, + this.projectId, { redirect_to: '/restricted' }, done ) }) it('should not allow another user write access to its content', function(done) { - return expect_no_content_write_access(this.other1, this.project_id, done) + expectNoContentWriteAccess(this.other1, this.projectId, done) }) it('should not allow another user write access to its settings', function(done) { - return expect_no_settings_write_access( + expectNoSettingsWriteAccess( this.other1, - this.project_id, + this.projectId, { redirect_to: '/restricted' }, done ) }) it('should not allow another user admin access to it', function(done) { - return expect_no_admin_access( - this.other1, - this.project_id, - { redirect_to: '/restricted' }, - done - ) + expectNoAdminAccess(this.other1, this.projectId, done) }) it('should not allow anonymous user read access to it', function(done) { - return expect_no_read_access( + expectNoReadAccess( this.anon, - this.project_id, + this.projectId, { redirect_to: '/restricted' }, done ) }) it('should not allow anonymous user write access to its content', function(done) { - return expect_no_content_write_access(this.anon, this.project_id, done) + expectNoContentWriteAccess(this.anon, this.projectId, done) }) it('should not allow anonymous user write access to its settings', function(done) { - return expect_no_settings_write_access( + expectNoSettingsWriteAccess( this.anon, - this.project_id, + this.projectId, { redirect_to: '/restricted' }, done ) }) it('should not allow anonymous user admin access to it', function(done) { - return expect_no_admin_access( - this.anon, - this.project_id, - { redirect_to: '/restricted' }, - done - ) + expectNoAnonymousAdminAccess(this.anon, this.projectId, done) }) it('should allow site admin users read access to it', function(done) { - return expect_read_access(this.site_admin, this.project_id, done) + expectReadAccess(this.site_admin, this.projectId, done) }) it('should allow site admin users write access to its content', function(done) { - return expect_content_write_access(this.site_admin, this.project_id, done) + expectContentWriteAccess(this.site_admin, this.projectId, done) }) it('should allow site admin users write access to its settings', function(done) { - return expect_settings_write_access( - this.site_admin, - this.project_id, - done - ) + expectSettingsWriteAccess(this.site_admin, this.projectId, done) }) it('should allow site admin users admin access to it', function(done) { - return expect_admin_access(this.site_admin, this.project_id, done) + expectAdminAccess(this.site_admin, this.projectId, done) }) }) @@ -397,217 +386,178 @@ describe('Authorization', function() { beforeEach(function(done) { this.rw_user = this.other1 this.ro_user = this.other2 - return this.owner.createProject( - 'private-project', - (error, project_id) => { - if (error != null) { - return done(error) - } - this.project_id = project_id - return this.owner.addUserToProject( - this.project_id, - this.ro_user, - 'readOnly', - error => { - if (error != null) { - return done(error) - } - return this.owner.addUserToProject( - this.project_id, - this.rw_user, - 'readAndWrite', - error => { - if (error != null) { - return done(error) - } - return done() - } - ) - } - ) + this.owner.createProject('private-project', (error, projectId) => { + if (error != null) { + return done(error) } - ) + this.projectId = projectId + this.owner.addUserToProject( + this.projectId, + this.ro_user, + 'readOnly', + error => { + if (error != null) { + return done(error) + } + this.owner.addUserToProject( + this.projectId, + this.rw_user, + 'readAndWrite', + error => { + if (error != null) { + return done(error) + } + done() + } + ) + } + ) + }) }) it('should allow the read-only user read access to it', function(done) { - return expect_read_access(this.ro_user, this.project_id, done) + expectReadAccess(this.ro_user, this.projectId, done) }) it('should not allow the read-only user write access to its content', function(done) { - return expect_no_content_write_access(this.ro_user, this.project_id, done) + expectNoContentWriteAccess(this.ro_user, this.projectId, done) }) it('should not allow the read-only user write access to its settings', function(done) { - return expect_no_settings_write_access( + expectNoSettingsWriteAccess( this.ro_user, - this.project_id, + this.projectId, { redirect_to: '/restricted' }, done ) }) it('should not allow the read-only user admin access to it', function(done) { - return expect_no_admin_access( - this.ro_user, - this.project_id, - { redirect_to: '/restricted' }, - done - ) + expectNoAdminAccess(this.ro_user, this.projectId, done) }) it('should allow the read-write user read access to it', function(done) { - return expect_read_access(this.rw_user, this.project_id, done) + expectReadAccess(this.rw_user, this.projectId, done) }) it('should allow the read-write user write access to its content', function(done) { - return expect_content_write_access(this.rw_user, this.project_id, done) + expectContentWriteAccess(this.rw_user, this.projectId, done) }) it('should allow the read-write user write access to its settings', function(done) { - return expect_settings_write_access(this.rw_user, this.project_id, done) + expectSettingsWriteAccess(this.rw_user, this.projectId, done) }) it('should not allow the read-write user admin access to it', function(done) { - return expect_no_admin_access( - this.rw_user, - this.project_id, - { redirect_to: '/restricted' }, - done - ) + expectNoAdminAccess(this.rw_user, this.projectId, done) }) }) describe('public read-write project', function() { beforeEach(function(done) { - return this.owner.createProject( - 'public-rw-project', - (error, project_id) => { - if (error != null) { - return done(error) - } - this.project_id = project_id - return this.owner.makePublic(this.project_id, 'readAndWrite', done) + this.owner.createProject('public-rw-project', (error, projectId) => { + if (error != null) { + return done(error) } - ) + this.projectId = projectId + this.owner.makePublic(this.projectId, 'readAndWrite', done) + }) }) it('should allow a user read access to it', function(done) { - return expect_read_access(this.other1, this.project_id, done) + expectReadAccess(this.other1, this.projectId, done) }) it('should allow a user write access to its content', function(done) { - return expect_content_write_access(this.other1, this.project_id, done) + expectContentWriteAccess(this.other1, this.projectId, done) }) it('should not allow a user write access to its settings', function(done) { - return expect_no_settings_write_access( + expectNoSettingsWriteAccess( this.other1, - this.project_id, + this.projectId, { redirect_to: '/restricted' }, done ) }) it('should not allow a user admin access to it', function(done) { - return expect_no_admin_access( - this.other1, - this.project_id, - { redirect_to: '/restricted' }, - done - ) + expectNoAdminAccess(this.other1, this.projectId, done) }) it('should allow an anonymous user read access to it', function(done) { - return expect_read_access(this.anon, this.project_id, done) + expectReadAccess(this.anon, this.projectId, done) }) it('should allow an anonymous user write access to its content', function(done) { - return expect_content_write_access(this.anon, this.project_id, done) + expectContentWriteAccess(this.anon, this.projectId, done) }) it('should not allow an anonymous user write access to its settings', function(done) { - return expect_no_settings_write_access( + expectNoSettingsWriteAccess( this.anon, - this.project_id, + this.projectId, { redirect_to: '/restricted' }, done ) }) it('should not allow an anonymous user admin access to it', function(done) { - return expect_no_admin_access( - this.anon, - this.project_id, - { redirect_to: '/restricted' }, - done - ) + expectNoAnonymousAdminAccess(this.anon, this.projectId, done) }) }) describe('public read-only project', function() { beforeEach(function(done) { - return this.owner.createProject( - 'public-ro-project', - (error, project_id) => { - if (error != null) { - return done(error) - } - this.project_id = project_id - return this.owner.makePublic(this.project_id, 'readOnly', done) + this.owner.createProject('public-ro-project', (error, projectId) => { + if (error != null) { + return done(error) } - ) + this.projectId = projectId + this.owner.makePublic(this.projectId, 'readOnly', done) + }) }) it('should allow a user read access to it', function(done) { - return expect_read_access(this.other1, this.project_id, done) + expectReadAccess(this.other1, this.projectId, done) }) it('should not allow a user write access to its content', function(done) { - return expect_no_content_write_access(this.other1, this.project_id, done) + expectNoContentWriteAccess(this.other1, this.projectId, done) }) it('should not allow a user write access to its settings', function(done) { - return expect_no_settings_write_access( + expectNoSettingsWriteAccess( this.other1, - this.project_id, + this.projectId, { redirect_to: '/restricted' }, done ) }) it('should not allow a user admin access to it', function(done) { - return expect_no_admin_access( - this.other1, - this.project_id, - { redirect_to: '/restricted' }, - done - ) + expectNoAdminAccess(this.other1, this.projectId, done) }) it('should allow an anonymous user read access to it', function(done) { - return expect_read_access(this.anon, this.project_id, done) + expectReadAccess(this.anon, this.projectId, done) }) it('should not allow an anonymous user write access to its content', function(done) { - return expect_no_content_write_access(this.anon, this.project_id, done) + expectNoContentWriteAccess(this.anon, this.projectId, done) }) it('should not allow an anonymous user write access to its settings', function(done) { - return expect_no_settings_write_access( + expectNoSettingsWriteAccess( this.anon, - this.project_id, + this.projectId, { redirect_to: '/restricted' }, done ) }) it('should not allow an anonymous user admin access to it', function(done) { - return expect_no_admin_access( - this.anon, - this.project_id, - { redirect_to: '/restricted' }, - done - ) + expectNoAnonymousAdminAccess(this.anon, this.projectId, done) }) }) }) diff --git a/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js b/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js index 0e763a4a13..e64a027345 100644 --- a/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js +++ b/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js @@ -1,76 +1,68 @@ -/* eslint-disable - camelcase, - max-len, - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const sinon = require('sinon') -const chai = require('chai') -const should = chai.should() -const { expect } = chai -const modulePath = - '../../../../app/src/Features/Authorization/AuthorizationMiddleware.js' +const { expect } = require('chai') const SandboxedModule = require('sandboxed-module') +const HttpErrors = require('@overleaf/o-error/http') const Errors = require('../../../../app/src/Features/Errors/Errors.js') +const MODULE_PATH = + '../../../../app/src/Features/Authorization/AuthorizationMiddleware.js' + describe('AuthorizationMiddleware', function() { beforeEach(function() { - this.user_id = 'user-id-123' + this.userId = 'user-id-123' this.project_id = 'project-id-123' this.token = 'some-token' this.AuthenticationController = { - getLoggedInUserId: sinon.stub().returns(this.user_id), + getLoggedInUserId: sinon.stub().returns(this.userId), isUserLoggedIn: sinon.stub().returns(true) } - this.AuthorizationMiddleware = SandboxedModule.require(modulePath, { + this.AuthorizationManager = {} + this.TokenAccessHandler = { + getRequestToken: sinon.stub().returns(this.token) + } + this.ObjectId = { + isValid: sinon + .stub() + .withArgs(this.project_id) + .returns(true) + } + this.AuthorizationManager = {} + this.AuthorizationMiddleware = SandboxedModule.require(MODULE_PATH, { globals: { console: console }, requires: { - './AuthorizationManager': (this.AuthorizationManager = {}), + './AuthorizationManager': this.AuthorizationManager, 'logger-sharelatex': { log() {} }, mongojs: { - ObjectId: (this.ObjectId = {}) + ObjectId: this.ObjectId }, + '@overleaf/o-error/http': HttpErrors, '../Errors/Errors': Errors, '../Authentication/AuthenticationController': this .AuthenticationController, - '../TokenAccess/TokenAccessHandler': (this.TokenAccessHandler = { - getRequestToken: sinon.stub().returns(this.token) - }) + '../TokenAccess/TokenAccessHandler': this.TokenAccessHandler } }) this.req = {} this.res = {} - this.ObjectId.isValid = sinon.stub() - this.ObjectId.isValid.withArgs(this.project_id).returns(true) - return (this.next = sinon.stub()) + this.next = sinon.stub() }) describe('_getUserId', function() { beforeEach(function() { - return (this.req = {}) + this.req = {} }) it('should get the user from session', function(done) { this.AuthenticationController.getLoggedInUserId = sinon .stub() .returns('1234') - return this.AuthorizationMiddleware._getUserId( - this.req, - (err, user_id) => { - expect(err).to.not.exist - expect(user_id).to.equal('1234') - return done() - } - ) + this.AuthorizationMiddleware._getUserId(this.req, (err, userId) => { + expect(err).to.not.exist + expect(userId).to.equal('1234') + done() + }) }) it('should get oauth_user from request', function(done) { @@ -78,14 +70,11 @@ describe('AuthorizationMiddleware', function() { .stub() .returns(null) this.req.oauth_user = { _id: '5678' } - return this.AuthorizationMiddleware._getUserId( - this.req, - (err, user_id) => { - expect(err).to.not.exist - expect(user_id).to.equal('5678') - return done() - } - ) + this.AuthorizationMiddleware._getUserId(this.req, (err, userId) => { + expect(err).to.not.exist + expect(userId).to.equal('5678') + done() + }) }) it('should fall back to null', function(done) { @@ -93,36 +82,31 @@ describe('AuthorizationMiddleware', function() { .stub() .returns(null) this.req.oauth_user = undefined - return this.AuthorizationMiddleware._getUserId( - this.req, - (err, user_id) => { - expect(err).to.not.exist - expect(user_id).to.equal(null) - return done() - } - ) + this.AuthorizationMiddleware._getUserId(this.req, (err, userId) => { + expect(err).to.not.exist + expect(userId).to.equal(null) + done() + }) }) }) const METHODS_TO_TEST = { ensureUserCanReadProject: 'canUserReadProject', ensureUserCanWriteProjectSettings: 'canUserWriteProjectSettings', - ensureUserCanWriteProjectContent: 'canUserWriteProjectContent', - ensureUserCanAdminProject: 'canUserAdminProject' + ensureUserCanWriteProjectContent: 'canUserWriteProjectContent' } - for (let middlewareMethod in METHODS_TO_TEST) { - const managerMethod = METHODS_TO_TEST[middlewareMethod] - ;((middlewareMethod, managerMethod) => + Object.entries(METHODS_TO_TEST).forEach( + ([middlewareMethod, managerMethod]) => { describe(middlewareMethod, function() { beforeEach(function() { this.req.params = { project_id: this.project_id } this.AuthorizationManager[managerMethod] = sinon.stub() - return (this.AuthorizationMiddleware.redirectToRestricted = sinon.stub()) + this.AuthorizationMiddleware.redirectToRestricted = sinon.stub() }) describe('with missing project_id', function() { beforeEach(function() { - return (this.req.params = {}) + this.req.params = {} }) it('should return an error to next', function() { @@ -131,21 +115,19 @@ describe('AuthorizationMiddleware', function() { this.res, this.next ) - return this.next.calledWith(new Error()).should.equal(true) + this.next.calledWith(new Error()).should.equal(true) }) }) describe('with logged in user', function() { beforeEach(function() { - return this.AuthenticationController.getLoggedInUserId.returns( - this.user_id - ) + this.AuthenticationController.getLoggedInUserId.returns(this.userId) }) describe('when user has permission', function() { beforeEach(function() { - return this.AuthorizationManager[managerMethod] - .withArgs(this.user_id, this.project_id, this.token) + this.AuthorizationManager[managerMethod] + .withArgs(this.userId, this.project_id, this.token) .yields(null, true) }) @@ -155,25 +137,25 @@ describe('AuthorizationMiddleware', function() { this.res, this.next ) - return this.next.called.should.equal(true) + this.next.called.should.equal(true) }) }) describe("when user doesn't have permission", function() { beforeEach(function() { - return this.AuthorizationManager[managerMethod] - .withArgs(this.user_id, this.project_id, this.token) + this.AuthorizationManager[managerMethod] + .withArgs(this.userId, this.project_id, this.token) .yields(null, false) }) - it('should redirect to redirectToRestricted', function() { + it('should raise a 403', function() { this.AuthorizationMiddleware[middlewareMethod]( this.req, this.res, this.next ) this.next.called.should.equal(false) - return this.AuthorizationMiddleware.redirectToRestricted + this.AuthorizationMiddleware.redirectToRestricted .calledWith(this.req, this.res, this.next) .should.equal(true) }) @@ -184,7 +166,7 @@ describe('AuthorizationMiddleware', function() { describe('when user has permission', function() { beforeEach(function() { this.AuthenticationController.getLoggedInUserId.returns(null) - return this.AuthorizationManager[managerMethod] + this.AuthorizationManager[managerMethod] .withArgs(null, this.project_id, this.token) .yields(null, true) }) @@ -195,14 +177,14 @@ describe('AuthorizationMiddleware', function() { this.res, this.next ) - return this.next.called.should.equal(true) + this.next.called.should.equal(true) }) }) describe("when user doesn't have permission", function() { beforeEach(function() { this.AuthenticationController.getLoggedInUserId.returns(null) - return this.AuthorizationManager[managerMethod] + this.AuthorizationManager[managerMethod] .withArgs(null, this.project_id, this.token) .yields(null, false) }) @@ -214,7 +196,7 @@ describe('AuthorizationMiddleware', function() { this.next ) this.next.called.should.equal(false) - return this.AuthorizationMiddleware.redirectToRestricted + this.AuthorizationMiddleware.redirectToRestricted .calledWith(this.req, this.res, this.next) .should.equal(true) }) @@ -224,11 +206,11 @@ describe('AuthorizationMiddleware', function() { describe('with malformed project id', function() { beforeEach(function() { this.req.params = { project_id: 'blah' } - return (this.ObjectId.isValid = sinon.stub().returns(false)) + this.ObjectId.isValid = sinon.stub().returns(false) }) it('should return a not found error', function(done) { - return this.AuthorizationMiddleware[middlewareMethod]( + this.AuthorizationMiddleware[middlewareMethod]( this.req, this.res, error => { @@ -238,26 +220,148 @@ describe('AuthorizationMiddleware', function() { ) }) }) - }))(middlewareMethod, managerMethod) - } + }) + } + ) - describe('ensureUserIsSiteAdmin', function() { + describe('ensureUserCanAdminProject', function() { beforeEach(function() { - this.AuthorizationManager.isUserSiteAdmin = sinon.stub() - return (this.AuthorizationMiddleware.redirectToRestricted = sinon.stub()) + this.req.params = { project_id: this.project_id } + this.AuthorizationManager.canUserAdminProject = sinon.stub() + this.AuthorizationMiddleware.redirectToRestricted = sinon.stub() + }) + + describe('with missing project_id', function() { + beforeEach(function() { + this.req.params = {} + }) + + it('should return an error to next', function() { + this.AuthorizationMiddleware.ensureUserCanAdminProject( + this.req, + this.res, + this.next + ) + this.next.calledWith(new Error()).should.equal(true) + }) }) describe('with logged in user', function() { beforeEach(function() { - return this.AuthenticationController.getLoggedInUserId.returns( - this.user_id - ) + this.AuthenticationController.getLoggedInUserId.returns(this.userId) }) describe('when user has permission', function() { beforeEach(function() { - return this.AuthorizationManager.isUserSiteAdmin - .withArgs(this.user_id) + this.AuthorizationManager.canUserAdminProject + .withArgs(this.userId, this.project_id, this.token) + .yields(null, true) + }) + + it('should return next', function() { + this.AuthorizationMiddleware.ensureUserCanAdminProject( + this.req, + this.res, + this.next + ) + this.next.called.should.equal(true) + }) + }) + + describe("when user doesn't have permission", function() { + beforeEach(function() { + this.AuthorizationManager.canUserAdminProject + .withArgs(this.userId, this.project_id, this.token) + .yields(null, false) + }) + + it('should raise a 403', function(done) { + this.AuthorizationMiddleware.ensureUserCanAdminProject( + this.req, + this.res, + err => { + expect(err).to.be.an.instanceof(HttpErrors.ForbiddenError) + done() + } + ) + }) + }) + }) + + describe('with anonymous user', function() { + describe('when user has permission', function() { + beforeEach(function() { + this.AuthenticationController.getLoggedInUserId.returns(null) + this.AuthorizationManager.canUserAdminProject + .withArgs(null, this.project_id, this.token) + .yields(null, true) + }) + + it('should return next', function() { + this.AuthorizationMiddleware.ensureUserCanAdminProject( + this.req, + this.res, + this.next + ) + this.next.called.should.equal(true) + }) + }) + + describe("when user doesn't have permission", function() { + beforeEach(function() { + this.AuthenticationController.getLoggedInUserId.returns(null) + this.AuthorizationManager.canUserAdminProject + .withArgs(null, this.project_id, this.token) + .yields(null, false) + }) + + it('should raise a 403', function(done) { + this.AuthorizationMiddleware.ensureUserCanAdminProject( + this.req, + this.res, + err => { + expect(err).to.be.an.instanceof(HttpErrors.ForbiddenError) + done() + } + ) + }) + }) + }) + + describe('with malformed project id', function() { + beforeEach(function() { + this.req.params = { project_id: 'blah' } + this.ObjectId.isValid = sinon.stub().returns(false) + }) + + it('should return a not found error', function(done) { + this.AuthorizationMiddleware.ensureUserCanAdminProject( + this.req, + this.res, + error => { + error.should.be.instanceof(Errors.NotFoundError) + return done() + } + ) + }) + }) + }) + + describe('ensureUserIsSiteAdmin', function() { + beforeEach(function() { + this.AuthorizationManager.isUserSiteAdmin = sinon.stub() + this.AuthorizationMiddleware.redirectToRestricted = sinon.stub() + }) + + describe('with logged in user', function() { + beforeEach(function() { + this.AuthenticationController.getLoggedInUserId.returns(this.userId) + }) + + describe('when user has permission', function() { + beforeEach(function() { + this.AuthorizationManager.isUserSiteAdmin + .withArgs(this.userId) .yields(null, true) }) @@ -267,14 +371,14 @@ describe('AuthorizationMiddleware', function() { this.res, this.next ) - return this.next.called.should.equal(true) + this.next.called.should.equal(true) }) }) describe("when user doesn't have permission", function() { beforeEach(function() { - return this.AuthorizationManager.isUserSiteAdmin - .withArgs(this.user_id) + this.AuthorizationManager.isUserSiteAdmin + .withArgs(this.userId) .yields(null, false) }) @@ -285,7 +389,7 @@ describe('AuthorizationMiddleware', function() { this.next ) this.next.called.should.equal(false) - return this.AuthorizationMiddleware.redirectToRestricted + this.AuthorizationMiddleware.redirectToRestricted .calledWith(this.req, this.res, this.next) .should.equal(true) }) @@ -296,7 +400,7 @@ describe('AuthorizationMiddleware', function() { describe('when user has permission', function() { beforeEach(function() { this.AuthenticationController.getLoggedInUserId.returns(null) - return this.AuthorizationManager.isUserSiteAdmin + this.AuthorizationManager.isUserSiteAdmin .withArgs(null) .yields(null, true) }) @@ -307,14 +411,14 @@ describe('AuthorizationMiddleware', function() { this.res, this.next ) - return this.next.called.should.equal(true) + this.next.called.should.equal(true) }) }) describe("when user doesn't have permission", function() { beforeEach(function() { this.AuthenticationController.getLoggedInUserId.returns(null) - return this.AuthorizationManager.isUserSiteAdmin + this.AuthorizationManager.isUserSiteAdmin .withArgs(null) .yields(null, false) }) @@ -326,7 +430,7 @@ describe('AuthorizationMiddleware', function() { this.next ) this.next.called.should.equal(false) - return this.AuthorizationMiddleware.redirectToRestricted + this.AuthorizationMiddleware.redirectToRestricted .calledWith(this.req, this.res, this.next) .should.equal(true) }) @@ -338,23 +442,21 @@ describe('AuthorizationMiddleware', function() { beforeEach(function() { this.AuthorizationManager.canUserReadProject = sinon.stub() this.AuthorizationMiddleware.redirectToRestricted = sinon.stub() - return (this.req.query = { project_ids: 'project1,project2' }) + this.req.query = { project_ids: 'project1,project2' } }) describe('with logged in user', function() { beforeEach(function() { - return this.AuthenticationController.getLoggedInUserId.returns( - this.user_id - ) + this.AuthenticationController.getLoggedInUserId.returns(this.userId) }) describe('when user has permission to access all projects', function() { beforeEach(function() { this.AuthorizationManager.canUserReadProject - .withArgs(this.user_id, 'project1', this.token) + .withArgs(this.userId, 'project1', this.token) .yields(null, true) - return this.AuthorizationManager.canUserReadProject - .withArgs(this.user_id, 'project2', this.token) + this.AuthorizationManager.canUserReadProject + .withArgs(this.userId, 'project2', this.token) .yields(null, true) }) @@ -364,17 +466,17 @@ describe('AuthorizationMiddleware', function() { this.res, this.next ) - return this.next.called.should.equal(true) + this.next.called.should.equal(true) }) }) describe("when user doesn't have permission to access one of the projects", function() { beforeEach(function() { this.AuthorizationManager.canUserReadProject - .withArgs(this.user_id, 'project1', this.token) + .withArgs(this.userId, 'project1', this.token) .yields(null, true) - return this.AuthorizationManager.canUserReadProject - .withArgs(this.user_id, 'project2', this.token) + this.AuthorizationManager.canUserReadProject + .withArgs(this.userId, 'project2', this.token) .yields(null, false) }) @@ -385,7 +487,7 @@ describe('AuthorizationMiddleware', function() { this.next ) this.next.called.should.equal(false) - return this.AuthorizationMiddleware.redirectToRestricted + this.AuthorizationMiddleware.redirectToRestricted .calledWith(this.req, this.res, this.next) .should.equal(true) }) @@ -400,7 +502,7 @@ describe('AuthorizationMiddleware', function() { this.AuthorizationManager.canUserReadProject .withArgs(null, 'project1', this.token) .yields(null, true) - return this.AuthorizationManager.canUserReadProject + this.AuthorizationManager.canUserReadProject .withArgs(null, 'project2', this.token) .yields(null, true) }) @@ -411,7 +513,7 @@ describe('AuthorizationMiddleware', function() { this.res, this.next ) - return this.next.called.should.equal(true) + this.next.called.should.equal(true) }) }) @@ -421,7 +523,7 @@ describe('AuthorizationMiddleware', function() { this.AuthorizationManager.canUserReadProject .withArgs(null, 'project1', this.token) .yields(null, true) - return this.AuthorizationManager.canUserReadProject + this.AuthorizationManager.canUserReadProject .withArgs(null, 'project2', this.token) .yields(null, false) }) @@ -433,7 +535,7 @@ describe('AuthorizationMiddleware', function() { this.next ) this.next.called.should.equal(false) - return this.AuthorizationMiddleware.redirectToRestricted + this.AuthorizationMiddleware.redirectToRestricted .calledWith(this.req, this.res, this.next) .should.equal(true) })