Opened 12 months ago
Closed 5 months ago
#7349 closed defect (bug) (fixed)
When user is not logged , in he/she clicks email link to view new messages gets 404 page when should get login page
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | 2.9 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Core | Keywords: | has-patch |
Cc: |
Description
As described in this link: https://buddypress.org/support/topic/error-404-for-non-logged-in-users/
To test this scenario the user must not be logged in!
Then a use get an email saying they have a new message.
Example: website.com/members/melissaverton/messages/view/8/
The user clicks on the link and they are brought to a 404 page and not a login page... User must login -> You must log in to access the page you requested.
Last time this functionality was working in version 2.5.3
The issue is in file bp-core-catchuri.php
function bp_core_no_access
Attachments (4)
Change History (26)
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
12 months ago
#3
@dkelm
12 months ago
- Summary changed from When user is not logged, he/she clicks email link to view new messages gets 404 page when should get login page to When user is not logged , in he/she clicks email link to view new messages gets 404 page when should get login page
Not Logged In
#4
@slaFFik
12 months ago
- Keywords dev-feedback added; needs-patch needs-testing removed
- Milestone changed from 2.7.3 to Under Consideration
- Priority changed from highest to normal
- Severity changed from blocker to normal
- Version 2.7.2 deleted
#5
@boonebgorges
12 months ago
- Keywords needs-patch added; dev-feedback removed
- Milestone changed from Under Consideration to Future Release
I think we should fix this in a general way.
Currently, subnav items with the user_has_access=false flag are never actually added to the component nav. The way it *should* work (and the way that BP_Group_Extension works, to some extent) is:
- Separate the concept of 'access' (user can visit the tab) from the concept of 'visibility' (user can see the subnav item. user_has_access could maybe be a back-compat way of defining them both at the same time.
- When visibility=false, don't add the nav item (or add it, but don't render it - this is a harder job, but maybe better in the long run). *Do* continue to register the screen function.
- When access=false, the screen function should be bp_core_no_access() (or a wrapper with appropriate redirects).
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
10 months ago
#7
follow-up:
↓ 8
@dcavins
10 months ago
I've just added a patch that uses the strategy in the BP group email subscription plugin: Send all links to a message thread through the login form so that the user has an opportunity to log in if not logged in. If logged in, the user will be redirected to the thread directly.
I was surprised to find that links to the friend requests management pane work OK without modification, even though they also look like http://bptest.local/members/three/friends/requests/. Huh.
#8
in reply to:
↑ 7
@dcavins
10 months ago
Replying to dcavins:
I was surprised to find that links to the friend requests management pane work OK without modification, even though they also look like http://bptest.local/members/three/friends/requests/. Huh.
A note for my future self: It appears the difference is the main_nav arguments in the component setup. Messages includes the parameter: 'show_for_displayed_user' => bp_core_can_edit_settings(), but the friends component does not. When I comment out that parameter, instead of a 404 I get a login prompt.
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
10 months ago
#10
@dcavins
10 months ago
- Keywords has-patch added; needs-patch removed
- Milestone changed from Future Release to 2.9
This ticket was mentioned in Slack in #buddypress by hnla. View the logs.
7 months ago
#13
@r-a-y
7 months ago
ray.patch implements the following:
- New 'visibility' parameter in bp_core_new_nav_item(). If set to false, this allows for a nav item to be hidden, but the screen function to still be registered. I still need to add PHPDoc for this parameter.
- Adds the login redirector from BP Group Email Subscription as per 7349.2.patch. Also adds the auth parameter to bp_core_no_access() so we don't have to manually call it as in 7349.2.patch. We should maybe think about renaming the auth parameter to something else.
- For the message single thread screen, use bp_core_no_access() instead of redirecting to /members/X/messages/ if not authenticated. Also removes the redirect from messages_action_conversation() since this shouldn't be added in this function.
If we want to be more cautious, use ray.02.patch. It's the same as ray.patch, but removes the visibility parameter from bp_core_new_nav_item().
This still fixes viewing the message single thread if not logged in, but is a little more conservative.
For example, if you are not logged in and you visit example.com/members/X/messages, in ray.patch, you will get redirected to login; in ray.02.patch, it 404s as before.
#14
@r-a-y
7 months ago
- Keywords needs-testing removed
ray.03.patch is even simpler.
If no objections, I will commit this to fix the immediate issue, but I'd want feedback on the rest of the mods in ray.patch to see if they can be added for 2.9.
#15
@boonebgorges
7 months ago
7349.ray.03.patch definitely seems appropriate.
The more general 'visibility' approach seems sound to me, though I haven't tested it. We should be clear on back compat: it could cause privacy issues if we start registering screen functions that previously weren't registered because of their access settings. I'm pretty sure that's what your patch does.
I mentioned here that it'd be nice to continue to add visibility=false items to the nav array, but exclude them at the time of rendering. This change would open up a lot of possibilities beyond the scope of this ticket, since it'd then be possible to modify nav in dynamic ways that require access to all possible nav items. I don't think anything in ray.patch excludes us from moving in this direction in the future, but I wanted to put it out there.
Thanks for working on this!
Please be sure when you test that you are not logged into the system.
Click link from email -> You should be brought to login page with error message: "You must log in to access the page you requested."