diff --git a/packages/plugins/policy/src/policy-handler.ts b/packages/plugins/policy/src/policy-handler.ts index ddc58ee7a..9754ca422 100644 --- a/packages/plugins/policy/src/policy-handler.ts +++ b/packages/plugins/policy/src/policy-handler.ts @@ -38,7 +38,6 @@ import { ValueNode, ValuesNode, WhereNode, - type Expression as KyselyExpression, type OperationNode, type QueryResult, type RootOperationNode, @@ -968,32 +967,30 @@ export class PolicyHandler extends OperationNodeTransf ? this.unwrapCreateValueRows(node.values, mutationModel, fields, isManyToManyJoinTable) : [[]]; - if (!isManyToManyJoinTable) { - // A create filter built only from `auth()` doesn't reference the row being inserted, - // so it holds for every row alike and checking it per row re-asks the same question. - const filter = this.buildPolicyFilter(mutationModel, undefined, 'create'); - if (isTrueNode(filter)) { - return; - } - } - - for (const values of valueRows) { - if (isManyToManyJoinTable) { + if (isManyToManyJoinTable) { + for (const values of valueRows) { await this.enforcePreCreatePolicyForManyToManyJoinTable( mutationModel, fields, values.map((v) => v.node), proceed, ); - } else { - await this.enforcePreCreatePolicyForOne( - mutationModel, - fields, - values.map((v) => v.node), - proceed, - ); } + return; + } + + const filter = this.buildPolicyFilter(mutationModel, undefined, 'create'); + if (isTrueNode(filter)) { + return; } + + await this.enforcePreCreatePolicyForRows( + mutationModel, + fields, + valueRows.map((values) => values.map((v) => v.node)), + filter, + proceed, + ); } private async enforcePreCreatePolicyForManyToManyJoinTable( @@ -1059,52 +1056,33 @@ export class PolicyHandler extends OperationNodeTransf } } - private async enforcePreCreatePolicyForOne( + private async enforcePreCreatePolicyForRows( model: string, fields: string[], - values: OperationNode[], + rows: OperationNode[][], + filter: OperationNode, proceed: ProceedKyselyQueryFunction, ) { const allFields = QueryUtils.getModelFields(this.client.$schema, model, { inherited: true }); - const allValues: KyselyExpression[] = []; + const fieldIndices = allFields.map((def) => fields.indexOf(def.name)); - for (const def of allFields) { - const index = fields.indexOf(def.name); - if (index >= 0) { - allValues.push(new ExpressionWrapper(values[index]!)); - } else { - // set non-provided fields to null - allValues.push(this.eb.lit(null)); - } - } - - // create a `SELECT column1 as field1, column2 as field2, ... FROM (VALUES (...))` table for policy evaluation - const valuesTable = this.dialect.buildValuesTableSelect(allFields, [allValues]); - - const filter = this.buildPolicyFilter(model, undefined, 'create'); + // NULL literals rather than parameters: the check has to fit the INSERT's parameter limit + const allRows = rows.map((values) => + fieldIndices.map((index) => (index >= 0 ? new ExpressionWrapper(values[index]!) : this.eb.lit(null))), + ); - // check if the provided values satisfy the create policy + // create a `SELECT column1 as field1, column2 as field2, ... FROM (VALUES (...), ...)` table for policy evaluation + const valuesTable = this.dialect.buildValuesTableSelect(allFields, allRows); - // `SELECT 1 FROM (VALUES (...)) AS t(column1, column2, ...) WHERE ` - const preCreateInner = this.eb + // Every row must satisfy the filter. Counting the rows that do (rather than looking for one + // that doesn't) also rejects rows whose filter evaluates to NULL. + // `SELECT COUNT(1) = AS $condition FROM (VALUES ...) AS t(column1, column2, ...) WHERE ` + const preCreateQuery = this.eb .selectFrom(valuesTable.as(model)) - .select(this.eb.lit(1).as('_')) + .select(this.eb(this.eb.fn('COUNT', [this.eb.lit(1)]), '=', this.eb.lit(rows.length)).as('$condition')) .where(() => new ExpressionWrapper(filter)); - const result = await proceed( - // `SELECT EXISTS(preCreateInner) AS $condition` - { - kind: 'SelectQueryNode', - selections: [ - SelectionNode.create( - AliasNode.create( - this.eb.exists(preCreateInner).toOperationNode(), - IdentifierNode.create('$condition'), - ), - ), - ], - } satisfies SelectQueryNode, - ); + const result = await proceed(preCreateQuery.toOperationNode()); if (!result.rows[0]?.$condition) { throw createRejectedByPolicyError(model, RejectedByPolicyReason.NO_ACCESS); } @@ -1145,7 +1123,8 @@ export class PolicyHandler extends OperationNodeTransf if (itemsAreNodes) { const itemNode = item as OperationNode; if (DefaultInsertValueNode.is(itemNode)) { - result.push({ node: ValueNode.create(null), raw: null }); + // inlined like the DEFAULT keyword it replaces, so it binds no parameter + result.push({ node: ValueNode.createImmediate(null), raw: null }); continue; } const fieldDef = QueryUtils.requireField(this.client.$schema, model, fields[i]!); @@ -1167,16 +1146,7 @@ export class PolicyHandler extends OperationNodeTransf value = this.dialect.transformInput(item, fieldDef.type as BuiltinType, !!fieldDef.array); } - // handle the case for list column - if (Array.isArray(value)) { - const fieldDef = QueryUtils.requireField(this.client.$schema, model, fields[i]!); - result.push({ - node: this.dialect.buildArrayValue(value, fieldDef.type).toOperationNode(), - raw: value, - }); - } else { - result.push({ node: ValueNode.create(value), raw: value }); - } + result.push({ node: ValueNode.create(value), raw: value }); } } return result; diff --git a/tests/regression/test/issue-2841.test.ts b/tests/regression/test/issue-2841.test.ts index e0acca7c9..e9a4f596a 100644 --- a/tests/regression/test/issue-2841.test.ts +++ b/tests/regression/test/issue-2841.test.ts @@ -20,8 +20,8 @@ model AuditLog { } model Note { - id String @id @default(cuid()) - ownerId String + id String @id @default(cuid()) + ownerId String? @@allow('create', ownerId == auth().id) @@allow('read', true) @@ -69,23 +69,37 @@ model Note { await expect(db.auditLog.findMany()).resolves.toHaveLength(0); }); - it('still checks each row when the create filter references the row', async () => { + it('checks a row-dependent create filter for the whole batch in one query', async () => { const { db, policyChecks } = await createClient(); const authDb = db.$setAuth({ id: 'u1', isSystem: true }); await expect( authDb.note.createMany({ - data: [{ ownerId: 'u1' }, { ownerId: 'u1' }], + data: Array.from({ length: 5 }, () => ({ ownerId: 'u1' })), }), - ).resolves.toMatchObject({ count: 2 }); - expect(policyChecks()).toHaveLength(2); + ).resolves.toMatchObject({ count: 5 }); + expect(policyChecks()).toHaveLength(1); // a single offending row still rejects the whole batch await expect( authDb.note.createMany({ - data: [{ ownerId: 'u1' }, { ownerId: 'someone-else' }], + data: [{ ownerId: 'u1' }, { ownerId: 'someone-else' }, { ownerId: 'u1' }], + }), + ).rejects.toThrow(/rejected by access policies/i); + expect(policyChecks()).toHaveLength(2); + await expect(db.note.findMany()).resolves.toHaveLength(5); + }); + + it('rejects the batch when the create filter evaluates to NULL for a row', async () => { + const { db } = await createClient(); + const authDb = db.$setAuth({ id: 'u1', isSystem: true }); + + // `NULL = 'u1'` is neither true nor false + await expect( + authDb.note.createMany({ + data: [{ ownerId: 'u1' }, { ownerId: null }], }), ).rejects.toThrow(/rejected by access policies/i); - await expect(db.note.findMany()).resolves.toHaveLength(2); + await expect(db.note.findMany()).resolves.toHaveLength(0); }); });