diff --git a/src/NuGetGallery/Areas/Admin/Controllers/ChangeUsernameController.cs b/src/NuGetGallery/Areas/Admin/Controllers/ChangeUsernameController.cs index 1ff85101e8..80f723f220 100644 --- a/src/NuGetGallery/Areas/Admin/Controllers/ChangeUsernameController.cs +++ b/src/NuGetGallery/Areas/Admin/Controllers/ChangeUsernameController.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Net; using System.Text.RegularExpressions; using System.Threading.Tasks; @@ -20,6 +21,7 @@ public class ChangeUsernameController : AdminControllerBase private readonly IEntitiesContext _entitiesContext; private readonly IDateTimeProvider _dateTimeProvider; private readonly IAuditingService _auditingService; + private readonly IPackageService _packageService; private readonly Regex UsernameValidationRegex = new Regex(GalleryConstants.UsernameValidationRegex); @@ -28,13 +30,15 @@ public ChangeUsernameController( IEntityRepository userRepository, IEntitiesContext entitiesContext, IDateTimeProvider dateTimeProvider, - IAuditingService auditingService) + IAuditingService auditingService, + IPackageService packageService) { _userService = userService ?? throw new ArgumentNullException(nameof(userService)); _userRepository = userRepository ?? throw new ArgumentNullException(nameof(userRepository)); _entitiesContext = entitiesContext ?? throw new ArgumentNullException(nameof(entitiesContext)); _dateTimeProvider = dateTimeProvider ?? throw new ArgumentNullException(nameof(dateTimeProvider)); _auditingService = auditingService ?? throw new ArgumentNullException(nameof(auditingService)); + _packageService = packageService ?? throw new ArgumentNullException(nameof(packageService)); } [HttpGet] @@ -83,14 +87,33 @@ public ActionResult VerifyAccount(string accountEmailOrUsername) } [HttpGet] - public ActionResult ValidateNewUsername(string newUsername) + public ActionResult ValidateNewUsername(string newUsername, bool checkOwnedPackages, string oldUsername) { if (string.IsNullOrEmpty(newUsername)) { return Json(HttpStatusCode.BadRequest, "Username cannot be null or empty.", JsonRequestBehavior.AllowGet); } - var result = ValidateUsername(newUsername); + if (string.IsNullOrEmpty(oldUsername)) + { + return Json(HttpStatusCode.BadRequest, "Old username cannot be null or empty.", JsonRequestBehavior.AllowGet); + } + + var oldAccount = _userService.FindByUsername(oldUsername); + if (oldAccount == null) + { + return Json(HttpStatusCode.NotFound, "Old username account was not found.", JsonRequestBehavior.AllowGet); + } + + var result = ValidateUsernameChange(oldAccount, newUsername); + + if (checkOwnedPackages) + { + var ownedPackages = _packageService.FindPackagesByOwner(oldAccount, includeUnlisted: true) + .Where(p => p.PackageStatusKey != PackageStatus.Deleted) + .Select(p => p.PackageRegistration.Id); + result.OwnedPackageIds = ownedPackages; + } return Json(result, JsonRequestBehavior.AllowGet); } @@ -116,39 +139,44 @@ public async Task ChangeUsername(string oldUsername, string newUse return Json(HttpStatusCode.NotFound, "Old username account was not found.", JsonRequestBehavior.AllowGet); } - var newUsernameValidation = ValidateUsername(newUsername); + var newUsernameValidation = ValidateUsernameChange(account, newUsername); if (!newUsernameValidation.IsFormatValid || !newUsernameValidation.IsAvailable) { return Json(HttpStatusCode.BadRequest, "New username validation failed.", JsonRequestBehavior.AllowGet); } - var newAccountForOldUsername = new User() + if (account.Username.Equals(newUsername, StringComparison.OrdinalIgnoreCase) == false) { - Username = account.Username, - EmailAllowed = false, - IsDeleted = true, - CreatedUtc = _dateTimeProvider.UtcNow - }; + // We're doing a full username change and not just a casing change so we need to lock the old username + var newAccountForOldUsername = new User() + { + Username = account.Username, + EmailAllowed = false, + IsDeleted = true, + CreatedUtc = _dateTimeProvider.UtcNow + }; + + _userRepository.InsertOnCommit(newAccountForOldUsername); + } account.Username = newUsername; await _auditingService.SaveAuditRecordAsync(new UserAuditRecord(account, AuditedUserAction.ChangeUsername)); - - _userRepository.InsertOnCommit(newAccountForOldUsername); - await _entitiesContext.SaveChangesAsync(); return Json(HttpStatusCode.OK, "Account renamed successfully!", JsonRequestBehavior.AllowGet); } - private ValidateUsernameResult ValidateUsername(string username) + private ValidateUsernameResult ValidateUsernameChange(User requestor, string username) { - var result = new ValidateUsernameResult(); - result.IsFormatValid = UsernameValidationRegex.IsMatch(username); - result.IsAvailable = _userService.FindByUsername(username, includeDeleted: true) == null; + var foundUser = _userService.FindByUsername(username, includeDeleted: true); - return result; + return new ValidateUsernameResult() + { + IsFormatValid = UsernameValidationRegex.IsMatch(username), + IsAvailable = foundUser == null || (requestor.Key == foundUser.Key && foundUser.Username != username) // The username check is in the event where we found a user in the DB but we're doing a cAsIng change + }; } } } diff --git a/src/NuGetGallery/Areas/Admin/Models/ValidateUsernameResult.cs b/src/NuGetGallery/Areas/Admin/Models/ValidateUsernameResult.cs index f7a16b2436..9b68f21e78 100644 --- a/src/NuGetGallery/Areas/Admin/Models/ValidateUsernameResult.cs +++ b/src/NuGetGallery/Areas/Admin/Models/ValidateUsernameResult.cs @@ -1,11 +1,14 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System.Collections.Generic; + namespace NuGetGallery.Areas.Admin.Models { public class ValidateUsernameResult { public bool IsFormatValid { get; set; } public bool IsAvailable { get; set; } + public IEnumerable OwnedPackageIds { get; set; } = new List(); } } \ No newline at end of file diff --git a/src/NuGetGallery/Areas/Admin/Views/ChangeUsername/Index.cshtml b/src/NuGetGallery/Areas/Admin/Views/ChangeUsername/Index.cshtml index bf83313769..159946a688 100644 --- a/src/NuGetGallery/Areas/Admin/Views/ChangeUsername/Index.cshtml +++ b/src/NuGetGallery/Areas/Admin/Views/ChangeUsername/Index.cshtml @@ -53,12 +53,19 @@ -

