From 2f387afd8693d678d7a19c7939fcb453484053c3 Mon Sep 17 00:00:00 2001 From: Sukma Nugraha Date: Thu, 4 Jul 2019 18:45:54 +0700 Subject: [PATCH 1/8] Show proper error message when creating new project type or product category with same key as deleted one. --- src/routes/productCategories/create.js | 4 ++-- src/routes/projectTypes/create.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/routes/productCategories/create.js b/src/routes/productCategories/create.js index e80bc596..811c9fa7 100644 --- a/src/routes/productCategories/create.js +++ b/src/routes/productCategories/create.js @@ -41,10 +41,10 @@ module.exports = [ }); // Check if duplicated key - return models.ProductCategory.findById(req.body.param.key) + return models.ProductCategory.findById(req.body.param.key, { paranoid: false }) .then((existing) => { if (existing) { - const apiErr = new Error(`Product category already exists for key ${req.params.key}`); + const apiErr = new Error(`Product category already exists(may be deleted) for key "${req.body.param.key}"`); apiErr.status = 422; return Promise.reject(apiErr); } diff --git a/src/routes/projectTypes/create.js b/src/routes/projectTypes/create.js index 8e73e1ec..028f21bb 100644 --- a/src/routes/projectTypes/create.js +++ b/src/routes/projectTypes/create.js @@ -42,10 +42,10 @@ module.exports = [ }); // Check if duplicated key - return models.ProjectType.findById(req.body.param.key) + return models.ProjectType.findById(req.body.param.key, { paranoid: false }) .then((existing) => { if (existing) { - const apiErr = new Error(`Project type already exists for key ${req.params.key}`); + const apiErr = new Error(`Project type already exists(may be deleted) for key "${req.body.param.key}"`); apiErr.status = 422; return Promise.reject(apiErr); } From d33094d5d2fe94333a47ae160a0b5eb8ca0111e1 Mon Sep 17 00:00:00 2001 From: Muhamad Fikri Alhawarizmi Date: Sat, 6 Jul 2019 19:27:41 +0700 Subject: [PATCH 2/8] 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 3/8] 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 4/8] 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 5/8] 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 00a0ca2b947cce42acd1a99f426c110f3980e973 Mon Sep 17 00:00:00 2001 From: Sukma Nugraha Date: Sun, 7 Jul 2019 23:59:27 +0700 Subject: [PATCH 6/8] Update code to NOT pass transactions explicitly. --- src/routes/milestoneTemplates/clone.js | 4 ++-- src/routes/milestoneTemplates/create.js | 5 ++--- src/routes/milestones/create.js | 5 ++--- src/routes/milestones/delete.js | 7 +++---- src/routes/timelines/create.js | 9 ++++----- 5 files changed, 13 insertions(+), 17 deletions(-) diff --git a/src/routes/milestoneTemplates/clone.js b/src/routes/milestoneTemplates/clone.js index bfe95a46..147f6dd1 100644 --- a/src/routes/milestoneTemplates/clone.js +++ b/src/routes/milestoneTemplates/clone.js @@ -30,7 +30,7 @@ module.exports = [ (req, res, next) => { let result; - return models.sequelize.transaction(tx => + return models.sequelize.transaction(() => // Find the product template models.MilestoneTemplate.findAll({ where: { @@ -48,7 +48,7 @@ module.exports = [ milestone.createdBy = req.authUser.userId; // eslint-disable-line no-param-reassign milestone.updatedBy = req.authUser.userId; // eslint-disable-line no-param-reassign }); - return models.MilestoneTemplate.bulkCreate(newMilestoneTemplates, { transaction: tx }); + return models.MilestoneTemplate.bulkCreate(newMilestoneTemplates); }) .then(() => { // eslint-disable-line arrow-body-style return models.MilestoneTemplate.findAll({ diff --git a/src/routes/milestoneTemplates/create.js b/src/routes/milestoneTemplates/create.js index 321c9b65..32ab860a 100644 --- a/src/routes/milestoneTemplates/create.js +++ b/src/routes/milestoneTemplates/create.js @@ -51,9 +51,9 @@ module.exports = [ }); let result; - return models.sequelize.transaction(tx => + return models.sequelize.transaction(() => // Create the milestone template - models.MilestoneTemplate.create(entity, { transaction: tx }) + models.MilestoneTemplate.create(entity) .then((createdEntity) => { // Omit deletedAt and deletedBy result = _.omit(createdEntity.toJSON(), 'deletedAt', 'deletedBy'); @@ -67,7 +67,6 @@ module.exports = [ id: { $ne: result.id }, order: { $gte: result.order }, }, - transaction: tx, }); }), ) diff --git a/src/routes/milestones/create.js b/src/routes/milestones/create.js index eadec1f4..e7a713e7 100644 --- a/src/routes/milestones/create.js +++ b/src/routes/milestones/create.js @@ -73,9 +73,9 @@ module.exports = [ return next(apiErr); } - return models.sequelize.transaction(tx => + return models.sequelize.transaction(() => // Save to DB - models.Milestone.create(entity, { transaction: tx }) + models.Milestone.create(entity) .then((createdEntity) => { // Omit deletedAt, deletedBy result = _.omit(createdEntity.toJSON(), 'deletedAt', 'deletedBy'); @@ -88,7 +88,6 @@ module.exports = [ id: { $ne: result.id }, order: { $gte: result.order }, }, - transaction: tx, }); }), ) diff --git a/src/routes/milestones/delete.js b/src/routes/milestones/delete.js index 45e5a41b..c2cf2bce 100644 --- a/src/routes/milestones/delete.js +++ b/src/routes/milestones/delete.js @@ -29,11 +29,10 @@ module.exports = [ id: req.params.milestoneId, }; - return models.sequelize.transaction(tx => + return models.sequelize.transaction(() => // Find the milestone models.Milestone.findOne({ where, - transaction: tx, }) .then((milestone) => { // Not found @@ -44,8 +43,8 @@ module.exports = [ } // Update the deletedBy, and soft delete - return milestone.update({ deletedBy: req.authUser.userId }, { transaction: tx }) - .then(() => milestone.destroy({ transaction: tx })); + return milestone.update({ deletedBy: req.authUser.userId }) + .then(() => milestone.destroy()); }), ) .then((deleted) => { diff --git a/src/routes/timelines/create.js b/src/routes/timelines/create.js index a37b7adb..b001f2f4 100644 --- a/src/routes/timelines/create.js +++ b/src/routes/timelines/create.js @@ -51,9 +51,9 @@ module.exports = [ let result; // Save to DB - models.sequelize.transaction((tx) => { + return models.sequelize.transaction(() => { req.log.debug('Started transaction'); - return models.Timeline.create(entity, { transaction: tx }) + return models.Timeline.create(entity) .then((createdEntity) => { // Omit deletedAt, deletedBy result = _.omit(createdEntity.toJSON(), 'deletedAt', 'deletedBy'); @@ -97,7 +97,7 @@ module.exports = [ } return milestone; }); - return models.Milestone.bulkCreate(milestones, { returning: true, transaction: tx }) + return models.Milestone.bulkCreate(milestones, { returning: true }) .then((createdMilestones) => { req.log.debug('Milestones created for timeline with template id %d', templateId); result.milestones = _.map(createdMilestones, cm => _.omit(cm.toJSON(), 'deletedAt', 'deletedBy')); @@ -109,8 +109,7 @@ module.exports = [ }); } return Promise.resolve(); - }) - .catch(next); + }); }) .then(() => { // Send event to bus From 0c6ca3b4e0db05aa32d3fc99b6f3bb40bf6790b0 Mon Sep 17 00:00:00 2001 From: Muhamad Fikri Alhawarizmi Date: Mon, 8 Jul 2019 19:09:08 +0700 Subject: [PATCH 7/8] 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); + }); }); From 8851cd662e48e12df2cf13c5f5520053faf9f77e Mon Sep 17 00:00:00 2001 From: Maksym Mykhailenko Date: Tue, 16 Jul 2019 10:49:10 +0800 Subject: [PATCH 8/8] fix error message formating --- src/routes/productCategories/create.js | 2 +- src/routes/projectTypes/create.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/routes/productCategories/create.js b/src/routes/productCategories/create.js index 811c9fa7..152a7d2e 100644 --- a/src/routes/productCategories/create.js +++ b/src/routes/productCategories/create.js @@ -44,7 +44,7 @@ module.exports = [ return models.ProductCategory.findById(req.body.param.key, { paranoid: false }) .then((existing) => { if (existing) { - const apiErr = new Error(`Product category already exists(may be deleted) for key "${req.body.param.key}"`); + const apiErr = new Error(`Product category already exists (may be deleted) for key "${req.body.param.key}"`); apiErr.status = 422; return Promise.reject(apiErr); } diff --git a/src/routes/projectTypes/create.js b/src/routes/projectTypes/create.js index 028f21bb..15cf3d49 100644 --- a/src/routes/projectTypes/create.js +++ b/src/routes/projectTypes/create.js @@ -45,7 +45,7 @@ module.exports = [ return models.ProjectType.findById(req.body.param.key, { paranoid: false }) .then((existing) => { if (existing) { - const apiErr = new Error(`Project type already exists(may be deleted) for key "${req.body.param.key}"`); + const apiErr = new Error(`Project type already exists (may be deleted) for key "${req.body.param.key}"`); apiErr.status = 422; return Promise.reject(apiErr); }