Problems with group artefact permissions due to misuse of $USER->can_view_artefact and $USER->can_edit_artefact

Bug #1262040 reported by Aaron Wells
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Mahara
Invalid
Low
Aaron Wells

Bug Description

While discussing the export to ZIP bug ( Bug 1013022 ) I realized that $USER->can_view_artefact() does not do what I thought it did. I thought that it was like can_view_view(), i.e. it was an easy way to tell whether a particular user is allowed to see the contents of a particular artefact.

But it does not mean that, as evidenced by the fact that it's not accessed at all on the artefact detail page, view/artefact.php. Instead, this function refers to whether or not the user should be able to see the artefact in their own or a group's Content area.

The reason it exists and has this name, is because of the group files permissions system (see http://manual.mahara.org/en/1.8/groups/inside_group.html#index-16 ). This defines three permission levels for a file: "View" lets you see the page in Contents and use it in Group pages, "Edit" lets you change the file's metadata, and "Publish" lets you use the file in your own Portfolio pages.

Anyway, I misunderstood it as doing the same thing as can_view_view(), which checks whether a particular user can see a particular Page in display-mode. The similar functionality for artefacts, as seen on view/artefact.php, is to provide an artefact ID & a page ID, and to check whether the artefact is in the page and the user can view the page.

Revision history for this message
Aaron Wells (u-aaronw) wrote :

Also, while implementing fixes to Bug 1211758 and Bug 1236636 (where we weren't validating artefact ownership before putting artefacts into blocks), we universally used $USER->can_edit_artefact() to check whether a user had the right to put an artefact into a block. We *should* have been using $USER->can_view_artefact() or $USER->can_publish_artefact() in nearly every one of these cases.

In fact, this has caused a notable regression. Currently, if I set a file to "view" permission only, then as a group member I see the file in the file picker, but receive an error when I try to select it.

This will also be tricky to implement, because knowing whether you need to use $USER->can_view_artefact() or $USER->can_publish_artefact() is dependent upon whether the Page is your own or a Group's.

Revision history for this message
Mahara Bot (dev-mahara) wrote : A change has been merged

Reviewed: https://reviews.mahara.org/2826
Committed: http://gitorious.org/mahara/mahara/commit/2f2d9c18fb16d3847af13f5df6ece85435104288
Submitter: Aaron Wells (<email address hidden>)
Branch: master

commit 2f2d9c18fb16d3847af13f5df6ece85435104288
Author: Aaron Wells <email address hidden>
Date: Wed Dec 18 16:28:13 2013 +1300

Add function descriptions to $USER->can_view_artefact() et al

Bug 1262040: Since it turns out these methods don't do exactly
what their name suggests, it's a good idea to at least put a comment
on there

Change-Id: I1fd8138d004768a534072c0485144bba3ebee5d1

Revision history for this message
Aaron Wells (u-aaronw) wrote : Re: I've been misusing $USER->can_view_artefact()

Okay, as a first step, I've added comments to the functions: https://reviews.mahara.org/#/c/2826/

Revision history for this message
Aaron Wells (u-aaronw) wrote :

Right, so the reason this is tagged to versions 1.6 - present is because of the regression described in this comment: https://bugs.launchpad.net/mahara/+bug/1262040/comments/1

no longer affects: mahara/1.6
no longer affects: mahara/1.7
no longer affects: mahara/1.8
Changed in mahara:
milestone: 1.9.0 → none
tags: added: groups permissions regression
summary: - I've been misusing $USER->can_view_artefact()
+ Problems with group artefact permissions due to misuse of
+ $USER->can_view_artefact and $USER->can_edit_artefact
Aaron Wells (u-aaronw)
no longer affects: mahara/1.6
Aaron Wells (u-aaronw)
no longer affects: mahara/1.7
tags: added: usermanualupdate
Revision history for this message
Aaron Wells (u-aaronw) wrote :

Changing from "Fix Committed" to "In Progress", because the one commit on this bug so far did not fix the problem. It was just a comments update.

tags: added: behat needs-behat
Revision history for this message
Jinelle Foley-Barnes (jinelleb) wrote :

Hi Aaron,

I know this is merged but do we need a Behat for this? If so could you please provide test instructions?

Thanks,
Jinelle

Aaron Wells (u-aaronw)
no longer affects: mahara/1.8
Robert Lyon (robertl-9)
no longer affects: mahara/1.9
Revision history for this message
Robert Lyon (robertl-9) wrote :

The problem in comment #1 doesn't seem to be a problem in 16.04 as I can select an image file set to member view only by another member of the group.

Revision history for this message
Aaron Wells (u-aaronw) wrote :

Well, the problem I had originally reported does seem to be cleared up. Specifically, if you put a file into a group, change its permissions for group members to "view" or "edit", and then log in as a group member, and try to put that file into one of your personal blocks, you will now find that you cannot do that through the UI. The Javascript prevents you from selecting it, and there's a "You cannot republish this" message if you hover over it.

On the other hand, I can still add the image to my block by manipulating the underlying HTTP request with Burp Proxy. The validation method that is supposed to check for permissions (BlockInstance->verify_attachment_permissions($id)) was updated in patch 9a2519b87f27 for Bug 1224750 (accessibility of public site files) so that it checks against can_view_artefact() rather than can_publish_artefact().

But... this problem is mitigated by the fact that even once I add the image to my block, it won't be displayed. There's verification on the display side as well, so the image is not shown and instead there's a warning message that says I don't have permission to republish that image.

So, still a little broken. But less so. So I'm going to lower this one to "low" priority unless we get some specific reports of bigger problems caused by it.

Robert Lyon (robertl-9)
no longer affects: mahara/1.10
no longer affects: mahara/15.04
no longer affects: mahara/15.10
no longer affects: mahara/16.04
Robert Lyon (robertl-9)
Changed in mahara:
milestone: 16.10.1 → 17.04.0
no longer affects: mahara/16.10
Changed in mahara:
milestone: 17.04.0 → none
milestone: none → 17.10.0
Revision history for this message
Robert Lyon (robertl-9) wrote :

This doesn't seem to be the case now - a group member does not see the 'select' tick when trying to add a group image to their personal page.

I set up a group with 2 images, one with full permission and one with view only permission for group members. both images could be added by a member to a group page but only the one with full perms could be added to their personal page

Changed in mahara:
status: Confirmed → Invalid
milestone: 17.10.0 → none
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.