ucfopen#2010 Changed tactics for user permission management to use a …

…user search instead of a dropdown with all possible users. Made necessary backend and frontend adjustments to correctly differentiate between role-based implicit permissions and explicit permissions. Added/updated unit tests.

packages/app/obojobo-express/__tests__/express_validators.test.js

@@ -519,6 +519,28 @@ describe('current user middleware', () => {
 		})
 	})
 
+	// requireCanViewAdminPage
+
+	test('requireCanViewAdminPage calls next and has no validation errors', () => {
+		mockUser.hasPermission = perm => perm === 'canViewAdminPage'
+		mockReq.requireCurrentUser = jest.fn().mockResolvedValue(mockUser)
+		return Validators.requireCanViewAdminPage(mockReq, mockRes, mockNext).then(() => {
+			expect(mockNext).toHaveBeenCalledTimes(1)
+			expect(mockRes.notAuthorized).toHaveBeenCalledTimes(0)
+			expect(mockReq._validationErrors).toBeUndefined()
+		})
+	})
+
+	test('requireCanViewAdminPage doesnt call next and has errors', () => {
+		mockUser.hasPermission = () => false
+		mockReq.requireCurrentUser = jest.fn().mockResolvedValue(mockUser)
+		return Validators.requireCanViewAdminPage(mockReq, mockRes, mockNext).then(() => {
+			expect(mockNext).toHaveBeenCalledTimes(0)
+			expect(mockRes.notAuthorized).toHaveBeenCalledTimes(1)
+			expect(mockReq._validationErrors).toBeUndefined()
+		})
+	})
+
 	// checkValidationRules
 
 	test('checkValidationRules calls next with no errors', () => {

packages/app/obojobo-express/server/models/user.js

@@ -102,22 +102,6 @@ class User {
 			})
 	}
 
-	static getAll() {
-		return db
-			.manyOrNone(
-				`SELECT
-					*
-				FROM users
-				ORDER BY first_name, last_name
-				LIMIT 25`
-			)
-			.then(results => results.map(r => User.dbResultToModel(r)))
-			.catch(error => {
-				logger.logError('Error getAll', error)
-				throw Error('Error fetching all users.')
-			})
-	}
-
 	saveOrCreate() {
 		return db
 			.one(

packages/app/obojobo-repository/server/models/admin_interface.js

@@ -3,72 +3,94 @@ const logger = require('obojobo-express/server/logger')
 
 const User = require('obojobo-express/server/models/user')
 
+const { PERMS_PER_ROLE } = require('../../shared/util/implicit-perms')
+
 class AdminInterface {
 	static addPermission(userId, permission) {
 		return new Promise((resolve, reject) => {
-			// First fetch user
 			User.fetchById(userId)
-			.then(u => {
-				let perms = u.perms
-				if (perms.includes(permission)) return
-				perms = [...perms, permission]
-
-				// Then add new permission (if it is new)
-				this._updateUserPerms(userId, perms)
-				.then(id => resolve(id))
+				.then(u => {
+					let perms = u.perms
+					if (perms.includes(permission)) return resolve(u)
+
+					perms = [...u.perms, permission]
+
+					const allRolePerms = new Set()
+
+					u.roles.forEach(role => {
+						PERMS_PER_ROLE[role].forEach(perm => allRolePerms.add(perm))
+					})
+
+					perms = perms.filter(p => !allRolePerms.has(p))
+
+					const dedupedPerms = new Set([...u.perms, ...perms])
+					u.perms = [...dedupedPerms]
+
+					// Then add new permission (if it is new)
+					this._updateUserPerms(userId, perms)
+						.then(() => resolve(u))
+						.catch(() => {
+							reject(logger.logError(`AdminInterface error adding permission`))
+						})
+				})
 				.catch(() => {
-					reject()
-					throw logger.logError(`AdminInterface error adding permission`)
+					reject(logger.logError(`AdminInterface error finding user with id ${userId}`))
 				})
-			})
-			.catch(err => {
-				reject()
-				throw logger.logError(`AdminInterface error finding user with id ${userId}`)
-			})
 		})
 	}
 
 	static removePermission(userId, permission) {
 		return new Promise((resolve, reject) => {
-			// First fetch user
 			User.fetchById(userId)
-			.then(u => {
-				const perms = u.perms
+				.then(u => {
+					const allRolePerms = new Set()
+
+					u.roles.forEach(role => {
+						PERMS_PER_ROLE[role].forEach(perm => allRolePerms.add(perm))
+					})
+
+					const perms = u.perms.filter(p => !allRolePerms.has(p))
+
+					const ix = perms.indexOf(permission)
+					if (ix === -1) return resolve(u)
+					perms.splice(ix, 1)
 
-				const ix = perms.indexOf(permission)
-				if (ix === -1) return
-				perms.splice(ix, 1)
+					// add any remaining perms to the 'allRolePerms' set so we can use it to set the user's new combined perms
+					perms.forEach(p => allRolePerms.add(p))
 
-				// Then remove permission (if present)
-				this._updateUserPerms(userId, perms)
-				.then(id => resolve(id))
+					u.perms = [...allRolePerms]
+
+					// Then remove permission (if present)
+					this._updateUserPerms(userId, perms)
+						.then(() => resolve(u))
+						.catch(() => {
+							reject(logger.logError(`AdminInterface error removing permission`))
+						})
+				})
 				.catch(() => {
-					reject()
-					throw logger.logError(`AdminInterface error removing permission`)
+					reject(logger.logError(`AdminInterface error finding user with id ${userId}`))
 				})
-			})
-			.catch(() => {
-				reject()
-				throw logger.logError(`AdminInterface Error finding user with id ${userId}`)
-			})
 		})
 	}
 
 	static _updateUserPerms(userId, perms) {
 		return new Promise((resolve, reject) => {
-			db.oneOrNone(`
-				UPDATE user_perms
-				SET perms = $[perms]
-				WHERE user_id = $[userId]
+			db.oneOrNone(
+				`
+				INSERT INTO user_perms (user_id, perms)
+				VALUES ($[userId], $[perms])
+				ON CONFLICT (user_id)
+				DO UPDATE SET perms = $[perms]
+				WHERE user_perms.user_id = $[userId]
 			`,
-			{ userId, perms })
-			.then(() => resolve(userId))
-			.catch(error => {
-				reject()
-				throw logger.logError('AdminInterface _updateUserPerms Error', error)
-			})
+				{ userId, perms }
+			)
+				.then(() => resolve(userId))
+				.catch(error => {
+					reject(logger.logError('AdminInterface _updateUserPerms error', error))
+				})
 		})
 	}
 }
 
-module.exports = AdminInterface
+module.exports = AdminInterface