Check for approved access request

backend/src/connectors/v2/authorisation/base.ts

@@ -5,7 +5,8 @@ import { ReleaseDoc } from '../../../models/v2/Release.js'
 import { SchemaDoc } from '../../../models/v2/Schema.js'
 import { UserDoc } from '../../../models/v2/User.js'
 import { Access, Action } from '../../../routes/v1/registryAuth.js'
-import { getAccessRequestsByModel } from '../../../services/v2/accessRequest.js'
+import { getModelAccessRequestsForUser } from '../../../services/v2/accessRequest.js'
+import { checkAccessRequestsApproved } from '../../../services/v2/review.js'
 import { Roles } from '../authentication/Base.js'
 import authentication from '../authentication/index.js'
 
@@ -68,15 +69,12 @@ export class BasicAuthorisationConnector {
     return true
   }
 
-  async hasAccessRequest(user: UserDoc, model: ModelDoc) {
-    const entities = await authentication.getEntities(user)
-
-    const accessRequests = await getAccessRequestsByModel(user, model.id)
-    const hasAccessRequest = accessRequests.some((request) =>
-      request.metadata.overview.entities.some((entity) => entities.includes(entity)),
-    )
-
-    return hasAccessRequest
+  async hasApprovedAccessRequest(user: UserDoc, model: ModelDoc) {
+    const accessRequests = await getModelAccessRequestsForUser(user, model.id)
+    if (accessRequests.length === 0) {
+      return false
+    }
+    return await checkAccessRequestsApproved(accessRequests.map((accessRequest) => accessRequest.id))
   }
 
   async model(user: UserDoc, model: ModelDoc, action: ModelActionKeys) {
@@ -206,7 +204,7 @@ export class BasicAuthorisationConnector {
     const hasModelRole = (await authentication.getUserModelRoles(user, model)).length !== 0
 
     // Does the user have a valid access request for this model?
-    const hasAccessRequest = await this.hasAccessRequest(user, model)
+    const hasApprovedAccessRequest = await this.hasApprovedAccessRequest(user, model)
 
     return Promise.all(
       files.map(async (file) => {
@@ -216,12 +214,16 @@ export class BasicAuthorisationConnector {
         }
 
         if (
-          !hasAccessRequest &&
+          !hasApprovedAccessRequest &&
           !hasModelRole &&
           [FileAction.Download].includes(action) &&
           !model.settings.ungovernedAccess
         ) {
-          return { success: false, info: 'You need to have a valid access request to download a file.', id: file.id }
+          return {
+            success: false,
+            info: 'You need to have an approved access request to download a file.',
+            id: file.id,
+          }
         }
 
         return { success: true, id: file.id }
@@ -234,7 +236,7 @@ export class BasicAuthorisationConnector {
     const hasModelRole = (await authentication.getUserModelRoles(user, model)).length !== 0
 
     // Does the user have a valid access request for this model?
-    const hasAccessRequest = await this.hasAccessRequest(user, model)
+    const hasAccessRequest = await this.hasApprovedAccessRequest(user, model)
 
     return Promise.all(
       accesses.map(async (access) => {
@@ -262,7 +264,7 @@ export class BasicAuthorisationConnector {
         ) {
           return {
             success: false,
-            info: 'You need to have a valid access request to download an image.',
+            info: 'You need to have an approved access request to download an image.',
             id: access.name,
           }
         }

backend/src/services/v2/accessRequest.ts

@@ -1,5 +1,6 @@
 import { Validator } from 'jsonschema'
 
+import authentication from '../../connectors/v2/authentication/index.js'
 import { AccessRequestAction } from '../../connectors/v2/authorisation/base.js'
 import authorisation from '../../connectors/v2/authorisation/index.js'
 import { AccessRequestInterface } from '../../models/v2/AccessRequest.js'
@@ -118,7 +119,7 @@ export async function updateAccessRequest(
   const accessRequest = await getAccessRequestById(user, accessRequestId)
   const model = await getModelById(user, accessRequest.modelId)
 
-  const auth = await authorisation.accessRequest(user, model, accessRequest, AccessRequestAction.Update)
+  const auth = await authorisation.accessRequest(user, model, accessRequest, AccessRequestAction.View)
   if (!auth.success) {
     throw Forbidden(auth.info, { userDn: user.dn, accessRequestId })
   }
@@ -142,3 +143,12 @@ export async function newAccessRequestComment(user: UserDoc, accessRequestId: st
 
   return accessRequest
 }
+
+export async function getModelAccessRequestsForUser(user: UserDoc, modelId: string) {
+  const accessRequests = await AccessRequest.find({
+    modelId,
+    'metadata.overview.entities': { $in: await authentication.getEntities(user) },
+  })
+
+  return accessRequests
+}

backend/src/services/v2/file.ts

@@ -80,6 +80,9 @@ export async function getFilesByModel(user: UserDoc, modelId: string) {
 
 export async function getFilesByIds(user: UserDoc, modelId: string, fileIds: string[]) {
   const model = await getModelById(user, modelId)
+  if (fileIds.length === 0) {
+    return []
+  }
   const files = await FileModel.find({ _id: { $in: fileIds } })
 
   if (files.length !== fileIds.length) {

backend/src/services/v2/review.ts

@@ -4,7 +4,7 @@ import authorisation from '../../connectors/v2/authorisation/index.js'
 import { AccessRequestDoc } from '../../models/v2/AccessRequest.js'
 import { CollaboratorEntry, ModelDoc, ModelInterface } from '../../models/v2/Model.js'
 import { ReleaseDoc } from '../../models/v2/Release.js'
-import Review, { ReviewDoc, ReviewInterface, ReviewResponse } from '../../models/v2/Review.js'
+import Review, { Decision, ReviewDoc, ReviewInterface, ReviewResponse } from '../../models/v2/Review.js'
 import { UserDoc } from '../../models/v2/User.js'
 import { WebhookEvent } from '../../models/v2/Webhook.js'
 import { ReviewKind, ReviewKindKeys } from '../../types/v2/enums.js'
@@ -22,6 +22,12 @@ import {
 } from './smtp/smtp.js'
 import { sendWebhooks } from './webhook.js'
 
+// This should be replaced by using the dynamic schema
+const requiredRoles = {
+  release: ['mtr', 'msro'],
+  accessRequest: ['msro'],
+}
+
 export async function findReviews(
   user: UserDoc,
   modelId?: string,
@@ -53,7 +59,7 @@ export async function findReviews(
 }
 
 export async function createReleaseReviews(model: ModelDoc, release: ReleaseDoc) {
-  const roleEntities = getRoleEntities(['mtr', 'msro'], model.collaborators)
+  const roleEntities = getRoleEntities(requiredRoles.release, model.collaborators)
 
   const createReviews = roleEntities.map((roleInfo) => {
     const review = new Review({
@@ -73,7 +79,7 @@ export async function createReleaseReviews(model: ModelDoc, release: ReleaseDoc)
 }
 
 export async function createAccessRequestReviews(model: ModelDoc, accessRequest: AccessRequestDoc) {
-  const roleEntities = getRoleEntities(['msro'], model.collaborators)
+  const roleEntities = getRoleEntities(requiredRoles.accessRequest, model.collaborators)
 
   const createReviews = roleEntities.map((roleInfo) => {
     const review = new Review({
@@ -188,6 +194,18 @@ export async function sendReviewResponseNotification(review: ReviewDoc, user: Us
   }
 }
 
+export async function checkAccessRequestsApproved(accessRequestIds: string[]) {
+  const reviews = await Review.find({
+    accessRequestId: accessRequestIds,
+    responses: {
+      $elemMatch: {
+        decision: Decision.Approve,
+      },
+    },
+  })
+  return reviews.some((review) => requiredRoles.accessRequest.includes(review.role))
+}
+
 function getRoleEntities(roles: string[], collaborators: CollaboratorEntry[]) {
   return roles.map((role) => {
     const entities = collaborators

backend/test/connectors/authorisation/base.spec.ts

@@ -0,0 +1,43 @@
+import { describe, expect, test, vi } from 'vitest'
+
+import { BasicAuthorisationConnector } from '../../../src/connectors/v2/authorisation/base.js'
+import { ModelDoc } from '../../../src/models/v2/Model.js'
+import { UserDoc } from '../../../src/models/v2/User.js'
+
+const mockAccessRequestService = vi.hoisted(() => ({
+  getModelAccessRequestsForUser: vi.fn(),
+}))
+vi.mock('../../../src/services/v2/accessRequest.js', () => mockAccessRequestService)
+
+const mockModelService = vi.hoisted(() => ({}))
+vi.mock('../../../src/services/v2/model.js', () => mockModelService)
+
+const mockReviewService = vi.hoisted(() => ({
+  checkAccessRequestsApproved: vi.fn(),
+}))
+vi.mock('../../../src/services/v2/review.js', () => mockReviewService)
+
+describe('connectors > authorisation > base', () => {
+  const user = { dn: 'testUser' } as UserDoc
+  const model = { id: 'testModel' } as ModelDoc
+
+  test('hasApprovedAccessRequest > no access requests for model', async () => {
+    const connector = new BasicAuthorisationConnector()
+    mockAccessRequestService.getModelAccessRequestsForUser.mockReturnValueOnce([])
+
+    const result = await connector.hasApprovedAccessRequest(user, model)
+
+    expect(result).toBe(false)
+  })
+
+  test('hasApprovedAccessRequest > return access request check', async () => {
+    const connector = new BasicAuthorisationConnector()
+    mockAccessRequestService.getModelAccessRequestsForUser.mockReturnValueOnce([{ id: 'accessRequest' }])
+    const approvedAccessRequest = true
+    mockReviewService.checkAccessRequestsApproved.mockReturnValueOnce(approvedAccessRequest)
+
+    const result = await connector.hasApprovedAccessRequest(user, model)
+
+    expect(result).toBe(approvedAccessRequest)
+  })
+})

backend/test/routes/model/accessRequest/deleteAccessRequest.spec.ts

@@ -4,10 +4,10 @@ import audit from '../../../../src/connectors/v2/audit/__mocks__/index.js'
 import { deleteAccessRequestSchema } from '../../../../src/routes/v2/model/accessRequest/deleteAccessRequest.js'
 import { createFixture, testDelete } from '../../../testUtils/routes.js'
 
-vi.mock('../../../../src/utils/config.js')
 vi.mock('../../../../src/utils/user.js')
 vi.mock('../../../../src/utils/v2/config.js')
 vi.mock('../../../../src/connectors/v2/audit/index.js')
+vi.mock('../../../../src/connectors/v2/authorisation/index.js')
 
 describe('routes > accessRequest > deleteAccessRequest', () => {
   test('200 > ok', async () => {

backend/test/routes/model/accessRequest/getAccessRequest.spec.ts

@@ -4,10 +4,10 @@ import audit from '../../../../src/connectors/v2/audit/__mocks__/index.js'
 import { getAccessRequestSchema } from '../../../../src/routes/v2/model/accessRequest/getAccessRequest.js'
 import { createFixture, testGet } from '../../../testUtils/routes.js'
 
-vi.mock('../../../../src/utils/config.js')
 vi.mock('../../../../src/utils/user.js')
 vi.mock('../../../../src/utils/v2/config.js')
 vi.mock('../../../../src/connectors/v2/audit/index.js')
+vi.mock('../../../../src/connectors/v2/authorisation/index.js')
 
 const accessRequestMock = vi.hoisted(() => {
   return {

backend/test/routes/model/accessRequest/getModelAccessRequests.spec.ts

@@ -4,10 +4,10 @@ import audit from '../../../../src/connectors/v2/audit/__mocks__/index.js'
 import { getModelAccessRequestsSchema } from '../../../../src/routes/v2/model/accessRequest/getModelAccessRequests.js'
 import { createFixture, testGet } from '../../../testUtils/routes.js'
 
-vi.mock('../../../../src/utils/config.js')
 vi.mock('../../../../src/utils/v2/config.js')
 vi.mock('../../../../src/utils/user.js')
 vi.mock('../../../../src/connectors/v2/audit/index.js')
+vi.mock('../../../../src/connectors/v2/authorisation/index.js')
 
 const accessRequestMock = vi.hoisted(() => {
   return {

backend/test/routes/model/accessRequest/patchAccessRequest.spec.ts

@@ -7,6 +7,7 @@ import { createFixture, testPatch } from '../../../testUtils/routes.js'
 vi.mock('../../../../src/utils/user.js')
 vi.mock('../../../../src/utils/v2/config.js')
 vi.mock('../../../../src/connectors/v2/audit/index.js')
+vi.mock('../../../../src/connectors/v2/authorisation/index.js')
 
 vi.mock('../../../../src/services/v2/accessRequest.js', async () => {
   return {

backend/test/routes/model/accessRequest/postAccessRequest.spec.ts

@@ -7,6 +7,7 @@ import { createFixture, testPost } from '../../../testUtils/routes.js'
 vi.mock('../../../../src/utils/v2/config.js')
 vi.mock('../../../../src/utils/user.js')
 vi.mock('../../../../src/connectors/v2/audit/index.js')
+vi.mock('../../../../src/connectors/v2/authorisation/index.js')
 
 vi.mock('../../../../src/services/v2/accessRequest.js', async () => ({
   createAccessRequest: vi.fn(() => ({ _id: 'test' })),

backend/test/routes/model/accessRequest/postAccessRequestComment.spec.ts

@@ -4,7 +4,6 @@ import audit from '../../../../src/connectors/v2/audit/__mocks__/index.js'
 import { postAccessRequestCommentSchema } from '../../../../src/routes/v2/model/accessRequest/postAccessRequestComment.js'
 import { createFixture, testPost } from '../../../testUtils/routes.js'
 
-vi.mock('../../../../src/utils/config.js')
 vi.mock('../../../../src/utils/v2/config.js')
 vi.mock('../../../../src/utils/user.js')
 vi.mock('../../../../src/connectors/v2/audit/index.js')

backend/test/services/__snapshots__/accessRequest.spec.ts.snap

@@ -7,3 +7,18 @@ exports[`services > accessRequest > getAccessRequestsByModel > good 1`] = `
   },
 ]
 `;
+
+exports[`services > accessRequest > getModelAccessRequestsForUser > query as expected 1`] = `
+[
+  [
+    {
+      "metadata.overview.entities": {
+        "$in": [
+          "user:testUser",
+        ],
+      },
+      "modelId": "test-model",
+    },
+  ],
+]
+`;

backend/test/services/__snapshots__/review.spec.ts.snap

@@ -1,5 +1,41 @@
 // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
 
+exports[`services > review > checkAccessRequestsApproved > approved access request exists 1`] = `
+[
+  [
+    {
+      "accessRequestId": [
+        "access-1",
+        "access-2",
+      ],
+      "responses": {
+        "$elemMatch": {
+          "decision": "approve",
+        },
+      },
+    },
+  ],
+]
+`;
+
+exports[`services > review > checkAccessRequestsApproved > no approved access requests with a required role 1`] = `
+[
+  [
+    {
+      "accessRequestId": [
+        "access-1",
+        "access-2",
+      ],
+      "responses": {
+        "$elemMatch": {
+          "decision": "approve",
+        },
+      },
+    },
+  ],
+]
+`;
+
 exports[`services > review > findReviews > active 1`] = `
 [
   {},

backend/test/services/accessRequest.spec.ts

@@ -1,9 +1,11 @@
 import { describe, expect, test, vi } from 'vitest'
 
 import authorisation from '../../src/connectors/v2/authorisation/index.js'
+import { UserDoc } from '../../src/models/v2/User.js'
 import {
   createAccessRequest,
   getAccessRequestsByModel,
+  getModelAccessRequestsForUser,
   newAccessRequestComment,
   removeAccessRequest,
 } from '../../src/services/v2/accessRequest.js'
@@ -127,4 +129,11 @@ describe('services > accessRequest', () => {
       /^You do not have permission to delete this access request./,
     )
   })
+
+  test('getModelAccessRequestsForUser > query as expected', async () => {
+    const user = { dn: 'testUser' } as UserDoc
+    await getModelAccessRequestsForUser(user, 'test-model')
+
+    expect(accessRequestModelMocks.find.mock.calls).matchSnapshot()
+  })
 })

backend/test/services/file.spec.ts

@@ -152,6 +152,18 @@ describe('services > file', () => {
     expect(files).toMatchSnapshot()
   })
 
+  test('getFilesByIds > no file ids', async () => {
+    fileModelMocks.find.mockResolvedValueOnce([{ example: 'file' }])
+
+    const user = { dn: 'testUser' } as UserDoc
+    const modelId = 'testModelId'
+    const fileIds = []
+
+    const files = await getFilesByIds(user, modelId, fileIds)
+
+    expect(files).toStrictEqual(fileIds)
+  })
+
   test('getFilesByIds > files not found', async () => {
     fileModelMocks.find.mockResolvedValueOnce([{ example: 'file', _id: { toString: vi.fn(() => 'testFileId') } }])