Skip to content

Conversation

@crazytonyli
Copy link
Contributor

I noticed a crash when testing #1126. The refresh application password logic that's built into the request builder macros is also running in WpComApiClient, which should not happen.

I have added an integration test to reproduce the crash. The crash happens when the following criteria are met:

  1. WpComApiClient is used to request a WP.com endpoint.
  2. An invalid token is used.
  3. The endpoint returns a 401 response.

get_support_conversation_list is one example.

@crazytonyli crazytonyli requested a review from jkmassel January 28, 2026 06:59
Copy link
Contributor

@jkmassel jkmassel left a comment

Choose a reason for hiding this comment

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

I wonder if we're solving this at the wrong layer? Should WpAuthenticationProvider have fetch_authentication_state so that the macro can allow each type of authentication to do the right thing? So then for Application Passwords we call ApplicationPasswordsRequestBuilder.retrieve_current_with_edit_context but for OAuth tokens we can inject some other implementation?

@crazytonyli
Copy link
Contributor Author

The refresh application password logic that's built into the request builder macros is also running in WpComApiClient, which should not happen.

Sorry, I was wrong about this in the PR description. At the moment, the refresh application password logic does not run in WpComApiClient. So it's not actually relevant to the crash fixed in this PR.


The the main purpose of fetch_authentication_state is checking if the WpAuthenticationProvider instance is still valid. I don't think WpAuthenticationProvider should do the check, because it has no context of how it's being used.

I feel like it might be more appropriate to move fetch_authentication_state to ApiUrlResolver. Maybe we shouldn't call it "url resolver" anymore. Something like HostingProvider may be more suitable? The "hosting provider" should know how to construct a wp/v2/ URL, and since it may also have its own authentication method, it should also know how to check if a WpAuthenticationProvider value is still valid.

#[uniffi::export(with_foreign)]
pub trait HostingProvider: Send + Sync {
    fn resolve(&self, namespace: String, endpoint_segments: Vec<String>) -> Arc<ParsedUrl>;

    fn authentication_state(&self, request_executor: Arc<dyn RequestExecutor>, auth: Arc<WpAuthenticationProvider>) -> Result<AuthenticationState, WpApiError>;
}

@crazytonyli crazytonyli requested a review from jkmassel February 1, 2026 22:05
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.

3 participants