[COLLAB-23]: Verify Tiles permissions on role change or pipe transfer (#1283)

kevinkim-ogp · kevinkim-ogp · commit df565c7f9300 · 2025-11-19T16:38:11.000+08:00
## TL;DR

This PR adds permission checks to ensure users have appropriate access to Tiles when:

1. Creating a flow transfer - verifies the current owner has editor rights to all Tiles in the Pipe
2. Approving a flow transfer - checks the old owner has editor permissions before adding the new owner as a collaborator
3. Adding flow collaborators - ensures the user adding collaborators has editor access to the Tile
4. Prevents downgrading Tile permissions when a Pipe collaborator is downgraded from editor to viewer

## Tests

- [x] Should not be able to create a pipe transfer if the current owner is not an editor of a Tile that is used in a step of the Pipe (could happen if the Tile action was created, then the Tile collaborator role was removed or changed to Viewer)
- [x] Should not be able to accept a pipe transfer if the old owner is not an editor of a Tile that is used in a step of the Pipe (could happen if the transfer was created, then the Tile collaborator role was removed or changed to Viewer)
- [x] Should not be able to add a new collaborator if the user is not an editor of a Tile
- [x] Tile collaborator role should not be downgraded if the Pipe role is changed from editor to viewer
diff --git a/packages/backend/src/graphql/__tests__/mutations/update-flow-transfer-status.itest.ts b/packages/backend/src/graphql/__tests__/mutations/update-flow-transfer-status.itest.ts
@@ -1,6 +1,6 @@
 import { randomUUID } from 'crypto'
 import { AES } from 'crypto-js'
-import { beforeEach, describe, expect, it } from 'vitest'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
 
 import appConfig from '@/config/app'
 import updateFlowTransferStatus from '@/graphql/mutations/update-flow-transfer-status'
@@ -10,10 +10,11 @@ import ExecutionStep from '@/models/execution-step'
 import Flow from '@/models/flow'
 import FlowTransfer from '@/models/flow-transfers'
 import Step from '@/models/step'
+import TableCollaborator from '@/models/table-collaborators'
 import User from '@/models/user'
 import Context from '@/types/express/context'
 
-import { generateMockContext } from './tiles/table.mock'
+import { generateMockContext, generateMockTable } from './tiles/table.mock'
 import { generateMockFlow, generateMockUser } from './flow.mock'
 
 describe('updateFlowTransferStatus', () => {
@@ -286,5 +287,108 @@ describe('updateFlowTransferStatus', () => {
       const slackStep = await Step.query().findById(slackStepId)
       expect(slackStep.status).toBe('completed')
     })
+
+    it('verifies old owner has editor access to table before adding new owner as collaborator', async () => {
+      const { table } = await generateMockTable({
+        userId: owner.id,
+        databaseType: 'pg',
+      })
+
+      // Create a tiles step that references the table
+      const tilesStepId = randomUUID()
+      await Step.query().insert({
+        id: tilesStepId,
+        flowId: mockFlow.id,
+        key: 'createTileRow',
+        appKey: 'tiles',
+        type: 'action',
+        position: 2,
+        parameters: { rowData: [], tableId: table.id },
+        status: 'completed',
+      })
+
+      // Spy on hasAccess to verify it's called with correct parameters
+      const hasAccessSpy = vi
+        .spyOn(TableCollaborator, 'hasAccess')
+        .mockResolvedValue(undefined)
+
+      const addCollaboratorSpy = vi
+        .spyOn(TableCollaborator, 'addCollaborator')
+        .mockResolvedValue(undefined)
+
+      // Approve the transfer as new owner
+      context.currentUser = newOwner
+      const result = await updateFlowTransferStatus(
+        null,
+        { input: { id: transfer.id, status: 'approved' } },
+        context,
+      )
+
+      expect(result.status).toBe('approved')
+
+      // Verify hasAccess was called with old owner's ID and 'editor' role
+      expect(hasAccessSpy).toHaveBeenCalledWith(
+        owner.id, // oldOwnerId
+        table.id, // tableId
+        'editor', // required role
+      )
+
+      // Verify addCollaborator was called to add new owner as editor
+      expect(addCollaboratorSpy).toHaveBeenCalledWith({
+        userId: newOwner.id,
+        tableId: table.id,
+        role: 'editor',
+        trx: expect.anything(),
+      })
+
+      hasAccessSpy.mockRestore()
+      addCollaboratorSpy.mockRestore()
+    })
+
+    it('throws error when old owner does not have editor access to table', async () => {
+      const { table } = await generateMockTable({
+        userId: owner.id,
+        databaseType: 'pg',
+      })
+
+      // Create a tiles step that references the table
+      const tilesStepId = randomUUID()
+      await Step.query().insert({
+        id: tilesStepId,
+        flowId: mockFlow.id,
+        key: 'createTileRow',
+        appKey: 'tiles',
+        type: 'action',
+        position: 2,
+        parameters: { rowData: [], tableId: table.id },
+        status: 'completed',
+      })
+
+      // Mock hasAccess to throw ForbiddenError (old owner doesn't have access)
+      const { ForbiddenError } = await import('@/errors/graphql-errors')
+      const hasAccessSpy = vi
+        .spyOn(TableCollaborator, 'hasAccess')
+        .mockRejectedValue(
+          new ForbiddenError(
+            'You do not have sufficient permissions for this tile',
+          ),
+        )
+
+      // Approve the transfer as new owner
+      context.currentUser = newOwner
+
+      // Should throw an error indicating old owner lacks permissions
+      await expect(
+        updateFlowTransferStatus(
+          null,
+          { input: { id: transfer.id, status: 'approved' } },
+          context,
+        ),
+      ).rejects.toThrow(
+        'Previous owner does not have sufficient permissions to add you as an Editor of the Tile.',
+      )
+
+      hasAccessSpy.mockRestore()
+    })
   })
 })
