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.

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

Support username casing change in Admin UI #9748

Draft
wants to merge 11 commits into
base: dev
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.Linq;
using System.Net;
using System.Text.RegularExpressions;
using System.Threading.Tasks;
Expand All @@ -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);

Expand All @@ -28,13 +30,15 @@ public ChangeUsernameController(
IEntityRepository<User> 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]
Expand Down Expand Up @@ -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);
Copy link
Contributor

@zhhyu zhhyu Jan 24, 2024

Choose a reason for hiding this comment

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

oldAccount

nit: oldAccount -> account

It may be better to use the same name for local variables under the same class. For example, line 130 uses "account". We can follow the same name convention and also the account itself is still the same but the name will be changed. Personally I think this is more readable and easier to maintain in the future.

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);
}
Expand All @@ -116,39 +139,44 @@ public async Task<ActionResult> 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

newUsernameValidation

nit: newUsernameValidation -> validatedResult

The same at line 103.


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)
mariaghiondea marked this conversation as resolved.
Show resolved Hide resolved
RiadGahlouz marked this conversation as resolved.
Show resolved Hide resolved
{
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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
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

};
}
}
}
Original file line number Diff line number Diff line change
@@ -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<string> OwnedPackageIds { get; set; } = new List<string>();
}
}
60 changes: 37 additions & 23 deletions src/NuGetGallery/Areas/Admin/Views/ChangeUsername/Index.cshtml
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,19 @@
</table>
</div>

<h2>Validate new username</h2>

<i>Validate if the new username is available and has a valid format.</i><br />
<h2>Validate &amp; Change new username</h2>
@using (Html.BeginForm())
{
@Html.AntiForgeryToken()
<i>Validate &amp; Change if the new username is available and has a valid format.</i><br />

<input type="text" placeholder="New username" autocomplete="off" autofocus style="width: 200px;" rows="1" data-bind="value: newUsername" />
<button type="submit" data-bind="click: validateNewUsername">Validate</button>
<input type="text" placeholder="Old Username" autocomplete="off" autofocus style="width: 200px;" rows="1" data-bind="value: oldUsername" />
<input type="text" placeholder="New username" autocomplete="off" autofocus style="width: 200px;" rows="1" data-bind="value: newUsername" />
<input type="checkbox" id="checkNonDeletedOwnedPackages" data-bind="checked: checkOwnedPackages" />
<label for="checkNonDeletedOwnedPackages">Check Non-Deleted Owned Packages</label>
<button type="submit" data-bind="click: validateNewUsername">Validate</button>
<button type="submit" data-bind="click: changeUsername">Change</button>
}
<div style="display:none" data-bind="visible: errorNewUsername">
@ViewHelpers.AlertDanger(@<text><span data-bind="text: errorNewUsername"></span></text>)
</div>
Expand All @@ -69,24 +76,24 @@
<div style="display:none;" data-bind="visible: validatationUsernameResult.IsAvailable()"><i class="ms-Icon ms-Icon--CompletedSolid" style="color: green"></i> Username is available.</div>
<div style="display:none;" data-bind="visible: !validatationUsernameResult.IsAvailable()"><i class="ms-Icon ms-Icon--StatusErrorFull" style="color: red"></i> Username is already taken.</div>
</div>
</div>

<div>
<h2>Change username</h2>
@using (Html.BeginForm())
{
@Html.AntiForgeryToken()
<i>Update current account username.</i><br />
<input type="text" placeholder="Old username" autocomplete="off" autofocus style="width: 200px;" rows="1" data-bind="value: changeOldUsername" />
<input type="text" placeholder="New username" autocomplete="off" autofocus style="width: 200px;" rows="1" data-bind="value: changeNewUsername" />
<button type="submit" data-bind="click: changeUsername">Change</button>
}
<div style="display:none" data-bind="visible: changeUsernameSuccessful">
@ViewHelpers.Alert(@<text><span data-bind="text: changeUsernameSuccessful"></span></text>, "success", "Info")
</div>
<div style="display:none" data-bind="visible: errorChangeUsername">
@ViewHelpers.AlertDanger(@<text><span data-bind="text: errorChangeUsername"></span></text>)
</div>
<table class="table" id="ownedPackages" style="display: none" data-bind="visible: ownedPackages().length > 0 && checkOwnedPackages" aria-label="owned packages">
<thead>
<tr>
<th>Owned Package Id</th>
</tr>
</thead>
<tbody data-bind="foreach: ownedPackages">
<tr>
<td><span data-bind="text: $data"></span></td>
</tr>
</tbody>
</table>
</div>
</section>

Expand All @@ -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)
Expand Down Expand Up @@ -161,14 +171,15 @@

this.validateNewUsername = function () {
$self.doneValidatingNewUsername(false);
$self.ownedPackages([]);
$self.errorNewUsername('');

if (!$self.newUsername()) {
$self.errorNewUsername('Username cannot be null or empty.');
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,
Expand All @@ -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) {
Expand All @@ -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;
}
Expand All @@ -224,7 +238,7 @@
})
.fail(function (jqXhr, textStatus, errorThrown) {
if (jqXhr) {
$self.errorChangeUsername(jqXhr.responseJSON);
$self.errorNewUsername(jqXhr.responseJSON);
}
else {
alert('Error: ' + errorThrown);
Expand Down
Loading