Merge pull request #1658 from CVEProject/dev

Feedback updates

Author: david-rocca

api-docs/openapi.json

@@ -5078,7 +5078,7 @@
         }
       }
     },
-    "/review/org/{uuid}": {
+    "/review/{uuid}": {
       "put": {
         "tags": [
           "Review Object"
@@ -5181,7 +5181,7 @@
         }
       }
     },
-    "/review/org/{uuid}/approve": {
+    "/review/{uuid}/approve": {
       "put": {
         "tags": [
           "Review Object"
@@ -5285,7 +5285,7 @@
         }
       }
     },
-    "/review/org/{uuid}/reject": {
+    "/review/{uuid}/reject": {
       "put": {
         "tags": [
           "Review Object"

package-lock.json

@@ -17,10 +17,8 @@ const validateUUID = require('uuid').validate
  */
 async function getOrgs (req, res, next) {
   try {
-    const session = await mongoose.startSession()
     const repo = req.ctx.repositories.getBaseOrgRepository()
     const CONSTANTS = getConstants()
-    let returnValue
 
     // temporary measure to allow tests to work after fixing #920
     // tests required changing the global limit to force pagination
@@ -32,11 +30,7 @@ async function getOrgs (req, res, next) {
     options.sort = { short_name: 'asc' }
     options.page = req.ctx.query.page ? parseInt(req.ctx.query.page) : CONSTANTS.PAGINATOR_PAGE // if 'page' query parameter is not defined, set 'page' to the default page value
 
-    try {
-      returnValue = await repo.getAllOrgs({ ...options, session }, true)
-    } finally {
-      await session.endSession()
-    }
+    const returnValue = await repo.getAllOrgs({ ...options }, true)
 
     logger.info({ uuid: req.ctx.uuid, message: 'The orgs were sent to the user.' })
     return res.status(200).json(returnValue)
@@ -58,7 +52,6 @@ async function getOrgs (req, res, next) {
  */
 async function getOrg (req, res, next) {
   try {
-    const session = await mongoose.startSession()
     const repo = req.ctx.repositories.getBaseOrgRepository()
     const requesterOrgShortName = req.ctx.org
     const identifier = req.ctx.params.identifier
@@ -67,26 +60,21 @@ async function getOrg (req, res, next) {
     let returnValue
 
     try {
-      session.startTransaction()
-      const requesterOrg = await repo.findOneByShortName(requesterOrgShortName, { session }, returnLegacyFormat)
+      const requesterOrg = await repo.findOneByShortName(requesterOrgShortName, {}, returnLegacyFormat)
       const requesterOrgIdentifier = identifierIsUUID ? requesterOrg.UUID : requesterOrgShortName
-      const isSecretariat = await repo.isSecretariat(requesterOrg, { session }, returnLegacyFormat)
+      const isSecretariat = await repo.isSecretariat(requesterOrg, {}, returnLegacyFormat)
 
       if (requesterOrgIdentifier !== identifier && !isSecretariat) {
         logger.info({ uuid: req.ctx.uuid, message: identifier + ' organization can only be viewed by the users of the same organization or the Secretariat.' })
         return res.status(403).json(error.notSameOrgOrSecretariat())
       }
 
-      returnValue = await repo.getOrg(identifier, identifierIsUUID, { session }, returnLegacyFormat)
+      returnValue = await repo.getOrg(identifier, identifierIsUUID, {}, returnLegacyFormat)
     } catch (error) {
-      await session.abortTransaction()
       // Handle the specific error thrown by BaseOrgRepository.createOrg
       if (error.message && error.message.includes('Unknown Org type requested')) {
         return res.status(400).json({ message: error.message })
       }
-      throw error
-    } finally {
-      await session.endSession()
     }
     if (!returnValue) { // an empty result can only happen if the requestor is the Secretariat
       logger.info({ uuid: req.ctx.uuid, message: identifier + ' organization does not exist.' })
@@ -187,13 +175,13 @@ async function getUser (req, res, next) {
       return res.status(404).json(error.userDne(username))
     }
 
-    const rawResult = result
+    const rawResult = result.toObject()
 
     delete rawResult._id
     delete rawResult.__v
     delete rawResult.secret
 
-    logger.info({ uuid: req.ctx.uuid, message: username + ' was sent to the user.', user: result })
+    logger.info({ uuid: req.ctx.uuid, message: username + ' was sent to the user.', user: rawResult })
     return res.status(200).json(rawResult)
   } catch (err) {
     next(err)
@@ -202,7 +190,7 @@ async function getUser (req, res, next) {
 
 /**
  * Get details on ID quota for an org with the specified org shortname.
- * Called by GET /api/registry/org/{shortname}/id_quota, GET /api/org/{shortname}/id_quota
+ * Called by GET /api/registry/org/{shortname}/hard_quota, GET /api/org/{shortname}/id_quota
  *
  * @param {Object} req - The request object
  * @param {Object} res - The response object
@@ -336,7 +324,7 @@ async function updateOrg (req, res, next) {
   const shortNameUrlParameter = req.ctx.params.shortname
   const orgRepository = req.ctx.repositories.getBaseOrgRepository()
 
-  const session = await mongoose.startSession()
+  const session = await mongoose.startSession({ causalConsistency: false })
   let responseMessage
   // Get the query parameters as JSON
   // These are validated by the middleware in org/index.js
@@ -448,7 +436,7 @@ async function createUser (req, res, next) {
           return res.status(400).json({ message: 'Parameters were invalid', errors: result.errors })
         }
       } else {
-        if (!body?.username || typeof body?.username !== 'string' || !body?.username.length > 0) {
+        if (!body?.username || typeof body?.username !== 'string') {
           return res.status(400).json({ message: 'Parameters were invalid', details: [{ param: 'username', msg: 'Parameter must be a non empty string' }] })
         }
       }

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

@@ -20,10 +20,9 @@ const validateUUID = require('uuid').validate
  */
 async function getAllOrgs (req, res, next) {
   try {
-    const session = await mongoose.startSession()
     const repo = req.ctx.repositories.getBaseOrgRepository()
     const conversationRepo = req.ctx.repositories.getConversationRepository()
-    const isSecretariat = await repo.isSecretariatByShortName(req.ctx.org, { session })
+    const isSecretariat = await repo.isSecretariatByShortName(req.ctx.org)
     const CONSTANTS = getConstants()
     let returnValue
 
@@ -38,14 +37,17 @@ async function getAllOrgs (req, res, next) {
     options.page = req.ctx.query.page ? parseInt(req.ctx.query.page) : CONSTANTS.PAGINATOR_PAGE // if 'page' query parameter is not defined, set 'page' to the default page value
 
     try {
-      returnValue = await repo.getAllOrgs({ ...options, session })
+      returnValue = await repo.getAllOrgs({ ...options })
       // fetch conversations
       for (let i = 0; i < returnValue.organizations.length; i++) {
-        const conversation = await conversationRepo.getAllByTargetUUID(returnValue.organizations[i].UUID, isSecretariat, { session })
+        const conversation = await conversationRepo.getAllByTargetUUID(returnValue.organizations[i].UUID, isSecretariat)
         returnValue.organizations[i].conversation = conversation?.length ? conversation : undefined
       }
-    } finally {
-      await session.endSession()
+    } catch (error) {
+      // Handle the specific error thrown by BaseOrgRepository.createOrg
+      if (error.message && error.message.includes('Unknown Org type requested')) {
+        return res.status(400).json({ message: error.message })
+      }
     }
 
     logger.info({ uuid: req.ctx.uuid, message: 'The orgs were sent to the user.' })
@@ -69,7 +71,6 @@ async function getAllOrgs (req, res, next) {
  */
 async function getOrg (req, res, next) {
   try {
-    const session = await mongoose.startSession()
     const repo = req.ctx.repositories.getBaseOrgRepository()
     const conversationRepo = req.ctx.repositories.getConversationRepository()
     // User passed in parameter to filter for
@@ -79,32 +80,28 @@ async function getOrg (req, res, next) {
     let returnValue
 
     try {
-      session.startTransaction()
-      const requesterOrg = await repo.findOneByShortName(requesterOrgShortName, { session })
+      const requesterOrg = await repo.findOneByShortName(requesterOrgShortName)
       const requesterOrgIdentifier = identifierIsUUID ? requesterOrg.UUID : requesterOrgShortName
-      const isSecretariat = await repo.isSecretariat(requesterOrg, { session })
+      const isSecretariat = await repo.isSecretariat(requesterOrg)
 
       if (requesterOrgIdentifier !== identifier && !isSecretariat) {
         logger.info({ uuid: req.ctx.uuid, message: identifier + ' organization can only be viewed by the users of the same organization or the Secretariat.' })
         return res.status(403).json(error.notSameOrgOrSecretariat())
       }
 
-      returnValue = await repo.getOrg(identifier, identifierIsUUID, { session })
+      returnValue = await repo.getOrg(identifier, identifierIsUUID)
 
       if (returnValue) {
         // fetch conversation
-        const conversation = await conversationRepo.getAllByTargetUUID(returnValue.UUID, isSecretariat, { session })
+        const conversation = await conversationRepo.getAllByTargetUUID(returnValue.UUID, isSecretariat)
         returnValue.conversation = conversation?.length ? _.map(conversation, c => _.omit(c, ['__v', '_id', 'UUID', 'previous_conversation_uuid', 'next_conversation_uuid', 'target_uuid', 'visibility'])) : undefined
       }
     } catch (error) {
-      await session.abortTransaction()
       // Handle the specific error thrown by BaseOrgRepository.createOrg
       if (error.message && error.message.includes('Unknown Org type requested')) {
         return res.status(400).json({ message: error.message })
       }
       throw error
-    } finally {
-      await session.endSession()
     }
     if (!returnValue) { // an empty result can only happen if the requestor is the Secretariat
       logger.info({ uuid: req.ctx.uuid, message: identifier + ' organization does not exist.' })
@@ -132,7 +129,7 @@ async function getOrg (req, res, next) {
  */
 async function createOrg (req, res, next) {
   try {
-    const session = await mongoose.startSession()
+    const session = await mongoose.startSession({ causalConsistency: false })
     const repo = req.ctx.repositories.getBaseOrgRepository()
     const body = req.ctx.body
     const isSecretariat = await repo.isSecretariatByShortName(req.ctx.org, { session })
@@ -230,7 +227,7 @@ async function createOrg (req, res, next) {
  */
 async function updateOrg (req, res, next) {
   try {
-    const session = await mongoose.startSession()
+    const session = await mongoose.startSession({ causalConsistency: false })
     const shortName = req.ctx.params.shortname
     const repo = req.ctx.repositories.getBaseOrgRepository()
     const userRepo = req.ctx.repositories.getBaseUserRepository()
@@ -316,15 +313,18 @@ async function updateOrg (req, res, next) {
         }
       }
 
+      // Update Org full will cause a write to the Conversations collection, to avoid a read-after-write issue, we need to get the previous conversation data first
+      const previousConversation = await conversationRepo.getAllByTargetUUID(await repo.getOrgUUID(shortName, { session }), isSecretariat, { session }) || []
+
       updatedOrg = await repo.updateOrgFull(shortName, req.ctx.body, { session }, false, requestingUser.UUID, isAdmin, isSecretariat)
       jointApprovalRequired = _.get(updatedOrg, 'joint_approval_required', false)
       _.unset(updatedOrg, 'joint_approval_required')
-
-      await session.commitTransaction()
-      session.startTransaction()
-      // Checking for existing Conversations
-      const existingConversations = await conversationRepo.getAllByTargetUUID(updatedOrg.UUID, isSecretariat, { session }) || []
-      updatedOrg.conversation = existingConversations.map(c => _.omit(c, ['__v', '_id', 'previous_conversation_uuid', 'next_conversation_uuid']))
+      // append previous conversations to any conversations that are in the org already
+      const currentConversations = Array.isArray(updatedOrg?.conversation) ? updatedOrg.conversation : []
+      const prevConversations = Array.isArray(previousConversation) ? previousConversation : []
+      if (updatedOrg) {
+        updatedOrg.conversation = [...currentConversations, ...prevConversations].map(c => _.omit(c, ['__v', '_id', 'previous_conversation_uuid', 'next_conversation_uuid']))
+      }
 
       await session.commitTransaction()
     } catch (updateErr) {
@@ -385,7 +385,7 @@ async function updateOrg (req, res, next) {
  */
 async function deleteOrg (req, res, next) {
   try {
-    const session = await mongoose.startSession()
+    const session = await mongoose.startSession({ causalConsistency: false })
     const repo = req.ctx.repositories.getBaseOrgRepository()
     const shortName = req.ctx.params.identifier
 
@@ -498,7 +498,7 @@ async function getUsers (req, res, next) {
  *              Called by POST /api/registryOrg/:shortname/user
  */
 async function createUserByOrg (req, res, next) {
-  const session = await mongoose.startSession()
+  const session = await mongoose.startSession({ causalConsistency: false })
   try {
     const body = req.ctx.body
     const userRepo = req.ctx.repositories.getBaseUserRepository()

src/controller/registry-org.controller/registry-org.controller.js

@@ -21,9 +21,7 @@ const _ = require('lodash')
 async function getAllUsers (req, res, next) {
   try {
     const CONSTANTS = getConstants()
-    const session = await mongoose.startSession()
     const repo = req.ctx.repositories.getBaseUserRepository()
-    let returnValue
 
     // temporary measure to allow tests to work after fixing #920
     // tests required changing the global limit to force pagination
@@ -35,36 +33,32 @@ async function getAllUsers (req, res, next) {
     options.sort = { short_name: 'asc' }
     options.page = req.ctx.query.page ? parseInt(req.ctx.query.page) : CONSTANTS.PAGINATOR_PAGE // if 'page' query parameter is not defined, set 'page' to the default page value
 
-    try {
-      returnValue = await repo.getAllUsers(options)
-      // Hydrate roles
-      const orgRepo = req.ctx.repositories.getBaseOrgRepository()
-      const distinctOrgUUIDs = [...new Set(returnValue.users.map(u => u.org_UUID))]
-
-      // Fetch all relevant orgs in one go (or in parallel) if possible, but map is easy for now
-      // Since we don't have a "getManyOrgsByUUID", we might need to do it one by one or improve repository
-      // For now, let's iterate and fetch. It's not optimal but safe given repo limitations.
-      // Optimization: We can build a map of orgUUID -> orgObject
-      const orgMap = {}
-      for (const uuid of distinctOrgUUIDs) {
-        // We need the org content to get admins
-        const org = await orgRepo.findOneByUUID(uuid)
-        if (org) {
-          orgMap[uuid] = org
-        }
+    const returnValue = await repo.getAllUsers(options)
+    // Hydrate roles
+    const orgRepo = req.ctx.repositories.getBaseOrgRepository()
+    const distinctOrgUUIDs = [...new Set(returnValue.users.map(u => u.org_UUID))]
+
+    // Fetch all relevant orgs in one go (or in parallel) if possible, but map is easy for now
+    // Since we don't have a "getManyOrgsByUUID", we might need to do it one by one or improve repository
+    // For now, let's iterate and fetch. It's not optimal but safe given repo limitations.
+    // Optimization: We can build a map of orgUUID -> orgObject
+    const orgMap = {}
+    for (const uuid of distinctOrgUUIDs) {
+      // We need the org content to get admins
+      const org = await orgRepo.findOneByUUID(uuid)
+      if (org) {
+        orgMap[uuid] = org
       }
-
-      returnValue.users.forEach(user => {
-        const org = orgMap[user.org_UUID]
-        if (org && org.admins && org.admins.includes(user.UUID)) {
-          user.role = 'ADMIN'
-        }
-        // If not admin, leave as is (undefined or empty or whatever it was)
-      })
-    } finally {
-      await session.endSession()
     }
 
+    returnValue.users.forEach(user => {
+      const org = orgMap[user.org_UUID]
+      if (org && org.admins && org.admins.includes(user.UUID)) {
+        user.role = 'ADMIN'
+      }
+      // If not admin, leave as is (undefined or empty or whatever it was)
+    })
+
     logger.info({ uuid: req.ctx.uuid, message: 'The user information was sent to the secretariat user.' })
     return res.status(200).json(returnValue)
   } catch (err) {
@@ -143,7 +137,7 @@ async function getUser (req, res, next) {
 }
 
 async function createUser (req, res, next) {
-  const session = await mongoose.startSession()
+  const session = await mongoose.startSession({ causalConsistency: false })
   try {
     const orgRepo = req.ctx.repositories.getBaseOrgRepository()
     const userRepo = req.ctx.repositories.getBaseUserRepository()

src/controller/registry-user.controller/registry-user.controller.js

@@ -324,7 +324,7 @@ router.get('/review/org/:identifier/reviews',
 )
 
 // Update a review object
-router.put('/review/org/:uuid',
+router.put('/review/:uuid',
   /*
   #swagger.tags = ['Review Object']
   #swagger.operationId = 'updateReviewObjectByReviewUUID'
@@ -407,7 +407,7 @@ router.put('/review/org/:uuid',
 )
 
 // Approve a review object
-router.put('/review/org/:uuid/approve',
+router.put('/review/:uuid/approve',
   /*
   #swagger.tags = ['Review Object']
   #swagger.operationId = 'approveReviewObject'
@@ -491,7 +491,7 @@ router.put('/review/org/:uuid/approve',
 )
 
 // Reject a review object
-router.put('/review/org/:uuid/reject',
+router.put('/review/:uuid/reject',
   /*
   #swagger.tags = ['Review Object']
   #swagger.operationId = 'rejectReviewObject'