Skip to content

Conversation

@crazytonyli
Copy link
Contributor

See the inline code comment for details.

Regression Notes

  1. Potential unintended areas of impact

  2. What I did to test those areas of impact (or what existing automated tests I relied on)

  3. What automated tests I added (or what prevented me from doing so)

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing checklist:

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@crazytonyli crazytonyli added this to the 25.6 milestone Oct 29, 2024
@crazytonyli crazytonyli requested review from jkmassel and kean October 29, 2024 00:30
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Oct 29, 2024

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr23715-8b3c7bc
Version25.4.1
Bundle IDorg.wordpress.alpha
Commit8b3c7bc
App Center BuildWPiOS - One-Offs #10910
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Oct 29, 2024

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr23715-8b3c7bc
Version25.4.1
Bundle IDcom.jetpack.alpha
Commit8b3c7bc
App Center Buildjetpack-installable-builds #9950
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

}

static func `for`(site: WordPressSite, in session: URLSession) throws -> WordPressClient {
static func `for`(site: WordPressSite) throws -> WordPressClient {
Copy link
Contributor

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:

Begin names of factory methods with “make”, e.g. x.makeIterator().

from https://www.swift.org/documentation/api-design-guidelines/

Copy link
Contributor Author

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

let client = WordPressClient.for(site)

I think site.makeClient() makes more sense than WordPressClient.make(site). But I have changed this function to be an initialiser in 3343c28.

//
// 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

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

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

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

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

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

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.

@crazytonyli
Copy link
Contributor Author

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.

I decided not to switch back to .shared. Because we are certain that REST API would fail when sending invalidate login cookies. Using an ephemeral session would guarantee that the issue does not occur.

Since we don't have any concrete issues in using ephemeral sessions, would you be okay with merging this change for now? @kean

Copy link
Contributor

@kean kean left a comment

Choose a reason for hiding this comment

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

Because we are certain that REST API would fail when sending invalidate login cookies. Using an ephemeral session would guarantee that the issue does not occur.

have any concrete issues in using ephemeral sessions

The ephemeral sessions are designed for private browsing and disable both persistent cookies and HTTP cache storage.

In terms of the HTTP cache, while many requests don't rely on HTTP cache, there may be requests that do to achieve acceptable performance. Disabling all persistent caching will likely result in some performance issues.

A WordPress server could also be using cookies for scenarios other than authentication, especially in unknown scenarios where plugins modify that WordPress server networking in some ways, including the existing REST APIs. This, of course, is less likely, but you never know.

If the issue is only with cookies, it should be addressed on that level. There are multiple options for configuration the cookies behavior, including httpShouldSetCookies, httpCookieAcceptPolicy. The client could also use a custom URLSession instance with a non-shared cookie storage, or, ideally, disable only certain cookies.

@crazytonyli
Copy link
Contributor Author

crazytonyli commented Nov 3, 2024

The ephemeral sessions are designed for private browsing.

Of course. That's a typical use case. It's not the only use case, though. That does not mean apps that do not have "private browsing" mode should never use ephemeral sessions. Not many apps have a "private browsing" mode.

In terms of the HTTP cache, while many requests don't rely on HTTP cache, there may be requests that do to achieve acceptable performance. Disabling all persistent caching will likely result in some performance issues.

Can you elaborate on this? I don't see how HTTP cache can help with REST API performance, considering responses are in plaintext, which should not be too large.

A WordPress server could also be using cookies for scenarios other than authentication, especially in unknown scenarios where plugins modify that WordPress server networking in some ways, including the existing REST APIs. This, of course, is less likely, but you never know.

I believe I answered this concern in #23715 (comment). Let me know if I did not.

If the issue is only with cookies, it should be addressed on that level. There are multiple options for configuration the cookies behavior, including httpShouldSetCookies, httpCookieAcceptPolicy.

But with those configurations, there won't be too many differences than just using ephemeral sessions. See this note on the API doc:

It is possible to customize a default session configuration object to obtain the same behavior (or any portion thereof) provided by an ephemeral session configuration object, but the use of this method is more convenient.

@kean kean self-requested a review November 4, 2024 12:04
Copy link
Contributor

@kean kean left a comment

Choose a reason for hiding this comment

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

Since you are set on using ephemeral sessions over other options, I'm approving it to unblock it. It's going to be easy to change later if needed.

Can you elaborate on this? I don't see how HTTP cache can help with REST API performance, considering responses are in plaintext, which should not be too large.

It's faster to not have to send the request than to wait for a round-trip even if the response is small. There is no point of disabling a possible optimization.

@crazytonyli crazytonyli added this pull request to the merge queue Nov 4, 2024
Merged via the queue into trunk with commit 4731816 Nov 4, 2024
24 checks passed
@crazytonyli crazytonyli deleted the self-hosted-sites-user-management branch November 4, 2024 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants