Currently neither Message Subscribe nor Message Notify check node access before sending out notifications to the list of subscribers.

I'm curious whether the maintainers feel that node access should be the responsibility of Message_notify, Message_subscribe, or the module that implements the Message* API (in this case, the Drupal Commons distribution) to implement node access restrictions.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amitaibu’s picture

Title: Node Access support » Entity Access support
Status: Active » Needs review
FileSize
1.5 KB

Yes, it should be part of Message subscribe.
Completely untested patch attached.

ezra-g’s picture

I tested this in conjunction with #1920560: Make message access alterable - I tested by having a non-administrative user follow a group, then posted 2 nodes into the group as an admin - one published and one unpublished. Only the message about the published group was sent.

So, +1 to #1 from me.

amitaibu’s picture

I tested by having a non-administrative user follow a group, then posted 2 nodes into the group as an admin - one published and one unpublished. Only the message about the published group was sent.

It's really helpful your explanation about testing, I can tell you are submerged with user-stories ;)

Added test.

Status: Needs review » Needs work

The last submitted patch, 1918666-entity-access-3.patch, failed testing.

amitaibu’s picture

Assigned: Unassigned » amitaibu
Status: Needs work » Needs review
FileSize
3.9 KB

Fixed failing test.

amitaibu’s picture

Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Mixologic’s picture

Category: feature » support
Status: Closed (fixed) » Active

I ran into a problem with this currently, and thought I'd document my findings here.

I am calling message_subscribe_process_message() within a hook_node_insert.

If you are using a module that implements node_grants (like content access), the entity_access check will fail when you have a user who is subscribed to either a taxonomy term or a user.

This is because when a new node is created, hook_node_insert is called *before* node_access_acquire_grants, therefore the user will not *yet* have view access to the node entity when this check is made, thus the access check will fail.

My solution was to add node_access_acquire_grants($node, 'update'); into my hook_node_insert to force it to happen before this check takes place, and then add a node_access_write_grants($node, NULL, NULL, TRUE); to blow away the grants that I just created, because node_save still expects them to not exist yet and will throw an exception.

Hopefully this helps somebody, and perhaps should be added to the example module implementations.

amitaibu’s picture

Category: support » bug
Status: Active » Closed (fixed)

Thanks for the docs, but that's beyond the scope of Message-subscribe -- that's Drupal's node-access API system.

Mixologic’s picture

Well, if you are using a drupal core feature, node grants, then message subscribe's entity access check is broken, so, well, bug report.

amitaibu’s picture

Assigned: amitaibu » Unassigned
Status: Closed (fixed) » Active

Ok, I now understand what you mean. Re-opened.

japerry’s picture

Status: Active » Needs review

I'm curious what Amitai's thoughts are on this, but #8 is the only way I could successfully use message subscribe with node access enabled. There is no nodeapi function to check the permissions after node_save, which is when we should be shooting the message anyway.

In lieu of that, we could add those two calls before and after the entity access call inside message subscribe or document it as something you must do in an implementing module.

Which would you prefer?

japerry’s picture

added a patch for core which will fix the problem described in #8:

#322636: node_access_acquire_grants() should run before node save/update

For commons we can roll it into our make file. For people working on other projects they'll have to apply #8 or do the patch above.

ezra-g’s picture

Status: Needs review » Needs work

#322636: node_access_acquire_grants() should run before node save/update has been won't fixed, so I believe this is "needs work" to implement hook_register_shutdown_function() from Message Subscribe's hook_node_save() implementation for notifications about new nodes.

ezra-g’s picture

Category: bug » support
Status: Needs work » Fixed

Upon further thought, the hook_register_shutdown() function belongs in the module that implements Message Subscribe. In our case, that's Drupal Commons.

Since there's no change that needs to happen to Message Subscribe, I think this is support request that's now fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Agence Web CoherActio’s picture

After having spent hours on the same problem and finally finding that drupal_register_shutdown_function is the solution, I'd suggest to add this approach to the README.TXT.

Thanks for that great module.

Laurent

Leeteq’s picture

Category: Support request » Task
Issue summary: View changes
Status: Closed (fixed) » Active

Yes, drupal_register_shutdown_function should be mentioned in the README.txt

amitaibu’s picture

Alright, please add a patch with the explanation.

  • amitaibu committed 0063ab0 on 8.x-1.x
    Issue #1918666 by Amitaibu | ezra-g: Added Entity Access support.
    
bluegeek9’s picture

Status: Active » Closed (outdated)