From ade033e37956f2a391fb36b250fc810778ecdf39 Mon Sep 17 00:00:00 2001 From: Alex Saveau Date: Tue, 14 Feb 2017 18:09:07 -0800 Subject: [PATCH 1/4] Make `AuthUI.SignInIntentBuilder#setProviders(List)` smarter Signed-off-by: Alex Saveau --- .../java/com/firebase/ui/auth/AuthUI.java | 78 +++++++++++++------ .../java/com/firebase/ui/auth/AuthUITest.java | 32 ++++++-- 2 files changed, 81 insertions(+), 29 deletions(-) diff --git a/auth/src/main/java/com/firebase/ui/auth/AuthUI.java b/auth/src/main/java/com/firebase/ui/auth/AuthUI.java index 5b9f502a9..0a59c32ce 100644 --- a/auth/src/main/java/com/firebase/ui/auth/AuthUI.java +++ b/auth/src/main/java/com/firebase/ui/auth/AuthUI.java @@ -55,7 +55,6 @@ import java.util.Collections; import java.util.HashSet; import java.util.IdentityHashMap; -import java.util.LinkedHashSet; import java.util.List; import java.util.Set; @@ -315,9 +314,9 @@ public static class IdpConfig implements Parcelable { private final String mProviderId; private final List mScopes; - private IdpConfig(@NonNull String providerId, List scopes) { - mScopes = scopes; + private IdpConfig(String providerId, List scopes) { mProviderId = providerId; + mScopes = scopes; } private IdpConfig(Parcel in) { @@ -356,11 +355,33 @@ public void writeToParcel(Parcel parcel, int i) { parcel.writeStringList(mScopes); } + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + + IdpConfig config = (IdpConfig) o; + + return mProviderId.equals(config.mProviderId); + } + + @Override + public int hashCode() { + return mProviderId.hashCode(); + } + + @Override + public String toString() { + return "IdpConfig{" + + "mProviderId='" + mProviderId + '\'' + + ", mScopes=" + mScopes + + '}'; + } + public static class Builder { private String mProviderId; private List mScopes = new ArrayList<>(); - /** * Builds the configuration parameters for an identity provider. * @@ -406,7 +427,7 @@ public IdpConfig build() { public final class SignInIntentBuilder { private int mLogo = NO_LOGO; private int mTheme = getDefaultTheme(); - private LinkedHashSet mProviders = new LinkedHashSet<>(); + private List mProviders = new ArrayList<>(); private String mTosUrl; private boolean mIsSmartLockEnabled = true; private boolean mAllowNewEmailAccounts = true; @@ -458,19 +479,30 @@ public SignInIntentBuilder setTosUrl(@Nullable String tosUrl) { */ public SignInIntentBuilder setProviders(@NonNull List idpConfigs) { mProviders.clear(); - Set configuredProviders = new HashSet<>(); - for (IdpConfig idpConfig : idpConfigs) { - if (configuredProviders.contains(idpConfig.getProviderId())) { - throw new IllegalArgumentException("Each provider can only be set once. " - + idpConfig.getProviderId() - + " was set twice."); + for (IdpConfig config : idpConfigs) { + if (mProviders.contains(config)) { + int i = mProviders.indexOf(config); + IdpConfig newConfig = + new IdpConfig.Builder(config.getProviderId()) + .setPermissions(getUniqueScopes(config.getScopes(), + mProviders.get(i).getScopes())) + .build(); + mProviders.set(i, newConfig); + } else { + mProviders.add(config); } - configuredProviders.add(idpConfig.getProviderId()); - mProviders.add(idpConfig); } return this; } + private List getUniqueScopes(List scopes1, List scopes2) { + List mergedScopes = new ArrayList<>(scopes2); + for (String scope : scopes1) { + if (!mergedScopes.contains(scope)) mergedScopes.add(scope); + } + return mergedScopes; + } + /** * Specifies the set of supported authentication providers. At least one provider * must be specified, and the set of providers must be a subset of @@ -495,6 +527,15 @@ public SignInIntentBuilder setProviders(@NonNull String... providers) { return this; } + private boolean isIdpAlreadyConfigured(@NonNull String providerId) { + for (IdpConfig config : mProviders) { + if (config.getProviderId().equals(providerId)) { + return true; + } + } + return false; + } + /** * Enables or disables the use of Smart Lock for Passwords in the sign in flow. *

@@ -515,15 +556,6 @@ public SignInIntentBuilder setAllowNewEmailAccounts(boolean enabled) { return this; } - private boolean isIdpAlreadyConfigured(@NonNull String providerId) { - for (IdpConfig config : mProviders) { - if (config.getProviderId().equals(providerId)) { - return true; - } - } - return false; - } - public Intent build() { return KickoffActivity.createIntent(mApp.getApplicationContext(), getFlowParams()); } @@ -531,7 +563,7 @@ public Intent build() { @VisibleForTesting() public FlowParameters getFlowParams() { return new FlowParameters(mApp.getName(), - new ArrayList<>(mProviders), + mProviders, mTheme, mLogo, mTosUrl, diff --git a/auth/src/test/java/com/firebase/ui/auth/AuthUITest.java b/auth/src/test/java/com/firebase/ui/auth/AuthUITest.java index b71412cd2..c78a51d9b 100644 --- a/auth/src/test/java/com/firebase/ui/auth/AuthUITest.java +++ b/auth/src/test/java/com/firebase/ui/auth/AuthUITest.java @@ -29,8 +29,11 @@ import org.robolectric.annotation.Config; import java.util.Arrays; +import java.util.Collections; +import java.util.List; import static junit.framework.Assert.assertEquals; +import static junit.framework.Assert.assertTrue; @RunWith(CustomRobolectricGradleTestRunner.class) @Config(constants = BuildConfig.class, sdk = 25) @@ -63,13 +66,30 @@ public void testCreateStartIntent_shouldHaveEmailAsDefaultProvider() { assertEquals(AuthUI.EMAIL_PROVIDER, flowParameters.providerInfo.get(0).getProviderId()); } - @Test(expected = IllegalArgumentException.class) + @Test public void testCreateStartIntent_shouldOnlyAllowOneInstanceOfAnIdp() { - SignInIntentBuilder startIntent = - AuthUI.getInstance(mFirebaseApp).createSignInIntentBuilder(); - startIntent.setProviders( - Arrays.asList(new IdpConfig.Builder(AuthUI.EMAIL_PROVIDER).build(), - new IdpConfig.Builder(AuthUI.EMAIL_PROVIDER).build())); + SignInIntentBuilder builder = AuthUI.getInstance(mFirebaseApp).createSignInIntentBuilder(); + builder.setProviders(Arrays.asList( + new IdpConfig.Builder(AuthUI.EMAIL_PROVIDER) + .setPermissions(Collections.singletonList("A")) + .build(), + new IdpConfig.Builder(AuthUI.EMAIL_PROVIDER) + .setPermissions(Arrays.asList("A", "B")) + .build(), + new IdpConfig.Builder(AuthUI.GOOGLE_PROVIDER) + .build())); + + + List providers = builder.getFlowParams().providerInfo; + assertTrue(providers.size() == 2); + + IdpConfig email = providers.get(0); + List emailScopes = email.getScopes(); + assertTrue(email.getProviderId().equals(AuthUI.EMAIL_PROVIDER)); + assertTrue(emailScopes.size() == 2); + assertTrue(emailScopes.contains("A") && emailScopes.contains("B")); + + assertTrue(providers.get(1).getProviderId().equals(AuthUI.GOOGLE_PROVIDER)); } @Test From 98da0681362c54569785980e96e4186d14b82ccf Mon Sep 17 00:00:00 2001 From: Alex Saveau Date: Wed, 15 Feb 2017 08:12:59 -0800 Subject: [PATCH 2/4] Move adding email provider to build method where it makes more sense Signed-off-by: Alex Saveau --- auth/src/main/java/com/firebase/ui/auth/AuthUI.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/auth/src/main/java/com/firebase/ui/auth/AuthUI.java b/auth/src/main/java/com/firebase/ui/auth/AuthUI.java index 0a59c32ce..3403ad699 100644 --- a/auth/src/main/java/com/firebase/ui/auth/AuthUI.java +++ b/auth/src/main/java/com/firebase/ui/auth/AuthUI.java @@ -433,7 +433,6 @@ public final class SignInIntentBuilder { private boolean mAllowNewEmailAccounts = true; private SignInIntentBuilder() { - mProviders.add(new IdpConfig.Builder(EMAIL_PROVIDER).build()); } /** @@ -562,6 +561,10 @@ public Intent build() { @VisibleForTesting() public FlowParameters getFlowParams() { + if (mProviders.isEmpty()) { + mProviders.add(new IdpConfig.Builder(EMAIL_PROVIDER).build()); + } + return new FlowParameters(mApp.getName(), mProviders, mTheme, From 44a5c6ae77a2d370661a7f72f6ad1dbf2e729bc2 Mon Sep 17 00:00:00 2001 From: Alex Saveau Date: Wed, 15 Feb 2017 08:15:16 -0800 Subject: [PATCH 3/4] Use assertEquals in tests Signed-off-by: Alex Saveau --- auth/src/test/java/com/firebase/ui/auth/AuthUITest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/auth/src/test/java/com/firebase/ui/auth/AuthUITest.java b/auth/src/test/java/com/firebase/ui/auth/AuthUITest.java index c78a51d9b..22d570bd2 100644 --- a/auth/src/test/java/com/firebase/ui/auth/AuthUITest.java +++ b/auth/src/test/java/com/firebase/ui/auth/AuthUITest.java @@ -81,15 +81,15 @@ public void testCreateStartIntent_shouldOnlyAllowOneInstanceOfAnIdp() { List providers = builder.getFlowParams().providerInfo; - assertTrue(providers.size() == 2); + assertEquals(2, providers.size()); IdpConfig email = providers.get(0); List emailScopes = email.getScopes(); - assertTrue(email.getProviderId().equals(AuthUI.EMAIL_PROVIDER)); - assertTrue(emailScopes.size() == 2); + assertEquals(AuthUI.EMAIL_PROVIDER, email.getProviderId()); + assertEquals(2, emailScopes.size()); assertTrue(emailScopes.contains("A") && emailScopes.contains("B")); - assertTrue(providers.get(1).getProviderId().equals(AuthUI.GOOGLE_PROVIDER)); + assertEquals(AuthUI.GOOGLE_PROVIDER, providers.get(1).getProviderId()); } @Test From 6bde398d7fd53a3e4e99efca216b1b8ef7186aef Mon Sep 17 00:00:00 2001 From: Alex Saveau Date: Thu, 16 Feb 2017 14:59:30 -0800 Subject: [PATCH 4/4] Remove non-perf related changes Signed-off-by: Alex Saveau --- .../java/com/firebase/ui/auth/AuthUI.java | 29 ++++++++++++++++- .../java/com/firebase/ui/auth/AuthUITest.java | 32 ++++--------------- 2 files changed, 34 insertions(+), 27 deletions(-) diff --git a/auth/src/main/java/com/firebase/ui/auth/AuthUI.java b/auth/src/main/java/com/firebase/ui/auth/AuthUI.java index f00437056..7db44dddf 100644 --- a/auth/src/main/java/com/firebase/ui/auth/AuthUI.java +++ b/auth/src/main/java/com/firebase/ui/auth/AuthUI.java @@ -355,6 +355,29 @@ public void writeToParcel(Parcel parcel, int i) { parcel.writeStringList(mScopes); } + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + + IdpConfig config = (IdpConfig) o; + + return mProviderId.equals(config.mProviderId); + } + + @Override + public int hashCode() { + return mProviderId.hashCode(); + } + + @Override + public String toString() { + return "IdpConfig{" + + "mProviderId='" + mProviderId + '\'' + + ", mScopes=" + mScopes + + '}'; + } + public static class Builder { private String mProviderId; private List mScopes = new ArrayList<>(); @@ -456,7 +479,11 @@ public SignInIntentBuilder setTosUrl(@Nullable String tosUrl) { public SignInIntentBuilder setProviders(@NonNull List idpConfigs) { mProviders.clear(); for (IdpConfig config : idpConfigs) { - if (!mProviders.contains(config)) { + if (mProviders.contains(config)) { + throw new IllegalArgumentException("Each provider can only be set once. " + + config.getProviderId() + + " was set twice."); + } else { mProviders.add(config); } } diff --git a/auth/src/test/java/com/firebase/ui/auth/AuthUITest.java b/auth/src/test/java/com/firebase/ui/auth/AuthUITest.java index 22d570bd2..b71412cd2 100644 --- a/auth/src/test/java/com/firebase/ui/auth/AuthUITest.java +++ b/auth/src/test/java/com/firebase/ui/auth/AuthUITest.java @@ -29,11 +29,8 @@ import org.robolectric.annotation.Config; import java.util.Arrays; -import java.util.Collections; -import java.util.List; import static junit.framework.Assert.assertEquals; -import static junit.framework.Assert.assertTrue; @RunWith(CustomRobolectricGradleTestRunner.class) @Config(constants = BuildConfig.class, sdk = 25) @@ -66,30 +63,13 @@ public void testCreateStartIntent_shouldHaveEmailAsDefaultProvider() { assertEquals(AuthUI.EMAIL_PROVIDER, flowParameters.providerInfo.get(0).getProviderId()); } - @Test + @Test(expected = IllegalArgumentException.class) public void testCreateStartIntent_shouldOnlyAllowOneInstanceOfAnIdp() { - SignInIntentBuilder builder = AuthUI.getInstance(mFirebaseApp).createSignInIntentBuilder(); - builder.setProviders(Arrays.asList( - new IdpConfig.Builder(AuthUI.EMAIL_PROVIDER) - .setPermissions(Collections.singletonList("A")) - .build(), - new IdpConfig.Builder(AuthUI.EMAIL_PROVIDER) - .setPermissions(Arrays.asList("A", "B")) - .build(), - new IdpConfig.Builder(AuthUI.GOOGLE_PROVIDER) - .build())); - - - List providers = builder.getFlowParams().providerInfo; - assertEquals(2, providers.size()); - - IdpConfig email = providers.get(0); - List emailScopes = email.getScopes(); - assertEquals(AuthUI.EMAIL_PROVIDER, email.getProviderId()); - assertEquals(2, emailScopes.size()); - assertTrue(emailScopes.contains("A") && emailScopes.contains("B")); - - assertEquals(AuthUI.GOOGLE_PROVIDER, providers.get(1).getProviderId()); + SignInIntentBuilder startIntent = + AuthUI.getInstance(mFirebaseApp).createSignInIntentBuilder(); + startIntent.setProviders( + Arrays.asList(new IdpConfig.Builder(AuthUI.EMAIL_PROVIDER).build(), + new IdpConfig.Builder(AuthUI.EMAIL_PROVIDER).build())); } @Test