-
Notifications
You must be signed in to change notification settings - Fork 270
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
Add cookie strategy configuration to PHP request handler #1753
base: trunk
Are you sure you want to change the base?
Add cookie strategy configuration to PHP request handler #1753
Conversation
@@ -348,6 +348,11 @@ async function run() { | |||
process.exit(0); | |||
} else { | |||
logger.log(`WordPress is running on ${absoluteUrl}`); | |||
if (args.login) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm starting to think we should just ship a WordPress plugin to log the user in the first time they interact with the site. It could go like this:
- The
login
step would install that mu-plugin. Or maybe the plugin would always be there andlogin
would just set some constant to activate it? - The plugin would check for a cookie like
wordpress_user_was_auto_logged
. If it's present, we're done. If it's missing, set it and also log the user in.
This should work in all runtimes (Studio, Playground CLI, WP-NOW, Playground webapp) without any runtime-specific loginc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude gave me this. Coming from AI, there's likely some problem with that code, but it seems like a good starting point:
<?php
/*
Plugin Name: Auto Admin Login
Description: Automatically logs in as admin if a specific cookie is not present
Version: 1.0
Author: Your Name
*/
// Function to check cookie and perform auto-login
function auto_admin_login() {
$cookie_name = 'wordpress_user_was_auto_logged';
// Check if the cookie exists
if (isset($_COOKIE[$cookie_name])) {
return;
}
// Cookie doesn't exist, set it
setcookie($cookie_name, '1', time() + (86400 * 30), '/'); // Cookie expires in 30 days
// Get the admin user
$admin_user = get_user_by('login', 'admin');
// If admin user exists, log them in
if ($admin_user) {
wp_set_current_user($admin_user->ID);
wp_set_auth_cookie($admin_user->ID);
do_action('wp_login', $admin_user->user_login, $admin_user);
}
}
// Hook the function to run on WordPress init
add_action('init', 'auto_admin_login');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good idea. However, I'm concerned about using a plugin in Studio's case, since we'd need to filter it out when exporting the site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I don't want to pollute the exports, filtering files out just doesn't work as a long term strategy. The login plugin would live in /internal/shared/mu-plugins
with the rest of the platform-level mu-plugins to keep the site clean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The login plugin would live in
/internal/shared/mu-plugins
with the rest of the platform-level mu-plugins to keep the site clean.
Ah, ok, I didn't know about this approach 😅.
Still, I wonder about how we could make this plugin work so the user can decide when it should auto-login and when not. If I understand correctly the code shared here, the init
hook will make that all requests are logged, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fluiddot here's my thinking:
- The runtime (Studio, Playground webapp etc.) decides whether or not the plugin is installed at any given time. The plugin is easy to install and easy to remove – it's just a php file.
- When the plugin is in place, it automatically logs the user in as an admin, or as another user when a constant / env variable like
AUTOLOGIN_USERNAME
is present. - Autologin sets auth cookies valid for a year, and an extra cookie called, say,
autologin_performed
- If the
autologin_performed
cookie is already set, no autologin happens.
This way the user will remain logged in until they explicitly log out, at which point they'll remain logged out until they explicitly log in. A private browsing session will still default to the logged in state.
There could also be a "querystring mode" where autologin is only performed when $_GET['playground-autologin']
is present. This would give Studio a link-based control while still allowing the Playground webapp and CLI to default to autologin.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach looks good to me, thanks @adamziel ! I'll try to work on this in the next few days. Note that the original issue lowered its priority, so I might delay updating the PR.
There could also be a "querystring mode" where autologin is only performed when
$_GET['playground-autologin']
is present. This would give Studio a link-based control while still allowing the Playground webapp and CLI to default to autologin.
At least for the Studio app, and probably when using node clients, this will be necessary to ensure that API requests to the site are not auto-logged. This is important when using, for instance, API keys as referenced in Automattic/studio#387.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A check like if(!$ajax && !$rest_api)
could also help solve for the API requests. Anyway, whatever's most useful here.
@fluiddot thank you so much! I left a few comments. This looks solid overall. |
Thank you so much @adamziel for reviewing the PR 🙇 ! I've addressed the comments you shared except the one related to using a plugin for the auto-login behavior. I wonder if you consider that a blocker to merge these changes, or if we could address it in a later-on PR. WDYT? |
Let's try to address the login plugin here if that's okay. If it turns out to be a time drain, I won't insist and let's just merge this PR, but it seems like a low-effort big win. |
680cd19
to
2e376d2
Compare
Motivation for the change, related issues
Related issue from the Studio app: Automattic/studio#387
Currently, cookies are persisted internally in the PHP request handler (reference). This ensures that cookies are not lost between requests when using Playground in environments that lack cookie management, like when embedding it in an IFrame.
However, when Playground is served via a server (e.g. running Playground CLI), this behavior implies that all accesses to the site will use the same cookies. For instance, if I log in to the site and then navigate to WP-Admin in incognito mode, it doesn't redirect to the login page because it's already authenticated. The same results are obtained when navigating using a different browser. This also entails the limitation of not being able to log in with different users on the site.
In this scenario, we don't need to persist cookies internally as they will be managed by the browser.
Implementation details
cookiesStrategy
) to the PHP requests handler that allows controlling whether the cookies will persist internally or not. The option is optional and will fall back to using the internal cookie store.boot
function of Playground CLI has been updated to ensure it avoids persisting cookies.Testing Instructions (or ideally a Blueprint)
Web
npm run dev
.http://127.0.0.1:5400/website-server/
).CLI
nx dev playground-cli server
.http://127.0.0.1:9400
).http://127.0.0.1:9400/wp-admin
).CLI (Auto-login)
nx dev playground-cli server --login
.http://127.0.0.1:9400/wp-admin/?playground-auto-login=true
).