diff --git a/Modules/Sources/NetworkingCore/ApplicationPassword/RequestProcessor.swift b/Modules/Sources/NetworkingCore/ApplicationPassword/RequestProcessor.swift index 7243191216e..ba89a4705f5 100644 --- a/Modules/Sources/NetworkingCore/ApplicationPassword/RequestProcessor.swift +++ b/Modules/Sources/NetworkingCore/ApplicationPassword/RequestProcessor.swift @@ -2,7 +2,7 @@ import Alamofire import Foundation protocol RequestProcessorDelegate: AnyObject { - func didFailToAuthenticateRequestWithAppPassword(siteID: Int64) + func didFailToAuthenticateRequestWithAppPassword(siteID: Int64, error: Error) } /// Authenticates and retries requests @@ -85,7 +85,7 @@ private extension RequestProcessor { generateApplicationPassword() } else { isAuthenticating = false - completeRequests(false) + completeRequests(false, error: error) if let currentSiteID { notifyFailureIfNeeded(error, for: currentSiteID) } @@ -131,7 +131,7 @@ private extension RequestProcessor { } }() if appPasswordNotSupported { - delegate?.didFailToAuthenticateRequestWithAppPassword(siteID: siteID) + delegate?.didFailToAuthenticateRequestWithAppPassword(siteID: siteID, error: error) } } @@ -146,8 +146,16 @@ private extension RequestProcessor { } } - func completeRequests(_ shouldRetry: Bool) { - let result: RetryResult = shouldRetry ? .retryWithDelay(0) : .doNotRetry + func completeRequests(_ shouldRetry: Bool, error: Error? = nil) { + let result: RetryResult = { + if shouldRetry { + .retryWithDelay(0) + } else if let error { + .doNotRetryWithError(error) + } else { + .doNotRetry + } + }() requestsToRetry.forEach { (completion) in completion(result) } @@ -165,4 +173,12 @@ public extension NSNotification.Name { /// Posted when generating an application password fails /// static let ApplicationPasswordsGenerationFailed = NSNotification.Name(rawValue: "ApplicationPasswordsGenerationFailed") + + /// Posted when site is flagged as unsupported for app password + /// + static let JetpackSiteFlaggedUnsupportedForApplicationPassword = NSNotification.Name(rawValue: "JetpackSiteFlaggedUnsupportedForApplicationPassword") + + /// Posted when site is detected as eligible for app password authentication + /// + static let JetpackSiteEligibleForAppPasswordSupport = NSNotification.Name(rawValue: "JetpackSiteEligibleForAppPasswordSupport") } diff --git a/Modules/Sources/NetworkingCore/Network/AlamofireNetwork.swift b/Modules/Sources/NetworkingCore/Network/AlamofireNetwork.swift index 84ae16d39ec..6c0479952f8 100644 --- a/Modules/Sources/NetworkingCore/Network/AlamofireNetwork.swift +++ b/Modules/Sources/NetworkingCore/Network/AlamofireNetwork.swift @@ -309,7 +309,7 @@ private extension AlamofireNetwork { network: self )) requestAuthenticator.delegate = self - errorHandler.resetFailureCount(for: site.siteID) // reset failure count + errorHandler.prepareAppPasswordSupport(for: site.siteID) // reset failure count updateAuthenticationMode(.appPasswordsWithJetpack) } } @@ -336,8 +336,13 @@ private extension AlamofireNetwork { // MARK: `RequestProcessorDelegate` conformance // extension AlamofireNetwork: RequestProcessorDelegate { - func didFailToAuthenticateRequestWithAppPassword(siteID: Int64) { - errorHandler.flagSiteAsUnsupported(for: siteID) + func didFailToAuthenticateRequestWithAppPassword(siteID: Int64, error: Error) { + errorHandler.flagSiteAsUnsupported( + for: siteID, + flow: .appPasswordGeneration, + cause: .majorError, + error: error + ) } } @@ -377,8 +382,8 @@ extension Alamofire.DataResponse { return error } - if case .some(AFError.requestAdaptationFailed) = error?.asAFError { - return error + if case .some(AFError.requestRetryFailed) = error?.asAFError { + return error?.asAFError } return response.flatMap { response in diff --git a/Modules/Sources/NetworkingCore/Network/AlamofireNetworkErrorHandler.swift b/Modules/Sources/NetworkingCore/Network/AlamofireNetworkErrorHandler.swift index 7bf5cb9f094..91d333a7be4 100644 --- a/Modules/Sources/NetworkingCore/Network/AlamofireNetworkErrorHandler.swift +++ b/Modules/Sources/NetworkingCore/Network/AlamofireNetworkErrorHandler.swift @@ -6,13 +6,17 @@ final class AlamofireNetworkErrorHandler { private let queue = DispatchQueue(label: "com.networkingcore.errorhandler", attributes: .concurrent) private let userDefaults: UserDefaults private let credentials: Credentials? + private let notificationCenter: NotificationCenter private var _appPasswordFailures: [Int64: Int] = [:] private var _retriedJetpackRequests: [RetriedJetpackRequest] = [] - init(credentials: Credentials?, userDefaults: UserDefaults = .standard) { + init(credentials: Credentials?, + userDefaults: UserDefaults = .standard, + notificationCenter: NotificationCenter = .default) { self.credentials = credentials self.userDefaults = userDefaults + self.notificationCenter = notificationCenter } // MARK: - Thread-safe property access @@ -41,8 +45,9 @@ final class AlamofireNetworkErrorHandler { // MARK: - Public interface - func resetFailureCount(for siteID: Int64) { + func prepareAppPasswordSupport(for siteID: Int64) { appPasswordFailures.removeValue(forKey: siteID) + notificationCenter.post(name: .JetpackSiteEligibleForAppPasswordSupport, object: siteID) } func shouldRetryJetpackRequest(originalRequest: URLRequestConvertible, @@ -51,13 +56,14 @@ final class AlamofireNetworkErrorHandler { guard let error = failure, let request = originalRequest as? JetpackRequest, convertedRequest is RESTRequest, + let convertedURLRequest = try? convertedRequest.asURLRequest(), case .some(.wpcom) = self.credentials else { return false } let isExpectedError: Bool = { switch error { - case AFError.requestAdaptationFailed: + case AFError.requestRetryFailed: return true case _ as NetworkError: return true @@ -69,6 +75,7 @@ final class AlamofireNetworkErrorHandler { if isExpectedError { let retriedRequest = RetriedJetpackRequest(request: request, error: error) retriedJetpackRequests.append(retriedRequest) + logRequestFailure(request: convertedURLRequest, error: error) return true } return false @@ -86,7 +93,7 @@ final class AlamofireNetworkErrorHandler { guard let index = retriedRequestIndex else { return } - let retriedRequest = retriedJetpackRequests[index] + let retriedRequest = retriedJetpackRequests.remove(at: index) if failure == nil { let siteID = retriedRequest.request.siteID @@ -95,19 +102,27 @@ final class AlamofireNetworkErrorHandler { case NetworkError.unacceptableStatusCode(statusCode: 401, _), NetworkError.unacceptableStatusCode(statusCode: 403, _), NetworkError.unacceptableStatusCode(statusCode: 429, _): - flagSiteAsUnsupported(for: siteID) + flagSiteAsUnsupported( + for: siteID, + flow: .apiRequest, + cause: .majorError, + error: originalFailure + ) default: if let networkError = originalFailure as? NetworkError, let code = networkError.errorCode, AppPasswordConstants.disabledCodes.contains(code) { - flagSiteAsUnsupported(for: siteID) + flagSiteAsUnsupported( + for: siteID, + flow: .apiRequest, + cause: .majorError, + error: originalFailure + ) } else { - incrementFailureCount(for: siteID) + incrementFailureCount(for: siteID, originalFailure: originalFailure) } } } - - retriedJetpackRequests.remove(at: index) } func handleFailureForDirectRequestIfNeeded(originalRequest: URLRequestConvertible, @@ -133,12 +148,24 @@ final class AlamofireNetworkErrorHandler { } } - func flagSiteAsUnsupported(for siteID: Int64) { + func flagSiteAsUnsupported(for siteID: Int64, flow: RequestFlow, cause: AppPasswordFlagCause, error: Error) { queue.sync(flags: .barrier) { var currentList = userDefaults.applicationPasswordUnsupportedList currentList[String(siteID)] = Date() userDefaults.applicationPasswordUnsupportedList = currentList } + + /// Tracks error + let apiErrorCode = (error as? NetworkError)?.errorCode ?? error.localizedDescription + let httpStatusCode = (error as? NetworkError)?.responseCode ?? (error as NSError).code + + let tracksProperties: [String: Any] = [ + TracksProperty.flow.rawValue: flow.rawValue, + TracksProperty.cause.rawValue: cause.rawValue, + TracksProperty.apiErrorCode.rawValue: apiErrorCode, + TracksProperty.httpStatusCode.rawValue: httpStatusCode + ] + notificationCenter.post(name: .JetpackSiteFlaggedUnsupportedForApplicationPassword, object: tracksProperties) } func siteFlaggedAsUnsupported(siteID: Int64, unsupportedList: [String: Date]) -> Bool { @@ -156,13 +183,38 @@ final class AlamofireNetworkErrorHandler { } } +enum RequestFlow: String { + case appPasswordGeneration = "app_password_generation" + case apiRequest = "api_request" +} + +enum AppPasswordFlagCause: String { + case majorError = "major_error" + case generalFailuresThresholdReached = "general_failures_threshold_reached" +} + // MARK: Private helpers private extension AlamofireNetworkErrorHandler { - func incrementFailureCount(for siteID: Int64) { + func incrementFailureCount(for siteID: Int64, originalFailure: Error) { let currentFailureCount = appPasswordFailures[siteID] ?? 0 let updatedCount = currentFailureCount + 1 if updatedCount == AppPasswordConstants.requestFailureThreshold { - flagSiteAsUnsupported(for: siteID) + let flow: RequestFlow + let failure: Error + switch originalFailure { + case AFError.requestRetryFailed(let error, _): + flow = .appPasswordGeneration + failure = error + default: + flow = .apiRequest + failure = originalFailure + } + flagSiteAsUnsupported( + for: siteID, + flow: flow, + cause: .generalFailuresThresholdReached, + error: failure + ) } appPasswordFailures[siteID] = updatedCount } @@ -176,9 +228,46 @@ private extension AlamofireNetworkErrorHandler { } } + func logRequestFailure(request: URLRequest, error: Error) { + let networkError: NetworkError? = { + switch error { + case AFError.requestRetryFailed(let retryError, _): + return (retryError as? NetworkError) + case let networkError as NetworkError: + return networkError + default: + return nil + } + }() + + let siteURL = request.url?.host() ?? "" + let path = request.url?.path(percentEncoded: false) ?? "" + let method = request.httpMethod ?? "" + let apiErrorCode = networkError?.errorCode ?? error.localizedDescription + let httpCode = networkError?.responseCode ?? (error as NSError).code + + DDLogError( + """ + ⛔️ Request failed using Application Passwords for Jetpack Site: + - Site URL: \(siteURL) + - Path: \(path) + - Method: \(method) + - Error: HTTP status code \(httpCode) + - Error message: \(apiErrorCode) + """ + ) + } + enum Constants { static let flagRefreshDuration: Double = 60 * 60 * 24 * 14 // flag can be reset after 14 days. } + + enum TracksProperty: String { + case flow + case cause + case apiErrorCode = "api_error_code" + case httpStatusCode = "http_status_code" + } } /// Helper type to keep track of retried requests with accompanied error struct RetriedJetpackRequest { diff --git a/Modules/Tests/NetworkingTests/ApplicationPassword/RequestProcessorTests.swift b/Modules/Tests/NetworkingTests/ApplicationPassword/RequestProcessorTests.swift index c731194fdd7..7aea2c24ae5 100644 --- a/Modules/Tests/NetworkingTests/ApplicationPassword/RequestProcessorTests.swift +++ b/Modules/Tests/NetworkingTests/ApplicationPassword/RequestProcessorTests.swift @@ -525,7 +525,7 @@ private class MockRequest: Alamofire.DataRequest, @unchecked Sendable { private class MockRequestProcessorDelegate: RequestProcessorDelegate { private(set) var didFailToAuthenticateRequestForSiteID: Int64? - func didFailToAuthenticateRequestWithAppPassword(siteID: Int64) { + func didFailToAuthenticateRequestWithAppPassword(siteID: Int64, error: Error) { didFailToAuthenticateRequestForSiteID = siteID } } diff --git a/Modules/Tests/NetworkingTests/Network/AlamofireNetworkErrorHandlerTests.swift b/Modules/Tests/NetworkingTests/Network/AlamofireNetworkErrorHandlerTests.swift index 384ef26544c..b7f834683b6 100644 --- a/Modules/Tests/NetworkingTests/Network/AlamofireNetworkErrorHandlerTests.swift +++ b/Modules/Tests/NetworkingTests/Network/AlamofireNetworkErrorHandlerTests.swift @@ -6,22 +6,25 @@ import Alamofire final class AlamofireNetworkErrorHandlerTests: XCTestCase { private var userDefaults: UserDefaults! private var errorHandler: AlamofireNetworkErrorHandler! + private var notificationCenter: NotificationCenter! override func setUp() { super.setUp() userDefaults = UserDefaults(suiteName: UUID().uuidString) - errorHandler = AlamofireNetworkErrorHandler(credentials: createWPComCredentials(), userDefaults: userDefaults) + notificationCenter = NotificationCenter() + errorHandler = AlamofireNetworkErrorHandler(credentials: createWPComCredentials(), userDefaults: userDefaults, notificationCenter: notificationCenter) } override func tearDown() { userDefaults = nil errorHandler = nil + notificationCenter = nil super.tearDown() } // MARK: - Basic Functionality Tests - func test_resetFailureCount_removes_failure_count_for_site() { + func test_prepareAppPasswordSupport_removes_failure_count_for_site() { // Given let siteID: Int64 = 123 @@ -29,7 +32,7 @@ final class AlamofireNetworkErrorHandlerTests: XCTestCase { simulateFailureCount(5, for: siteID) // When - errorHandler.resetFailureCount(for: siteID) + errorHandler.prepareAppPasswordSupport(for: siteID) // Then - should not flag site as unsupported even after more failures simulateFailureCount(5, for: siteID) // Would normally reach threshold @@ -38,7 +41,7 @@ final class AlamofireNetworkErrorHandlerTests: XCTestCase { func test_shouldRetryJetpackRequest_returns_false_for_nil_credentials() { // Given - let errorHandlerWithNilCredentials = AlamofireNetworkErrorHandler(credentials: nil, userDefaults: userDefaults) + let errorHandlerWithNilCredentials = AlamofireNetworkErrorHandler(credentials: nil, userDefaults: userDefaults, notificationCenter: notificationCenter) let jetpackRequest = createJetpackRequest(siteID: 123) let restRequest = createRESTRequest() let error = createNetworkError() @@ -57,7 +60,7 @@ final class AlamofireNetworkErrorHandlerTests: XCTestCase { func test_shouldRetryJetpackRequest_returns_false_for_non_wpcom_credentials() { // Given let wporgCredentials = Credentials.wporg(username: "user", password: "pass", siteAddress: "https://example.com") - let errorHandlerWithWporg = AlamofireNetworkErrorHandler(credentials: wporgCredentials, userDefaults: userDefaults) + let errorHandlerWithWporg = AlamofireNetworkErrorHandler(credentials: wporgCredentials, userDefaults: userDefaults, notificationCenter: notificationCenter) let jetpackRequest = createJetpackRequest(siteID: 123) let restRequest = createRESTRequest() let error = createNetworkError() @@ -79,7 +82,10 @@ final class AlamofireNetworkErrorHandlerTests: XCTestCase { let restRequest = createRESTRequest() let testCases: [Error] = [ - AFError.requestAdaptationFailed(error: NSError(domain: "test", code: 1)), + AFError.requestRetryFailed( + retryError: createNetworkError(), + originalError: AFError.requestAdaptationFailed(error: NSError(domain: "test", code: 0)) + ), createNetworkError() ] @@ -119,7 +125,7 @@ final class AlamofireNetworkErrorHandlerTests: XCTestCase { XCTAssertTrue(userDefaults.applicationPasswordUnsupportedList.isEmpty) // When - errorHandler.flagSiteAsUnsupported(for: siteID) + errorHandler.flagSiteAsUnsupported(for: siteID, flow: .apiRequest, cause: .majorError, error: NetworkError.notFound(response: nil)) // Then XCTAssertTrue(userDefaults.applicationPasswordUnsupportedList.keys.contains(String(siteID))) @@ -132,16 +138,76 @@ final class AlamofireNetworkErrorHandlerTests: XCTestCase { userDefaults.applicationPasswordUnsupportedList = [String(existingSiteID): Date()] // When - errorHandler.flagSiteAsUnsupported(for: newSiteID) + errorHandler.flagSiteAsUnsupported(for: newSiteID, flow: .apiRequest, cause: .majorError, error: NetworkError.notFound(response: nil)) // Then XCTAssertTrue(userDefaults.applicationPasswordUnsupportedList.keys.contains(String(existingSiteID))) XCTAssertTrue(userDefaults.applicationPasswordUnsupportedList.keys.contains(String(newSiteID))) } + // MARK: - Notification Tests + + func test_prepareAppPasswordSupport_posts_eligible_notification() { + // Given + let siteID: Int64 = 123 + var receivedNotifications: [Notification] = [] + + let observer = notificationCenter.addObserver( + forName: .JetpackSiteEligibleForAppPasswordSupport, + object: nil, + queue: nil + ) { notification in + receivedNotifications.append(notification) + } + + // When + errorHandler.prepareAppPasswordSupport(for: siteID) + + // Then + XCTAssertEqual(receivedNotifications.count, 1) + XCTAssertEqual(receivedNotifications.first?.name, .JetpackSiteEligibleForAppPasswordSupport) + XCTAssertEqual(receivedNotifications.first?.object as? Int64, siteID) + + notificationCenter.removeObserver(observer) + } + + func test_flagSiteAsUnsupported_posts_flagged_notification_with_properties() { + // Given + let siteID: Int64 = 456 + let error = NetworkError.unacceptableStatusCode(statusCode: 401, response: Data()) + var receivedNotifications: [Notification] = [] + + let observer = notificationCenter.addObserver( + forName: .JetpackSiteFlaggedUnsupportedForApplicationPassword, + object: nil, + queue: nil + ) { notification in + receivedNotifications.append(notification) + } + + // When + errorHandler.flagSiteAsUnsupported(for: siteID, flow: .apiRequest, cause: .majorError, error: error) + + // Then + XCTAssertEqual(receivedNotifications.count, 1) + XCTAssertEqual(receivedNotifications.first?.name, .JetpackSiteFlaggedUnsupportedForApplicationPassword) + + guard let tracksProperties = receivedNotifications.first?.object as? [String: Any] else { + XCTFail("Expected tracks properties dictionary") + return + } + + XCTAssertEqual(tracksProperties["flow"] as? String, "api_request") + XCTAssertEqual(tracksProperties["cause"] as? String, "major_error") + XCTAssertEqual(tracksProperties["http_status_code"] as? Int, 401) + XCTAssertNotNil(tracksProperties["api_error_code"]) + + notificationCenter.removeObserver(observer) + } + // MARK: - Thread Safety Tests - func test_concurrent_resetFailureCount_operations_are_thread_safe() { + func test_concurrent_prepareAppPasswordSupport_operations_are_thread_safe() { let expectation = XCTestExpectation(description: "All reset operations complete") let operationCount = 3 let siteIDs = Array(1...3).map { Int64($0) } @@ -152,7 +218,7 @@ final class AlamofireNetworkErrorHandlerTests: XCTestCase { for i in 0.. = [] + + /// Network switching notification observers + /// + private var networkNotificationObservers: [NSObjectProtocol]? + /// SessionManager: Persistent Storage for Session-Y Properties. /// This property is thread safe private(set) var sessionManager: SessionManagerProtocol { @@ -148,6 +156,9 @@ class DefaultStoresManager: StoresManager { self.cardPresentPaymentOnboardingStateCache = cardPresentPaymentOnboardingStateCache isLoggedIn = isAuthenticated + if isLoggedIn, case .some(.wpcom) = sessionManager.defaultCredentials { + startObservingNetworkNotifications() + } } /// This should only be invoked after all the ServiceLocator dependencies in this function are initialized to avoid circular reference. @@ -182,9 +193,11 @@ class DefaultStoresManager: StoresManager { if case .wpcom = credentials { listenToWPCOMInvalidWPCOMTokenNotification() applicationPasswordGenerationFailureObserver = nil + startObservingNetworkNotifications() } else { listenToApplicationPasswordGenerationFailureNotification() invalidWPCOMTokenNotificationObserver = nil + stopObservingNetworkNotifications() } return self @@ -240,10 +253,7 @@ class DefaultStoresManager: StoresManager { /// Prepares for changing the selected store and remains Authenticated. /// func removeDefaultStore() { - /// In case of store switching, new password might be saved locally before the deletion of old password is done. - /// Here we delete the password remotely only to avoid the race condition - /// when the new password is removed from the local storage by mistake. - sessionManager.deleteApplicationPassword(locally: false) + sessionManager.deleteApplicationPassword(locally: true) ServiceLocator.analytics.refreshUserData() ZendeskProvider.shared.reset() ServiceLocator.pushNotesManager.unregisterForRemoteNotifications() @@ -268,6 +278,8 @@ class DefaultStoresManager: StoresManager { func deauthenticate() -> StoresManager { applicationPasswordGenerationFailureObserver = nil invalidWPCOMTokenNotificationObserver = nil + stopObservingNetworkNotifications() + trackedEligibleSites.removeAll() if isAuthenticated { let resetAction = CardPresentPaymentAction.reset @@ -826,6 +838,62 @@ private extension DefaultStoresManager { systemPlugins: systemPlugins) } } + + /// Sets up network switching notification observers + /// + func startObservingNetworkNotifications() { + let eligibleSiteObserver = notificationCenter.addObserver( + forName: .JetpackSiteEligibleForAppPasswordSupport, + object: nil, + queue: .main) { [weak self] note in + self?.trackJetpackSiteEligible(note: note) + } + + let siteFlaggedObserver = notificationCenter.addObserver( + forName: .JetpackSiteFlaggedUnsupportedForApplicationPassword, + object: nil, + queue: .main) { [weak self] note in + self?.trackJetpackSiteFlagged(note: note) + } + + networkNotificationObservers = [eligibleSiteObserver, siteFlaggedObserver] + } + + /// Removes network switching notification observers + /// + func stopObservingNetworkNotifications() { + networkNotificationObservers?.forEach { observer in + notificationCenter.removeObserver(observer) + } + networkNotificationObservers = nil + } + + /// Tracks Jetpack site eligible for app password support with deduplication + /// + func trackJetpackSiteEligible(note: Notification) { + guard let siteID = note.object as? Int64, + sessionManager.defaultSite?.siteID == siteID else { + return + } + // Only track if we haven't already tracked this site + if !trackedEligibleSites.contains(siteID) { + trackedEligibleSites.insert(siteID) + ServiceLocator.analytics.track(.jetpackSiteEligibleForAppPasswordSupport) + } + } + + /// Tracks Jetpack site flagged as unsupported and removes from eligible tracking + /// + private func trackJetpackSiteFlagged(note: Notification) { + guard let properties = note.object as? [String: Any] else { + return + } + // Get the current site ID and remove from tracked sites + if let siteID = sessionManager.defaultStoreID { + trackedEligibleSites.remove(siteID) + } + ServiceLocator.analytics.track(.jetpackSiteFlaggedUnsupportedForAppPasswords, withProperties: properties) + } } diff --git a/WooCommerce/WooCommerceTests/Yosemite/StoresManagerTests.swift b/WooCommerce/WooCommerceTests/Yosemite/StoresManagerTests.swift index 23463a1f8a0..a56457d6f4f 100644 --- a/WooCommerce/WooCommerceTests/Yosemite/StoresManagerTests.swift +++ b/WooCommerce/WooCommerceTests/Yosemite/StoresManagerTests.swift @@ -332,7 +332,7 @@ final class StoresManagerTests: XCTestCase { // Then XCTAssertTrue(mockSessionManager.deleteApplicationPasswordInvoked) - XCTAssertFalse(mockSessionManager.deleteApplicationPasswordLocally) + XCTAssertTrue(mockSessionManager.deleteApplicationPasswordLocally) } func test_updating_default_storeID_sets_storePhoneNumber_to_nil() throws {