Merge pull request #66 from creative-commoners/pulls/3.0/fill-unit-te…

…st-gap Cover AuditHookMemberGroupSet and AuditHookManyManyList with test
silverstripe · Dec 21, 2023 · 8bce3d5 · 8bce3d5
2 parents 59e97af + df1093c
commit 8bce3d5
Show file tree

Hide file tree

Showing 5 changed files with 150 additions and 2 deletions.
diff --git a/code/AuditHookManyManyList.php b/code/AuditHookManyManyList.php
@@ -8,6 +8,10 @@
 use SilverStripe\Security\Member;
 use SilverStripe\Security\Security;
 
+/**
+ * AuditHookManyManyList is meant to override ManyManyList. When a Member is
+ * removed from a Group, it logs the event.
+ */
 class AuditHookManyManyList extends ManyManyList
 {
     /**
@@ -18,7 +22,9 @@ public function removeByID($itemID)
     {
         parent::removeByID($itemID);
 
-        if ($this->getJoinTable() == 'Group_Members') {
+        $memberGroupJoinTable = Member::singleton()->Groups()->getJoinTable();
+
+        if ($this->getJoinTable() === $memberGroupJoinTable) {
             $currentMember = Security::getCurrentUser();
             if (!($currentMember && $currentMember->exists())) {
                 return;

diff --git a/code/AuditHookMemberGroupSet.php b/code/AuditHookMemberGroupSet.php
@@ -8,6 +8,10 @@
 use SilverStripe\Security\Member_GroupSet;
 use SilverStripe\Security\Security;
 
+/**
+ * AuditHookMemberGroupSet is meant to override Member_GroupSet. When a Group
+ * is removed from a Member, it logs the event.
+ */
 class AuditHookMemberGroupSet extends Member_GroupSet
 {
     /**
@@ -18,7 +22,9 @@ public function removeByID($itemID)
     {
         parent::removeByID($itemID);
 
-        if ($this->getJoinTable() === 'Group_Members') {
+        $memberGroupJoinTable = Member::singleton()->Groups()->getJoinTable();
+
+        if ($this->getJoinTable() === $memberGroupJoinTable) {
             $currentMember = Security::getCurrentUser();
             if (!$currentMember || !$currentMember->exists()) {
                 return;

diff --git a/tests/AuditHookMFATest.yml b/tests/AuditHookMFATest.yml
@@ -1,5 +1,17 @@
+SilverStripe\Security\Group:
+  # One test could have ambiguous results if the member and the group have the same ID.
+  # So we're creating a bunch of dummy groups to make sure the IDs are different.
+  noop:
+    Title: Noop
+  foobar:
+    Title: Foobar
+  prisoners:
+    Title: Prisoners
+    Code: prisoners
+
 SilverStripe\Security\Member:
   leslie_lawless:
     FirstName: Leslie
     Surname: Lawless
     Email: leslie@example.com
+    Groups: '=>SilverStripe\Security\Group.prisoners'
diff --git a/tests/AuditHookManyManyListTest.php b/tests/AuditHookManyManyListTest.php
@@ -0,0 +1,62 @@
+<?php
+
+namespace SilverStripe\Auditor\Tests;
+
+use SilverStripe\Auditor\Tests\AuditHookTest\Logger;
+use SilverStripe\Core\Injector\Injector;
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Security\Member;
+use SilverStripe\Security\Group;
+use SilverStripe\Security\Security;
+
+class AuditHookManyManyListTest extends SapphireTest
+{
+    protected static $fixture_file = 'AuditHookMFATest.yml';
+
+    protected Logger $writer;
+
+    protected Member $member;
+
+    protected Group $group;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->writer = new Logger;
+
+        // Phase singleton out, so the message log is purged.
+        Injector::inst()->unregisterNamedObject('AuditLogger');
+        Injector::inst()->registerService($this->writer, 'AuditLogger');
+
+        $this->member = $this->objFromFixture(Member::class, 'leslie_lawless');
+        $this->group = $this->objFromFixture(Group::class, 'prisoners');
+    }
+
+    public function testRemoveByID(): void
+    {
+        $this->assertEquals(
+            1,
+            $this->group->Members()->filter('ID', $this->member->ID)->count(),
+            'Leslie Lawless is part of the prisoners group'
+        );
+
+        $this->group->Members()->removeByID($this->member->ID);
+
+        $message = $this->writer->getLastMessage();
+        $currentUser = Security::getCurrentUser();
+        $this->assertStringContainsString(
+            sprintf(
+                '"%s" (ID: %d) removed Member "%s" (ID: %d) from Group "%s" (ID: %d)',
+                $currentUser->Email,
+                $currentUser->ID,
+                $this->member->Email,
+                $this->member->ID,
+                $this->group->Title,
+                $this->group->ID,
+            ),
+            $message,
+            'Log contains who removed Leslie Lawless from the prisoners group'
+        );
+    }
+}
diff --git a/tests/AuditHookMemberGroupSetTest.php b/tests/AuditHookMemberGroupSetTest.php
@@ -0,0 +1,62 @@
+<?php
+
+namespace SilverStripe\Auditor\Tests;
+
+use SilverStripe\Auditor\Tests\AuditHookTest\Logger;
+use SilverStripe\Core\Injector\Injector;
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Security\Member;
+use SilverStripe\Security\Group;
+use SilverStripe\Security\Security;
+
+class AuditHookMemberGroupSetTest extends SapphireTest
+{
+    protected static $fixture_file = 'AuditHookMFATest.yml';
+
+    protected Logger $writer;
+
+    protected Member $member;
+
+    protected Group $group;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->writer = new Logger;
+
+        // Phase singleton out, so the message log is purged.
+        Injector::inst()->unregisterNamedObject('AuditLogger');
+        Injector::inst()->registerService($this->writer, 'AuditLogger');
+
+        $this->member = $this->objFromFixture(Member::class, 'leslie_lawless');
+        $this->group = $this->objFromFixture(Group::class, 'prisoners');
+    }
+
+    public function testRemoveByID(): void
+    {
+        $this->assertEquals(
+            1,
+            $this->member->Groups()->filter('ID', $this->group->ID)->count(),
+            'The Prisoners group is one of Leslie Lawless\' groups.'
+        );
+
+        $this->member->Groups()->removeByID($this->group->ID);
+
+        $message = $this->writer->getLastMessage();
+        $currentUser = Security::getCurrentUser();
+        $this->assertStringContainsString(
+            sprintf(
+                '"%s" (ID: %d) removed Member "%s" (ID: %d) from Group "%s" (ID: %d)',
+                $currentUser->Email,
+                $currentUser->ID,
+                $this->member->Email,
+                $this->member->ID,
+                $this->group->Title,
+                $this->group->ID,
+            ),
+            $message,
+            'Log contains who removed "Prisoners" from Leslie Lawless\' groups.'
+        );
+    }
+}