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;
}
Comment | File | Size | Author |
---|---|---|---|
#12 | acl-695852-12-acl-without-users.patch | 707 bytes | milesw |
#10 | add_records_for_empty_acls-695852-10.patch | 2.79 KB | derhasi |
#7 | add_records_for_empty_acls-695852-7.patch | 2.84 KB | derhasi |
Comments
Comment #1
salvis(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).
Comment #2
heacu CreditAttribution: heacu commentedthe 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?
Comment #3
heacu CreditAttribution: heacu commentedfollowing up, why not instead something like:
Comment #4
salvisThis is definitely wrong: $grant['priority'] comes from the database.
But you may be on to something. Please post a corrected patch.
Comment #5
kewlguy CreditAttribution: kewlguy commentedsubscribing
Comment #6
salvisLet'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).
Comment #7
derhasi CreditAttribution: derhasi commentedsalvis, 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).
Comment #8
salvisPatches 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:
Name this column 'supply_unused'.
Whether to return records with no users to hook_node_access_records().
Please respect my style of setting parentheses.
Ditto.
Ditto.
No unnecessary changes, please.
Have you checked how these grants look in DNA? You may need to adjust acl_node_access_explain(), too.
Comment #9
derhasi CreditAttribution: derhasi commentedOk, 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 ;)
Comment #10
derhasi CreditAttribution: derhasi commentedI finally got to rework the patch for the current dev ;) Hope it can be commited though.
Comment #11
salvisDid 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.
Comment #12
milesw CreditAttribution: milesw commentedYears later this is definitely still an issue. Perhaps it only affects certain use cases.
Here's a use case that ACL would suit perfectly if this issue were resolved:
- Each user can create a list of other users to be delegates
- A user's delegates get access to manage his/her nodes
- Users can add/remove delegates from their list at any time
The beauty of using ACL is that delegated users are not directly part of the node access records, so those records don't need to be updated when delegates change. This is what the OP was describing.
This is a valid concern. However, I would argue that a client module keeping records in the acl_node table is actually a request for node access records to be written.
Not sure if this is true with D6, but with D7 denies are implicit. Node access records will not even get written for deny all.
Here is a small patch to resolve the issue by omitting the acl_has_users() check and the record for deny all.
Comment #13
milesw CreditAttribution: milesw commentedUpdating issue status and version.
Comment #16
salvisLook at the node_access_acquire_grants() function in core. If no module returns any records, then core supplies one that grants read access to everyone.
We cannot return no record without risking to expose the node.
Comment #17
milesw CreditAttribution: milesw commentedHave a look at node_access_write_grants() though. Records that grant nothing are ignored. :)
Comment #18
salvisPlease think again...
Comment #19
milesw CreditAttribution: milesw commentedAh, thanks, I see what you're saying now. The empty grants supplied by ACL don't actually get written, they just suppress the default grants from core.
However, that means the patch in #12 does not change anything regarding access. Grants are still returned, core's default grants are still suppressed. It simply removes the requirement for an ACL to have users.
Again, I think that any module adding a node to an ACL is requesting that node be access controlled, regardless of the number of users on the ACL.
Comment #20
salvisYou're beginning to convince me.
Please post a screenshot of the two DNA blocks in debug mode when looking at a node with an ACL that has no users.
Comment #21
Elin Yordanov CreditAttribution: Elin Yordanov commentedI'm also facing with this very same issue. The patch resolves this and works good for me.
Comment #22
salvis@pc-wurm: Which patch are you referring to?
I've somewhat lost track of this issue and I'm not deep inside NA right now, but here's a thought, based on the title of this issue:
I think the proper way to handle the situation "when acl_id has no users then later gains users" is for the module that adds the users to also call node_access_acquire_grants() for each of the nodes controlled by that ACL. That should result in consistent NA data.
Comment #23
Elin Yordanov CreditAttribution: Elin Yordanov commented@salvis: I meant the patch #695852-12: access improperly denied when acl_id has no users then later gains users
- pc-wurm
Comment #24
salvisI stand by #22. ACL just provides an API for by-user access control. It doesn't know what the client module is doing, but it manages and provides the information that the client module needs for doing its thing.
In the general case the client module will need to call node_access_acquire_grants() anyway, and if ACL were to do this, it might be done twice.
I keep this open for the benefit of those who find it useful or who want to further discuss it, but at this point I don't think it's committable.
Comment #25
salvisFix status.