Decouple "visibility" and "access" properties

[smninja]

I'm trying to explain this as broad as possible to allow for a more abstract solution but will explain my issue as a use case for the proposed decoupling of "visibility" and "access" properties of the group. This solution may not be optimal but it's a starting point for enhancing the behavior of a group's access.

In my project, I want "private" groups to allow group information to be read but prohibit open membership, requiring a user requests membership and be approved by an admin.

The "$visibility" variable used when extending BP_Group_Extension seems to infer the extension's tab will be visible to non-members @ line 1279:bp-groups-classes.php "Will this extension be visible to non-members of a group? Options: public/private" -- in practice, however, this does not seem to be the case.

I have traced this visibility issue back to line 264:bp-groups-loader.php where two actions are hard-coded as a check for user's access:

elseif ( !bp_is_current_action( 'home' ) && !bp_is_current_action( 'request-membership' ) ) {

I can edit this conditional, adding my own slug but this is is only a stopgap measure. In the template, I've added an elseif,

elseif ( bp_get_group_type() == __( "Private Group", "buddypress" ) ) :
   locate_template( array( 'groups/single/front.php' ), true );

In my exploration of this feature, it seems the definitions of "access" and "visibility" have been muddled together. Perhaps I'm misunderstanding but I wanted to open the topic up for discussion as I'm sure plugin authors and theme builders would have additional use cases for extending the scope of "private" groups.

[boonebgorges]

The "$visibility" variable used when extending BP_Group_Extension seems to infer the extension's tab will be visible to non-members @ line 1279:bp-groups-classes.php "Will this extension be visible to non-members of a group? Options: public/private" -- in practice, however, this does not seem to be the case.

If this is true, it's a bug in the intended implementation. The visibility property is supposed to hide the tab, and also supposed to block access to the tab via direct URL.

I will say, though, that 'private' in the sense of BP_Group_Extension::visibility is, by design, different from 'private' in the sense of private (vs hidden vs public) groups. Private group extensions have their display tabs visible to group members only. this is independent of more general group privacy levels. Are you suggesting that they be linked somehow?

I'll take some time to investigate further to see if I can reproduce what you're reporting, but it would be helpful if you posted a working example that demonstrates the bug. Can you post the parts of your code that are relevant to the issue at hand, and describe what you're seeing vs what you expect to be seeing?

[smninja]

There is confusion with the definition of "private" in the scope of Group vs. a Tab within the group. Yes, I am suggesting an enhancement to link or extend the Group's definition of private / access with that of a tab within the group.

The initial idea would be to allow extensions to set an "access" variable that correlates with Group status (private, public, hidden). Perhaps you have a better solution.

That said, I've included a test case below to further illustrate my request.

Description of Issue

Tab with $visibility variable marked as "public" fires 'bp_core_no_access()' if group is private.

Expected Behavior

If Tab is marked as "public", I would expect it to be publicly visible, even if group is marked "private".

Actual Behavior

User is shown an error message. Guest is redirected.

Test case:

1. Activate the test plugin below
2. Mark a group as "private"
3. Navigate to the slug of the tab, i.e. ​http://mywebsite.com/groups/mygroup/test

<?php 
/**
Plugin Name: BuddyPress Groups Test Tab
Description: Test case for demonstrating inaccessibility to a tab marked public when group is private.
Version: 0.6
Author: Tyler Mulligan
**/

function bptt_init() {

    class Groups_Test_Tab extends BP_Group_Extension {

        var $visibility = 'public'; // 'public' will show your extension to non-group members, 'private' means you have to be a member of the group to view your extension.
         
        var $enable_create_step = false; // If your extension does not need a creation step, set this to false
        var $enable_nav_item = true; // If your extension does not need a navigation item, set this to false
        var $enable_edit_item = true; // If your extension does not need an edit screen, set this to false
     
        function Groups_Test_Tab() {
            $this->name = "Test";
            $this->slug = "test";
            $this->create_step_position = 21;
            $this->nav_item_position = 31;
        }

        function display() {
            echo "test";
        }

        function edit_screen() {
            echo "test";
        }
    }

    bp_register_group_extension( 'Groups_Test_Tab' );

}

add_action( 'bp_include', 'bptt_init' );

?>

Thank you for taking the time to look into this.

[boonebgorges]

smninja - Thanks very much for the additional details.

As for the issue that the ticket originally addresses - whether the ideas of "visibility" and "access" are convoluted in BP groups - I'm not convinced that this is anything more than a semantic point. That's not to say it's not important - we want our nomenclature to be as transparent as possible, to make the dev learning curve as non-steep as possible. But fundamentally, the fact that we use words like 'visibility' and 'public' here is not at the root of the problem.

However, I'm in agreement that we should have a more sensible way of deciding which group tabs to show - we shouldn't be hardcoding the check like we are now. Also, I agree that it makes sense to allow plugins to put public tabs on private groups (though, fwiw, it should be done sparingly and with great documentation, because it threatens to wreck the distinction between public and private groups).

I'm putting this into 1.8 because I think that the minimal solution - replacing the hardcoded check in bp-groups-loader.php with something more sensitive to the params in BP_Group_Extension - is something we can fairly easily do, and something that could have a nice impact.

[smninja]

Great. Thanks for understanding the need and identifying the best way to move forward. I agree with everything you said.

[boonebgorges]

I've been thinking about this some more, and I don't think that we can safely remove the hardcoded check in the way I suggested above. It'll cause previously hidden content to become publicly available, which is a serious problem for existing installations. I'm going to move this ticket to Future Release, so that we can do more thinking about how to build better group tab visibility support into BP_Group_Extension.

[dcavins]

I ran into this problem and would like to propose a stopgap so that plugin authors can create publicly visible tabs in a private group. (In my case, I'm adding a publicly visible blog to a private workgroup so they can share some of their work.) I'll attach a patch that adds a filter in the group access control logic of bp-groups-loader. The plugin author can then filter the visibility:

add_filter( 'bp_groups_access_exemption', 'filter_group_plugin_pane_access', 20 );
function filter_group_plugin_pane_access( $setting ) {
	if ( bp_is_current_action('my-plugin-slug') ) {
		return true;
	} else {
		return $setting;
	}
}

The danger is that if the plugin author messes up the filter, all private items will be visible. There's probably a better way to do this in the long run, but this works on my test setup. I'm also attaching a second patch that I think is easier to read and uses the same filter for home and request-membership.

[dcavins]

Adds a filter to allow publicly visible panes in private groups.

[dcavins]

Adds a filter to allow publicly visible panes in private groups. Leaves 'home' and 'request-membership' hard coded (and protected).

[dcavins]

Adds a filter to allow publicly visible panes in private groups. Moves 'home' and 'request-membership' out to use the new filter.

[boonebgorges]

Thanks for the patches, dcavins. While I think they would work, I don't think it's a very straightforward solution - it'd be far clearer if one could pass a parameter into BP_Group_Extension::init() that'd perform this task. Let's do something along these lines for 2.0.

[dcavins]

Yep. I'd love to see this done the right way.

It's kind of a tricky concept. 'Visibility' currently seems to only apply to public groups (as mentioned above, setting visibility to "public" for a private or hidden group has no result).

Public group
visibility => public | non-members can view
visibility => private | group members only

Private or hidden groups
visibility => public | no possible audience
visibility => private | group members only

Do we need to add a new parameter or expand the visibility parameter we have? Would it make more sense to have two parameters, one that applies to public group situations and one that applies to private groups? (I'm throwing out hidden groups since they're implicitly private.) I can imagine plugins that should be:

• visible to every visitor, public or private group
• visible to logged-in visitors, public or private group
• visible to group members only, public or private group
• visibility should reflect group visibility (public for public groups, group members only for private groups)

I'm sort of thinking that two parameters has a nice symmetry:
'public_group_visibility' => [anyone, logged-in, group-members, group-admins]
'private_group_visibility' => [anyone, logged-in, group-members, group-admins]

Privacy is always hard, and it's easy to come up with overly complicated schemes. Thanks for considering this problem.

[dcavins]

I added a couple of options to the BP_Group_Extension init arguments. They work for setting the display of the menu items, but I'm not figuring out how to hook into the access check early in bp_groups_loader.php to allow a per-extension access setting.

It seems that the access control is set group-wide via the user_has_access variable. Can anyone suggest how I might make a check based on the requested extension's settings that early in the load process (before bp_groups_loader.php acts on user_has_access)?

Thanks for your help.

[boonebgorges]

I've spent some time with this patch. I like the idea behind it. But I think it's not going to be possible to implement without some fairly major changes to the way that group access protection currently works.

At the moment, group access protection happens in the BP_Groups_Component setup_globals() method: ​https://buddypress.trac.wordpress.org/browser/tags/1.9.2/bp-groups/bp-groups-loader.php#L256 If a user is deemed not to have access according to the logic laid out here, then the user is either given a 404 (in the case of hidden groups) or redirected somewhere. The problem is that this is happening long before any group extensions have had a chance to load themselves. Group extension plugins are only invoked at 'bp_init', and really are only bootstrapped at 'bp_actions:8'. ​https://buddypress.trac.wordpress.org/browser/tags/1.9.2/bp-groups/bp-groups-classes.php#L3831

We can't well load group extension before the groups component. So the only other option is to try moving group access protection much later in the process. I've played with this a bit in 4785.02.patch. But I'm having a hard time getting it to work with all cases - I get some odd redirects. It's possible that this is due to my being bleary-eyed at the moment, so I'm posting the patch in case anyone else wants to pick it up from here.

If we *do* do something like 02.patch, you'll see that then we have a central value - bp_groups_user_has_access - that we can filter in the group extension. I still think there is some confusion about what 'visibility' means next to the idea of 'access', but at least we'd be partway to something workable.

If dcavins or anyone wants to run with my updated patch, please be my guest. I'll leave this in the milestone for now, but in the absence of substantial patches, I don't think I'll be able to put in the necessary time to get this to happen for 2.0 - there are too many moving parts, and too many bits that need testing.

[dcavins]

Thanks for doing a whole bunch of work on this. I was able to address a few issues, and this seems to be working as expected for me. The weird 404s you were getting were a result of user_has_access() being a protected method; the filter was unable to find/use it so returned false (I didn't know this-> ​http://codex.wordpress.org/Function_Reference/add_filter "The function returns true if the attempted function hook succeeds or false if not.")

The setup still seems a little over-complicated, though. In order to display pages from a private group to the public, your plugin must set all three of

'visibility'        => 'public',
'enable_nav_item'   => true,
'access'            => array(
					'public'  => 'anyone',
					'private' => 'anyone',
					'hidden'  => 'members',
				)

Could we condense these options to two:

1. Should the plugin be active for this group? (replacing enable_nav_item, as it appears that it currently means "show the tab," but if it's false, you can't reach the plugin's display screen even if the plugin is active, and that value is used for user_has_access, too: ​https://buddypress.trac.wordpress.org/browser/tags/1.9.2/bp-groups/bp-groups-classes.php#L3032)
2. Who should have access to the plugin's display screen? (the new access array)

(I'm trying to imagine a situation in which you'd want to display a navigation item for a page the user can't visit. The flip side is a page that the user can visit by URL but that doesn't have a navigation item. Since those cases seem unusual, I think replacing enable_nav_item with enable_plugin makes sense, and we could use the enable_nav_item setting for that setting if it doesn't exist to provide backwards compatibility. I didn't do this in this patch, though, since it seems like a pretty big departure.)

I also used the new filter in setup_display_hooks() to help determine if the subnav item should be created and also what value user_has_access should have. Finally, I made some updates to the theme file groups/single/home.php because it was also doing access checks based on bp_group_is_visible().

This would be a pretty exciting change for me, so please offer comments and guidance and I'll be happy to help in any way I can. I'll be even more thrilled if we can arrive at a simpler setup that would work and be backwards compatible.

On a side note, what would setting a plugin's visibility to "private" actually do? ​https://buddypress.trac.wordpress.org/browser/tags/1.9.2/bp-groups/bp-groups-classes.php#L2800 Who could visit the plugin's display screen then?

[boonebgorges]

Thanks, dcavins. This looks promising. I'll have more time to spend with it after the 2.0 release. But in the meantime, I'll make the following observations:

• The existing visibility and enable_nav_item properties probably date back to 1.1, when BP_Group_Extension was originally introduced. It's likely that their behavior is inconsistent. I'm totally on board with taking this opportunity to deprecate these properties in favor of centralized properties that make more sense.
• But, it's going to be much easier to do this if we have unit test coverage for the current functionality. I think that the process of writing these tests will give us a clearer grasp of what the properties are *meant* to do and what they *actually* do, which will in turn help us decide how to consolidate them moving forward. If you can start to look at automated tests for the existing functionality, it'd get us a lot closer to making some of the proposed changes.

[dcavins]

Hi boonebgorges-

Sounds really good to me. I've not done any test-writing. Can you recommend a good read to get me started or point me to some simple examples in the BP test code?

One thing that I suspect will be fussy to test is that some of the access control checks are currently handled in theme files, so we'll have to figure out how to account for that.

Thanks for looking at this problem.

[boonebgorges]

Can you recommend a good read to get me started or point me to some simple examples in the BP test code?

I'll have to think that over. I have a feeling that the existing functionality is going to be pretty hard to test. The group extension objects are loaded into the global scope with no predictable name attached to them; much of the logic happens in template files; and load order is sorta unpredictable. Ideally, we'd have centralized methods for checking access, but that's one of the problems that needs to be fixed here :) I'll try to find some time to write a test or two that demonstrates the current functionality.

[boonebgorges]

In 8538:

Add some tests for BP_Group_Extension enable_nav_item and visibility

See #4785

[boonebgorges]

In r8538 I've started writing unit tests that codify the way things currently work.

In a public group, visibility does nothing, because everyone has access to it. enable_nav_item can still be used to toggle the nav item.

In a non-public group, setting visibility to private will cause the nav item not to be created for users who do not have access to the group (ie non-members).

Here's the tricky case: In a non-public group, setting visibility to public allows the setting of enable_nav_item to be respected. That is, visibility=public and enable_nav_item=true leads to the creation of a nav item (in buddypress()->bp_options_nav, even for users without access to the group. However, clicking on this nav item will lead (via BP_Groups_Component::setup_globals() to a redirect to wp-login.php. I guess the use case here is when you want to show that the subtab exists, so that people can click and then have the login redirect logic (ie, the content of the tab is private, but the existence of the tab is not).

Gotta run for now, but hopefully this is a starting point in getting backward compatibility stuff straightened out.

[boonebgorges]

4785.04.patch is a first pass at fixing this API. High-level overview:

• Group access control has been moved out of BP_Groups_Component and into a separate function bp_groups_group_access_protection(). Running it late means that BP_Group_Extension plugins (and other plugins as well) can modify the default logic. (Separate idea: it'd be cool if BP's own subpages - like 'request-membership' - were registered using BP_Group_Extension. But at least for now, access to core pages can be customized with the 'bp_group_user_has_access' filter.)
• Deprecate the visibility and enable_nav_item params for BP_Group_Extension. In their place, introduce access (who is able to visit the tab) and show_tab (who is able to *see* the tab). Each param takes the following possible values: 'anyone', 'loggedin', 'member', 'mod', 'admin', 'noone', or any combination thereof in an array. Backward compatibility for visibility and enable_nav_item is provided. See the attached plugin bp4785.php for some examples, but in brief, the logic works like this:

// No 'access' or 'show_tab' defined:
//   - In public groups, tabs are fully publick
//   - In non-public groups, tabs and their content are members-only
// For most plugins, this will probably be the desired setup, so it's the default.
parent::init( array(
    'name' => 'Foo',
    'slug' => 'foo',
) );

// 'Access' set but 'show_tab' not set. 
// 'show_tab' defaults to the value of 'access'
parent::init( array(
    'name' => 'Foo',
    'slug' => 'foo',
    'access' => 'member',
) );

// In some cases, you may want to show the tab to all users but limit access to the tab to members
// When a non-member clicks on the tab, she'll go to a wp-login.php redirect
parent::init( array(
    'name' => 'Foo',
    'slug' => 'foo',
    'access' => 'member',
    'show_tab' => 'anyone',
) );

Backward compatibility is a bit tricky because the visibility and enable_nav_item features are implemented in such an inconsistent and confusing way. But, in essence, they translate like this:

// 1. This
'enable_nav_item' => true,
'visibility' => 'public', 

// becomes this for public groups
'access' => 'anyone',
'show_tab' => 'anyone',

// and this for private groups
'access' => 'members',
'show_tab' => 'anyone', // this is a quirk of the current implementation

// 2. This
'enable_nav_item' => true,
'visibility' => 'private',

// becomes for public groups
'access' => 'anyone',
'show_tab' => 'anyone', // another quirk of the current implementation

// and for private groups
'access' => 'members',
'show_tab' => 'members',

Users who want more sophisticated logic can override the user_can_visit() and user_can_see_nav_item() methods in their BP_Group_Extension implementations. See BP4785_Ext6 in the attached plugin.

Summary: I think that the 'access' + 'show_tab' API is much clearer and much more flexible than the old one. And between the attached unit tests and the sample plugin, I think I've done about as much as possible to ensure backward compatibility. Looking forward to feedback and thoughts from other devs.

[boonebgorges]

Plugin for testing various BP_Group_Extension visibility params

[dcavins]

Excellent. Reading through the patch, I found the logic easy to follow and I like all the spots where plugin authors can influence the outcome of the checks. So, are you thinking that if a plugin wants to mix and match settings based on group visibility, like

'access' => array( 
 	'public'  => 'anyone', 
 	'private' => 'anyone', 
 	'hidden'  => 'members', 
        ),

it should manage that by overriding user_can_visit() and user_can_see_nav_item()?

Thanks for all the work on this.

[boonebgorges]

So, are you thinking that if a plugin wants to mix and match settings based on group visibility ... should manage that by overriding user_can_visit() and user_can_see_nav_item()?

Yes. I'd originally written the patch with the group status settings, as you'd originally suggested. But it made it pretty complex to register a simple group extension. So I got to thinking about how often this would actually happen, and I guess I figured that in those relatively rare cases, it makes sense for plugin authors to write the logic themselves, rather than overcomplicating the API for the majority of users.

dcavins - Did you have a look at the example extensions in bp4785.php? They're maybe of the most interest to someone like you who's writing plugins. I want to have a sense if the new way of registering access settings makes sense to someone building extensions.

[dcavins]

Replying to boonebgorges:

I agree that mixed access settings based on group status is not the norm, and also agree that a completely flexible approach (overriding user_can_visit() and user_can_see_nav_item()) will handle those cases. I think most plugins won't need to specify anything and can rely on your defaults (public groups => anyone, private and hidden => members). We can add an example to the Group Extension API page with an example switch based on group status and user membership role to show a more complicated example.

Yes, I looked at the example extensions and like the directness of the new arguments. I mean, it loses some of the mystery and wonder for the developer, but I think everyone'll adjust. :)

I'll test this patch asap. Thanks again.

[dcavins]

Don't show site admins every possible tab.

[dcavins]

A quick comment: The base functions user_can_see_nav_item( ) and user_can_visit() were showing every tab to site admins, including those that didn't apply to the current group. I've clamped them down to not display if the parameter is set to "noone."

Which reminds me that we'll need to add to the API an example of how to add a function for enabling plugins per group (I'm guessing you'd want to set the access level to something other than "noone" for "enabled" or "noone" for "disabled" based on the group id.)

[dcavins]

Include "noone" access and show_tab cases

[boonebgorges]

Don't show site admins every possible tab.

Yes, this looks reasonable to me.

Include "noone" access and show_tab cases

Sweet, looks great.

Which reminds me that we'll need to add to the API an example of how to add a function for enabling plugins per group (I'm guessing you'd want to set the access level to something other than "noone" for "enabled" or "noone" for "disabled" based on the group id.)

The changes in this ticket aren't really designed to enable this. The changes being considered here are really relevant only to the display() method of BP_Group_Extension plugins; group-specific stuff would ideally happen in a more general way than that. See #2673 for background. That said, it would be possible to do what you're suggesting using custom user_can_see_nav_item() and user_can_visit() methods (something like: return in_array( $this->group_id, $array_of_groups_that_should_see_this );

[dcavins]

Replying to boonebgorges:

See #2673 for background.

I didn't realize that #2673 was about to land (but was thrilled when I read imath's comment reopening it). That's tremendously great news. Does that mean that 2.1 is a groups development overhaul release? :)

[boonebgorges]

Whoa whoa, I don't know about it being ready to land. All I'm saying is that the general issue of per-group extensions is better handled at that sort of level. We'll see what happens for 2.1.

[imath]

Hi, first: great work boonebgorges & dcavins !!

Then some feedbacks, i merged 04,05 & 06 patches and I think there are a few minor troubles with the patches.

Before applying the patches, if on a private group and trying to access to the group members by editing the browser url eg : site.url/groups/group-slug/members, i'm redirected to group's home.

1/ After applying the patch, i'm redirected to '', if testing on a MAMP/WAMP dev environnement i'm arriving on the localhost root. Else the user should land on the site's front page i guess.

2/ Testing the different group extensions in bp4785.php, everything seems to work as expected as soon as the group is private. If the group is public and trying to access to BP4785_Ext4 for instance, i'm redirected to the login.php file even if i'm already loggedin, it's then an endless redirection to login.php

3/ I might be wrong, but i think in BP_Group_Extension->user_can_see_nav_item() the check should be done on the $this->params['show_tab'] and in BP_Group_Extension->user_can_visit() the check should be done on $this->params['access']

In 4785.07.patch, I've tried to fix the problems, 1/ seems to find its origin in bp-core-buddybar.php and 2/ in the function bp_groups_group_access_protection() after the bp_group_user_has_access filter.

Now, this is great for group extensions, but it doesn't seem to work if a plugin tries to force the display of the group members for a private group for instance. I've managed to show the members tab but i didn't manage to display the group members, i'm always redirected to group home. So, unless i didn't understand well, this doesn't seem to be possible :

But at least for now, access to core pages can be customized with the 'bp_group_user_has_access' filter

Though, I might do it wrong.. So i'll try some other tests.

[boonebgorges]

imath - Many thanks for having a close look at the patch. 4785.08.patch has some revisions.

2/ Testing the different group extensions in bp4785.php, everything seems to work as expected as soon as the group is private. If the group is public and trying to access to BP4785_Ext4 for instance, i'm redirected to the login.php file even if i'm already loggedin, it's then an endless redirection to login.php

Ah. I'd only been testing with logged-out users. At its root, the problem is with bp_core_no_access(), which has default redirect arguments related to wp-login.php. I've chosen to handle this in a more modular way than you've suggested: I pass the $no_access_args to the 'bp_group_user_has_access' filter, and then modify them in the callback (see the new BP_Group_Extension::group_access_protection()). I think there is actually a more fundamental problem with bp_core_no_access() here (it should have its own failsafes against this kind of thing); I'll open a separate ticket and bring it to the attention of r-a-y.

3/ I might be wrong, but i think in BP_Group_Extension->user_can_see_nav_item() the check should be done on the $this->paramsshow_tab? and in BP_Group_Extension->user_can_visit() the check should be done on $this->paramsaccess?

Yes. Good catch.

1/ After applying the patch, i'm redirected to , if testing on a MAMP/WAMP dev environnement i'm arriving on the localhost root. Else the user should land on the site's front page i guess.

This looks like a very ugly problem, and your proposed solution is very ugly too :) It's amazing to me that we're only now discovering this problem, given that we've used bp_core_new_subnav_item() for BP_Group_Extension forever. Can you give details on how to reproduce your problem? I think we need to take a deeper look at the root cause instead of writing fixes that are too specific.

[boonebgorges]

Now, this is great for group extensions, but it doesn't seem to work if a plugin tries to force the display of the group members for a private group for instance. I've managed to show the members tab but i didn't manage to display the group members, i'm always redirected to group home.

Sorry, forgot to respond to this. Just to be clear: the current fixes are not designed to solve any problems with core tabs. It's an enhancement to BP_Group_Extension. I was just hypothesizing that it might be possible to apply this to something like the 'members' tab, by returning true out of the 'bp_group_user_has_access' filter. It might turn out that this is not possible, I haven't checked :)

[imath]

Replying to boonebgorges:

This looks like a very ugly problem, and your proposed solution is very ugly too :) It's amazing to me that we're only now discovering this problem, given that we've used bp_core_new_subnav_item() for BP_Group_Extension forever.

I agree about my solution :) I think, we're discovering this problem now because, before the group access logic was managed before any nav was set. So we were redirected before this could happen. bp_core_new_subnav_item() seems to mainly be designed for user nav, as, if no access, the logged in user is at the end redirected to bp_displayed_user_domain(). In case of a group, as we're not seeing a user the displayed user id is not set and the redirect url ends up to nothing ''

