Commit e5764a51 authored by Robert Lyon's avatar Robert Lyon Committed by Aaron Wells

Bug 1211758 - Make sure user has permission to publish artefacts

Change-Id: I262b2460195c663fd461964b9bd5a60efd8b297a
Signed-off-by: Robert Lyon's avatarRobert Lyon <robertl@catalyst.net.nz>
parent 1fb737b9
...@@ -301,6 +301,13 @@ function editpost_submit(Pieform $form, $values) { ...@@ -301,6 +301,13 @@ function editpost_submit(Pieform $form, $values) {
$old = $postobj->attachment_id_list(); $old = $postobj->attachment_id_list();
// $new = is_array($values['filebrowser']['selected']) ? $values['filebrowser']['selected'] : array(); // $new = is_array($values['filebrowser']['selected']) ? $values['filebrowser']['selected'] : array();
$new = is_array($values['filebrowser']) ? $values['filebrowser'] : array(); $new = is_array($values['filebrowser']) ? $values['filebrowser'] : array();
// only allow the attaching of files that exist and are editable by user
foreach ($new as $key => $fileid) {
$file = artefact_instance_from_id($fileid);
if (!($file instanceof ArtefactTypeFile) || !$USER->can_publish_artefact($file)) {
unset($new[$key]);
}
}
if (!empty($new) || !empty($old)) { if (!empty($new) || !empty($old)) {
foreach ($old as $o) { foreach ($old as $o) {
if (!in_array($o, $new)) { if (!in_array($o, $new)) {
......
...@@ -579,6 +579,21 @@ class PluginBlocktypeGallery extends PluginBlocktype { ...@@ -579,6 +579,21 @@ class PluginBlocktypeGallery extends PluginBlocktype {
); );
} }
public static function instance_config_validate($form, $values) {
global $USER;
if (!empty($values['images'])) {
foreach ($values['images'] as $id) {
$image = new ArtefactTypeImage($id);
if (!($image instanceof ArtefactTypeImage) || !$USER->can_publish_artefact($image)) {
$result['message'] = get_string('unrecoverableerror', 'error');
$form->set_error(null, $result['message']);
$form->reply(PIEFORM_ERR, $result);
}
}
}
}
public static function instance_config_save($values) { public static function instance_config_save($values) {
if ($values['select'] == 0) { if ($values['select'] == 0) {
$values['artefactid'] = $values['folder']; $values['artefactid'] = $values['folder'];
......
...@@ -97,6 +97,16 @@ function pieform_element_filebrowser(Pieform $form, $element) { ...@@ -97,6 +97,16 @@ function pieform_element_filebrowser(Pieform $form, $element) {
else { else {
$value = null; $value = null;
} }
// check to see if attached artefact items in $value array are actually allowed
// to be seen by this user
if (!empty($value)) {
foreach ($value as $k => $v) {
$file = artefact_instance_from_id($v);
if (!($file instanceof ArtefactTypeFile) || !$USER->can_publish_artefact($file)) {
unset($value[$k]);
}
}
}
$selected = $element['selectlistcallback']($value); $selected = $element['selectlistcallback']($value);
} }
$smarty->assign('selectedlist', $selected); $smarty->assign('selectedlist', $selected);
......
...@@ -477,7 +477,13 @@ abstract class ArtefactTypeFileBase extends ArtefactType { ...@@ -477,7 +477,13 @@ abstract class ArtefactTypeFileBase extends ArtefactType {
$item->size = ArtefactTypeFile::short_size($item->size, true); $item->size = ArtefactTypeFile::short_size($item->size, true);
} }
if ($group) { if ($group) {
if ($item->author == $USER->get('id')) { // site public files
if ($institution == 'mahara' && ArtefactTypeFolder::admin_public_folder_id() == $parentfolderid) {
$item->can_edit = 0;
$item->can_view = 1;
$item->can_republish = 1;
}
else if (!empty($item->author) && $item->author == $USER->get('id')) {
$item->can_edit = 1; $item->can_edit = 1;
$item->can_view = 1; $item->can_view = 1;
$item->can_republish = 1; $item->can_republish = 1;
...@@ -488,8 +494,10 @@ abstract class ArtefactTypeFileBase extends ArtefactType { ...@@ -488,8 +494,10 @@ abstract class ArtefactTypeFileBase extends ArtefactType {
$item->can_republish = $can_view_parent && $item->can_republish; $item->can_republish = $can_view_parent && $item->can_republish;
} }
} }
if ($group && $item->author == $USER->get('id')) { if (!empty($item->author)) {
$item->can_edit = 1; // This will show the delete, edit buttons in filelist, but doesn't change the actual permissions in the checkbox if ($group && $item->author == $USER->get('id')) {
$item->can_edit = 1; // This will show the delete, edit buttons in filelist, but doesn't change the actual permissions in the checkbox
}
} }
} }
$where = 'artefact IN (' . join(',', array_keys($filedata)) . ')'; $where = 'artefact IN (' . join(',', array_keys($filedata)) . ')';
......
...@@ -944,6 +944,8 @@ class User { ...@@ -944,6 +944,8 @@ class User {
} }
public function can_view_artefact($a) { public function can_view_artefact($a) {
global $USER;
$parent = $a->get_parent_instance(); $parent = $a->get_parent_instance();
if ($parent) { if ($parent) {
if (!$this->can_view_artefact($parent)) { if (!$this->can_view_artefact($parent)) {
...@@ -955,6 +957,15 @@ class User { ...@@ -955,6 +957,15 @@ class User {
|| ($a->get('institution') and $this->is_institutional_admin($a->get('institution')))) { || ($a->get('institution') and $this->is_institutional_admin($a->get('institution')))) {
return true; return true;
} }
// public site files
else if ($a->get('institution') == 'mahara') {
$thisparent = $a->get('parent');
// if we are looking at the public folder or items in it
if (($a->get('id') == ArtefactTypeFolder::admin_public_folder_id())
|| (!empty($thisparent) && $thisparent == ArtefactTypeFolder::admin_public_folder_id())) {
return true;
}
}
if ($a->get('group')) { if ($a->get('group')) {
// Only group artefacts can have artefact_access_role & artefact_access_usr records // Only group artefacts can have artefact_access_role & artefact_access_usr records
return (bool) count_records_sql("SELECT COUNT(*) FROM {artefact_access_role} ar return (bool) count_records_sql("SELECT COUNT(*) FROM {artefact_access_role} ar
......
...@@ -508,8 +508,31 @@ class BlockInstance { ...@@ -508,8 +508,31 @@ class BlockInstance {
throw new ParamOutOfRangeException("Field $field wasn't found in class " . get_class($this)); throw new ParamOutOfRangeException("Field $field wasn't found in class " . get_class($this));
} }
// returns false if it finds a bad attachment
// returns true if all attachments are allowed
private function verify_attachment_permissions($id) {
global $USER;
if (is_array($id)) {
foreach ($id as $id) {
$file = artefact_instance_from_id($id);
if (!$USER->can_publish_artefact($file)) {
// bail out now as at least one attachment is bad
return false;
}
}
}
else {
$file = artefact_instance_from_id($id);
if (!$USER->can_publish_artefact($file)) {
return false;
}
}
return true;
}
public function instance_config_store(Pieform $form, $values) { public function instance_config_store(Pieform $form, $values) {
global $SESSION; global $SESSION, $USER;
// Destroy form values we don't care about // Destroy form values we don't care about
unset($values['sesskey']); unset($values['sesskey']);
...@@ -520,6 +543,22 @@ class BlockInstance { ...@@ -520,6 +543,22 @@ class BlockInstance {
unset($values['change']); unset($values['change']);
unset($values['new']); unset($values['new']);
// make sure that user is allowed to publish artefact. This is to stop
// hacking of form value to attach other users private data.
$badattachment = false;
if (!empty($values['artefactid'])) {
$badattachment = !$this->verify_attachment_permissions($values['artefactid']);
}
if (!empty($values['artefactids'])) {
$badattachment = !$this->verify_attachment_permissions($values['artefactids']);
}
if ($badattachment) {
$result['message'] = get_string('unrecoverableerror', 'error');
$form->set_error(null, $result['message']);
$form->reply(PIEFORM_ERR, $result);
exit();
}
$redirect = '/view/blocks.php?id=' . $this->get('view'); $redirect = '/view/blocks.php?id=' . $this->get('view');
if (param_boolean('new', false)) { if (param_boolean('new', false)) {
$redirect .= '&new=1'; $redirect .= '&new=1';
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment