From 5a434486443a2cf8b8a288475aada54d0a068ca7 Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Wed, 15 Nov 2023 10:31:00 +0100 Subject: [PATCH] =?UTF-8?q?N=C2=B06458=20Security=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../userrightsprofile.class.inc.php | 5 +- .../userrightsprofile.db.class.inc.php | 6 +- application/applicationextension.inc.php | 2 + application/cmdbabstract.class.inc.php | 5 + application/utils.inc.php | 32 +++ core/coreexception.class.inc.php | 37 ++- core/dbobject.class.php | 112 +++++++- core/metamodel.class.php | 8 +- core/userrights.class.inc.php | 9 +- .../portal/src/Form/ObjectFormManager.php | 19 +- pages/UI.php | 2 + tests/php-unit-tests/README.md | 12 +- .../src/BaseTestCase/ItopDataTestCase.php | 33 ++- .../unitary-tests/core/DBObjectTest.php | 263 +++++++++++++++++- 14 files changed, 504 insertions(+), 41 deletions(-) diff --git a/addons/userrights/userrightsprofile.class.inc.php b/addons/userrights/userrightsprofile.class.inc.php index 9e0ed3230..588838cff 100644 --- a/addons/userrights/userrightsprofile.class.inc.php +++ b/addons/userrights/userrightsprofile.class.inc.php @@ -829,8 +829,9 @@ class UserRightsProfile extends UserRightsAddOnAPI } /** - * Find out which attribute is corresponding the the dimension 'owner org' - * returns null if no such attribute has been found (no filtering should occur) + * @param string $sClass + * @return string|null Find out which attribute is corresponding the dimension 'owner org' + * returns null if no such attribute has been found (no filtering should occur) */ public static function GetOwnerOrganizationAttCode($sClass) { diff --git a/addons/userrights/userrightsprofile.db.class.inc.php b/addons/userrights/userrightsprofile.db.class.inc.php index f35fa6af7..1d23fe811 100644 --- a/addons/userrights/userrightsprofile.db.class.inc.php +++ b/addons/userrights/userrightsprofile.db.class.inc.php @@ -604,10 +604,10 @@ class UserRightsProfile extends UserRightsAddOnAPI /** * Read and cache organizations allowed to the given user * - * @param $oUser - * @param $sClass (not used here but can be used in overloads) + * @param User $oUser + * @param string $sClass (not used here but can be used in overloads) * - * @return array + * @return array keys of the User allowed org * @throws \CoreException * @throws \Exception */ diff --git a/application/applicationextension.inc.php b/application/applicationextension.inc.php index e50bd73aa..734554833 100644 --- a/application/applicationextension.inc.php +++ b/application/applicationextension.inc.php @@ -1621,6 +1621,8 @@ class RestUtils * * @return DBObject The object found * @throws Exception If the input structure is not valid or it could not find exactly one object + * + * @see DBObject::CheckChangedExtKeysValues() generic method to check that we can access the linked object isn't used in that use case because values can be literal, OQL, friendlyname */ public static function FindObjectFromKey($sClass, $key, $bAllowNullValue = false) { diff --git a/application/cmdbabstract.class.inc.php b/application/cmdbabstract.class.inc.php index f4844313e..ba60776c6 100644 --- a/application/cmdbabstract.class.inc.php +++ b/application/cmdbabstract.class.inc.php @@ -4764,6 +4764,11 @@ EOF ); if ($bResult && (!$bPreview)) { + // doing the check will load multiple times same objects :/ + // but it shouldn't cost too much on execution time + // user can mitigate by selecting less extkeys/lnk to set and/or less objects to update 🤷‍♂️ + $oObj->CheckChangedExtKeysValues(); + $oObj->DBUpdate(); } } diff --git a/application/utils.inc.php b/application/utils.inc.php index 7133e20fa..6564dae43 100644 --- a/application/utils.inc.php +++ b/application/utils.inc.php @@ -2340,6 +2340,38 @@ class utils return in_array($sClass, $aHugeClasses); } + /** + * Helper around the native strlen() PHP method to test a string for null or empty value + * + * @link https://www.php.net/releases/8.1/en.php#deprecations_and_bc_breaks "Passing null to non-nullable internal function parameters is deprecated" + * + * @param string|null $sString + * + * @return bool if string null or empty + * @since 3.0.2 N°5302 + * @since 2.7.10 N°6458 add method in the 2.7 branch + */ + public static function IsNullOrEmptyString(?string $sString): bool + { + return $sString === null || strlen($sString) === 0; + } + + /** + * Helper around the native strlen() PHP method to test a string not null or empty value + * + * @link https://www.php.net/releases/8.1/en.php#deprecations_and_bc_breaks "Passing null to non-nullable internal function parameters is deprecated" + * + * @param string|null $sString + * + * @return bool if string is not null and not empty + * @since 3.0.2 N°5302 + * @since 2.7.10 N°6458 add method in the 2.7 branch + */ + public static function IsNotNullOrEmptyString(?string $sString): bool + { + return !static::IsNullOrEmptyString($sString); + } + /** * Check if iTop is in a development environment (VCS vs build number) * diff --git a/core/coreexception.class.inc.php b/core/coreexception.class.inc.php index b29ece9b1..38d862f19 100644 --- a/core/coreexception.class.inc.php +++ b/core/coreexception.class.inc.php @@ -229,12 +229,47 @@ class CoreUnexpectedValue extends CoreException { } +/** + * @since 2.7.10 3.0.4 3.1.1 3.2.0 N°6458 object creation + */ +class InvalidExternalKeyValueException extends CoreUnexpectedValue +{ + private const ENUM_PARAMS_OBJECT = 'current_object'; + private const ENUM_PARAMS_ATTCODE = 'attcode'; + private const ENUM_PARAMS_ATTVALUE = 'attvalue'; + private const ENUM_PARAMS_USER = 'current_user'; + + public function __construct($oObject, $sAttCode, $aContextData = null, $oPrevious = null) + { + $aContextData[self::ENUM_PARAMS_OBJECT] = get_class($oObject) . '::' . $oObject->GetKey(); + $aContextData[self::ENUM_PARAMS_ATTCODE] = $sAttCode; + $aContextData[self::ENUM_PARAMS_ATTVALUE] = $oObject->Get($sAttCode); + + $oCurrentUser = UserRights::GetUserObject(); + if (false === is_null($oCurrentUser)) { + $aContextData[self::ENUM_PARAMS_USER] = get_class($oCurrentUser) . '::' . $oCurrentUser->GetKey(); + } + + parent::__construct('Attribute pointing to an object that is either non existing or not readable by the current user', $aContextData, '', $oPrevious); + } + + public function GetAttCode(): string + { + return $this->getContextData()[self::ENUM_PARAMS_ATTCODE]; + } + + public function GetAttValue(): string + { + return $this->getContextData()[self::ENUM_PARAMS_ATTVALUE]; + } +} + class SecurityException extends CoreException { } /** - * Throwned when querying on an object that exists in the database but is archived + * Thrown when querying on an object that exists in the database but is archived * * @see N.1108 * @since 2.5.1 diff --git a/core/dbobject.class.php b/core/dbobject.class.php index 335c42c9e..fdf8ddba3 100644 --- a/core/dbobject.class.php +++ b/core/dbobject.class.php @@ -2235,6 +2235,83 @@ abstract class DBObject implements iDisplay return array($this->m_bCheckStatus, $this->m_aCheckIssues, $this->m_bSecurityIssue); } + /** + * Checks for extkey attributes values. This will throw exception on non-existing as well as non-accessible objects (silo, scopes). + * That's why the test is done for all users including Administrators + * + * Note that due to perf issues, this isn't called directly by the ORM, but has to be called by consumers when possible. + * + * @param callable(string, string):bool|null $oIsObjectLoadableCallback Override to check if object is accessible. + * Parameters are object class and key + * Return value should be false if cannot access object, true otherwise + * @return void + * + * @throws ArchivedObjectException + * @throws CoreException if cannot get object attdef list + * @throws CoreUnexpectedValue + * @throws InvalidExternalKeyValueException + * @throws MySQLException + * @throws SecurityException if one extkey is pointing to an invalid value + * + * @link https://github.com/Combodo/iTop/security/advisories/GHSA-245j-66p9-pwmh + * @since 2.7.10 3.0.4 3.1.1 3.2.0 N°6458 + * + * @see \RestUtils::FindObjectFromKey for the same check in the REST endpoint + */ + final public function CheckChangedExtKeysValues(callable $oIsObjectLoadableCallback = null) + { + if (is_null($oIsObjectLoadableCallback)) { + $oIsObjectLoadableCallback = function ($sClass, $sId) { + $oRemoteObject = MetaModel::GetObject($sClass, $sId, false); + if (is_null($oRemoteObject)) { + return false; + } + return true; + }; + } + + $aChanges = $this->ListChanges(); + $aAttCodesChanged = array_keys($aChanges); + foreach ($aAttCodesChanged as $sAttDefCode) { + $oAttDef = MetaModel::GetAttributeDef(get_class($this), $sAttDefCode); + + if ($oAttDef instanceof AttributeLinkedSetIndirect) { + /** @var ormLinkSet $oOrmSet */ + $oOrmSet = $this->Get($sAttDefCode); + while ($oLnk = $oOrmSet->Fetch()) { + $oLnk->CheckChangedExtKeysValues($oIsObjectLoadableCallback); + } + continue; + } + + /** @noinspection PhpConditionCheckedByNextConditionInspection */ + /** @noinspection NotOptimalIfConditionsInspection */ + if (($oAttDef instanceof AttributeHierarchicalKey) || ($oAttDef instanceof AttributeExternalKey)) { + $sRemoteObjectClass = $oAttDef->GetTargetClass(); + $sRemoteObjectKey = $this->Get($sAttDefCode); + } else if ($oAttDef instanceof AttributeObjectKey) { + $sRemoteObjectClassAttCode = $oAttDef->Get('class_attcode'); + $sRemoteObjectClass = $this->Get($sRemoteObjectClassAttCode); + $sRemoteObjectKey = $this->Get($sAttDefCode); + } else { + continue; + } + + /** @noinspection NotOptimalIfConditionsInspection */ + /** @noinspection TypeUnsafeComparisonInspection */ + if (utils::IsNullOrEmptyString($sRemoteObjectClass) + || utils::IsNullOrEmptyString($sRemoteObjectKey) + || ($sRemoteObjectKey == 0) // non-strict comparison as we might have bad surprises + ) { + continue; + } + + if (false === $oIsObjectLoadableCallback($sRemoteObjectClass, $sRemoteObjectKey)) { + throw new InvalidExternalKeyValueException($this, $sAttDefCode); + } + } + } + /** * Check if it is allowed to delete the existing object from the database * @@ -2380,13 +2457,13 @@ abstract class DBObject implements iDisplay * @api * @api-advanced * - * @see \DBObject::ListPreviousValuesForUpdatedAttributes() to get previous values anywhere in the CRUD stack - * @see https://www.itophub.io/wiki/page?id=latest%3Acustomization%3Asequence_crud iTop CRUD stack documentation - * @return array attname => currentvalue List the attributes that have been changed using {@see DBObject::Set()}. + * @return array attcode => currentvalue List the attributes that have been changed using {@see DBObject::Set()}. * Reset during {@see DBObject::DBUpdate()} * @throws Exception + * @see \DBObject::ListPreviousValuesForUpdatedAttributes() to get previous values anywhere in the CRUD stack + * @see https://www.itophub.io/wiki/page?id=latest%3Acustomization%3Asequence_crud iTop CRUD stack documentation * @uses m_aCurrValues - */ + */ public function ListChanges() { if ($this->m_bIsInDB) @@ -2677,7 +2754,6 @@ abstract class DBObject implements iDisplay * @throws \Exception * * @internal - * */ public function DBInsertNoReload() { @@ -2957,8 +3033,6 @@ abstract class DBObject implements iDisplay * Persist an object to the DB, for the first time * * @api - * @see DBWrite - * * @return int|null inserted object key * * @throws \ArchivedObjectException @@ -2968,10 +3042,12 @@ abstract class DBObject implements iDisplay * @throws \CoreWarning * @throws \MySQLException * @throws \OQLException + * + * @see DBWrite */ public function DBInsert() { - $this->DBInsertNoReload(); + $this->DBInsertNoReload(); if (MetaModel::DBIsReadOnly()) { @@ -3070,13 +3146,13 @@ abstract class DBObject implements iDisplay * Update an object in DB * * @api - * @see DBObject::DBWrite() - * * @return int object key * * @throws \CoreException * @throws \CoreCannotSaveObjectException if CheckToWrite() returns issues * @throws \Exception + * + * @see DBObject::DBWrite() */ public function DBUpdate() { @@ -3084,6 +3160,7 @@ abstract class DBObject implements iDisplay { throw new CoreException("DBUpdate: could not update a newly created object, please call DBInsert instead"); } + // Protect against reentrance (e.g. cascading the update of ticket logs) static $aUpdateReentrance = array(); $sKey = get_class($this).'::'.$this->GetKey(); @@ -3415,13 +3492,18 @@ abstract class DBObject implements iDisplay /** * Make the current changes persistent - clever wrapper for Insert or Update - * - * @api + * + * @api * * @return int - * - * @throws \CoreCannotSaveObjectException - * @throws \CoreException + * + * @throws ArchivedObjectException + * @throws CoreCannotSaveObjectException + * @throws CoreException + * @throws CoreUnexpectedValue + * @throws CoreWarning + * @throws MySQLException + * @throws OQLException */ public function DBWrite() { diff --git a/core/metamodel.class.php b/core/metamodel.class.php index 27d4c70e1..4634e7ff3 100644 --- a/core/metamodel.class.php +++ b/core/metamodel.class.php @@ -1213,8 +1213,10 @@ abstract class MetaModel * * @return AttributeDefinition[] * @throws \CoreException + * + * @see GetAttributesList for attcode list */ - final static public function ListAttributeDefs($sClass) + final public static function ListAttributeDefs($sClass) { self::_check_subclass($sClass); return self::$m_aAttribDefs[$sClass]; @@ -1223,8 +1225,10 @@ abstract class MetaModel /** * @param string $sClass * - * @return array + * @return string[] list of attcodes * @throws \CoreException + * + * @see ListAttributeDefs to get AttributeDefinition array instead */ final public static function GetAttributesList($sClass) { diff --git a/core/userrights.class.inc.php b/core/userrights.class.inc.php index 4597d77fb..f40f1209e 100644 --- a/core/userrights.class.inc.php +++ b/core/userrights.class.inc.php @@ -817,6 +817,9 @@ class UserRights } } + /** + * @return string connected {@see User} login field value, otherwise empty string + */ public static function GetUser() { if (is_null(self::$m_oUser)) @@ -829,7 +832,9 @@ class UserRights } } - /** User */ + /** + * @return User|null + */ public static function GetUserObject() { if (is_null(self::$m_oUser)) @@ -1029,7 +1034,7 @@ class UserRights /** * @param string $sClass - * @param int $iActionCode + * @param int $iActionCode see UR_ACTION_* constants * @param DBObjectSet $oInstanceSet * @param User $oUser * @return int (UR_ALLOWED_YES|UR_ALLOWED_NO|UR_ALLOWED_DEPENDS) diff --git a/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php b/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php index 40fa93b99..43350b0f2 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php +++ b/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php @@ -31,6 +31,7 @@ use Combodo\iTop\Form\Field\LabelField; use Combodo\iTop\Form\Form; use Combodo\iTop\Form\FormManager; use Combodo\iTop\Portal\Helper\ApplicationHelper; +use Combodo\iTop\Portal\Helper\SecurityHelper; use CoreCannotSaveObjectException; use DBObject; use DBObjectSearch; @@ -41,6 +42,7 @@ use DOMDocument; use DOMXPath; use Exception; use InlineImage; +use InvalidExternalKeyValueException; use IssueLog; use MetaModel; use Symfony\Component\DependencyInjection\ContainerInterface; @@ -48,6 +50,7 @@ use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Exception\HttpException; use UserRights; use utils; +use const UR_ACTION_READ; /** * Description of ObjectFormManager @@ -1135,8 +1138,11 @@ class ObjectFormManager extends FormManager $bWasModified = $this->oObject->IsModified(); $bActivateTriggers = (!$bIsNew && $bWasModified); + /** @var SecurityHelper $oSecurityHelper */ + $oSecurityHelper = $this->oContainer->get('security_helper'); + // Forcing allowed writing on the object if necessary. This is used in some particular cases. - $bAllowWrite = $this->oContainer->get('security_helper')->IsActionAllowed($bIsNew ? UR_ACTION_CREATE : UR_ACTION_MODIFY, $sObjectClass, $this->oObject->GetKey()); + $bAllowWrite = $oSecurityHelper->IsActionAllowed($bIsNew ? UR_ACTION_CREATE : UR_ACTION_MODIFY, $sObjectClass, $this->oObject->GetKey()); if ($bAllowWrite) { $this->oObject->AllowWrite(true); } @@ -1144,12 +1150,15 @@ class ObjectFormManager extends FormManager // Writing object to DB try { + $this->oObject->CheckChangedExtKeysValues(function ($sClass, $sId) use ($oSecurityHelper): bool { + return $oSecurityHelper->IsActionAllowed(UR_ACTION_READ, $sClass, $sId); + }); $this->oObject->DBWrite(); - } - catch (CoreCannotSaveObjectException $e) { + } catch (CoreCannotSaveObjectException $e) { throw new Exception($e->getHtmlMessage()); - } - catch (Exception $e) { + } catch (InvalidExternalKeyValueException $e) { + throw new Exception($e->getIssue()); + } catch (Exception $e) { if ($bIsNew) { throw new Exception(Dict::S('Portal:Error:ObjectCannotBeCreated')); } diff --git a/pages/UI.php b/pages/UI.php index 1d464904c..a4f8c9bbf 100644 --- a/pages/UI.php +++ b/pages/UI.php @@ -1002,6 +1002,7 @@ HTML { throw new CoreCannotSaveObjectException(array('id' => $oObj->GetKey(), 'class' => $sClass, 'issues' => $aErrors)); } + $oObj->CheckChangedExtKeysValues(); // Transactions are now handled in DBUpdate $oObj->DBUpdate(); $sMessage = Dict::Format('UI:Class_Object_Updated', MetaModel::GetName(get_class($oObj)), $oObj->GetName()); @@ -1233,6 +1234,7 @@ HTML throw new CoreCannotSaveObjectException(array('id' => $oObj->GetKey(), 'class' => $sClass, 'issues' => $aErrors)); } + $oObj->CheckChangedExtKeysValues(); $oObj->DBInsertNoReload();// No need to reload IssueLog::Trace('Object created', $sClass, array( diff --git a/tests/php-unit-tests/README.md b/tests/php-unit-tests/README.md index e4058802c..b216a6b10 100644 --- a/tests/php-unit-tests/README.md +++ b/tests/php-unit-tests/README.md @@ -4,4 +4,14 @@ - Covers an iTop PHP class or method? - Most likely in "unitary-tests". - Covers the consistency of some data through the app? - - Most likely in "integration-tests". \ No newline at end of file + - Most likely in "integration-tests". + +## Tips + +### Measure the time spent in a test + +Simply cut'n paste the following line at several places within the test function: + +```php +if (isset($fStarted)) {echo 'L'.__LINE__.': '.round(microtime(true) - $fStarted, 3)."\n";} $fStarted = microtime(true); +``` diff --git a/tests/php-unit-tests/src/BaseTestCase/ItopDataTestCase.php b/tests/php-unit-tests/src/BaseTestCase/ItopDataTestCase.php index 6b6aa550c..8202312a0 100644 --- a/tests/php-unit-tests/src/BaseTestCase/ItopDataTestCase.php +++ b/tests/php-unit-tests/src/BaseTestCase/ItopDataTestCase.php @@ -15,7 +15,6 @@ namespace Combodo\iTop\Test\UnitTest; use ArchivedObjectException; use CMDBSource; -use Config; use Contact; use DBObject; use DBObjectSet; @@ -30,7 +29,6 @@ use lnkFunctionalCIToTicket; use MetaModel; use Person; use Server; -use SetupUtils; use TagSetFieldData; use Ticket; use URP_UserProfile; @@ -479,6 +477,35 @@ abstract class ItopDataTestCase extends ItopTestCase return $oUser; } + /** + * @param string $sLogin + * @param int $iProfileId + * + * @return \UserLocal + * @throws Exception + */ + protected function CreateContactlessUser($sLogin, $iProfileId, $sPassword = null) + { + if (empty($sPassword)) { + $sPassword = $sLogin; + } + + $oUserProfile = new URP_UserProfile(); + $oUserProfile->Set('profileid', $iProfileId); + $oUserProfile->Set('reason', 'UNIT Tests'); + $oSet = DBObjectSet::FromObject($oUserProfile); + /** @var \UserLocal $oUser */ + $oUser = $this->createObject('UserLocal', array( + 'login' => $sLogin, + 'password' => $sPassword, + 'language' => 'EN US', + 'profile_list' => $oSet, + )); + $this->debug("Created {$oUser->GetName()} ({$oUser->GetKey()})"); + + return $oUser; + } + /** * @param \DBObject $oUser * @param int $iProfileId @@ -658,7 +685,7 @@ abstract class ItopDataTestCase extends ItopTestCase * @return array * @throws Exception */ - protected function AddCIToTicket($oCI, $oTicket, $sImpactCode) + protected function AddCIToTicket($oCI, $oTicket, $sImpactCode = 'manual') { $oNewLink = new lnkFunctionalCIToTicket(); $oNewLink->Set('functionalci_id', $oCI->GetKey()); diff --git a/tests/php-unit-tests/unitary-tests/core/DBObjectTest.php b/tests/php-unit-tests/unitary-tests/core/DBObjectTest.php index 3f5630ec3..06d8f41e9 100644 --- a/tests/php-unit-tests/unitary-tests/core/DBObjectTest.php +++ b/tests/php-unit-tests/unitary-tests/core/DBObjectTest.php @@ -17,18 +17,20 @@ // along with iTop. If not, see // -/** - * Created by PhpStorm. - * User: Eric - * Date: 02/10/2017 - * Time: 13:58 - */ - namespace Combodo\iTop\Test\UnitTest\Core; +use Attachment; use Combodo\iTop\Test\UnitTest\ItopDataTestCase; use DBObject; +use InvalidExternalKeyValueException; +use lnkPersonToTeam; use MetaModel; +use Organization; +use Person; +use Team; +use User; +use UserRights; +use utils; /** @@ -39,6 +41,7 @@ use MetaModel; class DBObjectTest extends ItopDataTestCase { const CREATE_TEST_ORG = true; + const INVALID_OBJECT_KEY = 123456789; protected function setUp(): void { @@ -121,4 +124,250 @@ class DBObjectTest extends ItopDataTestCase $this->debug("ERROR: N°4967 - 'Previous Values For Updated Attributes' not updated if DBUpdate is called without modifying the object"); //$this->assertCount(0, $oOrg->ListPreviousValuesForUpdatedAttributes()); } + + private function GetAlwaysTrueCallback(): callable + { + return static function () { + return true; + }; + } + + private function GetAlwaysFalseCallback(): callable + { + return static function () { + return false; + }; + } + + /** + * @covers DBObject::CheckChangedExtKeysValues() + */ + public function testCheckExtKeysSiloOnAttributeExternalKey() + { + //--- Preparing data... + $oAlwaysTrueCallback = $this->GetAlwaysTrueCallback(); + $oAlwaysFalseCallback = $this->GetAlwaysFalseCallback(); + + /** @var Organization $oDemoOrg */ + $oDemoOrg = MetaModel::GetObjectByName(Organization::class, 'Demo'); + /** @var Organization $oMyCompanyOrg */ + $oMyCompanyOrg = MetaModel::GetObjectByName(Organization::class, 'My Company/Department'); + + /** @var Person $oPersonOfDemoOrg */ + $oPersonOfDemoOrg = MetaModel::GetObjectByName(Person::class, 'Agatha Christie'); + /** @var Person $oPersonOfMyCompanyOrg */ + $oPersonOfMyCompanyOrg = MetaModel::GetObjectByName(Person::class, 'My first name My last name'); + + $sConfigurationManagerProfileId = 3; // Access to Person objects + $oUserWithAllowedOrgs = $this->CreateDemoOrgUser($oDemoOrg, $sConfigurationManagerProfileId); + + $oAdminUser = MetaModel::GetObjectByName(User::class, 'admin', false); + if (is_null($oAdminUser)) { + $oAdminUser = $this->CreateUser('admin', 1); + } + + /** @var Person $oPersonObject */ + $oPersonObject = $this->CreatePerson(0, $oMyCompanyOrg->GetKey()); + + //--- Now we can do some tests ! + UserRights::Login($oUserWithAllowedOrgs->Get('login')); + try { + $oPersonObject->CheckChangedExtKeysValues(); + } catch (InvalidExternalKeyValueException $eCannotSave) { + $this->fail('Should skip external keys already written in Database'); + } + + $oPersonObject->Set('manager_id', $oPersonOfDemoOrg->GetKey()); + try { + $oPersonObject->CheckChangedExtKeysValues(); + } catch (InvalidExternalKeyValueException $eCannotSave) { + $this->fail('Should allow objects in the same org as the current user'); + } + + try { + $oPersonObject->CheckChangedExtKeysValues($oAlwaysFalseCallback); + $this->fail('Should consider the callback returning "false"'); + } catch (InvalidExternalKeyValueException $eCannotSave) { + // Ok, the exception was expected + } + + $oPersonObject->Set('manager_id', $oPersonOfMyCompanyOrg->GetKey()); + try { + $oPersonObject->CheckChangedExtKeysValues(); + $this->fail('Should not allow objects not being in the allowed orgs of the current user'); + } catch (InvalidExternalKeyValueException $eCannotSave) { + $this->assertEquals('manager_id', $eCannotSave->GetAttCode(), 'Should report the wrong external key attcode'); + $this->assertEquals($oMyCompanyOrg->GetKey(), $eCannotSave->GetAttValue(), 'Should report the unauthorized external key value'); + } + + try { + $oPersonObject->CheckChangedExtKeysValues($oAlwaysTrueCallback); + } catch (InvalidExternalKeyValueException $eCannotSave) { + $this->fail('Should consider the callback returning "true"'); + } + + // ugly hack to remove cached SQL queries :( + //FIXME In 3.0+ this won't be necessary anymore thanks to UserRights::Logoff + $this->SetNonPublicStaticProperty(MetaModel::class, 'aQueryCacheGetObject', []); + + UserRights::Login($oAdminUser->Get('login')); + $oPersonObject->CheckChangedExtKeysValues(); + $this->assertTrue(true, 'Admin user can create objects in any org'); + } + + /** + * @covers DBObject::CheckChangedExtKeysValues() + */ + public function testCheckExtKeysOnAttributeLinkedSetIndirect() + { + //--- Preparing data... + /** @var Organization $oDemoOrg */ + $oDemoOrg = MetaModel::GetObjectByName(Organization::class, 'Demo'); + /** @var Person $oPersonOnItDepartmentOrg */ + $oPersonOnItDepartmentOrg = MetaModel::GetObjectByName(Person::class, 'Anna Gavalda'); + /** @var Person $oPersonOnDemoOrg */ + $oPersonOnDemoOrg = MetaModel::GetObjectByName(Person::class, 'Claude Monet'); + + $sConfigManagerProfileId = 3; // access to Team and Contact objects + $oUserWithAllowedOrgs = $this->CreateDemoOrgUser($oDemoOrg, $sConfigManagerProfileId); + + //--- Now we can do some tests ! + UserRights::Login($oUserWithAllowedOrgs->Get('login')); + + $oTeam = MetaModel::NewObject(Team::class, [ + 'name' => 'The A Team', + 'org_id' => $oDemoOrg->GetKey() + ]); + + // Part 1 - Test with an invalid id (non-existing object) + // + $oPersonLinks = \DBObjectSet::FromScratch(lnkPersonToTeam::class); + $oPersonLinks->AddObject(MetaModel::NewObject(lnkPersonToTeam::class, [ + 'person_id' => self::INVALID_OBJECT_KEY, + ])); + $oTeam->Set('persons_list', $oPersonLinks); + + try { + $oTeam->CheckChangedExtKeysValues(); + $this->fail('An unknown object should be detected as invalid'); + } catch (InvalidExternalKeyValueException $e) { + // we are getting the exception on the lnk class + // In consequence attcode is `lnkPersonToTeam.person_id` instead of `Team.persons_list` + $this->assertEquals('person_id', $e->GetAttCode(), 'The reported attcode should be the external key on the link'); + $this->assertEquals(self::INVALID_OBJECT_KEY, $e->GetAttValue(), 'The reported value should be the external key on the link'); + } + + try { + $oTeam->CheckChangedExtKeysValues($this->GetAlwaysTrueCallback()); + } catch (InvalidExternalKeyValueException $e) { + $this->fail('Should have no error when callback returns true'); + } + + // Part 2 - Test with an allowed object + // + $oPersonLinks = \DBObjectSet::FromScratch(lnkPersonToTeam::class); + $oPersonLinks->AddObject(MetaModel::NewObject(lnkPersonToTeam::class, [ + 'person_id' => $oPersonOnDemoOrg->GetKey(), + ])); + $oTeam->Set('persons_list', $oPersonLinks); + + try { + $oTeam->CheckChangedExtKeysValues(); + } catch (InvalidExternalKeyValueException $e) { + $this->fail('An authorized object should be detected as valid'); + } + + try { + $oTeam->CheckChangedExtKeysValues($this->GetAlwaysFalseCallback()); + $this->fail('Should cascade the callback result when it is "false"'); + } catch (InvalidExternalKeyValueException $e) { + // Ok, the exception was expected + } + + // Part 3 - Test with a not allowed object + // + $oPersonLinks = \DBObjectSet::FromScratch(lnkPersonToTeam::class); + $oPersonLinks->AddObject(MetaModel::NewObject(lnkPersonToTeam::class, [ + 'person_id' => $oPersonOnItDepartmentOrg->GetKey(), + ])); + $oTeam->Set('persons_list', $oPersonLinks); + + try { + $oTeam->CheckChangedExtKeysValues(); + $this->fail('An unauthorized object should be detected as invalid'); + } + catch (InvalidExternalKeyValueException $e) { + // Ok, the exception was expected + } + + try { + $oTeam->CheckChangedExtKeysValues($this->GetAlwaysTrueCallback()); + } catch (InvalidExternalKeyValueException $e) { + $this->fail('Should cascade the callback result when it is "true"'); + } + + $oTeam->DBInsert(); // persisting invalid value and resets the object changed values + try { + $oTeam->CheckChangedExtKeysValues(); + } + catch (InvalidExternalKeyValueException $e) { + $this->fail('An unauthorized value should be ignored when it is not being modified'); + } + } + + /** + * @covers DBObject::CheckChangedExtKeysValues() + */ + public function testCheckExtKeysSiloOnAttributeObjectKey() + { + //--- Preparing data... + /** @var Organization $oDemoOrg */ + $oDemoOrg = MetaModel::GetObjectByName(Organization::class, 'Demo'); + /** @var Person $oPersonOnItDepartmentOrg */ + $oPersonOnItDepartmentOrg = MetaModel::GetObjectByName(Person::class, 'Anna Gavalda'); + /** @var Person $oPersonOnDemoOrg */ + $oPersonOnDemoOrg = MetaModel::GetObjectByName(Person::class, 'Claude Monet'); + + $sConfigManagerProfileId = 3; // access to Team and Contact objects + $oUserWithAllowedOrgs = $this->CreateDemoOrgUser($oDemoOrg, $sConfigManagerProfileId); + + //--- Now we can do some tests ! + UserRights::Login($oUserWithAllowedOrgs->Get('login')); + + $oAttachment = MetaModel::NewObject(Attachment::class, [ + 'item_class' => Person::class, + 'item_id' => $oPersonOnDemoOrg->GetKey(), + ]); + try { + $oAttachment->CheckChangedExtKeysValues(); + } catch (InvalidExternalKeyValueException $e) { + $this->fail('Should be allowed to create an attachment pointing to a ticket in the allowed org list'); + } + + $oAttachment = MetaModel::NewObject(Attachment::class, [ + 'item_class' => Person::class, + 'item_id' => $oPersonOnItDepartmentOrg->GetKey(), + ]); + try { + $oAttachment->CheckChangedExtKeysValues(); + $this->fail('There should be an error on attachment pointing to a non allowed org object'); + } catch (InvalidExternalKeyValueException $e) { + $this->assertEquals('item_id', $e->GetAttCode(), 'Should report the object key attribute'); + $this->assertEquals($oPersonOnItDepartmentOrg->GetKey(), $e->GetAttValue(), 'Should report the object key value'); + } + } + + private function CreateDemoOrgUser(Organization $oDemoOrg, string $sProfileId): User + { + utils::GetConfig()->SetModuleSetting('authent-local', 'password_validation.pattern', ''); + $oUserWithAllowedOrgs = $this->CreateContactlessUser('demo_test_' . __CLASS__, $sProfileId); + /** @var \URP_UserOrg $oUserOrg */ + $oUserOrg = \MetaModel::NewObject('URP_UserOrg', ['allowed_org_id' => $oDemoOrg->GetKey(),]); + $oAllowedOrgList = $oUserWithAllowedOrgs->Get('allowed_org_list'); + $oAllowedOrgList->AddItem($oUserOrg); + $oUserWithAllowedOrgs->Set('allowed_org_list', $oAllowedOrgList); + $oUserWithAllowedOrgs->DBWrite(); + + return $oUserWithAllowedOrgs; + } }