Skip to:
Content

Opened 6 years ago

Last modified 20 months ago

#2210 new defect

Auto-set role on Multisite creates unusable links in toolbar and my-sites.php

Reported by: boonebgorges Owned by:
Milestone: 2.8 Priority: normal
Severity: normal Version: 2.1
Component: API - Roles/Capabilities Keywords: dev-feedback
Cc:

Description

When _bbp_allow_global_access is set to true and bbPress is network-activated, logged-in users are automatically added to any site they visit in a network, with the bbPress default role of 'participant'. This feature causes unexpected behavior in various parts of WordPress.

For my example, user 'boone' has been auto-joined to site 'Foo'.

  • When viewing the site 'Foo', the 'Foo' top-level menu appears on the toolbar (the 'site-name' menu), as well as the Dashboard submenu item. Clicking on Dashboard leads you to a "You do not have permission..." error screen.
  • In the toolbar, 'Foo' now appoars as one of the 'My Sites' items. On a large network, this can make the menu very unwieldy; moreover, because of recent changes to get_blog_option() and the fact that switch_to_blog() is used liberally in building 'My Sites', it can easily cause memory timeouts when the My Sites menu gets large. (This is a separate WP bug, but is exacerabted by bbPress's behavior.) Also, the Dashboard menu is added to the 'Foo' flyout.
  • When 'boone' visits Dashboard > My Sites, 'Foo' is listed, along with a non-functional link to the Dashboard.

On large networks, the result can be user confusion and performance issues.

I've got a couple suggestions for how to approach the issue. All of the suggestions have problems, but one or more of them might be worth looking into.

  1. Since all of the problems listed above eventually trace back to get_blogs_of_user(), bbPress could filter that function to remove blogs where users are only Participants. This won't fix performance issues (the blog list is still built, and performance takes the hit at get_blog_details()), but at least it avoids potential confusion in the interface.
  2. Manually remove the problematic toolbar items after the toolbar is built. For example, for the 'site-name' menu:
function bbg_remove_dashboard_link_for_participants( $wp_admin_bar ) {
	if ( ! current_user_can( 'read' ) ) {
		// If the user can't read, it's almost certainly a bbPress
		// error. Remove all subnav items to the site-name
		foreach ( $wp_admin_bar->get_nodes() as $node => $node_object ) {
			if ( 'site-name' == $node_object->parent ) {
				$wp_admin_bar->remove_node( $node );
			}
		}
	}
}
add_action( 'admin_bar_menu', 'bbg_remove_dashboard_link_for_participants', 1000 );

Something similar could be done for the My Sites > Foo > Dashboard menu item. This doesn't address my-sites.php.

  1. Stop auto-adding users when they visit sites. Wait until a more opportune moment (when they first visit a forum page; when they first visit the New Forum Post page; when they first attempt to post something). This ensures that users are only added to blogs where they've demonstrated interest (though including them in "My Sites" still seems a bit misleading).

I know this issue is a tricky one to address, as you need to have the roles in order for bbPress to do a lot of work, and at least part of the problem is in the way get_blogs_of_user() works. (It would be nice, for instance, if you could noop it and supply your own checks, or if you could supply a minimum cap.) But I've already seen the issue crop up on three or four client sites, and my workarounds (such as the remove_node() snippet above) only address part of the problem.

Thanks for your consideration.

Attachments (1)

2210.01.patch (598 bytes) - added by r-a-y 20 months ago.

Download all attachments as: .zip

Change History (18)

#1 @johnjamesjacoby
6 years ago

This shouldn't be an issue in trunk any longer. Can you confirm this is fixed in trunk?

#2 @boonebgorges
6 years ago

I've been testing with trunk (r4764). Can you tell me the relevant changeset(s)? I can have a look to see if I'm missing something.

#3 @johnjamesjacoby
6 years ago

bbPress no longer hooks to 'switch_blog' the way it did in 2.2. Now, the stack is:

  • bbp_set_current_user_default_role is hooked to bbp_setup_current_user
  • bbp_setup_current_user is hooked to set_current_user
  • set_current_user is called in get_currentuserinfo()
  • get_currentuserinfo() is called in wp_get_current_user()
  • wp_get_current_user() is called all over the place, including switch_to_blog(), which might be where the problem is.

Unfortunately, a/b/c aren't options, because the problem that auto-role'ing solves is users not having adequate caps when (or rather before) the first visit to the site hits the template loader. We want the caps preloaded, so that toolbars and other cap checks are accurate.

We could theoretically check the filter stack, and skip bbp_set_current_user_default_role if it's being called after 'init'. That's how switch_to_blog() checks to see if the user should be re-initialized. But, this poses another issue of not knowing when the original blog has been restored, and whether or not to append the role/caps if necessary.

Basically, it stinks.

Fundamentally, this is a WordPress core issue with the WP_Roles global creation and reinitialization. That said, turning off "Auto role" in bbPress's forums settings is an easy way around this for now.

Last edited 6 years ago by johnjamesjacoby (previous) (diff)

#4 @boonebgorges
6 years ago

Thanks for the explanation, johnjamesjacoby.

I can see that my suggestion (c) is pretty extreme, for the reasons you describe. But the issue of dud Dashboard links is still pretty clunky IMO. If users *must* be added as Participants on first visiting the site, how about *also* adding them as Subscribers at the same time? Then at least they'd be able to visit wp-admin without getting an error message. Or if that's too intrusive (it seems pretty ugly to me), what about filtering out the Dashboard links as I suggest in (b)?

#5 @johnjamesjacoby
6 years ago

I think the answer is turning off auto-role. With auto-role off, guest users are still able to perform the new topic/reply actions of a normal participant, they just don't get automatically added as a user of the site. In a situation where bbPress is network activated, and you don't want users added to each site, turning off auto-role is the way to go.

#6 follow-up: @boonebgorges
6 years ago

I think the answer is turning off auto-role

I don't have a problem doing that for specific installations. But, just for the record, that doesn't change the fact that, when auto-role is turned on, the Dashboard links that appear are unusable, which is (at least based on the client feedback I've gotten) a very irksome UX problem.

May I suggest that auto-role be disabled by default?

#7 in reply to: ↑ 6 @johnjamesjacoby
6 years ago

  • Milestone changed from Awaiting Review to 2.4

Replying to boonebgorges:

May I suggest that auto-role be disabled by default?

See r4336. It was this way originally, but the general complaints in the forums were the opposite of yours. We could handle this differently for single/multisite, which is how bbPress 2.0 originally launched. The current iteration was done to simplify everything down into 1 set of common options.

I'm going to put this into the 2.4 milestone so we can continue the discussion, outside of the AR/2.3 scope.

#8 @johnjamesjacoby
6 years ago

  • Keywords early added
  • Milestone changed from 2.4 to 2.5

No time. Moving to 2.5. Tagging as early, since it's still an issue.

#9 @johnjamesjacoby
5 years ago

  • Component changed from General to Roles/Capabilities
  • Keywords reporter-feedback added; dev-feedback early removed
  • Milestone changed from 2.5 to 2.6

Moving to 2.6.

Can you investigate bbp_set_current_user_default_role() and whether or not it's being flushed by a call to switch_to_blog()? I suspect this happens directly as a result of #WP23016.

#10 @johnjamesjacoby
5 years ago

  • Milestone changed from 2.6 to 2.7

Bump to 2.7.

#11 @jbermudez
5 years ago

I just submitted a ticket that is somewhat related to this conversation and pertains to multisite installs. If I enable bbpress on an individual site that I want to restrict to members only, I'm finding that bbpress has already added the user to the site due to a wrong default parameter in the call to bbp_allow_global_access, which occurs early on in the action/filter hook sequence as it is tied to set_current_user (most member restriction plugins tie in to template_redirect). According to the @param for that function, the default value should be false. However, the $default parameter is being set to 1 (true). If one looks at the settings page on a site, by default they would think that since the "allow global access" checkbox is unchecked, this behavior wouldn't happen. It should be a simple fix to change the default for that function from 1 to 0, but if you like I can submit a patch.

Thanks!

#12 @jbermudez
5 years ago

So, scratch that comment about the default value for that function - apparently the default is for the setting to be turned on, so just the function comments needs to be cleaned up. That said, I'm running into the same problems as @boonebborges and simply disabling auto-role isn't enough. It looks like the dynamic "participant" capability that is granted in the case that auto-role is disabled doesn't carry through (perhaps due to switch_to_blog resetting caps?) so a site with auto-role disabled means that someone who is registered with a network, but is not a member of a given blog with bbpress enabled will get a 404 (triggered by the bbp_forum_enforce_blocked function since the user wouldn't have the spectate capability). This is not ideal behavior.

#13 @netweb
5 years ago

Related #2592

#14 @johnjamesjacoby
4 years ago

  • Milestone changed from 2.7 to 2.8

Bumping all 2.7 to 2.8 milestone.

#15 @r-a-y
20 months ago

  • Keywords dev-feedback added; reporter-feedback removed

Just ran into this issue.

The admin bar problem occurs when a user does not have a WordPress role such as 'subscriber' and when bbPress' auto-role option is enabled.

To duplicate:

  • Have bbPress auto-role enabled.
  • Edit any user in the WP admin dashboard and remove their WordPress role and bbPress role.
  • Login as this user and visit a page on the frontend.
  • bbPress will set their role to 'bbp_participant'.

However, this user does not have a WordPress role.

Attached patch adds the default WordPress role to the current user if one doesn't exist, which should fix this problem. This is basically what Boone notes in comment:4.

I'm not sure of any adverse effects with this approach, so could use some dev feedback here. Perhaps adding the WordPress default role in bbp_set_current_user_default_role() might be too early and might conflict with other plugins. Just a quick thought, might not be accurate.

Also, some might not want a visiting user to have the WordPress default role added on each sub-site. If that is the case, only make this addition to the root blog?

Note: This will not fix issues for existing users that have a bbPress role, but no WordPress role though. You will need to manually add a WordPress role to these users to fix this issue.

Last edited 20 months ago by r-a-y (previous) (diff)

@r-a-y
20 months ago

#16 @r-a-y
20 months ago

Related: In BuddyPress, there is a bug with the activation process on multisite installs where a user role isn't added after activation - https://buddypress.trac.wordpress.org/ticket/7565

However, this ticket is still relevant.

Last edited 20 months ago by r-a-y (previous) (diff)

#17 @netweb
20 months ago

FYI The vast majority of the 8 million+ users on w.org do not have a WordPress role assigned and I'm not sure they should be required to either...

Anyway something to consider, I'm not entirely sure myself...

Note: See TracTickets for help on using tickets.