XML Sitemap taxonomy term edit page

This fieldset can be seen on Taxonomy term's edit page even for users without 'administer xmlsitemap' permission (when user_access('administer xmlsitemap') returns FALSE), so I had to set my own module's weight to be higher than XML Sitemap module's weight, and then in a MYMODULE_form_taxonomy_form_term_alter(), putting in a 

$form['#after_build'][] = 'MYMODULE_form_taxonomy_form_term_alter_after_build';

and then in MYMODULE_form_taxonomy_form_term_alter_after_build(), hide it with the following code:

  if(!user_access('administer xmlsitemap')){
    unset($form['xmlsitemap']);
  }

But this should be done inside the module.

CommentFileSizeAuthor
#12 xmlsitemap-taxonomy-permission-1817124-6.patch1 KBmakangus
#6 xmlsitemap-taxonomy-permission-1817124-4.patch1.03 KBmakangus
#2 xmlsitemap-taxonomy-permission-1817124.patch1.76 KBmakangus
XML_Sitemap_Taxonomy_edit_page.png6.15 KBSk8erPeter
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Version: 7.x-2.0-rc1 » 7.x-2.x-dev

Thanks for the report.

makangus’s picture

makangus CreditAttribution: makangus commented
Status: Active » Needs review
FileSize
xmlsitemap-taxonomy-permission-1817124.patch1.76 KB

How about this? The patch adds permission checking on the taxonomy vocab and taxonomy term forms.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Assigned: Unassigned » Dave Reid
Status: Needs review » Reviewed & tested by the community

Dave this looks good to me, how about you?

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

I'm not sure why this is actually showing up. The function xmlsitemap_add_link_bundle_settings() specifically has this code:

  $form['xmlsitemap'] = array(
    '#type' => 'fieldset',
    '#title' => t('XML sitemap'),
    '#collapsible' => TRUE,
    '#collapsed' => TRUE,
    '#access' => user_access('administer xmlsitemap'),
    ....

Is another module ignoring the '#access' property of the fieldset?

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Assigned: Dave Reid » Unassigned
Status: Reviewed & tested by the community » Needs review
makangus’s picture

makangus CreditAttribution: makangus commented
FileSize
xmlsitemap-taxonomy-permission-1817124-4.patch1.03 KB

I think I found the issue. In xmlsitemap_add_form_link_options(), the #access translates to permission for "administer xmlsitemap" OR "administer taxonomy". It should be "administer xmlsitemap" AND "administer taxonomy" instead. Attached a patch, please let me know if that breaks other things.

Status: Needs review » Needs work

The last submitted patch, xmlsitemap-taxonomy-permission-1817124-4.patch, failed testing.

Sk8erPeter’s picture

Sk8erPeter CreditAttribution: Sk8erPeter commented

@makangus: I haven't tried applying this patch yet, but I think that's correct: the user who sees the mentioned fieldset HAS the "administer taxonomy" right, BUT does NOT have the "administer xmlsitemap" right, which means he shouldn't see the XML Sitemap settings. Thanks for creating the patch!

makangus’s picture

makangus CreditAttribution: makangus commented
Status: Needs work » Needs review

#6: xmlsitemap-taxonomy-permission-1817124-4.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, xmlsitemap-taxonomy-permission-1817124-4.patch, failed testing.

makangus’s picture

makangus CreditAttribution: makangus commented

Seems the the patch and simpletest pointed out another issue in xmlsitemap_menu. The second part of the #access xmlsitemap_link_bundle_access($bundle_info) becomes user_access('administer menus'), but the permission to check for should be 'administer menu' (without the s) instead.

makangus’s picture

makangus CreditAttribution: makangus commented
Status: Needs work » Needs review
FileSize
xmlsitemap-taxonomy-permission-1817124-6.patch1 KB

Just had a thought. The xmlsitemap_add_form_link_options() function is only called in hook_form_alter() functions, which means the user already has access to the form. I think we only need to check if the user has the 'administer xmlsitemap' permission for the fieldset. Thoughts?

Anonymous’s picture

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

Neither of these are correct. You need to concentrate on why xmlsitemap_link_bundle_access() is returning true. The or logic is correct.

makangus’s picture

makangus CreditAttribution: makangus commented

xmlsitemap_link_bundle_access() is returning true because the user has the access to 'Administer vocabularies and terms', but the user does not have access to 'Administer xmlsitemap'. Unless, it is by design that if the user has access to the taxonomy term he/she should have access to the xmlsitemap even without the 'Adminster xmlsitemap' permission.

My point for the last patch is, if the user does not have access to the taxonomy term, we wouldn't have gotten to the part where we need to check xmlsitemap_link_bundle_access(); on the other hand if the user does have access to the taxonomy term, we don't have to check xmlsitemap_link_bundle_access(), we already know the user has access.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

IMO, xmlsitemap_link_bundle_access() should check for "administer xmlsitemap" privileges and we drop the specific check for it in xmlsitemap_add_form_link_options(). One of the problems I have is that these are generic functions that affect more than just taxonomy and a bundle could have more fine grained permissions to not allow the xmlsitemap fieldset to show on their specific form entries.

makangus’s picture

makangus CreditAttribution: makangus commented

earnie, maybe I am not understanding the issue correctly. Could you explain to me why the original OR logic is correct? Don't we want to give permission only when a user has access to administer xmlsitemap AND access to administer the bundle. The OR logic means a user can access without the administer xmlsitemap permission as long as they can administer the bundle.

Sk8erPeter’s picture

Sk8erPeter CreditAttribution: Sk8erPeter commented

Unless, it is by design that if the user has access to the taxonomy term he/she should have access to the xmlsitemap even without the 'Adminster xmlsitemap' permission.

I don't understand why it would be good.
Isn't it in a contradiction with what you said later?

Don't we want to give permission only when a user has access to administer xmlsitemap AND access to administer the bundle. The OR logic means a user can access without the administer xmlsitemap permission as long as they can administer the bundle.

I agree with the bold quoted text. Which means the patch you created earlier should be fine in my opinion (haven't tried that yet).

makangus’s picture

makangus CreditAttribution: makangus commented

Right, the first quote I am describing what's happening(I said UNLESS IT IS BY DESIGN) and the second is what I think should happen.

Sk8erPeter’s picture

Sk8erPeter CreditAttribution: Sk8erPeter commented

Oh, OK, sorry, I misunderstood you. ;)
I agree with your opinion!

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

I've been on vacation, I need to review this further.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Issue summary: View changes

explaining how to hide this stuff