Add reauthentication #583

amandle · 2017-02-09T02:17:22Z

Adds reauthentication as discussed in #563

There is now AuthUI.createReauthIntentBuilder() which returns an intent builder very similar to AuthUI.createSignInIntent() but adds the methods setReauthReason and setReauthEmail and removes setAllowNewEmailAccounts. Both builders extend a common builder which contains the common methods.
Account creation is skipped during the reauth flow
Account picker screen only lists the methods the user has linked.
The reauth reason gets displayed directly above the auth method buttons. This is totally open for discussion but it's where made the most sense to me.

SUPERCILEX · 2017-02-09T04:49:13Z

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

-        private boolean mAllowNewEmailAccounts = true;
-
-        private SignInIntentBuilder() {
+    @SuppressWarnings(value="unchecked")


@amandle the build should pass if you put space around the =.

SUPERCILEX

@amandle You're back! 😄 I left a few comments about style nits and some improvements I think could be made.

SUPERCILEX · 2017-02-09T04:52:56Z

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

+                                    getSelectedTheme(),
+                                    getSelectedProviders(),
+                                    getSelectedTosUrl(),
+                                    mEnableSmartLock.isEnabled())));


@amandle Maybe make the new SignedInActivity.SignedInConfig(...) part a method since it's a copy paste of the same thing above? (Actually, I think you could do a Ctrl + Alt + M on the whole startActivity thing and Intellij should only generate a response parameter which would be nicer than having a method for just the SignedInConfig part.)

good point, done

SUPERCILEX · 2017-02-09T04:57:51Z

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

+         * Set an explanation for why reauth was requested e.g. "To delete your account you must
+         * reauthenticate."
+         *
+         * @param reauthReason A string explaining why reauthentication was requested.


I think you're referring to the empty return? I have now removed it.

SUPERCILEX · 2017-02-09T05:00:21Z

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


    public final boolean allowNewEmailAccounts;

+    public final boolean isReauth;


@amandle Couldn't this be a method instead? It seems weird to have a parameter just for something that can be inferred programmatically.

Something like:

public boolean isReauth() { return !TextUtils.isEmpty(reauthEmail); }

Nevermind, I thought the reauthenticate method required an email but it doesn't so this won't work.

SUPERCILEX · 2017-02-09T05:03:28Z

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

+        if (visibleProviders.size() == 0) {
+            Log.e(TAG, "There are no configured providers associated with the user's account");
+        }
+        populateIdpList(visibleProviders);


Before this happens I think we should check to see if visibleProviders.size() == 1 and just launch the sign in right away if that is the case.

Agreed and I think we already have this logic somewhere to steal.

@samtstern Yup, it's right here: https://github.com/firebase/FirebaseUI-Android/blob/master/auth/src/main/java/com/firebase/ui/auth/util/signincontainer/SignInDelegate.java#L233.

SUPERCILEX · 2017-02-09T05:05:25Z

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

