Skip to content

fix: should not reject "update" when there's only field-level override but no model-level policy #1052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 24, 2024
Merged

fix: should not reject "update" when there's only field-level override but no model-level policy #1052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions packages/runtime/src/enhancements/policy/policy-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ export class PolicyUtil {
/**
* Checks if the given model has a policy guard for the given operation.
*/
hasAuthGuard(model: string, operation: PolicyOperationKind): boolean {
hasAuthGuard(model: string, operation: PolicyOperationKind) {
const guard = this.policy.guard[lowerCaseFirst(model)];
if (!guard) {
return false;
Expand All @@ -328,6 +328,21 @@ export class PolicyUtil {
return typeof provider !== 'boolean' || provider !== true;
}

/**
* Checks if the given model has any field-level override policy guard for the given operation.
*/
hasOverrideAuthGuard(model: string, operation: PolicyOperationKind) {
const guard = this.requireGuard(model);
switch (operation) {
case 'read':
return Object.keys(guard).some((k) => k.startsWith(FIELD_LEVEL_OVERRIDE_READ_GUARD_PREFIX));
case 'update':
return Object.keys(guard).some((k) => k.startsWith(FIELD_LEVEL_OVERRIDE_UPDATE_GUARD_PREFIX));
default:
return false;
}
}

/**
* Checks model creation policy based on static analysis to the input args.
*
Expand Down Expand Up @@ -731,7 +746,7 @@ export class PolicyUtil {
preValue?: any
) {
let guard = this.getAuthGuard(db, model, operation, preValue);
if (this.isFalse(guard)) {
if (this.isFalse(guard) && !this.hasOverrideAuthGuard(model, operation)) {
throw this.deniedByPolicy(
model,
operation,
Expand Down Expand Up @@ -904,7 +919,7 @@ export class PolicyUtil {
*/
tryReject(db: Record<string, DbOperations>, model: string, operation: PolicyOperationKind) {
const guard = this.getAuthGuard(db, model, operation);
if (this.isFalse(guard)) {
if (this.isFalse(guard) && !this.hasOverrideAuthGuard(model, operation)) {
throw this.deniedByPolicy(model, operation, undefined, CrudFailureReason.ACCESS_POLICY_VIOLATION);
}
}
Expand Down
53 changes: 53 additions & 0 deletions tests/integration/tests/regression/issue-1014.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { loadSchema } from '@zenstackhq/testtools';

describe('issue 1014', () => {
it('update', async () => {
const { prisma, enhance } = await loadSchema(
`
model User {
id Int @id() @default(autoincrement())
name String
posts Post[]
}

model Post {
id Int @id() @default(autoincrement())
title String
content String?
author User? @relation(fields: [authorId], references: [id])
authorId Int? @allow('update', true, true)

@@allow('read', true)
}
`,
{ logPrismaQuery: true }
);

const db = enhance();

const user = await prisma.user.create({ data: { name: 'User1' } });
const post = await prisma.post.create({ data: { title: 'Post1' } });
await expect(db.post.update({ where: { id: post.id }, data: { authorId: user.id } })).toResolveTruthy();
});
Comment on lines +3 to +31
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test case for update operations correctly sets up a schema with User and Post models, creates a user and a post, and then attempts to update the authorId of the post to simulate the issue. However, there are a few points to consider:

  • The loadSchema function is used with logPrismaQuery: true, which is great for debugging but might clutter test output in CI environments. Consider making logging configurable based on the environment.
  • The expect statement at line 30 uses toResolveTruthy(), which is not a standard Jest matcher. Ensure that a custom matcher or extension is properly configured to handle this assertion.
  • It's good practice to clean up created data after tests to avoid side effects. Consider adding an afterEach block to delete created posts and users.
- await expect(db.post.update({ where: { id: post.id }, data: { authorId: user.id } })).toResolveTruthy();
+ await expect(db.post.update({ where: { id: post.id }, data: { authorId: user.id } })).resolves.not.toThrow();

Consider adding cleanup logic and review the custom matcher or extension used for toResolveTruthy().

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
describe('issue 1014', () => {
it('update', async () => {
const { prisma, enhance } = await loadSchema(
`
model User {
id Int @id() @default(autoincrement())
name String
posts Post[]
}
model Post {
id Int @id() @default(autoincrement())
title String
content String?
author User? @relation(fields: [authorId], references: [id])
authorId Int? @allow('update', true, true)
@@allow('read', true)
}
`,
{ logPrismaQuery: true }
);
const db = enhance();
const user = await prisma.user.create({ data: { name: 'User1' } });
const post = await prisma.post.create({ data: { title: 'Post1' } });
await expect(db.post.update({ where: { id: post.id }, data: { authorId: user.id } })).toResolveTruthy();
});
describe('issue 1014', () => {
it('update', async () => {
const { prisma, enhance } = await loadSchema(
`
model User {
id Int @id() @default(autoincrement())
name String
posts Post[]
}
model Post {
id Int @id() @default(autoincrement())
title String
content String?
author User? @relation(fields: [authorId], references: [id])
authorId Int? @allow('update', true, true)
@@allow('read', true)
}
`,
{ logPrismaQuery: true }
);
const db = enhance();
const user = await prisma.user.create({ data: { name: 'User1' } });
const post = await prisma.post.create({ data: { title: 'Post1' } });
await expect(db.post.update({ where: { id: post.id }, data: { authorId: user.id } })).resolves.not.toThrow();
});


it('read', async () => {
const { prisma, enhance } = await loadSchema(
`
model Post {
id Int @id() @default(autoincrement())
title String @allow('read', true, true)
content String
}
`,
{ logPrismaQuery: true }
);

const db = enhance();

const post = await prisma.post.create({ data: { title: 'Post1', content: 'Content' } });
await expect(db.post.findUnique({ where: { id: post.id } })).toResolveNull();
await expect(db.post.findUnique({ where: { id: post.id }, select: { title: true } })).resolves.toEqual({
title: 'Post1',
});
});
Comment on lines +33 to +52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test case for read operations is well-structured, focusing on the ability to read fields with specific access policies. Points to consider:

  • The schema definition for the Post model at lines 36-40 includes an @allow directive on the title field, which aligns with the test's objective. However, the content field does not have an explicit access policy, which might be intentional but could also be an oversight. If testing access policies on multiple fields is desired, consider adding relevant policies.
  • The use of await expect(...).toResolveNull(); at line 48 suggests expecting a null result, which is not a standard Jest matcher. Ensure that a custom matcher or extension is properly configured for this assertion.
  • The test validates the access policy on the title field but does not explicitly test the default deny behavior for fields without an @allow directive. Consider adding an assertion to verify that accessing the content field without explicit permission results in the expected behavior.
- await expect(db.post.findUnique({ where: { id: post.id } })).toResolveNull();
+ await expect(db.post.findUnique({ where: { id: post.id } })).resolves.toBeNull();

Review the custom matcher or extension used for toResolveNull() and consider adding more assertions to cover different access policy scenarios.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
it('read', async () => {
const { prisma, enhance } = await loadSchema(
`
model Post {
id Int @id() @default(autoincrement())
title String @allow('read', true, true)
content String
}
`,
{ logPrismaQuery: true }
);
const db = enhance();
const post = await prisma.post.create({ data: { title: 'Post1', content: 'Content' } });
await expect(db.post.findUnique({ where: { id: post.id } })).toResolveNull();
await expect(db.post.findUnique({ where: { id: post.id }, select: { title: true } })).resolves.toEqual({
title: 'Post1',
});
});
it('read', async () => {
const { prisma, enhance } = await loadSchema(
`
model Post {
id Int @id() @default(autoincrement())
title String @allow('read', true, true)
content String
}
`,
{ logPrismaQuery: true }
);
const db = enhance();
const post = await prisma.post.create({ data: { title: 'Post1', content: 'Content' } });
await expect(db.post.findUnique({ where: { id: post.id } })).resolves.toBeNull();
await expect(db.post.findUnique({ where: { id: post.id }, select: { title: true } })).resolves.toEqual({
title: 'Post1',
});
});

});
10 changes: 0 additions & 10 deletions tests/integration/tests/tsconfig.template.json
View file
Open in desktop

This file was deleted.