Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Data Producer plugins for creating, updating and deleting entities. #1212

Open
wants to merge 7 commits into
base: 8.x-4.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 115 additions & 0 deletions src/Plugin/GraphQL/DataProducer/Entity/CreateEntity.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
<?php

namespace Drupal\graphql\Plugin\GraphQL\DataProducer\Entity;

use Drupal\Core\Access\AccessResultReasonInterface;
use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
use Drupal\graphql\Plugin\GraphQL\DataProducer\DataProducerPluginBase;
use Drupal\graphql\Plugin\GraphQL\DataProducer\EntityValidationTrait;
use Symfony\Component\DependencyInjection\ContainerInterface;

/**
* Creates an entity.
*
* @DataProducer(
* id = "create_entity",
* name = @Translation("Create Entity"),
* produces = @ContextDefinition("entity",
* label = @Translation("Entity")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it you are not producing an entity directly but an array that has the entity wrapped in it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, is "any" the most appropriate type here or can the array somehow be described?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not that I know of, so "any" will have to do for now.

* ),
* consumes = {
* "entity_type" = @ContextDefinition("string",
* label = @Translation("Entity Type"),
* required = TRUE
* ),
* "values" = @ContextDefinition("any",
* label = @Translation("Field values for creating the entity"),
* required = TRUE
* ),
* "entity_return_key" = @ContextDefinition("string",
* label = @Translation("Key name in the returned array where the entity will be placed"),
* required = TRUE
* ),
* "save" = @ContextDefinition("boolean",
* label = @Translation("Save entity"),
* required = FALSE,
* default_value = TRUE,
* ),
* }
* )
*/
class CreateEntity extends DataProducerPluginBase implements ContainerFactoryPluginInterface {

use EntityValidationTrait;

/**
* The entity type manager.
*
* @var \Drupal\Core\Entity\EntityTypeManager
*/
protected $entityTypeManager;

/**
* {@inheritdoc}
*/
public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
$instance = new static($configuration, $plugin_id, $plugin_definition);
$instance->entityTypeManager = $container->get('entity_type.manager');
return $instance;
}

/**
* Resolve the values for this producer.
*/
public function resolve(string $entity_type, array $values, string $entity_return_key, ?bool $save, $context) {
$storage = $this->entityTypeManager->getStorage($entity_type);
$accessHandler = $this->entityTypeManager->getAccessControlHandler($entity_type);

// Infer the bundle type from the response and return an error if the entity
// type expects one, but one is not present.
$entity_type = $this->entityTypeManager->getDefinition($entity_type);
$bundle = $entity_type->getKey('bundle') && !empty($values[$entity_type->getKey('bundle')]) ? $values[$entity_type->getKey('bundle')] : NULL;
if ($entity_type->getKey('bundle') && !$bundle) {
return [
'errors' => [$this->t('Entity type being created requried a bundle, but none was present.')],
];
}

// Ensure the user has access to create this kind of entity.
$access = $accessHandler->createAccess($bundle, NULL, [], TRUE);
$context->addCacheableDependency($access);
if (!$access->isAllowed()) {
return [
'errors' => [$access instanceof AccessResultReasonInterface && $access->getReason() ? $access->getReason() : $this->t('Access was forbidden.')],
];
}

$entity = $storage->create($values);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this take create field access into account? If yes we should add a code comment like "// The storage will check that only fields can be set that the user has access to."

Otherwise we would have a security vulnerability here.

Again looking at JSONAPI they return "/data/$data_member_name/$public_field_name", sprintf('The current user is not allowed to POST the selected field (%s).', so I think we should do the same.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, the core API seems a little sketchy.

From JSON API:

      // Only check 'edit' permissions for fields that were actually submitted
      // by the user. Field access makes no distinction between 'create' and
      // 'update', so the 'edit' operation is used here.

From the field access docs:

   * @param string $operation
   *   The operation access should be checked for.
   *   Usually one of "view" or "edit".
...
   * @param \Drupal\Core\Field\FieldItemListInterface $items
   *   (optional) The field values for which to check access, or NULL if access
   *    is checked for the field definition, without any specific value
   *    available. Defaults to NULL.
...
  public function fieldAccess($operation, FieldDefinitionInterface $field_definition, AccountInterface $account = NULL, FieldItemListInterface $items = NULL, $return_as_object = FALSE);

Kind of begs some additional questions like, is the API invoked before or after the field values are hydrated? In the JSON API case it looks like after, so I guess it makes sense to follow that approach.


// Core does not have a concept of create access for fields, so edit access
// is used instead. This is consistent with how other Drupal APIs handle
// field based create access.
$field_access_errors = [];
foreach ($values as $field_name => $value) {
$create_access = $entity->get($field_name)->access('edit', NULL, TRUE);
if (!$create_access->isALlowed()) {
$field_access_errors[] = sprintf('%s: %s', $field_name, $create_access instanceof AccessResultReasonInterface ? $create_access->getReason() : $this->t('Access was forbidden.'));
}
}
if (!empty($field_access_errors)) {
return ['errors' => $field_access_errors];
}

if ($violation_messages = $this->getViolationMessages($entity)) {
return ['errors' => $violation_messages];
}

if ($save) {
$entity->save();
}
return [
$entity_return_key => $entity,
];
}

}
46 changes: 46 additions & 0 deletions src/Plugin/GraphQL/DataProducer/Entity/DeleteEntity.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<?php

