| Project: | ACL |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
hi,
this seems to be a problem: if a particular acl grant should apply to a node but that acl id doesn't (yet) have any users associated with it, then the grant won't apply to that node when users are later added to the acl. for example, i create an acl_id "community_members" to allow community members to view a certain node. but, nobody is a community member yet. when the node is saved and grants written, node_access_records will not return the relevant grant and so will not allow a user who later becomes a community member to view the node. the problem is the "else deny access" clause below, which i've commented out. what i am not clear on is why this clause was motivated in the first place. is it really necessary?
of course, you can get around the problem by saving the node later after users are added to the acl id, but that's not really a solution because adding users to the acl "community members" is not connected to individual nodes, and so we don't know which nodes to save after a user's status has changed.
ed
p.s. note some discussion of this issue already at, for example, http://drupal.org/node/169991
/**
* Implementation of hook_node_access_records().
*/
function acl_node_access_records($node) {
if (!$node->nid) {
return;
}
$result = db_query("SELECT n.*, 'acl' AS realm, n.acl_id AS gid, a.module FROM {acl_node} n INNER JOIN {acl} a ON n.acl_id = a.acl_id WHERE nid = %d", $node->nid);
$grants = array();
while ($grant = db_fetch_array($result)) {
if (module_exists($grant['module']) && module_invoke($grant['module'], 'enabled')) {
//if (acl_has_users($grant['gid'])) {
$grants[] = $grant;
//}
/*else {
//just deny access
$grants[] = array(
'realm' => 'acl',
'gid' => 0,
'grant_view' => 0,
'grant_update' => 0,
'grant_delete' => 0,
'priority' => $grant['priority'],
);
}*/
}
}
return $grants;
}
Comments
#1
(Please use the <?php tag for listing code, so that indentation is preserved.)
I see the problem, but it's not clear what to do about it. ACL is an API module and as such it doesn't have the knowledge to decide how to handle this case:
ACL takes the third approach. The client module has a better chance to know what the intended behavior is.
For example, take Content Access: if the client module is going to save the node along with customized permissions for this node, then it would be a complete waste if ACL called node_access_acquire_grants() on its own, only to have it called again by CA updating the node.
If you comment out the "just deny access" code, then nodes will be visible when none of the installed node access modules is granting access (returning any grant).
#2
the question is why is the ACL module only interested in returning a grants array for a node if there are CURRENTLY users in that acl? this doesn't make sense to me. suppose that we equate a grant id with a site-wide role. if nobody has that role yet, then the grant id will just be ignored by ACL, which seems bizarre.
we already know that node_access_acquire_grants is called on node_save. no control over that. so if the grants for a node change (for example a node is now made accessible to those with role z), then why would we want that change to be totally ignored? why is it relevant whether an acl has any users or not, as long as that acl applies to the node?
#3
following up, why not instead something like:
<?php/**
* Implementation of hook_node_access_records().
*/
function acl_node_access_records($node) {
if (!$node->nid) {
return;
}
$result = db_query("SELECT n.*, 'acl' AS realm, n.acl_id AS gid, a.module FROM {acl_node} n INNER JOIN {acl} a ON n.acl_id = a.acl_id WHERE nid = %d", $node->nid);
$grants = array();
while ($grant = db_fetch_array($result)) {
if (module_exists($grant['module']) && module_invoke($grant['module'], 'enabled')) {
$grants[] = $grant;
}
}
if (count($grants) == 0) {
//just deny access
$grants[] = array(
'realm' => 'acl',
'gid' => 0,
'grant_view' => 0,
'grant_update' => 0,
'grant_delete' => 0,
'priority' => $grant['priority'],
);
}
return $grants;
}
?>
#4
This is definitely wrong: $grant['priority'] comes from the database.
But you may be on to something. Please post a corrected patch.
#5
subscribing
#6
Let's look at this again: I'm somewhat familiar with how two modules use ACL:
Forum Access uses it to implement forum moderators. Each forum has an {acl} entry. If you change the access of a forum, FA has to run acquire_node_access_grants($node) on each node in the forum. This calls acl_node_access_records($node) for each node, resulting in updated {node_access} records.
Content Access uses ACL to implement per-user access. Each node has its own {acl} entry, and adding or removing users to it involves saving/updating the node, which causes acquire_node_access_grants($node) to be called, again resulting in updated {node_access} records. CA also implements per-role access, but not through ACL.
What is your use case? Are you writing your own node access module as an ACL client? Is "community_members" a role or what? I wonder whether you can actually benefit from using ACL or whether you'd be better off doing this directly.
The reason for supplying an empty grant (with the client-defined priority) is twofold:
-- the empty grant denies access (at the proper priority) if no user has access
-- the empty grant is not stored but optimized away by core if there are other grants, which can be a huge saving on {node_access} table size
We cannot risk exploding everyone's {node_access} table because your custom module might benefit from this. The proper way to do this would be to extend the ACL API such that a client module can request its grants to be supplied even when they're not currently active (= have no users assigned).
#7
salvis, I get your "exploding {node_access} point, so I created a patch for the current stable to add an ability to contrib modules to let acl write empty ACLs to the node access records. This is by adding a column 'record_empty' to {acl_node} and an additional parameter to acl_node_add_acl(). See attached file.
I use ACL in my Audience module, as it uses acl to "cache" user lists. As these user lists need not to be associated with nodes, these lists might change without storing a node (e.g. User added a friend).
#8
Patches always need to go against the -dev versions, and against the latest one (currently 7.x-1.x-dev/master) first.
Here are some additional comments:
+++ b/sites/all/modules/contrib/acl/acl.install@@ -97,7 +97,15 @@ function acl_schema() {
+ 'record_empty' => array(
Name this column 'supply_unused'.
+++ b/sites/all/modules/contrib/acl/acl.install
@@ -97,7 +97,15 @@ function acl_schema() {
+ 'description' => 'Whether to write the node access record on empty users.',
+++ b/sites/all/modules/contrib/acl/acl.module
@@ -78,9 +78,9 @@ function acl_edit_form($acl_id, $label = NULL, $new_acl = FALSE) {
@@ -154,7 +154,7 @@ function acl_node_access_records($node) {
Whether to return records with no users to hook_node_access_records().
+++ b/sites/all/modules/contrib/acl/acl.install@@ -97,7 +97,15 @@ function acl_schema() {
+ 'default' => 0),
+ ),
Please respect my style of setting parentheses.
+++ b/sites/all/modules/contrib/acl/acl.install@@ -191,3 +199,19 @@ function acl_update_6002() {
+ * Add 'record_empty' column to {acl_node}.
Ditto.
+++ b/sites/all/modules/contrib/acl/acl.module@@ -78,9 +78,9 @@ function acl_edit_form($acl_id, $label = NULL, $new_acl = FALSE) {
+ db_query("INSERT INTO {acl_node} (acl_id, nid, grant_view, grant_update, grant_delete, priority, record_empty) VALUES (%d, %d, %d, %d, %d, %d, %d)", $acl_id, $nid, $view, $update, $delete, $priority, $record_empty);
Ditto.
+++ b/sites/all/modules/contrib/acl/acl.module@@ -261,4 +261,3 @@ function _acl_get_explanation($text, $acl_id, $module, $name, $number, $users =
-
No unnecessary changes, please.
Have you checked how these grants look in DNA? You may need to adjust acl_node_access_explain(), too.
#9
Ok, I'll post an altered patch these days, but at the moment I only can provide it for the Drupal 6 branch. Grants (acl_node_access_explain) look fine, as they say "no users".
Besides, I guess you know this style is not Drupal coding standard ;)
#10
I finally got to rework the patch for the current dev ;) Hope it can be commited though.
#11
Did you read the first sentence in #8?
In the D7 version we will need logic to update D7 as well as D6 with and without the additional column.