commit 32c3b6a411c3df72f7b76248b2180d7738d0a386 Author: Hugh Davenport Date: Wed Aug 15 12:07:58 2012 +1200 Fix permissions of group area (Bug #1034180) A user should not be able to view/publish an artefact if - they don't have view/publish permission of that artefact - they don't have view permission of all parents of that artefact A user should not be able to edit an artefact if - they don't have edit permission of that artefact - they don't have edit permission of the immediate parent of that artefact - they don't have view permission of any parents below the immediate This is similar to the UNIX permissions, you shouldn't be able to view a directory unless all directories below have read (r) and executeable (x) bits set. The same for editing, you need write (w) permissions of the immediate parent, and rx for all parents. In Mahara, there are no executeable bits, but it can be assumed that view is basically the same as rw for container artefacts, and the same as r for non container artefacts. Change-Id: I4f84aca05dd08d02b05fbe084e4724f78c8681a0 Signed-off-by: Hugh Davenport Conflicts: htdocs/artefact/file/lib.php htdocs/artefact/lib.php htdocs/auth/user.php diff --git a/htdocs/artefact/file/form/elements/filebrowser.php b/htdocs/artefact/file/form/elements/filebrowser.php index 9b08dd0..393871c 100644 --- a/htdocs/artefact/file/form/elements/filebrowser.php +++ b/htdocs/artefact/file/form/elements/filebrowser.php @@ -63,6 +63,9 @@ function pieform_element_filebrowser(Pieform $form, $element) { } $folder = $element['folder']; + if ($group && !pieform_element_filebrowser_view_group_folder($group, $folder)) { + $folder = null; + } $path = pieform_element_filebrowser_get_path($folder); $smarty->assign('folder', $folder); $smarty->assign('foldername', $path[0]->title); @@ -1031,6 +1034,24 @@ function pieform_element_filebrowser_edit_group_folder($group, $folder) { } +function pieform_element_filebrowser_view_group_folder($group, $folder) { + global $USER; + if ($folder) { + if (!$folder instanceof ArtefactTypeFolder) { + $folder = new ArtefactTypeFolder($folder); + } + return $USER->can_view_artefact($folder); + } + require_once(get_config('libroot') . 'group.php'); + // Group root directory: use default grouptype artefact permissions + if (!$role = group_user_access($group)) { + return false; + } + $permissions = group_get_default_artefact_permissions($group); + return $permissions[$role]->view; +} + + function pieform_element_filebrowser_changeowner(Pieform $form, $element) { $newtabdata = pieform_element_filebrowser_configure_tabs($element['tabs']); $smarty = smarty_core(); @@ -1100,6 +1121,12 @@ function pieform_element_filebrowser_changefolder(Pieform $form, $element, $fold // If changing to a group folder, check whether the user can edit it if ($g = ($owner ? $group : $form->get_property('group'))) { + if (!pieform_element_filebrowser_view_group_folder($g, $folder)) { + return array( + 'error' => true, + 'message' => get_string('cannotviewfolder', 'artefact.file'), + ); + } $editgroupfolder = pieform_element_filebrowser_edit_group_folder($g, $folder); } diff --git a/htdocs/artefact/file/lang/en.utf8/artefact.file.php b/htdocs/artefact/file/lang/en.utf8/artefact.file.php index 8c07237..936ff6b 100644 --- a/htdocs/artefact/file/lang/en.utf8/artefact.file.php +++ b/htdocs/artefact/file/lang/en.utf8/artefact.file.php @@ -33,6 +33,7 @@ $string['sitefilesloaded'] = 'Site files loaded'; $string['addafile'] = 'Add a file'; $string['archive'] = 'Archive'; $string['bytes'] = 'bytes'; +$string['cannotviewfolder'] = 'You do not have permission to view the content of this folder'; $string['cannoteditfolder'] = 'You do not have permission to add content to this folder'; $string['cannoteditfoldersubmitted'] = 'You cannot add content to a folder in a submitted page.'; $string['cannotremovefromsubmittedfolder'] = 'You cannot remove content from a folder in a submitted page.'; diff --git a/htdocs/artefact/file/lib.php b/htdocs/artefact/file/lib.php index 0ba76f2..01f053a 100644 --- a/htdocs/artefact/file/lib.php +++ b/htdocs/artefact/file/lib.php @@ -388,10 +388,18 @@ abstract class ArtefactTypeFileBase extends ArtefactType { $where .= ' AND a.parent = ? '; $phvals[] = $parentfolderid; + $parent = artefact_instance_from_id($parentfolderid); + $can_view_parent = $USER->can_view_artefact($parent); + if (!$can_view_parent) { + return null; + } + $can_edit_parent = $USER->can_edit_artefact($parent); } else { $where .= ' AND a.parent IS NULL'; + $can_edit_parent = true; + $can_view_parent = true; } $filedata = get_records_sql_assoc($select . $from . $where . $groupby, $phvals); @@ -407,6 +415,11 @@ abstract class ArtefactTypeFileBase extends ArtefactType { if ($item->size) { // Doing this here now for non-js users $item->size = ArtefactTypeFile::short_size($item->size, true); } + if ($group) { + $item->can_edit = $can_edit_parent && $item->can_edit; + $item->can_view = $can_view_parent && $item->can_view; + $item->can_republish = $can_view_parent && $item->can_republish; + } } $where = 'artefact IN (' . join(',', array_keys($filedata)) . ')'; $tags = get_records_select_array('artefact_tag', $where); diff --git a/htdocs/auth/user.php b/htdocs/auth/user.php index 92198ee..1ca5f39 100644 --- a/htdocs/auth/user.php +++ b/htdocs/auth/user.php @@ -824,6 +824,12 @@ class User { } public function can_view_artefact($a) { + $parent = $a->get_parent_instance(); + if ($parent) { + if (!$this->can_view_artefact($parent)) { + return false; + } + } if ($this->get('admin') || ($this->get('id') and $this->get('id') == $a->get('owner')) || ($a->get('institution') and $this->is_institutional_admin($a->get('institution')))) { @@ -839,7 +845,20 @@ class User { return false; } - public function can_edit_artefact($a) { + public function can_edit_artefact($a, $viewparent=false) { + $parent = $a->get_parent_instance(); + if ($parent) { + if ($viewparent) { + if (!$this->can_view_artefact($parent)) { + return false; + } + } + else { + if (!$this->can_edit_artefact($parent, true)) { + return false; + } + } + } if ($this->get('admin') || ($this->get('id') and $this->get('id') == $a->get('owner')) || ($a->get('institution') and $this->is_institutional_admin($a->get('institution')))) { @@ -862,6 +881,12 @@ class User { } public function can_publish_artefact($a) { + $parent = $a->get_parent_instance(); + if ($parent) { + if (!$this->can_view_artefact($parent)) { + return false; + } + } if (($this->get('id') and $this->get('id') == $a->get('owner')) || ($a->get('institution') and $this->is_institutional_admin($a->get('institution')))) { return true;