Fix JSON:API permission issue so non-admin users can upload files

[symbioquine]

No description provided.

[mstenta]

Relevant conversation in the farmOS chat: https://irc.farmos.org/bot/log/farmOS/2022-09-08#T79897

[mstenta]

More links/resources for context (pulled from the above-linked chat thread):

• Original change record that enabled uploading files via JSON:API: https://www.drupal.org/node/3024331
• Drupal StackExchange issue describing the same error message: https://drupal.stackexchange.com/questions/301117/uploading-an-image-with-jsonapi-getting-access-denied
• Relevant bits of core code that checks access for uploading files via JSON:API:
  • https://git.drupalcode.org/project/drupal/-/blob/ecbe3ad37c7648205c6d0825cff4108510d19209/core/modules/jsonapi/src/Controller/FileUpload.php#L193-214
  • https://git.drupalcode.org/project/drupal/-/blob/ecbe3ad37c7648205c6d0825cff4108510d19209/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php#L303-332
• Contrib entity module's checkCreateAccess(): https://git.drupalcode.org/project/entity/-/blob/56a04a7c1b4bb460a92422b7b2b5d886e647c411/src/EntityAccessControlHandlerBase.php#L101-119

Credit to @symbioquine and @paul121 for digging into these!

[mstenta]

More notes in chat after I had a chance to dig through this: https://irc.farmos.org/bot/log/farmOS/2022-09-09#T80032

Seems like this Drupal core issue added some of the logic we're encountering: https://www.drupal.org/project/drupal/issues/3154962

Which also led me to this one, which aims to unify some of the different file upload logic in core: https://www.drupal.org/project/drupal/issues/2940383

I wonder if that will fix this issue in the future.

[mstenta]

Couple of things I think we should do here before merging:

• Add an automated test, which ideally would pass if/when this issue gets resolved in Drupal core (if possible?).
• I think it makes more sense for this hook to be in farm_api.module instead of farm_entity.module, since that is where farmOS's JSON:API logic lives, and this is specific to JSON:API.
• I'd like to see more comments in the code to explain each step of reasoning, so that someone looking at this in the future understands exactly what's going on at a glance, along with links to this PR and/or the relevant core issues for reference.
• It would also be nice to describe the bug with steps to reproduce in this PR's description above, for future reference (and in case the irc.farmos.org link ever disappears).

[symbioquine]

https://irc.farmos.org/bot/log/farmOS/2022-09-09#T80102

