Merge phone auth into version-2.0.0 #740

samtstern · 2017-06-07T22:19:51Z

As mentioned we've been working on phone auth internally. Now that the Play services 11.0.0 SDK is out we can move into the open.

This merges all of the phone auth changes into version-2.0.0-dev

Change-Id: Ief6f138e79117a43aa789cc3e8ad5c9872a4e476

Change-Id: Ia3b861157127a1dee7e4625d9e786a6df45dea30

Change-Id: I989ee0354088a185d1373981532b52c9c0b0165e

1. Phone button background 2. Text changes Change-Id: I6d1227a902f375367e6323e1ce4bdb472c12cd29

Change-Id: I18fc00185d7505408f448079563564618339a32c

Change-Id: I901c8d8132d93582f30ee1589080bb3f882fa452

Change-Id: Id2cbfdfc49e82da03f319c209799648bb9c3bfc5

Change-Id: I1d759c469567545158b5d51de847da1d6cff297f

Fix unhandled error cases Change-Id: I39a5fb26297ebe1afd57a0687f709aee2dd529eb

Change-Id: I126347c68c0ce19604f0926f30fccb8c89532f5a

Change-Id: I6ba7ea8b8268d10cfea5db421db7a740bc20a8f7

Change-Id: I6b57fb46472367360cda4d1c4e88538f0495fa49

Change-Id: Icd1e115e51c37c1b599166422be6072fc0fd4f4c

Revert changes introduced in cred picker to support phone auth in commit a626685 Change-Id: I265be2d294218579d177c1d39b44fd6181f88767

Used the asset downloaded from: https://icons.googleplex.com/#icon=ic_phone&search=phone Used Android Studio to create the vector drawable, allowed RTL auto-mirroring and manually changed color from black to white. Bug: 38454930 Change-Id: I7e55d7e95284c0e20ff3aa99d6727bddc3e795b6

Change-Id: Id5beeacdb014632b7c9438dc7d42066fc05d28f0

Change-Id: I1b07ed83741832730b4277dd65934cdf11d658db

Change-Id: I20b9ee9c24180f8de19f030371541922d0f10104

Change-Id: I9928978e188599c852f675453f59ce624bc9602d

Change-Id: I6d2bedb15d96f39e3d3e71682e33388fa41f3439

We upgrade to twitter kit 3.0 and achieve the following 1. No more fabric deps. This shaves our sdk down even more and clears related bugs. 2. We use only twitter core and not other deps. Shaves even more. Total savings: 0.6 MB Change-Id: I64ae5b6c7e2f238c35e25d6b2a2b7089431a3a0a

Change-Id: I57574ae9e376e3c576218b4e18e16ad914a6c496

Problem: Apps are currently required to add the facebook and twitter dependency even if they do not require these login methods to be supported. Solution: In this change, we assume a provided scope dependency on facebook and twitter sdks and expect that the apps provide these dependencies. If these dependencies are not found, we fail early with an actionable error. Also, the size of the resulting apk reduces by about 2MB. Change-Id: I5979039c281dd6daca265f60f9c9dcf59596ced1

Bug: 62309027 Change-Id: Iccbdb2a34e8c0b14c6fcf186005e0f1a68ea180e

Change-Id: Ib18e49c565e39c30bba83333aa8188fc853e1775

googlebot · 2017-06-07T22:20:00Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

ashwinraghav · 2017-06-07T22:29:06Z

LGTM

samtstern · 2017-06-07T22:30:46Z

Here is an enumeration of the changes at a high level (in no particular order):

Phone auth (obviously)
Smart Lock enable flag split into two pieces: one for the hint selector and one for actual credentials
Facebook and Twitter made provided dependencies. This means apps not using these providers don't need the dep.
Adding translation_description attributes to all library strings (preparing for internationalization)

Change-Id: I5ff1752f718b91602020147a91fe56691b09318f

SUPERCILEX · 2017-06-07T23:27:52Z

@samtstern Ouch! That's a lot of changes! 😕 Glad to finally see phone auth come out though!!! 😄

I have a few questions:

The facebook and twitter things aren't particularly related to phone auth, right?
Also, I disagree with making people add them to their own deps so I think we should definitely try to figure out testing compatibility to upgrade to the 3.0 plugin and use implementation. That should be what we use for basically all our deps except the Firebase ones. Dunno if you've looked into it yet, but implementation basically means "my module needs this dep so let people compile it even if they aren't including the dep, but don't let them use it e.g. they have to include the dep if they want to use it". That seems like a much better solution than breaking everyone's dependencies when they try to upgrade.
I haven't looked at the PR, but does it also fix Consider allowing email hints even when smartlock disabled #558?

SUPERCILEX · 2017-06-07T23:31:45Z

