I've just created a module that integrates flag with organic groups (http://drupal.org/project/og_flag). In order to fully complete the integration I need to have a new hook call added to the flag module.

The new hook, hook_flag_user_access, allows other modules to perform user access checks.

The attached patch adds this hook to the 2.x branch. I'll also create a patch for the 3.x branch shortly.

NOTE: The existing hook_flag_access() is not appropriate here since implementing this hook could potentially completely bypass any additional non-role access checks that custom flags perform.

Files: 
CommentFileSizeAuthor
#15 1965040.flag_.hook-flag-access-return.15.patch8.1 KBjoachim
#14 flag-hook_flag_access-1965040-13.patch735 bytesalesr
PASSED: [[SimpleTest]]: [MySQL] 184 pass(es).
[ View ]
#13 flag-hook_flag_access-1965040-13.patch8.84 KBshawn_smiley
PASSED: [[SimpleTest]]: [MySQL] 254 pass(es).
[ View ]
#8 1965040-hook_flag_access.patch2.68 KBshawn_smiley
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]
#1 1965040-hook_flag_user_access.patch1.51 KBshawn_smiley
PASSED: [[SimpleTest]]: [MySQL] 61 pass(es).
[ View ]

Comments

shawn_smiley’s picture

StatusFileSize
new1.51 KB
PASSED: [[SimpleTest]]: [MySQL] 61 pass(es).
[ View ]

Attached is the patch for hook_user_access() on Flag 7.x-2.x.

shawn_smiley’s picture

Status:Active» Needs review
joachim’s picture

Version:7.x-2.x-dev» 7.x-3.x-dev
Status:Needs review» Postponed (maintainer needs more info)

Integration between Flag and OG sounds great!! :)

> NOTE: The existing hook_flag_access() is not appropriate here since implementing this hook could potentially completely bypass any additional non-role access checks that custom flags perform.

Could you explain more about this please?

We recently added hook_flag_validate(), so this would be a third 'can this flagging happen?' type hook, which is starting to feel like a lot.

This would also need to go on 3.x, not 2.x.

shawn_smiley’s picture

Status:Postponed (maintainer needs more info)» Needs review

I'll be working on a 3.x patch this coming weekend. I've taken a quick look at flag 3.x and it appears that the permissions checks/hook apis have changed significantly so most likely a different solution will be needed for compatibility with the 3.x branch of the Flag module.

We need this hook added to the 2.x branch since our specific use case for the og_flag module is in support of Drupal Commons 3.x which is still using Flag 2.x.

As to why hook_flag_access() isn't appropriate for use here, let me provide a specific use case where using hook_flag_access() doesn't work in this context:

We created a custom flag module (Commons Correct Answer) which adds a custom flag to Drupal Commons. This flag uses hook_flag_access() to make sure the flag can only be applied to one answer node for any related question node.

If the og_flag module also used hook_flag_accesss() to control visibility based on user permissions, then the og_flag module would end up completely bypassing the node relationship checks in the commons_correct_answer module. This is because the $flag->access() function, which invokes hook_flag_access(), works on the basis of the last executed hook implementation has final say on visibility. Thus if the og_flag module said the user had permission access, the node relationship check of the custom flag module would be ignored.

What we really needed to do within the og_flag module was to modify the result of $flag->user_access() since the conditional checking is entirely permission based. This allows custom flags to still maintain their own conditional display logic while relying on $flag->user_access() to tell them if the user has the necessary permission to use the flag.

joachim’s picture

Status:Needs review» Needs work

> This is because the $flag->access() function, which invokes hook_flag_access(), works on the basis of the last executed hook implementation has final say on visibility.

I don't think that's quite it, but clearly something of that ilk is going on:

    // Allow modules to disallow (or allow) access to flagging.
    $access_array = module_invoke_all('flag_access', $this, $entity_id, $action, $account);

    foreach ($access_array as $set_access) {
      if (isset($set_access)) {
        $access = $set_access;
      }
    }

I read that as the first module that either says 'Yes' or 'No' is the decider.
That's fine if it's a 'No', as we want a denial by a single module to deny totally.
But it's no good for granting access.

I would be more inclined to say this behaviour is incorrect and needs to be changed.

