Merge pull request #1592 from CVEProject/emathew/testing

Separate ParsePut Params in org middleware

Author: david-rocca

api-docs/openapi.json

@@ -3199,13 +3199,6 @@
             },
             "description": "The shortname of the organization"
           },
-          {
-            "name": "registry",
-            "in": "query",
-            "schema": {
-              "type": "string"
-            }
-          },
           {
             "$ref": "#/components/parameters/id_quota"
           },

src/controller/org.controller/index.js

@@ -4,7 +4,7 @@ const mw = require('../../middleware/middleware')
 const errorMsgs = require('../../middleware/errorMessages')
 const controller = require('./org.controller')
 const { body, param, query } = require('express-validator')
-const { parseGetParams, parsePostParams, parseError, isUserRole, isValidUsername, isOrgRole, validateUpdateOrgParameters } = require('./org.middleware')
+const { parseGetParams, parsePostParams, parsePutParams, parseError, isUserRole, isValidUsername, isOrgRole, validateUpdateOrgParameters } = require('./org.middleware')
 // Only God and Javascript know swhy its saying it is not used when it is.....
 // eslint-disable-next-line no-unused-vars
 const { toUpperCaseArray, isFlatStringArray, handleRegistryParameter } = require('../../middleware/middleware')
@@ -568,8 +568,9 @@ router.put('/registry/org/:shortname',
   mw.useRegistry(),
   mw.validateUser,
   mw.onlySecretariat,
+  validateUpdateOrgParameters(),
   parseError,
-  parsePostParams,
+  parsePutParams,
   controller.REGISTRY_UPDATE_ORG
 )
 
