fix: short-circuit post-read check when policy rules don't depend on model fields (#376)

ymc9 · web-flow · commit a54eba45f643 · 2023-05-02T18:48:17.000-07:00
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
     "name": "zenstack-monorepo",
-    "version": "1.0.0-alpha.112",
+    "version": "1.0.0-alpha.113",
     "description": "",
     "scripts": {
         "build": "pnpm -r build",
diff --git a/packages/language/package.json b/packages/language/package.json
@@ -1,6 +1,6 @@
 {
     "name": "@zenstackhq/language",
-    "version": "1.0.0-alpha.112",
+    "version": "1.0.0-alpha.113",
     "displayName": "ZenStack modeling language compiler",
     "description": "ZenStack modeling language compiler",
     "homepage": "https://zenstack.dev",
diff --git a/packages/next/package.json b/packages/next/package.json
@@ -1,6 +1,6 @@
 {
     "name": "@zenstackhq/next",
-    "version": "1.0.0-alpha.112",
+    "version": "1.0.0-alpha.113",
     "displayName": "ZenStack Next.js integration",
     "description": "ZenStack Next.js integration",
     "homepage": "https://zenstack.dev",
diff --git 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.0.0-alpha.112",
+  "version": "1.0.0-alpha.113",
   "description": "ZenStack plugin and runtime supporting OpenAPI",
   "main": "index.js",
   "repository": {
diff --git a/packages/plugins/react/package.json b/packages/plugins/react/package.json
@@ -1,7 +1,7 @@
 {
     "name": "@zenstackhq/react",
     "displayName": "ZenStack plugin and runtime for ReactJS",
-    "version": "1.0.0-alpha.112",
+    "version": "1.0.0-alpha.113",
     "description": "ZenStack plugin and runtime for ReactJS",
     "main": "index.js",
     "repository": {
diff --git 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.0.0-alpha.112",
+    "version": "1.0.0-alpha.113",
     "description": "ZenStack plugin for tRPC",
     "main": "index.js",
     "repository": {
diff --git a/packages/runtime/package.json b/packages/runtime/package.json
@@ -1,7 +1,7 @@
 {
     "name": "@zenstackhq/runtime",
     "displayName": "ZenStack Runtime Library",
-    "version": "1.0.0-alpha.112",
+    "version": "1.0.0-alpha.113",
     "description": "Runtime of ZenStack for both client-side and server-side environments.",
     "repository": {
         "type": "git",
diff --git a/packages/runtime/src/enhancements/policy/policy-utils.ts b/packages/runtime/src/enhancements/policy/policy-utils.ts
@@ -321,6 +321,10 @@ export class PolicyUtil {
      * omitted.
      */
     async postProcessForRead(entityData: any, model: string, args: any, operation: PolicyOperationKind) {
+        if (typeof entityData !== 'object' || !entityData) {
+            return;
+        }
+
         const ids = this.getEntityIds(model, entityData);
         if (Object.keys(ids).length === 0) {
             return;
@@ -739,6 +743,14 @@ export class PolicyUtil {
         operation: PolicyOperationKind,
         db: Record<string, DbOperations>
     ) {
+        const guard = await this.getAuthGuard(model, operation);
+        const schema = (operation === 'create' || operation === 'update') && (await this.getModelSchema(model));
+
+        if (guard === true && !schema) {
+            // unconditionally allowed
+            return;
+        }
+
         // DEBUG
         // this.logger.info(`Checking policy for ${model}#${JSON.stringify(filter)} for ${operation}`);
 
@@ -750,13 +762,19 @@ export class PolicyUtil {
         await this.flattenGeneratedUniqueField(model, queryFilter);
 
         const count = (await db[model].count({ where: queryFilter })) as number;
-        const guard = await this.getAuthGuard(model, operation);
+        if (count === 0) {
+            // there's nothing to filter out
+            return;
+        }
+
+        if (guard === false) {
+            // unconditionally denied
+            throw this.deniedByPolicy(model, operation, `${count} ${pluralize('entity', count)} failed policy check`);
+        }
 
         // build a query condition with policy injected
         const guardedQuery = { where: this.and(queryFilter, guard) };
 
-        const schema = (operation === 'create' || operation === 'update') && (await this.getModelSchema(model));
-
         if (schema) {
             // we've got schemas, so have to fetch entities and validate them
             const entities = await db[model].findMany(guardedQuery);
diff --git 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.0.0-alpha.112",
+    "version": "1.0.0-alpha.113",
     "author": {
         "name": "ZenStack Team"
     },
diff --git a/packages/schema/src/plugins/access-policy/policy-guard-generator.ts b/packages/schema/src/plugins/access-policy/policy-guard-generator.ts
@@ -9,6 +9,7 @@ import {
     isInvocationExpr,
     isMemberAccessExpr,
     isReferenceExpr,
+    isThisExpr,
     isUnaryExpr,
     MemberAccessExpr,
     Model,
@@ -33,9 +34,10 @@ import path from 'path';
 import { FunctionDeclaration, SourceFile, VariableDeclarationKind } from 'ts-morph';
 import { name } from '.';
 import { isFromStdlib } from '../../language-server/utils';
-import { getIdFields } from '../../utils/ast-utils';
+import { getIdFields, isAuthInvocation } from '../../utils/ast-utils';
 import { ALL_OPERATION_KINDS, getDefaultOutputFolder } from '../plugin-utils';
 import { ExpressionWriter } from './expression-writer';
+import TypeScriptExpressionTransformer from './typescript-expression-transformer';
 import { isFutureExpr } from './utils';
 import { ZodSchemaGenerator } from './zod-schema-generator';
 
@@ -332,18 +334,9 @@ export default class PolicyGenerator {
             .addBody();
 
         // check if any allow or deny rule contains 'auth()' invocation
-        let hasAuthRef = false;
-        for (const node of [...denies, ...allows]) {
-            for (const child of streamAllContents(node)) {
-                if (isInvocationExpr(child) && resolved(child.function).name === 'auth') {
-                    hasAuthRef = true;
-                    break;
-                }
-            }
-            if (hasAuthRef) {
-                break;
-            }
-        }
+        const hasAuthRef = [...denies, ...allows].some((rule) =>
+            streamAllContents(rule).some((child) => isAuthInvocation(child))
+        );
 
         if (hasAuthRef) {
             const userModel = model.$container.declarations.find(
@@ -365,47 +358,73 @@ export default class PolicyGenerator {
             );
         }
 
-        // r = <guard object>;
-        func.addStatements((writer) => {
-            writer.write('return ');
-            const exprWriter = new ExpressionWriter(writer, kind === 'postUpdate');
-            const writeDenies = () => {
-                writer.conditionalWrite(denies.length > 1, '{ AND: [');
-                denies.forEach((expr, i) => {
-                    writer.inlineBlock(() => {
-                        writer.write('NOT: ');
-                        exprWriter.write(expr);
-                    });
-                    writer.conditionalWrite(i !== denies.length - 1, ',');
+        const hasFieldAccess = [...denies, ...allows].some((rule) =>
+            streamAllContents(rule).some(
+                (child) =>
+                    // this.???
+                    isThisExpr(child) ||
+                    // future().???
+                    isFutureExpr(child) ||
+                    // field reference
+                    (isReferenceExpr(child) && isDataModelField(child.target.ref))
+            )
+        );
+
+        if (!hasFieldAccess) {
+            // none of the rules reference model fields, we can compile down to a plain boolean
+            // function in this case (so we can skip doing SQL queries when validating)
+            func.addStatements((writer) => {
+                const transformer = new TypeScriptExpressionTransformer(kind === 'postUpdate');
+                denies.forEach((rule) => {
+                    writer.write(`if (${transformer.transform(rule, false)}) { return false; }`);
                 });
-                writer.conditionalWrite(denies.length > 1, ']}');
-            };
-
-            const writeAllows = () => {
-                writer.conditionalWrite(allows.length > 1, '{ OR: [');
-                allows.forEach((expr, i) => {
-                    exprWriter.write(expr);
-                    writer.conditionalWrite(i !== allows.length - 1, ',');
+                allows.forEach((rule) => {
+                    writer.write(`if (${transformer.transform(rule, false)}) { return true; }`);
                 });
-                writer.conditionalWrite(allows.length > 1, ']}');
-            };
-
-            if (allows.length > 0 && denies.length > 0) {
-                writer.write('{ AND: [');
-                writeDenies();
-                writer.write(',');
-                writeAllows();
-                writer.write(']}');
-            } else if (denies.length > 0) {
-                writeDenies();
-            } else if (allows.length > 0) {
-                writeAllows();
-            } else {
-                // disallow any operation
-                writer.write(`{ ${GUARD_FIELD_NAME}: false }`);
-            }
-            writer.write(';');
-        });
+                writer.write('return false;');
+            });
+        } else {
+            func.addStatements((writer) => {
+                writer.write('return ');
+                const exprWriter = new ExpressionWriter(writer, kind === 'postUpdate');
+                const writeDenies = () => {
+                    writer.conditionalWrite(denies.length > 1, '{ AND: [');
+                    denies.forEach((expr, i) => {
+                        writer.inlineBlock(() => {
+                            writer.write('NOT: ');
+                            exprWriter.write(expr);
+                        });
+                        writer.conditionalWrite(i !== denies.length - 1, ',');
+                    });
+                    writer.conditionalWrite(denies.length > 1, ']}');
+                };
+
+                const writeAllows = () => {
+                    writer.conditionalWrite(allows.length > 1, '{ OR: [');
+                    allows.forEach((expr, i) => {
+                        exprWriter.write(expr);
+                        writer.conditionalWrite(i !== allows.length - 1, ',');
+                    });
+                    writer.conditionalWrite(allows.length > 1, ']}');
+                };
+
+                if (allows.length > 0 && denies.length > 0) {
+                    writer.write('{ AND: [');
+                    writeDenies();
+                    writer.write(',');
+                    writeAllows();
+                    writer.write(']}');
+                } else if (denies.length > 0) {
+                    writeDenies();
+                } else if (allows.length > 0) {
+                    writeAllows();
+                } else {
+                    // disallow any operation
+                    writer.write(`{ ${GUARD_FIELD_NAME}: false }`);
+                }
+                writer.write(';');
+            });
+        }
         return func;
     }
 }
diff --git a/packages/schema/src/plugins/access-policy/typescript-expression-transformer.ts b/packages/schema/src/plugins/access-policy/typescript-expression-transformer.ts
@@ -202,7 +202,10 @@ export default class TypeScriptExpressionTransformer {
                 normalizeUndefined
             )}) ?? false)`;
         } else {
-            return `(${this.transform(expr.left)} ${expr.operator} ${this.transform(expr.right, normalizeUndefined)})`;
+            return `(${this.transform(expr.left, normalizeUndefined)} ${expr.operator} ${this.transform(
+                expr.right,
+                normalizeUndefined
+            )})`;
         }
     }
 }
diff --git a/packages/schema/src/plugins/access-policy/utils.ts b/packages/schema/src/plugins/access-policy/utils.ts
@@ -1,9 +1,10 @@
-import { Expression, isInvocationExpr } from '@zenstackhq/language/ast';
+import { isInvocationExpr } from '@zenstackhq/language/ast';
+import { AstNode } from 'langium/lib/syntax-tree';
 import { isFromStdlib } from '../../language-server/utils';
 
 /**
  * Returns if the given expression is a "future()" method call.
  */
-export function isFutureExpr(expr: Expression) {
-    return !!(isInvocationExpr(expr) && expr.function.ref?.name === 'future' && isFromStdlib(expr.function.ref));
+export function isFutureExpr(node: AstNode) {
+    return !!(isInvocationExpr(node) && node.function.ref?.name === 'future' && isFromStdlib(node.function.ref));
 }
diff --git a/packages/schema/src/utils/ast-utils.ts b/packages/schema/src/utils/ast-utils.ts
@@ -17,10 +17,9 @@ import {
 } from '@zenstackhq/language/ast';
 import { PolicyOperationKind } from '@zenstackhq/runtime';
 import { getLiteral } from '@zenstackhq/sdk';
-import { AstNode, Mutable } from 'langium';
-import { isFromStdlib } from '../language-server/utils';
-import { getDocument, LangiumDocuments } from 'langium';
+import { AstNode, getDocument, LangiumDocuments, Mutable } from 'langium';
 import { URI, Utils } from 'vscode-uri';
+import { isFromStdlib } from '../language-server/utils';
 
 export function extractDataModelsWithAllowRules(model: Model): DataModel[] {
     return model.declarations.filter(
@@ -163,8 +162,8 @@ export function getIdFields(dataModel: DataModel) {
     return [];
 }
 
-export function isAuthInvocation(expr: Expression) {
-    return isInvocationExpr(expr) && expr.function.ref?.name === 'auth' && isFromStdlib(expr.function.ref);
+export function isAuthInvocation(node: AstNode) {
+    return isInvocationExpr(node) && node.function.ref?.name === 'auth' && isFromStdlib(node.function.ref);
 }
 
 export function isEnumFieldReference(expr: Expression) {
diff --git a/packages/schema/tests/plugins/policy.test.ts b/packages/schema/tests/plugins/policy.test.ts
@@ -0,0 +1,71 @@
+import { loadSchema } from '@zenstackhq/testtools';
+
+describe('Policy plugin tests', () => {
+    let origDir: string;
+
+    beforeEach(() => {
+        origDir = process.cwd();
+    });
+
+    afterEach(() => {
+        process.chdir(origDir);
+    });
+
+    it('short-circuit', async () => {
+        const model = `
+model User {
+    id String @id @default(cuid())
+    value Int
+}
+
+model M {
+    id String @id @default(cuid())
+    value Int
+    @@allow('read', auth() != null)
+    @@allow('create', auth().value > 0)
+
+    @@allow('update', auth() != null)
+    @@deny('update', auth().value == null || auth().value <= 0)
+}
+        `;
+
+        const { policy } = await loadSchema(model);
+
+        expect(policy.guard.m.read({ user: undefined })).toEqual(false);
+        expect(policy.guard.m.read({ user: { id: '1' } })).toEqual(true);
+
+        expect(policy.guard.m.create({ user: undefined })).toEqual(false);
+        expect(policy.guard.m.create({ user: { id: '1' } })).toEqual(false);
+        expect(policy.guard.m.create({ user: { id: '1', value: 0 } })).toEqual(false);
+        expect(policy.guard.m.create({ user: { id: '1', value: 1 } })).toEqual(true);
+
+        expect(policy.guard.m.update({ user: undefined })).toEqual(false);
+        expect(policy.guard.m.update({ user: { id: '1' } })).toEqual(false);
+        expect(policy.guard.m.update({ user: { id: '1', value: 0 } })).toEqual(false);
+        expect(policy.guard.m.update({ user: { id: '1', value: 1 } })).toEqual(true);
+    });
+
+    it('no short-circuit', async () => {
+        const model = `
+model User {
+    id String @id @default(cuid())
+    value Int
+}
+
+model M {
+    id String @id @default(cuid())
+    value Int
+    @@allow('read', auth() != null && value > 0)
+}
+        `;
+
+        const { policy } = await loadSchema(model);
+
+        expect(policy.guard.m.read({ user: undefined })).toEqual(
+            expect.objectContaining({ AND: [{ zenstack_guard: false }, { value: { gt: 0 } }] })
+        );
+        expect(policy.guard.m.read({ user: { id: '1' } })).toEqual(
+            expect.objectContaining({ AND: [{ zenstack_guard: true }, { value: { gt: 0 } }] })
+        );
+    });
+});
diff --git a/packages/sdk/package.json b/packages/sdk/package.json
diff --git a/packages/server/package.json b/packages/server/package.json
diff --git a/packages/testtools/package.json b/packages/testtools/package.json
diff --git a/packages/testtools/src/schema.ts b/packages/testtools/src/schema.ts
diff --git a/tests/integration/test-run/package-lock.json b/tests/integration/test-run/package-lock.json

Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "zenstack-monorepo",`
`3`		`- "version": "1.0.0-alpha.112",`
	`3`	`+ "version": "1.0.0-alpha.113",`
`4`	`4`	`"description": "",`
`5`	`5`	`"scripts": {`
`6`	`6`	`"build": "pnpm -r build",`
Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "@zenstackhq/language",`
`3`		`- "version": "1.0.0-alpha.112",`
	`3`	`+ "version": "1.0.0-alpha.113",`
`4`	`4`	`"displayName": "ZenStack modeling language compiler",`
`5`	`5`	`"description": "ZenStack modeling language compiler",`
`6`	`6`	`"homepage": "https://zenstack.dev",`
Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "@zenstackhq/next",`
`3`		`- "version": "1.0.0-alpha.112",`
	`3`	`+ "version": "1.0.0-alpha.113",`
`4`	`4`	`"displayName": "ZenStack Next.js integration",`
`5`	`5`	`"description": "ZenStack Next.js integration",`
`6`	`6`	`"homepage": "https://zenstack.dev",`
Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,7 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "@zenstackhq/openapi",`
`3`	`3`	`"displayName": "ZenStack Plugin and Runtime for OpenAPI",`
`4`		`- "version": "1.0.0-alpha.112",`
	`4`	`+ "version": "1.0.0-alpha.113",`
`5`	`5`	`"description": "ZenStack plugin and runtime supporting OpenAPI",`
`6`	`6`	`"main": "index.js",`
`7`	`7`	`"repository": {`
Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,7 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "@zenstackhq/react",`
`3`	`3`	`"displayName": "ZenStack plugin and runtime for ReactJS",`
`4`		`- "version": "1.0.0-alpha.112",`
	`4`	`+ "version": "1.0.0-alpha.113",`
`5`	`5`	`"description": "ZenStack plugin and runtime for ReactJS",`
`6`	`6`	`"main": "index.js",`
`7`	`7`	`"repository": {`
Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,7 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "@zenstackhq/trpc",`
`3`	`3`	`"displayName": "ZenStack plugin for tRPC",`
`4`		`- "version": "1.0.0-alpha.112",`
	`4`	`+ "version": "1.0.0-alpha.113",`
`5`	`5`	`"description": "ZenStack plugin for tRPC",`
`6`	`6`	`"main": "index.js",`
`7`	`7`	`"repository": {`
Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,7 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "@zenstackhq/runtime",`
`3`	`3`	`"displayName": "ZenStack Runtime Library",`
`4`		`- "version": "1.0.0-alpha.112",`
	`4`	`+ "version": "1.0.0-alpha.113",`
`5`	`5`	`"description": "Runtime of ZenStack for both client-side and server-side environments.",`
`6`	`6`	`"repository": {`
`7`	`7`	`"type": "git",`