Can you give details on how to reproduce your problem? I think we need to take a deeper look at the root cause instead of writing fixes that are too specific.

No problem, you just need to display a private group being logged in (and not a member of it). Then add members at the end of the url. If you do it without applying the patch you're redirected to group's home, if you do it after, you're redirected to ''

the current fixes are not designed to solve any problems with core tabs It's an enhancement to BP_Group_Extension. I was just hypothesizing that it might be possible to apply this to something like the 'members' tab..

Yes this is a great enhancement for group extensions ;) I think it might be not possible unless we can filter core tabs user access argument before parent::subnav() is called in BP Groups loader.

[boonebgorges]

4785.09.patch includes the 'no_access_url' enhancement from #5720. BP_Group_Extension subnav items are now registered using 'no_access_url = $group_link', as are the subnav items registered by BP_Groups_Component::setup_nav(). This should fix the redirect problems uncovered by imath above.

Testing and thoughts welcome. Thanks.

[dcavins]

Hi Boone-

I can't get the public tab in a private group case to work with a logged-in, non-group-member user (or logged-out user). At the end of BP_Group_Extension::setup_access_settings, my parameters look good:

[enable_nav_item] => 1
[visibility] => public
[user_can_see_nav_item] => 1
[user_can_visit] => 1

but when I attempt to navigate to the plugin's display screen, I'm bounced back to the group's front page. In bp-groups/bp-groups-actions.php::bp_groups_group_access_protection(), $user_has_access is true after line 72, so I'm not sure why access is being prevented.