namespace Drupal\graphql\Plugin\GraphQL\DataProducer\Entity;

use Drupal\Core\Access\AccessResultReasonInterface;
use Drupal\Core\Entity\ContentEntityInterface;
use Drupal\graphql\Plugin\GraphQL\DataProducer\DataProducerPluginBase;

/**
* Deletes an entity.
*
* @DataProducer(
* id = "delete_entity",
* name = @Translation("Delete Entity"),
* produces = @ContextDefinition("entities",
* label = @Translation("Entities")
* ),
* consumes = {
* "entity" = @ContextDefinition("entity",
* label = @Translation("Entity")
* ),
* }
* )
*/
class DeleteEntity extends DataProducerPluginBase {

/**
* Resolve the values for this producer.
*/
public function resolve(ContentEntityInterface $entity, $context) {
$access = $entity->access('delete', NULL, TRUE);
$context->addCacheableDependency($access);
if (!$access->isAllowed()) {
return [
'was_successful' => FALSE,
'errors' => [$access instanceof AccessResultReasonInterface ? $access->getReason() : 'Access was forbidden.'],
];
}

$entity->delete();
return [
'was_successful' => TRUE,
];
}

}
81 changes: 81 additions & 0 deletions src/Plugin/GraphQL/DataProducer/Entity/UpdateEntity.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
<?php

namespace Drupal\graphql\Plugin\GraphQL\DataProducer\Entity;

use Drupal\Core\Access\AccessResultReasonInterface;
use Drupal\Core\Entity\ContentEntityInterface;
use Drupal\graphql\Plugin\GraphQL\DataProducer\DataProducerPluginBase;
use Drupal\graphql\Plugin\GraphQL\DataProducer\EntityValidationTrait;

