Implement wp-login.php based cookie authentication #327

crazytonyli · 2024-09-27T11:22:05Z

This PR adds a new authentication strategy: WpAuthentication::UserAccount, which contains WordPress site user login credentials.

When using this new authentication method, the library automatically logs user in via wp-login.php and uses the returned cookies to authenticate REST API calls.

REST API cookie authentication requires two piece of information: WordPress site cookies and "rest nonce".

The rust library doesn't handle cookie storage. The platform specific RequestExecutor implementation should have a proper cookie jar implementation where it stores cookies from HTTP responses and sends stored cookies in HTTP requests.

The "rest nonce" can be fetched from wp-admin/admin-ajax.php?action=rest-nonce webpage. The rust library only fetch the nonce value upon receiving 401 response from REST API calls.

The "rest nonce" expires after one day (by default) if it's not used. To avoid repeatedly fetching the nonce value, we should store the nonce somewhere to be used in subsequent REST API calls, which is not implemented in this PR.

oguzkocer

@crazytonyli Thank you for the PR! I am done with my first review pass.

I think the most important parts to focus on are the changes to WpAuthentication and how the retry/re-authentication is handled. Especially the re-authentication can be tricky, as I am not entirely sure how it should look like either. However, I am happy to take a look at it if you like.

oguzkocer · 2024-09-30T18:31:22Z

wp_derive_request_builder/src/generate.rs

+
+                    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()


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 request_executor being cloned, the original executor's authentication won't be updated - meaning every request will have to be re-authenticated and retried. This might not happen in iOS, due to how URLSession works, but I believe it'll happen for other clients.

We want the generated code logic to remain simple and more importantly it shouldn't contain any "magic".

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 insert_rest_nonce function. But we can move them all out into a static function if that's easier to read.

We probably want a more structured retry and/or re-authenticate strategy that clients can opt into or out of.

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 nonce value from the site, and pass it to REST API calls. That is not something clients can opt-out of, if they want to use an actual account username and password to authenticate REST API calls.

due to the request_executor being cloned, the original executor's authentication won't be updated

The request_executor is Arc<dyn RequestExecutor>. I'm not sure what cloning it would do. Does it creat another copy of the actual trait RequestExecutor instance or just create a new pointer to the same trait RequestExecutor instance? Can trait RequestExecutor even be cloned?

BTW, the Arc<dyn RequestExecturo> passed to WpApiClient is cloned multiple times at the moment:

impl WpApiClient { pub fn new( site_url: Arc<ParsedUrl>, authentication: WpAuthentication, request_executor: Arc<dyn RequestExecutor>, ) -> Self { let api_base_url: Arc<ApiBaseUrl> = Arc::new(site_url.inner.clone().into()); Self { application_passwords: ApplicationPasswordsRequestExecutor::new( api_base_url.clone(), authentication.clone(), request_executor.clone(), ) .into(), plugins: PluginsRequestExecutor::new( api_base_url.clone(), authentication.clone(), request_executor.clone(), ) .into(), post_types: PostTypesRequestExecutor::new( api_base_url.clone(), authentication.clone(), request_executor.clone(), ) .into(), posts: PostsRequestExecutor::new( api_base_url.clone(), authentication.clone(), request_executor.clone(), ) .into(), site_settings: SiteSettingsRequestExecutor::new( api_base_url.clone(), authentication.clone(), request_executor.clone(), ) .into(), users: UsersRequestExecutor::new( api_base_url.clone(), authentication.clone(), request_executor.clone(), ) .into(), wp_site_health_tests: WpSiteHealthTestsRequestExecutor::new( api_base_url.clone(), authentication.clone(), request_executor.clone(), ) .into(), } } }

The request_executor is Arc. I'm not sure what cloning it would do. Does it creat another copy of the actual trait RequestExecutor instance or just create a new pointer to the same trait RequestExecutor instance? Can trait RequestExecutor even be cloned?

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 RequestExecutor.

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 nonce header, so wouldn't all of them fail at first?

