directus / directus

The flexible backend for all your projects 🐰 Turn your DB into a headless CMS, admin panels, or apps with a custom UI, instant APIs, auth & more.
https://directus.io
Other
27.39k stars 3.83k forks source link

Custom read permissions breaks count queries #12581

Closed abdonrd closed 2 years ago

abdonrd commented 2 years ago

Preflight Checklist

Describe the Bug

Custom read permissions breaks count queries.

In the Directus admin panel: /admin/content/challenges

Admin role:

https://user-images.githubusercontent.com/1007051/161741007-848f6dee-f680-4aa4-9423-193f60f7b35e.mov

Custom role:

https://user-images.githubusercontent.com/1007051/161740990-100bf4a8-93fe-4d1b-aa44-bc1f99492fe8.mov

(in this Custom role we just unchecked one field)

Screen Shot 2022-04-05 at 13 09 49

Fixed if I grant All Access:

Screen Shot 2022-04-05 at 13 10 21

To Reproduce

See avove.

Errors Shown

[{message: "You don't have permission to access this.", extensions: {code: "FORBIDDEN"}}]

What version of Directus are you using?

9.8.0

What version of Node.js are you using?

16

What database are you using?

Postgres 14

What browser are you using?

Chrome

What operating system are you using?

macOS Monterey (12)

How are you deploying Directus?

Running locally

abdonrd commented 2 years ago

Maybe this PR https://github.com/directus/directus/pull/12488?

rijkvanzanten commented 2 years ago

Maybe this PR #12488?

I think https://github.com/directus/directus/pull/12190

licitdev commented 2 years ago

I think #12190

It is unrelated to #12190. The error was thrown in validateFields before the filter validation occurs as * is not in the list of allowed columns. https://github.com/directus/directus/blob/c2cd010eec120f308e3ce43b181200cdf55bda6b/api/src/services/authorization.ts#L116-L124

@rijkvanzanten How should we go about this issue?

  1. Allow the use of count(*) as long as there's read access to primary key.
  2. Add in docs to state to use count(id) when not all fields are allowed.

@abdonrd You may temporarily use the second option above, changing * to id for count. Note, the count value will become nested within.

count: { id: 5 }
abdonrd commented 2 years ago

@abdonrd You may temporarily use the second option above, changing * to id for count. Note, the count value will become nested within.

count: { id: 5 }

Oh! But it's not me who is doing that query. It is the Directus app itself.

rijkvanzanten commented 2 years ago

@licitdev I think count(*) should be allowed as soon as you have any read access, doesn't matter what fields. count(*) doesn't expose any of the fields or data held within, so it shouldn't matter.

licitdev commented 2 years ago

@rijkvanzanten I think that the current way of allowing count(*) to work only if there's full permissions is a good where it can prevent leaks of the total count. The total count might be sensitive info in some use cases.

rijkvanzanten commented 2 years ago

The count is still applied with the permissions filter though, so count(*) will always return the count of times you have read access to based on the filter rules, not the total count of the table. The fields you're allowed to read in those roles shouldn't matter for the count * usage though 🙂