Motivation: I'm developing smartqueue_users.module, which provides a single subqueue per user. Nodequeue currently determines which users can manipulate (and and view) the subqueues in a queue based on user roles. I'd like to be able to define more granular control so that smartqueue_users can be configured to allow users to edit only their own subqueues, and not be able to see other subqueues to which they don't have access at the queue administration screen.

Implementation: This patch adds a new hook, nodequeue_reject_queue_access, which passes the complete queue or subqueue object (and an optional $account object) to the implementation of the hook by the queue\subqueue's owning module. The implementing module has the opportunity to specify a TRUE boolean value if the specified account or current user should not be allowed to manipulate or view the queue or subqueue that is specified.

It is important to prevent not only the queue manipulation forms from being displayed but also the individual links that these forms provide. nodequeue_reject_queue_access is also run inside of the presently existing nodequeue_queue_access function which is used to control access to the menu callbacks for queue administration forms. nodequeue_reject_queue_access is also run against the loaded subqueue object and used to define access in the menu callbacks for manipulating the subqueue. The function _nodequeue_add_ajax, which handles the menu callback from the path 'nodequeue/ajax/add' runs nodequeue_reject_queue_access internally.

Subqueues and queues from which the user is rejected are removed from the queue administration form to eliminate clutter and maintain the security of details about these subqeuues. Creating this patch I realized that queues that are configured to be manipulated by roles that the current user does not have will still show up for that user at admin/content/nodequeue. This patch could be re-rolled so that nodequeue.module automatically prevents those from being displayed.

A sample implementation of hook_nodequeue_reject_queue_access:

function smartqueue_users_reject_queue_access($queue, $account = NULL) {
  if (!$account) {
    global $user;
    $account = $user;
  }
  
  $access_all = (user_access("manipulate all queues", $account) || user_access('manipulate all user node queues', $account));
  $access_own = user_access('manipulate own user node queue');

  //If this is a subqueue
  if (!empty($queue->sqid)) {
    if (!($access_all || ($access_own && $account->uid == $queue->reference))) {
      return TRUE;
    }
  }
  
  //This is a queue, not a subqueue
  elseif (!($access_all || $access_own)) {
    return TRUE;
  }
}
CommentFileSizeAuthor
#18 nodequeue_access_4.patch5.31 KBezra-g
#12 nodequeue.access.patch5.09 KBsun
#7 nodequeue_access_3.patch5.28 KBezra-g
#5 nodequeue_access_2.patch5.11 KBezra-g
#2 nodequeue_access_1.patch5.09 KBezra-g
nodequeue_access.patch4.62 KBezra-g

Comments

merlinofchaos’s picture

merlinofchaos commented
Status: Active » Needs work

1) All smartqueue API calls are of the form nodequeue_api_* (or should be; those that aren't are in error).
2) the 'reject' reverses the usual logic for access and I mislike that. I'd prefer to see nodequeue_api_queue_access and have it default to TRUE. If the managing queue returns nothing (a NULL) translate that to a TRUE. It's pretty much the same thing and is more in line with existing access conventions.
3) I don't normally have queue and subqueue be interchangeable in functions. Instead I'm much more likely to use 2 different API calls, or make $queue and $subqueue both parameters, where $subqueue is optional.

Also, be sure to set patches to patch (code needs review) =)

ezra-g’s picture

ezra-g commented
Status: Needs work » Needs review
StatusFileSize
new nodequeue_access_1.patch5.09 KB

Thank you for the speedy review! This patch incorporates your feedback.

sirkitree’s picture

sirkitree commented

on line 1301 after the patch: this is an ajax call and i don't think you can return a drupal_not_found();

  if (!nodequeue_api_subqueue_access($subqueue)) {
    return drupal_not_found();
  }

You're going to have to do something similar to the other returns, maybe:

  if (!nodequeue_api_subqueue_access($subqueue)) {
    return array('error' => t('Invalid node'));
  }

Other then that I think this looks pretty good.

ezra-g’s picture

ezra-g commented

I thought of that when I wrote the patch, but thought it would be better to return drupal_not_found, since if someone without the proper access were trying to guess the url for a subqueue, an access denied message would reveal that a subqueue does in fact exist at that path. I can reroll though if that thinking is incorrect.

