-
Notifications
You must be signed in to change notification settings - Fork 3
Cookies+nonce authentication #994
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
Conversation
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.
Left some suggestions, I think the only blocking one is that we should extract the headers and send them back?
| // Since the "cookies" part is out of our control, we need to verify that the nonce we got | ||
| // is actually for the user we expect. |
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.
It's possible for a request executor to avoid sending and receiving cookies (and actually, that should be the default to reduce traffic back-and-forth and to avoid invertently leaking data) – we can read the Set-Cookie value out of response headers and send them alongside the nonce request which is much more explicit.
We should still validate the the user is the one we expect though.
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 didn't do that for two reasons:
- Most, if not all, HTTP clients should have proper cookie-jar support. If we don't use the HTTP client's cookie-jar support, we'd need to implement one ourselves, which means (if we want to do it properly) checking domain, expire time, etc.
- I think URLSession would overwrite the Cookies header in
Requestwith the cookies inHTTPCookiesStorage. I could be wrong about this though...
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.
we'd need to implement one ourselves, which means (if we want to do it properly) checking domain, expire time, etc.
Right – I hadn't considered this part of it. On the iOS side, WDYT about adding a preconditionFailure for httpShouldSetCookies != true (link) and/or httpCookieAcceptPolicy == nil (link) to ensure programmers don't use it wrong?
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.
Good call. Added in a5f4fb2.
| // The "cookies" part is implicitly handled by the HTTP client. | ||
| // Since nonce is refreshed often, when using this authentication method, | ||
| // the caller should not keep using the same nonce for a long time. | ||
| Nonce { nonce: String }, |
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.
WDYT about using username/password here and automatically getting the nonce prior to each request?
We should still document that this is bad for performance, but as long as the username/password don't change the client won't become invalid unexpectedly which seems slightly better?
I don't feel strongly about this either way.
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.
using username/password here and automatically getting the nonce prior to each request?
That's how the original PR #327 implements it. It got pretty complicated because we need to refresh the nonce internally, and automatically retry the original API request after refreshing.
Considering the use case right now is using username and password to make one request, rather than all requests on the site, I thought it's better to keep it simple.
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 was thinking we'd avoid the complexity by just assuming that the nonce is never valid and refreshing it prior to each request. That makes it simple but more expensive to use.
Still works great for our "we only plan to ever make one request" purpose, but ensures that if the client somehow persists for a while it doesn't "go bad" – it'll always work.
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 implemented it in a different way in 0154c20. The implementation takes advantage of the existing auth mechanism, using the DynamicAuthenticationProvider trait. See the new test_cookies_nonce function.
The implementation should just work if the nonce has expired. It should refresh the nonce and retry the request.
To summarize: We can now use the account username and password to create a WpApiClient (backed by CookiesNonceAuthenticationProvider), which uses cookies+nonce authentication method. The API client instance is not affected by the nonce expiration time, which means it can be kept around for a long time, like your regular WpApiClient with an application password. It stores the nonce internally and refreshes it when it's detected to have expired.
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.
Looks cool – nothing else required to use it from Swift? Android should be 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.
I don't think there is anything extra needed for Swift. The WordPressAPI already has public APIs that can accept CookiesNonceAuthenticationProvider. We can probably add a convenience initialiser to WordPressAPI to make the API more ergonomic.
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 there is anything extra needed for Swift. The WordPressAPI already has public APIs that can accept CookiesNonceAuthenticationProvider. We can probably add a convenience initialiser to WordPressAPI to make the API more ergonomic.
I'll approve the PR to unblock, feel free to add this now or later :)
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 plan to use the authenticateTemporarily added in this PR. So, I'll merge the PR as it is. When we need to frequently use the account username & password to call the REST API from the apps, I'll see what kind of helpers are needed.
| let mut url = self.details.parsed_site_url.inner.clone(); | ||
| url.path_segments_mut() | ||
| .expect("The site url is a full URL") | ||
| .push("wp-login.php"); |
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.
We could fetch self.details.application_passwords_authentication_url then parse the HTML we get back to find the form's action property to get this value, but I'm not sure it adds a ton of value.
| url.path_segments_mut() | ||
| .expect("The site url is a full URL") | ||
| .pop() // Remove the "wp-login.php" part | ||
| .push("wp-admin") |
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.
You can also get this relative to self.details.application_passwords_authentication_url by dropping the last path component, but I'm not sure that's really better?
9c44a00 to
1ebeeac
Compare
swiftly does not support debian 13 yet, which is the platform of newer WordPress docker images.
This PR implements a light-weight cookies+nonce authentication.
The new type
WpRestNonceRetrievalgets REST nonce via<site-url>/wp-admin/admin-ajax.php?action=rest-nonce. The nonce can be used along with cookies in the nativeRequestExecutorto make authenticatedwp/v2API requests.As you can see in the code comment, the new
WpAuthentication::Nonceauthentication method is only meant for making temporary API calls. So, the implementation in this PR is more straightforward than the original in #327; it does not attempt to refresh the nonce.