Currently the check to see if a role ID is valid is done in the menu access callbacks for TAXONOMY_ACCESS_CONFIG . '/role/%/edit' and TAXONOMY_ACCESS_CONFIG . '/role/%/delete'. This might be confusing, because it implies there's some actual content the user is allowed to access, when actually there's no content.

Instead, we should move these checks to page callbacks and return MENU_NOT_FOUND instead of FALSE. In fact, the access callbacks can probably be removed entirely and replaced with 'access arguments' => array('administer permissions'). The page callbacks would then return drupal_get_form('formname, args).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

For consistency, the page callbacks should be named taxonomy_access_formname_page(), and be in taxonomy_access.admin.inc.

sammyd56’s picture

I've had a go at getting my head around this, but haven't had much success as I'm not able to pass the variables correctly...

Warning: Missing argument 1 for taxonomy_access_admin_form_page() in taxonomy_access_admin_form_page() (line 67 of /var/www/drupal-dev/sites/all/modules/taxonomy_access/taxonomy_access.admin.inc).
Notice: Undefined variable: rid in taxonomy_access_admin_form_page() (line 76 of /var/www/drupal-dev/sites/all/modules/taxonomy_access/taxonomy_access.admin.inc).

Am I on the right track at least?

xjm’s picture

Status: Active » Needs work

This looks like a good start. It's not immediately obvious to me why you're getting that error messsage; the menu hook looks correct at first glance. I'll take a closer look later, but in the meanwhile, here's a couple other things to fix:

  1. +++ b/taxonomy_access.admin.incundefined
    @@ -61,6 +61,42 @@ function taxonomy_access_admin() {
    + * Page callbacks: Returns admin form if permissions are correct.
    

    Let's change this to "Page callback: Returns the role configuration form."

  2. +++ b/taxonomy_access.admin.incundefined
    @@ -61,6 +61,42 @@ function taxonomy_access_admin() {
    +
    +  // Allow access only if the user may administer permissions.
    +  if (!user_access('administer permissions')) {
    +    return FALSE;
    +  }
    +
    ...
    +  if (!user_access('administer permissions')) {
    +    return FALSE;
    +  }
    

    These two parts can be removed, because the access arguments will take care of that check.

  3. +++ b/taxonomy_access.admin.incundefined
    @@ -61,6 +61,42 @@ function taxonomy_access_admin() {
    +  // Do not render the form for invalid role IDs.
    

    Let's add this same comment to the same section in taxonomy_access_delete_role_page().

  4. +++ b/taxonomy_access.admin.incundefined
    @@ -61,6 +61,42 @@ function taxonomy_access_admin() {
    +  return drupal_get_form('taxonomy_access_admin_form', 5);
    ...
    +  return drupal_get_form('taxonomy_access_admin_delete_role', 5);
    

    For the second argument in these two function calls, it should be $rid rather than 5. (5 is just the number of the path component, and it will get passed in as $rid).

  5. +++ b/taxonomy_access.admin.incundefined
    @@ -61,6 +61,42 @@ function taxonomy_access_admin() {
    +function taxonomy_access_admin_delete_role_page($rid) {
    

    Similarly, let's add a docblock here that has the summary, "Page callback: Returns the role deletion confirmation form."

  6. +++ b/taxonomy_access.admin.incundefined
    @@ -61,6 +61,42 @@ function taxonomy_access_admin() {
    +    if (($rid == DRUPAL_ANONYMOUS_RID) || ($rid == DRUPAL_AUTHENTICATED_RID)) {
    

    Let's add a comment above this line that says "Do not allow access control to be disabled for the anonymous or authenticated user roles."

  7. +++ b/taxonomy_access.admin.incundefined
    @@ -61,6 +61,42 @@ function taxonomy_access_admin() {
    +    return FALSE;
    

    This line should also return MENU_NOT_FOUND rather than FALSE.

Finally, for the two page callbacks, let's add the path, the @param for rid, and the @see to our menu hook implementation. (See http://drupal.org/node/1354#menu-callback for an example.)

Thanks!

xjm’s picture

Oh--be sure to clear your site cache and reload once or twice each time you test a change.

sammyd56’s picture

Status: Needs work » Needs review
FileSize
3.91 KB

Updated as per #3, appears to be working fine now.

sammyd56’s picture

Oops, messed up the first comment block! This should be better...

xjm’s picture

Status: Needs review » Needs work

Cool, this looks correct now. Few more minor cleanup points:

  1. +++ b/taxonomy_access.admin.incundefined
    @@ -61,6 +61,59 @@ function taxonomy_access_admin() {
    + * TAXONOMY_ACCESS_CONFIG . /role/%/edit
    ...
    + * TAXONOMY_ACCESS_CONFIG . /role/%/delete
    

    These look good, but we should add the prefix Path: in front of them.

  2. +++ b/taxonomy_access.admin.incundefined
    @@ -61,6 +61,59 @@ function taxonomy_access_admin() {
    + * @see node_menu()
    ...
    + * @see node_menu()
    

    These should actually be taxonomy_access_menu(). :)

  3. +++ b/taxonomy_access.admin.incundefined
    @@ -61,6 +61,59 @@ function taxonomy_access_admin() {
    + */
    +
    +function taxonomy_access_admin_form_page($rid) {
    +
    ...
    +
    +}
    +
    +
    ...
    + */
    +
    ...
    +
    

    There's a few extra linebreaks here and there. (And possibly elsewhere in the module, but development is too active right now to clean other lines up.) For the lines added in the patch, there should be:

    • No linebreak between the docblock and the function name.
    • No linebreaks between the opening { and } and the function contents. (I prefer a linebreak before an inline comment generally, but they can be omitted at the top of the function.)
    • One linebreak between the function's closing } and the start of the next function's docblock.

Also, I think maybe we should put $rid = intval($rid); at the top of these, to sanitize the input immediately.

sammyd56’s picture

Status: Needs work » Needs review
FileSize
3.97 KB

Here's an updated patch. Sorry, have been trying to keep on top of coding standards but it's a lot to keep track of when you're starting out. Thanks for the pointers.

xjm’s picture

Cool, this looks great. I'll test it locally when I have a chance. If all goes well then we can add some automated tests as well, though I haven't thought through entirely everyhing they should test.

sammyd56’s picture

Oops, forgot to add an in-line comment in the second page callback.

In terms of tests, I guess we need to:

  • Assert that accessing /role/%/edit and /role/%/delete results in 404 for invalid user roles.
  • Assert that accessing /role/%/delete results in 404 for anonymous and authenticated user roles
  • Assert that accessing /role/%/edit and /role/%/delete returns the configuration form for valid user roles.
  • Assert that accessing /role/%/edit and /role/%/delete is forbidden for users without 'administer permissions' rights.

Anything I've missed?

xjm’s picture

Excellent, #10 covers it I think. For testing invalid roles, I'd try both numeric role IDs that don't exist, like 0 and something really big, 43993; and also a string ($this->randomName()).

sammyd56’s picture

Here's a first attempt at adding the tests. They seem to be working OK. Interdiff (#10-#12) included.

Status: Needs review » Needs work

The last submitted patch, interdiff-10-12-return_404_invalid_term_IDs-1366168.patch, failed testing.

sammyd56’s picture

I've opened a new issue for the exceptions: #1376064: Undefined index notices in role/%/edit title callback

xjm’s picture

Hmm, didn't expect that the title callback would be called before the page callback, but I guess it makes sense. I'd actually prefer to fix #1376064: Undefined index notices in role/%/edit title callback here, since it's not an issue if the access callback handles the request. How about this to fix those notices:

/**                                                                                                            
 * Title callback: Returns the title for the role edit form.                                                   
 */
function taxonomy_access_role_edit_title($rid) {
  $rid = intval($rid);
  $roles = _taxonomy_access_user_roles();
  if (empty($roles[$rid])) {
    return;
  }
  return t('Access rules for @role', array('@role' => $roles[$rid]));
}
xjm’s picture

+++ b/taxonomy_access.testundefined
@@ -486,6 +486,67 @@ class TaxonomyAccessConfigTest extends TaxonomyAccessTestCase {
+    // Try huge number (43993)
...
+    // Try 0
...
+    // Try string
...
+    // Visit role configuration form and role deletion confirmation form to
+    // verify that access is allowed.

Oh, minor thing, these comments should have articles and periods, e.g.:
// Try a string.

Also, I recommend giving interdiffs the extension .txt so that testbot doesn't try to test them. :) (Although, in this case, the interdiff was the same thing as the test-only patch to expose the fail, so that's good.) Thanks!

sammyd56’s picture

Status: Needs work » Needs review
FileSize
2.67 KB
8.15 KB

Updated patch

sammyd56’s picture

Issue summary: View changes

Updated issue summary.

rudolfbyker’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

This seems to work just fine.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: return_404_invalid_term_IDs-1366168-17.patch, failed testing.

The last submitted patch, 17: return_404_invalid_term_IDs-1366168-17.patch, failed testing.

nravens’s picture

This still seems to be an issue with the latest release. I'm using 7.x-1.0 and noticed that users who don't have permission to view content tagged with a TAC taxo get a 404 page not found message instead of a 403.

Is this fixed in the current dev version?