I am building services integration for this module, and it would be useful to be able to specify separate access control for viewing a variable and for setting it. Patch to follow introduces an 'access callback' key in the variable info array which takes an op ('view' or 'edit') as the first argument. I have tried to keep the API intact, so if you do not specify an access callback, the default behavior is as it was, and variable_access() defaults to using the 'edit' op -- there's a new function: variable_access_op() to do the op-specific check.

CommentFileSizeAuthor
#3 2132781-3-variable-access-op.patch5.76 KBwodenx
#1 2132781-variable-access-op.patch5.71 KBwodenx

Comments

wodenx’s picture

wodenx commented
Status: Active » Needs review
StatusFileSize
new 2132781-variable-access-op.patch5.71 KB
jose reyero’s picture

jose reyero commented
Status: Needs review » Needs work

The patch looks good, just two minor issues:
- Following the example of other Drupal access callbacks, we should move the $op value to the second place with some default ('edit'?) so it becomes optional.
- Better name for the new API function (variable_access_op), maybe 'variable_check_access' or something more meaningful, and use that one from now on for everything, keeping the old one only for backwards compatibility.

wodenx’s picture

wodenx commented
Status: Needs work » Needs review
StatusFileSize
new 2132781-3-variable-access-op.patch5.76 KB

- Following the example of other Drupal access callbacks, we should move the $op value to the second place with some default ('edit'?) so it becomes optional.

Not sure about this - hook_node_access() seems to be an anomaly in this respect. For example, entity api puts the $op in the first position:

function entity_access($op, $entity_type, $entity = NULL, $account = NULL) {
  if (($info = entity_get_info()) && isset($info[$entity_type]['access callback'])) {
    return $info[$entity_type]['access callback']($op, $entity, $account, $entity_type);
  }
}

As does hook_field_access():

/**
 * Determine whether the user has access to a given field.
 *
 * This hook is invoked from field_access() to let modules block access to
 * operations on fields. If no module returns FALSE, the operation is allowed.
 *
 * @param $op
 *   The operation to be performed. Possible values: 'edit', 'view'.
 * @param $field
 *   The field on which the operation is to be performed.
 * @param $entity_type
 *   The type of $entity; for example, 'node' or 'user'.
 * @param $entity
 *   (optional) The entity for the operation.
 * @param $account
 *   (optional) The account to check; if not given use currently logged in user.
 *
 * @return
 *   TRUE if the operation is allowed, and FALSE if the operation is denied.
 */
function hook_field_access($op, $field, $entity_type, $entity, $account) {
...

Renaming the funciton makes sense - have done so in the attached patch.

jose reyero’s picture

jose reyero commented
Status: Needs review » Needs work

Ok about the order of parameters, still some issues:
- Latest patch seems to be an inter-diff, please send full patch.
- Somehow this 'services_variable_access' function slipped in: 

return services_variable_access($op, $variable['parent'], $account);

- Are we removing the 'group' access feature? That one should stay just for backwards compatibility. Maybe we can just add-in group defaults, and then check the array only once. I mean

 $array_to_check = $variable + $group;

About the tests, we need a test for both cases ('edit' and 'view') (it may be a variable in variable_example).