From ef93b740eb68e418692149c9991f9fc556febe42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Cailly?= <42278610+ccailly@users.noreply.github.com> Date: Thu, 1 Feb 2024 17:11:38 +0100 Subject: [PATCH] Automatically reload updated rights --- .../update_10.0.x_to_10.1.0/profiles.php | 49 ++++++++++ install/mysql/glpi-empty.sql | 4 +- src/Migration.php | 35 ++++++++ src/Profile.php | 31 ++++--- src/ProfileRight.php | 26 ++---- src/Session.php | 40 ++++++++- tests/functional/ProfileRight.php | 90 +++++++++++++++++++ tests/functional/Search.php | 4 + tests/functional/Session.php | 61 +++++++++++++ tests/units/Migration.php | 82 +++++++++++++++++ 10 files changed, 388 insertions(+), 34 deletions(-) create mode 100644 install/migrations/update_10.0.x_to_10.1.0/profiles.php create mode 100644 tests/functional/ProfileRight.php diff --git a/install/migrations/update_10.0.x_to_10.1.0/profiles.php b/install/migrations/update_10.0.x_to_10.1.0/profiles.php new file mode 100644 index 00000000000..67cb0baed54 --- /dev/null +++ b/install/migrations/update_10.0.x_to_10.1.0/profiles.php @@ -0,0 +1,49 @@ +. + * + * --------------------------------------------------------------------- + */ + +/** + * @var \Migration $migration + */ + +$migration->addField( + 'glpi_profiles', + 'last_rights_update', + 'timestamp', + [ + 'null' => false, + 'value' => null + ] +); +$migration->addKey('glpi_profiles', 'last_rights_update'); diff --git a/install/mysql/glpi-empty.sql b/install/mysql/glpi-empty.sql index 0aa949222de..0b4237b7935 100644 --- a/install/mysql/glpi-empty.sql +++ b/install/mysql/glpi-empty.sql @@ -5807,6 +5807,7 @@ CREATE TABLE `glpi_profiles` ( `managed_domainrecordtypes` text, `date_creation` timestamp NULL DEFAULT NULL, `2fa_enforced` tinyint NOT NULL DEFAULT '0', + `last_rights_update` timestamp NULL DEFAULT NULL, PRIMARY KEY (`id`), KEY `name` (`name`), KEY `interface` (`interface`), @@ -5815,7 +5816,8 @@ CREATE TABLE `glpi_profiles` ( KEY `date_creation` (`date_creation`), KEY `tickettemplates_id` (`tickettemplates_id`), KEY `changetemplates_id` (`changetemplates_id`), - KEY `problemtemplates_id` (`problemtemplates_id`) + KEY `problemtemplates_id` (`problemtemplates_id`), + KEY `last_rights_update` (`last_rights_update`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci ROW_FORMAT=DYNAMIC; diff --git a/src/Migration.php b/src/Migration.php index ea5fbd50567..0fcfeb6a885 100644 --- a/src/Migration.php +++ b/src/Migration.php @@ -1265,6 +1265,8 @@ public function addRight($name, $rights = ALLSTANDARDRIGHT, $requiredrights = [' ], sprintf('%1$s add right for %2$s', $this->version, $name) ); + + $this->updateProfileLastRightsUpdate($profile['id']); } $this->displayWarning( @@ -1329,6 +1331,8 @@ public function addRightByInterface($name, $right, $interface = 'central') ], sprintf('%1$s update right for %2$s', $this->version, $name) ); + + $this->updateProfileLastRightsUpdate($profile['id']); } $this->displayWarning( @@ -1398,6 +1402,8 @@ public function updateRight($name, $rights, $requiredrights = ['config' => READ ], sprintf('%1$s update right for %2$s', $this->version, $name) ); + + $this->updateProfileLastRightsUpdate($profile['id']); } $this->displayWarning( @@ -1409,6 +1415,35 @@ public function updateRight($name, $rights, $requiredrights = ['config' => READ ); } + /** + * Update last rights update for given profile. + * + * @param int $profile_id + * @return void + */ + private function updateProfileLastRightsUpdate(int $profile_id): void + { + /** @var \DBmysql $DB */ + global $DB; + + // Check if the 'last_rights_update' field exists before trying to update it. + // This field may not exist yet as it is added by a migration, and other migrations + // that add a right could be executed before the migration that adds this field. + if (!$DB->fieldExists('glpi_profiles', 'last_rights_update')) { + return; + } + + $DB->updateOrDie( + 'glpi_profiles', + [ + 'last_rights_update' => Session::getCurrentTime() + ], + [ + 'id' => $profile_id, + ] + ); + } + public function setOutputHandler($output_handler) { diff --git a/src/Profile.php b/src/Profile.php index 65dcf19add9..553e3a9af22 100644 --- a/src/Profile.php +++ b/src/Profile.php @@ -422,20 +422,23 @@ public function prepareInputForUpdate($input) // Check if profile edit right was removed - $can_edit_profile = $this->fields['profile'] & UPDATE == UPDATE; - $updated_value = $input['_profile'][UPDATE . "_0"] ?? null; - $update_profiles_right_was_removed = $updated_value !== null && !(bool) $updated_value; - if ( - $can_edit_profile - && $update_profiles_right_was_removed - && $this->isLastSuperAdminProfile() - ) { - Session::addMessageAfterRedirect( - __("Can't remove update right on this profile as it is the only remaining profile with this right."), - false, - ERROR - ); - unset($input['_profile']); + // `$this->fields['profile']` will not be present if the `profile` right is not present in DB for the current profile + if (array_key_exists('profile', $this->fields)) { + $can_edit_profile = $this->fields['profile'] & UPDATE == UPDATE; + $updated_value = $input['_profile'][UPDATE . "_0"] ?? null; + $update_profiles_right_was_removed = $updated_value !== null && !(bool) $updated_value; + if ( + $can_edit_profile + && $update_profiles_right_was_removed + && $this->isLastSuperAdminProfile() + ) { + Session::addMessageAfterRedirect( + __("Can't remove update right on this profile as it is the only remaining profile with this right."), + false, + ERROR + ); + unset($input['_profile']); + } } if (isset($input['interface']) && $input['interface'] == 'helpdesk' && $this->isLastSuperAdminProfile()) { diff --git a/src/ProfileRight.php b/src/ProfileRight.php index 4b689d23483..9945f930bf8 100644 --- a/src/ProfileRight.php +++ b/src/ProfileRight.php @@ -302,39 +302,29 @@ public static function updateProfileRights($profiles_id, array $rights = []) public function post_addItem($history = true) { // Refresh session rights to avoid log out and login when rights change - $this->forceCurrentSessionRights($this->fields['profiles_id'], $this->fields['name'], $this->fields['rights']); + $this->updateProfileLastRightsUpdate($this->fields['profiles_id']); } public function post_updateItem($history = true) { // Refresh session rights to avoid log out and login when rights change - $this->forceCurrentSessionRights($this->fields['profiles_id'], $this->fields['name'], $this->fields['rights']); + $this->updateProfileLastRightsUpdate($this->fields['profiles_id']); } /** - * Force rights for given rightname on current session. + * Update last rights update for given profile. * * @param int $profile_id - * @param string $rightname - * @param int $rights * @return void */ - private function forceCurrentSessionRights(int $profile_id, string $rightname, int $rights): void + private function updateProfileLastRightsUpdate(int $profile_id): void { - if ( - isset($_SESSION['glpiactiveprofile']['id']) - && $_SESSION['glpiactiveprofile']['id'] == $profile_id - && ( - !isset($_SESSION['glpiactiveprofile'][$rightname]) - || $_SESSION['glpiactiveprofile'][$rightname] != $rights - ) - ) { - $_SESSION['glpiactiveprofile'][$rightname] = $rights; - unset($_SESSION['glpimenu']); - } + Profile::getById($profile_id)->update([ + 'id' => $profile_id, + 'last_rights_update' => Session::getCurrentTime() + ]); } - /** * @since 085 * diff --git a/src/Session.php b/src/Session.php index 03ffb9451b3..abac9410146 100644 --- a/src/Session.php +++ b/src/Session.php @@ -995,9 +995,11 @@ public static function checkValidSessionId() } else { $user_table = User::getTable(); $pu_table = Profile_User::getTable(); + $profile_table = Profile::getTable(); $result = $DB->request( [ 'COUNT' => 'count', + 'SELECT' => [$profile_table . '.last_rights_update'], 'FROM' => $user_table, 'LEFT JOIN' => [ $pu_table => [ @@ -1005,6 +1007,12 @@ public static function checkValidSessionId() Profile_User::getTable() => 'users_id', $user_table => 'id' ] + ], + $profile_table => [ + 'FKEY' => [ + $pu_table => 'profiles_id', + $profile_table => 'id' + ] ] ], 'WHERE' => [ @@ -1013,10 +1021,20 @@ public static function checkValidSessionId() $user_table . '.is_deleted' => 0, $pu_table . '.profiles_id' => $profile_id, ] + getEntitiesRestrictCriteria($pu_table, 'entities_id', $entity_id, true), + 'GROUPBY' => [$profile_table . '.id'], ] ); - if ($result->current()['count'] === 0) { + + $row = $result->current(); + + if ($row['count'] === 0) { $valid_user = false; + } elseif ( + $row['last_rights_update'] !== null + && $row['last_rights_update'] > $_SESSION['glpiactiveprofile']['last_rights_update'] ?? 0 + ) { + Session::reloadCurrentProfile(); + $_SESSION['glpiactiveprofile']['last_rights_update'] = $row['last_rights_update']; } } @@ -2170,4 +2188,24 @@ public static function canWriteSessionFiles(): bool return $session_handler !== false && (strtolower($session_handler) !== 'files' || is_writable(GLPI_SESSION_DIR)); } + + /** + * Reload the current profile from the database + * Update the session variable accordingly + * + * @return void + */ + public static function reloadCurrentProfile(): void + { + $current_profile_id = $_SESSION['glpiactiveprofile']['id']; + + $profile = new Profile(); + if ($profile->getFromDB($current_profile_id)) { + $profile->cleanProfile(); + $_SESSION['glpiactiveprofile'] = array_merge( + $_SESSION['glpiactiveprofile'], + $profile->fields + ); + } + } } diff --git a/tests/functional/ProfileRight.php b/tests/functional/ProfileRight.php new file mode 100644 index 00000000000..6309163d465 --- /dev/null +++ b/tests/functional/ProfileRight.php @@ -0,0 +1,90 @@ +. + * + * --------------------------------------------------------------------- + */ + +namespace tests\units; + +use DbTestCase; + +class ProfileRight extends DbTestCase +{ + public function testUpdateProfileLastRightsUpdate() + { + global $DB; + + // Create a profile + $profile = getItemByTypeName('Profile', 'Super-Admin'); + + // Check that the last_rights_update field is not null + $this->variable($profile->fields['last_rights_update'])->isNotNull(); + + // Update the last_rights_update field to null + $this->updateItem('Profile', $profile->getID(), [ + 'last_rights_update' => null + ]); + $profile->getFromDB($profile->getID()); + + // Check that the last_rights_update field is null + $this->variable($profile->fields['last_rights_update'])->isNull(); + + // Create a profile right + $profileRight = $this->createItem('ProfileRight', [ + 'profiles_id' => $profile->getID(), + 'name' => 'testUpdateProfileLastRightsUpdate', + 'rights' => READ + ]); + $profile->getFromDB($profile->getID()); + + // Check that the last_rights_update field is not null + $this->variable($profile->fields['last_rights_update'])->isNotNull(); + + // Update the last_rights_update field to null + $this->updateItem('Profile', $profile->getID(), [ + 'last_rights_update' => null + ]); + $profile->getFromDB($profile->getID()); + + // Check that the last_rights_update field is null + $this->variable($profile->fields['last_rights_update'])->isNull(); + + // Update the profile right + $this->updateItem('ProfileRight', $profileRight->getID(), [ + 'rights' => READ | UPDATE + ]); + $profile->getFromDB($profile->getID()); + + // Check that the last_rights_update field is not null + $this->variable($profile->fields['last_rights_update'])->isNotNull(); + } +} diff --git a/tests/functional/Search.php b/tests/functional/Search.php index edcf1e6c19f..b67439efc44 100644 --- a/tests/functional/Search.php +++ b/tests/functional/Search.php @@ -37,6 +37,7 @@ use CommonDBTM; use CommonITILActor; +use DateTime; use DBConnection; use DbTestCase; use Document; @@ -1615,6 +1616,9 @@ public function testTickets() 'Ticket' => (\Ticket::READMY + \Ticket::READNEWTICKET) ]); + // reload current profile to take into account the new rights + $this->login('tech', 'tech'); + // do search and check presence of the created problem $data = \Search::prepareDatasForSearch('Ticket', ['reset' => 'reset']); \Search::constructSQL($data); diff --git a/tests/functional/Session.php b/tests/functional/Session.php index c2ba107c5cd..e6d00a53631 100644 --- a/tests/functional/Session.php +++ b/tests/functional/Session.php @@ -667,4 +667,65 @@ public function testGetRightNameForError($module, $right, $expected) \Session::loadLanguage('fr_FR'); $this->string(\Session::getRightNameForError($module, $right))->isEqualTo($expected); } + + /** + * Test the reloadCurrentProfile method. + * This test creates a new profile, assigns it to a user, + * and checks if the profile is correctly reloaded. + * + * @return void + */ + public function testReloadCurrentProfile(): void + { + global $DB; + + // Login as a user + $user = $this->login()->user; + + // Create a new profile with name 'testReloadCurrentProfile' and interface 'central' + $profile = $this->createItem( + 'Profile', + [ + 'name' => 'testReloadCurrentProfile', + 'interface' => 'central', + ] + ); + + // Create a new Profile_User item with the created profile and user + $this->createItem( + 'Profile_User', + [ + 'profiles_id' => $profile->getID(), + 'users_id' => $user->getID(), + 'entities_id' => \Session::getActiveEntity(), + ] + ); + + // Login again to refresh the user data + $user = $this->login()->user; + + // Assign the new profile to the user + \Session::changeProfile($profile->getID()); + + // Update or insert a new profilerights item with the created profile and 'ticket' rights + $DB->updateOrInsert( + 'glpi_profilerights', + [ + 'rights' => \Ticket::READALL + ], + [ + 'profiles_id' => $profile->getID(), + 'name' => 'ticket' + ], + ); + + // Assert that the current profile does not have 'ticket' rights set to \Ticket::READALL + $this->variable($_SESSION['glpiactiveprofile']['ticket'])->isNotEqualTo(\Ticket::READALL); + + // Reload the current profile + \Session::reloadCurrentProfile(); + + // Assert that the current profile now has 'ticket' rights set to \Ticket::READALL + $this->variable($_SESSION['glpiactiveprofile']['ticket'])->isEqualTo(\Ticket::READALL); + } } diff --git a/tests/units/Migration.php b/tests/units/Migration.php index 96fee6a1131..40d4ea7b64c 100644 --- a/tests/units/Migration.php +++ b/tests/units/Migration.php @@ -1300,4 +1300,86 @@ public function testRemoveConfig() ] ])))->isEqualTo(0); } + + public function reloadCurrentProfileDataProvider() + { + $data = [ + [ + 'fn' => function () { + $this->migration->addRight('testReloadCurrentProfile', READ); + }, + 'expected' => 'New rights has been added for testReloadCurrentProfile, you should review ACLs after update' + ], + [ + 'fn' => function () { + $this->migration->addRightByInterface('testReloadCurrentProfile', READ, ['interface' => 'central']); + }, + 'expected' => 'Rights has been updated for testReloadCurrentProfile, you should review ACLs after update' + ], + [ + 'fn' => function () { + $this->migration->updateRight('testReloadCurrentProfile', READ); + }, + 'expected' => 'Rights has been updated for testReloadCurrentProfile, you should review ACLs after update' + ], + ]; + + return $data; + } + + /** + * @dataProvider reloadCurrentProfileDataProvider + */ + public function testReloadCurrentProfile($fn, $expected) + { + global $DB; + + // Clean DB to handle potential failure of previous test + $DB->delete('glpi_profilerights', [ + 'name' => [ + 'testReloadCurrentProfile' + ] + ]); + + $sub_query = new \Glpi\DBAL\QuerySubQuery([ + 'SELECT' => [ + 'profiles_id' + ], + 'FROM' => 'glpi_profilerights', + 'WHERE' => [ + 'name' => 'config', + 'rights' => READ | UPDATE + ] + ]); + + $DB->update('glpi_profiles', [ + 'last_rights_update' => null + ], [ + 'id' => $sub_query + ]); + + //Test adding a READ right when profile has READ and UPDATE config right (Default) + $this->output($fn)->isEqualTo($expected); + + $last_rights_updates = $DB->request([ + 'SELECT' => [ + 'last_rights_update' + ], + 'FROM' => 'glpi_profiles', + 'WHERE' => [ + 'id' => $sub_query + ] + ]); + + foreach ($last_rights_updates as $last_rights_update) { + $this->variable($last_rights_update['last_rights_update'])->isEqualTo($_SESSION['glpi_currenttime']); + } + + // Clean DB after test + $DB->delete('glpi_profilerights', [ + 'name' => [ + 'testReloadCurrentProfile' + ] + ]); + } }