Connection: reconcile the protected owner against WordPress.com on connect by darssen · Pull Request #52712 · Automattic/jetpack

darssen · 2026-09-23T14:56:45Z

Proposed changes

Adds Manager::reconcile_protected_owner(), hooked on jetpack_user_authorized. One call to WordPress.com settles the whole question: whether an owner exists, whether this site's anchor still names them, whether the user connecting is them, and what that user's own WordPress.com identity is.
Removes promote_protected_owner_on_connect() (CONNECT-454). Its question — "is the connecting user the anchored owner?" — is answered by the same reply, so it no longer needs a call of its own.
Removes the anchor's locked flag and Protected_Owner::set_locked(). An anchor nobody confirms is dropped rather than suspended, so holding one and being protected by it are the same thing.

Depends on a WordPress.com change

This calls jetpack.reconcileProtectedOwner, which CONNECT-461 will build. Three points to agree, none of them shipped yet, so this should not merge before that is settled:

Renamed from jetpack.getProtectedOwner. It no longer gets the protected owner — it never returns the owner at all now — and contract A's endpoint is already named for its contract (assertProtectedOwner). Free to change while nothing is built; a deploy-window fallback afterwards.
Adds caller_wpcom_user_id, always returned. It is the identity on the signing token, so it discloses nothing the site could not already fetch, and it restores something the old two-call path did as a side effect: writing the connecting user's binding, which connecting itself clears.
Drops wpcom_user_id, now redundant — it was non-zero only when is_caller was true, and in that case it equals the caller's ID by definition.

The response the site expects:

'has_owner'            => (bool) $owner,
'matches'              => (bool) ( $owner && $anchored && $owner === $anchored ),
'is_caller'            => $owner === (int) $this->token->user_id,
'caller_wpcom_user_id' => (int) $this->token->user_id,

Notes for review

The gate was a pure local read. Nothing kept the anchor honest once written: a record cleared at WordPress.com never reached the site, so it went on gating a live feature on its own copy.
Confirmation, not disclosure. The site sends the identity it holds and is told whether it still matches; the owner is never named to anyone else. The only identity the answer discloses is the caller's own. A bystander can therefore confirm an anchor but never create one.
Failing to confirm drops the anchor. Suspending it left an identity the site could no longer vouch for, which the next connection — anyone's — would then complete: a bystander's request carried the stored ID, WordPress.com confirmed it, and the lock returned without the owner involved. Clearing costs the cheaper recovery route, and that is the intended trade: only the owner can re-anchor now, and two tests pin both halves.
Lost-anchor recovery is new. The reconcile asks even with no anchor, so an owner connecting to a site that never recorded the claim, or lost it, is anchored from the answer instead of being asked to confirm again. This is the case raised on Connection: ask WordPress.com to record the protected owner first #52551.
Anchoring here is not establishing. WordPress.com has already accepted a deliberate claim; the site is only catching up.
Inert until wpcom change ships. Until then every reconcile fails closed, which is intended.

Does this pull request change what data or activity we track or use?

No.

Testing instructions

jp test php packages/connection — 1106 pass.
Falsification, each sabotage failing only the tests that own it: failing open when WordPress.com is unreachable; keeping an anchor that names another account; ignoring is_caller or matches; skipping either binding write; promoting regardless of role; re-pointing a stale anchor instead of replacing it; writing the binding before the anchor; a wrong endpoint name or payload key.
jp phan packages/connection — 0 issues.
On a connected site with no anchor and no WordPress.com handler, authorizing fails closed and changes nothing.

github-actions · 2026-09-23T14:58:11Z

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

To test on WoA, go to the Plugins menu on a WoA dev site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin (Jetpack or WordPress.com Site Helper), and enable the connect-410-po-pr5-verify-the-anchor-against-wpcom-on-connect branch.
To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack connect-410-po-pr5-verify-the-anchor-against-wpcom-on-connect

bin/jetpack-downloader test jetpack-mu-wpcom-plugin connect-410-po-pr5-verify-the-anchor-against-wpcom-on-connect

Interested in more tips and information?

In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
Read more about our development workflow here: PCYsg-eg0-p2
Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