PS: The failing test is because you guys aren't using Integer.parseInt() which does unnecessary boxing:

<a href="#DM_BOXED_PRIMITIVE_FOR_PARSING">Bug type DM_BOXED_PRIMITIVE_FOR_PARSING (click for details)</a>
<br/>In class com.firebase.ui.auth.ui.phone.CountryListSpinner<br/>In method com.firebase.ui.auth.ui.phone.CountryListSpinner.setSelectedForCountry(Locale, String)<br/>Called method Integer.intValue()<br/>Should call Integer.parseInt(String) instead<br/>At CountryListSpinner.java:[line 83]</p>

samtstern · 2017-06-07T23:33:46Z

@SUPERCILEX there's a few huge benefits of doing twitter/facebook this way:

Shaves 2MB+ off of your app if you were not using those providers before by avoiding the depdendency
No more adding that pesky twitter maven repo

I agree we should use implementation down the road, but I think in the meantime this is a big improvement.

SUPERCILEX · 2017-06-07T23:35:23Z

@samtstern Ohhhh, wait so you don't have to include them if you aren't using Facebook/Twitter? Won't there be a ClassDefNotFound exception somewhere down the line?

Change-Id: I0b884bdbe5e62713dc3a4edca8539cace74f3f18

samtstern · 2017-06-07T23:38:45Z

@SUPERCILEX we're using a trick to check for Facebook/Twitter SDK presence if you enable those providers (and at other entry points to the flow like signOut) and then throwing a RuntimeException before you can proceed saying "add the dependency"

samtstern · 2017-06-07T23:40:09Z

Two remaining test failures:

AuthMethodPickerActivityTest. testFacebookLoginFlow

java.lang.NoClassDefFoundError: com/facebook/R$style
	at com.facebook.FacebookSdk.<clinit>(FacebookSdk.java:87)
	at com.facebook.internal.Validate.sdkInitialized(Validate.java:144)
	at com.facebook.login.LoginManager.__constructor__(LoginManager.java:72)
	at com.facebook.login.LoginManager.<init>(LoginManager.java)
	at com.facebook.login.LoginManager.getInstance(LoginManager.java:83)
	at com.firebase.ui.auth.provider.FacebookProvider.startLogin(FacebookProvider.java:97)
	at com.firebase.ui.auth.ui.idp.AuthMethodPickerActivity$1.onClick(AuthMethodPickerActivity.java:127)
	at android.view.View.performClick(View.java:5639)

AuthMethodPickerActivityTest. testGoogleLoginFlow

junit.framework.ComparisonFailure: expected:<[email protected]> but was:<null>
	at junit.framework.Assert.assertEquals(Assert.java:100)
	at junit.framework.Assert.assertEquals(Assert.java:107)
	at com.firebase.ui.auth.testhelpers.TestHelper.verifySmartLockSave(TestHelper.java:82)

SUPERCILEX · 2017-06-07T23:42:32Z

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

