Specify additonal scopes through code rather than config.xml #342

amandle · 2016-10-06T22:22:08Z

No description provided.

samtstern · 2016-10-06T22:40:50Z

For scopes-only this approach LGTM, but I think we can give ourselves more breathing room if we instead have SignInIntentBuilder take some sort of IDPConfig<> object which has fields, one of which is a List<String> of scopes.

Then instead of calls to setProviders() and setAdditionalGoogleScopes() you could do something like enableProvider(GOOGLE_PROVIDER, mGoogleIdpConfig) or even have one method for each IDP like enableGoogleProvider(mGoogleIdpConfig).

This way if we want to add more customization in the future (IDPs can change underneath us) we don't need a breaking API change, we just add a new field to IDPConfig. It also makes the availability of the options clearer if you pass them at the time of enabling the IDP. This is like how in GmsCore with GoogleApiClient you do .addApi(mApi, mApiOptions).

amandle · 2016-10-08T23:44:08Z

I refactored quite heavily. There is now an IdpConfig class as you suggested, and setProviders takes a list of them. So as not to have two classes doing pretty much the same thing, and since the new IdpConfig class needs to be publicly available I removed IDPProviderParcel and replaced it with the IdpConfig class. This cleans things up a bit. This is kind of a large change now, apologies about that.

samtstern

This refactor looks really good, thanks! A few comments about how we can go even further.

samtstern · 2016-10-10T21:57:41Z

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

