Account Linking Phase 1 #1185

samtstern · 2018-03-13T18:06:39Z

See #1179 and #309

TODO:

Change-Id: I50b05d1e50865c9f2dc5e231569e237c631e0003

Change-Id: Icca4a88adb9423b7f8510c9e338285e68a7965e0

Change-Id: If381ab96faa6684a38de7316f6a825abe0f40625

Change-Id: Ib85147724ce25ef9e757e3d7a496cc3dea8f94c8

samtstern · 2018-03-14T20:06:58Z

@SUPERCILEX am I correct interpreting #309 to allow linking between any pre-signed-in account and not just anonymous?

samtstern · 2018-03-14T21:52:06Z

I am also a little confused how to handle email.

Let's say after CheckEmailFragment we know that the email already exists. Then we know that calling linkWithCredential will fail, so we are definitely going to have a merge conflict.

But, we want to make sure that in the merge conflict handler the AuthCredential object they get is always going to be valid ... and the only way to check if the email credential is valid is to attempt a sign-in which will destroy the Anonymous auth state.

Seems like in #309 you'd give the user a chance to read all data from the anon account before attempting the link, but not sure how it would work here.

Edit 1: after talking to @bojeil-google we need to create a temporary FirebaseApp to verify the credential.

Edit 2: Implemented working version in 5876159

Change-Id: I15adb6fab8306420fa7ca6a9a837e2260691369e

Change-Id: I51da641853cc581ea90aa2d31c2908bed3737e76

Change-Id: Id45100d620ca65ac8b7ca4311daea08987026386

Change-Id: I245ca7ee28e7d0b06bb7b691f44a3a51e1bca4dd

Change-Id: Ic253a2f5cac30e51baef2f33aa0cc88b00db95c6

Change-Id: Ibb5c3f19a8f63c59b7685728e4a92e84d6b7690d

samtstern · 2018-03-15T19:13:54Z

app/src/main/java/com/firebase/uidemo/auth/AccountLinkActivity.java

@@ -0,0 +1,197 @@
+package com.firebase.uidemo.auth;


Decided to do a new sample here since this is an "advanced" feature and I don't want to clutter the main sample.

Ooof! You sure about this? In my PR, I just added a checkbox that said something like "Allow account linking" and a button to open the AuthUiActivity from the signed in one so devs could fully customize the linking experience (I noticed you hardcoded the providers and all that).

If we were going your way with this feature (service, data callbacks, etc) then I'd agree with merging the samples. However in this case since you have to write extra code and really understand the merge-failure-upgrade flow I wanted to give it dedicated UI and keep the code separate so as not so confuse users of the 'basic' AuthUI flow with all the extra stuff.

If you try out the sample, you can see how it makes the various states extra clear.

Gotya, that totally makes sense so 👍.

samtstern · 2018-03-15T19:15:49Z

auth/src/main/java/com/firebase/ui/auth/util/signincontainer/IdpSignInContainer.java

