Skip to content

Remove dead WPCOM app authorization GET flow#3579

Open
joemcgill wants to merge 3 commits into
developfrom
worktree-remove-dead-wpcom-auth
Open

Remove dead WPCOM app authorization GET flow#3579
joemcgill wants to merge 3 commits into
developfrom
worktree-remove-dead-wpcom-auth

Conversation

@joemcgill

@joemcgill joemcgill commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes: GOOWOO-238

Round 1 — dead GET auth-url flow:

  • fetchWPComAppAuthorizationUrl (js/src/data/actions.js) lost its only caller when EnableNewProductSyncButton was removed in 60e15f4 (2025-08-07). It has had zero callers since.
  • The entire PHP chain it used to trigger was unreachable too: GET /wc/gla/rest-api/authorize -> AuthController::get_authorize_callback() -> OAuthService::get_auth_url() -> get_data_from_google() -> Middleware::get_sdi_auth_params() / get_sdi_auth_endpoint().

Round 2 — dead DELETE and PUT flow (rest of AuthController):

  • The DELETE handler (revoke_wpcom_api_auth) was reachable (disconnect-all-accounts UI + plugin deactivation), but revoking a WPCOM OAuth token is no longer valid now that the plugin uses client-credentials auth (per commit 42533bc44, INTEGRA-48).
  • The PUT handler (update_wpcom_api_authorization) was wired to a live-looking hook, but could never succeed — it checks a stored nonce that nothing writes anymore (the writer was removed in round 1), so it always threw "No stored nonce found."
  • Removing both handlers leaves AuthController with no route to register, so the whole class is deleted, along with everything exclusively feeding it: OAuthService::revoke_wpcom_api_auth/deactivate/get_wpcom_api_url (OAuthService is now a small constants-only class), AccountService::update_wpcom_api_authorization/reset_wpcom_api_authorization_data/delete_wpcom_api_auth_nonce/delete_wpcom_api_status_transient, the GOOGLE_WPCOM_AUTH_NONCE option, the DisconnectController fan-out entry and its JS error special-case, and the useUpdateRestAPIAuthorizeStatusByUrlQuery hook.

