We had a need to limit access to specific roles for the 'XML sitemap' fieldset on the node_form. I am submitting a patch that contains an additional perm in xmlsitemap.module and in xmlsitemap_node.module I changed the generic Drupal perm to this new, xmlsitemap specific perm. Please review.

CommentFileSizeAuthor
#6 xmlsitemap-DRUPAL-6--2.patch1.56 KBjwhat
#3 xmlsitemap-DRUPAL-6--2.patch1.53 KBjwhat
xmlsitemap.patch1.1 KBjwhat

Comments

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Why removing the 'administer nodes' permission check? People with that permission should see all possible options on the node.

Status: Needs review » Needs work

The last submitted patch, xmlsitemap.patch, failed testing.

jwhat’s picture

jwhat commented
Version: 6.x-2.0-beta1 » 6.x-2.x-dev
Status: Needs work » Needs review
StatusFileSize
new xmlsitemap-DRUPAL-6--2.patch1.53 KB

Because 'administer nodes' is too generic for our needs. We have many different node types and not all roles have access to every node type. We need this more granular permission to control access for editing the xmlsitemap settings on the node form.

I recreated the patch via CVS on the proper dev branch and modified the Version tag on this issue accordingly.

Anonymous’s picture

Anonymous (not verified) commented

So add to the permissions, not remove the 'administer nodes'. 

array('administer nodes', 'edit xmlsitemap node')
Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work
jwhat’s picture

jwhat commented
Status: Needs work » Needs review
StatusFileSize
new xmlsitemap-DRUPAL-6--2.patch1.56 KB

It's actually not an access arguments array, it's a Form API #access property. How about a compromise where both permissions are required like such:

$form['xmlsitemap']['#access'] = user_access('administer nodes') && user_access('edit xmlsitemap node');

I've attached a patch reflecting this change, however I still believe the 'administer nodes' perm is too generic for this purpose.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

Oh, yea, I was confusing hook_menu with FAPI. The orignal used |= so as to not destroy a positive value already set.

So instead of

-    $form['xmlsitemap']['#access'] |= user_access('administer nodes');
+    $form['xmlsitemap']['#access'] = user_access('administer nodes') && user_access('edit xmlsitemap node');

You'll want

-    $form['xmlsitemap']['#access'] |= user_access('administer nodes');
+    $form['xmlsitemap']['#access'] |= user_access('administer nodes') | user_access('edit xmlsitemap node');
jwhat’s picture

jwhat commented

I don't believe that'll work for this case because that implies an OR relationship, and we don't want 'administer nodes' alone to provide access to these settings. That's why we want to introduce this new 'edit xmlsitemap node' permission.

jwhat’s picture

jwhat commented

I just realized why the 'administer nodes' permission cannot be used to limit the access to the xmlsitemap settings on the node_form. If we apply the 'administer nodes' permission, then we will no longer be able to limit users to specific content types, since 'administer nodes' is a global perm for all content types. I would like to suggest again that my patch in comment #3 solves this issue and enhances the flexibility of this module.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

I looked through core to see what it does when adding things to node add/edit forms.

Comment module does '#access' => user_access('administer comments')
Menu module does '#acccess' => user_access('administer menu')
Path module does '#access' => user_access('create url aliases')
Upload module does '#access' => user_access('upload files')
Translation module does '#access' => user_access('translate content')

So I would be ok with changing that fieldset to just controlled by 'administer xmlsitemap' and removing the check on 'administer nodes'. How does that sound?

jwhat’s picture

jwhat commented

That's not 100% what we were going for, but it'll definitely work. Thanks!

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Fixed

Fixed in CVS.
http://drupal.org/cvs?commit=372654
http://drupal.org/cvs?commit=372656

Status: Fixed » Closed (fixed)

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