From a70ec696fee5b23886ebb1e8c06c99bf4e092787 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 27 Oct 2026 15:14:33 +0100 Subject: [PATCH 1/5] fix(agent): check the segment query permission wherever a live query segment is used Each route that builds its filter from a live query segment checks that the user may run that segment, on the collection whose filter carries it, whether the segment comes from the query string or the request body: listing, counting and exporting records, deleting them, associating and dissociating related records, running an action, its form hooks and its approval, and charts. Users allowed to run a segment keep every feature on it. Refs: PRD-1759 Co-Authored-By: Claude Opus 5.5 --- packages/agent/src/routes/access/chart.ts | 5 + .../agent/src/routes/access/count-related.ts | 4 + packages/agent/src/routes/access/count.ts | 4 + .../agent/src/routes/access/csv-related.ts | 4 + packages/agent/src/routes/access/csv.ts | 4 + .../agent/src/routes/access/list-related.ts | 4 + packages/agent/src/routes/access/list.ts | 4 + .../src/routes/modification/action/action.ts | 5 + .../routes/modification/associate-related.ts | 5 + .../agent/src/routes/modification/delete.ts | 5 + .../modification/dissociate-delete-related.ts | 5 + .../services/authorization/authorization.ts | 47 +++-- .../authorization/authorization.ts | 1 + .../agent/test/routes/access/chart.test.ts | 169 +++++++++++++++ .../test/routes/access/count-related.test.ts | 21 ++ .../agent/test/routes/access/count.test.ts | 21 ++ .../test/routes/access/csv-related.test.ts | 24 +++ packages/agent/test/routes/access/csv.test.ts | 19 ++ .../test/routes/access/list-related.test.ts | 25 +++ .../agent/test/routes/access/list.test.ts | 89 ++++++++ .../routes/modification/action/action.test.ts | 163 +++++++++++++++ .../modification/associate-related.test.ts | 30 +++ .../test/routes/modification/delete.test.ts | 118 +++++++++++ .../dissociate-delete-related.test.ts | 31 +++ .../authorization/authorization.test.ts | 196 +++++++++++------- 25 files changed, 900 insertions(+), 103 deletions(-) diff --git a/packages/agent/src/routes/access/chart.ts b/packages/agent/src/routes/access/chart.ts index 5dc6746874..e326126bf1 100644 --- a/packages/agent/src/routes/access/chart.ts +++ b/packages/agent/src/routes/access/chart.ts @@ -319,6 +319,11 @@ export default class ChartRoute extends CollectionRoute { } private async getFilter(context: Context): Promise { + await this.services.authorization.assertCanExecuteLiveQuerySegment( + context, + this.collection.name, + ); + const scope = await this.services.authorization.getScope(this.collection, context); return ContextFilterFactory.build(this.collection, context, scope); diff --git a/packages/agent/src/routes/access/count-related.ts b/packages/agent/src/routes/access/count-related.ts index 3d0a45d80d..2887c4a6ee 100644 --- a/packages/agent/src/routes/access/count-related.ts +++ b/packages/agent/src/routes/access/count-related.ts @@ -18,6 +18,10 @@ export default class CountRelatedRoute extends RelationRoute { public async handleCountRelated(context: Context): Promise { await this.services.authorization.assertCanBrowse(context, this.foreignCollection.name); + await this.services.authorization.assertCanExecuteLiveQuerySegment( + context, + this.foreignCollection.name, + ); if (this.foreignCollection.schema.countable) { await this.services.authorization.assertCanReadQueryFields(context, this.foreignCollection, [ diff --git a/packages/agent/src/routes/access/count.ts b/packages/agent/src/routes/access/count.ts index a6802e043e..153f07121e 100644 --- a/packages/agent/src/routes/access/count.ts +++ b/packages/agent/src/routes/access/count.ts @@ -14,6 +14,10 @@ export default class CountRoute extends CollectionRoute { public async handleCount(context: Context): Promise { await this.services.authorization.assertCanBrowse(context, this.collection.name); + await this.services.authorization.assertCanExecuteLiveQuerySegment( + context, + this.collection.name, + ); if (this.collection.schema.countable) { await this.services.authorization.assertCanReadQueryFields(context, this.collection, [ diff --git a/packages/agent/src/routes/access/csv-related.ts b/packages/agent/src/routes/access/csv-related.ts index ef5325cfb4..56e7c25d00 100644 --- a/packages/agent/src/routes/access/csv-related.ts +++ b/packages/agent/src/routes/access/csv-related.ts @@ -22,6 +22,10 @@ export default class CsvRelatedRoute extends RelationRoute { async handleRelatedCsv(context: Context): Promise { await this.services.authorization.assertCanBrowse(context, this.foreignCollection.name); + await this.services.authorization.assertCanExecuteLiveQuerySegment( + context, + this.foreignCollection.name, + ); await this.services.authorization.assertCanExport(context, this.foreignCollection.name); await this.services.authorization.assertCanReadQueryFields(context, this.foreignCollection); diff --git a/packages/agent/src/routes/access/csv.ts b/packages/agent/src/routes/access/csv.ts index 02eb795465..7683faa922 100644 --- a/packages/agent/src/routes/access/csv.ts +++ b/packages/agent/src/routes/access/csv.ts @@ -16,6 +16,10 @@ export default class CsvRoute extends CollectionRoute { async handleCsv(context: Context): Promise { await this.services.authorization.assertCanBrowse(context, this.collection.name); + await this.services.authorization.assertCanExecuteLiveQuerySegment( + context, + this.collection.name, + ); await this.services.authorization.assertCanExport(context, this.collection.name); await this.services.authorization.assertCanReadQueryFields(context, this.collection); diff --git a/packages/agent/src/routes/access/list-related.ts b/packages/agent/src/routes/access/list-related.ts index f12a17533a..edf83a7d3f 100644 --- a/packages/agent/src/routes/access/list-related.ts +++ b/packages/agent/src/routes/access/list-related.ts @@ -18,6 +18,10 @@ export default class ListRelatedRoute extends RelationRoute { public async handleListRelated(context: Context): Promise { await this.services.authorization.assertCanBrowse(context, this.foreignCollection.name); + await this.services.authorization.assertCanExecuteLiveQuerySegment( + context, + this.foreignCollection.name, + ); await this.services.authorization.assertCanReadQueryFields(context, this.foreignCollection); const parentId = IdUtils.unpackId(this.collection.schema, context.params.parentId); diff --git a/packages/agent/src/routes/access/list.ts b/packages/agent/src/routes/access/list.ts index 081cd6bb19..566dcf5bf9 100644 --- a/packages/agent/src/routes/access/list.ts +++ b/packages/agent/src/routes/access/list.ts @@ -12,6 +12,10 @@ export default class ListRoute extends CollectionRoute { public async handleList(context: Context) { await this.services.authorization.assertCanBrowse(context, this.collection.name); + await this.services.authorization.assertCanExecuteLiveQuerySegment( + context, + this.collection.name, + ); await this.services.authorization.assertCanReadQueryFields(context, this.collection); const scope = await this.services.authorization.getScope(this.collection, context); diff --git a/packages/agent/src/routes/modification/action/action.ts b/packages/agent/src/routes/modification/action/action.ts index c8795ad58b..ed5be82a3a 100644 --- a/packages/agent/src/routes/modification/action/action.ts +++ b/packages/agent/src/routes/modification/action/action.ts @@ -385,6 +385,11 @@ export default class ActionRoute extends CollectionRoute { const attributes = body?.data?.attributes; // Match user filter + search + scope? + segment. + await this.services.authorization.assertCanExecuteLiveQuerySegment( + context, + this.collection.name, + ); + const scope = includeUserScope ? await this.services.authorization.getScope(this.collection, context) : null; diff --git a/packages/agent/src/routes/modification/associate-related.ts b/packages/agent/src/routes/modification/associate-related.ts index 96967434c8..2e007f4a9f 100644 --- a/packages/agent/src/routes/modification/associate-related.ts +++ b/packages/agent/src/routes/modification/associate-related.ts @@ -59,6 +59,11 @@ export default class AssociateRelatedRoute extends RelationRoute { targetedRelationId: CompositeId, context: Context, ) { + await this.services.authorization.assertCanExecuteLiveQuerySegment( + context, + this.foreignCollection.name, + ); + const [id] = SchemaUtils.getPrimaryKeys(this.foreignCollection.schema); let value = await CollectionUtils.getValue( this.foreignCollection, diff --git a/packages/agent/src/routes/modification/delete.ts b/packages/agent/src/routes/modification/delete.ts index d0293fcb4d..f7df1bf1b8 100644 --- a/packages/agent/src/routes/modification/delete.ts +++ b/packages/agent/src/routes/modification/delete.ts @@ -36,6 +36,11 @@ export default class DeleteRoute extends CollectionRoute { } private async deleteRecords(context: Context, selectionIds: SelectionIds): Promise { + await this.services.authorization.assertCanExecuteLiveQuerySegment( + context, + this.collection.name, + ); + let selectedIds = ConditionTreeFactory.matchIds(this.collection.schema, selectionIds.ids); if (selectionIds.areExcluded) selectedIds = selectedIds.inverse(); diff --git a/packages/agent/src/routes/modification/dissociate-delete-related.ts b/packages/agent/src/routes/modification/dissociate-delete-related.ts index 49c6930419..16125dcfd5 100644 --- a/packages/agent/src/routes/modification/dissociate-delete-related.ts +++ b/packages/agent/src/routes/modification/dissociate-delete-related.ts @@ -105,6 +105,11 @@ export default class DissociateDeleteRelatedRoute extends RelationRoute { * The filter that is generated by this condition it _not_ restricted by the parent record */ private async getBaseForeignFilter(context: Context): Promise { + await this.services.authorization.assertCanExecuteLiveQuerySegment( + context, + this.foreignCollection.name, + ); + const selectionIds = BodyParser.parseSelectionIds(this.foreignCollection.schema, context); let selectedIds = ConditionTreeFactory.matchIds( this.foreignCollection.schema, diff --git a/packages/agent/src/services/authorization/authorization.ts b/packages/agent/src/services/authorization/authorization.ts index f14bfad821..e420dd5f91 100644 --- a/packages/agent/src/services/authorization/authorization.ts +++ b/packages/agent/src/services/authorization/authorization.ts @@ -187,6 +187,29 @@ export default class AuthorizationService { } } + public async assertCanExecuteLiveQuerySegment( + context: Context, + collectionName: string, + ): Promise { + const liveQuerySegment = context.request + ? QueryStringParser.parseLiveQuerySegment(context) + : null; + + if (!liveQuerySegment) return; + + const { id: userId, renderingId } = context.state.user; + const canExecuteSegmentQuery = + await this.forestAdminClient.permissionService.canExecuteSegmentQuery({ + userId, + collectionName, + renderingId, + segmentQuery: liveQuerySegment.query, + connectionName: liveQuerySegment.connectionName, + }); + + if (!canExecuteSegmentQuery) context.throw(HttpCode.Forbidden, 'Forbidden'); + } + /** The root is skipped: `browse` gates a listing, `read` a get, and the signed hash a chart. */ private async getReadPermissions( context: Context, @@ -207,7 +230,7 @@ export default class AuthorizationService { context: Context, collectionName: string, ) { - const { id: userId, renderingId } = context.state.user; + const { id: userId } = context.state.user; const canOnCollection = await this.forestAdminClient.permissionService.canOnCollection({ userId, @@ -215,28 +238,6 @@ export default class AuthorizationService { collectionName, }); - if ( - context.request?.query && - CollectionActionEvent.Browse === event && - (context.request.query as { segmentQuery?: string }).segmentQuery - ) { - const { segmentQuery, connectionName } = context.request.query as { - segmentQuery?: string; - connectionName?: string; - }; - - const canExecuteSegmentQuery = - await this.forestAdminClient.permissionService.canExecuteSegmentQuery({ - userId, - collectionName, - renderingId, - segmentQuery, - connectionName, - }); - - if (!canExecuteSegmentQuery) context.throw(HttpCode.Forbidden, 'Forbidden'); - } - if (!canOnCollection) { context.throw(HttpCode.Forbidden, 'Forbidden'); } diff --git a/packages/agent/test/__factories__/authorization/authorization.ts b/packages/agent/test/__factories__/authorization/authorization.ts index 321ae86312..4740e30192 100644 --- a/packages/agent/test/__factories__/authorization/authorization.ts +++ b/packages/agent/test/__factories__/authorization/authorization.ts @@ -14,6 +14,7 @@ export class AuthorizationsFactory extends Factory { Authorizations.assertCanExport = jest.fn(); Authorizations.getScope = jest.fn(); Authorizations.assertCanExecuteChart = jest.fn(); + Authorizations.assertCanExecuteLiveQuerySegment = jest.fn(); Authorizations.invalidateScopeCache = jest.fn(); Authorizations.canRead = jest.fn().mockResolvedValue(true); Authorizations.assertCanReadQueryFields = jest.fn(); diff --git a/packages/agent/test/routes/access/chart.test.ts b/packages/agent/test/routes/access/chart.test.ts index 3715ceaad7..6f7c9c9a74 100644 --- a/packages/agent/test/routes/access/chart.test.ts +++ b/packages/agent/test/routes/access/chart.test.ts @@ -6,6 +6,7 @@ import { import { createMockContext } from '@shopify/jest-koa-mocks'; import Chart from '../../../src/routes/access/chart'; +import AuthorizationService from '../../../src/services/authorization/authorization'; import * as factories from '../../__factories__'; describe('ChartRoute', () => { @@ -1128,4 +1129,172 @@ describe('ChartRoute', () => { expect(router.post).toHaveBeenCalledWith('/stats/books\\+\\?\\*', expect.any(Function)); }); }); + + describe('with a live query segment and the real authorization service', () => { + const throwingThrow = () => + jest.fn((status: number, message: string) => { + throw Object.assign(new Error(message), { status }); + }); + + const setup = (canExecuteSegmentQuery: boolean) => { + const forestAdminClient = factories.forestAdminClient.build(); + (forestAdminClient.permissionService.canExecuteChart as jest.Mock).mockResolvedValue(true); + (forestAdminClient.permissionService.canExecuteSegmentQuery as jest.Mock).mockResolvedValue( + canExecuteSegmentQuery, + ); + const services = factories.forestAdminHttpDriverServices.build({ + authorization: new AuthorizationService(forestAdminClient), + }); + (services.chartHandler.getChartWithContextInjected as jest.Mock).mockImplementation( + ({ chartRequest }) => chartRequest, + ); + const books = factories.collection.build({ + name: 'books', + schema: factories.collectionSchema.build({ + fields: { id: factories.columnSchema.uuidPrimaryKey().build() }, + }), + aggregate: jest.fn().mockResolvedValue([{ value: 1234, group: null }]), + }); + const dataSource = factories.dataSource.buildWithCollection(books); + const chart = new Chart( + services, + factories.forestAdminHttpDriverOptions.build(), + dataSource, + 'books', + ); + const context = createMockContext({ + throw: throwingThrow(), + requestBody: { type: 'Value', aggregator: 'Count', sourceCollectionName: 'books' }, + customProperties: { + query: { + timezone: 'Europe/Paris', + segmentQuery: 'SELECT id FROM books', + connectionName: 'main', + }, + }, + state: { user: { email: 'john.doe@domain.com', id: 100, renderingId: 7 } }, + }); + + return { forestAdminClient, books, chart, context }; + }; + + const expectedSegmentCheck = { + userId: 100, + collectionName: 'books', + renderingId: 7, + segmentQuery: 'SELECT id FROM books', + connectionName: 'main', + }; + + test('refuses a chart on a segment the user is not allowed to run', async () => { + const { forestAdminClient, books, chart, context } = setup(false); + + await expect(chart.handleChart(context)).rejects.toMatchObject({ status: 403 }); + + expect(forestAdminClient.permissionService.canExecuteSegmentQuery).toHaveBeenCalledWith( + expectedSegmentCheck, + ); + expect(books.aggregate).not.toHaveBeenCalled(); + }); + + test('checks a count leaderboard segment against the charted collection only', async () => { + const forestAdminClient = factories.forestAdminClient.build(); + (forestAdminClient.permissionService.canExecuteChart as jest.Mock).mockResolvedValue(true); + (forestAdminClient.permissionService.canOnCollection as jest.Mock).mockResolvedValue(true); + (forestAdminClient.permissionService.canExecuteSegmentQuery as jest.Mock).mockImplementation( + async ({ collectionName }) => collectionName === 'persons', + ); + const services = factories.forestAdminHttpDriverServices.build({ + authorization: new AuthorizationService(forestAdminClient), + }); + (services.chartHandler.getChartWithContextInjected as jest.Mock).mockImplementation( + ({ chartRequest }) => chartRequest, + ); + const persons = factories.collection.build({ + name: 'persons', + schema: factories.collectionSchema.build({ + fields: { + id: factories.columnSchema.uuidPrimaryKey().build(), + books: factories.oneToManySchema.build({ + foreignCollection: 'books', + originKey: 'authorId', + originKeyTarget: 'id', + }), + }, + }), + }); + const books = factories.collection.build({ + name: 'books', + schema: factories.collectionSchema.build({ + fields: { + id: factories.columnSchema.uuidPrimaryKey().build(), + authorId: factories.columnSchema.build({ columnType: 'Uuid' }), + author: factories.manyToOneSchema.build({ + foreignCollection: 'persons', + foreignKey: 'authorId', + foreignKeyTarget: 'id', + }), + }, + }), + aggregate: jest.fn().mockResolvedValue([{ value: 3, group: { 'author:id': 'a1' } }]), + }); + const dataSource = factories.dataSource.buildWithCollections([persons, books]); + const chart = new Chart( + services, + factories.forestAdminHttpDriverOptions.build(), + dataSource, + 'persons', + ); + const context = createMockContext({ + throw: throwingThrow(), + requestBody: { + type: 'Leaderboard', + aggregator: 'Count', + sourceCollectionName: 'persons', + labelFieldName: 'id', + relationshipFieldName: 'books', + limit: 5, + }, + customProperties: { + query: { + timezone: 'Europe/Paris', + segmentQuery: 'SELECT id FROM persons', + connectionName: 'main', + }, + }, + state: { user: { email: 'john.doe@domain.com', id: 100, renderingId: 7 } }, + }); + + await chart.handleChart(context); + + expect(forestAdminClient.permissionService.canExecuteSegmentQuery).toHaveBeenCalledTimes(1); + expect(forestAdminClient.permissionService.canExecuteSegmentQuery).toHaveBeenCalledWith({ + userId: 100, + collectionName: 'persons', + renderingId: 7, + segmentQuery: 'SELECT id FROM persons', + connectionName: 'main', + }); + expect(context.response.body).toMatchObject({ + data: { attributes: { value: [{ key: 'a1', value: 3 }] } }, + }); + }); + + test('computes a chart on a segment the user is allowed to run', async () => { + const { forestAdminClient, books, chart, context } = setup(true); + + await chart.handleChart(context); + + expect(forestAdminClient.permissionService.canExecuteSegmentQuery).toHaveBeenCalledWith( + expectedSegmentCheck, + ); + expect(books.aggregate).toHaveBeenCalledWith( + expect.objectContaining({ id: 100 }), + expect.objectContaining({ + liveQuerySegment: { query: 'SELECT id FROM books', connectionName: 'main' }, + }), + { field: undefined, groups: undefined, operation: 'Count' }, + ); + }); + }); }); diff --git a/packages/agent/test/routes/access/count-related.test.ts b/packages/agent/test/routes/access/count-related.test.ts index 6ef8ad53f2..827ecf970a 100644 --- a/packages/agent/test/routes/access/count-related.test.ts +++ b/packages/agent/test/routes/access/count-related.test.ts @@ -76,6 +76,27 @@ describe('CountRelatedRoute', () => { ); }); + test('refuses the count of the related records when their live query segment is not allowed', async () => { + const { services, dataSource, options } = setupWithOneToManyRelation(); + (services.authorization.assertCanExecuteLiveQuerySegment as jest.Mock).mockRejectedValueOnce( + new Error('Forbidden'), + ); + const aggregateRelation = jest + .spyOn(CollectionUtils, 'aggregateRelation') + .mockResolvedValue([{ value: 1, group: {} }]); + const route = new CountRelatedRoute(services, options, dataSource, 'books', 'myBookPersons'); + const context = setupContext(); + + await expect(route.handleCountRelated(context)).rejects.toThrow('Forbidden'); + + expect(services.authorization.assertCanExecuteLiveQuerySegment).toHaveBeenCalledWith( + context, + 'bookPersons', + ); + expect(aggregateRelation).not.toHaveBeenCalled(); + aggregateRelation.mockRestore(); + }); + describe('handleCountRelated', () => { describe('when the request is correct', () => { test('should aggregate the relation and return the result', async () => { diff --git a/packages/agent/test/routes/access/count.test.ts b/packages/agent/test/routes/access/count.test.ts index c0f4e379ed..f52e9f1a59 100644 --- a/packages/agent/test/routes/access/count.test.ts +++ b/packages/agent/test/routes/access/count.test.ts @@ -51,6 +51,27 @@ describe('CountRoute', () => { expect(context.response.body).toEqual({ count: 2 }); }); + test('refuses the count before aggregating when the live query segment is not allowed', async () => { + const aggregateSpy = jest.fn().mockReturnValue([{ value: 2 }]); + dataSource.getCollection('books').aggregate = aggregateSpy; + ( + services.authorization.assertCanExecuteLiveQuerySegment as jest.Mock + ).mockRejectedValueOnce(new Error('Forbidden')); + const count = new Count(services, options, dataSource, 'books'); + const context = createMockContext({ + customProperties: { query: { timezone: 'Europe/Paris' } }, + state: { user: { email: 'john.doe@domain.com' } }, + }); + + await expect(count.handleCount(context)).rejects.toThrow('Forbidden'); + + expect(services.authorization.assertCanExecuteLiveQuerySegment).toHaveBeenCalledWith( + context, + 'books', + ); + expect(aggregateSpy).not.toHaveBeenCalled(); + }); + test('should ignore the Forest-Projection header', async () => { const aggregateSpy = jest.fn().mockReturnValue([{ value: 2 }]); dataSource.getCollection('books').aggregate = aggregateSpy; diff --git a/packages/agent/test/routes/access/csv-related.test.ts b/packages/agent/test/routes/access/csv-related.test.ts index c25df6d350..bc596719fc 100644 --- a/packages/agent/test/routes/access/csv-related.test.ts +++ b/packages/agent/test/routes/access/csv-related.test.ts @@ -60,6 +60,30 @@ describe('CsvRelatedRoute', () => { ); }); + test('refuses the export of the related records when their live query segment is not allowed', async () => { + const { options, services, dataSource } = setupWithOneToManyRelation(); + (services.authorization.assertCanExecuteLiveQuerySegment as jest.Mock).mockRejectedValueOnce( + new Error('Forbidden'), + ); + const listRelation = jest.spyOn(CollectionUtils, 'listRelation').mockResolvedValue([]); + const csvRoute = new CsvRoute(services, options, dataSource, 'books', 'myPersons'); + const context = createMockContext({ + customProperties: { + params: { parentId: '2d162303-78bf-599e-b197-93590ac3d315' }, + query: { 'fields[persons]': 'name', timezone: 'Europe/Paris' }, + }, + }); + + await expect(csvRoute.handleRelatedCsv(context)).rejects.toThrow('Forbidden'); + + expect(services.authorization.assertCanExecuteLiveQuerySegment).toHaveBeenCalledWith( + context, + 'persons', + ); + expect(listRelation).not.toHaveBeenCalled(); + listRelation.mockRestore(); + }); + describe('handleRelatedCsv', () => { it('should set the response headers correctly', async () => { const { options, services, dataSource } = setupWithOneToManyRelation(); diff --git a/packages/agent/test/routes/access/csv.test.ts b/packages/agent/test/routes/access/csv.test.ts index 0fc4463d30..4b611fc827 100644 --- a/packages/agent/test/routes/access/csv.test.ts +++ b/packages/agent/test/routes/access/csv.test.ts @@ -38,6 +38,25 @@ describe('CsvRoute', () => { describe('handleCsv', () => { const limitExportSize = 5000; + it('refuses the export before listing when the live query segment is not allowed', async () => { + const { options, services, dataSource } = setup(); + (services.authorization.assertCanExecuteLiveQuerySegment as jest.Mock).mockRejectedValueOnce( + new Error('Forbidden'), + ); + const csvRoute = new CsvRoute(services, options, dataSource, 'books'); + const context = createMockContext({ + customProperties: { query: { 'fields[books]': 'name', timezone: 'Europe/Paris' } }, + }); + + await expect(csvRoute.handleCsv(context)).rejects.toThrow('Forbidden'); + + expect(services.authorization.assertCanExecuteLiveQuerySegment).toHaveBeenCalledWith( + context, + 'books', + ); + expect(dataSource.getCollection('books').list).not.toHaveBeenCalled(); + }); + it('should set the response headers correctly', async () => { const { options, services, dataSource } = setup(); const csvRoute = new CsvRoute(services, options, dataSource, 'books'); diff --git a/packages/agent/test/routes/access/list-related.test.ts b/packages/agent/test/routes/access/list-related.test.ts index 3b95976421..acb14b5080 100644 --- a/packages/agent/test/routes/access/list-related.test.ts +++ b/packages/agent/test/routes/access/list-related.test.ts @@ -60,6 +60,31 @@ describe('ListRelatedRoute', () => { ); }); + test('refuses the listing of the related records when their live query segment is not allowed', async () => { + const { services, dataSource, options } = setupWithOneToManyRelation(); + (services.authorization.assertCanExecuteLiveQuerySegment as jest.Mock).mockRejectedValueOnce( + new Error('Forbidden'), + ); + const listRelation = jest.spyOn(CollectionUtils, 'listRelation').mockResolvedValue([]); + const route = new ListRelatedRoute(services, options, dataSource, 'books', 'myPersons'); + const context = createMockContext({ + state: { user: { email: 'john.doe@domain.com' } }, + customProperties: { + params: { parentId: '2d162303-78bf-599e-b197-93590ac3d315' }, + query: { timezone: 'Europe/Paris' }, + }, + }); + + await expect(route.handleListRelated(context)).rejects.toThrow('Forbidden'); + + expect(services.authorization.assertCanExecuteLiveQuerySegment).toHaveBeenCalledWith( + context, + 'persons', + ); + expect(listRelation).not.toHaveBeenCalled(); + listRelation.mockRestore(); + }); + describe('handleListRelated', () => { describe('when the request is correct', () => { test('should return the record result', async () => { diff --git a/packages/agent/test/routes/access/list.test.ts b/packages/agent/test/routes/access/list.test.ts index 047a41709b..acf4622aac 100644 --- a/packages/agent/test/routes/access/list.test.ts +++ b/packages/agent/test/routes/access/list.test.ts @@ -2,6 +2,8 @@ import { Projection } from '@forestadmin/datasource-toolkit'; import { createMockContext } from '@shopify/jest-koa-mocks'; import List from '../../../src/routes/access/list'; +import AuthorizationService from '../../../src/services/authorization/authorization'; +import SegmentQueryHandler from '../../../src/services/segment-query-handler'; import * as factories from '../../__factories__'; describe('ListRoute', () => { @@ -284,4 +286,91 @@ describe('ListRoute', () => { expect(router.get).toHaveBeenCalledWith('/books\\+\\?\\*', expect.any(Function)); }); }); + + describe('with a live query segment and the real authorization service', () => { + const throwingThrow = () => + jest.fn((status: number, message: string) => { + throw Object.assign(new Error(message), { status }); + }); + + const setupWithRealAuthorization = (canExecuteSegmentQuery: boolean) => { + const { dataSource, options, collection } = setup(); + const forestAdminClient = factories.forestAdminClient.build(); + (forestAdminClient.permissionService.canOnCollection as jest.Mock).mockResolvedValue(true); + (forestAdminClient.permissionService.canExecuteSegmentQuery as jest.Mock).mockResolvedValue( + canExecuteSegmentQuery, + ); + ( + forestAdminClient.contextVariablesInstantiator.buildContextVariables as jest.Mock + ).mockResolvedValue({ getValue: jest.fn() }); + const services = factories.forestAdminHttpDriverServices.build({ + authorization: new AuthorizationService(forestAdminClient), + segmentQueryHandler: new SegmentQueryHandler( + forestAdminClient.contextVariablesInstantiator, + ), + }); + jest.spyOn(collection, 'list').mockResolvedValue([{ id: 1 }]); + + return { + forestAdminClient, + list: new List(services, options, dataSource, collection.name), + collection, + }; + }; + + const buildContext = () => + createMockContext({ + throw: throwingThrow(), + state: { user: { id: 35, renderingId: 42, email: 'john.doe@domain.com' } }, + customProperties: { + query: { + timezone: 'Europe/Paris', + segmentQuery: 'SELECT id FROM books', + connectionName: 'main', + }, + }, + }); + + const expectedSegmentCheck = { + userId: 35, + collectionName: 'books', + renderingId: 42, + segmentQuery: 'SELECT id FROM books', + connectionName: 'main', + }; + + test('refuses to list a segment the user is not allowed to run', async () => { + const { forestAdminClient, list, collection } = setupWithRealAuthorization(false); + const context = buildContext(); + + await expect(list.handleList(context)).rejects.toMatchObject({ status: 403 }); + + expect(forestAdminClient.permissionService.canExecuteSegmentQuery).toHaveBeenCalledWith( + expectedSegmentCheck, + ); + expect(collection.list).not.toHaveBeenCalled(); + }); + + test('lists a segment the user is allowed to run', async () => { + const { forestAdminClient, list, collection } = setupWithRealAuthorization(true); + const context = buildContext(); + + await list.handleList(context); + + expect(forestAdminClient.permissionService.canExecuteSegmentQuery).toHaveBeenCalledWith( + expectedSegmentCheck, + ); + expect(collection.list).toHaveBeenCalledWith( + expect.objectContaining({ timezone: 'Europe/Paris' }), + expect.objectContaining({ + liveQuerySegment: { + query: 'SELECT id FROM books', + connectionName: 'main', + contextVariables: {}, + }, + }), + expect.anything(), + ); + }); + }); }); diff --git a/packages/agent/test/routes/modification/action/action.test.ts b/packages/agent/test/routes/modification/action/action.test.ts index 6e3544e179..a16d11c7c0 100644 --- a/packages/agent/test/routes/modification/action/action.test.ts +++ b/packages/agent/test/routes/modification/action/action.test.ts @@ -6,6 +6,7 @@ import { Readable } from 'stream'; import ActionRoute from '../../../../src/routes/modification/action/action'; import CustomActionTriggerForbiddenError from '../../../../src/routes/modification/action/errors/custom-action-trigger-forbidden-error'; +import AuthorizationService from '../../../../src/services/authorization/authorization'; import * as factories from '../../../__factories__'; // This part of the context is the same in all tests. @@ -1927,4 +1928,166 @@ describe('ActionRoute', () => { }); }); }); + + describe('with a live query segment and the real authorization service', () => { + const throwingThrow = () => + jest.fn((status: number, message: string) => { + throw Object.assign(new Error(message), { status }); + }); + + let realServices: ReturnType; + + const segmentQuery = 'SELECT id FROM books'; + const expectedSegmentCheck = { + userId: '42', + collectionName: 'books', + renderingId: 7, + segmentQuery, + connectionName: 'main', + }; + + const buildContext = (attributes: Record = {}) => + createMockContext({ + ...baseContext, + throw: throwingThrow(), + state: { user: { ...baseContext.state.user, renderingId: 7 } }, + requestBody: { + data: { + attributes: { + ...baseContext.requestBody.data.attributes, + ids: [], + all_records: true, + values: { firstname: 'John' }, + all_records_subset_query: { segmentQuery, connectionName: 'main' }, + ...attributes, + }, + }, + }, + }); + + const allowSegmentQuery = (allowed: boolean) => + ( + options.forestAdminClient.permissionService.canExecuteSegmentQuery as jest.Mock + ).mockResolvedValue(allowed); + + beforeEach(() => { + realServices = factories.forestAdminHttpDriverServices.build({ + authorization: new AuthorizationService(options.forestAdminClient), + }); + dataSource = factories.dataSource.buildWithCollections([ + factories.collection.build({ + name: 'books', + schema: { + actions: { MyBulkAction: { scope: 'Bulk' } }, + fields: { id: factories.columnSchema.uuidPrimaryKey().build() }, + }, + getForm: jest.fn().mockResolvedValue([{ type: 'String', label: 'firstname' }]), + execute: jest.fn().mockResolvedValue({ type: 'Success', message: 'ok', invalidated: [] }), + }), + ]); + + route = new ActionRoute(realServices, options, dataSource, 'books', 'MyBulkAction'); + }); + + test('refuses to execute an action on a segment the user is not allowed to run', async () => { + allowSegmentQuery(false); + const context = buildContext(); + + // @ts-expect-error: test private method + await expect(route.handleExecute(context)).rejects.toMatchObject({ status: 403 }); + + expect( + options.forestAdminClient.permissionService.canExecuteSegmentQuery, + ).toHaveBeenCalledWith(expectedSegmentCheck); + expect(dataSource.getCollection('books').execute).not.toHaveBeenCalled(); + }); + + test('executes an action on a segment the user is allowed to run', async () => { + allowSegmentQuery(true); + const context = buildContext(); + + // @ts-expect-error: test private method + await route.handleExecute(context); + + expect( + options.forestAdminClient.permissionService.canExecuteSegmentQuery, + ).toHaveBeenCalledWith(expectedSegmentCheck); + expect(dataSource.getCollection('books').execute).toHaveBeenCalledWith( + expect.objectContaining({ id: '42' }), + 'MyBulkAction', + { firstname: 'John' }, + expect.objectContaining({ + liveQuerySegment: { query: segmentQuery, connectionName: 'main' }, + }), + ); + expect(context.response.body).toEqual(expect.objectContaining({ success: 'ok' })); + }); + + test('refuses to load an action form on a segment the user is not allowed to run', async () => { + allowSegmentQuery(false); + const context = buildContext(); + + // @ts-expect-error: test private method + await expect(route.handleHook(context)).rejects.toMatchObject({ status: 403 }); + + expect( + options.forestAdminClient.permissionService.canExecuteSegmentQuery, + ).toHaveBeenCalledWith(expectedSegmentCheck); + expect(dataSource.getCollection('books').getForm).not.toHaveBeenCalled(); + }); + + test('loads an action form on a segment the user is allowed to run', async () => { + allowSegmentQuery(true); + const context = buildContext(); + + // @ts-expect-error: test private method + await route.handleHook(context); + + expect( + options.forestAdminClient.permissionService.canExecuteSegmentQuery, + ).toHaveBeenCalledWith(expectedSegmentCheck); + expect(dataSource.getCollection('books').getForm).toHaveBeenCalledWith( + expect.objectContaining({ id: '42' }), + 'MyBulkAction', + null, + expect.objectContaining({ + liveQuerySegment: { query: segmentQuery, connectionName: 'main' }, + }), + expect.objectContaining({ includeHiddenFields: false }), + ); + }); + + test('refuses an approval on a segment the approver is not allowed to run', async () => { + allowSegmentQuery(false); + const context = buildContext({ requester_id: 12 }); + + // @ts-expect-error: test private method + await expect(route.handleExecute(context)).rejects.toMatchObject({ status: 403 }); + + expect( + options.forestAdminClient.permissionService.canExecuteSegmentQuery, + ).toHaveBeenCalledWith(expectedSegmentCheck); + expect(dataSource.getCollection('books').execute).not.toHaveBeenCalled(); + }); + + test('executes an approval on a segment the approver is allowed to run', async () => { + allowSegmentQuery(true); + const context = buildContext({ requester_id: 12 }); + + // @ts-expect-error: test private method + await route.handleExecute(context); + + expect( + options.forestAdminClient.permissionService.canExecuteSegmentQuery, + ).toHaveBeenCalledWith(expectedSegmentCheck); + expect(dataSource.getCollection('books').execute).toHaveBeenCalledWith( + expect.objectContaining({ id: '42' }), + 'MyBulkAction', + { firstname: 'John' }, + expect.objectContaining({ + liveQuerySegment: { query: segmentQuery, connectionName: 'main' }, + }), + ); + }); + }); }); diff --git a/packages/agent/test/routes/modification/associate-related.test.ts b/packages/agent/test/routes/modification/associate-related.test.ts index 462cf01f9c..a36676a5c5 100644 --- a/packages/agent/test/routes/modification/associate-related.test.ts +++ b/packages/agent/test/routes/modification/associate-related.test.ts @@ -58,6 +58,36 @@ describe('AssociateRelatedRoute', () => { ); }); + test('refuses the association before updating when the live query segment is not allowed', async () => { + const { services, dataSource, options } = setupWithOneToManyRelation(); + (services.authorization.assertCanExecuteLiveQuerySegment as jest.Mock).mockRejectedValueOnce( + new Error('Forbidden'), + ); + const route = new AssociateRelatedRoute( + services, + options, + dataSource, + 'books', + 'myBookPersons', + ); + const context = createMockContext({ + state: { user: { email: 'john.doe@domain.com' } }, + requestBody: { data: [{ id: '123e4567-e89b-12d3-a456-222222222222' }] }, + customProperties: { + query: { timezone: 'Europe/Paris' }, + params: { parentId: '123e4567-e89b-12d3-a456-111111111111' }, + }, + }); + + await expect(route.handleAssociateRelatedRoute(context)).rejects.toThrow('Forbidden'); + + expect(services.authorization.assertCanExecuteLiveQuerySegment).toHaveBeenCalledWith( + context, + 'bookPersons', + ); + expect(dataSource.getCollection('bookPersons').update).not.toHaveBeenCalled(); + }); + test('should update the related records', async () => { // given const { services, dataSource, options } = setupWithOneToManyRelation(); diff --git a/packages/agent/test/routes/modification/delete.test.ts b/packages/agent/test/routes/modification/delete.test.ts index 761b39d7be..c0a32fd409 100644 --- a/packages/agent/test/routes/modification/delete.test.ts +++ b/packages/agent/test/routes/modification/delete.test.ts @@ -1,6 +1,7 @@ import { createMockContext } from '@shopify/jest-koa-mocks'; import DeleteRoute from '../../../src/routes/modification/delete'; +import AuthorizationService from '../../../src/services/authorization/authorization'; import { HttpCode } from '../../../src/types'; import * as factories from '../../__factories__'; @@ -465,6 +466,123 @@ describe('DeleteRoute', () => { }); }); + describe('with a live query segment and the real authorization service', () => { + const throwingThrow = () => + jest.fn((status: number, message: string) => { + throw Object.assign(new Error(message), { status }); + }); + + const setup = (canExecuteSegmentQuery: boolean) => { + const forestAdminClient = factories.forestAdminClient.build(); + (forestAdminClient.permissionService.canOnCollection as jest.Mock).mockResolvedValue(true); + (forestAdminClient.permissionService.canExecuteSegmentQuery as jest.Mock).mockResolvedValue( + canExecuteSegmentQuery, + ); + const realServices = factories.forestAdminHttpDriverServices.build({ + authorization: new AuthorizationService(forestAdminClient), + }); + const bookCollection = factories.collection.build({ + name: 'books', + schema: factories.collectionSchema.build({ + fields: { id: factories.columnSchema.numericPrimaryKey().build() }, + }), + }); + const dataSource = factories.dataSource.buildWithCollection(bookCollection); + const deleteRoute = new DeleteRoute(realServices, options, dataSource, 'books'); + + return { forestAdminClient, bookCollection, deleteRoute }; + }; + + const listDeleteContext = () => + createMockContext({ + throw: throwingThrow(), + state: { user: { id: 35, renderingId: 42, email: 'john.doe@domain.com' } }, + customProperties: { query: { timezone: 'Europe/Paris' } }, + requestBody: { + data: { + attributes: { + ids: [], + all_records: true, + all_records_ids_excluded: [], + all_records_subset_query: { + segmentQuery: 'SELECT id FROM books', + connectionName: 'main', + }, + }, + }, + }, + }); + + test('refuses a bulk delete whose segment query the user is not allowed to run', async () => { + const { forestAdminClient, bookCollection, deleteRoute } = setup(false); + const context = listDeleteContext(); + + await expect(deleteRoute.handleListDelete(context)).rejects.toMatchObject({ + status: HttpCode.Forbidden, + }); + + expect(forestAdminClient.permissionService.canExecuteSegmentQuery).toHaveBeenCalledWith({ + userId: 35, + collectionName: 'books', + renderingId: 42, + segmentQuery: 'SELECT id FROM books', + connectionName: 'main', + }); + expect(bookCollection.delete).not.toHaveBeenCalled(); + }); + + test('deletes the records of a segment the user is allowed to run', async () => { + const { forestAdminClient, bookCollection, deleteRoute } = setup(true); + const context = listDeleteContext(); + + await deleteRoute.handleListDelete(context); + + expect(forestAdminClient.permissionService.canExecuteSegmentQuery).toHaveBeenCalledWith({ + userId: 35, + collectionName: 'books', + renderingId: 42, + segmentQuery: 'SELECT id FROM books', + connectionName: 'main', + }); + expect(bookCollection.delete).toHaveBeenCalledWith( + expect.objectContaining({ timezone: 'Europe/Paris' }), + expect.objectContaining({ + liveQuerySegment: { query: 'SELECT id FROM books', connectionName: 'main' }, + }), + ); + expect(context.response.status).toEqual(HttpCode.NoContent); + }); + + test('refuses a single delete whose query string carries a segment query the user is not allowed to run', async () => { + const { forestAdminClient, bookCollection, deleteRoute } = setup(false); + const context = createMockContext({ + throw: throwingThrow(), + state: { user: { id: 35, renderingId: 42, email: 'john.doe@domain.com' } }, + customProperties: { + params: { id: '1523' }, + query: { + timezone: 'Europe/Paris', + segmentQuery: 'SELECT id FROM books', + connectionName: 'main', + }, + }, + }); + + await expect(deleteRoute.handleDelete(context)).rejects.toMatchObject({ + status: HttpCode.Forbidden, + }); + + expect(forestAdminClient.permissionService.canExecuteSegmentQuery).toHaveBeenCalledWith({ + userId: 35, + collectionName: 'books', + renderingId: 42, + segmentQuery: 'SELECT id FROM books', + connectionName: 'main', + }); + expect(bookCollection.delete).not.toHaveBeenCalled(); + }); + }); + describe('with special characters in names', () => { it('should register a route with escaped characters', () => { const bookCollection = factories.collection.build({ diff --git a/packages/agent/test/routes/modification/dissociate-delete-related.test.ts b/packages/agent/test/routes/modification/dissociate-delete-related.test.ts index c4e49c8bc5..2f762d5c9a 100644 --- a/packages/agent/test/routes/modification/dissociate-delete-related.test.ts +++ b/packages/agent/test/routes/modification/dissociate-delete-related.test.ts @@ -65,6 +65,37 @@ describe('DissociateDeleteRelatedRoute', () => { ); }); + test('refuses to dissociate or delete related records when their live query segment is not allowed', async () => { + const { services, dataSource, options } = setupWithOneToManyRelation(); + (services.authorization.assertCanExecuteLiveQuerySegment as jest.Mock).mockRejectedValueOnce( + new Error('Forbidden'), + ); + const route = new DissociateDeleteRoute( + services, + options, + dataSource, + 'books', + 'myBookPersons', + ); + const context = createMockContext({ + state: { user: { email: 'john.doe@domain.com' } }, + customProperties: { + query: { timezone: 'Europe/Paris', delete: 'true' }, + params: { parentId: '123e4567-e89b-12d3-a456-426614174088' }, + }, + requestBody: { data: [{ id: '123e4567-e89b-12d3-a456-426614174001' }] }, + }); + + await expect(route.handleDissociateDeleteRelatedRoute(context)).rejects.toThrow('Forbidden'); + + expect(services.authorization.assertCanExecuteLiveQuerySegment).toHaveBeenCalledWith( + context, + 'bookPersons', + ); + expect(dataSource.getCollection('bookPersons').delete).not.toHaveBeenCalled(); + expect(dataSource.getCollection('bookPersons').update).not.toHaveBeenCalled(); + }); + describe('DissociateDeleteRelatedRoute > dissociate', () => { describe('when it is a one to many relation', () => { test('should update the foreign key in the foreign collection', async () => { diff --git a/packages/agent/test/services/authorization/authorization.test.ts b/packages/agent/test/services/authorization/authorization.test.ts index 30ebb5a815..36cbeb8cde 100644 --- a/packages/agent/test/services/authorization/authorization.test.ts +++ b/packages/agent/test/services/authorization/authorization.test.ts @@ -83,94 +83,130 @@ describe('AuthorizationService', () => { }); }); - describe('assertCanBrowseOnCollection', () => { - describe('when a live query segment is present', () => { - it('should not do anything for authorized users', async () => { - const forestAdminClient = factories.forestAdminClient.build(); - const authorizationService = new AuthorizationService(forestAdminClient); - - const context = { - request: { - query: { - segmentQuery: `SELECT id FROM books where type = 'comics'`, - connectionName: 'library', - }, - }, - state: { - user: { - id: 35, - renderingId: 42, - }, - }, - throw: jest.fn(), - } as unknown as Context; - - (forestAdminClient.permissionService.canOnCollection as jest.Mock).mockResolvedValue(true); - (forestAdminClient.permissionService.canExecuteSegmentQuery as jest.Mock).mockResolvedValue( - true, - ); - - await authorizationService.assertCanBrowse(context, 'books'); - - expect(context.throw).not.toHaveBeenCalled(); - - expect(forestAdminClient.permissionService.canOnCollection).toHaveBeenCalledWith({ - userId: 35, - event: CollectionActionEvent.Browse, - collectionName: 'books', - }); - - expect(forestAdminClient.permissionService.canExecuteSegmentQuery).toHaveBeenCalledWith({ - userId: 35, - collectionName: 'books', - renderingId: 42, - segmentQuery: `SELECT id FROM books where type = 'comics'`, - connectionName: 'library', - }); + describe('assertCanExecuteLiveQuerySegment', () => { + const buildContext = (request: { query?: object; body?: object }) => + ({ + request: { query: {}, ...request }, + state: { user: { id: 35, renderingId: 42 } }, + throw: jest.fn(), + } as unknown as Context); + + const subsetQueryBody = (segmentQuery: string, connectionName: string) => ({ + data: { attributes: { all_records_subset_query: { segmentQuery, connectionName } } }, + }); + + let forestAdminClient: ReturnType; + let authorizationService: AuthorizationService; + + beforeEach(() => { + forestAdminClient = factories.forestAdminClient.build(); + authorizationService = new AuthorizationService(forestAdminClient); + }); + + it('refuses the request when the segment query of the query string is not allowed', async () => { + (forestAdminClient.permissionService.canExecuteSegmentQuery as jest.Mock).mockResolvedValue( + false, + ); + const context = buildContext({ + query: { segmentQuery: 'SELECT id FROM books', connectionName: 'library' }, }); - describe('when query is not authorized', () => { - it('should throw forbidden error', async () => { - const forestAdminClient = factories.forestAdminClient.build(); - const authorizationService = new AuthorizationService(forestAdminClient); - - const context = { - request: { - query: { - segmentQuery: `DROP TABLE books;`, - connectionName: 'library', - }, - }, - state: { - user: { - id: 35, - renderingId: 42, - }, - }, - throw: jest.fn(), - } as unknown as Context; + await authorizationService.assertCanExecuteLiveQuerySegment(context, 'books'); - (forestAdminClient.permissionService.canOnCollection as jest.Mock).mockResolvedValue( - true, - ); - ( - forestAdminClient.permissionService.canExecuteSegmentQuery as jest.Mock - ).mockResolvedValue(false); + expect(forestAdminClient.permissionService.canExecuteSegmentQuery).toHaveBeenCalledWith({ + userId: 35, + collectionName: 'books', + renderingId: 42, + segmentQuery: 'SELECT id FROM books', + connectionName: 'library', + }); + expect(context.throw).toHaveBeenCalledWith(HttpCode.Forbidden, 'Forbidden'); + }); - await authorizationService.assertCanBrowse(context, 'books'); + it('refuses the request when the segment query of the request body is not allowed', async () => { + (forestAdminClient.permissionService.canExecuteSegmentQuery as jest.Mock).mockResolvedValue( + false, + ); + const context = buildContext({ body: subsetQueryBody('SELECT id FROM books', 'library') }); - expect(context.throw).toHaveBeenCalledWith(HttpCode.Forbidden, 'Forbidden'); + await authorizationService.assertCanExecuteLiveQuerySegment(context, 'books'); - expect(forestAdminClient.permissionService.canExecuteSegmentQuery).toHaveBeenCalledWith({ - userId: 35, - collectionName: 'books', - renderingId: 42, - segmentQuery: `DROP TABLE books;`, - connectionName: 'library', - }); - }); + expect(forestAdminClient.permissionService.canExecuteSegmentQuery).toHaveBeenCalledWith({ + userId: 35, + collectionName: 'books', + renderingId: 42, + segmentQuery: 'SELECT id FROM books', + connectionName: 'library', + }); + expect(context.throw).toHaveBeenCalledWith(HttpCode.Forbidden, 'Forbidden'); + }); + + it('checks the segment query the filter will use when both the body and the query string carry one', async () => { + (forestAdminClient.permissionService.canExecuteSegmentQuery as jest.Mock).mockResolvedValue( + true, + ); + const context = buildContext({ + query: { segmentQuery: 'SELECT id FROM books', connectionName: 'library' }, + body: subsetQueryBody('SELECT id FROM authors', 'archive'), + }); + + await authorizationService.assertCanExecuteLiveQuerySegment(context, 'books'); + + expect(forestAdminClient.permissionService.canExecuteSegmentQuery).toHaveBeenCalledTimes(1); + expect(forestAdminClient.permissionService.canExecuteSegmentQuery).toHaveBeenCalledWith({ + userId: 35, + collectionName: 'books', + renderingId: 42, + segmentQuery: 'SELECT id FROM authors', + connectionName: 'archive', }); }); + + it('allows the request when the segment query is allowed', async () => { + (forestAdminClient.permissionService.canExecuteSegmentQuery as jest.Mock).mockResolvedValue( + true, + ); + const context = buildContext({ body: subsetQueryBody('SELECT id FROM books', 'library') }); + + await authorizationService.assertCanExecuteLiveQuerySegment(context, 'books'); + + expect(forestAdminClient.permissionService.canExecuteSegmentQuery).toHaveBeenCalledWith({ + userId: 35, + collectionName: 'books', + renderingId: 42, + segmentQuery: 'SELECT id FROM books', + connectionName: 'library', + }); + expect(context.throw).not.toHaveBeenCalled(); + }); + + it('does not check any segment query when the request carries none', async () => { + const context = buildContext({ query: { timezone: 'Europe/Paris' } }); + + await authorizationService.assertCanExecuteLiveQuerySegment(context, 'books'); + + expect(forestAdminClient.permissionService.canExecuteSegmentQuery).not.toHaveBeenCalled(); + expect(context.throw).not.toHaveBeenCalled(); + }); + + it.each([ + 'assertCanAdd', + 'assertCanBrowse', + 'assertCanDelete', + 'assertCanEdit', + 'assertCanExport', + 'assertCanRead', + ])('%s leaves the segment query to the route whose filter uses it', async assertion => { + (forestAdminClient.permissionService.canOnCollection as jest.Mock).mockResolvedValue(true); + const context = buildContext({ + query: { segmentQuery: 'SELECT id FROM books', connectionName: 'library' }, + }); + + await authorizationService[assertion](context, 'authors'); + + expect(forestAdminClient.permissionService.canExecuteSegmentQuery).not.toHaveBeenCalled(); + expect(context.throw).not.toHaveBeenCalled(); + }); }); describe('getScope', () => { From a1bdbcf96425cebdd13e4dc0b75c36353631d2f2 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 27 Oct 2026 16:13:41 +0100 Subject: [PATCH 2/5] test(agent): drive the action segment tests through the registered routes Refs: PRD-1759 Co-Authored-By: Claude Opus 5.5 --- .../routes/modification/action/action.test.ts | 54 +++++++++++++------ 1 file changed, 38 insertions(+), 16 deletions(-) diff --git a/packages/agent/test/routes/modification/action/action.test.ts b/packages/agent/test/routes/modification/action/action.test.ts index a16d11c7c0..d6bab134aa 100644 --- a/packages/agent/test/routes/modification/action/action.test.ts +++ b/packages/agent/test/routes/modification/action/action.test.ts @@ -1965,6 +1965,36 @@ describe('ActionRoute', () => { }, }); + const callRoute = async (path: string, context: ReturnType) => { + const actionRouter = factories.router.mockAllMethods().build(); + route.setupRoutes(actionRouter); + const [, ...handlers] = (actionRouter.post as jest.Mock).mock.calls.find( + ([registeredPath]) => registeredPath === path, + ); + const dispatch = (index: number): Promise => + index < handlers.length ? handlers[index](context, () => dispatch(index + 1)) : undefined; + + return dispatch(0); + }; + + const executePath = '/_actions/books/0/mybulkaction'; + const loadHookPath = '/_actions/books/0/mybulkaction/hooks/load'; + + const buildApprovalContext = (allowed: boolean) => { + const signedRequest = buildContext({ requester_id: 12 }).request.body; + (options.forestAdminClient.verifySignedActionParameters as jest.Mock).mockReturnValue( + signedRequest, + ); + allowSegmentQuery(allowed); + + return createMockContext({ + ...baseContext, + throw: throwingThrow(), + state: { user: { ...baseContext.state.user, renderingId: 7 } }, + requestBody: { data: { attributes: { signed_approval_request: 'signed-request' } } }, + }); + }; + const allowSegmentQuery = (allowed: boolean) => ( options.forestAdminClient.permissionService.canExecuteSegmentQuery as jest.Mock @@ -1993,8 +2023,7 @@ describe('ActionRoute', () => { allowSegmentQuery(false); const context = buildContext(); - // @ts-expect-error: test private method - await expect(route.handleExecute(context)).rejects.toMatchObject({ status: 403 }); + await expect(callRoute(executePath, context)).rejects.toMatchObject({ status: 403 }); expect( options.forestAdminClient.permissionService.canExecuteSegmentQuery, @@ -2006,8 +2035,7 @@ describe('ActionRoute', () => { allowSegmentQuery(true); const context = buildContext(); - // @ts-expect-error: test private method - await route.handleExecute(context); + await callRoute(executePath, context); expect( options.forestAdminClient.permissionService.canExecuteSegmentQuery, @@ -2027,8 +2055,7 @@ describe('ActionRoute', () => { allowSegmentQuery(false); const context = buildContext(); - // @ts-expect-error: test private method - await expect(route.handleHook(context)).rejects.toMatchObject({ status: 403 }); + await expect(callRoute(loadHookPath, context)).rejects.toMatchObject({ status: 403 }); expect( options.forestAdminClient.permissionService.canExecuteSegmentQuery, @@ -2040,8 +2067,7 @@ describe('ActionRoute', () => { allowSegmentQuery(true); const context = buildContext(); - // @ts-expect-error: test private method - await route.handleHook(context); + await callRoute(loadHookPath, context); expect( options.forestAdminClient.permissionService.canExecuteSegmentQuery, @@ -2058,11 +2084,9 @@ describe('ActionRoute', () => { }); test('refuses an approval on a segment the approver is not allowed to run', async () => { - allowSegmentQuery(false); - const context = buildContext({ requester_id: 12 }); + const context = buildApprovalContext(false); - // @ts-expect-error: test private method - await expect(route.handleExecute(context)).rejects.toMatchObject({ status: 403 }); + await expect(callRoute(executePath, context)).rejects.toMatchObject({ status: 403 }); expect( options.forestAdminClient.permissionService.canExecuteSegmentQuery, @@ -2071,11 +2095,9 @@ describe('ActionRoute', () => { }); test('executes an approval on a segment the approver is allowed to run', async () => { - allowSegmentQuery(true); - const context = buildContext({ requester_id: 12 }); + const context = buildApprovalContext(true); - // @ts-expect-error: test private method - await route.handleExecute(context); + await callRoute(executePath, context); expect( options.forestAdminClient.permissionService.canExecuteSegmentQuery, From 5742a89117c7eb9e151eee033bb3f629ef5739b8 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 27 Oct 2026 16:19:56 +0100 Subject: [PATCH 3/5] test(agent): declare the segment permission helper before the route helpers using it Refs: PRD-1759 Co-Authored-By: Claude Opus 5.5 --- .../test/routes/modification/action/action.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/agent/test/routes/modification/action/action.test.ts b/packages/agent/test/routes/modification/action/action.test.ts index d6bab134aa..e6309539c7 100644 --- a/packages/agent/test/routes/modification/action/action.test.ts +++ b/packages/agent/test/routes/modification/action/action.test.ts @@ -1965,6 +1965,11 @@ describe('ActionRoute', () => { }, }); + const allowSegmentQuery = (allowed: boolean) => + ( + options.forestAdminClient.permissionService.canExecuteSegmentQuery as jest.Mock + ).mockResolvedValue(allowed); + const callRoute = async (path: string, context: ReturnType) => { const actionRouter = factories.router.mockAllMethods().build(); route.setupRoutes(actionRouter); @@ -1995,11 +2000,6 @@ describe('ActionRoute', () => { }); }; - const allowSegmentQuery = (allowed: boolean) => - ( - options.forestAdminClient.permissionService.canExecuteSegmentQuery as jest.Mock - ).mockResolvedValue(allowed); - beforeEach(() => { realServices = factories.forestAdminHttpDriverServices.build({ authorization: new AuthorizationService(options.forestAdminClient), From 409bce04bae7d6c1400c3e1d075640119c4d4d01 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Wed, 28 Oct 2026 15:25:31 +0100 Subject: [PATCH 4/5] fix(action): leave the segment out of an explicit id selection Explicit ids already pin the records, so the selection's live query segment is neither checked nor run, as forest-express and forest-rails do. Co-Authored-By: Claude Opus 5.5 --- .../src/routes/modification/action/action.ts | 31 +++++++++---- .../agent/test/routes/access/list.test.ts | 2 +- .../routes/modification/action/action.test.ts | 46 +++++++++++++++---- 3 files changed, 59 insertions(+), 20 deletions(-) diff --git a/packages/agent/src/routes/modification/action/action.ts b/packages/agent/src/routes/modification/action/action.ts index ed5be82a3a..d24bc4cebd 100644 --- a/packages/agent/src/routes/modification/action/action.ts +++ b/packages/agent/src/routes/modification/action/action.ts @@ -384,23 +384,34 @@ export default class ActionRoute extends CollectionRoute { const body = context.request.body as any; const attributes = body?.data?.attributes; + const actionScope = this.collection.schema.actions[this.actionName].scope; + const selectionIds = + actionScope === 'Global' + ? null + : BodyParser.parseSelectionIds(this.collection.schema, context); + // Explicit ids already pin the records: the selection's segment adds nothing, so it never runs. + const isExplicitIdSelection = selectionIds !== null && !selectionIds.areExcluded; + // Match user filter + search + scope? + segment. - await this.services.authorization.assertCanExecuteLiveQuerySegment( - context, - this.collection.name, - ); + if (!isExplicitIdSelection) { + await this.services.authorization.assertCanExecuteLiveQuerySegment( + context, + this.collection.name, + ); + } const scope = includeUserScope ? await this.services.authorization.getScope(this.collection, context) : null; - let filter = ContextFilterFactory.build(this.collection, context, scope); + let filter = ContextFilterFactory.build( + this.collection, + context, + scope, + isExplicitIdSelection ? { liveQuerySegment: undefined } : undefined, + ); // Restrict the filter to the selected records for single or bulk actions. - const actionScope = this.collection.schema.actions[this.actionName].scope; - - if (actionScope !== 'Global') { - const selectionIds = BodyParser.parseSelectionIds(this.collection.schema, context); - + if (selectionIds) { // A single action runs on its first record, or on nothing: refuse any selection but one id. if (actionScope === 'Single' && (selectionIds.areExcluded || selectionIds.ids.length !== 1)) { throw new UnprocessableError('This action can only be triggered on one record at a time.'); diff --git a/packages/agent/test/routes/access/list.test.ts b/packages/agent/test/routes/access/list.test.ts index acf4622aac..83c809e74c 100644 --- a/packages/agent/test/routes/access/list.test.ts +++ b/packages/agent/test/routes/access/list.test.ts @@ -369,7 +369,7 @@ describe('ListRoute', () => { contextVariables: {}, }, }), - expect.anything(), + ['id'], ); }); }); diff --git a/packages/agent/test/routes/modification/action/action.test.ts b/packages/agent/test/routes/modification/action/action.test.ts index e6309539c7..25847f6518 100644 --- a/packages/agent/test/routes/modification/action/action.test.ts +++ b/packages/agent/test/routes/modification/action/action.test.ts @@ -1803,7 +1803,7 @@ describe('ActionRoute', () => { ]); }); - test('resolves the live-query segment context variables before listing the audited selection', async () => { + test('lists an explicit audited selection by its ids, without its live-query segment', async () => { const context = createMockContext({ ...baseContext, customProperties: { @@ -1830,15 +1830,19 @@ describe('ActionRoute', () => { await route.handleExecute(context); expect(dataSource.getCollection('books').list).toHaveBeenCalledWith( - expect.anything(), + expect.objectContaining({ id: '42' }), expect.objectContaining({ - liveQuerySegment: expect.objectContaining({ - query: 'SELECT 1', - connectionName: 'main', - contextVariables: {}, + liveQuerySegment: expect.toBeNil(), + conditionTree: expect.objectContaining({ + field: 'id', + operator: 'In', + value: [ + '123e4567-e89b-12d3-a456-426614174000', + '123e4567-e89b-12d3-a456-426614174001', + ], }), }), - expect.anything(), + ['id'], ); }); @@ -1985,8 +1989,8 @@ describe('ActionRoute', () => { const executePath = '/_actions/books/0/mybulkaction'; const loadHookPath = '/_actions/books/0/mybulkaction/hooks/load'; - const buildApprovalContext = (allowed: boolean) => { - const signedRequest = buildContext({ requester_id: 12 }).request.body; + const buildApprovalContext = (allowed: boolean, attributes: Record = {}) => { + const signedRequest = buildContext({ requester_id: 12, ...attributes }).request.body; (options.forestAdminClient.verifySignedActionParameters as jest.Mock).mockReturnValue( signedRequest, ); @@ -2094,6 +2098,30 @@ describe('ActionRoute', () => { expect(dataSource.getCollection('books').execute).not.toHaveBeenCalled(); }); + test('executes an approval on selected records without checking or running their segment', async () => { + const selectedId = '123e4567-e89b-12d3-a456-426614174000'; + const context = buildApprovalContext(false, { all_records: false, ids: [selectedId] }); + + await callRoute(executePath, context); + + expect( + options.forestAdminClient.permissionService.canExecuteSegmentQuery, + ).not.toHaveBeenCalled(); + expect(dataSource.getCollection('books').execute).toHaveBeenCalledWith( + expect.objectContaining({ id: '42' }), + 'MyBulkAction', + { firstname: 'John' }, + expect.objectContaining({ + liveQuerySegment: expect.toBeNil(), + conditionTree: expect.objectContaining({ + field: 'id', + operator: 'Equal', + value: selectedId, + }), + }), + ); + }); + test('executes an approval on a segment the approver is allowed to run', async () => { const context = buildApprovalContext(true); From b68cd93e85942d155cdb6f0f992949c3587d44ac Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Fri, 30 Oct 2026 13:58:25 +0100 Subject: [PATCH 5/5] fix(datasource-sequelize): run live queries in a transaction that is always rolled back Live query segments and SQL charts only read. executeNativeQuery now runs them in a transaction it always rolls back, read-only on Postgres, so no statement they run is ever committed, whatever the dialect. Co-Authored-By: Claude Opus 5.5 --- .../datasource-sequelize/src/datasource.ts | 27 +++++-- .../test/datasource.unit.test.ts | 75 +++++++++++++++++-- .../native-query.integration.test.ts | 74 ++++++++++++++++++ 3 files changed, 165 insertions(+), 11 deletions(-) create mode 100644 packages/datasource-sequelize/test/integration/native-query/native-query.integration.test.ts diff --git a/packages/datasource-sequelize/src/datasource.ts b/packages/datasource-sequelize/src/datasource.ts index 9e9a642875..76bcedc710 100644 --- a/packages/datasource-sequelize/src/datasource.ts +++ b/packages/datasource-sequelize/src/datasource.ts @@ -58,13 +58,30 @@ export default class SequelizeDataSource extends BaseDataSource( - query, - { + const { instance } = this.nativeQueryConnections[connectionName] as NativeQueryConnection; + // Live queries only read: whatever their text runs, it is rolled back, never committed. + const transaction = await instance.transaction(); + let rows: R[]; + + try { + if (instance.getDialect() === 'postgres') { + await instance.query('SET TRANSACTION READ ONLY', { transaction }); + } + + rows = await instance.query(query, { bind: contextVariables, type: QueryTypes.SELECT, raw: true, - }, - ); + transaction, + }); + } catch (error) { + // The failed query may have ended the transaction already; its error is the one to report. + await transaction.rollback().catch(() => undefined); + throw error; + } + + await transaction.rollback(); + + return rows; } } diff --git a/packages/datasource-sequelize/test/datasource.unit.test.ts b/packages/datasource-sequelize/test/datasource.unit.test.ts index 2eb0f1e519..2aff1176af 100644 --- a/packages/datasource-sequelize/test/datasource.unit.test.ts +++ b/packages/datasource-sequelize/test/datasource.unit.test.ts @@ -65,25 +65,88 @@ describe('SequelizeDataSource', () => { }); describe('executeNativeQuery', () => { - it('should execute the given query on the correct connection', async () => { - const sequelize = new Sequelize({ dialect: 'postgres' }); + const setup = (dialect: 'postgres' | 'mssql') => { + const sequelize = new Sequelize({ dialect }); sequelize.define('cars', {}); - const logger = jest.fn(); - const spyQuery = jest.spyOn(sequelize, 'query').mockImplementation(); - const dataSource = new SequelizeDataSource(sequelize, logger, { + const transaction = { rollback: jest.fn().mockResolvedValue(undefined) }; + jest.spyOn(sequelize, 'transaction').mockResolvedValue(transaction as never); + const spyQuery = jest.spyOn(sequelize, 'query').mockResolvedValue([{ id: 1 }] as never); + const dataSource = new SequelizeDataSource(sequelize, jest.fn(), { liveQueryConnections: 'main', }); + return { dataSource, spyQuery, transaction }; + }; + + it('should execute the given query on the correct connection, in a transaction', async () => { + const { dataSource, spyQuery, transaction } = setup('mssql'); + await dataSource.executeNativeQuery('main', 'query', { something: 'value' }); - expect(spyQuery).toHaveBeenCalled(); expect(spyQuery).toHaveBeenCalledWith('query', { bind: { something: 'value' }, type: QueryTypes.SELECT, raw: true, + transaction, }); }); + it('should return the rows and roll the transaction back', async () => { + const { dataSource, transaction } = setup('mssql'); + + const rows = await dataSource.executeNativeQuery('main', 'query'); + + expect(rows).toEqual([{ id: 1 }]); + expect(transaction.rollback).toHaveBeenCalledTimes(1); + }); + + it('should make the transaction read-only on postgres before running the query', async () => { + const { dataSource, spyQuery, transaction } = setup('postgres'); + + await dataSource.executeNativeQuery('main', 'query'); + + expect(spyQuery).toHaveBeenNthCalledWith(1, 'SET TRANSACTION READ ONLY', { transaction }); + expect(spyQuery).toHaveBeenNthCalledWith( + 2, + 'query', + expect.objectContaining({ transaction }), + ); + }); + + it('should not set a read-only transaction outside postgres', async () => { + const { dataSource, spyQuery } = setup('mssql'); + + await dataSource.executeNativeQuery('main', 'query'); + + expect(spyQuery).toHaveBeenCalledTimes(1); + expect(spyQuery).toHaveBeenCalledWith('query', expect.anything()); + }); + + it('should roll back and rethrow the query error when the query fails', async () => { + const { dataSource, spyQuery, transaction } = setup('mssql'); + spyQuery.mockRejectedValue(new Error('syntax error')); + + await expect(dataSource.executeNativeQuery('main', 'query')).rejects.toThrow('syntax error'); + expect(transaction.rollback).toHaveBeenCalledTimes(1); + }); + + it('should report the query error when the rollback fails after it', async () => { + const { dataSource, spyQuery, transaction } = setup('mssql'); + spyQuery.mockRejectedValue(new Error('syntax error')); + transaction.rollback.mockRejectedValue(new Error('no transaction')); + + await expect(dataSource.executeNativeQuery('main', 'query')).rejects.toThrow('syntax error'); + }); + + it('should fail when the rollback fails after a successful query', async () => { + const { dataSource, transaction } = setup('mssql'); + transaction.rollback.mockRejectedValue(new Error('no transaction')); + + await expect(dataSource.executeNativeQuery('main', 'query')).rejects.toThrow( + 'no transaction', + ); + }); + describe('when giving an unknown connection name', () => { it('should throw an error', async () => { const sequelize = new Sequelize({ dialect: 'postgres' }); diff --git a/packages/datasource-sequelize/test/integration/native-query/native-query.integration.test.ts b/packages/datasource-sequelize/test/integration/native-query/native-query.integration.test.ts new file mode 100644 index 0000000000..21519397ef --- /dev/null +++ b/packages/datasource-sequelize/test/integration/native-query/native-query.integration.test.ts @@ -0,0 +1,74 @@ +import type { Dialect } from 'sequelize'; + +import { DataTypes, Sequelize } from 'sequelize'; + +import { SequelizeDataSource } from '../../../src'; +import CONNECTIONS from '../../__tests/connections'; + +const ROWS_DELETING_QUERY: Record string> = { + mysql: table => `DELETE FROM ${table}`, + mssql: table => `DELETE FROM ${table} OUTPUT deleted.id`, +}; + +function deleteAllRows(dialect: Dialect, table: string): string { + return (ROWS_DELETING_QUERY[dialect] ?? (t => `DELETE FROM ${t} RETURNING id`))(table); +} + +describe('Native queries', () => { + const sqlite = { + name: 'SQLite', + open: async () => new Sequelize({ dialect: 'sqlite', storage: ':memory:', logging: false }), + }; + const servers = CONNECTIONS.map(connection => ({ + name: connection.name, + open: async () => { + const db = 'forest_test_native_query'; + const server = new Sequelize(connection.uri(), { logging: false }); + await server.getQueryInterface().dropDatabase(db); + await connection.query.createDatabase(server, db); + await server.close(); + + return new Sequelize(connection.uri(db), { logging: false }); + }, + })); + + describe.each([sqlite, ...servers])('On $name', ({ open }) => { + let sequelize: Sequelize; + let dataSource: SequelizeDataSource; + let table: string; + + beforeAll(async () => { + sequelize = await open(); + sequelize.define('Label', { title: DataTypes.STRING }); + await sequelize.sync({ force: true }); + table = sequelize.getQueryInterface().quoteIdentifier('Labels'); + dataSource = new SequelizeDataSource(sequelize, jest.fn(), { liveQueryConnections: 'main' }); + }, 60_000); + + beforeEach(async () => { + await sequelize.models.Label.destroy({ truncate: true }); + await sequelize.models.Label.bulkCreate([{ title: 'a' }, { title: 'b' }]); + }); + + afterAll(async () => { + await sequelize?.close(); + }); + + it('should return the rows a query selects', async () => { + const rows = await dataSource.executeNativeQuery<{ title: string }>( + 'main', + `SELECT title FROM ${table} ORDER BY title`, + ); + + expect(rows).toEqual([{ title: 'a' }, { title: 'b' }]); + }); + + it('should leave the data untouched when a query writes', async () => { + await dataSource + .executeNativeQuery('main', deleteAllRows(sequelize.getDialect() as Dialect, table)) + .catch(() => undefined); + + expect(await sequelize.models.Label.count()).toBe(2); + }); + }); +});