In Drupal\search_api\Plugin\search_api\processor\ContentAccess~177 there is the following code:
if (!$node->access('view', $anonymous_user)) {
// Search node_access table for grants and store
else {
// Add node_access_all
}
The problem is this oversimplifies the access operation. For example, a project may have grant records that vary by domain. So when adding a node that is accessible for anonymous users on the CURRENT domain, but NOT on another domain, the grants are completely blown out and replaced with search api's node_access__all 'pseudo grant', leaving the door open for restricted content to appear on listing pages. At this time, there is a mismatch between the grant record stored in the node_access table and the one in the search_api 'node_grants' table.
In most cases, the actual grant will work, so upon clicking a link from a search_api view for example, the user will more or less have a 'broken link' to a 403. Still, I believe search_api wants to mirror the node_access grant records as closely as possible.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 2948707-16--fix_node_access_problem.patch | 5.07 KB | drunken monkey |
Comments
Comment #2
trwill commentedMy guess is this code exists to attempt to maybe to reduce the size of the table? What if instead of checking access, it just hits the node_access table like it does in the first part of the
ifabove, and checks the results. If there's only one result and its the corenode_access_allwe add our customnode_access__all(two underscores) to the field?Comment #3
trwill commentedComment #4
trwill commentedI've taken a shot with the following patch. This just says - lets check grants for everyone, and save anything that comes back from node_access table. If its empty for the node, use the pseudo grant.
Comment #5
trwill commentedComment #6
trwill commentedRealized this is failing tests now. The reason is that there is an explicit node_grant being provided in search_api_test.module. Before this patch, we were completely ignoring that grant. Now, my patch will ensure if any grants are provided they are stored in the search_api table.
Should I update this test to check for the grant being provided in search_api_test.module? Happy to do it, but let me know what you think about this approach in general and if there may be a better way to test.
Comment #7
trwill commentedThought about this a bit more. I think the test failure here perfectly illustrates the problem. In
search_api_test.module, there is a custom grant provided. However, it is discarded and replaced with thenode_access__allpseudo grant. At this time there is a mismatch between whats stored in the search_api 'node_grants' table and the actualnode_accesstable.To ensure proper functionality of the grants system I believe the
search_api_db_*_node_grantstable should match thenode_accesstable as closely as possible. Added tests for what I believe the desired outcome should be, since we are providing the custom grant explicitly.Comment #8
trwill commentedComment #9
trwill commentedComment #10
trwill commentedComment #11
drunken monkeyThanks a lot for reporting this issue!
Unfortunately, our node access implementation is known to mess up in edge cases, and I'm always very reluctant to change it since, as you can imagine, making a mistake here could easily break existing functionality, which would be a security risk in this case. So, while your explanation certainly makes sense, I'd really like some more feedback from others that this will indeed keep everything else working the same as before, before I'll be ready to commit this.
The whole thing is further hampered by me not actually being very familiar with the whole node access system. It seems not all of the "default" access restrictions are actually expressed as grants (e.g., see #2867441-4: ContentAccess processor filter by status), so the system is inconsistent to begin with. So it's hard for me to say for sure whether this check and custom grant we have is just, as you (and I) suspect, a performance improvement, or actually necessary for node access to work correctly in some cases. It seems the original code is 6.5 years old and for Drupal 7 (#955088: Provide (additional) access functionality), so – no idea at all.
NB: It seems the same thing was already proposed in #1617794-29: Make "Node access" compatible with additional contrib modules, just hidden in a D7 issue (which also contains lots of other complaints about our node access handling). Maybe some people from there can give further input here.
Comment #12
trwill commentedYeah fair enough. I also realize the test I modified doesn't make any sense and is now just a duplicate of the test immediately following it.
For some background, our project definitely fits the description of 'edge case'. We have a complex implementation of grants, both contrib and custom. We use the 'domain' and 'groups' module, and then have custom grants for the concept of 'compliance scope' to secure internal nodes. It has been a challenge to get all 3 grants to work properly together, namely making sure that one grant doesn't give access to content it shouldn't.
Ultimately for grants to work properly search_api needs to ensure that any custom grant provided is stored and respected, so it will work like drupal core does.
I can work on removing the redundant test in a follow up patch while we wait for others' input.
Comment #13
rjg commentedThe patch is working great in my project which has a mix of custom and contrib modules that implement node access grants/records
Comment #14
jordan.jamous commented#7 works for me initially, will monitor and check and get back to you later. Thanks a lot.
Comment #15
flocondetoileWorking on a similar module than domain, but based on content entity (micro_site module and its related module micro_node which implements node_access_records / node_grants system), I'm experiencing same issue described in the summary. A node published on a micro site (and so visible by anonymous users on the micro site, but not on the others sites) was indexed with the node_access_all tag, and so was visible on all other micro sites.
The index was set with the option "index immediately items".
I noticed that if nodes are NOT indexed immediately, but with the cron batch, then the correct grants are saved on the search api index.
Patch #7 works fine. I could report back later as I am currently building a project with this implementation.
Comment #16
drunken monkeySounds great, thanks for everyone’s feedback!
From all the positive feedback, I’d say this is almost ready to be committed.
I just went over the code again, reformatting a bit and fixing the test code. If this still works for all of you, and you’re fine with the changes, I’ll commit.
Patch attached, please test/review!
Comment #17
kristofferwiklund commentedI just found this bug.
Was playing around with hook_node_grants and on random allowing data access. That meant on random Anonymous has access to the content and that got indexed with node_access__all.
Applying the patch the indexed data has the correct data and everything works as (crazy) it should.
Comment #19
drunken monkeyAlright, thanks for all the feedback!
Committed.
Comment #21
slefevre@ccad.edu commentedHello, is anyone able to confirm or deny that this patch respects permissions from `tac_lite`? We have a site that uses it, and I'm testing search_api on it.
Using search_api_page, I was able to get search results of protected content when the anonymous user did a search, but was not able to click through to those results.
I am about to (attempt to) write a custom processor, but I wanted to post this question here, in case I had something misconfigured or something like that.
FWIW, as far as I can tell, tac_lite uses hook_node_access_records() for its implementation: