Woo REST API: fix Woo detection for non-eligible users #2628
Conversation
| return coroutineEngine.withDefaultContext(T.API, this, "Fetch site") { | ||
| val updatedSite = if (site.isUsingWpComRestApi) { | ||
| siteRestClient.fetchSite(site) | ||
| } else { | ||
| siteXMLRPCClient.fetchSite(site) | ||
| } | ||
|
|
||
| updateSite(updatedSite) |
There was a problem hiding this comment.
These functions in the SiteStore are suspendable, and yet, the DB logic wasn't offloaded correctly to the background thread, with this change, we follow the convention that the suspendable functions in the Stores use the default context.
This exact issue was fixed for fetchSites in #2254, and this PR updates the two other fetch functions.
| NOT_FOUND -> WooPayload(false) | ||
| FORBIDDEN -> { | ||
| // If we get a 403 status code, we can infer that Woo is installed, | ||
| // the user just doesn't have permission to access the Woo API |
There was a problem hiding this comment.
Just to discuss:
when a user doesn't get a permission to access API, is this exclusively because of their user role? Or can it be because of security plugins blocking certain users from using API, for example?
If there can be multiple reasons, do they need to be handled in this PR (maybe be clarified in comments)? I'm mostly asking because this PR seems to be tied to a task specifically about checking user role.
There was a problem hiding this comment.
when a user doesn't get a permission to access API, is this exclusively because of their user role? Or can it be because of security plugins blocking certain users from using API, for example?
We don't care about the cause here, our goal of this code is just to know whether Woo is installed or not, what happens later depends on other parts of the app: for example, we handle the role check, but if the cause is different, then the behavior won't be defined, same as what happens when accessing the site using WordPress.com or Jeptack CP.
Another point to keep in mind: this is needed just because we fetch the site using XMLRPC for now, if we get a chance to work on improving this part, we will handle the Woo detection differently, as we will be able to infer whether its present or not from fetching the root /wp-json/ endpoint, which will be used also to fetch site details (check my discussion with Huong here).
There was a problem hiding this comment.
Thank you for the explanation!
hafizrahman
left a comment
hafizrahman
left a comment
There was a problem hiding this comment.
This looks good to me, just added a comment for discussion.
When fetching a site using REST API or Jetpack CP, we use the endpoint
/wc/v3/settingsto infer if Woo is installed or not, and we were inferring a valid installation when we get a success, while 404 results in Woo missing, and other errors are reported as fetch errors.When working on the User role check for the REST API project, I faced an issue as the API returns
403. This PR adjusts the logic to handle the403status code too, and if we get it we can infer that Woo is installed, since this means that API call reached the Woo API controller and resulted in the permission error.For the second commit of the PR, check the comments below.
Testing
Please check the WCAndroid PR