From 445c137d3a440cd5dad49cec6f2a7c8edaa71e8d Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Wed, 3 Jul 2024 17:42:38 -0400 Subject: [PATCH 1/6] Remove WpRestApiUrl in favor of ParsedUrl --- wp_api/src/login.rs | 38 +++---------------------------- wp_api/src/login/url_discovery.rs | 19 ++++++++++------ 2 files changed, 15 insertions(+), 42 deletions(-) diff --git a/wp_api/src/login.rs b/wp_api/src/login.rs index 306700f6b..2534447c2 100644 --- a/wp_api/src/login.rs +++ b/wp_api/src/login.rs @@ -2,7 +2,7 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; use std::str; use std::sync::Arc; -use url::Url; +use url_discovery::ParsedUrl; pub use login_client::WpLoginClient; pub use url_discovery::{UrlDiscoveryState, UrlDiscoverySuccess}; @@ -22,10 +22,10 @@ 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, + url: Arc, ) -> Option { if let (Some(site_url), Some(user_login), Some(password)) = - url.as_url() + url.inner .query_pairs() .fold((None, None, None), |accum, (k, v)| { match k.to_string().as_str() { @@ -84,35 +84,3 @@ pub struct WpApiApplicationPasswordDetails { 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() - } -} - -impl From for WpRestApiUrl { - fn from(url: url::Url) -> Self { - WpRestApiUrl { - string_value: url.into(), - } - } -} - -impl From for String { - fn from(url: WpRestApiUrl) -> Self { - url.string_value - } -} diff --git a/wp_api/src/login/url_discovery.rs b/wp_api/src/login/url_discovery.rs index b3377107b..5c6a6c9ce 100644 --- a/wp_api/src/login/url_discovery.rs +++ b/wp_api/src/login/url_discovery.rs @@ -108,7 +108,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, @@ -227,10 +227,18 @@ impl From for FetchApiDetailsError { // TODO: Should be in a central place, used across the code base #[derive(Debug, Clone, uniffi::Object)] pub struct ParsedUrl { - url: Url, + pub(crate) inner: Url, } impl ParsedUrl { + fn new(url: Url) -> Self { + Self { inner: url } + } +} + +#[uniffi::export] +impl ParsedUrl { + #[uniffi::constructor] fn parse(input: &str) -> Result { Url::parse(input) .map_err(|e| match e { @@ -239,14 +247,11 @@ impl ParsedUrl { reason: e.to_string(), }, }) - .map(|url| Self { url }) + .map(Self::new) } -} -#[uniffi::export] -impl ParsedUrl { pub fn url(&self) -> String { - self.url.to_string() + self.inner.to_string() } } From 0e49aac092d9f3aef0fc51854b8d4988d3d41f35 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Wed, 3 Jul 2024 17:49:14 -0400 Subject: [PATCH 2/6] Move ParsedUrl into its own module --- wp_api/src/lib.rs | 3 +++ wp_api/src/login.rs | 3 ++- wp_api/src/login/login_client.rs | 6 ++--- wp_api/src/login/url_discovery.rs | 42 +------------------------------ wp_api/src/parsed_url.rs | 39 ++++++++++++++++++++++++++++ 5 files changed, 48 insertions(+), 45 deletions(-) create mode 100644 wp_api/src/parsed_url.rs diff --git a/wp_api/src/lib.rs b/wp_api/src/lib.rs index 08bffd283..05e83e6a8 100644 --- a/wp_api/src/lib.rs +++ b/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; diff --git a/wp_api/src/login.rs b/wp_api/src/login.rs index 2534447c2..69c91e3d1 100644 --- a/wp_api/src/login.rs +++ b/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_discovery::ParsedUrl; pub use login_client::WpLoginClient; pub use url_discovery::{UrlDiscoveryState, UrlDiscoverySuccess}; +use crate::ParsedUrl; + const KEY_APPLICATION_PASSWORDS: &str = "application-passwords"; mod login_client; diff --git a/wp_api/src/login/login_client.rs b/wp_api/src/login/login_client.rs index 3d3aadf80..6af74ea66 100644 --- a/wp_api/src/login/login_client.rs +++ b/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/"; diff --git a/wp_api/src/login/url_discovery.rs b/wp_api/src/login/url_discovery.rs index 5c6a6c9ce..10dbbc7c1 100644 --- a/wp_api/src/login/url_discovery.rs +++ b/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; @@ -224,45 +223,6 @@ impl From for FetchApiDetailsError { } } -// TODO: Should be in a central place, used across the code base -#[derive(Debug, Clone, uniffi::Object)] -pub struct ParsedUrl { - pub(crate) inner: Url, -} - -impl ParsedUrl { - fn new(url: Url) -> Self { - Self { inner: url } - } -} - -#[uniffi::export] -impl ParsedUrl { - #[uniffi::constructor] - fn parse(input: &str) -> Result { - Url::parse(input) - .map_err(|e| match e { - url::ParseError::RelativeUrlWithoutBase => ParseUrlError::RelativeUrlWithoutBase, - _ => ParseUrlError::Generic { - reason: e.to_string(), - }, - }) - .map(Self::new) - } - - pub fn url(&self) -> String { - self.inner.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::*; diff --git a/wp_api/src/parsed_url.rs b/wp_api/src/parsed_url.rs new file mode 100644 index 000000000..265260bb2 --- /dev/null +++ b/wp_api/src/parsed_url.rs @@ -0,0 +1,39 @@ +use url::Url; + +#[derive(Debug, Clone, 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 { + Url::parse(input) + .map_err(|e| match e { + url::ParseError::RelativeUrlWithoutBase => ParseUrlError::RelativeUrlWithoutBase, + _ => ParseUrlError::Generic { + reason: e.to_string(), + }, + }) + .map(Self::new) + } + + pub fn url(&self) -> String { + self.inner.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, +} From 8a8ac94bddb74bd2aadd17efd492accc47a928ac Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Wed, 3 Jul 2024 19:00:57 -0400 Subject: [PATCH 3/6] Add unit tests for ParsedUrl & new ParseUrlError variants --- wp_api/src/parsed_url.rs | 110 +++++++++++++++++++++++++++++++++++---- 1 file changed, 101 insertions(+), 9 deletions(-) diff --git a/wp_api/src/parsed_url.rs b/wp_api/src/parsed_url.rs index 265260bb2..0c7ba392c 100644 --- a/wp_api/src/parsed_url.rs +++ b/wp_api/src/parsed_url.rs @@ -1,6 +1,6 @@ use url::Url; -#[derive(Debug, Clone, uniffi::Object)] +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, uniffi::Object)] pub struct ParsedUrl { pub inner: Url, } @@ -16,13 +16,8 @@ impl ParsedUrl { #[uniffi::constructor] pub fn parse(input: &str) -> Result { Url::parse(input) - .map_err(|e| match e { - url::ParseError::RelativeUrlWithoutBase => ParseUrlError::RelativeUrlWithoutBase, - _ => ParseUrlError::Generic { - reason: e.to_string(), - }, - }) .map(Self::new) + .map_err(ParseUrlError::from) } pub fn url(&self) -> String { @@ -30,10 +25,107 @@ impl ParsedUrl { } } -#[derive(Debug, thiserror::Error, uniffi::Error)] +#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, thiserror::Error, uniffi::Error)] pub enum ParseUrlError { #[error("Error while parsing url: {}", reason)] Generic { reason: String }, - #[error("Relative URL without a base")] + #[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 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::parse(input) + } +} + +impl From for ParsedUrl { + fn from(input: Url) -> Self { + Self::new(input) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use rstest::rstest; + use url::Url; + + #[test] + fn success() { + assert_eq!( + "http://example.com/", + ParsedUrl::parse("http://example.com").unwrap().url() + ); + } + + #[test] + fn try_from_str() { + assert_eq!( + "http://example.com/", + ParsedUrl::try_from("http://example.com").unwrap().url() + ); + } + + #[test] + fn can_be_converted_to_url() { + assert_eq!( + Url::parse("http://example.com").unwrap(), + ParsedUrl::try_from("http://example.com").unwrap().inner + ); + } + + #[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 url_errors(#[case] input: &str, #[case] expected_err: ParseUrlError) { + assert_eq!(ParsedUrl::try_from(input).unwrap_err(), expected_err); + } } From b8ef8d5e855c95ca12444e9f191e45f35ca564e3 Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Wed, 3 Jul 2024 19:43:02 -0400 Subject: [PATCH 4/6] Add unit tests for extract_login_details_from_url --- wp_api/src/login.rs | 97 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 78 insertions(+), 19 deletions(-) diff --git a/wp_api/src/login.rs b/wp_api/src/login.rs index 69c91e3d1..c90174ccb 100644 --- a/wp_api/src/login.rs +++ b/wp_api/src/login.rs @@ -24,27 +24,25 @@ pub struct WpRestApiUrls { #[uniffi::export] pub fn extract_login_details_from_url( url: Arc, -) -> Option { - if let (Some(site_url), Some(user_login), Some(password)) = +) -> Result { + 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)] @@ -79,9 +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, } + +#[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, +} + +#[cfg(test)] +mod tests { + use super::*; + use rstest::rstest; + + #[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(), + }) + ); + } +} From b379b756761904365a743fd9d1a684cddec2313a Mon Sep 17 00:00:00 2001 From: Oguz Kocer Date: Wed, 3 Jul 2024 19:48:31 -0400 Subject: [PATCH 5/6] Use parametrized test for parse_url_success --- wp_api/src/parsed_url.rs | 30 +++++++----------------------- 1 file changed, 7 insertions(+), 23 deletions(-) diff --git a/wp_api/src/parsed_url.rs b/wp_api/src/parsed_url.rs index 0c7ba392c..03e6aee83 100644 --- a/wp_api/src/parsed_url.rs +++ b/wp_api/src/parsed_url.rs @@ -92,28 +92,12 @@ mod tests { use rstest::rstest; use url::Url; - #[test] - fn success() { - assert_eq!( - "http://example.com/", - ParsedUrl::parse("http://example.com").unwrap().url() - ); - } - - #[test] - fn try_from_str() { - assert_eq!( - "http://example.com/", - ParsedUrl::try_from("http://example.com").unwrap().url() - ); - } - - #[test] - fn can_be_converted_to_url() { - assert_eq!( - Url::parse("http://example.com").unwrap(), - ParsedUrl::try_from("http://example.com").unwrap().inner - ); + #[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] @@ -125,7 +109,7 @@ mod tests { #[case("", ParseUrlError::RelativeUrlWithoutBase)] // https://www.unicode.org/reports/tr46/#Validity_Criteria #[case("https://xn--u-ccb.com", ParseUrlError::IdnaError)] - fn url_errors(#[case] input: &str, #[case] expected_err: ParseUrlError) { + fn parse_url_error(#[case] input: &str, #[case] expected_err: ParseUrlError) { assert_eq!(ParsedUrl::try_from(input).unwrap_err(), expected_err); } } From 3c7d6a026e029e83f050080be8cdd2964a945bae Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Thu, 4 Jul 2024 11:51:25 -0600 Subject: [PATCH 6/6] Add Swift compatibility --- .../Sources/wordpress-api/WordPressAPI.swift | 21 ++++++------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/native/swift/Sources/wordpress-api/WordPressAPI.swift b/native/swift/Sources/wordpress-api/WordPressAPI.swift index 76e51301e..747bddd53 100644 --- a/native/swift/Sources/wordpress-api/WordPressAPI.swift +++ b/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) } }