+            if (mActivityHelper.getFlowParams().isReauth) {
+                String reauthEmail = mActivityHelper.getFlowParams().reauthEmail;
+                if (reauthEmail == null) {
+                    Log.e(TAG, "Attempting to reauth with email but no reauthEmail provided");


I think the build() method should prevent this and throw an IllegalArgumentException if the dev tries to do this.

As per Sam's comment reauthEmail is now removed, and the user is required to be logged in to start, so this whole block can go away

SUPERCILEX · 2017-02-09T05:07:14Z

auth/src/main/res/layout/auth_method_picker_layout.xml

+      android:layout_width="wrap_content"
+      android:orientation="vertical">
+
+        <TextView


It looks like this is just copy past from the landscape layout so we could use a <merge /> tag for this.

SUPERCILEX · 2017-02-09T05:07:58Z

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

+                true  /* allowNewEmailAccounts */,
+                false /* isReauth */,
+                null  /* reauthEmail */,
+                null  /* reauthReason */);


SUPERCILEX · 2017-02-09T05:18:11Z

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

+                        RC_EMAIL_DIRECT_SIGN_IN);
+            } else {
+                startActivityForResult(
+                        RegisterEmailActivity.createIntent(this, mActivityHelper.getFlowParams()),


@amandle I'm just being nit picky here (Sorry! 😄), but I feel like mActivityHelper.getFlowParams() could be Ctrl + Alt + Ved into something like params.

samtstern · 2017-02-09T16:48:34Z

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

+                "theme identifier is unknown or not a style definition");
            mTheme = theme;
-            return this;
+            return (T) this;


Sigh. I looked at this and thought "there's no way that Java really requires this generic + cast situation" but it looks like we do!

samtstern · 2017-02-09T16:51:59Z

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

+         *
+         * @param reauthEmail The email address of the account for which reauthentication is being
+         * requested
+         */


Is it possible to do re-auth and not use the currently signed in user? In which case this email would be FirebaseAuth.getInstance().getCurrentUser().getEmail() I feel like we could remove this setter and therefore reduce the chance of error/misunderstanding.

Nice, that simplifies things considerably.

samtstern · 2017-02-09T16:53:53Z

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

+        }
+        List<IdpConfig> visibleProviders = new ArrayList<>();
+        if (mActivityHelper.getFlowParams().isReauth) {
+            // display the reauthReason if available


We should ask UX about this, but instinctively a dialog feels like the "normal" reauth flow.

User clicks a button requiring reauth ("Delete Account" for example)

A dialog shows up that says "This action requires you to sign in again, would you like to continue"

Yes launches the flow, No returns RESULT_CANCELED

samtstern · 2017-02-09T16:54:51Z

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

+            // their account.
+            List<String> providerIds = mActivityHelper.getCurrentUser().getProviders();
+            if (providerIds.size() == 0) {
+                // zero providers indicates that it is an email account


Doesn't the providers array contain just "password" in this case or is that only when you call fetchProvidersForEmail()?

SUPERCILEX · 2017-02-09T19:20:10Z

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

+                    new ArrayList<>(mProviders),
+                    mTheme,
+                    mLogo,
+                    mTosUrl,


@amandle Have you tried reauthenticating with smart lock enabled? Given @samtstern's comment about showing a dialog before we show auth stuff, I feel like things will get complicated with smart lock enabled. @samtstern @amandle Am I missing something or could this be a problem?

Yeah, it's a little complicated/confusing, but I think we have to have it as an option for the case where the person doesn't know their password but has it stored in smartlock

samtstern · 2017-02-17T18:12:14Z

@amandle FYI there are merge conflicts between your branch and the base.

I left one more comment, but other than that (and the conflicts) this LGTM

samtstern · 2017-02-17T18:18:04Z

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

+            // their account.
+            List<String> providerIds = mHelper.getCurrentUser().getProviders();
+            if (providerIds.size() == 0) {
+                // zero providers indicates that it is an email account


For an email account when you do fetchProvidersForEmail you get a list that's just ["password"]. Is this different when you access the providers from the user object directly?

It does seem to be different, asking some of the Firebase folks for more information

samtstern · 2017-02-17T18:35:15Z

I checked out the branch and tried a few things. I am noticing some strange behavior when using Google Sign In:

If I have SmartLock enabled I get the "choose an account saved with Google Smart Lock" dialog, but if I click "None of the above" then the whole reauth flow ends. This does not happen with password login.
If I don't have SmartLock enabled I still get the dialog above when I try to reauth. It does not seem that I can actually disable SmartLock (maybe this is just a sample issue?)

Also the re authentication dialog does not seem to respect my theme colors, the buttons are always straight black. Ideally this dialog would inherit the theme passed to the sign in flow.

SUPERCILEX · 2017-02-17T20:45:26Z

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


            boolean smartLockEnabled = smartLockEnabledInt != 0;
            boolean allowNewEmailAccounts = allowNewEmailAccountsInt != 0;
+            boolean isReauth = isReauthInt != 0;


@amandle The merge conflicts are my fault, sorry about that! I removed this unnecessary abstraction so this should work once you merge:

boolean smartLockEnabled = in.readInt() != 0; boolean allowNewEmailAccounts = in.readInt() != 0; boolean isReauth = in.readInt() != 0; String reauthReason = in.readString();

amandle · 2017-02-17T22:46:20Z

If I have SmartLock enabled I get the "choose an account saved with Google Smart Lock" dialog, but if I click "None of the above" then the whole reauth flow ends. This does not happen with password login.

The reauth does happen though. Since the user is already signed in to Google and we know the email address it happens automatically. You can verify this by putting a breakpoint at IdpSignInContainer.java:134

If I don't have SmartLock enabled I still get the dialog above when I try to reauth. It does not seem that I can actually disable SmartLock (maybe this is just a sample issue?)

Ah, yep this is a bug in the sample app. Should be fixed now.

Also the re authentication dialog does not seem to respect my theme colors, the buttons are always straight black. Ideally this dialog would inherit the theme passed to the sign in flow.

Hmm, I'm not sure how to do this without going back to using standard activities instead of AlertDialogs. The RecoveryEmailSentDialog uses the same theme:

FirebaseUI-Android/auth/src/main/java/com/firebase/ui/auth/ui/email/RecoveryEmailSentDialog.java

Line 32 in e14170d

return new AlertDialog.Builder(getContext(), R.style.FirebaseUI_Dialog)

samtstern · 2017-02-21T17:21:27Z

@amandle I moved this to target the 2.0.0 release since this is a notable enough feature that we should pursue iOS parity. Hopefully @yramanchuk can implement a mirror of this.

The only remaining concern I still have is the bad UX with SmartLock and "None of the Above" where GSI succeeds even though the user never really did anything. Can we clarify this somehow? Options include:

For reauth change SmartLock settings to only include password accounts
Display a Toast after reauth success
Other (?)

amandle · 2017-03-10T01:57:08Z

@samtstern I think this is ready to review now. I added a toast when the automatic Google Sign In happens and skip Smart Lock if it's a reauth with a non-password account.

samtstern · 2017-03-13T16:52:47Z

@amandle approved! Going to squash and merge.

@yramanchuk @morganchen12 @bojeil-google FYI we need to coordinate the same abilities across platforms. This is merging into our next release branch.

develuxe · 2018-06-01T14:31:53Z

Would love to have this feature back but can't find it in the repo. Was it removed?

samtstern · 2018-06-01T14:40:03Z

This feature was never released actually since amandle left the project and we were focused on stability work.

…

On Fri, Jun 1, 2018, 7:31 AM develuxe ***@***.***> wrote: Would love to have this feature back but can't find it in the repo. Was it removed? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#583 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIEw6kxlVeTFBfpDlXAjtH9uZYs0LF1Eks5t4VBegaJpZM4L7nov> .

develuxe · 2018-06-01T14:42:18Z

Thanks for your quick response. Is there any best practice on how to reauthenticate the user?

ViniciusR · 2018-08-25T21:35:06Z

So sad they abandoned this feature. I'm actually facing this situation, it's so much complex to retrieve credentials from Google and other providers, Firebase's reauthenticate function is pretty easy, but it requires that credentials.

I think the easiest way is forcing sign out and ask user to sign in again - it would needed to login back to reauthenticate anyway.

bojeil-google · 2018-08-26T00:43:15Z

Hey folks, we have not abandoned our plans to support re-authentication in FirebaseUI (it is indeed a missing key functionality that we should support). The priority has been lowered in favor of some other popular/critical feature requests such as support for email link sign-in and account management UI.

ViniciusR · 2018-08-26T03:24:17Z

That's good!

jackz314 · 2018-12-22T04:20:00Z

Any updates on this feature? And would there be an ETA for this?

Heldenkrieger01 · 2020-04-14T21:46:20Z

Will this be released any time soon?

SUPERCILEX reviewed Feb 9, 2017

View reviewed changes

samtstern reviewed Feb 9, 2017

View reviewed changes

SUPERCILEX reviewed Feb 9, 2017

View reviewed changes

samtstern reviewed Feb 17, 2017

View reviewed changes

SUPERCILEX reviewed Feb 17, 2017

View reviewed changes

samtstern modified the milestones: 1.2.0, 2.0.0 Feb 21, 2017

samtstern mentioned this pull request Feb 21, 2017

Add a 'reauthenticate' function firebase/FirebaseUI-iOS#247

Closed

samtstern changed the base branch from version-1.2.0-dev to version-2.0.0-dev February 21, 2017 21:10

samtstern mentioned this pull request Feb 22, 2017

[Feature Request] Add a re-authenticate function firebase/firebaseui-web#105

Open

Aaron Mandle added 6 commits March 3, 2017 15:08

reauthentication wip

6ca1054

clean up some lint errors

aed0c47

Review feedback part 1

316a62f

Review feedback part 2

6f68e7a

Use the native cancel string

463fdf3

fix smartlock disabling in reauth

10d65c2

amandle force-pushed the reauthenticate branch from 4815e33 to 10d65c2 Compare March 3, 2017 23:21

Aaron Mandle added 2 commits March 9, 2017 16:54

show toast when logging in with a specific Google account

dab9bbd

skip smart lock for non-password reauths

0ea20ee

samtstern merged commit 9ae131a into firebase:version-2.0.0-dev Mar 13, 2017

samtstern mentioned this pull request Mar 13, 2017

Document reauthentication #627

Closed

SUPERCILEX mentioned this pull request Mar 13, 2017

Redo changes from #591 #630

Merged


		public final boolean allowNewEmailAccounts;

		public final boolean isReauth;

Add reauthentication #583

Add reauthentication #583

Uh oh!

Conversation

amandle commented Feb 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtstern commented Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtstern commented Feb 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amandle commented Feb 17, 2017

Uh oh!

samtstern commented Feb 21, 2017

Uh oh!

amandle commented Mar 10, 2017

Uh oh!

samtstern commented Mar 13, 2017

Uh oh!

samtstern commented Feb 17, 2017 •

edited

Loading