@@ -129,6 +129,7 @@ public void onSaveInstanceState(Bundle outState) {
    @Override
    public void onSuccess(@NonNull final IdpResponse response) {
        AuthCredential credential = ProviderUtils.getAuthCredential(response);
+        // TODO: Probably need to allow linking here


Note to self: need to check out this piece.

samtstern · 2018-03-15T19:16:32Z

auth/src/main/java/com/firebase/ui/auth/util/UpgradeFailureListener.java

+    protected void onUpgradeFailure(@NonNull FirebaseAuthUserCollisionException e) {
+        // In the case of Phone Auth the exception contains an updated credential we can
+        // user since the other credential is expired after one use.
+        AuthCredential credential = e.getUpdatedCredential() != null


So this is a cool trick I learned: you get a collision exception from Phone Auth the exception gives you a try-again credential so you don't have to show the UI again.

Ooooooh, fancy!

(In your comment, I think you meant use, not user. 😉 Everything starts looking like a user in Firebase Auth! 😆)

Change-Id: I7db94a0b5c5f7fc515278dcb82c2e51a1ac5da48

samtstern · 2018-03-23T04:22:46Z

Removing this from 3.3.0 as #1189 dropped a nuclear merge bomb on this :-)

SUPERCILEX · 2018-03-23T04:39:31Z

Oh noes! 😞 I completely forgot about this PR, sorry man!

samtstern · 2018-03-23T04:41:18Z

@SUPERCILEX haha no worries, I knew exactly what I was getting into.

Change-Id: Ibdf3e61f6dc1e1578640cb66562a217bf2fd74c0

Change-Id: Ib7956224986259b388b570d59c2f65b00251f8dc

Change-Id: I7eb7675c29c5d03c292499d9517d064bd3a02315

samtstern · 2018-03-23T18:44:33Z

@SUPERCILEX I've got the merge conflicts squared away.

The two places I am not sure how to apply the linking logic are in SocialProviderResponseHandler and LinkingSocialProviderResponseHandler.

Mind taking a look?

SUPERCILEX · 2018-03-23T19:30:49Z

Will do! I just realized I forgot some loading stuff in the providers... so PR incoming for that first! 😊

SUPERCILEX

Alrighty, I'm starting off with some general comments.

The gist I'm getting is you're creating a temporary, fake firebase app to test whether or not a credential already exists, thus seeing if there's going to be a collision. While that's crazy cool and I learned something new, I think it's a bit over the top . (Show off! 😆) In my PR, I literally just added an if (e instanceof FirebaseAuthUserCollisionException) in the welcome back stuff and returned an error if it was the case. I'm probably missing something though...

So for the SocialProviderResponseHandler, we'll want to add this:
https://github.com/SUPERCILEX/FirebaseUI-Android/blob/460f4307089d3933e2517058758147d725eea567/auth/src/main/java/com/firebase/ui/auth/ui/idp/CredentialSignInHandler.java#L105-L127
Except instead of performing the service merge, you'll return the pending credential.

For the LinkingSocialProviderResponseHandler, that's where you simply add the if statement and return the pending credential if it fails.

PS: I've forgotten, is the service thing permanently out of the picture? Or why aren't we doing it here? I feel like we've already talked about this somewhere, so sorry for the repetition.

SUPERCILEX · 2018-03-23T21:16:32Z

app/src/main/java/com/firebase/uidemo/auth/AccountLinkActivity.java

@@ -0,0 +1,197 @@
+package com.firebase.uidemo.auth;


Ooof! You sure about this? In my PR, I just added a checkbox that said something like "Allow account linking" and a button to open the AuthUiActivity from the signed in one so devs could fully customize the linking experience (I noticed you hardcoded the providers and all that).

SUPERCILEX · 2018-03-23T21:20:08Z

auth/src/main/java/com/firebase/ui/auth/IdpResponse.java

@@ -61,25 +63,36 @@ public IdpResponse createFromParcel(Parcel in) {

    private final FirebaseUiException mException;

+    private final AuthCredential mPendingCredential;


Could you move this above the exception?

SUPERCILEX · 2018-03-23T21:53:38Z

auth/src/main/java/com/firebase/ui/auth/IdpResponse.java

+        if (mException != null ? !mException.equals(response.mException) : response.mException != null) {
+            return false;
+        }
+        if (mPendingCredential != null ? !mPendingCredential.equals(response.mPendingCredential) : response.mPendingCredential != null) {


These if statements are kinda confusing... Also, the AuthCredential doesn't implement equals properly so adding that will make the response never have an equal. I think if you just add this line it'll work out:

&& (mPendingCredential == null ? response.mPendingCredential == null : mPendingCredential.getProvider().equals(response.mPendingCredential.getProvider()))

To be honest I just let IntelliJ write this, and since these things are now marked @Nullable this is what it came up with.

But good call on the AuthCredential part.

SUPERCILEX · 2018-03-23T21:54:15Z

auth/src/main/java/com/firebase/ui/auth/IdpResponse.java

+        result = 31 * result + (mToken != null ? mToken.hashCode() : 0);
+        result = 31 * result + (mSecret != null ? mSecret.hashCode() : 0);
+        result = 31 * result + (mException != null ? mException.hashCode() : 0);
+        result = 31 * result + (mPendingCredential != null ? mPendingCredential.hashCode() : 0);


Same here, we should just hash code the provider:

result = 31 * result + (mPendingCredential == null ? 0 : mPendingCredential.getProvider().hashCode());

SUPERCILEX · 2018-03-23T21:55:51Z

auth/src/main/java/com/firebase/ui/auth/data/remote/EmailSignInHandler.java

                setResult(Resource.forSuccess(response));
+            } else {
+                // TODO: Will this correctly pass back anonymous errors?


Yeah, I'm pretty sure it will.

SUPERCILEX · 2018-03-23T22:02:56Z

auth/src/main/java/com/firebase/ui/auth/ui/idp/AuthMethodPickerActivity.java

+import com.firebase.ui.auth.util.data.ProviderUtils;
+import com.google.android.gms.tasks.OnSuccessListener;
+import com.google.firebase.auth.AuthCredential;
+import com.google.firebase.auth.AuthResult;


?. Probs refactor mistake

SUPERCILEX · 2018-03-23T22:08:55Z

auth/src/main/java/com/firebase/ui/auth/util/UpgradeFailureListener.java

+    protected void onUpgradeFailure(@NonNull FirebaseAuthUserCollisionException e) {
+        // In the case of Phone Auth the exception contains an updated credential we can
+        // user since the other credential is expired after one use.
+        AuthCredential credential = e.getUpdatedCredential() != null


Ooooooh, fancy!

(In your comment, I think you meant use, not user. 😉 Everything starts looking like a user in Firebase Auth! 😆)

SUPERCILEX · 2018-03-23T22:10:22Z

auth/src/main/java/com/firebase/ui/auth/util/UpgradeFailureListener.java

+
+    protected void onUpgradeFailure(@NonNull IdpResponse response) {}
+
+    protected abstract void onNonUpgradeFailure(@NonNull Exception e);


How about calling it onUnknownFailure?

SUPERCILEX · 2018-03-23T22:11:10Z

auth/src/main/java/com/firebase/ui/auth/util/UpgradeFailureListener.java

+        }
+    }
+
+    protected void onUpgradeFailure(@NonNull FirebaseAuthUserCollisionException e) {


Can we make this final? I got confused about why you weren't calling super until I realized there was an overload. Or make it private and call it handleUpgradleFailure?

SUPERCILEX · 2018-03-23T22:17:05Z

auth/src/main/java/com/firebase/ui/auth/util/AnonymousUpgradeUtils.java

+    /**
+     * Uses type system to enforce the proper failure listener.
+     */
+    public static class UpgradeTaskWrapper<T> {


I like this idea, but it doesn't really scale. Or is the plan to force us to always add a failure listener first?

Yeah that was the plan, to make sure that we always handle that error case. But you're right that it doesn't quite work, because in some places we have continuations.

SUPERCILEX · 2018-03-24T19:15:07Z

Huh, I guess embedded code snippets don't work on forks. 😭

samtstern · 2018-03-26T15:19:56Z

@SUPERCILEX thanks for the comments.

The temporary FirebaseApp trick isn't to check for a collision, it's for when you know there's going to be a collision and want to make sure you're giving the user a valid AuthCredential. So you can check if the credential is valid but without messing up the current sign-in state, so they still have a chance to transfer data.

As for why we're not doing the Service thing: basically because in the API discussion for doing this feature cross-platform the only thing we could agree on in the short term was something minimal and correct like this, while leaving room to improve the developer experience in the future.

SUPERCILEX · 2018-03-26T18:41:53Z

Ohhhhhhhhh, I see. Thanks for clarifying! Re-reading your comments though, it does look like you're checking for a conflict. Maybe change them to say we're expecting a conflict, but want to ensure the credential is usable?

As for the service, thanks, I remember now.

samtstern · 2018-06-13T00:01:46Z

Closing this PR, it's very very conflicty and @lsirac is going to take over this task.

samtstern changed the title ~~Very basic linking~~ [WIP] Account linking phase 1 Mar 13, 2018

Very basic linking

cab8213

Change-Id: I50b05d1e50865c9f2dc5e231569e237c631e0003

samtstern force-pushed the anonymous-upgrade-phase-1 branch from 01f1ca1 to cab8213 Compare March 13, 2018 18:25

samtstern added 3 commits March 13, 2018 13:48

Fix tests

fcc57f1

Change-Id: Icca4a88adb9423b7f8510c9e338285e68a7965e0

Basic sample app

faf6d3c

Change-Id: If381ab96faa6684a38de7316f6a825abe0f40625

Standardize error handling

a5ed59c

Change-Id: Ib85147724ce25ef9e757e3d7a496cc3dea8f94c8

samtstern added 6 commits March 14, 2018 15:38

Properly handle existing email users

5876159

Change-Id: I15adb6fab8306420fa7ca6a9a837e2260691369e

Clean up and simplify

4be81ae

Change-Id: I51da641853cc581ea90aa2d31c2908bed3737e76

Fix lint

aa42772

Change-Id: Id45100d620ca65ac8b7ca4311daea08987026386

Support phone auth with updated credential

0405aa9

Change-Id: I245ca7ee28e7d0b06bb7b691f44a3a51e1bca4dd

Make tests pass

24781f9

Change-Id: Ic253a2f5cac30e51baef2f33aa0cc88b00db95c6

Lint found a context leak. Thanks lint

8dc3eec

Change-Id: Ibb5c3f19a8f63c59b7685728e4a92e84d6b7690d

samtstern changed the title ~~[WIP] Account linking phase 1~~ Account Linking Phase 1 Mar 15, 2018

samtstern commented Mar 15, 2018

View reviewed changes

Add upgrade flow in IdpContainer

67d6cef

Change-Id: I7db94a0b5c5f7fc515278dcb82c2e51a1ac5da48

samtstern requested a review from SUPERCILEX March 15, 2018 19:28

samtstern added this to the 3.3.0 milestone Mar 15, 2018

samtstern removed this from the 3.3.0 milestone Mar 23, 2018

samtstern changed the title ~~Account Linking Phase 1~~ [ON HOLD] Account Linking Phase 1 Mar 23, 2018

samtstern added 3 commits March 23, 2018 11:02

Merge branch 'version-3.3.0-dev' into anonymous-upgrade-phase-1

37a0929

Change-Id: Ibdf3e61f6dc1e1578640cb66562a217bf2fd74c0

Upgrade task helper

4edadf3

Change-Id: Ib7956224986259b388b570d59c2f65b00251f8dc

undo some changes

6fd0226

Change-Id: I7eb7675c29c5d03c292499d9517d064bd3a02315

samtstern changed the title ~~[ON HOLD] Account Linking Phase 1~~ [Account Linking Phase 1 Mar 23, 2018

samtstern changed the title ~~[Account Linking Phase 1~~ Account Linking Phase 1 Mar 23, 2018

SUPERCILEX reviewed Mar 24, 2018

View reviewed changes

samtstern changed the base branch from version-3.3.0-dev to version-3.3.1-dev March 29, 2018 15:29

samtstern changed the base branch from version-3.3.1-dev to version-3.4.0-dev April 16, 2018 16:59

samtstern changed the base branch from version-3.4.0-dev to version-4.0.0-dev May 4, 2018 17:24

curiousily mentioned this pull request May 27, 2018

Anonymous user and account conversion to permanent account #123

Closed

samtstern closed this Jun 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account Linking Phase 1 #1185

Account Linking Phase 1 #1185

samtstern commented Mar 13, 2018 •

edited by lsirac

samtstern commented Mar 14, 2018

samtstern commented Mar 14, 2018 •

edited

samtstern Mar 15, 2018

SUPERCILEX Mar 23, 2018

samtstern Mar 26, 2018

SUPERCILEX Mar 26, 2018

samtstern Mar 15, 2018

samtstern Mar 15, 2018

SUPERCILEX Mar 23, 2018

samtstern commented Mar 23, 2018

SUPERCILEX commented Mar 23, 2018

samtstern commented Mar 23, 2018

samtstern commented Mar 23, 2018

SUPERCILEX commented Mar 23, 2018

SUPERCILEX left a comment •

edited

SUPERCILEX Mar 23, 2018

SUPERCILEX Mar 23, 2018

SUPERCILEX Mar 23, 2018

samtstern Mar 26, 2018

SUPERCILEX Mar 23, 2018

SUPERCILEX Mar 23, 2018

SUPERCILEX Mar 23, 2018

SUPERCILEX Mar 23, 2018

SUPERCILEX Mar 23, 2018

SUPERCILEX Mar 23, 2018

SUPERCILEX Mar 23, 2018

samtstern Mar 26, 2018

SUPERCILEX commented Mar 24, 2018 •

edited

samtstern commented Mar 26, 2018

SUPERCILEX commented Mar 26, 2018

samtstern commented Jun 13, 2018

		@@ -61,25 +63,36 @@ public IdpResponse createFromParcel(Parcel in) {

		private final FirebaseUiException mException;

		private final AuthCredential mPendingCredential;


		protected void onUpgradeFailure(@NonNull IdpResponse response) {}

		protected abstract void onNonUpgradeFailure(@NonNull Exception e);

Account Linking Phase 1 #1185

Account Linking Phase 1 #1185

Conversation

samtstern commented Mar 13, 2018 • edited by lsirac

samtstern commented Mar 14, 2018

samtstern commented Mar 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samtstern commented Mar 23, 2018

SUPERCILEX commented Mar 23, 2018

samtstern commented Mar 23, 2018

samtstern commented Mar 23, 2018

SUPERCILEX commented Mar 23, 2018

SUPERCILEX left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SUPERCILEX commented Mar 24, 2018 • edited

samtstern commented Mar 26, 2018

SUPERCILEX commented Mar 26, 2018

samtstern commented Jun 13, 2018

samtstern commented Mar 13, 2018 •

edited by lsirac

samtstern commented Mar 14, 2018 •

edited

SUPERCILEX left a comment •

edited

SUPERCILEX commented Mar 24, 2018 •

edited