Skip to content

Bug fixes and perf improvements for setting providers in AuthUI #591

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.

Sign up for GitHub

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
samtstern merged 6 commits into from
Feb 24, 2017
Merged

Bug fixes and perf improvements for setting providers in AuthUI #591

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ade033e
Make `AuthUI.SignInIntentBuilder#setProviders(List<IdpConfig>)` smarter
SUPERCILEX Feb 15, 2017
98da068
Move adding email provider to build method where it makes more sense
SUPERCILEX Feb 15, 2017
44a5c6a
Use assertEquals in tests
SUPERCILEX Feb 15, 2017
fe38001
Merge remote-tracking branch 'firebase/version-1.2.0-dev' into providers
SUPERCILEX Feb 16, 2017
6bde398
Remove non-perf related changes
SUPERCILEX Feb 16, 2017
9ff62dd
Merge remote-tracking branch 'firebase/master' into providers
SUPERCILEX Feb 21, 2017
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
63 changes: 43 additions & 20 deletions auth/src/main/java/com/firebase/ui/auth/AuthUI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Copy link
Collaborator Author

@SUPERCILEX SUPERCILEX Feb 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samtstern Identifying providers by provider id only and completely ignoring scopes makes sense to me, do you agree?

}

@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<String> mScopes = new ArrayList<>();


/**
* Builds the configuration parameters for an identity provider.
*
Expand Down Expand Up @@ -406,13 +427,12 @@ public IdpConfig build() {
public final class SignInIntentBuilder {
private int mLogo = NO_LOGO;
private int mTheme = getDefaultTheme();
private LinkedHashSet<IdpConfig> mProviders = new LinkedHashSet<>();
private List<IdpConfig> mProviders = new ArrayList<>();
Copy link
Collaborator Author

@SUPERCILEX SUPERCILEX Feb 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much better, we were converting this to a list anyway which seems kinda pointless.

private String mTosUrl;
private boolean mIsSmartLockEnabled = true;
private boolean mAllowNewEmailAccounts = true;

private SignInIntentBuilder() {
mProviders.add(new IdpConfig.Builder(EMAIL_PROVIDER).build());
}

/**
Expand Down Expand Up @@ -458,15 +478,14 @@ public SignInIntentBuilder setTosUrl(@Nullable String tosUrl) {
*/
public SignInIntentBuilder setProviders(@NonNull List<IdpConfig> idpConfigs) {
mProviders.clear();
Set<String> configuredProviders = new HashSet<>();
for (IdpConfig idpConfig : idpConfigs) {
if (configuredProviders.contains(idpConfig.getProviderId())) {
for (IdpConfig config : idpConfigs) {
if (mProviders.contains(config)) {
throw new IllegalArgumentException("Each provider can only be set once. "
+ idpConfig.getProviderId()
+ config.getProviderId()
+ " was set twice.");
} else {
mProviders.add(config);
}
configuredProviders.add(idpConfig.getProviderId());
mProviders.add(idpConfig);
}
return this;
}
Expand Down Expand Up @@ -495,6 +514,15 @@ public SignInIntentBuilder setProviders(@NonNull String... providers) {
return this;
}

private boolean isIdpAlreadyConfigured(@NonNull String providerId) {
Copy link
Collaborator Author

@SUPERCILEX SUPERCILEX Feb 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it to where it's being used (in the deprecated setProviders).

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.
* <p>
Expand All @@ -515,23 +543,18 @@ 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());
}

@VisibleForTesting()
public FlowParameters getFlowParams() {
if (mProviders.isEmpty()) {
mProviders.add(new IdpConfig.Builder(EMAIL_PROVIDER).build());
}
Copy link
Collaborator Author

@SUPERCILEX SUPERCILEX Feb 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why we should always do this kind of thing in the build method: if people call setProviders(Collections.emptyList()), we will start the auth flow with no providers. Now, we can guarantee that at least the email provider is provided.


return new FlowParameters(mApp.getName(),
new ArrayList<>(mProviders),
mProviders,
mTheme,
mLogo,
mTosUrl,
Expand Down