Skip to content

Differentiate between user cancellation and actual errors #426

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 22 commits into from
Dec 6, 2016
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
37 changes: 24 additions & 13 deletions app/src/main/java/com/firebase/uidemo/auth/AuthUiActivity.java
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@

import com.firebase.ui.auth.AuthUI;
import com.firebase.ui.auth.AuthUI.IdpConfig;
import com.firebase.ui.auth.ErrorCodes;
import com.firebase.ui.auth.IdpResponse;
import com.firebase.ui.auth.ui.ResultCodes;
import com.firebase.ui.auth.ResultCodes;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The library version should probably be bumped to 1.1.0 since this is slight breakage.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would technically require a major version (if you're going by semver) since old code would break when updated.

Could we keep the .ui.ResultCodes class but have it be all @Deprecated constants that are just symlinks to the new class? Then it's fully backwards compatible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Awesome idea!

PS: how does deprecation work? When the library hits 2.0, will we remove the deprecated stuff or are we stuck with it for life?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove deprecated things after a major version change (if we want to). Or we can leave it for life. This will be the first example of deprecation in this library so there's no precedent for us, and every library does it a bit differently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, thanks for the info! (I'm pretty sure we also deprecated setProviders(String... providers))

import com.firebase.uidemo.R;
import com.google.android.gms.common.Scopes;
import com.google.firebase.auth.FirebaseAuth;
Expand Down Expand Up @@ -199,20 +200,30 @@ protected void onActivityResult(int requestCode, int resultCode, Intent data) {

@MainThread
private void handleSignInResponse(int resultCode, Intent data) {
if (resultCode == RESULT_OK) {
startActivity(SignedInActivity.createIntent(this, IdpResponse.fromResultIntent(data)));
finish();
return;
}

if (resultCode == RESULT_CANCELED) {
showSnackbar(R.string.sign_in_cancelled);
return;
}
IdpResponse response = IdpResponse.fromResultIntent(data);

if (resultCode == ResultCodes.RESULT_NO_NETWORK) {
showSnackbar(R.string.no_internet_connection);
// Successfully signed in
if (resultCode == ResultCodes.OK) {
startActivity(SignedInActivity.createIntent(this, response));
finish();
return;
} else {
// Sign in failed
if (response == null) {
// User pressed back button
showSnackbar(R.string.sign_in_cancelled);
return;
}

if (response.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_sign_in_response);
Expand Down
11 changes: 7 additions & 4 deletions app/src/main/java/com/firebase/uidemo/auth/SignedInActivity.java
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ public class SignedInActivity extends AppCompatActivity {
@BindView(R.id.user_enabled_providers)
TextView mEnabledProviders;

private IdpResponse mIdpResponse;

@Override
public void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
Expand All @@ -76,6 +78,8 @@ public void onCreate(Bundle savedInstanceState) {
return;
}

mIdpResponse = getIntent().getParcelableExtra(EXTRA_IDP_RESPONSE);

setContentView(R.layout.signed_in_layout);
ButterKnife.bind(this);
populateProfile();
Expand Down Expand Up @@ -177,10 +181,9 @@ private void populateProfile() {
}

private void populateIdpToken() {
IdpResponse idpResponse = getIntent().getParcelableExtra(EXTRA_IDP_RESPONSE);
if (idpResponse != null) {
String token = idpResponse.getIdpToken();
String secret = idpResponse.getIdpSecret();
if (mIdpResponse != null) {
String token = mIdpResponse.getIdpToken();
String secret = mIdpResponse.getIdpSecret();
if (token == null) {
findViewById(R.id.idp_token_layout).setVisibility(View.GONE);
} else {
Expand Down
1 change: 1 addition & 0 deletions app/src/main/res/values/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
<string name="photos">Photos</string>
<string name="idp_token">IDP Token</string>
<string name="idp_secret">IDP Secret</string>
<string name="unknown_error">An unknown error occurred</string>
<string name="send">Send</string>
<string name="download">Download</string>
<string name="upload">Upload</string>
Expand Down
55 changes: 30 additions & 25 deletions auth/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -216,42 +216,47 @@ startActivityForResult(
#### Handling the sign-in response

#####Response codes
The authentication flow only provides three response codes:
`Activity.RESULT_OK` if a user is signed in, `Activity.RESULT_CANCELLED` if
sign in failed, and `ResultCodes.RESULT_NO_NETWORK` if sign in failed due to a lack of network connectivity.
No further information on failure is provided as it is not
typically useful; the only recourse for most apps if sign in fails is to ask
the user to sign in again later, or proceed with anonymous sign-in if
supported.
The authentication flow provides several response codes of which the most common are as follows:
`ResultCodes.OK` if a user is signed in, `ResultCodes.CANCELLED` if the user manually canceled the sign in,
`ResultCodes.NO_NETWORK` if sign in failed due to a lack of network connectivity,
and `ResultCodes.UNKNOWN_ERROR` for all other errors.
Typically, the only recourse for most apps if sign in fails is to ask
the user to sign in again later, or proceed with anonymous sign-in if supported.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's the updated README


```java
protected void onActivityResult(int requestCode, int resultCode, Intent data) {
super.onActivityResult(requestCode, resultCode, data);
// RC_SIGN_IN is the request code you passed into startActivityForResult(...) when starting the sign in flow
// RC_SIGN_IN is the request code you passed into startActivityForResult(...) when starting the sign in flow.
if (requestCode == RC_SIGN_IN) {
if (resultCode == RESULT_OK) {
// user is signed in!
startActivity(new Intent(this, WelcomeBackActivity.class));
IdpResponse response = IdpResponse.fromResultIntent(data);

// Successfully signed in
if (resultCode == ResultCodes.OK) {
startActivity(SignedInActivity.createIntent(this, response));
finish();
return;
}
} else {
// Sign in failed
if (response == null) {
// User pressed back button
showSnackbar(R.string.sign_in_cancelled);
return;
}

// Sign in canceled
if (resultCode == RESULT_CANCELED) {
showSnackbar(R.string.sign_in_cancelled);
return;
}
if (response.getErrorCode() == ErrorCodes.NO_NETWORK) {
showSnackbar(R.string.no_internet_connection);
return;
}

// No network
if (resultCode == ResultCodes.RESULT_NO_NETWORK) {
showSnackbar(R.string.no_internet_connection);
return;
if (response.getErrorCode() == ErrorCodes.UNKNOWN_ERROR) {
showSnackbar(R.string.unknown_error);
return;
}
}

// User is not signed in. Maybe just wait for the user to press
// "sign in" again, or show a message.
showSnackbar(R.string.unknown_sign_in_response);
}
}
}
```

Alternatively, you can register a listener for authentication state changes;
Expand All @@ -266,7 +271,7 @@ Intent.
```java
protected void onActivityResult(int requestCode, int resultCode, Intent data) {
super.onActivityResult(requestCode, resultCode, data);
if (resultCode == RESULT_OK) {
if (resultCode == ResultCodes.OK) {
IdpResponse idpResponse = IdpResponse.fromResultIntent(data);
startActivity(new Intent(this, WelcomeBackActivity.class)
.putExtra("my_token", idpResponse.getIdpToken()));
Expand Down
7 changes: 4 additions & 3 deletions auth/src/main/java/com/firebase/ui/auth/AuthUI.java
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,9 @@
*
* <h3>Handling the Sign-in response</h3>
*
* The authentication flow provides only two response codes: {@link Activity#RESULT_OK RESULT_OK}
* if a user is signed in, and {@link Activity#RESULT_CANCELED RESULT_CANCELLED} if sign in
* The authentication flow provides only two response codes:
* {@link ResultCodes#OK RESULT_OK} if a user is signed in,
* and {@link ResultCodes#CANCELED RESULT_CANCELLED} if sign in
* failed. No further information on failure is provided as it is not typically useful; the only
* recourse for most apps if sign in fails is to ask the user to sign in again later, or proceed
* with an anonymous account if supported.
Expand All @@ -149,7 +150,7 @@
* protected void onActivityResult(int requestCode, int resultCode, Intent data) {
* super.onActivityResult(requestCode, resultCode, data);
* if (requestCode == RC_SIGN_IN) {
* if (resultCode == RESULT_OK) {
* if (resultCode == ResultCodes.OK) {
* // user is signed in!
* startActivity(new Intent(this, WelcomeBackActivity.class));
* finish();
Expand Down
20 changes: 20 additions & 0 deletions auth/src/main/java/com/firebase/ui/auth/ErrorCodes.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package com.firebase.ui.auth;

/**
* Error codes retrieved from {@link IdpResponse#getErrorCode()}.
*/
public final class ErrorCodes {
private ErrorCodes() {
// We don't want people to initialize this class
}

/**
* Sign in failed due to lack of network connection
**/
public static final int NO_NETWORK = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this one actually a result code and not an error? We still need to return this via onActivityResult in order to honor our previous API contract.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, I don't think so. It should be an error code, but to keep backwards compatibility I'm returning it in SignInDelegate which the old ResultCodes points to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok i see what you did here. So you're returning it in both places for SignInDelegate, with the recommendation being that from here on out people use the errorCode to make decisions when the result is not OK. That makes sense!


/**
* An unknown error has occurred
**/
public static final int UNKNOWN_ERROR = 20;
}
49 changes: 43 additions & 6 deletions auth/src/main/java/com/firebase/ui/auth/IdpResponse.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,21 @@
public class IdpResponse implements Parcelable {

private final String mProviderId;
@Nullable private final String mEmail;
@Nullable
private final String mEmail;
private final String mToken;
private final String mSecret;
private final int mErrorCode;

public IdpResponse(int errorCode) {
this(null, null, null, null, errorCode);
}

public IdpResponse(String providerId, @Nullable String email) {
public IdpResponse(String providerId, String email) {
this(providerId, email, null, null);
}

public IdpResponse(
String providerId, @Nullable String email, @Nullable String token) {
public IdpResponse(String providerId, @Nullable String email, @Nullable String token) {
this(providerId, email, token, null);
}

Expand All @@ -45,10 +50,20 @@ public IdpResponse(
@Nullable String email,
@Nullable String token,
@Nullable String secret) {
this(providerId, email, token, secret, ResultCodes.OK);
}

public IdpResponse(
String providerId,
@Nullable String email,
@Nullable String token,
@Nullable String secret,
int errorCode) {
mProviderId = providerId;
mEmail = email;
mToken = token;
mSecret = secret;
mErrorCode = errorCode;
}

public static final Creator<IdpResponse> CREATOR = new Creator<IdpResponse>() {
Expand All @@ -58,7 +73,8 @@ public IdpResponse createFromParcel(Parcel in) {
in.readString(),
in.readString(),
in.readString(),
in.readString()
in.readString(),
in.readInt()
);
}

Expand All @@ -71,6 +87,7 @@ public IdpResponse[] newArray(int size) {
/**
* Get the type of provider. e.g. {@link AuthUI#GOOGLE_PROVIDER}
*/
@Nullable
public String getProviderType() {
return mProviderId;
}
Expand Down Expand Up @@ -99,6 +116,13 @@ public String getEmail() {
return mEmail;
}

/**
* Get the error code for a failed sign in
*/
public int getErrorCode() {
return mErrorCode;
}

@Override
public int describeContents() {
return 0;
Expand All @@ -110,6 +134,7 @@ public void writeToParcel(Parcel dest, int flags) {
dest.writeString(mEmail);
dest.writeString(mToken);
dest.writeString(mSecret);
dest.writeInt(mErrorCode);
}

/**
Expand All @@ -120,6 +145,18 @@ public void writeToParcel(Parcel dest, int flags) {
*/
@Nullable
public static IdpResponse fromResultIntent(Intent resultIntent) {
return resultIntent.getParcelableExtra(ExtraConstants.EXTRA_IDP_RESPONSE);
if (resultIntent != null) {
return resultIntent.getParcelableExtra(ExtraConstants.EXTRA_IDP_RESPONSE);
} else {
return null;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since fromResultIntent is annotated as @Nullable, I think this was the original intention.

}

public static Intent getIntent(IdpResponse response) {
return new Intent().putExtra(ExtraConstants.EXTRA_IDP_RESPONSE, response);
}

public static Intent getErrorCodeIntent(int errorCode) {
return new Intent().putExtra(ExtraConstants.EXTRA_IDP_RESPONSE, new IdpResponse(errorCode));
}
}
23 changes: 23 additions & 0 deletions auth/src/main/java/com/firebase/ui/auth/ResultCodes.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package com.firebase.ui.auth;

import android.app.Activity;

/**
* Result codes returned when using {@link AuthUI.SignInIntentBuilder#build()} with
* {@code startActivityForResult}.
*/
public final class ResultCodes {
private ResultCodes() {
// We don't want people to initialize this class
}

/**
* Sign in succeeded
**/
public static final int OK = Activity.RESULT_OK;
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 decided to unify result codes and make ResultCodes a central place for those.

I decided to use the existing activity result codes to not break anything.


/**
* Sign in canceled by user
**/
public static final int CANCELED = Activity.RESULT_CANCELED;
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;

import com.firebase.ui.auth.IdpResponse;
import com.firebase.ui.auth.util.signincontainer.SaveSmartLock;
import com.google.firebase.auth.FirebaseUser;

Expand Down Expand Up @@ -48,7 +49,8 @@ public SaveSmartLock getSaveSmartLockInstance() {
public void saveCredentialsOrFinish(
@Nullable SaveSmartLock saveSmartLock,
FirebaseUser firebaseUser,
@NonNull String password) {
saveCredentialsOrFinish(saveSmartLock, mActivity, firebaseUser, password, null);
@NonNull String password,
IdpResponse response) {
saveCredentialsOrFinish(saveSmartLock, mActivity, firebaseUser, password, response);
}
}
6 changes: 3 additions & 3 deletions auth/src/main/java/com/firebase/ui/auth/ui/BaseHelper.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,14 @@
import android.support.v4.app.FragmentActivity;

import com.firebase.ui.auth.IdpResponse;
import com.firebase.ui.auth.ResultCodes;
import com.firebase.ui.auth.util.signincontainer.SaveSmartLock;
import com.google.android.gms.auth.api.Auth;
import com.google.android.gms.auth.api.credentials.CredentialsApi;
import com.google.firebase.FirebaseApp;
import com.google.firebase.auth.FirebaseAuth;
import com.google.firebase.auth.FirebaseUser;

import static android.app.Activity.RESULT_OK;
import static com.firebase.ui.auth.util.Preconditions.checkNotNull;

public class BaseHelper {
Expand Down Expand Up @@ -111,9 +111,9 @@ public void saveCredentialsOrFinish(
Activity activity,
FirebaseUser firebaseUser,
@Nullable String password,
@Nullable IdpResponse response) {
IdpResponse response) {
if (saveSmartLock == null) {
finishActivity(activity, RESULT_OK, new Intent());
finishActivity(activity, ResultCodes.OK, new Intent());
} else {
saveSmartLock.saveCredentialsOrFinish(
firebaseUser,
Expand Down
Loading