| [20:07:02] | <symbioquine[m]> | It works. If I change the last line to `return AccessResult::neutral();` then the permission failure is the same as before. If I change the last line to `return AccessResult::allowedIfHasPermission($account, "Xcreate " . $entity_bundle . " " . $entity_type_id);` then it fails with the message "The current user is not permitted to upload a file for this field. The 'Xcreate animal asset' permission is required." |
| [20:07:46] | <symbioquine[m]> | As written above, creating a file succeeds when I have the `farm_manager` permission. |
| [20:09:56] | <symbioquine[m]> | ... and gives me permission denied again if I change my user to only have `farm_viewer` permissions, but with the new message "The current user is not permitted to upload a file for this field. The 'create animal asset' permission is required." |
| [20:12:50] | <symbioquine[m]> | I'll send a PR... |
| [20:18:48] | <paul121[m]> | woohoo! that was a great find! |
| [20:19:38] | <paul121[m]> | and another reminder we need to test not using the admin user 😅 |
| [20:22:42] | <symbioquine[m]> | https://github.com/farmOS/farmOS/pull/563 |
| [21:30:12] | * GudjonEinarMagnu has quit (Ping timeout: 248 seconds) |
| [21:30:12] | * evered[m] has quit (Ping timeout: 248 seconds) |
| [21:30:12] | * symbioquine[m] has quit (Ping timeout: 248 seconds) |
| [21:30:12] | * jgaehring[m] has quit (Ping timeout: 248 seconds) |
| [21:35:30] | * dazinism[m] has quit (Ping timeout: 268 seconds) |
| [21:35:32] | * FarmerEd[m] has quit (Ping timeout: 268 seconds) |
| [21:35:32] | * KarsonWynne[m] has quit (Ping timeout: 268 seconds) |
| [21:35:32] | * mstenta[m] has quit (Ping timeout: 268 seconds) |
| [21:45:40] | * jgaehring[m] has joined #farmos |
| [21:51:55] | * jonasbits has quit (Quit: No Ping reply in 180 seconds.) |
| [22:24:46] | * GudjonEinarMagnu has joined #farmos |
| [22:32:22] | * evered[m] has joined #farmos |
| [22:34:24] | * symbioquine[m] has joined #farmos |
| [05:00:09] | * GerardoLisboa[m] has quit (Quit: You have been kicked for being idle) |
| [05:09:28] | * FarmerEd[m] has joined #farmos |
| [05:20:51] | * KarsonWynne[m] has joined #farmos |
| [05:41:29] | * mstenta[m] has joined #farmos |
| [06:03:06] | * dazinism[m] has joined #farmos |
| [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... |
| [09:12:38] | <mstenta[m]> | symbioquine: of course, if you just need this for asset link you could also put the hook in that module and punt on all this until you feel like it haha |
| [09:13:06] | <mstenta[m]> | and if anyone else runs into it we can expedite that |
| [09:13:07] | <symbioquine[m]> | :) I might do that |
| [09:13:38] | <symbioquine[m]> | One nice thing is that it can exist in multiple places without hurting anything AFAICT |
| [09:13:54] | <mstenta[m]> | i think that's true! |
| [09:13:56] | <symbioquine[m]> | it's so specific about what it's granting |
| [09:53:51] | <mstenta[m]> | Ah man... there may be more to the Drush update considerations: https://www.drupal.org/project/farm/issues/3308740#comment-14687238 |
| [09:54:04] | <mstenta[m]> | > Drush does not support migrate plus config entities |
| [09:56:14] | <symbioquine[m]> | ACTION uploaded an image: (86KiB) < https://libera.ems.host/_matrix/media/r0/download/matrix.org/EjZuWHkBRPS... > |
| [09:56:48] | <symbioquine[m]> | **RotatingPony.alink.vue**... (full message at https://libera.ems.host/_matrix/media/r0/download/libera.chat/8010089a6d...) |
| [13:03:34] | <paul121[m]> | <mstenta[m]> "(i think... maybe paul121 can..." <- making a patch to entity seems to make sense. but I like your idea of including in farmOS core farm_api with a test. It would be really nice to just have that done ourselves |
| [13:04:14] | <paul121[m]> | if we were to contribute it to `entity` are there other things we need to consider when generalizing it? |
| [13:05:15] | <paul121[m]> | right now this logic would run for all entity types. maybe we should adjust the logic to only run for entity types that use the `entity` decorated access control handler. those would be the only entity types that have the bundle permissions |
| [13:08:04] | <mstenta[m]> | All good points! |
| [13:10:02] | <paul121[m]> | Generalizing is hard :-/// |
