diff --git a/core/config.class.inc.php b/core/config.class.inc.php index 8dbc72ab4..3f4d5b22c 100644 --- a/core/config.class.inc.php +++ b/core/config.class.inc.php @@ -1740,6 +1740,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 7e1ae9d8f..ad5e0c6e5 100644 --- a/core/dbobjectsearch.class.php +++ b/core/dbobjectsearch.class.php @@ -1932,4 +1932,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 bcaf58236..64a755c3c 100644 --- a/core/dbsearch.class.php +++ b/core/dbsearch.class.php @@ -1048,21 +1048,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) { @@ -1524,4 +1510,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 0d300674b..5be4a4d7e 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/core/ormdocument.class.inc.php b/core/ormdocument.class.inc.php index 3efe00dbd..645cdb402 100644 --- a/core/ormdocument.class.inc.php +++ b/core/ormdocument.class.inc.php @@ -350,15 +350,18 @@ class ormDocument if (!is_object($oObj)) { // If access to the document is not granted, check if the access to the host object is allowed $oObj = MetaModel::GetObject($sClass, $id, false, true); + $bHasHostRights = false; if ($oObj instanceof Attachment) { $sItemClass = $oObj->Get('item_class'); $sItemId = $oObj->Get('item_id'); $oHost = MetaModel::GetObject($sItemClass, $sItemId, false, false); - if (!is_object($oHost)) { - $oObj = null; + if (is_object($oHost)) { + $bHasHostRights = true; } } - if (!is_object($oObj)) { + + // We could neither read the object nor get a host object matching our rights + if ($bHasHostRights !== true) { throw new Exception("Invalid id ($id) for class '$sClass' - the object does not exist or you are not allowed to view it"); } } 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); + } +} diff --git a/tests/php-unit-tests/unitary-tests/core/ormDocumentTest.php b/tests/php-unit-tests/unitary-tests/core/ormDocumentTest.php index be52ac079..3c238464a 100644 --- a/tests/php-unit-tests/unitary-tests/core/ormDocumentTest.php +++ b/tests/php-unit-tests/unitary-tests/core/ormDocumentTest.php @@ -7,14 +7,36 @@ namespace Combodo\iTop\Test\UnitTest\Core; +use Combodo\iTop\Application\WebPage\CaptureWebPage; use Combodo\iTop\Test\UnitTest\ItopDataTestCase; use ormDocument; +use UserRights; /** * Tests of the ormDocument class */ class ormDocumentTest extends ItopDataTestCase { + private const RESTRICTED_PROFILE = 'Configuration Manager'; + private int $iUserOrg; + private int $iOrgDifferentFromUser; + + protected function setUp(): void + { + parent::setUp(); + + $this->iUserOrg = $this->GivenObjectInDB('Organization', [ + 'name' => 'UserOrg', + ]); + + $this->iOrgDifferentFromUser = $this->GivenObjectInDB('Organization', [ + 'name' => 'OrgDifferentFromUser', + ]); + + $this->LoginRestrictedUser($this->iUserOrg, self::RESTRICTED_PROFILE); + $this->ResetMetaModelQueyCacheGetObject(); + } + /** * @inheritDoc */ @@ -248,4 +270,107 @@ class ormDocumentTest extends ItopDataTestCase $this->assertGreaterThanOrEqual($iMaxHeight, $aActualDimensions['height'], 'The new height should not be 0'); } + + /** + * Test that DownloadDocument enforces rights for documents + * + * @dataProvider DownloadDocumentRightsProvider + */ + public function testDownloadDocumentDifferentOrg(string $sTargetClass, string $sAttCode, string $sData, string $sFileName, ?string $sHostClass) + { + $iDeniedDocumentId = $this->CreateDownloadTargetInOrg($sTargetClass, $sAttCode, $this->iOrgDifferentFromUser, $sData, $sFileName, $sHostClass); + + $oPageDenied = new CaptureWebPage(); + ormDocument::DownloadDocument($oPageDenied, $sTargetClass, $iDeniedDocumentId, $sAttCode); + $sDeniedHtml = (string) $oPageDenied->GetHtml(); + $this->assertStringContainsString( + 'the object does not exist or you are not allowed to view it', + $sDeniedHtml, + 'Expected error message when rights are missing.' + ); + $this->assertStringNotContainsString($sData, $sDeniedHtml, 'Unexpected file data present when rights are missing.'); + } + + /** + * Test that DownloadDocument allows to retrieve document with the same org (or host object org) + * + * @dataProvider DownloadDocumentRightsProvider + */ + public function testDownloadDocumentSameOrg(string $sTargetClass, string $sAttCode, string $sData, string $sFileName, ?string $sHostClass) + { + $iAllowedDocumentId = $this->CreateDownloadTargetInOrg($sTargetClass, $sAttCode, $this->iUserOrg, $sData, $sFileName, $sHostClass); + + $oPageAllowed = new CaptureWebPage(); + ormDocument::DownloadDocument($oPageAllowed, $sTargetClass, $iAllowedDocumentId, $sAttCode); + $sAllowedHtml = (string) $oPageAllowed->GetHtml(); + $this->assertStringContainsString($sData, $sAllowedHtml, 'Expected file data present when rights are sufficient.'); + $this->assertStringNotContainsString('the object does not exist or you are not allowed to view it', $sAllowedHtml, 'Unexpected error message when rights are sufficient.'); + } + + public function DownloadDocumentRightsProvider(): array + { + return [ + 'DocumentFile' => [ + 'class' => 'DocumentFile', + 'data_attribute_id' => 'file', + 'data' => 'document_data', + 'file_name' => 'document.txt', + 'host_class' => null], + 'Attachment' => [ + 'class' => 'Attachment', + 'data_attribute_id' => 'contents', + 'data' => 'attachment_data', + 'file_name' => 'attachment.txt', + 'host_class' => 'UserRequest'], + ]; + } + + /** + * Helper to avoid duplicating object creation in tests + * Created objects and host objects depending on the Document class + * @param string $sTargetClass + * @param string $sAttCode + * @param int $iOrgId + * @param string $sData + * @param string $sFileName + * @param string|null $sHostClass + * @return int + * @throws \Exception + */ + private function CreateDownloadTargetInOrg(string $sTargetClass, string $sAttCode, int $iOrgId, string $sData, string $sFileName, ?string $sHostClass): int + { + + if ($sTargetClass === 'DocumentFile') { + return $this->GivenObjectInDB($sTargetClass, [ + 'name' => 'UnitTestDocFile_'.uniqid(), + 'org_id' => $iOrgId, + $sAttCode => new ormDocument($sData, 'text/plain', $sFileName), + ]); + } + + if ($sTargetClass === 'Attachment') { + $iHostId = $this->GivenObjectInDB($sHostClass, [ + 'title' => 'UnitTestUserRequest_'.uniqid(), + 'org_id' => $iOrgId, + 'description' => 'A user request for testing attachment download rights', + ]); + + return $this->GivenObjectInDB('Attachment', [ + 'item_class' => $sHostClass, + 'item_id' => $iHostId, + $sAttCode => new ormDocument($sData, 'text/plain', $sFileName), + ]); + } + + throw new \Exception("Unsupported target class: $sTargetClass"); + } + + private function LoginRestrictedUser(int $iAllowedOrgId, string $sProfileName): void + { + if (UserRights::IsLoggedIn()) { + UserRights::Logoff(); + } + $sLogin = $this->GivenUserRestrictedToAnOrganizationInDB($iAllowedOrgId, self::$aURP_Profiles[$sProfileName]); + UserRights::Login($sLogin); + } }