+
+        protected IdpConfig(Parcel in) {
+            mProviderId = in.readString();
+            mScopes = in.createStringArrayList();


This creates a new one, but it should read the value instead.

createStringArrayList actually does read the list, it just handles the creation of the list for you: https://developer.android.com/reference/android/os/Parcel.html#createStringArrayList()

Whoops, TIL!

samtstern · 2016-10-10T21:58:48Z

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

+             * https://developers.facebook.com/docs/facebook-login/android
+             * https://developers.facebook.com/docs/facebook-login/permissions
+             *
+             * For Google permissions see:


What about twitter?

Twitter scopes are configured only through the developer console. I will add that to the comment.

samtstern · 2016-10-10T21:59:56Z

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

    /**
     * Builder for the intent to start the user authentication flow.
     */
    public final class SignInIntentBuilder {
        private int mLogo = NO_LOGO;
        private int mTheme = getDefaultTheme();
-        private List<String> mProviders = Collections.singletonList(EMAIL_PROVIDER);
+        private List<IdpConfig> mProviders = new ArrayList<>();


Should this be a set? Seems like you want set-like properties in subsequent logic.

We do need set like properties, but we also need to retain the order. It looks like LinkedHashSet will do what we want.

samtstern · 2016-10-10T22:02:04Z

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

-    public FacebookProvider (Context appContext, IDPProviderParcel facebookParcel) {
+    public FacebookProvider (
+            Context appContext,
+            List<String> scopes) {


Why not just pass through the IDPConfig here? That way you only have to change this once.

samtstern · 2016-10-10T22:02:41Z

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

-    public GoogleProvider(FragmentActivity activity, IDPProviderParcel parcel, @Nullable String email) {
+    public GoogleProvider(
+            FragmentActivity activity,
+            @Nullable String email,


Same question here about just passing IDPConfig (and elsewhere, I'll stop repeating myself)

samtstern · 2016-10-10T22:02:55Z

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

        mActivity = activity;
-        String mClientId = parcel.getProviderExtra().getString(CLIENT_ID_KEY);
+        String mClientId = activity.getString(R.string.default_web_client_id);


Is this equivalent behavior before? What if they want to pass another client ID?

The behavior is the same, the clientId is still read from config.xml if they want to change it (same as before) but we are not sticking in a parcel and then reading it back, we're just using it directly.

samtstern · 2016-10-10T22:06:13Z

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

+     * In the simplest case, you can supply the provider ID and build the config like this:
+     * {@code new IdpConfig.Builder(AuthUI.GOOGLE_PROVIDER).build()}
+     */
+    public static class IdpConfig implements Parcelable {


Would it be better if each provider had a subclass of this? so new GoogleIdpConfig.Builder().build(), etc. This way you have more control over provider differences. For example doesn't look like you're supporting Twitter scopes so that builder would not have setScopes(). It would also hide the GOOGLE_PROVIDER and other Strings from the dev.

This one is up to you, I can see arguments both ways.

I tried this, but it makes parceling and passing a list of providers around much uglier. I would prefer to keep it this way.

Ok that's a good reason, keeping it as it is SGTM.

samtstern · 2016-10-10T22:07:08Z

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

+        } else {
+            mScopes = scopes;
+        }
+        String applicationId = appContext.getString(R.string.facebook_application_id);


If you took my suggestion of IdpConfig subclasses, these strings could be part of the builders and we could eliminate the resource-based config entirely.

samtstern · 2016-10-10T22:08:05Z

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

@@ -62,8 +65,7 @@ public GoogleProvider(FragmentActivity activity, IDPProviderParcel parcel, @Null
                .requestIdToken(mClientId);

        // Add additional scopes
-        String[] extraScopes = mActivity.getResources().getStringArray(R.array.google_permissions);


Bonus points: since we're removing these scopes as resources, maybe we could log a big warning message if we see them defined and they're non empty. Something like "DEVELOPER_WARNING: you have defined R.array.google_permissions but that is no longer respected as of FirebaseUI 1.0.0. See README"

Good idea, added.

samtstern · 2016-10-10T22:09:10Z

Also I merged master into version-1.0.0-dev today, I forgot this PR was outstanding so I apologize if there are conflicts for you (they should be simple if there are any)

samtstern

One last comment but LGTM!

samtstern · 2016-10-13T18:37:51Z

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

+
+        if (appContext.getResources().getIdentifier(
+                "google_permissions", "array", appContext.getPackageName()) != 0) {
+            Log.w(TAG, "DEVELOPER WARNING: You have defined R.array.facebook_permissions but that"


This says facebook_permissions in one place and google_permissions in another.

Oops, good catch.

amandle · 2016-10-14T21:03:33Z

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

@@ -229,22 +234,22 @@
     * Provider identifier for email and password credentials, for use with
     * {@link SignInIntentBuilder#setProviders}.
     */
-    public static final String EMAIL_PROVIDER = "email";
+    public static final String EMAIL_PROVIDER = EmailAuthProvider.PROVIDER_ID;


We had discussed this change previously and I was against it then, but since we are now exposing IdpConfig to the user of the library, the mapping is necessarily one to one, so I think this change makes sense now.

Agreed, seems like a sensible change.

samtstern · 2016-10-14T23:13:22Z

@amandle FYI you can carry my approval through from before. Merge when ready and the tests are green.

SUPERCILEX · 2016-10-16T22:34:58Z

@amandle Travis failed with exit code 137. I've been having that issue too, just restart the build or push some whitespace to get it going again. It's really random, but apparently it has something to do with the Travis sandbox only having 3GB of RAM available and us exceeding that.

amandle · 2016-10-16T22:42:03Z

@SUPERCILEX Thanks, I'll give that a try!

SUPERCILEX · 2016-10-16T23:27:15Z

@amandle I did some more Googling: see this issue travis-ci/travis-ci#5582 and this solution srsudar/MamasDelRioAndroid@ec64465.

SUPERCILEX · 2016-10-16T23:40:05Z

See #358

googlebot · 2016-10-17T18:03:22Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot · 2016-10-17T18:04:12Z

CLAs look good, thanks!

SUPERCILEX · 2016-10-17T18:09:53Z

Maybe undo f40ce5a to force a rerun?

samtstern · 2016-10-17T18:22:25Z

Note: when a build fails there is a Restart Build button on the page you can use:

amandle · 2016-10-17T18:26:16Z

@samtstern I still don't see that button :-(

SUPERCILEX · 2016-10-17T18:28:42Z

@samtstern unfortunately, that button is only available to repository admins (the person who set up Travis).

SUPERCILEX · 2016-10-17T18:31:53Z

@amandle Looks like a compile error:

/home/travis/build/firebase/FirebaseUI-Android/auth/src/main/java/com/firebase/ui/auth/ui/idp/IDPSignInContainerActivity.java:67: error: incompatible types: String cannot be converted to IdpConfig
            mIDPProvider = new GoogleProvider(this, mEmail, providerConfig);

SUPERCILEX · 2016-10-17T19:35:47Z

I'm sorry, I just realized that I merged the Travis fix into master instead of 1.0.0. @samtstern could you merge master into dev or @amandle you could just merge master into this PR directly since I think the Travis fix is the only difference between master and dev?

samtstern · 2016-10-17T20:33:48Z

@SUPERCILEX @amandle I merged master into 1.0.0-dev, the only changes were travis.yml and your small change to SaveCredentialsActivity

…rcel

This reverts commit 327bf1b.

SUPERCILEX · 2016-10-17T20:41:48Z

Yes! Thanks @samtstern it worked! Also, look at that build time!!! 4mins vs the usual ~10mins!

samtstern reviewed Oct 10, 2016

View reviewed changes

amandle force-pushed the issue_294_2 branch from 9368dd1 to f9c002f Compare October 13, 2016 18:30

samtstern approved these changes Oct 13, 2016

View reviewed changes

amandle force-pushed the issue_294_2 branch from c66b209 to 85f16cd Compare October 14, 2016 21:01

amandle commented Oct 14, 2016

View reviewed changes

amandle force-pushed the issue_294_2 branch from 2b4f4a2 to f40ce5a Compare October 17, 2016 18:04

amandle force-pushed the issue_294_2 branch from f40ce5a to c4266b5 Compare October 17, 2016 18:15

Aaron Mandle added 6 commits October 17, 2016 13:35

Specify additonal scopes thorugh code rather than config.xml

d4953f9

Refactor to use IdpConfig to configure IDPs, and remove IDPProviderPa…

d86dd6f

…rcel

review feedback

cc855b1

Fix facebook_permissions resource check

428a97e

Fix tests

146bab6

Update the README to reflect the new API changes

38aa969

Aaron Mandle added 4 commits October 17, 2016 13:35

Add sources to javadoc task

3d514fe

Force rerun of Travis

0f707e7

Revert "Force rerun of Travis"

9884147

This reverts commit 327bf1b.

fix merge mistake

a03e918

amandle force-pushed the issue_294_2 branch from 5437179 to a03e918 Compare October 17, 2016 20:35

amandle changed the title ~~Specify additonal scopes thorugh code rather than config.xml~~ Specify additonal scopes through code rather than config.xml Oct 17, 2016

amandle merged commit 897b455 into firebase:version-1.0.0-dev Oct 17, 2016

Specify additonal scopes through code rather than config.xml #342

Specify additonal scopes through code rather than config.xml #342

Uh oh!

Conversation

amandle commented Oct 6, 2016

Uh oh!

samtstern commented Oct 6, 2016

Uh oh!

amandle commented Oct 8, 2016

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!

samtstern Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Oct 10, 2016

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!

samtstern commented Oct 14, 2016

Uh oh!

SUPERCILEX commented Oct 16, 2016

Uh oh!

amandle commented Oct 16, 2016

Uh oh!

SUPERCILEX commented Oct 16, 2016

Uh oh!

SUPERCILEX commented Oct 16, 2016

Uh oh!

googlebot commented Oct 17, 2016

Uh oh!

googlebot commented Oct 17, 2016

Uh oh!

samtstern Oct 10, 2016 •

edited

Loading

SUPERCILEX commented Oct 17, 2016 •

edited

Loading

SUPERCILEX commented Oct 17, 2016 •

edited

Loading