Skip to content
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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
94 changes: 48 additions & 46 deletions FirebaseAuth/Sources/Swift/Auth/Auth.swift
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,9 @@ extension Auth: AuthInterop {
Task {
do {
let response = try await self.backend.call(with: request)
Auth.wrapMainAsync(callback: completion, withParam: response.signinMethods, error: nil)
Auth.wrapMainAsync(callback: completion, with: .success(response.signinMethods))
} catch {
Auth.wrapMainAsync(callback: completion, withParam: nil, error: error)
Auth.wrapMainAsync(callback: completion, with: .failure(error))
}
}
}
Expand Down Expand Up @@ -365,9 +365,9 @@ extension Auth: AuthInterop {
withEmail: email,
password: password
)
decoratedCallback(authData, nil)
decoratedCallback(.success(authData))
} catch {
decoratedCallback(nil, error)
decoratedCallback(.failure(error))
}
}
}
Expand Down Expand Up @@ -463,9 +463,9 @@ extension Auth: AuthInterop {
do {
let authData = try await self.internalSignInAndRetrieveData(withCredential: credential,
isReauthentication: false)
decoratedCallback(authData, nil)
decoratedCallback(.success(authData))
} catch {
decoratedCallback(nil, error)
decoratedCallback(.failure(error))
}
}
}
Expand Down Expand Up @@ -544,9 +544,9 @@ extension Auth: AuthInterop {
withCredential: credential,
isReauthentication: false
)
decoratedCallback(authData, nil)
decoratedCallback(.success(authData))
} catch {
decoratedCallback(nil, error)
decoratedCallback(.failure(error))
}
}
}
Expand Down Expand Up @@ -642,9 +642,9 @@ extension Auth: AuthInterop {
do {
let authData = try await self.internalSignInAndRetrieveData(withCredential: credential,
isReauthentication: false)
decoratedCallback(authData, nil)
decoratedCallback(.success(authData))
} catch {
decoratedCallback(nil, error)
decoratedCallback(.failure(error))
}
}
}
Expand Down Expand Up @@ -710,7 +710,7 @@ extension Auth: AuthInterop {
let decoratedCallback = self.signInFlowAuthDataResultCallback(byDecorating: completion)
if let currentUser = self._currentUser, currentUser.isAnonymous {
let result = AuthDataResult(withUser: currentUser, additionalUserInfo: nil)
decoratedCallback(result, nil)
decoratedCallback(.success(result))
return
}
let request = SignUpNewUserRequest(requestConfiguration: self.requestConfiguration)
Expand All @@ -728,10 +728,11 @@ extension Auth: AuthInterop {
profile: nil,
username: nil,
isNewUser: true)
decoratedCallback(AuthDataResult(withUser: user, additionalUserInfo: additionalUserInfo),
nil)
decoratedCallback(
.success(AuthDataResult(withUser: user, additionalUserInfo: additionalUserInfo))
)
} catch {
decoratedCallback(nil, error)
decoratedCallback(.failure(error))
}
}
}
Expand Down Expand Up @@ -790,10 +791,11 @@ extension Auth: AuthInterop {
profile: nil,
username: nil,
isNewUser: response.isNewUser)
decoratedCallback(AuthDataResult(withUser: user, additionalUserInfo: additionalUserInfo),
nil)
decoratedCallback(
.success(AuthDataResult(withUser: user, additionalUserInfo: additionalUserInfo))
)
} catch {
decoratedCallback(nil, error)
decoratedCallback(.failure(error))
}
}
}
Expand Down Expand Up @@ -866,7 +868,7 @@ extension Auth: AuthInterop {
action: AuthRecaptchaAction.signUpPassword) { response, error in
if let error {
DispatchQueue.main.async {
decoratedCallback(nil, error)
decoratedCallback(.failure(error))
}
return
}
Expand All @@ -881,7 +883,8 @@ extension Auth: AuthInterop {

func internalCreateUserWithEmail(request: SignUpNewUserRequest,
inResponse: SignUpNewUserResponse? = nil,
decoratedCallback: @escaping (AuthDataResult?, Error?) -> Void) {
decoratedCallback: @escaping (Result<AuthDataResult, Error>)
-> Void) {
Task {
do {
var response: SignUpNewUserResponse
Expand All @@ -900,11 +903,11 @@ extension Auth: AuthInterop {
profile: nil,
username: nil,
isNewUser: true)
decoratedCallback(AuthDataResult(withUser: user,
additionalUserInfo: additionalUserInfo),
nil)
decoratedCallback(
.success(AuthDataResult(withUser: user, additionalUserInfo: additionalUserInfo))
)
} catch {
decoratedCallback(nil, error)
decoratedCallback(.failure(error))
}
}
}
Expand Down Expand Up @@ -1009,9 +1012,9 @@ extension Auth: AuthInterop {
let actionCodeInfo = ActionCodeInfo(withOperation: operation,
email: email,
newEmail: response.verifiedEmail)
Auth.wrapMainAsync(callback: completion, withParam: actionCodeInfo, error: nil)
Auth.wrapMainAsync(callback: completion, with: .success(actionCodeInfo))
} catch {
Auth.wrapMainAsync(callback: completion, withParam: nil, error: error)
Auth.wrapMainAsync(callback: completion, with: .failure(error))
}
}
}
Expand Down Expand Up @@ -2238,23 +2241,19 @@ extension Auth: AuthInterop {
/// Invoked asynchronously on the main thread in the future.
/// - Returns: Returns a block that updates the current user.
func signInFlowAuthDataResultCallback(byDecorating callback:
((AuthDataResult?, Error?) -> Void)?) -> (AuthDataResult?, Error?) -> Void {
let authDataCallback: (((AuthDataResult?, Error?) -> Void)?, AuthDataResult?, Error?) -> Void =
{ callback, result, error in
Auth.wrapMainAsync(callback: callback, withParam: result, error: error)
}
return { authResult, error in
if let error {
authDataCallback(callback, nil, error)
return
}
do {
try self.updateCurrentUser(authResult?.user, byForce: false, savingToDisk: true)
} catch {
authDataCallback(callback, nil, error)
return
((AuthDataResult?, Error?) -> Void)?) -> (Result<AuthDataResult, Error>) -> Void {
return { result in
switch result {
case let .success(authResult):
do {
try self.updateCurrentUser(authResult.user, byForce: false, savingToDisk: true)
Auth.wrapMainAsync(callback: callback, with: .success(authResult))
} catch {
Auth.wrapMainAsync(callback: callback, with: .failure(error))
}
case let .failure(error):
Auth.wrapMainAsync(callback: callback, with: .failure(error))
}
authDataCallback(callback, authResult, nil)
}
}

Expand All @@ -2278,11 +2277,14 @@ extension Auth: AuthInterop {
}

class func wrapMainAsync<T: Any>(callback: ((T?, Error?) -> Void)?,
withParam param: T?,
error: Error?) -> Void {
if let callback {
DispatchQueue.main.async {
callback(param, error)
with result: Result<T, Error>) -> Void {
guard let callback else { return }
DispatchQueue.main.async {
switch result {
case let .success(success):
callback(success, nil)
case let .failure(error):
callback(nil, error)
}
}
}
Expand Down
23 changes: 14 additions & 9 deletions FirebaseAuth/Sources/Swift/AuthProvider/PhoneAuthProvider.swift
Copy link
Member Author

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:

/// Verify ownership of the second factor phone number by the current user.
/// - Parameter phoneNumber: The phone number to be verified.
/// - Parameter uiDelegate: An object used to present the SFSafariViewController. The object is
/// retained by this method until the completion block is executed.
/// - Parameter multiFactorSession: A session to identify the MFA flow. For enrollment, this
/// identifies the user trying to enroll. For sign-in, this identifies that the user already
/// passed the first factor challenge.
/// - Returns: The verification ID
@available(iOS 13, tvOS 13, macOS 10.15, watchOS 8, *)
open func verifyPhoneNumber(_ phoneNumber: String,
uiDelegate: AuthUIDelegate? = nil,
multiFactorSession: MultiFactorSession? = nil) async throws
-> String {

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).

Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
}
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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,
Expand All @@ -236,7 +236,7 @@ import Foundation
retryOnInvalidAppCredential: Bool,
multiFactorSession session: MultiFactorSession?,
uiDelegate: AuthUIDelegate?) async throws
-> String? {
-> String {
Copy link
Member Author

Choose a reason for hiding this comment

The 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(
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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:

struct AuthProtoStartMFAPhoneResponseInfo: AuthProto {
var sessionInfo: String?
init(dictionary: [String: AnyHashable]) {
sessionInfo = dictionary["sessionInfo"] as? String
}
}

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 nil?

} 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(
Expand All @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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 responseInfo.


init(dictionary: [String: AnyHashable]) throws {
if let data = dictionary["phoneResponseInfo"] as? [String: AnyHashable] {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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 nil?

}
self.verificationID = verificationID
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ import Foundation
let result = AuthDataResult(withUser: user, additionalUserInfo: nil)
let decoratedCallback = self.auth
.signInFlowAuthDataResultCallback(byDecorating: completion)
decoratedCallback(result, nil)
decoratedCallback(.success(result))
} catch {
if let completion {
completion(nil, error)
Expand Down
2 changes: 1 addition & 1 deletion FirebaseAuth/Tests/Unit/RPCBaseTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ class RPCBaseTests: XCTestCase {
XCTFail("decodedRequest is not a dictionary")
}
// Dummy response to unblock await.
let _ = try self.rpcIssuer?.respond(withJSON: [:])
let _ = try self.rpcIssuer?.respond(withJSON: ["sessionInfo": "verification_id_123"])
}
let _ = try await authBackend.call(with: request)
}
Expand Down
Loading