Skip to content

Commit 022e5dd

Browse files
committed
fix(orm): use raw alias for join keys when PK/FK has @Allow or @deny field policy (#2674)
1 parent ed01275 commit 022e5dd

5 files changed

Lines changed: 218 additions & 12 deletions

File tree

packages/orm/src/client/crud/dialects/lateral-join-dialect-base.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
getDelegateDescendantModels,
1010
getManyToManyRelation,
1111
isRelationField,
12+
joinKeyRef,
1213
requireField,
1314
requireIdFields,
1415
requireModel,
@@ -139,14 +140,17 @@ export abstract class LateralJoinDialectBase<Schema extends SchemaDef> extends B
139140
const relationIds = requireIdFields(this.schema, relationModel);
140141
invariant(parentIds.length === 1, 'many-to-many relation must have exactly one id field');
141142
invariant(relationIds.length === 1, 'many-to-many relation must have exactly one id field');
143+
// Use raw-alias refs so @deny CASE WHEN NULL on PK fields does not break the join.
144+
const relationIdRef = joinKeyRef(this.schema, relationModel, relationModelAlias, relationIds[0]!);
145+
const parentIdRef = joinKeyRef(this.schema, model, parentAlias, parentIds[0]!);
142146
query = query.where((eb) =>
143147
eb(
144-
eb.ref(`${relationModelAlias}.${relationIds[0]}`),
148+
eb.ref(relationIdRef),
145149
'in',
146150
eb
147151
.selectFrom(m2m.joinTable)
148152
.select(`${m2m.joinTable}.${m2m.otherFkName}`)
149-
.whereRef(`${parentAlias}.${parentIds[0]}`, '=', `${m2m.joinTable}.${m2m.parentFkName}`),
153+
.whereRef(parentIdRef, '=', `${m2m.joinTable}.${m2m.parentFkName}`),
150154
),
151155
);
152156
} else {

packages/orm/src/client/crud/dialects/sqlite.ts

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import {
2121
getDelegateDescendantModels,
2222
getManyToManyRelation,
2323
getRelationForeignKeyFieldPairs,
24+
joinKeyRef,
2425
requireField,
2526
requireIdFields,
2627
requireModel,
@@ -359,32 +360,35 @@ export class SqliteCrudDialect<Schema extends SchemaDef> extends BaseCrudDialect
359360
const relationIds = requireIdFields(this.schema, relationModel);
360361
invariant(parentIds.length === 1, 'many-to-many relation must have exactly one id field');
361362
invariant(relationIds.length === 1, 'many-to-many relation must have exactly one id field');
363+
// Use raw-alias refs so @deny CASE WHEN NULL on PK fields does not break the join.
364+
const relationIdRef = joinKeyRef(this.schema, relationModel, relationModelAlias, relationIds[0]!);
365+
const parentIdRef = joinKeyRef(this.schema, model, parentAlias, parentIds[0]!);
362366
selectModelQuery = selectModelQuery.where((eb) =>
363367
eb(
364-
eb.ref(`${relationModelAlias}.${relationIds[0]}`),
368+
eb.ref(relationIdRef),
365369
'in',
366370
eb
367371
.selectFrom(m2m.joinTable)
368372
.select(`${m2m.joinTable}.${m2m.otherFkName}`)
369-
.whereRef(`${parentAlias}.${parentIds[0]}`, '=', `${m2m.joinTable}.${m2m.parentFkName}`),
373+
.whereRef(parentIdRef, '=', `${m2m.joinTable}.${m2m.parentFkName}`),
370374
),
371375
);
372376
} else {
373377
const { keyPairs, ownedByModel } = getRelationForeignKeyFieldPairs(this.schema, model, relationField);
374378
keyPairs.forEach(({ fk, pk }) => {
375379
if (ownedByModel) {
376-
// the parent model owns the fk
380+
// the parent model owns the fk; use raw alias when @deny is present on either side
377381
selectModelQuery = selectModelQuery.whereRef(
378-
`${relationModelAlias}.${pk}`,
382+
joinKeyRef(this.schema, relationModel, relationModelAlias, pk),
379383
'=',
380-
`${parentAlias}.${fk}`,
384+
joinKeyRef(this.schema, model, parentAlias, fk),
381385
);
382386
} else {
383-
// the relation side owns the fk
387+
// the relation side owns the fk; use raw alias when @deny is present on either side
384388
selectModelQuery = selectModelQuery.whereRef(
385-
`${relationModelAlias}.${fk}`,
389+
joinKeyRef(this.schema, relationModel, relationModelAlias, fk),
386390
'=',
387-
`${parentAlias}.${pk}`,
391+
joinKeyRef(this.schema, model, parentAlias, pk),
388392
);
389393
}
390394
});

packages/orm/src/client/query-utils.ts

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,34 @@ export function isTypeDef(schema: SchemaDef, type: string) {
234234
return !!schema.typeDefs?.[type];
235235
}
236236

237+
/**
238+
* Prefix added to PK/FK columns in field-policy subqueries so join conditions
239+
* can reference the raw (never-nulled) value even when the field itself is
240+
* covered by a @deny rule that wraps it in CASE WHEN … THEN NULL.
241+
*/
242+
export const JOIN_KEY_RAW_PREFIX = '__zs_raw_';
243+
244+
/**
245+
* Returns true when the field has at least one @deny or @allow attribute declared,
246+
* meaning the policy handler may wrap it in CASE WHEN … THEN NULL in SELECT
247+
* subqueries and the join condition must use the raw-alias instead.
248+
*/
249+
export function fieldHasConditionalReadPolicy(schema: SchemaDef, model: string, field: string): boolean {
250+
const fieldDef = getField(schema, model, field);
251+
return fieldDef?.attributes?.some((a) => a.name === '@deny' || a.name === '@allow') ?? false;
252+
}
253+
254+
/**
255+
* Returns the column reference to use on the given side of a join condition.
256+
* When the field carries a @deny or @allow policy the policy handler adds a raw alias
257+
* (JOIN_KEY_RAW_PREFIX + fieldName) so the join is not broken by CASE WHEN NULL.
258+
*/
259+
export function joinKeyRef(schema: SchemaDef, model: string, tableAlias: string, field: string): string {
260+
return fieldHasConditionalReadPolicy(schema, model, field)
261+
? `${tableAlias}.${JOIN_KEY_RAW_PREFIX}${field}`
262+
: `${tableAlias}.${field}`;
263+
}
264+
237265
export function buildJoinPairs(
238266
schema: SchemaDef,
239267
model: string,
@@ -242,14 +270,21 @@ export function buildJoinPairs(
242270
relationModelAlias: string,
243271
): [string, string][] {
244272
const { keyPairs, ownedByModel } = getRelationForeignKeyFieldPairs(schema, model, relationField);
273+
const relationModel = requireField(schema, model, relationField).type;
245274

246275
return keyPairs.map(({ fk, pk }) => {
247276
if (ownedByModel) {
248277
// the parent model owns the fk
249-
return [`${relationModelAlias}.${pk}`, `${modelAlias}.${fk}`];
278+
return [
279+
joinKeyRef(schema, relationModel, relationModelAlias, pk),
280+
joinKeyRef(schema, model, modelAlias, fk),
281+
];
250282
} else {
251283
// the relation side owns the fk
252-
return [`${relationModelAlias}.${fk}`, `${modelAlias}.${pk}`];
284+
return [
285+
joinKeyRef(schema, relationModel, relationModelAlias, fk),
286+
joinKeyRef(schema, model, modelAlias, pk),
287+
];
253288
}
254289
});
255290
}

packages/plugins/policy/src/policy-handler.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -697,6 +697,18 @@ export class PolicyHandler<Schema extends SchemaDef> extends OperationNodeTransf
697697
);
698698
hasPolicies = hasPolicies || fieldHasPolicies;
699699
selections.push(selection);
700+
701+
// When a PK or FK field is wrapped in CASE WHEN … THEN NULL by a @deny policy,
702+
// also expose the raw value under a stable alias so lateral join conditions are
703+
// not broken (they reference this alias instead of the potentially-nulled column).
704+
if (fieldHasPolicies && (fieldDef.id || fieldDef.foreignKeyFor)) {
705+
const rawAlias = `${QueryUtils.JOIN_KEY_RAW_PREFIX}${fieldDef.name}`;
706+
selections.push(
707+
SelectionNode.create(
708+
AliasNode.create(ColumnNode.create(fieldDef.name), IdentifierNode.create(rawAlias)),
709+
),
710+
);
711+
}
700712
}
701713

702714
if (!hasPolicies) {
Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
import { createPolicyTestClient } from '@zenstackhq/testtools';
2+
import { describe, expect, it } from 'vitest';
3+
4+
// https://github.com/zenstackhq/zenstack/issues/2674
5+
describe('Regression for issue #2674', () => {
6+
it('@deny on PK should not break include (HasMany)', async () => {
7+
const db = await createPolicyTestClient(
8+
`
9+
model User {
10+
id String @id @default(cuid())
11+
role String
12+
@@allow('all', true)
13+
}
14+
15+
model Post {
16+
id Int @id @default(autoincrement()) @deny('all', auth().role != 'ADMIN')
17+
title String @unique
18+
comments Comment[]
19+
@@allow('all', true)
20+
}
21+
22+
model Comment {
23+
id Int @id @default(autoincrement())
24+
content String
25+
postId Int
26+
post Post @relation(fields: [postId], references: [id])
27+
@@allow('all', true)
28+
}
29+
`,
30+
);
31+
32+
const admin = { id: 'admin-1', role: 'ADMIN' };
33+
const user = { id: 'user-1', role: 'USER' };
34+
35+
await db.$setAuth(admin).post.create({
36+
data: {
37+
title: 'Test Post',
38+
comments: { create: [{ content: 'Comment 1' }, { content: 'Comment 2' }] },
39+
},
40+
});
41+
42+
// Admin sees everything normally
43+
const adminResult = await db.$setAuth(admin).post.findUnique({
44+
where: { title: 'Test Post' },
45+
include: { comments: true },
46+
});
47+
expect(adminResult?.id).toBeGreaterThan(0);
48+
expect(adminResult?.comments).toHaveLength(2);
49+
50+
// USER role: id is denied (returns null) but include should still populate comments
51+
const userResult = await db.$setAuth(user).post.findUnique({
52+
where: { title: 'Test Post' },
53+
include: { comments: true },
54+
});
55+
expect(userResult?.id).toBeNull();
56+
expect(userResult?.comments).toHaveLength(2);
57+
});
58+
59+
it('@deny on FK should not break include (BelongsTo)', async () => {
60+
const db = await createPolicyTestClient(
61+
`
62+
model User {
63+
id String @id @default(cuid())
64+
role String
65+
@@allow('all', true)
66+
}
67+
68+
model Post {
69+
id Int @id @default(autoincrement())
70+
title String
71+
comments Comment[]
72+
@@allow('all', true)
73+
}
74+
75+
model Comment {
76+
id Int @id @default(autoincrement())
77+
content String
78+
postId Int @deny('all', auth().role != 'ADMIN')
79+
post Post @relation(fields: [postId], references: [id])
80+
@@allow('all', true)
81+
}
82+
`,
83+
);
84+
85+
const admin = { id: 'admin-1', role: 'ADMIN' };
86+
const user = { id: 'user-1', role: 'USER' };
87+
88+
const post = await db.$setAuth(admin).post.create({
89+
data: { title: 'Test Post' },
90+
});
91+
92+
const comment = await db.$setAuth(admin).comment.create({
93+
data: { content: 'A comment', postId: post.id },
94+
});
95+
96+
// USER role: postId is denied (returns null) but include should still return the post
97+
const userResult = await db.$setAuth(user).comment.findUnique({
98+
where: { id: comment.id },
99+
include: { post: true },
100+
});
101+
expect(userResult?.postId).toBeNull();
102+
expect(userResult?.post).not.toBeNull();
103+
expect(userResult?.post?.title).toBe('Test Post');
104+
});
105+
106+
it('@deny on both PK and FK should not break include', async () => {
107+
const db = await createPolicyTestClient(
108+
`
109+
model User {
110+
id String @id @default(cuid())
111+
role String
112+
@@allow('all', true)
113+
}
114+
115+
model Post {
116+
id Int @id @default(autoincrement()) @deny('all', auth().role != 'ADMIN')
117+
title String @unique
118+
comments Comment[]
119+
@@allow('all', true)
120+
}
121+
122+
model Comment {
123+
id Int @id @default(autoincrement())
124+
content String
125+
postId Int @deny('all', auth().role != 'ADMIN')
126+
post Post @relation(fields: [postId], references: [id])
127+
@@allow('all', true)
128+
}
129+
`,
130+
);
131+
132+
const admin = { id: 'admin-1', role: 'ADMIN' };
133+
const user = { id: 'user-1', role: 'USER' };
134+
135+
await db.$setAuth(admin).post.create({
136+
data: {
137+
title: 'Test Post',
138+
comments: { create: [{ content: 'C1' }, { content: 'C2' }] },
139+
},
140+
});
141+
142+
// Find by non-denied field (title) so the WHERE is not affected by @deny on id
143+
const userResult = await db.$setAuth(user).post.findUnique({
144+
where: { title: 'Test Post' },
145+
include: { comments: true },
146+
});
147+
expect(userResult?.id).toBeNull();
148+
expect(userResult?.comments).toHaveLength(2);
149+
expect(userResult?.comments[0]?.postId).toBeNull();
150+
});
151+
});

0 commit comments

Comments
 (0)