Extract login details improvements

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

@@ -85,8 +85,9 @@ public struct WordPressAPI {
     }
 
     public struct Helpers {
-        public static func extractLoginDetails(from url: URL) -> WpApiApplicationPasswordDetails? {
-            return extractLoginDetailsFromUrl(url: url.asRestUrl())
+        public static func extractLoginDetails(from url: URL) throws -> WpApiApplicationPasswordDetails? {
+            let parsedUrl = try ParsedUrl.from(url: url)
+            return try extractLoginDetailsFromUrl(url: parsedUrl)
         }
     }
 
@@ -195,18 +196,8 @@ extension RequestMethod {
     }
 }
 
-extension WpRestApiUrl {
-    func asUrl() -> URL {
-        guard let url = URL(string: stringValue) else {
-            preconditionFailure("Invalid URL: \(stringValue)")
-        }
-
-        return url
-    }
-}
-
-extension URL {
-    func asRestUrl() -> WpRestApiUrl {
-        WpRestApiUrl(stringValue: self.absoluteString)
+extension ParsedUrl {
+    static func from(url: URL) throws -> ParsedUrl {
+        try parse(input: url.absoluteString)
     }
 }

wp_api/src/lib.rs

@@ -14,10 +14,13 @@ use std::sync::Arc;
 pub use api_error::{
     RequestExecutionError, WpApiError, WpRestError, WpRestErrorCode, WpRestErrorWrapper,
 };
+pub use parsed_url::{ParseUrlError, ParsedUrl};
 use plugins::*;
 use users::*;
 
 mod api_error; // re-exported relevant types
+mod parsed_url; // re-exported relevant types
+
 pub mod application_passwords;
 pub mod login;
 pub mod plugins;

wp_api/src/login.rs

@@ -2,11 +2,12 @@ use serde::{Deserialize, Serialize};
 use std::collections::HashMap;
 use std::str;
 use std::sync::Arc;
-use url::Url;
 
 pub use login_client::WpLoginClient;
 pub use url_discovery::{UrlDiscoveryState, UrlDiscoverySuccess};
 
+use crate::ParsedUrl;
+
 const KEY_APPLICATION_PASSWORDS: &str = "application-passwords";
 
 mod login_client;
@@ -22,28 +23,26 @@ pub struct WpRestApiUrls {
 // embedded as query params. This function parses that URL and extracts the login details as an object.
 #[uniffi::export]
 pub fn extract_login_details_from_url(
-    url: WpRestApiUrl,
-) -> Option<WpApiApplicationPasswordDetails> {
-    if let (Some(site_url), Some(user_login), Some(password)) =
-        url.as_url()
+    url: Arc<ParsedUrl>,
+) -> Result<WpApiApplicationPasswordDetails, OAuthResponseUrlError> {
+    let f = |key| {
+        url.inner
             .query_pairs()
-            .fold((None, None, None), |accum, (k, v)| {
-                match k.to_string().as_str() {
-                    "site_url" => (Some(v.to_string()), accum.1, accum.2),
-                    "user_login" => (accum.0, Some(v.to_string()), accum.2),
-                    "password" => (accum.0, accum.1, Some(v.to_string())),
-                    _ => accum,
-                }
-            })
-    {
-        Some(WpApiApplicationPasswordDetails {
-            site_url,
-            user_login,
-            password,
-        })
-    } else {
-        None
+            .find_map(|(k, v)| (k == key).then_some(v.to_string()))
+    };
+    if let Some(is_success) = f("success") {
+        if is_success == "false" {
+            return Err(OAuthResponseUrlError::UnsuccessfulLogin);
+        }
     }
+    let site_url = f("site_url").ok_or(OAuthResponseUrlError::MissingSiteUrl)?;
+    let user_login = f("user_login").ok_or(OAuthResponseUrlError::MissingUsername)?;
+    let password = f("password").ok_or(OAuthResponseUrlError::MissingPassword)?;
+    Ok(WpApiApplicationPasswordDetails {
+        site_url,
+        user_login,
+        password,
+    })
 }
 
 #[derive(Debug, Serialize, Deserialize, uniffi::Object)]
@@ -78,41 +77,70 @@ pub struct WpRestApiAuthenticationEndpoint {
     pub authorization: String,
 }
 
-#[derive(Debug, Serialize, Deserialize, uniffi::Record)]
+#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize, uniffi::Record)]
 pub struct WpApiApplicationPasswordDetails {
     pub site_url: String,
     pub user_login: String,
     pub password: String,
 }
 
-// A type that's guaranteed to represent a valid URL
-//
-// It is a programmer error to instantiate this object with an invalid URL
-#[derive(Debug, uniffi::Record)]
-pub struct WpRestApiUrl {
-    pub string_value: String,
-}
-
-impl WpRestApiUrl {
-    pub fn as_str(&self) -> &str {
-        self.string_value.as_str()
-    }
-
-    pub fn as_url(&self) -> url::Url {
-        Url::parse(self.string_value.as_str()).unwrap()
-    }
+#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, thiserror::Error, uniffi::Error)]
+pub enum OAuthResponseUrlError {
+    #[error("The given URL is missing the `site_url` query parameter")]
+    MissingSiteUrl,
+    #[error("The given URL is missing the `username` query parameter")]
+    MissingUsername,
+    #[error("The given URL is missing the `password` query parameter")]
+    MissingPassword,
+    #[error("Unsuccessful Login")]
+    UnsuccessfulLogin,
 }
 
-impl From<Url> for WpRestApiUrl {
-    fn from(url: url::Url) -> Self {
-        WpRestApiUrl {
-            string_value: url.into(),
-        }
-    }
-}
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use rstest::rstest;
 
-impl From<WpRestApiUrl> for String {
-    fn from(url: WpRestApiUrl) -> Self {
-        url.string_value
+    #[rstest]
+    #[case(
+        "exampleauth://login?site_url=http://example.com&user_login=test&password=1234",
+        Ok(())
+    )]
+    #[case(
+        "exampleauth://login?site_url=http://example.com&user_login=test&password=1234&foo=bar",
+        Ok(())
+    )]
+    #[case(
+        "exampleauth://login?user_login=test&password=1234",
+        Err(OAuthResponseUrlError::MissingSiteUrl)
+    )]
+    #[case(
+        "exampleauth://login?site_url=http://example.com&password=1234",
+        Err(OAuthResponseUrlError::MissingUsername)
+    )]
+    #[case(
+        "exampleauth://login?site_url=http://example.com&user_login=test",
+        Err(OAuthResponseUrlError::MissingPassword)
+    )]
+    #[case(
+        "exampleauth://login?success=false",
+        Err(OAuthResponseUrlError::UnsuccessfulLogin)
+    )]
+    #[case(
+        "exampleauth://login?success=true",
+        Err(OAuthResponseUrlError::MissingSiteUrl)
+    )]
+    fn test_extract_login_details_from_url(
+        #[case] input: &str,
+        #[case] expected_result: Result<(), OAuthResponseUrlError>,
+    ) {
+        assert_eq!(
+            extract_login_details_from_url(ParsedUrl::try_from(input).unwrap().into()),
+            expected_result.map(|_| WpApiApplicationPasswordDetails {
+                site_url: "http://example.com".to_string(),
+                user_login: "test".to_string(),
+                password: "1234".to_string(),
+            })
+        );
     }
 }

