Repository: CVEProject/cve-services (js)

Issue:
AUTH_ROLE_ENUM.BULK_DOWNLOAD might be inconsistent with onlyOrgWithRole
An organization with AUTH_ROLE_ENUM.BULK_DOWNLOAD needs to access the GET /cve endpoint, which is otherwise only available to the Secretariat. It does not need to be able to perform any write operations on the CVE Services database. However,
https://github.com/CVEProject/cve-services/blob/301322d7064bff0a968f3c6d7046ee6c48621629/src/middleware/middleware.js#L233-L247
enables certain write access by an organization with any role.

Possibly the simplest solution would be to rename onlyOrgWithRole to onlyOrgWithPartnerRole, and return the 403 error when the only role is in the dataRetrieval role category instead of the Partner role category. For example, if org.authority.active_roles.length equals 1, and the organization has the BULK_DOWNLOAD role, then the outcome is a 403 error.

There might be a future requirement for a SECRETARIAT_READ_ONLY role, in which any GET request is completed in the same way as for the SECRETARIAT role, but POST and PUT are denied. (I don't know whether this will happen, or what the time scale of the requirement might be.) 

Pull request:
Resolves #1013 Updated onlyOrgWithRole middleware to onlyOrgWithPartnerRole

Closes Issue #1013

## Summary
The `onlyOrgsWithRole` middleware function allowed the `BULK_DOWNLOAD` organization to access non GET endpoints. This goes against the principle of least privilege, so the middleware was updated to allow only orgs with partner roles, like Cna, Adp, to be granted access.

### Important Changes
`middleware.js`
- Updated `onlyOrgsWithRole` function to check for orgs with non-partner roles

### Testing

Steps to manually test updated functionality, if possible
- [x] 1) Run `npm run test:unit-tests`
- [x] 2) Ensure orgs with just the `BULK_DOWNLOAD` role can only access the GET /cve endpoint