Left untouched, confirmed still live:

  • AccountService::get_connected_status() / is_wpcom_api_status_healthy() / OptionsInterface::WPCOM_REST_API_STATUS — the current client-credentials health-check path.
  • OAuthService::STATUS_APPROVED / STATUS_ERROR constants — still read by get_connected_status().
  • The rest of the disconnect-all-accounts flow and DisconnectController's other fan-out endpoints.
  • reconnect-wpcom-account/* / ConnectWPComAccountCard — a separate Jetpack site-connection feature, not part of this auth flow.
  • JetpackWPCOM.php — unrelated Jetpack-connection-package compatibility shim.
  • The separate SDI merchant-update chain (get_sdi_endpoint, get_sdi_merchant_update_endpoint, update_sdi_merchant_account).

Test plan

  • composer run test-unit (or the project's WP test env) — DisconnectControllerTest, AccountServiceTest should pass; AuthControllerTest, OAuthServiceTest are deleted
  • npm run lint:js / npm run test:js — confirm no lingering references and disconnectAllAccounts tests still pass
  • Manually verify "Disconnect all accounts" in Settings still works end-to-end
  • Confirm plugin deactivation no longer attempts (or errors on) a WPCOM token revoke

🤖 Generated with Claude Code

`fetchWPComAppAuthorizationUrl` lost its only caller when
EnableNewProductSyncButton was removed (60e15f4), leaving the JS
action and the entire PHP chain it triggered (GET /rest-api/authorize
-> AuthController::get_authorize_callback -> OAuthService::get_auth_url
-> get_data_from_google -> Middleware::get_sdi_auth_params/
get_sdi_auth_endpoint) unreachable. Removes the action, the GET route
handler and its exclusive dependents, and their tests, while leaving
the route's still-live DELETE/PUT handlers untouched.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.5%. Comparing base (46d2cf6) to head (41f08c5).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             develop   #3579     +/-   ##
===========================================
- Coverage       66.8%   66.5%   -0.3%     
+ Complexity      5342    5308     -34     
===========================================
  Files            914     912      -2     
  Lines          28280   28027    -253     
  Branches        1720    1718      -2     
===========================================
- Hits           18883   18631    -252     
+ Misses          9136    9135      -1     
  Partials         261     261             
Flag Coverage Δ
js-unit-tests 64.0% <ø> (-0.1%) ⬇️
php-unit-tests 67.2% <ø> (-0.4%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
js/src/constants.js 100.0% <ø> (ø)
js/src/data/actions.js 12.5% <ø> (-0.4%) ⬇️
src/API/Google/Middleware.php 91.2% <ø> (-0.6%) ⬇️
src/API/Site/Controllers/DisconnectController.php 94.4% <ø> (-0.2%) ⬇️
src/API/WP/OAuthService.php 100.0% <ø> (ø)
...ernal/DependencyManagement/CoreServiceProvider.php 0.0% <ø> (ø)
...ernal/DependencyManagement/RESTServiceProvider.php 100.0% <ø> (ø)
src/MerchantCenter/AccountService.php 87.2% <ø> (-1.6%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Both remaining AuthController handlers are dead: revoking a WPCOM OAuth
token is no longer valid now that the plugin uses client-credentials
auth (INTEGRA-48), and the PUT status-update flow could never succeed
since nothing writes the nonce it checks for (removed in the prior GET
flow cleanup). Removing both leaves AuthController with no route, so
the whole class goes, along with its exclusive dependents:
OAuthService::revoke_wpcom_api_auth/deactivate/get_wpcom_api_url,
AccountService::update_wpcom_api_authorization/reset_wpcom_api_authorization_data/
delete_wpcom_api_auth_nonce/delete_wpcom_api_status_transient, the
GOOGLE_WPCOM_AUTH_NONCE option, the DisconnectController fan-out entry
and its JS error special-case, and the useUpdateRestAPIAuthorizeStatusByUrlQuery
hook. AccountService::get_connected_status()'s client-credentials
health check is untouched.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@joemcgill joemcgill left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI Code Review

Issue: Clean Up remaining API Pull Access code
Branch: worktree-remove-dead-wpcom-auth -> develop

Summary

This PR correctly and completely removes all dead code from the defunct WPCOM app authorization GET/DELETE/PUT flow — I verified zero remaining references to any deleted symbol (AuthController, revoke_wpcom_api_auth, update_wpcom_api_authorization, GOOGLE_WPCOM_AUTH_NONCE, etc.) anywhere in the codebase. The still-live paths (get_connected_status(), is_wpcom_api_status_healthy(), the rest of disconnect-all-accounts) are intact. Two cleanup gaps and a latent (pre-existing, not introduced by this PR) status bug are worth a look before merge.

🟡 Suggestions

  • OAuthService is still registered as a full DI service (src/Internal/DependencyManagement/CoreServiceProvider.php:195,243-244): Verified — no code in the repo ever fetches an OAuthService instance from the container anymore; AccountService only references its two remaining constants (OAuthService::STATUS_APPROVED/STATUS_ERROR) statically. The share_with_tags( OAuthService::class ) call and its PROVIDES entry are dead weight from this same cleanup and should be removed to finish the job.

  • WPCOM_REST_API_STATUS has no reset path once it reaches 'error', but this predates this PR (src/MerchantCenter/AccountService.php:229-246): get_connected_status() only ever flips null → approved → error; nothing in the current codebase (before or after this PR) writes it back to empty. Previously, reset_wpcom_api_authorization_data() could clear it, but that method was only reachable via a successful legacy OAuth-token revoke (revoke_wpcom_api_auth()) — which, per this PR's own round-2 note, already can't succeed under client-credentials auth. So this PR isn't introducing a new regression, just removing code whose reset path was already unreachable. Still, worth flagging as a follow-up (same spirit as the round-1 note about the dead PUT route) since a merchant who ever lands in 'error' has no way back to 'approved' short of a manual option delete.

  • OAuthService class docblock is stale (src/API/WP/OAuthService.php): Still describes "a service to handle WordPress.com OAuth," but the class is now two status constants. Worth a one-line update so future readers don't assume it has active OAuth behavior.

🔵 Nitpicks

  • (src/Hooks/README.md:698): The woocommerce_gla_partner_app_auth_failure hook entry is stale — its only trigger site (get_sdi_auth_params()) was deleted in round 1. Carry-over from the prior review round.
  • (changelog.txt): No entry for either round's removals. Given the size of this cleanup (a full controller, service, and several AccountService methods), a Dev - entry in the next unreleased block is warranted. Carry-over from the prior review round.

This review was generated by Claude. A human review is still required before merging.

Nothing resolves OAuthService from the container anymore since
AuthController was deleted; AccountService only reads its two
remaining constants statically. Flagged in code review as leftover
dead weight from the same cleanup.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@joemcgill

Copy link
Copy Markdown
Collaborator Author

OAuthService is still registered as a full DI service

Addressed in 41f08c5

@joemcgill joemcgill marked this pull request as ready for review July 10, 2026 13:04
@joemcgill

Copy link
Copy Markdown
Collaborator Author

The following feedback came in from @eason9487 in Slack, which I plan to address before merging:

Hi @joemcgill, thanks for the cleanup PR! Could you also remove the call to account:connect in this PR?

MerchantAccountState lists sdi_update as a setup step.

return [ 'set_id', 'verify', 'link', 'claim', 'sdi_update', 'link_ads' ];

AccountService calls update_sdi_merchant_account() for that step

case 'sdi_update':
$middleware->update_sdi_merchant_account();

The account:connect endpoint in Middleware

public function get_sdi_merchant_update_endpoint(): string {
return $this->get_sdi_endpoint() . 'account:connect';

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