Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 7 years 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: dkelm's profile dkelm 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)

7349.1.patch (1.2 KB) - added by dcavins 8 years ago.
Route message inbox URLs through the login form.
7349.2.patch (2.8 KB) - added by dcavins 8 years ago.
If the user is already logged in, don't stop by the login screen.
7349.ray.03.patch (1.4 KB) - added by r-a-y 8 years ago.
7349.ray.patch (3.7 KB) - added by r-a-y 8 years ago.
Refreshed after recent commits.

Download all attachments as: .zip

Change History (26)

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


8 years ago

#2 @dkelm
8 years ago

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."

#3 @dkelm
8 years 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

Last edited 8 years ago by dkelm (previous) (diff)

#4 @slaFFik
8 years 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

There are so many ways to fix this, that I'm not really sure how, when and should we fix this.

Related: #6307, #7341 (closed by me as wontfix)

#5 @boonebgorges
8 years 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.


8 years ago

@dcavins
8 years ago

Route message inbox URLs through the login form.

#7 follow-up: @dcavins
8 years 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.

Last edited 8 years ago by dcavins (previous) (diff)

#8 in reply to: ↑ 7 @dcavins
8 years 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.


8 years ago

@dcavins
8 years ago

If the user is already logged in, don't stop by the login screen.

#10 @dcavins
8 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 2.9

#11 @hnla
8 years ago

  • Keywords needs-testing added

This ticket was mentioned in Slack in #buddypress by hnla. View the logs.


8 years ago

#13 @r-a-y
8 years 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.

Last edited 8 years ago by r-a-y (previous) (diff)

@r-a-y
8 years ago

#14 @r-a-y
8 years 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.

Last edited 8 years ago by r-a-y (previous) (diff)

#15 @boonebgorges
8 years 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!

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


8 years ago

#17 @r-a-y
8 years ago

In 11537:

Messages: Fix redirection issue when attempting to view a valid private message thread.

Previously, attempting to go to a valid private message thread URL when
logged out would throw a 404.

This commit properly redirects non-authenticated users attempting to view
a private message thread to the login screen. If a user is authenticated,
but is attempting to view a private message thread without access rights,
we redirect the user back to their own "Messages" page with a notice.

See #7349.

#18 @r-a-y
8 years ago

In 11538:

Messages: Add better UX when attempting to view an invalid thread.

This commit adds a notice when attempting to view an invalid thread for
logged-in users.

See #7349.

@r-a-y
8 years ago

Refreshed after recent commits.

This ticket was mentioned in Slack in #buddypress by hnla. View the logs.


7 years ago

#20 @hnla
7 years ago

@r-a-y Is this commit worthy, do we want to squeeze in so we can test between now a rc, or punt to 3.0?

#21 @r-a-y
7 years ago

In 11610:

Core: Redirect login links to their rightful spot if already authenticated.

Previously, if a logged-in user clicked on a login link with the
'redirect_to' parameter, the user would need to re-authenticate in order
to get redirected to the given URL.

This commit bypasses the re-authentication process for logged-in users,
which improves the user experience for those clicking on login links via
email. wp_safe_redirect() is used to avoid redirecting to external
links that are not whitelisted by WordPress.

See #7349.

#22 @r-a-y
7 years ago

  • Resolution set to fixed
  • Status changed from new to closed

Going to mark as fixed.

I've split the 'visibility' stuff into #7552.

Thanks for everyone's feedback on this ticket!

Note: See TracTickets for help on using tickets.