Connect existing WordPress accounts to Google from the profile page by shervElmi · Pull Request #12757 · google/site-kit-wp

shervElmi · 2026-05-20T10:07:03Z

Summary

Addresses issue:

Existing user account should be linked to Google account using SiwG when already signed in, if SiwG was initiated from the user's profile/"edit profile" page #9994

Relevant technical choices

Created Profile_Authenticator as an Authenticator subclass instead of branching inside Authenticator.php. The connect flow has different preconditions (signed-in user only, no email matching, no new user creation, nonce required), so a subclass keeps both paths readable.
Web_Tag::set_is_connect_existing_profile() toggles the response.integration and response.connect_nonce lines in Web_Tag::render, since the IB's render_signinwithgoogle() insertion point was already refactored into that file before this branch.
Added a CSRF nonce (Authenticator::CONNECT_NONCE_ACTION) for the connect flow. The base login flow doesn't need one (reached via wp-login.php), but the profile-page flow runs inside an authenticated admin session where a cross-site POST could piggyback on the auth cookie.
Guarded the connect branch in render_sign_in_with_google_profile with ! $this->is_connected() so the placeholder does not render when SiwG has no clientID. Caught in code review, mirrors render_signinwithgoogle_woocommerce.

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 5.2 and PHP 7.4.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.
Ensure there are no unexpected significant changes to file sizes.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

github-actions · 2026-05-20T10:07:20Z

🤖 This comment is automatically updated by CI workflows. Each section is managed independently.

🎭 Playwright reports for 8a597c9:

📦 Build files for 8a597c9:

Build files have been deleted.

tofumatt

Is there any reason this PR has 37 commits? 😅

We're not in the habit of enforcing incredibly atomic commits, but this one can probably be squashed into fewer, as there seem to be a lot of in-progress or refactoring/revisiting commits in this PR?

For instance, there's this commit 5cb0517 that updates the year, but the first should have been committed with that year in the first place 😅

We don't change the copyright year of a file once it's been created, so let's remove commits that make it seem like we're updating it, as it might get a bit confusing/ambiguous.

Some merge conflicts need resolving but I don't think there's too many.

There are some naming and wording suggestions here, as well as some questions about the intent of some of the tests, etc.

I think the logic to determine which authenticator to loads needs some adjustment too, as the "Existing User"/Profile_Authenticator won't ever be loaded on a site with WooCommerce installed.

tofumatt · 2026-05-25T22:13:20Z

+		// becomes a real button on the user's own unlinked profile.
+		add_action( 'admin_footer', $this->get_method_proxy( 'maybe_render_profile_signinwithgoogle' ) );
+
+		// Surface the already-connected error from the profile-page connect flow.


Sorry, I don't quite follow what this comment means. Is the error that we're showing one that appears if the user already has a connected account? 🤔 This is a bit ambiguous.

Not quite. It shows when a user tries to link a Google account that's already linked to a different WordPress user. I've updated the comment to make it clearer.

Centralizes the guarded get_current_screen() pattern into a single static helper so callers don't repeat the function_exists / instanceof checks.

Lets signed-in users connect their WordPress account to a Google account from their profile page. Checks for duplicate Google links and verifies the request with a nonce.

Adds an is_connect_existing_profile flag so the tag hooks admin_footer instead of wp_footer, sets the integration marker, and includes a connect nonce in the rendered script.

Pull get_hashed_user_id() and build_registrable_tag() out of their inline call sites so they can be reused without duplicating the instantiation logic. Refs #9994.

Add the connect button to the user's own profile page, resolve the correct authenticator per integration, render admin notices for connect errors, and register the admin_footer and admin_notices hooks. Refs #9994.

Co-authored-by: Matthew Riley MacPherson <hi@tofumatt.com>