/**
* Updates entity values.
*
* @DataProducer(
* id = "update_entity",
* name = @Translation("Update Entity"),
* produces = @ContextDefinition("entities",
* label = @Translation("Entities")
* ),
* consumes = {
* "entity" = @ContextDefinition("entity",
* label = @Translation("Entity")
* ),
* "values" = @ContextDefinition("any",
* label = @Translation("Values to update"),
* required = TRUE
* ),
* "entity_return_key" = @ContextDefinition("string",
* label = @Translation("Entity Return Key"),
* required = TRUE
* ),
* }
* )
*/
class UpdateEntity extends DataProducerPluginBase {

use EntityValidationTrait;

/**
* Resolve the values for this producer.
*/
public function resolve(ContentEntityInterface $entity, array $values, string $entity_return_key, $context) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are type hinting to content entities - then I think we should rename all data producers to update_content_entity etc.?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, maybe some of them are using entity API in such a way that config entities wouldn't be off the table, but I haven't looked into that too deeply. At the very least, I can't imagine deleting a content and config entity would require any specific content entity functionality.

// Ensure the user has access to perform an update.
$access = $entity->access('update', NULL, TRUE);
$context->addCacheableDependency($access);
if (!$access->isAllowed()) {
return [
'errors' => [$access instanceof AccessResultReasonInterface ? $access->getReason() : 'Access was forbidden.'],
];
}

// Filter out keys the user does not have access to update, this may include
// things such as the owner of the entity or the ID of the entity.
$update_fields = array_filter($values, function (string $field_name) use ($entity, $context) {
if (!$entity->hasField($field_name)) {
throw new \Exception("Could not update '$field_name' field, since it does not exist on the given entity.");
}
$access = $entity->{$field_name}->access('edit', NULL, TRUE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could throw fatal errors if the field does not exist, for example if the sender made a typo. We should probably return a readable error here? Could also be the case that the schema is defined wrong, but a clear error message would be helpful.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I went for an exception here, since I guess it better signals someone has made a code error vs some runtime resolvable thing, but I believe the module gracefully handles those too?

$context->addCacheableDependency($access);
return $access->isAllowed();
}, ARRAY_FILTER_USE_KEY);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a good idea to silently filter out fields that the user does not have edit access to?

JSONAPI has more elaborate code + comments on this, check checkPatchFieldAccess() which gives reasoning for inaccessible read-only fields such as the ID vs. really not accessible fields.

I think we need to do the same.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe in this case it's better to simply return a list of errors if any fields are present the user does not have access to update, rather than to complete half the updates and fail on the other half. I don't know if this logic applies to graphql:

    // The user might not have access to edit the field, but still needs to
    // submit the current field value as part of the PATCH request. For
    // example, the entity keys required by denormalizers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, we should do the same as JSONAPI I think. Let the consumer submit special fields like ID but do not set them and ignore them.


// Hydrate the entity with the values.
foreach ($update_fields as $field_name => $field_value) {
$entity->set($field_name, $field_value);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

entity revision support is missing here like in JSONAPI EntityResource, but we don't have to do all the things in this PR, so fine for me if we skip it for now.

if ($violation_messages = $this->getViolationMessages($entity)) {
return [
'errors' => $violation_messages,
];
}

// Once access has been granted, the save can be committed and the entity
// can be returned to the client.
$entity->save();
return [
$entity_return_key => $entity,
];
}

}
44 changes: 44 additions & 0 deletions src/Plugin/GraphQL/DataProducer/EntityValidationTrait.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?php

namespace Drupal\graphql\Plugin\GraphQL\DataProducer;

use Drupal\Core\Entity\ContentEntityInterface;

/**
* Trait for entity validation.
*
* Ensure the entity passes validation, any violations will be reported back
* to the client. Validation will catch issues like invalid referenced entities,
* incorrect text formats, required fields etc. Additional validation of input
* should not be put here, but instead should be built into the entity
* validation system, so the same constraints are applied in the Drupal admin.
*/
trait EntityValidationTrait {

/**
* Get violation messages from an entity.
*
* @param \Drupal\Core\Entity\ContentEntityInterface $entity
* An entity to validate.
*
* @return array
* Get a list of violations.
*/
public function getViolationMessages(ContentEntityInterface $entity): array {
$violations = $entity->validate();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to filter out validation errors that the user does not have access to, like EntityValidationTrait in the JSONAPI core module.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, but no test added just yet.


// Remove violations of inaccessible fields as they cannot stem from our
// changes.
$violations->filterByFieldAccess();

if ($violations->count() > 0) {
$violation_messages = [];
foreach ($violations as $violation) {
$violation_messages[] = sprintf('%s: %s', $violation->getPropertyPath(), strip_tags($violation->getMessage()));
}
return $violation_messages;
}
return [];
}

}
Loading