wp_api/src/login/login_client.rs

@@ -5,11 +5,11 @@ use crate::request::endpoint::WpEndpointUrl;
 use crate::request::{
     RequestExecutor, RequestMethod, WpNetworkHeaderMap, WpNetworkRequest, WpNetworkResponse,
 };
+use crate::ParsedUrl;
 
 use super::url_discovery::{
-    self, FetchApiDetailsError, FetchApiRootUrlError, ParsedUrl, StateInitial,
-    UrlDiscoveryAttemptError, UrlDiscoveryAttemptSuccess, UrlDiscoveryError, UrlDiscoveryState,
-    UrlDiscoverySuccess,
+    self, FetchApiDetailsError, FetchApiRootUrlError, StateInitial, UrlDiscoveryAttemptError,
+    UrlDiscoveryAttemptSuccess, UrlDiscoveryError, UrlDiscoveryState, UrlDiscoverySuccess,
 };
 
 const API_ROOT_LINK_HEADER: &str = "https://api.w.org/";

wp_api/src/login/url_discovery.rs

@@ -1,9 +1,8 @@
 use std::sync::Arc;
-use url::Url;
 
 use crate::{
     request::{WpNetworkHeaderMap, WpNetworkResponse},
-    RequestExecutionError,
+    ParseUrlError, ParsedUrl, RequestExecutionError,
 };
 
 use super::WpApiDetails;
