ucfopen#2040 Explanatory text added to Module Permissions window expl…

…aining what each access level allows. Added logic to draft update API endpoint to prevent saving a draft if the user does not have full or partial access to it. Added logic to permissions update API endpoitn to prevent a user without full access to a draft to change other users' access to it.
FrenjaminBanklin · Jan 10, 2023 · 0833f21 · 0833f21
1 parent 63e28c3
commit 0833f21
Show file tree

Hide file tree

Showing 6 changed files with 238 additions and 9 deletions.
diff --git a/packages/app/obojobo-express/__tests__/routes/api/drafts.test.js b/packages/app/obojobo-express/__tests__/routes/api/drafts.test.js
@@ -410,7 +410,7 @@ describe('api draft route', () => {
 				expect(response.body.value).toHaveProperty('type', 'notAuthorized')
 				expect(response.body.value).toHaveProperty(
 					'message',
-					'Your access level must be "Partial" or higher to retrieve this information.'
+					'In order to edit this module you must have "Partial" or "Full" access.'
 				)
 			})
 	})
@@ -810,11 +810,12 @@ describe('api draft route', () => {
 
 	// update draft
 
-	test('updating a draft with xml returns successfully', () => {
+	test('updating a draft with xml returns successfully when user has full access', () => {
 		expect.assertions(5)
 		xml.mockReturnValueOnce({})
 		mockCurrentUser = { id: 99, hasPermission: perm => perm === 'canCreateDrafts' } // mock current logged in user
 		DraftModel.findDuplicateIds.mockReturnValueOnce(null) // no errors
+		DraftPermissions.getUserAccessLevelToDraft.mockResolvedValueOnce(FULL)
 		return request(app)
 			.post('/api/drafts/00000000-0000-0000-0000-000000000000')
 			.type('text/plain')
@@ -830,11 +831,77 @@ describe('api draft route', () => {
 				expect(xml).toHaveBeenCalledWith(basicXML, true)
 			})
 	})
+	test('updating a draft with xml returns successfully when user has partial access', () => {
+		expect.assertions(5)
+		xml.mockReturnValueOnce({})
+		mockCurrentUser = { id: 99, hasPermission: perm => perm === 'canCreateDrafts' } // mock current logged in user
+		DraftModel.findDuplicateIds.mockReturnValueOnce(null) // no errors
+		DraftPermissions.getUserAccessLevelToDraft.mockResolvedValueOnce(PARTIAL)
+		return request(app)
+			.post('/api/drafts/00000000-0000-0000-0000-000000000000')
+			.type('text/plain')
+			.accept('text/plain')
+			.send(basicXML)
+			.then(response => {
+				expect(response.header['content-type']).toContain('application/json')
+				expect(response.statusCode).toBe(200)
+				expect(response.body).toHaveProperty('status', 'ok')
+				expect(response.body).toHaveProperty('value', {
+					id: 'mockUpdatedContentId'
+				})
+				expect(xml).toHaveBeenCalledWith(basicXML, true)
+			})
+	})
+	test('updating a draft with xml fails when user has minimal access', () => {
+		expect.assertions(5)
+		xml.mockReturnValueOnce({})
+		mockCurrentUser = { id: 99, hasPermission: perm => perm === 'canCreateDrafts' } // mock current logged in user
+		DraftModel.findDuplicateIds.mockReturnValueOnce(null) // no errors
+		DraftPermissions.getUserAccessLevelToDraft.mockResolvedValueOnce(MINIMAL)
+		return request(app)
+			.post('/api/drafts/00000000-0000-0000-0000-000000000000')
+			.type('text/plain')
+			.accept('text/plain')
+			.send(basicXML)
+			.then(response => {
+				expect(response.header['content-type']).toContain('application/json')
+				expect(response.statusCode).toBe(401)
+				expect(response.body).toHaveProperty('status', 'error')
+				expect(response.body.value).toHaveProperty('type', 'notAuthorized')
+				expect(response.body.value).toHaveProperty(
+					'message',
+					'In order to edit this module you must have "Partial" or "Full" access.'
+				)
+			})
+	})
+	test('updating a draft with xml fails when user has no access', () => {
+		expect.assertions(5)
+		xml.mockReturnValueOnce({})
+		mockCurrentUser = { id: 99, hasPermission: perm => perm === 'canCreateDrafts' } // mock current logged in user
+		DraftModel.findDuplicateIds.mockReturnValueOnce(null) // no errors
+		DraftPermissions.getUserAccessLevelToDraft.mockResolvedValueOnce(null)
+		return request(app)
+			.post('/api/drafts/00000000-0000-0000-0000-000000000000')
+			.type('text/plain')
+			.accept('text/plain')
+			.send(basicXML)
+			.then(response => {
+				expect(response.header['content-type']).toContain('application/json')
+				expect(response.statusCode).toBe(401)
+				expect(response.body).toHaveProperty('status', 'error')
+				expect(response.body.value).toHaveProperty('type', 'notAuthorized')
+				expect(response.body.value).toHaveProperty(
+					'message',
+					'In order to edit this module you must have "Partial" or "Full" access.'
+				)
+			})
+	})
 
-	test('updating a draft with json returns successfully', () => {
+	test('updating a draft with json returns successfully when user has full access', () => {
 		expect.assertions(3)
 		mockCurrentUser = { id: 99, hasPermission: perm => perm === 'canCreateDrafts' } // mock current logged in user
 		DraftModel.findDuplicateIds.mockReturnValueOnce(null) // no errors
+		DraftPermissions.getUserAccessLevelToDraft.mockResolvedValueOnce(FULL)
 		return request(app)
 			.post('/api/drafts/00000000-0000-0000-0000-000000000000')
 			.type('application/json')
@@ -847,11 +914,67 @@ describe('api draft route', () => {
 				})
 			})
 	})
+	test('updating a draft with json returns successfully when user has partial access', () => {
+		expect.assertions(3)
+		mockCurrentUser = { id: 99, hasPermission: perm => perm === 'canCreateDrafts' } // mock current logged in user
+		DraftModel.findDuplicateIds.mockReturnValueOnce(null) // no errors
+		DraftPermissions.getUserAccessLevelToDraft.mockResolvedValueOnce(PARTIAL)
+		return request(app)
+			.post('/api/drafts/00000000-0000-0000-0000-000000000000')
+			.type('application/json')
+			.send('{"test":true}')
+			.then(response => {
+				expect(response.statusCode).toBe(200)
+				expect(response.body).toHaveProperty('status', 'ok')
+				expect(response.body).toHaveProperty('value', {
+					id: 'mockUpdatedContentId'
+				})
+			})
+	})
+	test('updating a draft with json fails when user has minimal access', () => {
+		expect.assertions(4)
+		mockCurrentUser = { id: 99, hasPermission: perm => perm === 'canCreateDrafts' } // mock current logged in user
+		DraftModel.findDuplicateIds.mockReturnValueOnce(null) // no errors
+		DraftPermissions.getUserAccessLevelToDraft.mockResolvedValueOnce(MINIMAL)
+		return request(app)
+			.post('/api/drafts/00000000-0000-0000-0000-000000000000')
+			.type('application/json')
+			.send('{"test":true}')
+			.then(response => {
+				expect(response.statusCode).toBe(401)
+				expect(response.body).toHaveProperty('status', 'error')
+				expect(response.body.value).toHaveProperty('type', 'notAuthorized')
+				expect(response.body.value).toHaveProperty(
+					'message',
+					'In order to edit this module you must have "Partial" or "Full" access.'
+				)
+			})
+	})
+	test('updating a draft with json fails when user has no access', () => {
+		expect.assertions(4)
+		mockCurrentUser = { id: 99, hasPermission: perm => perm === 'canCreateDrafts' } // mock current logged in user
+		DraftModel.findDuplicateIds.mockReturnValueOnce(null) // no errors
+		DraftPermissions.getUserAccessLevelToDraft.mockResolvedValueOnce(null)
+		return request(app)
+			.post('/api/drafts/00000000-0000-0000-0000-000000000000')
+			.type('application/json')
+			.send('{"test":true}')
+			.then(response => {
+				expect(response.statusCode).toBe(401)
+				expect(response.body).toHaveProperty('status', 'error')
+				expect(response.body.value).toHaveProperty('type', 'notAuthorized')
+				expect(response.body.value).toHaveProperty(
+					'message',
+					'In order to edit this module you must have "Partial" or "Full" access.'
+				)
+			})
+	})
 
 	test('updating a draft errors when xmlToDraftObject returns invalid object', () => {
 		expect.assertions(6)
 		xml.mockImplementationOnce(null)
 		mockCurrentUser = { id: 99, hasPermission: perm => perm === 'canCreateDrafts' } // mock current logged in user
+		DraftPermissions.getUserAccessLevelToDraft.mockResolvedValueOnce(FULL)
 		return request(app)
 			.post('/api/drafts/00000000-0000-0000-0000-000000000000')
 			.type('text/plain')
@@ -876,6 +999,7 @@ describe('api draft route', () => {
 			throw 'some-error'
 		})
 		mockCurrentUser = { id: 99, hasPermission: perm => perm === 'canCreateDrafts' } // mock current logged in user
+		DraftPermissions.getUserAccessLevelToDraft.mockResolvedValueOnce(FULL)
 		return request(app)
 			.post('/api/drafts/00000000-0000-0000-0000-000000000000')
 			.type('text/plain')
@@ -915,6 +1039,7 @@ describe('api draft route', () => {
 		expect.assertions(5)
 		xml.mockReturnValueOnce({})
 		mockCurrentUser = { id: 99, hasPermission: perm => perm === 'canCreateDrafts' } // mock current logged in user
+		DraftPermissions.getUserAccessLevelToDraft.mockResolvedValueOnce(FULL)
 		DraftModel.findDuplicateIds.mockReturnValueOnce('duplicate-id') // mock the findDuplicateIds method
 		return request(app)
 			.post('/api/drafts/00000000-0000-0000-0000-000000000000')
@@ -937,6 +1062,7 @@ describe('api draft route', () => {
 		expect.assertions(5)
 		xml.mockReturnValueOnce({})
 		mockCurrentUser = { id: 99, hasPermission: perm => perm === 'canCreateDrafts' } // mock current logged in user
+		DraftPermissions.getUserAccessLevelToDraft.mockResolvedValueOnce(FULL)
 		DraftModel.findDuplicateIds.mockImplementationOnce(() => {
 			throw 'oh no'
 		}) // mock the findDuplicateIds method

diff --git a/packages/app/obojobo-express/server/routes/api/drafts.js b/packages/app/obojobo-express/server/routes/api/drafts.js
@@ -45,7 +45,7 @@ router
 
 			if (!hasPerms) {
 				return res.notAuthorized(
-					'Your access level must be "Partial" or higher to retrieve this information.'
+					'In order to edit this module you must have "Partial" or "Full" access.'
 				)
 			}
 
@@ -208,7 +208,21 @@ router
 	.post([requireCanCreateDrafts, requireDraftId, checkValidationRules])
 	.post((req, res) => {
 		return Promise.resolve()
-			.then(() => {
+			.then(async () => {
+				// @TODO: checking permissions should probably be more dynamic, not hard-coded to the repository
+				const access_level = await DraftPermissions.getUserAccessLevelToDraft(
+					req.currentUser.id,
+					req.params.draftId
+				)
+
+				return access_level === FULL || access_level === PARTIAL
+			})
+			.then(canEdit => {
+				if (!canEdit) {
+					return res.notAuthorized(
+						'In order to edit this module you must have "Partial" or "Full" access.'
+					)
+				}
 				let xml
 				let documentInput
 

diff --git a/packages/app/obojobo-repository/server/routes/api.js b/packages/app/obojobo-repository/server/routes/api.js
@@ -272,6 +272,14 @@ router
 			const draftId = req.currentDocument.draftId
 			const targetLevel = req.body.accessLevel
 
+			// check currentUser's permissions
+			const canShare =
+				(await DraftPermissions.getUserAccessLevelToDraft(req.currentUser.id, draftId)) === FULL
+			if (!canShare) {
+				res.notAuthorized('Current User does not have permission to share this draft')
+				return
+			}
+
 			// Guard against invalid access levels
 			if (!levelNumber[targetLevel]) {
 				const msg = 'Invalid access level: ' + targetLevel

diff --git a/packages/app/obojobo-repository/server/routes/api.test.js b/packages/app/obojobo-repository/server/routes/api.test.js
@@ -641,10 +641,51 @@ describe('repository api route', () => {
 			})
 	})
 
-	// update draft access levels
-	test('post /drafts/:draftId/permission/update does not call updateAccessLevel if target access level matches current access level', () => {
+	test('post /drafts/:draftId/permission/update fails if current user has partial access', () => {
 		DraftPermissions.getUserAccessLevelToDraft.mockResolvedValueOnce(PARTIAL)
 
+		return request(app)
+			.post('/drafts/mockDraftId/permission/update')
+			.send({ accessLevel: levelName[MINIMAL], userId: 99 })
+			.then(response => {
+				expect(DraftPermissions.updateAccessLevel).not.toHaveBeenCalled()
+				expect(response.statusCode).toBe(401)
+				expect(response.body).toHaveProperty('status', 'error')
+				expect(response.body).toHaveProperty('value')
+				expect(response.body.value).toHaveProperty('type', 'notAuthorized')
+				expect(response.body.value).toHaveProperty(
+					'message',
+					'Current User does not have permission to share this draft'
+				)
+			})
+	})
+
+	test('post /drafts/:draftId/permission/update fails if current user has minimal access', () => {
+		DraftPermissions.getUserAccessLevelToDraft.mockResolvedValueOnce(MINIMAL)
+
+		return request(app)
+			.post('/drafts/mockDraftId/permission/update')
+			.send({ accessLevel: levelName[MINIMAL], userId: 99 })
+			.then(response => {
+				expect(DraftPermissions.updateAccessLevel).not.toHaveBeenCalled()
+				expect(response.statusCode).toBe(401)
+				expect(response.body).toHaveProperty('status', 'error')
+				expect(response.body).toHaveProperty('value')
+				expect(response.body.value).toHaveProperty('type', 'notAuthorized')
+				expect(response.body.value).toHaveProperty(
+					'message',
+					'Current User does not have permission to share this draft'
+				)
+			})
+	})
+
+	test('post /drafts/:draftId/permission/update does not call updateAccessLevel if target access level matches current access level', () => {
+		DraftPermissions.getUserAccessLevelToDraft
+			// first call checks the current user's access level
+			.mockResolvedValueOnce(FULL)
+			// second call checks the target user's access level
+			.mockResolvedValueOnce(PARTIAL)
+
 		return request(app)
 			.post('/drafts/mockDraftId/permission/update')
 			.send({ accessLevel: levelName[PARTIAL], userId: 99 })

diff --git a/packages/app/obojobo-repository/shared/components/module-permissions-dialog.jsx b/packages/app/obojobo-repository/shared/components/module-permissions-dialog.jsx
@@ -121,7 +121,24 @@ class ModulePermissionsDialog extends React.Component {
 				</div>
 				<div className="wrapper">
 					<h1 className="title">Module Access</h1>
-					<div className="sub-title">People who can access this module</div>
+					<div className="sub-title">
+						People who can access this module. Access levels are as follows, note that each level
+						contains the previous level.
+					</div>
+					<div className="access-level-descriptions">
+						<span>
+							<label>Minimal:</label>
+							<p>Can preview, view assessment statistics, and copy the module.</p>
+						</span>
+						<span>
+							<label>Partial:</label>
+							<p>Can edit the module.</p>
+						</span>
+						<span>
+							<label>Full:</label>
+							<p>Can add or change access or delete the module.</p>
+						</span>
+					</div>
 					<Button id="modulePermissionsDialog-addPeopleButton" onClick={this.openPeoplePicker}>
 						Add People
 					</Button>

diff --git a/packages/app/obojobo-repository/shared/components/module-permissions-dialog.scss b/packages/app/obojobo-repository/shared/components/module-permissions-dialog.scss
@@ -67,7 +67,30 @@
 
 	.sub-title {
 		font-size: 0.7em;
-		margin-bottom: 2em;
+		margin-bottom: 0.5em;
+		text-align: left;
+	}
+
+	.access-level-descriptions {
+		margin-bottom: 1em;
+
+		span {
+			display: flex;
+			flex-direction: row;
+
+			label,
+			p {
+				font-size: 0.7em;
+				display: block;
+			}
+
+			label {
+				width: 3em;
+				margin-right: 1.25em;
+				text-align: left;
+				font-weight: bold;
+			}
+		}
 	}
 
 	.access-list-wrapper {