Add Login Spec #532

jkmassel · 2025-02-06T16:58:09Z

As discussed with @oguzkocer – there are a lot of different things that can happen during login, so let's document them so we can ensure we're testing them for each HTTP implementation (the native Rust one, Apple platforms' URLSession, and Android)

oguzkocer

@jkmassel I've left some comments about the Rust implementation. I haven't reviewed the documentation or the Swift changes yet.

wp_api/src/request.rs

wp_api/src/login/url_discovery.rs

wp_api/src/login.rs

oguzkocer · 2025-02-15T01:08:03Z

Documentation/login-spec.md

+
+Reference Implementation: http://localhost
+
+# 3: WordPress Admin URL Entry


This says WordPress Admin, but it handles both /wp-login and wp-admin cases, should we rename it?

crazytonyli

Regarding the Rust API comments, I tried to make the changes myself. But as I was making changes to them, it came to me that you guys may have tried that already. So, I haven't gone too deep into them, but I'm happy to make some proof of concept changes if needed.

crazytonyli · 2025-04-02T08:09:21Z

native/swift/Sources/wordpress-api/Foundation+Extensions.swift

@@ -17,3 +17,23 @@ public extension Date {
        wordpressDateFormatter.date(from: string)
    }
 }
+
+public extension TimeInterval {
+    static func fromRetryHeaderValue(_ value: String, now: Date = Date()) -> TimeInterval? {


This function does not seem to be used anywhere except unit tests.

Removed in 8e99aa7 – this was moved to Rust!

crazytonyli · 2025-04-02T08:12:34Z

native/swift/Sources/wordpress-api/SafeRequestExecutor.swift

+
+    private func fetch(request: URLRequest) async throws -> (Data, URLResponse) {
+        if #available(macOS 12.0, *) {
+            return try await self.session.data(for: request, delegate: executorDelegate)


This function is available on all Apple platforms. Maybe we can change this function to be something like this?

#if os(Linux) return session.data(for: request) #else return session.data(for: request, delegate: executorDelegate) #endif

Addressed in 23fdee9 – this was because all the development happened on macOS

crazytonyli · 2025-04-02T08:20:03Z

native/swift/Sources/wordpress-api/SafeRequestExecutor.swift

    }

-    private func getPeerCertificateChain(_ error: Error) throws -> [SSLCertificateInfo] {
+    private func getPeerCertificateChain(_ error: Error) -> [SSLCertificateInfo]? {
        let nserror = error as NSError
        let info = nserror.userInfo

        guard let certChainArray = info["NSErrorPeerCertificateChainKey"] as? NSArray else {


We don't need the parseCertificateChain function below if changing this casting statement to info["NSErrorPeerCertificateChainKey"] as? [SecCertificate]

Nice! Done in c07115c

native/swift/Sources/wordpress-api/WordPressLoginClient.swift

crazytonyli · 2025-04-02T09:06:14Z

native/swift/Sources/wordpress-api/WordPressLoginClient.swift

+        forSite proposedSiteUrl: String
+    ) async throws(AutoDiscoveryAttemptFailure) -> AutoDiscoveryAttemptResult {
+
+        let discoveryResult = await client.apiDiscovery(siteUrl: proposedSiteUrl)


Can we change this function to fn api_discovery(&self, site_url: String) -> Result<Arc<AutoDiscoveryAttemptSuccess>, AutoDiscoveryAttemptFailure> and do the if-else checks below within Rust?

The function currently returns AutoDiscoveryResult/AutoDiscoveryUniffiResult which is the result of combination of multiple attempts, so this suggestion wouldn't work. Happy to elaborate on it if you need more details.

Yes. I'd love more details.

I haven't looked the how Kotlin uses the api_discovery function. But the Swift function here turns the AutoDiscoveryUniffiResult into either a success result or a failure, which essentially is Result<Arc<AutoDiscoveryAttemptSuccess>, AutoDiscoveryAttemptFailure>.

crazytonyli · 2025-04-02T09:12:16Z

native/swift/Sources/wordpress-api/SafeRequestExecutor.swift

+
+    init(
+        urlSession: URLSession,
+        additionalHttpHeadersForAllRequests: [String: String] = [:]


This argument is not used.

Not by our code, but this is a good back-door way to add a user agent (for instance).

Should that just be a separate PR?

The same feature can be achieved using URLSession. But I guess this argument would allow adding headers only for API requests originated from the library. I'm okay with it being in this PR if it's needed.

I'm hoping we can limit the number of things that a developer has to handle themselves in URLSession, but I guess we'll see?

crazytonyli · 2025-04-02T09:19:13Z

native/swift/Sources/wordpress-api/WordPressLoginClient.swift

+import FoundationNetworking
+#endif
+
+public final class WordPressLoginClient {


BTW, there is not much going on in this type. Can we use UniffiWpLoginClient directly? You'll need to add the loginURL(...) and credentials(...) helper functions to it, but I don't think that's a terrible thing to do.

I'd be fine if we get there eventually, but the name is wrong – nobody consuming this library should ever see UniffiWpLoginClient. At the very least we'll have to typealias it

oguzkocer

I haven't finished the review yet, but I left some pretty important comments about the Kotlin implementation which may affect the remaining files I haven't reviewed yet.

P.S: I usually review my comments before posting, but I am not able to do that at the moment and don't want to risk losing them. So, if there is anything poorly worded or plain incorrect, I apologize in advance as it was unintentional.

wp_api/src/parsed_url.rs

oguzkocer · 2025-04-02T17:03:50Z

native/kotlin/build.gradle.kts

+
+    tasks.register<Copy>("copySampleJSON") {
+        dependsOn(tasks.named("deleteTestResources"))
+        from("$cargoProjectRoot/native/swift/Tests/wordpress-api/Resources/Responses/localhost-json-root.json")


Probably best to put this somewhere in the native directory, rather than under swift or kotlin.

I'd say let's do this as soon as we have a second item

native/kotlin/api/kotlin/src/main/kotlin/rs/wordpress/api/kotlin/MockRequestExecutor.kt

native/kotlin/api/kotlin/src/main/kotlin/rs/wordpress/api/kotlin/WpRequestExecutor.kt

native/kotlin/api/kotlin/src/main/kotlin/rs/wordpress/api/kotlin/WpLoginClient.kt

crazytonyli · 2025-04-07T00:34:07Z

native/swift/Tests/wordpress-api/LoginTests.swift

    func testNotWordPressSite() async throws {
-        let client = WordPressLoginClient(urlSession: .shared)
+        await #expect(performing: {
+            _ = try await client.findLoginUrl(forSite: "https://google.com")


When the input is "google.com", the returned error is ParseUrlError.RelativeUrlWithoutBase. IMO that's an incorrect error. I'd expect the error to be probablyNotAWordPressSite.

I tested this in Rust with the new combinedResult function from #652 and it returned ProbablyNotAWordPressSite as expected. Once this PR is updated, the issue should be resolved for Swift as well.

// Full error message for `google.com` FindApiRoot { parsed_site_url: ParsedUrl { inner: Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some( Domain( "google.com", ), ), port: None, path: "/", query: None, fragment: None, }, }, find_api_root_failure: ProbablyNotAWordPressSite, }

@jkmassel Do you mind adding google.com as a test case, which should be fairly easy with @Test(arguments: ["google.com", "https://google.com"])?

crazytonyli

Left a couple of minor comments.

crazytonyli · 2025-04-09T02:29:52Z

native/swift/Sources/wordpress-api/WordPressLoginClient.swift

+
+    public protocol AuthenticatorProtocol {
+        func authenticate(url: URL, callbackURL: URL) async throws -> URL
+    }


This protocol can be removed.

We moved pretty much all the Kotlin stuff to #655

jkmassel force-pushed the add/login-scenarios branch 8 times, most recently from a87e570 to b351665 Compare February 12, 2025 02:38

jkmassel mentioned this pull request Feb 12, 2025

Add https auto discovery attempt for http urls #549

Merged

jkmassel force-pushed the add/login-scenarios branch 4 times, most recently from 68c4b18 to bc249b1 Compare February 14, 2025 20:26

oguzkocer reviewed Feb 14, 2025

View reviewed changes

jkmassel mentioned this pull request Feb 15, 2025

Adds helpers to WpApiDetails and tracks retry count in WpNetworkRequest #555

Merged

oguzkocer reviewed Feb 15, 2025

View reviewed changes

jkmassel force-pushed the add/login-scenarios branch 2 times, most recently from cd36fa5 to b35616a Compare February 20, 2025 17:53

jkmassel mentioned this pull request Feb 20, 2025

Move Login Application Errors to Rust #572

Merged

jkmassel force-pushed the add/login-scenarios branch from f394028 to 7e0c993 Compare February 20, 2025 20:59

jkmassel changed the base branch from trunk to move/login-errors-to-rust February 20, 2025 20:59

jkmassel force-pushed the add/login-scenarios branch 2 times, most recently from 4db0bf9 to c1983be Compare February 20, 2025 22:34

jkmassel force-pushed the move/login-errors-to-rust branch from 82ec5ab to 19ebcb2 Compare February 20, 2025 22:35

Base automatically changed from move/login-errors-to-rust to trunk February 20, 2025 23:02

jkmassel force-pushed the add/login-scenarios branch 5 times, most recently from 1de057f to 17c1c30 Compare February 21, 2025 18:35

jkmassel force-pushed the add/login-scenarios branch from 5c612dd to cf435e4 Compare March 17, 2025 21:03

jkmassel force-pushed the add/login-scenarios branch 6 times, most recently from 8343090 to 8ec93da Compare April 2, 2025 03:48

crazytonyli reviewed Apr 2, 2025

View reviewed changes

oguzkocer previously requested changes Apr 2, 2025

View reviewed changes

jkmassel marked this pull request as ready for review April 3, 2025 18:52

crazytonyli reviewed Apr 7, 2025

View reviewed changes

oguzkocer mentioned this pull request Apr 8, 2025

Extract Kotlin changes from Add Login Spec PR #655

Merged

jkmassel added 3 commits April 8, 2025 19:49

Add Login Spec

2da7bd5

Add Swift tests implementing all scenarios

ad923b4

Disable login tests on Linux for Swift

e8a1e3e

jkmassel force-pushed the add/login-scenarios branch from 8ec93da to e8a1e3e Compare April 9, 2025 01:49

jkmassel added 3 commits April 8, 2025 20:19

Remove Swift retry-after parsing

8e99aa7

Use the executorDelegate on all Apple platforms

23fdee9

Remove parseCertificateChain

c07115c

jkmassel requested review from crazytonyli and oguzkocer April 9, 2025 02:24

crazytonyli approved these changes Apr 9, 2025

View reviewed changes

Address multiple feedback items

83d2317

jkmassel enabled auto-merge (squash) April 9, 2025 02:36

Fix Linux tests

d13cd99

jkmassel merged commit fae32d5 into trunk Apr 9, 2025
20 checks passed

jkmassel deleted the add/login-scenarios branch April 9, 2025 03:39


		Reference Implementation: http://localhost

		# 3: WordPress Admin URL Entry

Add Login Spec #532

Add Login Spec #532

Uh oh!

Conversation

jkmassel commented Feb 6, 2025

Uh oh!

oguzkocer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crazytonyli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oguzkocer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crazytonyli Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crazytonyli left a comment

Choose a reason for hiding this comment

crazytonyli Apr 9, 2025 •

edited

Loading