Remove password credential from user when they link an external account #5382
Conversation
scottbommarito
requested review from
joelverhagen,
agr,
loic-sharma,
xavierdecoster,
shishirx34,
nugetlurker,
ryuyu,
anangaur,
chenriksson,
cristinamanum and
skofman1
| @@ -536,6 +536,12 @@ private ActionResult RedirectFromRegister(string returnUrl) | |||
|
|
|||
| await _authService.AddCredential(user.User, result.Credential); | |||
|
|
|||
| var passwordCredential = user.User.Credentials.SingleOrDefault(c => c.IsPassword()); | |||
There was a problem hiding this comment.
Extension method?
public static CredentialTypes {
public static GetPassword(this IEnumerable<Credential> credentials);
}
There was a problem hiding this comment.
done, although I put it on User itself because I can't imagine anything else having a list of credentials
| var passwordCredential = user.User.Credentials.SingleOrDefault(c => c.IsPassword()); | ||
| if (passwordCredential != null) | ||
| { | ||
| await _authService.RemoveCredential(user.User, passwordCredential); |
There was a problem hiding this comment.
Do we want this in the same transaction as AddCredential? Should the APIs support deferring the SaveChangesAsync?
There was a problem hiding this comment.
I think if removing the password fails, we should still proceed with the request, just leave the credential there--no need to remove a perfectly valid external credential.
There was a problem hiding this comment.
This is probably also a problem with @shishirx34's link and replace flow, where I based this code off of.
There was a problem hiding this comment.
I agree it would be much better to have an API for AddExternalAndRemovePasswordCredential but I think its fine, for now, to leave it as is. I won't say it be high priority right now.
| @@ -401,7 +401,7 @@ public ActionResult ChallengeAuthentication(string returnUrl, string provider) | |||
| await _authService.CreateSessionAsync(OwinContext, authenticatedUser); | |||
|
|
|||
| // Remove the password credential after linking to external account. | |||
| var passwordCred = user.Credentials.SingleOrDefault(c => c.IsPassword()); | |||
| var passwordCred = user.GetPassword(); | |||
There was a problem hiding this comment.
nit: perhaps GetPasswordCredential be more appropriate? GetPassword seems like we are getting the password value. Don't care though.
Short addition to #5338