From d33094d5d2fe94333a47ae160a0b5eb8ca0111e1 Mon Sep 17 00:00:00 2001 From: Muhamad Fikri Alhawarizmi Date: Sat, 6 Jul 2019 19:27:41 +0700 Subject: [PATCH 1/5] Fix copilotAndAbove permission to check that users is a member of the project --- src/permissions/copilotAndAbove.js | 45 ++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 6 deletions(-) diff --git a/src/permissions/copilotAndAbove.js b/src/permissions/copilotAndAbove.js index e5d5121a..800446e6 100644 --- a/src/permissions/copilotAndAbove.js +++ b/src/permissions/copilotAndAbove.js @@ -1,18 +1,51 @@ +import _ from 'lodash'; import util from '../util'; -import { MANAGER_ROLES, USER_ROLE } from '../constants'; +import { + USER_ROLE, + PROJECT_MEMBER_MANAGER_ROLES, + ADMIN_ROLES, +} from '../constants'; +import models from '../models'; /** - * Permission to alloow copilot and above roles to perform certain operations + * Permission to allow copilot and above roles to perform certain operations + * - User with Topcoder admins roles should be able to perform the operations. + * - Project members with copilot and manager Project roles should be also able to perform the operations. * @param {Object} req the express request instance * @return {Promise} returns a promise */ module.exports = req => new Promise((resolve, reject) => { - const hasAccess = util.hasRoles(req, [...MANAGER_ROLES, USER_ROLE.COPILOT]); + const projectId = _.parseInt(req.params.projectId); + const isAdmin = util.hasRoles(req, ADMIN_ROLES); - if (!hasAccess) { - return reject(new Error('You do not have permissions to perform this action')); + if (isAdmin) { + return resolve(true); } - return resolve(true); + const isManagerOrCopilot = util.hasRoles(req, [ + ...PROJECT_MEMBER_MANAGER_ROLES, + USER_ROLE.MANAGER, + USER_ROLE.TOPCODER_ACCOUNT_MANAGER, + USER_ROLE.COPILOT, + USER_ROLE.COPILOT_MANAGER, + ]); + + if (isManagerOrCopilot) { + return models.ProjectMember.getActiveProjectMembers(projectId) + .then((members) => { + req.context = req.context || {}; + req.context.currentProjectMembers = members; + // check if the copilot or manager has access to this project + const isMember = _.some(members, m => m.userId === req.authUser.userId); + + if (!isMember) { + // the copilot or manager is not a registered project member + return reject(new Error('You do not have permissions to perform this action')); + } + return resolve(true); + }); + } + + return reject(new Error('You do not have permissions to perform this action')); }); From ff7bb77a0087cd2717888610dad906cc7bd3afc2 Mon Sep 17 00:00:00 2001 From: Muhamad Fikri Alhawarizmi Date: Sat, 6 Jul 2019 19:46:46 +0700 Subject: [PATCH 2/5] update allowed roles --- src/permissions/copilotAndAbove.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/permissions/copilotAndAbove.js b/src/permissions/copilotAndAbove.js index 800446e6..2d168764 100644 --- a/src/permissions/copilotAndAbove.js +++ b/src/permissions/copilotAndAbove.js @@ -2,7 +2,7 @@ import _ from 'lodash'; import util from '../util'; import { USER_ROLE, - PROJECT_MEMBER_MANAGER_ROLES, + PROJECT_MEMBER_ROLE, ADMIN_ROLES, } from '../constants'; import models from '../models'; @@ -24,9 +24,9 @@ module.exports = req => new Promise((resolve, reject) => { } const isManagerOrCopilot = util.hasRoles(req, [ - ...PROJECT_MEMBER_MANAGER_ROLES, + PROJECT_MEMBER_ROLE.MANAGER, + PROJECT_MEMBER_ROLE.COPILOT, USER_ROLE.MANAGER, - USER_ROLE.TOPCODER_ACCOUNT_MANAGER, USER_ROLE.COPILOT, USER_ROLE.COPILOT_MANAGER, ]); From 1720640c41ab60b21c9f6c1fa1a232b07d08b02c Mon Sep 17 00:00:00 2001 From: Muhamad Fikri Alhawarizmi Date: Sat, 6 Jul 2019 21:06:08 +0700 Subject: [PATCH 3/5] update unit test --- src/routes/phaseProducts/create.spec.js | 51 +++++++++++++++++++++++- src/routes/phaseProducts/delete.spec.js | 45 ++++++++++++++++++++- src/routes/phaseProducts/update.spec.js | 53 ++++++++++++++++++++++++- src/routes/phases/create.spec.js | 53 ++++++++++++++++++++++++- src/routes/phases/delete.spec.js | 45 ++++++++++++++++++++- src/routes/phases/update.spec.js | 52 +++++++++++++++++++++++- 6 files changed, 290 insertions(+), 9 deletions(-) diff --git a/src/routes/phaseProducts/create.spec.js b/src/routes/phaseProducts/create.spec.js index 8f81a28b..0c3fad5b 100644 --- a/src/routes/phaseProducts/create.spec.js +++ b/src/routes/phaseProducts/create.spec.js @@ -71,6 +71,14 @@ describe('Phase Products', () => { isPrimary: true, createdBy: 1, updatedBy: 1, + }, { + id: 3, + userId: testUtil.userIds.manager, + projectId, + role: 'manager', + isPrimary: false, + createdBy: 1, + updatedBy: 1, }]).then(() => { models.ProjectPhase.create({ name: 'test project phase', @@ -177,7 +185,7 @@ describe('Phase Products', () => { request(server) .post(`/v4/projects/99999/phases/${phaseId}/products`) .set({ - Authorization: `Bearer ${testUtil.jwts.manager}`, + Authorization: `Bearer ${testUtil.jwts.admin}`, }) .send({ param: body }) .expect('Content-Type', /json/) @@ -220,6 +228,47 @@ describe('Phase Products', () => { }); }); + it('should return 201 if requested by admin', (done) => { + request(server) + .post(`/v4/projects/${projectId}/phases/${phaseId}/products`) + .set({ + Authorization: `Bearer ${testUtil.jwts.connectAdmin}`, + }) + .send({ param: body }) + .expect('Content-Type', /json/) + .expect(201) + .end(done); + }); + + it('should return 201 if requested by manager which is a member', (done) => { + request(server) + .post(`/v4/projects/${projectId}/phases/${phaseId}/products`) + .set({ + Authorization: `Bearer ${testUtil.jwts.manager}`, + }) + .send({ param: body }) + .expect('Content-Type', /json/) + .expect(201) + .end(done); + }); + + it('should return 403 if requested by non-member copilot', (done) => { + models.ProjectMember.destroy({ + where: { userId: testUtil.userIds.copilot, projectId }, + }) + .then(() => { + request(server) + .post(`/v4/projects/${projectId}/phases/${phaseId}/products`) + .set({ + Authorization: `Bearer ${testUtil.jwts.copilot}`, + }) + .send({ param: body }) + .expect('Content-Type', /json/) + .expect(403) + .end(done); + }); + }); + describe('Bus api', () => { let createEventSpy; const sandbox = sinon.sandbox.create(); diff --git a/src/routes/phaseProducts/delete.spec.js b/src/routes/phaseProducts/delete.spec.js index 69942fa6..9241a537 100644 --- a/src/routes/phaseProducts/delete.spec.js +++ b/src/routes/phaseProducts/delete.spec.js @@ -99,6 +99,14 @@ describe('Phase Products', () => { isPrimary: true, createdBy: 1, updatedBy: 1, + }, { + id: 3, + userId: testUtil.userIds.manager, + projectId, + role: 'manager', + isPrimary: false, + createdBy: 1, + updatedBy: 1, }]).then(() => { models.ProjectPhase.create({ name: 'test project phase', @@ -156,7 +164,7 @@ describe('Phase Products', () => { request(server) .delete(`/v4/projects/999/phases/${phaseId}/products/${productId}`) .set({ - Authorization: `Bearer ${testUtil.jwts.manager}`, + Authorization: `Bearer ${testUtil.jwts.admin}`, }) .expect('Content-Type', /json/) .expect(404, done); @@ -192,6 +200,41 @@ describe('Phase Products', () => { .end(err => expectAfterDelete(projectId, phaseId, productId, err, done)); }); + it('should return 204 if requested by admin', (done) => { + request(server) + .delete(`/v4/projects/${projectId}/phases/${phaseId}/products/${productId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.connectAdmin}`, + }) + .expect(204) + .end(done); + }); + + it('should return 204 if requested by manager which is a member', (done) => { + request(server) + .delete(`/v4/projects/${projectId}/phases/${phaseId}/products/${productId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.manager}`, + }) + .expect(204) + .end(done); + }); + + it('should return 403 if requested by non-member copilot', (done) => { + models.ProjectMember.destroy({ + where: { userId: testUtil.userIds.copilot, projectId }, + }) + .then(() => { + request(server) + .delete(`/v4/projects/${projectId}/phases/${phaseId}/products/${productId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.copilot}`, + }) + .expect(403) + .end(done); + }); + }); + describe('Bus api', () => { let createEventSpy; const sandbox = sinon.sandbox.create(); diff --git a/src/routes/phaseProducts/update.spec.js b/src/routes/phaseProducts/update.spec.js index 3c35871b..d20f9fcd 100644 --- a/src/routes/phaseProducts/update.spec.js +++ b/src/routes/phaseProducts/update.spec.js @@ -51,7 +51,7 @@ describe('Phase Products', () => { lastName: 'lName', email: 'some@abc.com', }; - before((done) => { + beforeEach((done) => { // mocks testUtil.clearDb() .then(() => { @@ -85,6 +85,14 @@ describe('Phase Products', () => { isPrimary: true, createdBy: 1, updatedBy: 1, + }, { + id: 3, + userId: testUtil.userIds.manager, + projectId, + role: 'manager', + isPrimary: false, + createdBy: 1, + updatedBy: 1, }]).then(() => { models.ProjectPhase.create({ name: 'test project phase', @@ -144,7 +152,7 @@ describe('Phase Products', () => { request(server) .patch(`/v4/projects/999/phases/${phaseId}/products/${productId}`) .set({ - Authorization: `Bearer ${testUtil.jwts.manager}`, + Authorization: `Bearer ${testUtil.jwts.admin}`, }) .send({ param: updateBody }) .expect('Content-Type', /json/) @@ -214,6 +222,47 @@ describe('Phase Products', () => { }); }); + it('should return 200 if requested by admin', (done) => { + request(server) + .patch(`/v4/projects/${projectId}/phases/${phaseId}/products/${productId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.connectAdmin}`, + }) + .send({ param: updateBody }) + .expect('Content-Type', /json/) + .expect(200) + .end(done); + }); + + it('should return 200 if requested by manager which is a member', (done) => { + request(server) + .patch(`/v4/projects/${projectId}/phases/${phaseId}/products/${productId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.manager}`, + }) + .send({ param: updateBody }) + .expect('Content-Type', /json/) + .expect(200) + .end(done); + }); + + it('should return 403 if requested by non-member copilot', (done) => { + models.ProjectMember.destroy({ + where: { userId: testUtil.userIds.copilot, projectId }, + }) + .then(() => { + request(server) + .patch(`/v4/projects/${projectId}/phases/${phaseId}/products/${productId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.copilot}`, + }) + .send({ param: updateBody }) + .expect('Content-Type', /json/) + .expect(403) + .end(done); + }); + }); + describe('Bus api', () => { let createEventSpy; const sandbox = sinon.sandbox.create(); diff --git a/src/routes/phases/create.spec.js b/src/routes/phases/create.spec.js index 69f45a4d..d15a0067 100644 --- a/src/routes/phases/create.spec.js +++ b/src/routes/phases/create.spec.js @@ -54,7 +54,7 @@ describe('Project Phases', () => { email: 'some@abc.com', }; let productTemplateId; - before((done) => { + beforeEach((done) => { // mocks testUtil.clearDb() .then(() => { @@ -89,6 +89,14 @@ describe('Project Phases', () => { isPrimary: true, createdBy: 1, updatedBy: 1, + }, { + id: 3, + userId: testUtil.userIds.manager, + projectId, + role: 'manager', + isPrimary: false, + createdBy: 1, + updatedBy: 1, }]); }); }) @@ -224,7 +232,7 @@ describe('Project Phases', () => { request(server) .post('/v4/projects/99999/phases/') .set({ - Authorization: `Bearer ${testUtil.jwts.manager}`, + Authorization: `Bearer ${testUtil.jwts.admin}`, }) .send({ param: body }) .expect('Content-Type', /json/) @@ -347,6 +355,47 @@ describe('Project Phases', () => { }); }); + it('should return 201 if requested by admin', (done) => { + request(server) + .post(`/v4/projects/${projectId}/phases/`) + .set({ + Authorization: `Bearer ${testUtil.jwts.admin}`, + }) + .send({ param: body }) + .expect('Content-Type', /json/) + .expect(201) + .end(done); + }); + + it('should return 201 if requested by manager which is a member', (done) => { + request(server) + .post(`/v4/projects/${projectId}/phases/`) + .set({ + Authorization: `Bearer ${testUtil.jwts.manager}`, + }) + .send({ param: body }) + .expect('Content-Type', /json/) + .expect(201) + .end(done); + }); + + it('should return 403 if requested by non-member copilot', (done) => { + models.ProjectMember.destroy({ + where: { userId: testUtil.userIds.copilot, projectId }, + }) + .then(() => { + request(server) + .post(`/v4/projects/${projectId}/phases/`) + .set({ + Authorization: `Bearer ${testUtil.jwts.copilot}`, + }) + .send({ param: body }) + .expect('Content-Type', /json/) + .expect(403) + .end(done); + }); + }); + describe('Bus api', () => { let createEventSpy; const sandbox = sinon.sandbox.create(); diff --git a/src/routes/phases/delete.spec.js b/src/routes/phases/delete.spec.js index 78453f39..7948f389 100644 --- a/src/routes/phases/delete.spec.js +++ b/src/routes/phases/delete.spec.js @@ -105,6 +105,14 @@ describe('Project Phases', () => { isPrimary: true, createdBy: 1, updatedBy: 1, + }, { + id: 3, + userId: testUtil.userIds.manager, + projectId, + role: 'manager', + isPrimary: false, + createdBy: 1, + updatedBy: 1, }]).then(() => { _.assign(body, { projectId }); models.ProjectPhase.create(body).then((phase) => { @@ -145,7 +153,7 @@ describe('Project Phases', () => { request(server) .delete(`/v4/projects/999/phases/${phaseId}`) .set({ - Authorization: `Bearer ${testUtil.jwts.manager}`, + Authorization: `Bearer ${testUtil.jwts.admin}`, }) .expect('Content-Type', /json/) .expect(404, done); @@ -167,9 +175,44 @@ describe('Project Phases', () => { .set({ Authorization: `Bearer ${testUtil.jwts.copilot}`, }) + .expect(204) .end(err => expectAfterDelete(projectId, phaseId, err, done)); }); + it('should return 204 if requested by admin', (done) => { + request(server) + .delete(`/v4/projects/${projectId}/phases/${phaseId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.admin}`, + }) + .expect(204) + .end(done); + }); + + it('should return 204 if requested by manager which is a member', (done) => { + request(server) + .delete(`/v4/projects/${projectId}/phases/${phaseId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.manager}`, + }) + .expect(204) + .end(done); + }); + + it('should return 403 if requested by non-member copilot', (done) => { + models.ProjectMember.destroy({ + where: { userId: testUtil.userIds.copilot, projectId }, + }).then(() => { + request(server) + .delete(`/v4/projects/${projectId}/phases/${phaseId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.copilot}`, + }) + .expect(403) + .end(done); + }); + }); + describe('Bus api', () => { let createEventSpy; const sandbox = sinon.sandbox.create(); diff --git a/src/routes/phases/update.spec.js b/src/routes/phases/update.spec.js index 85e8e44d..949da76c 100644 --- a/src/routes/phases/update.spec.js +++ b/src/routes/phases/update.spec.js @@ -68,7 +68,7 @@ describe('Project Phases', () => { lastName: 'lName', email: 'some@abc.com', }; - before((done) => { + beforeEach((done) => { // mocks testUtil.clearDb() .then(() => { @@ -103,6 +103,14 @@ describe('Project Phases', () => { isPrimary: true, createdBy: 1, updatedBy: 1, + }, { + id: 3, + userId: testUtil.userIds.manager, + projectId, + role: 'manager', + isPrimary: false, + createdBy: 1, + updatedBy: 1, }]).then(() => { _.assign(body, { projectId }); const phases = [ @@ -152,7 +160,7 @@ describe('Project Phases', () => { request(server) .patch(`/v4/projects/999/phases/${phaseId}`) .set({ - Authorization: `Bearer ${testUtil.jwts.manager}`, + Authorization: `Bearer ${testUtil.jwts.admin}`, }) .send({ param: updateBody }) .expect('Content-Type', /json/) @@ -272,6 +280,46 @@ describe('Project Phases', () => { }); }); + it('should return 200 if requested by admin', (done) => { + request(server) + .patch(`/v4/projects/${projectId}/phases/${phaseId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.admin}`, + }) + .send({ param: _.assign({ order: 1 }, updateBody) }) + .expect('Content-Type', /json/) + .expect(200) + .end(done); + }); + + it('should return 200 if requested by manager which is a member', (done) => { + request(server) + .patch(`/v4/projects/${projectId}/phases/${phaseId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.manager}`, + }) + .send({ param: _.assign({ order: 1 }, updateBody) }) + .expect('Content-Type', /json/) + .expect(200) + .end(done); + }); + + it('should return 403 if requested by non-member copilot', (done) => { + models.ProjectMember.destroy({ + where: { userId: testUtil.userIds.copilot, projectId }, + }).then(() => { + request(server) + .patch(`/v4/projects/${projectId}/phases/${phaseId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.copilot}`, + }) + .send({ param: _.assign({ order: 1 }, updateBody) }) + .expect('Content-Type', /json/) + .expect(403) + .end(done); + }); + }); + describe('Bus api', () => { let createEventSpy; const sandbox = sinon.sandbox.create(); From 6dc207547024986255d6c1e02d4d3334eee0fef1 Mon Sep 17 00:00:00 2001 From: Muhamad Fikri Alhawarizmi Date: Sun, 7 Jul 2019 10:27:55 +0700 Subject: [PATCH 4/5] add case for unit test "Users with Project manager roles CANNOT do actions if they are NOT member" --- src/routes/phaseProducts/create.spec.js | 39 ++++++--- src/routes/phaseProducts/delete.spec.js | 39 +++++---- src/routes/phaseProducts/update.spec.js | 43 ++++++---- src/routes/phases/create.spec.js | 101 +++++++++++++----------- src/routes/phases/delete.spec.js | 32 +++++--- src/routes/phases/update.spec.js | 40 +++++++--- 6 files changed, 184 insertions(+), 110 deletions(-) diff --git a/src/routes/phaseProducts/create.spec.js b/src/routes/phaseProducts/create.spec.js index 0c3fad5b..b8b462f8 100644 --- a/src/routes/phaseProducts/create.spec.js +++ b/src/routes/phaseProducts/create.spec.js @@ -71,14 +71,6 @@ describe('Phase Products', () => { isPrimary: true, createdBy: 1, updatedBy: 1, - }, { - id: 3, - userId: testUtil.userIds.manager, - projectId, - role: 'manager', - isPrimary: false, - createdBy: 1, - updatedBy: 1, }]).then(() => { models.ProjectPhase.create({ name: 'test project phase', @@ -185,7 +177,7 @@ describe('Phase Products', () => { request(server) .post(`/v4/projects/99999/phases/${phaseId}/products`) .set({ - Authorization: `Bearer ${testUtil.jwts.admin}`, + Authorization: `Bearer ${testUtil.jwts.connectAdmin}`, }) .send({ param: body }) .expect('Content-Type', /json/) @@ -196,7 +188,7 @@ describe('Phase Products', () => { request(server) .post(`/v4/projects/${projectId}/phases/99999/products`) .set({ - Authorization: `Bearer ${testUtil.jwts.manager}`, + Authorization: `Bearer ${testUtil.jwts.connectAdmin}`, }) .send({ param: body }) .expect('Content-Type', /json/) @@ -241,6 +233,28 @@ describe('Phase Products', () => { }); it('should return 201 if requested by manager which is a member', (done) => { + models.ProjectMember.create({ + id: 3, + userId: testUtil.userIds.manager, + projectId, + role: 'manager', + isPrimary: false, + createdBy: 1, + updatedBy: 1, + }).then(() => { + request(server) + .post(`/v4/projects/${projectId}/phases/${phaseId}/products`) + .set({ + Authorization: `Bearer ${testUtil.jwts.manager}`, + }) + .send({ param: body }) + .expect('Content-Type', /json/) + .expect(201) + .end(done); + }); + }); + + it('should return 403 if requested by manager which is not a member', (done) => { request(server) .post(`/v4/projects/${projectId}/phases/${phaseId}/products`) .set({ @@ -248,15 +262,14 @@ describe('Phase Products', () => { }) .send({ param: body }) .expect('Content-Type', /json/) - .expect(201) + .expect(403) .end(done); }); it('should return 403 if requested by non-member copilot', (done) => { models.ProjectMember.destroy({ where: { userId: testUtil.userIds.copilot, projectId }, - }) - .then(() => { + }).then(() => { request(server) .post(`/v4/projects/${projectId}/phases/${phaseId}/products`) .set({ diff --git a/src/routes/phaseProducts/delete.spec.js b/src/routes/phaseProducts/delete.spec.js index 9241a537..03db9a9d 100644 --- a/src/routes/phaseProducts/delete.spec.js +++ b/src/routes/phaseProducts/delete.spec.js @@ -99,14 +99,6 @@ describe('Phase Products', () => { isPrimary: true, createdBy: 1, updatedBy: 1, - }, { - id: 3, - userId: testUtil.userIds.manager, - projectId, - role: 'manager', - isPrimary: false, - createdBy: 1, - updatedBy: 1, }]).then(() => { models.ProjectPhase.create({ name: 'test project phase', @@ -164,7 +156,7 @@ describe('Phase Products', () => { request(server) .delete(`/v4/projects/999/phases/${phaseId}/products/${productId}`) .set({ - Authorization: `Bearer ${testUtil.jwts.admin}`, + Authorization: `Bearer ${testUtil.jwts.connectAdmin}`, }) .expect('Content-Type', /json/) .expect(404, done); @@ -174,7 +166,7 @@ describe('Phase Products', () => { request(server) .delete(`/v4/projects/${projectId}/phases/99999/products/${productId}`) .set({ - Authorization: `Bearer ${testUtil.jwts.manager}`, + Authorization: `Bearer ${testUtil.jwts.connectAdmin}`, }) .expect('Content-Type', /json/) .expect(404, done); @@ -184,7 +176,7 @@ describe('Phase Products', () => { request(server) .delete(`/v4/projects/${projectId}/phases/${phaseId}/products/99999`) .set({ - Authorization: `Bearer ${testUtil.jwts.manager}`, + Authorization: `Bearer ${testUtil.jwts.connectAdmin}`, }) .expect('Content-Type', /json/) .expect(404, done); @@ -211,20 +203,39 @@ describe('Phase Products', () => { }); it('should return 204 if requested by manager which is a member', (done) => { + models.ProjectMember.create({ + id: 3, + userId: testUtil.userIds.manager, + projectId, + role: 'manager', + isPrimary: false, + createdBy: 1, + updatedBy: 1, + }).then(() => { + request(server) + .delete(`/v4/projects/${projectId}/phases/${phaseId}/products/${productId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.manager}`, + }) + .expect(204) + .end(done); + }); + }); + + it('should return 403 if requested by manager which is not a member', (done) => { request(server) .delete(`/v4/projects/${projectId}/phases/${phaseId}/products/${productId}`) .set({ Authorization: `Bearer ${testUtil.jwts.manager}`, }) - .expect(204) + .expect(403) .end(done); }); it('should return 403 if requested by non-member copilot', (done) => { models.ProjectMember.destroy({ where: { userId: testUtil.userIds.copilot, projectId }, - }) - .then(() => { + }).then(() => { request(server) .delete(`/v4/projects/${projectId}/phases/${phaseId}/products/${productId}`) .set({ diff --git a/src/routes/phaseProducts/update.spec.js b/src/routes/phaseProducts/update.spec.js index d20f9fcd..5dcb8771 100644 --- a/src/routes/phaseProducts/update.spec.js +++ b/src/routes/phaseProducts/update.spec.js @@ -85,14 +85,6 @@ describe('Phase Products', () => { isPrimary: true, createdBy: 1, updatedBy: 1, - }, { - id: 3, - userId: testUtil.userIds.manager, - projectId, - role: 'manager', - isPrimary: false, - createdBy: 1, - updatedBy: 1, }]).then(() => { models.ProjectPhase.create({ name: 'test project phase', @@ -152,7 +144,7 @@ describe('Phase Products', () => { request(server) .patch(`/v4/projects/999/phases/${phaseId}/products/${productId}`) .set({ - Authorization: `Bearer ${testUtil.jwts.admin}`, + Authorization: `Bearer ${testUtil.jwts.connectAdmin}`, }) .send({ param: updateBody }) .expect('Content-Type', /json/) @@ -163,7 +155,7 @@ describe('Phase Products', () => { request(server) .patch(`/v4/projects/${projectId}/phases/99999/products/${productId}`) .set({ - Authorization: `Bearer ${testUtil.jwts.manager}`, + Authorization: `Bearer ${testUtil.jwts.copilot}`, }) .send({ param: updateBody }) .expect('Content-Type', /json/) @@ -174,7 +166,7 @@ describe('Phase Products', () => { request(server) .patch(`/v4/projects/${projectId}/phases/${phaseId}/products/99999`) .set({ - Authorization: `Bearer ${testUtil.jwts.manager}`, + Authorization: `Bearer ${testUtil.jwts.copilot}`, }) .send({ param: updateBody }) .expect('Content-Type', /json/) @@ -185,7 +177,7 @@ describe('Phase Products', () => { request(server) .patch(`/v4/projects/${projectId}/phases/${phaseId}/products/99999`) .set({ - Authorization: `Bearer ${testUtil.jwts.manager}`, + Authorization: `Bearer ${testUtil.jwts.copilot}`, }) .send({ param: { @@ -235,6 +227,28 @@ describe('Phase Products', () => { }); it('should return 200 if requested by manager which is a member', (done) => { + models.ProjectMember.create({ + id: 3, + userId: testUtil.userIds.manager, + projectId, + role: 'manager', + isPrimary: false, + createdBy: 1, + updatedBy: 1, + }).then(() => { + request(server) + .patch(`/v4/projects/${projectId}/phases/${phaseId}/products/${productId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.manager}`, + }) + .send({ param: updateBody }) + .expect('Content-Type', /json/) + .expect(200) + .end(done); + }); + }); + + it('should return 403 if requested by manager which is not a member', (done) => { request(server) .patch(`/v4/projects/${projectId}/phases/${phaseId}/products/${productId}`) .set({ @@ -242,15 +256,14 @@ describe('Phase Products', () => { }) .send({ param: updateBody }) .expect('Content-Type', /json/) - .expect(200) + .expect(403) .end(done); }); it('should return 403 if requested by non-member copilot', (done) => { models.ProjectMember.destroy({ where: { userId: testUtil.userIds.copilot, projectId }, - }) - .then(() => { + }).then(() => { request(server) .patch(`/v4/projects/${projectId}/phases/${phaseId}/products/${productId}`) .set({ diff --git a/src/routes/phases/create.spec.js b/src/routes/phases/create.spec.js index d15a0067..d36cdb4c 100644 --- a/src/routes/phases/create.spec.js +++ b/src/routes/phases/create.spec.js @@ -57,49 +57,39 @@ describe('Project Phases', () => { beforeEach((done) => { // mocks testUtil.clearDb() - .then(() => { - models.Project.create({ - type: 'generic', - billingAccountId: 1, - name: 'test1', - description: 'test project1', - status: 'draft', - details: {}, + .then(() => models.Project.create({ + type: 'generic', + billingAccountId: 1, + name: 'test1', + description: 'test project1', + status: 'draft', + details: {}, + createdBy: 1, + updatedBy: 1, + lastActivityAt: 1, + lastActivityUserId: '1', + }).then((p) => { + projectId = p.id; + projectName = p.name; + // create members + return models.ProjectMember.bulkCreate([{ + id: 1, + userId: copilotUser.userId, + projectId, + role: 'copilot', + isPrimary: false, createdBy: 1, updatedBy: 1, - lastActivityAt: 1, - lastActivityUserId: '1', - }).then((p) => { - projectId = p.id; - projectName = p.name; - // create members - models.ProjectMember.bulkCreate([{ - id: 1, - userId: copilotUser.userId, - projectId, - role: 'copilot', - isPrimary: false, - createdBy: 1, - updatedBy: 1, - }, { - id: 2, - userId: memberUser.userId, - projectId, - role: 'customer', - isPrimary: true, - createdBy: 1, - updatedBy: 1, - }, { - id: 3, - userId: testUtil.userIds.manager, - projectId, - role: 'manager', - isPrimary: false, - createdBy: 1, - updatedBy: 1, - }]); - }); - }) + }, { + id: 2, + userId: memberUser.userId, + projectId, + role: 'customer', + isPrimary: true, + createdBy: 1, + updatedBy: 1, + }]); + })) .then(() => models.ProductTemplate.create({ name: 'name 1', @@ -136,7 +126,7 @@ describe('Project Phases', () => { .then(() => done()); }); - after((done) => { + afterEach((done) => { testUtil.clearDb(done); }); @@ -368,6 +358,28 @@ describe('Project Phases', () => { }); it('should return 201 if requested by manager which is a member', (done) => { + models.ProjectMember.create({ + id: 3, + userId: testUtil.userIds.manager, + projectId, + role: 'manager', + isPrimary: false, + createdBy: 1, + updatedBy: 1, + }).then(() => { + request(server) + .post(`/v4/projects/${projectId}/phases/`) + .set({ + Authorization: `Bearer ${testUtil.jwts.manager}`, + }) + .send({ param: body }) + .expect('Content-Type', /json/) + .expect(201) + .end(done); + }); + }); + + it('should return 403 if requested by manager which is not a member', (done) => { request(server) .post(`/v4/projects/${projectId}/phases/`) .set({ @@ -375,15 +387,14 @@ describe('Project Phases', () => { }) .send({ param: body }) .expect('Content-Type', /json/) - .expect(201) + .expect(403) .end(done); }); it('should return 403 if requested by non-member copilot', (done) => { models.ProjectMember.destroy({ where: { userId: testUtil.userIds.copilot, projectId }, - }) - .then(() => { + }).then(() => { request(server) .post(`/v4/projects/${projectId}/phases/`) .set({ diff --git a/src/routes/phases/delete.spec.js b/src/routes/phases/delete.spec.js index 7948f389..bea3d2be 100644 --- a/src/routes/phases/delete.spec.js +++ b/src/routes/phases/delete.spec.js @@ -105,14 +105,6 @@ describe('Project Phases', () => { isPrimary: true, createdBy: 1, updatedBy: 1, - }, { - id: 3, - userId: testUtil.userIds.manager, - projectId, - role: 'manager', - isPrimary: false, - createdBy: 1, - updatedBy: 1, }]).then(() => { _.assign(body, { projectId }); models.ProjectPhase.create(body).then((phase) => { @@ -163,7 +155,7 @@ describe('Project Phases', () => { request(server) .delete(`/v4/projects/${projectId}/phases/999`) .set({ - Authorization: `Bearer ${testUtil.jwts.manager}`, + Authorization: `Bearer ${testUtil.jwts.admin}`, }) .expect('Content-Type', /json/) .expect(404, done); @@ -190,12 +182,32 @@ describe('Project Phases', () => { }); it('should return 204 if requested by manager which is a member', (done) => { + models.ProjectMember.create({ + id: 3, + userId: testUtil.userIds.manager, + projectId, + role: 'manager', + isPrimary: false, + createdBy: 1, + updatedBy: 1, + }).then(() => { + request(server) + .delete(`/v4/projects/${projectId}/phases/${phaseId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.manager}`, + }) + .expect(204) + .end(done); + }); + }); + + it('should return 403 if requested by manager which is not a member', (done) => { request(server) .delete(`/v4/projects/${projectId}/phases/${phaseId}`) .set({ Authorization: `Bearer ${testUtil.jwts.manager}`, }) - .expect(204) + .expect(403) .end(done); }); diff --git a/src/routes/phases/update.spec.js b/src/routes/phases/update.spec.js index 949da76c..7938be9e 100644 --- a/src/routes/phases/update.spec.js +++ b/src/routes/phases/update.spec.js @@ -103,14 +103,6 @@ describe('Project Phases', () => { isPrimary: true, createdBy: 1, updatedBy: 1, - }, { - id: 3, - userId: testUtil.userIds.manager, - projectId, - role: 'manager', - isPrimary: false, - createdBy: 1, - updatedBy: 1, }]).then(() => { _.assign(body, { projectId }); const phases = [ @@ -129,7 +121,7 @@ describe('Project Phases', () => { }); }); - after((done) => { + afterEach((done) => { testUtil.clearDb(done); }); @@ -171,7 +163,7 @@ describe('Project Phases', () => { request(server) .patch(`/v4/projects/${projectId}/phases/999`) .set({ - Authorization: `Bearer ${testUtil.jwts.manager}`, + Authorization: `Bearer ${testUtil.jwts.admin}`, }) .send({ param: updateBody }) .expect('Content-Type', /json/) @@ -182,7 +174,7 @@ describe('Project Phases', () => { request(server) .patch(`/v4/projects/${projectId}/phases/${phaseId}`) .set({ - Authorization: `Bearer ${testUtil.jwts.manager}`, + Authorization: `Bearer ${testUtil.jwts.admin}`, }) .send({ param: { @@ -197,7 +189,7 @@ describe('Project Phases', () => { request(server) .patch(`/v4/projects/${projectId}/phases/${phaseId}`) .set({ - Authorization: `Bearer ${testUtil.jwts.manager}`, + Authorization: `Bearer ${testUtil.jwts.admin}`, }) .send({ param: { @@ -293,6 +285,28 @@ describe('Project Phases', () => { }); it('should return 200 if requested by manager which is a member', (done) => { + models.ProjectMember.create({ + id: 3, + userId: testUtil.userIds.manager, + projectId, + role: 'manager', + isPrimary: false, + createdBy: 1, + updatedBy: 1, + }).then(() => { + request(server) + .patch(`/v4/projects/${projectId}/phases/${phaseId}`) + .set({ + Authorization: `Bearer ${testUtil.jwts.manager}`, + }) + .send({ param: _.assign({ order: 1 }, updateBody) }) + .expect('Content-Type', /json/) + .expect(200) + .end(done); + }); + }); + + it('should return 403 if requested by manager which is not a member', (done) => { request(server) .patch(`/v4/projects/${projectId}/phases/${phaseId}`) .set({ @@ -300,7 +314,7 @@ describe('Project Phases', () => { }) .send({ param: _.assign({ order: 1 }, updateBody) }) .expect('Content-Type', /json/) - .expect(200) + .expect(403) .end(done); }); From 0c6ca3b4e0db05aa32d3fc99b6f3bb40bf6790b0 Mon Sep 17 00:00:00 2001 From: Muhamad Fikri Alhawarizmi Date: Mon, 8 Jul 2019 19:09:08 +0700 Subject: [PATCH 5/5] update loginc for validating project member roles --- src/permissions/copilotAndAbove.js | 44 +++++++++++++----------------- 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/src/permissions/copilotAndAbove.js b/src/permissions/copilotAndAbove.js index 2d168764..54f41409 100644 --- a/src/permissions/copilotAndAbove.js +++ b/src/permissions/copilotAndAbove.js @@ -1,7 +1,6 @@ import _ from 'lodash'; import util from '../util'; import { - USER_ROLE, PROJECT_MEMBER_ROLE, ADMIN_ROLES, } from '../constants'; @@ -23,29 +22,24 @@ module.exports = req => new Promise((resolve, reject) => { return resolve(true); } - const isManagerOrCopilot = util.hasRoles(req, [ - PROJECT_MEMBER_ROLE.MANAGER, - PROJECT_MEMBER_ROLE.COPILOT, - USER_ROLE.MANAGER, - USER_ROLE.COPILOT, - USER_ROLE.COPILOT_MANAGER, - ]); + return models.ProjectMember.getActiveProjectMembers(projectId) + .then((members) => { + req.context = req.context || {}; + req.context.currentProjectMembers = members; + const validMemberProjectRoles = [ + PROJECT_MEMBER_ROLE.MANAGER, + PROJECT_MEMBER_ROLE.COPILOT, + ]; + // check if the copilot or manager has access to this project + const isMember = _.some( + members, +m => m.userId === req.authUser.userId && validMemberProjectRoles.includes(m.role), + ); - if (isManagerOrCopilot) { - return models.ProjectMember.getActiveProjectMembers(projectId) - .then((members) => { - req.context = req.context || {}; - req.context.currentProjectMembers = members; - // check if the copilot or manager has access to this project - const isMember = _.some(members, m => m.userId === req.authUser.userId); - - if (!isMember) { - // the copilot or manager is not a registered project member - return reject(new Error('You do not have permissions to perform this action')); - } - return resolve(true); - }); - } - - return reject(new Error('You do not have permissions to perform this action')); + if (!isMember) { + // the copilot or manager is not a registered project member + return reject(new Error('You do not have permissions to perform this action')); + } + return resolve(true); + }); });