feat: Facebook auth re-authentication to allow deleting user #1256
Conversation
| @@ -6,7 +6,7 @@ protocol EmailPasswordOperationReauthentication { | |||
| } | |||
|
|
|||
| extension EmailPasswordOperationReauthentication { | |||
| func reauthenticate() async throws -> AuthenticationToken { | |||
| @MainActor func reauthenticate() async throws -> AuthenticationToken { | |||
There was a problem hiding this comment.
I've made a number of API @MainActor to stop Xcode compiler error for passing User around which is non-sendable at the moment.
There was a problem hiding this comment.
SGTM, you could add TODOs here since User will eventually become Sendable but there's a strong argument for just keeping this API @MainActor regardless.
| public var phoneAuthProvider: (any PhoneAuthProviderAuthUIProtocol)? | ||
| private var googleProvider: (any GoogleProviderAuthUIProtocol)? | ||
| private var facebookProvider: (any FacebookProviderAuthUIProtocol)? | ||
| private var phoneAuthProvider: (any PhoneAuthProviderAuthUIProtocol)? |
There was a problem hiding this comment.
These are private now because they shouldn't be accessed, should use safeGoogleProvider, etc.
There was a problem hiding this comment.
nit: I'd rather have the private apis be named unsafeGoogleProvider and the public ones plainly googleProvider
| case let phone as PhoneAuthProviderAuthUIProtocol: | ||
| phoneAuthProvider = phone | ||
| providers.append(provider) | ||
| default: |
There was a problem hiding this comment.
This now assigns providers correctly without having to manually set + call register().
| let operation = EmailPasswordDeleteUserOperation(passwordPrompt: passwordPrompt) | ||
| try await operation(on: user) | ||
| } else if providerId == "facebook.com" { | ||
| try await safeFacebookProvider.deleteUser(user: user) |
There was a problem hiding this comment.
A bit tricky deleting user with Facebook re-authentication but I found the best way was to have it on the provider itself which makes sense to me anyway.
| facebookProvider = FacebookProviderAuthUI(scopes: scopes) | ||
| register(provider: facebookProvider!) | ||
| FacebookProviderAuthUI.configureSharedInstance(scopes: scopes) | ||
| register(provider: FacebookProviderAuthUI.shared) |
There was a problem hiding this comment.
Switched to having a private init() for Facebook provider and making it a singleton which is kept on shared as there really should only be one instance of it anyway. I can roll this back if you don't like.
| self.scopes = scopes ?? kDefaultFacebookScopes | ||
| rawNonce = CommonUtils.randomNonce() |
There was a problem hiding this comment.
Moved this logic so it recreates every time for limited login otherwise reauthentication will fail as the credential is the same.
| private var rawNonce: String? | ||
| private var shaNonce: String? | ||
| // Needed for reauthentication | ||
| var isLimitedLogin: Bool = true |
There was a problem hiding this comment.
Need this to track the initial login for re-authentication.
| // showCanceledAlert = true | ||
| case let .failed(error): | ||
| continuation.resume(throwing: error) | ||
| // errorMessage = authService.string.localizedErrorMessage(for: error) |
There was a problem hiding this comment.
errorMessage and alert are handled upstream, removed here.
| @@ -6,7 +6,7 @@ protocol EmailPasswordOperationReauthentication { | |||
| } | |||
|
|
|||
| extension EmailPasswordOperationReauthentication { | |||
| func reauthenticate() async throws -> AuthenticationToken { | |||
| @MainActor func reauthenticate() async throws -> AuthenticationToken { | |||
There was a problem hiding this comment.
SGTM, you could add TODOs here since User will eventually become Sendable but there's a strong argument for just keeping this API @MainActor regardless.
| public var phoneAuthProvider: (any PhoneAuthProviderAuthUIProtocol)? | ||
| private var googleProvider: (any GoogleProviderAuthUIProtocol)? | ||
| private var facebookProvider: (any FacebookProviderAuthUIProtocol)? | ||
| private var phoneAuthProvider: (any PhoneAuthProviderAuthUIProtocol)? |
There was a problem hiding this comment.
nit: I'd rather have the private apis be named unsafeGoogleProvider and the public ones plainly googleProvider
| @@ -119,7 +131,7 @@ public final class AuthService { | |||
| get throws { | |||
| guard let provider = facebookProvider else { | |||
| throw AuthServiceError | |||
| .notConfiguredProvider("`FacebookProviderSwift` has not been configured") | |||
| .notConfiguredProvider("`FacebookProviderAuthUI` has not been configured") | |||
There was a problem hiding this comment.
Should this not just be a fatalError given consumers won't have conditional configuration of providers?
| @@ -109,7 +119,7 @@ public class FacebookProviderAuthUI: FacebookProviderAuthUIProtocol { | |||
| if let idToken = AuthenticationToken.current { | |||
| let credential = OAuthProvider.credential(withProviderID: providerId, | |||
| idToken: idToken.tokenString, | |||
| rawNonce: rawNonce) | |||
| rawNonce: rawNonce!) | |||
There was a problem hiding this comment.
It's not immediately clear from the code that this optional will never be nil here, so maybe add a comment explaining why it's safe.
I've made comments in the PR so it is a bit clearer what is going on.