Cleanup + make sure IdpResponse is never null if resultCode == ResultCodes.OK #469
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,38 +40,40 @@ | |
| import com.firebase.ui.auth.ui.ExtraConstants; | ||
| import com.firebase.ui.auth.ui.FlowParameters; | ||
| import com.firebase.ui.auth.ui.TaskFailureLogger; | ||
| import com.firebase.ui.auth.ui.User; | ||
| import com.google.android.gms.tasks.OnCompleteListener; | ||
| import com.google.android.gms.tasks.OnFailureListener; | ||
| import com.google.android.gms.tasks.OnSuccessListener; | ||
| import com.google.android.gms.tasks.Task; | ||
| import com.google.firebase.auth.AuthCredential; | ||
| import com.google.firebase.auth.AuthResult; | ||
| import com.google.firebase.auth.FacebookAuthProvider; | ||
| import com.google.firebase.auth.FirebaseUser; | ||
| import com.google.firebase.auth.GoogleAuthProvider; | ||
| import com.google.firebase.auth.TwitterAuthProvider; | ||
|
|
||
| public class WelcomeBackIdpPrompt extends AppCompatBase implements IdpCallback { | ||
|
There was a problem hiding this comment. This one was a mess!!! I've been struggling to understand what's going on in #309 for a while now so I decided it was time for a rewrite. If you look at the class without all the crazy diff stuff, it's much clearer what's going on now. |
||
| private static final String TAG = "WelcomeBackIdpPrompt"; | ||
|
|
||
| private IdpProvider mIdpProvider; | ||
| private AuthCredential mPrevCredential; | ||
|
|
||
| @Override | ||
| protected void onCreate(Bundle savedInstanceState) { | ||
| super.onCreate(savedInstanceState); | ||
| setContentView(R.layout.welcome_back_idp_prompt_layout); | ||
|
|
||
| IdpResponse newUserIdpResponse = IdpResponse.fromResultIntent(getIntent()); | ||
| mPrevCredential = AuthCredentialHelper.getAuthCredential(newUserIdpResponse); | ||
|
|
||
| User oldUser = User.getUser(getIntent()); | ||
|
|
||
| String providerId = oldUser.getProvider(); | ||
| for (IdpConfig idpConfig : mActivityHelper.getFlowParams().providerInfo) { | ||
| if (providerId.equals(idpConfig.getProviderId())) { | ||
| switch (providerId) { | ||
| case GoogleAuthProvider.PROVIDER_ID: | ||
| mIdpProvider = new GoogleProvider(this, idpConfig, oldUser.getEmail()); | ||
| break; | ||
| case FacebookAuthProvider.PROVIDER_ID: | ||
| mIdpProvider = new FacebookProvider( | ||
|
|
@@ -89,21 +91,16 @@ protected void onCreate(Bundle savedInstanceState) { | |
| } | ||
| } | ||
|
|
||
| if (mIdpProvider == null) { | ||
| Log.w(TAG, "Firebase login unsuccessful." | ||
| + " Account linking failed due to provider not enabled by application: " | ||
| + providerId); | ||
| finish(ResultCodes.CANCELED, IdpResponse.getErrorCodeIntent(ErrorCodes.UNKNOWN_ERROR)); | ||
| return; | ||
| } | ||
|
|
||
| ((TextView) findViewById(R.id.welcome_back_idp_prompt)) | ||
| .setText(getIdpPromptString(oldUser.getEmail())); | ||
|
|
||
| mIdpProvider.setAuthenticationCallback(this); | ||
| findViewById(R.id.welcome_back_idp_button).setOnClickListener(new OnClickListener() { | ||
|
|
@@ -127,83 +124,71 @@ public void onActivityResult(int requestCode, int resultCode, Intent data) { | |
| } | ||
|
|
||
| @Override | ||
| public void onSuccess(final IdpResponse idpResponse) { | ||
| if (idpResponse == null) { | ||
| return; // do nothing | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Ahhhhhh!!! It took me so long in #309 to "understand" what was going on only to find out I was wrong: we never even used this click listener! That kills me! There was a problem hiding this comment. Yeah it took me a little while to see what was going on here too. Good find! This stray listener could have bitten us down the road. |
||
| AuthCredential newCredential = AuthCredentialHelper.getAuthCredential(idpResponse); | ||
| if (newCredential == null) { | ||
| Log.e(TAG, "No credential returned"); | ||
| finish(ResultCodes.CANCELED, IdpResponse.getErrorCodeIntent(ErrorCodes.UNKNOWN_ERROR)); | ||
| return; | ||
| } | ||
|
|
||
| FirebaseUser currentUser = mActivityHelper.getCurrentUser(); | ||
| if (currentUser == null) { | ||
| mActivityHelper.getFirebaseAuth() | ||
| .signInWithCredential(newCredential) | ||
| .addOnSuccessListener(new OnSuccessListener<AuthResult>() { | ||
| @Override | ||
| public void onSuccess(AuthResult result) { | ||
| if (mPrevCredential != null) { | ||
| result.getUser() | ||
| .linkWithCredential(mPrevCredential) | ||
| .addOnFailureListener(new TaskFailureLogger( | ||
| TAG, "Error signing in with previous credential")) | ||
| .addOnCompleteListener(new FinishListener(idpResponse)); | ||
| } else { | ||
| finish(ResultCodes.OK, IdpResponse.getIntent(idpResponse)); | ||
|
There was a problem hiding this comment. This There was a problem hiding this comment. I believe this was the original intention: if we have a previous credential, link it, else, finish ok. Previously, if we either didn't have a previous credential or the task failed, we would finish ok. Because we would finish ok if we didn't have a previous credential, this isn't a behavior change. |
||
| } | ||
| } | ||
| }) | ||
| .addOnFailureListener(new OnFailureListener() { | ||
| @Override | ||
| public void onFailure(@NonNull Exception e) { | ||
| finishWithError(); | ||
|
There was a problem hiding this comment. On failure, we fail. |
||
| } | ||
| }) | ||
| .addOnFailureListener( | ||
| new TaskFailureLogger(TAG, "Error signing in with new credential")); | ||
| } else { | ||
| currentUser | ||
| .linkWithCredential(newCredential) | ||
| .addOnFailureListener( | ||
| new TaskFailureLogger(TAG, "Error linking with credential")) | ||
| .addOnCompleteListener(new FinishListener(idpResponse)); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void onFailure(Bundle extra) { | ||
| finishWithError(); | ||
|
|
||
| } | ||
|
|
||
| private void finishWithError() { | ||
| Toast.makeText(this, R.string.general_error, Toast.LENGTH_LONG).show(); | ||
| finish(ResultCodes.CANCELED, IdpResponse.getErrorCodeIntent(ErrorCodes.UNKNOWN_ERROR)); | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. BAD! (I think) |
||
| public static Intent createIntent( | ||
| Context context, | ||
| FlowParameters flowParams, | ||
| User existingUser, | ||
| IdpResponse newUserResponse) { | ||
| return BaseHelper.createBaseIntent(context, WelcomeBackIdpPrompt.class, flowParams) | ||
| .putExtra(ExtraConstants.EXTRA_USER, existingUser) | ||
| .putExtra(ExtraConstants.EXTRA_IDP_RESPONSE, newUserResponse); | ||
| } | ||
|
|
||
| private class FinishListener implements OnCompleteListener<AuthResult> { | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only show the token/secret if we have one.