Posted by RockyRoad on March 15, 2009 at 4:38pm
Jump to:
| Project: | Planet profile |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (won't fix) |
| Issue tags: | access, planet node |
Issue Summary
Hi,
As I reviewed planet code for adding API documentation, I found that there's probably something unsafe in planet_access() :
<?php
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:
<?php
/**
* 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:
<?php
// 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
$authorvariable 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
Comments
#1
It appears that reference to
'edit own blog'permission string is a souvenir of an old feature of planet module. So it's not a good pick now.Here's another version:
<?php
function planet_access($op, $node, $account) {
// @DIFFINFO using planet-dedicated permissions
// Planet administrator has all permissions related to planet module
if (user_access('administer planet', $account))
return TRUE;
// Users allowed to edit their own planet nodes, are also allowed to create them, others are not.
$author = user_access('edit own planet feeds', $account);
if ($op == 'create') {
return $author;
}
// Does current user own requested node ?
$own = $account->uid == $node->uid;
if ($op == 'view')
return $own || user_access('view all planet nodes');
if ($op == 'update' || $op == 'delete') {
return $author && $own; // ok if $author owns the node
}
trigger_error("Unknown operation requested: $op");
return FALSE;
}
?>
I used a new permission string, which I need to declare:
<?php/**
* Implementation of hook_perm - Defines user permissions.
* The suggested naming convention for permissions is "action_verb modulename".
*
* @return An array of permissions strings.
*/
function planet_perm() {
return array('administer planet', 'administer own planet feeds', 'view all planet nodes');
}
?>
#2
#3
This project is being abandoned by the maintainer (swe3tdave) in favor of a similar project that has many of the bugs in this module taken care of. Please try to udplanet module (http://drupal.org/project/udplanet) to see if this bug still exists. If it does please report it. This module is actively maintained and the Ubuntu-Drupal team that maintains it will make every effort to ensure the issue is resolved as quickly as possible.
Because this project is now abandoned, I and setting the Status to Won't Fix and removing anyone assigned to it.
Thanks,
MTecknology