Merge pull request #569 from gets0ul/issue_560

Fix for Issue #560 Don't return members to user without enough permission

Author: maxceem

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);

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();
             }
           });

src/routes/projects/list.js

@@ -492,6 +492,11 @@ 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;
 
@@ -533,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);
@@ -562,12 +579,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);
   });

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')
@@ -1163,7 +1186,7 @@ describe('LIST Project', () => {
         request(server)
         .get('/v5/projects/')
         .set({
-          Authorization: `Bearer ${testUtil.jwts.member2}`,
+          Authorization: `Bearer ${testUtil.jwts.member}`,
         })
         .expect('Content-Type', /json/)
         .expect(200)
@@ -1185,7 +1208,7 @@ describe('LIST Project', () => {
         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)