From 34019426dd82f3c2eace241e7cef9152d8d8653e Mon Sep 17 00:00:00 2001 From: Andrew Paxley Date: Wed, 14 Jun 2023 13:48:46 +1200 Subject: [PATCH 1/3] NEW add OnlyTheseMembers Inherited Permission type --- src/Security/InheritedPermissions.php | 89 ++++++++++++++++--- .../InheritedPermissionsExtension.php | 10 ++- .../php/Security/InheritedPermissionsTest.php | 41 +++++++-- .../php/Security/InheritedPermissionsTest.yml | 16 ++++ 4 files changed, 139 insertions(+), 17 deletions(-) diff --git a/src/Security/InheritedPermissions.php b/src/Security/InheritedPermissions.php index a327cbf3b0d..d961d11cdf8 100644 --- a/src/Security/InheritedPermissions.php +++ b/src/Security/InheritedPermissions.php @@ -25,37 +25,43 @@ class InheritedPermissions implements PermissionChecker, MemberCacheFlusher /** * Delete permission */ - const DELETE = 'delete'; + public const DELETE = 'delete'; /** * View permission */ - const VIEW = 'view'; + public const VIEW = 'view'; /** * Edit permission */ - const EDIT = 'edit'; + public const EDIT = 'edit'; /** * Anyone canView permission */ - const ANYONE = 'Anyone'; + public const ANYONE = 'Anyone'; /** * Restrict to logged in users */ - const LOGGED_IN_USERS = 'LoggedInUsers'; + public const LOGGED_IN_USERS = 'LoggedInUsers'; /** + * @TODO - rename this to ONLY_THESE_GROUPS * Restrict to specific groups */ - const ONLY_THESE_USERS = 'OnlyTheseUsers'; + public const ONLY_THESE_USERS = 'OnlyTheseUsers'; + + /** + * Restrict to specific members + */ + public const ONLY_THESE_MEMBERS = 'OnlyTheseMembers'; /** * Inherit from parent */ - const INHERIT = 'Inherit'; + public const INHERIT = 'Inherit'; /** * Class name @@ -359,19 +365,27 @@ protected function batchPermissionCheckForStage( if ($member && $member->ID) { if (!Permission::checkMember($member, 'ADMIN')) { // Determine if this member matches any of the group or other rules - $groupJoinTable = $this->getJoinTable($type); + $groupJoinTable = $this->getGroupJoinTable($type); + $memberJoinTable = $this->getMemberJoinTable($type); $uninheritedPermissions = $stageRecords ->where([ - "(\"$typeField\" IN (?, ?) OR " . "(\"$typeField\" = ? AND \"$groupJoinTable\".\"{$baseTable}ID\" IS NOT NULL))" + "(\"$typeField\" IN (?, ?)" + . " OR (\"$typeField\" = ? AND \"$groupJoinTable\".\"{$baseTable}ID\" IS NOT NULL)" + . " OR (\"$typeField\" = ? AND \"$memberJoinTable\".\"{$baseTable}ID\" IS NOT NULL)" + . ")" => [ self::ANYONE, self::LOGGED_IN_USERS, - self::ONLY_THESE_USERS + self::ONLY_THESE_USERS, + self::ONLY_THESE_MEMBERS, ] ]) ->leftJoin( $groupJoinTable, "\"$groupJoinTable\".\"{$baseTable}ID\" = \"{$baseTable}\".\"ID\" AND " . "\"$groupJoinTable\".\"GroupID\" IN ($groupIDsSQLList)" + )->leftJoin( + $memberJoinTable, + "\"$memberJoinTable\".\"{$baseTable}ID\" = \"{$baseTable}\".\"ID\" AND " . "\"$memberJoinTable\".\"MemberID\" = {$member->ID}" )->column('ID'); } else { $uninheritedPermissions = $stageRecords->column('ID'); @@ -632,6 +646,18 @@ protected function getPermissionField($type) * @return string */ protected function getJoinTable($type) + { + return $this->getGroupJoinTable($type); + } + + /** + * Get group join table for type + * Defaults to those provided by {@see InheritedPermissionsExtension) + * + * @param string $type + * @return string + */ + protected function getGroupJoinTable($type) { switch ($type) { case self::DELETE: @@ -645,6 +671,27 @@ protected function getJoinTable($type) } } + /** + * Get member join table for type + * Defaults to those provided by {@see InheritedPermissionsExtension) + * + * @param string $type + * @return string + */ + protected function getMemberJoinTable($type) + { + switch ($type) { + case self::DELETE: + // Delete uses edit type - Drop through + case self::EDIT: + return $this->getEditorMembersTable(); + case self::VIEW: + return $this->getViewerMembersTable(); + default: + throw new InvalidArgumentException("Invalid argument type $type"); + } + } + /** * Determine default permission for a givion check * @@ -716,6 +763,28 @@ protected function getViewerGroupsTable() return "{$table}_ViewerGroups"; } + /** + * Get table to use for editor members relation + * + * @return string + */ + protected function getEditorMembersTable() + { + $table = DataObject::getSchema()->tableName($this->baseClass); + return "{$table}_EditorMembers"; + } + + /** + * Get table to use for viewer members relation + * + * @return string + */ + protected function getViewerMembersTable() + { + $table = DataObject::getSchema()->tableName($this->baseClass); + return "{$table}_ViewerMembers"; + } + /** * Gets the permission from cache * diff --git a/src/Security/InheritedPermissionsExtension.php b/src/Security/InheritedPermissionsExtension.php index df069b5c647..291e84bcd29 100644 --- a/src/Security/InheritedPermissionsExtension.php +++ b/src/Security/InheritedPermissionsExtension.php @@ -12,17 +12,21 @@ * @property string $CanEditType * @method ManyManyList ViewerGroups() * @method ManyManyList EditorGroups() + * @method ManyManyList ViewerMembers() + * @method ManyManyList EditorMembers() */ class InheritedPermissionsExtension extends DataExtension { private static array $db = [ - 'CanViewType' => "Enum('Anyone, LoggedInUsers, OnlyTheseUsers, Inherit', 'Inherit')", - 'CanEditType' => "Enum('LoggedInUsers, OnlyTheseUsers, Inherit', 'Inherit')", + 'CanViewType' => "Enum('Anyone, LoggedInUsers, OnlyTheseUsers, OnlyTheseMembers, Inherit', 'Inherit')", + 'CanEditType' => "Enum('LoggedInUsers, OnlyTheseUsers, OnlyTheseMembers, Inherit', 'Inherit')", ]; private static array $many_many = [ 'ViewerGroups' => Group::class, 'EditorGroups' => Group::class, + 'ViewerMembers' => Member::class, + 'EditorMembers' => Member::class, ]; private static array $defaults = [ @@ -33,5 +37,7 @@ class InheritedPermissionsExtension extends DataExtension private static array $cascade_duplicates = [ 'ViewerGroups', 'EditorGroups', + 'ViewerMembers', + 'EditorMembers', ]; } diff --git a/tests/php/Security/InheritedPermissionsTest.php b/tests/php/Security/InheritedPermissionsTest.php index 696351030ca..4eefa598642 100644 --- a/tests/php/Security/InheritedPermissionsTest.php +++ b/tests/php/Security/InheritedPermissionsTest.php @@ -68,6 +68,7 @@ protected function tearDown(): void public function testEditPermissions() { $editor = $this->objFromFixture(Member::class, 'editor'); + $freddie = $this->objFromFixture(Member::class, 'oneFileFreddie'); $about = $this->objFromFixture(TestPermissionNode::class, 'about'); $aboutStaff = $this->objFromFixture(TestPermissionNode::class, 'about-staff'); @@ -75,10 +76,12 @@ public function testEditPermissions() $products = $this->objFromFixture(TestPermissionNode::class, 'products'); $product1 = $this->objFromFixture(TestPermissionNode::class, 'products-product1'); $product4 = $this->objFromFixture(TestPermissionNode::class, 'products-product4'); + $freddiesFile = $this->objFromFixture(TestPermissionNode::class, 'freddies-file'); // Test logged out users cannot edit - Member::actAs(null, function () use ($aboutStaff) { + Member::actAs(null, function () use ($aboutStaff, $freddiesFile) { $this->assertFalse($aboutStaff->canEdit()); + $this->assertFalse($freddiesFile->canEdit()); }); // Can't edit a page that is locked to admins @@ -96,6 +99,10 @@ public function testEditPermissions() // Test that root node respects root permissions $this->assertTrue($history->canEdit($editor)); + // Test that only Freddie can edit Freddie's file + $this->assertFalse($freddiesFile->canEdit($editor)); + $this->assertTrue($freddiesFile->canEdit($freddie)); + TestPermissionNode::getInheritedPermissions()->clearCache(); $this->rootPermissions->setCanEdit(false); @@ -106,6 +113,7 @@ public function testEditPermissions() public function testDeletePermissions() { $editor = $this->objFromFixture(Member::class, 'editor'); + $freddie = $this->objFromFixture(Member::class, 'oneFileFreddie'); $about = $this->objFromFixture(TestPermissionNode::class, 'about'); $aboutStaff = $this->objFromFixture(TestPermissionNode::class, 'about-staff'); @@ -113,10 +121,12 @@ public function testDeletePermissions() $products = $this->objFromFixture(TestPermissionNode::class, 'products'); $product1 = $this->objFromFixture(TestPermissionNode::class, 'products-product1'); $product4 = $this->objFromFixture(TestPermissionNode::class, 'products-product4'); + $freddiesFile = $this->objFromFixture(TestPermissionNode::class, 'freddies-file'); // Test logged out users cannot edit - Member::actAs(null, function () use ($aboutStaff) { + Member::actAs(null, function () use ($aboutStaff, $freddiesFile) { $this->assertFalse($aboutStaff->canDelete()); + $this->assertFalse($freddiesFile->canDelete()); }); // Can't edit a page that is locked to admins @@ -134,6 +144,10 @@ public function testDeletePermissions() // Test that root node respects root permissions $this->assertTrue($history->canDelete($editor)); + // Test that only Freddie can delete Freddie's file + $this->assertFalse($freddiesFile->canDelete($editor)); + $this->assertTrue($freddiesFile->canDelete($freddie)); + TestPermissionNode::getInheritedPermissions()->clearCache(); $this->rootPermissions->setCanEdit(false); @@ -150,14 +164,17 @@ public function testViewPermissions() $secretNested = $this->objFromFixture(TestPermissionNode::class, 'secret-nested'); $protected = $this->objFromFixture(TestPermissionNode::class, 'protected'); $protectedChild = $this->objFromFixture(TestPermissionNode::class, 'protected-child'); - $editor = $this->objFromFixture(Member::class, 'editor'); $restricted = $this->objFromFixture(TestPermissionNode::class, 'restricted-page'); + $freddiesFile = $this->objFromFixture(TestPermissionNode::class, 'freddies-file'); + + $editor = $this->objFromFixture(Member::class, 'editor'); $admin = $this->objFromFixture(Member::class, 'admin'); + $freddie = $this->objFromFixture(Member::class, 'oneFileFreddie'); // Not logged in user can only access Inherit or Anyone pages Member::actAs( null, - function () use ($protectedChild, $secretNested, $protected, $secret, $history, $contact, $contactForm) { + function () use ($protectedChild, $secretNested, $protected, $secret, $history, $contact, $contactForm, $freddiesFile) { $this->assertTrue($history->canView()); $this->assertTrue($contact->canView()); $this->assertTrue($contactForm->canView()); @@ -166,6 +183,7 @@ function () use ($protectedChild, $secretNested, $protected, $secret, $history, $this->assertFalse($secretNested->canView()); $this->assertFalse($protected->canView()); $this->assertFalse($protectedChild->canView()); + $this->assertFalse($freddiesFile->canView()); } ); @@ -180,6 +198,10 @@ function () use ($protectedChild, $secretNested, $protected, $secret, $history, // Check root permissions $this->assertTrue($history->canView($editor)); + // Test that only Freddie can view Freddie's file + $this->assertFalse($freddiesFile->canView($editor)); + $this->assertTrue($freddiesFile->canView($freddie)); + TestPermissionNode::getInheritedPermissions()->clearCache(); $this->rootPermissions->setCanView(false); @@ -198,12 +220,16 @@ public function testUnstagedViewPermissions() $secretNested = $this->objFromFixture(UnstagedNode::class, 'secret-nested'); $protected = $this->objFromFixture(UnstagedNode::class, 'protected'); $protectedChild = $this->objFromFixture(UnstagedNode::class, 'protected-child'); + $freddiesFile = $this->objFromFixture(UnstagedNode::class, 'freddies-file'); + $editor = $this->objFromFixture(Member::class, 'editor'); + $freddie = $this->objFromFixture(Member::class, 'oneFileFreddie'); + // Not logged in user can only access Inherit or Anyone pages Member::actAs( null, - function () use ($protectedChild, $secretNested, $protected, $secret, $history, $contact, $contactForm) { + function () use ($protectedChild, $secretNested, $protected, $secret, $history, $contact, $contactForm, $freddiesFile) { $this->assertTrue($history->canView()); $this->assertTrue($contact->canView()); $this->assertTrue($contactForm->canView()); @@ -212,6 +238,7 @@ function () use ($protectedChild, $secretNested, $protected, $secret, $history, $this->assertFalse($secretNested->canView()); $this->assertFalse($protected->canView()); $this->assertFalse($protectedChild->canView()); + $this->assertFalse($freddiesFile->canView()); } ); @@ -226,6 +253,10 @@ function () use ($protectedChild, $secretNested, $protected, $secret, $history, // Check root permissions $this->assertTrue($history->canView($editor)); + // Test that only Freddie can view Freddie's file + $this->assertFalse($freddiesFile->canView($editor)); + $this->assertTrue($freddiesFile->canView($freddie)); + UnstagedNode::getInheritedPermissions()->clearCache(); $this->rootPermissions->setCanView(false); diff --git a/tests/php/Security/InheritedPermissionsTest.yml b/tests/php/Security/InheritedPermissionsTest.yml index 92141c361c4..558abe02bcb 100644 --- a/tests/php/Security/InheritedPermissionsTest.yml +++ b/tests/php/Security/InheritedPermissionsTest.yml @@ -33,6 +33,10 @@ SilverStripe\Security\Member: Groups: =>SilverStripe\Security\Group.allsections securityadmin: Groups: =>SilverStripe\Security\Group.securityadmins + oneFileFreddie: + FirstName: Freddie + Surname: Fantastic + Groups: =>SilverStripe\Security\Group.editors SilverStripe\Security\Tests\InheritedPermissionsTest\TestPermissionNode: about: @@ -104,6 +108,12 @@ SilverStripe\Security\Tests\InheritedPermissionsTest\TestPermissionNode: Title: Restricted Page CanViewType: OnlyTheseUsers ViewerGroups: =>SilverStripe\Security\Group.allsections + freddies-file: + Title: Freddies File + CanViewType: OnlyTheseMembers + CanEditType: OnlyTheseMembers + ViewerMembers: =>SilverStripe\Security\Member.oneFileFreddie + EditorMembers: =>SilverStripe\Security\Member.oneFileFreddie SilverStripe\Security\Tests\InheritedPermissionsTest\UnstagedNode: about: @@ -175,3 +185,9 @@ SilverStripe\Security\Tests\InheritedPermissionsTest\UnstagedNode: Title: Restricted Page CanViewType: OnlyTheseUsers ViewerGroups: =>SilverStripe\Security\Group.allsections + freddies-file: + Title: Freddies File + CanViewType: OnlyTheseMembers + CanEditType: OnlyTheseMembers + ViewerMembers: =>SilverStripe\Security\Member.oneFileFreddie + EditorMembers: =>SilverStripe\Security\Member.oneFileFreddie From 4b22ab4dfe8ee04e19d26f70d8fcbdbe92d81c06 Mon Sep 17 00:00:00 2001 From: Andrew Paxley Date: Mon, 19 Jun 2023 17:16:53 +1200 Subject: [PATCH 2/3] API deprecate InheritedPermissions::getJoinTable --- src/Security/InheritedPermissions.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Security/InheritedPermissions.php b/src/Security/InheritedPermissions.php index d961d11cdf8..9cd9dfd90e1 100644 --- a/src/Security/InheritedPermissions.php +++ b/src/Security/InheritedPermissions.php @@ -10,6 +10,7 @@ use SilverStripe\Versioned\Versioned; use Psr\SimpleCache\CacheInterface; use SilverStripe\Core\Cache\MemberCacheFlusher; +use SilverStripe\Dev\Deprecation; /** * Calculates batch permissions for nested objects for: @@ -48,7 +49,6 @@ class InheritedPermissions implements PermissionChecker, MemberCacheFlusher public const LOGGED_IN_USERS = 'LoggedInUsers'; /** - * @TODO - rename this to ONLY_THESE_GROUPS * Restrict to specific groups */ public const ONLY_THESE_USERS = 'OnlyTheseUsers'; @@ -642,11 +642,13 @@ protected function getPermissionField($type) * Get join table for type * Defaults to those provided by {@see InheritedPermissionsExtension) * + * @deprecated 5.1.0 Use getGroupJoinTable() instead * @param string $type * @return string */ protected function getJoinTable($type) { + Deprecation::notice('5.1.0', 'Use getGroupJoinTable() instead'); return $this->getGroupJoinTable($type); } From a03d0fdf684e00d388b67af6ce9bdc99877709af Mon Sep 17 00:00:00 2001 From: Andrew Paxley Date: Thu, 6 Jul 2023 17:51:16 +1200 Subject: [PATCH 3/3] FIX ListboxField entwine submissions --- src/Forms/ListboxField.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Forms/ListboxField.php b/src/Forms/ListboxField.php index cf2d583b14d..1c792ae5496 100644 --- a/src/Forms/ListboxField.php +++ b/src/Forms/ListboxField.php @@ -248,17 +248,20 @@ public function getValueArray() } $canary = reset($validValues); + $targetType = gettype($canary); if (is_array($value) && count($value) > 0) { $first = reset($value); // sanity check the values - make sure strings get strings, ints get ints etc - if (gettype($canary) !== gettype($first)) { + if ($targetType !== gettype($first)) { $replaced = []; foreach ($value as $item) { if (!is_array($item)) { $item = json_decode($item, true); } - if (isset($item['Value'])) { + if ($targetType === gettype($item)) { + $replaced[] = $item; + } elseif (isset($item['Value'])) { $replaced[] = $item['Value']; } }