Repository: CVEProject/cve-services
Pull request #571 (2022-03-10 11:03:26)

Issue #566
### Identify the Bug

Link to issue: #566 

### Description of the Change

Re-ordered `mw.onlyCnas` to ensure it comes after `mw.validateUser`. Also added in error handling into the `onlyCnas` function to capture the `null` return and return a 404 and the following error, this is mainly to handle the error gracefully in the event of  a race condition:

```json
{
    "error": "CNA_DOES_NOT_EXIST",
    "message": "The 'church_2222222' organization designated by the shortname parameter does not exist."
}
```

### Alternate Designs

You could just switch the order of `mw.onlyCnas` and `mw.validateUser` however, it is better to handle the race condition as well just so the API errors gracefully.

### Possible Drawbacks

None that I can see.

### Verification Process

Tested using the test case in #566, also created a test to cover the null value which is 🟢 

### Release Notes

Fixed a bug in some API endpoints where a non-existent CNA would cause a 500 internal server error



Patch:
diff --git a/src/controller/cve-id.controller/index.js b/src/controller/cve-id.controller/index.js
index 1cd9fc4f..ff7cc5ed 100644
--- a/src/controller/cve-id.controller/index.js
+++ b/src/controller/cve-id.controller/index.js
@@ -37,8 +37,8 @@ router.get('/cve-id/:id',
   parseGetParams,
   controller.CVEID_GET_SINGLE)
 router.put('/cve-id/:id',
-  mw.onlyCnas,
   mw.validateUser,
+  mw.onlyCnas,
   param(['id']).isString().matches(/^CVE-[0-9]{4}-[0-9]{4,}$/, 'i'),
   query(['state']).optional().isString().trim().escape().customSanitizer(val => { return val.toUpperCase() }).isIn(CHOICES),
   query(['org']).optional().isString().trim().escape(),
diff --git a/src/controller/cve.controller/index.js b/src/controller/cve.controller/index.js
index cc077966..52dbae0f 100644
--- a/src/controller/cve.controller/index.js
+++ b/src/controller/cve.controller/index.js
@@ -56,8 +56,8 @@ router.put('/cve/:id',
   controller.CVE_UPDATE_SINGLE)
 
 router.post('/cve/:id/cna',
-  mw.onlyCnas,
   mw.validateUser,
+  mw.onlyCnas,
   validateCveCnaContainerJsonSchema,
   param(['id']).isString().matches(/^CVE-[0-9]{4}-[0-9]{4,}$/i),
   parseError,
@@ -66,8 +66,8 @@ router.post('/cve/:id/cna',
   controller.CVE_SUBMIT_CNA)
 
 router.put('/cve/:id/cna',
-  mw.onlyCnas,
   mw.validateUser,
+  mw.onlyCnas,
   validateCveCnaContainerJsonSchema,
   param(['id']).isString().matches(/^CVE-[0-9]{4}-[0-9]{4,}$/i),
   parseError,
@@ -76,8 +76,8 @@ router.put('/cve/:id/cna',
   controller.CVE_UPDATE_CNA)
 
 router.post('/cve/:id/reject',
-  mw.onlyCnas,
   mw.validateUser,
+  mw.onlyCnas,
   validateRejectBody,
   param(['id']).isString().matches(/^CVE-[0-9]{4}-[0-9]{4,}$/i),
   body(['cnaContainer.rejectedReasons']).isArray().custom((arr) => {
@@ -93,8 +93,8 @@ router.post('/cve/:id/reject',
   controller.CVE_REJECT_RECORD)
 
 router.put('/cve/:id/reject',
-  mw.onlyCnas,
   mw.validateUser,
+  mw.onlyCnas,
   validateRejectBody,
   param(['id']).isString().matches(/^CVE-[0-9]{4}-[0-9]{4,}$/i),
   body(['cnaContainer.rejectedReasons']).isArray().custom((arr) => {
diff --git a/src/middleware/error.js b/src/middleware/error.js
index fd712380..6648e11b 100644
--- a/src/middleware/error.js
+++ b/src/middleware/error.js
@@ -28,6 +28,13 @@ class MiddlewareError extends idrErr.IDRError {
     return err
   }
 
+  cnaDoesNotExist (shortname) { // mw
+    const err = {}
+    err.error = 'CNA_DOES_NOT_EXIST'
+    err.message = `The '${shortname}' organization designated by the shortname parameter does not exist.`
+    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 81484b67..82646c7d 100644
--- a/src/middleware/middleware.js
+++ b/src/middleware/middleware.js
@@ -129,7 +129,10 @@ async function onlyCnas (req, res, next) {
 
   try {
     const org = await orgRepo.findOneByShortName(shortName) // org exists
-    if (org.authority.active_roles.includes(CONSTANTS.AUTH_ROLE_ENUM.SECRETARIAT)) {
+    if (org === null) {
+      logger.info({ uuid: req.ctx.uuid, message: shortName + ' is NOT a ' + CONSTANTS.AUTH_ROLE_ENUM.CNA })
+      return res.status(404).json(error.cnaDoesNotExist(shortName))
+    } else if (org.authority.active_roles.includes(CONSTANTS.AUTH_ROLE_ENUM.SECRETARIAT)) {
       logger.info({ uuid: req.ctx.uuid, message: org.short_name + ' is a ' + CONSTANTS.AUTH_ROLE_ENUM.SECRETARIAT + ' so until Root organizations are implemented this role is allowed.' })
       next()
     } else if (org.authority.active_roles.includes(CONSTANTS.AUTH_ROLE_ENUM.CNA)) { // the org is a CNA


Test patch:
diff --git a/test/unit-tests/middleware/onlyCnasMiddlewareTest.js b/test/unit-tests/middleware/onlyCnasMiddlewareTest.js
index 1a15273c..8a4542f0 100644
--- a/test/unit-tests/middleware/onlyCnasMiddlewareTest.js
+++ b/test/unit-tests/middleware/onlyCnasMiddlewareTest.js
@@ -160,5 +160,41 @@ describe('Test only CNA middleware', () => {
           done()
         })
     })
+
+    it('Requester organization shortname is not valid', function (done) {
+      class OrgOnlyCnasOrgNull {
+        async findOneByShortName () {
+          return null
+        }
+      }
+
+      app.route('/only-cnas-org-equals-null')
+        .post((req, res, next) => {
+          const factory = {
+            getOrgRepository: () => { return new OrgOnlyCnasOrgNull() }
+          }
+          req.ctx.repositories = factory
+          next()
+        }, middleware.onlyCnas, (req, res) => {
+          return res.status(200).json({ message: 'Success! You have reached the target endpoint.' })
+        })
+
+      chai.request(app)
+        .post('/only-cnas-org-equals-null')
+        .set(mwCnaFixtures.notCnaHeaders)
+        .send()
+        .end((err, res) => {
+          if (err) {
+            done(err)
+          }
+
+          expect(res).to.have.status(404)
+          expect(res).to.have.property('body').and.to.be.a('object')
+          const errObj = error.cnaDoesNotExist(mwCnaFixtures.notCnaHeaders['CVE-API-ORG'])
+          expect(res.body.error).to.equal(errObj.error)
+          expect(res.body.message).to.equal(errObj.message)
+          done()
+        })
+    })
   })
 })