Skip to content
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.

Sign up for GitHub

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

Remove password credential from user when they link an external account #5382

Merged
merged 6 commits into from
Feb 6, 2018
Merged

Remove password credential from user when they link an external account #5382

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
98547a2
change link external text
Feb 2, 2018
d19f753
remove password credential on login
Feb 2, 2018
9f26b52
Merge branch 'dev' into sb-removepasswordonlink
Feb 2, 2018
8b47b7a
get password extension method
Feb 2, 2018
1d79052
getpasswordcredential
Feb 6, 2018
3908539
Merge branch 'dev' into sb-removepasswordonlink
Feb 6, 2018
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
4 changes: 2 additions & 2 deletions src/NuGetGallery/Controllers/AuthenticationController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ public virtual async Task<ActionResult> LinkOrChangeExternalCredential(string re
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();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: perhaps GetPasswordCredential be more appropriate? GetPassword seems like we are getting the password value. Don't care though.


if (passwordCred != null)
{
Expand Down Expand Up @@ -536,7 +536,7 @@ private async Task<AuthenticatedUser> AssociateCredential(AuthenticatedUser user

await _authService.AddCredential(user.User, result.Credential);

var passwordCredential = user.User.Credentials.SingleOrDefault(c => c.IsPassword());
var passwordCredential = user.User.GetPassword();
if (passwordCredential != null)
{
await _authService.RemoveCredential(user.User, passwordCredential);
Copy link
Member

Choose a reason for hiding this comment

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

Do we want this in the same transaction as AddCredential? Should the APIs support deferring the SaveChangesAsync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is probably also a problem with @shishirx34's link and replace flow, where I based this code off of.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Expand Down
8 changes: 8 additions & 0 deletions src/NuGetGallery/Extensions/UserExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@ public static bool IsAdministrator(this User self)
return self.IsInRole(Constants.AdminRoleName);
}

/// <summary>
/// Get the user's <see cref="Credential"/> with a type of <see cref="CredentialTypes.Password"/>.
/// </summary>
public static Credential GetPassword(this User user)
{
return user.Credentials.SingleOrDefault(c => c.IsPassword());
}

/// <summary>
/// Get the current API key credential, if available.
/// </summary>
Expand Down