Use access callback to restrict access to forums [#1496402]

I recently had to work on a project that used forum access and taxonomy menu together. The client wanted the menu produced to display only the forums the user had access to view. This would happen automatically if the access callback for the forum menu item checked access for each individual forum rather than returning a true or false for everything.

I've attached a patch which does this but wanted to check as well if there was a reason it wasn't done this way to begin with that I may not have thought of.

Comment	File	Size	Author
#8	forum_access-forum-specific-access-callback-1496402-8.patch	2.43 KB	Dean Reilly
#6	forum_access__forum_specific_access_callback__1496402.patch	2.06 KB	Dean Reilly
	forum_access.patch	1.45 KB	Dean Reilly

Comments

Comment #1

salvis

he/his

commented 23 March 2012 at 17:05

Normally, there is one "Forum" menu item, which is visible when at least one forum is visible and not otherwise.

Please post BEFORE and AFTER screenshots to explain what this is about.

Comment #2

Dean Reilly commented 23 March 2012 at 17:36

It might be better if I provide steps to recreate.

Say I want to create two forums called, "General Discussions" (accessible at /forum/general-discussion) and "Restricted" (accessible at /forum/restricted). General Discussions should be viewable by everyone and Restricted should only be viewable by a group of moderators.

Now I want to create a new menu which contains a link to each of these forums. The problem is that both links will show for normal users as when drupal evaluates the access callback it will return true because normal users do have access to the general discussions forum. Ideally what I would like to happen would be normal users would only see the General Discussions link where as moderators would see the General Discussions link and the Restricted forum link.

My patch changes the access callback to one which will take into account what forum the user is trying to view and return the correct value which allows the menu system to skip over those links which the user has no right to view.

Comment #3

salvis

he/his

commented 23 March 2012 at 18:03

Version:	6.x-1.x-dev	» 7.x-1.x-dev
Status:	Needs review	» Needs work

Yes, that makes sense and the patch looks good, but D7 needs to go first.

Comment #4

Dean Reilly commented 25 March 2012 at 18:16

Version:	7.x-1.x-dev	» 6.x-1.x-dev
Status:	Needs work	» Needs review

I took a look at porting this patch for D7 today but found it doesn't actually require it. The forum module in D7 specifcies a second forum menu router item (forum/%forum_forum) for specific forums and containers. The auto-loader function forum_forum_load tries to load the taxonomy term and will be prevented from doing so by the forum_access_query_term_access_alter() function in forum_access. When it can't load the term the _menu_load_objects() function will cause the site to return a 404 error. This has the same effects as my patch above.

I looked into reimplementing my patch above in a way similar to D7 as a lot of the hook_query_term_access_alter code lives in the similar hook_db_rewrite_sql in Forum Access 6.x. However it feels a little backward to define an autoload function for access control when there is already a specific function to handle this. I think it may also add additional overhead to the load times as what is loaded will never be used unlike in the D7 version but I haven't run any tests to confirm this suspiscion.

Given this, I'm going to return this ticket version and status to 6.x and needs review respectively.

Comment #5

salvis

he/his

commented 28 March 2012 at 10:23

Status:

Needs review

» Needs work

Thank you for the analysis and detailed explanation of the D7 situation!

On second look the docblocks in the patch need a little more love. It's cute how you split the existing block in two, but the two functions each should have their own full docblock, not just a half.

Comment #6

Dean Reilly commented 28 March 2012 at 21:11

Status:

Needs work

» Needs review

Status	File	Size
new	forum_access__forum_specific_access_callback__1496402.patch	2.06 KB

Good point, I've extended the access callback function's docblock to include a description and a reference to this issue. I've left the _forum_access_access_any_forum() docblock the same for now. Is there anything else you'd like to see in the commenting?

I made a few other changes too:

The access callback's name now follows the convention used in the D7 version.
No longer check for uid 1 in the access callback. This happens in both function calls made from the callback so there is no need to repeat it.
Removed a sentence from the hook_init docblock that no longer applies.

Comment #7

salvis

he/his

commented 28 March 2012 at 23:15

Status:

Needs review

» Needs work

Good changes, thanks!

+++ b/forum_access.module
@@ -602,14 +591,26 @@ function forum_access_menu_alter(&$items) {
 /**
  * Access callback for the 'forum' menu item.
  *
+ * Returns TRUE if the user has access to the specified forum or containter. If ¶
+ * no forum or container is specified will return TRUE if the user has at least ¶
+ * one role that can access at least one forum.
+ *
+ * See issue #1496402 for rationale of providing a forum specific access ¶
+ * callback and why it's not neccessary in Drupal 7.
+ */
+function _forum_access_forum_access_callback($tid = NULL) {
+  return (!$tid && _forum_access_access_any_forum()) || forum_access_access($tid, 'view');
+}
+
+/**
  * Returns TRUE if the user has at least one role that can access
  * at least one forum.
  */

You're still leaving _forum_access_access_any_forum() with only half of what it had before:

BEFORE:

/**
 * Access callback for the 'forum' menu item.
 *
 * Returns TRUE if the user has at least one role that can access
 * at least one forum.
 */
function _forum_access_access_any_forum($account = NULL) {

AFTER:

/**
 * Returns TRUE if the user has at least one role that can access
 * at least one forum.
 */
function _forum_access_access_any_forum($account = NULL) {

The summary line should be something like "Helper function to determine the access to the 'forum' menu item."

And for _forum_access_forum_access_callback() something like "Access callback for the 'forum' menu path."

+++ b/forum_access.module
@@ -602,14 +591,26 @@ function forum_access_menu_alter(&$items) {
+ * Returns TRUE if the user has access to the specified forum or containter. If ¶
+ * no forum or container is specified will return TRUE if the user has at least ¶

While you're at it, please remove the trailing spaces.

+++ b/forum_access.module
@@ -602,14 +591,26 @@ function forum_access_menu_alter(&$items) {
+ *
+ * See issue #1496402 for rationale of providing a forum specific access ¶
+ * callback and why it's not neccessary in Drupal 7.

We don't really need that reference. CHANGELOG.txt and the commit message will reference the issue as usual, that should be enough.

Sorry about being so picky...