Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add plugin hook to and improve get_access_sql_suffix() (Trac #3047) #3047

Closed
elgg-gitbot opened this issue Feb 16, 2013 · 21 comments
Closed

Add plugin hook to and improve get_access_sql_suffix() (Trac #3047) #3047

elgg-gitbot opened this issue Feb 16, 2013 · 21 comments
Assignees
Labels

Comments

@elgg-gitbot
Copy link

@elgg-gitbot elgg-gitbot commented Feb 16, 2013

Original ticket http://trac.elgg.org/ticket/3047 on 41182017-04-19 by brettp, assigned to unknown.

Elgg version: 1.7

get_access_sql_suffix() is the primary way to decide if entities are available to the user. This function is used (and abused) by metadata, annotations, and river functions. I've attached a patch that cleans up the SQL generated, allows you to customize the column names, and emits a plugin hook so plugins can add to or remove restraints. In addition to removing the horrible hacks using str_replace() for river and metadata functions, I believe this is enough to implement a decent roles plugin using only plugin hooks.

@elgg-gitbot
Copy link
Author

@elgg-gitbot elgg-gitbot commented Feb 16, 2013

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

Loading

@elgg-gitbot
Copy link
Author

@elgg-gitbot elgg-gitbot commented Feb 16, 2013

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...

Loading

@elgg-gitbot
Copy link
Author

@elgg-gitbot elgg-gitbot commented Feb 16, 2013

cash wrote on 41491006-06-05

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

Loading

@elgg-gitbot
Copy link
Author

@elgg-gitbot elgg-gitbot commented Feb 16, 2013

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.

Loading

@elgg-gitbot
Copy link
Author

@elgg-gitbot elgg-gitbot commented Feb 16, 2013

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.

Loading

@elgg-gitbot
Copy link
Author

@elgg-gitbot elgg-gitbot commented Feb 16, 2013

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.

Loading

@elgg-gitbot
Copy link
Author

@elgg-gitbot elgg-gitbot commented Feb 16, 2013

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

Loading

@elgg-gitbot
Copy link
Author

@elgg-gitbot elgg-gitbot commented Feb 16, 2013

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.

Loading

@elgg-gitbot
Copy link
Author

@elgg-gitbot elgg-gitbot commented Feb 16, 2013

trac user jtilson wrote on 42216838-10-14

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

Loading

@elgg-gitbot
Copy link
Author

@elgg-gitbot elgg-gitbot commented Feb 16, 2013

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...?

Loading

@elgg-gitbot
Copy link
Author

@elgg-gitbot elgg-gitbot commented Feb 16, 2013

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

Loading

@jrtilson
Copy link
Contributor

@jrtilson jrtilson commented May 14, 2013

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..

Loading

@beck24
Copy link
Member

@beck24 beck24 commented May 14, 2013

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

Loading

@cash
Copy link
Contributor

@cash cash commented May 14, 2013

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).

Loading

@jrtilson
Copy link
Contributor

@jrtilson jrtilson commented May 16, 2013

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.

Loading

@cash
Copy link
Contributor

@cash cash commented May 16, 2013

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?

Loading

@jrtilson
Copy link
Contributor

@jrtilson jrtilson commented May 16, 2013

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.

Loading

jrtilson added a commit to THINKGlobalSchool/Elgg that referenced this issue May 16, 2013
@cash
Copy link
Contributor

@cash cash commented May 16, 2013

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().

Loading

@jrtilson
Copy link
Contributor

@jrtilson jrtilson commented May 16, 2013

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);

Loading

@Srokap
Copy link
Contributor

@Srokap Srokap commented May 16, 2013

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.

Loading

jrtilson added a commit to THINKGlobalSchool/Elgg that referenced this issue May 17, 2013
Removed get_access_restriction_sql.
Added a basic unit test for the new plugin hook. Elgg#3047
jrtilson added a commit to THINKGlobalSchool/Elgg that referenced this issue May 21, 2013
jrtilson added a commit to THINKGlobalSchool/Elgg that referenced this issue May 22, 2013
jrtilson added a commit to THINKGlobalSchool/Elgg that referenced this issue May 22, 2013
cash added a commit to cash/Elgg that referenced this issue Jul 1, 2013
cash added a commit to cash/Elgg that referenced this issue Jul 1, 2013
Removed get_access_restriction_sql.
Added a basic unit test for the new plugin hook. Elgg#3047
cash added a commit to cash/Elgg that referenced this issue Jul 1, 2013
cash added a commit to cash/Elgg that referenced this issue Jul 1, 2013
@ghost ghost assigned cash Jul 4, 2013
@cash
Copy link
Contributor

@cash cash commented Jul 14, 2013

PR has been merged into master

Loading

@cash cash closed this Jul 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants