Default phone by yramanchuk · Pull Request #906 · firebase/FirebaseUI-Android

yramanchuk · 2017-09-12T20:31:26Z

Implement phone Auth flow Phone number configuration parameter.
Related issue #887

SUPERCILEX · 2017-09-12T20:36:49Z

@yramanchuk I believe the result of that issue was that FirebaseUI isn't going to support this kind of flow. Could you explain why you already have a phone number? The user should be giving that to FirebaseUI in the auth flow, not to you directly. Hope this clears things up! 😄

ashwinraghav · 2017-09-12T22:20:45Z

@yramanchuk I think the issue that is linked to may not be completely comparable.

@SUPERCILEX The problem we are addressing is rooted in the fact that Firebase UI currently does not support 2FA (Say Facebook + Phone Auth). To work around this, apps that wish to use phone as the second factor can use Facebook login on their own outside Firebase UI and then use Firebase UI for the second factor phone auth.
This change helps apps that "know" the second factor phone number given a Facebook account, particularly when a user is logging into their secondary device like a tablet. Hope that helps.

yramanchuk · 2017-09-12T22:26:00Z

@ashwinraghav Thanks for the clarification.
@SUPERCILEX here's another example: application may have multi-dialog account creation flow and they have already requested user to enter his phone number, so they would like to show phone verification dialog with pre-filled phone number.

Here's related FirebaseUI-iOS issue: firebase/FirebaseUI-iOS#306
This request is required for the feature parity: firebase/FirebaseUI-iOS#334

ashwinraghav · 2017-09-12T22:48:31Z

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

                    break;
                case AuthUI.PHONE_VERIFICATION_PROVIDER:
-                    mProviders.add(new PhoneProvider(this, getFlowParams()));
+                    PhoneIdpConfig phoneIdpConfig = (PhoneIdpConfig) idpConfig;


Would this break older clients, breaking backward compatibility?

ashwinraghav · 2017-09-12T22:50:41Z

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

        }
    }

+    public static class PhoneIdpConfig extends IdpConfig {


I feel like introducing a config per for phone auth provider leaves me desiring symmetry for such a config in other providers.

Also, the type safety that this implementation gives is hard to enforce in a backward compatible way. This leads to unsafe typecasting like the one made in

FirebaseUI-Android/auth/src/main/java/com/firebase/ui/auth/ui/idp/AuthMethodPickerActivity.java

Line 118 in d1da327

PhoneIdpConfig phoneIdpConfig = (PhoneIdpConfig) idpConfig;

My suggestion would be to introduce a Bundle parameter into the existing IdpConfig that yields an API that would look like:

Bundle b = new Bundle();
b.putString(AuthUI.PHONE_VERIFICATION_PROVIDER.DEFAULT_PHONE_EXTRA, "+16478488383");

new AuthUI.IdpConfig.Builder(AuthUI.PHONE_PROVIDER).setParams(bundle).build()) and wire it into the providers from that point on.

@ashwinraghav Thhank's for the suggestion. I was thinking about extending existing IdpConfig but in that case were are adding changes to all configs (Google, Facebook) and they they don't need that parameter.

Anyway I think your solution is more elegant. I just want to propose to use List of Bundles instead of just Bundle:

AuthUI.IdpConfig.Builder(AuthUI.PHONE_PROVIDER).setParams(List).build())

In that case we can pass multiple params.
@ashwinraghav what do you think?

samtstern · 2017-09-12T23:52:37Z

@yramanchuk FYI I changed the target branch to the current development branch, version-2.3.1-dev.

samtstern · 2017-09-12T23:54:17Z

Note that the issue of IDP-specific configuration has been discussed here as well:
#806

samtstern

Mostly good, a few remaining comments,

samtstern · 2017-09-13T18:40:30Z

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

+            // Use the params Bundle to provide a default phone number that will populate the
+            // phone number field in PhoneVerificationActivity if the number is known in advance.
+            Bundle params = new Bundle();
+            params.putString(AuthUI.PHONE_VERIFICATION_DEFAULT_PHONE_EXTRA, "+13099126574");


This is a minor point, but I think putting a random phone number into the sample will cause confusion. Many people will want to use this sample to test phone auth with their real number.

This type of example snippet belongs in the README.

Moved to README, let me know if you don't think it's in an appropriate location.

samtstern · 2017-09-13T18:43:58Z

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

