CUNY Academic Commons

Having a rough target date in mind is good. I'll take a look again at this later in the week and see if I get anywhere.

Thanks for the insight and keeping this alive. If you remember let me know when the buddypress update happens.
Chris

Makes sense Boone. Thanks for taking this on. I think it will be helpful in the long run.

Ray, I'm adding you as a watcher for this ticket, because I'd like you to consider taking on this project as a part of this release cycle.

Essentially, what I'm thinking is this. Right now, at the beginning of many of BP's screen functions, there are various permissions checks. When they are not met, the function sometimes returns false, and sometimes does nothing at all. Here's a simple example: http://buddypress.trac.wordpress.org/browser/tags/1.2.8/bp-groups.php#L724 This has the effect of bouncing the user back to the home page.

This behavior is especially annoying when coupled with email notifications. For instance, if you get a notification of a forum post in a private group, it has a link that says "Visit the site to respond..." with a link. If you click that link but are not logged in, you get redirected to the home page with no explanation, and no obvious way to get to the page you were trying to reach.

This is a problem that must be fixed in BP itself. Instead of returning false, those instances should do three things: (1) add an error message via bp_core_add_message(), (2) redirect the user to a page with a login screen, and (3) after login, redirect to the originally requested page.

WP already does this internally (if you try to visit an admin page directly, for instance). The complication is that many BP sites do not use wp-login.php as their login portal, but instead have login boxes on the home page and elsewhere on the site. (This is not an issue for the Commons, but I would like this fix to work for BP more generally.) This suggests a wrapper function that looks something like this:

function bp_no_access( $redirect_to = false, $message = false ) {
    bp_core_add_message( $message, 'error' );
    $login_url = apply_filters( 'bp_no_access_login_page',  wp_login_url( $redirect_to ), $redirect_to );
    bp_core_redirect( $login_url );
}

This would be the sort of function that would be called when the permissions requirements for the page in question are not meant. (Obviously you could have more logic here - maybe even an admin setting for $login_url in addition to a filter)

There may be a few more twists. I don't know (mainly because I'm lazy and have not tested :) ) whether the redirect_to url argument will work for BP login boxes that are not located on wp-login.php. My guess is that you'd probably need to add the redirect_to argument to the form action of the login form (since wp-login.php is used internally for the login), but this is just a guess. So that would be a template-level change in bp-default. Or it could be that we'd need to build our own version of redirect_to support for an arbitrary login page.

Ray, does this kind of project interest you? This'd be a chance to fix one of the most annoying UI issues on the Academic Commons, and also contribute to BP on CUNY's dime :)

Yeah sure! I'll take this on.

The most critical thing at the moment is email notifications.

I'd like to propose changing all email links so they link to wp_login_url($url) (basically the stop-gap solution I proposed here).

We could add an error message on wp-login.php as well. Will have to look into this.

The complication is that many BP sites do not use wp-login.php as their login portal, but instead have login boxes on the home page and elsewhere on the site.

I don't believe this is a problem because wp-login.php always exists no matter what. The only issue I see here is the WP branding on wp-login.php. In the future, if BP ever creates its own login screen, we'll be safe with wp_login_url() anyway. View the Theme My Login plugin as an example.

Then, I'll look at more detailed redirect access like what you proposed above. Will probably need to run some ideas by you here!

Just an update, I'm working on something that patches bp_core_catch_no_access() in BP 1.2, however in BP 1.3, this function doesn't exist. I'm wondering why.

Anyway, this led me down a rabbit hole in BP 1.3 and I'm about to post a bunch of tickets on BP Trac to fix group activity permalinks.

Thanks, Ray. I'm not sure why bp_core_catch_no_access() was taken out, but I did manage to trace it to http://buddypress.trac.wordpress.org/changeset/2863, which is the commit where Andy introduced bp-pages and the new installation wizard. (O fateful changeset!) At a glance, it's not obvious to me why it was taken out - presumably, something else in BP was thought to do the same thing, but I can't see at a glance what it is. I'm guessing it has something to do with a greater reliance on WP's built-in access functions, which you get for free when you use WP pages.

Also, please do let me know if the solution to this turns out to be radically different for BP 1.2 than for 1.3. I don't want to put enormous amounts of energy into a patch for 1.2.

My approach is definitely not radically diferrent.

For BP 1.3, I bring back bp_core_catch_no_access() and in that function, I use a variation of what you suggested here. (I'll add a ticket on BP Trac in a bit.)

For CUNY, I'm about to push an update that will not touch the core by adding my mods to bp-custom.php.

Adds login access to control to CUNY:
https://github.com/castiron/cac/commit/2dbea5cbf0e11fb48a2f3c66349c724a44463e17

Test this on cdev. Try navigating to any item that requires logging in eg. /settings/notifications/ or /groups/my-private-group/members/.
It will redirect to wp-login.php and on successful login, it will redirect to the page in question.

If you don't have access to that page, you will get redirected to the root with an error message.

Using bp_core_no_access() with "mode=1" will require a hidden input variable called "redirect_to". I did not add this to the commit because CUNY doesn't use BP's sidebar login widget.

Feedback appreciated!

Thanks, Ray! Great work.

Glad you thought the patch was okay!

Boone Gorges wrote:

For instance, when you are logged in and try to access an internal page (like a forum post or a BuddyPress Doc) of a group you don't have access to, you get bounced to the site root. Ideally, bp_core_no_access() would handle this kind of case too, though clearly the behavior would be different from sending people to wp-login.php (in the case of groups, it'd probably send them to the group's home page, where the 'this is a private group' message renders properly).

I thought about this. Right now, everything is filterable. So you could do checks on bp_current_component() or $bp global and then filter the redirect and message. I guess I should add group redirects by default in bp_core_no_access()!

Will add a commit for this on CAC in a bit.

Will add a commit for this on CAC in a bit.

Cool. But don't feel like you have to go to a ton of trouble for this. It should be pretty easy since it's just a single check for group access, but don't feel like you have to unhook and rehook a ton of core functions if that's what it takes. We can wait for it to be integrated into BP for the more extensive stuff.

Boone Gorges wrote:

It should be pretty easy since it's just a single check for group access, but don't feel like you have to unhook and rehook a ton of core functions if that's what it takes.

Yup, just a simple check to see if we're on a group page!

Adds better support for groups in bp_core_no_access():
https://github.com/castiron/cac/commit/881b5844c376e36e5aa3b6441175d6fe8b3bc2e6

In this commit, I'm using bp_current_component(); I'm aware that we should use bp_is_current_component() for BP 1.3.

Sorry for the delay in looking at this, Ray. The group redirect stuff is quite nice.

I noticed that the redirection process has the side effect of removing all URL parameters, which means that the redirect URL for some inner pages (like BuddyPress Docs) can be incorrect. This is fairly low priority, and I'll open another ticket for it.