Skip to content
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.

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

accessibleBy does not throw an error as expected when using createPrismaAbility #794

Open
benediktms opened this issue Jul 21, 2023 · 3 comments

Comments

@benediktms
Copy link

Describe the bug
I am using casl with the Prisma plugin. So far I think everything is working really nicely, but I've noticed that that the accessibleBy function does not throw an error when a restricted resource is being accessed, instead it seems to simply narrow the search query, so that nothing is returned if the user does not have permission to access the resource.

To Reproduce
For example, here I've defined my ability:

type Action = 'read' | 'create' | 'update' | 'delete' | 'manage';

type AppSubjects =
  | 'all'
  | Subjects<{
      User: User;
      Client: Client;
	  // ...
    }>;

export type AppAbility = PureAbility<[Action, AppSubjects], PrismaQuery>;

// the following code is in a function that accepts a user object

const { can, cannot, build } = new AbilityBuilder<AppAbility>(
    createPrismaAbility,
);

// for each user role I define different abilities, for example this is for the Operator role
// A more liberal role e.g. an admin or something would be able to read everything to do with
// a client

can(['read', 'update'], 'Client', {
	operatorId: { equals: user.id },
}).because('You can only access your own clients.');


// then later on when fetching from the database I am constructing my query something like so:
const filter = accessibleBy(ability).Client
const where = {
	AND: [filter, { id: clientId }],
};

const client = await prisma.client.findFirstOrThrow({
    where,
    include: {
      operator: true
    },
});

Expected behavior
As mentioned in the documentation I am expecting a ForbiddenError to be thrown. However it seems that this query simply returns:

An operation failed because it depends on one or more records that were required but not found. Expected a record, found none.

suggesting that the can simply narrows the search results rather than throw an exception. Using a findFirst and not using any filters and then asserting afterwards like so:

ForbiddenError.from(ctx.ability).throwUnlessCan(
	'read',
	subject('Client', client),
);

seems to work, but this does not seem to be the intended behaviour.

CASL Version

"@casl/ability": "^6.5.0",
"@casl/prisma": "^1.4.0",
"@casl/react": "^3.1.0",

Environment:

node: ^18

@benediktms benediktms added the bug label Jul 21, 2023
@stalniy
Copy link
Owner

stalniy commented Jul 21, 2023

Unfortunately it's not possible (at least I cant find a good way to do this). accessibleBy throws ForbiddenError only in case it cannot generate where. This means either there is no rules defined for that particular action/subject or rules defined in a way that it's possible to predict empty result set.

In order to narrow down, conditions I need to return back some where conditions and this should be something like WHERE 1 != 1, so we force underlying database engine to return empty result set (in this case SQL). But very likely prisma doesn't support this kind of query (at least it was true few versions ago).

In mongoose, there is a way to hook into query execution and return empty result or throw an error. If smth is possible in Prisma ecosystem (maybe with extensions), then potentially we can implement the same logic in Prisma

@stalniy stalniy added enhancement and removed bug labels Jul 21, 2023
@stalniy
Copy link
Owner

stalniy commented Jul 22, 2023

In the past, casl-mongoose returned empty set in case user has no access to anything but later some people asked me to raise an exception instead.

The issue here is that in case of database query it's impossible to solve this issue, correct me if I'm wrong somewhere. Look at this scenarios:

Imagine we have a blog website where multiple authors can write articles and create drafts. Only draft author has access to own drafts.

Now we have these situations:

  1. database is empty and we fetch all drafts for particular author -> empty result set
  2. database has drafts of other authors but not the one who initiates request -> empty result set
  3. database has drafts for currently logged in user -> non-empty result set.

When in this situation should we throw an error? In non of this cases. Only when this user initiates request to fetch drafts of other users (not own) but this may be forbidden technically (e.g., by using /my/drafts -> no way to fetch others drafts from API standpoint).

Potentially we could check that query somehow partially matched to allowed object will return allowed objects but even this doesn't give 100% guarantee whether user has access to smth or not. Because of queries like > & <, geolocation queries, LIKE, REGEXP, mongo allow to pass $where expressions to conditions, how should we validate that smth potentially may be disallowed in that case? This brings a lot of complexity to casl and very likely will never be solved. In my opinion there is nothing bad to show empty result set or 404 instead of 403. Moreover it's even better because you do not disclose data in your db to potential hackers.

Imagine there is a resource in db and when smb access it without proper access, you return 403 -> this tells smth to the hacker:

  1. This (probably guessed) resource exists but it's protected by ACL
  2. This website has ACL, need to find out which one and try to crack it

When you return 404 instead, the hacker don't have these 2 points from the top ^^^ and for him now impossible to know whether this resource even exist. This is how github works, in case you access private repo, you will get 404 not 403. And from encapsulation and information hiding standpoint, this is nice.

@stalniy
Copy link
Owner

stalniy commented Jul 22, 2023

I found a way to return an empty list from db instead of throwing exception -> https://www.prisma.io/docs/concepts/components/prisma-client/null-and-undefined#the-effect-of-null-and-undefined-on-conditionals

Basically if there is { OR: [] }, Prisma should return an empty result set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants