Use Entity API query_access handler

Created on 22 September 2020, over 4 years ago
Updated 26 January 2024, 11 months ago

Problem/Motivation

Was just doing some research into advanced entity permissions such as "view all logs that are done"

I first came across this issue: https://www.drupal.org/project/drupal/issues/777578 ✨ Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() Needs work
...but that issue seems to recommend using the entity contrib module's query_access handler: https://www.drupal.org/node/3002038 β†’

So I dropped the default "query_access" = "\Drupal\entity\QueryAccess\UncacheableQueryAccessHandler" handler in to test. Made a custom view that displays all logs. With the "view own observation log" permission, you can truly only view your own observation logs! Without the query_access handler I can see all other logs, but the "View log" link is not generated.

I'm curious if that default query_access handler would be a good addition to the core Log module? Are there any considerations before adding that? Seems like the only permissions affecting this would be granular permissions such as "view {own/all} {bundle} log". And could it perhaps be considered a bug if you can "view" logs that don't have permission to?

πŸ› Bug report
Status

Needs review

Version

2.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States paul121 Spokane, WA

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡ΊπŸ‡ΈUnited States m.stenta

    @pcambra @paul121 and I have been discussing this at length, and we've come to the conclusion that this should *not* be implemented in the Log module. Rather, the farmOS distribution can make modifications to the log entity definition if it so desires.

    See #3415653: [META] Allow more granular access on default farmOS Views β†’

    Closing this as "won't fix".

  • Status changed to Active 11 months ago
  • πŸ‡ΊπŸ‡ΈUnited States m.stenta

    Reopening this. @pcambra @paul121 and I discussed more today and discovered that accessCheck(TRUE) is having no effect on log queries. So we decided to move forward with this after all as a bug fix.

    We *may* need to consider this a breaking change, though, which would necessitate a major version release.

  • πŸ‡«πŸ‡·France andypost

    As change notice said each query should use accessCheck(False) as log recods has no view permissions vary

  • πŸ‡ΊπŸ‡ΈUnited States m.stenta

    Thanks @andypost!

    The reason we should consider this a breaking change is that right now accessCheck(TRUE) and accessCheck(FALSE) make no difference to the list of logs returned based on user permissions. This is because the Log entity uses the Entity API module's permissions_provider handler, but not its query_access handler, which is responsible for modifying the Log entity queries based on user permissions.

    We are going to start using the Entity API query_access handler, which means that any lists of logs that are currently being built with accessCheck(TRUE) (including Views) may change for users. They will arguably change for the better, because they will actually be respecting user permissions better, so this is a bug fix. But nevertheless it is a bug fix that changes current behavior, so I think it's worth a major version bump to make it clear to users of the Log module that they might want to check their implementations.

  • πŸ‡ΊπŸ‡ΈUnited States m.stenta
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.2.1 + Environment: PHP 8.2 & MySQL 8
    last update 11 months ago
    13 pass, 4 fail
  • Status changed to Needs review 11 months ago
  • πŸ‡ΊπŸ‡ΈUnited States m.stenta
  • Status changed to Needs work 11 months ago
  • πŸ‡ΊπŸ‡ΈUnited States m.stenta

    Looks like we'll need to fix some failing tests...

  • Status changed to Needs review 11 months ago
  • πŸ‡ΊπŸ‡ΈUnited States m.stenta

    Hmm drupal.org tests are failing on 2.x in general... not specific to this MR. They are all passing locally for me (except for this know psql issue: πŸ› LogActionsTest::testRescheduleMultipleLogsRelative() fails on PostgreSQL Needs work ).

    So I think this is good. Setting back to Needs Review.

Production build 0.71.5 2024