Skip to content

Commit

Permalink
fix: post-update rules incorrectly reject update (zenstackhq#826)
Browse files Browse the repository at this point in the history
  • Loading branch information
ymc9 authored Nov 14, 2023
1 parent db9cc7f commit e85831e
Show file tree
Hide file tree
Showing 20 changed files with 309 additions and 53 deletions.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "zenstack-monorepo",
"version": "1.2.1",
"version": "1.2.2",
"description": "",
"scripts": {
"build": "pnpm -r build",
Expand Down
2 changes: 1 addition & 1 deletion packages/language/package.json
Original file line number Diff line number Diff line change
@@ -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",
Expand Down
2 changes: 1 addition & 1 deletion packages/plugins/openapi/package.json
Original file line number Diff line number Diff line change
@@ -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": {
Expand Down
2 changes: 1 addition & 1 deletion packages/plugins/swr/package.json
Original file line number Diff line number Diff line change
@@ -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": {
Expand Down
2 changes: 1 addition & 1 deletion packages/plugins/tanstack-query/package.json
Original file line number Diff line number Diff line change
@@ -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": {
Expand Down
2 changes: 1 addition & 1 deletion packages/plugins/trpc/package.json
Original file line number Diff line number Diff line change
@@ -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": {
Expand Down
2 changes: 1 addition & 1 deletion packages/runtime/package.json
Original file line number Diff line number Diff line change
@@ -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",
Expand Down
2 changes: 1 addition & 1 deletion packages/schema/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
Expand Down
86 changes: 54 additions & 32 deletions packages/schema/src/plugins/access-policy/expression-writer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand All @@ -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 = {
Expand Down Expand Up @@ -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');
}
Expand All @@ -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)) {
Expand All @@ -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);
}
}
});
}
Expand All @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down
22 changes: 17 additions & 5 deletions packages/schema/src/utils/typescript-expression-transformer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
DataModel,
Expression,
InvocationExpr,
isDataModel,
isEnumField,
isThisExpr,
LiteralExpr,
Expand All @@ -29,6 +30,7 @@ export class TypeScriptExpressionTransformerError extends Error {
type Options = {
isPostGuard?: boolean;
fieldReferenceContext?: string;
thisExprContext?: string;
context: ExpressionContext;
};

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion packages/sdk/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@zenstackhq/sdk",
"version": "1.2.1",
"version": "1.2.2",
"description": "ZenStack plugin development SDK",
"main": "index.js",
"scripts": {
Expand Down
2 changes: 1 addition & 1 deletion packages/server/package.json
Original file line number Diff line number Diff line change
@@ -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",
Expand Down
2 changes: 1 addition & 1 deletion packages/testtools/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@zenstackhq/testtools",
"version": "1.2.1",
"version": "1.2.2",
"description": "ZenStack Test Tools",
"main": "index.js",
"private": true,
Expand Down
11 changes: 11 additions & 0 deletions packages/testtools/src/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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}
}
`;
Expand Down
Loading

0 comments on commit e85831e

Please sign in to comment.