ezra-g’s picture

ezra-g commented
StatusFileSize
new nodequeue_access_2.patch5.11 KB

Here's a reroll incorporating the feedback from comment #3.

ezra-g’s picture

ezra-g commented
Status: Needs review » Needs work

In the process of trying to combine this with the patch in #265628: Malformed output due to missing queue access checks, I found that it causes errors when there is only one subqueue in a queue.

ezra-g’s picture

ezra-g commented
Status: Needs work » Needs review
StatusFileSize
new nodequeue_access_3.patch5.28 KB

Actually, the error was caused by an infinite loop when performing access checking on nodequeue-owned queues.
This patch corrects the error and incorporates the fixes from #265628: Malformed output due to missing queue access checks, one of which was not previously addressed by this patch and the other had some overlap.

Related to this patch, in nodequeue_view_queues(), it seems that the following code does not take into account cases where one queue will have multiple subqueues, since it will overwrite the $queue->subqueue value as it loops over each of the subqueues. If we wanted to take this patch a step further and have the number displayed in the 'subqueues' column at admin/content/nodequeue count only subqueues that the current user can manipulate, I could rewrite this part of the function and the related code, but it seems this a separate feature request.

// Relate all the subqueues we loaded back to our queues.
  foreach ($subqueues as $subqueue) {
    if (nodequeue_api_subqueue_access($subqueue, NULL, $queues[$subqueue->qid]));
      $queues[$subqueue->qid]->subqueue = $subqueue;
    }
sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Subscribing

merlinofchaos’s picture

merlinofchaos commented

ONly one thing jumps out at me:

- if (!user_access('manipulate queues')) {
+ if (!user_access('manipulate queues') || empty($queue->roles)) {

There's probably already a bug here, but I think manipulate all queues should pass anyway, right?

Also, we should doublecheck to see if manipulate all queues is being checked properly in this flow if it needs to be. manipulate all queues == UID 1, superuser access; regardless of role, UID1 can manipulate queues, though perhaps not necessarily subqueues. Need to be careful with that one.

ezra-g’s picture

ezra-g commented

I'm not sure I understand this comment.

With this patch users with manipulate all queues will still pass, since before that hunk there is:

 if (user_access('manipulate all queues')) {
    return TRUE;
 }

Also, unless I'm mistaken I don't think that there should be a case where UID 1 is denied access to any subqueue.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Category: feature » task
Status: Needs review » Reviewed & tested by the community

I tested this patch with regular node queues and it everything worked as it should. Wasn't able to test subqueues, because I have no setup for this available currently.

Since this patch also fixes #265628: Malformed output due to missing queue access checks, I'm changing category to task.

I agree with ezra-g that uid 1 should not play a special role in Nodequeue.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new nodequeue.access.patch5.09 KB

Well, just a minor code clean-up (missing leading spaces in comments, aso.)

ezra-g’s picture

ezra-g commented

My new IDE still keeps causing me to violate the coding standards. Thanks for catching that.

merlinofchaos’s picture

merlinofchaos commented

You'd be surprised just how many bug reports I received that UID 1 didn't have god access to the nodequeues. Same with Views. People just expect that superuser is super.

So I created 'manipulate all queues' with the intention of it having super user access to everything. We need to be sure to respect that.

It wasn't clear from the patch that manipulate all queues was there before that.

ezra-g’s picture

ezra-g commented

Does this need further revision?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Doesn't user_access() already account for special access handling for uid 1? Those "manage all xyz" permissions throughout Panels, Views, and NodeQueue are really confusing for users, which is why I created a (Views) patch in #258674: Access control not working / misleading permission to rename that permission to "access any view". Anyway, that's probably a different issue.

merlinofchaos’s picture

merlinofchaos commented

Sorry, my comment still stands. The || means that if '$queue->roles' is empty, it returns *regardless* of the results from user_access.

That's what I'm objecting to.

ezra-g’s picture

ezra-g commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new nodequeue_access_4.patch5.31 KB

This patch changes the offending line to:

if (!user_access('manipulate queues') || (empty($queue->roles) && $account->uid != 1)) {

ezra-g’s picture

ezra-g commented
Status: Needs review » Fixed

I just committed this. Will roll a release soon.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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