+                            .getString(AuthUI.PHONE_VERIFICATION_DEFAULT_PHONE_EXTRA);
                    startActivityForResult(
-                            PhoneVerificationActivity.createIntent(getContext(), flowParams, null),
+                            PhoneVerificationActivity.createIntent(getContext(), flowParams, phone),


This createIntent call uses ExtraConstants.EXTRA_PHONE. We should unify the values between this and AuthUI.PHONE_VERIFICATION_DEFAULT_PHONE_EXTRA.

I'd suggest:

class AuthUI { // ... public static final String EXTRA_DEFAULT_PHONE = ExtraConstants.EXTRA_PHONE; // ... }

That's reasonable, made your suggested change.

samtstern · 2017-09-13T18:45:30Z

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

                    break;
                case AuthUI.PHONE_VERIFICATION_PROVIDER:
-                    mProviders.add(new PhoneProvider(this, getFlowParams()));
+                    PhoneIdpConfig phoneIdpConfig = (PhoneIdpConfig) idpConfig;


This class PhoneIdpConfig is used here but I don't see it defined in this PR anymore. I think changes to this file need to be revisited.

Oops, gone now.

samtstern · 2017-09-13T18:46:49Z

@SUPERCILEX the main reason for supporting this flow is feature parity with what Digits used to offer. We are telling all Digits developers to move to FirebaseUI and some of them noticed this missing feature.

You are right though, it was not a flow we originally intended to support. Ashwin and Yury have provided some examples where it can be very useful though.

… README

SUPERCILEX

@yramanchuk @ashwinraghav @samtstern Thanks for all your explanations, I see why this is necessary now!

@yramanchuk I just have a few comments.

SUPERCILEX · 2017-09-13T21:21:54Z

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

            selectedProviders.add(
-                    new IdpConfig.Builder(AuthUI.PHONE_VERIFICATION_PROVIDER).build());
+                    new IdpConfig.Builder(AuthUI.PHONE_VERIFICATION_PROVIDER)
+                            .build());


Might as well undo the changes in this file since they're leftovers from previous commits.

SUPERCILEX · 2017-09-13T21:23:10Z

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

+                paramsValueBuilder.append(mParams.get(key).toString());
+                paramsValueBuilder.append(",");
+            }
+            paramsValueBuilder.append("}");


Doesn't Bundle already have a toString() with very similar behavior?

Yeah, just check the source and Bundle#toString() calls mMap.toString(). I'm assuming the formatting will be a little bit different, but the output should be effectively the same.

Ah, did not realize that. Switched to the Bundle toString.

SUPERCILEX · 2017-09-13T21:24:35Z

auth/src/main/java/com/firebase/ui/auth/provider/PhoneProvider.java

+        String phone = null;
+        for (AuthUI.IdpConfig idpConfig : mFlowParameters.providerInfo) {
+            if (idpConfig.getProviderId().equals(AuthUI.PHONE_VERIFICATION_PROVIDER)) {
+                phone = idpConfig.getParams().getString(EXTRA_DEFAULT_PHONE);


To be consistent, we shouldn't statically import this and instead use AuthUI.EXTRA_DEFAULT_PHONE.

SUPERCILEX · 2017-09-13T21:26:02Z

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

                case PhoneAuthProvider.PROVIDER_ID:
                    // Go directly to phone flow
+                    String phone = idpConfigs
+                            .get(0)


This same logic is used above so we should probably extract a firstIdpConfig and replace firstProvider with the method call.

Good point, done.

SUPERCILEX · 2017-09-13T21:37:45Z

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

        private IdpConfig(Parcel in) {
            mProviderId = in.readString();
            mScopes = Collections.unmodifiableList(in.createStringArrayList());
+            mParams = in.readBundle();


This should be in.readBundle(getClass().getClassLoader()) to make the build pass. Since we're just passing in strings right now we don't actually need this, but it'll ensure we're future proof (and make lint happy).

Ah, just noticed the CONTRIBUTING.md file. Apologies - just beginning contribution to the library and obviously wasn't paying full attention. Made the change and build should be passing now. Thanks for the review by the way!

SUPERCILEX · 2017-09-13T22:25:01Z

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

            return "IdpConfig{" +
                    "mProviderId='" + mProviderId + '\'' +
                    ", mScopes=" + mScopes +
+                    ", mParams=" + mParams.toString() +


One last nit! 😄 We don't need the toString() call, mParams alone is just fine.

SUPERCILEX

@yramanchuk Sweet, LGTM! 😄

samtstern

LGTM

samtstern · 2017-09-13T23:34:49Z

I will merge this once Travis is happy.

yramanchuk added 2 commits September 12, 2017 13:24

Add default phone number to the Phone Auth number flow.

d82aa47

Update comments.

d1da327

yramanchuk requested a review from samtstern September 12, 2017 20:31

ashwinraghav reviewed Sep 12, 2017

View reviewed changes

samtstern changed the base branch from master to version-2.3.1-dev September 12, 2017 23:52

Using Bundle for phone instead of new IdpConfig

5f2bf6e

samtstern reviewed Sep 13, 2017

View reviewed changes

Resolved PR comments - renamed key, removed old code, moved sample to…

528fd74

… README

SUPERCILEX reviewed Sep 13, 2017

View reviewed changes

Resolving more comments - style consistency and lint conformance

76a6161

SUPERCILEX reviewed Sep 13, 2017

View reviewed changes

Removed toString call

8efcc56

SUPERCILEX approved these changes Sep 13, 2017

View reviewed changes

samtstern approved these changes Sep 13, 2017

View reviewed changes

samtstern merged commit e4ece3a into version-2.3.1-dev Sep 14, 2017

samtstern added this to the 2.3.1 milestone Sep 14, 2017

samtstern deleted the default_phone branch September 29, 2017 00:00

Conversation

yramanchuk commented Sep 12, 2017

Uh oh!

SUPERCILEX commented Sep 12, 2017

Uh oh!

ashwinraghav commented Sep 12, 2017

Uh oh!

yramanchuk commented Sep 12, 2017

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 Sep 12, 2017

Uh oh!

samtstern commented Sep 12, 2017

Uh oh!

samtstern 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!

samtstern commented Sep 13, 2017

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!

SUPERCILEX left a comment

Choose a reason for hiding this comment

Uh oh!

samtstern left a comment

Choose a reason for hiding this comment

Uh oh!

samtstern commented Sep 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!