feat: authenticate yoast-ai requests with MyYoast OAuth tokens#23265
Open
thijsoo wants to merge 9 commits into
Open
feat: authenticate yoast-ai requests with MyYoast OAuth tokens#23265thijsoo wants to merge 9 commits into
thijsoo wants to merge 9 commits into
Conversation
Extends the AI HTTP layer so callers (specifically the upcoming OAuth auth strategy) can inspect response headers on thrown exceptions and build a DPoP proof's `htu` claim against the same URL the request will actually hit. - Request: add `with_added_headers()` and `with_added_body()` immutable helpers so strategies can decorate a request without callers pre-building auth headers. - Response: add headers field, lower-cased at the boundary by Response_Parser::normalize_headers() so callers can do plain array lookups. - Remote_Request_Exception and Payment_Required_Exception: optional response_headers constructor argument + getter so 401s can carry the DPoP-Nonce / WWW-Authenticate metadata needed to detect nonce challenges and insufficient_scope errors. - New Insufficient_Scope_Exception subclass of Forbidden_Exception so callers can distinguish scope errors from consent revocation. - API_Client: expose get_url() so the OAuth path can resolve the same URL the actual request will use (via the existing Yoast\\WP\\SEO\\ai_api_url filter). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The AI module's new OAuth auth strategy needs to build DPoP-bound requests without routing AI traffic through MyYoast_Client's own HTTP_Client. Three small additions to the facade enable that: - create_dpop_proof( method, url, Token_Set ): delegates to DPoP_Handler::create_proof(). - store_dpop_nonce( headers ): wrapper for DPoP_Handler::handle_nonce_response() so the AI strategy can stash a server-issued nonce before retry. - is_site_connected(): O(1) option-backed flag indicating whether any admin has completed the auth-code flow on this site. Set by exchange_authorization_code() on first successful exchange, cleared by deregister(). Gates the OAuth path so we don't issue client_credentials tokens before MyYoast has a verified redirect URI for the client (which would yield tokens missing the site_url claim yoast-ai requires). New DPoP_Proof_Provider_Interface port in application/ports/ keeps the Application layer free of direct Infrastructure dependencies; DPoP_Handler implements it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduces src/ai/authentication/ — a small onion module that selects between two auth strategies for outbound yoast-ai calls: - OAuth_Auth_Strategy: fetches a site-wide MyYoast OAuth token via client_credentials, generates a DPoP proof, decorates the AI Request with `Authorization: DPoP <token>` + `DPoP: <proof>` headers and a `user_id` body field, then dispatches via the existing AI Request_Handler/API_Client (so response parsing and exception mapping stay uniform). Bounded retries: nonce-challenge retry per RFC 9449 §8, invalid_token retry, then fall back to the Token strategy on persistent failure. - Token_Auth_Strategy: the legacy `access_jwt` flow. Pulls JWT via Token_Manager, attaches `Authorization: Bearer …`, retries once on 401 with fresh tokens. - Auth_Strategy_Factory: selects via wpseo_ai_auth_method filter override → YOAST_SEO_MYYOAST_CONNECTION feature flag → MyYoast_Client::is_registered() → MyYoast_Client::is_site_connected(). The OAuth model is site-wide: once any admin completes the auth-code flow, every WP user on the site uses the OAuth path. The shared site-level token is reused across users; the current user's WP id is self-reported in the request body for endpoints that need per-user identity. Insufficient_Scope_Exception is thrown distinctly from generic Forbidden_Exception so callers can distinguish scope errors from consent revocation. Includes 16 unit tests covering factory selection (5), OAuth strategy (8 — including shared-token-across-users, nonce challenge retry, invalid_token retry, repeated 401 fallback, insufficient_scope, token request failed, DPoP proof failure, non-user paths), and Token strategy (3 — happy path, 401 retry, persistent 401). Refs: Yoast/reserved-tasks#1207 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tory Switches the four direct yoast-ai consumers from constructing auth headers inline to asking the new Auth_Strategy_Factory for a strategy and delegating the dispatch: - Suggestions_Provider - Get_Usage_Route - Content_Outline_Command_Handler - Content_Suggestion_Command_Handler Each caller now passes only its domain headers (X-Yst-Cohort, etc.) into the Request and lets the chosen strategy add the auth header. The inline 401-retry block previously living in Suggestions_Provider moves into Token_Auth_Strategy so all callers benefit uniformly. The Forbidden_Exception → consent_revoke mapping stays in the caller layer (consent semantics belong there, not in the auth layer), but each caller now catches Insufficient_Scope_Exception first and re-throws it untouched — scope errors are a deployment/token-issuance problem and must not silently revoke the user's consent. Tests updated to mock Auth_Strategy_Factory + Auth_Strategy_Interface instead of Token_Manager + Request_Handler. Behavioural assertions on the route/provider stay the same. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thijsoo
added
innovation
- Inline the wpseo_ai_auth_method filter name in the factory so phpcs can see the wpseo_ prefix (it can't follow the constant through apply_filters()'s dynamic-hookname check). - Drop unused $request parameters from three test closures that pass through Mockery::andReturnUsing(). The closures only care about call count, not the arguments. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…atch
Splits the conflated `send()` responsibility on Auth_Strategy_Interface
into a pure decorator + a recovery hook, and introduces AI_Request_Sender
that owns dispatch, retry orchestration, and the optional fallback path.
Auth_Strategy_Interface (new shape):
- decorate(Request, WP_User): Request — attach auth headers/body.
- on_failure(Request, WP_User, Remote_Request_Exception, int): bool —
strategy-specific recovery; returns true to retry, false to give up.
May throw to override propagation with a typed exception.
AI_Request_Sender (new): the only public dispatch surface for AI calls.
Runs decorate → handle → on_failure loop with MAX_ATTEMPTS = 3. On
on_failure returning false, tries the configured fallback strategy.
Hard cap is enforced by throwing Auth_Strategy_Loop_Exception (a
LogicException sentinel — distinct from Token_Manager's RuntimeException
so callers can tell a buggy strategy from a real token-retrieval failure)
and is deliberately fail-closed: loop-exhausted does NOT trigger fallback.
OAuth_Auth_Strategy and Token_Auth_Strategy:
- Lost their Request_Handler dependency (dispatch moved to the sender).
- OAuth lost its Token_Auth_Strategy dependency (fallback wiring moved
to the factory).
- decorate() translates internal exceptions (Token_Request_Failed,
DPoP_Proof_Exception) into Bad_Request_Exception so callers see a
uniform exception surface.
- on_failure handles strategy-specific recovery + exception translation.
AI_Request_Sender_Factory (renamed from Auth_Strategy_Factory): now
returns a configured AI_Request_Sender (primary + optional fallback)
instead of a strategy. Same selection logic as before; callers do
`$sender = $factory->create($user); $sender->send($request, $user);`.
Code-review fixes applied on top of the refactor:
- Auth_Strategy_Loop_Exception (typed LogicException) replaces the
bare RuntimeException sentinel that previously collided with
Token_Manager::get_or_request_access_token's documented RuntimeException.
- OAuth_Forbidden_Exception (extends Forbidden_Exception): OAuth's
on_failure now translates non-scope 403s into this typed exception,
matching the pattern we already use for Insufficient_Scope_Exception.
The sender catches both before the generic Remote_Request_Exception
so neither triggers fallback (consent semantics don't apply on the
OAuth wire; falling back to Token would mask the real failure mode).
Callers (Suggestions_Provider, both content-planner handlers) catch
OAuth_Forbidden_Exception ahead of Forbidden_Exception so consent
isn't revoked on the user's behalf for an OAuth-wire 403.
- MyYoast_Client::is_site_connected() reverted to has_any_user_token()
— the original name accurately reflects what the underlying flag
tracks (whether any user has ever obtained a token via the auth-code
flow on this site). Docblock notes that the internal option key
keeps its historical SITE_CONNECTED_OPTION name to avoid storage
migration.
Refs: Yoast/reserved-tasks#1207
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Coverage Report for CI Build 0Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Warning No base build found for commit Coverage: 50.229%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
Wires up LoggerAwareInterface + NullLogger across the four authentication classes, matching the pattern used by MyYoast_Client and its handlers. The compiled DI container auto-wires the real Yoast\WP\SEO\Loggers\Logger at runtime; the NullLogger default keeps tests silent. Log points are concentrated where they aid debugging: - AI_Request_Sender_Factory: debug per selection branch (why oauth vs token was chosen — useful for "why didn't OAuth fire on this site?" reports). - OAuth_Auth_Strategy::decorate: warning on the two exception translations (Token_Request_Failed → Bad_Request, DPoP_Proof_Exception → Bad_Request), since the original cause is otherwise hidden behind the generic Bad_Request. - OAuth_Auth_Strategy::on_failure: debug on recovery actions (nonce stash, cached-token clear); warning on insufficient_scope and OAuth_Forbidden propagation. - Token_Auth_Strategy::on_failure: debug on the 401 → clear-JWTs retry. - AI_Request_Sender::send: warning when the primary exhausts recovery and fallback engages; error when the loop-budget sentinel trips (paired with the typed Auth_Strategy_Loop_Exception throw). Also moves Auth_Strategy_Loop_Exception from the application layer to domain/exceptions/ so all three new exception types (Insufficient_Scope, OAuth_Forbidden, Auth_Strategy_Loop) consistently live in a Domain layer. New namespace: Yoast\\WP\\SEO\\AI\\Authentication\\Domain\\Exceptions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
diedexx
reviewed
May 20, 2026
Member
diedexx
left a comment
diedexx
left a comment
There was a problem hiding this comment.
I put us in quite the awkward spot where the MyYoast client wants to do and handle request nonce errors itself. As does the Yoast AI client. Alternatively we expose some of its internals, like you did with the DPoP utilities. We should be careful that we don't end up repeating the same dpop/retry pattern over and over, everywhere we use one of our resource servers.
I think there could be solution where we don't have to leak internals of either auth approach. I'd love to brainstorm some ideas with you if you're open to it
| * | ||
| * @return bool True if the flag is currently set. | ||
| */ | ||
| public function has_any_user_token(): bool { |
Member
There was a problem hiding this comment.
Please use consistent naming with the mark_site_connected and clear_site_connected methods. The comment above this makes it sound like there's been a refactor/change of heart while working on this branch, but I don't think this reached any release yet.
Comment on lines
+501
to
+503
| if ( $this->has_any_user_token() ) { | ||
| return; | ||
| } |
Member
There was a problem hiding this comment.
update_option will do a get_option and short-circuit when there's no change to the value. We can simplify this method.
Suggested change
| if ( $this->has_any_user_token() ) { | |
| return; | |
| } |
| */ | ||
| public function decorate( Request $request, WP_User $user ): Request { | ||
| try { | ||
| $token_set = $this->myyoast_client->get_site_token( [ self::AI_SCOPE ] ); |
Member
There was a problem hiding this comment.
This should also require a resource parameter (RFC 8707), which the client doesn't support yet. I'll open up a PR to add support for it later.
| } catch ( Token_Request_Failed_Exception | Token_Storage_Exception $exception ) { | ||
| $this->logger->warning( 'OAuth decorate: site token unavailable ({error}); surfacing as OAUTH_TOKEN_UNAVAILABLE.', [ 'error' => $exception->getMessage() ] ); | ||
| // phpcs:disable WordPress.Security.EscapeOutput.ExceptionNotEscaped -- Internal exception data, not output. | ||
| throw new Bad_Request_Exception( 'OAUTH_TOKEN_UNAVAILABLE', 0, 'OAUTH_TOKEN_UNAVAILABLE', $exception ); |
Member
There was a problem hiding this comment.
I'm not sure Bad_Request_Exception is the right fit here. It's linked to the HTTP error respose type, but no request has been made. Having a layer-specific exception type would help understand and resolve the true error. e.g. Unmet_Auth_Strategy_Condition_Exception or something like that
| } catch ( DPoP_Proof_Exception $exception ) { | ||
| $this->logger->warning( 'OAuth decorate: DPoP proof generation failed ({error}); surfacing as DPOP_PROOF_FAILED.', [ 'error' => $exception->getMessage() ] ); | ||
| // phpcs:disable WordPress.Security.EscapeOutput.ExceptionNotEscaped -- Internal exception data, not output. | ||
| throw new Bad_Request_Exception( 'DPOP_PROOF_FAILED', 0, 'DPOP_PROOF_FAILED', $exception ); |
Comment on lines
+44
to
+61
| /** | ||
| * Path prefixes whose handler reads user identity from the request body. We forward the WP user id | ||
| * for these because the site-level OAuth token is shared across users (client_credentials) and | ||
| * yoast-ai needs the body field to run per-user license/usage checks. | ||
| * | ||
| * The set is deliberately narrow: only POST endpoints that today rely on JWT-encoded user identity | ||
| * AND build their request body via Suggestions_Provider / the content-planner handlers. The usage | ||
| * endpoint (/usage/...) is a GET with an empty body — API_Client drops the body for GET requests, | ||
| * so adding /usage/ here would silently lose the user_id. The yoast-ai team will need to expose | ||
| * usage identity through a different surface (query parameter or token claim) when the OAuth path | ||
| * becomes the default; tracked separately from this issue. | ||
| * | ||
| * @var string[] | ||
| */ | ||
| private const USER_BOUND_PATH_PREFIXES = [ | ||
| '/openai/suggestions/', | ||
| '/content-planner/', | ||
| ]; |
Member
There was a problem hiding this comment.
The id is extracted on the global level: when oauth is used, we want the externalUserId in either the query params (GET) or request body (POST/PUT/ETC). Easiest to just always include it when going through the oauth strategy
Member
There was a problem hiding this comment.
For simplicity sake we could consider always sending it. Even for the legacy path, as it's not strictly part of auth.
| * Thrown when the yoast-ai service returns a 403 on the MyYoast OAuth wire that isn't a scope error. | ||
| * | ||
| * Distinct from a generic Forbidden_Exception because the recovery path differs: a Token-flow 403 | ||
| * means consent was revoked (the caller clears the user's consent), but on the OAuth wire there is |
Member
There was a problem hiding this comment.
This is not right. even with oauth, we expliclty need to note user consent. With the legacy flow the authorization implicitly marks consent, but this doesn't happen with the new flow.
| * | ||
| * @return void | ||
| */ | ||
| public function handle_nonce_response( array $response_headers ): void; |
Member
There was a problem hiding this comment.
I think this is a bit of a smell. A Proof Provider doesn't know the first thing about nonces.
| public function exchange_authorization_code( int $user_id, string $code, string $state ): Token_Set { | ||
| $token_set = $this->auth_code_handler->exchange_code( $user_id, $code, $state ); | ||
| $this->user_token_storage->store( $user_id, $token_set ); | ||
| $this->mark_site_connected(); |
Member
There was a problem hiding this comment.
Ideally the myyoast-client facade is only a window to the rest of this module; a single entrypoint. It shouldn't be responsible for state/data integrity. You could end up with a faulty state when using the auth_code_handler directly. Maybe this connected state should be part of the Registered_Client model and its storage. The Auth_Code_Handler and Client_Registration could then be responsible for setting and clearing it (the latter would automatically be done by deregistering if the client and its connected state share the same storage).
…istration Push ownership of the "site has completed the auth-code flow" flag out of the MyYoast_Client facade. The flag now lives behind the Client_Registration_Interface port (is_site_connected / mark_site_connected); Authorization_Code_Handler sets it on a successful exchange, and the registration adapter clears it as part of forget_registration / delete_local_data so deregistration and stale-redirect-URI re-registration both reset it for free. The facade keeps a one-line is_site_connected() pass-through; the prior has_any_user_token / mark_site_connected / clear_site_connected members are removed outright, with no deprecation shim, since none of the new names have shipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Token_Managerflow (callback-deliveredaccess_jwtstored in user meta +Authorization: Bearer …). That handshake fails on unreachable / firewalled / local / Cloudflare-protected sites, which is the main reason AI features break in those environments.MyYoast_Client(src/myyoast-client/), a full OAuth/OIDC client with DPoP support. yoast-ai is being updated in Yoast/yoast-ai#337 to accept DPoP-bound MyYoast access tokens additively, alongside the legacy path.Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
client_credentialstoken is reused across users; the current user's WP id is self-reported in the request body for endpoints that need per-user identity. Required because MyYoast only embeds thesite_urlclaim inclient_credentialstokens after the client has at least one verified redirect URI.Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Do these tests on a site that is reachable from the outside.
Make sure you have the newest test helper installed to be able to choose which my yoast staging to use.
Also make sure you enable DB or file logging in the test helper
** Feature flag off (default behaviour, should be unchanged):**
YOAST_SEO_MYYOAST_CONNECTIONis NOT defined inwp-config.php(or isfalse).trunk. No regressions in AI features.Feature flag on but site not yet connected to MyYoast:
define( 'YOAST_SEO_MYYOAST_CONNECTION', true );towp-config.php.Path C — Feature flag on AND site connected (requires yoast-ai#337 deployed to staging):
/wp-admin/tools.php?page=yoast-test-helper.Domain Dropdownselectstaging-5as your MyYoast testing domain.Personal access tokenand copy this token.MyYoast PAT for this environmentand save.Stored credentials: yeswp yoast auth registerto register the site you are using with the staging my yoast (at a later stage this will be replaced by some UI).Authorization: DPoP …plus aDPoPproof header.wp yoast auth deregister.is_site_connected()returnsfalseafter deregister.Path D — Consent revocation still works:
Relevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
Get_Usage_Route).Insufficient_Scope_Exceptionpropagate unrevoked).Other environments
[shopify-seo], added test instructions for Shopify and attached theShopifylabel to this PR.[yoast-doc-extension], added test instructions for Yoast SEO for Google Docs and attached theGoogle Docs Add-onlabel to this PR.Documentation
MyYoast_Client::authenticated_request().Quality assurance
grunt build:imagesand committed the results, if my PR introduces or edits images or SVGs. (Not applicable — no image changes.)Innovation
innovationlabel.Fixes Yoast/reserved-tasks#1207