-
Notifications
You must be signed in to change notification settings - Fork 46
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
DRUP-624 Add revisions to API Docs #161
Changes from all commits
0f100af
93faf2c
878859a
3c5c111
901a1e5
a9b9c0a
8a79ed7
a1f845e
a8fafbe
f33fe5d
ed596f7
e88bc36
8805ecf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,16 +21,50 @@ | |
namespace Drupal\apigee_edge_apidocs; | ||
|
||
use Drupal\Core\Entity\EntityAccessControlHandler; | ||
use Drupal\Core\Entity\EntityHandlerInterface; | ||
use Drupal\Core\Entity\EntityInterface; | ||
use Drupal\Core\Entity\EntityTypeInterface; | ||
use Drupal\Core\Entity\EntityTypeManagerInterface; | ||
use Drupal\Core\Session\AccountInterface; | ||
use Drupal\Core\Access\AccessResult; | ||
use Symfony\Component\DependencyInjection\ContainerInterface; | ||
|
||
/** | ||
* Access controller for the API Doc entity. | ||
* | ||
* @see \Drupal\apigee_edge_apidocs\Entity\ApiDoc. | ||
*/ | ||
class ApiDocAccessControlHandler extends EntityAccessControlHandler { | ||
class ApiDocAccessControlHandler extends EntityAccessControlHandler implements EntityHandlerInterface { | ||
|
||
/** | ||
* The entity type manager. | ||
* | ||
* @var \Drupal\Core\Entity\EntityTypeManagerInterface | ||
*/ | ||
protected $entityTypeManager; | ||
|
||
/** | ||
* Constructs an access control handler instance. | ||
* | ||
* @param \Drupal\Core\Entity\EntityTypeInterface $entity_type | ||
* The entity type definition. | ||
* @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager | ||
* The entity type manager. | ||
*/ | ||
public function __construct(EntityTypeInterface $entity_type, EntityTypeManagerInterface $entityTypeManager) { | ||
parent::__construct($entity_type); | ||
$this->entityTypeManager = $entityTypeManager; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_type) { | ||
return new static( | ||
$entity_type, | ||
$container->get('entity_type.manager') | ||
); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
|
@@ -40,6 +74,12 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter | |
|
||
if (!$parent_access->isAllowed()) { | ||
/** @var \Drupal\apigee_edge_apidocs\Entity\ApiDocInterface $entity */ | ||
|
||
// Access control for revisions. | ||
if (!$entity->isDefaultRevision()) { | ||
return $this->checkAccessRevisions($entity, $operation, $account); | ||
} | ||
|
||
switch ($operation) { | ||
case 'view': | ||
if (!$entity->isPublished()) { | ||
|
@@ -59,6 +99,60 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter | |
return $parent_access; | ||
} | ||
|
||
/** | ||
* Additional access control for revisions. | ||
* | ||
* @param \Drupal\Core\Entity\EntityInterface $entity | ||
* The entity for which to check access. | ||
* @param string $operation | ||
* The entity operation. | ||
* @param \Drupal\Core\Session\AccountInterface $account | ||
* The user for which to check access. | ||
* | ||
* @return \Drupal\Core\Access\AccessResultInterface | ||
* The access result. | ||
*/ | ||
protected function checkAccessRevisions(EntityInterface $entity, $operation, AccountInterface $account) { | ||
/** @var \Drupal\Core\Entity\EntityStorageInterface $entity_storage */ | ||
$entity_storage = $this->entityTypeManager->getStorage($this->entityTypeId); | ||
|
||
// Must have access to the same operation on the default revision. | ||
$default_revision = $entity_storage->load($entity->id()); | ||
$has_default_entity_rev_access = $default_revision->access($operation, $account); | ||
if (!$has_default_entity_rev_access) { | ||
return AccessResult::forbidden(); | ||
} | ||
|
||
$map = [ | ||
'view' => "view all {$this->entityTypeId} revisions", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you are unintentionally mixing what is the scope that an entity access handler should handle and what is the scope of a route access handler. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So probably this is the right place to ask again, can we get rid of the custom entity access and entity route handlers that we implemented and rely on what the Entity module provides? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yet an other reason why the custom implementation should go: |
||
'update' => "revert all {$this->entityTypeId} revisions", | ||
'delete' => "delete all {$this->entityTypeId} revisions", | ||
]; | ||
$bundle = $entity->bundle(); | ||
$type_map = [ | ||
'view' => "view {$this->entityTypeId} $bundle revisions", | ||
'update' => "revert {$this->entityTypeId} $bundle revisions", | ||
'delete' => "delete {$this->entityTypeId} $bundle revisions", | ||
]; | ||
|
||
if (!$entity || !isset($map[$operation]) || !isset($type_map[$operation])) { | ||
// If there was no entity to check against, or the $op was not one of the | ||
// supported ones, we return access denied. | ||
return AccessResult::forbidden(); | ||
} | ||
|
||
$admin_permission = $this->entityType->getAdminPermission(); | ||
|
||
// Perform basic permission checks first. | ||
if ($account->hasPermission($map[$operation]) || | ||
$account->hasPermission($type_map[$operation]) || | ||
($admin_permission && $account->hasPermission($admin_permission))) { | ||
return AccessResult::allowed(); | ||
} | ||
|
||
return AccessResult::forbidden(); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,8 +20,10 @@ | |
|
||
namespace Drupal\apigee_edge_apidocs\Entity; | ||
|
||
use Drupal\Core\Entity\EditorialContentEntityBase; | ||
use Drupal\Core\Entity\EntityStorageInterface; | ||
use Drupal\Core\Entity\RevisionableInterface; | ||
use Drupal\Core\Field\BaseFieldDefinition; | ||
use Drupal\Core\Entity\ContentEntityBase; | ||
use Drupal\Core\Entity\EntityChangedTrait; | ||
use Drupal\Core\Entity\EntityTypeInterface; | ||
|
||
|
@@ -34,6 +36,8 @@ | |
* label_singular = @Translation("API Doc"), | ||
* label_plural = @Translation("API Docs"), | ||
* handlers = { | ||
* | ||
* "storage" = "Drupal\Core\Entity\Sql\SqlContentEntityStorage", | ||
* "view_builder" = "Drupal\Core\Entity\EntityViewBuilder", | ||
* "list_builder" = "Drupal\apigee_edge_apidocs\ApiDocListBuilder", | ||
* "views_data" = "Drupal\views\EntityViewsData", | ||
|
@@ -47,30 +51,43 @@ | |
* "access" = "Drupal\apigee_edge_apidocs\ApiDocAccessControlHandler", | ||
* "route_provider" = { | ||
* "html" = "Drupal\apigee_edge_apidocs\ApiDocHtmlRouteProvider", | ||
* "revision" = "Drupal\entity\Routing\RevisionRouteProvider", | ||
* }, | ||
* }, | ||
* base_table = "apidoc", | ||
* data_table = "apidoc_field_data", | ||
* revision_table = "apidoc_revision", | ||
* revision_data_table = "apidoc_field_revision", | ||
* show_revision_ui = TRUE, | ||
* translatable = TRUE, | ||
* admin_permission = "administer apidoc entities", | ||
* entity_keys = { | ||
* "id" = "id", | ||
* "label" = "name", | ||
* "uuid" = "uuid", | ||
* "langcode" = "langcode", | ||
* "status" = "status", | ||
* "published" = "status", | ||
* "revision" = "revision_id", | ||
* }, | ||
* revision_metadata_keys = { | ||
* "revision_user" = "revision_user", | ||
* "revision_created" = "revision_created", | ||
* "revision_log_message" = "revision_log_message", | ||
* }, | ||
* links = { | ||
* "canonical" = "/apidoc/{apidoc}", | ||
* "add-form" = "/admin/structure/apidoc/add", | ||
* "edit-form" = "/admin/structure/apidoc/{apidoc}/edit", | ||
* "delete-form" = "/admin/structure/apidoc/{apidoc}/delete", | ||
* "version-history" = "/admin/structure/apidoc/{apidoc}/revisions", | ||
* "revision" = "/admin/structure/apidoc/{apidoc}/revisions/{apidoc_revision}/view", | ||
* "revision-revert-form" = "/admin/structure/apidoc/{apidoc}/revisions/{apidoc_revision}/revert", | ||
* "collection" = "/admin/structure/apidoc", | ||
* }, | ||
* field_ui_base_route = "apigee_edge_apidocs.settings" | ||
* ) | ||
*/ | ||
class ApiDoc extends ContentEntityBase implements ApiDocInterface { | ||
class ApiDoc extends EditorialContentEntityBase implements ApiDocInterface { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After I saw copy-pasting from |
||
|
||
use EntityChangedTrait; | ||
|
||
|
@@ -122,16 +139,41 @@ public function setCreatedTime(int $timestamp) : ApiDocInterface { | |
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function isPublished() : bool { | ||
return (bool) $this->getEntityKey('status'); | ||
public function preSave(EntityStorageInterface $storage) { | ||
parent::preSave($storage); | ||
|
||
// If no revision author has been set explicitly, make the current user | ||
// the revision author. | ||
if (!$this->getRevisionUser()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like better what Node does.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In Node, the owner id is the uid field, which we don't have in the ApiDoc entity. And Node's implementation is a workaround to what we are already doing:
and
So I think the current approach works just as well:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Should not we have an owner on ApiDoc entities? :) This is kinda an OOTB feature on content entities (nodes) that now we are intentionally removing from API doc entities - although it could be useful in the content moderation workflows, ex.: the owner (creator) of an API documentation cannot publish the API documentation only an other evaluated user. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It sounds like a good idea, but not strictly related to this PR. Could we revisit it as another ticket? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since there is no BC guaranteed for experimental modules, we can revisit this later - together with replacing the custom entity access on this entity type with what Entity module provides. |
||
$this->setRevisionUserId(\Drupal::currentUser()->id()); | ||
} | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function setPublished(bool $published) : ApiDocInterface { | ||
$this->set('status', $published); | ||
return $this; | ||
public function preSaveRevision(EntityStorageInterface $storage, \stdClass $record) { | ||
parent::preSaveRevision($storage, $record); | ||
|
||
if (!$this->isNewRevision() && isset($this->original) && empty($record->revision_log_message)) { | ||
// If we are updating an existing entity without adding a new revision, we | ||
// need to make sure $entity->revision_log_message is reset whenever it is | ||
// empty. Therefore, this code allows us to avoid clobbering an existing | ||
// log entry with an empty one. | ||
$record->revision_log_message = $this->original->revision_log_message->value; | ||
} | ||
} | ||
|
||
/** | ||
* Gets whether a new revision should be created by default. | ||
* | ||
* @return bool | ||
* TRUE if a new revision should be created by default. | ||
*/ | ||
public function shouldCreateNewRevision() { | ||
$config = \Drupal::config('apigee_edge_apidocs.settings'); | ||
$default_revision = $config->get('default_revision'); | ||
return is_null($default_revision) ? TRUE : (bool) $default_revision; | ||
} | ||
|
||
/** | ||
|
@@ -143,6 +185,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) { | |
$fields['name'] = BaseFieldDefinition::create('string') | ||
->setLabel(t('Name')) | ||
->setDescription(t('The name of the API.')) | ||
->setRevisionable(TRUE) | ||
->setSettings([ | ||
'max_length' => 50, | ||
'text_processing' => 0, | ||
|
@@ -164,6 +207,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) { | |
$fields['description'] = BaseFieldDefinition::create('text_long') | ||
->setLabel(t('Description')) | ||
->setDescription(t('Description of the API.')) | ||
->setRevisionable(TRUE) | ||
->setDisplayOptions('view', [ | ||
'label' => 'hidden', | ||
'type' => 'text_default', | ||
|
@@ -179,6 +223,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) { | |
$fields['spec'] = BaseFieldDefinition::create('file') | ||
->setLabel('OpenAPI specification') | ||
->setDescription('The spec snapshot.') | ||
->setRevisionable(TRUE) | ||
->setSettings([ | ||
'file_directory' => 'apidoc_specs', | ||
'file_extensions' => 'yml yaml json', | ||
|
@@ -205,6 +250,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) { | |
$fields['api_product'] = BaseFieldDefinition::create('entity_reference') | ||
->setLabel(t('API Product')) | ||
->setDescription(t('The API Product this is documenting.')) | ||
->setRevisionable(TRUE) | ||
->setSetting('target_type', 'api_product') | ||
->setDisplayOptions('form', [ | ||
'label' => 'above', | ||
|
@@ -214,24 +260,38 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) { | |
->setDisplayConfigurable('form', TRUE) | ||
->setDisplayConfigurable('view', TRUE); | ||
|
||
$fields['status'] = BaseFieldDefinition::create('boolean') | ||
$fields['status'] | ||
->setLabel(t('Publishing status')) | ||
->setDescription(t('A boolean indicating whether the API Doc is published.')) | ||
->setDefaultValue(TRUE) | ||
->setDisplayOptions('form', [ | ||
'type' => 'boolean_checkbox', | ||
'weight' => 1, | ||
]); | ||
|
||
$fields['created'] = BaseFieldDefinition::create('created') | ||
->setLabel(t('Created')) | ||
->setDescription(t('The time that the entity was created.')); | ||
->setDescription(t('The time that the entity was created.')) | ||
->setRevisionable(TRUE); | ||
|
||
$fields['changed'] = BaseFieldDefinition::create('changed') | ||
->setLabel(t('Changed')) | ||
->setDescription(t('The time that the entity was last edited.')); | ||
->setDescription(t('The time that the entity was last edited.')) | ||
->setRevisionable(TRUE); | ||
|
||
return $fields; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
protected function urlRouteParameters($rel) { | ||
$uri_route_parameters = parent::urlRouteParameters($rel); | ||
|
||
if ($rel === 'revision-revert-form' && $this instanceof RevisionableInterface) { | ||
mxr576 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
$uri_route_parameters[$this->getEntityTypeId() . '_revision'] = $this->getRevisionId(); | ||
} | ||
|
||
return $uri_route_parameters; | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Results should be cached in a static storage.
For example: https://github.com/fago/entity/blob/85f76b56f25d7c3f5bc466f94db30a9b63d97fc5/src/Access/EntityRevisionRouteAccessChecker.php#L124
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The results are already being cached by Drupal\Core\Entity\EntityAccessControlHandler->access() (https://git.drupalcode.org/project/drupal/blob/8.6.x/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php#L78)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you extend this class then yes it is cached, but this comment is in connection with my next comment
https://github.com/apigee/apigee-edge-drupal/pull/161/files#r271145040 in which I described that I think you are mixing what an entity access handler and an entity route access handler should do. Consequently, this class should not extend this EntityAccessControlHandler base class.
If you do not believe the Entity (contrib) module, this is how it is implemented for the Node entity in Drupal core:
This is the route access handler for node revision access - which cares about revision elated permissions.
https://github.com/drupal/core/blob/17557c97a816bc78e70989a86064c562bc27027f/modules/node/node.services.yml#L11
https://github.com/drupal/core/blob/17557c97a816bc78e70989a86064c562bc27027f/modules/node/src/Access/NodeRevisionAccessCheck.php#L93-L103
and this is the entity access handler on the Node entity that does not care about revisioning related permissions...
https://github.com/drupal/core/blob/17557c97a816bc78e70989a86064c562bc27027f/modules/node/src/Entity/Node.php#L31
https://github.com/drupal/core/blob/c784c6a8e416505fdbd7f8ab1a6608ad58d37dd9/modules/node/src/NodeAccessControlHandler.php
Also, there is this major difference between entity and route access handlers: https://www.drupal.org/node/2122195#multiple
Please create a follow-up task about cleaning up the access check on the API documentation entity type.