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

Add cookie strategy configuration to PHP request handler #1753

Open
wants to merge 9 commits into
base: trunk
Choose a base branch
from

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Sep 11, 2024

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

  • A new option has been added to the configuration (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.
  • The boot function of Playground CLI has been updated to ensure it avoids persisting cookies.
  • Since the auto-login will no longer work for Playground CLI, an alternative has been implemented that uses a query parameter to perform the auto-login.

Testing Instructions (or ideally a Blueprint)

Web

  • Run the command npm run dev.
  • Navigate to the website server (e.g. http://127.0.0.1:5400/website-server/).
  • Observe the site loads successfully.
  • Navigate to the WP-Admin.
  • Observe WP-Admin loads successfully.

CLI

  • Run the command nx dev playground-cli server.
  • Navigate to the site (e.g. http://127.0.0.1:9400).
  • Observe the site loads successfully.
  • Navigate to the WP-Admin dashboard (http://127.0.0.1:9400/wp-admin).
  • Observe it redirects to the login page.

CLI (Auto-login)

  • Run the command nx dev playground-cli server --login.
  • Observe that options to auto-login are logged in the terminal.
  • Navigate to the site (e.g. http://127.0.0.1:9400/wp-admin/?playground-auto-login=true).
  • Observe it auto-logins to the WP-Admin dashboard.
  • Navigate to other pages.
  • Observe that the user is still logged.
Captura de pantalla 2024-09-11 a las 18 18 57

@@ -348,6 +348,11 @@ async function run() {
process.exit(0);
} else {
logger.log(`WordPress is running on ${absoluteUrl}`);
if (args.login) {
Copy link
Collaborator

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:

  1. The login step would install that mu-plugin. Or maybe the plugin would always be there and login would just set some constant to activate it?
  2. 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.

Copy link
Collaborator

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');

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

@adamziel adamziel Sep 17, 2024

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.

@adamziel
Copy link
Collaborator

@fluiddot thank you so much! I left a few comments. This looks solid overall.

@fluiddot
Copy link
Contributor Author

@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?

@adamziel
Copy link
Collaborator

adamziel commented Sep 13, 2024

I wonder if you consider that a blocker to merge these changes, or if we could address it in a later-on PR.

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.

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.

2 participants