From 8b5187392de77377fe8752f2f633221960a761d0 Mon Sep 17 00:00:00 2001 From: gets0ul Date: Thu, 7 May 2020 00:16:55 +0700 Subject: [PATCH 1/8] Only return members with project object in these GET /projects and GET /projects/{id} endpoints if user has permission READ_PROJECT_MEMBER. --- src/routes/projects/get.js | 8 ++++++-- src/routes/projects/get.spec.js | 2 +- src/routes/projects/list.js | 6 ++++-- src/routes/projects/list.spec.js | 33 ++++++++++++++++++++++++++++---- 4 files changed, 40 insertions(+), 9 deletions(-) diff --git a/src/routes/projects/get.js b/src/routes/projects/get.js index 148601b2..caa04531 100644 --- a/src/routes/projects/get.js +++ b/src/routes/projects/get.js @@ -3,6 +3,7 @@ import config from 'config'; import { middleware as tcMiddleware } from 'tc-core-library-js'; import models from '../../models'; import util from '../../util'; +import { PERMISSION } from '../../permissions/constants'; const ES_PROJECT_INDEX = config.get('elasticsearchConfig.indexName'); const ES_PROJECT_TYPE = config.get('elasticsearchConfig.docType'); @@ -103,7 +104,8 @@ const retrieveProjectFromES = (projectId, req) => { fields = fields ? fields.split(',') : []; fields = util.parseFields(fields, { projects: PROJECT_ATTRIBUTES, - project_members: util.addUserDetailsFieldsIfAllowed(PROJECT_MEMBER_ATTRIBUTES_ES, req), + project_members: util.hasPermissionByReq(PERMISSION.READ_PROJECT_MEMBER, req) + ? util.addUserDetailsFieldsIfAllowed(PROJECT_MEMBER_ATTRIBUTES_ES, req) : null, project_member_invites: PROJECT_MEMBER_INVITE_ATTRIBUTES, project_phases: PROJECT_PHASE_ATTRIBUTES, project_phases_products: PROJECT_PHASE_PRODUCTS_ATTRIBUTES, @@ -143,7 +145,9 @@ const retrieveProjectFromDB = (projectId, req) => { return Promise.reject(apiErr); } // check context for project members - project.members = _.map(req.context.currentProjectMembers, m => _.pick(m, fields.project_members)); + if (util.hasPermissionByReq(PERMISSION.READ_PROJECT_MEMBER, req)) { + project.members = _.map(req.context.currentProjectMembers, m => _.pick(m, fields.project_members)); + } // check if attachments field was requested if (!req.query.fields || _.indexOf(req.query.fields, 'attachments') > -1) { return util.getProjectAttachments(req, project.id); diff --git a/src/routes/projects/get.spec.js b/src/routes/projects/get.spec.js index a12e46f6..3faa8a0e 100644 --- a/src/routes/projects/get.spec.js +++ b/src/routes/projects/get.spec.js @@ -268,7 +268,7 @@ describe('GET Project', () => { should.not.exist(resJson.billingAccountId); should.exist(resJson.name); resJson.status.should.be.eql('draft'); - resJson.members.should.have.lengthOf(2); + should.not.exist(resJson.members); done(); } }); diff --git a/src/routes/projects/list.js b/src/routes/projects/list.js index a9ea8445..3694e3c4 100755 --- a/src/routes/projects/list.js +++ b/src/routes/projects/list.js @@ -493,7 +493,8 @@ const retrieveProjectsFromDB = (req, criteria, sort, ffields) => { // make sure project.id is part of fields if (_.indexOf(fields.projects, 'id') < 0) fields.projects.push('id'); const retrieveAttachments = !req.query.fields || req.query.fields.indexOf('attachments') > -1; - const retrieveMembers = !req.query.fields || !!fields.project_members.length; + const retrieveMembers = (!req.query.fields || !!fields.project_members.length) + && util.hasPermissionByReq(PERMISSION.READ_PROJECT_MEMBER, req); return models.Project.searchText({ filters: criteria.filters, @@ -551,7 +552,8 @@ const retrieveProjects = (req, criteria, sort, ffields) => { // parse the fields string to determine what fields are to be returned fields = util.parseFields(fields, { projects: PROJECT_ATTRIBUTES, - project_members: util.addUserDetailsFieldsIfAllowed(PROJECT_MEMBER_ATTRIBUTES_ES, req), + project_members: util.hasPermissionByReq(PERMISSION.READ_PROJECT_MEMBER, req) ? + util.addUserDetailsFieldsIfAllowed(PROJECT_MEMBER_ATTRIBUTES_ES, req) : null, project_member_invites: PROJECT_MEMBER_INVITE_ATTRIBUTES, project_phases: PROJECT_PHASE_ATTRIBUTES, project_phases_products: PROJECT_PHASE_PRODUCTS_ATTRIBUTES, diff --git a/src/routes/projects/list.spec.js b/src/routes/projects/list.spec.js index e454a181..fcc39226 100644 --- a/src/routes/projects/list.spec.js +++ b/src/routes/projects/list.spec.js @@ -405,6 +405,29 @@ describe('LIST Project', () => { }); }); + it('should not include the project members using M2M token without "read:project-members" scope', (done) => { + request(server) + .get('/v5/projects') + .set({ + Authorization: `Bearer ${testUtil.m2m['read:projects']}`, + }) + .expect('Content-Type', /json/) + .expect(200) + .end((err, res) => { + if (err) { + done(err); + } else { + const resJson = res.body; + should.exist(resJson); + resJson.should.have.lengthOf(3); + resJson.forEach((project) => { + should.not.exist(project.members); + }); + done(); + } + }); + }); + it('should return the project when project that is in reviewed state in which the copilot is its member or has been invited', (done) => { request(server) .get('/v5/projects') @@ -1159,7 +1182,8 @@ describe('LIST Project', () => { }); describe('URL Query fields', () => { - it('should not return "email" for project members when "fields" query param is not defined (to non-admin users)', (done) => { + it(`should not include project members when "fields" query param is not defined (to non-admin users) + without READ_PROJECT_MEMBER permission`, (done) => { request(server) .get('/v5/projects/') .set({ @@ -1174,14 +1198,15 @@ describe('LIST Project', () => { const resJson = res.body; should.exist(resJson); resJson.should.have.lengthOf(1); - resJson[0].members[0].should.not.have.property('email'); + should.not.exist(resJson[0].members); done(); } }); }); - it('should not return "email" for project members even if it\'s listed in "fields" query param (to non-admin users)', (done) => { + it(`should not include project members even if it's listed in "fields" query param (to non-admin users) + without READ_PROJECT_MEMBER permission`, (done) => { request(server) .get('/v5/projects/?fields=members.email,members.id') .set({ @@ -1196,7 +1221,7 @@ describe('LIST Project', () => { const resJson = res.body; should.exist(resJson); resJson.should.have.lengthOf(1); - resJson[0].members[0].should.not.have.property('email'); + should.not.exist(resJson[0].members); done(); } }); From 613beebe9138152f676002556c551be267b6a6a8 Mon Sep 17 00:00:00 2001 From: gets0ul Date: Thu, 7 May 2020 04:07:23 +0700 Subject: [PATCH 2/8] - use separate scope for invites - return project with empty invites if user doesn't have enough permission --- docs/permissions.html | 48 ++++++++++++++++---------------- src/constants.js | 5 ++++ src/permissions/constants.js | 36 ++++++++++++++++-------- src/routes/projects/get.js | 31 +++++++++++++++++++-- src/routes/projects/get.spec.js | 20 +++++++++++++ src/routes/projects/list.js | 18 ++++++++++++ src/routes/projects/list.spec.js | 27 ++++++++++++++++-- 7 files changed, 145 insertions(+), 40 deletions(-) diff --git a/docs/permissions.html b/docs/permissions.html index f1ed60d8..97f5d5f5 100644 --- a/docs/permissions.html +++ b/docs/permissions.html @@ -636,8 +636,8 @@

all:connect_project - all:project-members - read:project-members + all:project-invites + read:project-invites
@@ -670,8 +670,8 @@

all:connect_project - all:project-members - read:project-members + all:project-invites + read:project-invites
@@ -704,8 +704,8 @@

all:connect_project - all:project-members - write:project-members + all:project-invites + write:project-invites
@@ -734,8 +734,8 @@

all:connect_project - all:project-members - write:project-members + all:project-invites + write:project-invites
@@ -759,8 +759,8 @@

all:connect_project - all:project-members - write:project-members + all:project-invites + write:project-invites
@@ -782,8 +782,8 @@

all:connect_project - all:project-members - write:project-members + all:project-invites + write:project-invites
@@ -806,8 +806,8 @@

all:connect_project - all:project-members - write:project-members + all:project-invites + write:project-invites
@@ -831,8 +831,8 @@

all:connect_project - all:project-members - write:project-members + all:project-invites + write:project-invites
@@ -854,8 +854,8 @@

all:connect_project - all:project-members - write:project-members + all:project-invites + write:project-invites
@@ -879,8 +879,8 @@

all:connect_project - all:project-members - write:project-members + all:project-invites + write:project-invites
@@ -909,8 +909,8 @@

all:connect_project - all:project-members - write:project-members + all:project-invites + write:project-invites
@@ -934,8 +934,8 @@

all:connect_project - all:project-members - write:project-members + all:project-invites + write:project-invites
diff --git a/src/constants.js b/src/constants.js index 1babbe14..180bf7ea 100644 --- a/src/constants.js +++ b/src/constants.js @@ -281,6 +281,11 @@ export const M2M_SCOPES = { READ: 'read:project-members', WRITE: 'write:project-members', }, + PROJECT_INVITES: { + ALL: 'all:project-invites', + READ: 'read:project-invites', + WRITE: 'write:project-invites', + }, }; export const TIMELINE_REFERENCES = { diff --git a/src/permissions/constants.js b/src/permissions/constants.js index d89d8277..77b55b46 100644 --- a/src/permissions/constants.js +++ b/src/permissions/constants.js @@ -91,6 +91,18 @@ const SCOPES_PROJECT_MEMBERS_WRITE = [ M2M_SCOPES.PROJECT_MEMBERS.WRITE, ]; +const SCOPES_PROJECT_INVITES_READ = [ + M2M_SCOPES.CONNECT_PROJECT_ADMIN, + M2M_SCOPES.PROJECT_INVITES.ALL, + M2M_SCOPES.PROJECT_INVITES.READ, +]; + +const SCOPES_PROJECT_INVITES_WRITE = [ + M2M_SCOPES.CONNECT_PROJECT_ADMIN, + M2M_SCOPES.PROJECT_INVITES.ALL, + M2M_SCOPES.PROJECT_INVITES.WRITE, +]; + export const PERMISSION = { // eslint-disable-line import/prefer-default-export /* * Project @@ -303,7 +315,7 @@ export const PERMISSION = { // eslint-disable-line import/prefer-default-export description: 'Who can view own invite.', }, topcoderRoles: ALL, - scopes: SCOPES_PROJECT_MEMBERS_READ, + scopes: SCOPES_PROJECT_INVITES_READ, }, READ_PROJECT_INVITE_NOT_OWN: { @@ -314,7 +326,7 @@ export const PERMISSION = { // eslint-disable-line import/prefer-default-export }, topcoderRoles: TOPCODER_ROLES_MANAGERS_AND_ADMINS, projectRoles: ALL, - scopes: SCOPES_PROJECT_MEMBERS_READ, + scopes: SCOPES_PROJECT_INVITES_READ, }, CREATE_PROJECT_INVITE_CUSTOMER: { @@ -325,7 +337,7 @@ export const PERMISSION = { // eslint-disable-line import/prefer-default-export }, topcoderRoles: TOPCODER_ROLES_MANAGERS_AND_ADMINS, projectRoles: ALL, - scopes: SCOPES_PROJECT_MEMBERS_WRITE, + scopes: SCOPES_PROJECT_INVITES_WRITE, }, CREATE_PROJECT_INVITE_NON_CUSTOMER: { @@ -336,7 +348,7 @@ export const PERMISSION = { // eslint-disable-line import/prefer-default-export }, topcoderRoles: TOPCODER_ROLES_ADMINS, projectRoles: PROJECT_ROLES_MANAGEMENT, - scopes: SCOPES_PROJECT_MEMBERS_WRITE, + scopes: SCOPES_PROJECT_INVITES_WRITE, }, CREATE_PROJECT_INVITE_COPILOT_DIRECTLY: { @@ -349,7 +361,7 @@ export const PERMISSION = { // eslint-disable-line import/prefer-default-export ...TOPCODER_ROLES_ADMINS, USER_ROLE.COPILOT_MANAGER, ], - scopes: SCOPES_PROJECT_MEMBERS_WRITE, + scopes: SCOPES_PROJECT_INVITES_WRITE, }, UPDATE_PROJECT_INVITE_OWN: { @@ -359,7 +371,7 @@ export const PERMISSION = { // eslint-disable-line import/prefer-default-export description: 'Who can update own invite.', }, topcoderRoles: ALL, - scopes: SCOPES_PROJECT_MEMBERS_WRITE, + scopes: SCOPES_PROJECT_INVITES_WRITE, }, UPDATE_PROJECT_INVITE_NOT_OWN: { @@ -369,7 +381,7 @@ export const PERMISSION = { // eslint-disable-line import/prefer-default-export description: 'Who can update invites for other members.', }, topcoderRoles: TOPCODER_ROLES_ADMINS, - scopes: SCOPES_PROJECT_MEMBERS_WRITE, + scopes: SCOPES_PROJECT_INVITES_WRITE, }, UPDATE_PROJECT_INVITE_REQUESTED: { @@ -382,7 +394,7 @@ export const PERMISSION = { // eslint-disable-line import/prefer-default-export ...TOPCODER_ROLES_ADMINS, USER_ROLE.COPILOT_MANAGER, ], - scopes: SCOPES_PROJECT_MEMBERS_WRITE, + scopes: SCOPES_PROJECT_INVITES_WRITE, }, DELETE_PROJECT_INVITE_OWN: { @@ -392,7 +404,7 @@ export const PERMISSION = { // eslint-disable-line import/prefer-default-export description: 'Who can delete own invite.', }, topcoderRoles: ALL, - scopes: SCOPES_PROJECT_MEMBERS_WRITE, + scopes: SCOPES_PROJECT_INVITES_WRITE, }, DELETE_PROJECT_INVITE_NOT_OWN_CUSTOMER: { @@ -403,7 +415,7 @@ export const PERMISSION = { // eslint-disable-line import/prefer-default-export }, topcoderRoles: TOPCODER_ROLES_ADMINS, projectRoles: ALL, - scopes: SCOPES_PROJECT_MEMBERS_WRITE, + scopes: SCOPES_PROJECT_INVITES_WRITE, }, DELETE_PROJECT_INVITE_NOT_OWN_NON_CUSTOMER: { @@ -414,7 +426,7 @@ export const PERMISSION = { // eslint-disable-line import/prefer-default-export }, topcoderRoles: TOPCODER_ROLES_ADMINS, projectRoles: PROJECT_ROLES_MANAGEMENT, - scopes: SCOPES_PROJECT_MEMBERS_WRITE, + scopes: SCOPES_PROJECT_INVITES_WRITE, }, DELETE_PROJECT_INVITE_REQUESTED: { @@ -427,7 +439,7 @@ export const PERMISSION = { // eslint-disable-line import/prefer-default-export ...TOPCODER_ROLES_ADMINS, USER_ROLE.COPILOT_MANAGER, ], - scopes: SCOPES_PROJECT_MEMBERS_WRITE, + scopes: SCOPES_PROJECT_INVITES_WRITE, }, /** diff --git a/src/routes/projects/get.js b/src/routes/projects/get.js index 148601b2..a3b220fa 100644 --- a/src/routes/projects/get.js +++ b/src/routes/projects/get.js @@ -3,6 +3,7 @@ import config from 'config'; import { middleware as tcMiddleware } from 'tc-core-library-js'; import models from '../../models'; import util from '../../util'; +import { PERMISSION } from '../../permissions/constants'; const ES_PROJECT_INDEX = config.get('elasticsearchConfig.indexName'); const ES_PROJECT_TYPE = config.get('elasticsearchConfig.docType'); @@ -115,7 +116,23 @@ const retrieveProjectFromES = (projectId, req) => { const es = util.getElasticSearchClient(); es.search(searchCriteria).then((docs) => { const rows = _.map(docs.hits.hits, single => single._source); // eslint-disable-line no-underscore-dangle - accept(rows[0]); + const project = rows[0]; + if (project && project.invites) { + if (!util.hasPermissionByReq(PERMISSION.READ_PROJECT_INVITE_NOT_OWN, req)) { + let invites; + if (util.hasPermissionByReq(PERMISSION.READ_PROJECT_INVITE_OWN, req)) { + // only include own invites + const currentUserId = req.authUser.userId; + const email = req.authUser.email; + invites = _.filter(project.invites, invite => invite.userId === currentUserId || invite.email === email); + } else { + // return empty invites + invites = []; + } + _.set(project, 'invites', invites); + } + } + accept(project); }).catch(reject); }); }; @@ -156,7 +173,17 @@ const retrieveProjectFromDB = (projectId, req) => { if (attachments) { project.attachments = attachments; } - return models.ProjectMemberInvite.getPendingAndReguestedInvitesForProject(projectId); + if (util.hasPermissionByReq(PERMISSION.READ_PROJECT_INVITE_NOT_OWN, req)) { + // include all invites + return models.ProjectMemberInvite.getPendingAndReguestedInvitesForProject(projectId); + } else if (util.hasPermissionByReq(PERMISSION.READ_PROJECT_INVITE_OWN, req)) { + // include only own invites + const currentUserId = req.authUser.userId; + const email = req.authUser.email; + return models.ProjectMemberInvite.getPendingOrRequestedProjectInvitesForUser(projectId, email, currentUserId); + } + // empty + return Promise.resolve([]); }) .then((invites) => { project.invites = invites; diff --git a/src/routes/projects/get.spec.js b/src/routes/projects/get.spec.js index a12e46f6..732f65d5 100644 --- a/src/routes/projects/get.spec.js +++ b/src/routes/projects/get.spec.js @@ -274,6 +274,26 @@ describe('GET Project', () => { }); }); + it('should return the project with empty invites using M2M token without "read:project-invites" scope', (done) => { + request(server) + .get(`/v5/projects/${project1.id}`) + .set({ + Authorization: `Bearer ${testUtil.m2m['read:projects']}`, + }) + .expect('Content-Type', /json/) + .expect(200) + .end((err, res) => { + if (err) { + done(err); + } else { + const resJson = res.body; + should.exist(resJson); + resJson.invites.should.be.empty; + done(); + } + }); + }); + it('should return project with "members", "invites", and "attachments" by default when data comes from ES', (done) => { request(server) .get(`/v5/projects/${data[0].id}`) diff --git a/src/routes/projects/list.js b/src/routes/projects/list.js index a9ea8445..9d1c6afe 100755 --- a/src/routes/projects/list.js +++ b/src/routes/projects/list.js @@ -568,6 +568,24 @@ const retrieveProjects = (req, criteria, sort, ffields) => { const es = util.getElasticSearchClient(); es.search(searchCriteria).then((docs) => { const rows = _.map(docs.hits.hits, single => single._source); // eslint-disable-line no-underscore-dangle + if (rows) { + if (!util.hasPermissionByReq(PERMISSION.READ_PROJECT_INVITE_NOT_OWN, req)) { + if (util.hasPermissionByReq(PERMISSION.READ_PROJECT_INVITE_OWN, req)) { + // only include own invites + const currentUserId = req.authUser.userId; + const email = req.authUser.email; + _.forEach(rows, (fp) => { + const invites = _.filter(fp.invites, invite => invite.userId === currentUserId || invite.email === email); + _.set(fp, 'invites', invites); + }); + } else { + // return empty invites + _.forEach(rows, (fp) => { + _.set(fp, 'invites', []); + }); + } + } + } accept({ rows, count: docs.hits.total, pageSize: criteria.limit, page: criteria.page }); }).catch(reject); }); diff --git a/src/routes/projects/list.spec.js b/src/routes/projects/list.spec.js index e454a181..bc9867c7 100644 --- a/src/routes/projects/list.spec.js +++ b/src/routes/projects/list.spec.js @@ -405,6 +405,29 @@ describe('LIST Project', () => { }); }); + it('should return the project with empty invites using M2M token without "read:project-invites" scope', (done) => { + request(server) + .get('/v5/projects') + .set({ + Authorization: `Bearer ${testUtil.m2m['read:projects']}`, + }) + .expect('Content-Type', /json/) + .expect(200) + .end((err, res) => { + if (err) { + done(err); + } else { + const resJson = res.body; + should.exist(resJson); + resJson.should.have.lengthOf(3); + resJson.forEach((project) => { + project.invites.should.be.empty; + }); + done(); + } + }); + }); + it('should return the project when project that is in reviewed state in which the copilot is its member or has been invited', (done) => { request(server) .get('/v5/projects') @@ -1130,9 +1153,9 @@ describe('LIST Project', () => { should.exist(resJson); resJson.should.have.lengthOf(1); resJson[0].name.should.equal('test1'); - resJson[0].invites.should.have.lengthOf(2); + resJson[0].invites.should.have.lengthOf(1); resJson[0].invites[0].should.have.property('email'); - resJson[0].invites[1].email.should.equal('h***o@w***d.com'); + resJson[0].invites[0].userId.should.equal(40051335); done(); } }); From cb1517e78cb7f4a0a7146927506bdb218a2455db Mon Sep 17 00:00:00 2001 From: gets0ul Date: Fri, 8 May 2020 17:46:15 +0700 Subject: [PATCH 3/8] filter invites by email in case-insensitive way --- src/routes/projects/get.js | 7 +++++-- src/routes/projects/list.js | 5 ++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/routes/projects/get.js b/src/routes/projects/get.js index a3b220fa..5594594c 100644 --- a/src/routes/projects/get.js +++ b/src/routes/projects/get.js @@ -123,8 +123,11 @@ const retrieveProjectFromES = (projectId, req) => { if (util.hasPermissionByReq(PERMISSION.READ_PROJECT_INVITE_OWN, req)) { // only include own invites const currentUserId = req.authUser.userId; - const email = req.authUser.email; - invites = _.filter(project.invites, invite => invite.userId === currentUserId || invite.email === email); + const currentUserEmail = req.authUser.email; + invites = _.filter(project.invites, invite => ( + (invite.userId !== null && invite.userId === currentUserId) || + (invite.email && currentUserEmail && invite.email.toLowerCase() === currentUserEmail.toLowerCase()) + )); } else { // return empty invites invites = []; diff --git a/src/routes/projects/list.js b/src/routes/projects/list.js index 9d1c6afe..fce01062 100755 --- a/src/routes/projects/list.js +++ b/src/routes/projects/list.js @@ -575,7 +575,10 @@ const retrieveProjects = (req, criteria, sort, ffields) => { const currentUserId = req.authUser.userId; const email = req.authUser.email; _.forEach(rows, (fp) => { - const invites = _.filter(fp.invites, invite => invite.userId === currentUserId || invite.email === email); + const invites = _.filter(fp.invites, invite => ( + (invite.userId !== null && invite.userId === currentUserId) || + (invite.email && currentUserEmail && invite.email.toLowerCase() === currentUserEmail.toLowerCase()) + )); _.set(fp, 'invites', invites); }); } else { From 1c3cd6a1de438bc3dfa529c444055346fa6f6147 Mon Sep 17 00:00:00 2001 From: gets0ul Date: Fri, 8 May 2020 21:59:20 +0700 Subject: [PATCH 4/8] - Fix checking project members permission logic in GET /projects endpoint. - Revert test and use another regular user. --- src/routes/projects/list.js | 27 +++++++++++++++++++++++++-- src/routes/projects/list.spec.js | 14 ++++++-------- 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/src/routes/projects/list.js b/src/routes/projects/list.js index 3694e3c4..859072cc 100755 --- a/src/routes/projects/list.js +++ b/src/routes/projects/list.js @@ -552,8 +552,7 @@ const retrieveProjects = (req, criteria, sort, ffields) => { // parse the fields string to determine what fields are to be returned fields = util.parseFields(fields, { projects: PROJECT_ATTRIBUTES, - project_members: util.hasPermissionByReq(PERMISSION.READ_PROJECT_MEMBER, req) ? - util.addUserDetailsFieldsIfAllowed(PROJECT_MEMBER_ATTRIBUTES_ES, req) : null, + project_members: util.addUserDetailsFieldsIfAllowed(PROJECT_MEMBER_ATTRIBUTES_ES, req), project_member_invites: PROJECT_MEMBER_INVITE_ATTRIBUTES, project_phases: PROJECT_PHASE_ATTRIBUTES, project_phases_products: PROJECT_PHASE_PRODUCTS_ATTRIBUTES, @@ -564,12 +563,36 @@ const retrieveProjects = (req, criteria, sort, ffields) => { if (_.indexOf(fields.projects, 'id') < 0) { fields.projects.push('id'); } + // add userId to project_members field so it can be used to check READ_PROJECT_MEMBER permission below. + const addMembersUserId = fields.project_members.length > 0 && _.indexOf(fields.project_members, 'userId') < 0; + if (addMembersUserId) { + fields.project_members.push('userId'); + } const searchCriteria = parseElasticSearchCriteria(criteria, fields, order) || {}; return new Promise((accept, reject) => { const es = util.getElasticSearchClient(); es.search(searchCriteria).then((docs) => { const rows = _.map(docs.hits.hits, single => single._source); // eslint-disable-line no-underscore-dangle + if (rows) { + _.forEach(rows, (p) => { + const fp = p; + if (fp.members) { + // check if have permission to read project members + if (!util.hasPermission(PERMISSION.READ_PROJECT_MEMBER, req.authUser, fp.members)) { + delete fp.members; + } + if (fp.members && addMembersUserId) { + // remove the userId from the returned members array if it was added before + // as it is only needed for checking permission. + _.forEach(fp.members, (m) => { + const fm = m; + delete fm.userId; + }); + } + } + }); + } accept({ rows, count: docs.hits.total, pageSize: criteria.limit, page: criteria.page }); }).catch(reject); }); diff --git a/src/routes/projects/list.spec.js b/src/routes/projects/list.spec.js index fcc39226..f57ecdb3 100644 --- a/src/routes/projects/list.spec.js +++ b/src/routes/projects/list.spec.js @@ -1182,12 +1182,11 @@ describe('LIST Project', () => { }); describe('URL Query fields', () => { - it(`should not include project members when "fields" query param is not defined (to non-admin users) - without READ_PROJECT_MEMBER permission`, (done) => { + it('should not return "email" for project members when "fields" query param is not defined (to non-admin users)', (done) => { request(server) .get('/v5/projects/') .set({ - Authorization: `Bearer ${testUtil.jwts.member2}`, + Authorization: `Bearer ${testUtil.jwts.member}`, }) .expect('Content-Type', /json/) .expect(200) @@ -1198,19 +1197,18 @@ describe('LIST Project', () => { const resJson = res.body; should.exist(resJson); resJson.should.have.lengthOf(1); - should.not.exist(resJson[0].members); + resJson[0].members[0].should.not.have.property('email'); done(); } }); }); - it(`should not include project members even if it's listed in "fields" query param (to non-admin users) - without READ_PROJECT_MEMBER permission`, (done) => { + it('should not return "email" for project members even if it\'s listed in "fields" query param (to non-admin users)', (done) => { request(server) .get('/v5/projects/?fields=members.email,members.id') .set({ - Authorization: `Bearer ${testUtil.jwts.member2}`, + Authorization: `Bearer ${testUtil.jwts.member}`, }) .expect('Content-Type', /json/) .expect(200) @@ -1221,7 +1219,7 @@ describe('LIST Project', () => { const resJson = res.body; should.exist(resJson); resJson.should.have.lengthOf(1); - should.not.exist(resJson[0].members); + resJson[0].members[0].should.not.have.property('email'); done(); } }); From fc1a929e5ab4b494b1044d8faae589c1a2acbc47 Mon Sep 17 00:00:00 2001 From: gets0ul Date: Sat, 9 May 2020 23:57:46 +0700 Subject: [PATCH 5/8] Fix read project members permission logic from DB --- src/routes/projects/list.js | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/routes/projects/list.js b/src/routes/projects/list.js index 859072cc..3998e688 100755 --- a/src/routes/projects/list.js +++ b/src/routes/projects/list.js @@ -492,9 +492,13 @@ const retrieveProjectsFromDB = (req, criteria, sort, ffields) => { // make sure project.id is part of fields if (_.indexOf(fields.projects, 'id') < 0) fields.projects.push('id'); + // add userId to project_members field so it can be used to check READ_PROJECT_MEMBER permission below. + const addMembersUserId = fields.project_members.length > 0 && _.indexOf(fields.project_members, 'userId') < 0; + if (addMembersUserId) { + fields.project_members.push('userId'); + } const retrieveAttachments = !req.query.fields || req.query.fields.indexOf('attachments') > -1; - const retrieveMembers = (!req.query.fields || !!fields.project_members.length) - && util.hasPermissionByReq(PERMISSION.READ_PROJECT_MEMBER, req); + const retrieveMembers = !req.query.fields || !!fields.project_members.length; return models.Project.searchText({ filters: criteria.filters, @@ -534,7 +538,19 @@ const retrieveProjectsFromDB = (req, criteria, sort, ffields) => { const p = fp; // if values length is 1 it could be either attachments or members if (retrieveMembers) { - p.members = _.filter(allMembers, m => m.projectId === p.id); + const pMembers = _.filter(allMembers, m => m.projectId === p.id); + // check if have permission to read project members + if (util.hasPermission(PERMISSION.READ_PROJECT_MEMBER, req.authUser, pMembers)) { + if (addMembersUserId) { + // remove the userId from the returned members array if it was added before + // as it is only needed for checking permission. + _.forEach(pMembers, (m) => { + const fm = m; + delete fm.userId; + }); + } + p.members = pMembers; + } } if (retrieveAttachments) { p.attachments = _.filter(allAttachments, a => a.projectId === p.id); From b49c0a3147a040865a9b417096e30d3ca811ff22 Mon Sep 17 00:00:00 2001 From: gets0ul Date: Mon, 11 May 2020 13:36:05 +0700 Subject: [PATCH 6/8] fix typo and lint error --- src/routes/projects/list.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/routes/projects/list.js b/src/routes/projects/list.js index fce01062..3fc9010c 100755 --- a/src/routes/projects/list.js +++ b/src/routes/projects/list.js @@ -573,7 +573,7 @@ const retrieveProjects = (req, criteria, sort, ffields) => { if (util.hasPermissionByReq(PERMISSION.READ_PROJECT_INVITE_OWN, req)) { // only include own invites const currentUserId = req.authUser.userId; - const email = req.authUser.email; + const currentUserEmail = req.authUser.email; _.forEach(rows, (fp) => { const invites = _.filter(fp.invites, invite => ( (invite.userId !== null && invite.userId === currentUserId) || From f09e4dff9adfb69784ae30befb143234b97db0f2 Mon Sep 17 00:00:00 2001 From: Maksym Mykhailenko Date: Mon, 11 May 2020 12:42:35 +0300 Subject: [PATCH 7/8] fix: lint --- src/routes/projects/list.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/routes/projects/list.spec.js b/src/routes/projects/list.spec.js index eb348e1e..d48489c6 100644 --- a/src/routes/projects/list.spec.js +++ b/src/routes/projects/list.spec.js @@ -404,7 +404,7 @@ describe('LIST Project', () => { } }); }); - + it('should return the project with empty invites using M2M token without "read:project-invites" scope', (done) => { request(server) .get('/v5/projects') From da6dee68c5af03a498abc24bcfd2ddce3761aff0 Mon Sep 17 00:00:00 2001 From: Maksym Mykhailenko Date: Mon, 11 May 2020 12:43:00 +0300 Subject: [PATCH 8/8] fix: "generalPermission" middleware --- src/permissions/generalPermission.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/permissions/generalPermission.js b/src/permissions/generalPermission.js index ca7c8295..c436f8a8 100644 --- a/src/permissions/generalPermission.js +++ b/src/permissions/generalPermission.js @@ -38,7 +38,7 @@ module.exports = permissions => async (req) => { // if one of the `permission` requires to know Project Members, but current route doesn't belong to any project // this means such `permission` most likely has been applied by mistake, so we throw an error const permissionsRequireProjectMembers = _.isArray(permissions) - ? _.some(permissions, permission => util.hasPermissionByReq(permission, req)) + ? _.some(permissions, permission => util.isPermissionRequireProjectMembers(permission)) : util.isPermissionRequireProjectMembers(permissions); if (_.isUndefined(req.params.projectId) && permissionsRequireProjectMembers) { @@ -60,6 +60,7 @@ module.exports = permissions => async (req) => { // - if user has permissions to access endpoint even we don't know if he is a member or no, // then code would proceed and endpoint would decide to throw 404 if project doesn't exist // or perform endpoint operation if loading project members above failed because of some other reason + req.log.error(`Cannot load project members: ${err.message}.`); } }