@@ -108,7 +107,7 @@ impl StateParsedUrl {
         {
             Some(url) => Ok(StateFetchedApiRootUrl {
                 site_url: self.site_url,
-                api_root_url: ParsedUrl { url },
+                api_root_url: ParsedUrl::new(url),
             }),
             None => Err(FetchApiRootUrlError::ApiRootLinkHeaderNotFound {
                 header_map: response.header_map,
@@ -224,40 +223,6 @@ impl From<RequestExecutionError> for FetchApiDetailsError {
     }
 }
 
-// TODO: Should be in a central place, used across the code base
-#[derive(Debug, Clone, uniffi::Object)]
-pub struct ParsedUrl {
-    url: Url,
-}
-
-impl ParsedUrl {
-    fn parse(input: &str) -> Result<Self, ParseUrlError> {
-        Url::parse(input)
-            .map_err(|e| match e {
-                url::ParseError::RelativeUrlWithoutBase => ParseUrlError::RelativeUrlWithoutBase,
-                _ => ParseUrlError::Generic {
-                    reason: e.to_string(),
-                },
-            })
-            .map(|url| Self { url })
-    }
-}
-
-#[uniffi::export]
-impl ParsedUrl {
-    pub fn url(&self) -> String {
-        self.url.to_string()
-    }
-}
-
-#[derive(Debug, thiserror::Error, uniffi::Error)]
-pub enum ParseUrlError {
-    #[error("Error while parsing url: {}", reason)]
-    Generic { reason: String },
-    #[error("Relative URL without a base")]
-    RelativeUrlWithoutBase,
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;

wp_api/src/parsed_url.rs

@@ -0,0 +1,115 @@
+use url::Url;
+
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, uniffi::Object)]
+pub struct ParsedUrl {
+    pub inner: Url,
+}
+
+impl ParsedUrl {
+    pub fn new(url: Url) -> Self {
+        Self { inner: url }
+    }
+}
+
+#[uniffi::export]
+impl ParsedUrl {
+    #[uniffi::constructor]
+    pub fn parse(input: &str) -> Result<Self, ParseUrlError> {
+        Url::parse(input)
+            .map(Self::new)
+            .map_err(ParseUrlError::from)
+    }
+
+    pub fn url(&self) -> String {
+        self.inner.to_string()
+    }
+}
+
+#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, thiserror::Error, uniffi::Error)]
+pub enum ParseUrlError {
+    #[error("Error while parsing url: {}", reason)]
+    Generic { reason: String },
+    #[error("empty host")]
+    EmptyHost,
+    #[error("invalid international domain name")]
+    IdnaError,
+    #[error("invalid port number")]
+    InvalidPort,
+    #[error("invalid IPv4 address")]
+    InvalidIpv4Address,
+    #[error("invalid IPv6 address")]
+    InvalidIpv6Address,
+    #[error("invalid domain character")]
+    InvalidDomainCharacter,
+    #[error("relative URL without a base")]
+    RelativeUrlWithoutBase,
+    #[error("relative URL with a cannot-be-a-base base")]
+    RelativeUrlWithCannotBeABaseBase,
+    #[error("a cannot-be-a-base URL doesn’t have a host to set")]
+    SetHostOnCannotBeABaseUrl,
+    #[error("URLs more than 4 GB are not supported")]
+    Overflow,
+}
+
+impl From<url::ParseError> for ParseUrlError {
+    fn from(value: url::ParseError) -> Self {
+        use url::ParseError;
+        match value {
+            ParseError::EmptyHost => Self::EmptyHost,
+            ParseError::IdnaError => Self::IdnaError,
+            ParseError::InvalidPort => Self::InvalidPort,
+            ParseError::InvalidIpv4Address => Self::InvalidIpv4Address,
+            ParseError::InvalidIpv6Address => Self::InvalidIpv6Address,
+            ParseError::InvalidDomainCharacter => Self::InvalidDomainCharacter,
+            ParseError::RelativeUrlWithoutBase => Self::RelativeUrlWithoutBase,
+            ParseError::RelativeUrlWithCannotBeABaseBase => Self::RelativeUrlWithCannotBeABaseBase,
+            ParseError::SetHostOnCannotBeABaseUrl => Self::SetHostOnCannotBeABaseUrl,
+            ParseError::Overflow => Self::Overflow,
+            _ => Self::Generic {
+                reason: value.to_string(),
+            },
+        }
+    }
+}
+
+impl TryFrom<&str> for ParsedUrl {
+    type Error = ParseUrlError;
+
+    fn try_from(input: &str) -> Result<Self, Self::Error> {
+        Self::parse(input)
+    }
+}
+
+impl From<Url> for ParsedUrl {
+    fn from(input: Url) -> Self {
+        Self::new(input)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use rstest::rstest;
+    use url::Url;
+
+    #[rstest]
+    #[case("http://example.com")]
+    fn parse_url_success(#[case] input: &str) {
+        let parsed_url = ParsedUrl::parse(input).unwrap();
+        assert_eq!(parsed_url.url(), "http://example.com/",);
+        assert_eq!(parsed_url, Url::parse("http://example.com").unwrap().into());
+    }
+
+    #[rstest]
+    #[case("https://", ParseUrlError::EmptyHost)]
+    #[case("https://example.com:foo", ParseUrlError::InvalidPort)]
+    #[case("https://1.2.3.4.5", ParseUrlError::InvalidIpv4Address)]
+    #[case("https://[1", ParseUrlError::InvalidIpv6Address)]
+    #[case("https:// .com", ParseUrlError::InvalidDomainCharacter)]
+    #[case("", ParseUrlError::RelativeUrlWithoutBase)]
+    // https://www.unicode.org/reports/tr46/#Validity_Criteria
+    #[case("https://xn--u-ccb.com", ParseUrlError::IdnaError)]
+    fn parse_url_error(#[case] input: &str, #[case] expected_err: ParseUrlError) {
+        assert_eq!(ParsedUrl::try_from(input).unwrap_err(), expected_err);
+    }
+}