From b170325b24fda857e19c31e3bcc87bcca45589f5 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Thu, 8 Aug 2024 12:05:56 -0400 Subject: [PATCH 1/7] [Auth] Fix async/await crash from implicitly unwrapped nil error --- .../Sources/Swift/SystemService/AuthAPNSTokenManager.swift | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/FirebaseAuth/Sources/Swift/SystemService/AuthAPNSTokenManager.swift b/FirebaseAuth/Sources/Swift/SystemService/AuthAPNSTokenManager.swift index fdbe5c6bb74..9446fb17ffa 100644 --- a/FirebaseAuth/Sources/Swift/SystemService/AuthAPNSTokenManager.swift +++ b/FirebaseAuth/Sources/Swift/SystemService/AuthAPNSTokenManager.swift @@ -68,7 +68,10 @@ kAuthGlobalWorkQueue.asyncAfter(deadline: deadline) { // Only cancel if the pending callbacks remain the same, i.e., not triggered yet. if applicableCallbacks.count == self.pendingCallbacks.count { - self.callback(withToken: nil, error: nil) + self.callback( + withToken: nil, + error: AuthErrorUtils.missingAppTokenError(underlyingError: nil) + ) } } } From b34be0ec1becf81d18f50b9db616d2612fb9fd61 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Thu, 8 Aug 2024 12:17:02 -0400 Subject: [PATCH 2/7] Add changelog --- FirebaseAuth/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/FirebaseAuth/CHANGELOG.md b/FirebaseAuth/CHANGELOG.md index 66a0b4101eb..8abfc7b6cb0 100644 --- a/FirebaseAuth/CHANGELOG.md +++ b/FirebaseAuth/CHANGELOG.md @@ -3,6 +3,8 @@ - [added] Added custom provider support to `AuthProviderID`. Note that this change will be breaking to any code that implemented an exhaustive `switch` on `AuthProviderID` in 11.0.0 - the `switch` will need expansion. (#13429) +- [fixed] Fix crash in phone authentication flow from implicitly unwrapping + `nil` error. (#13470) # 11.0.0 - [fixed] Fixed auth domain matching code to prioritize matching `firebaseapp.com` over `web.app` From 7208398411cc46dd3af2a17d539d243fbe28b7e7 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Thu, 8 Aug 2024 12:46:49 -0400 Subject: [PATCH 3/7] Migrate to using result type internally --- .../SystemService/AuthAPNSTokenManager.swift | 28 ++++----- .../Unit/AuthAPNSTokenManagerTests.swift | 59 +++++++++++++------ .../Tests/Unit/PhoneAuthProviderTests.swift | 4 +- 3 files changed, 56 insertions(+), 35 deletions(-) diff --git a/FirebaseAuth/Sources/Swift/SystemService/AuthAPNSTokenManager.swift b/FirebaseAuth/Sources/Swift/SystemService/AuthAPNSTokenManager.swift index 9446fb17ffa..49521cae813 100644 --- a/FirebaseAuth/Sources/Swift/SystemService/AuthAPNSTokenManager.swift +++ b/FirebaseAuth/Sources/Swift/SystemService/AuthAPNSTokenManager.swift @@ -49,9 +49,9 @@ /// token becomes available, or when timeout occurs, whichever happens earlier. /// /// This function is internal to make visible for tests. - func getTokenInternal(callback: @escaping (AuthAPNSToken?, Error?) -> Void) { + func getTokenInternal(callback: @escaping (Result) -> Void) { if let token = tokenStore { - callback(token, nil) + callback(.success(token)) return } if pendingCallbacks.count > 0 { @@ -68,21 +68,19 @@ kAuthGlobalWorkQueue.asyncAfter(deadline: deadline) { // Only cancel if the pending callbacks remain the same, i.e., not triggered yet. if applicableCallbacks.count == self.pendingCallbacks.count { - self.callback( - withToken: nil, - error: AuthErrorUtils.missingAppTokenError(underlyingError: nil) - ) + self.callback(.failure(AuthErrorUtils.missingAppTokenError(underlyingError: nil))) } } } func getToken() async throws -> AuthAPNSToken { return try await withCheckedThrowingContinuation { continuation in - self.getTokenInternal { token, error in - if let token { + self.getTokenInternal { result in + switch result { + case let .success(token): continuation.resume(returning: token) - } else { - continuation.resume(throwing: error!) + case let .failure(error): + continuation.resume(throwing: error) } } } @@ -108,7 +106,7 @@ newToken = AuthAPNSToken(withData: setToken.data, type: detectedTokenType) } tokenStore = newToken - callback(withToken: newToken, error: nil) + callback(.success(newToken)) } } @@ -118,18 +116,18 @@ /// Cancels any pending `getTokenWithCallback:` request. /// - Parameter error: The error to return . func cancel(withError error: Error) { - callback(withToken: nil, error: error) + callback(.failure(error)) } /// Enable unit test faking. var application: AuthAPNSTokenApplication - private var pendingCallbacks: [(AuthAPNSToken?, Error?) -> Void] = [] + private var pendingCallbacks: [(Result) -> Void] = [] - private func callback(withToken token: AuthAPNSToken?, error: Error?) { + private func callback(_ result: Result) { let pendingCallbacks = self.pendingCallbacks self.pendingCallbacks = [] for callback in pendingCallbacks { - callback(token, error) + callback(result) } } diff --git a/FirebaseAuth/Tests/Unit/AuthAPNSTokenManagerTests.swift b/FirebaseAuth/Tests/Unit/AuthAPNSTokenManagerTests.swift index 585cd217b98..3406149ee90 100644 --- a/FirebaseAuth/Tests/Unit/AuthAPNSTokenManagerTests.swift +++ b/FirebaseAuth/Tests/Unit/AuthAPNSTokenManagerTests.swift @@ -63,21 +63,29 @@ XCTAssertFalse(fakeApplication!.registerCalled) var firstCallbackCalled = false let manager = try XCTUnwrap(manager) - manager.getTokenInternal { token, error in + manager.getTokenInternal { result in firstCallbackCalled = true - XCTAssertEqual(token?.data, self.data) - XCTAssertEqual(token?.type, .sandbox) - XCTAssertNil(error) + switch result { + case let .success(token): + XCTAssertEqual(token.data, self.data) + XCTAssertEqual(token.type, .sandbox) + case let .failure(error): + XCTFail("Unexpected error: \(error)") + } } XCTAssertFalse(firstCallbackCalled) // Add second callback, which is yet to be called either. var secondCallbackCalled = false - manager.getTokenInternal { token, error in + manager.getTokenInternal { result in secondCallbackCalled = true - XCTAssertEqual(token?.data, self.data) - XCTAssertEqual(token?.type, .sandbox) - XCTAssertNil(error) + switch result { + case let .success(token): + XCTAssertEqual(token.data, self.data) + XCTAssertEqual(token.type, .sandbox) + case let .failure(error): + XCTFail("Unexpected error: \(error)") + } } XCTAssertFalse(secondCallbackCalled) @@ -96,11 +104,15 @@ // Add third callback, which should be called back immediately. var thirdCallbackCalled = false - manager.getTokenInternal { token, error in + manager.getTokenInternal { result in thirdCallbackCalled = true - XCTAssertEqual(token?.data, self.data) - XCTAssertEqual(token?.type, .sandbox) - XCTAssertNil(error) + switch result { + case let .success(token): + XCTAssertEqual(token.data, self.data) + XCTAssertEqual(token.type, .sandbox) + case let .failure(error): + XCTFail("Unexpected error: \(error)") + } } XCTAssertTrue(thirdCallbackCalled) @@ -123,9 +135,16 @@ // Add callback to time out. let expectation = self.expectation(description: #function) - manager.getTokenInternal { token, error in - XCTAssertNil(token) - XCTAssertNil(error) + manager.getTokenInternal { result in + switch result { + case let .success(token): + XCTFail("Unexpected success: \(token)") + case let .failure(error): + XCTAssertEqual( + error as NSError, + AuthErrorUtils.missingAppTokenError(underlyingError: nil) as NSError + ) + } expectation.fulfill() } // Time out. @@ -153,9 +172,13 @@ // Add callback to cancel. var callbackCalled = false - manager.getTokenInternal { token, error in - XCTAssertNil(token) - XCTAssertEqual(error as? NSError, self.error as NSError) + manager.getTokenInternal { result in + switch result { + case let .success(token): + XCTFail("Unexpected success: \(token)") + case let .failure(error): + XCTAssertEqual(error as NSError, self.error as NSError) + } XCTAssertFalse(callbackCalled) // verify callback is not called twice callbackCalled = true } diff --git a/FirebaseAuth/Tests/Unit/PhoneAuthProviderTests.swift b/FirebaseAuth/Tests/Unit/PhoneAuthProviderTests.swift index 9b1e1caaf30..0030c52776d 100644 --- a/FirebaseAuth/Tests/Unit/PhoneAuthProviderTests.swift +++ b/FirebaseAuth/Tests/Unit/PhoneAuthProviderTests.swift @@ -690,9 +690,9 @@ } class FakeTokenManager: AuthAPNSTokenManager { - override func getTokenInternal(callback: @escaping (AuthAPNSToken?, Error?) -> Void) { + override func getTokenInternal(callback: @escaping (Result) -> Void) { let error = NSError(domain: "dummy domain", code: AuthErrorCode.missingAppToken.rawValue) - callback(nil, error) + callback(.failure(error)) } } From d0acdb85e5509a18ff318f68e40050187814c537 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Thu, 8 Aug 2024 14:32:57 -0400 Subject: [PATCH 4/7] Review --- FirebaseAuth/CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/FirebaseAuth/CHANGELOG.md b/FirebaseAuth/CHANGELOG.md index 8abfc7b6cb0..3c269a384d0 100644 --- a/FirebaseAuth/CHANGELOG.md +++ b/FirebaseAuth/CHANGELOG.md @@ -3,8 +3,8 @@ - [added] Added custom provider support to `AuthProviderID`. Note that this change will be breaking to any code that implemented an exhaustive `switch` on `AuthProviderID` in 11.0.0 - the `switch` will need expansion. (#13429) -- [fixed] Fix crash in phone authentication flow from implicitly unwrapping - `nil` error. (#13470) +- [fixed] Fix crash introduced in 11.0.0 in phone authentication flow from implicitly unwrapping + `nil` error after a token timeout. (#13470) # 11.0.0 - [fixed] Fixed auth domain matching code to prioritize matching `firebaseapp.com` over `web.app` From 187f06ff7440dfe8de36f81d23b46cf78cae6e6a Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Thu, 8 Aug 2024 14:33:53 -0400 Subject: [PATCH 5/7] Style changelog --- FirebaseAuth/CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/FirebaseAuth/CHANGELOG.md b/FirebaseAuth/CHANGELOG.md index 3c269a384d0..0f6d994d704 100644 --- a/FirebaseAuth/CHANGELOG.md +++ b/FirebaseAuth/CHANGELOG.md @@ -3,8 +3,8 @@ - [added] Added custom provider support to `AuthProviderID`. Note that this change will be breaking to any code that implemented an exhaustive `switch` on `AuthProviderID` in 11.0.0 - the `switch` will need expansion. (#13429) -- [fixed] Fix crash introduced in 11.0.0 in phone authentication flow from implicitly unwrapping - `nil` error after a token timeout. (#13470) +- [fixed] Fix crash introduced in 11.0.0 in phone authentication flow from + implicitly unwrapping `nil` error after a token timeout. (#13470) # 11.0.0 - [fixed] Fixed auth domain matching code to prioritize matching `firebaseapp.com` over `web.app` From fb5b0b57766834a61b1e933ae4ebd57c1b4fc5bc Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Thu, 8 Aug 2024 14:40:33 -0400 Subject: [PATCH 6/7] Fix sample app --- .../ViewControllers/AuthViewController.swift | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/FirebaseAuth/Tests/SampleSwift/AuthenticationExample/ViewControllers/AuthViewController.swift b/FirebaseAuth/Tests/SampleSwift/AuthenticationExample/ViewControllers/AuthViewController.swift index 78b0295edb8..145fb7e9d7b 100644 --- a/FirebaseAuth/Tests/SampleSwift/AuthenticationExample/ViewControllers/AuthViewController.swift +++ b/FirebaseAuth/Tests/SampleSwift/AuthenticationExample/ViewControllers/AuthViewController.swift @@ -468,14 +468,14 @@ class AuthViewController: UIViewController, DataSourceProviderDelegate { } private func verifyClient() { - AppManager.shared.auth().tokenManager.getTokenInternal { token, error in - if token == nil { + AppManager.shared.auth().tokenManager.getTokenInternal { result in + guard case .success(let token) = result else { print("Verify iOS Client failed.") return } let request = VerifyClientRequest( - withAppToken: token?.string, - isSandbox: token?.type == .sandbox, + withAppToken: token.string, + isSandbox: token.type == .sandbox, requestConfiguration: AppManager.shared.auth().requestConfiguration ) From 134879a0ade7ee503b802b5fe6589a9acc022cbe Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Thu, 8 Aug 2024 14:42:24 -0400 Subject: [PATCH 7/7] Style --- .../ViewControllers/AuthViewController.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/FirebaseAuth/Tests/SampleSwift/AuthenticationExample/ViewControllers/AuthViewController.swift b/FirebaseAuth/Tests/SampleSwift/AuthenticationExample/ViewControllers/AuthViewController.swift index 145fb7e9d7b..fdde2e0757d 100644 --- a/FirebaseAuth/Tests/SampleSwift/AuthenticationExample/ViewControllers/AuthViewController.swift +++ b/FirebaseAuth/Tests/SampleSwift/AuthenticationExample/ViewControllers/AuthViewController.swift @@ -469,7 +469,7 @@ class AuthViewController: UIViewController, DataSourceProviderDelegate { private func verifyClient() { AppManager.shared.auth().tokenManager.getTokenInternal { result in - guard case .success(let token) = result else { + guard case let .success(token) = result else { print("Verify iOS Client failed.") return }