From d99923f0d39928306c408929ef30c6f006029da4 Mon Sep 17 00:00:00 2001 From: ymc9 <104139426+ymc9@users.noreply.github.com> Date: Wed, 8 Nov 2023 16:32:39 -0800 Subject: [PATCH 1/2] fix: when field policy only has deny rule, access should be allowed when the rule doesn't satisfy --- .../src/enhancements/policy/policy-utils.ts | 17 ++++- .../access-policy/policy-guard-generator.ts | 17 ++++- .../with-policy/field-level-policy.test.ts | 76 +++++++++++++++++++ 3 files changed, 108 insertions(+), 2 deletions(-) diff --git a/packages/runtime/src/enhancements/policy/policy-utils.ts b/packages/runtime/src/enhancements/policy/policy-utils.ts index cc89942bc..17cf8cc04 100644 --- a/packages/runtime/src/enhancements/policy/policy-utils.ts +++ b/packages/runtime/src/enhancements/policy/policy-utils.ts @@ -143,6 +143,11 @@ export class PolicyUtil { const result: any = {}; for (const [key, value] of Object.entries(condition)) { + if (this.isFalse(result)) { + // already false, no need to continue + break; + } + if (value === null || value === undefined) { result[key] = value; continue; @@ -184,7 +189,16 @@ export class PolicyUtil { } case 'NOT': { - result[key] = this.reduce(value); + const r = this.reduce(value); + if (this.isFalse(r)) { + // NOT false => true, thus eliminated (not adding into result) + } else if (this.isTrue(r)) { + // NOT true => false, eliminate all other keys and set entire condition to false + Object.keys(result).forEach((k) => delete result[k]); + result['OR'] = []; // this will cause the outer loop to exit too + } else { + result[key] = r; + } break; } @@ -256,6 +270,7 @@ export class PolicyUtil { } if (!provider) { + // field access is allowed by default return this.makeTrue(); } const r = provider({ user: this.user }, db); 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..be71e2abf 100644 --- a/packages/schema/src/plugins/access-policy/policy-guard-generator.ts +++ b/packages/schema/src/plugins/access-policy/policy-guard-generator.ts @@ -606,7 +606,19 @@ export default class PolicyGenerator { throw err; } } - writer.write(`return ${FALSE};`); + + if (forField) { + if (allows.length === 0) { + // if there's no allow rule, for field-level rules, by default we allow + writer.write(`return ${TRUE};`); + } else { + // if there's any allow rule, we deny unless any allow rule evaluates to true + writer.write(`return ${FALSE};`); + } + } else { + // for model-level rules, the default is always deny + writer.write(`return ${FALSE};`); + } }); } else { statements.push((writer) => { @@ -634,14 +646,17 @@ export default class PolicyGenerator { }; if (allows.length > 0 && denies.length > 0) { + // include both allow and deny rules writer.write('{ AND: ['); writeDenies(); writer.write(','); writeAllows(); writer.write(']}'); } else if (denies.length > 0) { + // only deny rules writeDenies(); } else if (allows.length > 0) { + // only allow rules writeAllows(); } else { // disallow any operation diff --git a/tests/integration/tests/enhancements/with-policy/field-level-policy.test.ts b/tests/integration/tests/enhancements/with-policy/field-level-policy.test.ts index e43fd370d..0dec595b1 100644 --- a/tests/integration/tests/enhancements/with-policy/field-level-policy.test.ts +++ b/tests/integration/tests/enhancements/with-policy/field-level-policy.test.ts @@ -841,4 +841,80 @@ describe('With Policy: field-level policy', () => { expect(r.a).toBe(1); expect(r.b).toBe(2); }); + + it('deny only without field access', async () => { + const { prisma, withPolicy } = await loadSchema( + ` + model User { + id Int @id @default(autoincrement()) + role String @deny('update', auth().role != 'ADMIN') + + @@allow('all', true) + } + `, + { logPrismaQuery: true } + ); + + const user = await prisma.user.create({ + data: { role: 'USER' }, + }); + + await expect( + withPolicy({ id: 1, role: 'ADMIN' }).user.update({ + where: { id: user.id }, + data: { role: 'ADMIN' }, + }) + ).toResolveTruthy(); + + await expect( + withPolicy({ id: 1, role: 'USER' }).user.update({ + where: { id: user.id }, + data: { role: 'ADMIN' }, + }) + ).toBeRejectedByPolicy(); + }); + + it('deny only with field access', async () => { + const { prisma, withPolicy } = await loadSchema( + ` + model User { + id Int @id @default(autoincrement()) + locked Boolean @default(false) + role String @deny('update', auth().role != 'ADMIN' || locked) + + @@allow('all', true) + } + `, + { logPrismaQuery: true } + ); + + const user1 = await prisma.user.create({ + data: { role: 'USER' }, + }); + + await expect( + withPolicy({ id: 1, role: 'ADMIN' }).user.update({ + where: { id: user1.id }, + data: { role: 'ADMIN' }, + }) + ).toResolveTruthy(); + + await expect( + withPolicy({ id: 1, role: 'USER' }).user.update({ + where: { id: user1.id }, + data: { role: 'ADMIN' }, + }) + ).toBeRejectedByPolicy(); + + const user2 = await prisma.user.create({ + data: { role: 'USER', locked: true }, + }); + + await expect( + withPolicy({ id: 1, role: 'ADMIN' }).user.update({ + where: { id: user2.id }, + data: { role: 'ADMIN' }, + }) + ).toBeRejectedByPolicy(); + }); }); From 1bef81b4a73bd28b8139ff70391a06d19f563cc1 Mon Sep 17 00:00:00 2001 From: ymc9 <104139426+ymc9@users.noreply.github.com> Date: Wed, 8 Nov 2023 16:33:29 -0800 Subject: [PATCH 2/2] update test --- .../enhancements/with-policy/field-level-policy.test.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/tests/integration/tests/enhancements/with-policy/field-level-policy.test.ts b/tests/integration/tests/enhancements/with-policy/field-level-policy.test.ts index 0dec595b1..d8fa93872 100644 --- a/tests/integration/tests/enhancements/with-policy/field-level-policy.test.ts +++ b/tests/integration/tests/enhancements/with-policy/field-level-policy.test.ts @@ -132,8 +132,7 @@ describe('With Policy: field-level policy', () => { @@allow('all', true) } - `, - { logPrismaQuery: true } + ` ); await prisma.user.create({ data: { id: 1, admin: true } }); @@ -851,8 +850,7 @@ describe('With Policy: field-level policy', () => { @@allow('all', true) } - `, - { logPrismaQuery: true } + ` ); const user = await prisma.user.create({ @@ -884,8 +882,7 @@ describe('With Policy: field-level policy', () => { @@allow('all', true) } - `, - { logPrismaQuery: true } + ` ); const user1 = await prisma.user.create({