Make WordPress Core

Opened 6 years ago

Last modified 4 years ago

#44550 assigned defect (bug)

The confirmaction page should also be in the user language

Reported by: birgire's profile birgire Owned by: garrett-eclipse's profile garrett-eclipse
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-screenshots needs-testing has-patch needs-refresh
Focuses: administration Cc:

Description

We have the ticket

Privacy: The user request email should be sent in the user language #43985

so it seems reasonable to also have the "User action confirmed" page, in the same user language as the email.

Namely the page the user sees after clicking:

https://example.com/wp-login.php?action=confirmaction&request_id=...&confirm_key=...

from the "Confirm Action" email.

See screenshot below:

Attachments (4)

confirmed-export-action-screen.jpg (23.0 KB) - added by birgire 6 years ago.
44550.diff (724 bytes) - added by desrosj 6 years ago.
44550.2.diff (952 bytes) - added by desrosj 6 years ago.
44550.3.diff (4.4 KB) - added by desrosj 6 years ago.

Download all attachments as: .zip

Change History (26)

@desrosj
6 years ago

#1 @desrosj
6 years ago

  • Keywords needs-testing has-patch added
  • Milestone changed from Awaiting Review to 4.9.9

44550.diff will display the confirmation page in the locale of the user.

To test:

  • Click a confirmation link for a user with a locale other than en_US (default).
  • Click a confirmation link for a user with en_US selected when the site's locale is not the default.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


6 years ago

#3 @desrosj
6 years ago

  • Keywords needs-unit-tests added

44550.2.diff is a different approach that I like better. It utilizes code already in wp-login.php that detects the wp_lang URL parameter. This is utilized for all of the other screens (password reset, password change, log out, etc.) on wp-login.php. It also uses restore_previous_locale() instead of restore_current_locale(). The latter restores the original locale and removes any switching history, but the former only reverts the most recent switch.

Tests for this need to be added after or as a part of #43985 (depending which is committed first), as that ticket adds tests for the entire wp_send_user_request() function.

Last edited 6 years ago by desrosj (previous) (diff)

#4 @desrosj
6 years ago

  • Owner set to desrosj
  • Status changed from new to assigned

@desrosj
6 years ago

#5 @desrosj
6 years ago

  • Keywords needs-unit-tests removed

44550.3.diff adds unit tests and adds consideration for when the user has specifically chosen a locale and always adds the wp_lang URL parameter in that scenario (even if it is the same as the site's locale). This ensures that if a user selects a preferred language, it will persist on the confirmation page even if the site's locale changes.

@desrosj
6 years ago

#6 follow-up: @birgire
6 years ago

Thank you @desrosj

Modifying the confirmation url seems to be relevant approach for this one.

Whether to include the locale info in the confirmation url:

  1. Unregistered users (uses site's locale): No
  2. Registered users, with locale set: Yes
  3. Registered users, without locale set (fallback as site's locale): No

I think this is contained in the tests in 44550.3.diff, that run successfully through phpunit on my install.

The confirmaction page in cases 1) and 3) follows any changes made on the site's locale.

An edge cases for 2): A registered user changes to locale Y, after receiving an email containing the confirmation url with a previous locale X.

I've not checked, but wonder if the user is logged in, if that should override the wp_lang value in the confirmation url. Maybe it is so?

Version 0, edited 6 years ago by birgire (next)

#7 in reply to: ↑ 6 ; follow-up: @desrosj
6 years ago

Replying to birgire:

An edge cases for 2): A registered user changes to locale Y, after receiving an email containing the confirmation url with a previous locale X.

I don't think that can be prevented unless we look up the locale for the request's user when the page is visited. But because the user would not be logged in at that point, I don't know that it should.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


6 years ago

#9 in reply to: ↑ 7 @garrett-eclipse
6 years ago

Replying to desrosj:

Replying to birgire:

An edge cases for 2): A registered user changes to locale Y, after receiving an email containing the confirmation url with a previous locale X.

I don't think that can be prevented unless we look up the locale for the request's user when the page is visited. But because the user would not be logged in at that point, I don't know that it should.

In this case if the user is logged out we can't determine that they changed their locale so would make sense to use the locale supplied in the emailed URL.

Replying to birgire:

I've not checked, but wonder if the user from 2) is logged in, if that should override the wp_lang value in the confirmation url. Maybe it is so?

But as mentioned in the case the user is logged in when they land on the confirmaction page then a check can be made against their current locale which I feel makes sense to override the language set on the URL.

#10 @desrosj
6 years ago

I did not realize that the wp-login.php screen actually set up the current user if a user was logged in. I just did some testing, and this is true. TIL.

The patch could be changed to something like the following:

if ( is_user_logged_in() ) {
	$lang = get_user_locale();
} elseif ( ! empty( $_GET['wp_lang'] ) ) {
	sanitize_text_field( $_GET['wp_lang'] );
}

This would also affect other wp-login.php pages, though (login form, forgotten password, etc.).

An edge case I can think of with this is when user A does not log out properly and user B visits the login screen. If user A uses a different locale than the site default, user B may be unable to use the page. There is #14949 which would redirect the user to the admin when they visit the login screen and are already logged in. This ticket would eliminate that edge case.

I am leaning towards a new ticket using the logged in user's locale on wp-login.php so that each scenario where a logged in user can access wp-login.php can be evaluated.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


5 years ago

#14 @pento
5 years ago

  • Milestone changed from 4.9.9 to Future Release

This ticket was mentioned in Slack in #core-privacy by webdevlaw. View the logs.


5 years ago

#16 @desrosj
5 years ago

  • Keywords needs-refresh added

This will need a refresh after #44758 / r43776.

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


5 years ago

#18 @desrosj
4 years ago

  • Owner desrosj deleted

#19 @garrett-eclipse
4 years ago

  • Milestone changed from Future Release to 5.5
  • Owner set to garrett-eclipse

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

#22 @davidbaumwald
4 years ago

  • Milestone changed from 5.5 to Future Release

This ticket hasn't seen any movement this cycle, so it's being moved to Future Release. If any maintainer or committer feels this can be resolved in time or wishes to assume ownership during a specific cycle, feel free to update the milestone.

Note: See TracTickets for help on using tickets.