From 11f3c690a228a3c3bd3b3df7226dff2f73cee1ec Mon Sep 17 00:00:00 2001 From: Nick Cooke <36927374+ncooke3@users.noreply.github.com> Date: Thu, 5 Sep 2024 10:49:30 -0400 Subject: [PATCH 1/7] [Auth] Forward secure coding calls for TOTPMultiFactorInfo --- .../Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/FirebaseAuth/Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift b/FirebaseAuth/Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift index 2273b2aea9e..a035c096dd2 100644 --- a/FirebaseAuth/Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift +++ b/FirebaseAuth/Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift @@ -34,7 +34,13 @@ import Foundation @available(*, unavailable) required init?(coder: NSCoder) { - fatalError("init(coder:) has not been implemented") + totpInfo = nil + super.init(coder: coder) + } + + @available(*, unavailable) + override class var supportsSecureCoding: Bool { + super.supportsSecureCoding } } #endif From a5857a102dc9faaa74163aa05f71a966fdbe4b05 Mon Sep 17 00:00:00 2001 From: Nick Cooke <36927374+ncooke3@users.noreply.github.com> Date: Thu, 5 Sep 2024 12:28:29 -0400 Subject: [PATCH 2/7] Add regression test --- FirebaseAuth/Tests/Unit/UserTests.swift | 32 +++++++++++++++++-------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/FirebaseAuth/Tests/Unit/UserTests.swift b/FirebaseAuth/Tests/Unit/UserTests.swift index 619efac790a..5008c24e48a 100644 --- a/FirebaseAuth/Tests/Unit/UserTests.swift +++ b/FirebaseAuth/Tests/Unit/UserTests.swift @@ -141,12 +141,20 @@ class UserTests: RPCBaseTests { "phoneNumber": kPhoneNumber, "createdAt": String(Int(kCreationDateTimeIntervalInSeconds) * 1000), // to nanoseconds "lastLoginAt": String(Int(kLastSignInDateTimeIntervalInSeconds) * 1000), - "mfaInfo": [[ - "phoneInfo": kPhoneInfo, - "mfaEnrollmentId": kEnrollmentID, - "displayName": kDisplayName, - "enrolledAt": kEnrolledAt, - ]], + "mfaInfo": [ + [ + "phoneInfo": kPhoneInfo, + "mfaEnrollmentId": kEnrollmentID, + "displayName": kDisplayName, + "enrolledAt": kEnrolledAt, + ], + [ + "totpInfo": "123456", + "mfaEnrollmentId": kEnrollmentID, + "displayName": kDisplayName, + "enrolledAt": kEnrolledAt, + ], + ], ]] let expectation = self.expectation(description: #function) @@ -346,11 +354,15 @@ class UserTests: RPCBaseTests { // Verify MultiFactorInfo properties. let enrolledFactors = try XCTUnwrap(user.multiFactor.enrolledFactors) + XCTAssertEqual(enrolledFactors.count, 2) XCTAssertEqual(enrolledFactors[0].factorID, PhoneMultiFactorInfo.PhoneMultiFactorID) - XCTAssertEqual(enrolledFactors[0].uid, kEnrollmentID) - XCTAssertEqual(enrolledFactors[0].displayName, self.kDisplayName) - let date = try XCTUnwrap(enrolledFactors[0].enrollmentDate) - XCTAssertEqual("\(date)", kEnrolledAtMatch) + XCTAssertEqual(enrolledFactors[1].factorID, PhoneMultiFactorInfo.TOTPMultiFactorID) + for enrolledFactor in enrolledFactors { + XCTAssertEqual(enrolledFactor.uid, kEnrollmentID) + XCTAssertEqual(enrolledFactor.displayName, self.kDisplayName) + let date = try XCTUnwrap(enrolledFactor.enrollmentDate) + XCTAssertEqual("\(date)", kEnrolledAtMatch) + } #endif } catch { XCTFail("Caught an error in \(#function): \(error)") From 707d2c9bdaecd93c128b0847424b56dad1cfa6d2 Mon Sep 17 00:00:00 2001 From: Nick Cooke <36927374+ncooke3@users.noreply.github.com> Date: Thu, 5 Sep 2024 13:40:39 -0400 Subject: [PATCH 3/7] Remove unavailable attributes since they don't seem to matter --- .../Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift | 2 -- 1 file changed, 2 deletions(-) diff --git a/FirebaseAuth/Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift b/FirebaseAuth/Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift index a035c096dd2..08d39a46dd6 100644 --- a/FirebaseAuth/Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift +++ b/FirebaseAuth/Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift @@ -32,13 +32,11 @@ import Foundation super.init(proto: proto, factorID: PhoneMultiFactorInfo.TOTPMultiFactorID) } - @available(*, unavailable) required init?(coder: NSCoder) { totpInfo = nil super.init(coder: coder) } - @available(*, unavailable) override class var supportsSecureCoding: Bool { super.supportsSecureCoding } From 535a0fe6dad77eb24d388fdcc104e30be8c6d9b3 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Fri, 6 Sep 2024 18:06:52 -0400 Subject: [PATCH 4/7] Add changelog --- FirebaseAuth/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/FirebaseAuth/CHANGELOG.md b/FirebaseAuth/CHANGELOG.md index 96771bafd05..81388ecef86 100644 --- a/FirebaseAuth/CHANGELOG.md +++ b/FirebaseAuth/CHANGELOG.md @@ -7,6 +7,9 @@ in methods and a `nil` error. In such cases, an empty array is instead returned with the `nil` error. (#13550) - [Fixed] Fixed user session persistence in multi tenant projects. Introduced in 11.0.0. (#13565) +- [Fixed] Fixed encoding crash that occurs when using TOTP multi-factor + authentication. Note that this fix will not be in the 11.2.0 zip and Carthage + distributions, but will be included from 11.3.0 onwards. (#13591) # 11.1.0 - [fixed] Fixed `Swift.error` conformance for `AuthErrorCode`. (#13430) From 494a3a286d8c040d19dbb37ce23d5442ccecafe2 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Fri, 6 Sep 2024 19:29:41 -0400 Subject: [PATCH 5/7] Clean up the whole approach --- .../Swift/Backend/RPC/Proto/AuthProtoMFAEnrollment.swift | 2 ++ .../Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift | 9 ++++----- FirebaseAuth/Tests/Unit/Fakes/FakeBackendRPCIssuer.swift | 2 +- FirebaseAuth/Tests/Unit/UserTests.swift | 3 ++- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/FirebaseAuth/Sources/Swift/Backend/RPC/Proto/AuthProtoMFAEnrollment.swift b/FirebaseAuth/Sources/Swift/Backend/RPC/Proto/AuthProtoMFAEnrollment.swift index e742cf8cf77..494252ac56e 100644 --- a/FirebaseAuth/Sources/Swift/Backend/RPC/Proto/AuthProtoMFAEnrollment.swift +++ b/FirebaseAuth/Sources/Swift/Backend/RPC/Proto/AuthProtoMFAEnrollment.swift @@ -16,6 +16,8 @@ import Foundation class AuthProtoMFAEnrollment: NSObject, AuthProto { let phoneInfo: String? + // In practice, this will be an empty dictionary. The presence of which + // indicates TOTP MFA enrollment rather than phone MFA enrollment. let totpInfo: NSObject? let mfaEnrollmentID: String? let displayName: String? diff --git a/FirebaseAuth/Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift b/FirebaseAuth/Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift index 08d39a46dd6..34be8ce1277 100644 --- a/FirebaseAuth/Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift +++ b/FirebaseAuth/Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift @@ -22,21 +22,20 @@ import Foundation /// /// This class is available on iOS only. class TOTPMultiFactorInfo: MultiFactorInfo { - /// This is the totp info for the second factor. - let totpInfo: NSObject? - /// Initialize the AuthProtoMFAEnrollment instance with proto. /// - Parameter proto: AuthProtoMFAEnrollment proto object. init(proto: AuthProtoMFAEnrollment) { - totpInfo = proto.totpInfo super.init(proto: proto, factorID: PhoneMultiFactorInfo.TOTPMultiFactorID) } required init?(coder: NSCoder) { - totpInfo = nil super.init(coder: coder) } + override func encode(with coder: NSCoder) { + super.encode(with: coder) + } + override class var supportsSecureCoding: Bool { super.supportsSecureCoding } diff --git a/FirebaseAuth/Tests/Unit/Fakes/FakeBackendRPCIssuer.swift b/FirebaseAuth/Tests/Unit/Fakes/FakeBackendRPCIssuer.swift index f8a1001436b..ef9f59d1f3b 100644 --- a/FirebaseAuth/Tests/Unit/Fakes/FakeBackendRPCIssuer.swift +++ b/FirebaseAuth/Tests/Unit/Fakes/FakeBackendRPCIssuer.swift @@ -71,7 +71,7 @@ class FakeBackendRPCIssuer: NSObject, AuthBackendRPCIssuer { var respondBlock: (() throws -> Void)? var nextRespondBlock: (() throws -> Void)? - var fakeGetAccountProviderJSON: [[String: AnyHashable]]? + var fakeGetAccountProviderJSON: [[String: Any]]? var fakeSecureTokenServiceJSON: [String: AnyHashable]? var secureTokenNetworkError: NSError? var secureTokenErrorString: String? diff --git a/FirebaseAuth/Tests/Unit/UserTests.swift b/FirebaseAuth/Tests/Unit/UserTests.swift index 5008c24e48a..9b3b1704150 100644 --- a/FirebaseAuth/Tests/Unit/UserTests.swift +++ b/FirebaseAuth/Tests/Unit/UserTests.swift @@ -149,7 +149,8 @@ class UserTests: RPCBaseTests { "enrolledAt": kEnrolledAt, ], [ - "totpInfo": "123456", + // In practice, this will be an empty dictionary. + "totpInfo": [AnyHashable: Any](), "mfaEnrollmentId": kEnrollmentID, "displayName": kDisplayName, "enrolledAt": kEnrolledAt, From 4bc61c17de74e258ac8af6ff59a450b56d46ba3f Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Fri, 6 Sep 2024 19:36:25 -0400 Subject: [PATCH 6/7] remove useless override --- .../Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift | 4 ---- 1 file changed, 4 deletions(-) diff --git a/FirebaseAuth/Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift b/FirebaseAuth/Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift index 34be8ce1277..b1c3f7b7d95 100644 --- a/FirebaseAuth/Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift +++ b/FirebaseAuth/Sources/Swift/MultiFactor/TOTP/TOTPMultiFactorInfo.swift @@ -32,10 +32,6 @@ import Foundation super.init(coder: coder) } - override func encode(with coder: NSCoder) { - super.encode(with: coder) - } - override class var supportsSecureCoding: Bool { super.supportsSecureCoding } From 01f68dbaf2f83955bdc75304afd05f7e23b8b106 Mon Sep 17 00:00:00 2001 From: Nick Cooke Date: Fri, 6 Sep 2024 20:04:21 -0400 Subject: [PATCH 7/7] Fix warning --- FirebaseAuth/Tests/Unit/Fakes/FakeBackendRPCIssuer.swift | 2 +- FirebaseAuth/Tests/Unit/UserTests.swift | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/FirebaseAuth/Tests/Unit/Fakes/FakeBackendRPCIssuer.swift b/FirebaseAuth/Tests/Unit/Fakes/FakeBackendRPCIssuer.swift index ef9f59d1f3b..f8a1001436b 100644 --- a/FirebaseAuth/Tests/Unit/Fakes/FakeBackendRPCIssuer.swift +++ b/FirebaseAuth/Tests/Unit/Fakes/FakeBackendRPCIssuer.swift @@ -71,7 +71,7 @@ class FakeBackendRPCIssuer: NSObject, AuthBackendRPCIssuer { var respondBlock: (() throws -> Void)? var nextRespondBlock: (() throws -> Void)? - var fakeGetAccountProviderJSON: [[String: Any]]? + var fakeGetAccountProviderJSON: [[String: AnyHashable]]? var fakeSecureTokenServiceJSON: [String: AnyHashable]? var secureTokenNetworkError: NSError? var secureTokenErrorString: String? diff --git a/FirebaseAuth/Tests/Unit/UserTests.swift b/FirebaseAuth/Tests/Unit/UserTests.swift index 9b3b1704150..77c1449792f 100644 --- a/FirebaseAuth/Tests/Unit/UserTests.swift +++ b/FirebaseAuth/Tests/Unit/UserTests.swift @@ -150,11 +150,11 @@ class UserTests: RPCBaseTests { ], [ // In practice, this will be an empty dictionary. - "totpInfo": [AnyHashable: Any](), + "totpInfo": [AnyHashable: AnyHashable](), "mfaEnrollmentId": kEnrollmentID, "displayName": kDisplayName, "enrolledAt": kEnrolledAt, - ], + ] as [AnyHashable: AnyHashable], ], ]]