From 5bcdcb52b2afc3ae18238d2744243c4ba2308dc5 Mon Sep 17 00:00:00 2001 From: "denis.flaven@combodo.com" Date: Fri, 5 Nov 2021 11:29:23 +0100 Subject: [PATCH] =?UTF-8?q?N=C2=B04534=20-=20creation=20of=20a=20new=20cat?= =?UTF-8?q?egory=20'filter'=20to=20hide=20admins=20to=20non-admins=20witho?= =?UTF-8?q?ut=20breaking=20legacy=20code.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../userrightsprofile.class.inc.php | 9 ++- core/userrights.class.inc.php | 2 +- test/core/UserRightsTest.php | 65 +++++++++++++++++++ 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/addons/userrights/userrightsprofile.class.inc.php b/addons/userrights/userrightsprofile.class.inc.php index 60473f899..1fb9fc3b1 100644 --- a/addons/userrights/userrightsprofile.class.inc.php +++ b/addons/userrights/userrightsprofile.class.inc.php @@ -34,7 +34,7 @@ class URP_Profiles extends UserRightsBaseClassGUI { $aParams = array ( - "category" => "addon/userrights,grant_by_profile,silo", + "category" => "addon/userrights,grant_by_profile,filter", "key_type" => "autoincrement", "name_attcode" => "name", "state_attcode" => "", @@ -219,7 +219,7 @@ class URP_UserProfile extends UserRightsBaseClassGUI { $aParams = array ( - "category" => "addon/userrights,grant_by_profile,silo", + "category" => "addon/userrights,grant_by_profile,filter", "key_type" => "autoincrement", "name_attcode" => array("userlogin", "profile"), "state_attcode" => "", @@ -610,8 +610,11 @@ class UserRightsProfile extends UserRightsAddOnAPI { $this->LoadCache(); - if (!static::IsAdministrator($oUser)) // Let us pass an administrator for testing without the need of setting up complex profile + // Let us pass an administrator for bypassing the grant matrix check in order to test this method without the need to set up a complex profile + // In the nominal case Administrators never end up here (since they completely bypass GetSelectFilter) + if (!static::IsAdministrator($oUser) && (MetaModel::HasCategory($sClass, 'silo') || MetaModel::HasCategory($sClass, 'bizmodel'))) { + // N°4354 - Categories 'silo' and 'bizmodel' do check the grant matrix. Whereas 'filter' always allows to read (but the result can be filtered) $aObjectPermissions = $this->GetUserActionGrant($oUser, $sClass, UR_ACTION_READ); if ($aObjectPermissions['permission'] == UR_ALLOWED_NO) { diff --git a/core/userrights.class.inc.php b/core/userrights.class.inc.php index d303972b8..c4c189bed 100644 --- a/core/userrights.class.inc.php +++ b/core/userrights.class.inc.php @@ -1507,7 +1507,7 @@ class UserRights try { // Check Bug 1436 for details - if (MetaModel::HasCategory($sClass, 'bizmodel') || MetaModel::HasCategory($sClass, 'silo')) + if (MetaModel::HasCategory($sClass, 'bizmodel') || MetaModel::HasCategory($sClass, 'silo') || MetaModel::HasCategory($sClass, 'filter')) { return self::$m_oAddOn->GetSelectFilter(self::$m_oUser, $sClass, $aSettings); } diff --git a/test/core/UserRightsTest.php b/test/core/UserRightsTest.php index ca4713d2f..2848667e0 100644 --- a/test/core/UserRightsTest.php +++ b/test/core/UserRightsTest.php @@ -30,6 +30,7 @@ use Combodo\iTop\Test\UnitTest\ItopDataTestCase; use CoreCannotSaveObjectException; use CoreException; use DBObject; +use DBObjectSearch; use DBObjectSet; use DeleteException; use URP_UserProfile; @@ -460,4 +461,68 @@ class UserRightsTest extends ItopDataTestCase $_SESSION = []; } + /** + *@dataProvider NonAdminCanListOwnProfilesProvider + */ + public function testNonAdminCanListOwnProfiles($bHideAdministrators) + { + $oUser = $this->AddUser('test1', 2); // portal user + $_SESSION = []; + utils::GetConfig()->Set('security.hide_administrators', $bHideAdministrators); + UserRights::Login('test1'); + + // List the link between the User and the Profiles + $oSearch = new DBObjectSearch('URP_UserProfile'); + $oSearch->AddCondition('userid', $oUser->GetKey()); + $oSet = new DBObjectSet($oSearch); + $this->assertEquals(1, $oSet->Count()); + + // Get the Profiles as well + $oSearch = DBObjectSearch::FromOQL('SELECT URP_Profiles JOIN URP_UserProfile ON URP_UserProfile.profileid = URP_Profiles.id WHERE URP_UserProfile.userid='.$oUser->GetKey()); + $oSet = new DBObjectSet($oSearch); + $this->assertEquals(1, $oSet->Count()); + + // logout + $_SESSION = []; + } + + public function NonAdminCanListOwnProfilesProvider(): array + { + return [ + 'with Admins visible'=> [false], + 'with Admins hidden' => [true], + ]; + } + /** + *@dataProvider NonAdminCannotListAdminProfilesProvider + */ + public function testNonAdminCannotListAdminProfiles($bHideAdministrators, $iExpectedCount) + { + utils::GetConfig()->Set('security.hide_administrators', $bHideAdministrators); + + $this->AddUser('test1', 2); // portal user + $oUserAdmin = $this->AddUser('admin1', 1); + $_SESSION = []; + UserRights::Login('test1'); + + $oSearch = new DBObjectSearch('URP_UserProfile'); + $oSearch->AddCondition('userid', $oUserAdmin->GetKey()); + $oSet = new DBObjectSet($oSearch); + $this->assertEquals($iExpectedCount, $oSet->Count()); + // Get the Profiles as well + $oSearch = DBObjectSearch::FromOQL('SELECT URP_Profiles JOIN URP_UserProfile ON URP_UserProfile.profileid = URP_Profiles.id WHERE URP_UserProfile.userid='.$oUserAdmin->GetKey()); + $oSet = new DBObjectSet($oSearch); + $this->assertEquals($iExpectedCount, $oSet->Count()); + + // logout + $_SESSION = []; + } + + public function NonAdminCannotListAdminProfilesProvider(): array + { + return [ + 'with Admins visible'=> [false, 1], + 'with Admins hidden' => [true, 0], + ]; + } }