Skip to content

Give clients much more detailed errors #1168

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 3 commits into from
Feb 26, 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
14 changes: 6 additions & 8 deletions app/src/main/java/com/firebase/uidemo/auth/AuthUiActivity.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import android.support.design.widget.Snackbar;
import android.support.v7.app.AppCompatActivity;
import android.support.v7.app.AppCompatDelegate;
import android.util.Log;
import android.view.View;
import android.widget.Button;
import android.widget.CheckBox;
Expand All @@ -49,6 +50,8 @@
import butterknife.OnClick;

public class AuthUiActivity extends AppCompatActivity {
private static final String TAG = "AuthUiActivity";

private static final String GOOGLE_TOS_URL = "https://www.google.com/policies/terms/";
private static final String FIREBASE_TOS_URL = "https://firebase.google.com/terms/";
private static final String GOOGLE_PRIVACY_POLICY_URL = "https://www.google.com/policies/privacy/";
Expand Down Expand Up @@ -239,7 +242,6 @@ private void handleSignInResponse(int resultCode, Intent data) {
if (resultCode == RESULT_OK) {
startSignedInActivity(response);
finish();
return;
} else {
// Sign in failed
if (response == null) {
Expand All @@ -248,18 +250,14 @@ private void handleSignInResponse(int resultCode, Intent data) {
return;
}

if (response.getErrorCode() == ErrorCodes.NO_NETWORK) {
if (response.getError().getErrorCode() == ErrorCodes.NO_NETWORK) {
showSnackbar(R.string.no_internet_connection);
return;
}

if (response.getErrorCode() == ErrorCodes.UNKNOWN_ERROR) {
showSnackbar(R.string.unknown_error);
return;
}
showSnackbar(R.string.unknown_error);
Log.e(TAG, "Sign-in error: ", response.getError());
}

showSnackbar(R.string.unknown_sign_in_response);
}

private void startSignedInActivity(IdpResponse response) {
Expand Down
1 change: 0 additions & 1 deletion app/src/main/res/values/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@
<string name="require_name">Require first/last name with email accounts.</string>

<string name="unknown_response">Unexpected onActivityResult response code</string>
<string name="unknown_sign_in_response">Unknown response from AuthUI sign-in</string>
<string name="sign_in_cancelled">Sign in cancelled</string>
<string name="no_internet_connection">No internet connection</string>
<string name="unknown_error">An unknown error occurred</string>
Expand Down
43 changes: 35 additions & 8 deletions auth/src/main/java/com/firebase/ui/auth/ErrorCodes.java
Original file line number Diff line number Diff line change
@@ -1,20 +1,47 @@
package com.firebase.ui.auth;

import android.support.annotation.IntDef;

import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;

/**
* Error codes retrieved from {@link IdpResponse#getErrorCode()}.
* Error codes for failed sign-in attempts.
*/
public final class ErrorCodes {
/**
* Sign in failed due to lack of network connection
**/
public static final int NO_NETWORK = 10;
* Valid codes that can be returned from {@link FirebaseUiException#getErrorCode()}.
*/
@IntDef({
UNKNOWN_ERROR,
NO_NETWORK,
PLAY_SERVICES_UPDATE_CANCELLED,
DEVELOPER_ERROR
})
@Retention(RetentionPolicy.SOURCE)
public @interface Code {}

/**
* An unknown error has occurred.
*/
public static final int UNKNOWN_ERROR = 0;

/**
* Sign in failed due to lack of network connection.
*/
public static final int NO_NETWORK = 1;

/**
* A required update to Play Services was cancelled by the user.
*/
public static final int PLAY_SERVICES_UPDATE_CANCELLED = 2;

/**
* An unknown error has occurred
**/
public static final int UNKNOWN_ERROR = 20;
* A sign-in operation couldn't be completed due to a developer error.
*/
public static final int DEVELOPER_ERROR = 3;

private ErrorCodes() {
// no instance
throw new AssertionError("No instance for you!");
}
}
45 changes: 45 additions & 0 deletions auth/src/main/java/com/firebase/ui/auth/FirebaseUiException.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package com.firebase.ui.auth;

import android.support.annotation.NonNull;
import android.support.annotation.RestrictTo;

/**
* Base class for all FirebaseUI exceptions.
*/
public class FirebaseUiException extends Exception {
private final int mErrorCode;

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public FirebaseUiException(@ErrorCodes.Code int code) {
mErrorCode = code;
}

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public FirebaseUiException(@ErrorCodes.Code int code, @NonNull String message) {
super(message);
mErrorCode = code;
}

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public FirebaseUiException(@ErrorCodes.Code int code,
@NonNull String message,
@NonNull Throwable cause) {
super(message, cause);
mErrorCode = code;
}

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public FirebaseUiException(@ErrorCodes.Code int code, @NonNull Throwable cause) {
super(cause);
mErrorCode = code;
}

/**
* @return error code associated with this exception
* @see com.firebase.ui.auth.ErrorCodes
*/
@ErrorCodes.Code
public final int getErrorCode() {
return mErrorCode;
}
}
26 changes: 15 additions & 11 deletions auth/src/main/java/com/firebase/ui/auth/IdpResponse.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import android.support.annotation.RestrictTo;
import android.text.TextUtils;

import com.firebase.ui.auth.data.model.FirebaseUiException;
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 @@ -78,17 +77,17 @@ public static IdpResponse fromResultIntent(@Nullable Intent resultIntent) {
}

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public static IdpResponse fromError(@NonNull FirebaseUiException e) {
return new IdpResponse(e);
public static Intent getErrorIntent(@NonNull Exception e) {
return fromError(e).toIntent();
}

/**
* @deprecated migrate internals to {@link #fromError(FirebaseUiException)}
*/
@Deprecated
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public static Intent getErrorCodeIntent(int errorCode) {
return new IdpResponse(new FirebaseUiException(errorCode)).toIntent();
public static IdpResponse fromError(@NonNull Exception e) {
if (e instanceof FirebaseUiException) {
return new IdpResponse((FirebaseUiException) e);
} else {
return new IdpResponse(new FirebaseUiException(ErrorCodes.UNKNOWN_ERROR, e));
}
}

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
Expand Down Expand Up @@ -149,7 +148,10 @@ public String getIdpSecret() {

/**
* Get the error code for a failed sign in
*
* @deprecated use {@link #getError()} instead
*/
@Deprecated
public int getErrorCode() {
if (isSuccessful()) {
return Activity.RESULT_OK;
Expand All @@ -158,9 +160,11 @@ public int getErrorCode() {
}
}

/**
* Get the error for a failed sign in.
*/
@Nullable
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public FirebaseUiException getException() {
public FirebaseUiException getError() {
return mException;
}

Expand Down
13 changes: 6 additions & 7 deletions auth/src/main/java/com/firebase/ui/auth/KickoffActivity.java
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ protected void onCreate(Bundle savedInstance) {
if (savedInstance == null || savedInstance.getBoolean(IS_WAITING_FOR_PLAY_SERVICES)) {
if (isOffline()) {
Log.d(TAG, "No network connection");
finish(RESULT_CANCELED,
IdpResponse.getErrorCodeIntent(ErrorCodes.NO_NETWORK));
finish(RESULT_CANCELED, IdpResponse.getErrorIntent(
new FirebaseUiException(ErrorCodes.NO_NETWORK)));
return;
}

Expand All @@ -44,9 +44,8 @@ protected void onCreate(Bundle savedInstance) {
new DialogInterface.OnCancelListener() {
@Override
public void onCancel(DialogInterface dialog) {
finish(RESULT_CANCELED,
IdpResponse.getErrorCodeIntent(
ErrorCodes.UNKNOWN_ERROR));
finish(RESULT_CANCELED, IdpResponse.getErrorIntent(
new FirebaseUiException(ErrorCodes.PLAY_SERVICES_UPDATE_CANCELLED)));
}
});

Expand All @@ -73,8 +72,8 @@ protected void onActivityResult(int requestCode, int resultCode, Intent data) {
if (resultCode == RESULT_OK) {
start();
} else {
finish(RESULT_CANCELED,
IdpResponse.getErrorCodeIntent(ErrorCodes.UNKNOWN_ERROR));
finish(RESULT_CANCELED, IdpResponse.getErrorIntent(
new FirebaseUiException(ErrorCodes.PLAY_SERVICES_UPDATE_CANCELLED)));
}
} else {
SignInDelegate delegate = SignInDelegate.getInstance(this);
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
import com.facebook.login.LoginManager;
import com.facebook.login.LoginResult;
import com.firebase.ui.auth.AuthUI;
import com.firebase.ui.auth.ErrorCodes;
import com.firebase.ui.auth.FirebaseUiException;
import com.firebase.ui.auth.IdpResponse;
import com.firebase.ui.auth.R;
import com.firebase.ui.auth.data.model.User;
Expand Down Expand Up @@ -130,12 +132,12 @@ public void onCompleted(JSONObject object, GraphResponse response) {
FacebookRequestError requestError = response.getError();
if (requestError != null) {
Log.e(TAG, "Received Facebook error: " + requestError.getErrorMessage());
onFailure();
onFailure(requestError.getException());
return;
}
if (object == null) {
Log.w(TAG, "Received null response from Facebook GraphRequest");
onFailure();
onFailure(new FirebaseUiException(ErrorCodes.UNKNOWN_ERROR));
} else {
String email = null;
String name = null;
Expand Down Expand Up @@ -168,13 +170,13 @@ public void onCompleted(JSONObject object, GraphResponse response) {

@Override
public void onCancel() {
onFailure();
onFailure(new FirebaseUiException(ErrorCodes.UNKNOWN_ERROR));
}

@Override
public void onError(FacebookException error) {
Log.e(TAG, "Error logging in with Facebook. " + error.getMessage());
onFailure();
public void onError(FacebookException e) {
Log.e(TAG, "Error logging in with Facebook. " + e.getMessage());
onFailure(e);
}

private void onSuccess(LoginResult loginResult,
Expand All @@ -191,9 +193,9 @@ private void onSuccess(LoginResult loginResult,
.build());
}

private void onFailure() {
private void onFailure(Exception e) {
gcCallbackManager();
mCallbackObject.onFailure();
mCallbackObject.onFailure(e);
}

private void gcCallbackManager() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import android.widget.Toast;

import com.firebase.ui.auth.AuthUI.IdpConfig;
import com.firebase.ui.auth.ErrorCodes;
import com.firebase.ui.auth.FirebaseUiException;
import com.firebase.ui.auth.IdpResponse;
import com.firebase.ui.auth.R;
import com.firebase.ui.auth.data.model.User;
Expand Down Expand Up @@ -152,7 +154,7 @@ private void onError(GoogleSignInResult result) {

private void onError(String errorMessage) {
Log.e(TAG, "Error logging in with Google. " + errorMessage);
mIdpCallback.onFailure();
mIdpCallback.onFailure(new FirebaseUiException(ErrorCodes.UNKNOWN_ERROR, errorMessage));
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,17 @@

package com.firebase.ui.auth.provider;

import android.support.annotation.NonNull;
import android.support.annotation.RestrictTo;

import com.firebase.ui.auth.IdpResponse;

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public interface IdpProvider extends Provider {
interface IdpCallback {
void onSuccess(IdpResponse idpResponse);
void onSuccess(@NonNull IdpResponse idpResponse);

void onFailure();
void onFailure(@NonNull Exception e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that anyone was relying on this interface? If so this is a breaking change .... if this is meant to be internal let's annotate 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.

I don't think so, or at least, I know we've broken it before without notice. I'll annotate it.

}

void setAuthenticationCallback(IdpCallback callback);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
import android.content.Context;
import android.content.Intent;
import android.support.annotation.LayoutRes;
import android.support.annotation.RestrictTo;

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public interface Provider {
/** Retrieves the name of the IDP, for display on-screen. */
String getName(Context context);
Expand Down
Loading