Currently running entity_access() on a message entity (eg, as in #1918666]) will call message_access(), which simply returns the value of

user_access('create messages', $account);

In some cases, a module or modules may want to introduce a more complex set of access checking criteria. Currently, this can only be accomplished by implementing hook_entity_info_alter() and changing the 'access callback' value. One drawback of that approach is that only a single module or callback can be used within one installation of Drupal.

In Commons, we want the Commons Activity streams module to be enforce access criteria on all messages that it defines, and prevent users from seeing or receiving messages that reference nodes that the viewer/recipient cannot access.

Files: 
CommentFileSizeAuthor
#6 message-hook_message_access-1920560-6.patch4.5 KBpbuyle
PASSED: [[SimpleTest]]: [MySQL] 69 pass(es). View
#1 1920560-message-access-alterable.patch2.03 KBezra-g
PASSED: [[SimpleTest]]: [MySQL] 69 pass(es). View

Comments

ezra-g’s picture

Status: Active » Needs review
FileSize
2.03 KB
PASSED: [[SimpleTest]]: [MySQL] 69 pass(es). View

Patch attached defines hook_message_access_alter() and provides an example of how we're implementing this in Commons.

amitaibu’s picture

Status: Needs review » Closed (won't fix)

Currently, this can only be accomplished by implementing hook_entity_info_alter() and changing the 'access callback' value. One drawback of that approach is that only a single module or callback can be used within one installation of Drupal.

I believe this is the correct solution. Regarding the draw back, same thing can be said about hook_menu()'s access callback. I'm setting to won't fix, but please re-open if you think otherwise.

ezra-g’s picture

Status: Closed (won't fix) » Needs review

I believe this is the correct solution. Regarding the draw back, same thing can be said about hook_menu()'s access callback. I'm setting to won't fix, but please re-open if you think otherwise.

Thanks. For your consideration :

From my perspective, message access is different from a page access callback because it's more likely to have multiple different modules interacting with it depending on the situation.

For example, Commons wants to be able to restrict access for the Commons activity stream messages that contain certain entity reference fields, but have no effect on other message entities, such as the messages a user might add to her site unrelated to Commons.

This differs from a menu callback where it typically makes sense to have a single function responsible for a single path in all all cases.

ezra-g’s picture

You could add to the above example with a scenario where one site has Commons controlling access to its own Message entities, and another contrib or custom module listening and controlling access to only its own set of entities.

amitaibu’s picture

If anything I think we should follow the pattern of hook_node_access() were you return NODE_ACCESS_IGONRE/ DENY.
However I'd be happy if you could profile it, as it might add a lot of overhead - better to know exactly how much.

pbuyle’s picture

Issue summary: View changes
FileSize
4.5 KB
PASSED: [[SimpleTest]]: [MySQL] 69 pass(es). View

message-hook_message_access-1920560-6.patch provide a pluggable access check for message using a hook_message_access() pattern similar to what node_access() does. When no implementation of hook_message_access() is available, it falls back to the original behavior.

amitaibu’s picture

@mongolito404, thanks, it looks good. Any chance for a benchmark (e.g. compare view with 100 messages with and without the patch)?

pbuyle’s picture

Benchmark would be good. But I dont see a way to make a simpler/opimized version of the wanted behavior.

amitaibu’s picture

> But I dont see a way to make a simpler/opimized version of the wanted behavior.

We can always have a variable to skip it, but before adding something like this, lets figure out the performance penalty.

amitaibu’s picture

> We can always have a variable to skip it,

Oh, actually I see you already have a permission to bypass it.

amitaibu’s picture

Status: Needs review » Needs work
  1. +++ b/message.api.php
    @@ -134,5 +134,30 @@ function hook_default_message_type_category_alter(array &$defaults) {
    +function hook_message_access($message, $op, $account) {
    +
    +}
    

    We can type-hint Message $message
    Also can we add an example.

  2. +++ b/message.module
    @@ -800,7 +820,52 @@ function message_delete_multiple($mids = array()) {
    +  // If the $op was not one of the supported ones, we return access denied.
    +  if (!in_array($op, array('view', 'update', 'delete'), TRUE)) {
    +    return FALSE;
    +  }
    

    Lets remove this.

  3. +++ b/message.module
    @@ -800,7 +820,52 @@ function message_delete_multiple($mids = array()) {
    +    $account = $GLOBALS['user'];
    

    I prefer loading the user, to get the full object.

  4. +++ b/message.module
    @@ -800,7 +820,52 @@ function message_delete_multiple($mids = array()) {
    +  list($cid) = !empty($entity) ? entity_extract_ids($entity_type, $entity) : array($entity_type);
    

    $cid => $mid

Also, can we have a test? :)

heylookalive’s picture

I'm exposing message entities via RESTful and have just hit this as an issue, because users don't have the "create messages" permission they can't see the ID/mid field as they don't have access, but can access it's other properties. Anyway.

I agree on the hook_nodeapi approach of allowing modules to DENY or ALLOW access, but don't have time to put towards the work needed in this issue.

What's worth noting is that message, message_notify, message_subscribe or message_ui make use of the permission create messages, in which case it makes the whole check moot, and you might as well give all users this permission (in practice, not really but you get what I mean). As a way to tell if someone should or should not access a message, it's somewhat pointless. In my implementation of message there are other access checks around it which protects the content.