Skip to content

Completely rewrite provider login with new architecture #1189

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 41 commits into from
Mar 23, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
a1c7dda
Prep for final refactors
SUPERCILEX Mar 15, 2018
3325fda
Perf improvements
SUPERCILEX Mar 16, 2018
fde1bca
Fix loose ends
SUPERCILEX Mar 16, 2018
6f0e2c9
Start addressing review feedback
SUPERCILEX Mar 16, 2018
624aa6e
Move usage updates into getters
SUPERCILEX Mar 16, 2018
3acea2d
Merge improvements from future PRs
SUPERCILEX Mar 16, 2018
dd9ae69
Address review feedback
SUPERCILEX Mar 16, 2018
887ae0d
Fancisize tests
SUPERCILEX Mar 16, 2018
fdb9f94
Completely rewrite provider login with new architecture
SUPERCILEX Mar 16, 2018
5511513
Cleanup and pre-testing bug fixes
SUPERCILEX Mar 16, 2018
1be0b8b
Make lint happy
SUPERCILEX Mar 16, 2018
0a65776
Fix tests
SUPERCILEX Mar 16, 2018
f0dc341
It actually works!
SUPERCILEX Mar 16, 2018
057c561
Merge remote-tracking branch 'upstream/version-3.3.0-dev' into providers
SUPERCILEX Mar 16, 2018
7736254
Thank you TDD!
SUPERCILEX Mar 16, 2018
4c2eec6
Start addressing review feedback
SUPERCILEX Mar 16, 2018
705b9b9
Restore previous linking behavior
SUPERCILEX Mar 16, 2018
e17c101
Fix bugs found in real-world testing plus cleanup
SUPERCILEX Mar 16, 2018
27ff4d3
Add hack for https://github.com/googlesamples/google-services/issues/345
SUPERCILEX Mar 16, 2018
8732c11
Fix WelcomeBackIdpPrompt recursively attempting sign-ins with the sam…
SUPERCILEX Mar 16, 2018
95b024c
Tidy loose ends
SUPERCILEX Mar 17, 2018
e233c82
Merge remote-tracking branch 'upstream/version-3.3.0-dev' into providers
SUPERCILEX Mar 19, 2018
3b901c5
Address review feedback
SUPERCILEX Mar 20, 2018
e79388f
Make Twitter initialization prettier
SUPERCILEX Mar 20, 2018
82d6af6
Rename ProvidersHandlerBase
SUPERCILEX Mar 20, 2018
ff61518
Slightly cleaner user cancellation error message no-op
SUPERCILEX Mar 20, 2018
b1f2356
Merge remote-tracking branch 'upstream/version-3.3.0-dev' into providers
SUPERCILEX Mar 21, 2018
6f5207a
Better optional provider support
SUPERCILEX Mar 21, 2018
bbc00f0
Implement new resource usability logic
SUPERCILEX Mar 22, 2018
e8214a4
Address some more feedback
SUPERCILEX Mar 22, 2018
6b8eb7f
Add back unused tools declaration in strings
SUPERCILEX Mar 22, 2018
43a1d31
Simplify provider dependency tree
SUPERCILEX Mar 22, 2018
748cbc5
Forgot to make all exceptions be usable
SUPERCILEX Mar 22, 2018
2fad171
Minor cleanup
SUPERCILEX Mar 22, 2018
e679f80
Address comments
SUPERCILEX Mar 22, 2018
6fec29c
Add back strings and suppress lint
SUPERCILEX Mar 22, 2018
f4ec251
More simplification
SUPERCILEX Mar 22, 2018
e947030
Squishify the provider classes together
SUPERCILEX Mar 22, 2018
36a41f2
Fix welcome back crash if response failed
SUPERCILEX Mar 22, 2018
722f9c6
Consistency
SUPERCILEX Mar 22, 2018
d111daa
Fix pointless zygote logging
SUPERCILEX Mar 22, 2018
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
2 changes: 1 addition & 1 deletion app/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ android {
dependencies {
implementation "com.google.firebase:firebase-core:$firebaseVersion"
implementation "com.android.support:design:$supportLibraryVersion"
implementation 'com.android.support:multidex:1.0.2'
implementation 'com.android.support:multidex:1.0.3'

implementation project(path: ':auth')
implementation project(path: ':firestore')
Expand Down
12 changes: 10 additions & 2 deletions auth/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,17 @@
android:exported="false"
android:theme="@style/FirebaseUI.Transparent" />

<activity
android:name=".ui.idp.SingleSignInActivity"
android:label=""
android:exported="false"
android:theme="@style/FirebaseUI.Transparent" />

<activity
android:name=".ui.email.RecoverPasswordActivity"
android:label="@string/fui_title_recover_password_activity"
android:exported="false" />
android:exported="false"
android:windowSoftInputMode="adjustResize" />

<activity
android:name=".ui.email.EmailActivity"
Expand All @@ -43,7 +50,8 @@
<activity
android:name=".ui.idp.WelcomeBackIdpPrompt"
android:label="@string/fui_title_welcome_back_idp_prompt"
android:exported="false" />
android:exported="false"
android:windowSoftInputMode="adjustResize" />

<activity
android:name=".ui.email.WelcomeBackPasswordPrompt"
Expand Down
38 changes: 10 additions & 28 deletions auth/src/main/java/com/firebase/ui/auth/AuthUI.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@

import com.facebook.login.LoginManager;
import com.firebase.ui.auth.data.model.FlowParameters;
import com.firebase.ui.auth.provider.TwitterProvider;
import com.firebase.ui.auth.data.remote.FacebookSignInHandler;
import com.firebase.ui.auth.data.remote.TwitterSignInHandler;
import com.firebase.ui.auth.ui.idp.AuthMethodPickerActivity;
import com.firebase.ui.auth.util.ExtraConstants;
import com.firebase.ui.auth.util.GoogleApiUtils;
Expand Down Expand Up @@ -59,6 +60,7 @@
import com.google.firebase.auth.PhoneAuthProvider;
import com.google.firebase.auth.TwitterAuthProvider;
import com.google.firebase.auth.UserInfo;
import com.twitter.sdk.android.core.TwitterCore;

import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
Expand Down Expand Up @@ -177,10 +179,10 @@ public class AuthUI {
*/
public static final Set<String> SUPPORTED_PROVIDERS =
Collections.unmodifiableSet(new HashSet<>(Arrays.asList(
EmailAuthProvider.PROVIDER_ID,
GoogleAuthProvider.PROVIDER_ID,
FacebookAuthProvider.PROVIDER_ID,
TwitterAuthProvider.PROVIDER_ID,
EmailAuthProvider.PROVIDER_ID,
PhoneAuthProvider.PROVIDER_ID
)));

Expand Down Expand Up @@ -276,7 +278,7 @@ public Task<Void> signOut(@NonNull Context context) {
.disableAutoSignIn()
.continueWithTask(new Continuation<Void, Task<Void>>() {
@Override
public Task<Void> then(@NonNull Task<Void> task) throws Exception {
public Task<Void> then(@NonNull Task<Void> task) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did some linter tell you not to have throws Exception here? Does it actually change anything to remove it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, IntelliJ has been removing these, not sure why, but it doesn't seem to matter.

// We want to ignore a specific exception, since it's not a good reason
// to fail (see Issue 1156).
if (!task.isSuccessful() && (task.getException() instanceof ApiException)) {
Expand Down Expand Up @@ -357,22 +359,12 @@ public Task<Void> then(@NonNull Task<Void> task) {
}

private Task<Void> signOutIdps(@NonNull Context context) {
try {
if (FacebookSignInHandler.IS_AVAILABLE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is much less scary

LoginManager.getInstance().logOut();
} catch (NoClassDefFoundError e) {
// Do nothing: this is perfectly fine if the dev doesn't include Facebook/Twitter
// support
}

try {
TwitterProvider.signOut(context);
} catch (NoClassDefFoundError e) {
// See comment above
// Note: we need to have separate try/catch statements since devs can include
// _either_ one of the providers. If one crashes, we still need to sign out of
// the other one.
if (TwitterSignInHandler.IS_AVAILABLE) {
TwitterCore.getInstance().getSessionManager().clearActiveSession();
}

return GoogleSignIn.getClient(context, GoogleSignInOptions.DEFAULT_SIGN_IN).signOut();
}

Expand Down Expand Up @@ -787,17 +779,12 @@ public static final class FacebookBuilder extends Builder {
public FacebookBuilder() {
//noinspection deprecation taking a hit for the backcompat team
super(FacebookAuthProvider.PROVIDER_ID);

try {
//noinspection unused to possibly throw
Class c = com.facebook.FacebookSdk.class;
} catch (NoClassDefFoundError e) {
if (!FacebookSignInHandler.IS_AVAILABLE) {
throw new RuntimeException(
"Facebook provider cannot be configured " +
"without dependency. Did you forget to add " +
"'com.facebook.android:facebook-login:VERSION' dependency?");
}

Preconditions.checkConfigured(getApplicationContext(),
"Facebook provider unconfigured. Make sure to add a" +
" `facebook_application_id` string. See the docs for more info:" +
Expand Down Expand Up @@ -830,17 +817,12 @@ public static final class TwitterBuilder extends Builder {
public TwitterBuilder() {
//noinspection deprecation taking a hit for the backcompat team
super(TwitterAuthProvider.PROVIDER_ID);

try {
//noinspection unused to possibly throw
Class c = com.twitter.sdk.android.core.TwitterCore.class;
} catch (NoClassDefFoundError e) {
if (!TwitterSignInHandler.IS_AVAILABLE) {
throw new RuntimeException(
"Twitter provider cannot be configured " +
"without dependency. Did you forget to add " +
"'com.twitter.sdk.android:twitter-core:VERSION' dependency?");
}

Preconditions.checkConfigured(getApplicationContext(),
"Twitter provider unconfigured. Make sure to add your key and secret." +
" See the docs for more info:" +
Expand Down
8 changes: 7 additions & 1 deletion auth/src/main/java/com/firebase/ui/auth/ErrorCodes.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ public final class ErrorCodes {
UNKNOWN_ERROR,
NO_NETWORK,
PLAY_SERVICES_UPDATE_CANCELLED,
DEVELOPER_ERROR
DEVELOPER_ERROR,
PROVIDER_ERROR
})
@Retention(RetentionPolicy.SOURCE)
public @interface Code {}
Expand All @@ -41,6 +42,11 @@ public final class ErrorCodes {
*/
public static final int DEVELOPER_ERROR = 3;

/**
* An external sign-in provider error occurred.
*/
public static final int PROVIDER_ERROR = 4;

private ErrorCodes() {
throw new AssertionError("No instance for you!");
}
Expand Down
20 changes: 17 additions & 3 deletions auth/src/main/java/com/firebase/ui/auth/IdpResponse.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import android.support.annotation.RestrictTo;
import android.text.TextUtils;

import com.firebase.ui.auth.data.model.Resource;
import com.firebase.ui.auth.data.model.User;
import com.firebase.ui.auth.util.ExtraConstants;
import com.google.firebase.auth.GoogleAuthProvider;
Expand Down Expand Up @@ -97,20 +98,33 @@ public static IdpResponse fromResultIntent(@Nullable Intent resultIntent) {
}
}

@NonNull
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public static Intent getErrorIntent(@NonNull Exception e) {
return fromError(e).toIntent();
public static IdpResponse from(@NonNull Resource<IdpResponse> resource) {
IdpResponse response = resource.getValue();
if (resource.getException() != null) {
response = from(resource.getException());
}
return response;
}

@NonNull
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public static IdpResponse fromError(@NonNull Exception e) {
public static IdpResponse from(@NonNull Exception e) {
if (e instanceof FirebaseUiException) {
return new IdpResponse((FirebaseUiException) e);
} else {
return new IdpResponse(new FirebaseUiException(ErrorCodes.UNKNOWN_ERROR, e));
}
}

@NonNull
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public static Intent getErrorIntent(@NonNull Exception e) {
return from(e).toIntent();
}

@NonNull
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public Intent toIntent() {
return new Intent().putExtra(ExtraConstants.EXTRA_IDP_RESPONSE, this);
Expand Down
6 changes: 3 additions & 3 deletions auth/src/main/java/com/firebase/ui/auth/KickoffActivity.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@
import com.firebase.ui.auth.util.ExtraConstants;
import com.firebase.ui.auth.util.PlayServicesHelper;
import com.firebase.ui.auth.util.signincontainer.SignInDelegate;
import com.firebase.ui.auth.viewmodel.RequestCodes;

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public class KickoffActivity extends HelperActivityBase {
private static final String TAG = "KickoffActivity";
private static final String IS_WAITING_FOR_PLAY_SERVICES = "is_waiting_for_play_services";
private static final int RC_PLAY_SERVICES = 1;

private boolean mIsWaitingForPlayServices = false;

Expand All @@ -40,7 +40,7 @@ protected void onCreate(Bundle savedInstance) {

boolean isPlayServicesAvailable = PlayServicesHelper.makePlayServicesAvailable(
this,
RC_PLAY_SERVICES,
RequestCodes.PLAY_SERVICES_CHECK,
new DialogInterface.OnCancelListener() {
@Override
public void onCancel(DialogInterface dialog) {
Expand Down Expand Up @@ -68,7 +68,7 @@ public void onSaveInstanceState(Bundle outState) {
@Override
protected void onActivityResult(int requestCode, int resultCode, Intent data) {
super.onActivityResult(requestCode, resultCode, data);
if (requestCode == RC_PLAY_SERVICES) {
if (requestCode == RequestCodes.PLAY_SERVICES_CHECK) {
if (resultCode == RESULT_OK) {
start();
} else {
Expand Down
40 changes: 26 additions & 14 deletions auth/src/main/java/com/firebase/ui/auth/data/model/Resource.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,15 @@
*/
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public final class Resource<T> {
private State mState;
private final boolean mUsable;

private final State mState;
private final T mValue;
private final Exception mException;

private boolean mUsed;

private Resource(State state, T value, Exception exception) {
private Resource(State state, boolean usable, T value, Exception exception) {
mState = state;
mUsable = usable;
mValue = value;
mException = exception;
}
Expand All @@ -30,31 +30,47 @@ private Resource(State state, T value, Exception exception) {
*/
@NonNull
public static Resource<Void> forVoidSuccess() {
return new Resource<>(State.SUCCESS, null, null);
return new Resource<>(State.SUCCESS, false, null, null);
}

/**
* Creates a successful resource containing a value.
*/
@NonNull
public static <T> Resource<T> forSuccess(@NonNull T value) {
return new Resource<>(State.SUCCESS, value, null);
return new Resource<>(State.SUCCESS, false, value, null);
}

/**
* Similar to {@link #forSuccess(Object)}, but this resource can be used up.
*/
@NonNull
public static <T> Resource<T> forUsableSuccess(@NonNull T value) {
return new Resource<>(State.SUCCESS, true, value, null);
}

/**
* Creates a failed resource with an exception.
*/
@NonNull
public static <T> Resource<T> forFailure(@NonNull Exception e) {
return new Resource<>(State.FAILURE, null, e);
return new Resource<>(State.FAILURE, false, null, e);
}

/**
* Similar to {@link #forFailure(Exception)}, but this resource can be used up.
*/
@NonNull
public static <T> Resource<T> forUsableFailure(@NonNull Exception e) {
return new Resource<>(State.FAILURE, true, null, e);
}

/**
* Creates a resource in the loading state, without a value or an exception.
*/
@NonNull
public static <T> Resource<T> forLoading() {
return new Resource<>(State.LOADING, null, null);
return new Resource<>(State.LOADING, false, null, null);
}

@NonNull
Expand All @@ -64,20 +80,16 @@ public State getState() {

@Nullable
public final Exception getException() {
mUsed = true;
if (mUsable) { mState = State.USED; }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is sort of interesting. Previously, a Resource was immutable. You'd listen to a stream and get a new resource every time the state changed. Now the state can change from the inside.

How does this affect the pattern of listening to LiveData<Resource> and checking state? Is it a problem that Observers won't get a new callback when this happens?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, technically, the "resource" part is still immutable since the value and exception can't change. Also, the class is final so only we control the state's mutability, and for now, it can only go from SUCCESS/FAILURE -> USED so I don't think it'll be a problem unless we want to listen for USED events? I'm not sure when that would ever be applicable... I think of the state as more for helping us define our conditionals rather than defining the resource—it should be focused on the data, not the state. That's my opinion though, any digressions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems OK for now ... just something we may have to change in the future.

return mException;
}

@Nullable
public T getValue() {
mUsed = true;
if (mUsable) { mState = State.USED; }
return mValue;
}

public boolean isUsed() {
return mUsed;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public enum State {
SUCCESS, FAILURE, LOADING
SUCCESS, FAILURE, LOADING, USED
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package com.firebase.ui.auth.data.model;

import android.support.annotation.RestrictTo;

import com.firebase.ui.auth.ErrorCodes;
import com.firebase.ui.auth.FirebaseUiException;

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public class UserCancellationException extends FirebaseUiException {
public UserCancellationException() {
super(ErrorCodes.UNKNOWN_ERROR);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package com.firebase.ui.auth.data.remote;

import android.app.Application;
import android.content.Intent;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import android.support.annotation.RestrictTo;

import com.firebase.ui.auth.IdpResponse;
import com.firebase.ui.auth.data.model.Resource;
import com.firebase.ui.auth.data.model.UserCancellationException;
import com.firebase.ui.auth.ui.HelperActivityBase;
import com.firebase.ui.auth.ui.email.EmailActivity;
import com.firebase.ui.auth.viewmodel.RequestCodes;
import com.firebase.ui.auth.viewmodel.idp.ProviderSignInBase;

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public class EmailSignInHandler extends ProviderSignInBase<Void> {
public EmailSignInHandler(Application application) {
super(application);
}

@Override
public void startSignIn(@NonNull HelperActivityBase activity) {
activity.startActivityForResult(
EmailActivity.createIntent(activity, activity.getFlowParams()),
RequestCodes.EMAIL_FLOW);
}

@Override
public void onActivityResult(int requestCode, int resultCode, @Nullable Intent data) {
if (requestCode == RequestCodes.EMAIL_FLOW) {
IdpResponse response = IdpResponse.fromResultIntent(data);
if (response == null) {
setResult(Resource.<IdpResponse>forFailure(new UserCancellationException()));
} else {
setResult(Resource.forSuccess(response));
}
}
}
}
Loading