22 July 2013, 5:06

As part of the MyCollections plugin I've been writing to try to be added into core, I've been informed that to add it to the Profile page, there'd need to be some access code to ensure you are only seeing the collections that you have access to view, or none at all.

For pages/views this is done as a function in the lib/view.php code and each block can reference the function and have that function do the access checking.

So, finally to my question. Would I be better to add the access checking into the plugin, so that I'm not impacting on other code or be better changing lib/collections.php (by adding a new function similar to that in view.php) so that it works in the same way as pages/views and others can then use the checking, if needed.

22 July 2013, 12:23

Hi Wullie,

It'd be better to add a new function to lib/collections.php, and submit that change as a separate patch before the rest of the changes.

I'm actually working on some core changes right now which will change the way Collection access control is handled, so having it centralized in the library functions will help ensure that I just have to update it in one place in order to avoid things breaking.

What function from lib/view.php are you thinking of basing it on? (From a messy implementation standpoint, based on how Collection access controls are currently implemented, what you'll need to do is find the first View of each Collection and determine whether the user has access to view that View.)

Cheers,

Aaron

22 July 2013, 22:32

Aaron,

At the moment, my plugin uses the get_mycollections_data function which has no access checking in it.

I was thinking of adding a new function which would be taken from the view_search function of view.php and adapted to find the first page (as you'd suggested) and finding the access controls of that view to filter through those access checks).

I'm thinking that I could call that function from the if/elseif statements in the get_mycollections_data function and passing that into the SQL under those if's, so then anything that calls that function should work just the same as now (without any changes having to happen elsewhere).

Does that sound along the same lines as you were thinking?

23 July 2013, 14:20

I actually wasn't aware of the View::view_search() function until now... I was imagining something using can_view_view() in lib/mahara.php. But can_view_view() isn't as well suited for getting a list of collections; it's more about checking whether you can view a particular page (and it takes into account secret URL cookies, which View::view_search() doesn't).

Now, having thought about it, I'd suggest the following plan. The access control logic is so complex, it's best to avoid replicating it as much as possible. So I think the best thing to do is use View::view_search(), and filter the list of Collections from the results of that. Perhaps adding yet another parameter to View::view_search() that indicates that you're searching for Collections, which then limits the select clause to c.id, c.name, and adds a "group by c.id, c.name". Additionally maybe create a Collection::collection_search() method which is a wrap-around to View::view_search(). Or alternately, Collection::collection_search() could just take the full list of results from View::view_search() and extracting the list of unique collections from it with a for loop.

Cheers,

Aaron