github-actions · 2026-09-23T14:58:13Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
You can use GitHub's Reviewers functionality to request a review.
When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

Copilot

Copilot review overview

🟡 Changes recommended

Failed lock-state writes can prevent fail-closed behavior; changelog and transport-test updates are also needed.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 2

Open (3)

Handle set_locked failure to preserve fail-closed behavior · New
Capitalize the imperative changelog action · New
Add transport-level coverage for the protected-owner XML-RPC call · New

What changed in this PR

Adds connect-time reconciliation of the protected-owner anchor with WordPress.com.

Changes:

Verifies ownership after authorization and updates lock state.
Adds XML-RPC querying and protected-owner state mutation.
Adds reconciliation tests and a changelog entry.

File	Summary
`projects/packages/connection/tests/php/Protected_Owner_Test.php`	Tests reconciliation outcomes and hook ordering.
`projects/packages/connection/src/class-protected-owner.php`	Adds protected-owner lock-state updates.
`projects/packages/connection/src/class-manager.php`	Adds verification hooks, reconciliation logic, and XML-RPC querying.
`projects/packages/connection/changelog/connect-410-po-pr5-verify-the-anchor-against-wpcom-on-connect`	Documents the feature; wording and plugin changelog updates remain.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

jp-launch-control · 2026-09-23T15:13:23Z

Code Coverage Summary

Coverage changed in 2 files.

File	Coverage	Δ%	Δ Uncovered
projects/packages/connection/src/class-manager.php	960/1302 (73.73%)	0.44%	1 ❤️‍🩹
projects/packages/connection/src/class-protected-owner.php	27/29 (93.10%)	-0.44%	0 💚

Full summary · PHP report · JS report

…nnect The gate is a pure local read, so nothing kept the anchor honest once it was written: a record cleared at WordPress.com never reached the site, and a site went on gating a live feature on its own copy. Verification runs on `jetpack_user_authorized` at priority 11, after promote-on-connect has settled the master slot, when the site has a fresh user token and an answer is cheap. It asks whether the identity it has anchored is still the owner of record, so the answer does not depend on who is connecting — naming the owner instead would let a secondary user reconnecting unlock a good anchor. Fails closed without forgetting. Unreachable, refused and unimplemented all unlock rather than trust the anchor; the identity survives, so a later verification restores the lock without asking the owner to confirm again. No owner of record clears the anchor outright, which is how a wrongly anchored site recovers once support clears it at that end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…ording Every reconcile test stubs `query_protected_owner_record()`, so a wrong method name, payload key or identity would have reached production while the suite stayed green. The new test asserts the exact XML elements rather than substrings: `anchored_wpcom_user_id` matches inside `anchored_wpcom_user_idX`, so a looser assertion catches the method name and nothing else. The changelog entry also prefixed the package's own name, which the monorepo guidelines reserve for entries in other projects, and opened lowercase. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Promotion and verification asked WordPress.com two separate questions on the same hook: "is the connecting user the anchored owner?" and "is the anchor still right?". One `jetpack.getProtectedOwner` reply already answers both, so they are now one callback reading the whole answer instead of two reading a third of it each. Three things follow from that. The lookup behind promotion disappears: it compared a resolved ID against the local anchor, where `is_caller` compares against WordPress.com's record, which is the authority when the two have drifted. The priority 10/11 ordering goes away, and with it a test that existed only to guard it. And the reconcile now asks even with no anchor, so an owner connecting to a site that never recorded the claim — or lost it — is anchored from the answer rather than being asked to confirm all over again. Anchoring there is not establishing: WordPress.com has already accepted a claim, and the identity is disclosed only to the owner it names, so a bystander still learns nothing and can anchor nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Suspending an anchor kept an identity the site could no longer vouch for, and left it for the next connection — anyone's — to complete: a bystander's request carried the stored ID, WordPress.com confirmed it, and the lock came back without the owner involved. Clearing instead leaves nothing behind, at the cost of the cheaper recovery route. Only the owner can re-anchor now, because the answer names them to nobody else. With no suspended state left, the `locked` flag has nothing to say: holding an anchor and being protected by it are the same thing. `set_locked()` goes, and `get_locked()` and `is_locked()` stay as the names gates read by. Also fixes two things the tracing turned up. Re-pointing only moves the cached local ID, so adopting a *different* owner left the anchor naming the account that no longer owns the site; it is replaced when the identity changes. And the binding was written before the anchor, so a failed anchor write left one behind — the ordering this commit exists to avoid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`getProtectedOwner` no longer gets the protected owner — it never returns the owner at all — while contract A's endpoint is already named for its contract. Renamed to `jetpack.reconcileProtectedOwner` while nothing is built against it; afterwards it would cost a deploy-window fallback. The answer now carries `caller_wpcom_user_id`, the identity on the signing token, in place of an owner ID that was only ever returned to the owner. It discloses nothing the site could not already fetch, and it restores what the old two-call path did as a side effect: writing the connecting user's binding, which connecting itself clears. That write is back for every user, not only the owner — and on the bystander path it is free of the ordering constraint, because nothing is anchored there for it to strand. Pending CONNECT-461 building the endpoint this way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Replacing the promote and verify test blocks took out everything between them, which included the ten tests for `resolve_protected_owner_state()` and two for `Protected_Owner::repoint()` — neither of which this PR touches. The coverage check caught it: the uncovered lines it reported were almost entirely the state resolver's. Restored unchanged, apart from dropping the `locked` key from one fixture, and `test_connecting_without_an_anchor_changes_no_ownership` re-expressed against the reconcile: a site WordPress.com holds no owner for changes no ownership, whoever connects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1 · 2