Compare with node_access()'s code:

  // We grant access to the node if both of the following conditions are met:
  // - No modules say to deny access.
  // - At least one module says to grant access.
  // If no module specified either allow or deny, we fall back to the
  // node_access table.
  $access = module_invoke_all('node_access', $node, $op, $account);
  if (in_array(NODE_ACCESS_DENY, $access, TRUE)) {
    $rights[$account->uid][$cid][$op] = FALSE;
    return FALSE;
  }
  elseif (in_array(NODE_ACCESS_ALLOW, $access, TRUE)) {
    $rights[$account->uid][$cid][$op] = TRUE;
    return TRUE;
  }
shawn_smiley’s picture

I agree that the code in $flag->access() should be modified to be more like the access checks done by the node module.

I was reluctant to go down that path initially though since this is an existing API and this type of change potentially could break other modules if we change the way it works. I can certainly create a patch to implement this type of access change if you would like.

I think implementing the node_access style checks in the hook_flag_access() is better suited for the Flag 3.x branch. I'll look into that this weekend when I'm updating the og_flag module to work with Flag 3.

joachim’s picture

Title:Add hook_flag_user_access()» change handling of return values from hook_flag_user_access()

> since this is an existing API and this type of change potentially

That's what the new major version is for! :)

Changing title accordingly.

shawn_smiley’s picture

Status:Needs work» Needs review
StatusFileSize
new2.68 KB
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]

Here is a patch that changes the return values for hook_flag_access() to be more like way node_access() works.

joachim’s picture

Status:Needs review» Needs work
Issue tags:-integration, -hook, --Organic Groups

Changes look good, but we don't need to have new constants here:

+++ b/flag.api.php
@@ -175,8 +175,13 @@ function hook_flag_validate($action, $flag, $entity_id, $account, $skip_permissi
+ *    - FLAG_ACCESS_ALLOW : User has access to the flag.
+ *    - FLAG_ACCESS_DENY : User does not have access to the flag.
+ *    - FLAG_ACCESS_IGNORE : This module does not perform checks on this flag/action.

The standard in other parts of Drupal is:

TRUE: grant access
FALSE: explicitly deny access
NULL: say nothing

joachim’s picture

Issue tags:+Novice

I'm tagging this as Novice as the patch doesn't need much more work.

Would be great to get this in for 3.0, as it's an API change -- if it doesn't make it in soon, it's postponed till D8!

shawn_smiley’s picture

Thanks for the review and feedback. I should have some time this coming weekend to reroll the patch without the constants.

joachim’s picture

Great! Look forward to it.

After that, I'll roll a beta and hopefully a 3.0 soon after that!

shawn_smiley’s picture

Status:Needs work» Needs review
StatusFileSize
new8.84 KB
PASSED: [[SimpleTest]]: [MySQL] 254 pass(es).
[ View ]

Here is the updated patch. Changes in this patch include:

  • Removed the constants and replaced with checks for TRUE|FALSE|NULL
  • Updated documentation comments in the flag.api.php file to reflect this change.
  • Added a simpletest case for verifying the implementation.

NOTE: I had some trouble with my IDE applying the patch where it didn't create the new simpletest submodule. But everything works perfectly when applying the patch with "git apply".

alesr’s picture

StatusFileSize
new735 bytes
PASSED: [[SimpleTest]]: [MySQL] 184 pass(es).
[ View ]

Here is a simplified patch from #8.

EDIT: After I committed it I noticed that @shawn_smiley wrote a very similar patch few minutes before me ;)
Well, at least we're closer to get Flag 7.x-3.x stable release out now.

joachim’s picture

Status:Needs review» Fixed
StatusFileSize
new8.1 KB

Thanks -- both of you! :)
And tests, wow!

Committing this patch, which is #13 with a few tweaks to comments and a change to this assertion text:

> $this->assertNoLink('Flag this item', 0, 'The flag link does not appear on the page.');

I assume the message should say 'does not appear' rather than 'appears', since we're asserting it's not there.

git commit -m "Issue #1965040 by shawn_smiley: Changed handling of return values from hook_flag_user_access() to allow all implementing modules to be considered."

And with that, I'm rolling a beta release tonight!

joachim’s picture

Title:change handling of return values from hook_flag_user_access()» change handling of return values from hook_flag_access()

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