From 93981da74bf7c2f44be0c1f2cc43f69d561df340 Mon Sep 17 00:00:00 2001 From: Julio Montoya Date: Thu, 18 Feb 2021 15:23:08 +0100 Subject: [PATCH] Groups: Fix course group permissions --- config/packages/security.yaml | 6 +- public/main/inc/lib/api.lib.php | 6 +- src/CoreBundle/Entity/Course.php | 20 +--- src/CoreBundle/Entity/ResourceLink.php | 5 + .../EventListener/CourseListener.php | 4 +- .../Authorization/Voter/GroupVoter.php | 21 ++++- .../Authorization/Voter/ResourceNodeVoter.php | 94 ++++++++++--------- .../Authorization/Voter/ResourceVoter.php | 6 +- .../Authorization/Voter/SessionVoter.php | 14 +-- src/CourseBundle/Entity/CDocument.php | 2 +- src/CourseBundle/Entity/CGroup.php | 20 +++- src/LtiBundle/Util/Utils.php | 4 +- 12 files changed, 118 insertions(+), 84 deletions(-) diff --git a/config/packages/security.yaml b/config/packages/security.yaml index e59ee786a0..f5056643e4 100644 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -28,7 +28,7 @@ security: - ROLE_DIRECTOR - ROLE_JURY_PRESIDENT - ROLE_CURRENT_COURSE_TEACHER - - ROLE_CURRENT_SESSION_COURSE_TEACHER + - ROLE_CURRENT_COURSE_SESSION_TEACHER - ROLE_CURRENT_COURSE_GROUP_TEACHER ROLE_SUPER_ADMIN: [ROLE_ADMIN, ROLE_ALLOWED_TO_SWITCH] ROLE_GLOBAL_ADMIN: [ROLE_ADMIN, ROLE_ALLOWED_TO_SWITCH] @@ -41,8 +41,8 @@ security: ROLE_CURRENT_COURSE_TEACHER: [ROLE_CURRENT_COURSE_TEACHER, ROLE_CURRENT_COURSE_STUDENT] # Set in the course listener ROLE_CURRENT_COURSE_GROUP_STUDENT: [ROLE_CURRENT_COURSE_GROUP_STUDENT] # Set in the CourseListener ROLE_CURRENT_COURSE_GROUP_TEACHER: [ROLE_CURRENT_COURSE_GROUP_TEACHER, ROLE_CURRENT_COURSE_GROUP_STUDENT] - ROLE_CURRENT_SESSION_COURSE_STUDENT: [ROLE_CURRENT_SESSION_COURSE_STUDENT] - ROLE_CURRENT_SESSION_COURSE_TEACHER: [ROLE_CURRENT_SESSION_COURSE_STUDENT, ROLE_CURRENT_SESSION_COURSE_TEACHER] + ROLE_CURRENT_COURSE_SESSION_STUDENT: [ROLE_CURRENT_COURSE_SESSION_STUDENT] + ROLE_CURRENT_COURSE_SESSION_TEACHER: [ROLE_CURRENT_COURSE_SESSION_STUDENT, ROLE_CURRENT_COURSE_SESSION_TEACHER] ROLE_ANONYMOUS: [ROLE_ANONYMOUS] #access_decision_manager: # strategy can be: affirmative, unanimous or consensus diff --git a/public/main/inc/lib/api.lib.php b/public/main/inc/lib/api.lib.php index f7bbf07245..1bd14ba6d8 100644 --- a/public/main/inc/lib/api.lib.php +++ b/public/main/inc/lib/api.lib.php @@ -3240,7 +3240,7 @@ function api_is_course_admin() $user = api_get_current_user(); if ($user) { if ( - $user->hasRole('ROLE_CURRENT_SESSION_COURSE_TEACHER') || + $user->hasRole('ROLE_CURRENT_COURSE_SESSION_TEACHER') || $user->hasRole('ROLE_CURRENT_COURSE_TEACHER') ) { return true; @@ -7400,8 +7400,8 @@ function api_is_allowed_in_course() $user = api_get_current_user(); if ($user instanceof User) { - if ($user->hasRole('ROLE_CURRENT_SESSION_COURSE_STUDENT') || - $user->hasRole('ROLE_CURRENT_SESSION_COURSE_TEACHER') || + if ($user->hasRole('ROLE_CURRENT_COURSE_SESSION_STUDENT') || + $user->hasRole('ROLE_CURRENT_COURSE_SESSION_TEACHER') || $user->hasRole('ROLE_CURRENT_COURSE_STUDENT') || $user->hasRole('ROLE_CURRENT_COURSE_TEACHER') ) { diff --git a/src/CoreBundle/Entity/Course.php b/src/CoreBundle/Entity/Course.php index eea922eb49..2fc5b1d378 100644 --- a/src/CoreBundle/Entity/Course.php +++ b/src/CoreBundle/Entity/Course.php @@ -549,10 +549,7 @@ class Course extends AbstractResource implements ResourceInterface, ResourceWith return $this; } - /** - * @return bool - */ - public function hasUser(User $user) + public function hasUser(User $user): bool { $criteria = Criteria::create()->where( Criteria::expr()->eq('user', $user) @@ -561,10 +558,7 @@ class Course extends AbstractResource implements ResourceInterface, ResourceWith return $this->getUsers()->matching($criteria)->count() > 0; } - /** - * @return bool - */ - public function hasStudent(User $user) + public function hasStudent(User $user): bool { $criteria = Criteria::create()->where( Criteria::expr()->eq('user', $user) @@ -573,10 +567,7 @@ class Course extends AbstractResource implements ResourceInterface, ResourceWith return $this->getStudents()->matching($criteria)->count() > 0; } - /** - * @return bool - */ - public function hasTeacher(User $user) + public function hasTeacher(User $user): bool { $criteria = Criteria::create()->where( Criteria::expr()->eq('user', $user) @@ -585,10 +576,7 @@ class Course extends AbstractResource implements ResourceInterface, ResourceWith return $this->getTeachers()->matching($criteria)->count() > 0; } - /** - * @return bool - */ - public function hasGroup(CGroup $group) + public function hasGroup(CGroup $group): bool { /*$criteria = Criteria::create()->where( Criteria::expr()->eq('groups', $group) diff --git a/src/CoreBundle/Entity/ResourceLink.php b/src/CoreBundle/Entity/ResourceLink.php index 8d77c79a80..4ade09fda9 100644 --- a/src/CoreBundle/Entity/ResourceLink.php +++ b/src/CoreBundle/Entity/ResourceLink.php @@ -199,6 +199,11 @@ class ResourceLink return $this; } + public function hasCourse(): bool + { + return null !== $this->course; + } + public function hasGroup(): bool { return null !== $this->group; diff --git a/src/CoreBundle/EventListener/CourseListener.php b/src/CoreBundle/EventListener/CourseListener.php index 18a8c0bfde..78f6ccbd24 100644 --- a/src/CoreBundle/EventListener/CourseListener.php +++ b/src/CoreBundle/EventListener/CourseListener.php @@ -319,8 +319,8 @@ class CourseListener $user->removeRole('ROLE_CURRENT_COURSE_GROUP_STUDENT'); $user->removeRole('ROLE_CURRENT_COURSE_STUDENT'); $user->removeRole('ROLE_CURRENT_COURSE_TEACHER'); - $user->removeRole('ROLE_CURRENT_SESSION_COURSE_STUDENT'); - $user->removeRole('ROLE_CURRENT_SESSION_COURSE_TEACHER'); + $user->removeRole('ROLE_CURRENT_COURSE_SESSION_STUDENT'); + $user->removeRole('ROLE_CURRENT_COURSE_SESSION_TEACHER'); } } diff --git a/src/CoreBundle/Security/Authorization/Voter/GroupVoter.php b/src/CoreBundle/Security/Authorization/Voter/GroupVoter.php index 7fd3563fd5..e1259c3824 100644 --- a/src/CoreBundle/Security/Authorization/Voter/GroupVoter.php +++ b/src/CoreBundle/Security/Authorization/Voter/GroupVoter.php @@ -72,12 +72,23 @@ class GroupVoter extends Voter $group = $subject; // Legacy - return \GroupManager::userHasAccessToBrowse($user->getId(), $group); + //\GroupManager::userHasAccessToBrowse($user->getId(), $group); + $isTutor = $group->hasTutor($user); switch ($attribute) { case self::VIEW: - if (!$group->hasUserInCourse($user, $course)) { - $user->addRole(ResourceNodeVoter::ROLE_CURRENT_SESSION_COURSE_STUDENT); + if ($isTutor) { + $user->addRole(ResourceNodeVoter::ROLE_CURRENT_COURSE_GROUP_TEACHER); + + return true; + } + + if (0 === $group->getStatus()) { + return false; + } + + if ($group->hasMember($user)) { + $user->addRole(ResourceNodeVoter::ROLE_CURRENT_COURSE_GROUP_STUDENT); return true; } @@ -85,8 +96,8 @@ class GroupVoter extends Voter break; case self::EDIT: case self::DELETE: - if (!$session->hasCoachInCourseWithStatus($user, $course)) { - $user->addRole(ResourceNodeVoter::ROLE_CURRENT_SESSION_COURSE_TEACHER); + if ($isTutor) { + $user->addRole(ResourceNodeVoter::ROLE_CURRENT_COURSE_GROUP_TEACHER); return true; } diff --git a/src/CoreBundle/Security/Authorization/Voter/ResourceNodeVoter.php b/src/CoreBundle/Security/Authorization/Voter/ResourceNodeVoter.php index 7baf6eacf4..9d5014837c 100644 --- a/src/CoreBundle/Security/Authorization/Voter/ResourceNodeVoter.php +++ b/src/CoreBundle/Security/Authorization/Voter/ResourceNodeVoter.php @@ -33,8 +33,8 @@ class ResourceNodeVoter extends Voter public const ROLE_CURRENT_COURSE_STUDENT = 'ROLE_CURRENT_COURSE_STUDENT'; public const ROLE_CURRENT_COURSE_GROUP_TEACHER = 'ROLE_CURRENT_COURSE_GROUP_TEACHER'; public const ROLE_CURRENT_COURSE_GROUP_STUDENT = 'ROLE_CURRENT_COURSE_GROUP_STUDENT'; - public const ROLE_CURRENT_SESSION_COURSE_TEACHER = 'ROLE_CURRENT_SESSION_COURSE_TEACHER'; - public const ROLE_CURRENT_SESSION_COURSE_STUDENT = 'ROLE_CURRENT_SESSION_COURSE_STUDENT'; + public const ROLE_CURRENT_COURSE_SESSION_TEACHER = 'ROLE_CURRENT_COURSE_SESSION_TEACHER'; + public const ROLE_CURRENT_COURSE_SESSION_STUDENT = 'ROLE_CURRENT_COURSE_SESSION_STUDENT'; private $requestStack; private $security; @@ -93,10 +93,9 @@ class ResourceNodeVoter extends Voter protected function voteOnAttribute(string $attribute, $subject, TokenInterface $token): bool { error_log('resourceNode voteOnAttribute'); - $user = $token->getUser(); - // Make sure there is a user object (i.e. that the user is logged in) // Update. No, anons can enter a node depending in the visibility. + // $user = $token->getUser(); /*if (!$user instanceof UserInterface) { return false; }*/ @@ -127,6 +126,7 @@ class ResourceNodeVoter extends Voter break; } + $user = $token->getUser(); // Check if I'm the owner. $creator = $resourceNode->getCreator(); if ($creator instanceof UserInterface && @@ -144,14 +144,14 @@ class ResourceNodeVoter extends Voter $groupId = (int) $request->get('gid'); $links = $resourceNode->getResourceLinks(); - $linkFound = 0; + //$courseManager = $this->entityManager->getRepository(Course::class); //$sessionManager = $this->entityManager->getRepository(Session::class); - $course = null; + $linkFound = 0; $link = null; - $case = 0; // @todo implement view, edit, delete. + /** @var ResourceLink $link */ foreach ($links as $link) { // Block access if visibility is deleted. Creator and admin have already access. if (ResourceLink::VISIBILITY_DELETED === $link->getVisibility()) { @@ -241,9 +241,9 @@ class ResourceNodeVoter extends Voter break; }*/ } - //var_dump($linkFound, $link->getId()); exit; + //var_dump($linkFound, $link->getId(), $link->getVisibility()); exit; - // No link was found or not available. + // No link was found. if (0 === $linkFound) { return false; } @@ -253,7 +253,7 @@ class ResourceNodeVoter extends Voter $allowAnonsToSee = false; $rights = []; if ($rightFromResourceLink->count() > 0) { - // Taken rights from the link + // Taken rights from the link. $rights = $rightFromResourceLink; } else { // Taken the rights from the default tool @@ -267,8 +267,8 @@ class ResourceNodeVoter extends Voter $readerMask = self::getReaderMask(); $editorMask = self::getEditorMask(); - if ($courseId) { - // If is teacher. + if ($courseId && $link->hasCourse() && $link->getCourse()->getId() === $courseId) { + // If teacher. if ($this->security->isGranted(self::ROLE_CURRENT_COURSE_TEACHER)) { $resourceRight = new ResourceRight(); $resourceRight @@ -277,7 +277,7 @@ class ResourceNodeVoter extends Voter $rights[] = $resourceRight; } - // If is student. + // If student. if ($this->security->isGranted(self::ROLE_CURRENT_COURSE_STUDENT) && ResourceLink::VISIBILITY_PUBLISHED === $link->getVisibility() ) { @@ -289,7 +289,9 @@ class ResourceNodeVoter extends Voter } // For everyone. - if (ResourceLink::VISIBILITY_PUBLISHED === $link->getVisibility() && $link->getCourse()->isPublic()) { + if (ResourceLink::VISIBILITY_PUBLISHED === $link->getVisibility() && + $link->getCourse()->isPublic() + ) { $allowAnonsToSee = true; $resourceRight = new ResourceRight(); $resourceRight @@ -300,35 +302,43 @@ class ResourceNodeVoter extends Voter } if (!empty($groupId)) { - $resourceRight = new ResourceRight(); - $resourceRight - ->setMask($editorMask) - ->setRole(self::ROLE_CURRENT_COURSE_GROUP_TEACHER) - ; - $rights[] = $resourceRight; + /*var_dump($groupId); + foreach ($user->getRoles() as $role) { + var_dump($role); + }*/ + if ($this->security->isGranted(self::ROLE_CURRENT_COURSE_GROUP_TEACHER)) { + $resourceRight = new ResourceRight(); + $resourceRight + ->setMask($editorMask) + ->setRole(self::ROLE_CURRENT_COURSE_GROUP_TEACHER); + $rights[] = $resourceRight; + } - $resourceRight = new ResourceRight(); - $resourceRight - ->setMask($readerMask) - ->setRole(self::ROLE_CURRENT_COURSE_GROUP_STUDENT) - ; - $rights[] = $resourceRight; + if ($this->security->isGranted(self::ROLE_CURRENT_COURSE_GROUP_STUDENT)) { + $resourceRight = new ResourceRight(); + $resourceRight + ->setMask($readerMask) + ->setRole(self::ROLE_CURRENT_COURSE_GROUP_STUDENT); + $rights[] = $resourceRight; + } } if (!empty($sessionId)) { - $resourceRight = new ResourceRight(); - $resourceRight - ->setMask($editorMask) - ->setRole(self::ROLE_CURRENT_SESSION_COURSE_TEACHER) - ; - $rights[] = $resourceRight; + if ($this->security->isGranted(self::ROLE_CURRENT_COURSE_SESSION_TEACHER)) { + $resourceRight = new ResourceRight(); + $resourceRight + ->setMask($editorMask) + ->setRole(self::ROLE_CURRENT_COURSE_SESSION_TEACHER); + $rights[] = $resourceRight; + } - $resourceRight = new ResourceRight(); - $resourceRight - ->setMask($readerMask) - ->setRole(self::ROLE_CURRENT_SESSION_COURSE_STUDENT) - ; - $rights[] = $resourceRight; + if ($this->security->isGranted(self::ROLE_CURRENT_COURSE_SESSION_STUDENT)) { + $resourceRight = new ResourceRight(); + $resourceRight + ->setMask($readerMask) + ->setRole(self::ROLE_CURRENT_COURSE_SESSION_STUDENT); + $rights[] = $resourceRight; + } } if (empty($rights) && ResourceLink::VISIBILITY_PUBLISHED === $link->getVisibility()) { @@ -341,7 +351,7 @@ class ResourceNodeVoter extends Voter $rights[] = $resourceRight; } } - + //exit; //var_dump($allowAnonsToSee); /*foreach ($rights as $right) { var_dump($right->getRole()); @@ -367,8 +377,8 @@ class ResourceNodeVoter extends Voter $currentStudentGroup = new Role(self::ROLE_CURRENT_COURSE_GROUP_STUDENT); $currentTeacherGroup = new Role(self::ROLE_CURRENT_COURSE_GROUP_TEACHER); - $currentStudentSession = new Role(self::ROLE_CURRENT_SESSION_COURSE_STUDENT); - $currentTeacherSession = new Role(self::ROLE_CURRENT_SESSION_COURSE_TEACHER); + $currentStudentSession = new Role(self::ROLE_CURRENT_COURSE_SESSION_STUDENT); + $currentTeacherSession = new Role(self::ROLE_CURRENT_COURSE_SESSION_TEACHER); $superAdmin = new Role('ROLE_SUPER_ADMIN'); $admin = new Role('ROLE_ADMIN'); @@ -385,7 +395,7 @@ class ResourceNodeVoter extends Voter ->addRole($currentTeacher, self::ROLE_CURRENT_COURSE_STUDENT) ->addRole($currentStudentSession) - ->addRole($currentTeacherSession, self::ROLE_CURRENT_SESSION_COURSE_STUDENT) + ->addRole($currentTeacherSession, self::ROLE_CURRENT_COURSE_SESSION_STUDENT) ->addRole($currentStudentGroup) ->addRole($currentTeacherGroup, self::ROLE_CURRENT_COURSE_GROUP_STUDENT) diff --git a/src/CoreBundle/Security/Authorization/Voter/ResourceVoter.php b/src/CoreBundle/Security/Authorization/Voter/ResourceVoter.php index 52ee6f84a4..43e264727a 100644 --- a/src/CoreBundle/Security/Authorization/Voter/ResourceVoter.php +++ b/src/CoreBundle/Security/Authorization/Voter/ResourceVoter.php @@ -59,7 +59,7 @@ class ResourceVoter extends Voter self::DELETE, self::EXPORT, ]; - error_log('resource supports'); + //error_log('resource supports'); // if the attribute isn't one we support, return false if (!in_array($attribute, $options)) { return false; @@ -75,8 +75,8 @@ class ResourceVoter extends Voter protected function voteOnAttribute(string $attribute, $subject, TokenInterface $token): bool { - error_log('resource voteOnAttribute'); - $user = $token->getUser(); + /*error_log('resource voteOnAttribute'); + $user = $token->getUser();*/ return true; } diff --git a/src/CoreBundle/Security/Authorization/Voter/SessionVoter.php b/src/CoreBundle/Security/Authorization/Voter/SessionVoter.php index 8bde9aa943..0fce885f65 100644 --- a/src/CoreBundle/Security/Authorization/Voter/SessionVoter.php +++ b/src/CoreBundle/Security/Authorization/Voter/SessionVoter.php @@ -89,21 +89,21 @@ class SessionVoter extends Voter if (empty($session->getDuration())) { // General coach. if ($userIsGeneralCoach && $session->isActiveForCoach()) { - $user->addRole(ResourceNodeVoter::ROLE_CURRENT_SESSION_COURSE_TEACHER); + $user->addRole(ResourceNodeVoter::ROLE_CURRENT_COURSE_SESSION_TEACHER); return true; } // Course-Coach access. if ($userIsCourseCoach && $session->isActiveForCoach()) { - $user->addRole(ResourceNodeVoter::ROLE_CURRENT_SESSION_COURSE_TEACHER); + $user->addRole(ResourceNodeVoter::ROLE_CURRENT_COURSE_SESSION_TEACHER); return true; } // Student access if ($userIsStudent && $session->isActiveForStudent()) { - $user->addRole(ResourceNodeVoter::ROLE_CURRENT_SESSION_COURSE_STUDENT); + $user->addRole(ResourceNodeVoter::ROLE_CURRENT_COURSE_SESSION_STUDENT); //$token->setUser($user); @@ -113,19 +113,19 @@ class SessionVoter extends Voter if ($this->sessionIsAvailableByDuration($session, $user)) { if ($userIsGeneralCoach) { - $user->addRole(ResourceNodeVoter::ROLE_CURRENT_SESSION_COURSE_TEACHER); + $user->addRole(ResourceNodeVoter::ROLE_CURRENT_COURSE_SESSION_TEACHER); return true; } if ($userIsCourseCoach) { - $user->addRole(ResourceNodeVoter::ROLE_CURRENT_SESSION_COURSE_TEACHER); + $user->addRole(ResourceNodeVoter::ROLE_CURRENT_COURSE_SESSION_TEACHER); return true; } if ($userIsStudent) { - $user->addRole(ResourceNodeVoter::ROLE_CURRENT_SESSION_COURSE_STUDENT); + $user->addRole(ResourceNodeVoter::ROLE_CURRENT_COURSE_SESSION_STUDENT); return true; } @@ -137,7 +137,7 @@ class SessionVoter extends Voter $canEdit = $this->canEditSession($user, $session, false); if ($canEdit) { - $user->addRole(ResourceNodeVoter::ROLE_CURRENT_SESSION_COURSE_TEACHER); + $user->addRole(ResourceNodeVoter::ROLE_CURRENT_COURSE_SESSION_TEACHER); return true; } diff --git a/src/CourseBundle/Entity/CDocument.php b/src/CourseBundle/Entity/CDocument.php index 9ce946a39f..1303956e27 100644 --- a/src/CourseBundle/Entity/CDocument.php +++ b/src/CourseBundle/Entity/CDocument.php @@ -41,7 +41,7 @@ use Symfony\Component\Validator\Constraints as Assert; * "post"={ * "controller"=CreateResourceNodeFileAction::class, * "deserialize"=false, - * "security"="is_granted('ROLE_CURRENT_COURSE_TEACHER') or is_granted('ROLE_CURRENT_SESSION_COURSE_TEACHER')", + * "security"="is_granted('ROLE_CURRENT_COURSE_TEACHER') or is_granted('ROLE_CURRENT_COURSE_SESSION_TEACHER')", * "validation_groups"={"Default", "media_object_create", "document:write"}, * "openapi_context"={ * "requestBody"={ diff --git a/src/CourseBundle/Entity/CGroup.php b/src/CourseBundle/Entity/CGroup.php index 66c4167b1b..2146a50764 100644 --- a/src/CourseBundle/Entity/CGroup.php +++ b/src/CourseBundle/Entity/CGroup.php @@ -368,7 +368,7 @@ class CGroup extends AbstractResource implements ResourceInterface /** * Set wikiState. * - * @param bool $wikiState + * @param int $wikiState */ public function setWikiState($wikiState): self { @@ -501,6 +501,24 @@ class CGroup extends AbstractResource implements ResourceInterface return $this->members->count() > 0; } + public function hasMember(User $user): bool + { + $criteria = Criteria::create()->where( + Criteria::expr()->eq('user', $user) + ); + + return $this->members->matching($criteria)->count() > 0; + } + + public function hasTutor(User $user): bool + { + $criteria = Criteria::create()->where( + Criteria::expr()->eq('user', $user) + ); + + return $this->tutors->matching($criteria)->count() > 0; + } + public function getTutors() { return $this->tutors; diff --git a/src/LtiBundle/Util/Utils.php b/src/LtiBundle/Util/Utils.php index 1bce017cdb..d23291a47f 100644 --- a/src/LtiBundle/Util/Utils.php +++ b/src/LtiBundle/Util/Utils.php @@ -61,7 +61,9 @@ class Utils // return 'Learner,urn:lti:role:ims/lis/Learner/GuestLearner'; //} - if ($user->hasRole('ROLE_CURRENT_COURSE_STUDENT') || $user->hasRole('ROLE_CURRENT_SESSION_COURSE_STUDENT')) { + if ($user->hasRole('ROLE_CURRENT_COURSE_STUDENT') || + $user->hasRole('ROLE_CURRENT_COURSE_SESSION_STUDENT') + ) { return 'Learner'; }