Also, in bp_core_maybe_hook_new_subnav_screen_function(), $subnav_item['user_has_access'] is true.

Similarly, as a non-logged-in user, I'm redirected to the login page. I've deactivated nearly every plugin and am using a non-BP theme.

It seems like everything is working correctly, but I'm not getting access. What can I do to help troubleshoot?

I noticed another thing while re-reading the code that I think imath found earlier:

        public function user_can_visit( $user_can_visit = false ) { 
 	3496	                if ( 'noone' !== $this->params['show_tab'] && current_user_can( 'bp_moderate' ) ) { 
 	3497	                        return true; 
 	3498	                } 
 	3499	 
 	3500	                return $this->user_can_visit; 
 	3501	        } 

I think the first check should be 'noone' !== $this->params['access'] ...

[boonebgorges]

dcavins - Thanks for testing. 4785.10.patch corrects the user_can_visit() typo.

I can't reproduce the redirect issue that you're describing. Check out the sample plugin: ​https://buddypress.trac.wordpress.org/attachment/ticket/4785/bp4785.php - Ext2 and Ext3 should correspond to what you're testing ("public tab in a private group"), and they're working for me locally. Can you post the code that you're using to test?

If you want to troubleshoot more locally, you could try dropping some sort of debugger into wp_redirect() - maybe something like var_dump( debug_backtrace() ); die();

