Skip to content

Commit 52dd59b

Browse files
itsmeichigoitsmeichigo
committed
Set threshold for app password generation failures
1 parent 58d8adb commit 52dd59b

File tree

4 files changed

+74
-34
lines changed

4 files changed

+74
-34
lines changed

‎Modules/Sources/NetworkingCore/ApplicationPassword/RequestProcessor.swift‎

Lines changed: 31 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,13 @@
11
import Alamofire
22
import Foundation
33

4+
enum AppPasswordFailureReason {
5+
case notSupported
6+
case unknown
7+
}
8+
49
protocol RequestProcessorDelegate: AnyObject {
5-
func didFailToAuthenticateRequestWithApplicationPassword(siteID: Int64)
10+
func didFailToAuthenticateRequestWithAppPassword(siteID: Int64, reason: AppPasswordFailureReason)
611
}
712

813
/// Authenticates and retries requests
@@ -80,12 +85,15 @@ private extension RequestProcessor {
8085
// Post a notification for tracking
8186
notificationCenter.post(name: .ApplicationPasswordsGenerationFailed, object: error, userInfo: nil)
8287

83-
let shouldRetry = await checkIfRetryingGenerationIsNeeded(error: error)
88+
let shouldRetry = await checkIfRetryingGenerationIsNeeded(for: error)
8489
if shouldRetry {
8590
generateApplicationPassword()
8691
} else {
8792
isAuthenticating = false
8893
completeRequests(false)
94+
if let currentSiteID {
95+
notifyFailure(error, for: currentSiteID)
96+
}
8997
}
9098
}
9199
}
@@ -94,8 +102,8 @@ private extension RequestProcessor {
94102
/// Checks error code to retry or mark site as unsupported for app password.
95103
/// Returns whether retry is needed.
96104
@MainActor
97-
func checkIfRetryingGenerationIsNeeded(error: Error) async -> Bool {
98-
guard let currentSiteID else {
105+
func checkIfRetryingGenerationIsNeeded(for error: Error) async -> Bool {
106+
guard currentSiteID != nil else {
99107
return false
100108
}
101109
switch error {
@@ -107,21 +115,30 @@ private extension RequestProcessor {
107115
} catch {
108116
return false
109117
}
110-
case NetworkError.notFound:
111-
/// Site doesn't support application password
112-
delegate?.didFailToAuthenticateRequestWithApplicationPassword(siteID: currentSiteID)
113-
return false
114-
case let networkError as NetworkError:
115-
if let code = networkError.errorCode,
116-
Constants.disabledCodes.contains(code) {
117-
delegate?.didFailToAuthenticateRequestWithApplicationPassword(siteID: currentSiteID)
118-
}
119-
return false
120118
default:
121119
return false
122120
}
123121
}
124122

123+
func notifyFailure(_ error: Error, for siteID: Int64) {
124+
let reason: AppPasswordFailureReason = {
125+
switch error {
126+
case NetworkError.notFound:
127+
return .notSupported
128+
case let networkError as NetworkError:
129+
if let code = networkError.errorCode,
130+
AppPasswordConstants.disabledCodes.contains(code) {
131+
return .notSupported
132+
}
133+
return .unknown
134+
default:
135+
return .unknown
136+
}
137+
}()
138+
139+
delegate?.didFailToAuthenticateRequestWithAppPassword(siteID: siteID, reason: reason)
140+
}
141+
125142
func shouldRetry(_ error: Error) -> Bool {
126143
switch error {
127144
case RequestAuthenticatorError.applicationPasswordNotAvailable,
@@ -142,15 +159,6 @@ private extension RequestProcessor {
142159
}
143160
}
144161

145-
private extension RequestProcessor {
146-
enum Constants {
147-
static let disabledCodes = [
148-
"application_passwords_disabled",
149-
"application_passwords_disabled_for_user"
150-
]
151-
}
152-
}
153-
154162
// MARK: - Application Password Notifications
155163
//
156164
public extension NSNotification.Name {

‎Modules/Sources/NetworkingCore/Network/AlamofireNetwork.swift‎

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@ public class AlamofireNetwork: Network {
4848

4949
private var subscription: AnyCancellable?
5050

51+
/// Keeps track of failure counts for each site when switching to direct requests
52+
private var appPasswordFailures: [Int64: Int] = [:]
53+
5154
/// Public Initializer
5255
///
5356
///
@@ -215,19 +218,41 @@ private extension AlamofireNetwork {
215218
network: self
216219
))
217220
requestAuthenticator.delegate = self
221+
appPasswordFailures.removeValue(forKey: site.siteID) // reset failure count
218222
}
219223
}
220224
}
221225

222226
// MARK: `RequestProcessorDelegate` conformance
223227
//
224228
extension AlamofireNetwork: RequestProcessorDelegate {
225-
func didFailToAuthenticateRequestWithApplicationPassword(siteID: Int64) {
226-
let currentList = userDefaults.applicationPasswordUnsupportedList
227-
userDefaults.applicationPasswordUnsupportedList = currentList + [siteID]
229+
func didFailToAuthenticateRequestWithAppPassword(siteID: Int64, reason: AppPasswordFailureReason) {
230+
switch reason {
231+
case .notSupported:
232+
let currentList = userDefaults.applicationPasswordUnsupportedList
233+
userDefaults.applicationPasswordUnsupportedList = currentList + [siteID]
234+
case .unknown:
235+
let currentFailureCount = appPasswordFailures[siteID] ?? 0
236+
let updatedCount = currentFailureCount + 1
237+
if updatedCount == AppPasswordConstants.requestFailureThreshold {
238+
let currentList = userDefaults.applicationPasswordUnsupportedList
239+
userDefaults.applicationPasswordUnsupportedList = currentList + [siteID]
240+
}
241+
appPasswordFailures[siteID] = updatedCount
242+
}
228243
}
229244
}
230245

246+
// MARK: - Constants for direct request error handling
247+
enum AppPasswordConstants {
248+
// flag site as disabled after threshold is reached
249+
static let requestFailureThreshold = 10
250+
static let disabledCodes = [
251+
"application_passwords_disabled",
252+
"application_passwords_disabled_for_user"
253+
]
254+
}
255+
231256
private extension DataRequest {
232257
/// Validates only for `RESTRequest`
233258
///
@@ -279,7 +304,6 @@ extension UserDefaults {
279304
}
280305

281306
enum Key: String {
282-
/// This key is shared with the UI layer, so ensure to keep it in-sync with `UserDefaults+Woo`.
283307
case applicationPasswordUnsupportedList
284308
}
285309
}

‎Modules/Sources/NetworkingCore/Network/NetworkError.swift‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,6 @@ extension NetworkError: CustomStringConvertible {
134134

135135
struct NetworkErrorResponse: Decodable {
136136
let code: String?
137-
let message: String?
138137

139138
init(from decoder: Decoder) throws {
140139
let container = try decoder.container(keyedBy: CodingKeys.self)
@@ -145,14 +144,12 @@ struct NetworkErrorResponse: Decodable {
145144
}
146145
return try container.decodeIfPresent(String.self, forKey: .code)
147146
}()
148-
self.message = try container.decodeIfPresent(String.self, forKey: .message)
149147
}
150148

151149
/// Coding Keys
152150
///
153151
private enum CodingKeys: String, CodingKey {
154152
case error
155153
case code
156-
case message
157154
}
158155
}

‎Modules/Tests/NetworkingTests/ApplicationPassword/RequestProcessorTests.swift‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,8 @@ final class RequestProcessorTests: XCTestCase {
322322
// Then
323323
XCTAssertFalse(shouldRetry.retryRequired)
324324
waitUntil {
325-
mockDelegate.didFailToAuthenticateRequestForSiteID == 123
325+
mockDelegate.didFailToAuthenticateRequestForSiteID == 123 &&
326+
mockDelegate.didFailToAuthenticateRequestWithReason == .notSupported
326327
}
327328
}
328329

@@ -354,7 +355,8 @@ final class RequestProcessorTests: XCTestCase {
354355
// Then
355356
XCTAssertFalse(shouldRetry.retryRequired)
356357
waitUntil {
357-
mockDelegate.didFailToAuthenticateRequestForSiteID == 123
358+
mockDelegate.didFailToAuthenticateRequestForSiteID == 123 &&
359+
mockDelegate.didFailToAuthenticateRequestWithReason == .notSupported
358360
}
359361
}
360362

@@ -386,7 +388,8 @@ final class RequestProcessorTests: XCTestCase {
386388
// Then
387389
XCTAssertFalse(shouldRetry.retryRequired)
388390
waitUntil {
389-
mockDelegate.didFailToAuthenticateRequestForSiteID == 123
391+
mockDelegate.didFailToAuthenticateRequestForSiteID == 123 &&
392+
mockDelegate.didFailToAuthenticateRequestWithReason == .notSupported
390393
}
391394
}
392395

@@ -395,10 +398,12 @@ final class RequestProcessorTests: XCTestCase {
395398
let session = Alamofire.Session(configuration: URLSessionConfiguration.default)
396399
let request = try mockRequest()
397400
let wpcomCredentials = Networking.Credentials.wpcom(username: "test", authToken: "token", siteAddress: "test.com")
401+
let mockDelegate = MockRequestProcessorDelegate()
398402

399403
mockRequestAuthenticator.credentials = wpcomCredentials
400404
mockRequestAuthenticator.jetpackSiteID = 123
401405
sut.updateAuthenticator(mockRequestAuthenticator)
406+
sut.delegate = mockDelegate
402407

403408
let genericError = NSError(domain: "TestError", code: 500, userInfo: nil)
404409
mockRequestAuthenticator.mockErrorWhileGeneratingPassword = genericError
@@ -414,6 +419,10 @@ final class RequestProcessorTests: XCTestCase {
414419
// Then
415420
XCTAssertFalse(shouldRetry.retryRequired)
416421
XCTAssertFalse(mockRequestAuthenticator.deleteApplicationPasswordCalled)
422+
waitUntil {
423+
mockDelegate.didFailToAuthenticateRequestForSiteID == 123 &&
424+
mockDelegate.didFailToAuthenticateRequestWithReason == .unknown
425+
}
417426
}
418427

419428
func test_request_not_retried_when_password_deletion_fails_after_409_error_with_wpcom_authentication() throws {
@@ -522,8 +531,10 @@ private class MockRequest: Alamofire.DataRequest, @unchecked Sendable {
522531

523532
private class MockRequestProcessorDelegate: RequestProcessorDelegate {
524533
private(set) var didFailToAuthenticateRequestForSiteID: Int64?
534+
private(set) var didFailToAuthenticateRequestWithReason: AppPasswordFailureReason?
525535

526-
func didFailToAuthenticateRequestWithApplicationPassword(siteID: Int64) {
536+
func didFailToAuthenticateRequestWithAppPassword(siteID: Int64, reason: AppPasswordFailureReason) {
527537
didFailToAuthenticateRequestForSiteID = siteID
538+
didFailToAuthenticateRequestWithReason = reason
528539
}
529540
}

0 commit comments

Comments
 (0)