From 1134cb9d55975b0db5ec8ebe38604f94efe55c50 Mon Sep 17 00:00:00 2001 From: Lukas Kahwe Smith Date: Tue, 19 May 2026 19:28:26 +0200 Subject: [PATCH] fix(orm): restore HasMany includes when PK/FK has a field access policy (#2674) --- .../dialects/lateral-join-dialect-base.ts | 8 +- .../orm/src/client/crud/dialects/sqlite.ts | 22 +- packages/orm/src/client/query-utils.ts | 74 +++- packages/plugins/policy/src/policy-handler.ts | 13 + tests/regression/test/issue-2674.test.ts | 340 ++++++++++++++++++ 5 files changed, 444 insertions(+), 13 deletions(-) create mode 100644 tests/regression/test/issue-2674.test.ts diff --git a/packages/orm/src/client/crud/dialects/lateral-join-dialect-base.ts b/packages/orm/src/client/crud/dialects/lateral-join-dialect-base.ts index e4c13fe58..bf0f4dc91 100644 --- a/packages/orm/src/client/crud/dialects/lateral-join-dialect-base.ts +++ b/packages/orm/src/client/crud/dialects/lateral-join-dialect-base.ts @@ -9,6 +9,7 @@ import { getDelegateDescendantModels, getManyToManyRelation, isRelationField, + joinKeyRef, requireField, requireIdFields, requireModel, @@ -139,14 +140,17 @@ export abstract class LateralJoinDialectBase extends B const relationIds = requireIdFields(this.schema, relationModel); invariant(parentIds.length === 1, 'many-to-many relation must have exactly one id field'); invariant(relationIds.length === 1, 'many-to-many relation must have exactly one id field'); + // Use raw-alias refs so field access policies wrapping PK/FK in CASE WHEN NULL do not break the join. + const relationIdRef = joinKeyRef(this.schema, relationModel, relationModelAlias, relationIds[0]!); + const parentIdRef = joinKeyRef(this.schema, model, parentAlias, parentIds[0]!); query = query.where((eb) => eb( - eb.ref(`${relationModelAlias}.${relationIds[0]}`), + eb.ref(relationIdRef), 'in', eb .selectFrom(m2m.joinTable) .select(`${m2m.joinTable}.${m2m.otherFkName}`) - .whereRef(`${parentAlias}.${parentIds[0]}`, '=', `${m2m.joinTable}.${m2m.parentFkName}`), + .whereRef(parentIdRef, '=', `${m2m.joinTable}.${m2m.parentFkName}`), ), ); } else { diff --git a/packages/orm/src/client/crud/dialects/sqlite.ts b/packages/orm/src/client/crud/dialects/sqlite.ts index 48418072c..9a94c6bb0 100644 --- a/packages/orm/src/client/crud/dialects/sqlite.ts +++ b/packages/orm/src/client/crud/dialects/sqlite.ts @@ -21,6 +21,7 @@ import { getDelegateDescendantModels, getManyToManyRelation, getRelationForeignKeyFieldPairs, + joinKeyRef, requireField, requireIdFields, requireModel, @@ -359,32 +360,39 @@ export class SqliteCrudDialect extends BaseCrudDialect const relationIds = requireIdFields(this.schema, relationModel); invariant(parentIds.length === 1, 'many-to-many relation must have exactly one id field'); invariant(relationIds.length === 1, 'many-to-many relation must have exactly one id field'); + // Use raw-alias refs so field access policies wrapping PK/FK in CASE WHEN NULL do not break the join. + const relationIdRef = joinKeyRef(this.schema, relationModel, relationModelAlias, relationIds[0]!); + const parentIdRef = joinKeyRef(this.schema, model, parentAlias, parentIds[0]!); selectModelQuery = selectModelQuery.where((eb) => eb( - eb.ref(`${relationModelAlias}.${relationIds[0]}`), + eb.ref(relationIdRef), 'in', eb .selectFrom(m2m.joinTable) .select(`${m2m.joinTable}.${m2m.otherFkName}`) - .whereRef(`${parentAlias}.${parentIds[0]}`, '=', `${m2m.joinTable}.${m2m.parentFkName}`), + .whereRef(parentIdRef, '=', `${m2m.joinTable}.${m2m.parentFkName}`), ), ); } else { const { keyPairs, ownedByModel } = getRelationForeignKeyFieldPairs(this.schema, model, relationField); keyPairs.forEach(({ fk, pk }) => { if (ownedByModel) { - // the parent model owns the fk + // BelongsTo: model owns the FK, relation has the PK. + // Use raw alias only for the PK side. The FK side stays as a plain ref so that + // denying the FK intentionally hides the relation (the join evaluates to NULL). selectModelQuery = selectModelQuery.whereRef( - `${relationModelAlias}.${pk}`, + joinKeyRef(this.schema, relationModel, relationModelAlias, pk), '=', `${parentAlias}.${fk}`, ); } else { - // the relation side owns the fk + // HasMany: relation owns the FK, model has the PK. + // Use raw alias on both sides: the child's FK may be denied (to hide which parent + // it belongs to) but the parent must still be able to fetch its children. selectModelQuery = selectModelQuery.whereRef( - `${relationModelAlias}.${fk}`, + joinKeyRef(this.schema, relationModel, relationModelAlias, fk), '=', - `${parentAlias}.${pk}`, + joinKeyRef(this.schema, model, parentAlias, pk), ); } }); diff --git a/packages/orm/src/client/query-utils.ts b/packages/orm/src/client/query-utils.ts index 9fc830a25..97ac7e7d5 100644 --- a/packages/orm/src/client/query-utils.ts +++ b/packages/orm/src/client/query-utils.ts @@ -234,6 +234,61 @@ export function isTypeDef(schema: SchemaDef, type: string) { return !!schema.typeDefs?.[type]; } +/** + * Prefix added to PK/FK columns in field-policy subqueries so join conditions + * can reference the raw (never-nulled) value even when the field itself is + * covered by a @deny rule that wraps it in CASE WHEN … THEN NULL. + */ +export const JOIN_KEY_RAW_PREFIX = '__zs_raw_'; + +/** + * Returns true when the field has at least one @deny or @allow attribute that applies to + * read operations AND whose condition is not a compile-time constant that collapses the + * filter to trivially true (i.e. @allow('read', true) or @deny('read', false)). + * + * Only in those cases does createSelectAllFieldsWithPolicies emit a CASE WHEN … THEN NULL + * expression together with a __zs_raw_* alias that joinKeyRef can reference. + * + * Known limitation: this is a static schema analysis and cannot detect auth-context-dependent + * conditions that happen to collapse to trueNode at runtime (e.g. @allow('read', auth() == null) + * for a null-auth caller). In that scenario this function returns true, but the policy handler + * emits no CASE WHEN and no raw alias, so the join would reference a non-existent column. In + * practice, applying a conditional read policy to a PK/FK that is only meaningful for null auth + * is an extremely unusual pattern and is not currently supported. + */ +export function fieldHasConditionalReadPolicy(schema: SchemaDef, model: string, field: string): boolean { + const fieldDef = getField(schema, model, field); + return ( + fieldDef?.attributes?.some((attr) => { + if (attr.name !== '@deny' && attr.name !== '@allow') return false; + const opArg = attr.args?.[0]?.value; + if (!ExpressionUtils.isLiteral(opArg) || typeof opArg.value !== 'string') return false; + const ops = opArg.value.split(',').map((v) => v.trim()); + if (!ops.includes('all') && !ops.includes('read')) return false; + // Constant conditions that make buildFieldPolicyFilter return trueNode produce no + // CASE WHEN and therefore no raw alias – skip them. + const condArg = attr.args?.[1]?.value; + if (ExpressionUtils.isLiteral(condArg)) { + if (attr.name === '@allow' && condArg.value === true) return false; + if (attr.name === '@deny' && condArg.value === false) return false; + } + return true; + }) ?? false + ); +} + +/** + * Returns the column reference to use on the given side of a join condition. + * When the field has a non-trivial read policy the policy handler emits a raw alias + * (JOIN_KEY_RAW_PREFIX + fieldName) alongside the CASE WHEN expression so the join + * is not broken by the nulled value. + */ +export function joinKeyRef(schema: SchemaDef, model: string, tableAlias: string, field: string): string { + return fieldHasConditionalReadPolicy(schema, model, field) + ? `${tableAlias}.${JOIN_KEY_RAW_PREFIX}${field}` + : `${tableAlias}.${field}`; +} + export function buildJoinPairs( schema: SchemaDef, model: string, @@ -242,14 +297,25 @@ export function buildJoinPairs( relationModelAlias: string, ): [string, string][] { const { keyPairs, ownedByModel } = getRelationForeignKeyFieldPairs(schema, model, relationField); + const relationModel = requireField(schema, model, relationField).type; return keyPairs.map(({ fk, pk }) => { if (ownedByModel) { - // the parent model owns the fk - return [`${relationModelAlias}.${pk}`, `${modelAlias}.${fk}`]; + // BelongsTo: model owns the FK, relation has the PK. + // Use raw alias only for the PK side. The FK side stays as a plain ref so that + // denying the FK intentionally hides the relation (the join evaluates to NULL). + return [ + joinKeyRef(schema, relationModel, relationModelAlias, pk), + `${modelAlias}.${fk}`, + ]; } else { - // the relation side owns the fk - return [`${relationModelAlias}.${fk}`, `${modelAlias}.${pk}`]; + // HasMany: relation owns the FK, model has the PK. + // Use raw alias on both sides: the child's FK may be denied (to hide which parent it + // belongs to) but the parent must still be able to fetch its children. + return [ + joinKeyRef(schema, relationModel, relationModelAlias, fk), + joinKeyRef(schema, model, modelAlias, pk), + ]; } }); } diff --git a/packages/plugins/policy/src/policy-handler.ts b/packages/plugins/policy/src/policy-handler.ts index 93c6c334a..45d0d9bd3 100644 --- a/packages/plugins/policy/src/policy-handler.ts +++ b/packages/plugins/policy/src/policy-handler.ts @@ -697,6 +697,19 @@ export class PolicyHandler extends OperationNodeTransf ); hasPolicies = hasPolicies || fieldHasPolicies; selections.push(selection); + + // When a PK or FK field is wrapped in CASE WHEN … THEN NULL by a field access policy, + // also expose the raw value under a stable alias so join conditions are not broken. + // Used for PK (HasMany parent) and FK (HasMany child). BelongsTo parent FK is kept as + // a plain ref intentionally: denying that FK is designed to hide the relation entirely. + if (fieldHasPolicies && (fieldDef.id || fieldDef.foreignKeyFor)) { + const rawAlias = `${QueryUtils.JOIN_KEY_RAW_PREFIX}${fieldDef.name}`; + selections.push( + SelectionNode.create( + AliasNode.create(ColumnNode.create(fieldDef.name), IdentifierNode.create(rawAlias)), + ), + ); + } } if (!hasPolicies) { diff --git a/tests/regression/test/issue-2674.test.ts b/tests/regression/test/issue-2674.test.ts new file mode 100644 index 000000000..4c175232f --- /dev/null +++ b/tests/regression/test/issue-2674.test.ts @@ -0,0 +1,340 @@ +import { createPolicyTestClient } from '@zenstackhq/testtools'; +import { describe, expect, it } from 'vitest'; + +// https://github.com/zenstackhq/zenstack/issues/2674 +describe('Regression for issue #2674', () => { + it('@deny on PK should not break include (HasMany)', async () => { + const db = await createPolicyTestClient( + ` +model User { + id String @id @default(cuid()) + role String + @@allow('all', true) +} + +model Post { + id Int @id @default(autoincrement()) @deny('all', auth().role != 'ADMIN') + title String @unique + comments Comment[] + @@allow('all', true) +} + +model Comment { + id Int @id @default(autoincrement()) + content String + postId Int + post Post @relation(fields: [postId], references: [id]) + @@allow('all', true) +} + `, + ); + + const admin = { id: 'admin-1', role: 'ADMIN' }; + const user = { id: 'user-1', role: 'USER' }; + + await db.$setAuth(admin).post.create({ + data: { + title: 'Test Post', + comments: { create: [{ content: 'Comment 1' }, { content: 'Comment 2' }] }, + }, + }); + + // Admin sees everything normally + const adminResult = await db.$setAuth(admin).post.findUnique({ + where: { title: 'Test Post' }, + include: { comments: true }, + }); + expect(adminResult?.id).toBeGreaterThan(0); + expect(adminResult?.comments).toHaveLength(2); + + // USER role: id is denied (returns null) but include should still populate comments + const userResult = await db.$setAuth(user).post.findUnique({ + where: { title: 'Test Post' }, + include: { comments: true }, + }); + expect(userResult?.id).toBeNull(); + expect(userResult?.comments).toHaveLength(2); + }); + + it('@deny on FK hides the BelongsTo relation (by design)', async () => { + const db = await createPolicyTestClient( + ` +model User { + id String @id @default(cuid()) + role String + @@allow('all', true) +} + +model Post { + id Int @id @default(autoincrement()) + title String + comments Comment[] + @@allow('all', true) +} + +model Comment { + id Int @id @default(autoincrement()) + content String + postId Int @deny('all', auth().role != 'ADMIN') + post Post @relation(fields: [postId], references: [id]) + @@allow('all', true) +} + `, + ); + + const admin = { id: 'admin-1', role: 'ADMIN' }; + const user = { id: 'user-1', role: 'USER' }; + + const post = await db.$setAuth(admin).post.create({ + data: { title: 'Test Post' }, + }); + + const comment = await db.$setAuth(admin).comment.create({ + data: { content: 'A comment', postId: post.id }, + }); + + // Positive control: ADMIN sees the FK and resolves the relation + const adminResult = await db.$setAuth(admin).comment.findUnique({ + where: { id: comment.id }, + include: { post: true }, + }); + expect(adminResult?.postId).toBe(post.id); + expect(adminResult?.post).not.toBeNull(); + + // USER role: postId is denied — both the FK value and the relation are hidden (by design) + const userResult = await db.$setAuth(user).comment.findUnique({ + where: { id: comment.id }, + include: { post: true }, + }); + expect(userResult?.postId).toBeNull(); + expect(userResult?.post).toBeNull(); + }); + + it('@deny on both PK and FK should not break include', async () => { + const db = await createPolicyTestClient( + ` +model User { + id String @id @default(cuid()) + role String + @@allow('all', true) +} + +model Post { + id Int @id @default(autoincrement()) @deny('all', auth().role != 'ADMIN') + title String @unique + comments Comment[] + @@allow('all', true) +} + +model Comment { + id Int @id @default(autoincrement()) + content String + postId Int @deny('all', auth().role != 'ADMIN') + post Post @relation(fields: [postId], references: [id]) + @@allow('all', true) +} + `, + ); + + const admin = { id: 'admin-1', role: 'ADMIN' }; + const user = { id: 'user-1', role: 'USER' }; + + await db.$setAuth(admin).post.create({ + data: { + title: 'Test Post', + comments: { create: [{ content: 'C1' }, { content: 'C2' }] }, + }, + }); + + // Positive control: admin sees real id and two comments + const adminResult = await db.$setAuth(admin).post.findUnique({ + where: { title: 'Test Post' }, + include: { comments: true }, + }); + expect(adminResult?.id).toBeGreaterThan(0); + expect(adminResult?.comments).toHaveLength(2); + + // Find by non-denied field (title) so the WHERE is not affected by @deny on id + const userResult = await db.$setAuth(user).post.findUnique({ + where: { title: 'Test Post' }, + include: { comments: true }, + }); + expect(userResult?.id).toBeNull(); + expect(userResult?.comments).toHaveLength(2); + expect(userResult!.comments.every((c: { postId: number | null }) => c.postId === null)).toBe(true); + }); + + it('update-only @deny on PK/FK should not affect read joins', async () => { + // @deny('update', ...) must not cause joinKeyRef to emit a __zs_raw_* alias that + // was never projected (only read-scoped policies produce a CASE WHEN on select). + const db = await createPolicyTestClient( + ` +model User { + id String @id @default(cuid()) + role String + @@allow('all', true) +} + +model Post { + id Int @id @default(autoincrement()) @deny('update', auth().role != 'ADMIN') + title String @unique + comments Comment[] + @@allow('all', true) +} + +model Comment { + id Int @id @default(autoincrement()) + content String + postId Int @deny('update', auth().role != 'ADMIN') + post Post @relation(fields: [postId], references: [id]) + @@allow('all', true) +} + `, + ); + + const admin = { id: 'admin-1', role: 'ADMIN' }; + const user = { id: 'user-1', role: 'USER' }; + + await db.$setAuth(admin).post.create({ + data: { + title: 'Test Post', + comments: { create: [{ content: 'C1' }, { content: 'C2' }] }, + }, + }); + + const adminResult = await db.$setAuth(admin).post.findUnique({ + where: { title: 'Test Post' }, + include: { comments: true }, + }); + expect(adminResult?.id).toBeGreaterThan(0); + expect(adminResult?.comments).toHaveLength(2); + + // Non-admin: update-scoped @deny must not affect read-time join resolution + const userResult = await db.$setAuth(user).post.findUnique({ + where: { title: 'Test Post' }, + include: { comments: true }, + }); + expect(userResult?.id).toBeGreaterThan(0); + expect(userResult?.comments).toHaveLength(2); + }); + + it('@allow("read", true) on PK/FK should not emit a missing raw alias', async () => { + // @allow('read', true) collapses to a trivially-true filter so no CASE WHEN and no + // __zs_raw_* alias is projected. joinKeyRef must use the plain column ref in this case. + const db = await createPolicyTestClient( + ` +model User { + id String @id @default(cuid()) + role String + @@allow('all', true) +} + +model Post { + id Int @id @default(autoincrement()) @allow('read', true) + title String @unique + comments Comment[] + @@allow('all', true) +} + +model Comment { + id Int @id @default(autoincrement()) + content String + postId Int @allow('read', true) + post Post @relation(fields: [postId], references: [id]) + @@allow('all', true) +} + `, + ); + + const admin = { id: 'admin-1', role: 'ADMIN' }; + const user = { id: 'user-1', role: 'USER' }; + + await db.$setAuth(admin).post.create({ + data: { + title: 'Test Post', + comments: { create: [{ content: 'C1' }, { content: 'C2' }] }, + }, + }); + + const adminResult = await db.$setAuth(admin).post.findUnique({ + where: { title: 'Test Post' }, + include: { comments: true }, + }); + expect(adminResult?.id).toBeGreaterThan(0); + expect(adminResult?.comments).toHaveLength(2); + + // @allow('read', true) is a no-op policy; USER should see the same results + const userResult = await db.$setAuth(user).post.findUnique({ + where: { title: 'Test Post' }, + include: { comments: true }, + }); + expect(userResult?.id).toBeGreaterThan(0); + expect(userResult?.comments).toHaveLength(2); + }); + + it('@deny on PK should not break many-to-many include', async () => { + // M2M join filtering uses joinKeyRef for both model PKs; verify the raw alias path is + // exercised correctly in both the SQLite and lateral-join dialect M2M branches. + // Explicit join model is required because SchemaDbPusher does not create implicit join tables. + const db = await createPolicyTestClient( + ` +model User { + id String @id @default(cuid()) + role String + @@allow('all', true) +} + +model Post { + id Int @id @default(autoincrement()) @deny('all', auth().role != 'ADMIN') + title String @unique + tags PostTag[] + @@allow('all', true) +} + +model Tag { + id Int @id @default(autoincrement()) + name String @unique + posts PostTag[] + @@allow('all', true) +} + +model PostTag { + postId Int + tagId Int + post Post @relation(fields: [postId], references: [id]) + tag Tag @relation(fields: [tagId], references: [id]) + @@id([postId, tagId]) + @@allow('all', true) +} + `, + ); + + const admin = { id: 'admin-1', role: 'ADMIN' }; + const user = { id: 'user-1', role: 'USER' }; + + const alpha = await db.$setAuth(admin).tag.create({ data: { name: 'alpha' } }); + const beta = await db.$setAuth(admin).tag.create({ data: { name: 'beta' } }); + await db.$setAuth(admin).post.create({ + data: { + title: 'Tagged Post', + tags: { create: [{ tagId: alpha.id }, { tagId: beta.id }] }, + }, + }); + + // Admin: PK policy compiles to a no-deny expression; raw alias still projected and join works + const adminResult = await db.$setAuth(admin).post.findUnique({ + where: { title: 'Tagged Post' }, + include: { tags: true }, + }); + expect(adminResult?.id).toBeGreaterThan(0); + expect(adminResult?.tags).toHaveLength(2); + + // User: PK is denied (returns null) but join through PostTag must still load the tags + const userResult = await db.$setAuth(user).post.findUnique({ + where: { title: 'Tagged Post' }, + include: { tags: true }, + }); + expect(userResult?.id).toBeNull(); + expect(userResult?.tags).toHaveLength(2); + }); +});