From 84dd7a5c9bac61b03c1ab06a2a147d7c0bd22e9c Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Sat, 17 Feb 2024 15:06:02 -0400 Subject: [PATCH 01/17] Rework querying. Should reduce the number of joins when dealing with various multiple file and media fields in the same query, as it instead builds out a disjuction in the "join" condition against the LUT. --- src/Access/QueryTagger.php | 101 +++++++++--------- .../EmbargoAccessQueryTaggingAlterTest.php | 7 ++ 2 files changed, 55 insertions(+), 53 deletions(-) diff --git a/src/Access/QueryTagger.php b/src/Access/QueryTagger.php index 81f9c58..009af6a 100644 --- a/src/Access/QueryTagger.php +++ b/src/Access/QueryTagger.php @@ -90,7 +90,7 @@ public function __construct( * @param string $type * Either "node" or "file". */ - public function tagAccess(SelectInterface $query, string $type) { + public function tagAccess(SelectInterface $query, string $type) : void { if (!in_array($type, ['node', 'media', 'file'])) { throw new \InvalidArgumentException("Unrecognized type '$type'."); } @@ -104,53 +104,61 @@ public function tagAccess(SelectInterface $query, string $type) { $storage = $this->entityTypeManager->getStorage($type); $tables = $storage->getTableMapping()->getTableNames(); + $target_aliases = []; + foreach ($query->getTables() as $info) { if ($info['table'] instanceof SelectInterface) { continue; } elseif (in_array($info['table'], $tables)) { - $key = (strpos($info['table'], "{$type}__") === 0) ? 'entity_id' : (substr($type, 0, 1) . "id"); + $key = (str_starts_with($info['table'], "{$type}__")) ? 'entity_id' : (substr($type, 0, 1) . "id"); $alias = $info['alias']; - - $to_apply = $query; - if ($info['join type'] == 'LEFT') { - $to_apply = $query->orConditionGroup() - ->isNull("{$alias}.{$key}"); - $query->condition($to_apply); - } - if ($type === 'node') { - $to_apply->condition("{$alias}.{$key}", $this->buildInaccessibleEmbargoesCondition(), 'NOT IN'); - } - elseif ($type === 'media') { - $to_apply->condition("{$alias}.{$key}", $this->buildInaccessibleFileCondition('mid'), 'NOT IN'); - } - elseif ($type === 'file') { - $to_apply->condition("{$alias}.{$key}", $this->buildInaccessibleFileCondition('fid'), 'NOT IN'); - } - else { - throw new \InvalidArgumentException("Invalid type '$type'."); - } + $target_aliases[] = "{$alias}.{$key}"; } } - } - /** - * Builds the condition for file-typed embargoes that are inaccessible. - * - * @param string $lut_column - * The particular column of the LUT to return, as file embargoes apply to - * media ('mid') as well as files ('fid'). - * - * @return \Drupal\Core\Database\Query\SelectInterface - * The sub-query to be used that results in all file IDs that cannot be - * accessed. - */ - protected function buildInaccessibleFileCondition(string $lut_column) { - $query = $this->database->select('embargo', 'e'); - $lut_alias = $query->join(LUTGeneratorInterface::TABLE_NAME, 'lut', '%alias.nid = e.embargoed_node'); - return $query - ->fields($lut_alias, [$lut_column]) - ->condition('lut.nid', $this->buildAccessibleEmbargoesQuery(EmbargoInterface::EMBARGO_TYPE_FILE), 'NOT IN'); + if (empty($target_aliases)) { + return; + } + + $existence = $this->database->select('node', 'existence_node'); + $existence->fields('existence_node', ['nid']); + + if ($type !== 'node') { + $lut_column = match($type) { + 'file' => 'fid', + 'media' => 'mid', + }; + $existence_lut_alias = $existence->leftJoin(LUTGeneratorInterface::TABLE_NAME, 'lut', '%alias.nid = existence_node.nid'); + $existence->where(strtr('!field IS NULL OR !field IN (!targets)', [ + '!field' => "{$existence_lut_alias}.{$lut_column}", + '!targets' => implode(', ', $target_aliases), + ])); + } + else { + $existence->where(strtr('!field IN (!targets)', [ + '!field' => 'existence_node.nid', + '!targets' => implode(', ', $target_aliases), + ])); + } + + $exist_or = $existence->orConditionGroup(); + + // No embargo. + $embargo = $this->database->select('embargo', 'ee'); + $embargo->fields('ee', ['embargoed_node']); + $exist_or->condition("existence_node.nid", $embargo, 'NOT IN'); + + // Embargoed (and allowed). + $accessible_embargoes = $this->buildAccessibleEmbargoesQuery(match($type) { + 'file', 'media' => EmbargoInterface::EMBARGO_TYPE_FILE, + 'node' => EmbargoInterface::EMBARGO_TYPE_NODE, + }); + $exist_or->condition("existence_node.nid", $accessible_embargoes, 'IN'); + + $existence->condition($exist_or); + + $query->exists($existence); } /** @@ -164,7 +172,7 @@ protected function buildInaccessibleFileCondition(string $lut_column) { * @return \Drupal\Core\Database\Query\SelectInterface * A query returning things that should not be inaccessible. */ - protected function buildAccessibleEmbargoesQuery($type) : SelectInterface { + protected function buildAccessibleEmbargoesQuery(int $type) : SelectInterface { $query = $this->database->select('embargo', 'e') ->fields('e', ['embargoed_node']); @@ -197,17 +205,4 @@ protected function buildAccessibleEmbargoesQuery($type) : SelectInterface { return $query; } - /** - * Builds the condition for embargoes that are inaccessible. - * - * @return \Drupal\Core\Database\Query\SelectInterface - * The sub-query to be used that results in embargoed_node IDs that - * cannot be accessed. - */ - protected function buildInaccessibleEmbargoesCondition() : SelectInterface { - return $this->database->select('embargo', 'ein') - ->condition('ein.embargoed_node', $this->buildAccessibleEmbargoesQuery(EmbargoInterface::EMBARGO_TYPE_NODE), 'NOT IN') - ->fields('ein', ['embargoed_node']); - } - } diff --git a/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php b/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php index c30857f..b053fd8 100644 --- a/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php +++ b/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php @@ -13,6 +13,13 @@ class EmbargoAccessQueryTaggingAlterTest extends EmbargoKernelTestBase { use DatabaseQueryTestTraits; + /** + * Test embargo instance. + * + * @var \Drupal\embargo\EmbargoInterface + */ + protected EmbargoInterface $embargo; + /** * {@inheritdoc} */ From 3896ae6e3566acbfc7211773f0e2d5d086a0c0d7 Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Tue, 20 Feb 2024 09:39:27 -0400 Subject: [PATCH 02/17] Move more explicitly to existence check. --- src/Access/QueryTagger.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Access/QueryTagger.php b/src/Access/QueryTagger.php index 009af6a..0de4128 100644 --- a/src/Access/QueryTagger.php +++ b/src/Access/QueryTagger.php @@ -147,14 +147,16 @@ public function tagAccess(SelectInterface $query, string $type) : void { // No embargo. $embargo = $this->database->select('embargo', 'ee'); $embargo->fields('ee', ['embargoed_node']); - $exist_or->condition("existence_node.nid", $embargo, 'NOT IN'); + $embargo->where('existence_node.nid = ee.embargoed_node'); + $exist_or->notExists($embargo); // Embargoed (and allowed). $accessible_embargoes = $this->buildAccessibleEmbargoesQuery(match($type) { 'file', 'media' => EmbargoInterface::EMBARGO_TYPE_FILE, 'node' => EmbargoInterface::EMBARGO_TYPE_NODE, }); - $exist_or->condition("existence_node.nid", $accessible_embargoes, 'IN'); + $accessible_embargoes->where('existence_node.nid = e.embargoed_node'); + $exist_or->exists($accessible_embargoes); $existence->condition($exist_or); From 9a0bdb3d8c7376b771c88055aa6ab162a38cf09e Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Tue, 5 Mar 2024 16:19:38 -0400 Subject: [PATCH 03/17] Further attempt to avoid joins. Track things as metadata on the query? --- src/Access/QueryTagger.php | 71 +++++++++++++++++++++++--------------- 1 file changed, 43 insertions(+), 28 deletions(-) diff --git a/src/Access/QueryTagger.php b/src/Access/QueryTagger.php index 0de4128..d8a0524 100644 --- a/src/Access/QueryTagger.php +++ b/src/Access/QueryTagger.php @@ -26,28 +26,28 @@ class QueryTagger { * * @var \Drupal\Core\Session\AccountProxyInterface */ - protected $user; + protected AccountProxyInterface $user; /** * The IP of the request. * * @var string */ - protected $currentIp; + protected string $currentIp; /** * Instance of a Drupal database connection. * * @var \Drupal\Core\Database\Connection */ - protected $database; + protected Connection $database; /** * The entity type manager. * * @var \Drupal\Core\Entity\EntityTypeManagerInterface */ - protected $entityTypeManager; + protected EntityTypeManagerInterface $entityTypeManager; /** * Time service. @@ -106,6 +106,8 @@ public function tagAccess(SelectInterface $query, string $type) : void { $target_aliases = []; + $tagged_table_aliases = $query->getMetaData('embargo_tagged_table_aliases') ?? []; + foreach ($query->getTables() as $info) { if ($info['table'] instanceof SelectInterface) { continue; @@ -113,7 +115,10 @@ public function tagAccess(SelectInterface $query, string $type) : void { elseif (in_array($info['table'], $tables)) { $key = (str_starts_with($info['table'], "{$type}__")) ? 'entity_id' : (substr($type, 0, 1) . "id"); $alias = $info['alias']; - $target_aliases[] = "{$alias}.{$key}"; + if (!in_array($alias, $tagged_table_aliases)) { + $tagged_table_aliases[] = $alias; + $target_aliases[] = "{$alias}.{$key}"; + } } } @@ -121,15 +126,45 @@ public function tagAccess(SelectInterface $query, string $type) : void { return; } - $existence = $this->database->select('node', 'existence_node'); - $existence->fields('existence_node', ['nid']); + $query->addMetaData('embargo_tagged_table_aliases', $tagged_table_aliases); + $existence = $query->getMetaData('embargo_tagged_existence_query'); + + if (!$existence) { + $existence = $this->database->select('node', 'existence_node'); + $existence->fields('existence_node', ['nid']); + $existence_lut_alias = $existence->leftJoin(LUTGeneratorInterface::TABLE_NAME, 'lut', '%alias.nid = existence_node.nid'); + $query->addMetaData('embargo_tagged_existence_query', $existence); + $query->addMetaData('embargo_lut_alias', $existence_lut_alias); + + $exist_or = $existence->orConditionGroup(); + + // No embargo. + $embargo = $this->database->select('embargo', 'ee'); + $embargo->fields('ee', ['embargoed_node']); + $embargo->where('existence_node.nid = ee.embargoed_node'); + $exist_or->notExists($embargo); + + // Embargoed (and allowed). + $accessible_embargoes = $this->buildAccessibleEmbargoesQuery(match($type) { + 'file', 'media' => EmbargoInterface::EMBARGO_TYPE_FILE, + 'node' => EmbargoInterface::EMBARGO_TYPE_NODE, + }); + $accessible_embargoes->where('existence_node.nid = e.embargoed_node'); + $exist_or->exists($accessible_embargoes); + + $existence->condition($exist_or); + + $query->exists($existence); + } + else { + $existence_lut_alias = $query->getMetaData('embargo_lut_alias'); + } if ($type !== 'node') { $lut_column = match($type) { 'file' => 'fid', 'media' => 'mid', }; - $existence_lut_alias = $existence->leftJoin(LUTGeneratorInterface::TABLE_NAME, 'lut', '%alias.nid = existence_node.nid'); $existence->where(strtr('!field IS NULL OR !field IN (!targets)', [ '!field' => "{$existence_lut_alias}.{$lut_column}", '!targets' => implode(', ', $target_aliases), @@ -141,26 +176,6 @@ public function tagAccess(SelectInterface $query, string $type) : void { '!targets' => implode(', ', $target_aliases), ])); } - - $exist_or = $existence->orConditionGroup(); - - // No embargo. - $embargo = $this->database->select('embargo', 'ee'); - $embargo->fields('ee', ['embargoed_node']); - $embargo->where('existence_node.nid = ee.embargoed_node'); - $exist_or->notExists($embargo); - - // Embargoed (and allowed). - $accessible_embargoes = $this->buildAccessibleEmbargoesQuery(match($type) { - 'file', 'media' => EmbargoInterface::EMBARGO_TYPE_FILE, - 'node' => EmbargoInterface::EMBARGO_TYPE_NODE, - }); - $accessible_embargoes->where('existence_node.nid = e.embargoed_node'); - $exist_or->exists($accessible_embargoes); - - $existence->condition($exist_or); - - $query->exists($existence); } /** From 8465ffe362b5bf6fbc5a48e0cee899b9f71b347a Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Wed, 6 Mar 2024 18:57:22 -0400 Subject: [PATCH 04/17] Found some issues in the test, oddly enough. See: https://github.com/discoverygarden/embargo/pull/6#pullrequestreview-1920893091 Some mixed up assertions, assuming that the ::setUp() method changed at some point mid-implementation, and wasn't carried through? And that it that the existing implementation was indeed bearing a bug. --- .../EmbargoAccessQueryTaggingAlterTest.php | 86 ++++++++++++++++--- 1 file changed, 76 insertions(+), 10 deletions(-) diff --git a/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php b/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php index b053fd8..46eb041 100644 --- a/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php +++ b/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php @@ -3,6 +3,9 @@ namespace Drupal\Tests\embargo\Kernel; use Drupal\embargo\EmbargoInterface; +use Drupal\file\FileInterface; +use Drupal\media\MediaInterface; +use Drupal\node\NodeInterface; use Drupal\Tests\islandora_test_support\Traits\DatabaseQueryTestTraits; /** @@ -20,6 +23,48 @@ class EmbargoAccessQueryTaggingAlterTest extends EmbargoKernelTestBase { */ protected EmbargoInterface $embargo; + /** + * Embargoed node from ::setUp(). + * + * @var \Drupal\node\NodeInterface + */ + protected NodeInterface $embargoedNode; + + /** + * Embargoed media from ::setUp(). + * + * @var \Drupal\media\MediaInterface + */ + protected MediaInterface $embargoedMedia; + + /** + * Embargoed file from ::setUp(). + * + * @var \Drupal\file\FileInterface + */ + protected FileInterface $embargoedFile; + + /** + * Unembargoed node from ::setUp(). + * + * @var \Drupal\node\NodeInterface + */ + protected NodeInterface $unembargoedNode; + + /** + * Unembargoed media from ::setUp(). + * + * @var \Drupal\media\MediaInterface + */ + protected MediaInterface $unembargoedMedia; + + /** + * Unembargoed file from ::setUp(). + * + * @var \Drupal\file\FileInterface + */ + protected FileInterface $unembargoedFile; + /** * {@inheritdoc} */ @@ -27,11 +72,12 @@ public function setUp(): void { parent::setUp(); // Create two nodes one embargoed and one non-embargoed. - $embargoedNode = $this->createNode(); - $this->createMedia($this->createFile(), $embargoedNode); - $this->embargo = $this->createEmbargo($embargoedNode); + $this->embargoedNode = $this->createNode(); + $this->embargoedMedia = $this->createMedia($this->embargoedFile = $this->createFile(), $this->embargoedNode); + $this->embargo = $this->createEmbargo($this->embargoedNode); - $this->createNode(); + $this->unembargoedNode = $this->createNode(); + $this->unembargoedMedia = $this->createMedia($this->unembargoedFile = $this->createFile(), $this->unembargoedNode); } /** @@ -43,6 +89,7 @@ public function testEmbargoNodeQueryAlterAccess() { $query = $this->generateNodeSelectAccessQuery($this->user); $result = $query->execute()->fetchAll(); $this->assertCount(1, $result, 'User can only view non-embargoed node.'); + $this->assertEquals([$this->unembargoedNode->id()], array_column($result, 'nid')); } /** @@ -53,7 +100,8 @@ public function testEmbargoNodeQueryAlterAccess() { public function testNodeEmbargoReferencedMediaAccessQueryAlterAccessDenied() { $query = $this->generateMediaSelectAccessQuery($this->user); $result = $query->execute()->fetchAll(); - $this->assertCount(0, $result, 'Media of embargoed nodes cannot be viewed'); + $this->assertCount(1, $result, 'Media of embargoed nodes cannot be viewed'); + $this->assertEquals([$this->unembargoedMedia->id()], array_column($result, 'mid')); } /** @@ -65,6 +113,7 @@ public function testNodeEmbargoReferencedFileAccessQueryAlterAccessDenied() { $query = $this->generateFileSelectAccessQuery($this->user); $result = $query->execute()->fetchAll(); $this->assertCount(1, $result, 'File of embargoed nodes cannot be viewed'); + $this->assertEquals([$this->unembargoedFile->id()], array_column($result, 'fid')); } /** @@ -80,6 +129,10 @@ public function testDeletedNodeEmbargoNodeAccessQueryAlterAccessAllowed() { $result = $query->execute()->fetchAll(); $this->assertCount(2, $result, 'Non embargoed nodes can be viewed'); + $this->assertEqualsCanonicalizing([ + $this->embargoedNode->id(), + $this->unembargoedNode->id(), + ], array_column($result, 'nid')); } /** @@ -93,8 +146,12 @@ public function testDeletedNodeEmbargoMediaAccessQueryAlterAccessAllowed() { $this->embargo->delete(); $query = $this->generateMediaSelectAccessQuery($this->user); $result = $query->execute()->fetchAll(); - $this->assertCount(1, $result, + $this->assertCount(2, $result, 'Media of non embargoed nodes can be viewed'); + $this->assertEqualsCanonicalizing([ + $this->embargoedMedia->id(), + $this->unembargoedMedia->id(), + ], array_column($result, 'mid')); } /** @@ -111,6 +168,10 @@ public function testDeletedNodeEmbargoFileAccessQueryAlterAccessAllowed() { $result = $query->execute()->fetchAll(); $this->assertCount(2, $result, 'Files of non embargoed nodes can be viewed'); + $this->assertEqualsCanonicalizing([ + $this->embargoedFile->id(), + $this->unembargoedFile->id(), + ], array_column($result, 'fid')); } /** @@ -122,9 +183,10 @@ public function testPublishScheduledEmbargoAccess() { // Create an embargo scheduled to be unpublished in the future. $this->setEmbargoFutureUnpublishDate($this->embargo); - $nodeCount = $this->generateNodeSelectAccessQuery($this->user)->execute()->fetchAll(); - $this->assertCount(1, $nodeCount, + $result = $this->generateNodeSelectAccessQuery($this->user)->execute()->fetchAll(); + $this->assertCount(1, $result, 'Node is still embargoed.'); + $this->assertEqualsCanonicalizing([$this->unembargoedNode->id()], array_column($result, 'nid')); } /** @@ -137,9 +199,13 @@ public function testUnpublishScheduledEmbargoAccess() { // Create an embargo scheduled to be unpublished in the future. $this->setEmbargoPastUnpublishDate($this->embargo); - $nodeCount = $this->generateNodeSelectAccessQuery($this->user)->execute()->fetchAll(); - $this->assertCount(2, $nodeCount, + $result = $this->generateNodeSelectAccessQuery($this->user)->execute()->fetchAll(); + $this->assertCount(2, $result, 'Embargo has been unpublished.'); + $this->assertEqualsCanonicalizing([ + $this->embargoedNode->id(), + $this->unembargoedNode->id(), + ], array_column($result, 'nid')); } } From c484d01f39439290b27c2eab7b276b264d506395 Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Wed, 6 Mar 2024 19:58:06 -0400 Subject: [PATCH 05/17] Presently functional, need to DRY up a bit. ... DRY-ing up in progress, with the trait. --- embargo.module | 20 +-- embargo.services.yml | 7 + src/Access/QueryTagger.php | 116 ++++++++------ src/EmbargoExistenceQueryTrait.php | 78 +++++++++ ...ndoraHierarchicalAccessEventSubscriber.php | 149 ++++++++++++++++++ 5 files changed, 302 insertions(+), 68 deletions(-) create mode 100644 src/EmbargoExistenceQueryTrait.php create mode 100644 src/EventSubscriber/IslandoraHierarchicalAccessEventSubscriber.php diff --git a/embargo.module b/embargo.module index c5b35dc..34cb5e5 100644 --- a/embargo.module +++ b/embargo.module @@ -52,25 +52,7 @@ function embargo_file_download($uri) { function embargo_query_node_access_alter(AlterableInterface $query) { /** @var \Drupal\embargo\Access\QueryTagger $tagger */ $tagger = \Drupal::service('embargo.query_tagger'); - $tagger->tagAccess($query, 'node'); -} - -/** - * Implements hook_query_TAG_alter() for `media_access` tagged queries. - */ -function embargo_query_media_access_alter(AlterableInterface $query) { - /** @var \Drupal\embargo\Access\QueryTagger $tagger */ - $tagger = \Drupal::service('embargo.query_tagger'); - $tagger->tagAccess($query, 'media'); -} - -/** - * Implements hook_query_TAG_alter() for `file_access` tagged queries. - */ -function embargo_query_file_access_alter(AlterableInterface $query) { - /** @var \Drupal\embargo\Access\QueryTagger $tagger */ - $tagger = \Drupal::service('embargo.query_tagger'); - $tagger->tagAccess($query, 'file'); + $tagger->tagNode($query); } /** diff --git a/embargo.services.yml b/embargo.services.yml index 38b4738..b316143 100644 --- a/embargo.services.yml +++ b/embargo.services.yml @@ -31,3 +31,10 @@ services: - '@url_generator' tags: - { name: 'event_subscriber' } + embargo.event_subscriber.islandora_hierarchical_access: + class: Drupal\embargo\EventSubscriber\IslandoraHierarchicalAccessEventSubscriber + factory: [null, 'create'] + arguments: + - '@service_container' + tags: + - { name: 'event_subscriber' } diff --git a/src/Access/QueryTagger.php b/src/Access/QueryTagger.php index d8a0524..6422f0d 100644 --- a/src/Access/QueryTagger.php +++ b/src/Access/QueryTagger.php @@ -87,16 +87,16 @@ public function __construct( * * @param \Drupal\Core\Database\Query\SelectInterface $query * The query being executed. - * @param string $type - * Either "node" or "file". */ - public function tagAccess(SelectInterface $query, string $type) : void { - if (!in_array($type, ['node', 'media', 'file'])) { - throw new \InvalidArgumentException("Unrecognized type '$type'."); + public function tagNode(SelectInterface $query) : void { + if ($query->hasTag('islandora_hierarchical_access_subquery')) { + // Being run as a subquery, we do not want to add again. + return; } - elseif ($this->user->hasPermission('bypass embargo access')) { + if ($this->user->hasPermission('bypass embargo access')) { return; } + $type = 'node'; static::conjunctionQuery($query); @@ -127,54 +127,72 @@ public function tagAccess(SelectInterface $query, string $type) : void { } $query->addMetaData('embargo_tagged_table_aliases', $tagged_table_aliases); - $existence = $query->getMetaData('embargo_tagged_existence_query'); + $existence_query = $query->getMetaData('embargo_tagged_existence_query'); + + if (!$existence_query) { + $existence_query = $this->database->select('node', 'existence_node'); + $existence_query->fields('existence_node', ['nid']); + $query->addMetaData('embargo_tagged_existence_query', $existence_query); + + $query->exists($existence_query); + } + + $existence_query->where(strtr('!field IN (!targets)', [ + '!field' => 'existence_node.nid', + '!targets' => implode(', ', $target_aliases), + ])); - if (!$existence) { - $existence = $this->database->select('node', 'existence_node'); - $existence->fields('existence_node', ['nid']); - $existence_lut_alias = $existence->leftJoin(LUTGeneratorInterface::TABLE_NAME, 'lut', '%alias.nid = existence_node.nid'); - $query->addMetaData('embargo_tagged_existence_query', $existence); - $query->addMetaData('embargo_lut_alias', $existence_lut_alias); + if (!$query->hasTag('embargo_access')) { + $query->addTag('embargo_access'); - $exist_or = $existence->orConditionGroup(); + $embargo_alias = $existence_query->leftJoin('embargo', 'e', '%alias.embargoed_node = existence_node.nid'); + $user_alias = $existence_query->leftJoin('embargo__exempt_users', 'u', "%alias.entity_id = {$embargo_alias}.id"); + $existence_or = $existence_query->orConditionGroup(); // No embargo. - $embargo = $this->database->select('embargo', 'ee'); - $embargo->fields('ee', ['embargoed_node']); - $embargo->where('existence_node.nid = ee.embargoed_node'); - $exist_or->notExists($embargo); - - // Embargoed (and allowed). - $accessible_embargoes = $this->buildAccessibleEmbargoesQuery(match($type) { - 'file', 'media' => EmbargoInterface::EMBARGO_TYPE_FILE, - 'node' => EmbargoInterface::EMBARGO_TYPE_NODE, - }); - $accessible_embargoes->where('existence_node.nid = e.embargoed_node'); - $exist_or->exists($accessible_embargoes); - - $existence->condition($exist_or); - - $query->exists($existence); - } - else { - $existence_lut_alias = $query->getMetaData('embargo_lut_alias'); - } + // XXX: Might have to change to examine one of the fields outside the join + // condition? + $existence_or->isNull("{$embargo_alias}.embargoed_node"); + + // The user is exempt from the embargo. + $existence_or->condition("{$user_alias}.exempt_users_target_id", $this->user->id()); + + // ... the incoming IP is in an exempt range; or... + /** @var \Drupal\embargo\IpRangeStorageInterface $storage */ + $storage = $this->entityTypeManager->getStorage('embargo_ip_range'); + $applicable_ip_ranges = $storage->getApplicableIpRanges($this->currentIp); + if (!empty($applicable_ip_ranges)) { + $existence_or->condition("{$embargo_alias}.exempt_ips", array_keys($applicable_ip_ranges), 'IN'); + } - if ($type !== 'node') { - $lut_column = match($type) { - 'file' => 'fid', - 'media' => 'mid', - }; - $existence->where(strtr('!field IS NULL OR !field IN (!targets)', [ - '!field' => "{$existence_lut_alias}.{$lut_column}", - '!targets' => implode(', ', $target_aliases), - ])); - } - else { - $existence->where(strtr('!field IN (!targets)', [ - '!field' => 'existence_node.nid', - '!targets' => implode(', ', $target_aliases), - ])); + // With embargo, without exemption. + $embargo_and = $existence_or->andConditionGroup(); + + // Has an embargo of a relevant type. + $embargo_and->condition("{$embargo_alias}.embargo_type", EmbargoInterface::EMBARGO_TYPE_NODE); + + $current_date = $this->dateFormatter->format($this->time->getRequestTime(), 'custom', DateTimeItemInterface::DATE_STORAGE_FORMAT); + // No indefinite embargoes or embargoes expiring in the future. + $unexpired_embargo_subquery = $this->database->select('embargo', 'ue') + ->fields('ue', ['embargoed_node']); + $unexpired_embargo_subquery->condition($unexpired_embargo_subquery->orConditionGroup() + ->condition('ue.expiration_type', EmbargoInterface::EXPIRATION_TYPE_INDEFINITE) + ->condition($unexpired_embargo_subquery->andConditionGroup() + ->condition('ue.expiration_type', EmbargoInterface::EXPIRATION_TYPE_SCHEDULED) + ->condition('ue.expiration_date', $current_date, '>') + ) + ); + $embargo_and + ->condition( + "{$embargo_alias}.embargoed_node", + $unexpired_embargo_subquery, + 'NOT IN', + ) + ->condition("{$embargo_alias}.expiration_type", EmbargoInterface::EXPIRATION_TYPE_SCHEDULED) + ->condition("{$embargo_alias}.expiration_date", $current_date, '<='); + + $existence_or->condition($embargo_and); + $existence_query->condition($existence_or); } } diff --git a/src/EmbargoExistenceQueryTrait.php b/src/EmbargoExistenceQueryTrait.php new file mode 100644 index 0000000..8f1ce51 --- /dev/null +++ b/src/EmbargoExistenceQueryTrait.php @@ -0,0 +1,78 @@ +leftJoin('embargo', 'e', "%alias.embargoed_node = {$target_alias}.nid"); + $user_alias = $existence_query->leftJoin('embargo__exempt_users', 'u', "%alias.entity_id = {$embargo_alias}.id"); + $existence_or = $existence_query->orConditionGroup(); + + // No embargo. + // XXX: Might have to change to examine one of the fields outside the join + // condition? + $existence_or->isNull("{$embargo_alias}.embargoed_node"); + + // The user is exempt from the embargo. + $existence_or->condition("{$user_alias}.exempt_users_target_id", $this->user->id()); + + // ... the incoming IP is in an exempt range; or... + /** @var \Drupal\embargo\IpRangeStorageInterface $storage */ + $storage = $this->entityTypeManager->getStorage('embargo_ip_range'); + $applicable_ip_ranges = $storage->getApplicableIpRanges($this->currentIp); + if (!empty($applicable_ip_ranges)) { + $existence_or->condition("{$embargo_alias}.exempt_ips", array_keys($applicable_ip_ranges), 'IN'); + } + + // With embargo, without exemption. + $embargo_and = $existence_or->andConditionGroup(); + + // Has an embargo of a relevant type. + $embargo_and->condition("{$embargo_alias}.embargo_type", $embargo_types, 'IN'); + + $current_date = $this->dateFormatter->format($this->time->getRequestTime(), 'custom', DateTimeItemInterface::DATE_STORAGE_FORMAT); + // No indefinite embargoes or embargoes expiring in the future. + $unexpired_embargo_subquery = $this->database->select('embargo', 'ue') + ->fields('ue', ['embargoed_node']); + $unexpired_embargo_subquery->condition($unexpired_embargo_subquery->orConditionGroup() + ->condition('ue.expiration_type', EmbargoInterface::EXPIRATION_TYPE_INDEFINITE) + ->condition($unexpired_embargo_subquery->andConditionGroup() + ->condition('ue.expiration_type', EmbargoInterface::EXPIRATION_TYPE_SCHEDULED) + ->condition('ue.expiration_date', $current_date, '>') + ) + ); + $embargo_and + ->condition( + "{$embargo_alias}.embargoed_node", + $unexpired_embargo_subquery, + 'NOT IN', + ) + ->condition("{$embargo_alias}.expiration_type", EmbargoInterface::EXPIRATION_TYPE_SCHEDULED) + ->condition("{$embargo_alias}.expiration_date", $current_date, '<='); + + $existence_or->condition($embargo_and); + $existence_query->condition($existence_or); + } + +} diff --git a/src/EventSubscriber/IslandoraHierarchicalAccessEventSubscriber.php b/src/EventSubscriber/IslandoraHierarchicalAccessEventSubscriber.php new file mode 100644 index 0000000..5371f70 --- /dev/null +++ b/src/EventSubscriber/IslandoraHierarchicalAccessEventSubscriber.php @@ -0,0 +1,149 @@ +currentIp = $this->requestStack->getCurrentRequest()->getClientIp(); + } + + /** + * {@inheritDoc} + */ + public static function create(ContainerInterface $container) : self { + return new static( + $container->get('current_user'), + $container->get('request_stack'), + $container->get('database'), + $container->get('entity_type.manager'), + $container->get('datetime.time'), + $container->get('date.formatter'), + ); + } + + /** + * {@inheritDoc} + */ + public static function getSubscribedEvents() : array { + return [ + Event::class => 'processEvent', + ]; + } + + /** + * Process the islandora_hierarchical_access query alter event. + * + * @param \Drupal\islandora_hierarchical_access\Event\Event $event + * The event to process. + */ + public function processEvent(Event $event) : void { + $query = $event->getQuery(); + if ($event->getQuery()->hasTag(static::TAG)) { + return; + } + + $query->addTag(static::TAG); + + if ($this->user->hasPermission('bypass embargo access')) { + return; + } + + /** @var \Drupal\Core\Database\Query\SelectInterface $existence_query */ + $existence_query = $query->getMetaData('islandora_hierarchical_access_tagged_existence_query'); + $embargo_alias = $existence_query->leftJoin('embargo', 'e', '%alias.embargoed_node = lut.nid'); + $user_alias = $existence_query->leftJoin('embargo__exempt_users', 'u', "%alias.entity_id = {$embargo_alias}.id"); + $existence_or = $existence_query->orConditionGroup(); + + // No embargo. + // XXX: Might have to change to examine one of the fields outside the join + // condition? + $existence_or->isNull("{$embargo_alias}.embargoed_node"); + + // The user is exempt from the embargo. + $existence_or->condition("{$user_alias}.exempt_users_target_id", $this->user->id()); + + // ... the incoming IP is in an exempt range; or... + /** @var \Drupal\embargo\IpRangeStorageInterface $storage */ + $storage = $this->entityTypeManager->getStorage('embargo_ip_range'); + $applicable_ip_ranges = $storage->getApplicableIpRanges($this->currentIp); + if (!empty($applicable_ip_ranges)) { + $existence_or->condition("{$embargo_alias}.exempt_ips", array_keys($applicable_ip_ranges), 'IN'); + } + + // With embargo, without exemption. + $embargo_and = $existence_or->andConditionGroup(); + + // Has an embargo of a relevant type. + $embargo_and->condition( + "{$embargo_alias}.embargo_type", + match ($event->getType()) { + 'file', 'media' => [ + EmbargoInterface::EMBARGO_TYPE_FILE, + EmbargoInterface::EMBARGO_TYPE_NODE, + ], + 'node' => [EmbargoInterface::EMBARGO_TYPE_NODE], + }, + 'IN', + ); + + $current_date = $this->dateFormatter->format($this->time->getRequestTime(), 'custom', DateTimeItemInterface::DATE_STORAGE_FORMAT); + // No indefinite embargoes or embargoes expiring in the future. + $unexpired_embargo_subquery = $this->database->select('embargo', 'ue') + ->fields('ue', ['embargoed_node']); + $unexpired_embargo_subquery->condition($unexpired_embargo_subquery->orConditionGroup() + ->condition('ue.expiration_type', EmbargoInterface::EXPIRATION_TYPE_INDEFINITE) + ->condition($unexpired_embargo_subquery->andConditionGroup() + ->condition('ue.expiration_type', EmbargoInterface::EXPIRATION_TYPE_SCHEDULED) + ->condition('ue.expiration_date', $current_date, '>') + ) + ); + $embargo_and + ->condition( + "{$embargo_alias}.embargoed_node", + $unexpired_embargo_subquery, + 'NOT IN', + ) + ->condition("{$embargo_alias}.expiration_type", EmbargoInterface::EXPIRATION_TYPE_SCHEDULED) + ->condition("{$embargo_alias}.expiration_date", $current_date, '<='); + + $existence_or->condition($embargo_and); + $existence_query->condition($existence_or); + } + +} From 0e9937c7ea764eadb6980199eae2e37441114db3 Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Wed, 6 Mar 2024 20:22:29 -0400 Subject: [PATCH 06/17] DRY'd out, still needs coding standards addressed. --- src/Access/QueryTagger.php | 160 +----------------- src/EmbargoExistenceQueryTrait.php | 54 +++++- ...ndoraHierarchicalAccessEventSubscriber.php | 66 +------- 3 files changed, 62 insertions(+), 218 deletions(-) diff --git a/src/Access/QueryTagger.php b/src/Access/QueryTagger.php index 6422f0d..bf713eb 100644 --- a/src/Access/QueryTagger.php +++ b/src/Access/QueryTagger.php @@ -8,10 +8,10 @@ use Drupal\Core\Datetime\DateFormatterInterface; use Drupal\Core\Entity\EntityTypeManagerInterface; use Drupal\Core\Session\AccountProxyInterface; -use Drupal\datetime\Plugin\Field\FieldType\DateTimeItemInterface; +use Drupal\embargo\EmbargoExistenceQueryTrait; use Drupal\embargo\EmbargoInterface; use Drupal\islandora_hierarchical_access\Access\QueryConjunctionTrait; -use Drupal\islandora_hierarchical_access\LUTGeneratorInterface; +use Drupal\islandora_hierarchical_access\TaggedTargetsTrait; use Symfony\Component\HttpFoundation\RequestStack; /** @@ -19,49 +19,9 @@ */ class QueryTagger { + use EmbargoExistenceQueryTrait; use QueryConjunctionTrait; - - /** - * The current user. - * - * @var \Drupal\Core\Session\AccountProxyInterface - */ - protected AccountProxyInterface $user; - - /** - * The IP of the request. - * - * @var string - */ - protected string $currentIp; - - /** - * Instance of a Drupal database connection. - * - * @var \Drupal\Core\Database\Connection - */ - protected Connection $database; - - /** - * The entity type manager. - * - * @var \Drupal\Core\Entity\EntityTypeManagerInterface - */ - protected EntityTypeManagerInterface $entityTypeManager; - - /** - * Time service. - * - * @var \Drupal\Component\Datetime\TimeInterface - */ - protected TimeInterface $time; - - /** - * Date formatter service. - * - * @var \Drupal\Core\Datetime\DateFormatterInterface - */ - protected DateFormatterInterface $dateFormatter; + use TaggedTargetsTrait; /** * Constructor. @@ -90,7 +50,8 @@ public function __construct( */ public function tagNode(SelectInterface $query) : void { if ($query->hasTag('islandora_hierarchical_access_subquery')) { - // Being run as a subquery, we do not want to add again. + // Being run as a subquery: We do not want to touch it as we expect our + // IslandoraHierarchicalAccessEventSubscriber class to deal with it. return; } if ($this->user->hasPermission('bypass embargo access')) { @@ -104,23 +65,9 @@ public function tagNode(SelectInterface $query) : void { $storage = $this->entityTypeManager->getStorage($type); $tables = $storage->getTableMapping()->getTableNames(); - $target_aliases = []; - $tagged_table_aliases = $query->getMetaData('embargo_tagged_table_aliases') ?? []; - foreach ($query->getTables() as $info) { - if ($info['table'] instanceof SelectInterface) { - continue; - } - elseif (in_array($info['table'], $tables)) { - $key = (str_starts_with($info['table'], "{$type}__")) ? 'entity_id' : (substr($type, 0, 1) . "id"); - $alias = $info['alias']; - if (!in_array($alias, $tagged_table_aliases)) { - $tagged_table_aliases[] = $alias; - $target_aliases[] = "{$alias}.{$key}"; - } - } - } + $target_aliases = static::getTaggingTargets($query, $tagged_table_aliases, $tables, $type); if (empty($target_aliases)) { return; @@ -145,99 +92,8 @@ public function tagNode(SelectInterface $query) : void { if (!$query->hasTag('embargo_access')) { $query->addTag('embargo_access'); - $embargo_alias = $existence_query->leftJoin('embargo', 'e', '%alias.embargoed_node = existence_node.nid'); - $user_alias = $existence_query->leftJoin('embargo__exempt_users', 'u', "%alias.entity_id = {$embargo_alias}.id"); - $existence_or = $existence_query->orConditionGroup(); - - // No embargo. - // XXX: Might have to change to examine one of the fields outside the join - // condition? - $existence_or->isNull("{$embargo_alias}.embargoed_node"); - - // The user is exempt from the embargo. - $existence_or->condition("{$user_alias}.exempt_users_target_id", $this->user->id()); - - // ... the incoming IP is in an exempt range; or... - /** @var \Drupal\embargo\IpRangeStorageInterface $storage */ - $storage = $this->entityTypeManager->getStorage('embargo_ip_range'); - $applicable_ip_ranges = $storage->getApplicableIpRanges($this->currentIp); - if (!empty($applicable_ip_ranges)) { - $existence_or->condition("{$embargo_alias}.exempt_ips", array_keys($applicable_ip_ranges), 'IN'); - } - - // With embargo, without exemption. - $embargo_and = $existence_or->andConditionGroup(); - - // Has an embargo of a relevant type. - $embargo_and->condition("{$embargo_alias}.embargo_type", EmbargoInterface::EMBARGO_TYPE_NODE); - - $current_date = $this->dateFormatter->format($this->time->getRequestTime(), 'custom', DateTimeItemInterface::DATE_STORAGE_FORMAT); - // No indefinite embargoes or embargoes expiring in the future. - $unexpired_embargo_subquery = $this->database->select('embargo', 'ue') - ->fields('ue', ['embargoed_node']); - $unexpired_embargo_subquery->condition($unexpired_embargo_subquery->orConditionGroup() - ->condition('ue.expiration_type', EmbargoInterface::EXPIRATION_TYPE_INDEFINITE) - ->condition($unexpired_embargo_subquery->andConditionGroup() - ->condition('ue.expiration_type', EmbargoInterface::EXPIRATION_TYPE_SCHEDULED) - ->condition('ue.expiration_date', $current_date, '>') - ) - ); - $embargo_and - ->condition( - "{$embargo_alias}.embargoed_node", - $unexpired_embargo_subquery, - 'NOT IN', - ) - ->condition("{$embargo_alias}.expiration_type", EmbargoInterface::EXPIRATION_TYPE_SCHEDULED) - ->condition("{$embargo_alias}.expiration_date", $current_date, '<='); - - $existence_or->condition($embargo_and); - $existence_query->condition($existence_or); - } - } - - /** - * Get query to select accessible embargoed entities. - * - * @param int $type - * The type of embargo, expected to be one of: - * - EmbargoInterface::EMBARGO_TYPE_NODE; or, - * - EmbargoInterface::EMBARGO_TYPE_FILE. - * - * @return \Drupal\Core\Database\Query\SelectInterface - * A query returning things that should not be inaccessible. - */ - protected function buildAccessibleEmbargoesQuery(int $type) : SelectInterface { - $query = $this->database->select('embargo', 'e') - ->fields('e', ['embargoed_node']); - - // Things are visible if... - $group = $query->orConditionGroup() - // The selected embargo entity does not apply to the given type; or... - ->condition('e.embargo_type', $type, '!='); - - $group->condition($query->andConditionGroup() - // ... a scheduled embargo... - ->condition('e.expiration_type', EmbargoInterface::EXPIRATION_TYPE_SCHEDULED) - // ... has a date in the past. - ->condition('e.expiration_date', $this->dateFormatter->format($this->time->getRequestTime(), 'custom', DateTimeItemInterface::DATE_STORAGE_FORMAT), '<') - ); - - // ... the incoming IP is in an exempt range; or... - /** @var \Drupal\embargo\IpRangeStorageInterface $storage */ - $storage = $this->entityTypeManager->getStorage('embargo_ip_range'); - $applicable_ip_ranges = $storage->getApplicableIpRanges($this->currentIp); - if (!empty($applicable_ip_ranges)) { - $group->condition('e.exempt_ips', array_keys($applicable_ip_ranges), 'IN'); + $this->applyExistenceQuery($existence_query, 'existence_node', [EmbargoInterface::EMBARGO_TYPE_NODE]); } - - // ... the specific user is exempted from the embargo. - $user_alias = $query->leftJoin('embargo__exempt_users', 'u', 'e.id = %alias.entity_id'); - $group->condition("{$user_alias}.exempt_users_target_id", $this->user->id()); - - $query->condition($group); - - return $query; } } diff --git a/src/EmbargoExistenceQueryTrait.php b/src/EmbargoExistenceQueryTrait.php index 8f1ce51..7b6a9f8 100644 --- a/src/EmbargoExistenceQueryTrait.php +++ b/src/EmbargoExistenceQueryTrait.php @@ -1,6 +1,6 @@ leftJoin('embargo', 'e', "%alias.embargoed_node = {$target_alias}.nid"); $user_alias = $existence_query->leftJoin('embargo__exempt_users', 'u', "%alias.entity_id = {$embargo_alias}.id"); diff --git a/src/EventSubscriber/IslandoraHierarchicalAccessEventSubscriber.php b/src/EventSubscriber/IslandoraHierarchicalAccessEventSubscriber.php index 5371f70..7d46788 100644 --- a/src/EventSubscriber/IslandoraHierarchicalAccessEventSubscriber.php +++ b/src/EventSubscriber/IslandoraHierarchicalAccessEventSubscriber.php @@ -8,7 +8,7 @@ use Drupal\Core\DependencyInjection\ContainerInjectionInterface; use Drupal\Core\Entity\EntityTypeManagerInterface; use Drupal\Core\Session\AccountProxyInterface; -use Drupal\datetime\Plugin\Field\FieldType\DateTimeItemInterface; +use Drupal\embargo\EmbargoExistenceQueryTrait; use Drupal\embargo\EmbargoInterface; use Drupal\islandora_hierarchical_access\Event\Event; use Symfony\Component\DependencyInjection\ContainerInterface; @@ -20,14 +20,9 @@ */ class IslandoraHierarchicalAccessEventSubscriber implements EventSubscriberInterface, ContainerInjectionInterface { - const TAG = 'embargo_access'; + use EmbargoExistenceQueryTrait; - /** - * The IP of the current request. - * - * @var string|null - */ - protected ?string $currentIp; + const TAG = 'embargo_access'; /** * Constructor. @@ -86,64 +81,17 @@ public function processEvent(Event $event) : void { /** @var \Drupal\Core\Database\Query\SelectInterface $existence_query */ $existence_query = $query->getMetaData('islandora_hierarchical_access_tagged_existence_query'); - $embargo_alias = $existence_query->leftJoin('embargo', 'e', '%alias.embargoed_node = lut.nid'); - $user_alias = $existence_query->leftJoin('embargo__exempt_users', 'u', "%alias.entity_id = {$embargo_alias}.id"); - $existence_or = $existence_query->orConditionGroup(); - - // No embargo. - // XXX: Might have to change to examine one of the fields outside the join - // condition? - $existence_or->isNull("{$embargo_alias}.embargoed_node"); - - // The user is exempt from the embargo. - $existence_or->condition("{$user_alias}.exempt_users_target_id", $this->user->id()); - - // ... the incoming IP is in an exempt range; or... - /** @var \Drupal\embargo\IpRangeStorageInterface $storage */ - $storage = $this->entityTypeManager->getStorage('embargo_ip_range'); - $applicable_ip_ranges = $storage->getApplicableIpRanges($this->currentIp); - if (!empty($applicable_ip_ranges)) { - $existence_or->condition("{$embargo_alias}.exempt_ips", array_keys($applicable_ip_ranges), 'IN'); - } - - // With embargo, without exemption. - $embargo_and = $existence_or->andConditionGroup(); - - // Has an embargo of a relevant type. - $embargo_and->condition( - "{$embargo_alias}.embargo_type", + $this->applyExistenceQuery( + $existence_query, + 'lut', match ($event->getType()) { 'file', 'media' => [ EmbargoInterface::EMBARGO_TYPE_FILE, EmbargoInterface::EMBARGO_TYPE_NODE, ], 'node' => [EmbargoInterface::EMBARGO_TYPE_NODE], - }, - 'IN', + } ); - - $current_date = $this->dateFormatter->format($this->time->getRequestTime(), 'custom', DateTimeItemInterface::DATE_STORAGE_FORMAT); - // No indefinite embargoes or embargoes expiring in the future. - $unexpired_embargo_subquery = $this->database->select('embargo', 'ue') - ->fields('ue', ['embargoed_node']); - $unexpired_embargo_subquery->condition($unexpired_embargo_subquery->orConditionGroup() - ->condition('ue.expiration_type', EmbargoInterface::EXPIRATION_TYPE_INDEFINITE) - ->condition($unexpired_embargo_subquery->andConditionGroup() - ->condition('ue.expiration_type', EmbargoInterface::EXPIRATION_TYPE_SCHEDULED) - ->condition('ue.expiration_date', $current_date, '>') - ) - ); - $embargo_and - ->condition( - "{$embargo_alias}.embargoed_node", - $unexpired_embargo_subquery, - 'NOT IN', - ) - ->condition("{$embargo_alias}.expiration_type", EmbargoInterface::EXPIRATION_TYPE_SCHEDULED) - ->condition("{$embargo_alias}.expiration_date", $current_date, '<='); - - $existence_or->condition($embargo_and); - $existence_query->condition($existence_or); } } From 8bfa28cbb5a30198bdbe7764ff66d2c1319a9170 Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Wed, 6 Mar 2024 20:32:35 -0400 Subject: [PATCH 07/17] Bit more DRYing. --- src/Access/QueryTagger.php | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/Access/QueryTagger.php b/src/Access/QueryTagger.php index bf713eb..5b215db 100644 --- a/src/Access/QueryTagger.php +++ b/src/Access/QueryTagger.php @@ -61,13 +61,9 @@ public function tagNode(SelectInterface $query) : void { static::conjunctionQuery($query); - /** @var \Drupal\Core\Entity\Sql\SqlEntityStorageInterface $storage */ - $storage = $this->entityTypeManager->getStorage($type); - $tables = $storage->getTableMapping()->getTableNames(); - $tagged_table_aliases = $query->getMetaData('embargo_tagged_table_aliases') ?? []; - $target_aliases = static::getTaggingTargets($query, $tagged_table_aliases, $tables, $type); + $target_aliases = $this->getTaggingTargets($query, $tagged_table_aliases, $type); if (empty($target_aliases)) { return; From 0231df313d0a6a3e5fb925bc610b3711710a0048 Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Wed, 6 Mar 2024 20:36:05 -0400 Subject: [PATCH 08/17] Misc coding standards. --- .../src/Plugin/migrate/source/Entity.php | 8 +++----- src/EmbargoExistenceQueryTrait.php | 3 +++ tests/src/Kernel/EmbargoKernelTestBase.php | 2 +- tests/src/Kernel/IpRangeEmbargoTest.php | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/modules/migrate_embargoes_to_embargo/src/Plugin/migrate/source/Entity.php b/modules/migrate_embargoes_to_embargo/src/Plugin/migrate/source/Entity.php index 19756df..864c45e 100644 --- a/modules/migrate_embargoes_to_embargo/src/Plugin/migrate/source/Entity.php +++ b/modules/migrate_embargoes_to_embargo/src/Plugin/migrate/source/Entity.php @@ -2,13 +2,11 @@ namespace Drupal\migrate_embargoes_to_embargo\Plugin\migrate\source; -use Drupal\migrate\Plugin\migrate\source\SourcePluginBase; -use Drupal\migrate\Plugin\MigrationInterface; - -use Drupal\Core\Entity\EntityTypeManagerInterface; use Drupal\Core\Entity\EntityTypeInterface; +use Drupal\Core\Entity\EntityTypeManagerInterface; use Drupal\Core\Plugin\ContainerFactoryPluginInterface; - +use Drupal\migrate\Plugin\migrate\source\SourcePluginBase; +use Drupal\migrate\Plugin\MigrationInterface; use Symfony\Component\DependencyInjection\ContainerInterface; /** diff --git a/src/EmbargoExistenceQueryTrait.php b/src/EmbargoExistenceQueryTrait.php index 7b6a9f8..db554cb 100644 --- a/src/EmbargoExistenceQueryTrait.php +++ b/src/EmbargoExistenceQueryTrait.php @@ -10,6 +10,9 @@ use Drupal\Core\Session\AccountProxyInterface; use Drupal\datetime\Plugin\Field\FieldType\DateTimeItemInterface; +/** + * Helper trait; facilitate filtering of embargoed entities. + */ trait EmbargoExistenceQueryTrait { /** diff --git a/tests/src/Kernel/EmbargoKernelTestBase.php b/tests/src/Kernel/EmbargoKernelTestBase.php index c79114b..ce81111 100644 --- a/tests/src/Kernel/EmbargoKernelTestBase.php +++ b/tests/src/Kernel/EmbargoKernelTestBase.php @@ -2,9 +2,9 @@ namespace Drupal\Tests\embargo\Kernel; -use Drupal\embargo\Entity\IpRange; use Drupal\Core\Datetime\DrupalDateTime; use Drupal\embargo\EmbargoInterface; +use Drupal\embargo\Entity\IpRange; use Drupal\embargo\IpRangeInterface; use Drupal\node\NodeInterface; use Drupal\Tests\islandora_test_support\Kernel\AbstractIslandoraKernelTestBase; diff --git a/tests/src/Kernel/IpRangeEmbargoTest.php b/tests/src/Kernel/IpRangeEmbargoTest.php index 9b29a0d..60ef26a 100644 --- a/tests/src/Kernel/IpRangeEmbargoTest.php +++ b/tests/src/Kernel/IpRangeEmbargoTest.php @@ -2,9 +2,9 @@ namespace Drupal\Tests\embargo\Kernel; -use Drupal\node\NodeInterface; use Drupal\embargo\EmbargoInterface; use Drupal\embargo\IpRangeInterface; +use Drupal\node\NodeInterface; /** * Test IpRange embargo. From 5362f87ef17012f557fc5f101925bd3e93132090 Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Wed, 6 Mar 2024 20:41:05 -0400 Subject: [PATCH 09/17] Attempt to pull grab the updated dependency. --- .github/workflows/phpunit.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml index 0589725..bac290c 100644 --- a/.github/workflows/phpunit.yml +++ b/.github/workflows/phpunit.yml @@ -10,3 +10,10 @@ jobs: PHPUnit: uses: discoverygarden/phpunit-action/.github/workflows/phpunit.yml@v1 secrets: inherit + with: + composer_patches: |- + { + "discoverygarden/islandora_hierarchical_access": { + "dependent work from dependency": "https://github.com/discoverygarden/islandora_hierarchical_access/pull/19.patch" + } + } From 7f4e3827de41725cbba1b851373b030a34aaecef Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Thu, 7 Mar 2024 11:15:29 -0400 Subject: [PATCH 10/17] Explicitly acknowledge the media thumbnail file. Better than being left thinking there's a bug where we have undocumentedly expected there to be a file to exist that we didn't explicitly create in our `::setUp()`. --- .../EmbargoAccessQueryTaggingAlterTest.php | 85 ++++++++++++++++--- 1 file changed, 73 insertions(+), 12 deletions(-) diff --git a/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php b/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php index 46eb041..a224e55 100644 --- a/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php +++ b/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php @@ -4,6 +4,7 @@ use Drupal\embargo\EmbargoInterface; use Drupal\file\FileInterface; +use Drupal\media\Entity\Media; use Drupal\media\MediaInterface; use Drupal\node\NodeInterface; use Drupal\Tests\islandora_test_support\Traits\DatabaseQueryTestTraits; @@ -65,6 +66,35 @@ class EmbargoAccessQueryTaggingAlterTest extends EmbargoKernelTestBase { */ protected FileInterface $unembargoedFile; + /** + * Unassociated node from ::setUp(). + * + * @var \Drupal\node\NodeInterface + */ + protected NodeInterface $unassociatedNode; + + /** + * Unassociated media from ::setUp(). + * + * @var \Drupal\media\MediaInterface + */ + protected MediaInterface $unassociatedMedia; + + /** + * Unassociated file from ::setUp(). + * + * @var \Drupal\file\FileInterface + */ + protected FileInterface $unassociatedFile; + + /** + * Lazily created "default thumbnail" image file for (file) media. + * + * @var \Drupal\file\FileInterface + * @see https://git.drupalcode.org/project/drupal/-/blob/cd2c8e49c861a70b0f39b17c01051b16fd6a2662/core/modules/media/src/Entity/Media.php#L203-208 + */ + protected FileInterface $mediaTypeDefaultFile; + /** * {@inheritdoc} */ @@ -78,6 +108,19 @@ public function setUp(): void { $this->unembargoedNode = $this->createNode(); $this->unembargoedMedia = $this->createMedia($this->unembargoedFile = $this->createFile(), $this->unembargoedNode); + + $this->unassociatedNode = $this->createNode(); + $this->unassociatedMedia = Media::create([ + 'bundle' => $this->createMediaType('file', ['id' => 'file_two'])->id(), + ])->setPublished(); + $this->unassociatedMedia->save(); + $this->unassociatedFile = $this->createFile(); + + // XXX: Media lazily creates a "default thumbnail" image file by default. + // @see https://git.drupalcode.org/project/drupal/-/blob/cd2c8e49c861a70b0f39b17c01051b16fd6a2662/core/modules/media/src/Entity/Media.php#L203-208 + $files = $this->storage('file')->loadByProperties(['filename' => 'generic.png']); + $this->assertCount(1, $files, 'only the one generic file.'); + $this->mediaTypeDefaultFile = reset($files); } /** @@ -88,8 +131,11 @@ public function setUp(): void { public function testEmbargoNodeQueryAlterAccess() { $query = $this->generateNodeSelectAccessQuery($this->user); $result = $query->execute()->fetchAll(); - $this->assertCount(1, $result, 'User can only view non-embargoed node.'); - $this->assertEquals([$this->unembargoedNode->id()], array_column($result, 'nid')); + $this->assertCount(2, $result, 'User can only view non-embargoed nodes.'); + $this->assertEqualsCanonicalizing([ + $this->unembargoedNode->id(), + $this->unassociatedNode->id(), + ], array_column($result, 'nid')); } /** @@ -100,8 +146,11 @@ public function testEmbargoNodeQueryAlterAccess() { public function testNodeEmbargoReferencedMediaAccessQueryAlterAccessDenied() { $query = $this->generateMediaSelectAccessQuery($this->user); $result = $query->execute()->fetchAll(); - $this->assertCount(1, $result, 'Media of embargoed nodes cannot be viewed'); - $this->assertEquals([$this->unembargoedMedia->id()], array_column($result, 'mid')); + $this->assertCount(2, $result, 'Media of embargoed nodes cannot be viewed'); + $this->assertEqualsCanonicalizing([ + $this->unembargoedMedia->id(), + $this->unassociatedMedia->id(), + ], array_column($result, 'mid')); } /** @@ -112,8 +161,12 @@ public function testNodeEmbargoReferencedMediaAccessQueryAlterAccessDenied() { public function testNodeEmbargoReferencedFileAccessQueryAlterAccessDenied() { $query = $this->generateFileSelectAccessQuery($this->user); $result = $query->execute()->fetchAll(); - $this->assertCount(1, $result, 'File of embargoed nodes cannot be viewed'); - $this->assertEquals([$this->unembargoedFile->id()], array_column($result, 'fid')); + $this->assertCount(3, $result, 'File of embargoed nodes cannot be viewed'); + $this->assertEqualsCanonicalizing([ + $this->unembargoedFile->id(), + $this->unassociatedFile->id(), + $this->mediaTypeDefaultFile->id(), + ], array_column($result, 'fid')); } /** @@ -128,10 +181,11 @@ public function testDeletedNodeEmbargoNodeAccessQueryAlterAccessAllowed() { $query = $this->generateNodeSelectAccessQuery($this->user); $result = $query->execute()->fetchAll(); - $this->assertCount(2, $result, 'Non embargoed nodes can be viewed'); + $this->assertCount(3, $result, 'Non embargoed nodes can be viewed'); $this->assertEqualsCanonicalizing([ $this->embargoedNode->id(), $this->unembargoedNode->id(), + $this->unassociatedNode->id(), ], array_column($result, 'nid')); } @@ -146,11 +200,12 @@ public function testDeletedNodeEmbargoMediaAccessQueryAlterAccessAllowed() { $this->embargo->delete(); $query = $this->generateMediaSelectAccessQuery($this->user); $result = $query->execute()->fetchAll(); - $this->assertCount(2, $result, + $this->assertCount(3, $result, 'Media of non embargoed nodes can be viewed'); $this->assertEqualsCanonicalizing([ $this->embargoedMedia->id(), $this->unembargoedMedia->id(), + $this->unassociatedMedia->id(), ], array_column($result, 'mid')); } @@ -166,11 +221,13 @@ public function testDeletedNodeEmbargoFileAccessQueryAlterAccessAllowed() { $query = $this->generateFileSelectAccessQuery($this->user); $result = $query->execute()->fetchAll(); - $this->assertCount(2, $result, + $this->assertCount(4, $result, 'Files of non embargoed nodes can be viewed'); $this->assertEqualsCanonicalizing([ $this->embargoedFile->id(), $this->unembargoedFile->id(), + $this->unassociatedFile->id(), + $this->mediaTypeDefaultFile->id(), ], array_column($result, 'fid')); } @@ -184,9 +241,12 @@ public function testPublishScheduledEmbargoAccess() { $this->setEmbargoFutureUnpublishDate($this->embargo); $result = $this->generateNodeSelectAccessQuery($this->user)->execute()->fetchAll(); - $this->assertCount(1, $result, + $this->assertCount(2, $result, 'Node is still embargoed.'); - $this->assertEqualsCanonicalizing([$this->unembargoedNode->id()], array_column($result, 'nid')); + $this->assertEqualsCanonicalizing([ + $this->unembargoedNode->id(), + $this->unassociatedNode->id(), + ], array_column($result, 'nid')); } /** @@ -200,11 +260,12 @@ public function testUnpublishScheduledEmbargoAccess() { $this->setEmbargoPastUnpublishDate($this->embargo); $result = $this->generateNodeSelectAccessQuery($this->user)->execute()->fetchAll(); - $this->assertCount(2, $result, + $this->assertCount(3, $result, 'Embargo has been unpublished.'); $this->assertEqualsCanonicalizing([ $this->embargoedNode->id(), $this->unembargoedNode->id(), + $this->unassociatedNode->id(), ], array_column($result, 'nid')); } From 66490ad858707ad523c0fb10bebaf95dab9d6cfa Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Thu, 7 Mar 2024 12:25:25 -0400 Subject: [PATCH 11/17] Adjust to existence. --- src/EmbargoExistenceQueryTrait.php | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/EmbargoExistenceQueryTrait.php b/src/EmbargoExistenceQueryTrait.php index db554cb..61f26d6 100644 --- a/src/EmbargoExistenceQueryTrait.php +++ b/src/EmbargoExistenceQueryTrait.php @@ -97,7 +97,8 @@ protected function applyExistenceQuery(SelectInterface $existence_query, string $current_date = $this->dateFormatter->format($this->time->getRequestTime(), 'custom', DateTimeItemInterface::DATE_STORAGE_FORMAT); // No indefinite embargoes or embargoes expiring in the future. $unexpired_embargo_subquery = $this->database->select('embargo', 'ue') - ->fields('ue', ['embargoed_node']); + ->fields('ue', ['embargoed_node']) + ->where("ue.embargoed_node = {$embargo_alias}.embargoed_node"); $unexpired_embargo_subquery->condition($unexpired_embargo_subquery->orConditionGroup() ->condition('ue.expiration_type', EmbargoInterface::EXPIRATION_TYPE_INDEFINITE) ->condition($unexpired_embargo_subquery->andConditionGroup() @@ -106,11 +107,7 @@ protected function applyExistenceQuery(SelectInterface $existence_query, string ) ); $embargo_and - ->condition( - "{$embargo_alias}.embargoed_node", - $unexpired_embargo_subquery, - 'NOT IN', - ) + ->notExists($unexpired_embargo_subquery) ->condition("{$embargo_alias}.expiration_type", EmbargoInterface::EXPIRATION_TYPE_SCHEDULED) ->condition("{$embargo_alias}.expiration_date", $current_date, '<='); From e47a6a6e016c392a503f5557737b62988d01d1a4 Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Thu, 7 Mar 2024 20:10:34 -0400 Subject: [PATCH 12/17] Update tests, make things more explicit. --- .../EmbargoAccessQueryTaggingAlterTest.php | 93 +++++++++---------- 1 file changed, 42 insertions(+), 51 deletions(-) diff --git a/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php b/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php index a224e55..2b568a3 100644 --- a/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php +++ b/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php @@ -131,11 +131,11 @@ public function setUp(): void { public function testEmbargoNodeQueryAlterAccess() { $query = $this->generateNodeSelectAccessQuery($this->user); $result = $query->execute()->fetchAll(); - $this->assertCount(2, $result, 'User can only view non-embargoed nodes.'); - $this->assertEqualsCanonicalizing([ - $this->unembargoedNode->id(), - $this->unassociatedNode->id(), - ], array_column($result, 'nid')); + + $ids = array_column($result, 'nid'); + $this->assertNotContains($this->embargoedNode->id(), $ids, 'does not contain embargoed node'); + $this->assertContains($this->unembargoedNode->id(), $ids, 'contains unembargoed node'); + $this->assertContains($this->unassociatedNode->id(), $ids, 'contains unassociated node'); } /** @@ -146,11 +146,11 @@ public function testEmbargoNodeQueryAlterAccess() { public function testNodeEmbargoReferencedMediaAccessQueryAlterAccessDenied() { $query = $this->generateMediaSelectAccessQuery($this->user); $result = $query->execute()->fetchAll(); - $this->assertCount(2, $result, 'Media of embargoed nodes cannot be viewed'); - $this->assertEqualsCanonicalizing([ - $this->unembargoedMedia->id(), - $this->unassociatedMedia->id(), - ], array_column($result, 'mid')); + + $ids = array_column($result, 'mid'); + $this->assertNotContains($this->embargoedMedia->id(), $ids, 'does not contain embargoed media'); + $this->assertContains($this->unembargoedMedia->id(), $ids, 'contains unembargoed media'); + $this->assertContains($this->unassociatedMedia->id(), $ids, 'contains unassociated media'); } /** @@ -161,12 +161,12 @@ public function testNodeEmbargoReferencedMediaAccessQueryAlterAccessDenied() { public function testNodeEmbargoReferencedFileAccessQueryAlterAccessDenied() { $query = $this->generateFileSelectAccessQuery($this->user); $result = $query->execute()->fetchAll(); - $this->assertCount(3, $result, 'File of embargoed nodes cannot be viewed'); - $this->assertEqualsCanonicalizing([ - $this->unembargoedFile->id(), - $this->unassociatedFile->id(), - $this->mediaTypeDefaultFile->id(), - ], array_column($result, 'fid')); + + $ids = array_column($result, 'fid'); + $this->assertNotContains($this->embargoedFile->id(), $ids, 'does not contain embargoed file'); + $this->assertContains($this->unembargoedFile->id(), $ids, 'contains unembargoed file'); + $this->assertContains($this->unassociatedFile->id(), $ids, 'contains unassociated file'); + $this->assertContains($this->mediaTypeDefaultFile->id(), $ids, 'contains default mediatype file'); } /** @@ -181,12 +181,10 @@ public function testDeletedNodeEmbargoNodeAccessQueryAlterAccessAllowed() { $query = $this->generateNodeSelectAccessQuery($this->user); $result = $query->execute()->fetchAll(); - $this->assertCount(3, $result, 'Non embargoed nodes can be viewed'); - $this->assertEqualsCanonicalizing([ - $this->embargoedNode->id(), - $this->unembargoedNode->id(), - $this->unassociatedNode->id(), - ], array_column($result, 'nid')); + $ids = array_column($result, 'nid'); + $this->assertContains($this->embargoedNode->id(), $ids, 'contains formerly embargoed node'); + $this->assertContains($this->unembargoedNode->id(), $ids, 'contains unembargoed node'); + $this->assertContains($this->unassociatedNode->id(), $ids, 'contains unassociated node'); } /** @@ -200,13 +198,11 @@ public function testDeletedNodeEmbargoMediaAccessQueryAlterAccessAllowed() { $this->embargo->delete(); $query = $this->generateMediaSelectAccessQuery($this->user); $result = $query->execute()->fetchAll(); - $this->assertCount(3, $result, - 'Media of non embargoed nodes can be viewed'); - $this->assertEqualsCanonicalizing([ - $this->embargoedMedia->id(), - $this->unembargoedMedia->id(), - $this->unassociatedMedia->id(), - ], array_column($result, 'mid')); + + $ids = array_column($result, 'mid'); + $this->assertContains($this->embargoedMedia->id(), $ids, 'contains formerly embargoed media'); + $this->assertContains($this->unembargoedMedia->id(), $ids, 'contains unembargoed media'); + $this->assertContains($this->unassociatedMedia->id(), $ids, 'contains unassociated media'); } /** @@ -218,17 +214,15 @@ public function testDeletedNodeEmbargoMediaAccessQueryAlterAccessAllowed() { */ public function testDeletedNodeEmbargoFileAccessQueryAlterAccessAllowed() { $this->embargo->delete(); - $query = $this->generateFileSelectAccessQuery($this->user); + $query = $this->generateFileSelectAccessQuery($this->user); $result = $query->execute()->fetchAll(); - $this->assertCount(4, $result, - 'Files of non embargoed nodes can be viewed'); - $this->assertEqualsCanonicalizing([ - $this->embargoedFile->id(), - $this->unembargoedFile->id(), - $this->unassociatedFile->id(), - $this->mediaTypeDefaultFile->id(), - ], array_column($result, 'fid')); + + $ids = array_column($result, 'fid'); + $this->assertContains($this->embargoedFile->id(), $ids, 'contains formerly embargoed file'); + $this->assertContains($this->unembargoedFile->id(), $ids, 'contains unembargoed file'); + $this->assertContains($this->unassociatedFile->id(), $ids, 'contains unassociated file'); + $this->assertContains($this->mediaTypeDefaultFile->id(), $ids, 'contains default mediatype file'); } /** @@ -241,12 +235,11 @@ public function testPublishScheduledEmbargoAccess() { $this->setEmbargoFutureUnpublishDate($this->embargo); $result = $this->generateNodeSelectAccessQuery($this->user)->execute()->fetchAll(); - $this->assertCount(2, $result, - 'Node is still embargoed.'); - $this->assertEqualsCanonicalizing([ - $this->unembargoedNode->id(), - $this->unassociatedNode->id(), - ], array_column($result, 'nid')); + + $ids = array_column($result, 'nid'); + $this->assertNotContains($this->embargoedNode->id(), $ids, 'does not contain embargoed node'); + $this->assertContains($this->unembargoedNode->id(), $ids, 'contains unembargoed node'); + $this->assertContains($this->unassociatedNode->id(), $ids, 'contains unassociated node'); } /** @@ -260,13 +253,11 @@ public function testUnpublishScheduledEmbargoAccess() { $this->setEmbargoPastUnpublishDate($this->embargo); $result = $this->generateNodeSelectAccessQuery($this->user)->execute()->fetchAll(); - $this->assertCount(3, $result, - 'Embargo has been unpublished.'); - $this->assertEqualsCanonicalizing([ - $this->embargoedNode->id(), - $this->unembargoedNode->id(), - $this->unassociatedNode->id(), - ], array_column($result, 'nid')); + + $ids = array_column($result, 'nid'); + $this->assertContains($this->embargoedNode->id(), $ids, 'contains node with expired embargo'); + $this->assertContains($this->unembargoedNode->id(), $ids, 'contains unembargoed node'); + $this->assertContains($this->unassociatedNode->id(), $ids, 'contains unassociated node'); } } From 3bac38fea9bf54533ee2415a4a1499c9599f2237 Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Thu, 7 Mar 2024 20:11:29 -0400 Subject: [PATCH 13/17] Adjust for altered condition structures. --- src/Access/QueryTagger.php | 10 +++++++++- src/EmbargoExistenceQueryTrait.php | 15 ++++++++++++--- ...IslandoraHierarchicalAccessEventSubscriber.php | 5 ++++- 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/src/Access/QueryTagger.php b/src/Access/QueryTagger.php index 5b215db..907153a 100644 --- a/src/Access/QueryTagger.php +++ b/src/Access/QueryTagger.php @@ -88,7 +88,15 @@ public function tagNode(SelectInterface $query) : void { if (!$query->hasTag('embargo_access')) { $query->addTag('embargo_access'); - $this->applyExistenceQuery($existence_query, 'existence_node', [EmbargoInterface::EMBARGO_TYPE_NODE]); + $existence_query->condition($existence_condition = $existence_query->andConditionGroup()) + ->addMetaData('embargo_existence_condition', $existence_condition); + + $this->applyExistenceQuery( + $existence_query, + $existence_condition, + 'existence_node', + [EmbargoInterface::EMBARGO_TYPE_NODE], + ); } } diff --git a/src/EmbargoExistenceQueryTrait.php b/src/EmbargoExistenceQueryTrait.php index 61f26d6..92cdaa8 100644 --- a/src/EmbargoExistenceQueryTrait.php +++ b/src/EmbargoExistenceQueryTrait.php @@ -4,6 +4,7 @@ use Drupal\Component\Datetime\TimeInterface; use Drupal\Core\Database\Connection; +use Drupal\Core\Database\Query\ConditionInterface; use Drupal\Core\Database\Query\SelectInterface; use Drupal\Core\Datetime\DateFormatterInterface; use Drupal\Core\Entity\EntityTypeManagerInterface; @@ -67,10 +68,19 @@ trait EmbargoExistenceQueryTrait { * @param array $embargo_types * The types of embargo to deal with. */ - protected function applyExistenceQuery(SelectInterface $existence_query, string $target_alias, array $embargo_types) { + protected function applyExistenceQuery( + SelectInterface $existence_query, + ConditionInterface $existence_condition, + string $target_alias, + array $embargo_types, + ) { $embargo_alias = $existence_query->leftJoin('embargo', 'e', "%alias.embargoed_node = {$target_alias}.nid"); $user_alias = $existence_query->leftJoin('embargo__exempt_users', 'u', "%alias.entity_id = {$embargo_alias}.id"); - $existence_or = $existence_query->orConditionGroup(); + + + $existence_condition->condition( + $existence_or = $existence_condition->orConditionGroup() + ); // No embargo. // XXX: Might have to change to examine one of the fields outside the join @@ -112,7 +122,6 @@ protected function applyExistenceQuery(SelectInterface $existence_query, string ->condition("{$embargo_alias}.expiration_date", $current_date, '<='); $existence_or->condition($embargo_and); - $existence_query->condition($existence_or); } } diff --git a/src/EventSubscriber/IslandoraHierarchicalAccessEventSubscriber.php b/src/EventSubscriber/IslandoraHierarchicalAccessEventSubscriber.php index 7d46788..532836d 100644 --- a/src/EventSubscriber/IslandoraHierarchicalAccessEventSubscriber.php +++ b/src/EventSubscriber/IslandoraHierarchicalAccessEventSubscriber.php @@ -81,8 +81,11 @@ public function processEvent(Event $event) : void { /** @var \Drupal\Core\Database\Query\SelectInterface $existence_query */ $existence_query = $query->getMetaData('islandora_hierarchical_access_tagged_existence_query'); + /** @var \Drupal\Core\Database\Query\ConditionInterface $existence_condition */ + $existence_condition = $existence_query->getMetaData('islandora_hierarchical_access_tagged_existence_condition'); $this->applyExistenceQuery( $existence_query, + $existence_condition, 'lut', match ($event->getType()) { 'file', 'media' => [ @@ -90,7 +93,7 @@ public function processEvent(Event $event) : void { EmbargoInterface::EMBARGO_TYPE_NODE, ], 'node' => [EmbargoInterface::EMBARGO_TYPE_NODE], - } + }, ); } From 7507e6ba37144c1328014e7fb78c58c64c87ac65 Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Sat, 9 Mar 2024 13:33:36 -0400 Subject: [PATCH 14/17] Functional. --- src/Access/QueryTagger.php | 29 +----- src/EmbargoExistenceQueryTrait.php | 97 +++++++++++++------ ...ndoraHierarchicalAccessEventSubscriber.php | 7 +- 3 files changed, 68 insertions(+), 65 deletions(-) diff --git a/src/Access/QueryTagger.php b/src/Access/QueryTagger.php index 907153a..202c158 100644 --- a/src/Access/QueryTagger.php +++ b/src/Access/QueryTagger.php @@ -70,34 +70,7 @@ public function tagNode(SelectInterface $query) : void { } $query->addMetaData('embargo_tagged_table_aliases', $tagged_table_aliases); - $existence_query = $query->getMetaData('embargo_tagged_existence_query'); - - if (!$existence_query) { - $existence_query = $this->database->select('node', 'existence_node'); - $existence_query->fields('existence_node', ['nid']); - $query->addMetaData('embargo_tagged_existence_query', $existence_query); - - $query->exists($existence_query); - } - - $existence_query->where(strtr('!field IN (!targets)', [ - '!field' => 'existence_node.nid', - '!targets' => implode(', ', $target_aliases), - ])); - - if (!$query->hasTag('embargo_access')) { - $query->addTag('embargo_access'); - - $existence_query->condition($existence_condition = $existence_query->andConditionGroup()) - ->addMetaData('embargo_existence_condition', $existence_condition); - - $this->applyExistenceQuery( - $existence_query, - $existence_condition, - 'existence_node', - [EmbargoInterface::EMBARGO_TYPE_NODE], - ); - } + $this->applyExistenceQuery($query, $target_aliases, [EmbargoInterface::EMBARGO_TYPE_NODE]); } } diff --git a/src/EmbargoExistenceQueryTrait.php b/src/EmbargoExistenceQueryTrait.php index 92cdaa8..2ec2b8b 100644 --- a/src/EmbargoExistenceQueryTrait.php +++ b/src/EmbargoExistenceQueryTrait.php @@ -61,54 +61,83 @@ trait EmbargoExistenceQueryTrait { /** * Helper; apply existence checks to a node(-like) table. * - * @param \Drupal\Core\Database\Query\SelectInterface $existence_query - * The query to which to add. - * @param string $target_alias + * @param string[] $target_aliases * The alias of the node-like table in the query to which to attach things. * @param array $embargo_types * The types of embargo to deal with. */ protected function applyExistenceQuery( - SelectInterface $existence_query, ConditionInterface $existence_condition, - string $target_alias, + array $target_aliases, array $embargo_types, - ) { - $embargo_alias = $existence_query->leftJoin('embargo', 'e', "%alias.embargoed_node = {$target_alias}.nid"); - $user_alias = $existence_query->leftJoin('embargo__exempt_users', 'u', "%alias.entity_id = {$embargo_alias}.id"); - - + ) : void { $existence_condition->condition( - $existence_or = $existence_condition->orConditionGroup() + $existence_condition->orConditionGroup() + ->notExists($this->getNullQuery($target_aliases, $embargo_types)) + ->exists($this->getAccessibleEmbargoesQuery($target_aliases, $embargo_types)) ); + } + + protected function getNullQuery(array $target_aliases, array $embargo_types) : SelectInterface { + $embargo_alias = 'embargo_null'; + $query = $this->database->select('embargo', $embargo_alias); + $query->addExpression(1, 'embargo_null_e'); + + $query->where(strtr('!field IN (!targets)', [ + '!field' => "{$embargo_alias}.embargoed_node", + '!targets' => implode(', ', $target_aliases), + ])); + $query->condition("{$embargo_alias}.embargo_type", $embargo_types, 'IN'); + + return $query; + } - // No embargo. - // XXX: Might have to change to examine one of the fields outside the join - // condition? - $existence_or->isNull("{$embargo_alias}.embargoed_node"); + protected function getAccessibleEmbargoesQuery(array $target_aliases, array $embargo_types) : SelectInterface { + // Embargo exists for the entity, where: + $embargo_alias = 'embargo_existence'; + $embargo_existence = $this->database->select('embargo', $embargo_alias); + $embargo_existence->addExpression(1, 'embargo_allowed'); + + $embargo_existence->addMetaData('embargo_alias', $embargo_alias); + + $replacements = [ + '!field' => "{$embargo_alias}.embargoed_node", + '!targets' => implode(', ', $target_aliases), + ]; + $embargo_existence->condition( + $embargo_existence->orConditionGroup() + ->condition($existence_condition = $embargo_existence->andConditionGroup() + ->where(strtr('!field IN (!targets)', $replacements)) + ->condition($embargo_or = $embargo_existence->orConditionGroup()) + ) + ); - // The user is exempt from the embargo. - $existence_or->condition("{$user_alias}.exempt_users_target_id", $this->user->id()); + $embargo_existence->addMetaData('embargo_existence_condition', $existence_condition); - // ... the incoming IP is in an exempt range; or... + // - The request IP is exempt. /** @var \Drupal\embargo\IpRangeStorageInterface $storage */ $storage = $this->entityTypeManager->getStorage('embargo_ip_range'); $applicable_ip_ranges = $storage->getApplicableIpRanges($this->currentIp); - if (!empty($applicable_ip_ranges)) { - $existence_or->condition("{$embargo_alias}.exempt_ips", array_keys($applicable_ip_ranges), 'IN'); + if ($applicable_ip_ranges) { + $embargo_or->condition("{$embargo_alias}.exempt_ips", array_keys($applicable_ip_ranges), 'IN'); } - // With embargo, without exemption. - $embargo_and = $existence_or->andConditionGroup(); - - // Has an embargo of a relevant type. - $embargo_and->condition("{$embargo_alias}.embargo_type", $embargo_types, 'IN'); + // - The user is exempt. + // @todo Should the IP range constraint(s) take precedence? + $user_existence = $this->database->select('embargo__exempt_users', 'eeu'); + $user_existence->addExpression(1, 'user_existence'); + $user_existence->where("eeu.entity_id = {$embargo_alias}.id") + ->condition('eeu.exempt_users_target_id', $this->user->id()); + $embargo_or->exists($user_existence); + // - There's a scheduled embargo of an appropriate type and no other + // overriding embargo. $current_date = $this->dateFormatter->format($this->time->getRequestTime(), 'custom', DateTimeItemInterface::DATE_STORAGE_FORMAT); // No indefinite embargoes or embargoes expiring in the future. $unexpired_embargo_subquery = $this->database->select('embargo', 'ue') - ->fields('ue', ['embargoed_node']) - ->where("ue.embargoed_node = {$embargo_alias}.embargoed_node"); + ->where("ue.embargoed_node = {$embargo_alias}.embargoed_node") + ->condition('ue.embargo_type', $embargo_types, 'IN'); + $unexpired_embargo_subquery->addExpression(1, 'ueee'); $unexpired_embargo_subquery->condition($unexpired_embargo_subquery->orConditionGroup() ->condition('ue.expiration_type', EmbargoInterface::EXPIRATION_TYPE_INDEFINITE) ->condition($unexpired_embargo_subquery->andConditionGroup() @@ -116,12 +145,16 @@ protected function applyExistenceQuery( ->condition('ue.expiration_date', $current_date, '>') ) ); - $embargo_and - ->notExists($unexpired_embargo_subquery) - ->condition("{$embargo_alias}.expiration_type", EmbargoInterface::EXPIRATION_TYPE_SCHEDULED) - ->condition("{$embargo_alias}.expiration_date", $current_date, '<='); - $existence_or->condition($embargo_and); + $embargo_or->condition( + $embargo_or->andConditionGroup() + ->condition("{$embargo_alias}.embargo_type", $embargo_types, 'IN') + ->condition("{$embargo_alias}.expiration_type", EmbargoInterface::EXPIRATION_TYPE_SCHEDULED) + ->condition("{$embargo_alias}.expiration_date", $current_date, '<=') + ->notExists($unexpired_embargo_subquery) + ); + + return $embargo_existence; } } diff --git a/src/EventSubscriber/IslandoraHierarchicalAccessEventSubscriber.php b/src/EventSubscriber/IslandoraHierarchicalAccessEventSubscriber.php index 532836d..bad9e35 100644 --- a/src/EventSubscriber/IslandoraHierarchicalAccessEventSubscriber.php +++ b/src/EventSubscriber/IslandoraHierarchicalAccessEventSubscriber.php @@ -79,14 +79,11 @@ public function processEvent(Event $event) : void { return; } - /** @var \Drupal\Core\Database\Query\SelectInterface $existence_query */ - $existence_query = $query->getMetaData('islandora_hierarchical_access_tagged_existence_query'); /** @var \Drupal\Core\Database\Query\ConditionInterface $existence_condition */ - $existence_condition = $existence_query->getMetaData('islandora_hierarchical_access_tagged_existence_condition'); + $existence_condition = $query->getMetaData('islandora_hierarchical_access_tagged_existence_condition'); $this->applyExistenceQuery( - $existence_query, $existence_condition, - 'lut', + ['lut_exist.nid'], match ($event->getType()) { 'file', 'media' => [ EmbargoInterface::EMBARGO_TYPE_FILE, From 19bb47c2d7c389a2b19169bbd78ef354e51b3464 Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Sat, 9 Mar 2024 13:36:47 -0400 Subject: [PATCH 15/17] Coding standards. --- src/EmbargoExistenceQueryTrait.php | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/EmbargoExistenceQueryTrait.php b/src/EmbargoExistenceQueryTrait.php index 2ec2b8b..de6b77c 100644 --- a/src/EmbargoExistenceQueryTrait.php +++ b/src/EmbargoExistenceQueryTrait.php @@ -61,6 +61,8 @@ trait EmbargoExistenceQueryTrait { /** * Helper; apply existence checks to a node(-like) table. * + * @param \Drupal\Core\Database\Query\ConditionInterface $existence_condition + * The condition object to which to add the existence check. * @param string[] $target_aliases * The alias of the node-like table in the query to which to attach things. * @param array $embargo_types @@ -78,6 +80,17 @@ protected function applyExistenceQuery( ); } + /** + * Get query for negative assertion. + * + * @param array $target_aliases + * The target aliases on which to match. + * @param array $embargo_types + * The relevant types of embargoes to which to constrain. + * + * @return \Drupal\Core\Database\Query\SelectInterface + * The negative-asserting query. + */ protected function getNullQuery(array $target_aliases, array $embargo_types) : SelectInterface { $embargo_alias = 'embargo_null'; $query = $this->database->select('embargo', $embargo_alias); @@ -92,6 +105,17 @@ protected function getNullQuery(array $target_aliases, array $embargo_types) : S return $query; } + /** + * Get query for positive assertion. + * + * @param array $target_aliases + * The target aliases on which to match. + * @param array $embargo_types + * The relevant types of embargoes to which to constrain. + * + * @return \Drupal\Core\Database\Query\SelectInterface + * The positive-asserting query. + */ protected function getAccessibleEmbargoesQuery(array $target_aliases, array $embargo_types) : SelectInterface { // Embargo exists for the entity, where: $embargo_alias = 'embargo_existence'; From 19abd0367826497b48df6274bda638527ce0c5dc Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Sat, 9 Mar 2024 16:14:42 -0400 Subject: [PATCH 16/17] Test out the query tagging for IP range statements. Bit of formatting, and using constants. --- tests/src/Kernel/IpRangeEmbargoTest.php | 29 +++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/tests/src/Kernel/IpRangeEmbargoTest.php b/tests/src/Kernel/IpRangeEmbargoTest.php index 60ef26a..a858416 100644 --- a/tests/src/Kernel/IpRangeEmbargoTest.php +++ b/tests/src/Kernel/IpRangeEmbargoTest.php @@ -5,6 +5,7 @@ use Drupal\embargo\EmbargoInterface; use Drupal\embargo\IpRangeInterface; use Drupal\node\NodeInterface; +use Drupal\Tests\islandora_test_support\Traits\DatabaseQueryTestTraits; /** * Test IpRange embargo. @@ -13,6 +14,8 @@ */ class IpRangeEmbargoTest extends EmbargoKernelTestBase { + use DatabaseQueryTestTraits; + /** * Embargo for test. * @@ -105,8 +108,16 @@ public function setUp(): void { $this->embargoedNodeWithDifferentIpRange = $this->createNode(); $this->currentIpRangeEntity = $this->createIpRangeEntity($this->ipRange); $this->embargoWithoutIpRange = $this->createEmbargo($this->embargoedNodeWithoutIpRange); - $this->embargoWithCurrentIpRange = $this->createEmbargo($this->embargoedNodeWithCurrentIpRange, 1, $this->currentIpRangeEntity); - $this->embargoWithDifferentIpRange = $this->createEmbargo($this->embargoedNodeWithDifferentIpRange, 1, $this->createIpRangeEntity('0.0.0.0.1/29')); + $this->embargoWithCurrentIpRange = $this->createEmbargo( + $this->embargoedNodeWithCurrentIpRange, + EmbargoInterface::EMBARGO_TYPE_NODE, + $this->currentIpRangeEntity, + ); + $this->embargoWithDifferentIpRange = $this->createEmbargo( + $this->embargoedNodeWithDifferentIpRange, + EmbargoInterface::EMBARGO_TYPE_NODE, + $this->createIpRangeEntity('0.0.0.1/29'), + ); } /** @@ -127,4 +138,18 @@ public function testIpRangeEmbargoNodeAccess() { $this->assertTrue($this->embargoedNodeWithCurrentIpRange->access('view', $this->user)); } + /** + * Test IP range query tagging. + */ + public function testIpRangeQueryTagging() { + $results = $this->generateNodeSelectAccessQuery($this->user)->execute()->fetchAll(); + + $ids = array_column($results, 'nid'); + + $this->assertContains($this->nonEmbargoedNode->id(), $ids, 'non-embargoed node present'); + $this->assertNotContains($this->embargoedNodeWithoutIpRange->id(), $ids, 'generally embargoed node absent'); + $this->assertNotContains($this->embargoedNodeWithDifferentIpRange->id(), $ids, 'node exempted to other ranges absent'); + $this->assertContains($this->embargoedNodeWithCurrentIpRange->id(), $ids, 'node exempted to our range present'); + } + } From be8a746805a79ad11ecb0d41f32110025fcecc72 Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Sat, 9 Mar 2024 16:34:28 -0400 Subject: [PATCH 17/17] Roll some tests where multiple embargoes are involved. Other embargoes block access (but exemptions from _any_ are let through). --- .../EmbargoAccessQueryTaggingAlterTest.php | 46 ++++++++++++++++++- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php b/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php index 2b568a3..82c912d 100644 --- a/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php +++ b/tests/src/Kernel/EmbargoAccessQueryTaggingAlterTest.php @@ -231,7 +231,7 @@ public function testDeletedNodeEmbargoFileAccessQueryAlterAccessAllowed() { * @throws \Drupal\Core\Entity\EntityStorageException */ public function testPublishScheduledEmbargoAccess() { - // Create an embargo scheduled to be unpublished in the future. + // Create an embargo scheduled to be published in the future. $this->setEmbargoFutureUnpublishDate($this->embargo); $result = $this->generateNodeSelectAccessQuery($this->user)->execute()->fetchAll(); @@ -243,7 +243,7 @@ public function testPublishScheduledEmbargoAccess() { } /** - * Tests embargo scheduled to be unpublished in the past. + * Test embargo scheduled in the past, without any other embargo. * * @throws \Drupal\Core\Entity\EntityStorageException */ @@ -260,4 +260,46 @@ public function testUnpublishScheduledEmbargoAccess() { $this->assertContains($this->unassociatedNode->id(), $ids, 'contains unassociated node'); } + /** + * Test embargo scheduled in the past with another relevant scheduled embargo. + * + * @throws \Drupal\Core\Entity\EntityStorageException + */ + public function testUnpublishScheduledWithPublishedEmbargoAccess() { + $this->embargo->setExpirationType(EmbargoInterface::EXPIRATION_TYPE_SCHEDULED)->save(); + // Create an embargo scheduled to be unpublished in the future. + $this->setEmbargoPastUnpublishDate($this->embargo); + + $embargo = $this->createEmbargo($this->embargoedNode); + $embargo->setExpirationType(EmbargoInterface::EXPIRATION_TYPE_SCHEDULED)->save(); + $this->setEmbargoFutureUnpublishDate($embargo); + + $result = $this->generateNodeSelectAccessQuery($this->user)->execute()->fetchAll(); + + $ids = array_column($result, 'nid'); + $this->assertNotContains($this->embargoedNode->id(), $ids, 'does not contain node with expired embargo having other schedule embargo in future'); + $this->assertContains($this->unembargoedNode->id(), $ids, 'contains unembargoed node'); + $this->assertContains($this->unassociatedNode->id(), $ids, 'contains unassociated node'); + } + + /** + * Test embargo scheduled in the past, but with a separate indefinite embargo. + * + * @throws \Drupal\Core\Entity\EntityStorageException + */ + public function testUnpublishScheduledWithIndefiniteEmbargoAccess() { + $this->embargo->setExpirationType(EmbargoInterface::EXPIRATION_TYPE_SCHEDULED)->save(); + // Create an embargo scheduled to be unpublished in the future. + $this->setEmbargoPastUnpublishDate($this->embargo); + + $this->createEmbargo($this->embargoedNode); + + $result = $this->generateNodeSelectAccessQuery($this->user)->execute()->fetchAll(); + + $ids = array_column($result, 'nid'); + $this->assertNotContains($this->embargoedNode->id(), $ids, 'does not contain node with expired embargo having other indefinite embargo'); + $this->assertContains($this->unembargoedNode->id(), $ids, 'contains unembargoed node'); + $this->assertContains($this->unassociatedNode->id(), $ids, 'contains unassociated node'); + } + }