Adds anonymous upgrade for google account email conflict #407
Conversation
morganchen12
left a comment
morganchen12
left a comment
There was a problem hiding this comment.
Had some comments on callback handling, otherwise LGTM.
| code:FUIAuthErrorCodeMergeConflict | ||
| userInfo:userInfo]; | ||
| result(nil, mergeError); | ||
| completeSignInBlock(authResult, mergeError); |
There was a problem hiding this comment.
I know this isn't part of the change, but can this cause result to be called twice? Say result is nonnull.
resultis called on line 216.mergeErroris nonnull.completeSignInBlock()is called with nonnullresult.resultis called again on line 182 withnil, nilas its arguments.
Also, this is an unguarded call to result, which may be nil.
There was a problem hiding this comment.
that's a good catch, we seemed to have forgotten to remove this call after adding the completeSignInBlock.
| NSError *_Nullable error) { | ||
| if (error) { | ||
| result(nil, error); | ||
| completeSignInBlock(nil, error); |
There was a problem hiding this comment.
The comment above also applies here.
| NSError *_Nullable error) { | ||
| if (error) { | ||
| result(nil, error); | ||
| completeSignInBlock(nil, error); |
| code:FUIAuthErrorCodeMergeConflict | ||
| userInfo:userInfo]; | ||
| result(nil, mergeError); | ||
| completeSignInBlock(authResult, mergeError); |
| return; | ||
| } | ||
|
|
||
| [auth signInAndRetrieveDataWithCredential:credential completion:completion]; |
There was a problem hiding this comment.
Why does this use a different auth instance?
There was a problem hiding this comment.
Because we need to complete a sign-in of the existing credential before linking; however we do not want to lose the currently signed in user. A sign-in operation usually replaces the currently signed-in user if successful.
There was a problem hiding this comment.
Maybe add a comment describing that :)
FirebaseAuthUI/FUIAuth.m
Outdated
|
|
||
| - (nullable NSString *)federatedAuthProviderFromProviders:(NSArray <NSString *> *) providers { | ||
| NSSet *providerSet = | ||
| [[NSSet alloc] initWithArray:@[ FIRFacebookAuthProviderID, FIRGoogleAuthProviderID ]]; |
There was a problem hiding this comment.
nit: use +[NSSet setWithArray:]
Also, using NSSet for a faster contains when searching an array is not necessarily faster than using containsObject: on the array directly, since some cost is incurred when creating the set.
There was a problem hiding this comment.
Agreed, using the class method but leaving the set, just don't like the idea of creating an array for the sole purpose of look up :).
* Adds anonymous upgrade for Google email conflict * Addresses comments * Addresses comments.
2 participants
Adds logic to link a federated account with an existing google account if they share the same email during anonynous user upgrade.