diff --git a/packages/backend/src/graphql/__tests__/mutations/update-step.itest.ts b/packages/backend/src/graphql/__tests__/mutations/update-step.itest.ts
@@ -640,10 +640,12 @@ describe('updateStep mutation', () => {
     })
 
     it('should call add to flow_connections and add table collaborator for tiles app with tableId parameter', async () => {
-      const mockTableId = 'table-123'
+      const mockTableId = randomUUID()
       const { default: FlowConnections } = await import(
         '@/models/flow-connections'
       )
+      // mock the check that the user has access to the tile
+      vi.spyOn(TableCollaborator, 'hasAccess').mockResolvedValue(undefined)
       const addCollaboratorSpy = vi
         .spyOn(TableCollaborator, 'addCollaborator')
         .mockResolvedValue(undefined)
diff --git a/packages/backend/src/graphql/__tests__/mutations/upsert-flow-collaborator.itest.ts b/packages/backend/src/graphql/__tests__/mutations/upsert-flow-collaborator.itest.ts
@@ -166,6 +166,12 @@ describe('upsert flow collaborator', () => {
         db: 'pg',
       })
 
+      await TableCollaborator.query().insert({
+        tableId: tilesTableId,
+        userId: context.currentUser.id,
+        role: 'owner',
+      })
+
       await Step.query().insert([
         {
           id: randomUUID(),
@@ -647,6 +653,90 @@ describe('upsert flow collaborator', () => {
       })
       expect(tableCollaborators).toHaveLength(1)
     })
+
+    it('should not allow adding collaborators if they are not an owner or editor of the tile', async () => {
+      // add a Tile step
+      await Step.query().insert({
+        id: randomUUID(),
+        flowId: dummyFlow.id,
+        key: 'createTileRow',
+        appKey: 'tiles',
+        type: 'action',
+        parameters: { tableId: tilesTableId1 },
+        position: 1,
+      })
+
+      // add the editor as a collaborator of the Pipe first
+      await FlowCollaborator.query().insert({
+        flowId: dummyFlow.id,
+        userId: editor.id,
+        role: 'editor',
+        updatedBy: context.currentUser.id,
+      })
+
+      // editor should not be able to add the viewer as the editor is not a collaborator of the tile
+      context.currentUser = editor
+      await expect(
+        upsertFlowCollaborator(
+          null,
+          {
+            input: {
+              flowId: dummyFlow.id,
+              email: viewer.email,
+              role: 'viewer',
+            },
+          },
+          context,
+        ),
+      ).rejects.toThrow('You do not have sufficient permissions for this tile')
+    })
+
+    it('should not downgrade the role on Tiles when the Pipe collaborator is being downgraded from an Editor to a Viewer', async () => {
+      // add a Tile step
+      await Step.query().insert({
+        id: randomUUID(),
+        flowId: dummyFlow.id,
+        key: 'createTileRow',
+        appKey: 'tiles',
+        type: 'action',
+        parameters: { tableId: tilesTableId1 },
+        position: 1,
+      })
+
+      // add the editor as a collaborator of the Pipe first
+      await upsertFlowCollaborator(
+        null,
+        {
+          input: { flowId: dummyFlow.id, email: editor.email, role: 'editor' },
+        },
+        context,
+      )
+
+      // Check that only one table collaborator was added (duplicates should be handled)
+      const tableCollaborators = await TableCollaborator.query().where({
+        user_id: editor.id,
+      })
+      expect(tableCollaborators).toHaveLength(1)
+      expect(tableCollaborators[0].tableId).toBe(tilesTableId1)
+      expect(tableCollaborators[0].role).toBe('editor')
+
+      // now we downgrade the Pipe collaborator from an Editor to a Viewer
+      await upsertFlowCollaborator(
+        null,
+        {
+          input: { flowId: dummyFlow.id, email: editor.email, role: 'viewer' },
+        },
+        context,
+      )
+
+      // Check that the table collaborator role was not downgraded
+      const tableCollaborators2 = await TableCollaborator.query().where({
+        user_id: editor.id,
+      })
+      expect(tableCollaborators2).toHaveLength(1)
+      expect(tableCollaborators2[0].tableId).toBe(tilesTableId1)
+      expect(tableCollaborators2[0].role).toBe('editor')
+    })
   })
 
   it('should not allow adding collaborators if collaborators flag is false', async () => {
diff --git a/packages/backend/src/graphql/mutations/create-flow-transfer.ts b/packages/backend/src/graphql/mutations/create-flow-transfer.ts
@@ -1,4 +1,5 @@
 import FlowTransfer from '@/models/flow-transfers'
+import TableCollaborator from '@/models/table-collaborators'
 import User from '@/models/user'
 
 import type { MutationResolvers } from '../__generated__/types.generated'
@@ -11,8 +12,9 @@ const createFlowTransfer: MutationResolvers['createFlowTransfer'] = async (
   const { flowId, newOwnerEmail } = params.input
 
   // check if flow belongs to the old owner first
-  await context.currentUser
+  const flow = await context.currentUser
     .$relatedQuery('flows')
+    .withGraphFetched('steps')
     .where('id', flowId)
     .throwIfNotFound('This pipe does not belong to you.')
 
@@ -49,6 +51,24 @@ const createFlowTransfer: MutationResolvers['createFlowTransfer'] = async (
     )
   }
 
+  // COLLABORATORS:
+  // check that the current owner has the rights to make the new owner
+  // an editor of the Tile (if any)
+  const tilesSteps =
+    flow?.[0].steps?.filter((step) => step.appKey === 'tiles') ?? []
+
+  // check that the current owner has the rights to make the new owner
+  // an editor of all the Tiles in the Pipe
+  await Promise.all(
+    tilesSteps.map(async (step) => {
+      await TableCollaborator.hasAccess(
+        context.currentUser.id,
+        step.parameters.tableId as string,
+        'editor',
+      )
+    }),
+  )
+
   const newTransfer: FlowTransfer = await context.currentUser
     .$relatedQuery('sentFlowTransfers')
     .insert({
diff --git a/packages/backend/src/graphql/mutations/update-flow-transfer-status.ts b/packages/backend/src/graphql/mutations/update-flow-transfer-status.ts
@@ -1,4 +1,4 @@
-import { BadUserInputError } from '@/errors/graphql-errors'
+import { BadUserInputError, ForbiddenError } from '@/errors/graphql-errors'
 import { getConnectionDetails } from '@/helpers/get-shared-connection-details'
 import logger from '@/helpers/logger'
 import Connection from '@/models/connection'
@@ -201,6 +201,13 @@ const updateFlowTransferStatus: MutationResolvers['updateFlowTransferStatus'] =
         const tableInserts = await Promise.all(
           sharedTables.map(async (tableId) => {
             try {
+              // the old owner should have the permissions to add the new owner as an editor of the table
+              await TableCollaborator.hasAccess(
+                flowTransfer.oldOwnerId,
+                tableId,
+                'editor',
+              )
+
               // there may be instances where the new pipe owner is already the owner of the table
               // we catch the error but do nothing`
               await TableCollaborator.addCollaborator({
@@ -210,6 +217,11 @@ const updateFlowTransferStatus: MutationResolvers['updateFlowTransferStatus'] =
                 trx,
               })
             } catch (e) {
+              if (e instanceof ForbiddenError) {
+                throw new Error(
+                  'Previous owner does not have sufficient permissions to add you as an Editor of the Tile.',
+                )
+              }
               // do nothing if the new owner of the pipe is already the owner of the tile
               if (
                 !(e instanceof BadUserInputError) &&
diff --git a/packages/backend/src/helpers/add-flow-connection.ts b/packages/backend/src/helpers/add-flow-connection.ts
@@ -64,6 +64,11 @@ async function addFlowTableConnection({
   addedBy,
   trx,
 }: AddTableFlowConnectionParams): Promise<void> {
+  // COLLABORATORS:
+  // check that the user is already an Owner / Editor of the Tile first
+  // otherwise they should not be allowed to add a new collaborator
+  await TableCollaborator.hasAccess(addedBy, tableId, 'editor')
+
   // first try to add the connection to the flow_connections table
   const addedConnection = await FlowConnections.addFlowConnection({
     flowId,
diff --git a/packages/backend/src/models/table-collaborators.ts b/packages/backend/src/models/table-collaborators.ts
@@ -112,6 +112,17 @@ class TableCollaborator extends Base {
       if (existingCollaborator.role === 'owner') {
         throw new BadUserInputError('Cannot change owner role')
       }
+
+      /**
+       * COLLABORATORS:
+       * should not downgrade the role on Tiles when the Pipe collaborator is being
+       * downgraded from an Editor to a Viewer.
+       * Tile Owner / Editor should do that manually.
+       */
+      if (role === 'viewer' && existingCollaborator.role === 'editor') {
+        return
+      }
+
       await existingCollaborator
         .$query(trx)
         .patchAndFetch({
diff --git a/packages/frontend/src/components/EditorSettings/FlowShare/AddNewCollaborator.tsx b/packages/frontend/src/components/EditorSettings/FlowShare/AddNewCollaborator.tsx
@@ -72,12 +72,17 @@ const AddNewCollaborator = ({
   )
 
   const onConfirm = useCallback(async () => {
-    setIsAdding(true)
-    const email = getValues('email')
-    await onAdd(email, role)
-    resetField('email')
-    onClose()
-    setIsAdding(false)
+    try {
+      setIsAdding(true)
+      const email = getValues('email')
+      await onAdd(email, role)
+      resetField('email')
+    } catch (error) {
+      console.error(error)
+    } finally {
+      onClose()
+      setIsAdding(false)
+    }
   }, [onAdd, getValues, role, onClose, resetField])
 
   return (
