From 1e37cdca15458470a00d164a00d05e2700b11d97 Mon Sep 17 00:00:00 2001 From: Vinu Varshith S Date: Fri, 14 Aug 2020 06:43:12 +0530 Subject: [PATCH 01/10] Fix access condition check in SubscriptionController --- src/Controller/SubscriptionController.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Controller/SubscriptionController.php b/src/Controller/SubscriptionController.php index 279a19219..1ef845123 100644 --- a/src/Controller/SubscriptionController.php +++ b/src/Controller/SubscriptionController.php @@ -136,7 +136,8 @@ public function subscribe($entity_type_id, EntityInterface $group, OgMembershipT return new RedirectResponse($group->toUrl()->setAbsolute(TRUE)->toString()); } - if (!$this->ogAccess->userAccess($group, 'subscribe', $user) && !$this->ogAccess->userAccess($group, 'subscribe without approval', $user)) { + if ((($access = $this->ogAccess->userAccess($group, 'subscribe', $user)) && $access->isForbidden()) && + (($access = $this->ogAccess->userAccess($group, 'subscribe without approval', $user)) && $access->isForbidden())) { throw new AccessDeniedHttpException(); } From 5dcfae657acdf3a1a3a4b2d022885102aa391ad2 Mon Sep 17 00:00:00 2001 From: Vinu Varshith S Date: Fri, 14 Aug 2020 17:02:46 +0530 Subject: [PATCH 02/10] Review comment: Code improvement --- src/Controller/SubscriptionController.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Controller/SubscriptionController.php b/src/Controller/SubscriptionController.php index 1ef845123..b28449141 100644 --- a/src/Controller/SubscriptionController.php +++ b/src/Controller/SubscriptionController.php @@ -136,8 +136,9 @@ public function subscribe($entity_type_id, EntityInterface $group, OgMembershipT return new RedirectResponse($group->toUrl()->setAbsolute(TRUE)->toString()); } - if ((($access = $this->ogAccess->userAccess($group, 'subscribe', $user)) && $access->isForbidden()) && - (($access = $this->ogAccess->userAccess($group, 'subscribe without approval', $user)) && $access->isForbidden())) { + $subscribe = $this->ogAccess->userAccess($group, 'subscribe', $user); + $subscribeWithoutApproval = $this->ogAccess->userAccess($group, 'subscribe without approval', $user); + if ($subscribe->isForbidden() && $subscribeWithoutApproval->isForbidden()) { throw new AccessDeniedHttpException(); } From 5e0a53482cf98f242f160c3ebce630a9e8d1d493 Mon Sep 17 00:00:00 2001 From: Maarten Segers Date: Fri, 14 Aug 2020 17:06:03 +0200 Subject: [PATCH 03/10] Remove redundant $user argument --- src/Controller/SubscriptionController.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Controller/SubscriptionController.php b/src/Controller/SubscriptionController.php index b28449141..d35698cf7 100644 --- a/src/Controller/SubscriptionController.php +++ b/src/Controller/SubscriptionController.php @@ -136,8 +136,8 @@ public function subscribe($entity_type_id, EntityInterface $group, OgMembershipT return new RedirectResponse($group->toUrl()->setAbsolute(TRUE)->toString()); } - $subscribe = $this->ogAccess->userAccess($group, 'subscribe', $user); - $subscribeWithoutApproval = $this->ogAccess->userAccess($group, 'subscribe without approval', $user); + $subscribe = $this->ogAccess->userAccess($group, 'subscribe'); + $subscribeWithoutApproval = $this->ogAccess->userAccess($group, 'subscribe without approval'); if ($subscribe->isForbidden() && $subscribeWithoutApproval->isForbidden()) { throw new AccessDeniedHttpException(); } From e7259bf8797c83dd42f3bb10f54ffca4a2c1684d Mon Sep 17 00:00:00 2001 From: Maarten Segers Date: Fri, 14 Aug 2020 17:07:17 +0200 Subject: [PATCH 04/10] Get the current user No need to load the user entity again. --- src/Controller/SubscriptionController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Controller/SubscriptionController.php b/src/Controller/SubscriptionController.php index d35698cf7..7643137fc 100644 --- a/src/Controller/SubscriptionController.php +++ b/src/Controller/SubscriptionController.php @@ -86,7 +86,7 @@ public function subscribe($entity_type_id, EntityInterface $group, OgMembershipT throw new AccessDeniedHttpException(); } - $user = $this->entityTypeManager()->getStorage('user')->load($this->currentUser()->id()); + $user = $this->currentUser(); if ($user->isAnonymous()) { // Anonymous user can't request membership. From 4a513b51ca39064c88b8bc5ae1e523a4142798a8 Mon Sep 17 00:00:00 2001 From: Maarten Segers Date: Fri, 14 Aug 2020 17:07:56 +0200 Subject: [PATCH 05/10] Use snake case Co-authored-by: Amitai Burstein --- src/Controller/SubscriptionController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Controller/SubscriptionController.php b/src/Controller/SubscriptionController.php index 7643137fc..87f116c18 100644 --- a/src/Controller/SubscriptionController.php +++ b/src/Controller/SubscriptionController.php @@ -137,7 +137,7 @@ public function subscribe($entity_type_id, EntityInterface $group, OgMembershipT } $subscribe = $this->ogAccess->userAccess($group, 'subscribe'); - $subscribeWithoutApproval = $this->ogAccess->userAccess($group, 'subscribe without approval'); + $subscribe_without_approval = $this->ogAccess->userAccess($group, 'subscribe without approval'); if ($subscribe->isForbidden() && $subscribeWithoutApproval->isForbidden()) { throw new AccessDeniedHttpException(); } From 4ef3d6fc759a05114f5f0cc877c051616adf0d84 Mon Sep 17 00:00:00 2001 From: Maarten Segers Date: Fri, 14 Aug 2020 17:08:20 +0200 Subject: [PATCH 06/10] Use snake case --- src/Controller/SubscriptionController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Controller/SubscriptionController.php b/src/Controller/SubscriptionController.php index 87f116c18..3dfae940b 100644 --- a/src/Controller/SubscriptionController.php +++ b/src/Controller/SubscriptionController.php @@ -138,7 +138,7 @@ public function subscribe($entity_type_id, EntityInterface $group, OgMembershipT $subscribe = $this->ogAccess->userAccess($group, 'subscribe'); $subscribe_without_approval = $this->ogAccess->userAccess($group, 'subscribe without approval'); - if ($subscribe->isForbidden() && $subscribeWithoutApproval->isForbidden()) { + if ($subscribe->isForbidden() && $subscribe_without_approval->isForbidden()) { throw new AccessDeniedHttpException(); } From 41a2b7b8ca689bc03de1e7a0e0a17465f00080e9 Mon Sep 17 00:00:00 2001 From: Maarten Segers Date: Fri, 14 Aug 2020 18:50:46 +0200 Subject: [PATCH 07/10] Revert loading user --- src/Controller/SubscriptionController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Controller/SubscriptionController.php b/src/Controller/SubscriptionController.php index 3dfae940b..04d222ada 100644 --- a/src/Controller/SubscriptionController.php +++ b/src/Controller/SubscriptionController.php @@ -86,7 +86,7 @@ public function subscribe($entity_type_id, EntityInterface $group, OgMembershipT throw new AccessDeniedHttpException(); } - $user = $this->currentUser(); + $user = $this->entityTypeManager()->getStorage('user')->load($this->currentUser()->id()); if ($user->isAnonymous()) { // Anonymous user can't request membership. From 93b7eb8eea9d562cb9eff137ca71c5d5d9c2e394 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Sat, 15 Aug 2020 20:03:02 +0300 Subject: [PATCH 08/10] Document why we are doing an interface check before adding cacheability metadata. --- src/Event/AccessEventBase.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Event/AccessEventBase.php b/src/Event/AccessEventBase.php index 740864ce8..520c24cb8 100644 --- a/src/Event/AccessEventBase.php +++ b/src/Event/AccessEventBase.php @@ -88,6 +88,8 @@ public function getUser(): AccountInterface { public function getAccessResult(): AccessResultInterface { $access = $this->access; + // Enrich the access result object with our cacheability metadata in case it + // supports it. if ($access instanceof RefinableCacheableDependencyInterface) { $access->addCacheableDependency($this); } From 53f2461d962d331dde4f46871ae9f38de982ec45 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Sun, 16 Aug 2020 17:22:25 +0300 Subject: [PATCH 09/10] Test that access to the subscribe form is denied for groups that do not allow to subscribe. --- tests/src/Functional/GroupSubscribeTest.php | 49 +++++++++++++++++++-- 1 file changed, 45 insertions(+), 4 deletions(-) diff --git a/tests/src/Functional/GroupSubscribeTest.php b/tests/src/Functional/GroupSubscribeTest.php index 63b1b3295..79d69fd06 100644 --- a/tests/src/Functional/GroupSubscribeTest.php +++ b/tests/src/Functional/GroupSubscribeTest.php @@ -62,6 +62,13 @@ class GroupSubscribeTest extends BrowserTestBase { */ protected $group4; + /** + * Test entity group. + * + * @var \Drupal\node\NodeInterface + */ + protected $group5; + /** * A group bundle name. * @@ -76,6 +83,13 @@ class GroupSubscribeTest extends BrowserTestBase { */ protected $groupBundle2; + /** + * A group bundle name. + * + * @var string + */ + protected $groupBundle3; + /** * A membership type bundle name. * @@ -108,6 +122,8 @@ protected function setUp(): void { NodeType::create(['type' => $this->groupBundle1])->save(); $this->groupBundle2 = mb_strtolower($this->randomMachineName()); NodeType::create(['type' => $this->groupBundle2])->save(); + $this->groupBundle3 = mb_strtolower($this->randomMachineName()); + NodeType::create(['type' => $this->groupBundle3])->save(); $this->nonGroupBundle = mb_strtolower($this->randomMachineName()); NodeType::create(['type' => $this->nonGroupBundle])->save(); $this->membershipTypeBundle = mb_strtolower($this->randomMachineName()); @@ -116,11 +132,13 @@ protected function setUp(): void { // Define the entities as groups. Og::groupTypeManager()->addGroup('node', $this->groupBundle1); Og::groupTypeManager()->addGroup('node', $this->groupBundle2); + Og::groupTypeManager()->addGroup('node', $this->groupBundle3); // Create node author user. $user = $this->createUser(); - // Create groups. + // Create test groups. The first group has the 'subscribe without approval' + // permission. $this->group1 = Node::create([ 'type' => $this->groupBundle1, 'title' => $this->randomString(), @@ -128,6 +146,8 @@ protected function setUp(): void { ]); $this->group1->save(); + // A group which is using default permissions; it grants the 'subscribe' + // permission to non-members. $this->group2 = Node::create([ 'type' => $this->groupBundle2, 'title' => $this->randomString(), @@ -135,7 +155,7 @@ protected function setUp(): void { ]); $this->group2->save(); - // Create an unpublished node. + // An unpublished group. $this->group3 = Node::create([ 'type' => $this->groupBundle1, 'title' => $this->randomString(), @@ -152,12 +172,26 @@ protected function setUp(): void { ]); $this->group4->save(); - $role = OgRole::getRole('node', $this->groupBundle1, OgRoleInterface::ANONYMOUS); + // A group which is closed for subscription. It grants neither 'subscribe' + // nor 'subscribe without approval'. + $this->group5 = Node::create([ + 'type' => $this->groupBundle3, + 'title' => $this->randomString(), + 'uid' => $user->id(), + ]); + $this->group5->save(); - $role + // Grant the permission to 'subscribe without approval' to the first group + // type. + OgRole::getRole('node', $this->groupBundle1, OgRoleInterface::ANONYMOUS) ->grantPermission('subscribe without approval') ->save(); + // Revoke the permission to subscribe from the third group type. + OgRole::getRole('node', $this->groupBundle3, OgRoleInterface::ANONYMOUS) + ->revokePermission('subscribe') + ->save(); + // Create a new membership type. $membership_type = OgMembershipType::create([ 'type' => $this->membershipTypeBundle, @@ -222,6 +256,13 @@ public function testSubscribeAccess() { 'entity' => $this->group4, 'code' => 403, ], + + // A group which doesn't allow new subscriptions. + [ + 'entity' => $this->group5, + 'code' => 403, + ], + // A non existing entity type. [ 'entity_type_id' => mb_strtolower($this->randomMachineName()), From 8fd1072db045d93f0adf6b8a56fdaef0d31ac8bd Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Sun, 16 Aug 2020 17:27:51 +0300 Subject: [PATCH 10/10] Ensure that a neutral access result will not unexpectedly grant access. --- src/Controller/SubscriptionController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Controller/SubscriptionController.php b/src/Controller/SubscriptionController.php index 04d222ada..7ab1f6e62 100644 --- a/src/Controller/SubscriptionController.php +++ b/src/Controller/SubscriptionController.php @@ -138,7 +138,7 @@ public function subscribe($entity_type_id, EntityInterface $group, OgMembershipT $subscribe = $this->ogAccess->userAccess($group, 'subscribe'); $subscribe_without_approval = $this->ogAccess->userAccess($group, 'subscribe without approval'); - if ($subscribe->isForbidden() && $subscribe_without_approval->isForbidden()) { + if (!$subscribe->isAllowed() && !$subscribe_without_approval->isAllowed()) { throw new AccessDeniedHttpException(); }