-        LoginManager.getInstance().logOut();
+        try {
+            LoginManager.getInstance().logOut();
+        } catch (NoClassDefFoundError e) {


@SUPERCILEX we're using a trick to check for Facebook/Twitter SDK presence if you enable those providers (and at other entry points to the flow like signOut) and then throwing a RuntimeException before you can proceed saying "add the dependency"

Gotcha, just saw this. Now that I think about it, I'm definitely starting to really like the idea. For Instant apps, you could limit the providers to just Google and reap massive benefits. Snazzy! 😄

samtstern · 2017-06-07T23:49:00Z

cc: @ashwinraghav two remaining test failures in the phone auth branch. I pushed commits to fix all the lint/checkstyle/findbugs errors.

Change-Id: If1686bde8906f2e06d87c51fb8399be0b960028d

samtstern · 2017-06-08T00:28:10Z

@SUPERCILEX got all the tests to pass now but findbugs is erroring out. It only happens when I do a clean in the same command (./gradlew findbugs works ./gradlew clean assembleDebug check fails)

SUPERCILEX

Some general comments, questions, and code quality improvements.

SUPERCILEX · 2017-06-07T23:44:44Z

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

+
+                if (config.getProviderId().equals(FACEBOOK_PROVIDER)) {
+                    try {
+                        Class c = com.facebook.FacebookCallback.class;


Why FacebookCallback? Wouldn't FacebookSdk make more sense since it's something Facebook will probably never get rid of?

Yep, good call.

SUPERCILEX · 2017-06-07T23:45:21Z

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

+
+                if (config.getProviderId().equals(TWITTER_PROVIDER)) {
+                    try {
+                        Class c = com.twitter.sdk.android.core.TwitterCore.class;


Same, Twitter makes more sense than TwitterCore.

We're actually only depending on Twitter core now (which means we don't pull in Fabric). This is new with the upgrade to Twitter SDK 3.0

Oh, awesome! Gotta love that smaller APK size! 😄

SUPERCILEX · 2017-06-07T23:46:11Z

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

@@ -34,22 +34,25 @@
 public class IdpResponse implements Parcelable {
    private final String mProviderId;
    private final String mEmail;
+    private final String mPhoneNumber;


Lol, this isn't being used anywhere. 😆 There's no getter anywhere so we should just get rid of it.

We are setting it in PhoneVerificationActivity so I'll add a getter.

Oh, K so we do want the dev to get the phone number from IdpResponse. Cool beans! 😄

SUPERCILEX · 2017-06-07T23:46:51Z

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

+    }
+
+    @Override
+    public String getProviderId() {


Can we add @AuthUI.SupportedProvider?

Done, added on email too.

SUPERCILEX · 2017-06-07T23:47:04Z

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

+    }
+
+    @Override
+    public int getButtonLayout() {


Can we add @LayoutRes?

SUPERCILEX · 2017-06-08T00:25:55Z

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

@@ -93,7 +93,6 @@ public void onConnected(Bundle bundle) {
            finish();
            return;
        }
-


? nit: this space reduces cram so we should keep it.

SUPERCILEX · 2017-06-08T00:28:11Z

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

@@ -21,6 +21,7 @@
 import com.google.firebase.auth.FacebookAuthProvider;
 import com.google.firebase.auth.FirebaseUser;
 import com.google.firebase.auth.GoogleAuthProvider;
+import com.google.firebase.auth.PhoneAuthProvider;


This doesn't look necessary since there are no other changes in this class. Should there be some though? Wouldn't we want to update and use providerIdToAccountType somewhere?

SUPERCILEX · 2017-06-08T00:30:55Z

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

@@ -16,6 +16,7 @@

 import com.firebase.ui.auth.ui.ActivityHelper;
 import com.firebase.ui.auth.util.signincontainer.SaveSmartLock;
+import com.google.firebase.auth.PhoneAuthProvider;


Probs a refactor mistake, we don't need it.

SUPERCILEX · 2017-06-08T00:32:33Z

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

@@ -18,6 +18,7 @@

 public class TestConstants {
    public static final String EMAIL = "[email protected]";
+    public static final String PHONE = "+14347565353";


Lol, is this a real phone number? We should probably go with +15555555555 or something similar.

SUPERCILEX · 2017-06-08T00:35:25Z

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

@@ -81,4 +83,26 @@ public static void verifySmartLockSave(String providerId, String email, String p
        assertEquals(password, passwordCaptor.getValue());
        assertEquals(providerId, idpResponseCaptor.getValue().getProviderType());
    }
+
+    public static void verifySmartLockSave(String providerId, String email, String phoneNumber,


Could we fold this into the method above? I know this is a test, but still the method below is almost unreadable. We could just pass in null for the phoneNumber or have a simple overload that does that for us.

Good idea, will make this DRY

SUPERCILEX · 2017-06-08T00:48:30Z

@SUPERCILEX got all the tests to pass now but findbugs is erroring out. It only happens when I do a clean in the same command (./gradlew findbugs works but ./gradlew clean assembleDebug check fails)

@samtstern Looks like an issue with your machine 😄 (The Travis tests are passing)

Change-Id: I6eac4c2e92c13b1e5ad9a2953441566ad2d7c29e

samtstern · 2017-06-08T16:39:00Z

@SUPERCILEX thank you for the thorough review! I believe I have addressed all feedback with the following TODO items (which can be handled later):

Use RecyclerView for the country list
Consider removing TermsTextView and just making a static method in PreambleHandler

SUPERCILEX · 2017-06-08T19:14:53Z

@samtstern Awesome, LGTM!

ashwinraghav and others added 30 commits May 3, 2017 09:52

Phone Auth UI

a9b4c5f

Change-Id: Ief6f138e79117a43aa789cc3e8ad5c9872a4e476

Merge branch 'version-2.0.0-dev' into phone-auth

8fdf168

Change-Id: Ia3b861157127a1dee7e4625d9e786a6df45dea30

Fixing (potential) NPE for phone accounts saved in smartlock

2969232

Change-Id: I989ee0354088a185d1373981532b52c9c0b0165e

Bug fixes

3598ec6

1. Phone button background 2. Text changes Change-Id: I6d1227a902f375367e6323e1ce4bdb472c12cd29

Description for auth strings to enable transations

e14973c

Change-Id: I18fc00185d7505408f448079563564618339a32c

Merge branch 'version-2.0.0-dev' into phone-auth

a6ec46f

Merge branch 'version-2.0.0-dev' into phone-auth

f9fd1db

Update error strings to match spreadsheet.

58c5f02

Change-Id: I901c8d8132d93582f30ee1589080bb3f882fa452

Crash fix for app compat attribute lookup

4b0cdaa

Change-Id: Id2cbfdfc49e82da03f319c209799648bb9c3bfc5

Merge "Crash fix for app compat attribute lookup" into phone-auth

668da24

Remove period from the end of sentences

909aac5

Change-Id: I1d759c469567545158b5d51de847da1d6cff297f

Show keyboard in conf code screen

2d6db67

Fix unhandled error cases Change-Id: I39a5fb26297ebe1afd57a0687f709aee2dd529eb

Set country selection to default locale

09bf1d2

Change-Id: I126347c68c0ce19604f0926f30fccb8c89532f5a

Merge branch 'version-2.0.0-dev' into phone-auth

c882020

Change-Id: I6ba7ea8b8268d10cfea5db421db7a740bc20a8f7

Wiring in activity scoped phone number auth api

505415c

Change-Id: I6b57fb46472367360cda4d1c4e88538f0495fa49

Converting phone number from hint selector to e164 optimistically

bc672c4

Change-Id: Icd1e115e51c37c1b599166422be6072fc0fd4f4c

Introducing additional parameters for smart lock

6ad03e7

Revert changes introduced in cred picker to support phone auth in commit a626685 Change-Id: I265be2d294218579d177c1d39b44fd6181f88767

Merge "Fix phone number button vector drawable." into phone-auth

2531fd2

Phone auth terms of service

97f0e06

Change-Id: Id5beeacdb014632b7c9438dc7d42066fc05d28f0

Merge "Phone auth terms of service" into phone-auth

f47f706

Increasing auto retrieval timeout to 2 mins

6f20458

Change-Id: I1b07ed83741832730b4277dd65934cdf11d658db

Removing unnecessary translations

47ef4a0

Change-Id: I20b9ee9c24180f8de19f030371541922d0f10104

Merge "Remove period from the end of sentences" into phone-auth

01f876d

Translations refinement

cad63e9

Change-Id: I9928978e188599c852f675453f59ce624bc9602d

Merge branch 'version-2.0.0-dev' into phone-auth

46602c5

Change-Id: I6d2bedb15d96f39e3d3e71682e33388fa41f3439

Merge branch 'version-2.0.0-dev' into phone-auth

696aa44

Change-Id: I57574ae9e376e3c576218b4e18e16ad914a6c496

Merge "Merge branch 'version-2.0.0-dev' into phone-auth" into phone-auth

d340365

samtstern added 2 commits June 5, 2017 08:25

Fix case when only phone auth selected.

fbbd014

Bug: 62309027 Change-Id: Iccbdb2a34e8c0b14c6fcf186005e0f1a68ea180e

Merge branch 'version-2.0.0-dev' into phone-auth

aa63079

Change-Id: Ib18e49c565e39c30bba83333aa8188fc853e1775

Fix all lint and checkstyle violations

47c1a6c

Change-Id: I5ff1752f718b91602020147a91fe56691b09318f

Fix more check failures

2217102

Change-Id: I0b884bdbe5e62713dc3a4edca8539cace74f3f18

SUPERCILEX reviewed Jun 7, 2017

View reviewed changes

Fix test failures by working around Facebook SDK

c8b5ed9

Change-Id: If1686bde8906f2e06d87c51fb8399be0b960028d

SUPERCILEX reviewed Jun 8, 2017

View reviewed changes

Respond to SUPERCILEX review feedback

f55c73b

Change-Id: I6eac4c2e92c13b1e5ad9a2953441566ad2d7c29e

samtstern merged commit 4137a21 into version-2.0.0-dev Jun 8, 2017

samtstern deleted the phone-auth branch June 15, 2017 15:37

Merge phone auth into version-2.0.0 #740

Merge phone auth into version-2.0.0 #740

Uh oh!

Conversation

samtstern commented Jun 7, 2017

Uh oh!

googlebot commented Jun 7, 2017

Uh oh!

ashwinraghav commented Jun 7, 2017

Uh oh!

samtstern commented Jun 7, 2017

Uh oh!

SUPERCILEX commented Jun 7, 2017

Uh oh!

SUPERCILEX commented Jun 7, 2017

Uh oh!

samtstern commented Jun 7, 2017

Uh oh!

SUPERCILEX commented Jun 7, 2017

Uh oh!

samtstern commented Jun 7, 2017

Uh oh!

samtstern commented Jun 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtstern commented Jun 7, 2017

Uh oh!

samtstern commented Jun 8, 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!

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 commented Jun 8, 2017

Uh oh!

samtstern commented Jun 8, 2017

Uh oh!

SUPERCILEX commented Jun 8, 2017

Uh oh!

Uh oh!