From d921a7ca6bef0341ccf5bc50e195156695129e7f Mon Sep 17 00:00:00 2001 From: Yiming Date: Mon, 13 Nov 2023 16:26:40 -0800 Subject: [PATCH 1/2] fix: post-update rules incorrectly reject update (#826) --- package.json | 2 +- packages/language/package.json | 2 +- packages/plugins/openapi/package.json | 2 +- packages/plugins/swr/package.json | 2 +- packages/plugins/tanstack-query/package.json | 2 +- packages/plugins/trpc/package.json | 2 +- packages/runtime/package.json | 2 +- packages/schema/package.json | 2 +- .../access-policy/expression-writer.ts | 86 ++++++---- .../access-policy/policy-guard-generator.ts | 16 +- .../typescript-expression-transformer.ts | 22 ++- packages/sdk/package.json | 2 +- packages/server/package.json | 2 +- packages/testtools/package.json | 2 +- packages/testtools/src/schema.ts | 11 ++ .../with-policy/post-update.test.ts | 162 ++++++++++++++++++ .../enhancements/with-policy/refactor.test.ts | 2 +- .../tests/regression/issue-814.test.ts | 40 +++++ .../tests/regression/issue-825.test.ts | 39 +++++ tests/integration/tests/schema/todo.zmodel | 2 +- 20 files changed, 349 insertions(+), 53 deletions(-) create mode 100644 tests/integration/tests/regression/issue-814.test.ts create mode 100644 tests/integration/tests/regression/issue-825.test.ts diff --git a/package.json b/package.json index 9d9b9fab3..032b5e378 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "zenstack-monorepo", - "version": "1.2.1", + "version": "1.2.2", "description": "", "scripts": { "build": "pnpm -r build", diff --git a/packages/language/package.json b/packages/language/package.json index 4aff10044..8e0a9bab8 100644 --- a/packages/language/package.json +++ b/packages/language/package.json @@ -1,6 +1,6 @@ { "name": "@zenstackhq/language", - "version": "1.2.1", + "version": "1.2.2", "displayName": "ZenStack modeling language compiler", "description": "ZenStack modeling language compiler", "homepage": "https://zenstack.dev", diff --git a/packages/plugins/openapi/package.json b/packages/plugins/openapi/package.json index 01ca84dab..1b7c38a5a 100644 --- a/packages/plugins/openapi/package.json +++ b/packages/plugins/openapi/package.json @@ -1,7 +1,7 @@ { "name": "@zenstackhq/openapi", "displayName": "ZenStack Plugin and Runtime for OpenAPI", - "version": "1.2.1", + "version": "1.2.2", "description": "ZenStack plugin and runtime supporting OpenAPI", "main": "index.js", "repository": { diff --git a/packages/plugins/swr/package.json b/packages/plugins/swr/package.json index cd4d0b4f8..3e89b7024 100644 --- a/packages/plugins/swr/package.json +++ b/packages/plugins/swr/package.json @@ -1,7 +1,7 @@ { "name": "@zenstackhq/swr", "displayName": "ZenStack plugin for generating SWR hooks", - "version": "1.2.1", + "version": "1.2.2", "description": "ZenStack plugin for generating SWR hooks", "main": "index.js", "repository": { diff --git a/packages/plugins/tanstack-query/package.json b/packages/plugins/tanstack-query/package.json index c5a903fc5..c75983a71 100644 --- a/packages/plugins/tanstack-query/package.json +++ b/packages/plugins/tanstack-query/package.json @@ -1,7 +1,7 @@ { "name": "@zenstackhq/tanstack-query", "displayName": "ZenStack plugin for generating tanstack-query hooks", - "version": "1.2.1", + "version": "1.2.2", "description": "ZenStack plugin for generating tanstack-query hooks", "main": "index.js", "exports": { diff --git a/packages/plugins/trpc/package.json b/packages/plugins/trpc/package.json index b20fa8240..2c420b323 100644 --- a/packages/plugins/trpc/package.json +++ b/packages/plugins/trpc/package.json @@ -1,7 +1,7 @@ { "name": "@zenstackhq/trpc", "displayName": "ZenStack plugin for tRPC", - "version": "1.2.1", + "version": "1.2.2", "description": "ZenStack plugin for tRPC", "main": "index.js", "repository": { diff --git a/packages/runtime/package.json b/packages/runtime/package.json index a47c314ff..9fac34b2a 100644 --- a/packages/runtime/package.json +++ b/packages/runtime/package.json @@ -1,7 +1,7 @@ { "name": "@zenstackhq/runtime", "displayName": "ZenStack Runtime Library", - "version": "1.2.1", + "version": "1.2.2", "description": "Runtime of ZenStack for both client-side and server-side environments.", "repository": { "type": "git", diff --git a/packages/schema/package.json b/packages/schema/package.json index fb88a1a23..1564eb2cd 100644 --- a/packages/schema/package.json +++ b/packages/schema/package.json @@ -3,7 +3,7 @@ "publisher": "zenstack", "displayName": "ZenStack Language Tools", "description": "A toolkit for building secure CRUD apps with Next.js + Typescript", - "version": "1.2.1", + "version": "1.2.2", "author": { "name": "ZenStack Team" }, diff --git a/packages/schema/src/plugins/access-policy/expression-writer.ts b/packages/schema/src/plugins/access-policy/expression-writer.ts index f986d5d66..a88c1dc03 100644 --- a/packages/schema/src/plugins/access-policy/expression-writer.ts +++ b/packages/schema/src/plugins/access-policy/expression-writer.ts @@ -223,15 +223,28 @@ export class ExpressionWriter { } private writeCollectionPredicate(expr: BinaryExpr, operator: string) { - this.block(() => { - this.writeFieldCondition( - expr.left, - () => { - this.write(expr.right); - }, - operator === '?' ? 'some' : operator === '!' ? 'every' : 'none' - ); - }); + // check if the operand should be compiled to a relation query + // or a plain expression + const compileToRelationQuery = + (this.isPostGuard && this.isFutureMemberAccess(expr.left)) || + (!this.isPostGuard && !this.isFutureMemberAccess(expr.left)); + + if (compileToRelationQuery) { + this.block(() => { + this.writeFieldCondition( + expr.left, + () => { + // inner scope of collection expression is always compiled as non-post-guard + const innerWriter = new ExpressionWriter(this.writer, false); + innerWriter.write(expr.right); + }, + operator === '?' ? 'some' : operator === '!' ? 'every' : 'none' + ); + }); + } else { + const plain = this.plainExprBuilder.transform(expr); + this.writer.write(`${plain} ? ${TRUE} : ${FALSE}`); + } } private isFieldAccess(expr: Expression): boolean { @@ -275,6 +288,19 @@ export class ExpressionWriter { } } + private writeIdFieldsCheck(model: DataModel, value: Expression) { + const idFields = this.requireIdFields(model); + idFields.forEach((idField, idx) => { + // eg: id: user.id + this.writer.write(`${idField.name}:`); + this.plain(value); + this.writer.write(`.${idField.name}`); + if (idx !== idFields.length - 1) { + this.writer.write(','); + } + }); + } + private writeComparison(expr: BinaryExpr, operator: ComparisonOperator) { const leftIsFieldAccess = this.isFieldAccess(expr.left); const rightIsFieldAccess = this.isFieldAccess(expr.right); @@ -298,7 +324,7 @@ export class ExpressionWriter { operator = this.negateOperator(operator); } - if (isMemberAccessExpr(fieldAccess) && isFutureExpr(fieldAccess.operand)) { + if (this.isFutureMemberAccess(fieldAccess)) { // future().field should be treated as the "field" directly, so we // strip 'future().' and synthesize a reference expr fieldAccess = { @@ -338,8 +364,6 @@ export class ExpressionWriter { // right now this branch only serves comparison with `auth`, like // @@allow('all', owner == auth()) - const idFields = this.requireIdFields(dataModel); - if (operator !== '==' && operator !== '!=') { throw new PluginError(name, 'Only == and != operators are allowed'); } @@ -354,25 +378,13 @@ export class ExpressionWriter { } this.block(() => { - idFields.forEach((idField, idx) => { - const writeIdsCheck = () => { - // id: user.id - this.writer.write(`${idField.name}:`); - this.plain(operand); - this.writer.write(`.${idField.name}`); - if (idx !== idFields.length - 1) { - this.writer.write(','); - } - }; - - if (isThisExpr(fieldAccess) && operator === '!=') { - // wrap a not - this.writer.writeLine('NOT:'); - this.block(() => writeIdsCheck()); - } else { - writeIdsCheck(); - } - }); + if (isThisExpr(fieldAccess) && operator === '!=') { + // negate + this.writer.writeLine('isNot:'); + this.block(() => this.writeIdFieldsCheck(dataModel, operand)); + } else { + this.writeIdFieldsCheck(dataModel, operand); + } }); } else { if (this.equivalentRefs(fieldAccess, operand)) { @@ -386,7 +398,13 @@ export class ExpressionWriter { // we should generate a field reference (comparing fields in the same model) this.writeFieldReference(operand); } else { - this.plain(operand); + if (dataModel && this.isModelTyped(operand)) { + // the comparison is between model types, generate id fields comparison block + this.block(() => this.writeIdFieldsCheck(dataModel, operand)); + } else { + // scalar value, just generate the plain expression + this.plain(operand); + } } }); } @@ -400,6 +418,10 @@ export class ExpressionWriter { ); } + private isFutureMemberAccess(expr: Expression): expr is MemberAccessExpr { + return isMemberAccessExpr(expr) && isFutureExpr(expr.operand); + } + private requireIdFields(dataModel: DataModel) { const idFields = getIdFields(dataModel); if (!idFields || idFields.length === 0) { diff --git a/packages/schema/src/plugins/access-policy/policy-guard-generator.ts b/packages/schema/src/plugins/access-policy/policy-guard-generator.ts index 95edb942a..129966866 100644 --- a/packages/schema/src/plugins/access-policy/policy-guard-generator.ts +++ b/packages/schema/src/plugins/access-policy/policy-guard-generator.ts @@ -207,9 +207,16 @@ export default class PolicyGenerator { } private processUpdatePolicies(expressions: Expression[], postUpdate: boolean) { - return expressions - .map((expr) => this.visitPolicyExpression(expr, postUpdate)) - .filter((e): e is Expression => !!e); + const hasFutureReference = expressions.some((expr) => this.hasFutureReference(expr)); + if (postUpdate) { + // when compiling post-update rules, if any rule contains `future()` reference, + // we include all as post-update rules + return hasFutureReference ? expressions : []; + } else { + // when compiling pre-update rules, if any rule contains `future()` reference, + // we completely skip pre-update check and defer them to post-update + return hasFutureReference ? [] : expressions; + } } private visitPolicyExpression(expr: Expression, postUpdate: boolean): Expression | undefined { @@ -543,6 +550,9 @@ export default class PolicyGenerator { } else { return []; } + } else if (isInvocationExpr(expr)) { + // recurse into function arguments + return expr.args.flatMap((arg) => collectReferencePaths(arg.value)); } else { // recurse const children = streamContents(expr) diff --git a/packages/schema/src/utils/typescript-expression-transformer.ts b/packages/schema/src/utils/typescript-expression-transformer.ts index 74bb9d766..cd868d76c 100644 --- a/packages/schema/src/utils/typescript-expression-transformer.ts +++ b/packages/schema/src/utils/typescript-expression-transformer.ts @@ -5,6 +5,7 @@ import { DataModel, Expression, InvocationExpr, + isDataModel, isEnumField, isThisExpr, LiteralExpr, @@ -29,6 +30,7 @@ export class TypeScriptExpressionTransformerError extends Error { type Options = { isPostGuard?: boolean; fieldReferenceContext?: string; + thisExprContext?: string; context: ExpressionContext; }; @@ -99,7 +101,7 @@ export class TypeScriptExpressionTransformer { private this(_expr: ThisExpr) { // "this" is mapped to the input argument - return 'input'; + return this.options.thisExprContext ?? 'input'; } private memberAccess(expr: MemberAccessExpr, normalizeUndefined: boolean) { @@ -302,11 +304,19 @@ export class TypeScriptExpressionTransformer { return `(${expr.operator} ${this.transform(expr.operand, normalizeUndefined)})`; } + private isModelType(expr: Expression) { + return isDataModel(expr.$resolvedType?.decl); + } + private binary(expr: BinaryExpr, normalizeUndefined: boolean): string { - const _default = `(${this.transform(expr.left, normalizeUndefined)} ${expr.operator} ${this.transform( - expr.right, - normalizeUndefined - )})`; + let left = this.transform(expr.left, normalizeUndefined); + let right = this.transform(expr.right, normalizeUndefined); + if (this.isModelType(expr.left) && this.isModelType(expr.right)) { + // comparison between model type values, map to id comparison + left = `(${left}?.id ?? null)`; + right = `(${right}?.id ?? null)`; + } + const _default = `(${left} ${expr.operator} ${right})`; return match(expr.operator) .with( @@ -346,7 +356,9 @@ export class TypeScriptExpressionTransformer { const operand = this.transform(expr.left, normalizeUndefined); const innerTransformer = new TypeScriptExpressionTransformer({ ...this.options, + isPostGuard: false, fieldReferenceContext: '_item', + thisExprContext: '_item', }); const predicate = innerTransformer.transform(expr.right, normalizeUndefined); diff --git a/packages/sdk/package.json b/packages/sdk/package.json index 48145b484..e5c328269 100644 --- a/packages/sdk/package.json +++ b/packages/sdk/package.json @@ -1,6 +1,6 @@ { "name": "@zenstackhq/sdk", - "version": "1.2.1", + "version": "1.2.2", "description": "ZenStack plugin development SDK", "main": "index.js", "scripts": { diff --git a/packages/server/package.json b/packages/server/package.json index f699f8a34..48a8c5812 100644 --- a/packages/server/package.json +++ b/packages/server/package.json @@ -1,6 +1,6 @@ { "name": "@zenstackhq/server", - "version": "1.2.1", + "version": "1.2.2", "displayName": "ZenStack Server-side Adapters", "description": "ZenStack server-side adapters", "homepage": "https://zenstack.dev", diff --git a/packages/testtools/package.json b/packages/testtools/package.json index b315a3196..74e472c29 100644 --- a/packages/testtools/package.json +++ b/packages/testtools/package.json @@ -1,6 +1,6 @@ { "name": "@zenstackhq/testtools", - "version": "1.2.1", + "version": "1.2.2", "description": "ZenStack Test Tools", "main": "index.js", "private": true, diff --git a/packages/testtools/src/schema.ts b/packages/testtools/src/schema.ts index 8c285bd38..cf5a6dcd7 100644 --- a/packages/testtools/src/schema.ts +++ b/packages/testtools/src/schema.ts @@ -84,8 +84,19 @@ generator js { previewFeatures = ['clientExtensions'] } +plugin meta { + provider = '@core/model-meta' + preserveTsFiles = true +} + +plugin policy { + provider = '@core/access-policy' + preserveTsFiles = true +} + plugin zod { provider = '@core/zod' + preserveTsFiles = true modelOnly = ${!options.fullZod} } `; diff --git a/tests/integration/tests/enhancements/with-policy/post-update.test.ts b/tests/integration/tests/enhancements/with-policy/post-update.test.ts index cc8e3c746..c40d338a3 100644 --- a/tests/integration/tests/enhancements/with-policy/post-update.test.ts +++ b/tests/integration/tests/enhancements/with-policy/post-update.test.ts @@ -75,6 +75,168 @@ describe('With Policy: post update', () => { await expect(db.model.update({ where: { id: '2' }, data: { value: 4 } })).toResolveTruthy(); }); + it('functions pre-update', async () => { + const { prisma, withPolicy } = await loadSchema( + ` + model Model { + id String @id @default(uuid()) + value String + x Int + + @@allow('create,read', true) + @@allow('update', startsWith(value, 'hello') && future().x > 0) + } + ` + ); + + const db = withPolicy(); + + await prisma.model.create({ data: { id: '1', value: 'good', x: 1 } }); + await expect(db.model.update({ where: { id: '1' }, data: { value: 'hello' } })).toBeRejectedByPolicy(); + + await prisma.model.update({ where: { id: '1' }, data: { value: 'hello world' } }); + const r = await db.model.update({ where: { id: '1' }, data: { value: 'hello new world' } }); + expect(r.value).toBe('hello new world'); + }); + + it('functions post-update', async () => { + const { prisma, withPolicy } = await loadSchema( + ` + model Model { + id String @id @default(uuid()) + value String + x Int + + @@allow('create,read', true) + @@allow('update', x > 0 && startsWith(future().value, 'hello')) + } + `, + { logPrismaQuery: true } + ); + + const db = withPolicy(); + + await prisma.model.create({ data: { id: '1', value: 'good', x: 1 } }); + await expect(db.model.update({ where: { id: '1' }, data: { value: 'nice' } })).toBeRejectedByPolicy(); + + const r = await db.model.update({ where: { id: '1' }, data: { x: 0, value: 'hello world' } }); + expect(r.value).toBe('hello world'); + }); + + it('collection predicate pre-update', async () => { + const { prisma, withPolicy } = await loadSchema( + ` + model M1 { + id String @id @default(uuid()) + value Int + m2 M2[] + @@allow('read', true) + @@allow('update', m2?[value > 0] && future().value > 0) + } + + model M2 { + id String @id @default(uuid()) + value Int + m1 M1 @relation(fields: [m1Id], references:[id]) + m1Id String + + @@allow('all', true) + } + ` + ); + + const db = withPolicy(); + + await prisma.m1.create({ + data: { + id: '1', + value: 0, + m2: { + create: [{ id: '1', value: 0 }], + }, + }, + }); + + await expect( + db.m1.update({ + where: { id: '1' }, + data: { value: 1 }, + }) + ).toBeRejectedByPolicy(); + + await prisma.m2.create({ + data: { + id: '2', + m1: { connect: { id: '1' } }, + value: 1, + }, + }); + + await expect( + db.m1.update({ + where: { id: '1' }, + data: { value: 1 }, + }) + ).toResolveTruthy(); + }); + + it('collection predicate post-update', async () => { + const { prisma, withPolicy } = await loadSchema( + ` + model M1 { + id String @id @default(uuid()) + value Int + m2 M2[] + @@allow('read', true) + @@allow('update', value > 0 && future().m2?[value > 0]) + } + + model M2 { + id String @id @default(uuid()) + value Int + m1 M1 @relation(fields: [m1Id], references:[id]) + m1Id String + + @@allow('all', true) + } + ` + ); + + const db = withPolicy(); + + await prisma.m1.create({ + data: { + id: '1', + value: 1, + m2: { + create: [{ id: '1', value: 0 }], + }, + }, + }); + + await expect( + db.m1.update({ + where: { id: '1' }, + data: { value: 2 }, + }) + ).toBeRejectedByPolicy(); + + await prisma.m2.create({ + data: { + id: '2', + m1: { connect: { id: '1' } }, + value: 1, + }, + }); + + await expect( + db.m1.update({ + where: { id: '1' }, + data: { value: 2 }, + }) + ).toResolveTruthy(); + }); + it('nested to-many', async () => { const { withPolicy } = await loadSchema( ` diff --git a/tests/integration/tests/enhancements/with-policy/refactor.test.ts b/tests/integration/tests/enhancements/with-policy/refactor.test.ts index adc2599ec..f65b05f69 100644 --- a/tests/integration/tests/enhancements/with-policy/refactor.test.ts +++ b/tests/integration/tests/enhancements/with-policy/refactor.test.ts @@ -575,7 +575,7 @@ describe('With Policy: refactor tests', () => { where: { id: 2 }, data: { comments: { - update: { where: { content: 'Comment 2' }, data: { content: 'Comment 2 updated' } }, + update: { where: { id: 2 }, data: { content: 'Comment 2 updated' } }, }, }, }, diff --git a/tests/integration/tests/regression/issue-814.test.ts b/tests/integration/tests/regression/issue-814.test.ts new file mode 100644 index 000000000..ef38a80fc --- /dev/null +++ b/tests/integration/tests/regression/issue-814.test.ts @@ -0,0 +1,40 @@ +import { loadSchema } from '@zenstackhq/testtools'; + +describe('Regression: issue 814', () => { + it('regression', async () => { + const { prisma, enhance } = await loadSchema( + ` + model User { + id Int @id @default(autoincrement()) + profile Profile? + + @@allow('all', true) + } + + model Profile { + id Int @id @default(autoincrement()) + name String @allow('read', !private) + private Boolean @default(false) + user User @relation(fields: [userId], references: [id]) + userId Int @unique + + @@allow('all', true) + } + ` + ); + + const user = await prisma.user.create({ + data: { profile: { create: { name: 'Foo', private: true } } }, + include: { profile: true }, + }); + + const r = await enhance().profile.findFirst({ where: { id: user.profile.id } }); + expect(r.name).toBeUndefined(); + + const r1 = await enhance().user.findFirst({ + where: { id: user.id }, + include: { profile: true }, + }); + expect(r1.profile.name).toBeUndefined(); + }); +}); diff --git a/tests/integration/tests/regression/issue-825.test.ts b/tests/integration/tests/regression/issue-825.test.ts new file mode 100644 index 000000000..d45d5f782 --- /dev/null +++ b/tests/integration/tests/regression/issue-825.test.ts @@ -0,0 +1,39 @@ +import { loadSchema } from '@zenstackhq/testtools'; + +describe('Regression: issue 825', () => { + it('regression', async () => { + const { prisma, enhance } = await loadSchema( + ` + model User { + id Int @id @default(autoincrement()) + role String + + @@allow('read', true) + @@allow('update', auth().id == id || auth().role == 'superadmin' || auth().role == 'admin') + @@deny('update', + (role == 'superadmin' && auth().id != id) + || (role == 'admin' && auth().id != id && auth().role != 'superadmin') + || (role != future().role && auth().role != 'admin' && auth().role != 'superadmin') + || (role != future().role && future().role == 'superadmin') + || (role != future().role && future().role == 'admin' && auth().role != 'superadmin') + ) + } + ` + ); + + const admin = await prisma.user.create({ + data: { role: 'admin' }, + }); + + const user = await prisma.user.create({ + data: { role: 'customer' }, + }); + + const r = await enhance(admin).user.update({ + where: { id: user.id }, + data: { role: 'staff' }, + }); + + expect(r.role).toEqual('staff'); + }); +}); diff --git a/tests/integration/tests/schema/todo.zmodel b/tests/integration/tests/schema/todo.zmodel index 2bc57ac24..bb68c185f 100644 --- a/tests/integration/tests/schema/todo.zmodel +++ b/tests/integration/tests/schema/todo.zmodel @@ -116,7 +116,7 @@ model List { // when create, owner must be set to current user, and user must be in the space // update is not allowed to change owner - @@allow('update', owner == auth()&& space.members?[user == auth()] && future().owner == owner) + @@allow('update', owner == auth() && space.members?[user == auth()] && future().owner == owner) // can be deleted by owner @@allow('delete', owner == auth()) From b53e355b2b9f3d0a766e2c24a747d72ef71c86c1 Mon Sep 17 00:00:00 2001 From: ymc9 <104139426+ymc9@users.noreply.github.com> Date: Mon, 13 Nov 2023 16:54:30 -0800 Subject: [PATCH 2/2] remove unready test --- .../tests/regression/issue-814.test.ts | 40 ------------------- 1 file changed, 40 deletions(-) delete mode 100644 tests/integration/tests/regression/issue-814.test.ts diff --git a/tests/integration/tests/regression/issue-814.test.ts b/tests/integration/tests/regression/issue-814.test.ts deleted file mode 100644 index ef38a80fc..000000000 --- a/tests/integration/tests/regression/issue-814.test.ts +++ /dev/null @@ -1,40 +0,0 @@ -import { loadSchema } from '@zenstackhq/testtools'; - -describe('Regression: issue 814', () => { - it('regression', async () => { - const { prisma, enhance } = await loadSchema( - ` - model User { - id Int @id @default(autoincrement()) - profile Profile? - - @@allow('all', true) - } - - model Profile { - id Int @id @default(autoincrement()) - name String @allow('read', !private) - private Boolean @default(false) - user User @relation(fields: [userId], references: [id]) - userId Int @unique - - @@allow('all', true) - } - ` - ); - - const user = await prisma.user.create({ - data: { profile: { create: { name: 'Foo', private: true } } }, - include: { profile: true }, - }); - - const r = await enhance().profile.findFirst({ where: { id: user.profile.id } }); - expect(r.name).toBeUndefined(); - - const r1 = await enhance().user.findFirst({ - where: { id: user.id }, - include: { profile: true }, - }); - expect(r1.profile.name).toBeUndefined(); - }); -});