Use ephemeral URLSession to send .org REST API requests #23715
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,9 +45,19 @@ actor WordPressClient { | |
| self.rootUrl = rootUrl.url() | ||
| } | ||
|
|
||
| static func `for`(site: WordPressSite) throws -> WordPressClient { | ||
| let parsedUrl = try ParsedUrl.parse(input: site.baseUrl) | ||
|
|
||
| // Currently, the app supports both account passwords and application passwords. | ||
| // When a site is initially signed in with an account password, WordPress login cookies are stored | ||
| // in `URLSession.shared`. After switching the site to application password authentication, | ||
| // the stored cookies may interfere with application-password authentication, resulting in 401 | ||
| // errors from the REST API. | ||
| // | ||
| // To avoid this issue, we'll use an ephemeral URLSession for now (which stores cookies in memory | ||
| // rather than using the shared one on disk). | ||
| let session = URLSession(configuration: .ephemeral) | ||
|
Contributor
There was a problem hiding this comment.
Is there a way to clear these before switching to application passwords? Using
(Related) Is it still considered viable for an app?
Contributor
Author
There was a problem hiding this comment.
Yes. I think that's what should happen. I will create a follow-up PR for this change because there are a couple of things that need to be removed when removing a self-hosted site.
Do you mind clarifying what you mean by "nuclear"? I think it makes sense to use an ephemeral session for REST API because we don't need to store anything (cookie, cache, etc) on the device for REST API.
Yes. That's going to be the main way to communicate with REST API.
Contributor
There was a problem hiding this comment.
Ephemeral sessions are typically used for private browsing in web browsers and similar situations, which is not the case here. I don't think we can guarantee that no cookies or (persistent) caches will ever be needed, especially if plugins are involved. If the issue is the cookies, the app should remove the cookies, so that it's not limited by the ephemeral session.
For the record, I think it may become a problem because application passwords can be disabled in WordPress and can also be a bit confusing to use. Application passwords are usually used for automation.
Contributor
Author
There was a problem hiding this comment.
No, we can't. In the same way, we can't guarantee the site's REST API is enabled, or endpoints in the WP core are not modified.
I just opened #23725 for this.
That's definitely possible. But xmlrpc can also be disabled. Nothing is guaranteed when it comes to communicating with self-hosted sites. My understanding is, since xmlrpc is neither secure nor reliable and there are many articles suggest disabling it, application passwords authentication is the best (and probably only) alternative. /cc @jkmassel
Contributor
There was a problem hiding this comment.
Anyway, it's not the strongest argument in favor of avoiding
The REST API has to be a better option, but it can be used with regular cookie-based login. In fact, I'm pretty sure sure the app already does it in a few scenarios. I can't say that I'm completely sold on application passwords because, similarly to the ephemeral sessions, this is not exactly what they are designed for. You would typically use application passwords for either automation (CI) or for third-party apps that don't support 2FA (I remember using these for Gmail at some point). Are there compelling reasons to replace cookies with application passwords? This isn't directly related to the PR, and I'd be happy to move the discussion elsewhere.
Contributor
Author
There was a problem hiding this comment.
That wasn't meant to be an argument for or against using ephemeral sessions, though. I guess it's an argument for what we should expect from self-hosted sites. The app should expect the sites doesn't diverge too much from the "core". The app only uses the core endpoints, jetpack endpoints, and probably Gutenberg endpoints. I don't expect the app to use some random third-party plugin endpoints. If a self-host site's core endpoints diverge so much from the "core" that their REST API relies on non-session cookies, I think it's fine to consider the site "not compatible" with the WordPress app. I don't expect this would actually happen on any site though. Going back to the discussion about ephemeral
Yep. Cookie authentication would work. But that requires the app to jump more hoops to get it working. And, the app would need to store the account password, which is less secure than storing application passwords. |
||
|
|
||
| switch site.type { | ||
| case let .dotCom(authToken): | ||
| let api = WordPressAPI(urlSession: session, baseUrl: parsedUrl, authenticationStategy: .authorizationHeader(token: authToken)) | ||
|
|
||
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.
(nit) factory methods should begin with
make:from https://www.swift.org/documentation/api-design-guidelines/
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.
That probably doesn't apply here. Because the function is used as
I think
site.makeClient()makes more sense thanWordPressClient.make(site). But I have changed this function to be an initialiser in 3343c28.