Allow other modules to alter actions in scald_user_actions

It is difficult when many modules intercept that array in drupal_alter. I think something like hook_action_grant would be better.

1. I talked with mk.york, and we found that we could maybe put a hook_action_grant hook just before the return user_access part in the scald_user_actions function.

We could use as an example the node_access function:
http://api.drupal.org/api/drupal/modules%21node%21node.module/function/n...

Specifically the following part:

  // 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;
  }

We could do this for each action and action_key, and then depending on the return values we can set the bit in the given position to 0 or 1. Also if none of the hook implementations retun TRUE or FALSE then we can keep the original implementation but for only that one bit.

The hook could have the following parameters:
hook_action_grant($op, $account, $action_key, $atom)

2. We should also put either this hook or a different hook at around the 134th line of the scald_plugin_display_library.inc. To be able to define additional access control for the creation of individual atom types.

It could either be hook_action_grant with $op and $account required, and $action_key, $atom optional (as they are not available there), and $op = 'add', or a separate hook that only does this, like hook_add_grant($account), but the same mechanics could apply with less complexity.

These hooks could open up the way for modules like Scald OG or other custom modules to fine-tune the permission system.

1. Exactly. It's what I wanted to say.

2. We can add an "add" action, and $atom could be incomplete (for example, there is $atom->type but not $atom->sid). Scald could itself implement for the "add" action based on core permissions.

What is $action_key?

I meant this variable:
$actions_key = ($atom->publisher == $account->uid) ? 'own' : 'any';

So it is no longer necessary? The only hook that needs to add is

hook_scald_atom_access($atom, $action, $account = NULL)

I think it is necessary, as there are separate permissions for each action for each actions_key, so i might have edit right on my own atoms but not on any atom. Then the hook has to know if i need it for the edit action on my own atoms, or on any atom, as i guess we would like if the hook would return a single FALSE or TRUE or nothing, instead of a pair of values for own, and any.

Its true however that the hook implementation could calculate this variable as well. So yes the hook could have only 3 parameters.

