Attachment added by brettp on 41183080-09-28: 3047_access_plugin_hooks.patch

cash wrote on 41275181-11-04

Copying comments from email...

Cash:

It looks like a plugin author gets to insert where subclauses to the 
access clause. A typical clause could be something like "return entities 
that are public OR are at friends access level where user is a friend 
OR is private AND user is owner". A plugin could add an OR clause: "OR
user has relationship group_admin for container guid".

Am I understanding this correctly?

Brett:

The important part is get_access_sql_suffix().  This code is very old 
and hasn't been touched in a long time.  Its purpose is to add the 
access_id and enabled = 'yes' where clauses to various getter functions 
for entities, metadata, annotations, and the river.  Because the schema 
among those tables are slightly different, there were hacks that 
str_replaced()'d out bits of what get_access_sql_suffix() returned.  
The new function doesn't require those.

The plugin I'm writing allows you to set an entity to private and then 
share explicitly with other users.  It has a hook that returns this:

  $value['ors'][] = "{$table_prefix}{$guid_column} IN (
        SELECT guid_one FROM {$db_prefix}entity_relationships
        WHERE relationship='shared' AND guid_two=$owner_guid
        )";

This solves the "I need to override the access_id restrictions" 
that arise when writing a roles plugin.

There are a few problems I haven't solved with this implementation 
yet for non-entity tables:
 * Metastrings access is all or nothing based upon the entity, 
not the metadata/annotation.  There's not a slick way to check 
if access for a bit of metadata should be overridden.
 * Advanced queries would need to know if they're looking at 
an entity, metastring, or river table.

My goal for this is to provide enough extensibility to allow a 
roles plugin without rewriting the db model and without a huge 
performance hit...

cash wrote on 41491006-06-05

• Any uses of this patch in production?
• Do we want to pull it in for 1.8.0?

ewinslow wrote on 41494820-09-25

Definitely a fan of this idea. Seems like this almost obviates the need for the access collections table, actually. Everything can be tracked by relationships, plugin hooks, and magic access_id values. 1.8.0 would be nice, but we're not in that much of a rush I don't think.

cash wrote on 41496125-07-09

My preference is to stay away from adding features in the 1.8.x releases so are we okay with assigning this to 1.9. I think the goal should be to not take a year to release 1.9.

brettp wrote on 41496489-12-20

Agreed about not adding features in bugfix releases.

Setting the milestone at 1.9 because we have our hands full with 1.8 features already, and there are still some bugs to this approach.

Milestone changed to Elgg 1.9 by brettp on 41496489-12-20

ewinslow wrote on 41497287-04-28

Most definitely. Plus, 1.9 is supposedly dealing with plugin hook, so that's an appropriate time to revisit.

Also, thank you for saying that 1.9 shouldn't take a year... would like to discuss this in more detail before 1.9 dev starts.

trac user jtilson wrote on 42216838-10-14

+1 for this idea.. I've been wrestling with a problem that this will solve 100%.

brettp wrote on 42337292-02-24

This was set for 1.9, but somehow ended up in Near Term Future release without anyone changing it...?

Milestone changed to Elgg 1.9.0 by brettp on 42337292-02-24

Is there a plan for implementation of this? The original patch that Brett submitted is almost 2 years old now, so it's probably not a good starting point. I'm happy to help out with this any way I can..

Why have I never seen this ticket? Absolutely this should happen. I was just talking with Steve about this yesterday.

I don't think the access system has changed much at all in the past two years. To start on this, integrate the patch into a branch and write unit tests (should be able to use phpunit tests - see the viewtype unit tests for an example).

I've got that patch implemented on a branch from master (the easy part). Looks like the calls to get_access_sql_suffix() have been greatly reduced since the 1.7.x days...

Taking a look at the existing phpunit tests, they're all pretty minimal and they only require one or two libs. I'm thinking I'd probably need to load the whole engine to properly test any functionality in the access system.. (I'd need users, groups, access collections, etc. etc.).

I'm fine using phpunit going forward, but I'd appreciate any advice on what parts/how much of the Elgg engine 'should' be loaded in a unit test.

Also taking a good look at the old SimpleTest unit tests, I'm not seeing any coverage of the existing access system.. (besides creating/deleting/editing access collections). Obviously this concerns me now that I've made some pretty low level changes.

Re: PHPUnit vs SimpleTest. We're trying to break the complicated dependencies that exist in Elgg and writing PHPUnit tests has been a forcing function for us. If you start writing a PHPUnit test and find that there are a lot of dependencies, then it needs to be a SimpleTest. Right now, we don't support anything touch the database from PHPUnit tests.

I took a look at get_access_sql_suffix() and it calls:

• sanitise_string
• elgg_get_logged_in_user_guid
• elgg_check_access_overrides
• get_access_list
• get_access_restriction_sql

We can dump get_access_restriction_sql() by removing the enemy code. It's never been tested and looks like it was thrown in by the previous devs. Once we have the plugin hook, we can add it through a plugin for backward compatibility.

sanitise_string() is now a wrapper for a method on Elgg_Database so we should be fine there. Same with elgg_get_logged_in_user_guid() (uses ElggSession which can be mocked). elgg_check_access_overrides() looks okay.

get_access_list() has two function calls of its own:

• _elgg_get_access_cache()
• get_access_array()

_elgg_get_access_cache() looks ok.

get_access_array() is the hard part because it talks to the database. I suppose we could mock that since it is only two database calls. This is all the time I have right now - will look at this later. Do you have a public branch for this?

Thanks Cash, I'll commit the changes to the function sans-unit tests so you can take a look.

Maybe I'm thinking a little high level for the tests? I imagine I'd go about it by creating a couple users, some access collections/groups, and couple entities with various access_id's and if they can see the entity with/without the hook.

Those are more integration tests and what we have done with SimpleTest is a better fit. For the PHPUnit tests we've been focusing on single functions and classes. I'll try to throw together an example for get_access_array().

Integration tests seem that way to go in this case, no? (On top of some phpunit tests)

I think what I'm failing to understand is how I could simulate a logged in user to test anything that calls elgg_get_logged_in_user_guid(). You mentioned in your previous comment that the session can be mocked. I see some phpunit session tests for the session, but there's nothing there to do with logging in a user.

The only thing I've come up with for that function without having a user, is testing with/without elgg_set_ignore_access, ie:

$this->assertFalse(strpos(get_access_sql_suffix(), "1 = 1") !== false);
elgg_set_ignore_access(true);
$this->assertTrue(strpos(get_access_sql_suffix(), "1 = 1") !== false);

I've been working recently on reusing existing simpletests #5456 and in the end on Travis integration https://github.com/Srokap/Elgg/tree/ticket_5167 #5167

Having code testable without side effects is much better, so it's definitely prefferred, but I imagine that when having installed Elgg on Travis, we could implement complex test scenarios in PHPUnit as well as simpletests.

PR has been merged into master