diff --git a/core/config.class.inc.php b/core/config.class.inc.php index 7cf960a3a..ec8d44b1a 100644 --- a/core/config.class.inc.php +++ b/core/config.class.inc.php @@ -1730,6 +1730,14 @@ class Config 'source_of_value' => '', 'show_in_conf_sample' => false, ], + 'security.disable_joined_classes_filter' => [ + 'type' => 'bool', + 'description' => 'If true, scope filters aren\'t applied to joined classes or union classes not directly listed in the SELECT clause.', + 'default' => true, + 'value' => true, + 'source_of_value' => '', + 'show_in_conf_sample' => false, + ], 'security.hide_administrators' => [ 'type' => 'bool', 'description' => 'If true, non-administrator users will not be able to see the administrator accounts, the Administrator profile and the links between the administrator accounts and their profiles.', diff --git a/core/dbobjectsearch.class.php b/core/dbobjectsearch.class.php index bbe79b01c..0f32ab3ef 100644 --- a/core/dbobjectsearch.class.php +++ b/core/dbobjectsearch.class.php @@ -1925,4 +1925,37 @@ class DBObjectSearch extends DBSearch { return $this->GetCriteria()->ListParameters(); } + + /** + * @inheritDoc + * @return DBObjectSearch + */ + protected function ApplyDataFilters(): DBObjectSearch + { + if ($this->IsAllDataAllowed() || $this->IsDataFiltered()) { + return $this; + } + + $oSearch = $this; + $aClassesToFilter = $this->GetSelectedClasses(); + + // Opt-in for joined classes filtering, otherwise only filter the selected class(es) + if (MetaModel::GetConfig()->Get('security.disable_joined_classes_filter') === false) { + $aClassesToFilter = $this->GetJoinedClasses(); + } + + // Apply filter (this is similar to the one in DBSearch but the factorization could make it less readable) + foreach ($aClassesToFilter as $sClassAlias => $sClass) { + $oVisibleObjects = UserRights::GetSelectFilter($sClass, $this->GetModifierProperties('UserRightsGetSelectFilter')); + if ($oVisibleObjects === false) { + $oVisibleObjects = DBObjectSearch::FromEmptySet($sClass); + } + if (is_object($oVisibleObjects)) { + $oVisibleObjects->AllowAllData(); + $oSearch = $oSearch->Filter($sClassAlias, $oVisibleObjects); + $oSearch->SetDataFiltered(); + } + } + return $oSearch; + } } diff --git a/core/dbsearch.class.php b/core/dbsearch.class.php index db113f665..08ca8d658 100644 --- a/core/dbsearch.class.php +++ b/core/dbsearch.class.php @@ -1122,21 +1122,7 @@ abstract class DBSearch */ protected function GetSQLQuery($aOrderBy, $aArgs, $aAttToLoad, $aExtendedDataSpec, $iLimitCount, $iLimitStart, $bGetCount, $aGroupByExpr = null, $aSelectExpr = null) { - $oSearch = $this; - if (!$this->IsAllDataAllowed() && !$this->IsDataFiltered()) { - foreach ($this->GetSelectedClasses() as $sClassAlias => $sClass) { - $oVisibleObjects = UserRights::GetSelectFilter($sClass, $this->GetModifierProperties('UserRightsGetSelectFilter')); - if ($oVisibleObjects === false) { - // Make sure this is a valid search object, saying NO for all - $oVisibleObjects = DBObjectSearch::FromEmptySet($sClass); - } - if (is_object($oVisibleObjects)) { - $oVisibleObjects->AllowAllData(); - $oSearch = $oSearch->Filter($sClassAlias, $oVisibleObjects); - $oSearch->SetDataFiltered(); - } - } - } + $oSearch = $this->ApplyDataFilters(); if (is_array($aGroupByExpr)) { foreach ($aGroupByExpr as $sAlias => $oGroupByExp) { @@ -1608,4 +1594,33 @@ abstract class DBSearch * @return array{\VariableExpression} */ abstract public function GetExpectedArguments(): array; + + /** + * Apply data filters to the search, if needed + * + * @return DBSearch + * @throws CoreException + */ + protected function ApplyDataFilters(): DBSearch + { + if ($this->IsAllDataAllowed() || $this->IsDataFiltered()) { + return $this; + } + + $oSearch = $this; + $aClassesToFilter = $this->GetSelectedClasses(); + + foreach ($aClassesToFilter as $sClassAlias => $sClass) { + $oVisibleObjects = UserRights::GetSelectFilter($sClass, $this->GetModifierProperties('UserRightsGetSelectFilter')); + if ($oVisibleObjects === false) { + $oVisibleObjects = DBObjectSearch::FromEmptySet($sClass); + } + if (is_object($oVisibleObjects)) { + $oVisibleObjects->AllowAllData(); + $oSearch = $oSearch->Filter($sClassAlias, $oVisibleObjects); + $oSearch->SetDataFiltered(); + } + } + return $oSearch; + } } diff --git a/core/dbunionsearch.class.php b/core/dbunionsearch.class.php index a9c4beba8..a9118974f 100644 --- a/core/dbunionsearch.class.php +++ b/core/dbunionsearch.class.php @@ -673,4 +673,30 @@ class DBUnionSearch extends DBSearch return $aVariableCriteria; } + + /** + * @inheritDoc + * @return DBUnionSearch + */ + protected function ApplyDataFilters(): DBUnionSearch + { + if ($this->IsAllDataAllowed() || $this->IsDataFiltered()) { + return $this; + } + + // Opt-in for joined classes filtering, otherwise fallback on DBSearch filtering + if (MetaModel::GetConfig()->Get('security.disable_joined_classes_filter') === true) { + return parent::ApplyDataFilters(); + } + + // Apply filters per sub-search + $aFilteredSearches = []; + foreach ($this->GetSearches() as $oSubSearch) { + // Recursively call ApplyDataFilters on sub-searches + $aFilteredSearches[] = $oSubSearch->ApplyDataFilters(); + } + + $oSearch = new DBUnionSearch($aFilteredSearches); + return $oSearch; + } } diff --git a/tests/php-unit-tests/unitary-tests/core/DBSearchFilterJoinTest.php b/tests/php-unit-tests/unitary-tests/core/DBSearchFilterJoinTest.php new file mode 100644 index 000000000..9ab1d5e31 --- /dev/null +++ b/tests/php-unit-tests/unitary-tests/core/DBSearchFilterJoinTest.php @@ -0,0 +1,193 @@ +RequireOnceItopFile('application/startup.inc.php'); + $this->aData = $this->CreateDBSearchFilterTestData(); + DBSearch::EnableQueryCache(false, false); + $this->LoginRestrictedUser($this->aData['allowed_org_id'], self::RESTRICTED_PROFILE); + + } + + protected function tearDown(): void + { + parent::tearDown(); + } + + /** + * @dataProvider JoinedAndNestedOqlProvider + */ + public function testDBSearchFilterAppliedToJoinsWhenEnabled(string $sOql, int $iExpectedCount): void + { + $this->EnableJoinFilterConfig(true); + + $oSearch = DBObjectSearch::FromOQL($sOql, ['denied_org' => $this->aData['denied_org_name'], 'allowed_org' => $this->aData['allowed_org_name']]); + $oSet = new \DBObjectSet($oSearch); + CMDBSource::TestQuery($oSearch->MakeSelectQuery()); + $this->assertEquals($iExpectedCount, $oSet->Count()); + } + + /** + * @dataProvider JoinedAndNestedOqlProvider + */ + public function testDBSearchFilterAppliedToJoinsWhenDisabled(string $sOql, int $iExpectedCount, int $iExpectedDisabledCount): void + { + $this->EnableJoinFilterConfig(false); + + $oSearch = DBObjectSearch::FromOQL($sOql, ['denied_org' => $this->aData['denied_org_name'], 'allowed_org' => $this->aData['allowed_org_name']]); + $oSet = new \DBObjectSet($oSearch); + CMDBSource::TestQuery($oSearch->MakeSelectQuery()); + $this->assertEquals($iExpectedDisabledCount, $oSet->Count()); + } + + /** + * @dataProvider JoinedAndNestedOqlProvider + */ + public function testAllowAllDataBypassesDBSearchFilterWhenEnabled(string $sOql, int $iExpectedCount, int $iExpectedDisabledCount): void + { + $this->EnableJoinFilterConfig(true); + + $oSearch = DBObjectSearch::FromOQL($sOql, ['denied_org' => $this->aData['denied_org_name'], 'allowed_org' => $this->aData['allowed_org_name']]); + $oSearch->AllowAllData(); + $oSet = new \DBObjectSet($oSearch); + CMDBSource::TestQuery($oSearch->MakeSelectQuery()); + $this->assertEquals($iExpectedDisabledCount, $oSet->Count()); + } + + /** + * @dataProvider JoinedAndNestedOqlProvider + */ + public function testAllowAllDataBypassesDBSearchFilterWhenDisabled(string $sOql, int $iExpectedCount, int $iExpectedDisabledCount): void + { + $this->EnableJoinFilterConfig(false); + + $oSearch = DBObjectSearch::FromOQL($sOql, ['denied_org' => $this->aData['denied_org_name'], 'allowed_org' => $this->aData['allowed_org_name']]); + $oSearch->AllowAllData(); + $oSet = new \DBObjectSet($oSearch); + CMDBSource::TestQuery($oSearch->MakeSelectQuery()); + $this->assertEquals($iExpectedDisabledCount, $oSet->Count()); + } + + public function JoinedAndNestedOqlProvider(): array + { + return [ + 'join-filter-on-org' => [ + 'oql' => "SELECT OSF FROM OSFamily AS OSF JOIN VirtualMachine AS VM ON VM.osfamily_id = OSF.id JOIN Organization AS O ON VM.org_id = O.id WHERE O.name = :denied_org", + 'expected_filtered_count' => 0, + 'expected_unfiltered_count' => 1, + ], + 'nested-in-select' => [ + 'oql' => "SELECT OSF FROM OSFamily AS OSF WHERE OSF.id IN (SELECT OSF1 FROM OSFamily AS OSF1 JOIN VirtualMachine AS VM ON VM.osfamily_id = OSF1.id JOIN Organization AS O ON VM.org_id = O.id WHERE O.name = :denied_org)", + 'expected_filtered_count' => 0, + 'expected_unfiltered_count' => 1, + + ], + 'userrequest-join-person-org' => [ + 'oql' => "SELECT OSF FROM OSFamily AS OSF JOIN VirtualMachine AS VM ON VM.osfamily_id = OSF.id JOIN lnkFunctionalCIToTicket AS L ON L.functionalci_id = VM.id JOIN UserRequest AS UR ON L.ticket_id = UR.id JOIN Person AS P ON UR.caller_id = P.id JOIN Organization AS O ON P.org_id = O.id WHERE O.name = :denied_org", + 'expected_filtered_count' => 0, + 'expected_unfiltered_count' => 1, + ], + 'union-join-filter-on-org' => [ + 'oql' => "SELECT OSF FROM OSFamily AS OSF JOIN VirtualMachine AS VM ON VM.osfamily_id = OSF.id JOIN Organization AS O ON VM.org_id = O.id WHERE O.name = :denied_org UNION SELECT OSF2 FROM OSFamily AS OSF2 JOIN VirtualMachine AS VM2 ON VM2.osfamily_id = OSF2.id JOIN Organization AS O2 ON VM2.org_id = O2.id WHERE O2.name = :allowed_org", + 'expected_filtered_count' => 1, + 'expected_unfiltered_count' => 2, + ], + ]; + } + + private function EnableJoinFilterConfig(bool $bEnabled): void + { + $oConfig = MetaModel::GetConfig(); + $oConfig->Set('security.disable_joined_classes_filter', !$bEnabled); + } + + private function CreateDBSearchFilterTestData(): array + { + $sSuffix = 'DBSearchFilterJoinTest'; + + $sAllowedOrgName = 'DBSearchFilterAllowedOrg-'.$sSuffix; + $iAllowedOrgId = $this->GivenObjectInDB('Organization', [ + 'name' => $sAllowedOrgName, + ]); + + $this->debug("Org allowed id: $iAllowedOrgId"); + $sDeniedOrgName = 'DBSearchFilterDeniedOrg-'.$sSuffix; + $iDeniedOrgId = $this->GivenObjectInDB('Organization', [ + 'name' => $sDeniedOrgName, + ]); + $this->debug("Org denied id: $iDeniedOrgId"); + + $iDeniedOsFamilyId = $this->GivenObjectInDB('OSFamily', [ + 'name' => 'DBSearchFilterOsFamilyDenied-'.$sSuffix, + ]); + + $iAllowedOsFamilyId = $this->GivenObjectInDB('OSFamily', [ + 'name' => 'DBSearchFilterOsFamilyAllowed-'.$sSuffix, + ]); + + $iDeniedVMId = $this->GivenObjectInDB('VirtualMachine', [ + 'name' => 'DBSearchFilterVmDenied-'.$sSuffix, + 'org_id' => $iDeniedOrgId, + 'osfamily_id' => $iDeniedOsFamilyId, + 'virtualhost_id' => 1, + ]); + + $iVirtualHostId = $this->GivenObjectInDB('Hypervisor', [ + 'name' => 'DBSearchFilterVHost-'.$sSuffix, + 'org_id' => $iAllowedOrgId, + ]); + + $this->GivenObjectInDB('VirtualMachine', [ + 'name' => 'DBSearchFilterVmAllowed-'.$sSuffix, + 'org_id' => $iAllowedOrgId, + 'osfamily_id' => $iAllowedOsFamilyId, + 'virtualhost_id' => $iVirtualHostId, + ]); + + $oDeniedPerson = $this->CreatePerson('Denied-'.$sSuffix, $iDeniedOrgId); + + $oUserRequest = $this->CreateUserRequest('Denied'.$sSuffix, [ + 'caller_id' => $oDeniedPerson->GetKey(), + 'org_id' => $iDeniedOrgId, + ]); + + // Add Virtual Machine to UserRequest lnk + $oLinkSet = new ormLinkSet(UserRequest::class, 'functionalcis_list', DBObjectSet::FromScratch(lnkFunctionalCIToTicket::class)); + + $oLink = MetaModel::NewObject(lnkFunctionalCIToTicket::class, ['functionalci_id' => $iDeniedVMId]); + $oLinkSet->AddItem($oLink); + + $oUserRequest->Set('functionalcis_list', $oLinkSet); + $oUserRequest->DBUpdate(); + + return [ + 'allowed_org_id' => $iAllowedOrgId, + 'allowed_org_name' => $sAllowedOrgName, + 'denied_org_name' => $sDeniedOrgName, + ]; + } + + private function LoginRestrictedUser(int $iAllowedOrgId, string $sProfileName): void + { + $sLogin = $this->GivenUserRestrictedToAnOrganizationInDB($iAllowedOrgId, self::$aURP_Profiles[$sProfileName]); + UserRights::Login($sLogin); + } +}