Restrict API requests if the account is not connected #10304
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: -71 kB (-5%) ✅ Total Size: 1.29 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
The solution LGTM and works well!
I have small doubts whether to log error notices because it's considered normal situation - if account is not connected, we don't make requests to server (except onboarding and account fetch).
I suggested to show it as info level. Also I suggested more clear code comments. Pre-approving, we need only to align on log level and it's good to go from my POV.
@marcinbot could you make quick code-review only since you created this issue? 🙏
| return true; | ||
| } | ||
|
|
||
| $account = WC_Payments::get_account_service()->get_cached_account_data(); |
There was a problem hiding this comment.
Note that this method also sends a request so we should test for a scenario where this occurs.
I think something like infinite loop should be prevented with the self::ACCOUNTS_API === $api check above, but we could be potentially sending two requests here.
I think a safer thing to do might be to also check if the $api begins with the ACCOUNTS_API like we do for onboarding. Effectively whitelisting any accounts API calls.
There was a problem hiding this comment.
@dpaun1985 could you provide testing instructions how to test rejected/unlinked status? I tried updating it in DB, but after clearing account cache it becomes complete again
There was a problem hiding this comment.
pushed the changes in 88d4854 . thanks @marcinbot & @oaratovskyi
@oaratovskyi from what I could find, for unlinked status, there was a check introduced some months ago, and no account will be returned. For rejected status, I picked an account and linked it to my blog from Stripe dashboard.
There was a problem hiding this comment.
thanks @dpaun1985, changes LGTM!
For rejected status, I picked an account and linked it to my blog from Stripe dashboard.
you used console command or MC tool? I will try to reproduce locally
There was a problem hiding this comment.
you used console command or MC tool? I will try to reproduce locally
I used the link-account console command on platform side.
There was a problem hiding this comment.
Thank you for working on this, @dpaun1985! There is still some work left - see the comments.
Plus, we should have a (couple) of tests (in tests/unit/wc-payment-api/test-class-wc-payments-api-client.php) for the scenarios when requests are allowed and when they are not. I would just go through the various account states and call all public wrapper methods of the WC_Payments_API_Client, asserting which should be let through and which should be blocked. This way we can be confident that our changes don't inadvertently break something.
|
|
||
| $account = WC_Payments::get_account_service()->get_cached_account_data(); | ||
| // If the account is not set, we should not allow any API requests. | ||
| if ( ! is_array( $account ) || empty( $account ) ) { |
There was a problem hiding this comment.
WDYT about being a little more paranoid here and just check ! is_array( $account ) || empty( $account['status'] ) since the status entry should always be present?
If you agree, the isset( $account['status'] ) below can be removed.
| ->getMock(); | ||
| $mock_account_service | ||
| ->method( 'get_cached_account_data' ) | ||
| ->willReturn( [ 'id' => 1 ] ); |
There was a problem hiding this comment.
You should probably include a status entry also.
| ->getMock(); | ||
| $mock_account_service | ||
| ->method( 'get_cached_account_data' ) | ||
| ->willReturn( [ 'id' => 1 ] ); |
There was a problem hiding this comment.
You should probably include a status entry also.
| ->getMock(); | ||
| $mock_account_service | ||
| ->method( 'get_cached_account_data' ) | ||
| ->willReturn( [ 'id' => 1 ] ); |
There was a problem hiding this comment.
You should probably include a status entry also.
| @@ -2119,6 +2119,11 @@ public function send_request( Request $request ) { | |||
| * @throws API_Exception - If the account ID hasn't been set. | |||
| */ | |||
| protected function request( $params, $api, $method, $is_site_specific = true, $use_user_token = false, bool $raw_response = false ) { | |||
| if ( ! $this->is_api_request_allowed( $api ) ) { | |||
| Logger::info( "Platform API request '$api' wasn't executed. API requests are only performed if the account is connected." ); | |||
| return []; // this will result as 200 OK with empty array response. | |||
There was a problem hiding this comment.
I am not too happy or confident about returning 200 OK. It doesn't seem like the right thing to do.
To my mind, the caller needs to be informed that the request wasn't allowed rather than tricking it that it went through and there were no results.
I vote for throwing an API_Exception with a 403 HTTP response code because the request shouldn't have happened in the first place (someone didn't check the account state before calling).
I see no problem in throwing an exception and responding with a 403. The caller should be able to handle it - all public wrapper methods (e.g. WC_Payments_API_Client::get_payment_method) declare that they throw API_Exception.
WDYT @marcinbot? Is there something I am missing here?
There was a problem hiding this comment.
Not missing anything and tbh it rang the alarm bells for me as well. I'm just worried that there will be requests that don't catch things, which might lead to fatal errors on websites with no friendly messaging, or even bricking them completely.
TBH I'm not sure now if #9883 was a good idea to begin with: the API client shouldn't be responsible for checking the account status (the circular dependency on the account class is an indicator of this). It also takes control away from the callers. The API client should just be responsible for sending out authenticated HTTP requests.
And also there's too much risk in either returning a 200 or throwing.
The account check should happen in the caller, if necessary. The issue came on the back of one such problems (fetching the disputes on every page load #9716) and was addressed in that way there.
@dpaun1985 @oaratovskyi - would you be ok with closing this PR and #9883 without shipping it?
There was a problem hiding this comment.
the API client shouldn't be responsible for checking the account status (the circular dependency on the account class is an indicator of this). It also takes control away from the callers. The API client should just be responsible for sending out authenticated HTTP requests.
totally agree with you
would you be ok with closing this PR and #9883 without shipping it?
in this case yes, we can close this issue
thanks for the feedback @marcinbot
There was a problem hiding this comment.
Thanks! And I appreciate all the work that went into this! Let's close then.
|
After the discussions above, we've decided to close this issue as it will not be shipped. |
Fixes #9883
Changes proposed in this Pull Request
This PR reduces redundant server requests by restricting unnecessary client-side calls, that can, in some cases, spam the server ( ex. p1730711562014049-slack-C03V0SP07SL ).
To achieve this, requests to all endpoints—except for fetching account details and onboarding—will be restricted when:
Testing instructions
Pre-requirements:
Woocommerce -> Settings -> Advanced -> REST APIand create api credentialsTesting instructions
GET http://localhost:8082/wp-json/wc/v3/payments/transactions/summary?deposit_id=&_locale=user.Woocommerce -> Status -> Logs. The error should be logged.unlinkedorrejected....)GET http://localhost:8082/wp-json/wc/v3/payments/transactions/summary?deposit_id=&_locale=user.Woocommerce -> Status -> Logs. NO new errors should be logged.Also some regression testing would be good, to be sure that we don't block anything without intent.
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge