Inserts into node_access when OG access control disabled
| Project: | Organic groups |
| Version: | HEAD |
| Component: | og.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | moshe weitzman |
| Status: | closed |
Although if OG access control is disabled, there is still 1 insert query into the node_access table per node. This doesn't seem to be necessary (?).
I just verified with a current vanilla Drupal 5 installation and the latest OG version. Even if og access control was never on, the node_access table is being populated:
nid gid realm grant_view grant_update grant_delete
0 0 all 1 0 0
1 0 all 1 0 0
2 0 all 1 0 0OG seems to be doing what's necessary - it returns nothing on og_node_access_records() when og_enabled == FALSE.
This might be a Drupal problem, as node_access_acquire_grants() creates the following array $grants if no implementing module returns a value:
$grants[] = array('realm' => 'all', 'gid' => 0, 'grant_view' => 1, 'grant_update' => 0, 'grant_delete' => 0);
But further down node_access_acquire_grants() calls
node_access_write_grants($node, $grants);
which only skips entries to the node_access table, if:
if (count(module_implements('node_grants'))) {
foreach ($grants as $grant) {
if ($realm && $realm != $grant['realm']) {
continue;
}
drupal_set_message('heck! '. dprint_r($grant, true));
// Only write grants; denies are implicit.
if ($grant['grant_view'] || $grant['grant_update'] || $grant['grant_delete']) {
db_query("INSERT INTO {node_access} (nid, realm, gid, grant_view, grant_update, grant_delete) VALUES (%d, '%s', %d, %d, %d, %d)", $node->nid, $grant['realm'], $grant['gid'], $grant['grant_view'], $grant['grant_update'], $grant['grant_delete']);
}
}
}$realm is the third optional parameter of node_access_write_grants() but is being omitted in this call - is this the cause of the problem?
Or does OG need to return sth different on og_node_access_records()?
Or did I hit a bigger issue here that I am not aware of?
Alex
#1
i think i see what is going on. og is considered a node access module even if its node access is disabled (because it implements hook_node_grants()). as such, node_access_write_grants() feels compelled to write a grant for each node because it thinks that if it does not, then none will and acess will be denied.
there are a couple solutions, but i think this is a minor issue. as long as you have the all record with nid=0, then the node access system is not in effect and is not slowing down your site even a little.
#2
One solution that I'd support wholeheartedly would be pulling OG's access control out into a separate module that simply does exactly what the current OG access control does now.
#3
I considered that too, but decided that the majority of og users have access control enabled so this would be confusing and burdensome on them. the cure would be worse than the illness.
#4
Moshe,
I hit this issue when I looked at the cron query log of aggregation sites we're running. While the extra query isn't much tax on a single node creation it really adds up when creating 100s of nodes on cron - I would be glad if we could somehow address this issue.
I like the idea of the add on module that would kick in when activated - it could even save the "enable" button on the admin/og/og page.
Alex
#5
Since OG access control defaults to "disabled" upon installation, if we leave the "enable" button on admin/og/og like Alex suggests, there will be no perceived change to the end user ... as long as we can programmatically enable and disable the og_access_control (or whatever it will be called) without the user having to manually enable the module.
If the newly separated access control code is delivered along with the base OG code, the end user shouldn't notice a difference.
#6
While not having suggested to leave the "enable" button on its place, I think this is a great call: Why not include a og_node_access.inc file containing node access functions depending on the user setting?
Is there a boot process problem with this suggestion? Would the Drupal variable available at the required time?
#7
i think the conditional include will work.
#8
I will start working on this.
#9
This is a first version of a fix.
It moves og_grants() og_node_access_records() and og_node_access_explain() to a new og_node_access.inc file that is only included when necessary (see og_init() and og_settings_submit()).
I tested it on a vanilla Drupal install by creating group nodes and nodes in groups first without og access control (og_enabled == FALSE) and then turned og access control on. Conversion to access control works fine, when i convert back to no access control, however, the standard entry with nid == 0 is not set in the node_access table. I guss that this how og / na behaved all along and I wonder whether this matters at all (?).
Did I miss any functions that could be moved to og_node_access.inc ?
Is there a function in og_node_access.inc that shouldn't be there?
I rolled this patch against DRUPAL-5. I am currently testing this on my development box. I will push this out to staging within the next two days and keep you posted about results.
#10
the replacement of that universal grant happens in og_disable(). i guess it can stay there for now.
looks to me like you moved the right functions.
instead of
// $Id: $, I always do// $Id$#11
Having not tested this, it looks like upon og_settings_submit(), node_access_rebuild() is still seeing that someone (og) implements hook_node_grants() because og_node_access.inc gets included.
All the hard work in identifying the node access code that should be stripped out of OG has already been done -- it seems like making it a full-blown sub-module of OG would be the way to go.
The fact that og_node_access.inc needs to be included in multiple places is another possible warning that this would be cleaner as a small separate module.
#12
I mused about this and decided that a separate module is OK and cleaner. Also note that private groups should not even be mentioned if access control is disabled. We get this wrong today.
Help wanted.
#13
I like the seperate module approach. I will give this a shot.
#14
This is a first version
+ all node access functionality lives in og_access.module
+ enabling/disabling the module is the same as pushing the enable/disable button before
I tested:
- enabling/disabling/uninstalling the module
- posting private/public content
Access control functionality seems to work fine.
There are two issues:
- When installing, we can't rebuild the node access table in install() because the og_access module's hooks are not present. I shifted the node_access_rebuild() call to og_access_init() and added a toggle. This is not ideal. Any ideas?
- When deactivating the module, the node access table doesn't get rebuilt which leaves the site in limbo. Only when uninstalling the module, the node access module gets rebuilt. Here I am also looking for ideas on how to rebuild the node access table without needing to uninstall the module.
#15
there is a flag that you can set in D6 'needs rebuild on next request' which solves these issues. so lets work around as best we can for d5 and use the flag for d6 ... i will give into this a bit more soon.
#16
Here is the hack thats required for D5. See hook_disable() and hook_enable(). Se http://api.drupal.org/api/file/developer/examples/node_access_example.mo...
#17
We should add an update function to og_install which writes a 1 for og_access.module in system table if access control is enabled. We don't need or want a node_access_rebuild() in this case.
#18
Thanks for pointers. Will apply.
#19
I have enhanced this patch a lot and committed it. It isn't quite finished yet, so please wait until my next post before trying it out.
TODO
----------
- test module enable/disable/uninstall. og.module update function: change bootstrap column as needed.
- node create/edit as a non-admin
#20
I polished this some more and committed to HEAD and DRUPAL-5. Please test this and report back on your findings. I'd like to release this soon.
#21
Tested on 5.4:
1) installed og module
2) Created 2 groups
3) created a story in each of them with different permission roles
4) installed og_access module
5) created private (non public) stories
6) success - private stories not accesible for non group members
7) disabled og_access module - all nodes accessible
8) enabled og_access module - works as expected - private stories from before are not accessible any more
One issue I found was that public nodes have 2 entries in node_access table: http://skitch.com/alexbarth/jid1/localhost-localhost-devseed-og-node-acc...
is this how it should be?
Alex
#22
Ran tests against 5.x version of module (with patches from here: http://drupal.org/node/247485). There were no fails related to this patch.
#23
thanks for testing, alex_b. Yes, those 2 rows are expected even for public posts. I will investigate at some point to see if we can get by with one. It would simplify the node access query.
i'm about to roll a release candidate ...
#24
Automatically closed -- issue fixed for two weeks with no activity.