I am doubting myself, because I still don't think I fully understand how this type of authentication is supposed to work.

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 POST wp-login.php request. That's pretty much the same as submitting the login form from a web browser. If the login HTTP request is successful, the response would include necessary authentication related cookies for the site.

In this PR, I didn't handle cookies, because I defer that to RequestExecutor. Basically RequestExecutor needs to have the capability of storing cookies in HTTP responses and using stored cookies for HTTP requests.

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 /wp-admin/admin-ajax.php?action=rest-nonce. Once we get the nonce, we can put it in HTTP request headers or URL query.

And those two things are what REST API cookies authentication needs.

The original requests never contain the nonce header, so wouldn't all of them fail at first?

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.

oguzkocer · 2024-09-30T18:35:44Z

wp_api/src/request.rs

@@ -114,7 +115,7 @@ impl WpNetworkRequestBody {
 }

 // Has custom `Debug` trait implementation
-#[derive(uniffi::Object)]
+#[derive(Clone, uniffi::Object)]


Once the PR is close to being merged, let's re-visit this. If possible, I'd love to avoid deriving Clone as that's not a cheap operation and I want to prevent us from doing so by not allowing it.

I'd love to avoid deriving Clone as that's not a cheap operation

Correct me if I'm wrong. The "expensive" parts of WpNetworkRequest are Arc. Does that mean Clone is just clone references rather than actual HTTP headers and body?

I can remove the Clone here and create a function to clone the references. I'm not sure if that's an ideal way to implement that.

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,

I guess that's true in the current iteration - we made body & header_map an Arc. It shouldn't be a problem to derive Clone here 🤔

oguzkocer · 2024-09-30T18:41:35Z

wp_api/src/request/endpoint.rs

@@ -80,6 +80,39 @@ impl ApiBaseUrl {
        site_base_url.try_into()
    }

+    pub(crate) fn derived_wp_login_url(&self) -> Url {


Before I review this implementation, could you add some unit tests to help make the expectations clear?

oguzkocer · 2024-09-30T18:46:36Z

wp_api/src/login/login_client.rs

+        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())


Any reason we don't use HeaderName::from_str here? It internally calls from_bytes and I don't think there is any extra value in us using from_bytes directly.

oguzkocer · 2024-09-30T18:52:38Z

wp_api/src/login/login_client.rs