[07:43:46] <mstenta[m]> Nice work symbioquine and paul121 ! Just catching up on all this...
[07:44:19] <mstenta[m]> I did a little digging through the Drupal core issue queue and found this, which sounds related (but is "Closed (fixed)"): https://www.drupal.org/project/drupal/issues/3154962
[07:46:21] <mstenta[m]> Seems like this is the issue that may have added some of the logic in the first place, and dealt with this exact base/bundle field conundrum.
[07:46:56] <mstenta[m]> > This is happening because the file field is a base field. ...
[07:46:56] <mstenta[m]> > So the question because how can we check create access for files being attached to base fields. I think is we can't determine the bundle then checking the generic create access is all we can do.
[07:46:56] <mstenta[m]> - @alexpott
[07:47:02] <mstenta[m]> s/- @alexpott/ - @alexpott/
[07:47:09] <mstenta[m]> s/-/-/
[07:47:57] <mstenta[m]> > That means $bundle will be NULL for a node base field. It should use $entity->bundle() if the entity type supports bundles.
[07:47:57] <mstenta[m]> > ...
[07:47:57] <mstenta[m]> > Oh wait, that doesn't work because that code is only used when there is no entity. Not sure how we would get it to check on the right bundle then.
[07:47:57] <mstenta[m]> - @Berdir
[07:48:51] <mstenta[m]> So it seems like this issue was mainly to enable file uploads for entities that use base fields, but was not focused on the bundle issue we are running into... maybe?
[07:49:55] <mstenta[m]> eg: the original issue was about adding a file base field to user entities (which do not have bundles)
[07:50:26] <mstenta[m]> That thread then led me to this one: https://www.drupal.org/project/drupal/issues/2940383
[07:51:45] <mstenta[m]> I haven't dug through that one in great detail yet, but I wonder if it may address this. (Marked "Critical" and "Needs work")
[07:53:04] <mstenta[m]> In either case, it seems that the hook you wrote makes sense for now symbioquine! I would like to link to add some more comments and links to core issues in it though, so we remember why it's there, and potentially remove it if/when this gets resolved upstream.
[07:53:53] <mstenta[m]> Ideally I think we'd want an automated test for this too... if possible...
[07:54:33] <mstenta[m]> eg: If core fixes this upstream, and we no longer need the hook, the test would still pass if we removed the hook
[07:55:02] <mstenta[m]> Lastly, I think this hook belongs in the farm_api module, not farm_entity, because it's specific to JSON:API
[08:28:05] <symbioquine[m]> Makes sense. Thanks for looking into it!
[08:42:55] <symbioquine[m]> <mstenta[m]> "I haven't dug through that one..." <- Sadly, the current version of the patch seems to perpetuate the same behaviour (assuming I'm reading the code correctly), and the discussion doesn't appear to make any mention of this permission issue being in-scope.
[08:43:20] <mstenta[m]> Hmm OK
[08:43:21] <mstenta[m]> I'm not surprised
[08:43:23] <mstenta[m]> The focus is on unifying all the code
[08:43:32] <mstenta[m]> Feels like there should be an issue for this specifically
[08:43:38] <symbioquine[m]> Yeah, it's kind of a weird case we're talking about too...
[08:43:49] <symbioquine[m]> bundle-level permissions for a base field :)
[08:44:29] <mstenta[m]> I don't know if I consider it �that� weird
[08:44:44] <mstenta[m]> This would affect nodes in Drupal core too
[08:44:58] <mstenta[m]> (unless I'm mistaken)
[08:45:18] <symbioquine[m]> Is there precedent for the permissions working that way elsewhere?
[08:45:44] <mstenta[m]> I think the same issue would occur if you added a base file field to node entities
[08:46:58] <symbioquine[m]> Assuming the site does use bundles for the nodes and doesn't grant a blanket "create node" permission to the users who are doing the file uploads.
[08:47:21] <mstenta[m]> right - but that IS an issue IMO
[08:47:31] <mstenta[m]> it's too broad
[08:47:31] <symbioquine[m]> cool
[08:48:08] <mstenta[m]> (fyi nodes always use bundles, impossible not to)
[08:48:32] <mstenta[m]> (SOME entity types don't, like user... but most entity types, both in core and contrib, do)
[08:49:07] <mstenta[m]> (we use the entity module to auto-generate bundle-specific permissions for our entity types, but that really just mimics the same permissions that core applies to node)
[08:49:07] <symbioquine[m]> good to know
[08:49:15] <mstenta[m]> (so those permissions are "standard" in that sense)
[08:49:46] <mstenta[m]> (although maybe not "standard enough" because it's not exactly a core standard)
[08:50:13] <mstenta[m]> so yea... we'll probably have this hook for a while 😅
[08:50:33] <mstenta[m]> but i wonder if it would actually make sense to contribute it to the entity contrib module
[08:51:34] <mstenta[m]> the overall goal of entity (contrib) is to provide stuff that core hasn't yet
[08:51:53] <mstenta[m]> specifically to support other modules that provide their own entity types (like ours)
[08:52:11] <mstenta[m]> but in theory, that stuff �should� move to core over time
[08:52:23] <mstenta[m]> sort of a staging ground
[08:53:10] <mstenta[m]> so... i wonder... maybe instead of adding a hook to farmOS, should we make a patch for entity.module? and apply that in our composer.json? we already patch that module for a few other things
[08:54:42] <mstenta[m]> the movement of that module (and Drupal core) is glacial... so i wouldn't be too worried about chasing HEAD on a patch like that... and it is sort of specific to the permissions that the entity module itself are providing
[08:55:11] <mstenta[m]> (i think... maybe paul121 can point out flaws in this thinking)
[08:55:34] <mstenta[m]> just thinking out loud 😄
[08:56:20] <symbioquine[m]> mstenta[m]: So the idea would be to open the issue against the entity project with a patch that we would apply right away to farmOS and use the issue to track this long term?
[08:56:39] <mstenta[m]> yea
[08:56:50] <mstenta[m]> the benefit of that is then we have a tracking issue we're following - so it's explicit
[08:56:53] <mstenta[m]> and gets more folks involved
[08:56:59] <symbioquine[m]> Does the entity module already provide similar things connected with JSON:API?
[08:57:01] <mstenta[m]> because it's not necessarily farmOS-specific
[08:57:12] <mstenta[m]> good question... i don't know if it does
[08:57:24] <mstenta[m]> but this feels like a pretty harmless patch to toss up and start a convo
[08:57:58] <mstenta[m]> (still... i wouldn't expect much of a response initially... but this is the type of thing that others might stumble upon over time as JSON:API file uploads get more uptake)
[08:58:26] <mstenta[m]> (especially if the error message is explicitly included in the title, description, link to related issues, etc etc)
[08:58:59] <symbioquine[m]> This is kind of similar: https://git.drupalcode.org/project/entity/-/blob/56a04a7c1b4bb460a92422b...
[08:59:27] <mstenta[m]> oh yea ok
[08:59:53] <mstenta[m]> fyi, the readme for the module points to a number of upstream core issues that this module is essentially "shimming" and the permissions is one of them
[08:59:57] <mstenta[m]> https://git.drupalcode.org/project/entity/-/blob/8.x-1.x/README.txt
[09:00:04] <mstenta[m]> https://www.drupal.org/node/2809177
[09:00:12] <mstenta[m]> so... if that lands in core, then this DOES become a core issue
[09:01:11] <symbioquine[m]> interesting
[09:03:34] <symbioquine[m]> I like that direction! It might take me a little while to get back to it though... I need to keep pushing Asset Link through to an Alpha release and get back to Greg on the farmOS-map stuff...

[mstenta]

Add an automated test, which ideally would pass if/when this issue gets resolved in Drupal core (if possible?).

#638 might help with this?

[mstenta]

Which also led me to this one, which aims to unify some of the different file upload logic in core: https://www.drupal.org/project/drupal/issues/2940383

I wonder if that will fix this issue in the future.

Just to note, #2940383 has been marked "fixed" in Drupal core's 11.x branch. Curious if that affects this at all... might be worth testing on 11.x.