Yes, it can be easily deduced from $atom (if it is not a full atom, then the only action is "create", and we don't care about either "any" or "own"). I don't see any other use cases for another action, so I think it's good to go with at most 3 parameters.

I put together a patch keeping the above in mind.

I added the hook_scald_atom_access($atom, $action, $account = NULL) hook at the above mentioned two places, and also added it into the api documentation.

For the standard actions, i loop over all actions, and using the following procedure:
1. At least one of the hook implementations returned FALSE - deny the given action
2. None of the hooks returned FALSE, and at least one of them returned TRUE, allow the given action
3. None of the implementations returned anything (they all returned NULL (return;)) then we check the bypass permission and use the or bitwise operation on that action only!
4. None of the implementations returned anything, and no bypass permission, then we do the equivalent of what the code did previously and using the and bitwise operation on that action only.

Then just have to return the result.

For the create action i do the following:
1. If at least one of the hooks returned FALSE then deny the atom creation of the given type
2. None of the hooks returned FALSE, and at least one of them returned TRUE, allow the creation of the given type
3. Otherwise check the drupal permission of the given type
4. Otherwise deny the creation of the given type.

This module seems to work fine at the moment, was testing it with OG.

I did a quick review, as I didn't have time to get into the permission logic. It's sad to see the hook_scald_atom_access invoked in two places, could we have a helper function to check it once? However I know it's difficult to refactor them. BTW to make it sorter, Scald core could implement hook_scald_atom_access to return TRUE if there is the "bypass access restriction" permission (the logic in patch #10).

I haven't checked where the "bypass access restriction" permission is used, but I think it must have priority over the access hook.

It could be done of course, though the bypass did not affect atom creation before.

At the moment in patch #10 the hook can decide if it wants to use the bypass permission for something or not.
For example currently my OG module simply does a return; when the user has the bypass permission. But maybe some other custom code wants to return TRUE on the bypass permission to have all actions available in certain cases. As returning TRUE is different from the original behaviour (that would be when returning nothing) where the permission was like if atom had (edit, view, fetch) and user had (edit, fetch) then it became (edit, view ,fetch) on bypass, however with TRUE it would become (edit, view, fetch, delete). as then for each action we return TRUE, unless of course we only return TRUE where the bit is 1 with the same logic as before.

I guess its a question of whether we want other modules to define how the bypass is handled or its better if scald core handles that and the hook cannot do anything with it. Generally was following the node access example where the hooks take precedence over everything else.

About the atom creation, I believe the bypass permission check was an omission. We could add that.

If we following node_access pattern, should the bypass permission be checked first? As in http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/modules/no...

The bypass permission should be only given to "super admin" roles. It already has the "restrict access" flag on.

You are right!

However I believe we have 2 options:

1. In the helper function we return TRUE for each action if we have the bypass permission. Which then means that i will have view, edit, delete on all atoms if I have the permission.
Originally even if I had bypass maybe i did not have all actions, cause neither my user actions and the atom's actions had delete for example. Im not exactly sure why the mechanic was in place like that, but maybe then we dont need this anymore.

2. We can just implement the hook in scald itself and return TRUE if the bypass permission is set. In this case a FALSE from another hook implementation could veto access even if i have bypass permission.

There is a scenario when on top of OG roles, i would like if a drupal role had the edit action on each atom, but maybe not the delete action. And the user with this role can also have OG membership that would limit his rights, even thought that is not supposed to happen. With the first implementation he would have all actions including delete everywhere. And with the second action i can write a custom hook that returns FALSE for delete for this kind of user. Of course there could be other workarounds to fix this problem even with the first version, like creating a special OG role and assigning the user that role in each group, but thats not soo robust. Or if the bypass only grants actions that are there in both the atoms and the user, then this drupal role could be set up in a way to not have the delete action.
Of course this is just a scenario with OG and maybe in other scenarios the other variation is better.

Chose the first option, because realized that an OG module could work like an extender only, which would allow base permission and custom hook implementations to fine tune the permission handling.

I also realized that the create permission check happens at more than one place, so I had to refactor the code quite a bit. Scald already had a scald_atom_add_access callback function, but was not used anywhere else, so started using it at two other places, and moved the new permission code in there. Also created a helper function that handles the calling of the hooks, as that is needed at 2 places in the code.

Ok looks good, but I think I'll do some refactoring before commiting it.

New patch.
- Constants are used instead of TRUE/FALSE/NULL.
- Some inline comments are moved to the API file.
- Introduce scald_atom_access() function that will become the central hub. Scald will implement hook_scald_atom_access based on the permission. Previously scald_user_actions uses cache for performance, but with the introduction of the new hook, it became less relevant. The idea is to move to the scald implementation of hook_scald_atom_access.

If it ok for all of you, I think it is ok to be committed with a test for permission. The complete implementation in scald_scald_atom_access can be in a follow-up.

+++ b/includes/scald.constants.incundefined
@@ -55,3 +55,12 @@ define('SCALD_ADMIN_ACTIONS_BATCH_LIMIT', 1);
+define('SCALD_ATOM_ACCESS_IGNORE', NULL);

Each constant should have its own PHPDoc comments.

We could probably get the comments back from node.module, so, something like

/**
* Modules should return this value from hook_scald_atom_access() to allow access to an atom.
*/

+++ b/scald.moduleundefined
@@ -1067,6 +1067,58 @@ function scald_rendered_to_sas($string, $render_language = 'XHTML') {
+function scald_atom_access($atom, $action, $account = NULL) {

Both the name and the PHPdoc implies that this is really generic… when in fact it's not ?

As far as I can tell, the generic version after this patch is still scald_action_permitted ; calling scald_atom_access directly will only result in checking the overrides.

Overall, I'm not sure why we're adding a new scald_atom_access function, instead of keeping only scald_action_permitted ?

Indeed I forgot that function and also scald_atom_access was half baked and was not meant for "full usage" yet. So you are correct and I merged it back to scald_action_permit. There is some duplicate logic, because the conversion from role based permissions to hook based permissions does not finish, some checks happen twice.

Here you are the new patch.

1. One instance of scald_atom_access was not replaced in modules/library/scald_dnd_library/includes/scald_plugin_display_library.inc

2. There is a typo in
"+ // Check hook based permissions first, because role based permissions are
+ // "allow-like" and therefore can not negave hook based permission."

=> negate

This starts to look better :-)

There's still a few things that needs to be ironed out though:

• scald_action_permitted can receive an array of actions. That array should probably not be passed down to the various hooks, which means that the scald_invoke_atom_access should be moved to the inner foreach loop.
• That being said, I'm not sure why the change to scald_action_permitted is needed ; given the change to scald_user_actions, everything should already be working just fine ?
• This is changing quite deeply the meaning of the "bypass atom access restrictions" permissions. Previously, it meant that it would discard the "actions restrictions" that had been set by the user who created the atom, and the role thus be granted the permission that where checked on the permissions admin page. (e.g. if you had "bypass atom access restrictions" and "view any atom [marked as viewable]", then you could view any atom no matter if the atom had its "View" action checked or not, but you're not able to delete atoms… With this patch, any user with the bypass atom access restrictions can do anything to any atom.

The last one could be a problem, because it's completely removing something kinda useful. Maybe its edge-case-y enough to be moved to a separate module though, similar to what View Unpublished is doing for nodes.

Cross-post, restoring the status

That being said, I'm not sure why the change to scald_action_permitted is needed ; given the change to scald_user_actions, everything should already be working just fine ?

For example the 'create' action (as its not an action on the atom itself) is not handled by scald_user_actions.

This is changing quite deeply the meaning of the "bypass atom access restrictions" permissions. Previously, it meant that it would discard the "actions restrictions" that had been set by the user who created the atom, and the role thus be granted the permission that where checked on the permissions admin page. (e.g. if you had "bypass atom access restrictions" and "view any atom [marked as viewable]", then you could view any atom no matter if the atom had its "View" action checked or not, but you're not able to delete atoms… With this patch, any user with the bypass atom access restrictions can do anything to any atom.

For me "bypass atom access restrictions" would mean that I'm bypassing any access restriction on an atom, so in a sense the name of the permission is misleading, and the description is not better "Grants access to all atoms.".

I guess we could keep the bypass atom access restrictions as proposed in the patch, but there could be another permission and can be implemented in 2 ways:
1. A little change in the scald_user_actions after the scald_invoke_atom_access if its neither ALLOW or DENY

2. By implementing the hook.

The second can also be separated out into its own module.

On the other hand if a 3rd party hook returns FALSE to any action, that means that even though the user has this permission he will not have access, as in both cases that permission would not have priority.

How about "administer scald atoms"? Make it like the bypass access in the patch, and keep the bypass access permission like it was.

After long discussion on IRC, here is a new patch.

A minor change that I don't want to upload a new patch: 'restrict access' flag should be off for 'restrict atom access' permission.

Also the description of "restrict atom access" is wrong, atm its the same as the bypass one.

Fixed those suggestions, add more doc.

Come on! I went ahead and committed #30.