Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.In #2470914: Add Views support for individual fields, we are adding field handlers for displaying individual fields in a view, and relationships to also be able to display nested items. The relationship plugin doesn't really do anything, though, it just holds the configuration while the field handlers take care of loading the items they need, based on the information from the relationship handlers.
Therefore, it's also the field handlers that check whether the user can actually access the entities they are about to display. Contrary to the search results themselves, it's currently not possible to skip those access checks, because we'd have to put the option on the field handler, leading to two problems:
- It's more cumbersome and less intuitive for the user – instead of deciding this once in the place where it makes sense, they have to set the option for each new field handler.
- If the user would set some field handlers (using the same entities) to skip access checks and others not, the latter handlers might still show content that would be inaccessible to the user – which is a security risk, even though a rather theoretical one (I'd say).
So, the proper solution is to put the option on the relationship handler. However, since that doesn't play well with our current internal mechanics, this would need a larger re-write, which we should therefore keep in a separate issue – i.e., this one. (That logic needs a re-write anyways, though, to make proper use of multi-loading.)
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 2650364-12--views_relationship_based_access_checks.patch | 14.68 KB | drunken monkey |
| |||
Comments
Comment #2
drunken monkeyComment #3
drunken monkeyComment #4
ekes CreditAttribution: ekes as a volunteer commentedTripped over this one. My expectation was that 'skip access checks' would just skip access checks to any depth of referenced entities (indexed fields). Could that not be the default. The feature request then be choosing which relationships have access checks and which not?
Comment #5
drunken monkeyWould also be an option, sure. However, it sounds a lot more confusing than my suggestion: it seems you'd then have two places to enable or disable each access check, and it sounds like it would be hard to see what the actual result is. How would you envision this working, having the option both in the query and the relationship settings?
Comment #6
drunken monkeyThis is a WIP containing at least the relationship option.
Now, we only need to re-write
\Drupal\search_api\Plugin\views\field\SearchApiFieldTrait::preRender()– piece of cake. (Caution: Heavy sarcasm!) Probably easiest to just rip it out and write a new one. Should plan a few hours for that, at least, though. Finally, we'll probably want some tests – this is so complicated, tests are the best way to ensure this keeps working.However, best we wait for #2789431: Allow the display of processor-generated fields/properties in Views with this, since we're otherwise sure to run into a merge conflict.
Comment #7
drunken monkeyComment #8
drunken monkeyComment #10
drunken monkeyComment #11
drunken monkeyComment #12
drunken monkeyThe attached patch implements this and, I think, also eliminates a few other problems the code previously had. By far not all of them, unfortunately – I shied away from that after all.
The patch also includes the one from #2801743-4: Wrong entity loaded for "entity field rendering" , since testing would have been difficult otherwise. With it, I was even able to add tests verifying this new functionality.
Comment #13
borisson_that's a weird typehint.
|bool|null'd be better?This was the only thing I noticed while doing a code review. So RTBC from me.
Comment #15
drunken monkeyThanks a lot for the review, good to hear it's RTBC in your opinion, too!
Committed.
Not according to our coding standards:
Comment #16
drunken monkey