From a848cb28f17bcab91886d7586ac007dfd3c9b95f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eric=20Espi=C3=A9?= Date: Fri, 22 Jun 2018 16:07:35 +0000 Subject: [PATCH] =?UTF-8?q?N=C2=B01436=20-=20Access=20control=20updated=20?= =?UTF-8?q?for=20grant=5Fby=5Fprofile=20categories=20of=20classes=20-=20Fi?= =?UTF-8?q?x=20access=20to=20internal=20classes=20form=20the=20core=20engi?= =?UTF-8?q?ne?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SVN:trunk[5903] --- .../userrightsprofile.class.inc.php | 69 ++++++++++++++++++- core/ormlinkset.class.inc.php | 21 ++++++ core/userrights.class.inc.php | 46 ++++++++----- dictionaries/en.dictionary.itop.ui.php | 1 + dictionaries/fr.dictionary.itop.ui.php | 1 + 5 files changed, 121 insertions(+), 17 deletions(-) diff --git a/addons/userrights/userrightsprofile.class.inc.php b/addons/userrights/userrightsprofile.class.inc.php index d4defa3bd..a4d006d47 100644 --- a/addons/userrights/userrightsprofile.class.inc.php +++ b/addons/userrights/userrightsprofile.class.inc.php @@ -203,6 +203,12 @@ class URP_Profiles extends UserRightsBaseClassGUI // preserve DB integrity by deleting links to users protected function OnDelete() { + // Don't remove admin profile + if ($this->Get('name') === ADMIN_PROFILE_NAME) + { + throw new SecurityException(Dict::Format('UI:Login:Error:AccessAdmin')); + } + // Note: this may break the rule that says: "a user must have at least ONE profile" ! $oLnkSet = $this->Get('user_list'); while($oLnk = $oLnkSet->Fetch()) @@ -300,13 +306,38 @@ class URP_UserProfile extends UserRightsBaseClassGUI $this->CheckIfProfileIsAllowed(UR_ACTION_DELETE); } + /** + * @param $iActionCode + * + * @throws \ArchivedObjectException + * @throws \CoreException + * @throws \SecurityException + */ protected function CheckIfProfileIsAllowed($iActionCode) { + // When initializing or admin, we need to let everything pass trough + if (!UserRights::IsLoggedIn() || UserRights::IsAdministrator()) { return; } + + // Only administrators can manage administrators + $iOrigUserId = $this->GetOriginal('userid'); + if (!empty($iOrigUserId)) + { + $oUser = MetaModel::GetObject('User', $iOrigUserId, true, true); + if (UserRights::IsAdministrator($oUser) && !UserRights::IsAdministrator()) + { + throw new SecurityException(Dict::Format('UI:Login:Error:AccessRestricted')); + } + } + $oUser = MetaModel::GetObject('User', $this->Get('userid'), true, true); + if (UserRights::IsAdministrator($oUser) && !UserRights::IsAdministrator()) + { + throw new SecurityException(Dict::Format('UI:Login:Error:AccessRestricted')); + } if (!UserRights::IsActionAllowed(get_class($this), $iActionCode, DBObjectSet::FromObject($this))) { throw new SecurityException(Dict::Format('UI:Error:ObjectCannotBeUpdated')); } - if (UserRights::IsLoggedIn() && !UserRights::IsAdministrator() && ($this->Get('profile') === ADMIN_PROFILE_NAME)) + if (!UserRights::IsAdministrator() && ($this->Get('profile') === ADMIN_PROFILE_NAME)) { throw new SecurityException(Dict::Format('UI:Login:Error:AccessAdmin')); } @@ -352,6 +383,42 @@ class URP_UserOrg extends UserRightsBaseClassGUI { return Dict::Format('UI:UserManagement:LinkBetween_User_And_Org', $this->Get('userlogin'), $this->Get('allowed_org_name')); } + + + protected function OnInsert() + { + $this->CheckIfOrgIsAllowed(); + } + + protected function OnUpdate() + { + $this->CheckIfOrgIsAllowed(); + } + + protected function OnDelete() + { + $this->CheckIfOrgIsAllowed(); + } + + /** + * @throws \CoreException + */ + protected function CheckIfOrgIsAllowed() + { + if (UserRights::IsAdministrator()) { return; } + + $oUser = UserRights::GetUserObject(); + $oAddon = UserRights::GetModuleInstance(); + $aOrgs = $oAddon->GetUserOrgs($oUser, ''); + if (count($aOrgs) > 0) + { + $iOrigOrgId = $this->GetOriginal('allowed_org_id'); + if ((!empty($iOrigOrgId) && !in_array($iOrigOrgId, $aOrgs)) || !in_array($this->Get('allowed_org_id'), $aOrgs)) + { + throw new SecurityException(Dict::Format('Class:User/Error:OrganizationNotAllowed')); + } + } + } } diff --git a/core/ormlinkset.class.inc.php b/core/ormlinkset.class.inc.php index 152ea4d11..72fda32b5 100644 --- a/core/ormlinkset.class.inc.php +++ b/core/ormlinkset.class.inc.php @@ -539,6 +539,27 @@ class ormLinkSet implements iDBObjectSetIterator, Iterator, SeekableIterator } } + /** + * Get the list of all modified (added, modified and removed) links + * + * @return array of link objects + * @throws \Exception + */ + public function ListModifiedLinks() + { + $aAdded = $this->aAdded; + $aModified = $this->aModified; + $aRemoved = array(); + if (count($this->aRemoved) > 0) + { + $oSearch = new DBObjectSearch($this->sClass); + $oSearch->AddCondition('id', $this->aRemoved, 'IN'); + $oSet = new DBObjectSet($oSearch); + $aRemoved = $oSet->ToArray(); + } + return array_merge($aAdded, $aModified, $aRemoved); + } + /** * @param DBObject $oHostObject */ diff --git a/core/userrights.class.inc.php b/core/userrights.class.inc.php index 3a7e4ee2c..26de51e02 100644 --- a/core/userrights.class.inc.php +++ b/core/userrights.class.inc.php @@ -283,18 +283,21 @@ abstract class User extends cmdbAbstractObject } } } - // Check that this user has at least one profile assigned - $oSet = $this->Get('profile_list'); - if ($oSet->Count() == 0) + // Check that this user has at least one profile assigned when profiles have changed + if (array_key_exists('profile_list', $aChanges)) { - $this->m_aCheckIssues[] = Dict::Format('Class:User/Error:AtLeastOneProfileIsNeeded'); + $oSet = $this->Get('profile_list'); + if ($oSet->Count() == 0) + { + $this->m_aCheckIssues[] = Dict::Format('Class:User/Error:AtLeastOneProfileIsNeeded'); + } } // Only administrators can manage administrators if (UserRights::IsAdministrator($this) && !UserRights::IsAdministrator()) { $this->m_aCheckIssues[] = Dict::Format('UI:Login:Error:AccessRestricted'); } - // Check users with restricted organizations + if (!UserRights::IsAdministrator()) { $oUser = UserRights::GetUserObject(); @@ -304,19 +307,28 @@ abstract class User extends cmdbAbstractObject $aOrgs = $oAddon->GetUserOrgs($oUser, ''); if (count($aOrgs) > 0) { - /** @var ORMLinkset $oSet */ - $oSet = $this->Get('allowed_org_list'); - if ($oSet->Count() == 0) + // Check that the modified User belongs to one of our organization + if (!in_array($this->GetOriginal('org_id'), $aOrgs) || !in_array($this->Get('org_id'), $aOrgs)) { - $this->m_aCheckIssues[] = Dict::Format('Class:User/Error:AtLeastOneOrganizationIsNeeded'); + $this->m_aCheckIssues[] = Dict::Format('Class:User/Error:UserOrganizationNotAllowed'); } - else + // Check users with restricted organizations when allowed organizations have changed + if ($this->IsNew() || array_key_exists('allowed_org_list', $aChanges)) { - while ($oUserOrg = $oSet->Fetch()) + $oSet = $this->get('allowed_org_list'); + if ($oSet->Count() == 0) { - if (!in_array($oUserOrg->Get('allowed_org_id'), $aOrgs)) + $this->m_aCheckIssues[] = Dict::Format('Class:User/Error:AtLeastOneOrganizationIsNeeded'); + } + else + { + $aModifiedLinks = $oSet->ListModifiedLinks(); + foreach($aModifiedLinks as $oLink) { - $this->m_aCheckIssues[] = Dict::Format('Class:User/Error:OrganizationNotAllowed'); + if (!in_array($oLink->Get('allowed_org_id'), $aOrgs)) + { + $this->m_aCheckIssues[] = Dict::Format('Class:User/Error:OrganizationNotAllowed'); + } } } } @@ -961,10 +973,12 @@ class UserRights } /** + * Add additional filter for organization silos to all the requests. + * * @param $sClass * @param array $aSettings * - * @return bool + * @return bool|\Expression */ public static function GetSelectFilter($sClass, $aSettings = array()) { @@ -975,8 +989,8 @@ class UserRights try { - // In case of pb, use AllowAllData internally - if (MetaModel::HasCategory($sClass, 'bizmodel') || MetaModel::HasCategory($sClass, 'grant_by_profile')) + // Check Bug 1436 for details + if (MetaModel::HasCategory($sClass, 'bizmodel')) { return self::$m_oAddOn->GetSelectFilter(self::$m_oUser, $sClass, $aSettings); } diff --git a/dictionaries/en.dictionary.itop.ui.php b/dictionaries/en.dictionary.itop.ui.php index 2d50bb618..9b20f0935 100644 --- a/dictionaries/en.dictionary.itop.ui.php +++ b/dictionaries/en.dictionary.itop.ui.php @@ -140,6 +140,7 @@ Dict::Add('EN US', 'English', 'English', array( 'Class:User/Error:AtLeastOneProfileIsNeeded' => 'At least one profile must be assigned to this user.', 'Class:User/Error:AtLeastOneOrganizationIsNeeded' => 'At least one organization must be assigned to this user.', 'Class:User/Error:OrganizationNotAllowed' => 'Organization not allowed.', + 'Class:User/Error:UserOrganizationNotAllowed' => 'The user account does not belong to your allowed organizations.', 'Class:UserInternal' => 'User Internal', 'Class:UserInternal+' => 'User defined within iTop', )); diff --git a/dictionaries/fr.dictionary.itop.ui.php b/dictionaries/fr.dictionary.itop.ui.php index d2dfc41b9..3435d32a8 100644 --- a/dictionaries/fr.dictionary.itop.ui.php +++ b/dictionaries/fr.dictionary.itop.ui.php @@ -164,6 +164,7 @@ Dict::Add('FR FR', 'French', 'Français', array( 'Class:User/Error:AtLeastOneProfileIsNeeded' => 'L\'utilisateur doit avoir au moins un profil.', 'Class:User/Error:AtLeastOneOrganizationIsNeeded' => 'L\'utilisateur doit avoir au moins une organisation.', 'Class:User/Error:OrganizationNotAllowed' => 'Organisation non autorisée.', + 'Class:User/Error:UserOrganizationNotAllowed' => 'L\'utilisateur n\'appartient pas à vos organisations.', 'Class:UserInternal' => 'Utilisateur interne', 'Class:UserInternal+' => 'Utilisateur défini dans iTop', 'Class:URP_Dimensions' => 'Dimension',