-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Upgrading anonymous accounts #1358
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
Conversation
6ddb64f
to
150b518
Compare
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.
Looking pretty good so far! Some drive-by comments.
mEnableHintSelector.isChecked()) | ||
.build(), | ||
RC_SIGN_IN); | ||
FirebaseAuth.getInstance() |
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.
If you look at my (now-closed) PR I added a whole separate sample Activity
for anonymous. You might as well copy it, IMO it shows off the flow nicely without complicating the "normal" one.
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.
Done.
@@ -29,7 +30,7 @@ public void startSignIn(@NonNull HelperActivityBase activity) { | |||
|
|||
@Override | |||
public void onActivityResult(int requestCode, int resultCode, @Nullable Intent data) { | |||
if (requestCode == RequestCodes.EMAIL_FLOW) { | |||
if (requestCode == RequestCodes.EMAIL_FLOW && resultCode != ErrorCodes.ANONYMOUS_UPGRADE_MERGE_CONFLICT) { |
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.
nit: this may be a little clearer if you add a separate block at the top:
if (resultCode == ErrorCodes.ANONYMOUS_UPGRADE_MERGE_CONFLICT) {
// Add comment explaining why
return;
}
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.
Done.
@@ -182,7 +184,13 @@ public void onActivityResult(int requestCode, int resultCode, @Nullable Intent d | |||
} else if (response.isSuccessful()) { | |||
setResult(Resource.forSuccess(response)); | |||
} else { | |||
setResult(Resource.<IdpResponse>forFailure(response.getError())); | |||
if (response.getError().getErrorCode() == |
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.
nit: rather than:
else {
if {
} else {
}
}
Should just be another else if
followed by a final else
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.
Done.
@@ -98,6 +111,9 @@ protected void onFailure(@NonNull Exception e) { | |||
R.integer.fui_min_password_length)); | |||
} else if (e instanceof FirebaseAuthInvalidCredentialsException) { | |||
mEmailInput.setError(getString(R.string.fui_invalid_email_address)); | |||
} else if (e instanceof FirebaseAuthAnonymousUpgradeException) { | |||
IdpResponse response = ((FirebaseAuthAnonymousUpgradeException) e).getResponse(); | |||
((AnonymousUpgradeListener) getActivity()).onMergeFailure(response); |
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.
This is fine, however you could optionally assert the cast in onActivityCreated
and then store a listener reference rather than casting here.
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.
Done.
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) | ||
public class AnonymousUpgradeUtils { | ||
|
||
public static Task<AuthResult> createUserWithEmailAndPasswordOrLink(@NonNull FirebaseAuth auth, |
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.
naming nit: createOrLinkUserWith...
reads better to me.
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.
Done.
// Create a new FirebaseApp so that the anonymous user state is not lost in our | ||
// original FirebaseAuth instance. | ||
String randomName = UUID.randomUUID().toString(); | ||
FirebaseApp scratchApp = FirebaseApp.initializeApp( |
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.
I think initializeApp
is expensive enough that we'd want to cache a singleton "other app" to use for all scratch operations.
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.
Done.
setResult(Resource.<IdpResponse>forFailure(e)); | ||
} | ||
}); | ||
if (AnonymousUpgradeUtils.canUpgradeAnonymous(getAuth(), |
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.
Maybe instead of overloading this branch we should have the createOrLink
method transform collision errors into a custom error class with a Continuation
?
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.
TBD.
.continueWithTask(new ProfileMerger(outputResponse)) | ||
.addOnFailureListener(new TaskFailureLogger(TAG, "linkWithCredential+merge failed.")); | ||
AnonymousUpgradeUtils.validateCredential(FirebaseApp.getInstance(), credToValidate) | ||
.continueWith(new Continuation<AuthResult, Void>() { |
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.
Seems like there's no need for a Continuation
here since the chain ends. Maybe it should be an OnCompleteListener
?
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.
Done.
…WelcomeBackIdpPrompt).
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.
Looking good! Mostly small style/clarity comments.
By the way: if you want we can do this in multiple PRs. Like perfect what you've done so far and finish it in a second PR. That way the size of the PR stays sane and we can discuss a subset of the issues.
@@ -1107,10 +1107,23 @@ public Intent build() { | |||
* Builder for the intent to start the user authentication flow. | |||
*/ | |||
public final class SignInIntentBuilder extends AuthIntentBuilder<SignInIntentBuilder> { | |||
|
|||
private boolean mEnableAnonymousUpgrade; |
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.
Any reason why this boolean
is part of SignInIntentBuilder
and not AuthIntentBuilder
?
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 doc says to add it in SignInIntentBuilder. You also added it here in your PR.
"Token cannot be null when using a non-email provider."); | ||
if (mPendingCredential != null) { | ||
return new IdpResponse(mPendingCredential, new FirebaseUiException(ErrorCodes.ANONYMOUS_UPGRADE_MERGE_CONFLICT)); | ||
} else { |
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.
Since the if
returns, no need for this else
. We can remove a level of nesting here.
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.
👍
@@ -198,6 +216,9 @@ public void run() { | |||
public void onActivityCreated(@Nullable Bundle savedInstanceState) { | |||
super.onActivityCreated(savedInstanceState); | |||
requireActivity().setTitle(R.string.fui_title_register_email); | |||
FragmentActivity activity = getActivity(); |
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.
tiny nit: any reason we need can't just cast getActivity()
directly?
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.
I'm going to keep it like this and add a check that it implements AnonymousUpgradeListener and throw an exception if it doesn't.
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.
Perfect.
} | ||
}); | ||
|
||
TextView footerText = findViewById(R.id.email_footer_tos_and_pp_text); | ||
PrivacyDisclosureUtils.setupTermsOfServiceFooter(this, getFlowParams(), footerText); | ||
} | ||
|
||
public void onMergeFailure(IdpResponse response) { |
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.
Could this be moved into the base activity? Seems like it's pretty much the same everywhere?
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.
Yes.
IdpResponse response = ((FirebaseAuthAnonymousUpgradeException) e).getResponse(); | ||
onMergeFailure(response); | ||
} else { | ||
handleError(e); |
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.
Should the if/else
block be moved into handleError
? Seems weird to have a handle method that doesn't do all cases.
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.
Done.
protected void handleMergeFailure(@NonNull AuthCredential credential) { | ||
IdpResponse failureResponse = new IdpResponse.Builder(credential) | ||
.build(); | ||
setResult(Resource.<IdpResponse>forFailure(new FirebaseAuthAnonymousUpgradeException( |
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.
You could also make FirebaseAuthAnonymousUpgradeException
have a constructor that takes AuthCredential
since you're just converting the credential to an IdpResponse
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.
If not, this method should call the method below since they are the same setResult
call
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.
Done.
} | ||
}); | ||
|
||
} else { |
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.
Same nit as before.
When a method is just
if (condition) {
action1;
} else {
action2;
}
Consider:
if (condition) {
action1;
return;
}
action2
With something like you have here it helps from getting into crazy indentation levels.
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.
Done.
setResult(Resource.<IdpResponse>forFailure(task.getException())); | ||
return; | ||
}) | ||
.addOnCompleteListener(new OnCompleteListener<AuthResult>() { |
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.
nit: fine either way but when your onCompleteListener
just has a succcess-or-not branch you may find it clearer to use separate onSuccess and onFailure listeners.
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.
Done.
FirebaseAuthUserCollisionException){ | ||
handleMergeFailure(credential); | ||
} | ||
// I'm not sure why we ignore other failures here, but this mirrors |
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.
This comment is no longer relevant?
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.
Well we still ignore other failures though. We only deal w/ a merge failure. I'll remove it.
handleSuccess(response, result); | ||
} | ||
}) | ||
.addOnFailureListener(new OnFailureListener() { |
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.
optional: if you look at my PR you'll notice I made a type of Task
that uses the type system to force you to handle merge exceptions at all call sites. Since you are pretty constantly doing instanceof
checks on the exceptions for these operations you may want to consider something like that.
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.
I'll come back to this.
@lsirac just a warning, Github is stupid and collapses reviews sometimes. So look out for stuff like this: |
Will take a look later today. |
@@ -41,6 +41,7 @@ | |||
import com.google.android.gms.common.Scopes; | |||
import com.google.android.gms.tasks.OnCompleteListener; | |||
import com.google.android.gms.tasks.Task; | |||
import com.google.firebase.auth.AuthCredential; |
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.
Erroneous import
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.
Done.
* flow. This is disabled by default. | ||
*/ | ||
@NonNull | ||
public SignInIntentBuilder setAutoUpgradeAnonymousUsers() { |
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.
This should take in a boolean autoUpgrade
parameter.
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.
Sure but when would they ever call this with false? It's disabled by default.
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.
Oh, gotya. Then the javadoc needs to be updated to just Enables upgrading ...
and the method needs to be renamed to something like enableAutoUpgradeAnonymousUsers
.
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.
Yeah I'm going to ask if I can change the API actually. It'll probably have to take in a boolean to be the same as iOS/Web
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.
@lsirac you can definitely change the API to take a boolean without asking anyone.
@@ -67,6 +73,8 @@ public static String toFriendlyMessage(@Code int code) { | |||
return "Developer error"; | |||
case PROVIDER_ERROR: | |||
return "Provider error"; | |||
case ANONYMOUS_UPGRADE_MERGE_CONFLICT: | |||
return "Merge conflict"; |
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.
This should be a slightly more user friendly message like User account conflict
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.
👍
public FirebaseAuthAnonymousUpgradeException(@ErrorCodes.Code int code, | ||
@NonNull IdpResponse response) { | ||
super(ErrorCodes.toFriendlyMessage(code)); | ||
this.response = response; |
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.
Nit: no this
, use mResponse
. We'll finally be able to get rid of that convention when we start using Kotlin, but for now, we should be consistent.
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.
👍
@@ -218,7 +232,6 @@ public void writeToParcel(Parcel dest, int flags) { | |||
dest.writeString(mToken); | |||
dest.writeString(mSecret); | |||
dest.writeInt(mIsNewUser ? 1 : 0); | |||
|
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.
Nit: leave the space here since this is an unpleasant, unconventional write below.
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.
Done.
@@ -65,6 +65,10 @@ public T getValue() { | |||
return mValue; | |||
} | |||
|
|||
public boolean hasValue() { | |||
return mValue != null; | |||
} |
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.
Please remove this. It's not used anywhere and you should be checking the states instead.
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.
Oops, done.
@@ -42,6 +43,10 @@ protected void onActivityResult(int requestCode, int resultCode, Intent data) { | |||
if (requestCode == RequestCodes.CRED_SAVE_FLOW) { | |||
finish(resultCode, data); | |||
} | |||
|
|||
if (resultCode == ErrorCodes.ANONYMOUS_UPGRADE_MERGE_CONFLICT) { |
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.
Nit: merge this with the if
above.
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.
Done.
@@ -70,4 +75,8 @@ public void startSaveCredentials( | |||
this, getFlowParams(), credential, response); | |||
startActivityForResult(intent, RequestCodes.CRED_SAVE_FLOW); | |||
} | |||
|
|||
public void onMergeFailure(IdpResponse response) { | |||
finish(ErrorCodes.ANONYMOUS_UPGRADE_MERGE_CONFLICT, response.toIntent()); |
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.
I'd prefer we not do this. We have this inlined in all the other call sites so we don't end up with a God activity.
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.
What do you suggest we do?
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.
Oh, sorry for not being clearer, I was just saying to inline this like we do elsewhere. This would be easiest: https://www.jetbrains.com/help/idea/inline.html
@@ -198,6 +216,12 @@ public void run() { | |||
public void onActivityCreated(@Nullable Bundle savedInstanceState) { | |||
super.onActivityCreated(savedInstanceState); | |||
requireActivity().setTitle(R.string.fui_title_register_email); | |||
FragmentActivity activity = getActivity(); |
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.
Nit: merge with requireActivity
from above
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.
Done.
@@ -207,6 +216,11 @@ public FirebaseUiException getError() { | |||
return mException; | |||
} | |||
|
|||
@Nullable | |||
public AuthCredential getCredentialForLinking() { | |||
return mPendingCredential; |
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.
Is it still going to be valid after we use it?
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.
Yes.
@samtstern and there's really no way we can just go all the way with something like #309? The whole "fail sign-in on conflict and just let the dev deal with it" is kinda wishful thinking. 😊 |
@SUPERCILEX yeah I know ... at least for phase 1 I have been overruled. But I think we can make phase 2 look a lot like #309 ... |
Alrighty, then yeah, the design LGTM 👍 |
@lsirac do you have anything else you want to do here? |
I think there's a commit waiting to be pushed? Or at least, @lsirac responded to all my comments. 🤷♂️ (Thanks BTW ❤️) |
@samtstern There's a commit otw for changes based on Alex's feedback and federated sign-in using existing email (email belongs to federated account) w/ no mismatch cases (email belongs to federated acc and email belongs to password account) |
…mail mismatch and added code review changes.
@lsirac Travis is unhappy:
|
private static AuthOperationManager mAuthManager; | ||
|
||
@VisibleForTesting | ||
public FirebaseAuth mScratchAuth; |
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.
@SUPERCILEX fwiw I authorized this kind of test hackery since @lsirac was stuck on other ways to inject this auth. So if you have complaints direct them at me but if you have suggestions they are welcome!
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.
Haha, SGTM. I'll see if I can come up with something later, but for now, whatever works. 👍
enableAnonymousUsersAutoUpgrade or enableAutoUpgradeAnonymousUsers? |
auth/build.gradle.kts
Outdated
@@ -1,3 +1,6 @@ | |||
|
|||
import Config.Libs.Lint.api | |||
import Config.Plugins.android |
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.
Mmmm, I'd just get rid of these from GitHub. Looks like AS keeps adding them back for some reason.
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.
😢
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.
Let's not worry about it, I'd rather not fight AS esp when this isn't code we ship to developers.
And I'd say |
Actually, how about |
@lsirac I downloaded the PR and tried to run the sample app without the proper config and this happened:
I assume we're enabling all social providers by default in the anonymous sample? We probably need to detect which ones are actually configured. |
@lsirac I just tested (manually) the following cases and they all worked great!
I did run into one strange issue, with my testing account [email protected] (I have both Twitter and FB set up).
|
Oh and I was able to make the sample app crash after my "unknown error" by pressing back at the NASCAR screen:
|
@samtstern thanks for catching that! I fixed it and changed a unit test to catch that case. 😄 |
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.
Approved after merge conflicts.
For implementation details, see #1171.