From 7895adef94cc680eae45f8f8c6f23fc895d3475c Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 13 Dec 2022 12:06:58 +0700 Subject: [PATCH 01/30] Inject siteID from `DefaultStoresManager` to `AlamofireNetwork` --- Networking/Networking/Network/AlamofireNetwork.swift | 12 ++++++++++++ .../Classes/Yosemite/AuthenticatedState.swift | 12 +++++++++++- .../Classes/Yosemite/DefaultStoresManager.swift | 4 ++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/Networking/Networking/Network/AlamofireNetwork.swift b/Networking/Networking/Network/AlamofireNetwork.swift index 84a99d128d7..548590ffe69 100644 --- a/Networking/Networking/Network/AlamofireNetwork.swift +++ b/Networking/Networking/Network/AlamofireNetwork.swift @@ -15,6 +15,10 @@ public class AlamofireNetwork: Network { /// private let credentials: Credentials? + /// Currently selected site ID if any. + /// + private var siteID: Int64? + public var session: URLSession { SessionManager.default.session } /// Public Initializer @@ -107,6 +111,14 @@ public class AlamofireNetwork: Network { } } +public extension AlamofireNetwork { + /// Updates the current site ID. + /// + func updateCurrentSite(siteID: Int64) { + self.siteID = siteID + } +} + private extension AlamofireNetwork { func createRequest(wrapping request: URLRequestConvertible) -> URLRequestConvertible { credentials.map { AuthenticatedRequest(credentials: $0, request: request) } ?? diff --git a/WooCommerce/Classes/Yosemite/AuthenticatedState.swift b/WooCommerce/Classes/Yosemite/AuthenticatedState.swift index ec0452429c0..aff7d28b0b2 100644 --- a/WooCommerce/Classes/Yosemite/AuthenticatedState.swift +++ b/WooCommerce/Classes/Yosemite/AuthenticatedState.swift @@ -19,12 +19,16 @@ class AuthenticatedState: StoresManagerState { /// private var errorObserverToken: NSObjectProtocol? + /// The network to handle network requests. + /// + private let network: AlamofireNetwork + /// Designated Initializer /// init(credentials: Credentials) { let storageManager = ServiceLocator.storageManager - let network = AlamofireNetwork(credentials: credentials) + network = AlamofireNetwork(credentials: credentials) services = [ AccountStore(dispatcher: dispatcher, storageManager: storageManager, network: network, dotcomAuthToken: credentials.authToken), @@ -124,6 +128,12 @@ class AuthenticatedState: StoresManagerState { func onAction(_ action: Action) { dispatcher.dispatch(action) } + + /// Updates the network with the currently selected site. + /// + func updateCurrentSite(siteID: Int64) { + network.updateCurrentSite(siteID: siteID) + } } diff --git a/WooCommerce/Classes/Yosemite/DefaultStoresManager.swift b/WooCommerce/Classes/Yosemite/DefaultStoresManager.swift index e93e637a686..36b82e5340e 100644 --- a/WooCommerce/Classes/Yosemite/DefaultStoresManager.swift +++ b/WooCommerce/Classes/Yosemite/DefaultStoresManager.swift @@ -195,6 +195,10 @@ class DefaultStoresManager: StoresManager { restoreSessionSiteIfPossible() ServiceLocator.pushNotesManager.reloadBadgeCount() + if let state = self.state as? AuthenticatedState { + state.updateCurrentSite(siteID: storeID) + } + NotificationCenter.default.post(name: .StoresManagerDidUpdateDefaultSite, object: nil) } From 0099895004d1794f238df30397bbe0b53099f40a Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 13 Dec 2022 14:44:34 +0700 Subject: [PATCH 02/30] Add new request type for REST API requests --- .../Networking.xcodeproj/project.pbxproj | 4 ++ .../Networking/Requests/RESTRequest.swift | 47 +++++++++++++++++++ 2 files changed, 51 insertions(+) create mode 100644 Networking/Networking/Requests/RESTRequest.swift diff --git a/Networking/Networking.xcodeproj/project.pbxproj b/Networking/Networking.xcodeproj/project.pbxproj index c28b3307a40..1b7d1bf73fa 100644 --- a/Networking/Networking.xcodeproj/project.pbxproj +++ b/Networking/Networking.xcodeproj/project.pbxproj @@ -709,6 +709,7 @@ DEC51AF92769A212009F3DF4 /* SystemStatus+Settings.swift in Sources */ = {isa = PBXBuildFile; fileRef = DEC51AF82769A212009F3DF4 /* SystemStatus+Settings.swift */; }; DEC51AFB2769C66B009F3DF4 /* SystemStatusMapperTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = DEC51AFA2769C66B009F3DF4 /* SystemStatusMapperTests.swift */; }; DEC51B02276AFB35009F3DF4 /* SystemStatus+DropinMustUsePlugin.swift in Sources */ = {isa = PBXBuildFile; fileRef = DEC51B01276AFB34009F3DF4 /* SystemStatus+DropinMustUsePlugin.swift */; }; + DEFBA74E29485A7600C35BA9 /* RESTRequest.swift in Sources */ = {isa = PBXBuildFile; fileRef = DEFBA74D29485A7600C35BA9 /* RESTRequest.swift */; }; E12552C526385B05001CEE70 /* ShippingLabelAddressValidationSuccess.swift in Sources */ = {isa = PBXBuildFile; fileRef = E12552C426385B05001CEE70 /* ShippingLabelAddressValidationSuccess.swift */; }; E137619929151C7400FD098F /* error-wp-rest-forbidden.json in Resources */ = {isa = PBXBuildFile; fileRef = E137619829151C7400FD098F /* error-wp-rest-forbidden.json */; }; E137619B2915222100FD098F /* WordPressApiValidatorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E137619A2915222100FD098F /* WordPressApiValidatorTests.swift */; }; @@ -1467,6 +1468,7 @@ DEC51AF82769A212009F3DF4 /* SystemStatus+Settings.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "SystemStatus+Settings.swift"; sourceTree = ""; }; DEC51AFA2769C66B009F3DF4 /* SystemStatusMapperTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SystemStatusMapperTests.swift; sourceTree = ""; }; DEC51B01276AFB34009F3DF4 /* SystemStatus+DropinMustUsePlugin.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "SystemStatus+DropinMustUsePlugin.swift"; sourceTree = ""; }; + DEFBA74D29485A7600C35BA9 /* RESTRequest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RESTRequest.swift; sourceTree = ""; }; E12552C426385B05001CEE70 /* ShippingLabelAddressValidationSuccess.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShippingLabelAddressValidationSuccess.swift; sourceTree = ""; }; E137619829151C7400FD098F /* error-wp-rest-forbidden.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = "error-wp-rest-forbidden.json"; sourceTree = ""; }; E137619A2915222100FD098F /* WordPressApiValidatorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = WordPressApiValidatorTests.swift; sourceTree = ""; }; @@ -1905,6 +1907,7 @@ B557D9FF209754FF005962F4 /* JetpackRequest.swift */, DE34051228BDCA5100CF0D97 /* WordPressOrgRequest.swift */, 029C9E5B291507A40013E5EE /* UnauthenticatedRequest.swift */, + DEFBA74D29485A7600C35BA9 /* RESTRequest.swift */, ); path = Requests; sourceTree = ""; @@ -3044,6 +3047,7 @@ CE132BBC223859710029DB6C /* ProductTag.swift in Sources */, 26650332261FFA1A0079A159 /* ProductAddOnEnvelope.swift in Sources */, D88D5A47230BC838007B6E01 /* ProductReview.swift in Sources */, + DEFBA74E29485A7600C35BA9 /* RESTRequest.swift in Sources */, 456930A9264EB576009ED69D /* ShippingLabelCarriersAndRates.swift in Sources */, 741B950120EBC8A700DD6E2D /* OrderCouponLine.swift in Sources */, 020D07BA23D8542000FD9580 /* UploadableMedia.swift in Sources */, diff --git a/Networking/Networking/Requests/RESTRequest.swift b/Networking/Networking/Requests/RESTRequest.swift new file mode 100644 index 00000000000..37e0ebbc6de --- /dev/null +++ b/Networking/Networking/Requests/RESTRequest.swift @@ -0,0 +1,47 @@ +import Foundation +import Alamofire + +struct RESTRequest: URLRequestConvertible { + /// URL of the site to make the request with + /// + let siteURL: String + + /// HTTP Request Method + /// + let method: HTTPMethod + + /// RPC + /// + let path: String + + /// Parameters + /// + let parameters: [String: Any]? + + /// HTTP Headers + let headers: [String: String] + + /// Designated Initializer. + /// + /// - Parameters: + /// - method: HTTP Method we should use. + /// - path: RPC that should be executed. + /// - parameters: Collection of String parameters to be passed over to our target RPC. + /// + init(siteURL: String, method: HTTPMethod, path: String, parameters: [String: Any]? = nil, headers: [String: String]? = nil) { + self.siteURL = siteURL + self.method = method + self.path = path + self.parameters = parameters ?? [:] + self.headers = headers ?? [:] + } + + /// Returns a URLRequest instance representing the current WordPress.com Request. + /// + func asURLRequest() throws -> URLRequest { + let url = try (siteURL + path).asURL() + let request = try URLRequest(url: url, method: method, headers: headers) + + return try URLEncoding.default.encode(request, with: parameters) + } +} From 3efefba6325154c06e0c20f0f6c3b9fa7908ac3c Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 13 Dec 2022 14:55:01 +0700 Subject: [PATCH 03/30] Configure application password use case in AlamofireNetwork --- .../Networking/Network/AlamofireNetwork.swift | 31 ++++++++++++++++--- .../Classes/Yosemite/AuthenticatedState.swift | 2 +- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/Networking/Networking/Network/AlamofireNetwork.swift b/Networking/Networking/Network/AlamofireNetwork.swift index 548590ffe69..e0f06aa7630 100644 --- a/Networking/Networking/Network/AlamofireNetwork.swift +++ b/Networking/Networking/Network/AlamofireNetwork.swift @@ -2,6 +2,24 @@ import Combine import Foundation import Alamofire +// TODO: Replace with actual implementation. +final class TemporaryApplicationPasswordUseCase: ApplicationPasswordUseCase { + init(siteID: Int64, credentials: Credentials) { + // no-op + } + + var applicationPassword: ApplicationPassword? { + return nil + } + + func generateNewPassword() async throws -> ApplicationPassword { + return .init(wpOrgUsername: "test", password: .init("12345")) + } + + func deletePassword() async throws { + // no-op + } +} extension Alamofire.MultipartFormData: MultipartFormData {} @@ -15,9 +33,9 @@ public class AlamofireNetwork: Network { /// private let credentials: Credentials? - /// Currently selected site ID if any. + /// The use case to handle authentication with application passwords. /// - private var siteID: Int64? + private var applicationPasswordUseCase: ApplicationPasswordUseCase? public var session: URLSession { SessionManager.default.session } @@ -112,10 +130,13 @@ public class AlamofireNetwork: Network { } public extension AlamofireNetwork { - /// Updates the current site ID. + /// Updates the application password use case with a new site ID. /// - func updateCurrentSite(siteID: Int64) { - self.siteID = siteID + func configureApplicationPasswordHandler(with siteID: Int64) { + guard let credentials else { + return + } + self.applicationPasswordUseCase = TemporaryApplicationPasswordUseCase(siteID: siteID, credentials: credentials) } } diff --git a/WooCommerce/Classes/Yosemite/AuthenticatedState.swift b/WooCommerce/Classes/Yosemite/AuthenticatedState.swift index aff7d28b0b2..362671e5e85 100644 --- a/WooCommerce/Classes/Yosemite/AuthenticatedState.swift +++ b/WooCommerce/Classes/Yosemite/AuthenticatedState.swift @@ -132,7 +132,7 @@ class AuthenticatedState: StoresManagerState { /// Updates the network with the currently selected site. /// func updateCurrentSite(siteID: Int64) { - network.updateCurrentSite(siteID: siteID) + network.configureApplicationPasswordHandler(with: siteID) } } From f4e1519b8ed8073d7e7ace3e49979ffdeb0b40d0 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 13 Dec 2022 18:12:33 +0700 Subject: [PATCH 04/30] Add new method to update headers for REST requests --- .../Networking/Requests/RESTRequest.swift | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/Networking/Networking/Requests/RESTRequest.swift b/Networking/Networking/Requests/RESTRequest.swift index 37e0ebbc6de..aa8c41ebb53 100644 --- a/Networking/Networking/Requests/RESTRequest.swift +++ b/Networking/Networking/Requests/RESTRequest.swift @@ -45,3 +45,24 @@ struct RESTRequest: URLRequestConvertible { return try URLEncoding.default.encode(request, with: parameters) } } + +extension RESTRequest { + /// Updates the request headers with authentication information. + /// + func updateRequest(with applicationPassword: ApplicationPassword) throws -> URLRequest { + var request = try asURLRequest() + request.setValue("application/json", forHTTPHeaderField: "Accept") + request.setValue(UserAgent.defaultUserAgent, forHTTPHeaderField: "User-Agent") + + let username = "username" + let password = "password" + let loginString = "\(username):\(password)" + guard let loginData = loginString.data(using: .utf8) else { + return request + } + let base64LoginString = loginData.base64EncodedString() + + request.setValue("Basic \(base64LoginString)", forHTTPHeaderField: "Authorization") + return request + } +} From 823607af8fb1806b5a33d08d84d0d065ea2e0f19 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 13 Dec 2022 18:12:59 +0700 Subject: [PATCH 05/30] Update AlamofireNetwork to handle application password --- .../Networking/Network/AlamofireNetwork.swift | 83 +++++++++++++------ 1 file changed, 57 insertions(+), 26 deletions(-) diff --git a/Networking/Networking/Network/AlamofireNetwork.swift b/Networking/Networking/Network/AlamofireNetwork.swift index e0f06aa7630..4ae306943de 100644 --- a/Networking/Networking/Network/AlamofireNetwork.swift +++ b/Networking/Networking/Network/AlamofireNetwork.swift @@ -67,12 +67,17 @@ public class AlamofireNetwork: Network { /// - Yes. We do the above because the Jetpack Tunnel endpoint doesn't properly relay the correct statusCode. /// public func responseData(for request: URLRequestConvertible, completion: @escaping (Data?, Error?) -> Void) { - let request = createRequest(wrapping: request) - - Alamofire.request(request) - .responseData { response in - completion(response.value, response.networkingError) + Task(priority: .medium) { + do { + let request = try await createRequest(wrapping: request) + } catch { + completion(nil, error) } + Alamofire.request(request) + .responseData { response in + completion(response.value, response.networkingError) + } + } } /// Executes the specified Network Request. Upon completion, the payload will be sent back to the caller as a Data instance. @@ -85,10 +90,15 @@ public class AlamofireNetwork: Network { /// - completion: Closure to be executed upon completion. /// public func responseData(for request: URLRequestConvertible, completion: @escaping (Swift.Result) -> Void) { - let request = createRequest(wrapping: request) - - Alamofire.request(request).responseData { response in - completion(response.result.toSwiftResult()) + Task(priority: .medium) { + do { + let request = try await createRequest(wrapping: request) + } catch { + completion(.failure(error)) + } + Alamofire.request(request).responseData { response in + completion(response.result.toSwiftResult()) + } } } @@ -101,12 +111,17 @@ public class AlamofireNetwork: Network { /// - Parameter request: Request that should be performed. /// - Returns: A publisher that emits the result of the given request. public func responseDataPublisher(for request: URLRequestConvertible) -> AnyPublisher, Never> { - let request = createRequest(wrapping: request) - return Future() { promise in - Alamofire.request(request).responseData { response in - let result = response.result.toSwiftResult() - promise(Swift.Result.success(result)) + Task(priority: .medium) { + do { + let request = try await self.createRequest(wrapping: request) + } catch { + promise(Swift.Result.success(.failure(error))) + } + Alamofire.request(request).responseData { response in + let result = response.result.toSwiftResult() + promise(Swift.Result.success(result)) + } } }.eraseToAnyPublisher() } @@ -114,17 +129,22 @@ public class AlamofireNetwork: Network { public func uploadMultipartFormData(multipartFormData: @escaping (MultipartFormData) -> Void, to request: URLRequestConvertible, completion: @escaping (Data?, Error?) -> Void) { - let request = createRequest(wrapping: request) - - backgroundSessionManager.upload(multipartFormData: multipartFormData, with: request) { (encodingResult) in - switch encodingResult { - case .success(let upload, _, _): - upload.responseData { response in - completion(response.value, response.error) - } - case .failure(let error): + Task(priority: .medium) { + do { + let request = try await createRequest(wrapping: request) + } catch { completion(nil, error) } + backgroundSessionManager.upload(multipartFormData: multipartFormData, with: request) { (encodingResult) in + switch encodingResult { + case .success(let upload, _, _): + upload.responseData { response in + completion(response.value, response.error) + } + case .failure(let error): + completion(nil, error) + } + } } } } @@ -141,9 +161,20 @@ public extension AlamofireNetwork { } private extension AlamofireNetwork { - func createRequest(wrapping request: URLRequestConvertible) -> URLRequestConvertible { - credentials.map { AuthenticatedRequest(credentials: $0, request: request) } ?? - UnauthenticatedRequest(request: request) + func createRequest(wrapping request: URLRequestConvertible) async throws -> URLRequestConvertible { + guard let restRequest = request as? RESTRequest, + let useCase = applicationPasswordUseCase else { + return credentials.map { AuthenticatedRequest(credentials: $0, request: request) } ?? + UnauthenticatedRequest(request: request) + } + + let applicationPassword: ApplicationPassword = try await { + if let password = useCase.applicationPassword { + return password + } + return try await useCase.generateNewPassword() + }() + return try restRequest.updateRequest(with: applicationPassword) } } From 583d7569eba786ecd6f4b7094cf00353f48857fe Mon Sep 17 00:00:00 2001 From: Huong Do Date: Wed, 14 Dec 2022 12:02:09 +0700 Subject: [PATCH 06/30] Use Task only for fetching application password --- .../Networking/Network/AlamofireNetwork.swift | 64 +++++++++---------- 1 file changed, 29 insertions(+), 35 deletions(-) diff --git a/Networking/Networking/Network/AlamofireNetwork.swift b/Networking/Networking/Network/AlamofireNetwork.swift index 4ae306943de..ff6a9cadf5e 100644 --- a/Networking/Networking/Network/AlamofireNetwork.swift +++ b/Networking/Networking/Network/AlamofireNetwork.swift @@ -67,12 +67,7 @@ public class AlamofireNetwork: Network { /// - Yes. We do the above because the Jetpack Tunnel endpoint doesn't properly relay the correct statusCode. /// public func responseData(for request: URLRequestConvertible, completion: @escaping (Data?, Error?) -> Void) { - Task(priority: .medium) { - do { - let request = try await createRequest(wrapping: request) - } catch { - completion(nil, error) - } + createRequest(wrapping: request) { request in Alamofire.request(request) .responseData { response in completion(response.value, response.networkingError) @@ -90,12 +85,7 @@ public class AlamofireNetwork: Network { /// - completion: Closure to be executed upon completion. /// public func responseData(for request: URLRequestConvertible, completion: @escaping (Swift.Result) -> Void) { - Task(priority: .medium) { - do { - let request = try await createRequest(wrapping: request) - } catch { - completion(.failure(error)) - } + createRequest(wrapping: request) { request in Alamofire.request(request).responseData { response in completion(response.result.toSwiftResult()) } @@ -112,12 +102,7 @@ public class AlamofireNetwork: Network { /// - Returns: A publisher that emits the result of the given request. public func responseDataPublisher(for request: URLRequestConvertible) -> AnyPublisher, Never> { return Future() { promise in - Task(priority: .medium) { - do { - let request = try await self.createRequest(wrapping: request) - } catch { - promise(Swift.Result.success(.failure(error))) - } + self.createRequest(wrapping: request) { request in Alamofire.request(request).responseData { response in let result = response.result.toSwiftResult() promise(Swift.Result.success(result)) @@ -129,13 +114,9 @@ public class AlamofireNetwork: Network { public func uploadMultipartFormData(multipartFormData: @escaping (MultipartFormData) -> Void, to request: URLRequestConvertible, completion: @escaping (Data?, Error?) -> Void) { - Task(priority: .medium) { - do { - let request = try await createRequest(wrapping: request) - } catch { - completion(nil, error) - } - backgroundSessionManager.upload(multipartFormData: multipartFormData, with: request) { (encodingResult) in + createRequest(wrapping: request) { [weak self] request in + guard let self else { return } + self.backgroundSessionManager.upload(multipartFormData: multipartFormData, with: request) { (encodingResult) in switch encodingResult { case .success(let upload, _, _): upload.responseData { response in @@ -161,20 +142,33 @@ public extension AlamofireNetwork { } private extension AlamofireNetwork { - func createRequest(wrapping request: URLRequestConvertible) async throws -> URLRequestConvertible { + /// Wraps a request with application password or WPCOM token if possible. + /// + func createRequest(wrapping request: URLRequestConvertible, completion: @escaping (URLRequestConvertible) -> Void) { guard let restRequest = request as? RESTRequest, let useCase = applicationPasswordUseCase else { - return credentials.map { AuthenticatedRequest(credentials: $0, request: request) } ?? - UnauthenticatedRequest(request: request) + return completion(createAuthenticatedRequestIfPossible(for: request)) } - - let applicationPassword: ApplicationPassword = try await { - if let password = useCase.applicationPassword { - return password + Task(priority: .medium) { + do { + let applicationPassword: ApplicationPassword = try await { + if let password = useCase.applicationPassword { + return password + } + return try await useCase.generateNewPassword() + }() + completion(try restRequest.updateRequest(with: applicationPassword)) + } catch { + completion(createAuthenticatedRequestIfPossible(for: request)) } - return try await useCase.generateNewPassword() - }() - return try restRequest.updateRequest(with: applicationPassword) + } + } + + /// Attempts to create a request with WPCOM token if possible. + /// + func createAuthenticatedRequestIfPossible(for request: URLRequestConvertible) -> URLRequestConvertible { + credentials.map { AuthenticatedRequest(credentials: $0, request: request) } ?? + UnauthenticatedRequest(request: request) } } From 763ba7f5e2d0d33897e9278d0812ab972835806d Mon Sep 17 00:00:00 2001 From: Huong Do Date: Wed, 14 Dec 2022 12:41:25 +0700 Subject: [PATCH 07/30] Add a fallback Jetpack request to REST request to trigger when application password is blocked --- Networking/Networking/Network/AlamofireNetwork.swift | 4 +++- Networking/Networking/Requests/RESTRequest.swift | 6 +++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/Networking/Networking/Network/AlamofireNetwork.swift b/Networking/Networking/Network/AlamofireNetwork.swift index ff6a9cadf5e..4066f99afee 100644 --- a/Networking/Networking/Network/AlamofireNetwork.swift +++ b/Networking/Networking/Network/AlamofireNetwork.swift @@ -159,7 +159,9 @@ private extension AlamofireNetwork { }() completion(try restRequest.updateRequest(with: applicationPassword)) } catch { - completion(createAuthenticatedRequestIfPossible(for: request)) + // Get the fallback Jetpack request to handle if possible. + let fallbackRequest = restRequest.fallbackRequest ?? request + completion(createAuthenticatedRequestIfPossible(for: fallbackRequest)) } } } diff --git a/Networking/Networking/Requests/RESTRequest.swift b/Networking/Networking/Requests/RESTRequest.swift index aa8c41ebb53..4055d83f2ce 100644 --- a/Networking/Networking/Requests/RESTRequest.swift +++ b/Networking/Networking/Requests/RESTRequest.swift @@ -21,6 +21,9 @@ struct RESTRequest: URLRequestConvertible { /// HTTP Headers let headers: [String: String] + /// A fallback JetpackRequest if the REST request cannot be made with an application password. + let fallbackRequest: JetpackRequest? + /// Designated Initializer. /// /// - Parameters: @@ -28,12 +31,13 @@ struct RESTRequest: URLRequestConvertible { /// - path: RPC that should be executed. /// - parameters: Collection of String parameters to be passed over to our target RPC. /// - init(siteURL: String, method: HTTPMethod, path: String, parameters: [String: Any]? = nil, headers: [String: String]? = nil) { + init(siteURL: String, method: HTTPMethod, path: String, parameters: [String: Any]? = nil, headers: [String: String]? = nil, fallbackRequest: JetpackRequest?) { self.siteURL = siteURL self.method = method self.path = path self.parameters = parameters ?? [:] self.headers = headers ?? [:] + self.fallbackRequest = fallbackRequest } /// Returns a URLRequest instance representing the current WordPress.com Request. From 168a6ac20853961e9b943f1dac43fe19546733bd Mon Sep 17 00:00:00 2001 From: Huong Do Date: Wed, 14 Dec 2022 12:42:21 +0700 Subject: [PATCH 08/30] Fix line limit violation --- Networking/Networking/Requests/RESTRequest.swift | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Networking/Networking/Requests/RESTRequest.swift b/Networking/Networking/Requests/RESTRequest.swift index 4055d83f2ce..3f915322294 100644 --- a/Networking/Networking/Requests/RESTRequest.swift +++ b/Networking/Networking/Requests/RESTRequest.swift @@ -31,7 +31,12 @@ struct RESTRequest: URLRequestConvertible { /// - path: RPC that should be executed. /// - parameters: Collection of String parameters to be passed over to our target RPC. /// - init(siteURL: String, method: HTTPMethod, path: String, parameters: [String: Any]? = nil, headers: [String: String]? = nil, fallbackRequest: JetpackRequest?) { + init(siteURL: String, + method: HTTPMethod, + path: String, + parameters: [String: Any]? = nil, + headers: [String: String]? = nil, + fallbackRequest: JetpackRequest?) { self.siteURL = siteURL self.method = method self.path = path From 9b0d019ba79ba6707460267711d00eeb3fe5a514 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Wed, 14 Dec 2022 12:46:07 +0700 Subject: [PATCH 09/30] Update comment for RESTRequest initializer --- Networking/Networking/Requests/RESTRequest.swift | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Networking/Networking/Requests/RESTRequest.swift b/Networking/Networking/Requests/RESTRequest.swift index 3f915322294..bcfb4158bd8 100644 --- a/Networking/Networking/Requests/RESTRequest.swift +++ b/Networking/Networking/Requests/RESTRequest.swift @@ -30,18 +30,20 @@ struct RESTRequest: URLRequestConvertible { /// - method: HTTP Method we should use. /// - path: RPC that should be executed. /// - parameters: Collection of String parameters to be passed over to our target RPC. + /// - headers: Headers to be added to the request. + /// - fallbackRequest: A fallback Jetpack request to trigger if the REST request cannot be made. /// init(siteURL: String, method: HTTPMethod, path: String, - parameters: [String: Any]? = nil, - headers: [String: String]? = nil, + parameters: [String: Any] = [:], + headers: [String: String] = [:], fallbackRequest: JetpackRequest?) { self.siteURL = siteURL self.method = method self.path = path - self.parameters = parameters ?? [:] - self.headers = headers ?? [:] + self.parameters = parameters + self.headers = headers self.fallbackRequest = fallbackRequest } From 17cae6aae291e7d69167581e77243a4a202774a1 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Wed, 14 Dec 2022 13:19:02 +0700 Subject: [PATCH 10/30] Inject the complete use case to AlamofireNetwork from AuthenticatedState --- .../ApplicationPasswordUseCase.swift | 9 +++++-- .../Networking/Network/AlamofireNetwork.swift | 26 ++----------------- .../Classes/Yosemite/AuthenticatedState.swift | 25 +++++++++++++++++- 3 files changed, 33 insertions(+), 27 deletions(-) diff --git a/Networking/Networking/ApplicationPassword/ApplicationPasswordUseCase.swift b/Networking/Networking/ApplicationPassword/ApplicationPasswordUseCase.swift index 908146c5ed2..a386bc61201 100644 --- a/Networking/Networking/ApplicationPassword/ApplicationPasswordUseCase.swift +++ b/Networking/Networking/ApplicationPassword/ApplicationPasswordUseCase.swift @@ -1,7 +1,7 @@ import Foundation import WordPressShared -struct ApplicationPassword { +public struct ApplicationPassword { /// WordPress org username that the application password belongs to /// let wpOrgUsername: String @@ -9,9 +9,14 @@ struct ApplicationPassword { /// Application password /// let password: Secret + + public init(wpOrgUsername: String, password: Secret) { + self.wpOrgUsername = wpOrgUsername + self.password = password + } } -protocol ApplicationPasswordUseCase { +public protocol ApplicationPasswordUseCase { /// Returns the locally saved ApplicationPassword if available /// var applicationPassword: ApplicationPassword? { get } diff --git a/Networking/Networking/Network/AlamofireNetwork.swift b/Networking/Networking/Network/AlamofireNetwork.swift index 4066f99afee..c21da7a7977 100644 --- a/Networking/Networking/Network/AlamofireNetwork.swift +++ b/Networking/Networking/Network/AlamofireNetwork.swift @@ -2,25 +2,6 @@ import Combine import Foundation import Alamofire -// TODO: Replace with actual implementation. -final class TemporaryApplicationPasswordUseCase: ApplicationPasswordUseCase { - init(siteID: Int64, credentials: Credentials) { - // no-op - } - - var applicationPassword: ApplicationPassword? { - return nil - } - - func generateNewPassword() async throws -> ApplicationPassword { - return .init(wpOrgUsername: "test", password: .init("12345")) - } - - func deletePassword() async throws { - // no-op - } -} - extension Alamofire.MultipartFormData: MultipartFormData {} /// AlamofireWrapper: Encapsulates all of the Alamofire OP's @@ -133,11 +114,8 @@ public class AlamofireNetwork: Network { public extension AlamofireNetwork { /// Updates the application password use case with a new site ID. /// - func configureApplicationPasswordHandler(with siteID: Int64) { - guard let credentials else { - return - } - self.applicationPasswordUseCase = TemporaryApplicationPasswordUseCase(siteID: siteID, credentials: credentials) + func configureApplicationPasswordHandler(with applicationPasswordUseCase: ApplicationPasswordUseCase) { + self.applicationPasswordUseCase = applicationPasswordUseCase } } diff --git a/WooCommerce/Classes/Yosemite/AuthenticatedState.swift b/WooCommerce/Classes/Yosemite/AuthenticatedState.swift index 362671e5e85..e239f2094b4 100644 --- a/WooCommerce/Classes/Yosemite/AuthenticatedState.swift +++ b/WooCommerce/Classes/Yosemite/AuthenticatedState.swift @@ -23,10 +23,14 @@ class AuthenticatedState: StoresManagerState { /// private let network: AlamofireNetwork + /// WordPress.com Credentials. + /// + private let credentials: Credentials /// Designated Initializer /// init(credentials: Credentials) { + self.credentials = credentials let storageManager = ServiceLocator.storageManager network = AlamofireNetwork(credentials: credentials) @@ -132,10 +136,29 @@ class AuthenticatedState: StoresManagerState { /// Updates the network with the currently selected site. /// func updateCurrentSite(siteID: Int64) { - network.configureApplicationPasswordHandler(with: siteID) + let useCase = TemporaryApplicationPasswordUseCase(siteID: siteID, credentials: credentials) + network.configureApplicationPasswordHandler(with: useCase) } } +// TODO: Replace with actual implementation. +final class TemporaryApplicationPasswordUseCase: ApplicationPasswordUseCase { + init(siteID: Int64, credentials: Credentials) { + // no-op + } + + var applicationPassword: ApplicationPassword? { + return nil + } + + func generateNewPassword() async throws -> ApplicationPassword { + return .init(wpOrgUsername: "test", password: .init("12345")) + } + + func deletePassword() async throws { + // no-op + } +} // MARK: - Private Methods // From 5e0791159329841647a01d0443bcb33c79bf747c Mon Sep 17 00:00:00 2001 From: Huong Do Date: Wed, 14 Dec 2022 16:13:32 +0700 Subject: [PATCH 11/30] Update comments for AlamofireNetwork --- Networking/Networking/Network/AlamofireNetwork.swift | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Networking/Networking/Network/AlamofireNetwork.swift b/Networking/Networking/Network/AlamofireNetwork.swift index c21da7a7977..861043da9cb 100644 --- a/Networking/Networking/Network/AlamofireNetwork.swift +++ b/Networking/Networking/Network/AlamofireNetwork.swift @@ -120,11 +120,12 @@ public extension AlamofireNetwork { } private extension AlamofireNetwork { - /// Wraps a request with application password or WPCOM token if possible. + /// Updates a request with application password or WPCOM token if possible. /// func createRequest(wrapping request: URLRequestConvertible, completion: @escaping (URLRequestConvertible) -> Void) { guard let restRequest = request as? RESTRequest, let useCase = applicationPasswordUseCase else { + // Handle non-REST requests as before return completion(createAuthenticatedRequestIfPossible(for: request)) } Task(priority: .medium) { @@ -137,6 +138,8 @@ private extension AlamofireNetwork { }() completion(try restRequest.updateRequest(with: applicationPassword)) } catch { + DDLogWarn("⚠️ Error generating application password and update request: \(error)") + // TODO: add Tracks // Get the fallback Jetpack request to handle if possible. let fallbackRequest = restRequest.fallbackRequest ?? request completion(createAuthenticatedRequestIfPossible(for: fallbackRequest)) @@ -144,7 +147,7 @@ private extension AlamofireNetwork { } } - /// Attempts to create a request with WPCOM token if possible. + /// Attempts creating a request with WPCOM token if possible. /// func createAuthenticatedRequestIfPossible(for request: URLRequestConvertible) -> URLRequestConvertible { credentials.map { AuthenticatedRequest(credentials: $0, request: request) } ?? From 85c31f8cab7702728cfd985fe0ea17fbe1934d82 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Wed, 14 Dec 2022 16:19:27 +0700 Subject: [PATCH 12/30] Revert "Inject the complete use case to AlamofireNetwork from AuthenticatedState" This reverts commit 17cae6aae291e7d69167581e77243a4a202774a1. --- .../ApplicationPasswordUseCase.swift | 9 ++----- .../Networking/Network/AlamofireNetwork.swift | 26 +++++++++++++++++-- .../Classes/Yosemite/AuthenticatedState.swift | 25 +----------------- 3 files changed, 27 insertions(+), 33 deletions(-) diff --git a/Networking/Networking/ApplicationPassword/ApplicationPasswordUseCase.swift b/Networking/Networking/ApplicationPassword/ApplicationPasswordUseCase.swift index a386bc61201..908146c5ed2 100644 --- a/Networking/Networking/ApplicationPassword/ApplicationPasswordUseCase.swift +++ b/Networking/Networking/ApplicationPassword/ApplicationPasswordUseCase.swift @@ -1,7 +1,7 @@ import Foundation import WordPressShared -public struct ApplicationPassword { +struct ApplicationPassword { /// WordPress org username that the application password belongs to /// let wpOrgUsername: String @@ -9,14 +9,9 @@ public struct ApplicationPassword { /// Application password /// let password: Secret - - public init(wpOrgUsername: String, password: Secret) { - self.wpOrgUsername = wpOrgUsername - self.password = password - } } -public protocol ApplicationPasswordUseCase { +protocol ApplicationPasswordUseCase { /// Returns the locally saved ApplicationPassword if available /// var applicationPassword: ApplicationPassword? { get } diff --git a/Networking/Networking/Network/AlamofireNetwork.swift b/Networking/Networking/Network/AlamofireNetwork.swift index 861043da9cb..1c00f515606 100644 --- a/Networking/Networking/Network/AlamofireNetwork.swift +++ b/Networking/Networking/Network/AlamofireNetwork.swift @@ -2,6 +2,25 @@ import Combine import Foundation import Alamofire +// TODO: Replace with actual implementation. +final class TemporaryApplicationPasswordUseCase: ApplicationPasswordUseCase { + init(siteID: Int64, credentials: Credentials) { + // no-op + } + + var applicationPassword: ApplicationPassword? { + return nil + } + + func generateNewPassword() async throws -> ApplicationPassword { + return .init(wpOrgUsername: "test", password: .init("12345")) + } + + func deletePassword() async throws { + // no-op + } +} + extension Alamofire.MultipartFormData: MultipartFormData {} /// AlamofireWrapper: Encapsulates all of the Alamofire OP's @@ -114,8 +133,11 @@ public class AlamofireNetwork: Network { public extension AlamofireNetwork { /// Updates the application password use case with a new site ID. /// - func configureApplicationPasswordHandler(with applicationPasswordUseCase: ApplicationPasswordUseCase) { - self.applicationPasswordUseCase = applicationPasswordUseCase + func configureApplicationPasswordHandler(with siteID: Int64) { + guard let credentials else { + return + } + self.applicationPasswordUseCase = TemporaryApplicationPasswordUseCase(siteID: siteID, credentials: credentials) } } diff --git a/WooCommerce/Classes/Yosemite/AuthenticatedState.swift b/WooCommerce/Classes/Yosemite/AuthenticatedState.swift index e239f2094b4..362671e5e85 100644 --- a/WooCommerce/Classes/Yosemite/AuthenticatedState.swift +++ b/WooCommerce/Classes/Yosemite/AuthenticatedState.swift @@ -23,14 +23,10 @@ class AuthenticatedState: StoresManagerState { /// private let network: AlamofireNetwork - /// WordPress.com Credentials. - /// - private let credentials: Credentials /// Designated Initializer /// init(credentials: Credentials) { - self.credentials = credentials let storageManager = ServiceLocator.storageManager network = AlamofireNetwork(credentials: credentials) @@ -136,29 +132,10 @@ class AuthenticatedState: StoresManagerState { /// Updates the network with the currently selected site. /// func updateCurrentSite(siteID: Int64) { - let useCase = TemporaryApplicationPasswordUseCase(siteID: siteID, credentials: credentials) - network.configureApplicationPasswordHandler(with: useCase) + network.configureApplicationPasswordHandler(with: siteID) } } -// TODO: Replace with actual implementation. -final class TemporaryApplicationPasswordUseCase: ApplicationPasswordUseCase { - init(siteID: Int64, credentials: Credentials) { - // no-op - } - - var applicationPassword: ApplicationPassword? { - return nil - } - - func generateNewPassword() async throws -> ApplicationPassword { - return .init(wpOrgUsername: "test", password: .init("12345")) - } - - func deletePassword() async throws { - // no-op - } -} // MARK: - Private Methods // From 9740cbb09377e0d81274fdbf409d61c6b6dbf4f7 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Wed, 14 Dec 2022 16:39:20 +0700 Subject: [PATCH 13/30] Separate authentication logic to a new class --- .../Networking.xcodeproj/project.pbxproj | 4 + .../Networking/Network/AlamofireNetwork.swift | 78 ++--------------- .../Network/RequestAuthenticator.swift | 85 +++++++++++++++++++ 3 files changed, 97 insertions(+), 70 deletions(-) create mode 100644 Networking/Networking/Network/RequestAuthenticator.swift diff --git a/Networking/Networking.xcodeproj/project.pbxproj b/Networking/Networking.xcodeproj/project.pbxproj index 1b7d1bf73fa..9a4f6415085 100644 --- a/Networking/Networking.xcodeproj/project.pbxproj +++ b/Networking/Networking.xcodeproj/project.pbxproj @@ -710,6 +710,7 @@ DEC51AFB2769C66B009F3DF4 /* SystemStatusMapperTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = DEC51AFA2769C66B009F3DF4 /* SystemStatusMapperTests.swift */; }; DEC51B02276AFB35009F3DF4 /* SystemStatus+DropinMustUsePlugin.swift in Sources */ = {isa = PBXBuildFile; fileRef = DEC51B01276AFB34009F3DF4 /* SystemStatus+DropinMustUsePlugin.swift */; }; DEFBA74E29485A7600C35BA9 /* RESTRequest.swift in Sources */ = {isa = PBXBuildFile; fileRef = DEFBA74D29485A7600C35BA9 /* RESTRequest.swift */; }; + DEFBA7542949CE6600C35BA9 /* RequestAuthenticator.swift in Sources */ = {isa = PBXBuildFile; fileRef = DEFBA7532949CE6600C35BA9 /* RequestAuthenticator.swift */; }; E12552C526385B05001CEE70 /* ShippingLabelAddressValidationSuccess.swift in Sources */ = {isa = PBXBuildFile; fileRef = E12552C426385B05001CEE70 /* ShippingLabelAddressValidationSuccess.swift */; }; E137619929151C7400FD098F /* error-wp-rest-forbidden.json in Resources */ = {isa = PBXBuildFile; fileRef = E137619829151C7400FD098F /* error-wp-rest-forbidden.json */; }; E137619B2915222100FD098F /* WordPressApiValidatorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E137619A2915222100FD098F /* WordPressApiValidatorTests.swift */; }; @@ -1469,6 +1470,7 @@ DEC51AFA2769C66B009F3DF4 /* SystemStatusMapperTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SystemStatusMapperTests.swift; sourceTree = ""; }; DEC51B01276AFB34009F3DF4 /* SystemStatus+DropinMustUsePlugin.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "SystemStatus+DropinMustUsePlugin.swift"; sourceTree = ""; }; DEFBA74D29485A7600C35BA9 /* RESTRequest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RESTRequest.swift; sourceTree = ""; }; + DEFBA7532949CE6600C35BA9 /* RequestAuthenticator.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RequestAuthenticator.swift; sourceTree = ""; }; E12552C426385B05001CEE70 /* ShippingLabelAddressValidationSuccess.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShippingLabelAddressValidationSuccess.swift; sourceTree = ""; }; E137619829151C7400FD098F /* error-wp-rest-forbidden.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = "error-wp-rest-forbidden.json"; sourceTree = ""; }; E137619A2915222100FD098F /* WordPressApiValidatorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = WordPressApiValidatorTests.swift; sourceTree = ""; }; @@ -1697,6 +1699,7 @@ B518662320A099BF00037A38 /* AlamofireNetwork.swift */, B518662620A09BCC00037A38 /* MockNetwork.swift */, D87F6150226591E10031A13B /* NullNetwork.swift */, + DEFBA7532949CE6600C35BA9 /* RequestAuthenticator.swift */, ); path = Network; sourceTree = ""; @@ -3061,6 +3064,7 @@ 020D07B823D852BB00FD9580 /* Media.swift in Sources */, B5BB1D0C20A2050300112D92 /* DateFormatter+Woo.swift in Sources */, 743E84EE2217244C00FAC9D7 /* ShipmentTrackingListMapper.swift in Sources */, + DEFBA7542949CE6600C35BA9 /* RequestAuthenticator.swift in Sources */, 451A97E5260B631E0059D135 /* ShippingLabelPredefinedPackage.swift in Sources */, BAB373722795A1FB00837B4A /* OrderTaxLine.swift in Sources */, EE54C8942947229800A9BF61 /* ApplicationPasswordUseCase.swift in Sources */, diff --git a/Networking/Networking/Network/AlamofireNetwork.swift b/Networking/Networking/Network/AlamofireNetwork.swift index 1c00f515606..b23bfd194ff 100644 --- a/Networking/Networking/Network/AlamofireNetwork.swift +++ b/Networking/Networking/Network/AlamofireNetwork.swift @@ -2,25 +2,6 @@ import Combine import Foundation import Alamofire -// TODO: Replace with actual implementation. -final class TemporaryApplicationPasswordUseCase: ApplicationPasswordUseCase { - init(siteID: Int64, credentials: Credentials) { - // no-op - } - - var applicationPassword: ApplicationPassword? { - return nil - } - - func generateNewPassword() async throws -> ApplicationPassword { - return .init(wpOrgUsername: "test", password: .init("12345")) - } - - func deletePassword() async throws { - // no-op - } -} - extension Alamofire.MultipartFormData: MultipartFormData {} /// AlamofireWrapper: Encapsulates all of the Alamofire OP's @@ -29,20 +10,16 @@ public class AlamofireNetwork: Network { private let backgroundSessionManager: Alamofire.SessionManager - /// WordPress.com Credentials. + /// Authenticator to update requests authorization header if possible. /// - private let credentials: Credentials? - - /// The use case to handle authentication with application passwords. - /// - private var applicationPasswordUseCase: ApplicationPasswordUseCase? + private let requestAuthenticator: RequestAuthenticator public var session: URLSession { SessionManager.default.session } /// Public Initializer /// public required init(credentials: Credentials?) { - self.credentials = credentials + self.requestAuthenticator = RequestAuthenticator(credentials: credentials) // A unique ID is included in the background session identifier so that the session does not get invalidated when the initializer is called multiple // times (e.g. when logging in). @@ -67,7 +44,7 @@ public class AlamofireNetwork: Network { /// - Yes. We do the above because the Jetpack Tunnel endpoint doesn't properly relay the correct statusCode. /// public func responseData(for request: URLRequestConvertible, completion: @escaping (Data?, Error?) -> Void) { - createRequest(wrapping: request) { request in + requestAuthenticator.authenticateRequest(request) { request in Alamofire.request(request) .responseData { response in completion(response.value, response.networkingError) @@ -85,7 +62,7 @@ public class AlamofireNetwork: Network { /// - completion: Closure to be executed upon completion. /// public func responseData(for request: URLRequestConvertible, completion: @escaping (Swift.Result) -> Void) { - createRequest(wrapping: request) { request in + requestAuthenticator.authenticateRequest(request) { request in Alamofire.request(request).responseData { response in completion(response.result.toSwiftResult()) } @@ -102,7 +79,7 @@ public class AlamofireNetwork: Network { /// - Returns: A publisher that emits the result of the given request. public func responseDataPublisher(for request: URLRequestConvertible) -> AnyPublisher, Never> { return Future() { promise in - self.createRequest(wrapping: request) { request in + self.requestAuthenticator.authenticateRequest(request) { request in Alamofire.request(request).responseData { response in let result = response.result.toSwiftResult() promise(Swift.Result.success(result)) @@ -114,7 +91,7 @@ public class AlamofireNetwork: Network { public func uploadMultipartFormData(multipartFormData: @escaping (MultipartFormData) -> Void, to request: URLRequestConvertible, completion: @escaping (Data?, Error?) -> Void) { - createRequest(wrapping: request) { [weak self] request in + requestAuthenticator.authenticateRequest(request) { [weak self] request in guard let self else { return } self.backgroundSessionManager.upload(multipartFormData: multipartFormData, with: request) { (encodingResult) in switch encodingResult { @@ -134,46 +111,7 @@ public extension AlamofireNetwork { /// Updates the application password use case with a new site ID. /// func configureApplicationPasswordHandler(with siteID: Int64) { - guard let credentials else { - return - } - self.applicationPasswordUseCase = TemporaryApplicationPasswordUseCase(siteID: siteID, credentials: credentials) - } -} - -private extension AlamofireNetwork { - /// Updates a request with application password or WPCOM token if possible. - /// - func createRequest(wrapping request: URLRequestConvertible, completion: @escaping (URLRequestConvertible) -> Void) { - guard let restRequest = request as? RESTRequest, - let useCase = applicationPasswordUseCase else { - // Handle non-REST requests as before - return completion(createAuthenticatedRequestIfPossible(for: request)) - } - Task(priority: .medium) { - do { - let applicationPassword: ApplicationPassword = try await { - if let password = useCase.applicationPassword { - return password - } - return try await useCase.generateNewPassword() - }() - completion(try restRequest.updateRequest(with: applicationPassword)) - } catch { - DDLogWarn("⚠️ Error generating application password and update request: \(error)") - // TODO: add Tracks - // Get the fallback Jetpack request to handle if possible. - let fallbackRequest = restRequest.fallbackRequest ?? request - completion(createAuthenticatedRequestIfPossible(for: fallbackRequest)) - } - } - } - - /// Attempts creating a request with WPCOM token if possible. - /// - func createAuthenticatedRequestIfPossible(for request: URLRequestConvertible) -> URLRequestConvertible { - credentials.map { AuthenticatedRequest(credentials: $0, request: request) } ?? - UnauthenticatedRequest(request: request) + requestAuthenticator.updateApplicationPasswordHandler(with: siteID) } } diff --git a/Networking/Networking/Network/RequestAuthenticator.swift b/Networking/Networking/Network/RequestAuthenticator.swift new file mode 100644 index 00000000000..71877a11a26 --- /dev/null +++ b/Networking/Networking/Network/RequestAuthenticator.swift @@ -0,0 +1,85 @@ +import Alamofire +import Foundation + +// TODO: Replace with actual implementation. +final class TemporaryApplicationPasswordUseCase: ApplicationPasswordUseCase { + init(siteID: Int64, credentials: Credentials) { + // no-op + } + + var applicationPassword: ApplicationPassword? { + return nil + } + + func generateNewPassword() async throws -> ApplicationPassword { + return .init(wpOrgUsername: "test", password: .init("12345")) + } + + func deletePassword() async throws { + // no-op + } +} + +/// Helper class to update requests with authorization header if possible. +/// +final class RequestAuthenticator { + /// WordPress.com Credentials. + /// + private let credentials: Credentials? + + /// The use case to handle authentication with application passwords. + /// + private var applicationPasswordUseCase: ApplicationPasswordUseCase? + + init(credentials: Credentials?) { + self.credentials = credentials + } + + /// Updates the application password use case with a new site ID. + /// + func updateApplicationPasswordHandler(with siteID: Int64) { + guard let credentials else { + return + } + self.applicationPasswordUseCase = TemporaryApplicationPasswordUseCase(siteID: siteID, credentials: credentials) + } + + /// Updates a request with application password or WPCOM token if possible. + /// + func authenticateRequest(_ request: URLRequestConvertible, completion: @escaping (URLRequestConvertible) -> Void) { + guard let restRequest = request as? RESTRequest, + let useCase = applicationPasswordUseCase else { + // Handle non-REST requests as before + return completion(createAuthenticatedRequestIfPossible(for: request)) + } + Task(priority: .medium) { + do { + let applicationPassword: ApplicationPassword = try await { + if let password = useCase.applicationPassword { + return password + } + return try await useCase.generateNewPassword() + }() + let updatedRequest = try restRequest.updateRequest(with: applicationPassword) + await MainActor.run { + completion(updatedRequest) + } + } catch { + DDLogWarn("⚠️ Error generating application password and update request: \(error)") + // TODO: add Tracks + // Get the fallback Jetpack request to handle if possible. + let fallbackRequest = restRequest.fallbackRequest ?? request + await MainActor.run { + completion(createAuthenticatedRequestIfPossible(for: fallbackRequest)) + } + } + } + } + + /// Attempts creating a request with WPCOM token if possible. + /// + private func createAuthenticatedRequestIfPossible(for request: URLRequestConvertible) -> URLRequestConvertible { + credentials.map { AuthenticatedRequest(credentials: $0, request: request) } ?? + UnauthenticatedRequest(request: request) + } +} From cc87a50a8c6a9225c0ed5b3e3f1b98f2fd4241d2 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Wed, 14 Dec 2022 16:45:00 +0700 Subject: [PATCH 14/30] Make ApplicationPasswordUseCase injectable --- Networking/Networking/Network/AlamofireNetwork.swift | 10 +++++++++- .../Networking/Network/RequestAuthenticator.swift | 7 ++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/Networking/Networking/Network/AlamofireNetwork.swift b/Networking/Networking/Network/AlamofireNetwork.swift index b23bfd194ff..f66f58f502a 100644 --- a/Networking/Networking/Network/AlamofireNetwork.swift +++ b/Networking/Networking/Network/AlamofireNetwork.swift @@ -7,6 +7,9 @@ extension Alamofire.MultipartFormData: MultipartFormData {} /// AlamofireWrapper: Encapsulates all of the Alamofire OP's /// public class AlamofireNetwork: Network { + /// WordPress.com Credentials. + /// + private let credentials: Credentials? private let backgroundSessionManager: Alamofire.SessionManager @@ -19,6 +22,7 @@ public class AlamofireNetwork: Network { /// Public Initializer /// public required init(credentials: Credentials?) { + self.credentials = credentials self.requestAuthenticator = RequestAuthenticator(credentials: credentials) // A unique ID is included in the background session identifier so that the session does not get invalidated when the initializer is called multiple @@ -111,7 +115,11 @@ public extension AlamofireNetwork { /// Updates the application password use case with a new site ID. /// func configureApplicationPasswordHandler(with siteID: Int64) { - requestAuthenticator.updateApplicationPasswordHandler(with: siteID) + guard let credentials else { + return + } + let applicationPasswordUseCase = TemporaryApplicationPasswordUseCase(siteID: siteID, credentials: credentials) + requestAuthenticator.updateApplicationPasswordHandler(with: applicationPasswordUseCase) } } diff --git a/Networking/Networking/Network/RequestAuthenticator.swift b/Networking/Networking/Network/RequestAuthenticator.swift index 71877a11a26..53f762df855 100644 --- a/Networking/Networking/Network/RequestAuthenticator.swift +++ b/Networking/Networking/Network/RequestAuthenticator.swift @@ -37,11 +37,8 @@ final class RequestAuthenticator { /// Updates the application password use case with a new site ID. /// - func updateApplicationPasswordHandler(with siteID: Int64) { - guard let credentials else { - return - } - self.applicationPasswordUseCase = TemporaryApplicationPasswordUseCase(siteID: siteID, credentials: credentials) + func updateApplicationPasswordHandler(with useCase: ApplicationPasswordUseCase) { + applicationPasswordUseCase = useCase } /// Updates a request with application password or WPCOM token if possible. From ad695561b001cafec45112f9678e431ea8fda9a7 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Wed, 14 Dec 2022 17:30:30 +0700 Subject: [PATCH 15/30] Add unit tests for RequestAuthenticator --- .../Networking.xcodeproj/project.pbxproj | 4 + .../Network/RequestAuthenticator.swift | 4 +- .../Network/RequestAuthenticatorTests.swift | 148 ++++++++++++++++++ 3 files changed, 154 insertions(+), 2 deletions(-) create mode 100644 Networking/NetworkingTests/Network/RequestAuthenticatorTests.swift diff --git a/Networking/Networking.xcodeproj/project.pbxproj b/Networking/Networking.xcodeproj/project.pbxproj index 9a4f6415085..9c27565889c 100644 --- a/Networking/Networking.xcodeproj/project.pbxproj +++ b/Networking/Networking.xcodeproj/project.pbxproj @@ -711,6 +711,7 @@ DEC51B02276AFB35009F3DF4 /* SystemStatus+DropinMustUsePlugin.swift in Sources */ = {isa = PBXBuildFile; fileRef = DEC51B01276AFB34009F3DF4 /* SystemStatus+DropinMustUsePlugin.swift */; }; DEFBA74E29485A7600C35BA9 /* RESTRequest.swift in Sources */ = {isa = PBXBuildFile; fileRef = DEFBA74D29485A7600C35BA9 /* RESTRequest.swift */; }; DEFBA7542949CE6600C35BA9 /* RequestAuthenticator.swift in Sources */ = {isa = PBXBuildFile; fileRef = DEFBA7532949CE6600C35BA9 /* RequestAuthenticator.swift */; }; + DEFBA7562949D17400C35BA9 /* RequestAuthenticatorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = DEFBA7552949D17300C35BA9 /* RequestAuthenticatorTests.swift */; }; E12552C526385B05001CEE70 /* ShippingLabelAddressValidationSuccess.swift in Sources */ = {isa = PBXBuildFile; fileRef = E12552C426385B05001CEE70 /* ShippingLabelAddressValidationSuccess.swift */; }; E137619929151C7400FD098F /* error-wp-rest-forbidden.json in Resources */ = {isa = PBXBuildFile; fileRef = E137619829151C7400FD098F /* error-wp-rest-forbidden.json */; }; E137619B2915222100FD098F /* WordPressApiValidatorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E137619A2915222100FD098F /* WordPressApiValidatorTests.swift */; }; @@ -1471,6 +1472,7 @@ DEC51B01276AFB34009F3DF4 /* SystemStatus+DropinMustUsePlugin.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "SystemStatus+DropinMustUsePlugin.swift"; sourceTree = ""; }; DEFBA74D29485A7600C35BA9 /* RESTRequest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RESTRequest.swift; sourceTree = ""; }; DEFBA7532949CE6600C35BA9 /* RequestAuthenticator.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RequestAuthenticator.swift; sourceTree = ""; }; + DEFBA7552949D17300C35BA9 /* RequestAuthenticatorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RequestAuthenticatorTests.swift; sourceTree = ""; }; E12552C426385B05001CEE70 /* ShippingLabelAddressValidationSuccess.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShippingLabelAddressValidationSuccess.swift; sourceTree = ""; }; E137619829151C7400FD098F /* error-wp-rest-forbidden.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = "error-wp-rest-forbidden.json"; sourceTree = ""; }; E137619A2915222100FD098F /* WordPressApiValidatorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = WordPressApiValidatorTests.swift; sourceTree = ""; }; @@ -2379,6 +2381,7 @@ isa = PBXGroup; children = ( B57B1E6621C916850046E764 /* NetworkErrorTests.swift */, + DEFBA7552949D17300C35BA9 /* RequestAuthenticatorTests.swift */, ); path = Network; sourceTree = ""; @@ -3466,6 +3469,7 @@ 0212683524C046CB00F8A892 /* MockNetwork+Path.swift in Sources */, 68BD37B328D9B8BD00C2A517 /* CustomerRemoteTests.swift in Sources */, B554FA932180C17200C54DFF /* NoteHashListMapperTests.swift in Sources */, + DEFBA7562949D17400C35BA9 /* RequestAuthenticatorTests.swift in Sources */, CC07866526790B1100BA9AC1 /* ShippingLabelPurchaseMapperTests.swift in Sources */, 74002D6A2118B26100A63C19 /* SiteVisitStatsMapperTests.swift in Sources */, 743E84FA221742E300FAC9D7 /* ShipmentsRemoteTests.swift in Sources */, diff --git a/Networking/Networking/Network/RequestAuthenticator.swift b/Networking/Networking/Network/RequestAuthenticator.swift index 53f762df855..cffc57a77a6 100644 --- a/Networking/Networking/Network/RequestAuthenticator.swift +++ b/Networking/Networking/Network/RequestAuthenticator.swift @@ -57,8 +57,8 @@ final class RequestAuthenticator { } return try await useCase.generateNewPassword() }() - let updatedRequest = try restRequest.updateRequest(with: applicationPassword) - await MainActor.run { + try await MainActor.run { + let updatedRequest = try restRequest.updateRequest(with: applicationPassword) completion(updatedRequest) } } catch { diff --git a/Networking/NetworkingTests/Network/RequestAuthenticatorTests.swift b/Networking/NetworkingTests/Network/RequestAuthenticatorTests.swift new file mode 100644 index 00000000000..f2441faff19 --- /dev/null +++ b/Networking/NetworkingTests/Network/RequestAuthenticatorTests.swift @@ -0,0 +1,148 @@ +import XCTest +import Alamofire +@testable import Networking + +final class RequestAuthenticatorTests: XCTestCase { + + func test_authenticateRequest_returns_unauthenticated_request_for_non_REST_request_without_WPCOM_credentials() { + // Given + let authenticator = RequestAuthenticator(credentials: nil) + let request = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test") + + // When + var result: URLRequestConvertible? + authenticator.authenticateRequest(request) { updatedRequest in + result = updatedRequest + } + + // Then + XCTAssertTrue(result is UnauthenticatedRequest) + } + + func test_authenticatedRequest_returns_authenticated_request_for_non_REST_request_with_WPCOM_credentials() { + // Given + let credentials = Credentials(authToken: "secret") + let authenticator = RequestAuthenticator(credentials: credentials) + let request = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test") + + // When + var result: URLRequestConvertible? + authenticator.authenticateRequest(request) { updatedRequest in + result = updatedRequest + } + + // Then + XCTAssertTrue(result is AuthenticatedRequest) + } + + func test_authenticatedRequest_returns_REST_request_with_authorization_header_if_application_password_is_available() throws { + // Given + let credentials = Credentials(authToken: "secret") + let applicationPassword = ApplicationPassword(wpOrgUsername: "admin", password: .init("supersecret")) + let authenticator = RequestAuthenticator(credentials: credentials) + let useCase = MockApplicationPasswordUseCase(mockApplicationPassword: applicationPassword) + let fallbackRequest = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test") + let restRequest = RESTRequest(siteURL: "https://test.com", method: .get, path: "/test", fallbackRequest: fallbackRequest) + + // When + var result: URLRequestConvertible? + authenticator.updateApplicationPasswordHandler(with: useCase) + waitForExpectation { expectation in + authenticator.authenticateRequest(restRequest) { updatedRequest in + result = updatedRequest + expectation.fulfill() + } + } + + // Then + let request = try XCTUnwrap(result as? URLRequest) + let expectedURL = "https://test.com/test" + assertEqual(expectedURL, request.url?.absoluteString) + let authorizationValue = try XCTUnwrap(request.allHTTPHeaderFields?["Authorization"]) + XCTAssertTrue(authorizationValue.hasPrefix("Basic")) + } + + func test_authenticatedRequest_returns_REST_request_with_authorization_header_if_application_password_generation_succeeds() throws { + // Given + let credentials = Credentials(authToken: "secret") + let applicationPassword = ApplicationPassword(wpOrgUsername: "admin", password: .init("supersecret")) + let authenticator = RequestAuthenticator(credentials: credentials) + let useCase = MockApplicationPasswordUseCase(mockGeneratedPassword: applicationPassword) + let fallbackRequest = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test") + let restRequest = RESTRequest(siteURL: "https://test.com", method: .get, path: "/test", fallbackRequest: fallbackRequest) + + // When + var result: URLRequestConvertible? + authenticator.updateApplicationPasswordHandler(with: useCase) + waitForExpectation { expectation in + authenticator.authenticateRequest(restRequest) { updatedRequest in + result = updatedRequest + expectation.fulfill() + } + } + + // Then + let request = try XCTUnwrap(result as? URLRequest) + let expectedURL = "https://test.com/test" + assertEqual(expectedURL, request.url?.absoluteString) + let authorizationValue = try XCTUnwrap(request.allHTTPHeaderFields?["Authorization"]) + XCTAssertTrue(authorizationValue.hasPrefix("Basic")) + } + + func test_authenticatedRequest_returns_fallback_request_if_generating_application_password_fails_for_REST_request() { + // Given + let credentials = Credentials(authToken: "secret") + let authenticator = RequestAuthenticator(credentials: credentials) + let useCase = MockApplicationPasswordUseCase(mockGenerationError: NetworkError.timeout) + let fallbackRequest = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test") + let restRequest = RESTRequest(siteURL: "https://test.com", method: .get, path: "/test", fallbackRequest: fallbackRequest) + + // When + var result: URLRequestConvertible? + authenticator.updateApplicationPasswordHandler(with: useCase) + waitForExpectation { expectation in + authenticator.authenticateRequest(restRequest) { updatedRequest in + result = updatedRequest + expectation.fulfill() + } + } + + // Then + XCTAssertTrue(result is AuthenticatedRequest) + let expectedURL = "https://public-api.wordpress.com/rest/v1.1/jetpack-blogs/123/rest-api/?json=true&path=/wc/v1/test%26_method%3Dget" + assertEqual(expectedURL, result?.urlRequest?.url?.absoluteString) + } +} + +/// MOCK: application password use case +/// +private final class MockApplicationPasswordUseCase: ApplicationPasswordUseCase { + let mockApplicationPassword: ApplicationPassword? + let mockGeneratedPassword: ApplicationPassword? + let mockGenerationError: Error? + let mockDeletionError: Error? + init(mockApplicationPassword: ApplicationPassword? = nil, + mockGeneratedPassword: ApplicationPassword? = nil, + mockGenerationError: Error? = nil, + mockDeletionError: Error? = nil) { + self.mockApplicationPassword = mockApplicationPassword + self.mockGeneratedPassword = mockGeneratedPassword + self.mockGenerationError = mockGenerationError + self.mockDeletionError = mockDeletionError + } + + var applicationPassword: Networking.ApplicationPassword? { + mockApplicationPassword + } + + func generateNewPassword() async throws -> Networking.ApplicationPassword { + if let mockGeneratedPassword { + return mockGeneratedPassword + } + throw mockGenerationError ?? NetworkError.notFound + } + + func deletePassword() async throws { + throw mockDeletionError ?? NetworkError.notFound + } +} From 4b5423e02dfb54c8e2874778381993974812e57d Mon Sep 17 00:00:00 2001 From: Huong Do Date: Wed, 14 Dec 2022 17:51:10 +0700 Subject: [PATCH 16/30] Update comments for RESTRequest --- Networking/Networking/Requests/RESTRequest.swift | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Networking/Networking/Requests/RESTRequest.swift b/Networking/Networking/Requests/RESTRequest.swift index bcfb4158bd8..6e754e46e4a 100644 --- a/Networking/Networking/Requests/RESTRequest.swift +++ b/Networking/Networking/Requests/RESTRequest.swift @@ -1,6 +1,8 @@ import Foundation import Alamofire +/// Wraps up a URLRequestConvertible Instance, and injects the Authorization + User Agent whenever the actual Request is required. +/// struct RESTRequest: URLRequestConvertible { /// URL of the site to make the request with /// @@ -27,9 +29,10 @@ struct RESTRequest: URLRequestConvertible { /// Designated Initializer. /// /// - Parameters: + /// - siteURL: URL of the site to send the REST request to. /// - method: HTTP Method we should use. - /// - path: RPC that should be executed. - /// - parameters: Collection of String parameters to be passed over to our target RPC. + /// - path: path to the target endpoint. + /// - parameters: Collection of String parameters to be passed over to our target endpoint. /// - headers: Headers to be added to the request. /// - fallbackRequest: A fallback Jetpack request to trigger if the REST request cannot be made. /// @@ -47,7 +50,7 @@ struct RESTRequest: URLRequestConvertible { self.fallbackRequest = fallbackRequest } - /// Returns a URLRequest instance representing the current WordPress.com Request. + /// Returns a URLRequest instance representing the current REST API Request. /// func asURLRequest() throws -> URLRequest { let url = try (siteURL + path).asURL() From 3ede699bfea37eb735796dc02e431d76c6f9a4f4 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Thu, 15 Dec 2022 11:16:31 +0700 Subject: [PATCH 17/30] Fix incorrect username and password when authenticating rest request --- Networking/Networking/Network/RequestAuthenticator.swift | 2 +- Networking/Networking/Requests/RESTRequest.swift | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Networking/Networking/Network/RequestAuthenticator.swift b/Networking/Networking/Network/RequestAuthenticator.swift index cffc57a77a6..f5e4a6af04a 100644 --- a/Networking/Networking/Network/RequestAuthenticator.swift +++ b/Networking/Networking/Network/RequestAuthenticator.swift @@ -58,7 +58,7 @@ final class RequestAuthenticator { return try await useCase.generateNewPassword() }() try await MainActor.run { - let updatedRequest = try restRequest.updateRequest(with: applicationPassword) + let updatedRequest = try restRequest.authenticateRequest(with: applicationPassword) completion(updatedRequest) } } catch { diff --git a/Networking/Networking/Requests/RESTRequest.swift b/Networking/Networking/Requests/RESTRequest.swift index 6e754e46e4a..d7ad5b6964c 100644 --- a/Networking/Networking/Requests/RESTRequest.swift +++ b/Networking/Networking/Requests/RESTRequest.swift @@ -63,13 +63,13 @@ struct RESTRequest: URLRequestConvertible { extension RESTRequest { /// Updates the request headers with authentication information. /// - func updateRequest(with applicationPassword: ApplicationPassword) throws -> URLRequest { + func authenticateRequest(with applicationPassword: ApplicationPassword) throws -> URLRequest { var request = try asURLRequest() request.setValue("application/json", forHTTPHeaderField: "Accept") request.setValue(UserAgent.defaultUserAgent, forHTTPHeaderField: "User-Agent") - let username = "username" - let password = "password" + let username = applicationPassword.wpOrgUsername + let password = applicationPassword.wpOrgUsername let loginString = "\(username):\(password)" guard let loginData = loginString.data(using: .utf8) else { return request From d1ea84cdc022ecc80b449d0b0fe7ed12c819aff4 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Thu, 15 Dec 2022 11:25:52 +0700 Subject: [PATCH 18/30] Reformat code for RequestAuthenticator --- .../Network/RequestAuthenticator.swift | 46 +++++++++++-------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/Networking/Networking/Network/RequestAuthenticator.swift b/Networking/Networking/Network/RequestAuthenticator.swift index f5e4a6af04a..41c2353070b 100644 --- a/Networking/Networking/Network/RequestAuthenticator.swift +++ b/Networking/Networking/Network/RequestAuthenticator.swift @@ -47,35 +47,41 @@ final class RequestAuthenticator { guard let restRequest = request as? RESTRequest, let useCase = applicationPasswordUseCase else { // Handle non-REST requests as before - return completion(createAuthenticatedRequestIfPossible(for: request)) + return completion(authenticateUsingWPCOMTokenIfPossible(request)) } Task(priority: .medium) { - do { - let applicationPassword: ApplicationPassword = try await { - if let password = useCase.applicationPassword { - return password - } - return try await useCase.generateNewPassword() - }() - try await MainActor.run { - let updatedRequest = try restRequest.authenticateRequest(with: applicationPassword) - completion(updatedRequest) - } - } catch { - DDLogWarn("⚠️ Error generating application password and update request: \(error)") - // TODO: add Tracks - // Get the fallback Jetpack request to handle if possible. - let fallbackRequest = restRequest.fallbackRequest ?? request - await MainActor.run { - completion(createAuthenticatedRequestIfPossible(for: fallbackRequest)) + let result = await authenticateUsingApplicationPassword(restRequest, useCase: useCase) + await MainActor.run { + completion(result) + } + } + } + + /// Attempts authenticating a request with application password. + /// + private func authenticateUsingApplicationPassword(_ restRequest: RESTRequest, useCase: ApplicationPasswordUseCase) async -> URLRequestConvertible { + do { + let applicationPassword: ApplicationPassword = try await { + if let password = useCase.applicationPassword { + return password } + return try await useCase.generateNewPassword() + }() + return try await MainActor.run { + return try restRequest.authenticateRequest(with: applicationPassword) } + } catch { + DDLogWarn("⚠️ Error generating application password and update request: \(error)") + // TODO: add Tracks + // Get the fallback Jetpack request to handle if possible. + let fallbackRequest: URLRequestConvertible = restRequest.fallbackRequest ?? restRequest + return authenticateUsingWPCOMTokenIfPossible(fallbackRequest) } } /// Attempts creating a request with WPCOM token if possible. /// - private func createAuthenticatedRequestIfPossible(for request: URLRequestConvertible) -> URLRequestConvertible { + private func authenticateUsingWPCOMTokenIfPossible(_ request: URLRequestConvertible) -> URLRequestConvertible { credentials.map { AuthenticatedRequest(credentials: $0, request: request) } ?? UnauthenticatedRequest(request: request) } From 139348b2ee8195ce855e109d563fbaf1466e9b95 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Fri, 16 Dec 2022 15:52:34 +0700 Subject: [PATCH 19/30] Remove fallback request and header from --- .../Networking/Requests/RESTRequest.swift | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/Networking/Networking/Requests/RESTRequest.swift b/Networking/Networking/Requests/RESTRequest.swift index d7ad5b6964c..a10f50b38d6 100644 --- a/Networking/Networking/Requests/RESTRequest.swift +++ b/Networking/Networking/Requests/RESTRequest.swift @@ -20,42 +20,31 @@ struct RESTRequest: URLRequestConvertible { /// let parameters: [String: Any]? - /// HTTP Headers - let headers: [String: String] - - /// A fallback JetpackRequest if the REST request cannot be made with an application password. - let fallbackRequest: JetpackRequest? - /// Designated Initializer. /// /// - Parameters: /// - siteURL: URL of the site to send the REST request to. /// - method: HTTP Method we should use. /// - path: path to the target endpoint. - /// - parameters: Collection of String parameters to be passed over to our target endpoint. + /// - parameters: Collection of String parameters to be passed over to our target endpoint. This can be encoded to the URL request query if the HTTP method is `.get`. /// - headers: Headers to be added to the request. /// - fallbackRequest: A fallback Jetpack request to trigger if the REST request cannot be made. /// init(siteURL: String, method: HTTPMethod, path: String, - parameters: [String: Any] = [:], - headers: [String: String] = [:], - fallbackRequest: JetpackRequest?) { + parameters: [String: Any] = [:]) { self.siteURL = siteURL self.method = method self.path = path self.parameters = parameters - self.headers = headers - self.fallbackRequest = fallbackRequest } /// Returns a URLRequest instance representing the current REST API Request. /// func asURLRequest() throws -> URLRequest { let url = try (siteURL + path).asURL() - let request = try URLRequest(url: url, method: method, headers: headers) - + let request = try URLRequest(url: url, method: method) return try URLEncoding.default.encode(request, with: parameters) } } From b7942a57cfea9a56a56ebbd48e8d2d7b5b97e82c Mon Sep 17 00:00:00 2001 From: Huong Do Date: Fri, 16 Dec 2022 16:09:41 +0700 Subject: [PATCH 20/30] Update JetpackRequest to be able to converted to a REST request --- .../Networking/Requests/JetpackRequest.swift | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/Networking/Networking/Requests/JetpackRequest.swift b/Networking/Networking/Requests/JetpackRequest.swift index 4eb9ac6227d..c8ae500b63c 100644 --- a/Networking/Networking/Requests/JetpackRequest.swift +++ b/Networking/Networking/Requests/JetpackRequest.swift @@ -34,6 +34,10 @@ struct JetpackRequest: Request { /// let parameters: [String: Any] + /// Whether this request should be transformed to a REST request if application password is available. + /// + let availableAsRESTRequest: Bool + /// Designated Initializer. /// @@ -43,8 +47,15 @@ struct JetpackRequest: Request { /// - siteID: Identifier of the Jetpack-Connected site we'll query. /// - path: RPC that should be called. /// - parameters: Collection of Key/Value parameters, to be forwarded to the Jetpack Connected site. - /// - init(wooApiVersion: WooAPIVersion, method: HTTPMethod, siteID: Int64, locale: String? = nil, path: String, parameters: [String: Any]? = nil) { + /// - availableAsRESTRequest: Whether the request should be transformed to a REST request if application password is available. + /// + init(wooApiVersion: WooAPIVersion, + method: HTTPMethod, + siteID: Int64, + locale: String? = nil, + path: String, + parameters: [String: Any]? = nil, + availableAsRESTRequest: Bool = false) { if [.mark1, .mark2].contains(wooApiVersion) { DDLogWarn("⚠️ You are using an older version of the Woo REST API: \(wooApiVersion.rawValue), for path: \(path)") } @@ -54,6 +65,7 @@ struct JetpackRequest: Request { self.locale = locale self.path = path self.parameters = parameters ?? [:] + self.availableAsRESTRequest = availableAsRESTRequest } @@ -69,6 +81,10 @@ struct JetpackRequest: Request { func responseDataValidator() -> ResponseDataValidator { return DotcomValidator() } + + func createRESTRequest(with siteURL: String) -> RESTRequest { + RESTRequest(siteURL: siteURL, method: method, path: path, parameters: parameters) + } } From 63a76f63f4ff31dfb9355f3bdf2a762afa48a4b5 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Fri, 16 Dec 2022 16:37:37 +0700 Subject: [PATCH 21/30] Update RequestAuthenticator to return error if application password cannot be generated --- .../Networking/Network/AlamofireNetwork.swift | 60 ++++++++++++------- .../Network/RequestAuthenticator.swift | 41 ++++++------- 2 files changed, 61 insertions(+), 40 deletions(-) diff --git a/Networking/Networking/Network/AlamofireNetwork.swift b/Networking/Networking/Network/AlamofireNetwork.swift index f66f58f502a..44d10a7a442 100644 --- a/Networking/Networking/Network/AlamofireNetwork.swift +++ b/Networking/Networking/Network/AlamofireNetwork.swift @@ -48,11 +48,16 @@ public class AlamofireNetwork: Network { /// - Yes. We do the above because the Jetpack Tunnel endpoint doesn't properly relay the correct statusCode. /// public func responseData(for request: URLRequestConvertible, completion: @escaping (Data?, Error?) -> Void) { - requestAuthenticator.authenticateRequest(request) { request in - Alamofire.request(request) - .responseData { response in - completion(response.value, response.networkingError) - } + requestAuthenticator.authenticateRequest(request) { result in + switch result { + case .success(let request): + Alamofire.request(request) + .responseData { response in + completion(response.value, response.networkingError) + } + case .failure(let error): + completion(nil, error) + } } } @@ -66,9 +71,14 @@ public class AlamofireNetwork: Network { /// - completion: Closure to be executed upon completion. /// public func responseData(for request: URLRequestConvertible, completion: @escaping (Swift.Result) -> Void) { - requestAuthenticator.authenticateRequest(request) { request in - Alamofire.request(request).responseData { response in - completion(response.result.toSwiftResult()) + requestAuthenticator.authenticateRequest(request) { result in + switch result { + case .success(let request): + Alamofire.request(request).responseData { response in + completion(response.result.toSwiftResult()) + } + case .failure(let error): + completion(.failure(error)) } } } @@ -83,10 +93,15 @@ public class AlamofireNetwork: Network { /// - Returns: A publisher that emits the result of the given request. public func responseDataPublisher(for request: URLRequestConvertible) -> AnyPublisher, Never> { return Future() { promise in - self.requestAuthenticator.authenticateRequest(request) { request in - Alamofire.request(request).responseData { response in - let result = response.result.toSwiftResult() - promise(Swift.Result.success(result)) + self.requestAuthenticator.authenticateRequest(request) { result in + switch result { + case .success(let request): + Alamofire.request(request).responseData { response in + let result = response.result.toSwiftResult() + promise(.success(result)) + } + case .failure(let error): + promise(.success(.failure(error))) } } }.eraseToAnyPublisher() @@ -95,17 +110,22 @@ public class AlamofireNetwork: Network { public func uploadMultipartFormData(multipartFormData: @escaping (MultipartFormData) -> Void, to request: URLRequestConvertible, completion: @escaping (Data?, Error?) -> Void) { - requestAuthenticator.authenticateRequest(request) { [weak self] request in + requestAuthenticator.authenticateRequest(request) { [weak self] result in guard let self else { return } - self.backgroundSessionManager.upload(multipartFormData: multipartFormData, with: request) { (encodingResult) in - switch encodingResult { - case .success(let upload, _, _): - upload.responseData { response in - completion(response.value, response.error) + switch result { + case .success(let request): + self.backgroundSessionManager.upload(multipartFormData: multipartFormData, with: request) { (encodingResult) in + switch encodingResult { + case .success(let upload, _, _): + upload.responseData { response in + completion(response.value, response.error) + } + case .failure(let error): + completion(nil, error) } - case .failure(let error): - completion(nil, error) } + case .failure(let error): + completion(nil, error) } } } diff --git a/Networking/Networking/Network/RequestAuthenticator.swift b/Networking/Networking/Network/RequestAuthenticator.swift index 41c2353070b..6cd7186a842 100644 --- a/Networking/Networking/Network/RequestAuthenticator.swift +++ b/Networking/Networking/Network/RequestAuthenticator.swift @@ -43,14 +43,23 @@ final class RequestAuthenticator { /// Updates a request with application password or WPCOM token if possible. /// - func authenticateRequest(_ request: URLRequestConvertible, completion: @escaping (URLRequestConvertible) -> Void) { - guard let restRequest = request as? RESTRequest, + func authenticateRequest(_ request: URLRequestConvertible, completion: @escaping (Swift.Result) -> Void) { + guard let jetpackRequest = request as? JetpackRequest, + jetpackRequest.availableAsRESTRequest, let useCase = applicationPasswordUseCase else { // Handle non-REST requests as before - return completion(authenticateUsingWPCOMTokenIfPossible(request)) + return completion(.success(authenticateUsingWPCOMTokenIfPossible(request))) } + // TODO: get site URL from wporg credentials + let restRequest = jetpackRequest.createRESTRequest(with: "") Task(priority: .medium) { - let result = await authenticateUsingApplicationPassword(restRequest, useCase: useCase) + let result: Swift.Result + do { + let authenticatedRequest = try await authenticateUsingApplicationPassword(restRequest, useCase: useCase) + result = .success(authenticatedRequest) + } catch { + result = .failure(error) + } await MainActor.run { completion(result) } @@ -59,23 +68,15 @@ final class RequestAuthenticator { /// Attempts authenticating a request with application password. /// - private func authenticateUsingApplicationPassword(_ restRequest: RESTRequest, useCase: ApplicationPasswordUseCase) async -> URLRequestConvertible { - do { - let applicationPassword: ApplicationPassword = try await { - if let password = useCase.applicationPassword { - return password - } - return try await useCase.generateNewPassword() - }() - return try await MainActor.run { - return try restRequest.authenticateRequest(with: applicationPassword) + private func authenticateUsingApplicationPassword(_ restRequest: RESTRequest, useCase: ApplicationPasswordUseCase) async throws -> URLRequestConvertible { + let applicationPassword: ApplicationPassword = try await { + if let password = useCase.applicationPassword { + return password } - } catch { - DDLogWarn("⚠️ Error generating application password and update request: \(error)") - // TODO: add Tracks - // Get the fallback Jetpack request to handle if possible. - let fallbackRequest: URLRequestConvertible = restRequest.fallbackRequest ?? restRequest - return authenticateUsingWPCOMTokenIfPossible(fallbackRequest) + return try await useCase.generateNewPassword() + }() + return try await MainActor.run { + return try restRequest.authenticateRequest(with: applicationPassword) } } From 012d6c85231a23bc252f7766e7ebcae9fda0f459 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Fri, 16 Dec 2022 16:45:17 +0700 Subject: [PATCH 22/30] Revert changes to AuthenticatedState and DefaultStoresManager --- .../Networking/Network/AlamofireNetwork.swift | 13 ------------- .../Classes/Yosemite/AuthenticatedState.swift | 6 ------ .../Classes/Yosemite/DefaultStoresManager.swift | 4 ---- 3 files changed, 23 deletions(-) diff --git a/Networking/Networking/Network/AlamofireNetwork.swift b/Networking/Networking/Network/AlamofireNetwork.swift index 44d10a7a442..cfd6e6f688f 100644 --- a/Networking/Networking/Network/AlamofireNetwork.swift +++ b/Networking/Networking/Network/AlamofireNetwork.swift @@ -131,19 +131,6 @@ public class AlamofireNetwork: Network { } } -public extension AlamofireNetwork { - /// Updates the application password use case with a new site ID. - /// - func configureApplicationPasswordHandler(with siteID: Int64) { - guard let credentials else { - return - } - let applicationPasswordUseCase = TemporaryApplicationPasswordUseCase(siteID: siteID, credentials: credentials) - requestAuthenticator.updateApplicationPasswordHandler(with: applicationPasswordUseCase) - } -} - - // MARK: - Alamofire.DataResponse: Helper Methods // extension Alamofire.DataResponse { diff --git a/WooCommerce/Classes/Yosemite/AuthenticatedState.swift b/WooCommerce/Classes/Yosemite/AuthenticatedState.swift index 362671e5e85..3de225ac2bb 100644 --- a/WooCommerce/Classes/Yosemite/AuthenticatedState.swift +++ b/WooCommerce/Classes/Yosemite/AuthenticatedState.swift @@ -128,12 +128,6 @@ class AuthenticatedState: StoresManagerState { func onAction(_ action: Action) { dispatcher.dispatch(action) } - - /// Updates the network with the currently selected site. - /// - func updateCurrentSite(siteID: Int64) { - network.configureApplicationPasswordHandler(with: siteID) - } } diff --git a/WooCommerce/Classes/Yosemite/DefaultStoresManager.swift b/WooCommerce/Classes/Yosemite/DefaultStoresManager.swift index 36b82e5340e..e93e637a686 100644 --- a/WooCommerce/Classes/Yosemite/DefaultStoresManager.swift +++ b/WooCommerce/Classes/Yosemite/DefaultStoresManager.swift @@ -195,10 +195,6 @@ class DefaultStoresManager: StoresManager { restoreSessionSiteIfPossible() ServiceLocator.pushNotesManager.reloadBadgeCount() - if let state = self.state as? AuthenticatedState { - state.updateCurrentSite(siteID: storeID) - } - NotificationCenter.default.post(name: .StoresManagerDidUpdateDefaultSite, object: nil) } From f779ff273a3ee4683594b3912a98f35f7053e110 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Fri, 16 Dec 2022 16:47:07 +0700 Subject: [PATCH 23/30] Fix line length violation --- Networking/Networking/Requests/RESTRequest.swift | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Networking/Networking/Requests/RESTRequest.swift b/Networking/Networking/Requests/RESTRequest.swift index a10f50b38d6..b06d30180f9 100644 --- a/Networking/Networking/Requests/RESTRequest.swift +++ b/Networking/Networking/Requests/RESTRequest.swift @@ -26,7 +26,8 @@ struct RESTRequest: URLRequestConvertible { /// - siteURL: URL of the site to send the REST request to. /// - method: HTTP Method we should use. /// - path: path to the target endpoint. - /// - parameters: Collection of String parameters to be passed over to our target endpoint. This can be encoded to the URL request query if the HTTP method is `.get`. + /// - parameters: Collection of String parameters to be passed over to our target endpoint. + /// This can be encoded to the URL request query if the HTTP method is `.get`. /// - headers: Headers to be added to the request. /// - fallbackRequest: A fallback Jetpack request to trigger if the REST request cannot be made. /// From c87f2745abe41827b16da6e19335859931659453 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Fri, 16 Dec 2022 16:49:18 +0700 Subject: [PATCH 24/30] Keep `network` local in AuthenticatedState intializer --- WooCommerce/Classes/Yosemite/AuthenticatedState.swift | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/WooCommerce/Classes/Yosemite/AuthenticatedState.swift b/WooCommerce/Classes/Yosemite/AuthenticatedState.swift index 3de225ac2bb..f27b93b2ed6 100644 --- a/WooCommerce/Classes/Yosemite/AuthenticatedState.swift +++ b/WooCommerce/Classes/Yosemite/AuthenticatedState.swift @@ -19,16 +19,11 @@ class AuthenticatedState: StoresManagerState { /// private var errorObserverToken: NSObjectProtocol? - /// The network to handle network requests. - /// - private let network: AlamofireNetwork - - /// Designated Initializer /// init(credentials: Credentials) { let storageManager = ServiceLocator.storageManager - network = AlamofireNetwork(credentials: credentials) + let network = AlamofireNetwork(credentials: credentials) services = [ AccountStore(dispatcher: dispatcher, storageManager: storageManager, network: network, dotcomAuthToken: credentials.authToken), From fdf956668c0114a5f602bebb78e512f497371f40 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Mon, 19 Dec 2022 14:46:53 +0700 Subject: [PATCH 25/30] Update unit tests for RequestAuthenticator --- .../Network/RequestAuthenticatorTests.swift | 58 +++++++++---------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/Networking/NetworkingTests/Network/RequestAuthenticatorTests.swift b/Networking/NetworkingTests/Network/RequestAuthenticatorTests.swift index f2441faff19..76e0666dd1b 100644 --- a/Networking/NetworkingTests/Network/RequestAuthenticatorTests.swift +++ b/Networking/NetworkingTests/Network/RequestAuthenticatorTests.swift @@ -7,32 +7,32 @@ final class RequestAuthenticatorTests: XCTestCase { func test_authenticateRequest_returns_unauthenticated_request_for_non_REST_request_without_WPCOM_credentials() { // Given let authenticator = RequestAuthenticator(credentials: nil) - let request = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test") + let request = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test", availableAsRESTRequest: false) // When - var result: URLRequestConvertible? - authenticator.authenticateRequest(request) { updatedRequest in - result = updatedRequest + var updatedRequest: URLRequestConvertible? + authenticator.authenticateRequest(request) { result in + updatedRequest = try? result.get() } // Then - XCTAssertTrue(result is UnauthenticatedRequest) + XCTAssertTrue(updatedRequest is UnauthenticatedRequest) } func test_authenticatedRequest_returns_authenticated_request_for_non_REST_request_with_WPCOM_credentials() { // Given let credentials = Credentials(authToken: "secret") let authenticator = RequestAuthenticator(credentials: credentials) - let request = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test") + let request = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test", availableAsRESTRequest: false) // When - var result: URLRequestConvertible? - authenticator.authenticateRequest(request) { updatedRequest in - result = updatedRequest + var updatedRequest: URLRequestConvertible? + authenticator.authenticateRequest(request) { result in + updatedRequest = try? result.get() } // Then - XCTAssertTrue(result is AuthenticatedRequest) + XCTAssertTrue(updatedRequest is AuthenticatedRequest) } func test_authenticatedRequest_returns_REST_request_with_authorization_header_if_application_password_is_available() throws { @@ -41,21 +41,20 @@ final class RequestAuthenticatorTests: XCTestCase { let applicationPassword = ApplicationPassword(wpOrgUsername: "admin", password: .init("supersecret")) let authenticator = RequestAuthenticator(credentials: credentials) let useCase = MockApplicationPasswordUseCase(mockApplicationPassword: applicationPassword) - let fallbackRequest = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test") - let restRequest = RESTRequest(siteURL: "https://test.com", method: .get, path: "/test", fallbackRequest: fallbackRequest) + let jetpackRequest = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test", availableAsRESTRequest: true) // When - var result: URLRequestConvertible? + var updatedRequest: URLRequestConvertible? authenticator.updateApplicationPasswordHandler(with: useCase) waitForExpectation { expectation in - authenticator.authenticateRequest(restRequest) { updatedRequest in - result = updatedRequest + authenticator.authenticateRequest(jetpackRequest) { result in + updatedRequest = try? result.get() expectation.fulfill() } } // Then - let request = try XCTUnwrap(result as? URLRequest) + let request = try XCTUnwrap(updatedRequest as? URLRequest) let expectedURL = "https://test.com/test" assertEqual(expectedURL, request.url?.absoluteString) let authorizationValue = try XCTUnwrap(request.allHTTPHeaderFields?["Authorization"]) @@ -68,49 +67,46 @@ final class RequestAuthenticatorTests: XCTestCase { let applicationPassword = ApplicationPassword(wpOrgUsername: "admin", password: .init("supersecret")) let authenticator = RequestAuthenticator(credentials: credentials) let useCase = MockApplicationPasswordUseCase(mockGeneratedPassword: applicationPassword) - let fallbackRequest = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test") - let restRequest = RESTRequest(siteURL: "https://test.com", method: .get, path: "/test", fallbackRequest: fallbackRequest) + let jetpackRequest = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test", availableAsRESTRequest: true) // When - var result: URLRequestConvertible? + var updatedRequest: URLRequestConvertible? authenticator.updateApplicationPasswordHandler(with: useCase) waitForExpectation { expectation in - authenticator.authenticateRequest(restRequest) { updatedRequest in - result = updatedRequest + authenticator.authenticateRequest(jetpackRequest) { result in + updatedRequest = try? result.get() expectation.fulfill() } } // Then - let request = try XCTUnwrap(result as? URLRequest) + let request = try XCTUnwrap(updatedRequest as? URLRequest) let expectedURL = "https://test.com/test" assertEqual(expectedURL, request.url?.absoluteString) let authorizationValue = try XCTUnwrap(request.allHTTPHeaderFields?["Authorization"]) XCTAssertTrue(authorizationValue.hasPrefix("Basic")) } - func test_authenticatedRequest_returns_fallback_request_if_generating_application_password_fails_for_REST_request() { + func test_authenticatedRequest_returns_error_if_generating_application_password_fails_for_REST_request() throws { // Given let credentials = Credentials(authToken: "secret") let authenticator = RequestAuthenticator(credentials: credentials) let useCase = MockApplicationPasswordUseCase(mockGenerationError: NetworkError.timeout) - let fallbackRequest = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test") - let restRequest = RESTRequest(siteURL: "https://test.com", method: .get, path: "/test", fallbackRequest: fallbackRequest) + let jetpackRequest = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test", availableAsRESTRequest: true) // When - var result: URLRequestConvertible? + var error: Error? authenticator.updateApplicationPasswordHandler(with: useCase) waitForExpectation { expectation in - authenticator.authenticateRequest(restRequest) { updatedRequest in - result = updatedRequest + authenticator.authenticateRequest(jetpackRequest) { result in + error = result.failure expectation.fulfill() } } // Then - XCTAssertTrue(result is AuthenticatedRequest) - let expectedURL = "https://public-api.wordpress.com/rest/v1.1/jetpack-blogs/123/rest-api/?json=true&path=/wc/v1/test%26_method%3Dget" - assertEqual(expectedURL, result?.urlRequest?.url?.absoluteString) + let networkError = try XCTUnwrap(error as? NetworkError) + XCTAssertEqual(networkError, NetworkError.timeout) } } From ab3a53bf2fc24ade0569d44b51d8f3596ad76e0d Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 20 Dec 2022 14:36:57 +0700 Subject: [PATCH 26/30] Update RequestAuthenticator to set up application password usecase in the initializer --- .../Network/RequestAuthenticator.swift | 31 ++++++++++++------- .../Network/RequestAuthenticatorTests.swift | 19 +++++------- 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/Networking/Networking/Network/RequestAuthenticator.swift b/Networking/Networking/Network/RequestAuthenticator.swift index 6cd7186a842..112b9e90123 100644 --- a/Networking/Networking/Network/RequestAuthenticator.swift +++ b/Networking/Networking/Network/RequestAuthenticator.swift @@ -29,16 +29,24 @@ final class RequestAuthenticator { /// The use case to handle authentication with application passwords. /// - private var applicationPasswordUseCase: ApplicationPasswordUseCase? + private let applicationPasswordUseCase: ApplicationPasswordUseCase? - init(credentials: Credentials?) { - self.credentials = credentials - } - - /// Updates the application password use case with a new site ID. + /// Sets up the authenticator with optional credentials and application password use case. + /// `applicationPasswordUseCase` is injectable for testability. /// - func updateApplicationPasswordHandler(with useCase: ApplicationPasswordUseCase) { - applicationPasswordUseCase = useCase + init(credentials: Credentials?, applicationPasswordUseCase: ApplicationPasswordUseCase? = nil) { + self.credentials = credentials + let useCase: ApplicationPasswordUseCase? = { + if let applicationPasswordUseCase { + return applicationPasswordUseCase + } else if let credentials, case .wporg = credentials { + // TODO: setup DefaultApplicationPasswordUseCase + return nil + } else { + return nil + } + }() + self.applicationPasswordUseCase = useCase } /// Updates a request with application password or WPCOM token if possible. @@ -46,12 +54,13 @@ final class RequestAuthenticator { func authenticateRequest(_ request: URLRequestConvertible, completion: @escaping (Swift.Result) -> Void) { guard let jetpackRequest = request as? JetpackRequest, jetpackRequest.availableAsRESTRequest, - let useCase = applicationPasswordUseCase else { + let useCase = applicationPasswordUseCase, + case let .some(.wporg(_, _, siteAddress)) = credentials else { // Handle non-REST requests as before return completion(.success(authenticateUsingWPCOMTokenIfPossible(request))) } - // TODO: get site URL from wporg credentials - let restRequest = jetpackRequest.createRESTRequest(with: "") + + let restRequest = jetpackRequest.createRESTRequest(with: siteAddress) Task(priority: .medium) { let result: Swift.Result do { diff --git a/Networking/NetworkingTests/Network/RequestAuthenticatorTests.swift b/Networking/NetworkingTests/Network/RequestAuthenticatorTests.swift index 76e0666dd1b..78bfa3898f1 100644 --- a/Networking/NetworkingTests/Network/RequestAuthenticatorTests.swift +++ b/Networking/NetworkingTests/Network/RequestAuthenticatorTests.swift @@ -37,15 +37,14 @@ final class RequestAuthenticatorTests: XCTestCase { func test_authenticatedRequest_returns_REST_request_with_authorization_header_if_application_password_is_available() throws { // Given - let credentials = Credentials(authToken: "secret") - let applicationPassword = ApplicationPassword(wpOrgUsername: "admin", password: .init("supersecret")) - let authenticator = RequestAuthenticator(credentials: credentials) + let credentials: Credentials = .wporg(username: "admin", password: "supersecret", siteAddress: "https://test.com/") + let applicationPassword = ApplicationPassword(wpOrgUsername: credentials.username, password: .init(credentials.secret)) let useCase = MockApplicationPasswordUseCase(mockApplicationPassword: applicationPassword) + let authenticator = RequestAuthenticator(credentials: credentials, applicationPasswordUseCase: useCase) let jetpackRequest = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test", availableAsRESTRequest: true) // When var updatedRequest: URLRequestConvertible? - authenticator.updateApplicationPasswordHandler(with: useCase) waitForExpectation { expectation in authenticator.authenticateRequest(jetpackRequest) { result in updatedRequest = try? result.get() @@ -63,15 +62,14 @@ final class RequestAuthenticatorTests: XCTestCase { func test_authenticatedRequest_returns_REST_request_with_authorization_header_if_application_password_generation_succeeds() throws { // Given - let credentials = Credentials(authToken: "secret") - let applicationPassword = ApplicationPassword(wpOrgUsername: "admin", password: .init("supersecret")) - let authenticator = RequestAuthenticator(credentials: credentials) + let credentials: Credentials = .wporg(username: "admin", password: "supersecret", siteAddress: "https://test.com/") + let applicationPassword = ApplicationPassword(wpOrgUsername: credentials.username, password: .init(credentials.secret)) let useCase = MockApplicationPasswordUseCase(mockGeneratedPassword: applicationPassword) + let authenticator = RequestAuthenticator(credentials: credentials, applicationPasswordUseCase: useCase) let jetpackRequest = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test", availableAsRESTRequest: true) // When var updatedRequest: URLRequestConvertible? - authenticator.updateApplicationPasswordHandler(with: useCase) waitForExpectation { expectation in authenticator.authenticateRequest(jetpackRequest) { result in updatedRequest = try? result.get() @@ -89,14 +87,13 @@ final class RequestAuthenticatorTests: XCTestCase { func test_authenticatedRequest_returns_error_if_generating_application_password_fails_for_REST_request() throws { // Given - let credentials = Credentials(authToken: "secret") - let authenticator = RequestAuthenticator(credentials: credentials) + let credentials: Credentials = .wporg(username: "admin", password: "supersecret", siteAddress: "https://test.com/") let useCase = MockApplicationPasswordUseCase(mockGenerationError: NetworkError.timeout) + let authenticator = RequestAuthenticator(credentials: credentials, applicationPasswordUseCase: useCase) let jetpackRequest = JetpackRequest(wooApiVersion: .mark1, method: .get, siteID: 123, path: "test", availableAsRESTRequest: true) // When var error: Error? - authenticator.updateApplicationPasswordHandler(with: useCase) waitForExpectation { expectation in authenticator.authenticateRequest(jetpackRequest) { result in error = result.failure From f3fa7aae6ad8b8e1ef5fdaea6e8113d6ba18c199 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Wed, 21 Dec 2022 12:43:57 +0700 Subject: [PATCH 27/30] Create application password use case in RequestAuthenticator --- .../ApplicationPasswordUseCase.swift | 3 +- .../Network/RequestAuthenticator.swift | 29 ++++--------------- 2 files changed, 7 insertions(+), 25 deletions(-) diff --git a/Networking/Networking/ApplicationPassword/ApplicationPasswordUseCase.swift b/Networking/Networking/ApplicationPassword/ApplicationPasswordUseCase.swift index 5cfcb0f8552..a1b19a12303 100644 --- a/Networking/Networking/ApplicationPassword/ApplicationPasswordUseCase.swift +++ b/Networking/Networking/ApplicationPassword/ApplicationPasswordUseCase.swift @@ -63,11 +63,10 @@ final class DefaultApplicationPasswordUseCase: ApplicationPasswordUseCase { } } - @MainActor init(username: String, password: String, siteAddress: String, - network: Network? = nil) async throws { + network: Network? = nil) throws { self.siteAddress = siteAddress self.username = username diff --git a/Networking/Networking/Network/RequestAuthenticator.swift b/Networking/Networking/Network/RequestAuthenticator.swift index 112b9e90123..dc2ba846b44 100644 --- a/Networking/Networking/Network/RequestAuthenticator.swift +++ b/Networking/Networking/Network/RequestAuthenticator.swift @@ -1,25 +1,6 @@ import Alamofire import Foundation -// TODO: Replace with actual implementation. -final class TemporaryApplicationPasswordUseCase: ApplicationPasswordUseCase { - init(siteID: Int64, credentials: Credentials) { - // no-op - } - - var applicationPassword: ApplicationPassword? { - return nil - } - - func generateNewPassword() async throws -> ApplicationPassword { - return .init(wpOrgUsername: "test", password: .init("12345")) - } - - func deletePassword() async throws { - // no-op - } -} - /// Helper class to update requests with authorization header if possible. /// final class RequestAuthenticator { @@ -32,16 +13,18 @@ final class RequestAuthenticator { private let applicationPasswordUseCase: ApplicationPasswordUseCase? /// Sets up the authenticator with optional credentials and application password use case. - /// `applicationPasswordUseCase` is injectable for testability. + /// `applicationPasswordUseCase` can be injected for unit tests. /// init(credentials: Credentials?, applicationPasswordUseCase: ApplicationPasswordUseCase? = nil) { self.credentials = credentials let useCase: ApplicationPasswordUseCase? = { if let applicationPasswordUseCase { return applicationPasswordUseCase - } else if let credentials, case .wporg = credentials { - // TODO: setup DefaultApplicationPasswordUseCase - return nil + } else if let credentials, + case let .wporg(username, password, siteAddress) = credentials { + return try? DefaultApplicationPasswordUseCase(username: username, + password: password, + siteAddress: siteAddress) } else { return nil } From aa6d8e9d9ffe1e715068325ea90e53af5ad3052d Mon Sep 17 00:00:00 2001 From: Huong Do Date: Wed, 21 Dec 2022 15:23:01 +0700 Subject: [PATCH 28/30] Return completion when self is not found --- Networking/Networking/Network/AlamofireNetwork.swift | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Networking/Networking/Network/AlamofireNetwork.swift b/Networking/Networking/Network/AlamofireNetwork.swift index cfd6e6f688f..4c45d4b8980 100644 --- a/Networking/Networking/Network/AlamofireNetwork.swift +++ b/Networking/Networking/Network/AlamofireNetwork.swift @@ -111,7 +111,9 @@ public class AlamofireNetwork: Network { to request: URLRequestConvertible, completion: @escaping (Data?, Error?) -> Void) { requestAuthenticator.authenticateRequest(request) { [weak self] result in - guard let self else { return } + guard let self else { + return completion(nil, nil) + } switch result { case .success(let request): self.backgroundSessionManager.upload(multipartFormData: multipartFormData, with: request) { (encodingResult) in From 67b9316144a73dc32965d14ef076555f66369927 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Wed, 21 Dec 2022 15:25:07 +0700 Subject: [PATCH 29/30] Simplify condition checks for enum case --- Networking/Networking/Network/RequestAuthenticator.swift | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Networking/Networking/Network/RequestAuthenticator.swift b/Networking/Networking/Network/RequestAuthenticator.swift index dc2ba846b44..bf332c5253e 100644 --- a/Networking/Networking/Network/RequestAuthenticator.swift +++ b/Networking/Networking/Network/RequestAuthenticator.swift @@ -20,8 +20,7 @@ final class RequestAuthenticator { let useCase: ApplicationPasswordUseCase? = { if let applicationPasswordUseCase { return applicationPasswordUseCase - } else if let credentials, - case let .wporg(username, password, siteAddress) = credentials { + } else if case let .wporg(username, password, siteAddress) = credentials { return try? DefaultApplicationPasswordUseCase(username: username, password: password, siteAddress: siteAddress) @@ -38,7 +37,7 @@ final class RequestAuthenticator { guard let jetpackRequest = request as? JetpackRequest, jetpackRequest.availableAsRESTRequest, let useCase = applicationPasswordUseCase, - case let .some(.wporg(_, _, siteAddress)) = credentials else { + case let .wporg(_, _, siteAddress) = credentials else { // Handle non-REST requests as before return completion(.success(authenticateUsingWPCOMTokenIfPossible(request))) } From e66619ae60bd06e1a674cd7215665f5a40041833 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Wed, 21 Dec 2022 15:27:38 +0700 Subject: [PATCH 30/30] Rename createRESTRequest to asRESTRequest --- Networking/Networking/Network/RequestAuthenticator.swift | 5 ++--- Networking/Networking/Requests/JetpackRequest.swift | 9 ++++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/Networking/Networking/Network/RequestAuthenticator.swift b/Networking/Networking/Network/RequestAuthenticator.swift index bf332c5253e..d931202cc73 100644 --- a/Networking/Networking/Network/RequestAuthenticator.swift +++ b/Networking/Networking/Network/RequestAuthenticator.swift @@ -35,14 +35,13 @@ final class RequestAuthenticator { /// func authenticateRequest(_ request: URLRequestConvertible, completion: @escaping (Swift.Result) -> Void) { guard let jetpackRequest = request as? JetpackRequest, - jetpackRequest.availableAsRESTRequest, let useCase = applicationPasswordUseCase, - case let .wporg(_, _, siteAddress) = credentials else { + case let .wporg(_, _, siteAddress) = credentials, + let restRequest = jetpackRequest.asRESTRequest(with: siteAddress) else { // Handle non-REST requests as before return completion(.success(authenticateUsingWPCOMTokenIfPossible(request))) } - let restRequest = jetpackRequest.createRESTRequest(with: siteAddress) Task(priority: .medium) { let result: Swift.Result do { diff --git a/Networking/Networking/Requests/JetpackRequest.swift b/Networking/Networking/Requests/JetpackRequest.swift index c8ae500b63c..f7a119a766c 100644 --- a/Networking/Networking/Requests/JetpackRequest.swift +++ b/Networking/Networking/Requests/JetpackRequest.swift @@ -36,7 +36,7 @@ struct JetpackRequest: Request { /// Whether this request should be transformed to a REST request if application password is available. /// - let availableAsRESTRequest: Bool + private let availableAsRESTRequest: Bool /// Designated Initializer. @@ -82,8 +82,11 @@ struct JetpackRequest: Request { return DotcomValidator() } - func createRESTRequest(with siteURL: String) -> RESTRequest { - RESTRequest(siteURL: siteURL, method: method, path: path, parameters: parameters) + func asRESTRequest(with siteURL: String) -> RESTRequest? { + guard availableAsRESTRequest else { + return nil + } + return RESTRequest(siteURL: siteURL, method: method, path: path, parameters: parameters) } }