Feature #227
closedRedirect to Login Page when Authorized page is requested without Login
0%
Description
Right now when someone requests a page that requires logins, for example their notifications, and they are not logged in (so they can't see the page) they are redirected to the home page.
This can be confusing for users. For example if you get an email with a friend request from someone it will have a link in the email to your notifications page. If you're not logged in and click the link they you will just get redirected to the home page.
Instead you should be redirected to the login page (with a message there ideally)and then after you login get redirected back to the page you were originally intending to go to.
Related issues
Updated by Boone Gorges almost 14 years ago
- Category name set to BuddyPress (misc)
- Target version set to 1.1
I spent a few minutes looking at this the other day and didn't get anywhere. But I think it's pretty important, and has the potential to be of real benefit to the WP/BP community more broadly if properly implemented.
Updated by Boone Gorges almost 14 years ago
- Status changed from New to Assigned
Hi Chris,
Have you had a chance to look more closely at this? I know that it's a big job, and if we were angling for a release around the end of October (let's say), we'd have to get started soon. No pressure, of course.
Updated by Chris Stein almost 14 years ago
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.
Updated by Boone Gorges over 13 years ago
- Target version changed from 1.1 to Future release
This isn't going to be feasible with the way that BuddyPress currently works. Many of the screen functions do a check at the very beginning, before rendering, and when the current user doesn't meet the check the function simply returns false. For us to fix this in the current version of BuddyPress would mean several dozen core hacks, which is not desirable or sustainable.
I'm hoping that the roles and capabilities overhaul promised in version 1.3 of BuddyPress can be coupled with an overhaul of the way that individual page provisioning works. If they all go through a central place, it'll be easy to add a hook that lets us determine the redirects and/or the error messages.
Updated by Chris Stein over 13 years ago
Thanks for the insight and keeping this alive. If you remember let me know when the buddypress update happens.
Chris
Updated by Boone Gorges over 13 years ago
- Target version changed from Future release to 1.3
This is going to take a lot of BuddyPress core refactoring, but I'm committing to giving it a go for 1.3.
Updated by Boone Gorges over 13 years ago
- Assignee changed from Chris Stein to Boone Gorges
Reassigning to myself because of the nature of the task.
Updated by Chris Stein over 13 years ago
Makes sense Boone. Thanks for taking this on. I think it will be helpful in the long run.
Updated by Boone Gorges over 13 years ago
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 :)
Updated by Raymond Hoh over 13 years ago
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!
Updated by Boone Gorges over 13 years ago
- Assignee changed from Boone Gorges to Raymond Hoh
Awesome!
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).
I assume you're proposing this so that we can get it done sooner rather than later? In the long run I don't like the idea of putting wp-login.php links in emails. BP URLs tend to be nice and pretty and semantic, and your suggestion will make 'em ugly. But if that's all we can do in the short run, then I think it'll be OK. Since that's an easy fix, let's hold off on it for now.
We could add an error message on wp-login.php as well. Will have to look into this.
I have not looked closely at it, but wp-login.php natively shows password errors, etc. I'm assuming it does this with a hook? If not, we can probably pass a patch upstream to WP to make it possible.
The only issue I see here is the WP branding on wp-login.php.
Right, that was my concern. Some people will be quite unhappy being redirected to a totally different-looking page. On the Commons, I hack wp-login.php to include the proper header and footer, but most people won't (and shouldn't have to) do this. Maybe a dedicated BP login screen is a solution, if the form action trick I suggest above doesn't work. Or, better yet, maybe someone can write a patch for WP that makes wp-login.php themeable. But that's a tall order :)
Ray, for posterity's sake, we should probably have discussions about how this will affect BP on BP Trac. I know there are a few open tickets on this issue - maybe you could just pick one to be the canonical ticket, and post general discussion there? Questions specific to our installation (or the occasional status update) can happen in this ticket.
I'm excited to have this one fixed!!
Updated by Raymond Hoh over 13 years ago
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.
Updated by Boone Gorges over 13 years ago
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.
Updated by Raymond Hoh over 13 years ago
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.
Updated by Raymond Hoh over 13 years ago
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!
Updated by Boone Gorges over 13 years ago
- Status changed from Assigned to Resolved
- Target version changed from 1.3 to 1.2.2
This is really slick, Ray. There are a few tweaks I'd make to the implementation when it goes into BP, but they're mainly stylistic. And, as far as BP goes, it's worth thinking about how this kind of thing can/should be integrated into BP's internal permissions. 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).
For the Commons, I'm going to move this back to the next bugfix milestone, as I think it's ready to go as is. We can iterate on some of the finer points, but the benefit of this patch is too great to put off until the next feature release.
I tweaked the login box message just a little bit for our installation.
Marking as resolved. Ray, we can continue general BP discussion on BP Trac. Iterations to the Commons implementation can get new, specific tickets. Thanks for your work on this!
Updated by Raymond Hoh over 13 years ago
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.
Updated by Boone Gorges over 13 years ago
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.
Updated by Raymond Hoh over 13 years ago
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.
Updated by Boone Gorges about 13 years ago
- Target version changed from 1.2.2 to 1.2.3
Updated by Boone Gorges about 13 years ago
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.