diff --git a/server/graphql/modules/manualReviewTool.ts b/server/graphql/modules/manualReviewTool.ts index 1d3a0a4..4d14efa 100644 --- a/server/graphql/modules/manualReviewTool.ts +++ b/server/graphql/modules/manualReviewTool.ts @@ -2460,10 +2460,11 @@ const Mutation: GQLMutationResolvers = { throw forbiddenError('User does not have permission to edit MRT queues'); } - await context.services.ManualReviewToolService.addAccessibleQueuesForUser( - params.input.userId, - params.input.queueIds, - ); + await context.services.ManualReviewToolService.addAccessibleQueuesForUser({ + orgId: user.orgId, + userId: params.input.userId, + queueIds: params.input.queueIds, + }); // TODO: try/catch and return failure cases return gqlSuccessResult( @@ -2481,8 +2482,11 @@ const Mutation: GQLMutationResolvers = { } await context.services.ManualReviewToolService.removeAccessibleQueuesForUser( - params.input.userId, - params.input.queueIds, + { + orgId: user.orgId, + userId: params.input.userId, + queueIds: params.input.queueIds, + }, ); // TODO: try/catch and return failure cases diff --git a/server/services/manualReviewToolService/dbTypes.ts b/server/services/manualReviewToolService/dbTypes.ts index b80aa16..3bccd11 100644 --- a/server/services/manualReviewToolService/dbTypes.ts +++ b/server/services/manualReviewToolService/dbTypes.ts @@ -6,6 +6,7 @@ import { } from '../../storage/dataWarehouse/warehouseDateTypes.js'; import { type JsonOf } from '../../utils/encoding.js'; import { type ConditionSetWithResultAsLogged } from '../analyticsLoggers/ruleExecutionLoggingUtils.js'; +import { type CoreAppTablesPg } from '../coreAppTables.js'; import { type NormalizedItemData } from '../itemProcessingService/toNormalizedItemDataOrErrors.js'; import { type ConditionSet, @@ -74,6 +75,8 @@ export type RoutingRuleExecutionsRow = { ); export type ManualReviewToolServicePg = { + // Shared with CoreAppTablesPg so org-scoping checks can query public.users. + 'public.users': CoreAppTablesPg['public.users']; 'manual_review_tool.manual_review_queues': { id: string; name: string; diff --git a/server/services/manualReviewToolService/manualReviewToolService.ts b/server/services/manualReviewToolService/manualReviewToolService.ts index 0c4dae0..2f09e0a 100644 --- a/server/services/manualReviewToolService/manualReviewToolService.ts +++ b/server/services/manualReviewToolService/manualReviewToolService.ts @@ -838,18 +838,28 @@ export class ManualReviewToolService { }); } - async addAccessibleQueuesForUser( - userId: string, - queueIds: readonly string[], - ) { - return this.queueOps.addAccessibleQueuesForUser([userId], queueIds); + async addAccessibleQueuesForUser(opts: { + orgId: string; + userId: string; + queueIds: readonly string[]; + }) { + return this.queueOps.addAccessibleQueuesForUser({ + orgId: opts.orgId, + userIds: [opts.userId], + queueIds: opts.queueIds, + }); } - async removeAccessibleQueuesForUser( - userId: string, - queueIds: readonly string[], - ) { - return this.queueOps.removeAccessibleQueuesForUser(userId, queueIds); + async removeAccessibleQueuesForUser(opts: { + orgId: string; + userId: string; + queueIds: readonly string[]; + }) { + return this.queueOps.removeAccessibleQueuesForUser({ + orgId: opts.orgId, + userId: opts.userId, + queueIds: opts.queueIds, + }); } /** diff --git a/server/services/manualReviewToolService/modules/JobRouting.test.ts b/server/services/manualReviewToolService/modules/JobRouting.test.ts index 828d97b..30c01d0 100644 --- a/server/services/manualReviewToolService/modules/JobRouting.test.ts +++ b/server/services/manualReviewToolService/modules/JobRouting.test.ts @@ -5,6 +5,7 @@ import { uid } from 'uid'; import getBottle from '../../../iocContainer/index.js'; import createContentItemTypes from '../../../test/fixtureHelpers/createContentItemTypes.js'; import createOrg from '../../../test/fixtureHelpers/createOrg.js'; +import createUser from '../../../test/fixtureHelpers/createUser.js'; import { makeTestWithFixture } from '../../../test/utils.js'; import { toCorrelationId } from '../../../utils/correlationIds.js'; import { jsonStringify } from '../../../utils/encoding.js'; @@ -30,7 +31,11 @@ describe('JobRouting tests', () => { }, uid(), ); - const userId = uid(); + const { user, cleanup: userCleanup } = await createUser( + container.KyselyPg, + org.id, + ); + const userId = user.id; const { itemTypes, cleanup: itemTypesCleanup } = await createContentItemTypes({ moderationConfigService: container.ModerationConfigService, @@ -249,6 +254,7 @@ describe('JobRouting tests', () => { noPolicyQueue.id, ); await itemTypesCleanup(); + await userCleanup(); await orgCleanup(); await container.closeSharedResourcesForShutdown(); }, diff --git a/server/services/manualReviewToolService/modules/QueueOperations.test.ts b/server/services/manualReviewToolService/modules/QueueOperations.test.ts index f3d2713..24dd887 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.test.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.test.ts @@ -7,6 +7,7 @@ import createContentItemTypes from '../../../test/fixtureHelpers/createContentIt import createMrtQueue from '../../../test/fixtureHelpers/createMrtQueue.js'; import createOrg from '../../../test/fixtureHelpers/createOrg.js'; import createUser from '../../../test/fixtureHelpers/createUser.js'; +import { makeTransactionalTestWithFixture } from '../../../test/harness/transactionalTest.js'; import { makeTestWithFixture } from '../../../test/utils.js'; import { UserPermission } from '../../userManagementService/index.js'; import { @@ -231,4 +232,179 @@ describe('QueueOperations', () => { ).resolves.toBeUndefined(); }, ); + + const testWithTwoOrgs = () => + makeTransactionalTestWithFixture(async ({ deps }) => { + const buildOrg = async () => { + const { org } = await createOrg( + { + KyselyPg: deps.KyselyPg, + ModerationConfigService: deps.ModerationConfigService, + ApiKeyService: deps.ApiKeyService, + }, + uid(), + ); + const { user } = await createUser(deps.KyselyPg, org.id); + const { queue } = await createMrtQueue({ + orgId: org.id, + mrtService: deps.ManualReviewToolService, + userId: user.id, + }); + return { org, user, queue }; + }; + + return { + attacker: await buildOrg(), + victim: await buildOrg(), + mrtService: deps.ManualReviewToolService, + }; + }); + + testWithTwoOrgs()( + 'addAccessibleQueuesForUser must not grant access to a queue in a different org', + async ({ attacker, victim, mrtService }) => { + await expect( + mrtService.addAccessibleQueuesForUser({ + orgId: attacker.org.id, + userId: attacker.user.id, + queueIds: [victim.queue.id], + }), + ).rejects.toBeDefined(); + + const viewers = await mrtService.getUsersWhoCanSeeQueue({ + orgId: victim.org.id, + queueId: victim.queue.id, + userId: attacker.user.id, + }); + expect(viewers.map((v) => v.userId)).not.toContain(attacker.user.id); + }, + ); + + testWithTwoOrgs()( + 'addAccessibleQueuesForUser must not grant access for a user in a different org', + async ({ attacker, victim, mrtService }) => { + await expect( + mrtService.addAccessibleQueuesForUser({ + orgId: attacker.org.id, + userId: victim.user.id, + queueIds: [attacker.queue.id], + }), + ).rejects.toBeDefined(); + + const viewers = await mrtService.getUsersWhoCanSeeQueue({ + orgId: attacker.org.id, + queueId: attacker.queue.id, + userId: victim.user.id, + }); + expect(viewers.map((v) => v.userId)).not.toContain(victim.user.id); + }, + ); + + testWithTwoOrgs()( + 'removeAccessibleQueuesForUser must not revoke access for a queue in a different org', + async ({ attacker, victim, mrtService }) => { + await mrtService.addAccessibleQueuesForUser({ + orgId: victim.org.id, + userId: victim.user.id, + queueIds: [victim.queue.id], + }); + + await expect( + mrtService.removeAccessibleQueuesForUser({ + orgId: attacker.org.id, + userId: attacker.user.id, + queueIds: [victim.queue.id], + }), + ).rejects.toBeDefined(); + + const viewers = await mrtService.getUsersWhoCanSeeQueue({ + orgId: victim.org.id, + queueId: victim.queue.id, + userId: victim.user.id, + }); + expect(viewers.map((v) => v.userId)).toContain(victim.user.id); + }, + ); + + testWithTwoOrgs()( + 'removeAccessibleQueuesForUser must not revoke access for a user in a different org', + async ({ attacker, victim, mrtService }) => { + await expect( + mrtService.removeAccessibleQueuesForUser({ + orgId: attacker.org.id, + userId: victim.user.id, + queueIds: [attacker.queue.id], + }), + ).rejects.toBeDefined(); + + const viewers = await mrtService.getUsersWhoCanSeeQueue({ + orgId: attacker.org.id, + queueId: attacker.queue.id, + userId: attacker.user.id, + }); + expect(viewers.map((v) => v.userId)).toContain(attacker.user.id); + }, + ); + + testWithTwoOrgs()( + 'addAccessibleQueuesForUser grants access within the same org', + async ({ attacker, mrtService }) => { + await expect( + mrtService.addAccessibleQueuesForUser({ + orgId: attacker.org.id, + userId: attacker.user.id, + queueIds: [attacker.queue.id], + }), + ).resolves.toBeDefined(); + + const viewers = await mrtService.getUsersWhoCanSeeQueue({ + orgId: attacker.org.id, + queueId: attacker.queue.id, + userId: attacker.user.id, + }); + expect(viewers.map((v) => v.userId)).toContain(attacker.user.id); + }, + ); + + testWithTwoOrgs()( + 'createManualReviewQueue must not grant access to a user in a different org', + async ({ attacker, victim, mrtService }) => { + await expect( + mrtService.createManualReviewQueue({ + name: 'attacker-queue', + description: null, + userIds: [victim.user.id], + hiddenActionIds: [], + isAppealsQueue: false, + invokedBy: { + userId: attacker.user.id, + permissions: [UserPermission.EDIT_MRT_QUEUES], + orgId: attacker.org.id, + }, + }), + ).rejects.toMatchObject({ name: 'AccessibleQueueNotInOrgError' }); + }, + ); + + testWithTwoOrgs()( + 'updateManualReviewQueue must not grant access to a user in a different org', + async ({ attacker, victim, mrtService }) => { + await expect( + mrtService.updateManualReviewQueue({ + orgId: attacker.org.id, + queueId: attacker.queue.id, + userIds: [attacker.user.id, victim.user.id], + actionIdsToHide: [], + actionIdsToUnhide: [], + }), + ).rejects.toMatchObject({ name: 'AccessibleQueueNotInOrgError' }); + + const viewers = await mrtService.getUsersWhoCanSeeQueue({ + orgId: attacker.org.id, + queueId: attacker.queue.id, + userId: attacker.user.id, + }); + expect(viewers.map((v) => v.userId)).not.toContain(victim.user.id); + }, + ); }); diff --git a/server/services/manualReviewToolService/modules/QueueOperations.ts b/server/services/manualReviewToolService/modules/QueueOperations.ts index 77b8ab4..b92c654 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.ts @@ -104,7 +104,8 @@ export type ManualReviewQueueErrorType = 'ManualReviewQueueNameExistsError'; export type QueueOperationsErrorType = | 'DeleteAllJobsUnauthorizedError' | 'QueueDoesNotExistError' - | 'UnableToDeleteDefaultQueueError'; + | 'UnableToDeleteDefaultQueueError' + | 'AccessibleQueueNotInOrgError'; // Compound identifier for a queue. orgId is needed for security, but also // because queues are/will be actually sharded across redis instances for @@ -266,6 +267,11 @@ export default class QueueOperations { ); } + // Defense-in-depth org-scoping: every user granted access to the new + // queue must belong to the caller's org. The queue itself is always + // created in the caller's org (orgId from the invoker). + await assertUsersInOrg(this.pgQuery, { orgId, userIds }); + try { return await this.transactionWithRetry(async (transaction) => { // In newer versions of kysely, this is greatly simplified with @@ -358,6 +364,16 @@ export default class QueueOperations { clearReportsTriggerActionIds, } = input; + // Defense-in-depth org-scoping: the target queue and every user granted + // access must belong to the caller's org. Reject before any write so a + // cross-org queueId or userId can't mutate users_and_accessible_queues + // rows belonging to another org. + await assertUsersAndQueuesInOrg(this.pgQuery, { + orgId, + userIds, + queueIds: [queueId], + }); + return this.transactionWithRetry(async (transaction) => { const [updatedQueue, _, __] = await Promise.all([ transaction @@ -655,10 +671,18 @@ export default class QueueOperations { .execute(); } - async addAccessibleQueuesForUser( - userIds: string[], - queueIds: readonly string[], - ) { + async addAccessibleQueuesForUser(opts: { + orgId: string; + userIds: readonly string[]; + queueIds: readonly string[]; + }) { + const { orgId, userIds, queueIds } = opts; + await assertUsersAndQueuesInOrg(this.pgQuery, { + orgId, + userIds, + queueIds, + }); + return this.pgQuery .insertInto('manual_review_tool.users_and_accessible_queues') .values( @@ -670,10 +694,18 @@ export default class QueueOperations { .execute(); } - async removeAccessibleQueuesForUser( - userId: string, - queueIds: readonly string[], - ) { + async removeAccessibleQueuesForUser(opts: { + orgId: string; + userId: string; + queueIds: readonly string[]; + }) { + const { orgId, userId, queueIds } = opts; + await assertUsersAndQueuesInOrg(this.pgQuery, { + orgId, + userIds: [userId], + queueIds, + }); + return this.pgQuery .deleteFrom('manual_review_tool.users_and_accessible_queues') .where('user_id', '=', userId) @@ -1883,6 +1915,80 @@ const makeQueueDoesNotExistError = (data: ErrorInstanceData) => { }); }; +/** + * Thrown when a target queue or user does not belong to the caller's org. + */ +const makeAccessibleQueueNotInOrgError = (data: ErrorInstanceData) => + new CoopError({ + status: 403, + type: [ErrorType.Unauthorized], + title: "Queue or user does not belong to the caller's organization", + name: 'AccessibleQueueNotInOrgError', + ...data, + }); + +/** + * Rejects before any write if a target queue does not belong to the caller's + * org. Defense-in-depth org-scoping for accessible-queue mutations. + */ +async function assertQueuesInOrg( + db: Kysely, + opts: { orgId: string; queueIds: readonly string[] }, +) { + const { orgId, queueIds } = opts; + const inOrgQueues = await db + .selectFrom('manual_review_tool.manual_review_queues') + .select('id') + .where('org_id', '=', orgId) + .where('id', 'in', queueIds) + .execute(); + if (inOrgQueues.length !== new Set(queueIds).size) { + throw makeAccessibleQueueNotInOrgError({ shouldErrorSpan: true }); + } +} + +/** + * Rejects before any write if a target user does not belong to the caller's + * org. Defense-in-depth org-scoping for accessible-queue mutations. + */ +async function assertUsersInOrg( + db: Kysely, + opts: { orgId: string; userIds: readonly string[] }, +) { + const { orgId, userIds } = opts; + const inOrgUsers = await db + .selectFrom('public.users') + .select('id') + .where('org_id', '=', orgId) + .where('id', 'in', userIds) + .execute(); + if (inOrgUsers.length !== new Set(userIds).size) { + throw makeAccessibleQueueNotInOrgError({ shouldErrorSpan: true }); + } +} + +/** + * Rejects before any write if a target queue or user does not belong to the + * caller's org. + */ +async function assertUsersAndQueuesInOrg( + db: Kysely, + opts: { + orgId: string; + userIds: readonly string[]; + queueIds: readonly string[]; + }, +) { + await assertQueuesInOrg(db, { + orgId: opts.orgId, + queueIds: opts.queueIds, + }); + await assertUsersInOrg(db, { + orgId: opts.orgId, + userIds: opts.userIds, + }); +} + export const makeUnableToDeleteDefaultQueueError = ( data: ErrorInstanceData, ) => {