TemporaryJsonapiFileFieldUploader::checkFileUploadAccess() checks for bundle

This is happening because the file field is a base field. The docs for \Drupal\Core\Field\FieldDefinitionInterface::getTargetBundle() are clear.

   *   The bundle the field is defined for, or NULL if it is a base field; i.e.,
   *   it is not bundle-specific.

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.

Whilst we're at it the docs seem to be wrong here too

   * @param \Drupal\Core\Entity\EntityInterface $entity
   *   (optional) The entity to which the file is to be uploaded, if it exists.
   *   If the entity does not exist and it is not given, create access to the
   *   file will be checked.

It's not create access to the file will be checked. - it's create access to the entity to which the file field is attached.

So I think we can improve the assertion.

Here's a test. While this whole class is supposed to be removed at some point I think test coverage of this specific method seems like a good idea.

Because we're adding a test with a setup we'll need 9.x and 8.x versions of this patch.

Whoops test in the wrong namespace.

#7's patch had more than I bargained for in it. Needed to rebase properly.

Tidied up the test a bit and addressed #10.2

#3: nice research! 👏

1. +++ b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
   @@ -255,13 +255,17 @@ public function validateAndParseContentDispositionHeader(Request $request) {
   +      // Base fields do not have target bundles.
   +      (is_null($field_definition->getTargetBundle()) || $field_definition->getTargetBundle() === $entity->bundle())
   

   This is the change, and this comment is critical.

   Note that this logic mimics logic from elsewhere in Drupal core because the is no standardized FileFieldUploader in core yet.

   Perhaps those other places also need to be updated to account for this?

   (I'm glad we were testing our assumptions! It took over a year for somebody to set up a scenario that proved those assumptions wrong 😳 So while this assertion was perhaps annoying to update and to test for, that seems to have been valuable nonetheless.)

2. +++ b/core/modules/jsonapi/tests/src/Kernel/Controller/TemporaryJsonapiFileFieldUploaderTest.php
   @@ -0,0 +1,176 @@
   +    // While the method is only used to check file fields it should work without
   +    // error for any field whether it is a base field or a bundle field.
   +    $base_field_definition = $this->container->get('entity_field.manager')->getBaseFieldDefinitions('node')['title'];
   +    $bundle_field_definition = $this->container->get('entity_field.manager')->getFieldDefinitions('node', 'article')['field_relationships'];
   

   🤔 So … this is using non-file fields to test this particular logic.

   Fascinating for sure.

   It's probably okay. But the larger question is the one I raised in my previous point.

Added probable related issue.

Re #12.1 No I don't the other instances of similar code have the same problem - for example \Drupal\file\Plugin\rest\resource\FileUploadResource::validateAndLoadFieldDefinition() copes fine when the entity doesn't have bundles and it's a base field.

Re #12.2 It seemed simpler to use available fields.

For sure #2940383: [META] Unify file upload logic of REST and JSON:API will help reduce the bugs in this area by sharing the logic amongst all three similar bits of code.

1. +++ b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
   @@ -255,13 +255,17 @@ public function validateAndParseContentDispositionHeader(Request $request) {
   -    assert(is_null($entity) || $field_definition->getTargetEntityTypeId() === $entity->getEntityTypeId() && $field_definition->getTargetBundle() === $entity->bundle());
   +    assert(is_null($entity) ||
   +      $field_definition->getTargetEntityTypeId() === $entity->getEntityTypeId() &&
   +      // Base fields do not have target bundles.
   +      (is_null($field_definition->getTargetBundle()) || $field_definition->getTargetBundle() === $entity->bundle())
   +    );
        $entity_type_manager = \Drupal::entityTypeManager();
   

   I'm honestly unsure what exactly the purpose of this assert is. It can't be an actual access check, because assert is not enabled on production.

   What if we turn it around and just do $entity->hasField($field_definition->getName())? that works for all kinds of fields.

2. +++ b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
   @@ -255,13 +255,17 @@ public function validateAndParseContentDispositionHeader(Request $request) {
        $bundle = $entity_type_manager->getDefinition($field_definition->getTargetEntityTypeId())->hasKey('bundle') ? $field_definition->getTargetBundle() : NULL;
   

   this doesn't seem right.

   That means $bundle will be NULL for a node base field. It should use $entity->bundle() if the entity type supports bundles.

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.

Can we split out methods for entity vs no entity? Would make it easier to read the logic.

Can we split out methods for entity vs no entity? Would make it easier to read the logic.

This is not the place for that imo, we could do that in another issue. But this would also mean extra methods in this temporary class. I would like it if we don't touch this too much.

Other than that, reading this thread it seems everything has been adressed, we are expanding the tests to test the TemporaryJsonapiFileFieldUploader code and fixing the assertion error.

I'm setting this to RTBC.

Adding credits for reviewers.

Fails on 9.4

Can we also get 10.x version?

Thanks

Patch applies to all branches and passes on all with minor fix to test data... so setting back to rtbc.

Thanks @alexpott!

+++ b/core/modules/jsonapi/tests/src/Kernel/Controller/TemporaryJsonapiFileFieldUploaderTest.php
@@ -0,0 +1,179 @@
+    // While the method is only used to check file fields it should work without
+    // error for any field whether it is a base field or a bundle field.
+    $base_field_definition = $this->container->get('entity_field.manager')->getBaseFieldDefinitions('node')['title'];

sneaky, I like that we commented here because it makes it obvious why we're using title to check a file field access

Committed dce52d1 and pushed to 10.0.x. Thanks!

Backported to 9.4.x and 9.3.x as we have a green run and the risk of disruption here is low.