Skip to content

Rank providers by usability when picking top provider #1362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 28, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public class FlowParameters implements Parcelable {
public final String appName;

@NonNull
public final List<IdpConfig> providerInfo;
public final List<IdpConfig> providers;

@StyleRes
public final int themeId;
Expand All @@ -60,7 +60,7 @@ public class FlowParameters implements Parcelable {

public FlowParameters(
@NonNull String appName,
@NonNull List<IdpConfig> providerInfo,
@NonNull List<IdpConfig> providers,
@StyleRes int themeId,
@DrawableRes int logoId,
@Nullable String termsOfServiceUrl,
Expand All @@ -69,8 +69,8 @@ public FlowParameters(
boolean enableHints,
boolean enableAnonymousUpgrade) {
this.appName = Preconditions.checkNotNull(appName, "appName cannot be null");
this.providerInfo = Collections.unmodifiableList(
Preconditions.checkNotNull(providerInfo, "providerInfo cannot be null"));
this.providers = Collections.unmodifiableList(
Preconditions.checkNotNull(providers, "providers cannot be null"));
this.themeId = themeId;
this.logoId = logoId;
this.termsOfServiceUrl = termsOfServiceUrl;
Expand All @@ -90,7 +90,7 @@ public static FlowParameters fromIntent(Intent intent) {
@Override
public void writeToParcel(Parcel dest, int flags) {
dest.writeString(appName);
dest.writeTypedList(providerInfo);
dest.writeTypedList(providers);
dest.writeInt(themeId);
dest.writeInt(logoId);
dest.writeString(termsOfServiceUrl);
Expand Down Expand Up @@ -137,7 +137,7 @@ public FlowParameters[] newArray(int size) {
};

public boolean isSingleProviderFlow() {
return providerInfo.size() == 1;
return providers.size() == 1;
}

public boolean isTermsOfServiceUrlProvided() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public SignInKickstarter(Application application) {
public void start() {
// Only support password credentials if email auth is enabled
boolean supportPasswords = ProviderUtils.getConfigFromIdps(
getArguments().providerInfo, EmailAuthProvider.PROVIDER_ID) != null;
getArguments().providers, EmailAuthProvider.PROVIDER_ID) != null;
List<String> accountTypes = getCredentialAccountTypes();

// If the request will be empty, avoid the step entirely
Expand Down Expand Up @@ -97,7 +97,7 @@ public void onComplete(@NonNull Task<CredentialRequestResponse> task) {
private void startAuthMethodChoice() {
// If there is only one provider selected, launch the flow directly
if (getArguments().isSingleProviderFlow()) {
AuthUI.IdpConfig firstIdpConfig = getArguments().providerInfo.get(0);
AuthUI.IdpConfig firstIdpConfig = getArguments().providers.get(0);
String firstProvider = firstIdpConfig.getProviderId();
switch (firstProvider) {
case EmailAuthProvider.PROVIDER_ID:
Expand Down Expand Up @@ -157,7 +157,7 @@ private void redirectSignIn(String provider, String id) {

private List<String> getCredentialAccountTypes() {
List<String> accounts = new ArrayList<>();
for (AuthUI.IdpConfig idpConfig : getArguments().providerInfo) {
for (AuthUI.IdpConfig idpConfig : getArguments().providers) {
@AuthUI.SupportedProvider String providerId = idpConfig.getProviderId();
if (providerId.equals(GoogleAuthProvider.PROVIDER_ID)) {
accounts.add(ProviderUtils.providerIdToAccountType(providerId));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public void fetchCredential() {

public void fetchProvider(final String email) {
setResult(Resource.<User>forLoading());
ProviderUtils.fetchTopProvider(getAuth(), email)
ProviderUtils.fetchTopProvider(getAuth(), getArguments(), email)
.addOnCompleteListener(new OnCompleteListener<String>() {
@Override
public void onComplete(@NonNull Task<String> task) {
Expand All @@ -53,7 +53,7 @@ public void onActivityResult(int requestCode, int resultCode, @Nullable Intent d
setResult(Resource.<User>forLoading());
final Credential credential = data.getParcelableExtra(Credential.EXTRA_KEY);
final String email = credential.getId();
ProviderUtils.fetchTopProvider(getAuth(), email)
ProviderUtils.fetchTopProvider(getAuth(), getArguments(), email)
.addOnCompleteListener(new OnCompleteListener<String>() {
@Override
public void onComplete(@NonNull Task<String> task) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public void onNewUser(User user) {
TextInputLayout emailLayout = findViewById(R.id.email_layout);

AuthUI.IdpConfig emailConfig = ProviderUtils.getConfigFromIdpsOrThrow(
getFlowParams().providerInfo, EmailAuthProvider.PROVIDER_ID);
getFlowParams().providers, EmailAuthProvider.PROVIDER_ID);
if (emailConfig.getParams().getBoolean(ExtraConstants.ALLOW_NEW_EMAILS, true)) {
RegisterEmailFragment fragment = RegisterEmailFragment.newInstance(user);
FragmentTransaction ft = getSupportFragmentManager().beginTransaction()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceStat

// Get configuration
AuthUI.IdpConfig emailConfig = ProviderUtils.getConfigFromIdpsOrThrow(
getFlowParams().providerInfo, EmailAuthProvider.PROVIDER_ID);
getFlowParams().providers, EmailAuthProvider.PROVIDER_ID);
boolean requireName = emailConfig.getParams()
.getBoolean(ExtraConstants.REQUIRE_NAME, true);
mPasswordFieldValidator = new PasswordFieldValidator(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ protected void onCreate(@Nullable Bundle savedInstanceState) {
mHandler = ViewModelProviders.of(this).get(SocialProviderResponseHandler.class);
mHandler.init(params);

populateIdpList(params.providerInfo, mHandler);
populateIdpList(params.providers, mHandler);

int logoId = params.logoId;
if (logoId == AuthUI.NO_LOGO) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ protected void onCreate(@Nullable Bundle savedInstanceState) {
String provider = user.getProviderId();

AuthUI.IdpConfig providerConfig =
ProviderUtils.getConfigFromIdps(getFlowParams().providerInfo, provider);
ProviderUtils.getConfigFromIdps(getFlowParams().providers, provider);
if (providerConfig == null) {
finish(RESULT_CANCELED, IdpResponse.getErrorIntent(new FirebaseUiException(
ErrorCodes.DEVELOPER_ERROR,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ protected void onCreate(@Nullable Bundle savedInstanceState) {

String providerId = existingUser.getProviderId();
AuthUI.IdpConfig config =
ProviderUtils.getConfigFromIdps(getFlowParams().providerInfo, providerId);
ProviderUtils.getConfigFromIdps(getFlowParams().providers, providerId);
if (config == null) {
finish(RESULT_CANCELED, IdpResponse.getErrorIntent(new FirebaseUiException(
ErrorCodes.DEVELOPER_ERROR,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

import com.firebase.ui.auth.AuthUI;
import com.firebase.ui.auth.IdpResponse;
import com.firebase.ui.auth.util.Preconditions;
import com.firebase.ui.auth.data.model.FlowParameters;
import com.google.android.gms.auth.api.credentials.IdentityProviders;
import com.google.android.gms.tasks.Continuation;
import com.google.android.gms.tasks.Task;
Expand All @@ -36,6 +36,7 @@
import com.google.firebase.auth.SignInMethodQueryResult;
import com.google.firebase.auth.TwitterAuthProvider;

import java.util.ArrayList;
import java.util.List;

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
Expand Down Expand Up @@ -154,41 +155,63 @@ public static AuthUI.IdpConfig getConfigFromIdpsOrThrow(List<AuthUI.IdpConfig> i
return config;
}

public static Task<String> fetchTopProvider(FirebaseAuth auth, @NonNull String email) {
public static Task<List<String>> fetchSortedProviders(@NonNull FirebaseAuth auth,
@NonNull final FlowParameters params,
@NonNull String email) {
if (TextUtils.isEmpty(email)) {
return Tasks.forException(new NullPointerException("Email cannot be empty"));
}

return auth.fetchSignInMethodsForEmail(email)
.continueWith(new Continuation<SignInMethodQueryResult, String>() {
.continueWith(new Continuation<SignInMethodQueryResult, List<String>>() {
@Override
public String then(@NonNull Task<SignInMethodQueryResult> task) {
if (!task.isSuccessful()) return null;

public List<String> then(@NonNull Task<SignInMethodQueryResult> task) {
List<String> methods = task.getResult().getSignInMethods();
return methods == null || methods.isEmpty()
? null : methods.get(methods.size() - 1);
if (methods == null) { methods = new ArrayList<>(); }

List<String> allowedProviders = new ArrayList<>(params.providers.size());
for (AuthUI.IdpConfig provider : params.providers) {
allowedProviders.add(provider.getProviderId());
}

List<String> lastSignedInProviders = new ArrayList<>(methods.size());
for (String method : methods) {
String id = signInMethodToProviderId(method);
if (allowedProviders.contains(id)) {
lastSignedInProviders.add(0, id);
}
}

// Reorder providers from most to least usable. Usability is determined by
// how many steps a user needs to perform to log in.
maximizePriority(lastSignedInProviders, GoogleAuthProvider.PROVIDER_ID);

return lastSignedInProviders;
}

private void maximizePriority(List<String> providers, String id) {
if (providers.remove(id)) { providers.add(0, id); }
}
}).continueWith(new Continuation<String, String>() {
});
}

public static Task<String> fetchTopProvider(
@NonNull FirebaseAuth auth,
@NonNull FlowParameters params,
@NonNull String email) {
return fetchSortedProviders(auth, params, email)
.continueWith(new Continuation<List<String>, String>() {
@Override
public String then(@NonNull Task<String> task) {
String method = task.getResult();
if (method == null) {
public String then(@NonNull Task<List<String>> task) {
if (!task.isSuccessful()) return null;
List<String> providers = task.getResult();

if (providers.isEmpty()) {
return null;
} else {
return signInMethodToProviderId(method);
return providers.get(0);
}
}
});
}

public static String getTopProvider(@NonNull List<String> providers) {
return providers == null || providers.isEmpty() ? null :
providers.get(providers.size() - 1);
}

public static boolean isExistingProvider(@NonNull List<String> providers, String provider) {
if (providers == null) throw new IllegalArgumentException("The list of providers is null.");
return providers.contains(provider);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public void onFailure(@NonNull Exception e) {
// Collision with existing user email without anonymous upgrade
// it should be very hard for the user to even get to this error
// due to CheckEmailFragment.
ProviderUtils.fetchTopProvider(getAuth(), email)
ProviderUtils.fetchTopProvider(getAuth(), getArguments(), email)
.addOnSuccessListener(new StartWelcomeBackFlow(email))
.addOnFailureListener(new OnFailureListener() {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import com.google.firebase.auth.AuthResult;
import com.google.firebase.auth.EmailAuthProvider;
import com.google.firebase.auth.FirebaseAuthUserCollisionException;
import com.google.firebase.auth.SignInMethodQueryResult;

import java.util.List;

Expand Down Expand Up @@ -64,7 +63,7 @@ public void onSuccess(AuthResult result) {
})
.addOnFailureListener(new OnFailureListener() {
@Override
public void onFailure(@NonNull final Exception e) {
public void onFailure(@NonNull Exception e) {
if (e instanceof FirebaseAuthUserCollisionException) {
final String email = response.getEmail();
if (email == null) {
Expand All @@ -80,20 +79,22 @@ public void onFailure(@NonNull final Exception e) {
// existing user.
// CASE 3: CASE 2 with an anonymous user. We link the new IDP to the
// same account before handling invoking a merge failure.
getAuth().fetchSignInMethodsForEmail(email)
.addOnSuccessListener(new OnSuccessListener<SignInMethodQueryResult>() {
ProviderUtils.fetchSortedProviders(getAuth(), getArguments(), email)
.addOnSuccessListener(new OnSuccessListener<List<String>>() {
@Override
public void onSuccess(SignInMethodQueryResult result) {
List<String> providers = result.getSignInMethods();
if (ProviderUtils.isExistingProvider(providers,
response.getProviderType())) {
public void onSuccess(List<String> providers) {
if (providers.contains(response.getProviderType())) {
// Case 1
handleMergeFailure(credential);
} else if (providers.isEmpty()) {
setResult(Resource.<IdpResponse>forFailure(
new FirebaseUiException(
ErrorCodes.DEVELOPER_ERROR,
"No supported providers.")));
} else {
// Case 2 & 3 - we need to link
startWelcomeBackFlowForLinking(
ProviderUtils.getTopProvider(providers),
response);
providers.get(0), response);
}
}
})
Expand Down
6 changes: 3 additions & 3 deletions auth/src/test/java/com/firebase/ui/auth/AuthUITest.java
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ public void testCreateStartIntent_shouldHaveEmailAsDefaultProvider() {
.createSignInIntentBuilder()
.build()
.getParcelableExtra(ExtraConstants.FLOW_PARAMS);
assertEquals(1, flowParameters.providerInfo.size());
assertEquals(1, flowParameters.providers.size());
assertEquals(EmailAuthProvider.PROVIDER_ID,
flowParameters.providerInfo.get(0).getProviderId());
flowParameters.providers.get(0).getProviderId());
}

@Test(expected = IllegalArgumentException.class)
Expand All @@ -75,7 +75,7 @@ public void testCreatingStartIntent() {
.build()
.getParcelableExtra(ExtraConstants.FLOW_PARAMS);

assertEquals(4, flowParameters.providerInfo.size());
assertEquals(4, flowParameters.providers.size());
assertEquals(TestHelper.MOCK_APP.getName(), flowParameters.appName);
assertEquals(TestConstants.TOS_URL, flowParameters.termsOfServiceUrl);
assertEquals(TestConstants.PRIVACY_URL, flowParameters.privacyPolicyUrl);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -311,8 +311,7 @@ public void testSignInIdp_anonymousUserUpgradeEnabledAndExistingPasswordUserWith

private void setupAnonymousUpgrade() {
// enableAnonymousUpgrade must be set to true
FlowParameters testParams = TestHelper.getFlowParameters(Collections.singletonList(
EmailAuthProvider.PROVIDER_ID), /* enableAnonymousUpgrade */ true);
FlowParameters testParams = TestHelper.getFlowParameters(AuthUI.SUPPORTED_PROVIDERS, /* enableAnonymousUpgrade */ true);
mHandler.initializeForTesting(testParams, mMockAuth, null, null);

when(mUser.isAnonymous()).thenReturn(true);
Expand Down