Rename `Profile_Authenticator` to `Existing_User_Authenticator`, and rename the `profile-connect` CSS class to `existing-user-connect-button` so the names match the flow. Keep the `CONNECT` prefix on the nonce constant. Rename `render_profile_admin_notices` to `render_profile_admin_errors` since the method only renders errors. Switch the profile-page tag output to use `Web_Tag::register()` instead of a separate `render_tag()` method, so the tag follows the same rendering pattern as the other flows. Add a test that confirms the existing-user flow is picked when WooCommerce is also loaded. Drop jargon from docblocks, inline comments, and assertion messages. Replace "short-circuit", "anti-vacuous", "hooked", "forces", "wired up", "piggyback", and "win" with plain English. Match project conventions by removing typed properties and void return types that weren't used anywhere else in `includes/`. Refs #9994.

The profile flow now renders its tag through `Web_Tag::register()`, which hooks the render onto `admin_footer`. The trigger was also running on `admin_footer`, so the render was getting added to a hook that was already firing, and WordPress dropped it. The button never showed up. Move the trigger to `in_admin_footer`, which runs just before `admin_footer`, so the render is queued in time. Update the profile render tests to fire the hooks in that order. Also, alias the test case instead of `stdClass` in the WooCommerce stubs, since `class_alias()` rejects `stdClass`, and add a description to the `Current_Screen::get()` return tag to keep PHPCS happy. Refs #9994.

The two WooCommerce-active tests alias `WooCommerce` so the `class_exists( 'WooCommerce' )` checks pass. PHP can't remove a class once it's added, so the alias stayed for later tests and made `test_inline_data_has_woocommerce` see WooCommerce as active, which caused every PHP test job to fail. Mark both tests with `@runInSeparateProcess` so the alias only lives inside that test, matching the existing `WooCommerceTest` pattern. Refs #9994.

Change `e.g.` to `eg.` in the profile error docblock so it matches the rest of the PR's comments, and reword "Fire the hooks" to "Run the hooks" so it doesn't use "fire" as an action. Refs #9994.

tofumatt

Looks good, I have some further recommendations and some questions, but they're mostly minor and I understand the flows here better now 👍🏻

I think this one is nearly ready-to-go 👍🏻

tofumatt · 2026-06-04T11:43:40Z

+	 * Query arg for the existing-user link flow error. Namespaced to avoid
+	 * WordPress core's empty `?error=` notice on `wp-admin/profile.php` and `wp-admin/user-edit.php`.


To avoid what? Do you mean to override reading/overwriting it? I get the idea of this but I think the wording could just be a bit more clear 🙂

To stop the user seeing an empty error box. On the profile and edit-user pages, WordPress shows an error notice for any ?error= value, but only writes the message for ones it knows (like new-email). Ours isn't one, so reusing ?error= would put an empty error next to our real one. Our own arg avoids that. I've reworded the docblock to say it.

tofumatt · 2026-06-04T11:55:59Z

+			return $this->get_error_redirect_url( self::ERROR_INVALID_REQUEST );
+		}
+
+		$g_user_hid = $this->get_hashed_google_user_id( $payload );


Just to make this a bit clearer:

Suggested change

$g_user_hid = $this->get_hashed_google_user_id( $payload );

$google_user_hashed_id = $this->get_hashed_google_user_id( $payload );

tofumatt · 2026-06-04T12:20:49Z

+		// Do not use `error=` as a query arg here, because
+		// `wp-admin/profile.php` and `wp-admin/user-edit.php` will
+		// show an error for any unrecognized `?error=` value.


Ah, this one is helpful, thanks. I'll add that to my earlier question about the ?error= comment above. 👍🏻

Thanks! I've polished that to:

// Do not use `error=` as the query arg here. On // `wp-admin/profile.php` and `wp-admin/user-edit.php`, the user would // see an empty error box for any `?error=` value core doesn't know.

tofumatt · 2026-06-04T12:45:47Z

+		// Logged-in users normally skip the button render loop. The connect flow flips that gate
+		// so the placeholder div on the profile page can still be wired up.


Sorry, what is the existing-user link flow winning? Do you mean something like

This tests that the authenticator flow is one that connects an existing user to the Google account, not one that uses the WooCommerce (or another) authenticator that would cause a new user to be created.

?

tofumatt · 2026-06-04T13:17:17Z

