FIX Check canArchive() permission instead of canDelete()

silverstripe · Aug 7, 2024 · 05f0bd7 · 05f0bd7
1 parent 77a1497
commit 05f0bd7
Show file tree

Hide file tree

Showing 3 changed files with 30 additions and 7 deletions.
diff --git a/src/Controllers/LinkFieldController.php b/src/Controllers/LinkFieldController.php
@@ -127,17 +127,20 @@ private function getLinkData(Link $link): array
      */
     public function linkDelete(): HTTPResponse
     {
-        $link = $this->linkFromRequest();
-        if (!$link->canDelete()) {
-            $this->jsonError(403);
-        }
         // Check security token on destructive operation
         if (!SecurityToken::inst()->checkRequest($this->getRequest())) {
             $this->jsonError(400);
         }
+        $link = $this->linkFromRequest();
         if ($link->hasExtension(Versioned::class)) {
+            if (!$link->canArchive()) {
+                $this->jsonError(403);
+            }
             $link->doArchive();
         } else {
+            if (!$link->canDelete()) {
+                $this->jsonError(403);
+            }
             $link->delete();
         }
         // Send response

diff --git a/tests/php/Controllers/LinkFieldControllerTest.php b/tests/php/Controllers/LinkFieldControllerTest.php
@@ -11,6 +11,7 @@
 use SilverStripe\LinkField\Controllers\LinkFieldController;
 use SilverStripe\LinkField\Tests\Models\LinkTest\LinkOwner;
 use SilverStripe\Versioned\Versioned;
+use SilverStripe\LinkField\Models\Link;
 
 class LinkFieldControllerTest extends FunctionalTest
 {
@@ -528,14 +529,20 @@ public function provideLinkData(): array
     }
 
     /**
-     * @dataProvider provideLinkDelete
+     * @dataProvider provideLinkArchive
      */
-    public function testLinkDelete(
+    public function testLinkArchive(
         string $idType,
         string $fail,
         int $expectedCode
     ): void {
         TestPhoneLink::$fail = $fail;
+        if ($fail == 'can-delete') {
+            // Remove the Versioned extension because there's logic in LinkFieldController
+            // to use either canArchive() or canDelete() based on the presence of the Versioned extension
+            // Note that you must remove the extension on the base class rather than a TestOnly subclass
+            Link::remove_extension(Versioned::class);
+        }
         $owner = $this->getFixtureLinkOwner();
         $ownerID = $owner->ID;
         $ownerClass = urlencode($owner->ClassName);
@@ -560,16 +567,24 @@ public function testLinkDelete(
         } else {
             $this->assertNull(TestPhoneLink::get()->byID($fixtureID));
         }
+        if ($fail == 'can-delete') {
+            Link::add_extension(Versioned::class);
+        }
     }
 
-    public function provideLinkDelete(): array
+    public function provideLinkArchive(): array
     {
         return [
             'Valid' => [
                 'idType' => 'existing',
                 'fail' => '',
                 'expectedCode' => 204,
             ],
+            'Reject fail canArchive()' => [
+                'idType' => 'existing',
+                'fail' => 'can-archive',
+                'expectedCode' => 403,
+            ],
             'Reject fail canDelete()' => [
                 'idType' => 'existing',
                 'fail' => 'can-delete',

diff --git a/tests/php/Controllers/LinkFieldControllerTest/TestPhoneLink.php b/tests/php/Controllers/LinkFieldControllerTest/TestPhoneLink.php
@@ -43,6 +43,11 @@ public function canDelete($member = null)
         return TestPhoneLink::$fail !== 'can-delete';
     }
 
+    public function canArchive($member = null)
+    {
+        return TestPhoneLink::$fail !== 'can-archive';
+    }
+
     public function validate(): ValidationResult
     {
         $validationResult = parent::validate();