Validate new username

- - Validate if the new username is available and has a valid format.
+

Validate & Change new username

+ @using (Html.BeginForm()) + { + @Html.AntiForgeryToken() + Validate & Change if the new username is available and has a valid format.
- - + + + + + + + }
@ViewHelpers.AlertDanger(@)
@@ -69,24 +76,24 @@
Username is available.
Username is already taken.
- - -
-

Change username

- @using (Html.BeginForm()) - { - @Html.AntiForgeryToken() - Update current account username.
- - - - }
@ViewHelpers.Alert(@, "success", "Info")
@ViewHelpers.AlertDanger(@)
+ + + + + + + + + + + +
@@ -105,6 +112,9 @@ } this.accountAdministrators = ko.observableArray([]); this.newUsername = ko.observable(''); + this.oldUsername = ko.observable(''); + this.checkOwnedPackages = ko.observable(false); + this.ownedPackages = ko.observableArray([]); this.validatationUsernameResult = { IsFormatValid: ko.observable(false), IsAvailable: ko.observable(false) @@ -161,6 +171,7 @@ this.validateNewUsername = function () { $self.doneValidatingNewUsername(false); + $self.ownedPackages([]); $self.errorNewUsername(''); if (!$self.newUsername()) { @@ -168,7 +179,7 @@ return; } - var queryParams = '?newUsername=' + encodeURIComponent($self.newUsername().trim()); + var queryParams = '?newUsername=' + encodeURIComponent($self.newUsername().trim()) + '&oldUsername=' + encodeURIComponent($self.oldUsername().trim()) + '&checkOwnedPackages=' + encodeURIComponent($self.checkOwnedPackages()); $.ajax({ url: actionUrlValidateNewUsername + queryParams, @@ -177,6 +188,7 @@ success: function (data) { $self.validatationUsernameResult.IsFormatValid(data.IsFormatValid); $self.validatationUsernameResult.IsAvailable(data.IsAvailable); + $self.ownedPackages(data.OwnedPackageIds); } }) .fail(function (jqXhr, textStatus, errorThrown) { @@ -193,16 +205,18 @@ }; this.changeUsername = function (model, event) { - $self.errorChangeUsername(''); + $self.doneValidatingNewUsername(false); + + $self.errorNewUsername(''); $self.changeUsernameSuccessful(''); var data = { - oldUsername: $self.changeOldUsername().trim(), - newUsername: $self.changeNewUsername().trim(), + oldUsername: $self.oldUsername().trim(), + newUsername: $self.newUsername().trim(), }; if (!data.oldUsername || !data.newUsername) { - $self.errorChangeUsername('Usernames should not be empty.'); + $self.errorNewUsername('Usernames should not be empty.'); event.preventDefault(); return; } @@ -224,7 +238,7 @@ }) .fail(function (jqXhr, textStatus, errorThrown) { if (jqXhr) { - $self.errorChangeUsername(jqXhr.responseJSON); + $self.errorNewUsername(jqXhr.responseJSON); } else { alert('Error: ' + errorThrown); diff --git a/tests/NuGetGallery.Facts/Areas/Admin/Controllers/ChangeUsernameControllerFacts.cs b/tests/NuGetGallery.Facts/Areas/Admin/Controllers/ChangeUsernameControllerFacts.cs index 857cff2634..d19053253f 100644 --- a/tests/NuGetGallery.Facts/Areas/Admin/Controllers/ChangeUsernameControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Areas/Admin/Controllers/ChangeUsernameControllerFacts.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System.Collections.Generic; using System.Linq; using System.Net; using System.Threading.Tasks; @@ -104,29 +105,66 @@ public void WhenValidOrganizationAccountReturnsAccountWithAdministrators() public class ValidateNewUsernameMethod : FactsBase { [Theory] - [InlineData("")] - [InlineData(null)] - public void WhenInvalidNewUsernameReturnsBadRequestStatusCode(string newUsername) + [InlineData("", true)] + [InlineData("", false)] + [InlineData(null, true)] + [InlineData(null, false)] + public void WhenInvalidNewUsernameReturnsBadRequestStatusCode(string newUsername, bool checkOwnedPackages) { - var result = ChangeUsernameController.ValidateNewUsername(newUsername) as JsonResult; + var result = ChangeUsernameController.ValidateNewUsername(newUsername, checkOwnedPackages, oldUsername: "testUser") as JsonResult; Assert.Equal(((int)HttpStatusCode.BadRequest), ChangeUsernameController.Response.StatusCode); Assert.Equal("Username cannot be null or empty.", result.Data); } + [Theory] + [InlineData("", true)] + [InlineData("", false)] + [InlineData(null, true)] + [InlineData(null, false)] + public void WhenInvalidOldUsernameReturnsBadRequestStatusCode(string oldUsername, bool checkOwnedPackages) + { + var result = ChangeUsernameController.ValidateNewUsername("testUser", checkOwnedPackages, oldUsername) as JsonResult; + + Assert.Equal(((int)HttpStatusCode.BadRequest), ChangeUsernameController.Response.StatusCode); + Assert.Equal("Old username cannot be null or empty.", result.Data); + } + [Theory] [InlineData("_aaa_", false, true)] [InlineData("testUser", true, false)] [InlineData("availableUsername", true, true)] public void WhenValidNewUsernameReturnsValidations(string newUsername, bool isFormatValid, bool isAvailable) { - var result = ChangeUsernameController.ValidateNewUsername(newUsername) as JsonResult; + var result = ChangeUsernameController.ValidateNewUsername(newUsername, checkOwnedPackages: false, oldUsername: "testUser") as JsonResult; var validations = result.Data as ValidateUsernameResult; Assert.Equal(isFormatValid, validations.IsFormatValid); Assert.Equal(isAvailable, validations.IsAvailable); } + + [Theory] + [InlineData("someNewUsername", "testOrganization", false)] + [InlineData("someNewUsername", "testUser", true)] + public void WhenCheckOwnedPackagesReturnThem(string newUsername, string oldUsername, bool expectPackages) + { + var result = ChangeUsernameController.ValidateNewUsername(newUsername, checkOwnedPackages: true, oldUsername) as JsonResult; + + var validations = result.Data as ValidateUsernameResult; + + Assert.True(validations.IsFormatValid); + Assert.True(validations.IsAvailable); + + if(expectPackages) + { + Assert.NotEmpty(validations.OwnedPackageIds); + } + else + { + Assert.Empty(validations.OwnedPackageIds); + } + } } public class ChangeUsernameMethod : FactsBase @@ -211,6 +249,32 @@ public async void WhenValidAccountAndNewUsernameReturnsOkStatusCode() Assert.Equal(((int)HttpStatusCode.OK), ChangeUsernameController.Response.StatusCode); Assert.Equal("Account renamed successfully!", result.Data); } + + [Fact] + public async void WhenValidAccountCasingChangeAndNewUsernameReturnsOkStatusCode() + { + var oldUsername = IndividualAccount.Username; + + var result = await ChangeUsernameController.ChangeUsername(IndividualAccount.Username, IndividualAccount.Username.ToUpper()) as JsonResult; + + GetMock().Verify(u => u.FindByUsername(oldUsername, false)); + GetMock().Verify(u => u.FindByUsername(oldUsername.ToUpper(), true)); + GetMock>().Verify(r => r.InsertOnCommit(It.IsAny()), Times.Never()); + GetMock().Verify(c => c.SaveChangesAsync()); + + Assert.Equal(oldUsername.ToUpper(), IndividualAccount.Username); + Assert.Equal(((int)HttpStatusCode.OK), ChangeUsernameController.Response.StatusCode); + Assert.Equal("Account renamed successfully!", result.Data); + } + + [Fact] + public async void FailWhenAccountCasingNotAvailable() + { + var result = await ChangeUsernameController.ChangeUsername(IndividualAccount.Username, OrganizationAccount.Username.ToUpper()) as JsonResult; + + Assert.Equal(((int)HttpStatusCode.BadRequest), ChangeUsernameController.Response.StatusCode); + Assert.Equal("New username validation failed.", result.Data); + } } public class FactsBase : TestContainer @@ -227,29 +291,36 @@ public FactsBase() OrganizationAccount = new Fakes().Organization; OrganizationAdministrator = new Fakes().OrganizationAdmin; + // FindByUsername is case-insensitive in the database, so we need to simulate that here GetMock() - .Setup(u => u.FindByUsername(IndividualAccount.Username, It.IsAny())) + .Setup(u => u.FindByUsername(It.Is(x => string.Equals(IndividualAccount.Username, x, System.StringComparison.OrdinalIgnoreCase)), It.IsAny())) .Returns(IndividualAccount); GetMock() - .Setup(u => u.FindByUsername(IndividualAccount.EmailAddress, false)) + .Setup(u => u.FindByUsername(It.Is(x => string.Equals(IndividualAccount.EmailAddress, x, System.StringComparison.OrdinalIgnoreCase)), false)) .Returns(IndividualAccount); GetMock() - .Setup(u => u.FindByUsername(OrganizationAccount.Username, true)) + .Setup(u => u.FindByUsername(It.Is(x => string.Equals(OrganizationAccount.Username, x, System.StringComparison.OrdinalIgnoreCase)), true)) .Returns(OrganizationAccount); GetMock() - .Setup(u => u.FindByUsername(OrganizationAccount.EmailAddress, true)) + .Setup(u => u.FindByUsername(It.Is(x => string.Equals(OrganizationAccount.EmailAddress, x, System.StringComparison.OrdinalIgnoreCase)), true)) .Returns(OrganizationAccount); GetMock() - .Setup(u => u.FindByUsername(AvailableUsername, true)) + .Setup(u => u.FindByUsername(It.Is(x => string.Equals(AvailableUsername, x, System.StringComparison.OrdinalIgnoreCase)), true)) .ReturnsNull(); + GetMock().Setup(p => p.FindPackagesByOwner(It.IsAny(), It.IsAny(), It.IsAny())) + .Returns(new List()); + GetMock().Setup(p => p.FindPackagesByOwner(It.Is(u => u.Username == "testUser"), It.IsAny(), It.IsAny())) + .Returns(new List() { new Package() { PackageRegistration = new PackageRegistration() { Id = "testPackage" } }}); + ChangeUsernameController = new ChangeUsernameController( GetMock().Object, GetMock>().Object, GetMock().Object, GetMock().Object, - GetMock().Object); + GetMock().Object, + GetMock().Object); TestUtility.SetupHttpContextMockForUrlGeneration(new Mock(), ChangeUsernameController); } diff --git a/tests/NuGetGallery.Facts/Framework/Fakes.cs b/tests/NuGetGallery.Facts/Framework/Fakes.cs index c59329f0b6..e95b1a7ccc 100644 --- a/tests/NuGetGallery.Facts/Framework/Fakes.cs +++ b/tests/NuGetGallery.Facts/Framework/Fakes.cs @@ -31,7 +31,7 @@ public Fakes() ApiKeyV3PlaintextValue = "889e180e-335c-491a-ac26-e83c4bd31d87"; - User = new User("testUser") + User = new User("testUser") // NOTE: Do not change the casing of this username. It will break tests for the ChangeUsername in the Admin Panel { Key = key++, EmailAddress = "confirmed0@example.com", @@ -58,7 +58,7 @@ public Fakes() ApiKeyV4PlaintextValue = apiKeyV4PlaintextValue; - Organization = new Organization("testOrganization") + Organization = new Organization("testOrganization") // NOTE: Do not change the casing of this username. It will break tests for the ChangeUsername in the Admin Panel { Key = key++, EmailAddress = "confirmedOrganization@example.com",