Right now, notification triggers a notification event in notifications_content_nodeapi("insert"...). If, for example, a user is subscribed to a taxonomy term that matches the node being inserted, the node insert notification event will match their taxonomy subscription. If immediate notifications are enabled, this means that notifications_process_rows() is called while still within the node_invokenodeapi() rounds.

This causes a problem, as node_save() calls node_access_acquire_grants() AFTER the various nodeapi calls, which means when notifications_process_rows() calls notifications_user_allowed(), that the node_access grants have not yet been written for the node being inserted, which potentially results in notifications_content_node_allow() indicating that the user does NOT have access to the node being inserted, when after the grants are written, they may indeed have access.

Example Scenario:
- Drupal 5.9
- OG 5.7 with og_access enabled
- notifications/notifications_content/notifications_tags 5.x-1.0-rc1
- "immediate sending" is checked in notifications configuration
- mimemail enabled
- content type in question: 'story'
- 'story' content type associated with vocabulary 'topics'
- Access to 'story' nodes in groups is limited to group members
- User1 is a member of 'example group' (an og group)
- User1 is subscribed to the 'topics' term 'term1' with immediate notification
- User2 (also member of 'example group') submits a story node tagged as 'term1' with audience of group 'example group' not marked public

Expected Behavior: User2 should receive notification email of a new story node being created.

Actual Behavior: No email is sent to User2.

Suspected reason: notifications_content_node_all() reports that User2 does not have access to the new story node. This happens due to immediate sending happening before the node access grants are written.

CommentFileSizeAuthor
#5 fix-immediate-notifications-for-nodes-with-node-access-289211-D7.patch1.92 KBwebflo

Comments

jose reyero’s picture

jose reyero commented
Status: Active » Fixed

Inmediate sending has been moved to hook_exit(), this should fix the issue.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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

webflo’s picture

webflo commented
Version: 5.x-1.0-rc1 » 7.x-1.x-dev
Status: Closed (fixed) » Active

Hey this bug is an regression. hook_exit() is removed (e86557d). Notifications for nodes are send via hook_node_insert / hook_node_update.

Here is a backtrace for quick reference.

#0  Notifications_Node->user_access called at [/Users/florian/Sites/notifications.dev/sites/all/modules/contrib/notifications/notifications.event.inc:548]
#2  Notifications_Event->send_list called at [/Users/florian/Sites/notifications.dev/sites/all/modules/contrib/notifications/notifications.event.inc:459]
#3  Notifications_Event->send_all() called at [/Users/florian/Sites/notifications.dev/sites/all/modules/contrib/notifications/notifications.event.inc:279]
#4  Notifications_Event->send() called at [/Users/florian/Sites/notifications.dev/sites/all/modules/contrib/notifications/notifications.event.inc:265]
#5  Notifications_Event->dispatch() called at [/Users/florian/Sites/notifications.dev/sites/all/modules/contrib/notifications/notifications.event.inc:235]
#6  Notifications_Event->trigger() called at [/Users/florian/Sites/notifications.dev/sites/all/modules/contrib/notifications/notifications_content/notifications_content.inc:91]
#7  Notifications_Node_Event->trigger() called at [/Users/florian/Sites/notifications.dev/sites/all/modules/contrib/notifications/notifications_content/notifications_content.module:462]
#8  notifications_content_node_post_action called at [/Users/florian/Sites/notifications.dev/includes/actions.inc:132]
#9  actions_do called at [/Users/florian/Sites/notifications.dev/modules/trigger/trigger.module:273]
#10 _trigger_node called at [/Users/florian/Sites/notifications.dev/sites/all/modules/contrib/notifications/notifications_content/notifications_content.module:629]
#11 notifications_content_node_insert
#12 call_user_func_array
#13 module_invoke_all(node_insert, stdClass Object ([uid] => 1,[name] => admin ... called at [/Users/florian/Sites/notifications.dev/modules/node/node.module:1135]
#14 node_save called at [/Users/florian/Sites/notifications.dev/modules/node/node.pages.inc:406]
#15 node_form_submit

node_access() is called in Notifications_Node->user_access (#0) but node access for this is not build yet. Because its build in node_save after node_invoke($node, 'insert').

Here is the relevant part form node_save.

    // Call the node specific callback (if any). This can be
    // node_invoke($node, 'insert') or
    // node_invoke($node, 'update').
    node_invoke($node, $op);

    // Save fields.
    $function = "field_attach_$op";
    $function('node', $node);

    module_invoke_all('node_' . $op, $node);
    module_invoke_all('entity_' . $op, $node, 'node');

    // Update the node access table for this node. There's no need to delete
    // existing records if the node is new.
    $delete = $op == 'update';
    node_access_acquire_grants($node, $delete);

    // Clear internal properties.
    unset($node->is_new);
    unset($node->original);
    // Clear the static loading cache.
    entity_get_controller('node')->resetCache(array($node->nid));

Whats the best way to fix this? Implement hook_exit in notifications_content.module again and trigger all content notifications?

webflo’s picture

webflo commented

Drupal core issue #690520: Calling node_access() from inside hook_node_save() results in wrong access for new nodes

webflo’s picture

webflo commented
Title: Immediate notifications on node insert fail with node access module enabled » Immediate notifications on nodes with node access fails
Status: Active » Needs review
StatusFileSize
new fix-immediate-notifications-for-nodes-with-node-access-289211-D7.patch1.92 KB

Workaround via hook_exit(). It works but is very ugly.

zeezhao’s picture

zeezhao commented

+1. Please is this applicable to latest dev version? I still have issues with getting notifications... Thanks

Fyi - I am using acl & content_access modules

webflo’s picture

webflo commented

Yes its sill an issue. Its not committed yet. The patch should apply.

webflo’s picture

webflo commented
Status: Needs review » Fixed

Committed the patch. I track the core issue.

Status: Fixed » Closed (fixed)

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