zenstackhq / zenstack

Fullstack TypeScript toolkit that enhances Prisma ORM with flexible Authorization layer for RBAC/ABAC/PBAC/ReBAC, offering auto-generated type-safe APIs and frontend hooks.
https://zenstack.dev
MIT License
2.07k stars 88 forks source link

Futures do not work with date fields #1475

Closed CollinKempkes closed 4 months ago

CollinKempkes commented 4 months ago

Description and expected behavior It seems that there is some kind of bug which breaks futures which date fields.

To reproduce do this:

model Test {
  id         String    @id @default(dbgenerated("uuid_generate_v4()")) @db.Uuid
  deleted_at DateTime? @db.Timestamp(6)

  @@map("test")
  @@schema("public")

  @@allow('all', true)
  @@deny("update", 
    deleted_at != null ||
    future().deleted_at > now()
  )
}

test.spec

import { PrismaClient } from '@equitybytes/nest-prisma';
import { faker } from '@faker-js/faker';
import { Test } from '@prisma/client';
import { ExtendedPrismaClient } from '@vdrip-database/database.types';
import {
  enhanceClientWithUser,
  extendClient,
} from '@vdrip-database/database.util';
import { add } from 'date-fns';
import { clearDbWithPrismaClient } from 'src/test-utils/db.test-helper';

describe('Test', () => {
  let test: Test;
  let extendedClient: ExtendedPrismaClient;
  let enhancedClient: ExtendedPrismaClient;

  beforeAll(async () => {
    const prismaClient = new PrismaClient();
    extendedClient = extendClient(prismaClient);

    const plattformUser =
      await extendedClient.user.findFirstPermissionsAndRestrictions({
        where: {
          id: process.env['VDRIP_PLATFORM_USER_ID'] as string,
        },
      });
    enhancedClient = enhanceClientWithUser(prismaClient, plattformUser);
  });

  beforeEach(async () => {
    await clearDbWithPrismaClient(extendedClient);

    test = await enhancedClient.test.create({
      data: {
        id: faker.string.uuid(),
      },
    });
  });

  it('should work', async () => {
    await enhancedClient.test.update({
      where: {
        id: test.id,
      },
      data: {
        deleted_at: new Date(),
      },
    });
  });

  it('should work', async () => {
    await enhancedClient.test.update({
      where: {
        id: test.id,
      },
      data: {
        id: faker.string.uuid(),
      },
    });
  });

  it('should not work', async () => {
    try {
      await enhancedClient.test.update({
        where: {
          id: test.id,
        },
        data: {
          deleted_at: add(new Date(), { hours: 1 }),
        },
      });
    } catch (error) {
      expect(1).toEqual(1);
    }

    expect.assertions(1);
  });
});

Result of the Test:

 FAIL   api  apps/api/src/app/stripe/test.spec.ts (5.158 s, 568 MB heap size)
  Test
    ✓ should work (279 ms)
    ✕ should work (182 ms)
    ✓ should not work (269 ms)

  ● Test › should work

    Error calling enhanced Prisma method `test.update`: denied by policy: test entities failed 'postUpdate' check, entity {"id":"b916d3e4-8642-4c7d-86f5-d8d8f30817c5"} failed policy check

      at Object.<anonymous> (src/app/stripe/test.spec.ts:52:31),

Test Suites: 1 failed, 1 total
Tests:       1 failed, 2 passed, 3 total
Snapshots:   0 total
Time:        5.334 s, estimated 6 s
Ran all test suites matching /test.spec/i.

If I turn it to a positive permission (allow instead of deny):

model Test {
  id         String    @id @default(dbgenerated("uuid_generate_v4()")) @db.Uuid
  deleted_at DateTime? @db.Timestamp(6)

  @@map("test")
  @@schema("public")

  @@allow('all', true)
  @@allow("update", 
    deleted_at == null ||
    future().deleted_at <= now()
  )
}

then this happens inside the suite:

 FAIL   api  apps/api/src/app/stripe/test.spec.ts (184 MB heap size)
  Test
    ✓ should work (265 ms)
    ✓ should work (198 ms)
    ✕ should not work (163 ms)

  ● Test › should not work

    expect.assertions(1)

    Expected one assertion to be called but received zero assertion calls.

      74 |     }
      75 |
    > 76 |     expect.assertions(1);
         |            ^
      77 |   });
      78 | });
      79 |

      at Object.<anonymous> (src/app/stripe/test.spec.ts:76:12)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 2 passed, 3 total
Snapshots:   0 total
Time:        3.629 s, estimated 6 s
Ran all test suites matching /test.spec/i.

So the positive case (allow) is leading to an ignorance of the rule, while the negative case (deny) leads to unexpected errors. As on the deny it fails even though the deleted_at wasn't updated.

Environment (please complete the following information):

ymc9 commented 4 months ago

Hi @CollinKempkes , thanks for reporting this. I'll look into it and let you know my findings.

ymc9 commented 4 months ago

Hi @CollinKempkes , I think this is related to how Postgres deals with NULL values.

For the "post-update" rule

  @@deny("update", 
    deleted_at != null ||
    future().deleted_at > now()
  )

At the "post-update" check time, the deleted_at != null is a constant false, so there's the future().deleted_at > now() part to evaluate. ZenStack translate it to see if it can find the updated record with the following filter (the NOT is there because it's a deny rule):

{
  NOT: {
    deleted_at: { gt: new Date() }
  }
}

This is translated to SQL by Prisma like:

SELECT "public"."test"."id" FROM "public"."test" WHERE (NOT "public"."test"."deleted_at" > ...)

In Postgres, because comparing anything with a NULL field results in NULL, the filter eventually evaluates to false, so the post-update rule check failed.

I think you can update the rule to be the following and it should work:

  @@deny("update", 
    future().deleted_at != null &&
    future().deleted_at > now()
  )
CollinKempkes commented 4 months ago

It seems to work that way, good to know that data == null is not working well with this combination! :)

Thank you very much!