+		unset( $_GET[ Existing_User_Authenticator::ERROR_QUERY_ARG ] );
+	}
+
+	public function test_render_profile_admin_errors__skips_outside_profile_pages() {


Suggested change

public function test_render_profile_admin_errors__skips_outside_profile_pages() {

public function test_render_profile_admin_errors__does_not_render_when_not_on_user_profile_pages() {

tofumatt · 2026-06-04T13:18:35Z

+		// `Tag_Guard::can_activate()` rejects `wp_login_url()` when it's not
+		// HTTPS, so `build_registrable_tag()` would return null otherwise and
+		// the callback would return early before rendering anything.


Suggested change

// `Tag_Guard::can_activate()` rejects `wp_login_url()` when it's not

// HTTPS, so `build_registrable_tag()` would return null otherwise and

// the callback would return early before rendering anything.

// `Tag_Guard::can_activate()` checks for HTTPS, so make sure

// HTTPS login appears supported for this test.

tofumatt · 2026-06-04T13:19:51Z

+		// With HTTPS set up, `Tag_Guard::can_activate()` passes. Now only
+		// the `did_action` check can stop the connect script from rendering.


Is that happening here? Are we documenting the behaviour of the did_action check here or the test's goals? I'm just a bit unclear by what this comment is explaining 😅

You're right it was unclear. The goal is the connect button shouldn't show when the user isn't on their own profile. I've reworded the comment to say that, and renamed the test to test_maybe_render_profile_signinwithgoogle__does_not_show_connect_button_outside_own_profile.

tofumatt · 2026-06-04T13:30:00Z

+		$this->assertStringNotContainsString( "response.integration='existing_user'", $output, 'admin_footer should not render the connect script outside the profile page.' );
+	}
+
+	public function test_maybe_render_profile_signinwithgoogle__skips_when_user_already_linked() {


Isn't this what we're testing here? 🤔

Suggested change

public function test_maybe_render_profile_signinwithgoogle__skips_when_user_already_linked() {

public function test_maybe_render_profile_signinwithgoogle__shows_disconnect_button_when_user_already_linked() {

I renamed it to test_maybe_render_profile_signinwithgoogle__does_not_show_connect_button_when_user_already_linked.

This test checks the connect button doesn't show, not that the disconnect button shows. The disconnect button is already covered by test_render_sign_in_with_google_profile__disconnect_button.

tofumatt · 2026-06-04T13:30:52Z

+
+		$output = $this->capture_action( 'admin_footer' );
+
+		$this->assertStringNotContainsString( "response.integration='existing_user'", $output, 'Should not render connect script when user already has a linked Google account.' );


This test could probably be combined with the test that checks for a disconnect button, rather than being discrete.

I'd keep them separate. They check different things, and the already-linked behavior is already covered across two tests:

test_render_sign_in_with_google_profile__disconnect_button checks the disconnect button shows.

test_maybe_render_profile_signinwithgoogle__does_not_show_connect_button_when_user_already_linked checks the connect button doesn't render.

Rename profile render tests to describe the behavior they check instead of the code branch. Reword connect-flow comments and test assertion messages to describe the user-facing outcome. Clarify the docblocks for `ERROR_QUERY_ARG` and the profile footer render method, plus the redirect comment in `Web_Tag`.

tofumatt

Just a few tweaks I'll make and then this is good to go 👍🏻

Co-authored-by: Matthew Riley MacPherson <hi@tofumatt.com>

shervElmi marked this pull request as ready for review May 21, 2026 17:18

shervElmi commented May 21, 2026

View reviewed changes

Comment thread includes/Modules/Sign_In_With_Google.php

tofumatt requested changes May 27, 2026

View reviewed changes

shervElmi added 5 commits May 28, 2026 00:37

Introduce Current_Screen utility for safe screen access

fbcd7f9

Centralizes the guarded get_current_screen() pattern into a single static helper so callers don't repeat the function_exists / instanceof checks.

Add Profile_Authenticator for linking existing accounts

ddef14a

Lets signed-in users connect their WordPress account to a Google account from their profile page. Checks for duplicate Google links and verifies the request with a nonce.

Extend Web_Tag for the profile-connect flow

5df477f

Adds an is_connect_existing_profile flag so the tag hooks admin_footer instead of wp_footer, sets the integration marker, and includes a connect nonce in the rendered script.

Extract shared helpers in Sign_In_With_Google module.

daee975

Pull get_hashed_user_id() and build_registrable_tag() out of their inline call sites so they can be reused without duplicating the instantiation logic. Refs #9994.

Wire up profile-connect flow in Sign_In_With_Google module.

f6652f4

Add the connect button to the user's own profile page, resolve the correct authenticator per integration, render admin notices for connect errors, and register the admin_footer and admin_notices hooks. Refs #9994.

shervElmi force-pushed the feature/9994-link-existing-account-from-profile branch from c250512 to f6652f4 Compare May 27, 2026 22:19

shervElmi and others added 2 commits May 28, 2026 13:20

Capitalize Sign in the profile-connect prompt.

d864cc2

Co-authored-by: Matthew Riley MacPherson <hi@tofumatt.com>

Address feedback from code review.

a374357

shervElmi force-pushed the feature/9994-link-existing-account-from-profile branch from d9150e6 to a374357 Compare May 28, 2026 15:44

shervElmi force-pushed the feature/9994-link-existing-account-from-profile branch from 5607d91 to 9fa83d5 Compare May 29, 2026 10:40

shervElmi and others added 7 commits May 29, 2026 14:55

Tidy two comments to match the writing-style rules.

92e30af

Change `e.g.` to `eg.` in the profile error docblock so it matches the rest of the PR's comments, and reword "Fire the hooks" to "Run the hooks" so it doesn't use "fire" as an action. Refs #9994.

Rename variable.

729c2f4

Tweak comment for ERROR_QUERY_ARG.

20561d5

Adjust test comment.

3308214

Tweak test skip comment.

a5aac30

tofumatt requested changes Jun 4, 2026

View reviewed changes

shervElmi and others added 2 commits June 5, 2026 15:14

Adjust PHP comment.

5efe1fb

tofumatt reviewed Jun 8, 2026

View reviewed changes

Adjust code comments.

8a597c9

Co-authored-by: Matthew Riley MacPherson <hi@tofumatt.com>

tofumatt approved these changes Jun 8, 2026

View reviewed changes

tofumatt merged commit 478dae0 into develop Jun 10, 2026
33 checks passed

tofumatt deleted the feature/9994-link-existing-account-from-profile branch June 10, 2026 01:00

		* Query arg for the existing-user link flow error. Namespaced to avoid
		* WordPress core's empty `?error=` notice on `wp-admin/profile.php` and `wp-admin/user-edit.php`.

	$g_user_hid = $this->get_hashed_google_user_id( $payload );
	$google_user_hashed_id = $this->get_hashed_google_user_id( $payload );

		// Logged-in users normally skip the button render loop. The connect flow flips that gate
		// so the placeholder div on the profile page can still be wired up.

	public function test_render_profile_admin_errors__skips_outside_profile_pages() {
	public function test_render_profile_admin_errors__does_not_render_when_not_on_user_profile_pages() {

		// With HTTPS set up, `Tag_Guard::can_activate()` passes. Now only
		// the `did_action` check can stop the connect script from rendering.

	public function test_maybe_render_profile_signinwithgoogle__skips_when_user_already_linked() {
	public function test_maybe_render_profile_signinwithgoogle__shows_disconnect_button_when_user_already_linked() {


		$output = $this->capture_action( 'admin_footer' );

		$this->assertStringNotContainsString( "response.integration='existing_user'", $output, 'Should not render connect script when user already has a linked Google account.' );

Conversation

shervElmi commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

Uh oh!

github-actions Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tofumatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tofumatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shervElmi Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shervElmi Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tofumatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

shervElmi commented May 20, 2026 •

edited

Loading

github-actions Bot commented May 20, 2026 •

edited

Loading

shervElmi Jun 4, 2026 •

edited

Loading

shervElmi Jun 5, 2026 •

edited

Loading