-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Auth] Use result type to wrap completion handlers internally #13563
base: main
Are you sure you want to change the base?
Changes from all commits
05d5a14
5741f72
b2c894c
48d4e2a
2dda025
b839e62
347828a
735ee6a
ccda624
1272db0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -86,9 +86,9 @@ import Foundation | |||||||||||||||
uiDelegate: uiDelegate, | ||||||||||||||||
multiFactorSession: multiFactorSession | ||||||||||||||||
) | ||||||||||||||||
Auth.wrapMainAsync(callback: completion, withParam: verificationID, error: nil) | ||||||||||||||||
Auth.wrapMainAsync(callback: completion, with: .success(verificationID)) | ||||||||||||||||
} catch { | ||||||||||||||||
Auth.wrapMainAsync(callback: completion, withParam: nil, error: error) | ||||||||||||||||
Auth.wrapMainAsync(callback: completion, with: .failure(error)) | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
|
@@ -184,7 +184,7 @@ import Foundation | |||||||||||||||
private func internalVerify(phoneNumber: String, | ||||||||||||||||
uiDelegate: AuthUIDelegate?, | ||||||||||||||||
multiFactorSession: MultiFactorSession? = nil) async throws | ||||||||||||||||
-> String? { | ||||||||||||||||
-> String { | ||||||||||||||||
guard phoneNumber.count > 0 else { | ||||||||||||||||
throw AuthErrorUtils.missingPhoneNumberError(message: nil) | ||||||||||||||||
} | ||||||||||||||||
|
@@ -209,7 +209,7 @@ import Foundation | |||||||||||||||
private func verifyClAndSendVerificationCode(toPhoneNumber phoneNumber: String, | ||||||||||||||||
retryOnInvalidAppCredential: Bool, | ||||||||||||||||
uiDelegate: AuthUIDelegate?) async throws | ||||||||||||||||
-> String? { | ||||||||||||||||
-> String { | ||||||||||||||||
let codeIdentity = try await verifyClient(withUIDelegate: uiDelegate) | ||||||||||||||||
let request = SendVerificationCodeRequest(phoneNumber: phoneNumber, | ||||||||||||||||
codeIdentity: codeIdentity, | ||||||||||||||||
|
@@ -236,7 +236,7 @@ import Foundation | |||||||||||||||
retryOnInvalidAppCredential: Bool, | ||||||||||||||||
multiFactorSession session: MultiFactorSession?, | ||||||||||||||||
uiDelegate: AuthUIDelegate?) async throws | ||||||||||||||||
-> String? { | ||||||||||||||||
-> String { | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This return type bubbles up to the public API, which we need to make non-optional if possible, so this is why this is being changed to non-optional String. |
||||||||||||||||
if let settings = auth.settings, | ||||||||||||||||
settings.isAppVerificationDisabledForTesting { | ||||||||||||||||
let request = SendVerificationCodeRequest( | ||||||||||||||||
|
@@ -264,15 +264,20 @@ import Foundation | |||||||||||||||
enrollmentInfo: startMFARequestInfo, | ||||||||||||||||
requestConfiguration: auth.requestConfiguration) | ||||||||||||||||
let response = try await auth.backend.call(with: request) | ||||||||||||||||
return response.phoneSessionInfo?.sessionInfo | ||||||||||||||||
guard let sessionInfo = response.phoneSessionInfo?.sessionInfo else { | ||||||||||||||||
throw AuthErrorUtils.error(code: .missingMultiFactorInfo) | ||||||||||||||||
} | ||||||||||||||||
return sessionInfo | ||||||||||||||||
Comment on lines
+267
to
+270
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here and below, we need to unwrap the underlying session info which is current an optional: Lines 17 to 23 in 380a861
Throwing an error is new behavior here. I think this may be ok as long there aren't success cases where the sessionInfo should be |
||||||||||||||||
} else { | ||||||||||||||||
let request = StartMFASignInRequest(MFAPendingCredential: session.mfaPendingCredential, | ||||||||||||||||
MFAEnrollmentID: session.multiFactorInfo?.uid, | ||||||||||||||||
signInInfo: startMFARequestInfo, | ||||||||||||||||
requestConfiguration: auth.requestConfiguration) | ||||||||||||||||
|
||||||||||||||||
let response = try await auth.backend.call(with: request) | ||||||||||||||||
return response.responseInfo?.sessionInfo | ||||||||||||||||
guard let sessionInfo = response.responseInfo.sessionInfo else { | ||||||||||||||||
throw AuthErrorUtils.error(code: .multiFactorInfoNotFound) | ||||||||||||||||
} | ||||||||||||||||
return sessionInfo | ||||||||||||||||
} | ||||||||||||||||
} catch { | ||||||||||||||||
return try await handleVerifyErrorWithRetry( | ||||||||||||||||
|
@@ -289,7 +294,7 @@ import Foundation | |||||||||||||||
phoneNumber: String, | ||||||||||||||||
retryOnInvalidAppCredential: Bool, | ||||||||||||||||
multiFactorSession session: MultiFactorSession?, | ||||||||||||||||
uiDelegate: AuthUIDelegate?) async throws -> String? { | ||||||||||||||||
uiDelegate: AuthUIDelegate?) async throws -> String { | ||||||||||||||||
if (error as NSError).code == AuthErrorCode.invalidAppCredential.rawValue { | ||||||||||||||||
if retryOnInvalidAppCredential { | ||||||||||||||||
auth.appCredentialManager.clearCredential() | ||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ import Foundation | |
|
||
@available(iOS 13, tvOS 13, macOS 10.15, macCatalyst 13, watchOS 7, *) | ||
struct StartMFASignInResponse: AuthRPCResponse { | ||
var responseInfo: AuthProtoStartMFAPhoneResponseInfo? | ||
let responseInfo: AuthProtoStartMFAPhoneResponseInfo | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This diff should be a no-op in practice because the initializer will throw if it could not unwrap the value being assigned to |
||
|
||
init(dictionary: [String: AnyHashable]) throws { | ||
if let data = dictionary["phoneResponseInfo"] as? [String: AnyHashable] { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,10 +14,14 @@ | |
|
||
import Foundation | ||
|
||
@available(iOS 13, tvOS 13, macOS 10.15, macCatalyst 13, watchOS 7, *) | ||
struct SendVerificationCodeResponse: AuthRPCResponse { | ||
var verificationID: String? | ||
let verificationID: String | ||
|
||
init(dictionary: [String: AnyHashable]) throws { | ||
verificationID = dictionary["sessionInfo"] as? String | ||
guard let verificationID = dictionary["sessionInfo"] as? String else { | ||
throw AuthErrorUtils.unexpectedResponse(deserializedResponse: dictionary) | ||
Comment on lines
+19
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally, this should be non-optional. I'm adding the error for cases where it's not. Are there success cases where verificationID should be |
||
} | ||
self.verificationID = verificationID | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior changes made here map to this public API:
firebase-ios-sdk/FirebaseAuth/Sources/Swift/AuthProvider/PhoneAuthProvider.swift
Lines 97 to 109 in 380a861
Is there a valid success case where the returned String should be
nil
(or an empty string) and the error is also non-nil? This PR enforces that it is either one or the other (non-optional String OR error).