Repository: CVEProject/cve-services
Pull request #324 (2021-02-24 18:22:17)

Adding utils to check if a user is an Admin
Fixing 500 error when resetting a user api secret by adding utils functions.


Patch:
diff --git a/src/controller/org.controller/org.controller.js b/src/controller/org.controller/org.controller.js
index b107596d..1ec2819e 100644
--- a/src/controller/org.controller/org.controller.js
+++ b/src/controller/org.controller/org.controller.js
@@ -692,7 +692,7 @@ async function resetSecret (req, res, next) {
     const orgRepo = req.ctx.repositories.getOrgRepository()
     const orgUUID = await orgRepo.getOrgUUID(orgShortName) // userUUID may be null if user does not exist
     const isSecretariat = await orgRepo.isSecretariat(requesterShortName)
-    const isOrgAdmin = await userRepo.isOrgAdmin(requesterUsername, orgUUID)
+    const isAdmin = await userRepo.isAdmin(requesterUsername, requesterShortName)
 
     if (!orgUUID) {
       logger.info({ uuid: req.ctx.uuid, messsage: orgShortName + ' organization does not exist.' })
@@ -705,9 +705,13 @@ async function resetSecret (req, res, next) {
       return res.status(404).json(error.userDne(username))
     }
 
-    if ((orgShortName !== requesterShortName || username !== requesterUsername) && !isSecretariat && !isOrgAdmin) {
-      logger.info({ uuid: req.ctx.uuid, message: 'The api secret can only be reset by the Secretariat, an Org admin or if the requester is the user.' })
-      return res.status(403).json(error.notSameUserOrSecretariat())
+    // check if the user is not the requester or if the requester is not a secretariat
+    if ((orgShortName !== requesterShortName || username !== requesterUsername) && !isSecretariat) {
+      // check if the requester is not and admin; if admin, the requester must be from the same org as the user
+      if (!isAdmin || (isAdmin && orgShortName !== requesterShortName)) {
+        logger.info({ uuid: req.ctx.uuid, message: 'The api secret can only be reset by the Secretariat, an Org admin or if the requester is the user.' })
+        return res.status(403).json(error.notSameUserOrSecretariat())
+      }
     }
 
     logger.info({ uuid: req.ctx.uuid, message: `The API secret was successfully reset and sent to ${username}` })
diff --git a/src/repositories/userRepository.js b/src/repositories/userRepository.js
index 47f32b82..3a701dec 100644
--- a/src/repositories/userRepository.js
+++ b/src/repositories/userRepository.js
@@ -11,6 +11,14 @@ class UserRepository extends BaseRepository {
     return utils.getUserUUID(userName, orgUUID)
   }
 
+  async isAdmin (username, shortname) {
+    return utils.isAdmin(username, shortname)
+  }
+
+  async isAdminUUID (username, orgUUID) {
+    return utils.isAdminUUID(username, orgUUID)
+  }
+
   async findOneByUUID (UUID) {
     return this.collection.findOne().byUUID(UUID)
   }
@@ -22,20 +30,6 @@ class UserRepository extends BaseRepository {
   async updateByUserNameAndOrgUUID (username, orgUUID, user, options = {}) {
     return this.collection.findOneAndUpdate().byUserNameAndOrgUUID(username, orgUUID).updateOne(user).setOptions(options)
   }
-
-  async isOrgAdmin (username, orgUUID) {
-    let result = false
-    const user = this.collection.findOne().byUserNameAndOrgUUID(username, orgUUID)
-    if (user) {
-      const roles = user.authority.active_roles
-      roles.forEach((x) => {
-        if (x === 'ADMIN') {
-          result = true
-        }
-      })
-    }
-    return result
-  }
 }
 
 module.exports = UserRepository
diff --git a/src/utils/utils.js b/src/utils/utils.js
index e2a40a68..3f0d48d6 100644
--- a/src/utils/utils.js
+++ b/src/utils/utils.js
@@ -51,6 +51,35 @@ async function isSecretariatUUID (orgUUID) {
   return result // org is not secretariat
 }
 
+async function isAdmin (requesterUsername, requesterShortName) {
+  let result = false
+  const requesterOrgUUID = await getOrgUUID(requesterShortName) // may be null if org does not exists
+
+  if (requesterOrgUUID) {
+    const user = await User.findOne().byUserNameAndOrgUUID(requesterUsername, requesterOrgUUID)
+
+    if (user) {
+      result = user.authority.active_roles.includes(CONSTANTS.USER_ROLE_ENUM.ADMIN)
+    }
+  }
+
+  return result // org is not secretariat
+}
+
+async function isAdminUUID (requesterUsername, requesterOrgUUID) {
+  let result = false
+
+  if (requesterOrgUUID) {
+    const user = await User.findOne().byUserNameAndOrgUUID(requesterUsername, requesterOrgUUID)
+
+    if (user) {
+      result = user.authority.active_roles.includes(CONSTANTS.USER_ROLE_ENUM.ADMIN)
+    }
+  }
+
+  return result // org is not secretariat
+}
+
 function reqCtxMapping (req, keyType, keys) {
   if (!(keyType in req.ctx)) {
     req.ctx[keyType] = {}
@@ -87,6 +116,8 @@ function setPaginatorHeaders (res, pg) {
 
 module.exports = {
   isSecretariat,
+  isAdmin,
+  isAdminUUID,
   isEmptyObject,
   isSecretariatUUID,
   getOrgUUID,


Test patch:
diff --git a/test-utils/index.js b/test-utils/index.js
index 0e1c1586..3656ea46 100644
--- a/test-utils/index.js
+++ b/test-utils/index.js
@@ -457,7 +457,7 @@ app.route('/user-secret-reset-sameOrg/:shortname/:username')
   .post((req, res, next) => {
     const factory = {
       getOrgRepository: () => { return new repos.OrgUserSecretResetNotSecretariat() },
-      getUserRepository: () => { return new repos.UserSecretReset() }
+      getUserRepository: () => { return new repos.UserSecretResetNotAdmin() }
     }
     req.ctx.repositories = factory
     next()
@@ -513,6 +513,16 @@ app.route('/user-secret-not-reset-userIsOrgAdminForDifferentOrg/:shortname/:user
     next()
   }, orgParams.parsePostParams, orgController.USER_RESET_SECRET)
 
+app.route('/user-secret-not-reset-userIsNotUserAdminOrSecretariat/:shortname/:username')
+  .post((req, res, next) => {
+    const factory = {
+      getOrgRepository: () => { return new repos.OrgUserSecretResetNotSecretariat() },
+      getUserRepository: () => { return new repos.UserSecretResetNotAdmin() }
+    }
+    req.ctx.repositories = factory
+    next()
+  }, orgParams.parsePostParams, orgController.USER_RESET_SECRET)
+
 // --- get user --- //
 app.route('/user-get-user-org-doesnt-exist/:shortname/:username')
   .get((req, res, next) => {
diff --git a/test-utils/repositories.js b/test-utils/repositories.js
index a064dab7..90fbae0f 100644
--- a/test-utils/repositories.js
+++ b/test-utils/repositories.js
@@ -2404,7 +2404,7 @@ class NullUserRepo {
     return null
   }
 
-  async isOrgAdmin () {
+  async isAdmin () {
     return null
   }
 }
@@ -2613,7 +2613,7 @@ class UserSecretNotResetUserDoesntExist {
     return { n: 0, nModified: 0, ok: 1 }
   }
 
-  async isOrgAdmin () {
+  async isAdmin () {
     return false
   }
 }
@@ -2633,10 +2633,27 @@ class UserSecretReset {
     return orgMockObj.userA.UUID
   }
 
-  async isOrgAdmin (username, orgUUID) {
-    if (username === orgMockObj.userD.username && orgUUID === orgMockObj.userD.org_UUID) {
+  async isAdmin (username, shortname) {
+    if (username === orgMockObj.userD.username && shortname === orgMockObj.existentOrgDummy.short_name) {
+      return true
+    } else if (username === orgMockObj.userA.username && shortname === orgMockObj.existentOrgDummy.short_name) {
       return true
     }
+
+    return false
+  }
+}
+
+class UserSecretResetNotAdmin {
+  async updateByUserNameAndOrgUUID () {
+    return { n: 1, nModified: 1, ok: 1 }
+  }
+
+  async getUserUUID () {
+    return null
+  }
+
+  async isAdmin () {
     return false
   }
 }
@@ -2808,6 +2825,7 @@ module.exports = {
   OrgUserSecretNotResetUserDoesntExist,
   OrgUserSecretReset,
   OrgUserSecretResetNotSecretariat,
+  UserSecretResetNotAdmin,
   OrgGetUserOrgDoesntExist,
   OrgGetUser,
   OrgGetUserUpdated,
diff --git a/test/unit-tests/org/mockObjects.org.js b/test/unit-tests/org/mockObjects.org.js
index 47aa1fa4..053a2f66 100644
--- a/test/unit-tests/org/mockObjects.org.js
+++ b/test/unit-tests/org/mockObjects.org.js
@@ -285,7 +285,7 @@ const userD = { // same org_UUID as userA but org admin
     last: 'Smith'
   },
   authority: {
-    active_roles: CONSTANTS.USER_ROLES
+    active_roles: [CONSTANTS.USER_ROLE_ENUM.ADMIN]
   },
   org_UUID: existentOrgDummy.UUID,
   UUID: '33394284-4acf-423b-b199-9e57656ee451',
diff --git a/test/unit-tests/org/userTest.js b/test/unit-tests/org/userTest.js
index ef1eb96f..c78e2424 100644
--- a/test/unit-tests/org/userTest.js
+++ b/test/unit-tests/org/userTest.js
@@ -466,6 +466,7 @@ describe('Test user functions in Org Controller', () => {
         })
     })
 
+    // requester is not the same user (same org but different username)
     it('Requester is from same org but has different username: User secret is not reset because requester is not the same user or is the secretariat or org admin', (done) => {
       chai.request(server)
         .post(`/user-secret-reset-sameOrg/${existentOrgDummy.short_name}/${userC.username}`) // requester has same org as user but different username
@@ -484,6 +485,7 @@ describe('Test user functions in Org Controller', () => {
         })
     })
 
+    // requester is not the same user (same username but different org)
     it('Requester has same username but is from different org: User secret is not reset because requester is not the same user or is the secretariat or org admin', (done) => {
       chai.request(server)
         .post(`/user-secret-reset-sameUsername/${owningOrg.short_name}/${userB.username}`) // requester has same username as user but different org
@@ -502,18 +504,40 @@ describe('Test user functions in Org Controller', () => {
         })
     })
 
-    it('Secret is reset because requester is a secretariat', (done) => {
+    // requester is admin but does not belong to the same user's org
+    it('Secret is not reset because requester is org admin but does not belong to the same org', (done) => {
       chai.request(server)
-        .post(`/user-secret-reset-secretariat/${existentOrg.short_name}/${existentUser.username}`)
-        .set(secretariatHeader)
+        .post(`/user-secret-not-reset-userIsOrgAdminForDifferentOrg/${owningOrg.short_name}/${userB.username}`)
+        .set(userDHeader)
         .end((err, res) => {
           if (err) {
             done(err)
           }
 
-          expect(res).to.have.status(200)
+          expect(res).to.have.status(403)
           expect(res).to.have.property('body').and.to.be.a('object')
-          expect(res.body).to.have.property('API-secret').and.to.be.a('string')
+          const errObj = error.notSameUserOrSecretariat()
+          expect(res.body.error).to.equal(errObj.error)
+          expect(res.body.message).to.equal(errObj.message)
+          done()
+        })
+    })
+
+    // requester is not a secretariat or an admin
+    it('Secret is not reset because requester is not the user, a secretariat or an admin', (done) => {
+      chai.request(server)
+        .post(`/user-secret-not-reset-userIsNotUserAdminOrSecretariat/${existentOrgDummy.short_name}/${userC.username}`)
+        .set(owningOrgHeader)
+        .end((err, res) => {
+          if (err) {
+            done(err)
+          }
+
+          expect(res).to.have.status(403)
+          expect(res).to.have.property('body').and.to.be.a('object')
+          const errObj = error.notSameUserOrSecretariat()
+          expect(res.body.error).to.equal(errObj.error)
+          expect(res.body.message).to.equal(errObj.message)
           done()
         })
     })
@@ -534,35 +558,37 @@ describe('Test user functions in Org Controller', () => {
         })
     })
 
-    it('Secret is not reset because requester is org admin but not for designated org', (done) => {
+    it('Secret is reset because requester is a secretariat', (done) => {
       chai.request(server)
-        .post(`/user-secret-not-reset-userIsOrgAdminForDifferentOrg/${owningOrg.short_name}/${userB.username}`)
-        .set(userDHeader)
+        .post(`/user-secret-reset-secretariat/${existentOrg.short_name}/${existentUser.username}`)
+        .set(secretariatHeader)
         .end((err, res) => {
           if (err) {
             done(err)
           }
 
-          expect(res).to.have.status(403)
+          expect(res).to.have.status(200)
           expect(res).to.have.property('body').and.to.be.a('object')
-          const errObj = error.notSameUserOrSecretariat()
-          expect(res.body.error).to.equal(errObj.error)
-          expect(res.body.message).to.equal(errObj.message)
+          expect(res.body).to.have.property('API-secret').and.to.be.a('string')
           done()
         })
     })
 
-    it('Secret is reset because requester is the org admin', (done) => {
+    it('Secret is reset because requester is an admin that belongs to the same org', (done) => {
+      userA.authority.active_roles = [CONSTANTS.USER_ROLE_ENUM.ADMIN]
+
       chai.request(server)
         .post(`/user-secret-reset-userIsOrgAdmin/${existentOrgDummy.short_name}/${userC.username}`)
-        .set(userDHeader)
+        .set(userAHeader)
         .end((err, res) => {
           if (err) {
             done(err)
           }
+
           expect(res).to.have.status(200)
           expect(res).to.have.property('body').and.to.be.a('object')
           expect(res.body).to.have.property('API-secret').and.to.be.a('string')
+          userA.authority.active_roles = []
           done()
         })
     })