@@ -774,7 +775,7 @@ router.put('/registry/org/:shortname/user/:username',
     .customSanitizer(toUpperCaseArray)
     .custom(isUserRole).withMessage(errorMsgs.USER_ROLES),
   parseError,
-  parsePostParams,
+  parsePutParams,
   controller.USER_UPDATE_SINGLE)
 
 router.put('/registry/org/:shortname/user/:username/reset_secret',
@@ -1026,6 +1027,7 @@ router.post(
     .customSanitizer(toUpperCaseArray)
     .custom(isOrgRole),
   body(['policies.id_quota']).optional().not().isArray().isInt({ min: CONSTANTS.MONGOOSE_VALIDATION.Org_policies_id_quota_min, max: CONSTANTS.MONGOOSE_VALIDATION.Org_policies_id_quota_max }).withMessage(errorMsgs.ID_QUOTA),
+  query().custom((query) => { return mw.validateQueryParameterNames(query, ['']) }),
   parseError,
   parsePostParams,
   controller.ORG_CREATE_SINGLE
@@ -1179,7 +1181,7 @@ router.put('/org/:shortname',
   mw.onlySecretariat,
   validateUpdateOrgParameters(),
   parseError,
-  parsePostParams,
+  parsePutParams,
   controller.ORG_UPDATE_SINGLE)
 
 router.get('/org/:shortname/id_quota',
@@ -1607,7 +1609,7 @@ router.put('/org/:shortname/user/:username',
     .customSanitizer(toUpperCaseArray)
     .custom(isUserRole).withMessage(errorMsgs.USER_ROLES),
   parseError,
-  parsePostParams,
+  parsePutParams,
   controller.USER_UPDATE_SINGLE)
 
 router.put('/org/:shortname/user/:username/reset_secret',

src/controller/org.controller/org.controller.js

@@ -190,7 +190,7 @@ async function getOrgIdQuota (req, res, next) {
 
     try {
       session.startTransaction()
-      const requesterOrg = await orgRepo.findOneByShortName(requesterOrgShortName)
+      const requesterOrg = await orgRepo.findOneByShortName(requesterOrgShortName, { session })
       const isSecretariat = await orgRepo.isSecretariat(requesterOrg, !req.useRegistry)
 
       if (requesterOrgShortName !== shortName && !isSecretariat) {
@@ -351,12 +351,14 @@ async function registryUpdateOrg (req, res, next) {
       session.startTransaction()
       if (queryParametersJson['active_roles.add']) {
         if (!Array.isArray(queryParametersJson.active_roles?.add) || queryParametersJson.active_roles?.add.some(item => typeof item !== 'string')) {
+          await session.abortTransaction()
           return res.status(400).json({ message: 'Parameters were invalid', details: [{ param: 'authority', msg: 'Parameter must be a one-dimensional array of strings' }] })
         }
       }
 
       if (queryParametersJson['active_roles.remove']) {
         if (!Array.isArray(queryParametersJson.active_roles?.remove) || queryParametersJson.active_roles?.remove.some(item => typeof item !== 'string')) {
+          await session.abortTransaction()
           return res.status(400).json({ message: 'Parameters were invalid', details: [{ param: 'authority', msg: 'Parameter must be a one-dimensional array of strings' }] })
         }
       }

src/controller/org.controller/org.middleware.js

@@ -161,7 +161,7 @@ function validateCreateOrgParameters () {
 
 function validateUserIdOrUsername () {
   return async (req, res, next) => {
-    const useRegistry = req.query.registry === 'true'
+    const useRegistry = req.useRegistry === 'true'
     const validations = []
     if (useRegistry) {
       validations.push(
@@ -188,18 +188,15 @@ function validateUserIdOrUsername () {
 
 function validateUpdateOrgParameters () {
   return async (req, res, next) => {
-    const useRegistry = req.query.registry === 'true'
-
-    const legacyParametersOnly = ['id_quota', 'name']
-    const registryParametersOnly = ['hard_quota', 'long_name', 'cve_program_org_function', 'oversees', 'root_or_tlr', 'charter_or_scope', 'disclosure_policy', 'product_list', 'cna_role_type', 'cna_country', 'vulnerability_advisory_locations', 'advisory_location_require_credentials', 'industry', 'tl_root_start_date', 'is_cna_discussion_list']
-    const sharedParameters = ['new_short_name', 'active_roles.add', 'active_roles.remove', 'registry']
+    const useRegistry = req.query === 'true'
+    const allowedParams = [...QUERY_PARAMETERS.shared]
+    const registryParametersOnly = [...QUERY_PARAMETERS.registryOnly]
 
-    const allParameters = [
-      ...legacyParametersOnly, ...registryParametersOnly, ...sharedParameters
-    ]
-
-    const validations = [query().custom((query) => { return mw.validateQueryParameterNames(query, allParameters) }),
-      query(allParameters).custom((val) => { return mw.containsNoInvalidCharacters(val) }),
+    const validations = [query().custom((query) => { return mw.validateQueryParameterNames(query, allowedParams) }),
+      query(allowedParams).custom((val) => { return mw.containsNoInvalidCharacters(val) }),
+      query(['id_quota']).optional().not().isArray().isInt({ min: CONSTANTS.MONGOOSE_VALIDATION.Org_policies_id_quota_min, max: CONSTANTS.MONGOOSE_VALIDATION.Org_policies_id_quota_max }).withMessage(errorMsgs.ID_QUOTA),
+      query(['name']).optional().isString().trim().notEmpty(),
+      // Shared parameter validations
       query(['new_short_name']).optional().isString().trim().notEmpty().isLength({ min: CONSTANTS.MIN_SHORTNAME_LENGTH, max: CONSTANTS.MAX_SHORTNAME_LENGTH }),
       query(['active_roles.add']).optional().toArray()
         .custom(isFlatStringArray)
@@ -209,27 +206,17 @@ function validateUpdateOrgParameters () {
         .custom(isFlatStringArray)
         .customSanitizer(toUpperCaseArray)
         .custom(isOrgRole).withMessage(errorMsgs.ORG_ROLES),
+      // Path parameter validation
       param(['shortname']).isString().trim().isLength({ min: CONSTANTS.MIN_SHORTNAME_LENGTH, max: CONSTANTS.MAX_SHORTNAME_LENGTH })]
-
     if (useRegistry) {
       validations.push(
-        query(['hard_quota'])
-          .optional()
-          .not()
-          .isArray()
-          .isInt({
-            min: CONSTANTS.MONGOOSE_VALIDATION.Org_policies_id_quota_min,
-            max: CONSTANTS.MONGOOSE_VALIDATION.Org_policies_id_quota_max
-          })
-          .withMessage(errorMsgs.ID_QUOTA),
-        query(['long_name']).optional().isString().trim().notEmpty(),
         query(['oversees']).optional().isArray(),
         query(['root_or_tlr']).optional().isBoolean(),
         query([
-          'cve_program_org_function',
           'charter_or_scope',
           'disclosure_policy',
           'product_list',
+          'reports_to',
           'contact_info.poc',
           'contact_info.poc_email',
           'contact_info.poc_phone',
@@ -244,17 +231,13 @@ function validateUpdateOrgParameters () {
           'is_cna_discussion_list'
         ])
           .optional()
-          .isString(),
-        ...isNotAllowedQuery(...legacyParametersOnly)
-        // if we decide that we want to allow more, we can add them here.
+          .isString()
+          .trim()
       )
     } else {
       validations.push(
-
-        query(['id_quota']).optional().not().isArray().isInt({ min: CONSTANTS.MONGOOSE_VALIDATION.Org_policies_id_quota_min, max: CONSTANTS.MONGOOSE_VALIDATION.Org_policies_id_quota_max }).withMessage(errorMsgs.ID_QUOTA),
-        query(['name']).optional().isString().trim().notEmpty(),
+        // Block registry-only parameters
         ...isNotAllowedQuery(...registryParametersOnly)
-
       )
     }
 
@@ -304,49 +287,76 @@ function isUserRole (val) {
   return true
 }
 
-function parsePostParams (req, res, next) {
-  utils.reqCtxMapping(req, 'body', [])
-  utils.reqCtxMapping(req, 'query', [
+const QUERY_PARAMETERS = {
+  // Parameters that apply to BOTH systems
+  shared: [
     'new_short_name',
-    'name',
-    'id_quota',
-    'active',
+    'name', // Updates 'name' in legacy, 'long_name' in registry
+    'active_roles',
     'active_roles.add',
     'active_roles.remove',
-    'new_username',
-    'org_short_name',
-    'name.first',
-    'name.last',
-    'name.middle',
-    'name.suffix',
-    'long_name',
-    'cve_program_org_function',
+    'id_quota' // For registry, maps to 'hard_quota' for CNAOrg
+  ],
+  // Registry-only parameters
+  registryOnly: [
+    'root_or_tlr',
     'charter_or_scope',
     'disclosure_policy',
     'product_list',
+    'oversees',
+    'reports_to',
+    'contact_info',
     'contact_info.poc',
     'contact_info.poc_email',
     'contact_info.poc_phone',
     'contact_info.org_email',
-    'hard_quota',
     'contact_info.website',
-    'root_or_tlr',
-    'oversees',
     'cna_role_type',
     'cna_country',
     'vulnerability_advisory_locations',
     'advisory_location_require_credentials',
     'industry',
     'tl_root_start_date',
-    'is_cna_discussion_list'
-  ])
-  utils.reqCtxMapping(req, 'params', ['shortname', 'username'])
+    'is_cna_discussion_list',
+    'hard_quota' // not directly used in query parameters
+  ],
+  // User-related parameters
+  userParams: [
+    'active',
+    'new_username',
+    'org_short_name',
+    'name.first',
+    'name.last',
+    'name.middle',
+    'name.suffix',
+    'active_roles.add', // For user roles
+    'active_roles.remove' // For user roles
+  ]
+}
+
+function parsePutParams (req, res, next) {
+  utils.reqCtxMapping(req, 'body', [])
+  // Extract all possible query parameters
+  const allQueryParams = [
+    ...QUERY_PARAMETERS.shared,
+    ...QUERY_PARAMETERS.registryOnly,
+    ...QUERY_PARAMETERS.userParams
+  ]
+  utils.reqCtxMapping(req, 'query', allQueryParams)
+  utils.reqCtxMapping(req, 'params', ['shortname', 'username', 'identifier'])
+  next()
+}
+
+function parsePostParams (req, res, next) {
+  utils.reqCtxMapping(req, 'body', [])
+  utils.reqCtxMapping(req, 'query', [])
+  utils.reqCtxMapping(req, 'params', ['shortname', 'username', 'identifier'])
   next()
 }
 
 function parseGetParams (req, res, next) {
-  utils.reqCtxMapping(req, 'params', ['shortname', 'username', 'identifier', 'registry'])
-  utils.reqCtxMapping(req, 'query', ['page', 'registry'])
+  utils.reqCtxMapping(req, 'params', ['shortname', 'username', 'identifier'])
+  utils.reqCtxMapping(req, 'query', ['page'])
   next()
 }
 
@@ -369,6 +379,7 @@ function isValidUsername (val) {
 }
 
 module.exports = {
+  parsePutParams,
   parsePostParams,
   parseGetParams,
   parseError,

src/repositories/baseOrgRepository.js

@@ -788,7 +788,7 @@ class BaseOrgRepository extends BaseRepository {
 
   convertLegacyToRegistry (legacyOrg) {
     let newRoles = []
-    if (legacyOrg?.authority?.active_roles.includes('SECRETARIAT')) {
+    if (legacyOrg?.authority?.active_roles?.includes('SECRETARIAT')) {
       newRoles.push('SECRETARIAT')
     } else {
       newRoles = legacyOrg?.authority?.active_roles

src/scripts/populate.js

@@ -20,6 +20,7 @@ const BaseOrg = require('../model/baseorg')
 const BaseUser = require('../model/baseuser')
 const ReviewObject = require('../model/reviewobject')
 const Conversation = require('../model/conversation')
+const Audit = require('../model/audit')
 
 const error = new errors.IDRError()
 
@@ -32,7 +33,8 @@ const populateTheseCollections = {
   BaseOrg: BaseOrg,
   BaseUser: BaseUser,
   ReviewObject: ReviewObject,
-  Conversation: Conversation
+  Conversation: Conversation,
+  Audit: Audit
 }
 
 const indexesToCreate = {

test/integration-tests/audit/registryOrgCreatesAuditTest.js

@@ -109,9 +109,10 @@ describe('Create and Update Audit Collection with Org Endpoints', () => {
 
     // Now update with same values
     const updateResAgain = await chai.request(app)
-      .put(`/api/registry/org/${org.shortName}?long_name=${org.longName}`)
+      .put(`/api/registry/org/${org.shortName}?name=${org.longName}`)
       .set(secretariatHeaders)
     expect(updateResAgain).to.have.status(200)
+    expect(updateResAgain.body.updated.long_name).to.equal(org.longName)
 
     // Check audit history
     const auditRes = await chai.request(app)

test/integration-tests/org/registryOrg.js

@@ -101,13 +101,25 @@ describe('Testing Secretariat functionality for Orgs', () => {
         })
     })
 
-    it('Secretariat can update the ID quota for an org', async () => {
+    it('Secretariat can update the ID quota for a CNA org', async () => {
       await chai.request(app)
-        .put('/api/registry/org/mitre?hard_quota=100000')
+        .post('/api/registry/org')
+        .set(secretariatHeaders)
+        .send({
+          short_name: 'test_registry_org_cna',
+          long_name: 'Testing Registry Org CNA',
+          hard_quota: 123,
+          authority: ['CNA']
+        }).then((res) => {
+          expect(res).to.have.status(200)
+        })
+      await chai.request(app)
+        .put('/api/registry/org/test_registry_org_cna?id_quota=100000')
         .set(secretariatHeaders)
         .then((res) => {
           expect(res).to.have.status(200)
-          expect(res.body.message).to.equal('mitre organization was successfully updated.')
+          expect(res.body.updated.hard_quota).to.equal(100000)
+          expect(res.body.message).to.equal('test_registry_org_cna organization was successfully updated.')
         })
     })
 
@@ -412,7 +424,7 @@ describe('Testing Secretariat functionality for Orgs', () => {
     it('Fails to update an org that does not exist', async () => {
       const nonExistentOrg = 'nonexistent_org'
       await chai.request(app)
-        .put(`/api/registry/org/${nonExistentOrg}?hard_quota=100`)
+        .put(`/api/registry/org/${nonExistentOrg}?id_quota=100`)
         .set(secretariatHeaders)
         .then((res) => {
           expect(res).to.have.status(404)
@@ -433,7 +445,7 @@ describe('Testing Secretariat functionality for Orgs', () => {
           .then((res) => {
             expect(res).to.have.status(400)
             expect(res.body.message).to.equal('Parameters were invalid')
-            expect(res.body.details[0].param).to.equal('authority')
+            expect(res.body.details[0].param).to.equal('active_roles.add')
             expect(res.body.details[0].msg).to.equal('Parameter must be a one-dimensional array of strings')
           })
       })

test/unit-tests/org/orgUpdateTest.js

@@ -165,7 +165,7 @@ describe('Testing the PUT /org/:shortname endpoint in Org Controller', () => {
           }
           req.ctx.repositories = factory
           next()
-        }, orgParams.parsePostParams, orgController.ORG_UPDATE_SINGLE)
+        }, orgParams.parsePutParams, orgController.ORG_UPDATE_SINGLE)
 
       chai.request(app)
         .put(`/org-not-updated-shortname-exists/${orgFixtures.existentOrg.short_name}?new_short_name=cisco`)