-
Notifications
You must be signed in to change notification settings - Fork 0
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
Implement wp-login.php based cookie authentication #327
base: trunk
Are you sure you want to change the base?
Changes from 1 commit
de6b9a0
97c9bbf
b3e0d05
5d977f2
1366c95
f8e73e6
2721dfd
aeb2ae5
a0613fd
083a7d3
bf61854
2cba32e
52c8a5c
06ef60d
527c176
53f6f66
1d0c91c
9ca673b
776ba65
3112f32
3fcdda3
0afe44d
c30f3fb
a5884e7
86caf42
2bf1ec5
40404ff
012a36a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,11 +1,14 @@ | ||||||
use std::str; | ||||||
use std::sync::Arc; | ||||||
|
||||||
use crate::request::endpoint::WpEndpointUrl; | ||||||
use url::Url; | ||||||
|
||||||
use crate::request::endpoint::{ApiBaseUrl, WpEndpointUrl}; | ||||||
use crate::request::{ | ||||||
RequestExecutor, RequestMethod, WpNetworkHeaderMap, WpNetworkRequest, WpNetworkResponse, | ||||||
RequestExecutor, RequestMethod, WpNetworkHeaderMap, WpNetworkRequest, WpNetworkRequestBody, | ||||||
WpNetworkResponse, | ||||||
}; | ||||||
use crate::ParsedUrl; | ||||||
use crate::{ParsedUrl, WpLoginCredentials}; | ||||||
|
||||||
use super::url_discovery::{ | ||||||
self, FetchApiDetailsError, FetchApiRootUrlError, StateInitial, UrlDiscoveryAttemptError, | ||||||
|
@@ -156,4 +159,92 @@ impl WpLoginClient { | |||||
.await | ||||||
.map_err(FetchApiDetailsError::from) | ||||||
} | ||||||
|
||||||
pub(crate) async fn insert_rest_nonce( | ||||||
&self, | ||||||
request: &WpNetworkRequest, | ||||||
api_base_url: &ApiBaseUrl, | ||||||
login: &WpLoginCredentials, | ||||||
) -> Option<WpNetworkRequest> { | ||||||
// Only attempt login if the request is to the WordPress site. | ||||||
if Url::parse(api_base_url.as_str()).ok()?.host_str() | ||||||
!= Url::parse(request.url.0.as_str()).ok()?.host_str() | ||||||
{ | ||||||
return None; | ||||||
} | ||||||
|
||||||
let nonce = self.get_rest_nonce(api_base_url, login).await?; | ||||||
|
||||||
let mut request = request.clone(); | ||||||
let mut headers = request.header_map.as_header_map(); | ||||||
headers.insert( | ||||||
http::header::HeaderName::from_bytes("X-WP-Nonce".as_bytes()) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason we don't use |
||||||
.expect("This conversion should never fail"), | ||||||
nonce.try_into().expect("This conversion should never fail"), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
); | ||||||
request.header_map = WpNetworkHeaderMap::new(headers).into(); | ||||||
|
||||||
Some(request) | ||||||
} | ||||||
|
||||||
async fn get_rest_nonce( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This implementation seems to be a multi-step process. It'd be good to separate the steps into their own functions and if necessary have a function that combines them. That'll make it easier to re-use parts of it and more importantly, easier to understand. |
||||||
&self, | ||||||
api_base_url: &ApiBaseUrl, | ||||||
login: &WpLoginCredentials, | ||||||
) -> Option<String> { | ||||||
let rest_nonce_url = api_base_url.derived_rest_nonce_url(); | ||||||
let rest_nonce_url_clone = rest_nonce_url.clone(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of cloning, you can create the |
||||||
let nonce_request = WpNetworkRequest { | ||||||
method: RequestMethod::GET, | ||||||
url: rest_nonce_url.into(), | ||||||
header_map: WpNetworkHeaderMap::new(http::HeaderMap::new()).into(), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
body: None, | ||||||
}; | ||||||
|
||||||
let nonce_from_request = |request: WpNetworkRequest| async move { | ||||||
self.request_executor | ||||||
.execute(request.into()) | ||||||
.await | ||||||
.ok() | ||||||
.and_then(|response| { | ||||||
// A 200 OK response from the `rest_nonce_url` (a.k.a `wp-admin/admin-ajax.php`) | ||||||
// should be the nonce value. However, just in case the site is configured to | ||||||
// return a 200 OK response with other content (for example redirection to | ||||||
// other webpage), here we check the body length here for a light validation of | ||||||
// the nonce value. | ||||||
if response.status_code == 200 { | ||||||
let body = response.body_as_string(); | ||||||
if body.len() < 50 { | ||||||
return Some(body); | ||||||
} | ||||||
} | ||||||
None | ||||||
}) | ||||||
}; | ||||||
|
||||||
if let Some(nonce) = nonce_from_request(nonce_request).await { | ||||||
return Some(nonce); | ||||||
} | ||||||
|
||||||
let mut headers = http::HeaderMap::new(); | ||||||
headers.insert( | ||||||
http::header::CONTENT_TYPE, | ||||||
"application/x-www-form-urlencoded".parse().unwrap(), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be a constant similar to |
||||||
); | ||||||
let body = serde_urlencoded::to_string([ | ||||||
["log", login.username.as_str()], | ||||||
["pwd", login.password.as_str()], | ||||||
["rememberme", "true"], | ||||||
["redirect_to", rest_nonce_url_clone.to_string().as_str()], | ||||||
]) | ||||||
.unwrap(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't use |
||||||
let login_request = WpNetworkRequest { | ||||||
method: RequestMethod::POST, | ||||||
url: api_base_url.derived_wp_login_url().into(), | ||||||
header_map: WpNetworkHeaderMap::new(headers).into(), | ||||||
body: Some(WpNetworkRequestBody::new(body.into_bytes()).into()), | ||||||
}; | ||||||
|
||||||
nonce_from_request(login_request).await | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,6 +67,7 @@ impl InnerRequestBuilder { | |
); | ||
match self.authentication { | ||
WpAuthentication::None => (), | ||
WpAuthentication::UserAccount { ref login } => (), | ||
WpAuthentication::AuthorizationHeader { ref token } => { | ||
let hv = HeaderValue::from_str(&format!("Basic {}", token)); | ||
let hv = hv.expect("It shouldn't be possible to build WpAuthentication::AuthorizationHeader with an invalid token"); | ||
|
@@ -101,7 +102,7 @@ pub struct WpNetworkRequestBody { | |
} | ||
|
||
impl WpNetworkRequestBody { | ||
fn new(body: Vec<u8>) -> Self { | ||
pub fn new(body: Vec<u8>) -> Self { | ||
Self { inner: body } | ||
} | ||
} | ||
|
@@ -114,7 +115,7 @@ impl WpNetworkRequestBody { | |
} | ||
|
||
// Has custom `Debug` trait implementation | ||
#[derive(uniffi::Object)] | ||
#[derive(Clone, uniffi::Object)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once the PR is close to being merged, let's re-visit this. If possible, I'd love to avoid deriving There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong. The "expensive" parts of I can remove the diff --git a/wp_api/src/authenticator.rs b/wp_api/src/authenticator.rs
index 283d915..046e728 100644
--- a/wp_api/src/authenticator.rs
+++ b/wp_api/src/authenticator.rs
@@ -268,7 +268,7 @@ impl RequestExecutor for AuthenticatedRequestExecutor {
&self,
request: Arc<WpNetworkRequest>,
) -> Result<WpNetworkResponse, RequestExecutionError> {
- let mut original = (*request).clone();
+ let mut original = clone_request(&request);
// Authenticate the initial request.
match self.authenticator.authenticate(&original).await {
@@ -286,7 +286,7 @@ impl RequestExecutor for AuthenticatedRequestExecutor {
// Retry if the request fails due to authentication failure
if let Ok(response) = &initial_response {
if self.authenticator.should_reauthenticate(response) {
- let mut original = (*request).clone();
+ let mut original = clone_request(&request);
if let Ok(headers) = self
.authenticator
.re_authenticate(&original, response)
@@ -302,3 +302,12 @@ impl RequestExecutor for AuthenticatedRequestExecutor {
initial_response
}
}
+
+fn clone_request(request: &Arc<WpNetworkRequest>) -> WpNetworkRequest {
+ WpNetworkRequest {
+ method: request.method.clone(),
+ url: request.url.clone(),
+ header_map: request.header_map.clone(),
+ body: request.body.clone(),
+ }
+}
diff --git a/wp_api/src/request.rs b/wp_api/src/request.rs
index f1fef34..d658722 100644
--- a/wp_api/src/request.rs
+++ b/wp_api/src/request.rs
@@ -85,7 +85,7 @@ pub trait RequestExecutor: Send + Sync + Debug {
) -> Result<WpNetworkResponse, RequestExecutionError>;
}
-#[derive(Clone, uniffi::Object)]
+#[derive(uniffi::Object)]
pub struct WpNetworkRequestBody {
inner: Vec<u8>,
}
@@ -104,7 +104,7 @@ impl WpNetworkRequestBody {
}
// Has custom `Debug` trait implementation
-#[derive(Clone, uniffi::Object)]
+#[derive(uniffi::Object)]
pub struct WpNetworkRequest {
pub(crate) method: RequestMethod,
pub(crate) url: WpEndpointUrl,
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess that's true in the current iteration - we made |
||
pub struct WpNetworkRequest { | ||
pub(crate) method: RequestMethod, | ||
pub(crate) url: WpEndpointUrl, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,6 +80,39 @@ impl ApiBaseUrl { | |
site_base_url.try_into() | ||
} | ||
|
||
pub(crate) fn derived_wp_login_url(&self) -> Url { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before I review this implementation, could you add some unit tests to help make the expectations clear? |
||
let mut url = self.url.clone(); | ||
|
||
if let Some(segments) = url.path_segments() { | ||
if segments.last() == Some("") { | ||
url.path_segments_mut() | ||
.expect("ApiBaseUrl is a full HTTP URL") | ||
.pop(); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These lines were one of the reasons I asked about the unit tests. I've ran the unit tests by commenting out this implementation and they were still successful. Could you add a test case that is handled by this implementation? I can't remember if/when the segment could be an empty string, so even after adding a unit test case, a comment might be useful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It'd be empty if there is trailing
So, I kinda cheated here. I didn't add new unit tests to cover this code block because they would fail. The cause is not this new code though. It's because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added more test cases in c30f3fb |
||
|
||
url.path_segments_mut() | ||
.expect("ApiBaseUrl is a full HTTP URL") | ||
.pop() | ||
.push("wp-login.php"); | ||
|
||
url | ||
} | ||
|
||
pub(crate) fn derived_rest_nonce_url(&self) -> Url { | ||
let mut url = self.derived_wp_login_url(); | ||
|
||
url.path_segments_mut() | ||
.expect("login url is a full HTTP URL") | ||
.pop() | ||
.push("wp-admin") | ||
.push("admin-ajax.php"); | ||
|
||
url.query_pairs_mut().append_pair("action", "rest-nonce"); | ||
|
||
url | ||
} | ||
|
||
fn by_appending(&self, segment: &str) -> Url { | ||
self.url | ||
.clone() | ||
|
@@ -106,7 +139,7 @@ impl ApiBaseUrl { | |
.expect("ApiBaseUrl is already parsed, so this can't result in an error") | ||
} | ||
|
||
fn as_str(&self) -> &str { | ||
pub fn as_str(&self) -> &str { | ||
self.url.as_str() | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,7 +64,23 @@ fn generate_async_request_executor(config: &Config, parsed_enum: &ParsedEnum) -> | |
quote! { | ||
pub async #fn_signature -> Result<#output_type, #static_wp_api_error_type> { | ||
#request_from_request_builder | ||
self.request_executor.execute(std::sync::Arc::new(request)).await?.parse() | ||
|
||
let cloned_request = request.clone(); | ||
let result = self.request_executor.execute(request.into()).await; | ||
|
||
if let Ok(response) = &result { | ||
if response.status_code == 401 { | ||
if let crate::WpAuthentication::UserAccount { ref login } = self.request_builder.inner.authentication { | ||
let client = crate::login::WpLoginClient::new(self.request_executor.clone()); | ||
let api_base_url = self.request_builder.endpoint.api_base_url.clone(); | ||
if let Some(request) = client.insert_rest_nonce(&cloned_request, &api_base_url, login).await { | ||
return self.request_executor.execute(request.into()).await?.parse(); | ||
} | ||
} | ||
} | ||
} | ||
|
||
result?.parse() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this logic belongs here. We want the generated code logic to remain simple and more importantly it shouldn't contain any "magic". Having said that, it's tricky to move this to somewhere else. We probably want a more structured retry and/or re-authenticate strategy that clients can opt into or out of. Also worth noting that the current design choice leads to some inefficiencies, but it's better to look into them once we are done with moving pieces around. Finally, if I am not misunderstanding how this code works, due to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I tried to do that, by having minimal code in here. Maybe it's because I'm new to Rust, but writing and debugging code in this proc macro is not a super straightforward experience. These statements are the minimal code to call the
Interesting. I don't see this new code as retry or re-authenticate though. It's part of the cookie authentication, where we have to get the
The request_executor is BTW, the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry, this is something I saw at the last moment and it got mixed up in my mind with cloning the client 🤦 It's totally fine to clone the Having said that, I think the original point is maybe still accurate? I am doubting myself, because I still don't think I fully understand how this type of authentication is supposed to work. I guess maybe it works for all clients if the request executor does cookie retention? The original requests never contain the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Regarding how it works, I included a brief summary in this PR. The official doc have more details. But I can re-cap it here in the context of this library. First of all, users have to actually sign in their site. In this PR, that's implemented using a In this PR, I didn't handle cookies, because I defer that to However, that's not enough for REST API. It also needs a "WP REST Nonce" thing. This nonce is generated and stored in server to verify incoming REST requests. We can get this nonce using And those two things are what REST API cookies authentication needs.
Yes, it would. As I noted in my PR description, we should store the nonce somewhere and re-use it. But that's not implemented in this PR. |
||
} | ||
} | ||
}) | ||
|
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've discussed this with @jkmassel as well, and I think this authentication method should take a
nonce
token and acookie
string and attach them to every request.In my opinion, we should handle the re-authentication as its own thing instead of the current built-in mechanism.
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.
If we want to implement "cookie authentication" this way, we don't actually have to do anything to support it in this library. The client can just make their
RequestExector
implementation including cookies and nonce in their requests.My intention in this PR is making clients do as minimal work as possible. They only need to supply username and password, we'll take care of the nitty gritty of cookie 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.
I think we can give all of the necessary pieces to the clients/wrappers, but the decisions are explicit.
I am slowly understanding that a lot of my concerns are related to how weird this type of authentication is. Still, I want to find a way to make everything more explicit, rather than being buried.
Also, I think you mentioned on Slack that you needed to retain some state related to the
nonce
token, wouldn't making this change help with that?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 guess it depends on what "the necessary pieces" are. In this PR, those are the account username and password: we'll do the cookies and nonce things internally. In your previous comment, you mentioned your preference is clients supplying cookies and nonces (which means clients need to make necessary HTTP requests to obtain those things).
I stopped looking after your answer about self-referential types. 🙈
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.
Although that's my preference, I also think that we should provide helpers to make the necessary HTTP requests. In my opinion, the best setup is we have individual easy to use pieces and the clients glue them up with a few lines of code. I am not 100% sure that this is possible or will work out as good as I have it in my mind, but I think it's worth a try.
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 have made some changes regarding this suggestion. Can you please have another look at this PR? If you think that's an acceptable solution, I can further refine some details and address your other comments.
Authentication (including application-password) is now in their dedicated "Authenticator" types. They are now decoupled from the "HTTP request building" code.