+        let nonce_request = WpNetworkRequest {
+            method: RequestMethod::GET,
+            url: rest_nonce_url.into(),
+            header_map: WpNetworkHeaderMap::new(http::HeaderMap::new()).into(),


Suggested change

header_map: WpNetworkHeaderMap::new(http::HeaderMap::new()).into(),

header_map: WpNetworkHeaderMap::default().into(),

oguzkocer · 2024-09-30T19:06:24Z

wp_api/src/login/login_client.rs

+        headers.insert(
+            http::header::HeaderName::from_bytes("X-WP-Nonce".as_bytes())
+                .expect("This conversion should never fail"),
+            nonce.try_into().expect("This conversion should never fail"),


nonce is a value we get back from a request, right? So, why can the conversation not fail?

oguzkocer · 2024-10-01T20:26:34Z

wp_api/src/lib.rs

 #[derive(Debug, Clone, uniffi::Enum)]
 pub enum WpAuthentication {
    AuthorizationHeader { token: String },
+    UserAccount { login: WpLoginCredentials },


I've discussed this with @jkmassel as well, and I think this authentication method should take a nonce token and a cookie 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.

I think this authentication method should take a nonce token and a cookie string and attach them to every request.

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.

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?

I think we can give all of the necessary pieces to the clients/wrappers, but the decisions are explicit.

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).

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?

I stopped looking after your answer about self-referential types. 🙈

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).

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.

In my opinion, we should handle the re-authentication as its own thing instead of the current built-in mechanism.

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.

oguzkocer · 2024-10-01T20:37:21Z

wp_api_integration_tests/tests/test_cookie_authentication.rs

@@ -0,0 +1,66 @@
+use wp_api::{WpApiClient, WpAuthentication, WpLoginCredentials};


The file name should include the _immut suffix for consistency.

oguzkocer · 2024-10-01T20:38:25Z

wp_api_integration_tests/tests/test_cookie_authentication.rs

+#[serial]
+async fn test_cookie_authentication() {
+    let login = WpLoginCredentials {
+        username: "[email protected]".to_string(),


Suggested change

username: "[email protected]".to_string(),

username: TestCredentials::instance().admin_username.to_string(),

This suggestion applies to other 2 tests as well.

oguzkocer · 2024-10-01T20:38:54Z

wp_api_integration_tests/tests/test_cookie_authentication.rs

+async fn test_cookie_authentication() {
+    let login = WpLoginCredentials {
+        username: "[email protected]".to_string(),
+        password: "strongpassword".to_string(),


We should add the password to test_credentials.json and read it from there similar to all other test data.

crazytonyli · 2024-10-06T21:05:17Z

wp_api/src/api_client.rs

-            wp_site_health_tests
-        )
-    }
-}


I removed these codes because they are not used anywhere. If you think they are important, I can add them back.

It's not unused code. It's part of the public API that supports manual request building and execution as showcased by the now removed integration test. Please keep in mind that this library is supposed to be used by clients other than our own.

Also, even if we were going to remove something like this, it shouldn't be part of a completely unrelated PR as that buries the information and makes it harder to bring back.

I have reverted this change.

oguzkocer · 2024-10-06T21:32:18Z

wp_api/src/authenticator.rs

+#[async_trait::async_trait]
+pub trait Authenticator: Send + Sync + std::fmt::Debug {
+    // TODO: Use Result instead
+    async fn authenticate(&self, request: &mut WpNetworkRequest) -> bool;


This sort of approach could work, but ideally these methods would return the necessary information rather than mutate a parameter. Even though Rust does a good job of limiting the issues with mutation, we should still prefer unit functions when it's possible to achieve the same result. There are various benefits to unit functions, but the one I am most interested in this case is avoiding the Authenticator trait being abused to make changes besides adding authentication headers.

In this case, I believe it'll be possible to return headers to be included in the request, rather than mutate the request. Let me know if I am missing anything.

Note that I only had a very quick look at this since it's still Sunday for me. I can properly review tomorrow if necessary.

I added a TODO here to remind me to change the mut reference to a Result. As I mentioned in #327 (comment), I can make these changes after you think the current approach is acceptable. (Otherwise, I'm just wasting my time on changing um-mergeable code).

oguzkocer

@crazytonyli I think this is coming along really nicely. I have completed a pass of the general authenticator design and made some suggestions, but so far I don't have any major concerns.

The part I didn't fully review yet is CookieAuthenticator. Even though I've read that code many times by now 😅 I am leaving it to later for now. I don't think it's handling some of the issues about multiple authentications happening at the same time, but I could be wrong. I'll try to prove that as a test case later on.

oguzkocer · 2024-10-15T16:45:14Z

wp_api/src/authenticator.rs

+pub trait Authenticator: Send + Sync + std::fmt::Debug {
+    async fn authenticate(
+        &self,
+        request: &WpNetworkRequest,


It looks like we are only using the request parameter to only authenticate if we are making requests to the WordPress site the authentication is for. I think this logic can be lifted to a should_authenticate function that only takes the request url so that we can drop this parameter.

oguzkocer · 2024-10-15T16:46:26Z

wp_api/src/authenticator.rs

+
+#[async_trait::async_trait]
+pub trait Authenticator: Send + Sync + std::fmt::Debug {
+    async fn authenticate(


How do you feel about renaming this to authentication_headers instead? Since it's no longer modifying the request but returning authentication headers, I think that's more appropriate and will be easier to understand its purpose from its call sites.

Good catch! I have refactored the functions in this trait in the latest changes. Can you please have another review?

oguzkocer · 2024-10-15T16:52:14Z

wp_api/src/authenticator.rs

+        request: &WpNetworkRequest,
+    ) -> Result<HeaderMap, AuthenticationError>;
+
+    fn should_reauthenticate(&self, response: &WpNetworkResponse) -> bool {


Suggested change

fn should_reauthenticate(&self, response: &WpNetworkResponse) -> bool {

fn should_reauthenticate(&self, response_status_code: u16) -> bool {

I think we should limit the function signature as much as possible. For now, sharing the status code should be enough. In the future, we might need to get a little more sophisticated, but doing this should help discourage ourselves from doing any unnecessary processing of the response. For example, we wouldn't want this code to parse the response as that'll potentially duplicating the parsing logic.

oguzkocer · 2024-10-15T16:56:15Z

wp_api/src/authenticator.rs

+    async fn authenticate(
+        &self,
+        request: &WpNetworkRequest,
+    ) -> Result<HeaderMap, AuthenticationError>;


I think Result<Option<HeaderMap>, AuthenticationError> is more appropriate here. We can return an empty HeaderMap instead of None, but using Option<HeaderMap> would better convey the intent.

oguzkocer · 2024-10-15T17:00:09Z

wp_api/src/authenticator.rs

+            Err(error) => {
+                // Do nothing.
+                // Any authentication error will be returned later after sending the request.
+            }


I think we should either change the authenticate function to return an Option<HeaderMap> instead of a Result or lift the error to the caller. Having a Result and not doing anything with the error would be a misleading design.

In this case, I think it's more appropriate to lift the error because we have errors such as AuthenticationError::IncorrectCredentials. Although we should probably include the server response as well.

I think we should either change the authenticate function to return an Option instead of a Result or lift the error to the caller.

I have changed the return value to Option<HeaderMap>. Because we would always send a request to the endpoint, with or without the true authentication headers. That request will give us a full REST API response, which will be parsed into an "unauthorized" WpApiError instance if authentication fails.

oguzkocer · 2024-10-15T17:08:36Z

wp_api/src/authenticator.rs

+}
+
+#[derive(Debug)]
+pub struct AuthenticatedRequestExecutor {


I am not sure if there is too much value in this particular abstraction. I wonder if you decided to introduce it due to my initial comment about the complicated logic in the WpDerivedRequest macro. If that's the case, I think this new approach won't have the same problem.

What do you think? Do you see any significant value in having an extra abstraction for an authenticated request executor, or can we bake the logic into the existing layers?

I wonder if you decided to introduce it due to my initial comment about the complicated logic in the WpDerivedRequest macro.

Yes. I like the current change because there are minimal changes to the macro implementation. To be honest, it's not very straightforward to make changes to wp_derive_request_builder.

oguzkocer · 2024-10-15T17:10:21Z

wp_api/src/request.rs

@@ -114,7 +115,7 @@ impl WpNetworkRequestBody {
 }

 // Has custom `Debug` trait implementation
-#[derive(uniffi::Object)]
+#[derive(Clone, uniffi::Object)]


I guess that's true in the current iteration - we made body & header_map an Arc. It shouldn't be a problem to derive Clone here 🤔

oguzkocer · 2024-10-15T17:15:36Z

wp_api/src/authenticator.rs

+        &self,
+        request: &WpNetworkRequest,
+    ) -> Result<HeaderMap, AuthenticationError> {
+        // Only attempt login if the request is to the WordPress site.


It's worth noting that we don't allow random requests to be executed and afaik all requests currently being executed are the requests we build ourselves. However, I can see how having a check like this could be useful especially as a future-proof. Having said that, as I mentioned in another comment, I think this should in its own function such as should_authenticate.

Also, I believe the re_authenticate is currently bypassing this check. If we have something like this in place, we might as well check should_authenticate from re_authenticate as well.

oguzkocer

@crazytonyli I've completed another review. I think the biggest issue at the moment is the CookieAuthenticator implementation.

Besides the line comments I've left, I've looked into the concern I raised on Slack and on my previous review about the nonce token being updated my multiple requests instead of requests waiting in a queue until the authentication is completed. As I suspected, if we make multiple requests at the same time, we'll end up having to do multiple re-authentications. You can use the test case below to see this in action.

#[tokio::test]
#[serial_test::parallel]
async fn showcase_multiple_nonce() {
    let login = WpLoginCredentials {
        username: TestCredentials::instance().admin_username.to_string(),
        password: TestCredentials::instance()
            .admin_account_password
            .to_string(),
    };
    let request_executor = std::sync::Arc::new(AsyncWpNetworking::with_cookie_store());
    let client = WpApiClient::new(
        test_site_url(),
        WpAuthentication::UserAccount { login },
        request_executor,
    );

    futures::future::join_all((0..100).map(|_| client.users().retrieve_me_with_edit_context()))
        .await;
}

In order to see the re-authentications, you can add println!("nonce changed: {}", fetched); to wp_api/src/authenticator.rs after line 152 and run the tests with cargo test -p wp_api_integration_tests --test 'test_cookie_authentication_immut' -- --nocapture.

The number of re-authentications change depending on the number of consecutive requests. However, even if we make only 5 requests at the same time, my tests showed at least 2-3 updates to the nonce token. Sometimes I'd get lucky and get the same token, but the token would be updated multiple times. Sometimes I'd only get 2 updates but they'll be for different tokens. As the number of consecutive requests increase, so does the re-authentications. Here are logs from a few of my runs with 5 consecutive requests:

nonce changed: dd7126501a
nonce changed: dd7126501a
nonce changed: dd7126501a
nonce changed: dd7126501a
nonce changed: dd7126501a
test showcase_multiple_nonce ... ok

nonce changed: 281f6eb466
nonce changed: 523fae10de
nonce changed: b509372cfb
test showcase_multiple_nonce ... ok

nonce changed: 45021c20e8
nonce changed: c829286cf6
nonce changed: a1890e0bb7
test showcase_multiple_nonce ... ok

nonce changed: 8755079511
nonce changed: 8755079511
nonce changed: 8755079511
nonce changed: 8755079511
nonce changed: 8755079511
test showcase_multiple_nonce ... ok

nonce changed: e146aa7843
nonce changed: e65b91f1fa
test showcase_multiple_nonce ... ok

Hope the review and these test cases will prove useful! 🙇‍♂️

oguzkocer · 2024-10-22T16:43:07Z

wp_api/src/api_client.rs

+        let authenticator: Arc<dyn Authenticator> = match &authentication {
+            WpAuthentication::AuthorizationHeader { token } => {
+                Arc::new(ApplicationPasswordAuthenticator::new(
+                    site_url.inner.host_str().unwrap_or("").into(),


I think we should use Option<String> instead of an empty string.

oguzkocer · 2024-10-22T16:45:25Z

wp_api/src/api_client.rs

+                ))
+            }
+            WpAuthentication::UserAccount { login } => Arc::new(CookieAuthenticator::new(
+                site_url.inner.clone().into(),


Should we clone the Arc<ParsedUrl> instead of inner?

oguzkocer · 2024-10-22T16:53:26Z

wp_api/src/request/endpoint.rs

+        if let Some(segments) = url.path_segments() {
+            if segments.last() == Some("") {
+                url.path_segments_mut()
+                    .expect("ApiBaseUrl is a full HTTP URL")
+                    .pop();
+            }
+        }


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.

I can't remember if/when the segment could be an empty string

It'd be empty if there is trailing /, like https://example.com/f/

Could you add a test case that is handled by this implementation?

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 ApiBaseUrl can't parse some URLs with trailing slash. I have fixed the issue in #353. Do you mind having a look at that PR first?

I have added more test cases in c30f3fb

oguzkocer · 2024-10-22T16:55:57Z

wp_api/src/request/endpoint.rs

+        // HTTP URL always has a host.
+        self.url.host_str().unwrap_or("")


I think with this comment, we should use .expect() instead. In case we are wrong, returning an empty string will prevent us from realizing the problem and result in issues due to incorrect assumptions.

Using .expect also better convey our intention.

oguzkocer · 2024-10-22T16:59:14Z

wp_api_integration_tests/tests/test_cookie_authentication_immut.rs

+use serial_test::serial;
+
+#[tokio::test]
+#[serial]


Suggested change

#[serial]

#[parallel]

The reason we use #[serial] in mutable tests is because the execution of a mutable test can interfere with other tests - both mutable and immutable ones. However, the same doesn't apply for immutable tests, and it's safe to run them in parallel.

oguzkocer · 2024-10-22T17:24:53Z

wp_api/src/authenticator.rs

+    }
+
+    fn reset(&self) {
+        *self.nonce.write().expect("Failed to unlock nonce") = None;


This .expect looks wrong to me. There are valid cases where the lock wouldn't be released to us. On the contrary, this looks like a case we need to handle with a Result and maybe retry after a grace period.

I can turn this into an if-let and only handle the ok case, which should be safer.

There are valid cases where the lock wouldn't be released to us.

Can you expand on this?

Poisoning

An RwLock, like Mutex, will become poisoned on a panic. Note, however, that an RwLock may only be poisoned if a panic occurs while it is locked exclusively (write mode). If a panic occurs in any reader, then the lock will not be poisoned.

Reading the api doc, it doesn't look like there is much we can do about the error.

Hmm.. I completely misremembered how RwLock works. read & write blocks the current thread until the lock is available, instead of returning an error as try_read & try_write does. 🤔 Sorry about that.

In that case, the lock could be used to address the consecutive requests issue by retrieving the lock to write when the authentication process is started. Then any read access will be blocked until the authentication is complete.

We probably need to keep track of our authentication attempts, otherwise we'll keep trying to authenticate even if we have bad credentials - but that doesn't have to happen in this PR.

oguzkocer · 2024-10-22T17:27:23Z

wp_api/src/authenticator.rs

+    }
+
+    async fn get_rest_nonce(&self) -> Option<String> {
+        if let Some(cache) = self.nonce.read().expect("Failed to unlock nonce").clone() {


Similar to the case where we are writing the nonce, there are valid cases where we can't read it. We can't just crash the app in such cases.

oguzkocer · 2024-10-22T17:28:24Z

wp_api/src/authenticator.rs

+            self.nonce
+                .write()
+                .expect("Failed to unlock nonce")


Similar to my other 2 comments, there are valid cases where the lock is not released to us and we can't crash the app in such cases.

nonce is changed to futures::lock::Mutex which doesn't throw errors.

oguzkocer · 2024-10-22T17:40:03Z

wp_api/src/authenticator.rs

+        }
+    }
+
+    async fn get_rest_nonce(&self) -> Option<String> {


In my opinion, this function signature is misleading. At a glance, it seems to be returning a nonce token, but in fact it also mutates the state. At the very least we should name it such that it communicates the mutation.

This issue is mostly due to the interior mutability pattern we are using, so I'd suggest skipping this comment for now as I'd like to discuss the usage of RwLock in a separate thread first. We can come back to it afterwards if it's still applicable.

I have renamed this function to fetch_rest_nonce. Let me know if you have any suggestions regarding the name.

I like fetch_rest_nonce, but it looks like it's still mutating the hold nonce value inside. I'd strongly prefer if a fetch function only returns the value and the update happens somewhere else. If that's not a good option, then I was thinking we should go with something like fetch_and_update_rest_nonce instead.

Having said all that, I am looking at the implementation and it looks like the nonce value we hold is meant to be only accessed through the fetch_rest_nonce 🤔

I have some concerns, but I don't have a clear suggestion at the moment, so let's leave this as is for this PR. I'll think about it again when/if I get to work on this particular code - or if we have any issues with it.

oguzkocer · 2024-10-22T17:42:41Z

wp_api/src/authenticator.rs

+    api_base_url: ApiBaseUrl,
+    credentials: WpLoginCredentials,
+    request_executor: std::sync::Arc<dyn RequestExecutor>,
+    nonce: RwLock<Option<String>>,


Could you explain why we need/want to use RwLock?

I've gone through the implementation and it looks like we are switching from Rust's compile time borrow checker guarantees to the runtime checks with the RwLock and then immediately give up on them and crash any time there are more than 1 threads writing at the same time, or if there is a write & read happening at the same time.

When there are concurrent requests, the nonce will be read/write concurrently. I think we'll have to use some sort of lock to prevent concurrency issues.

In 527c176, I have changed this std::sync::RwLock to futures::lock::Mutex to work with async functions better and prevent concurrent read & write to nonce.

oguzkocer · 2024-10-23T18:30:42Z

wp_api/src/authenticator.rs

+    }
+
+    async fn reset(&self) {
+        *self.nonce.lock().await = None;


I wonder what will happen now with the nonce with the new concurrent handling when an authentication is going on and another thread resets the nonce. I think it might end up removing the newly acquired nonce.

I could be wrong, but I feel like the removing the expired token should be part of the current fetch_rest_nonce implementation. With the current naming maybe this suggestion doesn't sound correct, but as for the implementation, I think it makes sense to keep the mutex modification to a single place. It sort of works like:

If the cached token is expired, clear it

If we have a valid token return it

If we don't have a valid token, retrieve it, cache it and return it

Furthermore, I had some concerns about the reset function in general, because I felt it was a CookieAuthenticator specific thing. So, I was wondering if it would make sense to pass an argument to authentication_headers function indicating that it should clear its cached token. Maybe that way we don't need the re_authenticate either.

What do you think?

I think the same issue will happen with my suggestion 🤔

I feel like the removing the expired token should be part of the current fetch_rest_nonce implementation.

The tricky thing is we don't know if a nonce is valid or not, until we make an HTTP request with it.

With the implementation in this PR, when there is an existing nonce in memory, the best scenario (which would be 90% of the time for the apps because nonce is valid for one day by default, which probably would outlive the app) is we only need to make one HTTP request, which is the REST API request, and the worst scenario (when nonce expires) is making three HTTP requests, which are one REST API request, one request to get the valid nonce, retry the REST API request.

If the cached token is expired, clear it

We don't know when it expires. If we were to use an HTTP request to check if it's expired, we'd need to make an additional nonce-fetching HTTP request for every single REST API call, which I don't think is ideal. If we were to go with this solution( I don't think we should), we don't need to cache nonce at all.

We don't know when it expires

When I say, if the cached token is expired, I mean the case where we manually call the reset function.

I have made another change to address your concerns: 86caf42

I wonder what will happen now with the nonce with the new concurrent handling when an authentication is going on and another thread resets the nonce. I think it might end up removing the newly acquired nonce.

Great catch! The new implementation now checks the previously used nonce and only clears the cached nonce if they are the same. That means the second request that's waiting for the ongoing authentication request won't reset the nonce. I'm looking into adding an unit test to cover this scenario.

I could be wrong, but I feel like the removing the expired token should be part of the current fetch_rest_nonce implementation.

This is also done as part of the above change. Also, fetch_rest_nonce is now renamed to update_reset_nonce. Maybe update_rest_nonce_if_needed would be a better name? But I'm not sure if that's too verbose...

Furthermore, I had some concerns about the reset function in general, because I felt it was a CookieAuthenticator specific thing.

Indeed. I have simplified the trait design but generalized authentication_headers arguments (changing URL and status code to request and response objects) to avoid coupling the trait API with one specific implementation.

I have added some unit tests, but they are quite verbose and not very easy to read. Let me know if you have any thoughts in improving them.

Implement wp-login.php based cookie authentication

de6b9a0

crazytonyli requested review from oguzkocer and jkmassel September 27, 2024 11:22

Add a few integration tests for cookie authentication

97c9bbf

oguzkocer requested changes Oct 1, 2024

View reviewed changes

crazytonyli commented Oct 6, 2024

View reviewed changes

crazytonyli requested a review from oguzkocer October 6, 2024 21:05

oguzkocer reviewed Oct 6, 2024

View reviewed changes

Implement authentication methods in dedicated types

b3e0d05

crazytonyli force-pushed the wp-login-cookie-authentication branch from 830e7cc to b3e0d05 Compare October 7, 2024 01:10

crazytonyli added 8 commits October 14, 2024 15:14

Save fetched nonce

5d977f2

Merge branch 'trunk' into wp-login-cookie-authentication

1366c95

Add TestCredentials.admin_account_password

f8e73e6

Do not mutate request in Authenticator

2721dfd

Add unit tests for derived url functions

aeb2ae5

Format code

a0613fd

Add a constrant for "application/x-www-form-urlencoded"

083a7d3

Derive Default on InnerRequestBuilder

bf61854

crazytonyli requested a review from oguzkocer October 15, 2024 09:34

oguzkocer reviewed Oct 15, 2024

View reviewed changes

crazytonyli added 2 commits October 16, 2024 12:50

Refactor authenticator to extra authentication logic

2cba32e

Remove AuthenticationError

52c8a5c

oguzkocer requested changes Oct 22, 2024

View reviewed changes

crazytonyli added 7 commits October 23, 2024 11:09

Add an unit test to verify nonce is reused across multiple requests

06ef60d

Prevent repeatedly fetching nonce when sending concurrent requests

527c176

Replace empty default value with assertions

53f6f66

Clone instead of re-creating ApiBaseUrl

1d0c91c

Run cookie authentication tests in parallel

9ca673b

Add an API to document RequestExecutor must support cookie-jar

776ba65

Update function parameters

3112f32

Rename get_reset_nonce to fetch_rest_nonce

3fcdda3

oguzkocer reviewed Oct 23, 2024

View reviewed changes

crazytonyli added 7 commits October 24, 2024 11:54

Merge branch 'trunk' into wp-login-cookie-authentication

0afe44d

Add more test cases to wp-login and rest-nonce unit tests

c30f3fb

Simplify deriving wp-login url implementation

a5884e7

Simplify Authenticator trait design

86caf42

Fix a compiling issue

2bf1ec5

Pass previous authentication header instead of the request object

40404ff

Add unit tests for CookieAuthenticator

012a36a

	header_map: WpNetworkHeaderMap::new(http::HeaderMap::new()).into(),
	header_map: WpNetworkHeaderMap::default().into(),

		@@ -0,0 +1,66 @@
		use wp_api::{WpApiClient, WpAuthentication, WpLoginCredentials};

	username: "[email protected]".to_string(),
	username: TestCredentials::instance().admin_username.to_string(),

	fn should_reauthenticate(&self, response: &WpNetworkResponse) -> bool {
	fn should_reauthenticate(&self, response_status_code: u16) -> bool {

		// HTTP URL always has a host.
		self.url.host_str().unwrap_or("")

Implement wp-login.php based cookie authentication #327

Are you sure you want to change the base?

Implement wp-login.php based cookie authentication #327

Conversation

crazytonyli commented Sep 27, 2024

oguzkocer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazytonyli Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oguzkocer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oguzkocer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazytonyli Oct 2, 2024 •

edited

Loading