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 3 commits
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
6 changes: 6 additions & 0 deletions src/NuGetGallery/Controllers/AuthenticationController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,12 @@ private async Task<AuthenticatedUser> AssociateCredential(AuthenticatedUser user

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

var passwordCredential = user.User.Credentials.SingleOrDefault(c => c.IsPassword());
Copy link
Member

Choose a reason for hiding this comment

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

Extension method?

public static CredentialTypes {
    public static GetPassword(this IEnumerable<Credential> credentials);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, although I put it on User itself because I can't imagine anything else having a list of credentials

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.

}

// Notify the user of the change
_messageService.SendCredentialAddedNotice(user.User, _authService.DescribeCredential(result.Credential));

Expand Down
5 changes: 5 additions & 0 deletions src/NuGetGallery/Views/Authentication/LinkExternal.cshtml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@
{
<p class="text-center">
We found an account with this email address. Sign in with your password to link with this Microsoft account.
</p>
<p class="text-center">
Note that <b>your existing password login will be disabled</b> and you will need to use this Microsoft account to sign into NuGet.org
</p>
<p class="text-center">
Please <a href="mailto:@Config.Current.GalleryOwner.Address">contact support</a> if you need more assistance.
</p>

Expand Down
10 changes: 8 additions & 2 deletions tests/NuGetGallery.Facts/Controllers/AuthenticationControllerFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,11 +243,14 @@ public async Task WillLogTheUserOnWithUsernameEvenWithoutConfirmedEmailAddress()
GetMock<AuthenticationService>().VerifyAll();
}

public async Task WhenAttemptingToLinkExternalToExistingAccountWithNoExternalAccounts_AllowsLinking()
public async Task WhenAttemptingToLinkExternalToExistingAccountWithNoExternalAccounts_AllowsLinkingAndRemovesPassword()
{
// Arrange
var passwordCredential = new Credential(CredentialTypes.Password.Prefix, "thePassword");
var user = new User("theUsername") { EmailAddress = "[email protected]", Credentials = new[] { passwordCredential } };

var authUser = new AuthenticatedUser(
new User("theUsername") { EmailAddress = "[email protected]" },
user,
new Credential() { Type = "foo" });

var authResult =
Expand Down Expand Up @@ -278,6 +281,9 @@ public async Task WhenAttemptingToLinkExternalToExistingAccountWithNoExternalAcc
GetMock<AuthenticationService>()
.Verify(x => x.CreateSessionAsync(controller.OwinContext, authUser));

GetMock<AuthenticationService>()
.Verify(x => x.RemoveCredential(user, passwordCredential));

GetMock<IMessageService>()
.Verify(x => x.SendCredentialAddedNotice(It.IsAny<User>(), It.IsAny<CredentialViewModel>()));
}
Expand Down