Support account linking #309

SUPERCILEX · 2016-09-17T19:50:01Z

If this has been discussed in an issue, make sure to mention the issue number here. If not, go file an issue about this to make sure this is a desirable change.

If this is a new feature please co-ordinate with someone on FirebaseUI-iOS to make sure that we can implement this on both platforms and maintain feature parity.

Anyone?

TODO:

Make sure linking non-anonymous accounts works
Add documentation
Update sample
Wait for Refactor SaveCredentialsActivity #254 to get merged
Add tests
Update documentation to IdpResponse
Fix profile and name not being added (turns out this is also caused by displayName returning null firebase-ui-auth 1.0.0 #409)

SUPERCILEX

Stuff that needs to be addressed before the PR is merged.

SUPERCILEX · 2016-09-17T19:56:31Z

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

+        FirebaseUser user = FirebaseAuth.getInstance().getCurrentUser();
+        if (user != null) {
+            if (!user.isAnonymous()) {
+                startActivity(SignedInActivity.createIntent(this));


Should I make SignedInActivity and AuthUiActivity fragments and put them in a FragmentStatePagerAdapter so that people can link multiple accounts?

Let's discuss changes to the sample last, I want to focus on the changes to the library at first since that will have effects on how the sample is structured.

SUPERCILEX · 2016-09-17T20:27:14Z

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

@@ -309,6 +311,10 @@ public SignInIntentBuilder createSignInIntentBuilder() {
        return new SignInIntentBuilder();
    }

+    public void notifyOnMergeFailedListeners(String prevUid) {


I'm pretty sure this is not okay, but I don't know how to get rid of it. How would I serialize the listener into FlowParameters?

SUPERCILEX · 2016-09-17T20:28:25Z

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

@@ -417,6 +425,15 @@ public SignInIntentBuilder setIsSmartLockEnabled(boolean enabled) {
            return this;
        }

+        /**
+         * Links the current user to a new one.


TODO: bad wording

SUPERCILEX · 2016-09-17T20:33:23Z

auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackIDPPrompt.java

+                            FirebaseAuth auth = FirebaseAuth.getInstance();
+                            FirebaseUser user = auth.getCurrentUser();
+                            if (!task.isSuccessful() && user != null && mPrevCredential != null) {
+                                user.delete();


This is terrible idea, but user will be the new user if I call delete() in the OnCompleteListener because the variable is a reference.

Can you explain exactly what you're doing in this bit? I'm not clear on what you are trying to achieve.

SUPERCILEX · 2016-09-17T20:33:58Z

auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackIDPPrompt.java

+                                            public void onComplete(@NonNull Task<AuthResult> task) {
+                                                if (task.isSuccessful()) {
+                                                    AuthUI.getInstance()
+                                                            .notifyOnMergeFailedListeners(prevUid);


This is where I want to call user.delete().

In this block you are notifying the merge failed listeners on successful sign in, why is that the case?

SUPERCILEX · 2016-09-17T20:34:22Z

auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackIDPPrompt.java

+                            FirebaseUser user = auth.getCurrentUser();
+                            if (!task.isSuccessful() && user != null && mPrevCredential != null) {
+                                user.delete();
+                                final String prevUid = user.getUid();


Need this because of java references.

SUPERCILEX · 2016-09-17T20:37:14Z

auth/src/test/java/com/firebase/ui/auth/test_helpers/TestHelper.java

@@ -57,7 +57,8 @@ public static FlowParameters getFlowParameters(Context context, List<String> pro
                AuthUI.getDefaultTheme(),
                AuthUI.NO_LOGO,
                null,
-                true);
+                true,
+                false);


We should probably make a test for this.

SUPERCILEX · 2016-09-17T22:45:20Z

I have one bug that I know of that isn't fixed yet: when going from an anonymous account that doesn't have a name or profile picture to a Google account, the profile picture and name don't get added to the account. Only the email gets added. Does anyone know why this is happening? I could just add a UserProfileChangeRequest once the account is converted, but that complicates things.

samtstern · 2016-09-20T16:42:04Z

@SUPERCILEX thank you (again!) for the big contribution, we really appreciate how involved you are with this repo lately. I will take a look and leave comments, but as you know, we try to keep FirebaseUI-Android in sync with FirebaseUI-iOS, so before this feature could be released we'd have to get some commitment there.

That said if things look good we can always put this on a feature branch and merge it back into master or version-1.0.0-dev later.

samtstern · 2016-09-20T16:45:57Z

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

+        FirebaseUser user = FirebaseAuth.getInstance().getCurrentUser();
+        if (user != null) {
+            if (!user.isAnonymous()) {
+                startActivity(SignedInActivity.createIntent(this));


Let's discuss changes to the sample last, I want to focus on the changes to the library at first since that will have effects on how the sample is structured.

samtstern · 2016-09-20T16:48:19Z

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

+        /**
+         * Links the current user to a new one.
+         */
+        public SignInIntentBuilder linkWithCurrentUser(OnMergeFailedListener onMergeFailedListener) {


I find the pattern of adding a listener to an Intent builder strange. The result of the action in the Intent should be returned via onActivityResult if the Activity that fired it did so for result.

samtstern · 2016-09-20T16:50:06Z

auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackIDPPrompt.java

+                            FirebaseAuth auth = FirebaseAuth.getInstance();
+                            FirebaseUser user = auth.getCurrentUser();
+                            if (!task.isSuccessful() && user != null && mPrevCredential != null) {
+                                user.delete();


Can you explain exactly what you're doing in this bit? I'm not clear on what you are trying to achieve.

samtstern · 2016-09-20T16:51:32Z

auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackIDPPrompt.java

+                                            public void onComplete(@NonNull Task<AuthResult> task) {
+                                                if (task.isSuccessful()) {
+                                                    AuthUI.getInstance()
+                                                            .notifyOnMergeFailedListeners(prevUid);


In this block you are notifying the merge failed listeners on successful sign in, why is that the case?

samtstern · 2016-09-20T16:52:58Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

+        }
+    }
+
+    private void addListeners(Task<AuthResult> task, final String name, final String password) {


IMO this is a confusing abstraction, why not just add the listeners directly as before?

samtstern · 2016-09-20T16:54:04Z

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

+
+        if (user != null && mActivityHelper.getFlowParams().shouldLinkUser) {
+            user
+                    .linkWithCredential(credential)


style nit: prefer not to have a continuation for the first method call in a chain if it would fit on the previous line. user is just kinda floating out there.

samtstern · 2016-09-20T16:57:11Z

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

+                    .addOnCompleteListener(handler);
+        } else {
+            firebaseAuth
+                    .signInWithCredential(credential)


Seems like in this if/else block you are attaching the same complete/failure listener but switching on the Task type. Would be clearer to do:

// Either sign the user in or link the credential Task<AuthResult> authTask; if (user != null && mActivityHelper.getFlowParams().shouldLinkUser) { authTask = user.linkWithCredential(credential); } else { authTask = firebaseAuth.signInWithCredential(credential); } task.addOnFailureListener(logger); task.addOnCompleteListener(handler);

samtstern · 2016-09-20T17:00:22Z

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

@@ -0,0 +1,11 @@
+package com.firebase.ui.auth.util;
+
+public interface OnMergeFailedListener {


Building on my previous comment, I don't think we need to use the listener pattern for merge failures. The only information returned on a merge failure is the previous user ID, so why not just do it through Activity results? The developer could handle it like this?

@Override public void onActivityResult(int requestCode, int resultCode, Intent data) { super.onActivityResult(requestCode, resultCode, data); if (requestCode == RC_SIGN_IN) { if (resultCode == RESULT_MERGE_FAILED) { String prevUid = data.getStringExtra(EXTRA_PREV_UID); // Handle merge failure .... } } }

This also solves the issue you raised of needing to somehow store the listener reference in the FlowParams, which won't work (as you mentioned).

SUPERCILEX · 2016-09-25T01:53:14Z

SignInActivity test failing at

assertEquals(
                TestConstants.EMAIL,
                nextIntent.intent.getExtras().getString(ExtraConstants.EXTRA_EMAIL)

Not sure why...

SUPERCILEX

Addressed all your comments. Thanks!

SUPERCILEX · 2016-09-25T01:13:29Z

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

+    public Intent getMergeFailedIntent() {
+        FirebaseUser user = getCurrentUser();
+        if (mFlowParams.shouldLinkUser && user != null) {
+            if (user.isAnonymous()) user.delete();


Is this the expected behavior or should I be deleting any user anonymous or not?

I don't think there's a need to delete anonymous users here.

I don't think we want to delete the anonymous user.

I'm deleting the anonymous user here because the expected usage of getMergeFailedIntent() is that a non anonymous user will be signed into immediately after this method is called. If I don't delete the anonymous users, there will be a bunch of them hanging around that will never be user again. AFAIK, you can't log back into an existing anonymous user so not deleting them would make a messy user base in the Firebase console.

What I would really like to do is to delete the anonymous user once I'm signed in, but I can't figure out how to do that because I keep getting references to the newly signed in user.

If you believe we should play it safe and not delete the anonymous user, that's fine with me but I had one last idea: maybe I could delete the anonymous user and if signing into the new user fails, I just create a new anonymous user and pass down the old anonymous user's id through the Intent. What do you think? It would probably just be easier to not delete the anonymous user, but again, I take issue with the messy Firebase console. Do you know if Firebase automatically delete anonymous users if they haven't logged in for a while? That would solve the issue.

We are looking for ways to clean up "zombie" anonymous users in the Firebase console after a while, so let's not delete the users and count on that happening eventually.

SUPERCILEX · 2016-09-25T01:14:53Z

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

                break;
            case RC_PLAY_SERVICES:
                if (resultCode != RESULT_OK) {
-                    finish(resultCode, new Intent());


Was there a reason for these new intents? I need them to pass the uid down the chain of activities. This is where #320 would come in handy

You can use these intents to plumb data back through to the calling activities, I think you're doing the right thign.

Your change is correct, the new Intent() was in the absence of anything important to plumb down.

SUPERCILEX · 2016-09-25T01:56:10Z

auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackIDPPrompt.java

+                                            public void onComplete(@NonNull Task<AuthResult> task) {
+                                                if (task.isSuccessful()) {
+                                                    mActivityHelper.dismissDialog();
+                                                    finish(Activity.RESULT_OK, getIntent());


This stuff is much clearer now.

I'm actually not sure what's going on here. It looks like if we fail to link with a credential we try to sign in anyway. If the sign in succeeds (but the linking did not) we say it was a success?

SUPERCILEX · 2016-09-25T02:03:28Z

auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackIDPPrompt.java

+                        public void onComplete(@NonNull Task<AuthResult> task) {
+                            FirebaseAuth auth = FirebaseAuth.getInstance();
+                            if (!task.isSuccessful() && task.getException() instanceof FirebaseAuthUserCollisionException) {
+                                setIntent(new Intent().putExtras(mActivityHelper.getMergeFailedIntent()));


The is the new way I'm managing sending the old uid.

nit: any reason not to just drop the new Intent() piece?

SUPERCILEX · 2016-09-25T02:19:54Z

auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackIDPPrompt.java

+                        public void onComplete(@NonNull Task<AuthResult> task) {
+                            FirebaseAuth auth = FirebaseAuth.getInstance();
+                            if (!task.isSuccessful() && task.getException() instanceof FirebaseAuthUserCollisionException) {
+                                setIntent(new Intent().putExtras(mActivityHelper.getMergeFailedIntent()));


The new version is a little bit different, but I'm basically doing the same thing as before.
If the task isn't successful, it means the link failed so I need to delete the anonymous user and get the old uid. delete() also signs out the user so I then need to reauthenticate the user and pass on the intent containing the old uid to be manually merged. This scenario only happens when the Google/Facebook/Twitter etc. account already exists.

samtstern

This looks better now. Left a few more comments.

samtstern · 2016-09-28T18:27:55Z

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

@@ -150,6 +161,42 @@ protected void onActivityResult(int requestCode, int resultCode, Intent data) {
    @MainThread
    private void handleSignInResponse(int resultCode, Intent data) {
        if (resultCode == RESULT_OK) {
+            final String prevUid = data.getStringExtra(ExtraConstants.EXTRA_MERGE_FAILED);
+            if (prevUid != null) {
+                FirebaseDatabase.getInstance()


Can we remove all this? There shouldn't be anything referencing the DB or the chat sample in the AuthUI sample.

We can remove it, but I think it should be moved to the documentation so that beginners know what to do with the previous user id.

Ok that's fine, documentation comes last anyway. Let's save that for another time.

samtstern · 2016-09-28T18:28:35Z

app/src/main/java/com/firebase/uidemo/database/Chat.java

@@ -0,0 +1,28 @@
+package com.firebase.uidemo.database;


nit: let's just leave chat where it was. We don't need to access it outside of that context anyway.

samtstern · 2016-09-28T18:29:22Z

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

+         *
+         * <p>Linking is disabled by default.
+         */
+        public SignInIntentBuilder linkWithCurrentUser(boolean shouldLinkUser) {


nit: name this setShouldLinkUser or something more consistent with setIsSmartLockEnabled and the others.

samtstern · 2016-09-28T18:29:53Z

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

+    public Intent getMergeFailedIntent() {
+        FirebaseUser user = getCurrentUser();
+        if (mFlowParams.shouldLinkUser && user != null) {
+            if (user.isAnonymous()) user.delete();


I don't think we want to delete the anonymous user.

samtstern · 2016-09-28T18:30:25Z

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

                break;
            case RC_PLAY_SERVICES:
                if (resultCode != RESULT_OK) {
-                    finish(resultCode, new Intent());


Your change is correct, the new Intent() was in the absence of anything important to plumb down.

samtstern · 2016-09-28T18:31:21Z

auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackIDPPrompt.java

+                        public void onComplete(@NonNull Task<AuthResult> task) {
+                            FirebaseAuth auth = FirebaseAuth.getInstance();
+                            if (!task.isSuccessful() && task.getException() instanceof FirebaseAuthUserCollisionException) {
+                                setIntent(new Intent().putExtras(mActivityHelper.getMergeFailedIntent()));


nit: any reason not to just drop the new Intent() piece?

samtstern · 2016-09-28T18:32:48Z

auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackIDPPrompt.java

+                        @Override
+                        public void onComplete(@NonNull Task<AuthResult> task) {
+                            FirebaseAuth auth = FirebaseAuth.getInstance();
+                            if (!task.isSuccessful() && task.getException() instanceof FirebaseAuthUserCollisionException) {


nit: prefer separate onSuccess and onFailure listeners here, rather than checking isSuccessful() in the onCompleteListener

samtstern · 2016-09-28T18:34:35Z

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

-                    null /* password */, mResponse.getProviderType());
+            SmartlockUtil.saveCredentialOrFinish(mActivity,
+                                                 mSaveCredentialsResultCode,
+                                                 new Intent(),


nit: I'd prefer to pass null /* intent */ here and have saveCredentialOrFinish handle the null case.

SUPERCILEX

Addressed your comments.

SUPERCILEX · 2016-10-01T05:27:45Z

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

@@ -150,6 +161,42 @@ protected void onActivityResult(int requestCode, int resultCode, Intent data) {
    @MainThread
    private void handleSignInResponse(int resultCode, Intent data) {
        if (resultCode == RESULT_OK) {
+            final String prevUid = data.getStringExtra(ExtraConstants.EXTRA_MERGE_FAILED);
+            if (prevUid != null) {
+                FirebaseDatabase.getInstance()


We can remove it, but I think it should be moved to the documentation so that beginners know what to do with the previous user id.

SUPERCILEX · 2016-10-01T06:00:30Z

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

+    public Intent getMergeFailedIntent() {
+        FirebaseUser user = getCurrentUser();
+        if (mFlowParams.shouldLinkUser && user != null) {
+            if (user.isAnonymous()) user.delete();


I'm deleting the anonymous user here because the expected usage of getMergeFailedIntent() is that a non anonymous user will be signed into immediately after this method is called. If I don't delete the anonymous users, there will be a bunch of them hanging around that will never be user again. AFAIK, you can't log back into an existing anonymous user so not deleting them would make a messy user base in the Firebase console.

What I would really like to do is to delete the anonymous user once I'm signed in, but I can't figure out how to do that because I keep getting references to the newly signed in user.

If you believe we should play it safe and not delete the anonymous user, that's fine with me but I had one last idea: maybe I could delete the anonymous user and if signing into the new user fails, I just create a new anonymous user and pass down the old anonymous user's id through the Intent. What do you think? It would probably just be easier to not delete the anonymous user, but again, I take issue with the messy Firebase console. Do you know if Firebase automatically delete anonymous users if they haven't logged in for a while? That would solve the issue.

SUPERCILEX · 2016-10-01T06:02:38Z

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

                                              FlowParameters parameters,
                                              FirebaseUser firebaseUser,
                                              @Nullable String password,
                                              @Nullable String provider) {
+        if (data != null) {
+            data = new Intent();
+        }


Is this what you were looking for?

I think you mean if (data == null) not !=.

Yeah, I fixed it in a newer diff.

SUPERCILEX · 2016-10-01T06:03:36Z

Any idea why the test is failing?

SUPERCILEX · 2016-10-01T06:50:21Z

Figured out why the test was failing: it might be a bug in the Android framework.

This works:

data.putExtras(SaveCredentialsActivity.createIntent(
    activity,
    parameters,
    firebaseUser,
    password,
    provider));

This doesn't:

SaveCredentialsActivity.createIntent(
    activity,
    parameters,
    firebaseUser,
    password,
    provider).putExtras(data);

I have no idea why one works, but not the other. Any thoughts?

SUPERCILEX

The above didn't work either, so as a workaround, I reset the Intent in each Activity calling saveCredentialOrFinish. I don't like this, see my comment below.

SUPERCILEX · 2016-10-01T18:25:54Z

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

+     *
+     * @param activity     the calling Activity.
+     * @param requestCode  request code to use when starting the save operation.
+     * @param data         An Intent to be merged. Must be a {@code new Intent().putExtras(...)}.


I'm really not a fan of this: I had to switch everything to be a new Intent() to workaround the merge bug. Do you think we should just make a parameter prevUid?

Can you explain a little what happened when you did the merge the other way? What's the buggy behavior you're seeing?

See pics:

The two differeent ways of merging intents:

The left intent merge up top produces the left version below and vice versa:

So basically, the left version works in terms of keeping the email alive, but the it doesn't in the sense that it says it comes from the class that it originated from eg. SignInActivity instead of SaveCredentialsActivity (the test expects SaveCredentialsActivity).

Ok I just looked through some Android source and here's whats happening. When you merge the extras of two bundles (say dst.Extras(src)) it basically does this (pseudocode):

for (String key : src.getExtras().getKeys()) { dst.putExtra(key, src.getExtra(key)); }

This means for a given key if src.getExtra(key) is null then it will erase the extra with the same key in dst. I think that's why your email value is disappearing.

Maybe you want to create a utility method to merge extras only if they're non-null? I don't want to reverse the order of operations here just to get around the bug, as you said it makes the result of the intent counter-intuitive.

Thanks for looking into that! Do you have any ideas on how to not use instanceof?

I'm stuck with something like this:

for (String key : intent.getExtras().keySet()) { Object o = intent.getExtras().get(key); if (o != null) { if (o instanceof String) { data.putExtra(key, (String) o); } else if... // More sadness } }

Oooh yeah that's not gonna be pretty, you're right. Not sure there's any way around that.

Maybe let's find out why there's a null extra in this situation and just prevent it from happening in the first place?

YESSSSS!!!! It took me like a half hour of debugging, but it finally dawned on me to actually look at what the test was doing:

Real life in AcquireEmailHelper:

if (email != null && !email.isEmpty()) { // Only proceeds if email is not null }

Test:

Intent startIntent = SignInActivity.createIntent( RuntimeEnvironment.application, TestHelper.getFlowParameters( RuntimeEnvironment.application, Collections.<String>emptyList()), ***null***);

So anyway, I fixed the test and life is good again. The end.

@SUPERCILEX 👍 💥

samtstern · 2016-10-04T17:38:31Z

@amandle can you take a look at this CL as well? It's very complex and I would like a second set of eyes.

amandle

Thanks for the pull request! I have just a few questions/suggestions

amandle · 2016-10-06T17:30:47Z

auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackIDPPrompt.java

+                                            public void onComplete(@NonNull Task<AuthResult> task) {
+                                                if (task.isSuccessful()) {
+                                                    mActivityHelper.dismissDialog();
+                                                    finish(Activity.RESULT_OK, getIntent());


I'm actually not sure what's going on here. It looks like if we fail to link with a credential we try to sign in anyway. If the sign in succeeds (but the linking did not) we say it was a success?

amandle · 2016-10-06T17:36:27Z

auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackPasswordPrompt.java

@@ -114,6 +113,7 @@ protected void onActivityResult(int requestCode, int resultCode, Intent data) {

    private void next(String email, final String password) {
        final FirebaseAuth firebaseAuth = mActivityHelper.getFirebaseAuth();
+        setIntent(getIntent().putExtras(mActivityHelper.getMergeFailedIntent()));


I think this might be clearer if this were closer to where it is used on line 145

It can't be used anywhere after signInWithEmailAndPassword because then I wouldn't be saving the old uid.

amandle · 2016-10-06T17:41:41Z

auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailActivity.java

-                .addOnFailureListener(new TaskFailureLogger(TAG, "Error creating user"))
+        FirebaseUser user = firebaseAuth.getCurrentUser();
+        Task<AuthResult> task;
+        setIntent(getIntent().putExtras(mActivityHelper.getMergeFailedIntent()));


this only needs to happen in the true case of the if statement on line 145

Woops! That's actually not supposed to happen at all since we're linking with a new credential (eg the account doesn't exist yet).

samtstern · 2016-10-06T18:05:47Z

My "approval" is a hand off to @amandle who understands the logic best. I just have the following final requests:

Undo the sample changes that integrate the DB, as mentioned in previous comments.
This should either be merged into a feature branch or the version-1.0.0-dev branch. Feature branch might be safest since iOS is not guaranteed to do this in 1.0.

SUPERCILEX · 2016-10-08T20:10:46Z

@amandle Because Sam was confused too, I added an explanation to what I'm doing in WelcomeBackIDPPrompt.java: https://github.com/firebase/FirebaseUI-Android/pull/309/files#diff-7c82ecab41019b31cff372a5f3480a02R212.

SUPERCILEX · 2016-10-08T20:17:23Z

@samtstern Thanks for all your help so far!!!

I undid the changes in the sample
I created Merge changes from master into v1.0.0 #344 because this PR comes from master and I wanted to make it clearer that master would be merged into v1.0.0 by merging this PR. Once Merge changes from master into v1.0.0 #344 is merged, we can change the branch this PR is being merged into to v1.0.0 without the changes from master also showing up as changes from this PR.

SUPERCILEX · 2016-10-12T04:27:58Z

auth/README.md

@@ -197,14 +197,27 @@ startActivityForResult(
    RC_SIGN_IN);
 ```

+If anonymous user account conversion to permanent is required:


Thoughts on documentation?

I would expand this sentence as there are a lot of advanced concepts at play here. Something like this:

"The default FirebaseUI sign-in flow shows UI to either create a new account or sign into an existing account. If you are using anonymous authentication (link) in your application before calling FirebaseUI, you may want to link the anonymous account to the permanent account the user selects in the UI flow."

SUPERCILEX · 2016-10-12T04:30:41Z

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

+         *
+         * <p>Linking is disabled by default.
+         */
+        public SignInIntentBuilder setShouldLinkAccounts(boolean shouldLinkAccounts) {


Thoughts on the new name? setShouldLinkAccounts vs setShouldLinkUser. I prefer the first but it's up to debate.

I like the new name, since we call the feature "Account Linking"

hijamoya · 2016-11-10T09:12:45Z

any update of this feature? we need this function. Thanks!!

SUPERCILEX · 2016-11-10T15:07:04Z

This waiting on #357

SUPERCILEX · 2016-11-19T06:15:42Z

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

+        } else {
+            ((TextView) findViewById(R.id.idp_secret)).setText(secret);
+        }
+    }


I did some cleanup here so that if IdpResponse is null, we don't show the layouts.

SUPERCILEX · 2016-11-19T06:16:12Z

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

+        if (prevUid == null) {
+            findViewById(R.id.prev_uid_layout).setVisibility(View.GONE);
+        } else {
+            ((TextView) findViewById(R.id.prev_uid)).setText(prevUid);


Is this an appropriate update to the sample?

SUPERCILEX · 2016-11-19T06:24:35Z

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

+             response.getEmail(),
+             response.getIdpToken(),
+             response.getIdpSecret(),
+             prevUid);


This is a mess, but I don't know how to get around it. I need to add the previous user id to an existing IdpResponse and I chose not to use a setter since this is public api. Any thoughts on a better way to do this? Should I be using a setter? Also, if we stick with this, what is the convention for this kind of constructor? (response.getProviderType() vs response.mProviderId)

SUPERCILEX · 2016-11-19T06:25:21Z

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

+     * See the <a href="https://github.com/SUPERCILEX/FirebaseUI-Android/blob/master/auth/README.md#handling-account-link-failures">README</a>
+     * for a much more detailed explanation.
+     *
+     * @see AuthUI.SignInIntentBuilder#setShouldLinkAccounts(boolean)


Is this clear enough?

SUPERCILEX · 2016-11-19T06:33:36Z

auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackPasswordPrompt.java

@@ -83,8 +83,6 @@ protected void onCreate(Bundle savedInstanceState) {
        TextView bodyTextView = ((TextView) findViewById(R.id.welcome_back_password_body));
        bodyTextView.setText(spannableStringBuilder);

-        ((TextInputLayout) findViewById(R.id.password_layout)).setPasswordVisibilityToggleEnabled(false);


See the support lib release notes. In 25.0.1, they set the default to false.

# Conflicts: # auth/README.md # auth/src/main/java/com/firebase/ui/auth/ui/phone/PhoneActivity.java # auth/src/main/java/com/firebase/ui/auth/ui/phone/SubmitConfirmationCodeFragment.java # auth/src/main/java/com/firebase/ui/auth/util/AuthHelper.java # auth/src/main/java/com/firebase/ui/auth/viewmodel/idp/LinkingSocialProviderResponseHandler.java # auth/src/test/java/com/firebase/ui/auth/testhelpers/AuthHelperShadow.java # auth/src/test/java/com/firebase/ui/auth/ui/phone/PhoneActivityTest.java

Signed-off-by: Alex Saveau <[email protected]>

This reverts commit a97133b Signed-off-by: Alex Saveau <[email protected]>

googlebot · 2018-08-05T03:28:25Z

CLAs look good, thanks!

Signed-off-by: Alex Saveau <[email protected]>

Borbarad13 · 2018-08-09T17:18:12Z

As I am not fully of the full release process here, when could this be expected time wise: Are we talking about days, weeks, month or years here? Don't get me wrong, I am not blaming - I am actually very thankful that such projects exist and that people contribute so much. I just need to know if if it makes sense to wait until this is released for my project or I have to move on to an alternative.

SUPERCILEX · 2018-08-10T07:07:21Z

Please ignore my previous post (now deleted), was the wrong tab. 😳😊

@Borbarad13 because this depends on the other platforms implementing this feature too (which I'm not familiar with), waiting on it is not a viable solution. However, lots of people have been using my fork in #123 (comment) to handle every possible merge case. I'll post some new commit hashes tomorrow.

…ous-auth

SUPERCILEX · 2018-08-14T18:33:48Z

@Borbarad13 Done: #123 (comment)

FanciROS · 2021-12-03T17:32:53Z

Is someone authorized to proceed, can resolve the conflicts and merge the pull request ? This feature is what I am looking for !
Thank you in advance for your work.

danijorda1 · 2021-12-31T14:19:26Z

Hi, any update here? Is this feature avialble already? Doesn't seem to be the case. Thanks

danijorda1 · 2022-01-11T12:19:54Z

@samtstern can you proceed, and resolve the conflicts and merge the pull request?
This feature will be very useful to use Auth UI for Account Linking!
Thanks!!

samtstern · 2022-01-11T18:08:02Z

@danijorda1 this code is from 2016. Even the legendary @SUPERCILEX could not possibly resolve the merge conflicts and make this PR functional again, the library has been significantly refactored over the years.

We already have some account linking functionality in FirebaseUI. If you need specific improvements to that functionality, please file a new feature request or bug report.

danijorda1 · 2022-01-11T18:50:08Z

Hi @samtstern which account linking functionality are you referring to? Couldn't find anything in the Documentation. Thanks

danijorda1 · 2022-01-11T19:10:33Z

As far as I can see only Upgrading anonymous users is supported currently while 309, 1823, 894, 1235 and 123 are all related to being able to use Firebase Auth UI to link accounts. That is, when a user is signed in, the user has the possibility to press a Link Account button where a new Sign in activity will be launched allowing the user to link accounts with the selected Provider.

As far as I can see all these feature requests/bug reports have been closed and no solution has been provided except for @SUPERCILEX solution that was never merged.... Will Account linking (other than Anonymous upgrade) be supported at some point?

Thanks in advance.

samtstern · 2022-01-11T19:16:20Z

https://github.com/firebase/FirebaseUI-Android/tree/master/auth#upgrading-anonymous-users

You're right. The other features are not yet supported. I am no longer working on this project so I can't say if they ever will be.

danijorda1 · 2022-01-11T21:36:06Z

Do you know who is working on this project so I can directly ask him/her?
Thanks

SUPERCILEX commented Sep 17, 2016

View reviewed changes

samtstern suggested changes Sep 20, 2016

View reviewed changes

SUPERCILEX commented Sep 25, 2016

View reviewed changes

SUPERCILEX added a commit to SUPERCILEX/Robot-Scouter that referenced this pull request Sep 26, 2016

Auth flow refactor. Waiting for firebase/FirebaseUI-Android#309.

a145d24

samtstern reviewed Sep 28, 2016

View reviewed changes

SUPERCILEX commented Oct 1, 2016

View reviewed changes

amandle reviewed Oct 6, 2016

View reviewed changes

samtstern assigned amandle Oct 6, 2016

samtstern approved these changes Oct 6, 2016

View reviewed changes

This was referenced Oct 8, 2016

Merge changes from master into v1.0.0 #344

Closed

Refactor SaveCredentialsActivity #254

Closed

SUPERCILEX changed the base branch from master to version-1.0.0-dev October 10, 2016 18:15

SUPERCILEX commented Oct 12, 2016

View reviewed changes

SUPERCILEX mentioned this pull request Oct 21, 2016

Support for multiple accounts-Multiple providers login need it #228

Closed

SUPERCILEX mentioned this pull request Nov 5, 2016

Automatically send verification email upon account creation #387

Closed

SUPERCILEX changed the base branch from version-1.0.0-dev to master November 5, 2016 05:41

SUPERCILEX commented Nov 19, 2016

View reviewed changes

SUPERCILEX and others added 11 commits June 18, 2018 20:44

Fix merge mistakes

3886b0b

Signed-off-by: Alex Saveau <[email protected]>

Fix broken phone auth merging

be283dd

Signed-off-by: Alex Saveau <[email protected]>

Merge branch 'version-4.1.0-dev' into anonymous-auth

95eb5d0

Version 4.2.0

9f257b6

Adds continue as guest (#1375)

2631d09

Speed up full build (#1386)

0c13d77

Translations for continue as guest (#1390)

a108176

Save phone verification ID across process deaths (#1393)

71fa34a

Revert "Upgrading anonymous accounts (#1368)"

f8affdf

This reverts commit a97133b Signed-off-by: Alex Saveau <[email protected]>

Merge branch 'tmp' into anonymous-auth

8552cc4

SUPERCILEX added 2 commits August 4, 2018 20:39

Fix merge mistakes

e0fe130

Signed-off-by: Alex Saveau <[email protected]>

Fix GitHub provider NPE when no perms are supplied (#1405)

c387047

Merge remote-tracking branch 'upstream/version-4.2.0-dev' into anonym…

f1d5374

…ous-auth

NikiFoD mentioned this pull request Sep 4, 2018

Handling anonymous user upgrade merge conflicts: Wrong error code #1435

Closed

tynn mentioned this pull request Aug 3, 2020

Provide FlowParameters for linking credentials to authenticated users #1823

Open

samtstern closed this Jan 11, 2022

samtstern mentioned this pull request Jan 11, 2022

Add support for Play Games provider #1211

Closed

		@@ -0,0 +1,11 @@
		package com.firebase.ui.auth.util;

		public interface OnMergeFailedListener {

Support account linking #309

Support account linking #309

Conversation

SUPERCILEX commented Sep 17, 2016 • edited Loading

SUPERCILEX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SUPERCILEX Sep 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SUPERCILEX Sep 17, 2016 • edited Loading

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 Sep 17, 2016

samtstern commented Sep 20, 2016

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 Sep 25, 2016

SUPERCILEX left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samtstern left a 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 left a 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 Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SUPERCILEX commented Oct 1, 2016

SUPERCILEX commented Oct 1, 2016

SUPERCILEX left a 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

samtstern commented Oct 4, 2016

amandle left a comment

SUPERCILEX commented Sep 17, 2016 •

edited

Loading

SUPERCILEX Sep 17, 2016 •

edited

Loading

SUPERCILEX Sep 17, 2016 •

edited

Loading

samtstern Oct 4, 2016 •

edited

Loading