planet_access hook logic

Hi,

As I reviewed planet code for adding API documentation, I found that there's probably something unsafe in planet_access() :

function planet_access($op, $node, $account) {
  if ($op == 'create') {
    return user_access('edit own blog', $account) && $account->uid;
  }
  if ($op == 'update' || $op == 'delete') {
    if (user_access('edit own blog', $account) && ($account->uid == $node->uid) || user_access('administer nodes', $account)) {
      return TRUE;
    }
  }
}

Return value is undetermined (can be randomly TRUE or FALSE) in many cases:

• any value for $op apart from ('create', 'update', 'delete'),
  i.e. 'view' or inknown operator.
• when $op is 'update' or 'delete', and user should not be allowed to perform operation.

I don't know yet the module well enough to be sure about the intent, but I wrote something equivalent to the current logic, with comments to set it clearly.

So here's the version I suggest:

/**
 * Implementation of hook_access() - Define access permissions for planet nodes  (aka CRUD) .
 * @param $op  The operation to be performed: 'create', 'view', 'update', 'delete'
 * @param $node  The node on which the operation is to be performed, or, if it does not yet exist, the type of node to be created.
 * @param $account  A user object representing the user for whom the operation is to be performed.
 * @return true if the user is allowed to perform operation, false otherwise.
 */
function planet_access($op, $node, $account) {
  // Everyone can read planet nodes
  if ($op == 'view')
    return TRUE;
  // Node administrator has all permissions. 
  if (user_access('administer nodes', $account)) 
    return TRUE;
  // Users allowed to edit their own planet nodes, are also allowed to create them, others are not.
  // Permissions on planet nodes follow permissions on blog nodes.
  $author = user_access('edit own blog', $account);
  if ($op == 'create') {
    return $author;
  }
  if ($op == 'update' || $op == 'delete') {
    // ok if $author owns the node
    return $author && ($account->uid == $node->uid);
  }
  trigger_error("Unknown operation requested: $op");
  return FALSE;
}

But I still don't feel very well with it:

What should permission string 'administer planet' be used for ?

Maybe the clause about 'administer nodes' should be replaced, or completed with:

  // Planet administrator has all permissions related to planet module
  if (user_access('administer planet', $account)) 
    return TRUE;

What happens if blog module is not activated ?

It would probably be clearer if we used our own dedicated permission string.

To make it easier I used a distinct $author variable so the string appears only in one place.

What means 'update' for a planet node ?

Isn't it always generated from the RSS feed ?

Shouldn't any visitor be able to update the items from source feed ?

Best regards,
Michelle