Skip to content

Commit 6b73ca8

Browse files
ywarnierywarnier
authored
Merge pull request #8663 from ywarnier/8661
Fix login as admin/hr + access to sessions report as HR
2 parents 215573f + 5fda380 commit 6b73ca8

4 files changed

Lines changed: 245 additions & 24 deletions

File tree

‎public/main/inc/ajax/model.ajax.php‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,15 @@
148148
api_not_allowed(true);
149149
}
150150
} elseif (in_array($action, $adminActions, true)) {
151-
api_protect_admin_script(true);
151+
// Admin actions whose result sets are already scoped to the entities the HR manager
152+
// (DRH) actually follows. Only these may be opened to DRH; the rest (e.g. get_sessions,
153+
// which lists every session on the platform) stay admin / session-admin only.
154+
$drhScopedActions = [
155+
'get_sessions_tracking',
156+
'get_user_course_report',
157+
'get_user_course_report_resumed',
158+
];
159+
api_protect_admin_script(true, in_array($action, $drhScopedActions, true));
152160
} else {
153161
api_protect_admin_script(true);
154162
}

‎src/CoreBundle/Repository/Node/UserRepository.php‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -735,6 +735,28 @@ public function getSessionAdmins(User $user)
735735
return $qb->getQuery()->getResult();
736736
}
737737

