-
Notifications
You must be signed in to change notification settings - Fork 2
Introduce WpAppNotifier
#693
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
wp_api/src/lib.rs
Outdated
impl WpAppNotification { | ||
pub fn from_wp_network_response(response: &WpNetworkResponse) -> Option<Self> { | ||
if response.status_code == 401 { | ||
Some(Self::UnauthorizedRequest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the purpose of finding out whether the current application password is still valid, only a 401 status code check is not enough, since not all 401 responses mean that an invalid application password is used.
We'll probably want to use a stricter check like
fn is_unauthorized_error(response: &WpNetworkResponse) -> bool {
if response.status_code == 401 {
if let Some(original_error) = WpError::try_parse(&response.body.clone()) {
return original_error.code == WpErrorCode::Unauthorized;
}
}
false
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we always get back WpErrorCode::Unauthorized
. For example,
> curl --user [email protected]:invalid "http://localhost/wp-json/wp/v2/users?context=edit"
> {"code":"rest_forbidden_context","message":"Sorry, you are not allowed to list users.","data":{"status":401}}
So, at least for now, I have changed the condition to be true for both WpErrorCode::Unauthorized
and WpErrorCode::ForbiddenContext
in 8085903. Let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should check whether the HTTP response status code is 401 instead? So far, it looks like the core would always return a 401 status code, but may return a different {"code": ...}
in the response JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initial implementation was checking for 401
, but I tried updating it as per your previous suggestion. I agree though, I think we should only check the status code since we are going to make a request to check if the application password is valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right. The original approach of creating WpAppNotification
from HTTP response probably won't work. But I see it's updated to invoke a callback after application password check. 👍
fn api_client_as_unauthenticated_with_notifier(notifier: Arc<FooAppNotifier>) -> WpApiClient { | ||
WpApiClient::new( | ||
test_site_url(), | ||
WpAuthentication::None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test set up is not suitable for the purpose of detecting an invalid application password. We should pass a hard-coded username and invalid application password pair here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in d3b5895.
wp_api/src/lib.rs
Outdated
#[uniffi::export(with_foreign)] | ||
#[async_trait::async_trait] | ||
pub trait WpAppNotifier: Send + Sync + std::fmt::Debug { | ||
async fn unauthorized_request(&self); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be more specific, like authentication_becomes_invalid
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 6bbb9bb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
becomes_invalid
may be a little misleading because we currently don't check if the authentication is valid when we first build the client. Maybe we should go with requested_with_invalid_authentication
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in a93523c.
wp_api/src/lib.rs
Outdated
let r = ApplicationPasswordsRequestBuilder::new(api_root_url, authentication) | ||
.retrieve_current_with_edit_context() | ||
.into(); | ||
request_executor.execute(r).await.is_ok() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, receiving a RequestExecutorError
does not mean the request is unauthorized. I wonder if we should explicitly check the WpErrorCode
instead here?
let parsed_result: Result<#response_type_ident, #error_type> = response.parse(); | ||
if let Err(ref err) = parsed_result { | ||
if let Some(wp_error_code) = err.wp_error_code() { | ||
if #crate_ident::is_unauthorized_request(response_status_code, wp_error_code, self.request_executor.clone(), self.api_root_url.clone(), self.authentication.clone()).await { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed above, these if statements can be changed to if is_unauthorized_request == 401 && #crate_ident::is_unauthorized_request(...) {
Introduces
WpAppNotifier
trait with therequested_with_invalid_authentication
function. This will get triggered by request executor if the response status is401
and the newly addedis_valid_authentication
returnsIsValidAuthenticationResult::Unauthorized
.The new
is_valid_authentication
function makes a request toGET /wp/v2/users/<user_id>)/application-passwords/introspect
and return eitherAuthenticated
,Unauthorized
,ErrWpApiError
orErrRequestExecutionError
. We only returnAuthenticated
orUnauthorized
if we parse the response and find what we are looking for.Also, each request executor now has a
has_valid_authentication
function which internally calls theis_valid_authentication
to make it easy to manually check the authentication state.