Patch:
diff --git a/src/controller/org.controller/index.js b/src/controller/org.controller/index.js
index 2be6777a..b2b5c128 100644
--- a/src/controller/org.controller/index.js
+++ b/src/controller/org.controller/index.js
@@ -545,7 +545,7 @@ router.post('/org/:shortname/user',
   */
   mw.validateUser,
   mw.onlySecretariatOrAdmin,
-  mw.onlyOrgWithRole,
+  mw.onlyOrgWithPartnerRole,
   param(['shortname']).isString().trim().escape().notEmpty().isLength({ min: CONSTANTS.MIN_SHORTNAME_LENGTH, max: CONSTANTS.MAX_SHORTNAME_LENGTH }),
   body(['username']).isString().trim().escape().notEmpty().custom(isValidUsername),
   body(['org_uuid']).optional().isString().trim().escape(),
@@ -712,7 +712,7 @@ router.put('/org/:shortname/user/:username',
   }
   */
   mw.validateUser,
-  mw.onlyOrgWithRole,
+  mw.onlyOrgWithPartnerRole,
   query().custom((query) => {
     return mw.validateQueryParameterNames(query, ['active', 'new_username', 'org_short_name', 'name.first', 'name.last', 'name.middle',
       'name.suffix', 'active_roles.add', 'active_roles.remove'])
@@ -806,7 +806,7 @@ router.put('/org/:shortname/user/:username/reset_secret',
   }
   */
   mw.validateUser,
-  mw.onlyOrgWithRole,
+  mw.onlyOrgWithPartnerRole,
   param(['shortname']).isString().trim().escape().notEmpty().isLength({ min: CONSTANTS.MIN_SHORTNAME_LENGTH, max: CONSTANTS.MAX_SHORTNAME_LENGTH }),
   param(['username']).isString().trim().escape().notEmpty().custom(isValidUsername),
   parseError,
diff --git a/src/middleware/error.js b/src/middleware/error.js
index 5d667328..afc658a1 100644
--- a/src/middleware/error.js
+++ b/src/middleware/error.js
@@ -63,6 +63,13 @@ class MiddlewareError extends idrErr.IDRError {
     return err
   }
 
+  orgHasNoPartnerRole (shortname) { // mw
+    const err = {}
+    err.error = 'ORG_HAS_NO_PARTNER_ROLE'
+    err.message = `The '${shortname}' organization designated by the shortname parameter does not have any partner roles.`
+    return err
+  }
+
   orgDoesNotOwnId (org, id) {
     const err = {
       error: 'ORG_DOES_NOT_OWN_ID',
diff --git a/src/middleware/middleware.js b/src/middleware/middleware.js
index babe8376..8e1ff53f 100644
--- a/src/middleware/middleware.js
+++ b/src/middleware/middleware.js
@@ -255,7 +255,7 @@ async function onlyAdps (req, res, next) {
   }
 }
 // Checks that an org has a role or any sort
-async function onlyOrgWithRole (req, res, next) {
+async function onlyOrgWithPartnerRole (req, res, next) {
   const shortName = req.ctx.org
   const orgRepo = req.ctx.repositories.getOrgRepository()
 
@@ -264,12 +264,15 @@ async function onlyOrgWithRole (req, res, next) {
     if (org === null) {
       logger.info({ uuid: req.ctx.uuid, message: shortName + ' does NOT exist ' })
       return res.status(404).json(error.orgDoesNotExist(shortName))
+    } else if (org.authority.active_roles.length === 1 && org.authority.active_roles[0] === 'BULK_DOWNLOAD') {
+      logger.info({ uuid: req.ctx.uuid, message: org.short_name + 'only has BULK_DOWNLOAD role ' })
+      return res.status(403).json(error.orgHasNoPartnerRole(shortName))
     } else if (org.authority.active_roles.length > 0) {
       logger.info({ uuid: req.ctx.uuid, message: org.short_name + ' has a role ' })
       next()
     } else {
       logger.info({ uuid: req.ctx.uuid, message: org.short_name + ' does NOT have a role ' })
-      return res.status(403).json(error.orgHasNoRole(shortName))
+      return res.status(403).json(error.orgHasNoPartnerRole(shortName))
     }
   } catch (err) {
     next(err)
@@ -445,7 +448,7 @@ module.exports = {
   onlySecretariatOrAdmin,
   onlyCnas,
   onlyAdps,
-  onlyOrgWithRole,
+  onlyOrgWithPartnerRole,
   validateQueryParameterNames,
   cnaMustOwnID,
   createCtxAndReqUUID,


Test patch:
diff --git a/test/unit-tests/middleware/onlyOrgWithPartnerRoleTest.js b/test/unit-tests/middleware/onlyOrgWithPartnerRoleTest.js
new file mode 100644
index 00000000..789588a3
--- /dev/null
+++ b/test/unit-tests/middleware/onlyOrgWithPartnerRoleTest.js
@@ -0,0 +1,189 @@
+/* eslint-disable no-unused-expressions */
+
+const chai = require('chai')
+const sinon = require('sinon')
+const { faker } = require('@faker-js/faker')
+const expect = chai.expect
+
+const OrgRepository = require('../../../src/repositories/orgRepository.js')
+const { onlyOrgWithPartnerRole } = require('../../../src/middleware/middleware.js')
+const errors = require('../../../src/middleware/error.js')
+const error = new errors.MiddlewareError()
+
+const stubAdpOrg = {
+  short_name: 'adpOrg',
+  name: 'test_adp',
+  UUID: faker.datatype.uuid(),
+  authority: {
+    active_roles: [
+      'ADP'
+    ]
+  }
+}
+
+const stubCnaOrg = {
+  short_name: 'cnaOrg',
+  name: 'test_cna',
+  UUID: faker.datatype.uuid(),
+  authority: {
+    active_roles: [
+      'CNA'
+    ]
+  }
+}
+
+const stubBulkDownloadOrg = {
+  short_name: 'bdOrg',
+  name: 'test_bd',
+  UUID: faker.datatype.uuid(),
+  authority: {
+    active_roles: [
+      'BULK_DOWNLOAD'
+    ]
+  }
+}
+
+const stubOrgNoRole = {
+  short_name: 'NoRole',
+  name: 'test_org',
+  UUID: faker.datatype.uuid(),
+  authority: {
+    active_roles: []
+  }
+}
+
+const stubSecretariat = {
+  short_name: 'secOrg',
+  name: 'test_sec',
+  UUID: faker.datatype.uuid(),
+  authority: {
+    active_roles: [
+      'SECRETARIAT'
+    ]
+  }
+}
+
+describe('Testing onlyOrgWithPartnerRole middleware', () => {
+  let status, json, res, next, getOrgRepository, orgRepo
+  beforeEach(() => {
+    status = sinon.stub()
+    json = sinon.spy()
+    res = { json, status }
+    next = sinon.spy()
+    status.returns(res)
+    orgRepo = new OrgRepository()
+    getOrgRepository = sinon.stub()
+    getOrgRepository.returns(orgRepo)
+  })
+  context('Negative Tests', () => {
+    it('Should return 403 for users from orgs without a partner role ', async () => {
+      const req = {
+        ctx: {
+          org: stubBulkDownloadOrg.short_name,
+          uuid: stubBulkDownloadOrg.UUID,
+          repositories: {
+            getOrgRepository
+          }
+        }
+      }
+      const stub = sinon.stub(orgRepo, 'findOneByShortName').returns(stubBulkDownloadOrg)
+
+      await onlyOrgWithPartnerRole(req, res, next)
+      expect(stub.calledOnce).to.be.true
+      expect(status.calledOnce).to.be.true
+      expect(status.args[0][0]).to.equal(403)
+      expect(res.json.args[0][0].error).to.equal(error.orgHasNoPartnerRole().error)
+    })
+    it('Should return 403 for users from orgs without a role ', async () => {
+      const req = {
+        ctx: {
+          org: stubOrgNoRole.short_name,
+          uuid: stubOrgNoRole.UUID,
+          repositories: {
+            getOrgRepository
+          }
+        }
+      }
+      const stub = sinon.stub(orgRepo, 'findOneByShortName').returns(stubOrgNoRole)
+
+      await onlyOrgWithPartnerRole(req, res, next)
+      expect(stub.calledOnce).to.be.true
+      expect(status.calledOnce).to.be.true
+      expect(status.args[0][0]).to.equal(403)
+      expect(res.json.args[0][0].error).to.equal(error.orgHasNoPartnerRole().error)
+    })
+
+    it('Should return 404 if the requester org does not exist', async () => {
+      const req = {
+        ctx: {
+          org: stubCnaOrg.short_name,
+          uuid: stubCnaOrg.UUID,
+          repositories: {
+            getOrgRepository
+          }
+        }
+      }
+      const stub = sinon.stub(orgRepo, 'findOneByShortName').returns(null)
+
+      await onlyOrgWithPartnerRole(req, res, next)
+      expect(stub.calledOnce).to.be.true
+      expect(status.calledOnce).to.be.true
+      expect(status.args[0][0]).to.equal(404)
+      expect(res.json.args[0][0].error).to.equal(error.orgDoesNotExist(stubCnaOrg.short_name).error)
+    })
+  })
+
+  context('Positive Tests', () => {
+    it('Should allow orgs with ADP partner role through by calling next()  ', async () => {
+      const req = {
+        ctx: {
+          org: stubAdpOrg.short_name,
+          uuid: stubAdpOrg.UUID,
+          repositories: {
+            getOrgRepository
+          }
+        }
+      }
+      const stub = sinon.stub(orgRepo, 'findOneByShortName').returns(stubAdpOrg)
+
+      await onlyOrgWithPartnerRole(req, res, next)
+      expect(stub.calledOnce).to.be.true
+      expect(status.calledOnce).to.be.false
+      expect(next.calledOnce).to.be.true
+    })
+    it('Should allow orgs with CNA partner role through by calling next()  ', async () => {
+      const req = {
+        ctx: {
+          org: stubCnaOrg.short_name,
+          uuid: stubCnaOrg.UUID,
+          repositories: {
+            getOrgRepository
+          }
+        }
+      }
+      const stub = sinon.stub(orgRepo, 'findOneByShortName').returns(stubCnaOrg)
+
+      await onlyOrgWithPartnerRole(req, res, next)
+      expect(stub.calledOnce).to.be.true
+      expect(status.calledOnce).to.be.false
+      expect(next.calledOnce).to.be.true
+    })
+    it('Should allow orgs with Secretariat role through by calling next()  ', async () => {
+      const req = {
+        ctx: {
+          org: stubSecretariat.short_name,
+          uuid: stubSecretariat.UUID,
+          repositories: {
+            getOrgRepository
+          }
+        }
+      }
+      const stub = sinon.stub(orgRepo, 'findOneByShortName').returns(stubSecretariat)
+
+      await onlyOrgWithPartnerRole(req, res, next)
+      expect(stub.calledOnce).to.be.true
+      expect(status.calledOnce).to.be.false
+      expect(next.calledOnce).to.be.true
+    })
+  })
+})