[boonebgorges]

As we're nearing the end of the dev cycle, and as I can't reproduce dcavins's bug locally, I'm going to commit these changes. If there are problems, they should come up during the beta.

[boonebgorges]

In 8605:

Overhaul access and visibility control for group tabs

Previously, access control to group tabs was handled in two ways:

• for BP_Group_Extension tabs, the 'enable_nav_item' and 'visibility' provided some control over access to plugin developers, though it was inconsistent, buggy, and difficult to implement properly
• for tabs provided by bp-groups, access to the tabs of non-public groups was controlled directly in the BP_Groups_Component::setup_globals() method

Aside from being unclear for developers, this technique for controlling access
was also inflexible. For non-public groups, tab access was hardcoded and
handled before BP_Group_Extension plugins even had a chance to load. As a
result, it was essentially impossible to add public tabs to non-public groups
(among other non-standard customizations).

The current changeset comprises a number of changes that make tab access more
consistent and flexible:

• Access control is moved to the new bp_groups_group_access_protection() function. This function has the necessary filters to customize access protection in arbitrary ways. And because it loads at 'bp_actions' - just before the page begins to render - all extensions have had a chance to load and register themselves with the desired access settings.
• The 'visibility' and 'enable_nav_item' properties of BP_Group_Extension are phased out in favor of 'access' and 'show_tab' params. 'access' controls who can visit the tab, while 'show_tab' controls who can see the item in the navigation. These new properties have intelligent defaults (based on the privacy level of the group), but can be overridden with a number of custom settings: 'admin', 'mod', 'member', 'loggedin', 'anyone', or 'noone'. Backward compatibility is maintained, so that existing BP_Group_Extension plugins that use enable_nav_item or visibility will continue to work as before.

Fixes #4785

Props boonebgorges, dcavins, imath

[dcavins]

Ah, this new patch doesn't work if BP Group Hierarchy is activated. I'll look into what is happening, but wanted to update my earlier comment with what appears to be the cause of my problems.

Thanks for working on this. I'm really looking forward to using the new system in my plugins.

[r-a-y]

In 8827:

Do not process BP_Group_Extension::setup_access_settings() if no group ID is available.

If no group ID is available, the BP_Group_Extension::setup_access_settings()
method should not do its checks. This is to avoid running DB queries
unnecessarily. (See #4785.)

Fixes #5812.