Skip to content

Use ephemeral URLSession to send .org REST API requests #23715

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Nov 4, 2024
Merged

Use ephemeral URLSession to send .org REST API requests #23715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
01c5e4f
Use ephemeral URLSession to send .org REST API requests
crazytonyli Oct 29, 2024
2f91057
Fix grammar issues in code comment
crazytonyli Oct 29, 2024
3343c28
Change static functions to initialisers
crazytonyli Oct 29, 2024
5e50400
Remove an unnecessary throw
crazytonyli Oct 29, 2024
8b3c7bc
Merge branch 'trunk' into self-hosted-sites-user-management
crazytonyli Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 25 additions & 9 deletions WordPress/Classes/Networking/WordPressClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,20 @@ struct WordPressSite {
case selfHosted(username: String, authToken: String)
}

let baseUrl: String
let baseUrl: URL
let type: WordPressSite.SiteType

init(baseUrl: ParsedUrl, type: WordPressSite.SiteType) {
self.baseUrl = baseUrl.url()
self.baseUrl = baseUrl.asURL()
self.type = type
}

static func from(blog: Blog) throws -> WordPressSite {
init(blog: Blog) throws {
let url = try ParsedUrl.parse(input: blog.getUrlString())
if let account = blog.account {
return WordPressSite(baseUrl: url, type: .dotCom(authToken: account.authToken))
self.init(baseUrl: url, type: .dotCom(authToken: account.authToken))
} else {
return WordPressSite(baseUrl: url, type: .selfHosted(
self.init(baseUrl: url, type: .selfHosted(
username: try blog.getUsername(),
authToken: try blog.getApplicationToken())
)
Expand All @@ -45,16 +45,32 @@ actor WordPressClient {
self.rootUrl = rootUrl.url()
}

static func `for`(site: WordPressSite, in session: URLSession) throws -> WordPressClient {
let parsedUrl = try ParsedUrl.parse(input: site.baseUrl)
init(site: WordPressSite) {
// `site.barUrl` is a legal HTTP URL, which should be convertable to the `ParsedUrl` type.
let parsedUrl: ParsedUrl
do {
parsedUrl = try ParsedUrl.parse(input: site.baseUrl.absoluteString)
} catch {
fatalError("Failed to cast URL (\(site.baseUrl.absoluteString)) to ParsedUrl: \(error)")
}

// 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)
Copy link
Contributor

@kean kean Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After switching the site to application password authentication, the stored cookies may interfere with application-password authentication

Is there a way to clear these before switching to application passwords? Using .ephemeral is a nuclear option.

application-password authentication

(Related) Is it still considered viable for an app?

Copy link
Contributor Author

@crazytonyli crazytonyli Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to clear these before switching to application passwords?

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.

Using .ephemeral is a nuclear option.

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.

(Related) Is it still considered viable for an app?

Yes. That's going to be the main way to communicate with REST API.

Copy link
Contributor

@kean kean Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind clarifying what you mean by "nuclear"?

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.

Yes. That's going to be the main way to communicate with REST API.

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.

Copy link
Contributor Author

@crazytonyli crazytonyli Oct 30, 2024

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 can guarantee that no cookies or (persistent) caches will ever be needed, especially if plugins are involved.

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.

If the issue is the cookies, the app should remove the cookies, so that it's not limited by the ephemeral session.

I just opened #23725 for this.

I think it may become a problem because application passwords can be disabled in WordPress

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

Copy link
Contributor

@kean kean Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or endpoints in the WP core are not modified

Anyway, it's not the strongest argument in favor of avoiding .ephemeral. I just wanted to say that it shouldn't be the first option because it's not exactly what it's designed for. I'd steer towards using the defaults (cookies/caches enabled) unless it's not possible.

xmlrpc

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.

Copy link
Contributor Author

@crazytonyli crazytonyli Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or endpoints in the WP core are not modified

Anyway, it's not the strongest argument in favor of avoiding .ephemeral.

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 URLSession, I'm fine with switching back to using .shared, now that the root cause of the issue is fixed in #23725.

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.
[...]
Are there compelling reasons to replace cookies with application passwords?

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))
return WordPressClient(api: api, rootUrl: parsedUrl)
self.init(api: api, rootUrl: parsedUrl)
case .selfHosted(let username, let authToken):
let api = WordPressAPI.init(urlSession: session, baseUrl: parsedUrl, authenticationStategy: .init(username: username, password: authToken))
return WordPressClient(api: api, rootUrl: parsedUrl)
self.init(api: api, rootUrl: parsedUrl)
}
}

Expand Down
2 changes: 1 addition & 1 deletion WordPress/Classes/ViewRelated/Blog/Blog Dashboard/ViewModel/BlogDashboardViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ final class BlogDashboardViewModel {
var _error: Error?

do {
self.wordpressClient = try WordPressClient.for(site: .from(blog: self.blog), in: .shared)
self.wordpressClient = try WordPressClient(site: .init(blog: self.blog))
} catch {
_error = error
self.wordpressClient = nil
Expand Down
6 changes: 3 additions & 3 deletions ...ress/Classes/ViewRelated/Blog/Blog Details/BlogDetailsViewController+SectionHelpers.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ extension BlogDetailsViewController {

@objc func shouldShowApplicationPasswordRow() -> Bool {
// Only available for application-password authenticated self-hosted sites.
return self.blog.account == nil && self.blog.userID != nil && (try? WordPressSite.from(blog: self.blog)) != nil
return self.blog.account == nil && self.blog.userID != nil && (try? WordPressSite(blog: self.blog)) != nil
}

private func createApplicationPasswordService() -> ApplicationPasswordService? {
Expand All @@ -129,8 +129,8 @@ extension BlogDetailsViewController {
}

do {
let site = try WordPressSite.from(blog: self.blog)
let client = try WordPressClient.for(site: site, in: .shared)
let site = try WordPressSite(blog: self.blog)
let client = WordPressClient(site: site)
return ApplicationPasswordService(api: client, currentUserId: userId)
} catch {
DDLogError("Failed to create WordPressClient: \(error)")
Expand Down