diff --git a/packages/plugins/policy/src/expression-transformer.ts b/packages/plugins/policy/src/expression-transformer.ts index b0037e040..0b1864a6f 100644 --- a/packages/plugins/policy/src/expression-transformer.ts +++ b/packages/plugins/policy/src/expression-transformer.ts @@ -91,6 +91,12 @@ export type ExpressionTransformerContext = { */ memberSelect?: SelectionNode; + /** + * In case of transforming a collection predicate's LHS, the table alias to use for the innermost + * relation (the one the predicate filter is compiled against) + */ + memberAlias?: string; + /** * The value object that fields should be evaluated against */ @@ -129,6 +135,9 @@ function expr(kind: Expression['kind']) { * Utility for transforming a ZModel expression into a Kysely OperationNode. */ export class ExpressionTransformer { + // counter for allocating unique relation table aliases + private aliasCounter = 0; + private readonly dialect: BaseCrudDialect; private readonly eb = expressionBuilder(); @@ -197,8 +206,8 @@ export class ExpressionTransformer { if (!fieldDef.relation) { return this.createColumnRef(expr.field, context); } else { - const { memberFilter, memberSelect, ...restContext } = context; - const relation = this.transformRelationAccess(expr.field, fieldDef.type, restContext); + const { memberFilter, memberSelect, memberAlias, ...restContext } = context; + const relation = this.transformRelationAccess(expr.field, fieldDef.type, restContext, memberAlias); return { ...relation, where: this.mergeWhere(relation.where, memberFilter), @@ -414,17 +423,22 @@ export class ExpressionTransformer { } } + // alias of the innermost relation table that the predicate filter is compiled against; relation + // tables are always given a unique alias so that they never shadow an enclosing table (which + // matters for self-relations and for nested predicates traversing the same relation) + const memberAlias = this.newRelationAlias(this.getLastMemberName(expr.left)); + const bindingScope = expr.binding ? { ...(context.bindingScope ?? {}), - [expr.binding]: { type: newContextModel, alias: newContextModel }, + [expr.binding]: { type: newContextModel, alias: memberAlias }, } : context.bindingScope; let predicateFilter = this.transform(expr.right, { ...context, modelOrType: newContextModel, - alias: undefined, + alias: memberAlias, // binding values (if any) remain available through `bindingScope` contextValue: undefined, bindingScope: bindingScope, @@ -446,9 +460,28 @@ export class ExpressionTransformer { ...context, memberSelect: SelectionNode.create(AliasNode.create(predicateResult, IdentifierNode.create('_'))), memberFilter: predicateFilter, + memberAlias, }); } + private getLastMemberName(expr: Expression) { + if (ExpressionUtils.isField(expr)) { + return expr.field; + } + invariant(ExpressionUtils.isMember(expr) && expr.members.length > 0, 'expected field or member expression'); + return expr.members[expr.members.length - 1]!; + } + + /** + * Allocates a table alias for a relation subquery, unique within this transformer. Aliasing every + * relation subquery avoids the related table shadowing an enclosing one when the relation points + * back to the same model (self-relation), including across nested collection predicates. + * The counter is per transformer instance so the same policy always compiles to the same SQL. + */ + private newRelationAlias(field: string) { + return `${field}$${++this.aliasCounter}`; + } + private ensureCollectionPredicateOperator(op: BinaryOperator): asserts op is CollectionPredicateOperator { invariant(CollectionPredicateOperator.includes(op as any), 'expected "?" or "!" or "^" operator'); } @@ -704,8 +737,9 @@ export class ExpressionTransformer { let members = expr.members; let receiver: OperationNode; + let receiverAlias: string; let startType: string | undefined; - const { memberFilter, memberSelect, ...restContext } = context; + const { memberFilter, memberSelect, memberAlias, ...restContext } = context; if (ExpressionUtils.isThis(expr.receiver)) { if (expr.members.length === 1) { @@ -722,12 +756,18 @@ export class ExpressionTransformer { // transform the first segment into a relation access, then continue with the rest of // the members; root the chain at the correct context model (thisType/thisAlias) const firstMemberFieldDef = QueryUtils.requireField(this.schema, context.thisType, expr.members[0]!); - receiver = this.transformRelationAccess(expr.members[0]!, firstMemberFieldDef.type, { - ...restContext, - alias: context.thisAlias, - modelOrType: context.thisType, - contextValue: undefined, - }); + receiverAlias = this.newRelationAlias(expr.members[0]!); + receiver = this.transformRelationAccess( + expr.members[0]!, + firstMemberFieldDef.type, + { + ...restContext, + alias: context.thisAlias, + modelOrType: context.thisType, + contextValue: undefined, + }, + receiverAlias, + ); members = expr.members.slice(1); // startType should be the type of the relation access startType = firstMemberFieldDef.type; @@ -747,17 +787,26 @@ export class ExpressionTransformer { // transform the first segment into a relation access, then continue with the rest of the members const bindingScope = this.requireBindingScope(expr.receiver, context); const firstMemberFieldDef = QueryUtils.requireField(this.schema, bindingScope.type, expr.members[0]!); - receiver = this.transformRelationAccess(expr.members[0]!, firstMemberFieldDef.type, { - ...restContext, - modelOrType: bindingScope.type, - alias: bindingScope.alias, - }); + receiverAlias = this.newRelationAlias(expr.members[0]!); + receiver = this.transformRelationAccess( + expr.members[0]!, + firstMemberFieldDef.type, + { + ...restContext, + modelOrType: bindingScope.type, + alias: bindingScope.alias, + }, + receiverAlias, + ); members = expr.members.slice(1); // startType should be the type of the relation access startType = firstMemberFieldDef.type; } } else { - receiver = this.transform(expr.receiver, restContext); + // field receiver, pass the alias to use for the relation table via `memberAlias` + invariant(ExpressionUtils.isField(expr.receiver), 'expected receiver to be a field expression'); + receiverAlias = this.newRelationAlias(expr.receiver.field); + receiver = this.transform(expr.receiver, { ...restContext, memberAlias: receiverAlias }); } invariant(SelectQueryNode.is(receiver), 'expected receiver to be select query'); @@ -772,27 +821,37 @@ export class ExpressionTransformer { } } - // traverse forward to collect member types - const memberFields: { fromModel: string; fieldDef: FieldDef }[] = []; + // traverse forward to collect member types and assign table aliases for each hop + const memberFields: { fromModel: string; fromAlias: string; fieldDef: FieldDef; alias: string }[] = []; let currType = startType; + let currAlias = receiverAlias; for (const member of members) { const fieldDef = QueryUtils.requireField(this.schema, currType, member); - memberFields.push({ fieldDef, fromModel: currType }); + const alias = fieldDef.relation ? this.newRelationAlias(member) : currAlias; + memberFields.push({ fieldDef, fromModel: currType, fromAlias: currAlias, alias }); currType = fieldDef.type; + currAlias = alias; } let currNode: SelectQueryNode | ColumnNode | ReferenceNode | undefined = undefined; for (let i = members.length - 1; i >= 0; i--) { const member = members[i]!; - const { fieldDef, fromModel } = memberFields[i]!; + const { fieldDef, fromModel, fromAlias, alias } = memberFields[i]!; if (fieldDef.relation) { - const relation = this.transformRelationAccess(member, fieldDef.type, { - ...restContext, - modelOrType: fromModel, - alias: undefined, - }); + const relation = this.transformRelationAccess( + member, + fieldDef.type, + { + ...restContext, + modelOrType: fromModel, + alias: fromAlias, + }, + // the innermost relation uses the alias the collection predicate filter (if any) + // was compiled against + i === members.length - 1 ? (memberAlias ?? alias) : alias, + ); if (currNode) { currNode = { @@ -813,7 +872,7 @@ export class ExpressionTransformer { invariant(i === members.length - 1, 'plain field access must be the last segment'); invariant(!currNode, 'plain field access must be the last segment'); - currNode = ColumnNode.create(member); + currNode = ReferenceNode.create(ColumnNode.create(member), TableNode.create(fromAlias)); } } @@ -854,14 +913,20 @@ export class ExpressionTransformer { return curr; } + /** + * Builds a `SELECT ... FROM AS WHERE ` subquery + * for accessing relation `field` from the current context model. The related table is always + * aliased so that it never shadows the outer table (which matters for self-relations). + */ private transformRelationAccess( field: string, relationModel: string, context: ExpressionTransformerContext, + relationAlias = this.newRelationAlias(field), ): SelectQueryNode { const m2m = QueryUtils.getManyToManyRelation(this.schema, context.modelOrType, field); if (m2m) { - return this.transformManyToManyRelationAccess(m2m, context); + return this.transformManyToManyRelationAccess(m2m, context, relationAlias); } const fromModel = context.modelOrType; @@ -890,7 +955,7 @@ export class ExpressionTransformer { return BinaryOperationNode.create( fkRef, OperatorNode.create('='), - ReferenceNode.create(ColumnNode.create(pk), TableNode.create(relationModel)), + ReferenceNode.create(ColumnNode.create(pk), TableNode.create(relationAlias)), ); }), ); @@ -902,7 +967,7 @@ export class ExpressionTransformer { BinaryOperationNode.create( ReferenceNode.create(ColumnNode.create(pk), TableNode.create(context.alias ?? fromModel)), OperatorNode.create('='), - ReferenceNode.create(ColumnNode.create(fk), TableNode.create(relationModel)), + ReferenceNode.create(ColumnNode.create(fk), TableNode.create(relationAlias)), ), ), ); @@ -910,7 +975,9 @@ export class ExpressionTransformer { return { kind: 'SelectQueryNode', - from: FromNode.create([TableNode.create(relationModel)]), + from: FromNode.create([ + AliasNode.create(TableNode.create(relationModel), IdentifierNode.create(relationAlias)), + ]), where: WhereNode.create(condition), }; } @@ -918,18 +985,21 @@ export class ExpressionTransformer { private transformManyToManyRelationAccess( m2m: NonNullable>, context: ExpressionTransformerContext, + relationAlias: string, ) { const eb = expressionBuilder(); + // alias the join table too so that nested traversals through the same relation don't shadow + const joinTableAlias = `${relationAlias}$join`; const relationQuery = eb - .selectFrom(m2m.otherModel) + .selectFrom(`${m2m.otherModel} as ${relationAlias}`) // inner join with join table and additionally filter by the parent model - .innerJoin(m2m.joinTable, (join) => + .innerJoin(`${m2m.joinTable} as ${joinTableAlias}`, (join) => join // relation model pk to join table fk - .onRef(`${m2m.otherModel}.${m2m.otherPKName}`, '=', `${m2m.joinTable}.${m2m.otherFkName}`) + .onRef(`${relationAlias}.${m2m.otherPKName}`, '=', `${joinTableAlias}.${m2m.otherFkName}`) // parent model pk to join table fk .onRef( - `${m2m.joinTable}.${m2m.parentFkName}`, + `${joinTableAlias}.${m2m.parentFkName}`, '=', `${context.alias ?? context.modelOrType}.${m2m.parentPKName}`, ), diff --git a/tests/e2e/orm/policy/self-relation.test.ts b/tests/e2e/orm/policy/self-relation.test.ts new file mode 100644 index 000000000..177db519f --- /dev/null +++ b/tests/e2e/orm/policy/self-relation.test.ts @@ -0,0 +1,213 @@ +import { createPolicyTestClient } from '@zenstackhq/testtools'; +import { describe, expect, it } from 'vitest'; + +describe('Self-relation policy tests', () => { + const tenantSchema = ` +type Auth { + id String @id + tenantId String + @@auth +} + +model Item { + id String @id + tenantId String + parentId String? + parent Item? @relation("ItemParent", fields: [parentId], references: [id]) + children Item[] @relation("ItemParent") + + @@allow('all', auth().tenantId == tenantId) + @@deny('create', parentId != null && parent.tenantId != this.tenantId) + @@deny('post-update', parentId != null && parent.tenantId != this.tenantId) +} +`; + + it('denies cross-tenant parent via foreign key on create and update', async () => { + const db = await createPolicyTestClient(tenantSchema); + const a = db.$setAuth({ id: 'user-a', tenantId: 'a' }); + const b = db.$setAuth({ id: 'user-b', tenantId: 'b' }); + + await a.item.create({ data: { id: 'a-parent', tenantId: 'a' } }); + await b.item.create({ data: { id: 'b-parent', tenantId: 'b' } }); + await expect(a.item.findUnique({ where: { id: 'b-parent' } })).toResolveNull(); + + // same-tenant create is allowed + await expect(a.item.create({ data: { id: 'a-child', tenantId: 'a', parentId: 'a-parent' } })).toResolveTruthy(); + + // cross-tenant create via fk is denied + await expect( + a.item.create({ data: { id: 'a-cross', tenantId: 'a', parentId: 'b-parent' } }), + ).toBeRejectedByPolicy(); + + // cross-tenant update via fk is denied + await expect( + a.item.update({ where: { id: 'a-child' }, data: { parentId: 'b-parent' } }), + ).toBeRejectedByPolicy(); + // cross-tenant update via connect is rejected (target not readable) + await expect( + a.item.update({ where: { id: 'a-child' }, data: { parent: { connect: { id: 'b-parent' } } } }), + ).toBeRejectedNotFound(); + + // same-tenant update and clearing the relation are allowed + await expect(a.item.update({ where: { id: 'a-child' }, data: { parentId: null } })).toResolveTruthy(); + await expect(a.item.update({ where: { id: 'a-child' }, data: { parentId: 'a-parent' } })).toResolveTruthy(); + await expect(a.item.update({ where: { id: 'a-child' }, data: { parentId: null } })).toResolveTruthy(); + + // nothing leaked + await expect(db.$unuseAll().item.findMany({ where: { parentId: 'b-parent' } })).resolves.toHaveLength(0); + }); + + it('reads through self relation with distinct outer/inner rows', async () => { + const db = await createPolicyTestClient( + ` +model Node { + id Int @id + value Int + parentId Int? + parent Node? @relation("Tree", fields: [parentId], references: [id]) + children Node[] @relation("Tree") + + @@allow('create', true) + @@allow('read', parent.value > this.value) +} +`, + ); + const raw = db.$unuseAll(); + await raw.node.create({ data: { id: 1, value: 10 } }); + await raw.node.create({ data: { id: 2, value: 5, parentId: 1 } }); + await raw.node.create({ data: { id: 3, value: 20, parentId: 1 } }); + + // root has no parent -> not readable; 2's parent value 10 > 5 -> readable; 3's 10 > 20 false + await expect(db.node.findMany()).resolves.toEqual([expect.objectContaining({ id: 2 })]); + }); + + it('works with collection predicate on self relation referencing this', async () => { + const db = await createPolicyTestClient( + ` +model Node { + id Int @id + value Int + parentId Int? + parent Node? @relation("Tree", fields: [parentId], references: [id]) + children Node[] @relation("Tree") + + @@allow('create', true) + @@allow('read', children?[value > this.value]) +} +`, + ); + const raw = db.$unuseAll(); + await raw.node.create({ data: { id: 1, value: 10 } }); + await raw.node.create({ data: { id: 2, value: 5, parentId: 1 } }); + await raw.node.create({ data: { id: 3, value: 20, parentId: 1 } }); + await raw.node.create({ data: { id: 4, value: 1, parentId: 3 } }); + + // 1 has child 3 (20 > 10) -> readable; 3 has child 4 (1 > 20 false); 2 and 4 have no children + await expect(db.node.findMany()).resolves.toEqual([expect.objectContaining({ id: 1 })]); + }); + + it('works with multi-hop self relation access', async () => { + const db = await createPolicyTestClient( + ` +model Node { + id Int @id + value Int + parentId Int? + parent Node? @relation("Tree", fields: [parentId], references: [id]) + children Node[] @relation("Tree") + + @@allow('create', true) + @@allow('read', parent.parent.value > this.value) +} +`, + ); + const raw = db.$unuseAll(); + await raw.node.create({ data: { id: 1, value: 10 } }); + await raw.node.create({ data: { id: 2, value: 100, parentId: 1 } }); + await raw.node.create({ data: { id: 3, value: 5, parentId: 2 } }); + await raw.node.create({ data: { id: 4, value: 50, parentId: 2 } }); + + // 3's grandparent is 1 (10 > 5) -> readable; 4: 10 > 50 false + await expect(db.node.findMany()).resolves.toEqual([expect.objectContaining({ id: 3 })]); + }); + + it('works with this-rooted self relation nested inside a collection predicate', async () => { + const db = await createPolicyTestClient( + ` +model Node { + id Int @id + value Int + parentId Int? + parent Node? @relation("Tree", fields: [parentId], references: [id]) + children Node[] @relation("Tree") + + @@allow('create', true) + // readable if it has two distinct children with the same value + @@allow('read', children?[c, this.children?[id != c.id && value == c.value]]) +} +`, + ); + const raw = db.$unuseAll(); + await raw.node.create({ data: { id: 1, value: 0 } }); + await raw.node.create({ data: { id: 2, value: 5, parentId: 1 } }); + await raw.node.create({ data: { id: 3, value: 5, parentId: 1 } }); + await raw.node.create({ data: { id: 4, value: 0 } }); + await raw.node.create({ data: { id: 5, value: 1, parentId: 4 } }); + await raw.node.create({ data: { id: 6, value: 2, parentId: 4 } }); + + await expect(db.node.findMany()).resolves.toEqual([expect.objectContaining({ id: 1 })]); + }); + + it('works with binding-rooted self relation nested inside a collection predicate', async () => { + const db = await createPolicyTestClient( + ` +model Node { + id Int @id + value Int + parentId Int? + parent Node? @relation("Tree", fields: [parentId], references: [id]) + children Node[] @relation("Tree") + + @@allow('create', true) + // readable if it has a child that has two distinct children with the same value + @@allow('read', children?[c, c.children?[d, c.children?[id != d.id && value == d.value]]]) +} +`, + ); + const raw = db.$unuseAll(); + await raw.node.create({ data: { id: 1, value: 0 } }); + await raw.node.create({ data: { id: 2, value: 0, parentId: 1 } }); + await raw.node.create({ data: { id: 3, value: 7, parentId: 2 } }); + await raw.node.create({ data: { id: 4, value: 7, parentId: 2 } }); + await raw.node.create({ data: { id: 5, value: 0 } }); + await raw.node.create({ data: { id: 6, value: 0, parentId: 5 } }); + await raw.node.create({ data: { id: 7, value: 1, parentId: 6 } }); + await raw.node.create({ data: { id: 8, value: 2, parentId: 6 } }); + + await expect(db.node.findMany()).resolves.toEqual([expect.objectContaining({ id: 1 })]); + }); + + it('works with self many-to-many relation', async () => { + const db = await createPolicyTestClient( + ` +model User { + id Int @id + name String + friends User[] @relation("Friends") + friendOf User[] @relation("Friends") + + @@allow('create', true) + @@allow('read', friends?[name == this.name]) +} +`, + { usePrismaPush: true }, + ); + const raw = db.$unuseAll(); + await raw.user.create({ data: { id: 1, name: 'x' } }); + await raw.user.create({ data: { id: 2, name: 'x', friends: { connect: { id: 1 } } } }); + await raw.user.create({ data: { id: 3, name: 'y', friends: { connect: { id: 1 } } } }); + + // 2 has friend 1 with same name -> readable; 3's friend 1 has a different name; 1 has no friends + await expect(db.user.findMany()).resolves.toEqual([expect.objectContaining({ id: 2 })]); + }); +});