Merge pull request #1042 from sharelatex/ta-user-membership-access

User Membership Access Refactor

GitOrigin-RevId: 23e8d342bc4829450625146213ff92cb042550dd

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)

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()

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'

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

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

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()

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()

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()