From b123f830ff34903c2763d100420986e61bc61134 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Alby?= Date: Wed, 24 Oct 2018 15:50:34 +0200 Subject: [PATCH] Merge pull request #1042 from sharelatex/ta-user-membership-access User Membership Access Refactor GitOrigin-RevId: 23e8d342bc4829450625146213ff92cb042550dd --- .../UserMembershipAuthorization.coffee | 32 ++++++++ .../UserMembershipController.coffee | 60 ++++++--------- .../UserMembershipEntityConfigs.coffee | 3 + .../UserMembershipHandler.coffee | 3 +- .../UserMembershipRouter.coffee | 18 ++--- .../UserMembershipAuthorizationTests.coffee | 75 +++++++++++++++++++ .../UserMembershipControllerTests.coffee | 64 ++++++---------- .../UserMembershipHandlerTests.coffee | 4 +- 8 files changed, 168 insertions(+), 91 deletions(-) create mode 100644 services/web/app/coffee/Features/UserMembership/UserMembershipAuthorization.coffee create mode 100644 services/web/test/unit/coffee/UserMembership/UserMembershipAuthorizationTests.coffee diff --git a/services/web/app/coffee/Features/UserMembership/UserMembershipAuthorization.coffee b/services/web/app/coffee/Features/UserMembership/UserMembershipAuthorization.coffee new file mode 100644 index 0000000000..334498ad8c --- /dev/null +++ b/services/web/app/coffee/Features/UserMembership/UserMembershipAuthorization.coffee @@ -0,0 +1,32 @@ +AuthenticationController = require('../Authentication/AuthenticationController') +AuthorizationMiddlewear = require('../Authorization/AuthorizationMiddlewear') +UserMembershipHandler = require('./UserMembershipHandler') +EntityConfigs = require('./UserMembershipEntityConfigs') +Errors = require('../Errors/Errors') +logger = require("logger-sharelatex") + +module.exports = + requireEntityAccess: (entityName) -> + (req, res, next) -> + loggedInUser = AuthenticationController.getSessionUser(req) + unless loggedInUser + return AuthorizationMiddlewear.redirectToRestricted req, res, next + + entityId = req.params.id + getEntity entityName, entityId, loggedInUser, (error, entity, entityConfig) -> + return next(error) if error? + unless entity? + return AuthorizationMiddlewear.redirectToRestricted(req, res, next) + + req.entity = entity + req.entityConfig = entityConfig + next() + +getEntity = (entityName, entityId, userId, callback = (error, entity, entityConfig)->) -> + entityConfig = EntityConfigs[entityName] + unless entityConfig + return callback(new Errors.NotFoundError("No such entity: #{entityName}")) + + UserMembershipHandler.getEntity entityId, entityConfig, userId, (error, entity)-> + return callback(error) if error? + callback(null, entity, entityConfig) diff --git a/services/web/app/coffee/Features/UserMembership/UserMembershipController.coffee b/services/web/app/coffee/Features/UserMembership/UserMembershipController.coffee index 94531f46e9..1f5ef5d42d 100644 --- a/services/web/app/coffee/Features/UserMembership/UserMembershipController.coffee +++ b/services/web/app/coffee/Features/UserMembership/UserMembershipController.coffee @@ -5,50 +5,36 @@ Errors = require('../Errors/Errors') logger = require("logger-sharelatex") module.exports = - index: (entityName, req, res, next)-> - getEntity entityName, req, (error, entity, entityConfig) -> + index: (req, res, next)-> + { entity, entityConfig } = req + UserMembershipHandler.getUsers entity, entityConfig, (error, users)-> return next(error) if error? - UserMembershipHandler.getUsers entity, entityConfig, (error, users)-> - return next(error) if error? - res.render "user_membership/index", - users: users - groupSize: entity.membersLimit if entityConfig.hasMembersLimit - translations: entityConfig.translations - paths: entityConfig.pathsFor(entity._id.toString()) + entityPrimaryKey = entity[entityConfig.fields.primaryKey].toString() + res.render "user_membership/index", + users: users + groupSize: entity.membersLimit if entityConfig.hasMembersLimit + translations: entityConfig.translations + paths: entityConfig.pathsFor(entityPrimaryKey) - add: (entityName, req, res, next)-> + add: (req, res, next)-> + { entity, entityConfig } = req email = req.body.email return res.sendStatus 422 unless email - getEntity entityName, req, (error, entity, entityConfig) -> + if entityConfig.readOnly + return next(new Errors.NotFoundError("Cannot add users to entity")) + + UserMembershipHandler.addUser entity, entityConfig, email, (error, user)-> return next(error) if error? - if entityConfig.readOnly - return next(new Errors.NotFoundError("Cannot add users to entity")) + res.json(user: user) - UserMembershipHandler.addUser entity, entityConfig, email, (error, user)-> - return next(error) if error? - res.json(user: user) - - remove: (entityName, req, res, next)-> + remove: (req, res, next)-> + { entity, entityConfig } = req userId = req.params.userId - getEntity entityName, req, (error, entity, entityConfig) -> + if entityConfig.readOnly + return next(new Errors.NotFoundError("Cannot remove users from entity")) + + UserMembershipHandler.removeUser entity, entityConfig, userId, (error, user)-> return next(error) if error? - if entityConfig.readOnly - return next(new Errors.NotFoundError("Cannot remove users from entity")) - - UserMembershipHandler.removeUser entity, entityConfig, userId, (error, user)-> - return next(error) if error? - res.send() - -getEntity = (entityName, req, callback) -> - entityConfig = EntityConfigs[entityName] - unless entityConfig - return callback(new Errors.NotFoundError("No such entity: #{entityName}")) - - loggedInUser = AuthenticationController.getSessionUser(req) - entityId = req.params.id - UserMembershipHandler.getEntity entityId, entityConfig, loggedInUser, (error, entity)-> - return callback(error) if error? - return callback(new Errors.NotFoundError()) unless entity? - callback(null, entity, entityConfig) + res.send() diff --git a/services/web/app/coffee/Features/UserMembership/UserMembershipEntityConfigs.coffee b/services/web/app/coffee/Features/UserMembership/UserMembershipEntityConfigs.coffee index 64195dc67d..9d1ec5fdbf 100644 --- a/services/web/app/coffee/Features/UserMembership/UserMembershipEntityConfigs.coffee +++ b/services/web/app/coffee/Features/UserMembership/UserMembershipEntityConfigs.coffee @@ -4,6 +4,7 @@ module.exports = readOnly: true hasMembersLimit: true fields: + primaryKey: '_id' read: ['invited_emails', 'teamInvites', 'member_ids'] write: null access: 'manager_ids' @@ -21,6 +22,7 @@ module.exports = groupManagers: modelName: 'Subscription' fields: + primaryKey: '_id' read: ['manager_ids'] write: 'manager_ids' access: 'manager_ids' @@ -36,6 +38,7 @@ module.exports = institution: modelName: 'Institution' fields: + primaryKey: 'v1Id' read: ['managerIds'] write: 'managerIds' access: 'managerIds' diff --git a/services/web/app/coffee/Features/UserMembership/UserMembershipHandler.coffee b/services/web/app/coffee/Features/UserMembership/UserMembershipHandler.coffee index 62d07935bd..015f32c4dc 100644 --- a/services/web/app/coffee/Features/UserMembership/UserMembershipHandler.coffee +++ b/services/web/app/coffee/Features/UserMembership/UserMembershipHandler.coffee @@ -10,8 +10,9 @@ logger = require('logger-sharelatex') module.exports = getEntity: (entityId, entityConfig, loggedInUser, callback = (error, entity) ->) -> + entityId = ObjectId(entityId) if ObjectId.isValid(entityId.toString()) query = Object.assign({}, entityConfig.baseQuery) - query._id = ObjectId(entityId) + query[entityConfig.fields.primaryKey] = entityId unless loggedInUser.isAdmin query[entityConfig.fields.access] = ObjectId(loggedInUser._id) EntityModels[entityConfig.modelName].findOne query, callback diff --git a/services/web/app/coffee/Features/UserMembership/UserMembershipRouter.coffee b/services/web/app/coffee/Features/UserMembership/UserMembershipRouter.coffee index 97ffa88ffa..79f9401add 100644 --- a/services/web/app/coffee/Features/UserMembership/UserMembershipRouter.coffee +++ b/services/web/app/coffee/Features/UserMembership/UserMembershipRouter.coffee @@ -1,11 +1,11 @@ -AuthenticationController = require('../Authentication/AuthenticationController') +UserMembershipAuthorization = require './UserMembershipAuthorization' UserMembershipController = require './UserMembershipController' module.exports = apply: (webRouter) -> webRouter.get '/manage/groups/:id/members', - AuthenticationController.requireLogin(), - (req, res, next) -> UserMembershipController.index('group', req, res, next) + UserMembershipAuthorization.requireEntityAccess('group'), + UserMembershipController.index regularEntitites = @@ -14,13 +14,13 @@ module.exports = for pathName, entityName of regularEntitites do (pathName, entityName) -> webRouter.get "/manage/#{pathName}/:id/managers", - AuthenticationController.requireLogin(), - (req, res, next) -> UserMembershipController.index(entityName, req, res, next) + UserMembershipAuthorization.requireEntityAccess(entityName), + UserMembershipController.index webRouter.post "/manage/#{pathName}/:id/managers", - AuthenticationController.requireLogin(), - (req, res, next) -> UserMembershipController.add(entityName, req, res, next) + UserMembershipAuthorization.requireEntityAccess(entityName), + UserMembershipController.add webRouter.delete "/manage/#{pathName}/:id/managers/:userId", - AuthenticationController.requireLogin(), - (req, res, next) -> UserMembershipController.remove(entityName, req, res, next) + UserMembershipAuthorization.requireEntityAccess(entityName), + UserMembershipController.remove diff --git a/services/web/test/unit/coffee/UserMembership/UserMembershipAuthorizationTests.coffee b/services/web/test/unit/coffee/UserMembership/UserMembershipAuthorizationTests.coffee new file mode 100644 index 0000000000..eb96166369 --- /dev/null +++ b/services/web/test/unit/coffee/UserMembership/UserMembershipAuthorizationTests.coffee @@ -0,0 +1,75 @@ +sinon = require('sinon') +chai = require('chai') +expect = require('chai').expect +modulePath = "../../../../app/js/Features/UserMembership/UserMembershipAuthorization.js" +SandboxedModule = require('sandboxed-module') +MockRequest = require "../helpers/MockRequest" +EntityConfigs = require("../../../../app/js/Features/UserMembership/UserMembershipEntityConfigs") +Errors = require("../../../../app/js/Features/Errors/Errors") + +describe "UserMembershipAuthorization", -> + beforeEach -> + @req = new MockRequest() + @req.params.id = 'mock-entity-id' + @user = _id: 'mock-user-id' + @subscription = { _id: 'mock-subscription-id'} + + @AuthenticationController = + getSessionUser: sinon.stub().returns(@user) + @UserMembershipHandler = + getEntity: sinon.stub().yields(null, @subscription) + @AuthorizationMiddlewear = + redirectToRestricted: sinon.stub().yields() + @UserMembershipAuthorization = SandboxedModule.require modulePath, requires: + '../Authentication/AuthenticationController': @AuthenticationController + '../Authorization/AuthorizationMiddlewear': @AuthorizationMiddlewear + './UserMembershipHandler': @UserMembershipHandler + './EntityConfigs': EntityConfigs + '../Errors/Errors': Errors + "logger-sharelatex": + log: -> + err: -> + + describe 'requireEntityAccess', -> + it 'get entity', (done) -> + middlewear = @UserMembershipAuthorization.requireEntityAccess 'group' + middlewear @req, null, (error) => + expect(error).to.not.extist + sinon.assert.calledWithMatch( + @UserMembershipHandler.getEntity, + @req.params.id, + modelName: 'Subscription', + @user + ) + expect(@req.entity).to.equal @subscription + expect(@req.entityConfig).to.exist + done() + + it 'handle unknown entity', (done) -> + middlewear = @UserMembershipAuthorization.requireEntityAccess 'foo' + middlewear @req, null, (error) => + expect(error).to.extist + expect(error).to.be.an.instanceof(Errors.NotFoundError) + sinon.assert.notCalled(@UserMembershipHandler.getEntity) + expect(@req.entity).to.not.exist + done() + + it 'handle entity not found', (done) -> + @UserMembershipHandler.getEntity.yields(null, null) + middlewear = @UserMembershipAuthorization.requireEntityAccess 'institution' + middlewear @req, null, (error) => + expect(error).to.extist + sinon.assert.called(@AuthorizationMiddlewear.redirectToRestricted) + sinon.assert.called(@UserMembershipHandler.getEntity) + expect(@req.entity).to.not.exist + done() + + it 'handle anonymous user', (done) -> + @AuthenticationController.getSessionUser.returns(null) + middlewear = @UserMembershipAuthorization.requireEntityAccess 'institution' + middlewear @req, null, (error) => + expect(error).to.extist + sinon.assert.called(@AuthorizationMiddlewear.redirectToRestricted) + sinon.assert.notCalled(@UserMembershipHandler.getEntity) + expect(@req.entity).to.not.exist + done() diff --git a/services/web/test/unit/coffee/UserMembership/UserMembershipControllerTests.coffee b/services/web/test/unit/coffee/UserMembership/UserMembershipControllerTests.coffee index d4a16d91e8..58176fbab6 100644 --- a/services/web/test/unit/coffee/UserMembership/UserMembershipControllerTests.coffee +++ b/services/web/test/unit/coffee/UserMembership/UserMembershipControllerTests.coffee @@ -19,6 +19,7 @@ describe "UserMembershipController", -> @user = _id: 'mock-user-id' @newUser = _id: 'mock-new-user-id', email: 'new-user-email@foo.bar' @subscription = { _id: 'mock-subscription-id'} + @institution = _id: 'mock-institution-id', v1Id: 123 @users = [{ _id: 'mock-member-id-1' }, { _id: 'mock-member-id-2' }] @AuthenticationController = @@ -37,18 +38,12 @@ describe "UserMembershipController", -> err: -> describe 'index', -> - it 'get entity', (done) -> - @UserMembershipController.index 'group', @req, render: () => - sinon.assert.calledWithMatch( - @UserMembershipHandler.getEntity, - @req.params.id, - modelName: 'Subscription', - @user - ) - done() + beforeEach -> + @req.entity = @subscription + @req.entityConfig = EntityConfigs.group it 'get users', (done) -> - @UserMembershipController.index 'group', @req, render: () => + @UserMembershipController.index @req, render: () => sinon.assert.calledWithMatch( @UserMembershipHandler.getUsers, @subscription, @@ -57,7 +52,7 @@ describe "UserMembershipController", -> done() it 'render group view', (done) -> - @UserMembershipController.index 'group', @req, render: (viewPath, viewParams) => + @UserMembershipController.index @req, render: (viewPath, viewParams) => expect(viewPath).to.equal 'user_membership/index' expect(viewParams.users).to.deep.equal @users expect(viewParams.groupSize).to.equal @subscription.membersLimit @@ -66,7 +61,8 @@ describe "UserMembershipController", -> done() it 'render group managers view', (done) -> - @UserMembershipController.index 'groupManagers', @req, render: (viewPath, viewParams) => + @req.entityConfig = EntityConfigs.groupManagers + @UserMembershipController.index @req, render: (viewPath, viewParams) => expect(viewPath).to.equal 'user_membership/index' expect(viewParams.groupSize).to.equal undefined expect(viewParams.translations.title).to.equal 'group_managers' @@ -74,43 +70,23 @@ describe "UserMembershipController", -> done() it 'render institution view', (done) -> - @UserMembershipController.index 'institution', @req, render: (viewPath, viewParams) => + @req.entity = @institution + @req.entityConfig = EntityConfigs.institution + @UserMembershipController.index @req, render: (viewPath, viewParams) => expect(viewPath).to.equal 'user_membership/index' expect(viewParams.groupSize).to.equal undefined expect(viewParams.translations.title).to.equal 'institution_managers' expect(viewParams.paths.exportMembers).to.be.undefined done() - it 'handle unknown entity', (done) -> - @UserMembershipController.index 'foo', @req, null, (error) => - expect(error).to.extist - expect(error).to.be.an.instanceof(Errors.NotFoundError) - done() - - - it 'handle entity not found', (done) -> - @UserMembershipHandler.getEntity.yields(null, null) - @UserMembershipController.index 'group', @req, null, (error) => - expect(error).to.extist - expect(error).to.be.an.instanceof(Errors.NotFoundError) - done() - describe 'add', -> beforeEach -> @req.body.email = @newUser.email - - it 'get entity', (done) -> - @UserMembershipController.add 'groupManagers', @req, json: () => - sinon.assert.calledWithMatch( - @UserMembershipHandler.getEntity, - @req.params.id, - modelName: 'Subscription', - @user - ) - done() + @req.entity = @subscription + @req.entityConfig = EntityConfigs.groupManagers it 'add user', (done) -> - @UserMembershipController.add 'groupManagers', @req, json: () => + @UserMembershipController.add @req, json: () => sinon.assert.calledWithMatch( @UserMembershipHandler.addUser, @subscription, @@ -120,12 +96,13 @@ describe "UserMembershipController", -> done() it 'return user object', (done) -> - @UserMembershipController.add 'groupManagers', @req, json: (payload) => + @UserMembershipController.add @req, json: (payload) => payload.user.should.equal @newUser done() it 'handle readOnly entity', (done) -> - @UserMembershipController.add 'group', @req, null, (error) => + @req.entityConfig = EntityConfigs.group + @UserMembershipController.add @req, null, (error) => expect(error).to.extist expect(error).to.be.an.instanceof(Errors.NotFoundError) done() @@ -133,9 +110,11 @@ describe "UserMembershipController", -> describe 'remove', -> beforeEach -> @req.params.userId = @newUser._id + @req.entity = @subscription + @req.entityConfig = EntityConfigs.groupManagers it 'remove user', (done) -> - @UserMembershipController.remove 'groupManagers', @req, send: () => + @UserMembershipController.remove @req, send: () => sinon.assert.calledWithMatch( @UserMembershipHandler.removeUser, @subscription, @@ -145,7 +124,8 @@ describe "UserMembershipController", -> done() it 'handle readOnly entity', (done) -> - @UserMembershipController.remove 'group', @req, null, (error) => + @req.entityConfig = EntityConfigs.group + @UserMembershipController.remove @req, null, (error) => expect(error).to.extist expect(error).to.be.an.instanceof(Errors.NotFoundError) done() diff --git a/services/web/test/unit/coffee/UserMembership/UserMembershipHandlerTests.coffee b/services/web/test/unit/coffee/UserMembership/UserMembershipHandlerTests.coffee index bc97e63b5b..a96977af55 100644 --- a/services/web/test/unit/coffee/UserMembership/UserMembershipHandlerTests.coffee +++ b/services/web/test/unit/coffee/UserMembership/UserMembershipHandlerTests.coffee @@ -80,9 +80,9 @@ describe 'UserMembershipHandler', -> describe 'institutions', -> it 'get institution', (done) -> - @UserMembershipHandler.getEntity @fakeEntityId, EntityConfigs.institution, @user, (error, institution) => + @UserMembershipHandler.getEntity @institution.v1Id, EntityConfigs.institution, @user, (error, institution) => should.not.exist(error) - expectedQuery = _id: @fakeEntityId, managerIds: ObjectId(@user._id) + expectedQuery = v1Id: @institution.v1Id, managerIds: ObjectId(@user._id) assertCalledWith(@Institution.findOne, expectedQuery) expect(institution).to.equal @institution done()