Open (4)

Verify protected-owner deletion succeeds · New
Handle legacy unlocked anchors during migration · New
Add affected plugin changelog entries · New
Update docblock to describe clearing the anchor · New

Resolved since last review (3)

Handle set_locked failure to preserve fail-closed behavior
Add transport-level coverage for the protected-owner XML-RPC call
Capitalize the imperative changelog action

darssen · 2026-09-25T12:49:38Z

+		// dropped rather than suspended: an anchor nobody confirmed is not state to leave lying
+		// around for somebody else's connection to complete.
+		if ( ! is_array( $record ) || ! isset( $record['has_owner'] ) ) {
+			Protected_Owner::clear();


Hey @bindlegirl , this is very similar to #52712 (comment) .
What do you think is the best course of action here? Since failind to delete an option should be pretty rare, I think we are OK leaving the anchor as it is and returning false.

Agreed. Leaving it sounds fine. We could add some error logging but most likely not here. Maybe in reconcile_protected_owner() ?

It still described failing closed without forgetting, which the code stopped doing when suspending gave way to clearing. It now says what happens instead, including that a refused delete leaves the anchor standing — that write is the only mechanism there is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

bindlegirl · 2026-09-25T14:07:22Z

@darssen Thanks for working on this. I like this second approach better 😄

Stale locked in test fixtures. Two fixtures still carry 'locked' => true after the key was removed from the documented schema: projects/packages/connection/tests/php/Jetpack_Options_Test.php:54 and projects/packages/sync/tests/php/modules/Options_Module_Test.php:76. Harmless (the code ignores the key, and the round-trip test only checks storage), but they now describe an anchor shape that no longer exists. Worth dropping to keep the fixtures honest.
Protected_Owner::set() docblock still opens "Record a confirmed protected owner and lock the anchor" — there's no lock flag anymore, so "lock" is now metaphorical. Small staleness given this PR is what removed the flag.

Two fixtures still built anchors carrying `locked`, and `Protected_Owner::set()` still opened by saying it locks one. Harmless — the code ignores the key — but they describe a shape this PR removed, which is the kind of thing that misleads whoever reads them next. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Two corrections to the shape this PR arrived at. Asking on every authorization meant a site with no protected owner made a WordPress.com request it could not act on, which is not "behaves exactly as today" for the overwhelming majority of sites. Recovering a lost anchor was the reason, and it turns out not to need this: the owner reaches the same confirm dialog, the claim answers `already_yours`, and the anchor is written. Same button, one click, no new surface — so the reconcile goes back to maintaining what a claim created. And silence is not an answer. Unreachable, refused, unimplemented and malformed now leave the anchor exactly as it was. Dropping a lock because WordPress.com had a bad minute costs a merchant their payouts until the owner happens to connect, and a request that never arrived is no evidence against a record that was confirmed once. State moves only when WordPress.com actually says something: no owner, a different owner, or this caller is the owner. Nothing is ever left half-suspended, which was the worry that argued for clearing in the first place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

darssen · 2026-09-25T15:34:57Z

@darssen Thanks for working on this. I like this second approach better 😄

Stale locked in test fixtures. Two fixtures still carry 'locked' => true after the key was removed from the documented schema: projects/packages/connection/tests/php/Jetpack_Options_Test.php:54 and projects/packages/sync/tests/php/modules/Options_Module_Test.php:76. Harmless (the code ignores the key, and the round-trip test only checks storage), but they now describe an anchor shape that no longer exists. Worth dropping to keep the fixtures honest.

Protected_Owner::set() docblock still opens "Record a confirmed protected owner and lock the anchor" — there's no lock flag anymore, so "lock" is now metaphorical. Small staleness given this PR is what removed the flag.

Thanks for the review @bindlegirl!

I implemented the changes asked here and also, as internally discussed, I've decided to not clear the anchor when there is a wpcom failure and only try to reconcile when an anchor exists. Note that this loses the ability to self-recovery but it avoids asking for every authorize when an anchor doesn't exist.

bindlegirl

One more small test comment:
test_connecting_to_an_unowned_site_changes_no_ownership (1156) — no anchor, stubs a has_owner => false record that's never consulted. It's also stranded above the // ── reconcile_protected_owner ── section header (leftover from the restore-tests commit). The has_owner === false clear path is already properly covered — with an anchor — by test_no_record_at_wpcom_clears_the_anchor (1329).
test_a_bystander_cannot_anchor_a_site_with_no_record (1466) — no anchor, stubs bystander_answer( false ) that's never consulted.

Both collapse to the same assertion as test_an_unanchored_site_asks_nothing (1351), which tests it precisely (query… expected never()). I'd delete these two, or if you want to keep the "master slot untouched" flavor of #1, rename it and drop the dead $record.

bindlegirl · 2026-09-28T08:45:39Z

+	 * them, and whether the user connecting is them — so the lock, the binding and the master slot
+	 * are all decided together rather than from two calls that could disagree.
+	 *
+	 * Fails closed. Unreachable, refused and unimplemented all drop the anchor rather than trust


This isn't true any longer, right?

No, it isn't. Good catch 👍 . Every sentence of that paragraph describes the
clear-on-failure behaviour, which we reverted: silence now leaves the anchor
exactly as it was. It also didn't mention the anchor gate, which is the first
thing the method does.

Fixed in d32a34a.

bindlegirl · 2026-09-28T08:49:38Z

-			return;
+		// The identity is disclosed only to the owner it names, so this branch is the one place the
+		// site can learn it. Anchoring here is not establishing: WordPress.com already accepted a
+		// claim, and this catches up a site that never recorded it or lost the record.


I'm confused by this comment too. Is it correct?

Yes, correct too. I fixed it in f6e60d8.

bindlegirl · 2026-09-28T08:57:34Z

+	/**
+	 * A failed anchor write leaves no binding behind for a later connection to build on.
+	 */
+	public function test_a_failed_anchor_write_leaves_no_binding() {


I think there is a $this->anchor(); missing here?

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…branch Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…hat is precise Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

darssen · 2026-09-28T09:24:14Z

One more small test comment: test_connecting_to_an_unowned_site_changes_no_ownership (1156) — no anchor, stubs a has_owner => false record that's never consulted. It's also stranded above the // ── reconcile_protected_owner ── section header (leftover from the restore-tests commit). The has_owner === false clear path is already properly covered — with an anchor — by test_no_record_at_wpcom_clears_the_anchor (1329). test_a_bystander_cannot_anchor_a_site_with_no_record (1466) — no anchor, stubs bystander_answer( false ) that's never consulted.

Both collapse to the same assertion as test_an_unanchored_site_asks_nothing (1351), which tests it precisely (query… expected never()). I'd delete these two, or if you want to keep the "master slot untouched" flavor of #1, rename it and drop the dead $record.

Thanks for the re-review.

Fixed tests in 1a38294. I deleted both and folded the master-slot assertion into test_an_unanchored_site_asks_nothing. The property was worth keeping; nothing else asserts that this hook leaves master_user alone on an unanchored site, but keeping it as a separate test just recreates the overlap in a tidier form. One test now says the whole truth about an unanchored site: no call, no anchor, no ownership change.

bindlegirl

Thank you for addressing the remaining issues. I think we are good to go.

darssen self-assigned this Sep 23, 2026

darssen added [Package] Connection [Status] In Progress [Tests] Includes Tests labels Sep 23, 2026

darssen requested a lite review from Copilot September 23, 2026 15:04

Copilot started reviewing on behalf of darssen September 23, 2026 15:05 View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

Comment thread projects/packages/connection/src/class-manager.php Outdated

Comment thread .../packages/connection/changelog/connect-410-po-pr5-verify-the-anchor-against-wpcom-on-connect Outdated

Comment thread projects/packages/connection/src/class-manager.php Outdated

darssen marked this pull request as ready for review September 23, 2026 16:13

darssen requested a review from a team as a code owner September 23, 2026 16:13

Base automatically changed from connect-411-po-pr6a-set-protected-owner-asserts-to-wpcom to trunk September 24, 2026 13:37

darssen and others added 3 commits September 25, 2026 12:37

darssen force-pushed the connect-410-po-pr5-verify-the-anchor-against-wpcom-on-connect branch from 83b62c2 to a7318ce Compare September 25, 2026 10:39

darssen and others added 3 commits September 25, 2026 13:10

darssen requested a lite review from Copilot September 25, 2026 12:26

Copilot started reviewing on behalf of darssen September 25, 2026 12:27 View session

Copilot AI reviewed Sep 25, 2026

View reviewed changes

darssen added [Status] Needs Review This PR is ready for review. DO NOT MERGE don't merge it! and removed [Status] In Progress labels Sep 25, 2026

github-actions Bot added the [Package] Sync label Sep 25, 2026

darssen requested a review from bindlegirl September 25, 2026 15:35

bindlegirl requested changes Sep 28, 2026

View reviewed changes

darssen and others added 3 commits September 28, 2026 11:16

Connection: describe what the reconcile does now, not what it used to

d32a34a

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Connection: comment the owner branch for what it decides, beside the …

f6e60d8

…branch Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Connection: fold two tests the anchor gate made vacant into the one t…

1a38294

…hat is precise Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

darssen requested a review from bindlegirl September 28, 2026 09:24

bindlegirl approved these changes Sep 28, 2026

View reviewed changes

darssen merged commit 319b609 into trunk Sep 28, 2026
125 checks passed

darssen deleted the connect-410-po-pr5-verify-the-anchor-against-wpcom-on-connect branch September 28, 2026 11:21

github-actions Bot added [Status] UI Changes Add this to PRs that change the UI so documentation can be updated. and removed [Status] Needs Review This PR is ready for review. labels Sep 28, 2026

bindlegirl removed the DO NOT MERGE don't merge it! label Sep 28, 2026

Conversation

darssen commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Depends on a WordPress.com change

Notes for review

Does this pull request change what data or activity we track or use?

Testing instructions

Uh oh!

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

Uh oh!

Uh oh!

github-actions Bot commented Sep 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jp-launch-control Bot commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

darssen Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

bindlegirl Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bindlegirl commented Sep 25, 2026

Uh oh!

darssen commented Sep 25, 2026

Uh oh!

bindlegirl left a comment

Choose a reason for hiding this comment

Uh oh!

bindlegirl Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

darssen Sep 28, 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!

bindlegirl Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

darssen Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

bindlegirl Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

darssen commented Sep 28, 2026

Uh oh!

bindlegirl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

darssen commented Sep 23, 2026 •

edited

Loading

github-actions Bot commented Sep 23, 2026 •

edited

Loading

jp-launch-control Bot commented Sep 23, 2026 •

edited

Loading

darssen Sep 28, 2026 •

edited

Loading