From 87c40c90d65adead7eb9627c0694e7dd94e4b142 Mon Sep 17 00:00:00 2001 From: Julio Date: Tue, 24 Aug 2021 11:16:58 +0200 Subject: [PATCH] User roles: Use roles instead of status --- config/packages/security.yaml | 10 +-- public/main/inc/lib/api.lib.php | 64 ++++++++++--------- public/main/inc/lib/usermanager.lib.php | 30 +++------ src/CoreBundle/Entity/User.php | 20 ++++-- .../Schema/V200/Version20201212114910.php | 2 + .../Authorization/Voter/ResourceNodeVoter.php | 2 +- .../Repository/UserRepositoryTest.php | 42 ++++++++++++ 7 files changed, 110 insertions(+), 60 deletions(-) create mode 100644 tests/CoreBundle/Repository/UserRepositoryTest.php diff --git a/config/packages/security.yaml b/config/packages/security.yaml index 772a0da405..b59323d15b 100644 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -14,12 +14,13 @@ security: class: Chamilo\CoreBundle\Entity\User property: username role_hierarchy: - ROLE_STUDENT: [ROLE_USER, ROLE_STUDENT] + ROLE_STUDENT: [ROLE_USER] ROLE_ADMIN: - ROLE_USER + - ROLE_STUDENT + - ROLE_TEACHER - ROLE_QUESTION_MANAGER - ROLE_SESSION_MANAGER - - ROLE_TEACHER - ROLE_CURRENT_COURSE_TEACHER - ROLE_CURRENT_COURSE_SESSION_TEACHER - ROLE_CURRENT_COURSE_GROUP_TEACHER @@ -27,9 +28,10 @@ security: ROLE_GLOBAL_ADMIN: [ROLE_ADMIN, ROLE_ALLOWED_TO_SWITCH] # The user that installed the platform. ROLE_TEACHER: [ROLE_STUDENT] ROLE_RRHH: [ROLE_TEACHER, ROLE_ALLOWED_TO_SWITCH] - ROLE_QUESTION_MANAGER: [ROLE_STUDENT, ROLE_QUESTION_MANAGER] - ROLE_SESSION_MANAGER: [ROLE_STUDENT, ROLE_SESSION_MANAGER, ROLE_ALLOWED_TO_SWITCH] + ROLE_QUESTION_MANAGER: [ROLE_STUDENT] + ROLE_SESSION_MANAGER: [ROLE_STUDENT, ROLE_ALLOWED_TO_SWITCH] ROLE_STUDENT_BOSS: [ROLE_STUDENT] + ROLE_INVITEE: [ROLE_STUDENT] ROLE_CURRENT_COURSE_STUDENT: [ROLE_CURRENT_COURSE_STUDENT] # Set in the CourseListener ROLE_CURRENT_COURSE_TEACHER: [ROLE_CURRENT_COURSE_TEACHER, ROLE_CURRENT_COURSE_STUDENT] # Set in the course listener diff --git a/public/main/inc/lib/api.lib.php b/public/main/inc/lib/api.lib.php index 0602a7d68c..5edfee1d6b 100644 --- a/public/main/inc/lib/api.lib.php +++ b/public/main/inc/lib/api.lib.php @@ -2532,7 +2532,7 @@ function api_get_session_image($sessionId, User $user) { $sessionId = (int) $sessionId; $image = ''; - if ($user->hasRole('ROLE_STUDENT')) { + if (!$user->hasRole('ROLE_STUDENT')) { // Check whether is not a student if ($sessionId > 0) { $image = '  '.Display::return_icon( @@ -5341,40 +5341,41 @@ function api_is_global_platform_admin($user_id = null) /** * @param int $admin_id_to_check - * @param int $my_user_id + * @param int $userId * @param bool $allow_session_admin * * @return bool */ function api_global_admin_can_edit_admin( $admin_id_to_check, - $my_user_id = null, + $userId = 0, $allow_session_admin = false ) { - if (empty($my_user_id)) { - $my_user_id = api_get_user_id(); + if (empty($userId)) { + $userId = api_get_user_id(); } - $iam_a_global_admin = api_is_global_platform_admin($my_user_id); + $iam_a_global_admin = api_is_global_platform_admin($userId); $user_is_global_admin = api_is_global_platform_admin($admin_id_to_check); if ($iam_a_global_admin) { // Global admin can edit everything return true; - } else { - // If i'm a simple admin - $is_platform_admin = api_is_platform_admin_by_id($my_user_id); + } - if ($allow_session_admin) { - $is_platform_admin = api_is_platform_admin_by_id($my_user_id) || (SESSIONADMIN == api_get_user_status($my_user_id)); - } + // If i'm a simple admin + $is_platform_admin = api_is_platform_admin_by_id($userId); - if ($is_platform_admin) { - if ($user_is_global_admin) { - return false; - } else { - return true; - } + if ($allow_session_admin && !$is_platform_admin) { + $user = api_get_user_entity($userId); + $is_platform_admin = $user->hasRole('ROLE_SESSION_MANAGER'); + } + + if ($is_platform_admin) { + if ($user_is_global_admin) { + return false; + } else { + return true; } } @@ -5383,14 +5384,14 @@ function api_global_admin_can_edit_admin( /** * @param int $admin_id_to_check - * @param int $my_user_id + * @param int $userId * @param bool $allow_session_admin * * @return bool|null */ -function api_protect_super_admin($admin_id_to_check, $my_user_id = null, $allow_session_admin = false) +function api_protect_super_admin($admin_id_to_check, $userId = null, $allow_session_admin = false) { - if (api_global_admin_can_edit_admin($admin_id_to_check, $my_user_id, $allow_session_admin)) { + if (api_global_admin_can_edit_admin($admin_id_to_check, $userId, $allow_session_admin)) { return true; } else { api_not_allowed(); @@ -6859,18 +6860,23 @@ function api_register_campus($listCampus = true) } } +function api_user_has_role(string $role): bool +{ + $currentUser = api_get_current_user(); + + if (null === $currentUser) { + return false; + } + + return $currentUser->hasRole($role); +} + /** * Checks whether current user is a student boss. - * - * @global array $_user - * - * @return bool */ -function api_is_student_boss() +function api_is_student_boss(): bool { - $_user = api_get_user_info(); - - return isset($_user['status']) && STUDENT_BOSS == $_user['status']; + return api_user_has_role('ROLE_STUDENT_BOSS'); } /** diff --git a/public/main/inc/lib/usermanager.lib.php b/public/main/inc/lib/usermanager.lib.php index c676420728..da2ce65b7c 100644 --- a/public/main/inc/lib/usermanager.lib.php +++ b/public/main/inc/lib/usermanager.lib.php @@ -14,8 +14,6 @@ use ChamiloSession as Session; use Symfony\Component\HttpFoundation\File\UploadedFile; /** - * Class UserManager. - * * This library provides functions for user management. * Include/require it in your code to use its functionality. * @@ -289,8 +287,10 @@ class UserManager $user->setExpirationDate($expirationDate); } + $user->setRoleFromStatus($status); + // Add user to a group - $statusToGroup = [ + /*$statusToGroup = [ COURSEMANAGER => 'TEACHER', STUDENT => 'STUDENT', DRH => 'RRHH', @@ -304,7 +304,7 @@ class UserManager if ($group) { $user->addGroup($group); } - } + }*/ $repo->updateUser($user, true); @@ -1048,10 +1048,7 @@ class UserManager if (!empty($expiration_date)) { $expiration_date = api_get_utc_datetime($expiration_date); - $expiration_date = new \DateTime( - $expiration_date, - new DateTimeZone('UTC') - ); + $expiration_date = new \DateTime($expiration_date, new DateTimeZone('UTC')); } $user @@ -1075,28 +1072,19 @@ class UserManager $user->setPlainPassword($password); } - $statusToGroup = [ - COURSEMANAGER => 'TEACHER', - STUDENT => 'STUDENT', - DRH => 'RRHH', - SESSIONADMIN => 'SESSION_ADMIN', - STUDENT_BOSS => 'STUDENT_BOSS', - INVITEE => 'INVITEE', - ]; - - $group = Container::$container->get(GroupRepository::class)->findOneBy(['code' => $statusToGroup[$status]]); + $user->setRoleFromStatus($status); + /*$group = Container::$container->get(GroupRepository::class)->findOneBy(['code' => $statusToGroup[$status]]); if ($group) { $user->addGroup($group); - } + }*/ $userManager->updateUser($user, true); Event::addEvent(LOG_USER_UPDATE, LOG_USER_ID, $user_id); if (1 == $change_active) { + $event_title = LOG_USER_DISABLE; if (1 == $active) { $event_title = LOG_USER_ENABLE; - } else { - $event_title = LOG_USER_DISABLE; } Event::addEvent($event_title, LOG_USER_ID, $user_id); } diff --git a/src/CoreBundle/Entity/User.php b/src/CoreBundle/Entity/User.php index 54f3ffd6bf..907d104671 100644 --- a/src/CoreBundle/Entity/User.php +++ b/src/CoreBundle/Entity/User.php @@ -1790,6 +1790,21 @@ class User implements UserInterface, EquatableInterface, ResourceInterface, Reso return $this->hasRole('ROLE_SUPER_ADMIN'); } + public function setRoleFromStatus(int $status): void + { + $role = match ($status) { + COURSEMANAGER => 'ROLE_TEACHER', + STUDENT => 'ROLE_STUDENT', + DRH => 'ROLE_RRHH', + SESSIONADMIN => 'ROLE_SESSION_MANAGER', + STUDENT_BOSS => 'ROLE_STUDENT_BOSS', + INVITEE => 'ROLE_INVITEE', + default => 'ROLE_USER', + }; + + $this->addRole($role); + } + public function hasRole(string $role): bool { return \in_array(strtoupper($role), $this->getRoles(), true); @@ -1839,11 +1854,6 @@ class User implements UserInterface, EquatableInterface, ResourceInterface, Reso return $this; } - public function isUser(self $user = null): bool - { - return null !== $user && $this->getId() === $user->getId(); - } - public function removeRole(string $role): self { if (false !== $key = array_search(strtoupper($role), $this->roles, true)) { diff --git a/src/CoreBundle/Migrations/Schema/V200/Version20201212114910.php b/src/CoreBundle/Migrations/Schema/V200/Version20201212114910.php index c1f4528134..0b28445d4c 100644 --- a/src/CoreBundle/Migrations/Schema/V200/Version20201212114910.php +++ b/src/CoreBundle/Migrations/Schema/V200/Version20201212114910.php @@ -70,6 +70,8 @@ final class Version20201212114910 extends AbstractMigrationChamilo $this->write("Migrating user: #$userId"); $userEntity->setUuid(Uuid::v4()); + $userEntity->setRoleFromStatus($userEntity->getStatus()); + $creatorId = $userEntity->getCreatorId(); $creator = null; if (isset($userList[$adminId])) { diff --git a/src/CoreBundle/Security/Authorization/Voter/ResourceNodeVoter.php b/src/CoreBundle/Security/Authorization/Voter/ResourceNodeVoter.php index c4bb55c876..6f2c89e7df 100644 --- a/src/CoreBundle/Security/Authorization/Voter/ResourceNodeVoter.php +++ b/src/CoreBundle/Security/Authorization/Voter/ResourceNodeVoter.php @@ -434,7 +434,7 @@ class ResourceNodeVoter extends Voter // Admin can do everything $acl->allow($admin); - $acl->allow($superAdmin); + //$acl->allow($superAdmin); //if ($token instanceof AnonymousToken) { if ($token instanceof NullToken) { diff --git a/tests/CoreBundle/Repository/UserRepositoryTest.php b/tests/CoreBundle/Repository/UserRepositoryTest.php new file mode 100644 index 0000000000..b317c5f2ff --- /dev/null +++ b/tests/CoreBundle/Repository/UserRepositoryTest.php @@ -0,0 +1,42 @@ +get('doctrine')->getManager(); + + $userRepo = self::getContainer()->get(UserRepository::class); + $student = $this->createUser('student'); + $this->assertHasNoEntityViolations($student); + + $this->assertSame(1, \count($student->getRoles())); + $this->assertTrue(\in_array('ROLE_USER', $student->getRoles(), true)); + + $student->addRole('ROLE_STUDENT'); + $userRepo->update($student); + + $this->assertSame(2, \count($student->getRoles())); + + $student->addRole('ROLE_STUDENT'); + $userRepo->update($student); + + $this->assertSame(2, \count($student->getRoles())); + } +}