23302 manage user consent through yoast ai#23306
Conversation
pls78
added
the
changelog: non-user-facing
pls78
changed the base branch from
trunk
to
23301-make-the-token_manager-consent-agnostic
Coverage Report for CI Build 0Coverage decreased (-1.1%) to 52.711%Details
Uncovered Changes
Coverage Regressions4 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
pls78
force-pushed
the
23302-manage-user-consent-through-yoast-ai
branch
from
b0f2e6e to
879a3d1
Compare
pls78
force-pushed
the
23302-manage-user-consent-through-yoast-ai
branch
from
879a3d1 to
a2074da
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the AI consent flow so that granting/revoking consent updates both the local user meta and the Yoast AI service’s per-user consent state. It also refactors the AI HTTP request layer to use an explicit HTTP method (GET/POST/DELETE) so the new DELETE /user/consent endpoint can be called.
Changes:
- Refactors AI HTTP requests from a
bool $is_postflag to astring $http_methodwithRequest::METHOD_*constants (adds DELETE support). - Updates consent handling to call the Yoast AI service when granting/revoking consent, with transactional grant and security-first revoke behavior.
- Extends/updates unit tests to cover the new request method handling and consent sync flows (including DELETE).
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/AI/HTTP_Request/Infrastructure/API_Client/Perform_Request_Post_Test.php | Updates API client unit test to pass explicit POST method constant. |
| tests/Unit/AI/HTTP_Request/Infrastructure/API_Client/Perform_Request_Get_Test.php | Updates API client unit test to pass explicit GET method constant. |
| tests/Unit/AI/HTTP_Request/Infrastructure/API_Client/Perform_Request_Error_Test.php | Updates API client error-path unit test to pass explicit POST method constant. |
| tests/Unit/AI/HTTP_Request/Infrastructure/API_Client/Perform_Request_Delete_Test.php | Adds unit test coverage for DELETE requests via wp_remote_request. |
| tests/Unit/AI/HTTP_Request/Application/Request_Handler/Handle_Test.php | Updates request handler unit test to use get_http_method() instead of is_post(). |
| tests/Unit/AI/Consent/Application/Consent_Handler/Revoke_Consent_Test.php | Expands revoke consent tests to cover happy path + swallowed HTTP-layer failures + non-swallowed runtime exceptions. |
| tests/Unit/AI/Consent/Application/Consent_Handler/Grant_Consent_Test.php | Expands grant consent tests to cover happy path + propagated failures (transactional behavior). |
| tests/Unit/AI/Consent/Application/Consent_Handler/Constructor_Test.php | Updates constructor test for new injected dependencies (token manager, request handler, logger). |
| tests/Unit/AI/Consent/Application/Consent_Handler/Abstract_Consent_Handler_Test.php | Updates test base to construct Consent_Handler with new dependencies and stub get_user_by. |
| src/ai/http-request/infrastructure/api-client.php | Implements switching on HTTP method and adds DELETE support. |
| src/ai/http-request/infrastructure/api-client-interface.php | Updates API client interface to accept an HTTP method string instead of a boolean. |
| src/ai/http-request/domain/request.php | Replaces is_post with validated http_method and adds METHOD_* constants. |
| src/ai/http-request/application/request-handler.php | Passes through the request’s HTTP method to the API client. |
| src/ai/generator/user-interface/get-usage-route.php | Updates usage call to build a GET request using Request::METHOD_GET. |
| src/ai/consent/application/consent-handler.php | Implements remote consent grant/revoke calls (POST/DELETE) and logging on revoke failures. |
| src/ai-http-request/infrastructure/api-client.php | Same HTTP method refactor + DELETE support for the legacy AI_HTTP_Request namespace path. |
| src/ai-http-request/infrastructure/api-client-interface.php | Same interface signature update for the legacy AI_HTTP_Request namespace path. |
| src/ai-http-request/domain/request.php | Same request HTTP method refactor for the legacy AI_HTTP_Request namespace path. |
| src/ai-http-request/application/request-handler.php | Same request handler update to pass get_http_method() for the legacy path. |
| src/ai-generator/user-interface/get-usage-route.php | Same usage route update to use Request::METHOD_GET for the legacy path. |
| src/ai-consent/application/consent-handler.php | Same remote consent grant/revoke implementation for the legacy AI_Consent namespace path. |
| public function grant_consent( int $user_id ) { | ||
| $user = \get_user_by( 'id', $user_id ); | ||
| $jwt = $this->token_manager->get_or_request_access_token( $user ); | ||
|
|
There was a problem hiding this comment.
I think it's a valid feedback but then again the way we use this doesn't leave a window for the user to be false (because we're doing a get_current_user_id() going in).
That might change in the future, so maybe we want this defensive coding. But then again, we do a similar thing to every get_or_request_access_token() usage, so I'll leave it up for debate for now.
There was a problem hiding this comment.
I agree, let's be future-proof when we can 🙂
leonidasmi
left a comment
leonidasmi
left a comment
There was a problem hiding this comment.
CR: ❌
Aside from the inline comments, I also want us to consider adding proper UX for the unhappy paths, at least for the granting consent flow. Explaining:
- If a request that grants consent fails, right now nothing indicates that it did
- For doing so via the Profile page means that the consent modal is closed, the button stays at
Grant Consentand the user is left wondering what happened - For doing so right before we use an AI feature in the editor, it even goes to the next screen even if consent wasn't really given
- For doing so via the Profile page means that the consent modal is closed, the button stays at
It does increase the scope significantly though, so let's discuss.
| $user = \get_user_by( 'id', $user_id ); | ||
| $jwt = $this->token_manager->get_or_request_access_token( $user ); | ||
|
|
||
| $this->request_handler->handle( |
There was a problem hiding this comment.
I understand that grant_consent() is transactional and that if that request to the AI API fails there's nothing we want to do, but why not catching Remote_Request_Exception | WP_Request_Exception errors here as well, much like how we do when revoking consent?
This means that if the API returns with an errored response or is simply unavailable when a user grants consent, we are going to throw a fatal and I think we should avoid that and fail gracefully.
There was a problem hiding this comment.
I have implemented the error management part and proper UI: will detail this in the Relevant technical choices section.
| public function grant_consent( int $user_id ) { | ||
| $user = \get_user_by( 'id', $user_id ); | ||
| $jwt = $this->token_manager->get_or_request_access_token( $user ); | ||
|
|
There was a problem hiding this comment.
I think it's a valid feedback but then again the way we use this doesn't leave a window for the user to be false (because we're doing a get_current_user_id() going in).
That might change in the future, so maybe we want this defensive coding. But then again, we do a similar thing to every get_or_request_access_token() usage, so I'll leave it up for debate for now.
| */ | ||
| public function revoke_consent( int $user_id ) { | ||
| $this->user_helper->delete_meta( $user_id, '_yoast_wpseo_ai_consent' ); | ||
| public function grant_consent( int $user_id ) { |
There was a problem hiding this comment.
here we mark the $user_id as int in other places (eg. in the token_invalidate( $user_id ) we mark it as string. Maybe worth to unify?
There was a problem hiding this comment.
Done, Token_Manager was wrongly treating user ids as strings.
No need to specify only some of them
…ring error management to the caller
…still flag the local consent as revoked)
The component is now used also in the user profile page where the only store available is yoast-seo/ai-consent. Conversely, in the Block editor data is fetched from yoast-seo/ai-generator
Also pass the store name to use
This reflects the local state.
Also, we don't want to show the spinner when the request errored
leonidasmi
left a comment
leonidasmi
left a comment
There was a problem hiding this comment.
Aside from the stuff that I already shared with this review, there's also this point:
Tokens may not be invalidated on a failed remote revoke (behavioral regression, security-relevant).
In consent-route.php:124-129:
$this->consent_handler->revoke_consent( $user_id ); // nowTHROWS on remote DELETE failure$this->token_manager->token_invalidate( $user_id ); // skippedwhen the above throws
Previously revoke swallowed remote failures, so token_invalidate() (which also calls clear_tokens() to delete the cached access/refresh JWTs locally — token-manager.php:156) always ran. Now, if the remote DELETE /user/consent fails, the exception propagates, the catch returns 500, and token_invalidate() never runs. Net result: local consent meta is cleared but the user's cached JWT tokens remain on the site.
That contradicts the "security-first" intent. Consider invalidating tokens even when the remote revoke fails (e.g. move token_invalidate so it runs regardless, or invalidate inside the handler before the remote DELETE). Worth a deliberate decision rather than an accident of ordering.
| return new WP_REST_Response( ( $consent ) ? 'Failed to store consent.' : 'Failed to revoke consent.', 500 ); | ||
| } catch ( Remote_Request_Exception | RuntimeException $e ) { | ||
| $this->logger->error( $e->getMessage(), [ 'exception' => $e ] ); | ||
| return new WP_REST_Response( ( $consent ) ? 'Failed to give consent.' : 'Failed to revoke consent.', 500 ); |
There was a problem hiding this comment.
instead of hardcoding 500, we should use the code in the $e
| { error && | ||
| <GenericAlert className="yst-mt-3" linkStoreName={ linkStoreName } /> | ||
| } |
There was a problem hiding this comment.
This probably needs some margin between the text and the alert. Can we get an approval from the UX team for this?
| $jwt = $this->token_manager->get_or_request_access_token( $user ); | ||
|
|
||
| $this->request_handler->handle( | ||
| new Request( '/user/consent', [], [ 'Authorization' => "Bearer $jwt" ], Request::METHOD_POST ), |
There was a problem hiding this comment.
This now breaks the AI API contract and requests fail with 400 - request/body must be object as a result.
The merge of #23306 replaced Request::is_post (bool) with an http_method string enum, but several call sites in the OAuth auth path still used the old API. These would throw or hit the wrong URL at runtime: - Request::with_added_headers() passed the removed $this->is_post property, breaking every request sent through the Token auth path. - API_Client's DELETE branch appended the action path twice, since get_url() already includes it; consent revocation hit /user/consent/user/consent. - AI_Request_Sender::get_usage() built a Request with a bool, which the new constructor rejects with InvalidArgumentException. - OAuth_Auth_Strategy::send() called the removed Request::is_post(). Update the affected unit tests to assert on get_http_method() instead of is_post(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
RequestandAPI_Clientfrom abool $is_postflag to astring $http_method(withRequest::METHOD_GET/POST/DELETEconstants), to accommodate the newDELETE /user/consentcall.grant_consent()is transactional (any HTTP failure → the PSR-3 logger + propagate, local meta untouched: this means the local meta is only written if the remote call succeeded);revoke_consent()is security-first (HTTP failure → log via the PSR-3 logger + propagate, local meta always cleared no matter what).Consent_Handler(theConsent_Routeclass), which returns an error responseTest instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
wp-content/mu-plugins/api-calls.phpor through a plugin to log every outbound HTTP call toyoa.stin the PHP error log:tail -f wp-content/debug.logwithWP_DEBUG_LOGon).Test the Happy path
Grant consent, click it and confirm in the modal.Revoke consentand confirm in the modal.Test the grant consent failure
wp-content/mu-plugins/yoast-ai-failure-sim.php. It pre-empts thePOST /user/consentcall with a synthetic 500 and wires up a concrete PSR-3 logger implementation so you can verify the error is logged:From the user profile:
Grant consent.Verify an error message appears in the modal as shown below:
Verify the AI features stay disabled.
In
wp_usermeta, verify_yoast_wpseo_ai_consentfor your user is not set (local meta is left untouched).In the error log, verify you see a
[Yoast ERROR]entry containingSimulated AI service failure.From the Block editor:
Open any post in the Block editor and trigger an AI feature from the Yoast sidebar.
[Yoast ERROR]entry as above.Remove
yoast-ai-failure-sim.phpwhen done with this section.Test the revoke consent failure
Re-install the failure-sim mu-plugin from the previous section, but change
'POST'to'DELETE'in the method check.Make sure your user currently HAS consent (grant first if needed).
Visit your WordPress user Profile and click
Revoke consent.wp_usermeta, verify_yoast_wpseo_ai_consentfor your user is deleted despite the simulated failure.[Yoast ERROR]entry containingSimulated AI service failure.Remove
yoast-ai-failure-sim.phpwhen done with this section.Test the user has correct permissions
Log in as a Subscriber (a user without
edit_posts).Attempt to call
POST /wp-json/yoast/v1/ai_generator/consentdirectly (e.g. via the browser console or a REST client).403 Forbidden.Relevant test scenarios
Test instructions for QA when the code is in the RC
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
Request/API_Clienttouches the primitive used by every outbound AI service call. Worth a smoke test of all AI-related features (Generate, Optimize, Summarize, Content Planner, Usage) on top of the consent-specific tests above. Please note https://github.com/Yoast/wordpress-seo-premium/pull/4971 needs to be merged first/checked out to test Premium features with this PR.UI changes
Other environments
[shopify-seo], added test instructions for Shopify and attached theShopifylabel to this PR.Documentation
Quality assurance
Innovation
innovationlabel.Fixes #23302