738+
/**
739+
* Whether $targetUserId is followed by the HR manager $drhId through the UserRelUser
740+
* RRHH relation. Native replacement for UserManager::is_user_followed_by_drh().
741+
*/
742+
public function isUserFollowedByDrh(int $targetUserId, int $drhId): bool
743+
{
744+
$count = (int) $this->getEntityManager()->createQueryBuilder()
745+
->select('COUNT(uru.id)')
746+
->from(UserRelUser::class, 'uru')
747+
->where('uru.user = :target')
748+
->andWhere('uru.friend = :drh')
749+
->andWhere('uru.relationType = :relationType')
750+
->setParameter('target', $targetUserId)
751+
->setParameter('drh', $drhId)
752+
->setParameter('relationType', UserRelUser::USER_RELATION_TYPE_RRHH)
753+
->getQuery()
754+
->getSingleScalarResult()
755+
;
756+
757+
return $count > 0;
758+
}
759+
738760
/**
739761
* Get number of users in URL.
740762
*

‎src/CoreBundle/Security/Authorization/LoginAsAuthorizationChecker.php‎

Lines changed: 41 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,9 @@
77
namespace Chamilo\CoreBundle\Security\Authorization;
88

99
use Chamilo\CoreBundle\Entity\User;
10+
use Chamilo\CoreBundle\Repository\Node\UserRepository;
1011
use Chamilo\CoreBundle\Settings\SettingsManager;
11-
use SessionManager;
12-
use UserManager;
12+
use Symfony\Component\Security\Core\Role\RoleHierarchyInterface;
1313

1414
use const COURSEMANAGER;
1515
use const STUDENT;
@@ -27,16 +27,19 @@
2727
* 4. Impersonator is a session administrator (ROLE_SESSION_MANAGER) -> may impersonate
2828
* only users with status STUDENT, plus COURSEMANAGER when the setting
2929
* session.allow_session_admin_login_as_teacher is enabled.
30-
* 5. Impersonator is a HR manager (ROLE_HR / DRH) -> may impersonate either
31-
* a) users followed via the UserRelUser RRHH relation, or
32-
* b) users belonging to one of their sessions when the setting
33-
* drh_can_access_all_session_content is enabled.
30+
* 5. Impersonator is a HR manager (ROLE_HR / DRH) -> may impersonate ONLY users they
31+
* follow directly via the UserRelUser RRHH relation, and only when the target holds no
32+
* privilege the DRH itself lacks. Merely belonging to a session the DRH coaches does
33+
* NOT grant impersonation rights, and a followed user with higher privileges (session
34+
* admin, platform admin, etc.) can never be impersonated.
3435
* 6. Default deny.
3536
*/
3637
final class LoginAsAuthorizationChecker
3738
{
3839
public function __construct(
3940
private readonly SettingsManager $settingsManager,
41+
private readonly UserRepository $userRepository,
42+
private readonly RoleHierarchyInterface $roleHierarchy,
4043
) {}
4144

4245
/**
@@ -82,17 +85,21 @@ private function isPlatformAdmin(User $user): bool
8285
}
8386

8487
/**
85-
* Replicates api_is_global_platform_admin(): the user is a platform admin AND is
86-
* registered on access URL #1 (the main installation). Falls back to the legacy
87-
* helper to honor multi-URL setups.
88+
* Replicates api_is_global_platform_admin(): a platform admin who is also a global
89+
* (super) admin. Resolved directly from the already-loaded User entity instead of the
90+
* legacy api_is_global_platform_admin() helper, which reloads the user through the
91+
* legacy Container. That Container is only wired by LegacyListener (kernel.request,
92+
* priority 7), but the Symfony firewall fires switch_user earlier (priority 8) — so
93+
* during a "login as" attempt the legacy Container is still null and the legacy helper
94+
* would fatal with "Call to a member function get() on null".
8895
*/
8996
private function isGlobalPlatformAdmin(User $user): bool
9097
{
9198
if (!$this->isPlatformAdmin($user)) {
9299
return false;
93100
}
94101

95-
return api_is_global_platform_admin($user->getId());
102+
return $user->isSuperAdmin();
96103
}
97104

98105
/**
@@ -130,23 +137,34 @@ private function canSessionAdminLoginAs(User $target): bool
130137
}
131138

132139
/**
133-
* HR manager (ROLE_HR / DRH) may login as:
134-
* - users they explicitly follow (UserRelUser RRHH relation), or
135-
* - users in one of their sessions when drh_can_access_all_session_content is enabled.
140+
* HR manager (ROLE_HR / DRH) may login as a user only when BOTH hold:
141+
* - the user is followed directly via the UserRelUser RRHH relation (legacy parity:
142+
* belonging to a session the DRH coaches is intentionally NOT enough), and
143+
* - the user does not hold any privilege the DRH lacks, so impersonation can never be
144+
* used to escalate privileges (e.g. a followed session admin or admin is refused).
145+
*
146+
* The RRHH lookup goes through UserRepository (Doctrine) instead of the legacy
147+
* UserManager helper, so the policy no longer depends on the legacy Container being
148+
* bootstrapped — which the Symfony firewall has not yet done when switch_user fires.
136149
*/
137150
private function canDrhLoginAs(User $impersonator, User $target): bool
138151
{
139-
if ('true' === (string) $this->settingsManager->getSetting('drh_can_access_all_session_content')) {
140-
$users = SessionManager::getAllUsersFromCoursesFromAllSessionFromStatus('drh_all', $impersonator->getId());
141-
if (\is_array($users)) {
142-
foreach ($users as $row) {
143-
if (isset($row['id']) && (int) $row['id'] === $target->getId()) {
144-
return true;
145-
}
146-
}
147-
}
152+
if (!$this->userRepository->isUserFollowedByDrh((int) $target->getId(), (int) $impersonator->getId())) {
153+
return false;
148154
}
149155

150-
return UserManager::is_user_followed_by_drh($target->getId(), $impersonator->getId());
156+
return !$this->targetHasHigherPrivileges($impersonator, $target);
157+
}
158+
159+
/**
160+
* Whether $target holds at least one (hierarchy-expanded) role that $impersonator does
161+
* not have. Used as a privilege-escalation guard for non-admin impersonators.
162+
*/
163+
private function targetHasHigherPrivileges(User $impersonator, User $target): bool
164+
{
165+
$impersonatorRoles = $this->roleHierarchy->getReachableRoleNames($impersonator->getRoles());
166+
$targetRoles = $this->roleHierarchy->getReachableRoleNames($target->getRoles());
167+
168+
return [] !== array_diff($targetRoles, $impersonatorRoles);
151169
}
152170
}

‎tests/CoreBundle/Security/Authorization/LoginAsAuthorizationCheckerTest.php‎

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,173 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/* For licensing terms, see /license.txt */
6+
7+
namespace Chamilo\Tests\CoreBundle\Security\Authorization;
8+
9+
use Chamilo\CoreBundle\Entity\Session;
10+
use Chamilo\CoreBundle\Entity\User;
11+
use Chamilo\CoreBundle\Entity\UserRelUser;
12+
use Chamilo\CoreBundle\Framework\Container;
13+
use Chamilo\CoreBundle\Repository\SessionRepository;
14+
use Chamilo\CoreBundle\Security\Authorization\LoginAsAuthorizationChecker;
15+
use Chamilo\Tests\AbstractApiTest;
16+
use Chamilo\Tests\ChamiloTestTrait;
17+
18+
/**
19+
* Authorization policy coverage for "login as" (switch_user).
20+
*
21+
* Every decision below is taken with the legacy Container forced to null, reproducing the
22+
* request ordering in production: the Symfony firewall fires switch_user on kernel.request
23+
* (priority 8) before LegacyListener (priority 7) wires the legacy Container. The checker
24+
* must therefore resolve every path natively (User entity + injected repositories), never
25+
* through legacy helpers that would fatal with "Call to a member function get() on null".
26+
*/
27+
class LoginAsAuthorizationCheckerTest extends AbstractApiTest
28+
{
29+
use ChamiloTestTrait;
30+
31+
// --- Platform admin paths ---------------------------------------------------------
32+
33+
public function testGlobalAdminCanImpersonateAdmin(): void
34+
{
35+
$impersonator = $this->createUser('login_as_global_admin', '', '', 'ROLE_GLOBAL_ADMIN');
36+
$target = $this->createUser('login_as_target_admin', '', '', 'ROLE_ADMIN');
37+
38+
$this->assertTrue($this->decide($impersonator, $target));
39+
}
40+
41+
public function testPlainAdminCanImpersonateAdmin(): void
42+
{
43+
$impersonator = $this->createUser('login_as_plain_admin', '', '', 'ROLE_ADMIN');
44+
$target = $this->createUser('login_as_other_admin', '', '', 'ROLE_ADMIN');
45+
46+
$this->assertTrue($this->decide($impersonator, $target));
47+
}
48+
49+
public function testPlainAdminCannotImpersonateGlobalAdmin(): void
50+
{
51+
$impersonator = $this->createUser('login_as_plain_admin_2', '', '', 'ROLE_ADMIN');
52+
$target = $this->createUser('login_as_global_admin_target', '', '', 'ROLE_GLOBAL_ADMIN');
53+
54+
$this->assertFalse($this->decide($impersonator, $target));
55+
}
56+
57+
// --- HR manager (DRH) paths -------------------------------------------------------
58+
59+
public function testHrManagerCanImpersonateRrhhFollowedStudent(): void
60+
{
61+
$drh = $this->createUser('login_as_drh', '', '', 'ROLE_HR');
62+
$student = $this->createUser('login_as_drh_student');
63+
$this->linkRrhh($student, $drh);
64+
65+
$this->assertTrue($this->decide($drh, $student));
66+
}
67+
68+
public function testHrManagerCannotImpersonateUnfollowedStudent(): void
69+
{
70+
$drh = $this->createUser('login_as_drh_2', '', '', 'ROLE_HR');
71+
$student = $this->createUser('login_as_unrelated_student');
72+
73+
$this->assertFalse($this->decide($drh, $student));
74+
}
75+
76+
/**
77+
* Legacy parity: belonging to a session the DRH coaches must NOT grant impersonation
78+
* rights — only the direct RRHH relation does.
79+
*/
80+
public function testHrManagerCannotImpersonateUserInCoachedSession(): void
81+
{
82+
$drh = $this->createUser('login_as_drh_3', '', '', 'ROLE_HR');
83+
$student = $this->createUser('login_as_session_student');
84+
// DRH coaches the session and the student is enrolled, but there is no RRHH relation.
85+
$this->enrolStudentInDrhSession($student, $drh);
86+
87+
$this->assertFalse($this->decide($drh, $student));
88+
}
89+
90+
/**
91+
* Privilege-escalation guard: a followed user holding a role the DRH lacks
92+
* (here ROLE_SESSION_MANAGER) must never be impersonable.
93+
*/
94+
public function testHrManagerCannotImpersonateFollowedSessionAdmin(): void
95+
{
96+
$drh = $this->createUser('login_as_drh_4', '', '', 'ROLE_HR');
97+
$sessionAdmin = $this->createUser('login_as_followed_session_admin', '', '', 'ROLE_SESSION_MANAGER');
98+
$this->linkRrhh($sessionAdmin, $drh);
99+
100+
$this->assertFalse($this->decide($drh, $sessionAdmin));
101+
}
102+
103+
public function testHrManagerCannotImpersonateFollowedAdmin(): void
104+
{
105+
$drh = $this->createUser('login_as_drh_5', '', '', 'ROLE_HR');
106+
$admin = $this->createUser('login_as_followed_admin', '', '', 'ROLE_ADMIN');
107+
$this->linkRrhh($admin, $drh);
108+
109+
$this->assertFalse($this->decide($drh, $admin));
110+
}
111+
112+
/**
113+
* A teacher is not "higher" than a DRH: ROLE_HR already reaches ROLE_TEACHER through the
114+
* role hierarchy, so impersonating a followed teacher is not an escalation.
115+
*/
116+
public function testHrManagerCanImpersonateFollowedTeacher(): void
117+
{
118+
$drh = $this->createUser('login_as_drh_6', '', '', 'ROLE_HR');
119+
$teacher = $this->createUser('login_as_followed_teacher', '', '', 'ROLE_TEACHER');
120+
$this->linkRrhh($teacher, $drh);
121+
122+
$this->assertTrue($this->decide($drh, $teacher));
123+
}
124+
125+
// --- Helpers ----------------------------------------------------------------------
126+
127+
/**
128+
* Runs canLoginAs() with the legacy Container forced to null, proving the decision is
129+
* free of any legacy-Container dependency.
130+
*/
131+
private function decide(User $impersonator, User $target): bool
132+
{
133+
/** @var LoginAsAuthorizationChecker $checker */
134+
$checker = self::getContainer()->get(LoginAsAuthorizationChecker::class);
135+
136+
$previous = Container::$container;
137+
Container::$container = null;
138+
139+
try {
140+
return $checker->canLoginAs($impersonator, $target);
141+
} finally {
142+
Container::$container = $previous;
143+
}
144+
}
145+
146+
private function linkRrhh(User $student, User $drh): void
147+
{
148+
$em = $this->getEntityManager();
149+
$em->persist(
150+
(new UserRelUser())
151+
->setUser($student)
152+
->setFriend($drh)
153+
->setRelationType(UserRelUser::USER_RELATION_TYPE_RRHH)
154+
);
155+
$em->flush();
156+
}
157+
158+
private function enrolStudentInDrhSession(User $student, User $drh): void
159+
{
160+
$sessionRepo = self::getContainer()->get(SessionRepository::class);
161+
162+
$course = $this->createCourse('LoginAs DRH course');
163+
$session = $this->createSession('LoginAs DRH session');
164+
$session
165+
->addCourse($course)
166+
->addUserInSession(Session::DRH, $drh)
167+
;
168+
$sessionRepo->update($session);
169+
170+
$sessionRepo->addUserInCourse(Session::STUDENT, $student, $course, $session);
171+
$sessionRepo->update($session);
172+
}
173+
}

0 commit comments

Comments
 (0)