Fix email and Google providers creating duplicate credentials#1229
Fix email and Google providers creating duplicate credentials#1229samtstern merged 8 commits intofirebase:version-3.3.1-devfrom SUPERCILEX:creds-save
Conversation
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
| user.getEmail() == null ? user.getPhoneNumber() : user.getEmail()) | ||
| .setAccountType(type) | ||
| .build()); | ||
| // Also add an email credential to ensure password accounts get deleted. |
There was a problem hiding this comment.
This confuses me a bit ... feels wrong that the getCredentialsFromFirebaseUser method will always return a password credential with a made-up password.
Seems like something that could surprise us down the road, and it makes the method contract kind of a lie.
There was a problem hiding this comment.
Huge thank you on catching that one! Refactor mistake, I've fixed it.
| Uri profilePictureUri = | ||
| user.getPhotoUrl() == null ? null : Uri.parse(user.getPhotoUrl().toString()); | ||
|
|
||
| if (TextUtils.isEmpty(email) && TextUtils.isEmpty(phone) |
There was a problem hiding this comment.
Nit: this would be clearer if separated into two if-return-null statements.
if (no phone and no email) {
return null;
}
if (no password and no account type) {
return null
}
| import com.firebase.ui.auth.ui.HelperActivityBase; | ||
|
|
||
| @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) | ||
| public abstract class VoidResourceObserver extends ResourceObserver<Void> { |
|
|
||
| private void deleteUnusedCredentials() { | ||
| if (mResponse.getProviderType().equals(GoogleAuthProvider.PROVIDER_ID)) { | ||
| // Since Google accounts upgrade email ones, we don't want to end up |
There was a problem hiding this comment.
Ok so want to make sure what's going on here:
Whenever we sign in as a Google user we look for matching email credentials and delete them since the Google one is "superior"?
There was a problem hiding this comment.
Close. You should try this yourself: the "bug" everyone has been complaining about is that when you use the Google provider, it basically overwrites existing providers and pretends that they never existed. AFAIK, this is expected behavior so this removes the no-longer valid credential. Hope that clarifies things! 😄
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
| TextUtils.isEmpty(user.getEmail()) ? user.getPhoneNumber() : user.getEmail()) | ||
| .setAccountType(type) | ||
| .build()); | ||
| if (type == null) { |
There was a problem hiding this comment.
Let's just add a comment here for future-us explaining why we're doing this hack with the "pass" credential.
There was a problem hiding this comment.
Well actually, this isn't a hack anymore. If the account type is null, it's an email credential so we're just deleting it as we should. Or did you mean explaining the fake password? Either way, I'm happy to add a comment once I understand which piece you're talking about. 😊
There was a problem hiding this comment.
Yeah just explaining why the password doesn't matter (since we're just creating a credential for deletion).
Oh and also that null means email for account type, since that's not obvious when reading this method (it is when reading the other method)
There was a problem hiding this comment.
Sweet, will do!
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
2 participants
Ideally this should be merged before #1212
Since the Google provider upgrades email accounts we end up with two credentials which will pointlessly force Smart Lock UI.
In addition, this PR fixes: