Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redirect on renamed login page when logged in #109

Merged
merged 1 commit into from
Mar 31, 2021

Conversation

tvartom
Copy link
Contributor

@tvartom tvartom commented Mar 27, 2021

This PR change the redirect behavior when trying to access the renamed login-page, when the user is already logged in.

This makes it possible to use links like this:
https://www.example.com/my-custom-login?redirect_to=/protected-page/
which will work regardless if the user is logged in, or not.
It also makes it possible to change the behavior with the login_redirect filter.

Before:

  • All requests to the renamed login-page where directed to admin dashboard then the user where already logged in.

New behavior:

  • Honer the redirect_to GET-parameter, if supplied.
  • Use the login_redirect filter, which give other plugins power to customize this.

This tries to make it the same behavior as on normal login:
https://core.trac.wordpress.org/browser/tags/5.7/src/wp-login.php#L1154

@jakevoelcker
Copy link

I have been working on this issue as well!

Your suggested code fails for requests such as:
https://www.example.com/my-custom-admin/post.php?post=123&action=edit

Whereas stock WP works:
https://www.example.com/wp-admin/post.php?post=123&action=edit

As a solution, I have been testing the following code and it seems to work:

$dirname = untrailingslashit(pathinfo($parsed_url['path'], PATHINFO_DIRNAME));

if($dirname === $home_url_with_slug || (!get_option('permalink_structure') && isset($_GET[$login_slug]))){

#if(untrailingslashit($parsed_url['path']) === $home_url_with_slug || (!get_option('permalink_structure') && isset($_GET[$login_slug]))){
	if(empty($action) && is_user_logged_in()){
		//if user is already logged in but tries to access the renamed login page, send them to the dashboard
		AIOWPSecurity_Utility::redirect_to_url(AIOWPSEC_WP_URL."/wp-admin");                
	} else if(is_user_logged_in()) {
		//if user is already logged in but tries to access the renamed login page, and specifies an admin action, honour that admin action
		$redirect_string = "/wp-admin/".pathinfo($parsed_url['path'], PATHINFO_BASENAME)."?".$parsed_url['query'];
		wp_safe_redirect(AIOWPSEC_WP_URL.$redirect_string);
		die;
	} else {

Would it be possible to combine this with your pull request?
Or do you think it would be better if I start a new pull request?

@tvartom
Copy link
Contributor Author

tvartom commented Mar 28, 2021

From what I can see, your action-handler is a seperate else if-statement, which probably will not interfere with my update.
I haven't dug into the code deeper, and I don't know the diffrence to AIOWPSecurity_Utility::redirect_to_url and wp_safe_redirect either, so I can't tell which one to use. (I kept the AIOWPSecurity_Utility::redirect_to_url)

I don't think it would be necessary to use login_redirect-filter in your else if-statement, but I don't know what stock WP does.
I changed from hard-coded /wp-admin to admin_url() to mimic the behavior in wp-login.php.
admin_url() can also take a path as a parameter, which probably fits your query-building.

So since it is a separate issue, I think it is better to do a separate PR, and let the maintainer(s) decide on each issue.
Or combine them if it their wish.

@jakevoelcker
Copy link

Thanks.

I have tested your code and realised I can get it to work if I format the URL like this:
https://www.example.com/my-custom-login?redirect_to=%2Fwp-admin%2Fpost.php%3Fpost%3D123%26action%3Dedit

So I won't submit a separate PR, I'll just wait and hope your PR gets merged into the next release.

@Arsenal21 Arsenal21 merged commit 9316034 into Arsenal21:master Mar 31, 2021
@Arsenal21
Copy link
Owner

It looks good. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants