access improperly denied when acl_id has no users then later gains users

(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:

• Tell core that permissions need to be rebuilt globally, or
• rebuild permissions for each of the affected nodes (needs to load each one!), or
• let the client module decide at which point a call to node_access_acquire_grants() is appropriate

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

This is definitely wrong: $grant['priority'] comes from the database.

But you may be on to something. Please post a corrected patch.

subscribing

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

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

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.

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

Please respect my style of setting parentheses.

Besides, I guess you know this style is not Drupal coding standard ;)

I finally got to rework the patch for the current dev ;) Hope it can be commited though.

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.

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

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

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.

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

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.

Updating issue status and version.

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.

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

Have a look at node_access_write_grants() though. Records that grant nothing are ignored. :)

...
// Only write grants; denies are implicit.
if ($grant['grant_view'] || $grant['grant_update'] || $grant['grant_delete']) {
  $grant['nid'] = $node->nid;
  $query->values($grant);
}
...

Please think again...

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

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

I'm also facing with this very same issue. The patch resolves this and works good for me.

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

@salvis: I meant the patch #695852-12: access improperly denied when acl_id has no users then later gains users

- pc-wurm

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

Fix status.