Implizite Berechtigungen aller API-Routen und relevanter UI-Stellen prüfen

[dennis531]

Die impliziten Berechtigungen scheinen an ein paar Stellen inkonsistent zu sein.

Notizen:

• Werden global konfigurierte Tutorrechte korrekt berücksichtigt?

[dennis531]

Suspicious locations

• Note: write perm includes the owner perm unless otherwise specified.

API-Endpunkte:

• Students can change the default playlists for planned recordings and livestreams #1205

• hardcoded dozent and should be write perm:

  studip-opencast-plugin/lib/Models/Videos.php

  Lines 574 to 576 in 25addf7

  	if ($this->haveCoursePerm('dozent', $user_id)) {
  	return 'owner';
  	}

• Routes\Video\VideoList::class:

  • hardcoded global tutor:

    studip-opencast-plugin/lib/Models/Videos.php

    Line 195 in 25addf7

    if ($perm->have_perm('tutor', $user_id)) {

    
    

    studip-opencast-plugin/lib/Models/Videos.php

    Line 65 in 25addf7

    if ($perm->have_perm('tutor', $user_id)) {

  • hardcoded tutor
    

    studip-opencast-plugin/lib/Models/Helpers.php

    Line 42 in c0f2d2a

    AND (seminar_user.status = 'dozent' OR seminar_user.status = 'tutor')

  • looks unnecessary complicated but should work:

    studip-opencast-plugin/lib/Models/Helpers.php

    Lines 81 to 103 in c0f2d2a

    	$stmt = \DBManager::get()->prepare("SELECT DISTINCT seminare.Seminar_id FROM seminare
    	$p_sql
    	INNER JOIN seminar_inst ON (seminare.Seminar_id = seminar_inst.seminar_id)
    	INNER JOIN Institute ON (seminar_inst.institut_id = Institute.Institut_id)
    	LEFT JOIN sem_types ON (sem_types.id = seminare.status)
    	LEFT JOIN sem_classes ON (sem_classes.id = sem_types.class)
    	INNER JOIN seminar_user AS dozenten ON (dozenten.Seminar_id = seminare.Seminar_id AND dozenten.status = 'dozent')
    	INNER JOIN auth_user_md5 AS dozentendata ON (dozenten.user_id = dozentendata.user_id)
    	WHERE sem_classes.studygroup_mode = '0'
    	AND (
    	CONCAT_WS(' ', seminare.VeranstaltungsNummer, seminare.name, seminare.Untertitel, dozentendata.Nachname) LIKE :search
    	OR CONCAT(dozentendata.Nachname, ', ', dozentendata.Vorname) LIKE :search
    	OR CONCAT_WS(' ', dozentendata.Vorname, dozentendata.Nachname) LIKE :search
    	OR dozentendata.Vorname LIKE :search OR dozentendata.Nachname LIKE :search
    	)
    	AND seminar_inst.institut_id IN (:institute_ids)
    	");
    	$stmt->execute([
    	':search' => '%%%',
    	':institute_ids' => $institute_ids,
    	':plugin_id' => $plugin_id
    	]);

• Routes\Video\VideoCopyToCourse:

  • hardcoded tutor perm:

    studip-opencast-plugin/lib/Routes/Video/VideoCopyToCourse.php

    Line 28 in d77885e

    if (!$perm->have_studip_perm('tutor', $course_id)) {

• Routes\Video\VideoSharesList & Routes\Video\VideoSharesUpdate:

  • Should be write perm & remove insecure cid check as redundant & hardcoded dozent
    

    studip-opencast-plugin/lib/Routes/Video/VideoSharesUpdate.php

    Lines 38 to 42 in d03027e

    	$access_denied = (empty($perm) || $perm != 'owner');
    	// if in a course, then dozents are allowed!
    	if ($access_denied && !empty($course_id)) {
    	$access_denied = !$GLOBALS['perm']->have_studip_perm('dozent', $course_id);
    	}

    
    

    studip-opencast-plugin/lib/Routes/Video/VideoSharesList.php

    Lines 35 to 39 in d03027e

    	$access_denied = (empty($perm) || $perm != 'owner');
    	// if in a course, then dozents are allowed!
    	if ($access_denied && !empty($course_id)) {
    	$access_denied = !$GLOBALS['perm']->have_studip_perm('dozent', $course_id);
    	}

• Playlists Model: A user with explicit read perm and as a lecturer always obtains read perm instead of the highest perm:

  studip-opencast-plugin/lib/Models/Playlists.php

  Lines 307 to 327 in 2f65a65

  	foreach ($this->perms as $uperm) {
  	if ($uperm->user_id == $user->id) {
  	$ret_perm = $uperm->perm;
  	}
  	}
  	if (!$ret_perm) {
  	// check if user is lecturer and therefore has owner rights or at least
  	// has read access through a linked course
  	foreach ($this->courses as $course) {
  	if ($perm->have_studip_perm('dozent', $course->id)) {
  	return 'owner';
  	}
  	if ($course->getParticipantStatus($user->id)) {
  	$ret_perm = 'read';
  	}
  	}
  	}
  	return $ret_perm;

• Hardcoded tutor:

  studip-opencast-plugin/lib/Routes/Playlist/PlaylistCopy.php

  Line 44 in 0ce5a87

  if (!$perm->have_studip_perm('tutor', $destination_course)) {

  
  

  studip-opencast-plugin/lib/Routes/Playlist/Authority.php

  Line 17 in 6d25e34

  if ($playlist->haveCoursePerm('tutor') && $video->haveCoursePerm('tutor')) {

  
  

  studip-opencast-plugin/lib/Routes/Playlist/Authority.php

  Line 45 in 6d25e34

  if ($playlist->haveCoursePerm('tutor') && $video->haveCoursePerm('tutor')) {

  
  

  studip-opencast-plugin/lib/Routes/Playlist/PlaylistAddToCourse.php

  Line 46 in ec63090

  if ($perm->have_studip_perm('tutor', $course['id'])) {

  
  

  studip-opencast-plugin/lib/Routes/Course/CourseConfig.php

  Line 39 in d4ebbeb

  if ($perm->have_studip_perm('tutor', $course_id)) {

  
  

  studip-opencast-plugin/lib/Routes/Course/CourseSetVisibility.php

  Line 27 in 6b839de

  if (!$perm->have_studip_perm('tutor', $course_id)) {

  
  

  studip-opencast-plugin/lib/Routes/Course/CourseSetUpload.php

  Line 27 in 6b839de

  if (!$perm->have_studip_perm('tutor', $course_id)) {

• Broken check as it is possible to pass a valid cid to get access to arbitrary playlists:

  studip-opencast-plugin/lib/Routes/Playlist/PlaylistVideoList.php

  Lines 33 to 49 in c8c1e75

  	// check if playlist is connected to the passed course and user is part of that course as well
  	$permission = false;
  	if ($params['cid']) {
  	if ($perm->have_studip_perm('user', $params['cid'])) {
  	$permission = true;
  	}
  	}
  	if (!$params['cid'] || !$permission) {
  	// check what permissions the current user has on the playlist
  	$uperm = $playlist->getUserPerm();
  	if (empty($uperm) || !$uperm)
  	{
  	throw new \AccessDeniedException();
  	}
  	}

  
  

  studip-opencast-plugin/lib/Routes/Course/CourseListForPlaylistVideos.php

  Lines 33 to 49 in c8c1e75

  	// check if playlist is connected to the passed course and user is part of that course as well
  	$permission = false;
  	if ($params['cid']) {
  	if ($perm->have_studip_perm('user', $params['cid'])) {
  	$permission = true;
  	}
  	}
  	if (!$params['cid'] || !$permission) {
  	// check what permissions the current user has on the playlist
  	$uperm = $playlist->getUserPerm();
  	if (empty($uperm) || !$uperm)
  	{
  	throw new \AccessDeniedException();
  	}
  	}

  
  

  studip-opencast-plugin/lib/Routes/Tags/TagListForPlaylistVideos.php

  Lines 31 to 46 in c8c1e75

  	$permission = false;
  	if ($params['cid']) {
  	if ($perm->have_studip_perm('user', $params['cid'])) {
  	$permission = true;
  	}
  	}
  	if (!$params['cid'] || !$permission) {
  	// check what permissions the current user has on the playlist
  	$uperm = $playlist->getUserPerm();
  	if (empty($uperm) || !$uperm)
  	{
  	throw new \AccessDeniedException();
  	}
  	}

• Student / Owner videos should be able to delete from course playlist by the owner:
  

  studip-opencast-plugin/lib/Routes/Playlist/Authority.php

  Lines 39 to 60 in 6d25e34

  	public static function canRemoveVideoFromPlaylist(\Seminar_User $user, Playlists $playlist, Videos $video, $course_id = null)
  	{
  	if ($GLOBALS['perm']->have_perm('root')) {
  	return true;
  	}
  	if ($playlist->haveCoursePerm('tutor') && $video->haveCoursePerm('tutor')) {
  	return true;
  	}
  	$perm_playlist = $playlist->getUserPerm();
  	$perm_video = reset($video->perms->findBy('user_id', $user->id)->toArray());
  	if (
  	(empty($perm_playlist) || ($perm_playlist != 'owner' && $perm_playlist != 'write')) ||
  	(empty($perm_video) || ($perm_video['perm'] != 'owner' && $perm_video['perm'] != 'write'))
  	) {
  	return false;
  	}
  	return true;
  	}

  
  Confusing or incorrect? use of this function:
  

  studip-opencast-plugin/lib/Routes/Playlist/PlaylistRemoveVideos.php

  Line 35 in 2f65a65

  if (!Authority::canAddVideoToPlaylist($user, $playlist, $video, $course_id)) {

• Hardcoded tutor and incorrect global perm:
  

  studip-opencast-plugin/lib/Routes/Schedule/ScheduleAdd.php

  Lines 20 to 22 in 6fffff2

  	if (!$perm->have_perm('tutor')) {
  	throw new \AccessDeniedException();
  	}

  
  

  studip-opencast-plugin/lib/Routes/Schedule/ScheduleBulk.php

  Lines 20 to 22 in 6fffff2

  	if (!$perm->have_perm('tutor')) {
  	throw new \AccessDeniedException();
  	}

  
  

  studip-opencast-plugin/lib/Routes/Schedule/ScheduleDelete.php

  Lines 20 to 22 in e12d085

  	if (!$perm->have_perm('tutor')) {
  	throw new \AccessDeniedException();
  	}

  
  

  studip-opencast-plugin/lib/Routes/Schedule/ScheduleUpdate.php

  Lines 20 to 22 in e12d085

  	if (!$perm->have_perm('tutor')) {
  	throw new \AccessDeniedException();
  	}

  
  

  studip-opencast-plugin/lib/Routes/Playlist/PlaylistScheduleUpdate.php

  Lines 21 to 23 in 6fffff2

  	if (!$perm->have_perm('tutor')) {
  	throw new \AccessDeniedException();
  	}

• Implicit perm check is missing:
  

  studip-opencast-plugin/lib/Routes/Course/CourseAddPlaylist.php

  Lines 42 to 45 in 996cdbc

  	if (empty($perm_playlist))
  	{
  	throw new \AccessDeniedException();
  	}

• Hardcoded dozent:
  

  studip-opencast-plugin/lib/Models/Tags.php

  Line 118 in 2ff87bc

  if (!$perm->have_studip_perm('dozent', $course_id)) {

• Hardcoded dozent and global permission check:
  

  studip-opencast-plugin/lib/Models/Tags.php

  Line 78 in 2ff87bc

  if (!(empty($cid) || $perm->have_perm('dozent'))) {

• Feedback should be allowed for users with read perm:
  

  studip-opencast-plugin/lib/Routes/Video/VideoReport.php

  Lines 27 to 29 in 6a40b53

  	if (empty($perm) ||
  	($perm != 'owner' && $perm != 'write'))
  	{

Courseware

• No perm check in set payload:
  

  studip-opencast-plugin/lib/BlockTypes/OpencastBlockV3.php

  Lines 71 to 76 in 9aa3965

  	public function setPayload($payload): void
  	{
  	$rangeId = $this->block->container->structural_element->range_id;
  	VideoCoursewareBlocks::setRecord($rangeId, $payload['token'], $this->block['id']);
  	parent::setPayload($payload);
  	}

Vue / UI:

• Remove because deprecated?:
  

  studip-opencast-plugin/vueapp/components/Courses/CoursesSidebar.vue

  Lines 355 to 357 in 63e521c

  	canToggleVisibility() {
  	return window.OpencastPlugin.STUDIP_VERSION == '4.6' && this.canEdit;
  	},

• Useless prop:
  

  studip-opencast-plugin/vueapp/components/Playlists/PlaylistAddVideos.vue

  Line 71 in 6d25e34

  canEdit: {

• Unreachable state but at the moment it is fine as these functions lead to perm problems:
  

  studip-opencast-plugin/vueapp/components/Videos/VideosTable.vue

  Line 109 in 25addf7

  <span v-if="canEdit && !isCourse && !trashBin">

  
  

  studip-opencast-plugin/vueapp/components/Videos/VideosTable.vue

  Line 121 in 25addf7

  <span v-if="(canEdit || !isCourse) && trashBin">

• Don't show bulk action for students if student upload is allowed?:
  

  studip-opencast-plugin/vueapp/components/Videos/VideosTable.vue

  Lines 104 to 106 in 25addf7

  	<StudipButton v-if="canUpload" icon="remove" @click.prevent="removeVideosFromPlaylist" :disabled="!hasCheckedVideos">
  	{{ $gettext('Aus Wiedergabeliste entfernen') }}
  	</StudipButton>

  
  

  studip-opencast-plugin/vueapp/components/Videos/VideosTable.vue

  Line 312 in 25addf7

  return this.selectable || this.canEdit || this.canUpload;

  
  Should be write perm (done):
  

  studip-opencast-plugin/vueapp/components/Videos/VideoRow.vue

  Line 358 in 1506f41

  return this.selectable || (this.canUpload && this.event.perm == 'owner');

  
  or don't provide select all check box as it seems broken:
  

  studip-opencast-plugin/vueapp/components/Videos/VideosTable.vue

  Lines 35 to 41 in 25addf7

  	<th v-if="showCheckbox" data-sort="false">
  	<input
  	type="checkbox"
  	:checked="selectAll"
  	@click.stop="toggleAll"
  	:title="$gettext('Alle Videos auswählen')">
  	</th>

• Should only be write perm:
  

  studip-opencast-plugin/vueapp/components/Videos/VideoRow.vue

  Lines 593 to 595 in 1506f41

  	canShare() {
  	return this.event?.perm === 'owner' || this.currentUser.can_edit;
  	},

• Unused props:
  

  studip-opencast-plugin/vueapp/components/Videos/VideoRow.vue

  Lines 195 to 202 in 1506f41

  	canMoveUp: {
  	type: Boolean,
  	default: false
  	},
  	canMoveDown: {
  	type: Boolean,
  	default: false
  	},

• These menu items should require at least write perm:
  

  studip-opencast-plugin/vueapp/components/Videos/VideoRow.vue

  Line 440 in 1506f41

  if (this.event.perm == 'owner') {

[ferishili]

Don't show bulk action for students if student upload is allowed?

With my proposal, the bulk action is now showing to the students when the upload is allowed and if the student has any video with perm write or owner!
As the concept of canUpload was used, I decided to keep it! of course, we can discuss whether to provide the bulk without that right!

Question: Do we provide bulk action in Courseware video table? It is not there, atm!