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.I am not sure if this is a bug but have isolated that the issue appeared with RC4. I read the notes in the jsonapi.api.php about the new access testing that RC4 introduced and that's what is causing the API to return no data.
First - here is the API Request. The filedepot_folder content type is very basic but has a entity reference back to itself in order to represent a nested file folder structure. In this case, we are looking for all folder nodes that don't have the parent_folder field set -- ie: are top level root folders.
- This worked fine up to RC4 and continues to work if the client app is logged in as Admin "uid 1"
- Does not work even if the client user has the administrative role
- Works if I grant Bypasss content access control permission to the client user
http://{site_url}/jsonapi/node/filedepot_folder?sort=-nid&fields[node--filedepot_folder]=title,uid,field_parent_folder,changed,drupal_internal__nid&include=uid,field_parent_folder&fields[user--user]=name,mail&filter[folder][condition][path]=field_parent_folder.id&filter[folder][condition][operator]=IS%20NULL&page[limit]=50
Result:
{
"data":[
],
"jsonapi":{
"version":"1.0",
"meta":{
"links":{
"self":{
"href":"http:\/\/jsonapi.org\/format\/1.0\/"
}
}
}
},
"links":{
"self":{
"href":"http:\/\/d8react2.ddev.local:8080\/jsonapi\/node\/filedepot_folder?fields%5Bnode--filedepot_folder%5D=title%2Cuid%2Cfield_parent_folder%2Cchanged%2Cdrupal_internal__nid\u0026fields%5Buser--user%5D=name%2Cmail\u0026filter%5Bfolder%5D%5Bcondition%5D%5Boperator%5D=IS%20NULL\u0026filter%5Bfolder%5D%5Bcondition%5D%5Bpath%5D=field_parent_folder.id\u0026include=uid%2Cfield_parent_folder\u0026page%5Blimit%5D=50\u0026sort=-nid"
}
}
}Tested with enabling all content view related permissions, including contact and user-information. Also tested API without the uid and 'user--user' fields - same result.
But if I use a different [operator] and filter condition, then the query works and data is returned.
http://d8.6.5.ddev.local:8080/jsonapi/node/filedepot_folder?sort=-nid&fields[node--filedepot_folder]=title,uid,field_parent_folder,changed,drupal_internal__nid&include=uid,field_parent_folder&fields[user--user]=name,mail&filter[folder][condition][path]=field_parent_folder.id&filter[folder][condition][operator]=%3D&filter[folder][condition][value]=f71b6e54-71c5-4cda-8861-cd9e67f4f604&page[limit]=50
Is this a bug or usage issue?
Thanks!
| Comment | File | Size | Author |
|---|---|---|---|
| #40 | 3025372-40.patch | 6.71 KB | ravi.shankar |
| #33 | interdiff.3025372.32-33.txt | 1.1 KB | aleevas |
| #33 | 3025372-33.patch | 7.17 KB | aleevas |
| #20 | 3025372-20.patch | 5.9 KB | gabesullice |
| #8 | 2019-01-11_15-06-26.png | 727.66 KB | blainelang |
Comments
Comment #2
blainelang CreditAttribution: blainelang commentedComment #3
gabesullice@blainelang, can you verify that the user has the
access contentpermission and that yourfiledepot_foldernodes are published?Comment #4
blainelang CreditAttribution: blainelang commentedYes and thanks for your quick reply. The user has "View Published" and View own unpublished content perms. All nodes are published and plenty of cleared caches executed. I wouldn't be retrieving the folder nodes when I change the filter condition otherwise so I was confident that I've eliminated the normal perms issues -- still very odd that enabling Bypass make it work and also very odd that giving the client uid 2 administrative role does not work. Only uid 1 works unless Bypass content access is enabled.
Comment #5
gabesullice@blainelang, man I'm a little stumped. If you look at
jsonapi.module:jsonapi_jsonapi_entity_filter_accessandjsonapi_jsonapi_node_filter_access, you'll see how we're handling node permissions. That's where the bug would have to be.Do you have the ability to use a step debugger like XDebug? If so, stepping through that logic and
TemporaryQueryGuardmight help identify the bug, then you could describe it better here.If you do not, the next best thing (perhaps better actually) would be to write a regression test in
JsonApiRegressionTest.phpthat demonstrates the bug.If you cannot do that, finding a way to reproduce the problem with a fresh install and minimal configuration and then providing us with steps to reproduce would help immensely!
Comment #6
gabesulliceSome light speculation based on the above:
The interaction of
jsonapi_jsonapi_entity_filter_accessandjsonapi_jsonapi_node_filter_accesscould be conflicting.E.g. the first hook returns:
JSONAPI_FILTER_AMONG_ALL => AccessResult::allowedIfHasPermission($account, $admin_permission)and the second hook returns:
JSONAPI_FILTER_AMONG_ALL => AccessResult::forbiddenIf(!$account->hasPermission('access content')).If the user has the admin permission but not 'access content', then when those results are
orIf()'d together, the forbidden result might win.The way to test this would be to either give your test user both
administer contentANDaccess content, or to change the result injsonapi_jsonapi_node_filter_accessfor theaccess contentcase to 'neutral' instead of forbidden.Comment #7
blainelang CreditAttribution: blainelang commentedGabe, I have xdebug so I'll step into the access controller and see what I can find. This is a fresh install and as minimum config as you can get right now - well I am using OAUTH "simple_oauth". Calls to /oauth/debug?_format=json show that I have the expected role and access content permission is TRUE.
Comment #8
blainelang CreditAttribution: blainelang commentedThe function jsonapi_jsonapi_entity_filter_access returns false from getAdminPermission().
The function jsonapi_jsonapi_node_filter_access is a bit more interesting and passes the tests for
if (!$account->hasPermission('access content')) {and executes the bottom return stmt in the function. Attached is the debugger display for $combined_access_results
Comment #9
gabesulliceComment #10
gabesulliceSorry for such a long delay on this!
I've finally replicated the issue locally. I no longer believe the issue is with
TemporaryQueryGuard. Everything looks fine there.What I think actually caused this was the interplay between JSON:API filter paths now closely match JSON:API's output structure and the security issue.
Since query has the following filter:
Your filter is actually saying that you want a node with an entity reference to a node whose UUID is NULL, not that the entity reference field is empty. This might technically have accidentally worked in the past because of the way that the entity query system works (iow, because of a leaky abstraction). With the introduction of the query guard, this accidental behavior ceased working. Why?
According to the filter path, you're technically trying to filter on values of a referenced node. Because of this, the query guard then adds an additional condition to ensure that you are only able to filter on published nodes. Of coure, there are no folders that both have an empty
field_parent_folderfield and reference a published node! Therefore, your query "correctly" returns an empty result.What I think the bug boils down to is that your query needs to specify that you want nodes with an empty
field_parent_folderrelationship.In the past, you would have been able to have a path of
field_parent_folderfor this, but the change record I linked to above prevents it by giving you an error message of:"Invalid nested filtering. The field `field_parent`, given in the path `field_parent` is incomplete, it must end with one of the following specifiers: `id`."I think that message makes sense for everything except the
IS NULLandIS NOT NULLoperator and that those need special behavior.Comment #11
Wim LeersThanks for doing that research! Your analysis makes sense to me. As does your suggested solution.
Comment #12
Wim LeersComment #13
Wim LeersI think #3021137: Ability to filter terms by "virtual" parent is related. At least conceptually, but potentially also in terms of patch.
Comment #14
koalater CreditAttribution: koalater commentedRunning into the same issue here. Is there a patch or a temporary workaround for this?
Comment #15
koalater CreditAttribution: koalater commentedRunning into the same issue here. Is there a patch or a temporary workaround for this?
Comment #16
koalater CreditAttribution: koalater commentedRunning into the same issue here. Is there a patch or a temporary workaround for this?
Comment #17
eskappiah CreditAttribution: eskappiah commented@koalater could you find a workaround or a solution to this, please? I'm in the same situation
Comment #18
gabesulliceMoving this to the core queue so it can get tested.
Comment #20
gabesulliceSlightly changing the logic, it seems I got it a bit wrong. Added a comment too.
Comment #21
gabesulliceJust needs a kernel test slightly updated.
Comment #22
alonaoneill CreditAttribution: alonaoneill at Hook 42 commentedComment #23
gabesullice@alonaoneill, please do not remove tags from issues unless you know they're incorrect. Thank you!
Also, is there a reason you set this issue back to "Needs Work"?
Comment #24
Wim LeersComment #25
psf_ CreditAttribution: psf_ at SDOS commentedHi,
I test the patch in #21, and then I filter a taxonomy term endpoint without luck.
I tried this:
I'll change the issue status, but I don't know if this issue applies only to node. I'm sorry if it's not :)
Comment #26
psf_ CreditAttribution: psf_ at SDOS commentedI'm sorry, just now I see that the user 1 can see results to:
jsonapi/taxonomy_term/poi_category?filter[root_filter][condition][path]=parent.id&filter[root_filter][condition][operator]=IS NULLBut, I think that a user that can see an element with parent virtual must see the virtual one too... This must set this issue how "need work", I think
Comment #27
janip CreditAttribution: janip commentedJust came to say that the patch in #21 seems to work fine, thank you!
Comment #28
rowrowrowrow CreditAttribution: rowrowrowrow as a volunteer commentedI'm able to filter by parent id is 'virtual' using the following workaround:
filter[parent.0]=undefined
Is there something wrong with this method?
Comment #30
andsigno82 CreditAttribution: andsigno82 commentedconfirmed #21 works for me in this form
?filter[myfilter][condition][path]=myRelationship&filter[myfilter][condition][operator]=IS NULLworth to notice that, in order to test the nullity of the relationship, i didn't use
myRelationship.idas if i would check for a given value, but onlymyRelationshipUPDATE - WARNING
after patching, i got this error while sorting
Got error 'PHP message: ArgumentCountError: Too few arguments to function Drupal\\jsonapi\\Context\\FieldResolver::resolveInternalEntityQueryPath(), 2 passed in /home/mysite/public_html/contenta/web/core/modules/jsonapi/src/Controller/EntityResource.php on line 880 and at least 3 expected in /home/mysite/public_html/contenta/web/core/modules/jsonapi/src/Context/FieldResolver.php on line 269I tried a temporary workaround mocking the 3rd param and seems to work again
/core/modules/jsonapi/src/Controller/EntityResource.php on line 880
Before patch
$query->range($pagination->getOffset(), $pagination->getSize() + 1);After patch
$query->range($pagination->getOffset(), $pagination->getSize() + 1, null);Comment #31
bander2 CreditAttribution: bander2 as a volunteer commentedRerolled #21 for D8.8.
Comment #32
cantrellnm CreditAttribution: cantrellnm at Oxbow Labs commentedD8.8 changed up the parameters for
resolveInternalEntityQueryPath. With patch #31 it assumes the presence of a third parameter$operatormeans that it's being called with the old string parameters when it's not.I've added an additional check to see if the first parameter is a string before making this assumption (there's probably a better way to fix it but this seems to work), and fixed the function call in
Drupal\Tests\jsonapi\Kernel\Query\FilterTest.Comment #33
aleevasRerolled up to 8.9
Comment #34
gabesulliceThanks for all the confirmations of #21 and thanks for the reroll in #33 @aleevas! I think it's safe to put this back to RTBC.
Comment #35
gabesulliceWhoops, forgot one! Thanks for the reroll in #32 @cantrellnm!
Let me clarify why I think this is RTBC:
This issue was moved to "Needs work" by @psf_ in #25 because, IIUC, he expected to see the "virtual" taxonomy term in the collection of terms once he filtered terms' parent ID with
IS NULL. I think that's a separate issue, perhaps even a feature request.Given that this issue has 23 followers, is a verified regression and is marked major, it makes sense to me that we should land the patch we have and address @psf_'s concerns, if he still has them, in a new issue.
Comment #37
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedIt looks like an unrelated test failure.
Comment #39
xjmThanks for your work on this!
It looks like the patch does not apply to 9.1.x, so we need a D9 version as well here. Thanks!
Comment #40
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded patch for Drupal 9.1.x.
Comment #41
bbralaWent through the reroll, there has been no changes in logic with the reroll, so i guess it should be put back to RBTC
Comment #42
gabesulliceThanks @bbrala!
Comment #45
catchCommitted/pushed to 9.1.x, cherry-picked to 9.0.x, and committed the 8.9.x patch there too, thanks!
Comment #47
blainelang CreditAttribution: blainelang commentedThanks everyone :)