From 7a86e823a1f75cf66a8dfaa43921555acda6ade3 Mon Sep 17 00:00:00 2001 From: Scott Bommarito Date: Thu, 22 Feb 2018 16:07:00 -0800 Subject: [PATCH 01/24] [BugBash] Prevent added model errors from being reset by JQuery Validate (#5502) --- src/NuGetGallery/Controllers/UsersController.cs | 12 ++++++------ src/NuGetGallery/ExtensionMethods.cs | 17 +++++++++++++---- .../ViewModels/PasswordResetRequestViewModel.cs | 1 + .../ViewModels/TransformAccountViewModel.cs | 2 +- .../Views/Users/ForgotPassword.cshtml | 3 +++ src/NuGetGallery/Views/Users/Transform.cshtml | 3 +++ .../Controllers/UsersControllerFacts.cs | 12 ++++++------ 7 files changed, 33 insertions(+), 17 deletions(-) diff --git a/src/NuGetGallery/Controllers/UsersController.cs b/src/NuGetGallery/Controllers/UsersController.cs index 8826bc88ef..f608a47b4b 100644 --- a/src/NuGetGallery/Controllers/UsersController.cs +++ b/src/NuGetGallery/Controllers/UsersController.cs @@ -126,14 +126,14 @@ public virtual async Task TransformToOrganization(TransformAccount var adminUser = UserService.FindByUsername(transformViewModel.AdminUsername); if (adminUser == null) { - ModelState.AddModelError("AdminUsername", String.Format(CultureInfo.CurrentCulture, + ModelState.AddModelError(string.Empty, String.Format(CultureInfo.CurrentCulture, Strings.TransformAccount_AdminAccountDoesNotExist, transformViewModel.AdminUsername)); return View(transformViewModel); } if (!UserService.CanTransformUserToOrganization(accountToTransform, adminUser, out var errorReason)) { - ModelState.AddModelError("AdminUsername", errorReason); + ModelState.AddModelError(string.Empty, errorReason); return View(transformViewModel); } @@ -420,10 +420,10 @@ public virtual async Task ForgotPassword(ForgotPasswordViewModel m switch (result.Type) { case PasswordResetResultType.UserNotConfirmed: - ModelState.AddModelError("Email", Strings.UserIsNotYetConfirmed); + ModelState.AddModelError(string.Empty, Strings.UserIsNotYetConfirmed); break; case PasswordResetResultType.UserNotFound: - ModelState.AddModelError("Email", Strings.CouldNotFindAnyoneWithThatUsernameOrEmail); + ModelState.AddModelError(string.Empty, Strings.CouldNotFindAnyoneWithThatUsernameOrEmail); break; case PasswordResetResultType.Success: return SendPasswordResetEmail(result.User, forgotPassword: true); @@ -481,7 +481,7 @@ public virtual async Task ResetPassword(string username, string to } catch (InvalidOperationException ex) { - ModelState.AddModelError("", ex.Message); + ModelState.AddModelError(string.Empty, ex.Message); return View(model); } @@ -489,7 +489,7 @@ public virtual async Task ResetPassword(string username, string to if (!ViewBag.ResetTokenValid) { - ModelState.AddModelError("", Strings.InvalidOrExpiredPasswordResetToken); + ModelState.AddModelError(string.Empty, Strings.InvalidOrExpiredPasswordResetToken); return View(model); } diff --git a/src/NuGetGallery/ExtensionMethods.cs b/src/NuGetGallery/ExtensionMethods.cs index c5f2a7c542..09e20f2b5e 100644 --- a/src/NuGetGallery/ExtensionMethods.cs +++ b/src/NuGetGallery/ExtensionMethods.cs @@ -345,15 +345,24 @@ public static HtmlString ShowValidationMessagesFor(this HtmlH var metadata = ModelMetadata.FromLambdaExpression(expression, html.ViewData); var propertyName = metadata.PropertyName.ToLower(); - return html.ValidationMessageFor(expression, validationMessage: null, htmlAttributes: new Dictionary + return html.ValidationMessageFor(expression, validationMessage: null, htmlAttributes: new Dictionary(ValidationHtmlAttributes) { { "id", $"{propertyName}-validation-message" }, - { "class", "help-block" }, - { "role", "alert" }, - { "aria-live", "assertive" }, }); } + public static MvcHtmlString ShowValidationMessagesForEmpty(this HtmlHelper html) + { + return html.ValidationMessage(modelName: string.Empty, htmlAttributes: ValidationHtmlAttributes); + } + + private static IDictionary ValidationHtmlAttributes = new Dictionary + { + { "class", "help-block" }, + { "role", "alert" }, + { "aria-live", "assertive" }, + }; + public static string ToShortNameOrNull(this NuGetFramework frameworkName) { if (frameworkName == null) diff --git a/src/NuGetGallery/ViewModels/PasswordResetRequestViewModel.cs b/src/NuGetGallery/ViewModels/PasswordResetRequestViewModel.cs index 7f214a0684..d94c0aa6be 100644 --- a/src/NuGetGallery/ViewModels/PasswordResetRequestViewModel.cs +++ b/src/NuGetGallery/ViewModels/PasswordResetRequestViewModel.cs @@ -7,6 +7,7 @@ namespace NuGetGallery public class ForgotPasswordViewModel { [Required] + [StringLength(255)] [Display(Name = "Email")] public string Email { get; set; } } diff --git a/src/NuGetGallery/ViewModels/TransformAccountViewModel.cs b/src/NuGetGallery/ViewModels/TransformAccountViewModel.cs index a09a7107d2..fd3f4f2ca3 100644 --- a/src/NuGetGallery/ViewModels/TransformAccountViewModel.cs +++ b/src/NuGetGallery/ViewModels/TransformAccountViewModel.cs @@ -8,7 +8,7 @@ namespace NuGetGallery public class TransformAccountViewModel { [Required] - [StringLength(255)] + [StringLength(64)] [Display(Name = "Administrator")] public string AdminUsername { get; set; } } diff --git a/src/NuGetGallery/Views/Users/ForgotPassword.cshtml b/src/NuGetGallery/Views/Users/ForgotPassword.cshtml index d67d7c7445..a2eb5b8e90 100644 --- a/src/NuGetGallery/Views/Users/ForgotPassword.cshtml +++ b/src/NuGetGallery/Views/Users/ForgotPassword.cshtml @@ -26,6 +26,9 @@ @Html.ShowTextBoxFor(m => m.Email) @Html.ShowValidationMessagesFor(m => m.Email) +
+ @Html.ShowValidationMessagesForEmpty() +
diff --git a/src/NuGetGallery/Views/Users/Transform.cshtml b/src/NuGetGallery/Views/Users/Transform.cshtml index f5fd1a5bec..1d8cdab0e9 100644 --- a/src/NuGetGallery/Views/Users/Transform.cshtml +++ b/src/NuGetGallery/Views/Users/Transform.cshtml @@ -45,6 +45,9 @@ @Html.ShowTextBoxFor(m => m.AdminUsername) @Html.ShowValidationMessagesFor(m => m.AdminUsername) +
+ @Html.ShowValidationMessagesForEmpty() +
diff --git a/tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs index 7358e6495d..847ee24706 100644 --- a/tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs @@ -283,7 +283,7 @@ public async Task ShowsErrorIfUserWasNotFound() Assert.NotNull(result); Assert.IsNotType(typeof(RedirectResult), result); - Assert.Contains(Strings.CouldNotFindAnyoneWithThatUsernameOrEmail, result.ViewData.ModelState["Email"].Errors.Select(e => e.ErrorMessage)); + Assert.Contains(Strings.CouldNotFindAnyoneWithThatUsernameOrEmail, result.ViewData.ModelState[string.Empty].Errors.Select(e => e.ErrorMessage)); } [Fact] @@ -301,7 +301,7 @@ public async Task ShowsErrorIfUnconfirmedAccount() Assert.NotNull(result); Assert.IsNotType(typeof(RedirectResult), result); - Assert.Contains(Strings.UserIsNotYetConfirmed, result.ViewData.ModelState["Email"].Errors.Select(e => e.ErrorMessage)); + Assert.Contains(Strings.UserIsNotYetConfirmed, result.ViewData.ModelState[string.Empty].Errors.Select(e => e.ErrorMessage)); } [Fact] @@ -2275,8 +2275,8 @@ public async Task WhenCanTransformReturnsFalse_ShowsError() // Assert Assert.NotNull(result); - Assert.Equal(1, controller.ModelState["AdminUsername"].Errors.Count); - Assert.Equal("error", controller.ModelState["AdminUsername"].Errors.First().ErrorMessage); + Assert.Equal(1, controller.ModelState[string.Empty].Errors.Count); + Assert.Equal("error", controller.ModelState[string.Empty].Errors.First().ErrorMessage); } [Fact] @@ -2294,11 +2294,11 @@ public async Task WhenAdminIsNotFound_ShowsError() // Assert Assert.NotNull(result); - Assert.Equal(1, controller.ModelState["AdminUsername"].Errors.Count); + Assert.Equal(1, controller.ModelState[string.Empty].Errors.Count); Assert.Equal( String.Format(CultureInfo.CurrentCulture, Strings.TransformAccount_AdminAccountDoesNotExist, "AdminThatDoesNotExist"), - controller.ModelState["AdminUsername"].Errors.First().ErrorMessage); + controller.ModelState[string.Empty].Errors.First().ErrorMessage); } [Fact] From 433de7c658f67f4535f28765ee05425bc7b6ac4b Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Wed, 21 Feb 2018 09:21:56 -0800 Subject: [PATCH 02/24] Enable reflowing of packages that don't pass valid version range rule (#5524) Fix https://github.com/NuGet/NuGetGallery/issues/5522 --- .../Packaging/ManifestValidator.cs | 2 +- .../Packaging/PackageMetadata.cs | 7 +- .../Controllers/PackagesController.cs | 9 +- src/NuGetGallery/Services/PackageService.cs | 8 +- .../Services/ReflowPackageService.cs | 4 +- .../Packaging/PackageMetadataFacts.cs | 38 +++++++- .../Controllers/ApiControllerFacts.cs | 4 +- .../Services/PackageUploadServiceFacts.cs | 4 +- .../Services/ReflowPackageServiceFacts.cs | 92 +++++++++++++++++-- 9 files changed, 146 insertions(+), 22 deletions(-) diff --git a/src/NuGetGallery.Core/Packaging/ManifestValidator.cs b/src/NuGetGallery.Core/Packaging/ManifestValidator.cs index 024bdca0fd..167f03ee17 100644 --- a/src/NuGetGallery.Core/Packaging/ManifestValidator.cs +++ b/src/NuGetGallery.Core/Packaging/ManifestValidator.cs @@ -22,7 +22,7 @@ public static IEnumerable Validate(Stream nuspecStream, out Nu var rawMetadata = nuspecReader.GetMetadata(); if (rawMetadata != null && rawMetadata.Any()) { - return ValidateCore(PackageMetadata.FromNuspecReader(nuspecReader)); + return ValidateCore(PackageMetadata.FromNuspecReader(nuspecReader, strict: true)); } } catch (Exception ex) diff --git a/src/NuGetGallery.Core/Packaging/PackageMetadata.cs b/src/NuGetGallery.Core/Packaging/PackageMetadata.cs index dacf48be29..b39f457400 100644 --- a/src/NuGetGallery.Core/Packaging/PackageMetadata.cs +++ b/src/NuGetGallery.Core/Packaging/PackageMetadata.cs @@ -147,15 +147,18 @@ private Uri GetValue(string key, Uri alternateValue) /// Gets package metadata from a the provided instance. /// /// The instance used to read the + /// + /// Whether or not to be strict when reading the . This should be true + /// on initial ingestion but false when a package that has already been accepted is being processed. /// /// We default to use a strict version-check on dependency groups. /// When an invalid dependency version range is detected, a will be thrown. /// - public static PackageMetadata FromNuspecReader(NuspecReader nuspecReader) + public static PackageMetadata FromNuspecReader(NuspecReader nuspecReader, bool strict) { return new PackageMetadata( nuspecReader.GetMetadata().ToDictionary(kvp => kvp.Key, kvp => kvp.Value), - nuspecReader.GetDependencyGroups(useStrictVersionCheck: true), + nuspecReader.GetDependencyGroups(useStrictVersionCheck: strict), nuspecReader.GetFrameworkReferenceGroups(), nuspecReader.GetPackageTypes(), nuspecReader.GetMinClientVersion() diff --git a/src/NuGetGallery/Controllers/PackagesController.cs b/src/NuGetGallery/Controllers/PackagesController.cs index 9391ac7f7d..adabfa170f 100644 --- a/src/NuGetGallery/Controllers/PackagesController.cs +++ b/src/NuGetGallery/Controllers/PackagesController.cs @@ -169,7 +169,8 @@ public async virtual Task UploadPackage() try { packageMetadata = PackageMetadata.FromNuspecReader( - package.GetNuspecReader()); + package.GetNuspecReader(), + strict: true); } catch (Exception ex) { @@ -407,7 +408,8 @@ public virtual async Task UploadPackage(HttpPostedFileBase uploadFil try { packageMetadata = PackageMetadata.FromNuspecReader( - package.GetNuspecReader()); + package.GetNuspecReader(), + strict: true); } catch (Exception ex) { @@ -1545,7 +1547,8 @@ public virtual async Task VerifyPackage(VerifyPackageRequest formDat Debug.Assert(nugetPackage != null); var packageMetadata = PackageMetadata.FromNuspecReader( - nugetPackage.GetNuspecReader()); + nugetPackage.GetNuspecReader(), + strict: true); // Rule out problem scenario with multiple tabs - verification request (possibly with edits) was submitted by user // viewing a different package to what was actually most recently uploaded diff --git a/src/NuGetGallery/Services/PackageService.cs b/src/NuGetGallery/Services/PackageService.cs index 4b992a452a..e6cb346922 100644 --- a/src/NuGetGallery/Services/PackageService.cs +++ b/src/NuGetGallery/Services/PackageService.cs @@ -45,7 +45,9 @@ public void EnsureValid(PackageArchiveReader packageArchiveReader) { try { - var packageMetadata = PackageMetadata.FromNuspecReader(packageArchiveReader.GetNuspecReader()); + var packageMetadata = PackageMetadata.FromNuspecReader( + packageArchiveReader.GetNuspecReader(), + strict: true); ValidateNuGetPackageMetadata(packageMetadata); @@ -82,7 +84,9 @@ public async Task CreatePackageAsync(PackageArchiveReader nugetPackage, try { - packageMetadata = PackageMetadata.FromNuspecReader(nugetPackage.GetNuspecReader()); + packageMetadata = PackageMetadata.FromNuspecReader( + nugetPackage.GetNuspecReader(), + strict: true); ValidateNuGetPackageMetadata(packageMetadata); diff --git a/src/NuGetGallery/Services/ReflowPackageService.cs b/src/NuGetGallery/Services/ReflowPackageService.cs index 87c3113aab..44c9acec1e 100644 --- a/src/NuGetGallery/Services/ReflowPackageService.cs +++ b/src/NuGetGallery/Services/ReflowPackageService.cs @@ -50,7 +50,9 @@ public async Task ReflowAsync(string id, string version) Size = packageStream.Length, }; - var packageMetadata = PackageMetadata.FromNuspecReader(packageArchive.GetNuspecReader()); + var packageMetadata = PackageMetadata.FromNuspecReader( + packageArchive.GetNuspecReader(), + strict: false); // 3) Clear referenced objects that will be reflowed ClearSupportedFrameworks(package); diff --git a/tests/NuGetGallery.Core.Facts/Packaging/PackageMetadataFacts.cs b/tests/NuGetGallery.Core.Facts/Packaging/PackageMetadataFacts.cs index 5bdca379d8..84f2cbebcd 100644 --- a/tests/NuGetGallery.Core.Facts/Packaging/PackageMetadataFacts.cs +++ b/tests/NuGetGallery.Core.Facts/Packaging/PackageMetadataFacts.cs @@ -23,7 +23,9 @@ public static void CanReadBasicMetadataProperties() var nuspec = nupkg.GetNuspecReader(); // Act - var packageMetadata = PackageMetadata.FromNuspecReader(nuspec); + var packageMetadata = PackageMetadata.FromNuspecReader( + nuspec, + strict: true); // Assert Assert.Equal("TestPackage", packageMetadata.Id); @@ -47,7 +49,39 @@ public static void ThrowsPackagingExceptionWhenInvalidDepencencyVersionRangeDete var nuspec = nupkg.GetNuspecReader(); // Act & Assert - Assert.Throws(() => PackageMetadata.FromNuspecReader(nuspec)); + Assert.Throws(() => PackageMetadata.FromNuspecReader( + nuspec, + strict: true)); + } + + [Fact] + public static void DoesNotThrowInvalidDepencencyVersionRangeDetectedAndParsingIsNotStrict() + { + var packageStream = CreateTestPackageStreamWithInvalidDependencyVersion(); + var nupkg = new PackageArchiveReader(packageStream, leaveStreamOpen: false); + var nuspec = nupkg.GetNuspecReader(); + + // Act + var packageMetadata = PackageMetadata.FromNuspecReader( + nuspec, + strict: false); + + // Assert + Assert.Equal("TestPackage", packageMetadata.Id); + Assert.Equal(NuGetVersion.Parse("0.0.0.1"), packageMetadata.Version); + Assert.Equal("Package A", packageMetadata.Title); + Assert.Equal(2, packageMetadata.Authors.Count); + Assert.Equal("ownera, ownerb", packageMetadata.Owners); + Assert.False(packageMetadata.RequireLicenseAcceptance); + Assert.Equal("package A description.", packageMetadata.Description); + Assert.Equal("en-US", packageMetadata.Language); + Assert.Equal("http://www.nuget.org/", packageMetadata.ProjectUrl.ToString()); + Assert.Equal("http://www.nuget.org/", packageMetadata.IconUrl.ToString()); + Assert.Equal("http://www.nuget.org/", packageMetadata.LicenseUrl.ToString()); + var dependencyGroup = Assert.Single(packageMetadata.GetDependencyGroups()); + var dependency = Assert.Single(dependencyGroup.Packages); + Assert.Equal("SampleDependency", dependency.Id); + Assert.Equal(VersionRange.All, dependency.VersionRange); } private static Stream CreateTestPackageStream() diff --git a/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs index dad3a64e66..93add14028 100644 --- a/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs @@ -96,7 +96,9 @@ public TestableApiController( MockPackageUploadService.Setup(x => x.GeneratePackageAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) .Returns((string id, PackageArchiveReader nugetPackage, PackageStreamMetadata packageStreamMetadata, User owner, User currentUser) => { - var packageMetadata = PackageMetadata.FromNuspecReader(nugetPackage.GetNuspecReader()); + var packageMetadata = PackageMetadata.FromNuspecReader( + nugetPackage.GetNuspecReader(), + strict: true); var package = new Package(); package.PackageRegistration = new PackageRegistration { Id = packageMetadata.Id, IsVerified = false }; diff --git a/tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs index 9f99dcc6ef..a9329ddfbf 100644 --- a/tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs @@ -36,7 +36,9 @@ private static PackageUploadService CreateService( .CreatePackageAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) .Returns((PackageArchiveReader packageArchiveReader, PackageStreamMetadata packageStreamMetadata, User owner, User currentUser, bool isVerified) => { - var packageMetadata = PackageMetadata.FromNuspecReader(packageArchiveReader.GetNuspecReader()); + var packageMetadata = PackageMetadata.FromNuspecReader( + packageArchiveReader.GetNuspecReader(), + strict: true); var newPackage = new Package(); newPackage.PackageRegistration = new PackageRegistration { Id = packageMetadata.Id, IsVerified = isVerified }; diff --git a/tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs b/tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs index ce437df4b3..5d0ce1e86b 100644 --- a/tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs @@ -262,6 +262,46 @@ public async Task CallsUpdateIsLatestAsync() // Assert packageService.Verify(s => s.UpdateIsLatestAsync(package.PackageRegistration, false), Times.Once); } + + [Fact] + public async Task AllowsInvalidPackageDependencyVersion() + { + // Arrange + var package = PackageServiceUtility.CreateTestPackage(); + + var packageService = SetupPackageService(package); + var entitiesContext = SetupEntitiesContext(); + var packageFileService = SetupPackageFileService( + package, + CreateInvalidDependencyVersionTestPackageStream()); + + var service = CreateService( + packageService: packageService, + entitiesContext: entitiesContext, + packageFileService: packageFileService); + + // Act + var result = await service.ReflowAsync("test", "1.0.0"); + + // Assert + Assert.Equal("test", result.PackageRegistration.Id); + Assert.Equal("1.0.0", result.NormalizedVersion); + + Assert.True(result.Dependencies.Any(d => + d.Id == "WebActivator" + && d.VersionSpec == "(, )" + && d.TargetFramework == "net40")); + + Assert.True(result.Dependencies.Any(d => + d.Id == "PackageC" + && d.VersionSpec == "[1.1.0, 2.0.1)" + && d.TargetFramework == "net40")); + + Assert.True(result.Dependencies.Any(d => + d.Id == "jQuery" + && d.VersionSpec == "(, )" + && d.TargetFramework == "net451")); + } } private static Mock SetupPackageService(Package package) @@ -325,27 +365,50 @@ private static Mock SetupEntitiesContext() return entitiesContext; } - private static Mock SetupPackageFileService(Package package) + private static Mock SetupPackageFileService(Package package, Stream packageStream = null) { var packageFileService = new Mock(); packageFileService .Setup(s => s.DownloadPackageFileAsync(package)) - .Returns(Task.FromResult(CreateTestPackageStream())) + .Returns(Task.FromResult(packageStream ?? CreateTestPackageStream())) .Verifiable(); return packageFileService; } + private static Stream CreateInvalidDependencyVersionTestPackageStream() + { + return CreateTestPackageStream(@" + + + test + 1.0.0 + Test package + authora, authorb + ownera + false + package A description. + en-US + http://www.nuget.org/ + http://www.nuget.org/ + http://www.nuget.org/ + + + + + + + + + + + "); + } + private static Stream CreateTestPackageStream() { - var packageStream = new MemoryStream(); - using (var packageArchive = new ZipArchive(packageStream, ZipArchiveMode.Create, true)) - { - var nuspecEntry = packageArchive.CreateEntry("TestPackage.nuspec", CompressionLevel.Fastest); - using (var streamWriter = new StreamWriter(nuspecEntry.Open())) - { - streamWriter.WriteLine(@" + return CreateTestPackageStream(@" test @@ -370,6 +433,17 @@ private static Stream CreateTestPackageStream() "); + } + + private static Stream CreateTestPackageStream(string nuspec) + { + var packageStream = new MemoryStream(); + using (var packageArchive = new ZipArchive(packageStream, ZipArchiveMode.Create, true)) + { + var nuspecEntry = packageArchive.CreateEntry("TestPackage.nuspec", CompressionLevel.Fastest); + using (var streamWriter = new StreamWriter(nuspecEntry.Open())) + { + streamWriter.WriteLine(nuspec); } packageArchive.CreateEntry("content\\HelloWorld.cs", CompressionLevel.Fastest); From d0a8c6183ba70309b5d55770cf43cca124e90c7c Mon Sep 17 00:00:00 2001 From: Scott Bommarito Date: Tue, 27 Feb 2018 08:28:37 -0800 Subject: [PATCH 03/24] Remove redundant TempData["Message"] when changing email (#5499) --- .../Controllers/AccountsController.cs | 10 ------ .../Controllers/OrganizationsController.cs | 4 +-- .../Controllers/UsersController.cs | 4 +-- src/NuGetGallery/Strings.Designer.cs | 36 ------------------- src/NuGetGallery/Strings.resx | 12 ------- .../Controllers/AccountsControllerFacts.cs | 4 --- 6 files changed, 2 insertions(+), 68 deletions(-) diff --git a/src/NuGetGallery/Controllers/AccountsController.cs b/src/NuGetGallery/Controllers/AccountsController.cs index 5f581ace99..031042743f 100644 --- a/src/NuGetGallery/Controllers/AccountsController.cs +++ b/src/NuGetGallery/Controllers/AccountsController.cs @@ -23,10 +23,6 @@ public class ViewMessages public string EmailPreferencesUpdated { get; set; } public string EmailUpdateCancelled { get; set; } - - public string EmailUpdated { get; set; } - - public string EmailUpdatedWithConfirmationRequired { get; set; } } public AuthenticationService AuthenticationService { get; } @@ -237,12 +233,6 @@ public virtual async Task ChangeEmail(TAccountViewModel model) if (account.Confirmed) { SendEmailChangedConfirmationNotice(account); - - TempData["Message"] = Messages.EmailUpdatedWithConfirmationRequired; - } - else - { - TempData["Message"] = Messages.EmailUpdated; } return RedirectToAction(AccountAction); diff --git a/src/NuGetGallery/Controllers/OrganizationsController.cs b/src/NuGetGallery/Controllers/OrganizationsController.cs index b0a7a418d3..f728d4a3fa 100644 --- a/src/NuGetGallery/Controllers/OrganizationsController.cs +++ b/src/NuGetGallery/Controllers/OrganizationsController.cs @@ -29,9 +29,7 @@ public OrganizationsController( { EmailConfirmed = Strings.OrganizationEmailConfirmed, EmailPreferencesUpdated = Strings.OrganizationEmailPreferencesUpdated, - EmailUpdateCancelled = Strings.OrganizationEmailUpdateCancelled, - EmailUpdated = Strings.OrganizationEmailUpdated, - EmailUpdatedWithConfirmationRequired = Strings.OrganizationEmailUpdatedWithConfirmationRequired + EmailUpdateCancelled = Strings.OrganizationEmailUpdateCancelled }; protected override void SendNewAccountEmail(User account) diff --git a/src/NuGetGallery/Controllers/UsersController.cs b/src/NuGetGallery/Controllers/UsersController.cs index f608a47b4b..36bd75c3a5 100644 --- a/src/NuGetGallery/Controllers/UsersController.cs +++ b/src/NuGetGallery/Controllers/UsersController.cs @@ -56,9 +56,7 @@ public UsersController( { EmailConfirmed = Strings.UserEmailConfirmed, EmailPreferencesUpdated = Strings.UserEmailPreferencesUpdated, - EmailUpdateCancelled = Strings.UserEmailUpdateCancelled, - EmailUpdated = Strings.UserEmailUpdated, - EmailUpdatedWithConfirmationRequired = Strings.UserEmailUpdatedWithConfirmationRequired + EmailUpdateCancelled = Strings.UserEmailUpdateCancelled }; protected override void SendNewAccountEmail(User account) diff --git a/src/NuGetGallery/Strings.Designer.cs b/src/NuGetGallery/Strings.Designer.cs index b88108c7b4..53959e582f 100644 --- a/src/NuGetGallery/Strings.Designer.cs +++ b/src/NuGetGallery/Strings.Designer.cs @@ -1021,24 +1021,6 @@ public static string OrganizationEmailUpdateCancelled { } } - /// - /// Looks up a localized string similar to The new organization email address was saved!. - /// - public static string OrganizationEmailUpdated { - get { - return ResourceManager.GetString("OrganizationEmailUpdated", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to The organization email address has been changed! We sent a confirmation email to verify the new email. When the new email address is confirmed, it will take effect and we will forget the old one.. - /// - public static string OrganizationEmailUpdatedWithConfirmationRequired { - get { - return ResourceManager.GetString("OrganizationEmailUpdatedWithConfirmationRequired", resourceCulture); - } - } - /// /// Looks up a localized string similar to Member name is required.. /// @@ -1862,24 +1844,6 @@ public static string UserEmailUpdateCancelled { } } - /// - /// Looks up a localized string similar to Your new email address was saved!. - /// - public static string UserEmailUpdated { - get { - return ResourceManager.GetString("UserEmailUpdated", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to Your email address has been changed! We sent a confirmation email to verify your new email. When you confirm the new email address, it will take effect and we will forget the old one.. - /// - public static string UserEmailUpdatedWithConfirmationRequired { - get { - return ResourceManager.GetString("UserEmailUpdatedWithConfirmationRequired", resourceCulture); - } - } - /// /// Looks up a localized string similar to You cannot reset your password until you confirm your account.. /// diff --git a/src/NuGetGallery/Strings.resx b/src/NuGetGallery/Strings.resx index d6007d4301..82edeac578 100644 --- a/src/NuGetGallery/Strings.resx +++ b/src/NuGetGallery/Strings.resx @@ -294,12 +294,6 @@ The {1} Team Your email preferences have been updated. - - Your new email address was saved! - - - Your email address has been changed! We sent a confirmation email to verify your new email. When you confirm the new email address, it will take effect and we will forget the old one. - This package requires version '{0}' of NuGet, which this gallery does not currently support. Please contact us if you have questions. @@ -774,12 +768,6 @@ If you wish to update the linked Microsoft account you can do so from the accoun You canceled your organization email address change request. - - The new organization email address was saved! - - - The organization email address has been changed! We sent a confirmation email to verify the new email. When the new email address is confirmed, it will take effect and we will forget the old one. - You have successfully confirmed your email address! diff --git a/tests/NuGetGallery.Facts/Controllers/AccountsControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/AccountsControllerFacts.cs index 207f2a8aa9..e3cec9f2e3 100644 --- a/tests/NuGetGallery.Facts/Controllers/AccountsControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/AccountsControllerFacts.cs @@ -234,8 +234,6 @@ public virtual async Task WhenNewEmailIsDifferentAndWasConfirmed_SavesChanges() GetMock().Verify(u => u.ChangeEmailAddress(It.IsAny(), It.IsAny()), Times.Once); ResultAssert.IsRedirectToRoute(result, new { action = controller.AccountAction }); - Assert.Equal(controller.Messages.EmailUpdatedWithConfirmationRequired, controller.TempData["Message"]); - GetMock() .Verify(m => m.SendEmailChangeConfirmationNotice(It.IsAny(), It.IsAny()), Times.Once); @@ -260,8 +258,6 @@ public virtual async Task WhenNewEmailIsDifferentAndWasUnconfirmed_SavesChanges( GetMock().Verify(u => u.ChangeEmailAddress(It.IsAny(), It.IsAny()), Times.Once); ResultAssert.IsRedirectToRoute(result, new { action = controller.AccountAction }); - Assert.Equal(controller.Messages.EmailUpdated, controller.TempData["Message"]); - GetMock() .Verify(m => m.SendEmailChangeConfirmationNotice(It.IsAny(), It.IsAny()), Times.Never); From 72090c614d428e2c76e7579ca3939349ce2f9c82 Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Mon, 26 Feb 2018 16:47:57 -0800 Subject: [PATCH 04/24] Add UpdatePackageStreamMetadataAsync to keep stream details up to date (#5548) Address https://github.com/NuGet/Engineering/issues/1191 --- .../Services/CorePackageService.cs | 36 ++++++ .../Services/ICorePackageService.cs | 9 ++ .../Services/CorePackageServiceFacts.cs | 107 ++++++++++++++++++ 3 files changed, 152 insertions(+) diff --git a/src/NuGetGallery.Core/Services/CorePackageService.cs b/src/NuGetGallery.Core/Services/CorePackageService.cs index e214acf5fe..fcc547f183 100644 --- a/src/NuGetGallery.Core/Services/CorePackageService.cs +++ b/src/NuGetGallery.Core/Services/CorePackageService.cs @@ -6,6 +6,7 @@ using System.Linq; using System.Threading.Tasks; using NuGet.Versioning; +using NuGetGallery.Packaging; namespace NuGetGallery { @@ -18,6 +19,41 @@ public CorePackageService(IEntityRepository packageRepository) _packageRepository = packageRepository ?? throw new ArgumentNullException(nameof(packageRepository)); } + public virtual async Task UpdatePackageStreamMetadataAsync( + Package package, + PackageStreamMetadata metadata, + bool commitChanges = true) + { + if (package == null) + { + throw new ArgumentNullException(nameof(package)); + } + + if (metadata == null) + { + throw new ArgumentNullException(nameof(metadata)); + } + + package.Hash = metadata.Hash; + package.HashAlgorithm = metadata.HashAlgorithm; + package.PackageFileSize = metadata.Size; + + var now = DateTime.UtcNow; + package.LastUpdated = now; + + /// If the package is available, consider this change as an "edit" so that the package appears for cursors + /// on the field. + if (package.PackageStatusKey == PackageStatus.Available) + { + package.LastEdited = now; + } + + if (commitChanges) + { + await _packageRepository.CommitChangesAsync(); + } + } + public virtual async Task UpdatePackageStatusAsync( Package package, PackageStatus newPackageStatus, diff --git a/src/NuGetGallery.Core/Services/ICorePackageService.cs b/src/NuGetGallery.Core/Services/ICorePackageService.cs index 993d7584c3..66799cfd39 100644 --- a/src/NuGetGallery.Core/Services/ICorePackageService.cs +++ b/src/NuGetGallery.Core/Services/ICorePackageService.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Threading.Tasks; +using NuGetGallery.Packaging; namespace NuGetGallery { @@ -10,6 +11,14 @@ namespace NuGetGallery /// public interface ICorePackageService { + /// + /// Updates the package properties related to the package stream itself. + /// + /// The package to update the stream details of. + /// The new package stream metadata. + /// Whether or not to commit the changes to the entity context. + Task UpdatePackageStreamMetadataAsync(Package package, PackageStreamMetadata metadata, bool commitChanges = true); + /// /// Set the status on the package and any other related package properties. /// diff --git a/tests/NuGetGallery.Core.Facts/Services/CorePackageServiceFacts.cs b/tests/NuGetGallery.Core.Facts/Services/CorePackageServiceFacts.cs index 97c0ddc934..b2d4dd9b99 100644 --- a/tests/NuGetGallery.Core.Facts/Services/CorePackageServiceFacts.cs +++ b/tests/NuGetGallery.Core.Facts/Services/CorePackageServiceFacts.cs @@ -5,12 +5,119 @@ using System.Collections.Generic; using System.Threading.Tasks; using Moq; +using NuGetGallery.Packaging; using Xunit; namespace NuGetGallery { public class CorePackageServiceFacts { + public class TheUpdatePackageStreamMetadataMethod + { + [Fact] + public async Task RejectsNullPackage() + { + // Arrange + Package package = null; + var metadata = new PackageStreamMetadata(); + var service = CreateService(); + + // Act & Assert + await Assert.ThrowsAsync( + () => service.UpdatePackageStreamMetadataAsync(package, metadata, commitChanges: true)); + } + + [Fact] + public async Task RejectsNullStreamMetadata() + { + // Arrange + var package = new Package(); + PackageStreamMetadata metadata = null; + var service = CreateService(); + + // Act & Assert + await Assert.ThrowsAsync( + () => service.UpdatePackageStreamMetadataAsync(package, metadata, commitChanges: true)); + } + + [Theory] + [InlineData(false, 0)] + [InlineData(true, 1)] + public async Task CommitsTheCorrectNumberOfTimes(bool commitChanges, int commitCount) + { + // Arrange + var packageRepository = new Mock>(); + var package = new Package(); + var metadata = new PackageStreamMetadata(); + var service = CreateService(packageRepository: packageRepository); + + // Act + await service.UpdatePackageStreamMetadataAsync(package, metadata, commitChanges); + + // Assert + packageRepository.Verify(x => x.CommitChangesAsync(), Times.Exactly(commitCount)); + } + + [Fact] + public async Task UpdatesTheStreamMetadata() + { + // Arrange + var package = new Package + { + Hash = "hash-before", + HashAlgorithm = "hash-algorithm-before", + PackageFileSize = 23, + LastUpdated = new DateTime(2017, 1, 1, 8, 30, 0), + LastEdited = new DateTime(2017, 1, 1, 7, 30, 0), + PackageStatusKey = PackageStatus.Available, + }; + var metadata = new PackageStreamMetadata + { + Hash = "hash-after", + HashAlgorithm = "hash-algorithm-after", + Size = 42, + }; + var service = CreateService(); + + // Act + var before = DateTime.UtcNow; + await service.UpdatePackageStreamMetadataAsync(package, metadata, commitChanges: true); + var after = DateTime.UtcNow; + + // Assert + Assert.Equal("hash-after", package.Hash); + Assert.Equal("hash-algorithm-after", package.HashAlgorithm); + Assert.Equal(42, package.PackageFileSize); + Assert.InRange(package.LastUpdated, before, after); + Assert.NotNull(package.LastEdited); + Assert.InRange(package.LastEdited.Value, before, after); + Assert.Equal(package.LastUpdated, package.LastEdited); + } + + [Theory] + [InlineData(PackageStatus.Deleted)] + [InlineData(PackageStatus.Validating)] + [InlineData(PackageStatus.FailedValidation)] + public async Task DoesNotUpdateLastEditedWhenNotAvailable(PackageStatus packageStatus) + { + // Arrange + var originalLastEdited = new DateTime(2017, 1, 1, 7, 30, 0); + var package = new Package + { + LastEdited = originalLastEdited, + PackageStatusKey = packageStatus, + }; + var metadata = new PackageStreamMetadata(); + var service = CreateService(); + + // Act + await service.UpdatePackageStreamMetadataAsync(package, metadata, commitChanges: true); + + // Assert + Assert.Equal(originalLastEdited, package.LastEdited); + } + } + public class TheUpdatePackageStatusMethod { [Fact] From c2eac1f283729e90ade78ff497dd4c7bf5f71c4e Mon Sep 17 00:00:00 2001 From: Svetlana Kofman Date: Tue, 27 Feb 2018 16:59:53 -0800 Subject: [PATCH 05/24] ApiKey hash tool - fix whatif flag (#5554) --- src/GalleryTools/Commands/HashCommand.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/GalleryTools/Commands/HashCommand.cs b/src/GalleryTools/Commands/HashCommand.cs index 7c0b91682d..43f44870ed 100644 --- a/src/GalleryTools/Commands/HashCommand.cs +++ b/src/GalleryTools/Commands/HashCommand.cs @@ -63,7 +63,7 @@ private static async Task Hash(string connectionString, bool whatIf) do { - batch = allCredentials.Where(predicate).Take(BatchSize).ToList(); + batch = allCredentials.Where(predicate).OrderBy(x => x.Key).Take(BatchSize).ToList(); if (batch.Count > 0) { @@ -97,6 +97,7 @@ private static async Task Hash(string connectionString, bool whatIf) else { Console.WriteLine("Skipping DB save."); + allCredentials = allCredentials.Where(predicate).OrderBy(x => x.Key).Skip(batch.Count); } batchNumber++; From 8eacea69c81eb9eeeaf3389a93c6e3fe46137ee5 Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 23 Feb 2018 14:26:59 -0800 Subject: [PATCH 06/24] Add telemetry for unlist, list, delete, reflow, and revalidate (#5540) Progress on https://github.com/NuGet/NuGetGallery/pull/5540 --- .../Controllers/PackagesController.cs | 3 +- src/NuGetGallery/Services/ITelemetryClient.cs | 2 - .../Services/ITelemetryService.cs | 12 ++ .../Services/PackageDeleteService.cs | 11 +- src/NuGetGallery/Services/PackageService.cs | 9 +- .../Services/ReflowPackageService.cs | 16 +- .../Services/TelemetryClientWrapper.cs | 12 -- src/NuGetGallery/Services/TelemetryService.cs | 203 +++++++++++++----- .../Services/ValidationService.cs | 7 +- .../Services/PackageDeleteServiceFacts.cs | 50 ++++- .../Services/PackageServiceFacts.cs | 35 ++- .../Services/ReflowPackageServiceFacts.cs | 35 ++- .../Services/TelemetryServiceFacts.cs | 57 +++-- .../Services/ValidationServiceFacts.cs | 15 +- .../TestUtils/TestServiceUtility.cs | 5 +- 15 files changed, 358 insertions(+), 114 deletions(-) diff --git a/src/NuGetGallery/Controllers/PackagesController.cs b/src/NuGetGallery/Controllers/PackagesController.cs index adabfa170f..d7e80f218a 100644 --- a/src/NuGetGallery/Controllers/PackagesController.cs +++ b/src/NuGetGallery/Controllers/PackagesController.cs @@ -1052,7 +1052,8 @@ public virtual async Task Reflow(string id, string version) var reflowPackageService = new ReflowPackageService( _entitiesContext, (PackageService)_packageService, - _packageFileService); + _packageFileService, + _telemetryService); try { diff --git a/src/NuGetGallery/Services/ITelemetryClient.cs b/src/NuGetGallery/Services/ITelemetryClient.cs index f0f481f997..99c7ff37f7 100644 --- a/src/NuGetGallery/Services/ITelemetryClient.cs +++ b/src/NuGetGallery/Services/ITelemetryClient.cs @@ -11,8 +11,6 @@ namespace NuGetGallery /// public interface ITelemetryClient { - void TrackEvent(string eventName, IDictionary properties = null, IDictionary metrics = null); - void TrackMetric(string metricName, double value, IDictionary properties = null); void TrackException(Exception exception, IDictionary properties = null, IDictionary metrics = null); diff --git a/src/NuGetGallery/Services/ITelemetryService.cs b/src/NuGetGallery/Services/ITelemetryService.cs index a094f2844a..0031abeac9 100644 --- a/src/NuGetGallery/Services/ITelemetryService.cs +++ b/src/NuGetGallery/Services/ITelemetryService.cs @@ -13,6 +13,18 @@ public interface ITelemetryService void TrackPackagePushEvent(Package package, User user, IIdentity identity); + void TrackPackageUnlisted(Package package); + + void TrackPackageListed(Package package); + + void TrackPackageDelete(Package package, bool isHardDelete); + + void TrackPackageReflow(Package package); + + void TrackPackageHardDeleteReflow(string packageId, string packageVersion); + + void TrackPackageRevalidate(Package package); + void TrackPackageReadMeChangeEvent(Package package, string readMeSourceType, PackageEditReadMeState readMeState); void TrackCreatePackageVerificationKeyEvent(string packageId, string packageVersion, User user, IIdentity identity); diff --git a/src/NuGetGallery/Services/PackageDeleteService.cs b/src/NuGetGallery/Services/PackageDeleteService.cs index da46b9e722..a0a516730d 100644 --- a/src/NuGetGallery/Services/PackageDeleteService.cs +++ b/src/NuGetGallery/Services/PackageDeleteService.cs @@ -270,6 +270,8 @@ await _packageService.UpdatePackageStatusAsync( packageDelete.Packages.Add(package); await _auditingService.SaveAuditRecordAsync(CreateAuditRecord(package, package.PackageRegistration, AuditedPackageAction.SoftDelete, reason)); + + _telemetryService.TrackPackageDelete(package, isHardDelete: false); } _packageDeletesRepository.InsertOnCommit(packageDelete); @@ -314,6 +316,8 @@ await ExecuteSqlCommandAsync(_entitiesContext.GetDatabase(), await _auditingService.SaveAuditRecordAsync(CreateAuditRecord(package, package.PackageRegistration, AuditedPackageAction.Delete, reason)); + _telemetryService.TrackPackageDelete(package, isHardDelete: true); + package.PackageRegistration.Packages.Remove(package); _packageRepository.DeleteOnCommit(package); } @@ -338,7 +342,7 @@ await ExecuteSqlCommandAsync(_entitiesContext.GetDatabase(), UpdateSearchIndex(); } - public Task ReflowHardDeletedPackageAsync(string id, string version) + public async Task ReflowHardDeletedPackageAsync(string id, string version) { if (string.IsNullOrEmpty(id)) { @@ -381,7 +385,10 @@ public Task ReflowHardDeletedPackageAsync(string id, string version) registrationRecord: null, action: AuditedPackageAction.Delete, reason: "reflow hard-deleted package"); - return _auditingService.SaveAuditRecordAsync(auditRecord); + + await _auditingService.SaveAuditRecordAsync(auditRecord); + + _telemetryService.TrackPackageHardDeleteReflow(normalizedId, normalizedVersionString); } protected virtual async Task ExecuteSqlCommandAsync(IDatabase database, string sql, params object[] parameters) diff --git a/src/NuGetGallery/Services/PackageService.cs b/src/NuGetGallery/Services/PackageService.cs index e6cb346922..a1a1c7fe61 100644 --- a/src/NuGetGallery/Services/PackageService.cs +++ b/src/NuGetGallery/Services/PackageService.cs @@ -20,16 +20,19 @@ public class PackageService : CorePackageService, IPackageService private readonly IEntityRepository _packageRegistrationRepository; private readonly IPackageNamingConflictValidator _packageNamingConflictValidator; private readonly IAuditingService _auditingService; + private readonly ITelemetryService _telemetryService; public PackageService( IEntityRepository packageRegistrationRepository, IEntityRepository packageRepository, IPackageNamingConflictValidator packageNamingConflictValidator, - IAuditingService auditingService) : base(packageRepository) + IAuditingService auditingService, + ITelemetryService telemetryService) : base(packageRepository) { _packageRegistrationRepository = packageRegistrationRepository ?? throw new ArgumentNullException(nameof(packageRegistrationRepository)); _packageNamingConflictValidator = packageNamingConflictValidator ?? throw new ArgumentNullException(nameof(packageNamingConflictValidator)); _auditingService = auditingService ?? throw new ArgumentNullException(nameof(auditingService)); + _telemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService)); } /// @@ -416,6 +419,8 @@ public async Task MarkPackageListedAsync(Package package, bool commitChanges = t await _auditingService.SaveAuditRecordAsync(new PackageAuditRecord(package, AuditedPackageAction.List)); + _telemetryService.TrackPackageListed(package); + if (commitChanges) { await _packageRepository.CommitChangesAsync(); @@ -445,6 +450,8 @@ public async Task MarkPackageUnlistedAsync(Package package, bool commitChanges = await _auditingService.SaveAuditRecordAsync(new PackageAuditRecord(package, AuditedPackageAction.Unlist)); + _telemetryService.TrackPackageUnlisted(package); + if (commitChanges) { await _packageRepository.CommitChangesAsync(); diff --git a/src/NuGetGallery/Services/ReflowPackageService.cs b/src/NuGetGallery/Services/ReflowPackageService.cs index 44c9acec1e..db75d65179 100644 --- a/src/NuGetGallery/Services/ReflowPackageService.cs +++ b/src/NuGetGallery/Services/ReflowPackageService.cs @@ -14,15 +14,18 @@ public class ReflowPackageService private readonly IEntitiesContext _entitiesContext; private readonly IPackageService _packageService; private readonly IPackageFileService _packageFileService; + private readonly ITelemetryService _telemetryService; public ReflowPackageService( IEntitiesContext entitiesContext, IPackageService packageService, - IPackageFileService packageFileService) + IPackageFileService packageFileService, + ITelemetryService telemetryService) { - _entitiesContext = entitiesContext; - _packageService = packageService; - _packageFileService = packageFileService; + _entitiesContext = entitiesContext ?? throw new ArgumentNullException(nameof(entitiesContext)); + _packageService = packageService ?? throw new ArgumentNullException(nameof(packageService)); + _packageFileService = packageFileService ?? throw new ArgumentNullException(nameof(packageFileService)); + _telemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService)); } public async Task ReflowAsync(string id, string version) @@ -75,7 +78,10 @@ public async Task ReflowAsync(string id, string version) // 5) Update IsLatest so that reflow can correct concurrent updates (see Gallery #2514) await _packageService.UpdateIsLatestAsync(package.PackageRegistration, commitChanges: false); - // 6) Save and profit + // 6) Emit telemetry. + _telemetryService.TrackPackageReflow(package); + + // 7) Save and profit await _entitiesContext.SaveChangesAsync(); } } diff --git a/src/NuGetGallery/Services/TelemetryClientWrapper.cs b/src/NuGetGallery/Services/TelemetryClientWrapper.cs index e5266d42e4..d5d7f1fdbe 100644 --- a/src/NuGetGallery/Services/TelemetryClientWrapper.cs +++ b/src/NuGetGallery/Services/TelemetryClientWrapper.cs @@ -29,18 +29,6 @@ private TelemetryClientWrapper() internal TelemetryClient UnderlyingClient { get; } - public void TrackEvent(string eventName, IDictionary properties = null, IDictionary metrics = null) - { - try - { - UnderlyingClient.TrackEvent(eventName, properties, metrics); - } - catch - { - // logging failed, don't allow exception to escape - } - } - public void TrackException(Exception exception, IDictionary properties = null, IDictionary metrics = null) { try diff --git a/src/NuGetGallery/Services/TelemetryService.cs b/src/NuGetGallery/Services/TelemetryService.cs index 66298333c2..154a181cb3 100644 --- a/src/NuGetGallery/Services/TelemetryService.cs +++ b/src/NuGetGallery/Services/TelemetryService.cs @@ -24,6 +24,12 @@ internal class Events public const string CredentialAdded = "CredentialAdded"; public const string UserPackageDeleteCheckedAfterHours = "UserPackageDeleteCheckedAfterHours"; public const string UserPackageDeleteExecuted = "UserPackageDeleteExecuted"; + public const string PackageReflow = "PackageReflow"; + public const string PackageUnlisted = "PackageUnlisted"; + public const string PackageListed = "PackageListed"; + public const string PackageDelete = "PackageDelete"; + public const string PackageHardDeleteReflow = "PackageHardDeleteReflow"; + public const string PackageRevalidate = "PackageRevalidate"; } private IDiagnosticsSource _diagnosticsSource; @@ -71,6 +77,9 @@ internal class Events // User package delete executed properties public const string Success = "Success"; + // Package delete properties + public const string IsHardDelete = "IsHardDelete"; + public TelemetryService(IDiagnosticsService diagnosticsService, ITelemetryClient telemetryClient = null) { if (diagnosticsService == null) @@ -100,7 +109,7 @@ public void TraceException(Exception exception) public void TrackODataQueryFilterEvent(string callContext, bool isEnabled, bool isAllowed, string queryPattern) { - TrackEvent(Events.ODataQueryFilter, properties => + TrackMetric(Events.ODataQueryFilter, 1, properties => { properties.Add(CallContext, callContext); properties.Add(IsEnabled, $"{isEnabled}"); @@ -122,9 +131,9 @@ public void TrackPackageReadMeChangeEvent(Package package, string readMeSourceTy throw new ArgumentNullException(nameof(readMeSourceType)); } - TrackEvent(Events.PackageReadMeChanged, properties => { + TrackMetric(Events.PackageReadMeChanged, 1, properties => { properties.Add(PackageId, package.PackageRegistration.Id); - properties.Add(PackageVersion, package.Version); + properties.Add(PackageVersion, package.NormalizedVersion); properties.Add(ReadMeSourceType, readMeSourceType); properties.Add(ReadMeState, Enum.GetName(typeof(PackageEditReadMeState), readMeState)); }); @@ -137,17 +146,17 @@ public void TrackPackagePushEvent(Package package, User user, IIdentity identity throw new ArgumentNullException(nameof(package)); } - TrackPackageForEvent(Events.PackagePush, package.PackageRegistration.Id, package.Version, user, identity); + TrackMetricForPackage(Events.PackagePush, package.PackageRegistration.Id, package.NormalizedVersion, user, identity); } public void TrackPackagePushNamespaceConflictEvent(string packageId, string packageVersion, User user, IIdentity identity) { - TrackPackageForEvent(Events.PackagePushNamespaceConflict, packageId, packageVersion, user, identity); + TrackMetricForPackage(Events.PackagePushNamespaceConflict, packageId, packageVersion, user, identity); } public void TrackCreatePackageVerificationKeyEvent(string packageId, string packageVersion, User user, IIdentity identity) { - TrackPackageForEvent(Events.CreatePackageVerificationKey, packageId, packageVersion, user, identity); + TrackMetricForPackage(Events.CreatePackageVerificationKey, packageId, packageVersion, user, identity); } public void TrackVerifyPackageKeyEvent(string packageId, string packageVersion, User user, IIdentity identity, int statusCode) @@ -163,7 +172,7 @@ public void TrackVerifyPackageKeyEvent(string packageId, string packageVersion, } var hasVerifyScope = identity.HasScopeThatAllowsActions(NuGetScopes.PackageVerify).ToString(); - TrackEvent(Events.VerifyPackageKey, properties => + TrackMetric(Events.VerifyPackageKey, 1, properties => { properties.Add(PackageId, packageId); properties.Add(PackageVersion, packageVersion); @@ -175,15 +184,78 @@ public void TrackVerifyPackageKeyEvent(string packageId, string packageVersion, public void TrackNewUserRegistrationEvent(User user, Credential credential) { - TrackAccountActivityForEvent(Events.NewUserRegistration, user, credential); + TrackMetricForAccountActivity(Events.NewUserRegistration, user, credential); } public void TrackNewCredentialCreated(User user, Credential credential) { - TrackAccountActivityForEvent(Events.CredentialAdded, user, credential); + TrackMetricForAccountActivity(Events.CredentialAdded, user, credential); } - private void TrackAccountActivityForEvent(string eventName, User user, Credential credential) + public void TrackUserPackageDeleteExecuted(int packageKey, string packageId, string packageVersion, ReportPackageReason reason, bool success) + { + if (packageId == null) + { + throw new ArgumentNullException(nameof(packageId)); + } + + if (packageVersion == null) + { + throw new ArgumentNullException(nameof(packageVersion)); + } + + TrackMetric(Events.UserPackageDeleteExecuted, 1, properties => { + properties.Add(PackageKey, packageKey.ToString()); + properties.Add(PackageId, packageId); + properties.Add(PackageVersion, packageVersion); + properties.Add(ReportPackageReason, reason.ToString()); + properties.Add(Success, success.ToString()); + }); + } + + public void TrackPackageUnlisted(Package package) + { + TrackMetricForPackage(Events.PackageUnlisted, package); + } + + public void TrackPackageListed(Package package) + { + TrackMetricForPackage(Events.PackageListed, package); + } + + public void TrackPackageDelete(Package package, bool isHardDelete) + { + TrackMetricForPackage(Events.PackageDelete, package, properties => + { + properties.Add(IsHardDelete, isHardDelete.ToString()); + }); + } + + public void TrackPackageReflow(Package package) + { + TrackMetricForPackage(Events.PackageReflow, package); + } + + public void TrackPackageHardDeleteReflow(string packageId, string packageVersion) + { + TrackMetricForPackage(Events.PackageHardDeleteReflow, packageId, packageVersion); + } + + public void TrackPackageRevalidate(Package package) + { + TrackMetricForPackage(Events.PackageRevalidate, package); + } + + public void TrackException(Exception exception, Action> addProperties) + { + var telemetryProperties = new Dictionary(); + + addProperties(telemetryProperties); + + _telemetryClient.TrackException(exception, telemetryProperties, metrics: null); + } + + private void TrackMetricForAccountActivity(string eventName, User user, Credential credential) { if (user == null) { @@ -195,7 +267,7 @@ private void TrackAccountActivityForEvent(string eventName, User user, Credentia throw new ArgumentNullException(nameof(credential)); } - TrackEvent(eventName, properties => { + TrackMetric(eventName, 1, properties => { properties.Add(ClientVersion, GetClientVersion()); properties.Add(ProtocolVersion, GetProtocolVersion()); properties.Add(AccountCreationDate, GetAccountCreationDate(user)); @@ -240,7 +312,34 @@ private static string GetApiKeyCreationDate(User user, IIdentity identity) return apiKey?.Created.ToString("O") ?? "N/A"; } - private void TrackPackageForEvent(string eventValue, string packageId, string packageVersion, User user, IIdentity identity) + private void TrackMetricForPackage( + string metricName, + Package package, + User user, + IIdentity identity, + Action> addProperties = null) + { + if (package == null) + { + throw new ArgumentNullException(nameof(package)); + } + + TrackMetricForPackage( + metricName, + package.PackageRegistration.Id, + package.NormalizedVersion, + user, + identity, + addProperties); + } + + private void TrackMetricForPackage( + string metricName, + string packageId, + string packageVersion, + User user, + IIdentity identity, + Action> addProperties = null) { if (user == null) { @@ -252,7 +351,7 @@ private void TrackPackageForEvent(string eventValue, string packageId, string pa throw new ArgumentNullException(nameof(identity)); } - TrackEvent(eventValue, properties => { + TrackMetric(metricName, 1, properties => { properties.Add(ClientVersion, GetClientVersion()); properties.Add(ProtocolVersion, GetProtocolVersion()); properties.Add(ClientInformation, GetClientInformation()); @@ -262,6 +361,40 @@ private void TrackPackageForEvent(string eventValue, string packageId, string pa properties.Add(AuthenticationMethod, identity.GetAuthenticationType()); properties.Add(KeyCreationDate, GetApiKeyCreationDate(user, identity)); properties.Add(IsScoped, identity.IsScopedAuthentication().ToString()); + addProperties?.Invoke(properties); + }); + } + + private void TrackMetricForPackage( + string metricName, + Package package, + Action> addProperties = null) + { + if (package == null) + { + throw new ArgumentNullException(nameof(package)); + } + + TrackMetricForPackage( + metricName, + package.PackageRegistration.Id, + package.NormalizedVersion, + addProperties); + } + + private void TrackMetricForPackage( + string metricName, + string packageId, + string packageVersion, + Action> addProperties = null) + { + TrackMetric(metricName, 1, properties => { + properties.Add(ClientVersion, GetClientVersion()); + properties.Add(ProtocolVersion, GetProtocolVersion()); + properties.Add(ClientInformation, GetClientInformation()); + properties.Add(PackageId, packageId); + properties.Add(PackageVersion, packageVersion); + addProperties?.Invoke(properties); }); } @@ -288,36 +421,11 @@ public void TrackUserPackageDeleteChecked(UserPackageDeleteEvent details, UserPa }); } - public void TrackUserPackageDeleteExecuted(int packageKey, string packageId, string packageVersion, ReportPackageReason reason, bool success) - { - if (packageId == null) - { - throw new ArgumentNullException(nameof(packageId)); - } - - if (packageVersion == null) - { - throw new ArgumentNullException(nameof(packageVersion)); - } - - TrackMetric(Events.UserPackageDeleteExecuted, 1, properties => { - properties.Add(PackageKey, packageKey.ToString()); - properties.Add(PackageId, packageId); - properties.Add(PackageVersion, packageVersion); - properties.Add(ReportPackageReason, reason.ToString()); - properties.Add(Success, success.ToString()); - }); - } - - protected virtual void TrackEvent(string eventName, Action> addProperties) - { - var telemetryProperties = new Dictionary(); - - addProperties(telemetryProperties); - - _telemetryClient.TrackEvent(eventName, telemetryProperties, metrics: null); - } - + /// + /// We use instead of + /// + /// because events don't flow properly into our internal metrics and monitoring solution. + /// protected virtual void TrackMetric(string metricName, double value, Action> addProperties) { var telemetryProperties = new Dictionary(); @@ -326,14 +434,5 @@ protected virtual void TrackMetric(string metricName, double value, Action> addProperties) - { - var telemetryProperties = new Dictionary(); - - addProperties(telemetryProperties); - - _telemetryClient.TrackException(exception, telemetryProperties, metrics: null); - } } } \ No newline at end of file diff --git a/src/NuGetGallery/Services/ValidationService.cs b/src/NuGetGallery/Services/ValidationService.cs index 1d4bf81a33..8394e62a09 100644 --- a/src/NuGetGallery/Services/ValidationService.cs +++ b/src/NuGetGallery/Services/ValidationService.cs @@ -16,15 +16,18 @@ public class ValidationService : IValidationService private readonly IPackageService _packageService; private readonly IPackageValidationInitiator _initiator; private readonly IEntityRepository _validationSets; + private readonly ITelemetryService _telemetryService; public ValidationService( IPackageService packageService, IPackageValidationInitiator initiator, - IEntityRepository validationSets) + IEntityRepository validationSets, + ITelemetryService telemetryService) { _packageService = packageService ?? throw new ArgumentNullException(nameof(packageService)); _initiator = initiator ?? throw new ArgumentNullException(nameof(initiator)); _validationSets = validationSets ?? throw new ArgumentNullException(nameof(validationSets)); + _telemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService)); } public async Task StartValidationAsync(Package package) @@ -40,6 +43,8 @@ await _packageService.UpdatePackageStatusAsync( public async Task RevalidateAsync(Package package) { await _initiator.StartValidationAsync(package); + + _telemetryService.TrackPackageRevalidate(package); } public IReadOnlyList GetLatestValidationIssues(Package package) diff --git a/tests/NuGetGallery.Facts/Services/PackageDeleteServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageDeleteServiceFacts.cs index 753e89928f..ac3639b5bf 100644 --- a/tests/NuGetGallery.Facts/Services/PackageDeleteServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageDeleteServiceFacts.cs @@ -698,6 +698,23 @@ public async Task WillCreateAuditRecordUsingAuditService() Assert.Equal(package.Version, testService.LastAuditRecord.Version); auditingService.Verify(x => x.SaveAuditRecordAsync(testService.LastAuditRecord)); } + + [Fact] + public async Task EmitsTelemetry() + { + var telemetryService = new Mock(); + var service = CreateService(telemetryService: telemetryService); + var packageRegistration = new PackageRegistration(); + var package = new Package { PackageRegistration = packageRegistration, Version = "1.0.0", Hash = _packageHashForTests }; + packageRegistration.Packages.Add(package); + var user = new User("test"); + var reason = "Unit testing"; + var signature = "The Terminator"; + + await service.SoftDeletePackagesAsync(new[] { package }, user, reason, signature); + + telemetryService.Verify(x => x.TrackPackageDelete(package, false)); + } } public class TheHardDeletePackagesAsyncMethod @@ -953,6 +970,23 @@ public async Task WillCreateAuditRecordUsingAuditService() Assert.Equal(package.Version, testService.LastAuditRecord.Version); auditingService.Verify(x => x.SaveAuditRecordAsync(testService.LastAuditRecord)); } + + [Fact] + public async Task EmitsTelemetry() + { + var telemetryService = new Mock(); + var service = CreateService(telemetryService: telemetryService); + var packageRegistration = new PackageRegistration(); + var package = new Package { PackageRegistration = packageRegistration, Version = "1.0.0", Hash = _packageHashForTests }; + packageRegistration.Packages.Add(package); + var user = new User("test"); + var reason = "Unit testing"; + var signature = "The Terminator"; + + await service.HardDeletePackagesAsync(new[] { package }, user, reason, signature, deleteEmptyPackageRegistration: false); + + telemetryService.Verify(x => x.TrackPackageDelete(package, true)); + } } public class TheReflowHardDeletedPackagesAsyncMethod @@ -998,7 +1032,13 @@ private async Task ReflowHardDeletedPackage(string id, string version, string ex var auditingService = new Mock(); - var service = CreateService(packageRepository: packageRepository, packageRegistrationRepository: packageRegistrationRepository, auditingService: auditingService); + var telemetryService = new Mock(); + + var service = CreateService( + packageRepository: packageRepository, + packageRegistrationRepository: packageRegistrationRepository, + auditingService: auditingService, + telemetryService: telemetryService); if (succeeds) { @@ -1009,7 +1049,13 @@ private async Task ReflowHardDeletedPackage(string id, string version, string ex await Assert.ThrowsAsync(() => service.ReflowHardDeletedPackageAsync(id, version)); } - auditingService.Verify(x => x.SaveAuditRecordAsync(It.IsAny()), succeeds ? Times.Once() : Times.Never()); + auditingService.Verify( + x => x.SaveAuditRecordAsync(It.IsAny()), + succeeds ? Times.Once() : Times.Never()); + + telemetryService.Verify( + x => x.TrackPackageHardDeleteReflow(id, version), + succeeds ? Times.Once() : Times.Never()); } } } diff --git a/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs index 72a7f10328..6b1429ee32 100644 --- a/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs @@ -24,11 +24,13 @@ private static IPackageService CreateService( Mock> packageRepository = null, IPackageNamingConflictValidator packageNamingConflictValidator = null, IAuditingService auditingService = null, + Mock telemetryService = null, Action> setup = null) { packageRegistrationRepository = packageRegistrationRepository ?? new Mock>(); packageRepository = packageRepository ?? new Mock>(); auditingService = auditingService ?? new TestAuditingService(); + telemetryService = telemetryService ?? new Mock(); if (packageNamingConflictValidator == null) { @@ -41,7 +43,8 @@ private static IPackageService CreateService( packageRegistrationRepository.Object, packageRepository.Object, packageNamingConflictValidator, - auditingService); + auditingService, + telemetryService.Object); packageService.CallBase = true; @@ -1428,6 +1431,21 @@ public async Task WritesAnAuditRecord() && ar.Id == package.PackageRegistration.Id && ar.Version == package.Version)); } + + [Fact] + public async Task EmitsTelemetry() + { + var packageRegistration = new PackageRegistration { Id = "theId" }; + var package = new Package { Version = "1.0", PackageRegistration = packageRegistration, Listed = false }; + var telemetryService = new Mock(); + var service = CreateService(telemetryService: telemetryService); + + await service.MarkPackageListedAsync(package); + + telemetryService.Verify( + x => x.TrackPackageListed(package), + Times.Once); + } } public class TheMarkPackageUnlistedMethod @@ -1536,6 +1554,21 @@ public async Task WritesAnAuditRecord() && ar.Id == package.PackageRegistration.Id && ar.Version == package.Version)); } + + [Fact] + public async Task EmitsTelemetry() + { + var packageRegistration = new PackageRegistration { Id = "theId" }; + var package = new Package { Version = "1.0", PackageRegistration = packageRegistration, Listed = true }; + var telemetryService = new Mock(); + var service = CreateService(telemetryService: telemetryService); + + await service.MarkPackageUnlistedAsync(package); + + telemetryService.Verify( + x => x.TrackPackageUnlisted(package), + Times.Once); + } } public class ThePublishPackageMethod diff --git a/tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs b/tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs index 5d0ce1e86b..f3ff942cc7 100644 --- a/tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs @@ -23,6 +23,7 @@ private static ReflowPackageService CreateService( Mock entitiesContext = null, Mock packageService = null, Mock packageFileService = null, + Mock telemetryService = null, Action> setup = null) { var dbContext = new Mock(); @@ -31,11 +32,13 @@ private static ReflowPackageService CreateService( packageService = packageService ?? new Mock(); packageFileService = packageFileService ?? new Mock(); + telemetryService = telemetryService ?? new Mock(); var reflowPackageService = new Mock( entitiesContext.Object, packageService.Object, - packageFileService.Object); + packageFileService.Object, + telemetryService.Object); reflowPackageService.CallBase = true; @@ -263,6 +266,32 @@ public async Task CallsUpdateIsLatestAsync() packageService.Verify(s => s.UpdateIsLatestAsync(package.PackageRegistration, false), Times.Once); } + [Fact] + public async Task EmitsTelemetry() + { + // Arrange + var package = PackageServiceUtility.CreateTestPackage(); + + var packageService = SetupPackageService(package); + var entitiesContext = SetupEntitiesContext(); + var packageFileService = SetupPackageFileService(package); + var telemetryService = new Mock(); + + var service = CreateService( + packageService: packageService, + entitiesContext: entitiesContext, + packageFileService: packageFileService, + telemetryService: telemetryService); + + // Act + var result = await service.ReflowAsync("test", "1.0.0"); + + // Assert + telemetryService.Verify( + x => x.TrackPackageReflow(package), + Times.Once); + } + [Fact] public async Task AllowsInvalidPackageDependencyVersion() { @@ -312,12 +341,14 @@ private static Mock SetupPackageService(Package package) packageRegistrationRepository.Object, packageRepository.Object); var auditingService = new TestAuditingService(); + var telemetryService = new Mock(); var packageService = new Mock( packageRegistrationRepository.Object, packageRepository.Object, packageNamingConflictValidator, - auditingService); + auditingService, + telemetryService.Object); packageService.CallBase = true; diff --git a/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs b/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs index da8e10713d..fcd952f8dd 100644 --- a/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs @@ -29,7 +29,7 @@ public class TheTrackEventMethod : BaseFacts { private static Fakes fakes = new Fakes(); - public static IEnumerable TrackEventNames_Data + public static IEnumerable TrackMetricNames_Data { get { @@ -44,6 +44,30 @@ public static IEnumerable TrackEventNames_Data (TrackAction)(s => s.TrackPackagePushEvent(package, fakes.User, identity)) }; + yield return new object[] { "PackageUnlisted", + (TrackAction)(s => s.TrackPackageUnlisted(package)) + }; + + yield return new object[] { "PackageListed", + (TrackAction)(s => s.TrackPackageListed(package)) + }; + + yield return new object[] { "PackageDelete", + (TrackAction)(s => s.TrackPackageDelete(package, isHardDelete: true)) + }; + + yield return new object[] { "PackageReflow", + (TrackAction)(s => s.TrackPackageReflow(package)) + }; + + yield return new object[] { "PackageHardDeleteReflow", + (TrackAction)(s => s.TrackPackageHardDeleteReflow(fakes.Package.Id, package.Version)) + }; + + yield return new object[] { "PackageRevalidate", + (TrackAction)(s => s.TrackPackageRevalidate(package)) + }; + yield return new object[] { "CreatePackageVerificationKey", (TrackAction)(s => s.TrackCreatePackageVerificationKeyEvent(fakes.Package.Id, package.Version, fakes.User, identity)) }; @@ -67,13 +91,7 @@ public static IEnumerable TrackEventNames_Data yield return new object[] { "CredentialAdded", (TrackAction)(s => s.TrackNewCredentialCreated(fakes.User, fakes.User.Credentials.First())) }; - } - } - public static IEnumerable TrackMetricNames_Data - { - get - { yield return new object[] { "UserPackageDeleteCheckedAfterHours", (TrackAction)(s => s.TrackUserPackageDeleteChecked( new UserPackageDeleteEvent( @@ -105,28 +123,11 @@ public static IEnumerable TrackMetricNames_Data public void TrackEventNamesIncludesAllEvents() { var expectedCount = typeof(TelemetryService.Events).GetFields().Length; - var actualCount = TrackEventNames_Data.Count() + TrackMetricNames_Data.Count(); + var actualCount = TrackMetricNames_Data.Count(); Assert.Equal(expectedCount, actualCount); } - [Theory] - [MemberData(nameof(TrackEventNames_Data))] - public void TrackEventNames(string eventName, TrackAction track) - { - // Arrange - var service = CreateService(); - - // Act - track(service); - - // Assert - service.TelemetryClient.Verify(c => c.TrackEvent(eventName, - It.IsAny>(), - It.IsAny>()), - Times.Once); - } - [Theory] [MemberData(nameof(TrackMetricNames_Data))] public void TrackMetricNames(string metricName, TrackAction track) @@ -348,12 +349,6 @@ public TelemetryServiceWrapper CreateService() traceService.Setup(s => s.GetSource(It.IsAny())) .Returns(traceSource.Object); - telemetryClient.Setup(c => c.TrackEvent( - It.IsAny(), - It.IsAny>(), - It.IsAny>())) - .Verifiable(); - var telemetryService = new TelemetryServiceWrapper(traceService.Object, telemetryClient.Object); telemetryService.TraceSource = traceSource; diff --git a/tests/NuGetGallery.Facts/Services/ValidationServiceFacts.cs b/tests/NuGetGallery.Facts/Services/ValidationServiceFacts.cs index 89afd36911..461649e5f8 100644 --- a/tests/NuGetGallery.Facts/Services/ValidationServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/ValidationServiceFacts.cs @@ -83,6 +83,16 @@ public async Task DoesNotChangeThePackageStatus() /// to do this. Assert.Equal(PackageStatus.Available, _package.PackageStatusKey); } + + [Fact] + public async Task EmitsTelemetry() + { + // Arrange & Act + await _target.RevalidateAsync(_package); + + // Assert + _telemetryService.Verify(x => x.TrackPackageRevalidate(_package), Times.Once); + } } public class TheGetLatestValidationIssuesMethod : FactsBase @@ -303,6 +313,7 @@ public class FactsBase protected readonly Mock _packageService; protected readonly Mock _initiator; protected readonly Mock> _validationSets; + protected readonly Mock _telemetryService; protected readonly Package _package; protected readonly ValidationService _target; @@ -311,13 +322,15 @@ public FactsBase() _packageService = new Mock(); _initiator = new Mock(); _validationSets = new Mock>(); + _telemetryService = new Mock(); _package = new Package(); _target = new ValidationService( _packageService.Object, _initiator.Object, - _validationSets.Object); + _validationSets.Object, + _telemetryService.Object); } } } diff --git a/tests/NuGetGallery.Facts/TestUtils/TestServiceUtility.cs b/tests/NuGetGallery.Facts/TestUtils/TestServiceUtility.cs index 722c528cd0..da4f88f6d9 100644 --- a/tests/NuGetGallery.Facts/TestUtils/TestServiceUtility.cs +++ b/tests/NuGetGallery.Facts/TestUtils/TestServiceUtility.cs @@ -287,11 +287,14 @@ private Mock SetupPackageService() packageRepository.Object); var auditingService = new TestAuditingService(); + var telemetryService = new Mock(); + var packageService = new Mock( packageRegistrationRepository.Object, packageRepository.Object, packageNamingConflictValidator, - auditingService); + auditingService, + telemetryService.Object); packageService.CallBase = true; From 0ec515d561015f4940a237ef9dbd3466814200e1 Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Mon, 26 Feb 2018 18:14:36 -0800 Subject: [PATCH 07/24] Move StorePackageFileInBackupLocationAsync to NuGetGallery.Core (#5549) Progress on https://github.com/NuGet/Engineering/issues/1192 --- src/NuGetGallery.Core/CoreConstants.cs | 1 + .../Services/CorePackageFileService.cs | 82 +++++++ .../Services/ICorePackageFileService.cs | 5 + src/NuGetGallery/Constants.cs | 2 - .../Services/IPackageFileService.cs | 5 - .../Services/PackageFileService.cs | 52 ---- .../Services/CorePackageFileServiceFacts.cs | 226 ++++++++++++++++++ .../Services/PackageFileServiceFacts.cs | 145 ----------- 8 files changed, 314 insertions(+), 204 deletions(-) diff --git a/src/NuGetGallery.Core/CoreConstants.cs b/src/NuGetGallery.Core/CoreConstants.cs index 0050fabc8c..c0ea412777 100644 --- a/src/NuGetGallery.Core/CoreConstants.cs +++ b/src/NuGetGallery.Core/CoreConstants.cs @@ -10,6 +10,7 @@ public static class CoreConstants public const int MaxPackageIdLength = 128; public const string PackageFileSavePathTemplate = "{0}.{1}{2}"; + public const string PackageFileBackupSavePathTemplate = "{0}/{1}/{2}.{3}"; public const string NuGetPackageFileExtension = ".nupkg"; diff --git a/src/NuGetGallery.Core/Services/CorePackageFileService.cs b/src/NuGetGallery.Core/Services/CorePackageFileService.cs index c84db2ca4a..2bfe534df0 100644 --- a/src/NuGetGallery.Core/Services/CorePackageFileService.cs +++ b/src/NuGetGallery.Core/Services/CorePackageFileService.cs @@ -5,6 +5,8 @@ using System.Globalization; using System.IO; using System.Threading.Tasks; +using System.Web; +using NuGet.Versioning; namespace NuGetGallery { @@ -128,6 +130,86 @@ public Task DoesValidationPackageFileExistAsync(Package package) return _fileStorageService.FileExistsAsync(CoreConstants.ValidationFolderName, fileName); } + public async Task StorePackageFileInBackupLocationAsync(Package package, Stream packageFile) + { + if (package == null) + { + throw new ArgumentNullException(nameof(package)); + } + + if (packageFile == null) + { + throw new ArgumentNullException(nameof(packageFile)); + } + + if (package.PackageRegistration == null || + string.IsNullOrWhiteSpace(package.PackageRegistration.Id) || + (string.IsNullOrWhiteSpace(package.NormalizedVersion) && string.IsNullOrWhiteSpace(package.Version))) + { + throw new ArgumentException(CoreStrings.PackageIsMissingRequiredData, nameof(package)); + } + + string version; + if (string.IsNullOrEmpty(package.NormalizedVersion)) + { + version = NuGetVersion.Parse(package.Version).ToNormalizedString(); + } + else + { + version = package.NormalizedVersion; + } + + var fileName = BuildBackupFileName( + package.PackageRegistration.Id, + version, + package.Hash); + + // If the package already exists, don't even bother uploading it. The file name is based off of the hash so + // we know the upload isn't necessary. + if (await _fileStorageService.FileExistsAsync(CoreConstants.PackageBackupsFolderName, fileName)) + { + return; + } + + try + { + await _fileStorageService.SaveFileAsync(CoreConstants.PackageBackupsFolderName, fileName, packageFile); + } + catch (InvalidOperationException) + { + // If the package file already exists, swallow the exception since we know the content is the same. + return; + } + } + + private static string BuildBackupFileName(string id, string version, string hash) + { + if (id == null) + { + throw new ArgumentNullException(nameof(id)); + } + + if (version == null) + { + throw new ArgumentNullException(nameof(version)); + } + + if (hash == null) + { + throw new ArgumentNullException(nameof(hash)); + } + + var hashBytes = Convert.FromBase64String(hash); + + return string.Format( + CultureInfo.InvariantCulture, + CoreConstants.PackageFileBackupSavePathTemplate, + id.ToLowerInvariant(), + version.ToLowerInvariant(), + HttpServerUtility.UrlTokenEncode(hashBytes), + CoreConstants.NuGetPackageFileExtension); + } + protected static string BuildFileName(Package package, string format, string extension) { if (package == null) diff --git a/src/NuGetGallery.Core/Services/ICorePackageFileService.cs b/src/NuGetGallery.Core/Services/ICorePackageFileService.cs index f7e8fd17d0..f0efb7910f 100644 --- a/src/NuGetGallery.Core/Services/ICorePackageFileService.cs +++ b/src/NuGetGallery.Core/Services/ICorePackageFileService.cs @@ -81,5 +81,10 @@ public interface ICorePackageFileService /// The package ID. This value is case-insensitive. /// The package version. This value is case-insensitive and need not be normalized. Task DeletePackageFileAsync(string id, string version); + + /// + /// Copies the contents of the package represented by the stream into the file storage backup location. + /// + Task StorePackageFileInBackupLocationAsync(Package package, Stream packageFile); } } \ No newline at end of file diff --git a/src/NuGetGallery/Constants.cs b/src/NuGetGallery/Constants.cs index f0d74a8b24..1a16b1a0cc 100644 --- a/src/NuGetGallery/Constants.cs +++ b/src/NuGetGallery/Constants.cs @@ -38,8 +38,6 @@ public static class Constants public const string ReadMeFileSavePathTemplateActive = "active/{0}/{1}{2}"; public const string ReadMeFileSavePathTemplatePending = "pending/{0}/{1}{2}"; - public const string PackageFileBackupSavePathTemplate = "{0}/{1}/{2}.{3}"; - public const string MarkdownFileExtension = ".md"; public const string HtmlFileExtension = ".html"; public const string JsonFileExtension = ".json"; diff --git a/src/NuGetGallery/Services/IPackageFileService.cs b/src/NuGetGallery/Services/IPackageFileService.cs index 8104e1bbfc..4cc8e7b9cb 100644 --- a/src/NuGetGallery/Services/IPackageFileService.cs +++ b/src/NuGetGallery/Services/IPackageFileService.cs @@ -20,11 +20,6 @@ public interface IPackageFileService : ICorePackageFileService /// Task CreateDownloadPackageActionResultAsync(Uri requestUrl, string unsafeId, string unsafeVersion); - /// - /// Copies the contents of the package represented by the stream into the file storage backup location. - /// - Task StorePackageFileInBackupLocationAsync(Package package, Stream packageFile); - /// /// Deletes the package readme.md file from storage. /// diff --git a/src/NuGetGallery/Services/PackageFileService.cs b/src/NuGetGallery/Services/PackageFileService.cs index d77de4b8f4..f32c64ce24 100644 --- a/src/NuGetGallery/Services/PackageFileService.cs +++ b/src/NuGetGallery/Services/PackageFileService.cs @@ -39,30 +39,6 @@ public Task CreateDownloadPackageActionResultAsync(Uri requestUrl, return _fileStorageService.CreateDownloadFileActionResultAsync(requestUrl, CoreConstants.PackagesFolderName, fileName); } - public Task StorePackageFileInBackupLocationAsync(Package package, Stream packageFile) - { - if (package == null) - { - throw new ArgumentNullException(nameof(package)); - } - - if (packageFile == null) - { - throw new ArgumentNullException(nameof(packageFile)); - } - - if (package.PackageRegistration == null || - string.IsNullOrWhiteSpace(package.PackageRegistration.Id) || - (string.IsNullOrWhiteSpace(package.NormalizedVersion) && string.IsNullOrWhiteSpace(package.Version))) - { - throw new ArgumentException(CoreStrings.PackageIsMissingRequiredData, nameof(package)); - } - - var fileName = BuildBackupFileName(package.PackageRegistration.Id, string.IsNullOrEmpty(package.NormalizedVersion) - ? NuGetVersion.Parse(package.Version).ToNormalizedString() : package.NormalizedVersion, package.Hash); - return _fileStorageService.SaveFileAsync(CoreConstants.PackageBackupsFolderName, fileName, packageFile); - } - /// /// Deletes the package readme.md file from storage. /// @@ -126,33 +102,5 @@ public async Task DownloadReadMeMdFileAsync(Package package) return null; } - - private static string BuildBackupFileName(string id, string version, string hash) - { - if (id == null) - { - throw new ArgumentNullException(nameof(id)); - } - - if (version == null) - { - throw new ArgumentNullException(nameof(version)); - } - - if (hash == null) - { - throw new ArgumentNullException(nameof(hash)); - } - - var hashBytes = Convert.FromBase64String(hash); - - return string.Format( - CultureInfo.InvariantCulture, - Constants.PackageFileBackupSavePathTemplate, - id, - version, - HttpServerUtility.UrlTokenEncode(hashBytes), - CoreConstants.NuGetPackageFileExtension); - } } } \ No newline at end of file diff --git a/tests/NuGetGallery.Core.Facts/Services/CorePackageFileServiceFacts.cs b/tests/NuGetGallery.Core.Facts/Services/CorePackageFileServiceFacts.cs index 9035875076..f24b8ebb20 100644 --- a/tests/NuGetGallery.Core.Facts/Services/CorePackageFileServiceFacts.cs +++ b/tests/NuGetGallery.Core.Facts/Services/CorePackageFileServiceFacts.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.IO; using System.Threading.Tasks; +using System.Web; using Moq; using Xunit; @@ -495,6 +496,219 @@ public async Task WillUseFileStorageService() } } + public class TheStorePackageFileInBackupLocationAsyncMethod + { + private string packageHashForTests = "NzMzMS1QNENLNEczSDQ1SA=="; + + [Fact] + public async Task WillThrowIfPackageIsNull() + { + var service = CreateService(); + + var ex = await Assert.ThrowsAsync(() => service.StorePackageFileInBackupLocationAsync(null, Stream.Null)); + + Assert.Equal("package", ex.ParamName); + } + + [Fact] + public async Task WillThrowIfPackageFileIsNull() + { + var service = CreateService(); + + var ex = await Assert.ThrowsAsync(() => service.StorePackageFileInBackupLocationAsync(new Package { PackageRegistration = new PackageRegistration() }, null)); + + Assert.Equal("packageFile", ex.ParamName); + } + + [Fact] + public async Task WillThrowIfPackageIsMissingPackageRegistration() + { + var service = CreateService(); + var package = new Package { PackageRegistration = null }; + + var ex = await Assert.ThrowsAsync(() => service.StorePackageFileInBackupLocationAsync(package, CreatePackageFileStream())); + + Assert.True(ex.Message.StartsWith("The package is missing required data.")); + Assert.Equal("package", ex.ParamName); + } + + [Fact] + public async Task WillThrowIfPackageIsMissingPackageRegistrationId() + { + var service = CreateService(); + var packageRegistraion = new PackageRegistration { Id = null }; + var package = new Package { PackageRegistration = packageRegistraion }; + + var ex = await Assert.ThrowsAsync(() => service.StorePackageFileInBackupLocationAsync(package, CreatePackageFileStream())); + + Assert.True(ex.Message.StartsWith("The package is missing required data.")); + Assert.Equal("package", ex.ParamName); + } + + [Fact] + public async Task WillThrowIfPackageIsMissingNormalizedVersionAndVersion() + { + var service = CreateService(); + var packageRegistraion = new PackageRegistration { Id = "theId" }; + var package = new Package { PackageRegistration = packageRegistraion, NormalizedVersion = null, Version = null }; + + var ex = await Assert.ThrowsAsync(() => service.StorePackageFileInBackupLocationAsync(package, CreatePackageFileStream())); + + Assert.True(ex.Message.StartsWith("The package is missing required data.")); + Assert.Equal("package", ex.ParamName); + } + + [Fact] + public async Task WillUseNormalizedRegularVersionIfNormalizedVersionMissing() + { + var fileStorageSvc = new Mock(); + var service = CreateService(fileStorageService: fileStorageSvc); + var packageRegistraion = new PackageRegistration { Id = "theId" }; + var package = new Package { PackageRegistration = packageRegistraion, NormalizedVersion = null, Version = "01.01.01", Hash = packageHashForTests }; + package.Hash = packageHashForTests; + + fileStorageSvc.Setup(x => x.SaveFileAsync(It.IsAny(), BuildBackupFileName("theId", "1.1.1", packageHashForTests), It.IsAny(), It.Is(b => b))) + .Completes() + .Verifiable(); + + await service.StorePackageFileInBackupLocationAsync(package, CreatePackageFileStream()); + + fileStorageSvc.VerifyAll(); + } + + [Fact] + public async Task WillUseLowercaseNormalizedIdAndVersion() + { + var fileStorageSvc = new Mock(); + var service = CreateService(fileStorageService: fileStorageSvc); + string path = null; + fileStorageSvc + .Setup(x => x.SaveFileAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .Returns(Task.FromResult(true)) + .Callback((_, p, __, ___) => path = p); + + var package = CreatePackage(); + package.PackageRegistration.Id = Id; + package.NormalizedVersion = NormalizedVersion; + package.Hash = packageHashForTests; + + await service.StorePackageFileInBackupLocationAsync(package, CreatePackageFileStream()); + + Assert.Equal("nuget.versioning/4.3.0-beta/NzMzMS1QNENLNEczSDQ1SA2..nupkg", path); + } + + [Fact] + public async Task WillSaveTheFileViaTheFileStorageServiceUsingThePackagesFolder() + { + var fileStorageSvc = new Mock(); + var service = CreateService(fileStorageService: fileStorageSvc); + fileStorageSvc.Setup(x => x.SaveFileAsync(CoreConstants.PackageBackupsFolderName, It.IsAny(), It.IsAny(), It.Is(b => b))) + .Completes() + .Verifiable(); + + var package = CreatePackage(); + package.Hash = packageHashForTests; + + await service.StorePackageFileInBackupLocationAsync(package, CreatePackageFileStream()); + + fileStorageSvc.VerifyAll(); + } + + [Fact] + public async Task WillSaveTheFileViaTheFileStorageServiceUsingAFileNameWithIdAndNormalizedersion() + { + var fileStorageSvc = new Mock(); + var service = CreateService(fileStorageService: fileStorageSvc); + fileStorageSvc.Setup(x => x.SaveFileAsync(It.IsAny(), BuildBackupFileName("theId", "theNormalizedVersion", packageHashForTests), It.IsAny(), It.Is(b => b))) + .Completes() + .Verifiable(); + + var package = CreatePackage(); + package.Hash = packageHashForTests; + + await service.StorePackageFileInBackupLocationAsync(package, CreatePackageFileStream()); + + fileStorageSvc.VerifyAll(); + } + + [Fact] + public async Task WillSaveTheFileStreamViaTheFileStorageService() + { + var fileStorageSvc = new Mock(); + var fakeStream = new MemoryStream(); + var service = CreateService(fileStorageService: fileStorageSvc); + fileStorageSvc.Setup(x => x.SaveFileAsync(It.IsAny(), It.IsAny(), fakeStream, It.Is(b => b))) + .Completes() + .Verifiable(); + + var package = CreatePackage(); + package.Hash = packageHashForTests; + + await service.StorePackageFileInBackupLocationAsync(package, fakeStream); + + fileStorageSvc.VerifyAll(); + } + + [Fact] + public async Task WillNotUploadThePackageIfItAlreadyExists() + { + var fileStorageSvc = new Mock(); + var fakeStream = new MemoryStream(); + var service = CreateService(fileStorageService: fileStorageSvc); + fileStorageSvc + .Setup(x => x.FileExistsAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync(true); + + var package = CreatePackage(); + package.Hash = packageHashForTests; + + await service.StorePackageFileInBackupLocationAsync(package, fakeStream); + + fileStorageSvc.Verify( + x => x.SaveFileAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never); + } + + [Fact] + public async Task WillSwallowInvalidOperationException() + { + var fileStorageSvc = new Mock(); + var fakeStream = new MemoryStream(); + var service = CreateService(fileStorageService: fileStorageSvc); + fileStorageSvc + .Setup(x => x.SaveFileAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .Throws(new InvalidOperationException("File already exists.")); + + var package = CreatePackage(); + package.Hash = packageHashForTests; + + await service.StorePackageFileInBackupLocationAsync(package, fakeStream); + + fileStorageSvc.Verify( + x => x.SaveFileAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), + Times.Once); + } + + [Fact] + public async Task WillNotSwallowOtherExceptions() + { + var fileStorageSvc = new Mock(); + var fakeStream = new MemoryStream(); + var service = CreateService(fileStorageService: fileStorageSvc); + var exception = new ArgumentException("Bad!"); + fileStorageSvc + .Setup(x => x.SaveFileAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .Throws(exception); + + var package = CreatePackage(); + package.Hash = packageHashForTests; + + var actual = await Assert.ThrowsAsync( + () => service.StorePackageFileInBackupLocationAsync(package, fakeStream)); + Assert.Same(exception, actual); + } + } + static string BuildFileName( string id, string version, string extension, string path) @@ -506,6 +720,18 @@ static string BuildFileName( extension); } + private static string BuildBackupFileName(string id, string version, string hash) + { + var hashBytes = Convert.FromBase64String(hash); + + return string.Format( + CoreConstants.PackageFileBackupSavePathTemplate, + id.ToLowerInvariant(), + version.ToLowerInvariant(), + HttpServerUtility.UrlTokenEncode(hashBytes), + CoreConstants.NuGetPackageFileExtension); + } + static Package CreatePackage() { var packageRegistration = new PackageRegistration { Id = "theId", Packages = new HashSet() }; diff --git a/tests/NuGetGallery.Facts/Services/PackageFileServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageFileServiceFacts.cs index e953fd0847..b5188b2744 100644 --- a/tests/NuGetGallery.Facts/Services/PackageFileServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageFileServiceFacts.cs @@ -180,139 +180,6 @@ public async Task WillReturnTheResultFromTheFileStorageService() Assert.Equal(fakeResult, result); } } - - public class TheStorePackageFileInBackupLocationAsyncMethod - { - private string packageHashForTests = "NzMzMS1QNENLNEczSDQ1SA=="; - - [Fact] - public async Task WillThrowIfPackageIsNull() - { - var service = CreateService(); - - var ex = await Assert.ThrowsAsync(() => service.StorePackageFileInBackupLocationAsync(null, Stream.Null)); - - Assert.Equal("package", ex.ParamName); - } - - [Fact] - public async Task WillThrowIfPackageFileIsNull() - { - var service = CreateService(); - - var ex = await Assert.ThrowsAsync(() => service.StorePackageFileInBackupLocationAsync(new Package { PackageRegistration = new PackageRegistration() }, null)); - - Assert.Equal("packageFile", ex.ParamName); - } - - [Fact] - public async Task WillThrowIfPackageIsMissingPackageRegistration() - { - var service = CreateService(); - var package = new Package { PackageRegistration = null }; - - var ex = await Assert.ThrowsAsync(() => service.StorePackageFileInBackupLocationAsync(package, CreatePackageFileStream())); - - Assert.True(ex.Message.StartsWith("The package is missing required data.")); - Assert.Equal("package", ex.ParamName); - } - - [Fact] - public async Task WillThrowIfPackageIsMissingPackageRegistrationId() - { - var service = CreateService(); - var packageRegistraion = new PackageRegistration { Id = null }; - var package = new Package { PackageRegistration = packageRegistraion }; - - var ex = await Assert.ThrowsAsync(() => service.StorePackageFileInBackupLocationAsync(package, CreatePackageFileStream())); - - Assert.True(ex.Message.StartsWith("The package is missing required data.")); - Assert.Equal("package", ex.ParamName); - } - - [Fact] - public async Task WillThrowIfPackageIsMissingNormalizedVersionAndVersion() - { - var service = CreateService(); - var packageRegistraion = new PackageRegistration { Id = "theId" }; - var package = new Package { PackageRegistration = packageRegistraion, NormalizedVersion = null, Version = null }; - - var ex = await Assert.ThrowsAsync(() => service.StorePackageFileInBackupLocationAsync(package, CreatePackageFileStream())); - - Assert.True(ex.Message.StartsWith("The package is missing required data.")); - Assert.Equal("package", ex.ParamName); - } - - [Fact] - public async Task WillUseNormalizedRegularVersionIfNormalizedVersionMissing() - { - var fileStorageSvc = new Mock(); - var service = CreateService(fileStorageSvc: fileStorageSvc); - var packageRegistraion = new PackageRegistration { Id = "theId" }; - var package = new Package { PackageRegistration = packageRegistraion, NormalizedVersion = null, Version = "01.01.01", Hash = packageHashForTests}; - package.Hash = packageHashForTests; - - fileStorageSvc.Setup(x => x.SaveFileAsync(It.IsAny(), BuildBackupFileName("theId", "1.1.1", packageHashForTests), It.IsAny(), It.Is(b => b))) - .Completes() - .Verifiable(); - - await service.StorePackageFileInBackupLocationAsync(package, CreatePackageFileStream()); - - fileStorageSvc.VerifyAll(); - } - - [Fact] - public async Task WillSaveTheFileViaTheFileStorageServiceUsingThePackagesFolder() - { - var fileStorageSvc = new Mock(); - var service = CreateService(fileStorageSvc: fileStorageSvc); - fileStorageSvc.Setup(x => x.SaveFileAsync(CoreConstants.PackageBackupsFolderName, It.IsAny(), It.IsAny(), It.Is(b => b))) - .Completes() - .Verifiable(); - - var package = CreatePackage(); - package.Hash = packageHashForTests; - - await service.StorePackageFileInBackupLocationAsync(package, CreatePackageFileStream()); - - fileStorageSvc.VerifyAll(); - } - - [Fact] - public async Task WillSaveTheFileViaTheFileStorageServiceUsingAFileNameWithIdAndNormalizedersion() - { - var fileStorageSvc = new Mock(); - var service = CreateService(fileStorageSvc: fileStorageSvc); - fileStorageSvc.Setup(x => x.SaveFileAsync(It.IsAny(), BuildBackupFileName("theId", "theNormalizedVersion", packageHashForTests), It.IsAny(), It.Is(b => b))) - .Completes() - .Verifiable(); - - var package = CreatePackage(); - package.Hash = packageHashForTests; - - await service.StorePackageFileInBackupLocationAsync(package, CreatePackageFileStream()); - - fileStorageSvc.VerifyAll(); - } - - [Fact] - public async Task WillSaveTheFileStreamViaTheFileStorageService() - { - var fileStorageSvc = new Mock(); - var fakeStream = new MemoryStream(); - var service = CreateService(fileStorageSvc: fileStorageSvc); - fileStorageSvc.Setup(x => x.SaveFileAsync(It.IsAny(), It.IsAny(), fakeStream, It.Is(b => b))) - .Completes() - .Verifiable(); - - var package = CreatePackage(); - package.Hash = packageHashForTests; - - await service.StorePackageFileInBackupLocationAsync(package, fakeStream); - - fileStorageSvc.VerifyAll(); - } - } public class TheDeleteReadMeMdFileAsync { @@ -478,18 +345,6 @@ static string BuildFileName( extension); } - private static string BuildBackupFileName(string id, string version, string hash) - { - var hashBytes = Convert.FromBase64String(hash); - - return string.Format( - Constants.PackageFileBackupSavePathTemplate, - id, - version, - HttpServerUtility.UrlTokenEncode(hashBytes), - CoreConstants.NuGetPackageFileExtension); - } - static Package CreatePackage() { var packageRegistration = new PackageRegistration { Id = "theId", Packages = new HashSet() }; From 388736480de4bbb83ed6d2454ed8804ed8cf1d84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Sharma?= Date: Wed, 28 Feb 2018 09:22:44 -0800 Subject: [PATCH 08/24] Add delay in validation enqueue (#5529) Reduce the time the Orchestrator waits to start a package's validation by delaying the delivery of the validation request by 5 seconds. --- src/NuGetGallery/Configuration/AppConfiguration.cs | 2 ++ src/NuGetGallery/Configuration/IAppConfiguration.cs | 7 +++++++ .../Services/AsynchronousPackageValidationInitiator.cs | 4 +++- src/NuGetGallery/Web.config | 1 + .../AsynchronousPackageValidationInitiatorFacts.cs | 7 ++++--- 5 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/NuGetGallery/Configuration/AppConfiguration.cs b/src/NuGetGallery/Configuration/AppConfiguration.cs index c90a8e60df..8974b1b76e 100644 --- a/src/NuGetGallery/Configuration/AppConfiguration.cs +++ b/src/NuGetGallery/Configuration/AppConfiguration.cs @@ -66,6 +66,8 @@ public class AppConfiguration : IAppConfiguration public bool BlockingAsynchronousPackageValidationEnabled { get; set; } + public TimeSpan AsynchronousPackageValidationDelay { get; set; } + /// /// Gets the URI to the search service /// diff --git a/src/NuGetGallery/Configuration/IAppConfiguration.cs b/src/NuGetGallery/Configuration/IAppConfiguration.cs index ce9f25c636..2d9d916cd8 100644 --- a/src/NuGetGallery/Configuration/IAppConfiguration.cs +++ b/src/NuGetGallery/Configuration/IAppConfiguration.cs @@ -88,6 +88,13 @@ public interface IAppConfiguration : ICoreMessageServiceConfiguration /// bool BlockingAsynchronousPackageValidationEnabled { get; set; } + /// + /// If is set to true, + /// this is the delay that downstream validations should wait before starting + /// to process a package. + /// + TimeSpan AsynchronousPackageValidationDelay { get; set; } + /// /// Gets the URI to the search service /// diff --git a/src/NuGetGallery/Services/AsynchronousPackageValidationInitiator.cs b/src/NuGetGallery/Services/AsynchronousPackageValidationInitiator.cs index 7f2b6eeb49..e65f5f5e43 100644 --- a/src/NuGetGallery/Services/AsynchronousPackageValidationInitiator.cs +++ b/src/NuGetGallery/Services/AsynchronousPackageValidationInitiator.cs @@ -51,7 +51,9 @@ public async Task StartValidationAsync(Package package) $"{data.PackageId} {data.PackageVersion} ({data.ValidationTrackingId})"; using (_diagnosticsSource.Activity(activityName)) { - await _enqueuer.StartValidationAsync(data); + var postponeProcessingTill = DateTimeOffset.UtcNow + _appConfiguration.AsynchronousPackageValidationDelay; + + await _enqueuer.StartValidationAsync(data, postponeProcessingTill); } if (_appConfiguration.BlockingAsynchronousPackageValidationEnabled) diff --git a/src/NuGetGallery/Web.config b/src/NuGetGallery/Web.config index ebd677c464..51fa33188f 100644 --- a/src/NuGetGallery/Web.config +++ b/src/NuGetGallery/Web.config @@ -40,6 +40,7 @@ + diff --git a/tests/NuGetGallery.Facts/Services/AsynchronousPackageValidationInitiatorFacts.cs b/tests/NuGetGallery.Facts/Services/AsynchronousPackageValidationInitiatorFacts.cs index b8db017fba..1e32385128 100644 --- a/tests/NuGetGallery.Facts/Services/AsynchronousPackageValidationInitiatorFacts.cs +++ b/tests/NuGetGallery.Facts/Services/AsynchronousPackageValidationInitiatorFacts.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; using System.Collections.Generic; using System.Threading.Tasks; using Moq; @@ -41,7 +42,7 @@ public async Task UsesProvidedPackageIdAndVersion() // Assert _enqueuer.Verify( - x => x.StartValidationAsync(It.IsAny()), + x => x.StartValidationAsync(It.IsAny(), It.IsAny()), Times.Once); Assert.Equal(1, _data.Count); Assert.NotNull(_data[0]); @@ -125,9 +126,9 @@ public FactsBase() { _enqueuer = new Mock(); _enqueuer - .Setup(x => x.StartValidationAsync(It.IsAny())) + .Setup(x => x.StartValidationAsync(It.IsAny(), It.IsAny())) .Returns(Task.CompletedTask) - .Callback(x => _data.Add(x)); + .Callback((d, o) => _data.Add(d)); _appConfiguration = new Mock(); _appConfiguration From bcfe2bdf1b823fa28fcbdfc65c1fa178dcd3cab4 Mon Sep 17 00:00:00 2001 From: Andrei Grigorev Date: Wed, 28 Feb 2018 16:08:54 -0800 Subject: [PATCH 09/24] Added new DEV environment cert to the list of accepted. (#5487) --- tests/NuGetGallery.FunctionalTests.Core/EnvironmentSettings.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/NuGetGallery.FunctionalTests.Core/EnvironmentSettings.cs b/tests/NuGetGallery.FunctionalTests.Core/EnvironmentSettings.cs index ca313b544b..f5251ba207 100644 --- a/tests/NuGetGallery.FunctionalTests.Core/EnvironmentSettings.cs +++ b/tests/NuGetGallery.FunctionalTests.Core/EnvironmentSettings.cs @@ -450,7 +450,8 @@ public static IEnumerable TrustedHttpsCertificates // returned from HTTPS browser interactions with the gallery. pieces = new List { - "8c11c16610b7a147d10bbcc6a65ce23d321c12c2", // *.nugettest.org + "8c11c16610b7a147d10bbcc6a65ce23d321c12c2", // *.nugettest.org (old) + "6cd4e9738ae52ba11e7b81da8caafbeadf89488f", // *.nugettest.org (new) "9d984f91f40d8b3a1fb29153179415523c4e64d1", // *.int.nugettest.org "3751cb513b93ee67ec9f18a1f2aec1eac87af9bc", // *.nuget.org (old) "03984834f27d5c94f46b3bb190e5a8099787268a" // *.nuget.org (new) From 04c982888b1af2ced6a2187c5bf889b5c879421f Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Wed, 28 Feb 2018 14:54:32 -0800 Subject: [PATCH 10/24] Enable TLS 1.2 in build.ps1 (#5563) Progress on https://github.com/NuGet/NuGetGallery/issues/5551 --- build.ps1 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/build.ps1 b/build.ps1 index 4eab978485..f52fd28182 100644 --- a/build.ps1 +++ b/build.ps1 @@ -25,6 +25,10 @@ trap { if (-not (Test-Path "$PSScriptRoot/build")) { New-Item -Path "$PSScriptRoot/build" -ItemType "directory" } + +# Enable TLS 1.2 since GitHub requires it. +[Net.ServicePointManager]::SecurityProtocol = [Net.ServicePointManager]::SecurityProtocol -bor [Net.SecurityProtocolType]::Tls12 + wget -UseBasicParsing -Uri "https://raw.githubusercontent.com/NuGet/ServerCommon/$BuildBranch/build/init.ps1" -OutFile "$PSScriptRoot/build/init.ps1" . "$PSScriptRoot/build/init.ps1" -BuildBranch "$BuildBranch" From dc09a1d247b8a4bdf2ac57fa7e59772e356f173b Mon Sep 17 00:00:00 2001 From: Scott Bommarito Date: Thu, 1 Mar 2018 12:26:41 -0800 Subject: [PATCH 11/24] [Organizations] Add Organization (#5526) --- src/Bootstrap/dist/css/bootstrap-theme.css | 3 + .../less/theme/page-manage-packages.less | 4 + src/NuGetGallery/App_Start/AppActivator.cs | 5 + .../App_Start/DefaultDependenciesModule.cs | 5 + src/NuGetGallery/App_Start/Routes.cs | 5 + src/NuGetGallery/Constants.cs | 11 + .../img/manage-organizations-260x150.png | Bin 0 -> 5405 bytes .../gallery/img/manage-organizations.svg | 338 ++++++++++++++++++ .../Controllers/OrganizationsController.cs | 34 ++ .../Infrastructure/UserSafeException.cs | 4 +- src/NuGetGallery/NuGetGallery.csproj | 2 + src/NuGetGallery/RouteName.cs | 1 + src/NuGetGallery/Scripts/gallery/md5.js | 251 +++++++++++++ .../Scripts/gallery/page-add-organization.js | 44 +++ .../Security/ISecurityPolicyService.cs | 2 +- .../Security/SecurityPolicyService.cs | 7 +- src/NuGetGallery/Services/IUserService.cs | 2 + src/NuGetGallery/Services/UserService.cs | 98 ++++- src/NuGetGallery/Strings.Designer.cs | 38 +- src/NuGetGallery/Strings.resx | 6 +- src/NuGetGallery/UrlExtensions.cs | 8 + .../ViewModels/AddOrganizationViewModel.cs | 20 ++ .../ViewModels/ChangeEmailViewModel.cs | 2 +- src/NuGetGallery/ViewModels/LogOnViewModel.cs | 13 +- .../ManageOrganizationsItemViewModel.cs | 2 +- .../ViewModels/ReportAbuseViewModel.cs | 2 +- .../Views/Organizations/Add.cshtml | 70 ++++ src/NuGetGallery/Views/Shared/Confirm.cshtml | 4 +- .../Views/Users/Organizations.cshtml | 32 +- .../OrganizationsControllerFacts.cs | 72 ++++ .../EmailValidationRegex.cs | 12 +- .../Security/SecurePushSubscriptionFacts.cs | 88 +++-- .../Services/UserServiceFacts.cs | 168 ++++++++- .../TestUtils/TestServiceUtility.cs | 4 + 34 files changed, 1268 insertions(+), 89 deletions(-) create mode 100644 src/NuGetGallery/Content/gallery/img/manage-organizations-260x150.png create mode 100644 src/NuGetGallery/Content/gallery/img/manage-organizations.svg create mode 100644 src/NuGetGallery/Scripts/gallery/md5.js create mode 100644 src/NuGetGallery/Scripts/gallery/page-add-organization.js create mode 100644 src/NuGetGallery/ViewModels/AddOrganizationViewModel.cs create mode 100644 src/NuGetGallery/Views/Organizations/Add.cshtml diff --git a/src/Bootstrap/dist/css/bootstrap-theme.css b/src/Bootstrap/dist/css/bootstrap-theme.css index 8af7c600ef..4ac1f366b4 100644 --- a/src/Bootstrap/dist/css/bootstrap-theme.css +++ b/src/Bootstrap/dist/css/bootstrap-theme.css @@ -1065,6 +1065,9 @@ img.reserved-indicator-icon { margin-top: 0; margin-bottom: 10px; } +.page-manage-packages .subtitle { + margin-top: 24px; +} .page-manage-packages .form-section-title { margin-top: 40px; } diff --git a/src/Bootstrap/less/theme/page-manage-packages.less b/src/Bootstrap/less/theme/page-manage-packages.less index a2f62bba4b..d19d478642 100644 --- a/src/Bootstrap/less/theme/page-manage-packages.less +++ b/src/Bootstrap/less/theme/page-manage-packages.less @@ -10,6 +10,10 @@ margin-top: 0; } + .subtitle { + margin-top: 24px; + } + .form-section-title { margin-top: @section-margin-top; diff --git a/src/NuGetGallery/App_Start/AppActivator.cs b/src/NuGetGallery/App_Start/AppActivator.cs index ac89a4c22b..ab25f14c7d 100644 --- a/src/NuGetGallery/App_Start/AppActivator.cs +++ b/src/NuGetGallery/App_Start/AppActivator.cs @@ -235,6 +235,11 @@ private static void BundlingPostStart() var manageOrganizationScriptBundle = new ScriptBundle("~/Scripts/gallery/page-manage-organization.min.js") .Include("~/Scripts/gallery/page-manage-organization.js"); BundleTable.Bundles.Add(manageOrganizationScriptBundle); + + var addOrganizationScriptBundle = new ScriptBundle("~/Scripts/gallery/page-add-organization.min.js") + .Include("~/Scripts/gallery/page-add-organization.js") + .Include("~/Scripts/gallery/md5.js"); + BundleTable.Bundles.Add(addOrganizationScriptBundle); } private static void AppPostStart(IAppConfiguration configuration) diff --git a/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs b/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs index d8870ddfc0..056b29d71f 100644 --- a/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs +++ b/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs @@ -154,6 +154,11 @@ protected override void Load(ContainerBuilder builder) .As>() .InstancePerLifetimeScope(); + builder.RegisterType>() + .AsSelf() + .As>() + .InstancePerLifetimeScope(); + builder.RegisterType() .AsSelf() .As() diff --git a/src/NuGetGallery/App_Start/Routes.cs b/src/NuGetGallery/App_Start/Routes.cs index a12cbab7d5..62924bd000 100644 --- a/src/NuGetGallery/App_Start/Routes.cs +++ b/src/NuGetGallery/App_Start/Routes.cs @@ -297,6 +297,11 @@ public static void RegisterUIRoutes(RouteCollection routes) "account/{action}", new { controller = "Users", action = "Account" }); + routes.MapRoute( + RouteName.AddOrganization, + "organization/add", + new { controller = "Organizations", action = "Add" }); + routes.MapRoute( RouteName.OrganizationAccount, "organization/{accountName}/{action}", diff --git a/src/NuGetGallery/Constants.cs b/src/NuGetGallery/Constants.cs index 1a16b1a0cc..a6dacd4517 100644 --- a/src/NuGetGallery/Constants.cs +++ b/src/NuGetGallery/Constants.cs @@ -59,6 +59,17 @@ public static class Constants public const string UrlValidationRegEx = @"(https?):\/\/[^ ""]+$"; public const string UrlValidationErrorMessage = "This doesn't appear to be a valid HTTP/HTTPS URL"; + // Note: regexes must be tested to work in JavaScript + // We do NOT follow strictly the RFCs at this time, and we choose not to support many obscure email address variants. + // Specifically the following are not supported by-design: + // * Addresses containing () or [] + // * Second parts with no dots (i.e. foo@localhost or foo@com) + // * Addresses with quoted (" or ') first parts + // * Addresses with IP Address second parts (foo@[127.0.0.1]) + internal const string EmailValidationRegexFirstPart = @"[-A-Za-z0-9!#$%&'*+\/=?^_`{|}~\.]+"; + internal const string EmailValidationRegexSecondPart = @"[A-Za-z0-9]+[\w\.-]*[A-Za-z0-9]*\.[A-Za-z0-9][A-Za-z\.]*[A-Za-z]"; + public const string EmailValidationRegex = "^" + EmailValidationRegexFirstPart + "@" + EmailValidationRegexSecondPart + "$"; + internal const string ApiKeyHeaderName = "X-NuGet-ApiKey"; // X-NuGet-Client-Version header was deprecated and replaced with X-NuGet-Protocol-Version header // It stays here for backwards compatibility diff --git a/src/NuGetGallery/Content/gallery/img/manage-organizations-260x150.png b/src/NuGetGallery/Content/gallery/img/manage-organizations-260x150.png new file mode 100644 index 0000000000000000000000000000000000000000..fa489d8a9f07c7229b67c94360bef6030039312e GIT binary patch literal 5405 zcmV+&72@iNP)#~B`s0S7P~;lqUl z-I7*{01+Qjm58d6*HV!30~r!hMQy0C(?t0ZrNPaQM*R`nQd{LmQ%IvojiSmViWDg- zg}7A|DO4PPfQ#yhlhuQW(6wY9Zm|}WOV`&f_g*%Ep^hgWx?zPH7AP^xWLWi{@{ZT*p3}Ls;0Ga z<;wC^4UiV3328$fK5C)eDjR`7F$76u027S%y2;?tM;|Q*44otTh7B8H z<$37PA=c5+!Hyg`!WJx8F!ebE9>4kKn{u4Ng{Rxv+V)Xx5lA86l7VK$3@VZV06H!N z3g3M5&Di;ZGI;5wm!^>c0P=*qJ(NPPRaOFl6ape49N;gy^E2p>l^!F4QgCY$y!O!LVh$`761-SNTKU%PfK+q!kDR8oMv9(?dY z_s5!5b^?J2K_FgpzJYlFvuDqqQUjlU`suVU@7lFXDk(tzP>zp2`p9dJ!fRB0gi4^G zVmNgG>l2PSKZD6ZKnQ>@zW9Q5b#<|)o_dP)^z=;oEzBMG`s=URo;`bn#1h^ zXU>%KhBAzgkF(>)kGsFRp4K}MNFj*9cIO*dq-2u;06O8}!-v_6FTQx+4`nC?nBxHX zLpcCYHYj7Idms>q5TZkOzFD+rk>{8EW*cIX0;KD;D9WRZI#nZq2qC_^aNoX%9;0tn z$c`y^l(Asl3xPm{5ChZ50A>b8UAZWe6rhamnUgI3Hf{tW1R?}CA;f`9QXoPg5FsF_ zZ|56lFi|uq%orPD^FP_h>dze#QgDy5lFL9KLQoM-QaF0_DErTeqio*awzA13YuW9G zHjse`L36wjM0%CSE|bUpivv`11TWA`#elH(DV*9S&F*kQ zDq(DB%1scd3~Zb~f4+P<&_mm{6G$Pb$jHcKKp+qy;J|Rcfi26@LIxgXHK;lXL14bwVOi1QZ{iy3&7 zk;s5Rgdhl?Ip18ocu^TL@SFbZQO3`x8VN)QVleG|0|?f+19q@M32am%TN%JiN7#e~ z@~>1r_dO5@Lm7!@;Ze2n~~fVEeMc!NL31CSbb>uoDvi@`rLj{&v}%?t*m> z1cE2($poPRd!y<08#s-5@#4khJr&_pCFq#2HbHjG0Jdd=DbV&-Y7jfvv}u!9*$D)H zx0Xy09_~Y0V0rPHHEX2136*f-#0j^%K!>#u$YnrO3DbOar|&*x+@Uk<_Cr6CP6kjG zkJtgJ0|F@oM3n#spG-Sl8O~<__aRAKy?T|MK7HE#@h+|oI;qwOq!4gbg5mrCIwEvP zKR}=_$g%K#fEdLSPdwrKh|z}(3{nRKA_Qdc5!d7|PxH$RN(vBoJbU(Ri5r-=**q!1KP3hUi51psv1W5GJw3-6B!VqBGH+O@9#f(-)Tq!QF{5w)uv=pC~>8*%2Enj-2aDJjc~!H zr;Uw`~v2jyW*^A;4S(4LK`lG@YCX_DQHWorygg(zJ-Xm_j1h%ka<1G%cd^P7H%q>_YF;=h_xdY@ZHI)W3XlL?|!fVg0%fJnK zH-UOIIm0eerBQL!U`y|2t25{oTpl2Ses*thQZa%00%dbrMqa#*W~6`+4oGknys%z| zf~rrTaV3Eq&%2~{|00lzQ&*|UCWM6v8gwDGDdkCA^;4;nRGFj@NS)N&sI8H? zlWo7WmVrPn1KB1qLeQzPr3dgBkW}0>v?N>~TGj}mR(xR^l?bVHsiYBtwJ?T2NOL>M zWj+788f$DCbvaU1DYi!;ON5|8<03*(e?+7@N)UA!Z(IcOle45Q(Sw>Y2_a97Ep=!n z6Vz7QsjD%A5+SIs>RQO`$+_L+G7t#q8B8+ultG1{B1vHNJ3)bL;ScOsMy*XOHglsP=l7cGgGM9QnPzXd!ib&HGx(%w3R3Qt5pivhiX-ZHZ^|jWx z&i}bhUp{i2YQ74-y4+Xz_tLgR@@~3ua`W9ksKrPB7&#tVT@KG~Dqn)pk_AFgX}6*S z)ZMFN$<0%V<^AyR(H;C{hf4O?bn4fkfAg>JZR6MTsj7EbE(0x+Nf{7Ew!j#lIgctJ z<1qwfFh-3{d0c(q5i){5GVzo_SEsRkpPE!4g|Li_peQgO%huPLF}Y9I^TTbw?e_ZL zjW-y4-bGE0vB7+DVV^4vtJvrjLq=9T^WI|J0XiU@UbR;-x1pFyg`nW3BzfxYYGb^< zv8McXl_0}qP>m2Y>T=a_T_E%8V)m)#MgE0wttf zRiY78EgsZDibZjEL6tJpB0K~_k)j`=#0*<*8IL_ z-7oicS0#=&%8Cz2elx$eAaxeiRAs6kJLc4D@F(?`pIlvxk%Y;w4o(33Xs}8aybem% zpG8g%J^XbizuGh2hS3O$f{M|Qbuv)FXKsl=4ko`k_@6}73KK%sf_SAGPzGAc%ik$b zO)A-bvC37er$6g4e)}PfBWnA}RxGEObUD(fyio~3Rd$VuVhwag6`s4tZ94~vFx68C zsN4cKc7p!) zWLJK{sNwsgiTe4x!@cMJo$>OMs|^K+pqsyJXS7zMygc}0UdP+^SDyZSZNb|7`}_Y| zO#$SI5X8Yy^uB?p?wfdXcy&*t-|+?r%Atc3Ln)yS(96U>drzI@xAqCxYs-9??Ktn| zIp6P}UDDc~LAxm(LnzjW9f2s0S?lIylYySmM%@|G4xo%qJAMA77ib})lsZDNr1Y*D zBG}#p_J7!%F&jdgX^o(^Kp9MnePnn&M2XvHU+M_KB8AQxA_ZuWvUglc{iEHx6Svr% z;mhpy`4epXSJP#-(~)LnE6pcTtu}p`AC- z9$A4#p3hX{c=z3Rvl2@PtFk;v_{IY(GSQ&!-Wq0i$41zl!3*XqFFfChnU}OZ74M8* zFR;<;Y%-f9(DUl6uXZaE%V4!e2%HpGIY@D8|3h=^e-(c>dF4AcKDIwicFFPk%kj2m zrk)U(_-ES&oBkA|AUcobW8Y5wJyF+SUYWJB-g* zu&Alkl_!iuTT~dCd0Cugz<&H`M3nZ`Fd$&?u*uo@dFSt1pLv7b_50%bq0f7M{F~3a z1=t>zT$fLpirV%9^q&JrHlKzh4tvFFtOZwN6%DU)_dBFq(xl+b|6oc9p(Mcd(Oqd7 z5^O){L(D?JNv0ShzEXRHz$-!X7-i5Z-A5iKO$g0C<-#OK(T&rLqYzx3@t{OPC?dMX zZrikEfe@MIs*qs6P>+|_nzXEXt&8IvvIsg{Y zEDYJsat=F`5+DQv*=3Lsr;vV>TM?NMOytV0)-m*i2%#Xs#(pD~=-A1@h~%n!q#=_K z`d#Fd#yEu(LXhF#=R6vM6hg3E6e$tl3TR2dWhf$#R$9#m*V1%qch0>{uh0CZXc1#Xf9h zB7K6A6{E>XRtM7_xw_&+qVFxN5-mj_bfNY@R80oE`c*|Ui#mxHveH4alg+PR$fParY z=_%-03{!+4d4f`s0mXg~QW8ltLDSW%C?r{gFyQ0Hq$W-wlMvFns3~YCJ19Gj;F^ph z+g)(lMs(xs51MX;2}x1vP?SQzR0*hY3T#5a_U|&JC+A8Lq^`*OPvT4n6S>icCB;$#Wp%azCn4mC5aj#! zc~^(Tz`+QWB}*Yx3#Q2+L0h_uyk|2+x|Jp*MJ1UyC?dNIsEeG?2tks6uSu@#pyZLm znyjPX{R@vHxX8U)x_Uqxn3C;*H7FD@;}mrg8Y`xMUyxAQrHGX!LnRaxAOx((twyzU z158MALRO4L<|3y;ng{{azqgEyJ?SZI*-Kt>9!Y`e2{iUf4K$CG?!tg=Ocj+_!tAiv zaSBXAz}~c^;KQ1UDFoL9C2WOIP^2>qAjwxFK&J{5Ln%tKhlxoDuKG@u%aDadO_erA zku9O)bOsp;L3HB`LNq#Z8;df67emB9;@VtKBOzpYQIi)A`pEh$#t$$Fp<=pWCuMPB zFD76U>_tw(;}n>LfbHMg#!hlkQ#BW(mIy)gV=!mBT9x=gKT``>B#9oUKp_N4i<)HY z0T8rxpGL2UaLTeVd4X_R08%QCo@%qz~nA!!X$)&m<>XgU|$vdXW{qj zQhkLg3ejL{A+?K~>eAR?T}B>6tk|fo2=)XeRRW4}?G%(~aur;LVubW$5hSKcK#5ae z5dyY|%jxq>P^k_KfW5cK} zlgt&dMP!!&B~Brn5L|_;l|@Z5JBcbY6g@kd;Fw9#k#5ez24U~tNWdmKa{CfWIv5H; zhJRm>P{DD_jx|9^bBvBk5JHrllB3qSMGTQ*C=w5CnWo;-w8#mA5Ohq5=33OG+JqE8 z1{X;PEy!dmjIaa~P0pk7_s8r=vg$uIi=67?{J5{Lui#Py?CjaI&6AUp_ez=1=lFMo zN~UYqt`&xdhs%G@ojZ4-$n=Mg|0>@&a;p6I$UM#L>-$rzw9lM5!|vRwz*Y`&h8}fFl+3j@BoHO--z!p+Q*MkDU30t>PEp^va*I@Awr5wk>#t zv6U-V#xCFO+qc=dbLYx`FJ8Qu&7VJC8ChW8D<|o4M-`ne+$-F3T$`>r^$uHp>7T;t zI0%_7Sg?RCTC_-b`j;N4#0lZG~X*Vosh7}-`I0Fh+i z#?3j)Vy9u34^#ql=D4tJYmJyn`3cT@6$$A0HW@l?5Vh`a38>e=^4sbI&UzCGXB+Ub zJE->qd;rg(5du$KtK=uz1;c8vFgiFJ3KniPG{sJXOU3)j9S0K=6Uq{T$ouqa^gsyE zvEsI`iTZic2|>hQ;DuSUW_b=49;6UBhA|@oAwUM#8G%4Z3tEH56G9KOJYn!liezH~ zAr&yf1Q(;~?x%?q2n6K;eYBO#Kp+qZ1OkCTAP@xM{{jpEI7QJ{7UbGN00000NkvXX Hu0mjf2Zs6X literal 0 HcmV?d00001 diff --git a/src/NuGetGallery/Content/gallery/img/manage-organizations.svg b/src/NuGetGallery/Content/gallery/img/manage-organizations.svg new file mode 100644 index 0000000000..806897b943 --- /dev/null +++ b/src/NuGetGallery/Content/gallery/img/manage-organizations.svg @@ -0,0 +1,338 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/NuGetGallery/Controllers/OrganizationsController.cs b/src/NuGetGallery/Controllers/OrganizationsController.cs index f728d4a3fa..2ae76cab8a 100644 --- a/src/NuGetGallery/Controllers/OrganizationsController.cs +++ b/src/NuGetGallery/Controllers/OrganizationsController.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; using System.Linq; using System.Net; using System.Net.Mail; @@ -45,6 +46,39 @@ protected override void SendEmailChangedConfirmationNotice(User account) MessageService.SendEmailChangeConfirmationNotice(new MailAddress(account.UnconfirmedEmailAddress, account.Username), confirmationUrl); } + [HttpGet] + [UIAuthorize] + public ActionResult Add() + { + return View(new AddOrganizationViewModel()); + } + + [HttpPost] + [UIAuthorize] + [ValidateAntiForgeryToken] + public async Task Add(AddOrganizationViewModel model) + { + var organizationName = model.OrganizationName; + var organizationEmailAddress = model.OrganizationEmailAddress; + var adminUser = GetCurrentUser(); + + string errorMessage; + + try + { + var organization = await UserService.AddOrganizationAsync(organizationName, organizationEmailAddress, adminUser); + SendNewAccountEmail(organization); + return RedirectToAction(nameof(ManageOrganization), new { accountName = organization.Username }); + } + catch (EntityException e) + { + errorMessage = e.Message; + } + + TempData["ErrorMessage"] = errorMessage; + return View(model); + } + [HttpGet] [UIAuthorize] public virtual ActionResult ManageOrganization(string accountName) diff --git a/src/NuGetGallery/Infrastructure/UserSafeException.cs b/src/NuGetGallery/Infrastructure/UserSafeException.cs index e2b2024f00..8830db406e 100644 --- a/src/NuGetGallery/Infrastructure/UserSafeException.cs +++ b/src/NuGetGallery/Infrastructure/UserSafeException.cs @@ -1,9 +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; -using System.Collections.Generic; -using System.Linq; -using System.Web; namespace NuGetGallery { diff --git a/src/NuGetGallery/NuGetGallery.csproj b/src/NuGetGallery/NuGetGallery.csproj index e28afd075e..54615521be 100644 --- a/src/NuGetGallery/NuGetGallery.csproj +++ b/src/NuGetGallery/NuGetGallery.csproj @@ -1070,6 +1070,7 @@ + @@ -2054,6 +2055,7 @@ + diff --git a/src/NuGetGallery/RouteName.cs b/src/NuGetGallery/RouteName.cs index 55ef10f202..c5c2debd5f 100644 --- a/src/NuGetGallery/RouteName.cs +++ b/src/NuGetGallery/RouteName.cs @@ -7,6 +7,7 @@ public static class RouteName { public const string Account = "Account"; public const string OrganizationAccount = "ManageOrganization"; + public const string AddOrganization = "AddOrganization"; public const string ChangeOrganizationEmailSubscription = "ChangeOrganizationEmailSubscription"; public const string TransformToOrganization = "TransformToOrganization"; public const string TransformToOrganizationConfirmation = "ConfirmTransformToOrganization"; diff --git a/src/NuGetGallery/Scripts/gallery/md5.js b/src/NuGetGallery/Scripts/gallery/md5.js new file mode 100644 index 0000000000..584b405471 --- /dev/null +++ b/src/NuGetGallery/Scripts/gallery/md5.js @@ -0,0 +1,251 @@ +/** + * + * MD5 (Message-Digest Algorithm) + * http://www.webtoolkit.info/ + * + **/ + +(function (exports) { + 'use strict'; + + var MD5 = function (string) { + + function RotateLeft(lValue, iShiftBits) { + return (lValue << iShiftBits) | (lValue >>> (32 - iShiftBits)); + } + + function AddUnsigned(lX, lY) { + var lX4, lY4, lX8, lY8, lResult; + lX8 = (lX & 0x80000000); + lY8 = (lY & 0x80000000); + lX4 = (lX & 0x40000000); + lY4 = (lY & 0x40000000); + lResult = (lX & 0x3FFFFFFF) + (lY & 0x3FFFFFFF); + if (lX4 & lY4) { + return (lResult ^ 0x80000000 ^ lX8 ^ lY8); + } + if (lX4 | lY4) { + if (lResult & 0x40000000) { + return (lResult ^ 0xC0000000 ^ lX8 ^ lY8); + } else { + return (lResult ^ 0x40000000 ^ lX8 ^ lY8); + } + } else { + return (lResult ^ lX8 ^ lY8); + } + } + + function F(x, y, z) { + return (x & y) | ((~x) & z); + } + + function G(x, y, z) { + return (x & z) | (y & (~z)); + } + + function H(x, y, z) { + return (x ^ y ^ z); + } + + function I(x, y, z) { + return (y ^ (x | (~z))); + } + + function FF(a, b, c, d, x, s, ac) { + a = AddUnsigned(a, AddUnsigned(AddUnsigned(F(b, c, d), x), ac)); + return AddUnsigned(RotateLeft(a, s), b); + }; + + function GG(a, b, c, d, x, s, ac) { + a = AddUnsigned(a, AddUnsigned(AddUnsigned(G(b, c, d), x), ac)); + return AddUnsigned(RotateLeft(a, s), b); + }; + + function HH(a, b, c, d, x, s, ac) { + a = AddUnsigned(a, AddUnsigned(AddUnsigned(H(b, c, d), x), ac)); + return AddUnsigned(RotateLeft(a, s), b); + }; + + function II(a, b, c, d, x, s, ac) { + a = AddUnsigned(a, AddUnsigned(AddUnsigned(I(b, c, d), x), ac)); + return AddUnsigned(RotateLeft(a, s), b); + }; + + function ConvertToWordArray(string) { + var lWordCount; + var lMessageLength = string.length; + var lNumberOfWords_temp1 = lMessageLength + 8; + var lNumberOfWords_temp2 = (lNumberOfWords_temp1 - (lNumberOfWords_temp1 % 64)) / 64; + var lNumberOfWords = (lNumberOfWords_temp2 + 1) * 16; + var lWordArray = Array(lNumberOfWords - 1); + var lBytePosition = 0; + var lByteCount = 0; + while (lByteCount < lMessageLength) { + lWordCount = (lByteCount - (lByteCount % 4)) / 4; + lBytePosition = (lByteCount % 4) * 8; + lWordArray[lWordCount] = (lWordArray[lWordCount] | (string.charCodeAt(lByteCount) << lBytePosition)); + lByteCount++; + } + lWordCount = (lByteCount - (lByteCount % 4)) / 4; + lBytePosition = (lByteCount % 4) * 8; + lWordArray[lWordCount] = lWordArray[lWordCount] | (0x80 << lBytePosition); + lWordArray[lNumberOfWords - 2] = lMessageLength << 3; + lWordArray[lNumberOfWords - 1] = lMessageLength >>> 29; + return lWordArray; + }; + + function WordToHex(lValue) { + var WordToHexValue = "", + WordToHexValue_temp = "", + lByte, lCount; + for (lCount = 0; lCount <= 3; lCount++) { + lByte = (lValue >>> (lCount * 8)) & 255; + WordToHexValue_temp = "0" + lByte.toString(16); + WordToHexValue = WordToHexValue + WordToHexValue_temp.substr(WordToHexValue_temp.length - 2, 2); + } + return WordToHexValue; + }; + + function Utf8Encode(string) { + string = string.replace(/\r\n/g, "\n"); + var utftext = ""; + + for (var n = 0; n < string.length; n++) { + + var c = string.charCodeAt(n); + + if (c < 128) { + utftext += String.fromCharCode(c); + } else if ((c > 127) && (c < 2048)) { + utftext += String.fromCharCode((c >> 6) | 192); + utftext += String.fromCharCode((c & 63) | 128); + } else { + utftext += String.fromCharCode((c >> 12) | 224); + utftext += String.fromCharCode(((c >> 6) & 63) | 128); + utftext += String.fromCharCode((c & 63) | 128); + } + + } + + return utftext; + }; + + var x = Array(); + var k, AA, BB, CC, DD, a, b, c, d; + var S11 = 7, + S12 = 12, + S13 = 17, + S14 = 22; + var S21 = 5, + S22 = 9, + S23 = 14, + S24 = 20; + var S31 = 4, + S32 = 11, + S33 = 16, + S34 = 23; + var S41 = 6, + S42 = 10, + S43 = 15, + S44 = 21; + + string = Utf8Encode(string); + + x = ConvertToWordArray(string); + + a = 0x67452301; + b = 0xEFCDAB89; + c = 0x98BADCFE; + d = 0x10325476; + + for (k = 0; k < x.length; k += 16) { + AA = a; + BB = b; + CC = c; + DD = d; + a = FF(a, b, c, d, x[k + 0], S11, 0xD76AA478); + d = FF(d, a, b, c, x[k + 1], S12, 0xE8C7B756); + c = FF(c, d, a, b, x[k + 2], S13, 0x242070DB); + b = FF(b, c, d, a, x[k + 3], S14, 0xC1BDCEEE); + a = FF(a, b, c, d, x[k + 4], S11, 0xF57C0FAF); + d = FF(d, a, b, c, x[k + 5], S12, 0x4787C62A); + c = FF(c, d, a, b, x[k + 6], S13, 0xA8304613); + b = FF(b, c, d, a, x[k + 7], S14, 0xFD469501); + a = FF(a, b, c, d, x[k + 8], S11, 0x698098D8); + d = FF(d, a, b, c, x[k + 9], S12, 0x8B44F7AF); + c = FF(c, d, a, b, x[k + 10], S13, 0xFFFF5BB1); + b = FF(b, c, d, a, x[k + 11], S14, 0x895CD7BE); + a = FF(a, b, c, d, x[k + 12], S11, 0x6B901122); + d = FF(d, a, b, c, x[k + 13], S12, 0xFD987193); + c = FF(c, d, a, b, x[k + 14], S13, 0xA679438E); + b = FF(b, c, d, a, x[k + 15], S14, 0x49B40821); + a = GG(a, b, c, d, x[k + 1], S21, 0xF61E2562); + d = GG(d, a, b, c, x[k + 6], S22, 0xC040B340); + c = GG(c, d, a, b, x[k + 11], S23, 0x265E5A51); + b = GG(b, c, d, a, x[k + 0], S24, 0xE9B6C7AA); + a = GG(a, b, c, d, x[k + 5], S21, 0xD62F105D); + d = GG(d, a, b, c, x[k + 10], S22, 0x2441453); + c = GG(c, d, a, b, x[k + 15], S23, 0xD8A1E681); + b = GG(b, c, d, a, x[k + 4], S24, 0xE7D3FBC8); + a = GG(a, b, c, d, x[k + 9], S21, 0x21E1CDE6); + d = GG(d, a, b, c, x[k + 14], S22, 0xC33707D6); + c = GG(c, d, a, b, x[k + 3], S23, 0xF4D50D87); + b = GG(b, c, d, a, x[k + 8], S24, 0x455A14ED); + a = GG(a, b, c, d, x[k + 13], S21, 0xA9E3E905); + d = GG(d, a, b, c, x[k + 2], S22, 0xFCEFA3F8); + c = GG(c, d, a, b, x[k + 7], S23, 0x676F02D9); + b = GG(b, c, d, a, x[k + 12], S24, 0x8D2A4C8A); + a = HH(a, b, c, d, x[k + 5], S31, 0xFFFA3942); + d = HH(d, a, b, c, x[k + 8], S32, 0x8771F681); + c = HH(c, d, a, b, x[k + 11], S33, 0x6D9D6122); + b = HH(b, c, d, a, x[k + 14], S34, 0xFDE5380C); + a = HH(a, b, c, d, x[k + 1], S31, 0xA4BEEA44); + d = HH(d, a, b, c, x[k + 4], S32, 0x4BDECFA9); + c = HH(c, d, a, b, x[k + 7], S33, 0xF6BB4B60); + b = HH(b, c, d, a, x[k + 10], S34, 0xBEBFBC70); + a = HH(a, b, c, d, x[k + 13], S31, 0x289B7EC6); + d = HH(d, a, b, c, x[k + 0], S32, 0xEAA127FA); + c = HH(c, d, a, b, x[k + 3], S33, 0xD4EF3085); + b = HH(b, c, d, a, x[k + 6], S34, 0x4881D05); + a = HH(a, b, c, d, x[k + 9], S31, 0xD9D4D039); + d = HH(d, a, b, c, x[k + 12], S32, 0xE6DB99E5); + c = HH(c, d, a, b, x[k + 15], S33, 0x1FA27CF8); + b = HH(b, c, d, a, x[k + 2], S34, 0xC4AC5665); + a = II(a, b, c, d, x[k + 0], S41, 0xF4292244); + d = II(d, a, b, c, x[k + 7], S42, 0x432AFF97); + c = II(c, d, a, b, x[k + 14], S43, 0xAB9423A7); + b = II(b, c, d, a, x[k + 5], S44, 0xFC93A039); + a = II(a, b, c, d, x[k + 12], S41, 0x655B59C3); + d = II(d, a, b, c, x[k + 3], S42, 0x8F0CCC92); + c = II(c, d, a, b, x[k + 10], S43, 0xFFEFF47D); + b = II(b, c, d, a, x[k + 1], S44, 0x85845DD1); + a = II(a, b, c, d, x[k + 8], S41, 0x6FA87E4F); + d = II(d, a, b, c, x[k + 15], S42, 0xFE2CE6E0); + c = II(c, d, a, b, x[k + 6], S43, 0xA3014314); + b = II(b, c, d, a, x[k + 13], S44, 0x4E0811A1); + a = II(a, b, c, d, x[k + 4], S41, 0xF7537E82); + d = II(d, a, b, c, x[k + 11], S42, 0xBD3AF235); + c = II(c, d, a, b, x[k + 2], S43, 0x2AD7D2BB); + b = II(b, c, d, a, x[k + 9], S44, 0xEB86D391); + a = AddUnsigned(a, AA); + b = AddUnsigned(b, BB); + c = AddUnsigned(c, CC); + d = AddUnsigned(d, DD); + } + + var temp = WordToHex(a) + WordToHex(b) + WordToHex(c) + WordToHex(d); + + return temp.toLowerCase(); + }; + + // Expose the class either via AMD, CommonJS or the global object + if (typeof define === 'function' && define.amd) { + define(function () { + return MD5; + }); + } else if (typeof module === 'object' && module.exports) { + module.exports = MD5; + } else { + exports.MD5 = MD5; + } +})(this); \ No newline at end of file diff --git a/src/NuGetGallery/Scripts/gallery/page-add-organization.js b/src/NuGetGallery/Scripts/gallery/page-add-organization.js new file mode 100644 index 0000000000..e0949ce53e --- /dev/null +++ b/src/NuGetGallery/Scripts/gallery/page-add-organization.js @@ -0,0 +1,44 @@ +$(function () { + 'use strict'; + + var _gravatarTimeout = 0; + var _gravatarDelay = 500; + var _gravatarIsUpdating = false; + + var _emailBox = $("#" + emailBoxGroupId + " > input"); + var _gravatar = $("#" + gravatarId); + var _template = "https://secure.gravatar.com/avatar/{email}?s=512&r=g&d=retro"; + + // When the email in the form changes, update the Gravatar displayed as the logo. + // If the user is continuing to type, wait until they are done before updating the Gravatar. + _emailBox.on("keyup", function (e) { + if ((e.keyCode >= 46 && e.keyCode <= 90) // delete, 0-9, a-z + || (e.keyCode >= 96 && e.keyCode <= 111) // numpad + || (e.keyCode >= 186) // punctuation + || (e.keyCode === 8)) // backspace + { + clearTimeout(_gravatarTimeout); + _gravatarTimeout = setTimeout(UpdateGravatar, _gravatarDelay); + } + }); + + // When the user switches focus from the email textbox, update the Gravatar displayed as the logo. + // This is because the user can change the email without typing (for example, by pasting contents from somewhere else). + _emailBox.on("blur", function (e) { + if (!_gravatarTimeout) { + UpdateGravatar() + } + }); + + function UpdateGravatar() { + _gravatarTimeout = 0; + var src = defaultImage; + + var email = _emailBox.val(); + if (email.match(emailValidationRegex)) { + src = _template.replace("{email}", MD5(email)); + } + + _gravatar.attr("src", src); + } +}); \ No newline at end of file diff --git a/src/NuGetGallery/Security/ISecurityPolicyService.cs b/src/NuGetGallery/Security/ISecurityPolicyService.cs index 3c01b390dc..e3155bdcf8 100644 --- a/src/NuGetGallery/Security/ISecurityPolicyService.cs +++ b/src/NuGetGallery/Security/ISecurityPolicyService.cs @@ -36,7 +36,7 @@ public interface ISecurityPolicyService /// /// Subscribe a user to one or more security policies. /// - Task SubscribeAsync(User user, IUserSecurityPolicySubscription subscription); + Task SubscribeAsync(User user, IUserSecurityPolicySubscription subscription, bool commitChanges = true); /// /// Unsubscribe a user from one or more security policies. diff --git a/src/NuGetGallery/Security/SecurityPolicyService.cs b/src/NuGetGallery/Security/SecurityPolicyService.cs index 520b9274e9..f70fe5708e 100644 --- a/src/NuGetGallery/Security/SecurityPolicyService.cs +++ b/src/NuGetGallery/Security/SecurityPolicyService.cs @@ -253,7 +253,7 @@ public Task SubscribeAsync(User user, string subscriptionName) /// Subscribe a user to one or more security policies. /// /// True if user was subscribed, false if not (i.e., was already subscribed). - public async Task SubscribeAsync(User user, IUserSecurityPolicySubscription subscription) + public async Task SubscribeAsync(User user, IUserSecurityPolicySubscription subscription, bool commitChanges = true) { if (user == null) { @@ -282,7 +282,10 @@ public async Task SubscribeAsync(User user, IUserSecurityPolicySubscriptio await Auditing.SaveAuditRecordAsync( new UserAuditRecord(user, AuditedUserAction.SubscribeToPolicies, subscription.Policies)); - await EntitiesContext.SaveChangesAsync(); + if (commitChanges) + { + await EntitiesContext.SaveChangesAsync(); + } Diagnostics.Information($"User is now subscribed to '{subscription.SubscriptionName}'."); diff --git a/src/NuGetGallery/Services/IUserService.cs b/src/NuGetGallery/Services/IUserService.cs index d53fe8111f..003202c3ff 100644 --- a/src/NuGetGallery/Services/IUserService.cs +++ b/src/NuGetGallery/Services/IUserService.cs @@ -41,5 +41,7 @@ public interface IUserService Task RequestTransformToOrganizationAccount(User accountToTransform, User adminUser); Task TransformUserToOrganization(User accountToTransform, User adminUser, string token); + + Task AddOrganizationAsync(string username, string emailAddress, User adminUser); } } \ No newline at end of file diff --git a/src/NuGetGallery/Services/UserService.cs b/src/NuGetGallery/Services/UserService.cs index 97a024afe1..ffaa13bee2 100644 --- a/src/NuGetGallery/Services/UserService.cs +++ b/src/NuGetGallery/Services/UserService.cs @@ -23,32 +23,41 @@ public class UserService : IUserService public IEntityRepository CredentialRepository { get; protected set; } + public IEntityRepository OrganizationRepository { get; protected set; } + public IAuditingService Auditing { get; protected set; } public IEntitiesContext EntitiesContext { get; protected set; } + public IContentObjectService ContentObjectService { get; protected set; } public ISecurityPolicyService SecurityPolicyService { get; set; } + public IDateTimeProvider DateTimeProvider { get; protected set; } + protected UserService() { } public UserService( IAppConfiguration config, IEntityRepository userRepository, IEntityRepository credentialRepository, + IEntityRepository organizationRepository, IAuditingService auditing, IEntitiesContext entitiesContext, IContentObjectService contentObjectService, - ISecurityPolicyService securityPolicyService) + ISecurityPolicyService securityPolicyService, + IDateTimeProvider dateTimeProvider) : this() { Config = config; UserRepository = userRepository; CredentialRepository = credentialRepository; + OrganizationRepository = organizationRepository; Auditing = auditing; EntitiesContext = entitiesContext; ContentObjectService = contentObjectService; SecurityPolicyService = securityPolicyService; + DateTimeProvider = dateTimeProvider; } public async Task AddMemberAsync(Organization organization, string memberName, bool isAdmin) @@ -314,7 +323,7 @@ public bool CanTransformUserToOrganization(User accountToTransform, out string e else if (!ContentObjectService.LoginDiscontinuationConfiguration.AreOrganizationsSupportedForUser(accountToTransform)) { errorReason = String.Format(CultureInfo.CurrentCulture, - Strings.TransformAccount_FailedReasonNotInDomainWhitelist, accountToTransform.Username); + Strings.Organizations_NotInDomainWhitelist, accountToTransform.Username); } return errorReason == null; @@ -344,11 +353,11 @@ public bool CanTransformUserToOrganization(User accountToTransform, User adminUs } else { - var tenantId = adminUser.Credentials.GetAzureActiveDirectoryCredential()?.TenantId; + var tenantId = GetAzureActiveDirectoryCredentialTenant(adminUser); if (string.IsNullOrWhiteSpace(tenantId)) { errorReason = String.Format(CultureInfo.CurrentCulture, - Strings.TransformAccount_AdminAccountDoesNotHaveTenant, adminUser.Username); + Strings.Organizations_AdminAccountDoesNotHaveTenant, adminUser.Username); } } @@ -357,19 +366,92 @@ public bool CanTransformUserToOrganization(User accountToTransform, User adminUs public async Task TransformUserToOrganization(User accountToTransform, User adminUser, string token) { - var tenantId = adminUser.Credentials.GetAzureActiveDirectoryCredential()?.TenantId; + if (!await SubscribeOrganizationToTenantPolicy(accountToTransform, adminUser)) + { + return false; + } + + return await EntitiesContext.TransformUserToOrganization(accountToTransform, adminUser, token); + } + + public async Task AddOrganizationAsync(string username, string emailAddress, User adminUser) + { + if (!ContentObjectService.LoginDiscontinuationConfiguration.AreOrganizationsSupportedForUser(adminUser)) + { + throw new EntityException(String.Format(CultureInfo.CurrentCulture, + Strings.Organizations_NotInDomainWhitelist, adminUser.Username)); + } + + var existingUserWithIdentity = EntitiesContext.Users + .FirstOrDefault(u => + u.Username.Equals(username, StringComparison.OrdinalIgnoreCase) || + string.Equals(u.EmailAddress, emailAddress, StringComparison.OrdinalIgnoreCase)); + if (existingUserWithIdentity != null) + { + if (existingUserWithIdentity.Username.Equals(username, StringComparison.OrdinalIgnoreCase)) + { + throw new EntityException(Strings.UsernameNotAvailable, username); + } + + if (string.Equals(existingUserWithIdentity.EmailAddress, emailAddress, StringComparison.OrdinalIgnoreCase)) + { + throw new EntityException(Strings.EmailAddressBeingUsed, emailAddress); + } + } + + var organization = new Organization(username) + { + EmailAllowed = true, + UnconfirmedEmailAddress = emailAddress, + EmailConfirmationToken = Crypto.GenerateToken(), + NotifyPackagePushed = true, + CreatedUtc = DateTimeProvider.UtcNow, + Members = new List() + }; + + var membership = new Membership { Organization = organization, Member = adminUser, IsAdmin = true }; + + organization.Members.Add(membership); + adminUser.Organizations.Add(membership); + + OrganizationRepository.InsertOnCommit(organization); + + if (string.IsNullOrEmpty(GetAzureActiveDirectoryCredentialTenant(adminUser))) + { + throw new EntityException(String.Format(CultureInfo.CurrentCulture, + Strings.Organizations_AdminAccountDoesNotHaveTenant, adminUser.Username)); + } + + if (!await SubscribeOrganizationToTenantPolicy(organization, adminUser, commitChanges: false)) + { + throw new EntityException(Strings.DefaultUserSafeExceptionMessage); + } + + await EntitiesContext.SaveChangesAsync(); + + return organization; + } + + private async Task SubscribeOrganizationToTenantPolicy(User organization, User adminUser, bool commitChanges = true) + { + var tenantId = GetAzureActiveDirectoryCredentialTenant(adminUser); if (string.IsNullOrWhiteSpace(tenantId)) { return false; } var tenantPolicy = RequireOrganizationTenantPolicy.Create(tenantId); - if (!await SecurityPolicyService.SubscribeAsync(accountToTransform, tenantPolicy)) + if (!await SecurityPolicyService.SubscribeAsync(organization, tenantPolicy, commitChanges)) { return false; } - - return await EntitiesContext.TransformUserToOrganization(accountToTransform, adminUser, token); + + return true; + } + + private string GetAzureActiveDirectoryCredentialTenant(User user) + { + return user.Credentials.GetAzureActiveDirectoryCredential()?.TenantId; } } } \ No newline at end of file diff --git a/src/NuGetGallery/Strings.Designer.cs b/src/NuGetGallery/Strings.Designer.cs index 53959e582f..34ea87be8d 100644 --- a/src/NuGetGallery/Strings.Designer.cs +++ b/src/NuGetGallery/Strings.Designer.cs @@ -1030,6 +1030,24 @@ public static string OrganizationMemberNameIsRequired { } } + /// + /// Looks up a localized string similar to Administrator account '{0}' is not linked to an AAD credential with an organization tenant.. + /// + public static string Organizations_AdminAccountDoesNotHaveTenant { + get { + return ResourceManager.GetString("Organizations_AdminAccountDoesNotHaveTenant", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Account '{0}' does not support organizations.. + /// + public static string Organizations_NotInDomainWhitelist { + get { + return ResourceManager.GetString("Organizations_NotInDomainWhitelist", resourceCulture); + } + } + /// /// Looks up a localized string similar to Organization accounts cannot create credentials.. /// @@ -1574,15 +1592,6 @@ public static string TransformAccount_AdminAccountDoesNotExist { } } - /// - /// Looks up a localized string similar to Administrator account '{0}' is not linked to an AAD credential with an organization tenant.. - /// - public static string TransformAccount_AdminAccountDoesNotHaveTenant { - get { - return ResourceManager.GetString("TransformAccount_AdminAccountDoesNotHaveTenant", resourceCulture); - } - } - /// /// Looks up a localized string similar to Administrator account '{0}' cannot be an organization.. /// @@ -1611,7 +1620,7 @@ public static string TransformAccount_AdminMustBeDifferentAccount { } /// - /// Looks up a localized string similar to An unexpected error occurred while transforming this account. Contact support for assistance.. + /// Looks up a localized string similar to An unexpected error occurred while transforming this account. Contact support for assistance.. /// public static string TransformAccount_Failed { get { @@ -1619,15 +1628,6 @@ public static string TransformAccount_Failed { } } - /// - /// Looks up a localized string similar to Account '{0}' does not support organizations.. - /// - public static string TransformAccount_FailedReasonNotInDomainWhitelist { - get { - return ResourceManager.GetString("TransformAccount_FailedReasonNotInDomainWhitelist", resourceCulture); - } - } - /// /// Looks up a localized string similar to Organization account '{0}' does not exist.. /// diff --git a/src/NuGetGallery/Strings.resx b/src/NuGetGallery/Strings.resx index 82edeac578..d0cf4e8899 100644 --- a/src/NuGetGallery/Strings.resx +++ b/src/NuGetGallery/Strings.resx @@ -702,7 +702,7 @@ For more information, please contact '{2}'. The user '{0}' doesn't exist. You cannot upload a package as a user that doesn't exist. - An unexpected error occurred while transforming this account. Contact support for assistance. + An unexpected error occurred while transforming this account. Contact support for assistance. Organization account '{0}' does not exist. @@ -716,7 +716,7 @@ For more information, please contact '{2}'. Account '{0}' should be a confirmed user. - + Account '{0}' does not support organizations. @@ -801,7 +801,7 @@ If you wish to update the linked Microsoft account you can do so from the accoun User '{0}' has not linked their account to an AAD credential matching this organization. - + Administrator account '{0}' is not linked to an AAD credential with an organization tenant. diff --git a/src/NuGetGallery/UrlExtensions.cs b/src/NuGetGallery/UrlExtensions.cs index 879422e2a3..e3e9e9ee74 100644 --- a/src/NuGetGallery/UrlExtensions.cs +++ b/src/NuGetGallery/UrlExtensions.cs @@ -764,6 +764,14 @@ public static string ManageMyOrganizations(this UrlHelper url, bool relativeUrl return GetActionLink(url, "Organizations", "Users", relativeUrl); } + public static string AddOrganization(this UrlHelper url, bool relativeUrl = true) + { + return GetActionLink(url, + "Add", + "Organizations", + relativeUrl); + } + public static string ManageMyOrganization(this UrlHelper url, string accountName, bool relativeUrl = true) { return GetActionLink(url, diff --git a/src/NuGetGallery/ViewModels/AddOrganizationViewModel.cs b/src/NuGetGallery/ViewModels/AddOrganizationViewModel.cs new file mode 100644 index 0000000000..18d358862c --- /dev/null +++ b/src/NuGetGallery/ViewModels/AddOrganizationViewModel.cs @@ -0,0 +1,20 @@ +// 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.ComponentModel.DataAnnotations; + +namespace NuGetGallery +{ + public class AddOrganizationViewModel + { + [Required] + [StringLength(64)] + [Display(Name = "Organization Name")] + public string OrganizationName { get; set; } + + [Required] + [StringLength(255)] + [Display(Name = "Organization Email Address")] + public string OrganizationEmailAddress { get; set; } + } +} \ No newline at end of file diff --git a/src/NuGetGallery/ViewModels/ChangeEmailViewModel.cs b/src/NuGetGallery/ViewModels/ChangeEmailViewModel.cs index a06d3a6d46..3ae1f534a4 100644 --- a/src/NuGetGallery/ViewModels/ChangeEmailViewModel.cs +++ b/src/NuGetGallery/ViewModels/ChangeEmailViewModel.cs @@ -11,7 +11,7 @@ public class ChangeEmailViewModel [Required] [StringLength(255)] [Display(Name = "New Email Address")] - [RegularExpression(RegisterViewModel.EmailValidationRegex, ErrorMessage = RegisterViewModel.EmailValidationErrorMessage)] + [RegularExpression(Constants.EmailValidationRegex, ErrorMessage = RegisterViewModel.EmailValidationErrorMessage)] public string NewEmail { get; set; } [Required] diff --git a/src/NuGetGallery/ViewModels/LogOnViewModel.cs b/src/NuGetGallery/ViewModels/LogOnViewModel.cs index fde56c1163..7962941f10 100644 --- a/src/NuGetGallery/ViewModels/LogOnViewModel.cs +++ b/src/NuGetGallery/ViewModels/LogOnViewModel.cs @@ -67,17 +67,6 @@ public SignInViewModel(string userNameOrEmail, string password) public class RegisterViewModel { - // Note: regexes must be tested to work in JavaScript - // We do NOT follow strictly the RFCs at this time, and we choose not to support many obscure email address variants. - // Specifically the following are not supported by-design: - // * Addresses containing () or [] - // * Second parts with no dots (i.e. foo@localhost or foo@com) - // * Addresses with quoted (" or ') first parts - // * Addresses with IP Address second parts (foo@[127.0.0.1]) - internal const string FirstPart = @"[-A-Za-z0-9!#$%&'*+\/=?^_`{|}~\.]+"; - internal const string SecondPart = @"[A-Za-z0-9]+[\w\.-]*[A-Za-z0-9]*\.[A-Za-z0-9][A-Za-z\.]*[A-Za-z]"; - internal const string EmailValidationRegex = "^" + FirstPart + "@" + SecondPart + "$"; - internal const string EmailValidationErrorMessage = "This doesn't appear to be a valid email address."; public const string EmailHint = "Your email will not be public unless you choose to disclose it. " + "It is required to verify your registration and for password retrieval, important notifications, etc. "; @@ -93,7 +82,7 @@ public class RegisterViewModel [Required] [StringLength(255)] [Display(Name = "Email")] - [RegularExpression(EmailValidationRegex, ErrorMessage = EmailValidationErrorMessage)] + [RegularExpression(Constants.EmailValidationRegex, ErrorMessage = EmailValidationErrorMessage)] [Subtext("We use Gravatar to get your profile picture", AllowHtml = true)] public string EmailAddress { get; set; } diff --git a/src/NuGetGallery/ViewModels/ManageOrganizationsItemViewModel.cs b/src/NuGetGallery/ViewModels/ManageOrganizationsItemViewModel.cs index 548afe12ee..8caa8b48bd 100644 --- a/src/NuGetGallery/ViewModels/ManageOrganizationsItemViewModel.cs +++ b/src/NuGetGallery/ViewModels/ManageOrganizationsItemViewModel.cs @@ -17,7 +17,7 @@ public ManageOrganizationsItemViewModel(Membership membership, IPackageService p { var organization = membership.Organization; Username = organization.Username; - EmailAddress = organization.EmailAddress; + EmailAddress = organization.EmailAddress ?? organization.UnconfirmedEmailAddress; CurrentUserIsAdmin = membership.IsAdmin; MemberCount = organization.Members.Count(); PackagesCount = packageService.FindPackageRegistrationsByOwner(membership.Organization).Count(); diff --git a/src/NuGetGallery/ViewModels/ReportAbuseViewModel.cs b/src/NuGetGallery/ViewModels/ReportAbuseViewModel.cs index d3d431c184..a947d4c557 100644 --- a/src/NuGetGallery/ViewModels/ReportAbuseViewModel.cs +++ b/src/NuGetGallery/ViewModels/ReportAbuseViewModel.cs @@ -14,7 +14,7 @@ public class ReportAbuseViewModel : ReportViewModel [Required(ErrorMessage = "Please enter your email address.")] [StringLength(4000)] [Display(Name = "Your Email Address")] - [RegularExpression(RegisterViewModel.EmailValidationRegex, + [RegularExpression(Constants.EmailValidationRegex, ErrorMessage = "This doesn't appear to be a valid email address.")] public string Email { get; set; } diff --git a/src/NuGetGallery/Views/Organizations/Add.cshtml b/src/NuGetGallery/Views/Organizations/Add.cshtml new file mode 100644 index 0000000000..82cb13714e --- /dev/null +++ b/src/NuGetGallery/Views/Organizations/Add.cshtml @@ -0,0 +1,70 @@ +@model AddOrganizationViewModel +@{ + ViewBag.Title = "Add Organization"; + ViewBag.MdPageColumns = Constants.ColumnsFormMd; + Layout = "~/Views/Shared/Gallery/Layout.cshtml"; + + var gravatarId = "gravatar-image"; + var emailBoxGroupId = "email-box-group"; +} + +
+
+
+

+ Organizations + > + Add New +

+

+ Organizations allow you to manage and publish packages as a team or a group. +

+
+ + +
+ @using (Html.BeginForm("Add", "Organizations", FormMethod.Post)) + { + @Html.AntiForgeryToken() + +
+ @Html.ShowLabelFor(m => m.OrganizationName) + @Html.ShowTextBoxFor(m => m.OrganizationName) + @Html.ShowValidationMessagesFor(m => m.OrganizationName) + This will be your organization account on @Url.User("your_name_here", relativeUrl: false). +
+ +
+ @Html.ShowLabelFor(m => m.OrganizationEmailAddress) + @Html.ShowTextBoxFor(m => m.OrganizationEmailAddress) + @Html.ShowValidationMessagesFor(m => m.OrganizationEmailAddress) + Users can contact you at this email address. +
+ +
+
+ +
+
+ Cancel +
+
+ } +
+
+
+
+
+ +@section BottomScripts { + + @Scripts.Render("~/Scripts/gallery/page-add-organization.min.js") +} \ No newline at end of file diff --git a/src/NuGetGallery/Views/Shared/Confirm.cshtml b/src/NuGetGallery/Views/Shared/Confirm.cshtml index 40b01cc9d2..116ee31dd4 100644 --- a/src/NuGetGallery/Views/Shared/Confirm.cshtml +++ b/src/NuGetGallery/Views/Shared/Confirm.cshtml @@ -12,7 +12,7 @@
@if (Model.SuccessfulConfirmation) { -

@(Model.ConfirmingNewAccount ? "Account" : "Email") Confirmed

+

@(Model.ConfirmingNewAccount ? (Model.IsOrganization ? "Organization" : "Account") : "Email") Confirmed

} else { @@ -25,7 +25,7 @@ @if (Model.SuccessfulConfirmation) {

- Your email address has been verified@(Model.ConfirmingNewAccount ? " and your account is confirmed" : ""). + Your@(Model.IsOrganization ? " organization's" : "") email address has been verified@(Model.ConfirmingNewAccount ? " and your " + (Model.IsOrganization ? "organization" : "account") + " is confirmed" : "").

} else diff --git a/src/NuGetGallery/Views/Users/Organizations.cshtml b/src/NuGetGallery/Views/Users/Organizations.cshtml index 2f63c3a112..54ef93505e 100644 --- a/src/NuGetGallery/Views/Users/Organizations.cshtml +++ b/src/NuGetGallery/Views/Users/Organizations.cshtml @@ -4,16 +4,28 @@ Layout = "~/Views/Shared/Gallery/Layout.cshtml"; } -
+
-

Organizations

+ +
+

Organizations

+ @if (Model.Organizations.Any()) + { +
+ +
+ } +
+ @if (Model.Organizations.Any()) { var orgCount = Model.Organizations.Count(); var orgCountString = orgCount > 1 ? orgCount + " organizations" : orgCount + " organization"; -

You have @(orgCountString).

+
+

You have @(orgCountString).

+
@@ -58,7 +70,19 @@ } else { -

You do not have any organizations.

+
+ Learn to use packages +
+
+

Organizations allow you to manage and publish packages as a team or a group.

+
+
+
+ + +
} diff --git a/tests/NuGetGallery.Facts/Controllers/OrganizationsControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/OrganizationsControllerFacts.cs index aec6d9c7c4..d66eada8dd 100644 --- a/tests/NuGetGallery.Facts/Controllers/OrganizationsControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/OrganizationsControllerFacts.cs @@ -2,9 +2,11 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Net; +using System.Net.Mail; using System.Threading.Tasks; using System.Web.Mvc; using Moq; +using NuGetGallery.Framework; using Xunit; namespace NuGetGallery @@ -312,6 +314,76 @@ public async Task WhenUserIsNotMember_ReturnsNonSuccess() } } + public class TheCreateAction : TestContainer + { + private const string OrgName = "TestOrg"; + private const string OrgEmail = "TestOrg@testorg.com"; + + private AddOrganizationViewModel Model = + new AddOrganizationViewModel { OrganizationName = OrgName, OrganizationEmailAddress = OrgEmail }; + + private User Admin; + + private Fakes Fakes; + + public TheCreateAction() + { + Fakes = Get(); + Admin = Fakes.User; + } + + [Fact] + public async Task WhenAddOrganizationThrowsEntityException_ReturnsViewWithMessage() + { + var message = "message"; + + var mockUserService = GetMock(); + mockUserService + .Setup(x => x.AddOrganizationAsync(OrgName, OrgEmail, Admin)) + .Throws(new EntityException(message)); + + var controller = GetController(); + controller.SetCurrentUser(Admin); + + var result = await controller.Add(Model); + + ResultAssert.IsView(result); + Assert.Equal(message, controller.TempData["ErrorMessage"]); + } + + [Fact] + public async Task WhenAddOrganizationSucceeds_RedirectsToManageOrganization() + { + var token = "token"; + var org = new Organization("newlyCreated") + { + UnconfirmedEmailAddress = OrgEmail, + EmailConfirmationToken = token + }; + + var mockUserService = GetMock(); + mockUserService + .Setup(x => x.AddOrganizationAsync(OrgName, OrgEmail, Admin)) + .Returns(Task.FromResult(org)); + + var messageService = GetMock(); + + var controller = GetController(); + controller.SetCurrentUser(Admin); + + var result = await controller.Add(Model); + + ResultAssert.IsRedirectToRoute(result, + new { accountName = org.Username, action = nameof(OrganizationsController.ManageOrganization) }); + + messageService.Verify( + x => x.SendNewAccountEmail( + It.Is(m => m.Address == OrgEmail && m.DisplayName == org.Username), + It.Is(s => s.Contains(token))), + Times.Once()); + } + } + public class TheAddMemberAction : AccountsControllerTestContainer { private const string defaultMemberName = "member"; diff --git a/tests/NuGetGallery.Facts/EmailValidationRegex.cs b/tests/NuGetGallery.Facts/EmailValidationRegex.cs index 2d4e08fb4e..33b00124fc 100644 --- a/tests/NuGetGallery.Facts/EmailValidationRegex.cs +++ b/tests/NuGetGallery.Facts/EmailValidationRegex.cs @@ -19,7 +19,7 @@ public class EmailValidationRegex [InlineData("a@b.c.d.e.f")] public void TheWholeAllows(string address) { - var match = new Regex(RegisterViewModel.EmailValidationRegex).IsMatch(address); + var match = new Regex(Constants.EmailValidationRegex).IsMatch(address); Assert.True(match); } @@ -29,7 +29,7 @@ public void TheWholeAllows(string address) [InlineData("fred@.com")] public void TheWholeDoesntAllow(string testWhole) { - var match = new Regex(RegisterViewModel.EmailValidationRegex).IsMatch(testWhole); + var match = new Regex(Constants.EmailValidationRegex).IsMatch(testWhole); Assert.False(match); } @@ -43,7 +43,7 @@ public void TheWholeDoesntAllow(string testWhole) [InlineData("fred~`'.baz")] public void TheFirstPartMatches(string testFirstPart) { - var match = new Regex("^" + RegisterViewModel.FirstPart + "$").IsMatch(testFirstPart); + var match = new Regex("^" + Constants.EmailValidationRegexFirstPart + "$").IsMatch(testFirstPart); Assert.True(match); } @@ -58,7 +58,7 @@ public void TheFirstPartMatches(string testFirstPart) [InlineData("abc.\"def\\\"\"ghi\".xyz")] // thanks Wikipedia, but in practice nobody uses these email addresses. public void TheFirstPartDoesntAllow(string testFirstPart) { - var match = new Regex("^" + RegisterViewModel.FirstPart + "$").IsMatch(testFirstPart); + var match = new Regex("^" + Constants.EmailValidationRegexFirstPart + "$").IsMatch(testFirstPart); Assert.False(match); } @@ -71,7 +71,7 @@ public void TheFirstPartDoesntAllow(string testFirstPart) [InlineData("a.b.c.d.e.f")] public void TheSecondPartMatches(string testSecondPart) { - var match = new Regex("^" + RegisterViewModel.SecondPart + "$").IsMatch(testSecondPart); + var match = new Regex("^" + Constants.EmailValidationRegexSecondPart + "$").IsMatch(testSecondPart); Assert.True(match); } @@ -83,7 +83,7 @@ public void TheSecondPartMatches(string testSecondPart) [InlineData("[IPv6:2001:db8:1ff::a0b:dbd0]")] //no IP addresses public void TheSecondPartDoesntAllow(string testSecondPart) { - var match = new Regex("^" + RegisterViewModel.SecondPart + "$").IsMatch(testSecondPart); + var match = new Regex("^" + Constants.EmailValidationRegexSecondPart + "$").IsMatch(testSecondPart); Assert.False(match); } } diff --git a/tests/NuGetGallery.Facts/Security/SecurePushSubscriptionFacts.cs b/tests/NuGetGallery.Facts/Security/SecurePushSubscriptionFacts.cs index 381eff3655..ad0c126e9c 100644 --- a/tests/NuGetGallery.Facts/Security/SecurePushSubscriptionFacts.cs +++ b/tests/NuGetGallery.Facts/Security/SecurePushSubscriptionFacts.cs @@ -10,6 +10,7 @@ using NuGet.Packaging; using NuGetGallery.Auditing; using NuGetGallery.Diagnostics; +using NuGetGallery.Framework; using Xunit; namespace NuGetGallery.Security @@ -44,64 +45,103 @@ public void Policies_ReturnsMinClientAndPackageVerifyScopePolicies() Assert.Equal("{\"v\":\"4.1.0\"}", policy1.Value); } + public static IEnumerable OnSubscribeAsync_ExpiresPushApiKeys_Data + { + get + { + foreach (var scopes in new[] { "", "[{\"a\":\"package:push\", \"s\":\"theId\"}]", "[{\"a\":\"package:pushversion\", \"s\":\"theId\"}]" }) + { + foreach (var commitChanges in new[] { false, true }) + { + yield return MemberDataHelper.AsData(scopes, commitChanges); + } + } + } + } + [Theory] - [InlineData("")] - [InlineData("[{\"a\":\"package:push\", \"s\":\"theId\"}]")] - [InlineData("[{\"a\":\"package:pushversion\", \"s\":\"theId\"}]")] - public async Task OnSubscribeAsync_ExpiresPushApiKeys(string scopes) + [MemberData(nameof(OnSubscribeAsync_ExpiresPushApiKeys_Data))] + public async Task OnSubscribeAsync_ExpiresPushApiKeys(string scopes, bool commitChanges) { // Arrange & Act. - var user = (await SubscribeUserToSecurePushAsync(CredentialTypes.ApiKey.V2, scopes)).Item1; + var user = (await SubscribeUserToSecurePushAsync(CredentialTypes.ApiKey.V2, scopes, commitChanges)).Item1; // Assert. Assert.Equal(2, user.SecurityPolicies.Count()); Assert.True(DateTime.UtcNow.AddDays(_expectedPushKeyExpiration) >= user.Credentials.Single().Expires); } + public static IEnumerable OnSubscribeAsync_DoesNotExpireNonPushCredentials_Data + { + get + { + foreach (var commitChanges in new[] { false, true }) + { + yield return MemberDataHelper.AsData(CredentialTypes.Password.V3, "", commitChanges); + yield return MemberDataHelper.AsData(CredentialTypes.ApiKey.V2, "[{\"a\":\"package:unlist\", \"s\":\"theId\"}]", commitChanges); + yield return MemberDataHelper.AsData(CredentialTypes.ApiKey.VerifyV1, "[{\"a\":\"package:verify\", \"s\":\"theId\"}]", commitChanges); + } + } + } + [Theory] - [InlineData("password.v3", "")] - [InlineData("apikey.v2", "[{\"a\":\"package:unlist\", \"s\":\"theId\"}]")] - [InlineData("apikey.verify.v1", "[{\"a\":\"package:verify\", \"s\":\"theId\"}]")] - public async Task OnSubscribeAsync_DoesNotExpireNonPushCredentials(string type, string scopes) + [MemberData(nameof(OnSubscribeAsync_DoesNotExpireNonPushCredentials_Data))] + public async Task OnSubscribeAsync_DoesNotExpireNonPushCredentials(string type, string scopes, bool commitChanges) { // Arrange & Act. - var user = (await SubscribeUserToSecurePushAsync(type, scopes)).Item1; + var user = (await SubscribeUserToSecurePushAsync(type, scopes, commitChanges)).Item1; // Assert. Assert.Equal(2, user.SecurityPolicies.Count()); Assert.False(DateTime.UtcNow.AddDays(_expectedPushKeyExpiration) >= user.Credentials.Single().Expires); } + public static IEnumerable OnSubscribeAsync_DoesNotChangeExpiringPushCredentials_Data + { + get + { + foreach (var scopes in new[] { "", "[{\"a\":\"package:push\", \"s\":\"theId\"}]", "[{\"a\":\"package:pushversion\", \"s\":\"theId\"}]" }) + { + foreach (var commitChanges in new[] { false, true }) + { + yield return MemberDataHelper.AsData(scopes, commitChanges); + } + } + } + } + [Theory] - [InlineData("")] - [InlineData("[{\"a\":\"package:push\", \"s\":\"theId\"}]")] - [InlineData("[{\"a\":\"package:pushversion\", \"s\":\"theId\"}]")] - public async Task OnSubscribeAsync_DoesNotChangeExpiringPushCredentials(string scopes) + [MemberData(nameof(OnSubscribeAsync_DoesNotChangeExpiringPushCredentials_Data))] + public async Task OnSubscribeAsync_DoesNotChangeExpiringPushCredentials(string scopes, bool commitChanges) { // Arrange & Act. - var user = (await SubscribeUserToSecurePushAsync(CredentialTypes.ApiKey.V2,scopes, expiresInDays: 2)).Item1; + var user = (await SubscribeUserToSecurePushAsync(CredentialTypes.ApiKey.V2,scopes, commitChanges, expiresInDays: 2)).Item1; // Assert. Assert.Equal(2, user.SecurityPolicies.Count()); Assert.True(DateTime.UtcNow.AddDays(2) >= user.Credentials.Single().Expires); } - [Fact] - public async Task OnSubscribeAsync_SavesAuditRecordIfKeysSetToExpire() + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task OnSubscribeAsync_SavesAuditRecordIfKeysSetToExpire(bool commitChanges) { // Arrange & Act. - var service = (await SubscribeUserToSecurePushAsync(CredentialTypes.ApiKey.V2, "")).Item2; + var service = (await SubscribeUserToSecurePushAsync(CredentialTypes.ApiKey.V2, "", commitChanges)).Item2; // Assert. service.MockAuditingService.Verify(s => s.SaveAuditRecordAsync(It.IsAny()), /* subscription and key expiration */Times.Exactly(2)); } - [Fact] - public async Task OnSubscribeAsync_DoesNotSaveAuditRecordIfKeysNotSetToExpire() + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task OnSubscribeAsync_DoesNotSaveAuditRecordIfKeysNotSetToExpire(bool commitChanges) { // Arrange & Act. - var service = (await SubscribeUserToSecurePushAsync(CredentialTypes.ApiKey.V2, "", expiresInDays: 2)).Item2; + var service = (await SubscribeUserToSecurePushAsync(CredentialTypes.ApiKey.V2, "", commitChanges, expiresInDays: 2)).Item2; // Assert. service.MockAuditingService.Verify(s => s.SaveAuditRecordAsync(It.IsAny()), @@ -132,7 +172,7 @@ private TestSecurityPolicyService CreateSecurityPolicyService() } private async Task> SubscribeUserToSecurePushAsync( - string type, string scopes, int expiresInDays = 100) + string type, string scopes, bool commitChanges, int expiresInDays = 100) { // Arrange. var service = CreateSecurityPolicyService(); @@ -146,9 +186,9 @@ private async Task> SubscribeUserToSecure user.Credentials.Add(credential); // Act. - await service.SubscribeAsync(user, service.UserSubscriptions.Single()); + await service.SubscribeAsync(user, service.UserSubscriptions.Single(), commitChanges); - service.MockEntitiesContext.Verify(c => c.SaveChangesAsync(), Times.Once); + service.MockEntitiesContext.Verify(c => c.SaveChangesAsync(), commitChanges ? Times.Once() : Times.Never()); return Tuple.Create(user, service); } diff --git a/tests/NuGetGallery.Facts/Services/UserServiceFacts.cs b/tests/NuGetGallery.Facts/Services/UserServiceFacts.cs index e12048c2fe..45093fdf48 100644 --- a/tests/NuGetGallery.Facts/Services/UserServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/UserServiceFacts.cs @@ -688,7 +688,7 @@ public void WhenAccountIsNotInWhitelist_ReturnsFalse() // Assert Assert.False(result); Assert.Equal(errorReason, String.Format(CultureInfo.CurrentCulture, - Strings.TransformAccount_FailedReasonNotInDomainWhitelist, user.Username)); + Strings.Organizations_NotInDomainWhitelist, user.Username)); } [Fact] @@ -940,13 +940,177 @@ private Task InvokeTransformUserToOrganization(int affectedRecords, User a .Returns(Task.FromResult(affectedRecords)); service.MockSecurityPolicyService - .Setup(sp => sp.SubscribeAsync(It.IsAny(), It.IsAny())) + .Setup(sp => sp.SubscribeAsync(It.IsAny(), It.IsAny(), true)) .Returns(Task.FromResult(true)); // Act return service.TransformUserToOrganization(account, admin, "token"); } } + + public class TheCreateOrganizationAccountMethod + { + private const string OrgName = "myOrg"; + private const string OrgEmail = "myOrg@myOrg.com"; + private const string AdminName = "orgAdmin"; + + private static DateTime OrgCreatedUtc = new DateTime(2018, 2, 21); + + private TestableUserService _service = new TestableUserService(); + + [Fact] + public async Task WithUserNotSupportedForOrganizations_ThrowsEntityException() + { + SetupOrganizationsSupportedForUser(supported: false); + var exception = await Assert.ThrowsAsync(() => InvokeCreateOrganization()); + Assert.Equal(String.Format(CultureInfo.CurrentCulture, Strings.Organizations_NotInDomainWhitelist, AdminName), exception.Message); + + _service.MockOrganizationRepository.Verify(x => x.InsertOnCommit(It.IsAny()), Times.Never()); + _service.MockSecurityPolicyService.Verify(sp => sp.SubscribeAsync(It.IsAny(), It.IsAny(), false), Times.Never()); + _service.MockEntitiesContext.Verify(x => x.SaveChangesAsync(), Times.Never()); + } + + [Fact] + public async Task WithUsernameConflict_ThrowsEntityException() + { + var conflictUsername = "ialreadyexist"; + + _service.MockEntitiesContext + .Setup(x => x.Users) + .Returns(new[] { new User(conflictUsername) }.MockDbSet().Object); + + SetupOrganizationsSupportedForUser(); + + var exception = await Assert.ThrowsAsync(() => InvokeCreateOrganization(orgName: conflictUsername)); + Assert.Equal(String.Format(CultureInfo.CurrentCulture, Strings.UsernameNotAvailable, conflictUsername), exception.Message); + + _service.MockOrganizationRepository.Verify(x => x.InsertOnCommit(It.IsAny()), Times.Never()); + _service.MockSecurityPolicyService.Verify(sp => sp.SubscribeAsync(It.IsAny(), It.IsAny(), false), Times.Never()); + _service.MockEntitiesContext.Verify(x => x.SaveChangesAsync(), Times.Never()); + } + + [Fact] + public async Task WithEmailConflict_ThrowsEntityException() + { + var conflictEmail = "ialreadyexist@existence.com"; + + _service.MockEntitiesContext + .Setup(x => x.Users) + .Returns(new[] { new User("user") { EmailAddress = conflictEmail } }.MockDbSet().Object); + + SetupOrganizationsSupportedForUser(); + + var exception = await Assert.ThrowsAsync(() => InvokeCreateOrganization(orgEmail: conflictEmail)); + Assert.Equal(String.Format(CultureInfo.CurrentCulture, Strings.EmailAddressBeingUsed, conflictEmail), exception.Message); + + _service.MockOrganizationRepository.Verify(x => x.InsertOnCommit(It.IsAny()), Times.Never()); + _service.MockSecurityPolicyService.Verify(sp => sp.SubscribeAsync(It.IsAny(), It.IsAny(), false), Times.Never()); + _service.MockEntitiesContext.Verify(x => x.SaveChangesAsync(), Times.Never()); + } + + [Fact] + public async Task WhenAdminHasNoTenant_ThrowsEntityException() + { + _service.MockEntitiesContext + .Setup(x => x.Users) + .Returns(Enumerable.Empty().MockDbSet().Object); + + var adminUsername = "adminWithNoTenant"; + SetupOrganizationsSupportedForUser(adminUsername); + var exception = await Assert.ThrowsAsync(() => InvokeCreateOrganization(admin: new User(adminUsername))); + Assert.Equal(String.Format(CultureInfo.CurrentCulture, Strings.Organizations_AdminAccountDoesNotHaveTenant, adminUsername), exception.Message); + + _service.MockOrganizationRepository.Verify(x => x.InsertOnCommit(It.IsAny()), Times.Once()); + _service.MockSecurityPolicyService.Verify(sp => sp.SubscribeAsync(It.IsAny(), It.IsAny(), false), Times.Never()); + _service.MockEntitiesContext.Verify(x => x.SaveChangesAsync(), Times.Never()); + } + + [Fact] + public async Task WhenSubscribingToPolicyFails_ThrowsUserSafeException() + { + _service.MockEntitiesContext + .Setup(x => x.Users) + .Returns(Enumerable.Empty().MockDbSet().Object); + + _service.MockSecurityPolicyService + .Setup(sp => sp.SubscribeAsync(It.IsAny(), It.IsAny(), false)) + .Returns(Task.FromResult(false)); + SetupOrganizationsSupportedForUser(); + + var exception = await Assert.ThrowsAsync(() => InvokeCreateOrganization()); + Assert.Equal(Strings.DefaultUserSafeExceptionMessage, exception.Message); + + _service.MockOrganizationRepository.Verify(x => x.InsertOnCommit(It.IsAny()), Times.Once()); + _service.MockSecurityPolicyService.Verify(sp => sp.SubscribeAsync(It.IsAny(), It.IsAny(), false), Times.Once()); + _service.MockEntitiesContext.Verify(x => x.SaveChangesAsync(), Times.Never()); + } + + [Fact] + public async Task WhenSubscribingToPolicySucceeds_ReturnsNewOrg() + { + _service.MockEntitiesContext + .Setup(x => x.Users) + .Returns(Enumerable.Empty().MockDbSet().Object); + + _service.MockSecurityPolicyService + .Setup(sp => sp.SubscribeAsync(It.IsAny(), It.IsAny(), false)) + .Returns(Task.FromResult(true)); + SetupOrganizationsSupportedForUser(); + + var org = await InvokeCreateOrganization(); + + Assert.Equal(OrgName, org.Username); + Assert.Equal(OrgEmail, org.UnconfirmedEmailAddress); + Assert.Equal(OrgCreatedUtc, org.CreatedUtc); + Assert.True(org.EmailAllowed); + Assert.True(org.NotifyPackagePushed); + Assert.True(!string.IsNullOrEmpty(org.EmailConfirmationToken)); + + // Both the organization and the admin must have a membership to each other. + Func hasMembership = m => m.Member.Username == AdminName && m.Organization.Username == OrgName && m.IsAdmin; + Assert.True( + org.Members.Any( + m => hasMembership(m) && m.Member.Organizations.Any(hasMembership))); + + _service.MockOrganizationRepository.Verify(x => x.InsertOnCommit(It.IsAny()), Times.Once()); + _service.MockSecurityPolicyService.Verify(sp => sp.SubscribeAsync(It.IsAny(), It.IsAny(), false), Times.Once()); + _service.MockEntitiesContext.Verify(x => x.SaveChangesAsync(), Times.Once()); + } + + private Task InvokeCreateOrganization(string orgName = OrgName, string orgEmail = OrgEmail, User admin = null) + { + // Arrange + admin = admin ?? new User(AdminName) + { + Credentials = new Credential[] { + new CredentialBuilder().CreateExternalCredential( + issuer: "AzureActiveDirectory", + value: "abc123", + identity: "Admin", + tenantId: "zyx987") + } + }; + + _service.MockDateTimeProvider + .Setup(x => x.UtcNow) + .Returns(OrgCreatedUtc); + + // Act + return _service.AddOrganizationAsync(orgName, orgEmail, admin); + } + + private void SetupOrganizationsSupportedForUser(string adminUsername = null, bool supported = true) + { + adminUsername = adminUsername ?? AdminName; + + var mockLoginDiscontinuationConfiguration = new Mock(); + mockLoginDiscontinuationConfiguration + .Setup(x => x.AreOrganizationsSupportedForUser(It.Is(u => u.Username == adminUsername))) + .Returns(supported); + + _service.MockConfigObjectService.Setup(x => x.LoginDiscontinuationConfiguration).Returns(mockLoginDiscontinuationConfiguration.Object); + } + } } } diff --git a/tests/NuGetGallery.Facts/TestUtils/TestServiceUtility.cs b/tests/NuGetGallery.Facts/TestUtils/TestServiceUtility.cs index da4f88f6d9..d98bae66f5 100644 --- a/tests/NuGetGallery.Facts/TestUtils/TestServiceUtility.cs +++ b/tests/NuGetGallery.Facts/TestUtils/TestServiceUtility.cs @@ -137,9 +137,11 @@ public class TestableUserService : UserService public Mock MockSecurityPolicyService { get; protected set; } public Mock> MockUserRepository { get; protected set; } public Mock> MockCredentialRepository { get; protected set; } + public Mock> MockOrganizationRepository { get; protected set; } public Mock MockEntitiesContext { get; protected set; } public Mock MockDatabase { get; protected set; } public Mock MockConfigObjectService { get; protected set; } + public Mock MockDateTimeProvider { get; protected set; } public TestableUserService() { @@ -147,8 +149,10 @@ public TestableUserService() SecurityPolicyService = (MockSecurityPolicyService = new Mock()).Object; UserRepository = (MockUserRepository = new Mock>()).Object; CredentialRepository = (MockCredentialRepository = new Mock>()).Object; + OrganizationRepository = (MockOrganizationRepository = new Mock>()).Object; EntitiesContext = (MockEntitiesContext = new Mock()).Object; ContentObjectService = (MockConfigObjectService = new Mock()).Object; + DateTimeProvider = (MockDateTimeProvider = new Mock()).Object; Auditing = new TestAuditingService(); // Set ConfirmEmailAddress to a default of true From d8371bddd11e96d8f12793b7116b18bd45653f9f Mon Sep 17 00:00:00 2001 From: Scott Bommarito Date: Thu, 1 Mar 2018 12:41:53 -0800 Subject: [PATCH 12/24] [Organizations] Include reserved namespaces and package ownership requests in Manage Packages page filtering (#5491) --- .../Controllers/PackagesController.cs | 16 +- .../Controllers/UsersController.cs | 15 +- src/NuGetGallery/NuGetGallery.csproj | 2 - src/NuGetGallery/RouteName.cs | 2 + .../Scripts/gallery/page-manage-packages.js | 147 +++++++++- src/NuGetGallery/UrlExtensions.cs | 88 +++++- .../OwnerRequestsListItemViewModel.cs | 15 +- .../ViewModels/OwnerRequestsListViewModel.cs | 12 +- .../ViewModels/OwnerRequestsViewModel.cs | 4 +- src/NuGetGallery/Views/Users/Packages.cshtml | 250 ++++++++++++++++-- .../Views/Users/_OwnerRequestsList.cshtml | 66 ----- .../Users/_ReservedNamespacesList.cshtml | 50 ---- .../Controllers/PackagesControllerFacts.cs | 92 ++++--- 13 files changed, 552 insertions(+), 207 deletions(-) delete mode 100644 src/NuGetGallery/Views/Users/_OwnerRequestsList.cshtml delete mode 100644 src/NuGetGallery/Views/Users/_ReservedNamespacesList.cshtml diff --git a/src/NuGetGallery/Controllers/PackagesController.cs b/src/NuGetGallery/Controllers/PackagesController.cs index d7e80f218a..a586419acd 100644 --- a/src/NuGetGallery/Controllers/PackagesController.cs +++ b/src/NuGetGallery/Controllers/PackagesController.cs @@ -1301,6 +1301,8 @@ private async Task HandleOwnershipRequest(string id, string userna if (package.Owners.Any(o => o.MatchesUser(user))) { + // If the user is already an owner, clean up the invalid request. + await _packageOwnershipManagementService.DeletePackageOwnershipRequestAsync(package, user); return View("ConfirmOwner", new PackageOwnerConfirmationModel(id, user.Username, ConfirmOwnershipResult.AlreadyOwner)); } @@ -1337,19 +1339,23 @@ private async Task HandleOwnershipRequest(string id, string userna [RequiresAccountConfirmation("cancel pending ownership request")] public virtual async Task CancelPendingOwnershipRequest(string id, string requestingUsername, string pendingUsername) { - if (!string.Equals(requestingUsername, User.Identity.Name, StringComparison.OrdinalIgnoreCase)) + var package = _packageService.FindPackageRegistrationById(id); + if (package == null) + { + return HttpNotFound(); + } + + if (ActionsRequiringPermissions.ManagePackageOwnership.CheckPermissionsOnBehalfOfAnyAccount(GetCurrentUser(), package) != PermissionsCheckResult.Allowed) { return View("ConfirmOwner", new PackageOwnerConfirmationModel(id, requestingUsername, ConfirmOwnershipResult.NotYourRequest)); } - var package = _packageService.FindPackageRegistrationById(id); - if (package == null) + var requestingUser = _userService.FindByUsername(requestingUsername); + if (requestingUser == null) { return HttpNotFound(); } - var requestingUser = GetCurrentUser(); - var pendingUser = _userService.FindByUsername(pendingUsername); if (pendingUser == null) { diff --git a/src/NuGetGallery/Controllers/UsersController.cs b/src/NuGetGallery/Controllers/UsersController.cs index 36bd75c3a5..cdc76d8a16 100644 --- a/src/NuGetGallery/Controllers/UsersController.cs +++ b/src/NuGetGallery/Controllers/UsersController.cs @@ -366,11 +366,20 @@ public virtual ActionResult Packages() .Select(p => new ListPackageItemViewModel(p, currentUser)).OrderBy(p => p.Id) .ToList(); - var received = _packageOwnerRequestService.GetPackageOwnershipRequests(newOwner: currentUser); - var sent = _packageOwnerRequestService.GetPackageOwnershipRequests(requestingOwner: currentUser); + var userReceived = _packageOwnerRequestService.GetPackageOwnershipRequests(newOwner: currentUser); + var orgReceived = currentUser.Organizations + .Where(m => ActionsRequiringPermissions.HandlePackageOwnershipRequest.CheckPermissions(currentUser, m.Organization) == PermissionsCheckResult.Allowed) + .SelectMany(m => _packageOwnerRequestService.GetPackageOwnershipRequests(newOwner: m.Organization)); + var received = userReceived.Union(orgReceived); + + var sent = packages.SelectMany(p => _packageOwnerRequestService.GetPackageOwnershipRequests(package: p.PackageRegistration)); var ownerRequests = new OwnerRequestsViewModel(received, sent, currentUser, _packageService); - var reservedPrefixes = new ReservedNamespaceListViewModel(currentUser.ReservedNamespaces); + + var userReservedNamespaces = currentUser.ReservedNamespaces; + var organizationsReservedNamespaces = currentUser.Organizations.SelectMany(m => m.Organization.ReservedNamespaces); + + var reservedPrefixes = new ReservedNamespaceListViewModel(userReservedNamespaces.Union(organizationsReservedNamespaces).ToArray()); var model = new ManagePackagesViewModel { diff --git a/src/NuGetGallery/NuGetGallery.csproj b/src/NuGetGallery/NuGetGallery.csproj index 54615521be..dc1ce3d170 100644 --- a/src/NuGetGallery/NuGetGallery.csproj +++ b/src/NuGetGallery/NuGetGallery.csproj @@ -2030,10 +2030,8 @@ - - diff --git a/src/NuGetGallery/RouteName.cs b/src/NuGetGallery/RouteName.cs index c5c2debd5f..9e3153fd8c 100644 --- a/src/NuGetGallery/RouteName.cs +++ b/src/NuGetGallery/RouteName.cs @@ -24,7 +24,9 @@ public static class RouteName public const string UploadPackage = "UploadPackage"; public const string UploadPackageProgress = "UploadPackageProgress"; public const string PackageVersionAction = "PackageVersionAction"; + public const string ConfirmPendingOwnershipRequest = "ConfirmPendingOwnershipRequest"; public const string PackageOwnerConfirmation = "PackageOwnerConfirmation"; + public const string RejectPendingOwnershipRequest = "RejectPendingOwnershipRequest"; public const string PackageOwnerRejection = "PackageOwnerRejection"; public const string PackageOwnerCancellation = "PackageOwnerCancellation"; public const string PackageAction = "PackageAction"; diff --git a/src/NuGetGallery/Scripts/gallery/page-manage-packages.js b/src/NuGetGallery/Scripts/gallery/page-manage-packages.js index ea4fa6193f..65ffd41c0a 100644 --- a/src/NuGetGallery/Scripts/gallery/page-manage-packages.js +++ b/src/NuGetGallery/Scripts/gallery/page-manage-packages.js @@ -3,16 +3,16 @@ function showInitialPackagesData(dataSelector, packagesList) { var downloadsCount = 0; - $.each(packagesList, function () { downloadsCount += this.TotalDownloadCount }); + $.each(packagesList, function () { downloadsCount += this.TotalDownloadCount; }); $(dataSelector).text(formatPackagesData(packagesList.length, downloadsCount)); } function formatPackagesData(packagesCount, downloadsCount) { return packagesCount.toLocaleString() - + ' package' + (packagesCount == 1 ? '' : 's') + + ' package' + (packagesCount === 1 ? '' : 's') + ' / ' + downloadsCount.toLocaleString() - + ' download' + (downloadsCount == 1 ? '' : 's'); + + ' download' + (downloadsCount === 1 ? '' : 's'); } $(function () { @@ -90,6 +90,141 @@ }, this); } + function showInitialReservedNamespaceData(dataSelector, namespacesList) { + $(dataSelector).text(formatReservedNamespacesData(namespacesList.length)); + } + + function formatReservedNamespacesData(namespacesCount) { + return namespacesCount.toLocaleString() + " namespace" + (namespacesCount === 1 ? '' : 's'); + } + + function ReservedNamespaceListItemViewModel(reservedNamespaceListViewModel, namespaceItem) { + var self = this; + + this.ReservedNamespaceListViewModel = reservedNamespaceListViewModel; + this.Pattern = namespaceItem.Pattern; + this.SearchUrl = namespaceItem.SearchUrl; + this.Owners = namespaceItem.Owners; + this.IsPublic = namespaceItem.IsPublic; + + this.Visible = ko.observable(true); + + this.UpdateVisibility = function (ownerFilter) { + var visible = ownerFilter === "All packages"; + if (!visible) { + for (var i in self.Owners) { + if (ownerFilter === self.Owners[i].Username) { + visible = true; + break; + } + } + } + this.Visible(visible); + }; + } + + function ReservedNamespaceListViewModel(managePackagesViewModel, namespaces) { + var self = this; + + this.ManagePackagesViewModel = managePackagesViewModel; + this.Namespaces = $.map(namespaces, function (data) { + return new ReservedNamespaceListItemViewModel(self, data); + }); + this.VisibleNamespacesCount = ko.observable(); + this.VisibleNamespacesHeading = ko.pureComputed(function () { + return formatReservedNamespacesData(ko.unwrap(self.VisibleNamespacesCount())); + }); + + this.ManagePackagesViewModel.OwnerFilter.subscribe(function (newOwner) { + var namespacesCount = 0; + for (var i in self.Namespaces) { + self.Namespaces[i].UpdateVisibility(newOwner.Username); + if (self.Namespaces[i].Visible()) { + namespacesCount++; + } + } + this.VisibleNamespacesCount(namespacesCount); + }, this); + } + + function showInitialOwnerRequestsData(dataSelector, requestsList) { + $(dataSelector).text(formatOwnerRequestsData(requestsList.length)); + } + + function formatOwnerRequestsData(requestsCount) { + return requestsCount.toLocaleString() + " request" + (requestsCount === 1 ? '' : 's'); + } + + function OwnerRequestsItemViewModel(ownerRequestsListViewModel, ownerRequestItem, showReceived, showSent) { + var self = this; + + this.OwnerRequestsListViewModel = ownerRequestsListViewModel; + this.Id = ownerRequestItem.Id; + this.Requesting = ownerRequestItem.Requesting; + this.New = ownerRequestItem.New; + this.Owners = ownerRequestItem.Owners; + this.PackageIconUrl = ownerRequestItem.PackageIconUrl + ? ownerRequestItem.PackageIconUrl + : this.OwnerRequestsListViewModel.ManagePackagesViewModel.DefaultPackageIconUrl; + this.PackageUrl = ownerRequestItem.PackageUrl; + this.CanAccept = ownerRequestItem.CanAccept; + this.CanCancel = ownerRequestItem.CanCancel; + this.ConfirmUrl = ownerRequestItem.ConfirmUrl; + this.RejectUrl = ownerRequestItem.RejectUrl; + this.CancelUrl = ownerRequestItem.CancelUrl; + this.ShowReceived = showReceived; + this.ShowSent = showSent; + + this.Visible = ko.observable(true); + + this.UpdateVisibility = function (ownerFilter) { + var visible = ownerFilter === "All packages"; + if (!visible) { + if (self.ShowReceived && ownerFilter === self.New.Username) { + visible = true; + } + + if (self.ShowSent) { + for (var i in self.Owners) { + if (ownerFilter === self.Owners[i].Username) { + visible = true; + break; + } + } + } + } + this.Visible(visible); + }; + this.PackageIconUrlFallback = ko.pureComputed(function () { + var url = this.OwnerRequestsListViewModel.ManagePackagesViewModel.PackageIconUrlFallback; + return "this.src='" + url + "'; this.onerror = null;"; + }, this); + } + + function OwnerRequestsListViewModel(managePackagesViewModel, requests, showReceived, showSent) { + var self = this; + + this.ManagePackagesViewModel = managePackagesViewModel; + this.Requests = $.map(requests, function (data) { + return new OwnerRequestsItemViewModel(self, data, showReceived, showSent); + }); + this.VisibleRequestsCount = ko.observable(); + this.VisibleRequestsHeading = ko.pureComputed(function () { + return formatOwnerRequestsData(ko.unwrap(self.VisibleRequestsCount())); + }, this); + + this.ManagePackagesViewModel.OwnerFilter.subscribe(function (newOwner) { + var requestsCount = 0; + for (var i in self.Requests) { + self.Requests[i].UpdateVisibility(newOwner.Username); + if (self.Requests[i].Visible()) { + requestsCount++; + } + } + this.VisibleRequestsCount(requestsCount); + }, this); + } + function ManagePackagesViewModel(initialData) { var self = this; @@ -105,11 +240,17 @@ this.ListedPackages = new PackagesListViewModel(this, "published", initialData.ListedPackages); this.UnlistedPackages = new PackagesListViewModel(this, "unlisted", initialData.UnlistedPackages); + this.ReservedNamespaces = new ReservedNamespaceListViewModel(this, initialData.ReservedNamespaces); + this.RequestsReceived = new OwnerRequestsListViewModel(this, initialData.RequestsReceived, true, false); + this.RequestsSent = new OwnerRequestsListViewModel(this, initialData.RequestsSent, false, true); } // Immediately load initial expander data showInitialPackagesData("#listed-data", initialData.ListedPackages); showInitialPackagesData("#unlisted-data", initialData.UnlistedPackages); + showInitialReservedNamespaceData("#namespaces-data", initialData.ReservedNamespaces); + showInitialOwnerRequestsData("#requests-received-data", initialData.RequestsReceived); + showInitialOwnerRequestsData("#requests-sent-data", initialData.RequestsSent); // Set up the data binding. var managePackagesViewModel = new ManagePackagesViewModel(initialData); diff --git a/src/NuGetGallery/UrlExtensions.cs b/src/NuGetGallery/UrlExtensions.cs index e3e9e9ee74..534bcb9519 100644 --- a/src/NuGetGallery/UrlExtensions.cs +++ b/src/NuGetGallery/UrlExtensions.cs @@ -447,6 +447,22 @@ public static string Register(this UrlHelper url, bool relativeUrl = true) return GetActionLink(url, "LogOn", "Authentication", relativeUrl); } + public static RouteUrlTemplate SearchTemplate(this UrlHelper url, bool relativeUrl = true) + { + var routesGenerator = new Dictionary> + { + { "q", s => s } + }; + + Func linkGenerator = rv => GetRouteLink( + url, + RouteName.ListPackages, + relativeUrl, + routeValues: rv); + + return new RouteUrlTemplate(linkGenerator, routesGenerator); + } + public static string Search(this UrlHelper url, string searchTerm, bool relativeUrl = true) { return GetRouteLink( @@ -873,6 +889,12 @@ public static string RemovePackageOwner(this UrlHelper url, bool relativeUrl = t return GetActionLink(url, "RemovePackageOwner", "JsonApi", relativeUrl); } + public static RouteUrlTemplate ConfirmPendingOwnershipRequestTemplate( + this UrlHelper url, bool relativeUrl = true) + { + return HandlePendingOwnershipRequestTemplate(url, RouteName.ConfirmPendingOwnershipRequest, relativeUrl); + } + public static string ConfirmPendingOwnershipRequest( this UrlHelper url, string packageId, @@ -880,18 +902,50 @@ public static string ConfirmPendingOwnershipRequest( string confirmationCode, bool relativeUrl = true) { - var routeValues = new RouteValueDictionary + return HandlePendingOwnershipRequest(url, RouteName.ConfirmPendingOwnershipRequest, packageId, username, confirmationCode, relativeUrl); + } + + public static RouteUrlTemplate RejectPendingOwnershipRequestTemplate( + this UrlHelper url, bool relativeUrl = true) + { + return HandlePendingOwnershipRequestTemplate(url, RouteName.RejectPendingOwnershipRequest, relativeUrl); + } + + public static string RejectPendingOwnershipRequest( + this UrlHelper url, + string packageId, + string username, + string confirmationCode, + bool relativeUrl = true) + { + return HandlePendingOwnershipRequest(url, RouteName.RejectPendingOwnershipRequest, packageId, username, confirmationCode, relativeUrl); + } + + private static RouteUrlTemplate HandlePendingOwnershipRequestTemplate( + this UrlHelper url, + string routeName, + bool relativeUrl = true) + { + var routesGenerator = new Dictionary> { - ["id"] = packageId, - ["username"] = username, - ["token"] = confirmationCode + { "id", r => r.Package.Id }, + { "username", r => r.Request.NewOwner.Username }, + { "token", r => r.Request.ConfirmationCode } }; - return GetActionLink(url, "ConfirmPendingOwnershipRequest", "Packages", relativeUrl, routeValues); + Func linkGenerator = rv => GetActionLink( + url, + routeName, + "Packages", + relativeUrl, + routeValues: rv); + + return new RouteUrlTemplate(linkGenerator, routesGenerator); } - public static string RejectPendingOwnershipRequest( + private static string HandlePendingOwnershipRequest( this UrlHelper url, + string routeName, string packageId, string username, string confirmationCode, @@ -904,7 +958,27 @@ public static string RejectPendingOwnershipRequest( ["token"] = confirmationCode }; - return GetActionLink(url, "RejectPendingOwnershipRequest", "Packages", relativeUrl, routeValues); + return GetActionLink(url, routeName, "Packages", relativeUrl, routeValues); + } + + public static RouteUrlTemplate CancelPendingOwnershipRequestTemplate( + this UrlHelper url, bool relativeUrl = true) + { + var routesGenerator = new Dictionary> + { + { "id", r => r.Package.Id }, + { "requestingUsername", r => r.Request.RequestingOwner.Username }, + { "pendingUsername", r => r.Request.NewOwner.Username } + }; + + Func linkGenerator = rv => GetActionLink( + url, + "CancelPendingOwnershipRequest", + "Packages", + relativeUrl, + routeValues: rv); + + return new RouteUrlTemplate(linkGenerator, routesGenerator); } public static string CancelPendingOwnershipRequest( diff --git a/src/NuGetGallery/ViewModels/OwnerRequestsListItemViewModel.cs b/src/NuGetGallery/ViewModels/OwnerRequestsListItemViewModel.cs index 0d694cf7bb..9a503b29df 100644 --- a/src/NuGetGallery/ViewModels/OwnerRequestsListItemViewModel.cs +++ b/src/NuGetGallery/ViewModels/OwnerRequestsListItemViewModel.cs @@ -5,14 +5,23 @@ namespace NuGetGallery { public class OwnerRequestsListItemViewModel { - public OwnerRequestsListItemViewModel(PackageOwnerRequest request, IPackageService packageService) + public OwnerRequestsListItemViewModel(PackageOwnerRequest request, IPackageService packageService, User currentUser) { Request = request; - Package = packageService.FindPackageByIdAndVersion(request.PackageRegistration.Id, version: null, semVerLevelKey: SemVerLevelKey.SemVer2, allowPrerelease: true); + + var package = packageService.FindPackageByIdAndVersion(request.PackageRegistration.Id, version: null, semVerLevelKey: SemVerLevelKey.SemVer2, allowPrerelease: true); + Package = new ListPackageItemViewModel(package, currentUser); + + CanAccept = ActionsRequiringPermissions.HandlePackageOwnershipRequest.CheckPermissions(currentUser, Request.NewOwner) == PermissionsCheckResult.Allowed; + CanCancel = Package.CanManageOwners; } public PackageOwnerRequest Request { get; } - public Package Package { get; } + public ListPackageItemViewModel Package { get; } + + public bool CanAccept { get; } + + public bool CanCancel { get; } } } \ No newline at end of file diff --git a/src/NuGetGallery/ViewModels/OwnerRequestsListViewModel.cs b/src/NuGetGallery/ViewModels/OwnerRequestsListViewModel.cs index 5d985c5fd4..f5e748e1bc 100644 --- a/src/NuGetGallery/ViewModels/OwnerRequestsListViewModel.cs +++ b/src/NuGetGallery/ViewModels/OwnerRequestsListViewModel.cs @@ -8,17 +8,11 @@ namespace NuGetGallery { public class OwnerRequestsListViewModel { - public OwnerRequestsListViewModel(IEnumerable requests, string name, User currentUser, IPackageService packageService) + public OwnerRequestsListViewModel(IEnumerable requests, User currentUser, IPackageService packageService) { - RequestItems = requests.Select(r => new OwnerRequestsListItemViewModel(r, packageService)).ToArray(); - Name = name; - CurrentUser = currentUser; + Requests = requests.Select(r => new OwnerRequestsListItemViewModel(r, packageService, currentUser)).ToArray(); } - public IEnumerable RequestItems { get; } - - public string Name { get; } - - public User CurrentUser { get; } + public IEnumerable Requests { get; } } } \ No newline at end of file diff --git a/src/NuGetGallery/ViewModels/OwnerRequestsViewModel.cs b/src/NuGetGallery/ViewModels/OwnerRequestsViewModel.cs index 7058a59498..58f44b1fac 100644 --- a/src/NuGetGallery/ViewModels/OwnerRequestsViewModel.cs +++ b/src/NuGetGallery/ViewModels/OwnerRequestsViewModel.cs @@ -23,8 +23,8 @@ public OwnerRequestsViewModel( User currentUser, IPackageService packageService) { - Received = new OwnerRequestsListViewModel(received, nameof(Received), currentUser, packageService); - Sent = new OwnerRequestsListViewModel(sent, nameof(Sent), currentUser, packageService); + Received = new OwnerRequestsListViewModel(received, currentUser, packageService); + Sent = new OwnerRequestsListViewModel(sent, currentUser, packageService); } } } \ No newline at end of file diff --git a/src/NuGetGallery/Views/Users/Packages.cshtml b/src/NuGetGallery/Views/Users/Packages.cshtml index c70befc131..206d47aab2 100644 --- a/src/NuGetGallery/Views/Users/Packages.cshtml +++ b/src/NuGetGallery/Views/Users/Packages.cshtml @@ -4,16 +4,12 @@ ViewBag.Title = "Manage My Package"; ViewBag.Tab = "Packages"; Layout = "~/Views/Shared/Gallery/Layout.cshtml"; - - var namespacesCount = Model.ReservedNamespaces.ReservedNamespaces.Count(); - var ownerRequestsReceivedCount = Model.OwnerRequests.Received.RequestItems.Count(); - var ownerRequestsSentCount = Model.OwnerRequests.Sent.RequestItems.Count(); }
- +

Manage Packages

@@ -44,36 +40,49 @@
, expanded: false) - @if (namespacesCount > 0) + @if (Model.ReservedNamespaces.ReservedNamespaces.Any()) { @ViewHelpers.Section(this, "namespaces", @Reserved Namespaces, - @@Html.Raw(namespacesCount + " namespace" + (namespacesCount == 1 ? "" : "s")), @ - @Html.Partial("_ReservedNamespacesList", Model.ReservedNamespaces) + + , + @ +
+
+
, expanded: false) } - @if (Model.OwnerRequests.Received.RequestItems.Any()) + @if (Model.OwnerRequests.Received.Requests.Any()) { @ViewHelpers.Section(this, "requests-received", @Ownership Requests (Received), - @@Html.Raw(ownerRequestsReceivedCount + " request" + (ownerRequestsReceivedCount == 1 ? "" : "s")), @ - @Html.Partial("_OwnerRequestsList", Model.OwnerRequests.Received) + + , + @ +
+
+
, expanded: false) } - @if (Model.OwnerRequests.Sent.RequestItems.Any()) + @if (Model.OwnerRequests.Sent.Requests.Any()) { @ViewHelpers.Section(this, "requests-sent", @Ownership Requests (Sent), - @@Html.Raw(ownerRequestsSentCount + " request" + (ownerRequestsSentCount == 1 ? "" : "s")), @ - @Html.Partial("_OwnerRequestsList", Model.OwnerRequests.Sent) + + , + @ +
+
+
, expanded: false) }
+
+ + + + @functions { + // Performance: RouteCollection.VirtualPath is expensive, so resolve the path once and save as a template. + // Then substitute route values into the template path when rendering links for each package on the page. private RouteUrlTemplate userUrlTemplate; + private RouteUrlTemplate packageUrlTemplate; private RouteUrlTemplate editUrlTemplate; private RouteUrlTemplate manageOwnersUrlTemplate; private RouteUrlTemplate deleteUrlTemplate; + private RouteUrlTemplate searchUrlTemplate; + + private RouteUrlTemplate confirmUrlTemplate; + private RouteUrlTemplate rejectUrlTemplate; + private RouteUrlTemplate cancelUrlTemplate; + dynamic GetSerializablePackage(ListPackageItemViewModel p) { - if (deleteUrlTemplate == null) + if (packageUrlTemplate == null) { - // Performance: RouteCollection.VirtualPath is expensive, so resolve the path once and save as a template. - // Then substitute route values into the template path when rendering links for each package on the page. - userUrlTemplate = Url.UserTemplate(); packageUrlTemplate = Url.PackageRegistrationTemplate(); + } + + if (editUrlTemplate == null) + { editUrlTemplate = Url.EditPackageTemplate(); + } + + if (manageOwnersUrlTemplate == null) + { manageOwnersUrlTemplate = Url.ManagePackageOwnersTemplate(); + } + + if (deleteUrlTemplate == null) + { deleteUrlTemplate = Url.DeletePackageTemplate(); } return new { p.Id, - Owners = p.Owners.Select(o => new - { - o.Username, - ProfileUrl = userUrlTemplate.Resolve(o), - IsOrganization = o is Organization - }), + Owners = GetSerializableOwners(p.Owners), p.TotalDownloadCount, LatestVersion = p.FullVersion.Abbreviate(25), PackageIconUrl = PackageHelper.ShouldRenderUrl(p.IconUrl) ? p.IconUrl : null, @@ -184,6 +300,80 @@ CanDelete = p.CanUnlistOrRelist }; } + + dynamic GetSerializableNamespace(ReservedNamespaceListItemViewModel n) + { + if (searchUrlTemplate == null) + { + searchUrlTemplate = Url.SearchTemplate(); + } + + return new + { + Pattern = n.GetPattern(), + SearchUrl = searchUrlTemplate.Resolve(n.Value), + Owners = GetSerializableOwners(n.Owners), + n.IsPublic + }; + } + + dynamic GetSerializableOwnerRequest(OwnerRequestsListItemViewModel r) + { + if (packageUrlTemplate == null) + { + packageUrlTemplate = Url.PackageRegistrationTemplate(); + } + + if (confirmUrlTemplate == null) + { + confirmUrlTemplate = Url.ConfirmPendingOwnershipRequestTemplate(); + } + + if (rejectUrlTemplate == null) + { + rejectUrlTemplate = Url.RejectPendingOwnershipRequestTemplate(); + } + + if (cancelUrlTemplate == null) + { + cancelUrlTemplate = Url.CancelPendingOwnershipRequestTemplate(); + } + + return new + { + r.Request.PackageRegistration.Id, + PackageIconUrl = PackageHelper.ShouldRenderUrl(r.Package.IconUrl) ? r.Package.IconUrl : null, + PackageUrl = packageUrlTemplate.Resolve(r.Package), + Owners = GetSerializableOwners(r.Package.Owners), + Requesting = GetSerializableOwner(r.Request.RequestingOwner), + New = GetSerializableOwner(r.Request.NewOwner), + CanAccept = r.CanAccept, + CanCancel = r.CanCancel, + ConfirmUrl = confirmUrlTemplate.Resolve(r), + RejectUrl = rejectUrlTemplate.Resolve(r), + CancelUrl = cancelUrlTemplate.Resolve(r) + }; + } + + dynamic GetSerializableOwners(IEnumerable owners) + { + return owners.Select(o => GetSerializableOwner(o)); + } + + dynamic GetSerializableOwner(User user) + { + if (userUrlTemplate == null) + { + userUrlTemplate = Url.UserTemplate(); + } + + return new + { + user.Username, + ProfileUrl = userUrlTemplate.Resolve(user), + IsOrganization = user is Organization + }; + } } @section BottomScripts { @@ -195,6 +385,12 @@ .Select(p => GetSerializablePackage(p)), UnlistedPackages = Model.UnlistedPackages .Select(p => GetSerializablePackage(p)), + ReservedNamespaces = Model.ReservedNamespaces.ReservedNamespaces + .Select(n => GetSerializableNamespace(n)), + RequestsReceived = Model.OwnerRequests.Received.Requests + .Select(r => GetSerializableOwnerRequest(r)), + RequestsSent = Model.OwnerRequests.Sent.Requests + .Select(r => GetSerializableOwnerRequest(r)), DefaultPackageIconUrl = Url.Absolute("~/Content/gallery/img/default-package-icon.svg"), PackageIconUrlFallback = Url.Absolute("~/Content/gallery/img/default-package-icon-256x256.png") })); diff --git a/src/NuGetGallery/Views/Users/_OwnerRequestsList.cshtml b/src/NuGetGallery/Views/Users/_OwnerRequestsList.cshtml deleted file mode 100644 index 0bbf806709..0000000000 --- a/src/NuGetGallery/Views/Users/_OwnerRequestsList.cshtml +++ /dev/null @@ -1,66 +0,0 @@ -@model OwnerRequestsListViewModel -@{ - var requestsString = "request" + (Model.RequestItems.Count() != 1 ? "s" : ""); -} - -
-
-
-

- You have @Model.RequestItems.Count().ToNuGetNumberString() @Model.Name.ToLowerInvariant() @requestsString -

-
-
- - - - - - - - - - - @foreach (var requestItem in @Model.RequestItems) - { - var packageId = requestItem.Request.PackageRegistration.Id; - - - - - - - - - } - -
Package IDExisting OwnerNew Owner
@Html.BreakWord(packageId) - @requestItem.Request.RequestingOwner.Username - - @requestItem.Request.NewOwner.Username - - @if (CurrentUser.Key == requestItem.Request.NewOwnerKey) - { - - - - - - - - } - else - { - - - - } -
-
-
-
-
\ No newline at end of file diff --git a/src/NuGetGallery/Views/Users/_ReservedNamespacesList.cshtml b/src/NuGetGallery/Views/Users/_ReservedNamespacesList.cshtml deleted file mode 100644 index ea7dc4ee33..0000000000 --- a/src/NuGetGallery/Views/Users/_ReservedNamespacesList.cshtml +++ /dev/null @@ -1,50 +0,0 @@ -@model ReservedNamespaceListViewModel - -
-
-
-
- - - - - - - - - - - @foreach (var reservedNamespace in @Model.ReservedNamespaces) - { - - - - - @if (reservedNamespace.IsPublic) - { - - } - else - { - - } - - } - -
Package ID or PrefixOwnersUpload Restrictions
- @reservedNamespace.GetPattern() - - @foreach (var owner in reservedNamespace.Owners) - { - @owner.Username - } - Any NuGet.org AccountPrefix or ID Owners Only
-
-
-
-
\ No newline at end of file diff --git a/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs index 5c7ed7fd17..b5113763ab 100644 --- a/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs @@ -854,11 +854,13 @@ public async Task WithOwnerReturnsAlreadyOwnerResult(InvokeOwnershipRequest invo packageService.Setup(p => p.FindPackageRegistrationById(package.Id)).Returns(package); var userService = new Mock(); userService.Setup(x => x.FindByUsername(user.Username)).Returns(user); + var packageOwnershipManagementService = new Mock(); var controller = CreateController( GetConfigurationService(), httpContext: mockHttpContext, packageService: packageService, - userService: userService); + userService: userService, + packageOwnershipManagementService: packageOwnershipManagementService); controller.SetCurrentUser(user); TestUtility.SetupHttpContextMockForUrlGeneration(mockHttpContext, controller); @@ -868,6 +870,7 @@ public async Task WithOwnerReturnsAlreadyOwnerResult(InvokeOwnershipRequest invo // Assert var model = ResultAssert.IsView(result, "ConfirmOwner"); Assert.Equal(ConfirmOwnershipResult.AlreadyOwner, model.Result); + packageOwnershipManagementService.Verify(x => x.DeletePackageOwnershipRequestAsync(package, user)); } public delegate Expression> PackageOwnershipManagementServiceRequestExpression(PackageRegistration package, User user); @@ -1005,48 +1008,75 @@ public async Task ReturnsSuccessIfTokenIsValid( public class TheCancelPendingOwnershipRequestMethod : TestContainer { + + public static IEnumerable NotOwner_Data + { + get + { + yield return MemberDataHelper.AsData((User)null, TestUtility.FakeUser); + yield return MemberDataHelper.AsData(TestUtility.FakeUser, new User { Key = 1553 }); + yield return MemberDataHelper.AsData(TestUtility.FakeOrganizationCollaborator, TestUtility.FakeOrganization); + } + } + + public static IEnumerable Owner_Data + { + get + { + yield return MemberDataHelper.AsData(TestUtility.FakeUser, TestUtility.FakeUser); + yield return MemberDataHelper.AsData(TestUtility.FakeAdminUser, TestUtility.FakeUser); + yield return MemberDataHelper.AsData(TestUtility.FakeOrganizationAdmin, TestUtility.FakeOrganization); + } + } + [Fact] - public async Task WithIdentityNotMatchingUserInRequestReturnsViewWithMessage() + public async Task WithNonExistentPackageIdReturnsHttpNotFound() { // Arrange var controller = CreateController(GetConfigurationService()); - controller.SetCurrentUser(new User("userA")); + controller.SetCurrentUser(new User { Username = "userA" }); // Act - var result = await controller.CancelPendingOwnershipRequest("foo", "userB", "userC"); + var result = await controller.CancelPendingOwnershipRequest("foo", "userA", "userB"); // Assert - var model = ResultAssert.IsView(result, "ConfirmOwner"); - Assert.Equal(ConfirmOwnershipResult.NotYourRequest, model.Result); - Assert.Equal("userB", model.Username); + Assert.IsType(result); } - [Fact] - public async Task WithNonExistentPackageIdReturnsHttpNotFound() + [Theory] + [MemberData(nameof(NotOwner_Data))] + public async Task WithNonOwningCurrentUserReturnsNotYourRequest(User currentUser, User owner) { // Arrange - var controller = CreateController(GetConfigurationService()); - controller.SetCurrentUser(new User { Username = "userA" }); + var package = new PackageRegistration { Id = "foo", Owners = new[] { owner } }; + var packageService = new Mock(); + packageService.Setup(p => p.FindPackageRegistrationById("foo")).Returns(package); + var controller = CreateController( + GetConfigurationService(), + packageService: packageService); + controller.SetCurrentUser(currentUser); // Act var result = await controller.CancelPendingOwnershipRequest("foo", "userA", "userB"); // Assert - Assert.IsType(result); + var model = ResultAssert.IsView(result, "ConfirmOwner"); + Assert.Equal(ConfirmOwnershipResult.NotYourRequest, model.Result); + Assert.Equal("userA", model.Username); } - [Fact] - public async Task WithNonExistentPendingUserReturnsHttpNotFound() + [Theory] + [MemberData(nameof(Owner_Data))] + public async Task WithNonExistentPendingUserReturnsHttpNotFound(User currentUser, User owner) { // Arrange - var package = new PackageRegistration { Id = "foo" }; - var user = new User { Username = "userA" }; + var package = new PackageRegistration { Id = "foo", Owners = new[] { owner } }; var packageService = new Mock(); packageService.Setup(p => p.FindPackageRegistrationById("foo")).Returns(package); var controller = CreateController( GetConfigurationService(), packageService: packageService); - controller.SetCurrentUser(user); + controller.SetCurrentUser(currentUser); // Act var result = await controller.CancelPendingOwnershipRequest("foo", "userA", "userB"); @@ -1055,12 +1085,13 @@ public async Task WithNonExistentPendingUserReturnsHttpNotFound() Assert.IsType(result); } - [Fact] - public async Task WithNonExistentPackageOwnershipRequestReturnsHttpNotFound() + [Theory] + [MemberData(nameof(Owner_Data))] + public async Task WithNonExistentPackageOwnershipRequestReturnsHttpNotFound(User currentUser, User owner) { // Arrange var packageId = "foo"; - var package = new PackageRegistration { Id = packageId }; + var package = new PackageRegistration { Id = packageId, Owners = new[] { owner } }; var packageService = new Mock(); packageService.Setup(p => p.FindPackageRegistrationById(packageId)).Returns(package); @@ -1079,7 +1110,7 @@ public async Task WithNonExistentPackageOwnershipRequestReturnsHttpNotFound() GetConfigurationService(), userService: userService, packageService: packageService); - controller.SetCurrentUser(userA); + controller.SetCurrentUser(owner); // Act var result = await controller.CancelPendingOwnershipRequest(packageId, userAName, userBName); @@ -1088,22 +1119,23 @@ public async Task WithNonExistentPackageOwnershipRequestReturnsHttpNotFound() Assert.IsType(result); } - [Fact] - public async Task ReturnsCancelledIfPackageOwnershipRequestExists() + [Theory] + [MemberData(nameof(Owner_Data))] + public async Task ReturnsCancelledIfPackageOwnershipRequestExists(User currentUser, User owner) { // Arrange - var packageId = "foo"; - var package = new PackageRegistration { Id = packageId }; - - var packageService = new Mock(); - packageService.Setup(p => p.FindPackageRegistrationById(packageId)).Returns(package); - var userAName = "userA"; var userA = new User { Username = userAName }; var userBName = "userB"; var userB = new User { Username = userBName }; + var packageId = "foo"; + var package = new PackageRegistration { Id = packageId, Owners = new[] { owner } }; + + var packageService = new Mock(); + packageService.Setup(p => p.FindPackageRegistrationById(packageId)).Returns(package); + var userService = new Mock(); userService.Setup(u => u.FindByUsername(userAName)).Returns(userA); userService.Setup(u => u.FindByUsername(userBName)).Returns(userB); @@ -1121,7 +1153,7 @@ public async Task ReturnsCancelledIfPackageOwnershipRequestExists() packageService: packageService, packageOwnershipManagementService: packageOwnershipManagementRequestService, messageService: messageService); - controller.SetCurrentUser(userA); + controller.SetCurrentUser(currentUser); // Act var result = await controller.CancelPendingOwnershipRequest(packageId, userAName, userBName); From 276e9fec938b49ed7817268c9065834464ee9e9b Mon Sep 17 00:00:00 2001 From: Scott Bommarito Date: Thu, 1 Mar 2018 15:16:46 -0800 Subject: [PATCH 13/24] [Organizations] Small UI fixes (#5567) --- src/Bootstrap/dist/css/bootstrap-theme.css | 66 +++++++++++++++ src/Bootstrap/less/theme/all.less | 3 +- .../less/theme/page-manage-organizations.less | 16 ++++ .../less/theme/page-transform-account.less | 24 ++++++ src/NuGetGallery/App_Code/ViewHelpers.cshtml | 2 +- src/NuGetGallery/Strings.Designer.cs | 4 +- src/NuGetGallery/Strings.resx | 4 +- .../_OrganizationAccountManageMembers.cshtml | 28 +++---- src/NuGetGallery/Views/Users/Transform.cshtml | 82 ++++++++++--------- 9 files changed, 169 insertions(+), 60 deletions(-) create mode 100644 src/Bootstrap/less/theme/page-transform-account.less diff --git a/src/Bootstrap/dist/css/bootstrap-theme.css b/src/Bootstrap/dist/css/bootstrap-theme.css index 4ac1f366b4..300543e089 100644 --- a/src/Bootstrap/dist/css/bootstrap-theme.css +++ b/src/Bootstrap/dist/css/bootstrap-theme.css @@ -1018,6 +1018,18 @@ img.reserved-indicator-icon { .manage-members-listing { margin-bottom: 0; } +.manage-members-listing .heading-left { + padding-left: 5px; +} +.manage-members-listing .heading-right { + padding-right: 5px; +} +.manage-members-listing .icon-left { + padding-left: 25px; +} +.manage-members-listing .member-icon { + vertical-align: middle; +} .page-manage-owners h2 .ms-Icon { position: relative; top: -2px; @@ -1418,6 +1430,60 @@ img.reserved-indicator-icon { .page-status .table tbody + tbody { border: none; } +.page-transform-account .center-box { + display: -webkit-box; + display: -webkit-flex; + display: -ms-flexbox; + display: flex; + + -webkit-box-pack: justify; + -webkit-justify-content: space-between; + -ms-flex-pack: justify; + justify-content: space-between; + -webkit-box-align: stretch; + -webkit-align-items: stretch; + -ms-flex-align: stretch; + align-items: stretch; +} +.page-transform-account .center-box .form-box { + display: -webkit-box; + display: -webkit-flex; + display: -ms-flexbox; + display: flex; + flex-direction: column; + + -webkit-box-pack: justify; + -webkit-justify-content: space-between; + -ms-flex-pack: justify; + justify-content: space-between; + -webkit-box-orient: vertical; + -webkit-box-direction: normal; + -webkit-flex-direction: column; + -ms-flex-direction: column; +} +.page-transform-account .center-box .logo-box { + display: -webkit-box; + display: -webkit-flex; + display: -ms-flexbox; + display: flex; + margin-bottom: 50px; + flex-direction: column; + + -webkit-box-pack: justify; + -webkit-justify-content: space-between; + -ms-flex-pack: justify; + justify-content: space-between; + -webkit-box-orient: vertical; + -webkit-box-direction: normal; + -webkit-flex-direction: column; + -ms-flex-direction: column; +} +.page-transform-account .center-box .logo-box .logo { + -webkit-box-flex: 1; + -webkit-flex-grow: 1; + -ms-flex-positive: 1; + flex-grow: 1; +} .page-upload #browse-for-package-button { margin: 0; } diff --git a/src/Bootstrap/less/theme/all.less b/src/Bootstrap/less/theme/all.less index d65be97d39..3eae94d985 100644 --- a/src/Bootstrap/less/theme/all.less +++ b/src/Bootstrap/less/theme/all.less @@ -29,4 +29,5 @@ @import "page-statistics-overview.less"; @import "page-statistics-per-package.less"; @import "page-status.less"; -@import "page-upload.less"; +@import "page-transform-account.less"; +@import "page-upload.less"; \ No newline at end of file diff --git a/src/Bootstrap/less/theme/page-manage-organizations.less b/src/Bootstrap/less/theme/page-manage-organizations.less index e5bb19ca39..79ad41c436 100644 --- a/src/Bootstrap/less/theme/page-manage-organizations.less +++ b/src/Bootstrap/less/theme/page-manage-organizations.less @@ -45,4 +45,20 @@ .manage-members-listing { margin-bottom: 0; + + .heading-left { + padding-left: 5px; + } + + .heading-right { + padding-right: 5px; + } + + .icon-left { + padding-left: 25px; + } + + .member-icon { + vertical-align: middle; + } } \ No newline at end of file diff --git a/src/Bootstrap/less/theme/page-transform-account.less b/src/Bootstrap/less/theme/page-transform-account.less new file mode 100644 index 0000000000..f51a3a9973 --- /dev/null +++ b/src/Bootstrap/less/theme/page-transform-account.less @@ -0,0 +1,24 @@ +.page-transform-account { + .center-box { + display: flex; + justify-content: space-between; + align-items: stretch; + + .form-box { + display: flex; + justify-content: space-between; + flex-direction: column; + } + + .logo-box { + display: flex; + justify-content: space-between; + flex-direction: column; + margin-bottom: 50px; + + .logo { + flex-grow: 1; + } + } + } +} \ No newline at end of file diff --git a/src/NuGetGallery/App_Code/ViewHelpers.cshtml b/src/NuGetGallery/App_Code/ViewHelpers.cshtml index 7831577342..c5509e516b 100644 --- a/src/NuGetGallery/App_Code/ViewHelpers.cshtml +++ b/src/NuGetGallery/App_Code/ViewHelpers.cshtml @@ -105,7 +105,7 @@ { @AlertWarning( @ - NuGet.org password login is being deprecated. Please use a Microsoft account to sign into NuGet gallery. + NuGet.org password login is deprecated. Please use a Microsoft account to sign into NuGet gallery. ) } diff --git a/src/NuGetGallery/Strings.Designer.cs b/src/NuGetGallery/Strings.Designer.cs index 34ea87be8d..6e46b1c2d5 100644 --- a/src/NuGetGallery/Strings.Designer.cs +++ b/src/NuGetGallery/Strings.Designer.cs @@ -721,7 +721,7 @@ public static string Emails_CredentialRemoved_Subject { /// ///[{0}]({0}) /// - ///Note that NuGet.org password login is being deprecated. Please use Microsoft account to sign into {1}. + ///Note that NuGet.org password login is deprecated. Please use Microsoft account to sign into {1}. /// ///Thanks, ///The {1} Team. @@ -749,7 +749,7 @@ public static string Emails_ForgotPassword_Subject { /// ///[{0}]({0}) /// - ///Note that NuGet.org password login is being deprecated. Please use Microsoft account to sign into {1}. + ///Note that NuGet.org password login is deprecated. Please use Microsoft account to sign into {1}. /// ///Thanks, ///The {1} Team. diff --git a/src/NuGetGallery/Strings.resx b/src/NuGetGallery/Strings.resx index d0cf4e8899..0438cc938a 100644 --- a/src/NuGetGallery/Strings.resx +++ b/src/NuGetGallery/Strings.resx @@ -245,7 +245,7 @@ Click the following link within the next hour to reset your password: [{0}]({0}) -Note that NuGet.org password login is being deprecated. Please use Microsoft account to sign into {1}. +Note that NuGet.org password login is deprecated. Please use Microsoft account to sign into {1}. Thanks, The {1} Team @@ -261,7 +261,7 @@ Click the following link within the next hour to set your password: [{0}]({0}) -Note that NuGet.org password login is being deprecated. Please use Microsoft account to sign into {1}. +Note that NuGet.org password login is deprecated. Please use Microsoft account to sign into {1}. Thanks, The {1} Team diff --git a/src/NuGetGallery/Views/Organizations/_OrganizationAccountManageMembers.cshtml b/src/NuGetGallery/Views/Organizations/_OrganizationAccountManageMembers.cshtml index e9a060ff17..69e5f299eb 100644 --- a/src/NuGetGallery/Views/Organizations/_OrganizationAccountManageMembers.cshtml +++ b/src/NuGetGallery/Views/Organizations/_OrganizationAccountManageMembers.cshtml @@ -22,33 +22,31 @@ + @ViewHelpers.SectionsScript(this) + @Scripts.Render("~/Scripts/gallery/page-signin.min.js") +} diff --git a/src/NuGetGallery/Views/Authentication/SignInNugetAccount.cshtml b/src/NuGetGallery/Views/Authentication/SignInNugetAccount.cshtml index 78ac015196..f56f4d837d 100644 --- a/src/NuGetGallery/Views/Authentication/SignInNugetAccount.cshtml +++ b/src/NuGetGallery/Views/Authentication/SignInNugetAccount.cshtml @@ -8,9 +8,12 @@ } @section bottomScripts { - @ViewHelpers.SectionsScript(this); + @ViewHelpers.SectionsScript(this) @Scripts.Render("~/Scripts/gallery/page-account.min.js") } diff --git a/src/NuGetGallery/Views/Users/ForgotPassword.cshtml b/src/NuGetGallery/Views/Users/ForgotPassword.cshtml index a2eb5b8e90..69d84bfcc0 100644 --- a/src/NuGetGallery/Views/Users/ForgotPassword.cshtml +++ b/src/NuGetGallery/Views/Users/ForgotPassword.cshtml @@ -7,9 +7,13 @@ }
-
- @ViewHelpers.AlertPasswordDeprecation() -
+ @if (this.Config.Current.DeprecateNuGetPasswordLogins) + { +
+ @ViewHelpers.AlertPasswordDeprecation() +
+ } +

Forgot Password

diff --git a/src/NuGetGallery/Web.config b/src/NuGetGallery/Web.config index 51fa33188f..9560f184d6 100644 --- a/src/NuGetGallery/Web.config +++ b/src/NuGetGallery/Web.config @@ -41,6 +41,7 @@ + diff --git a/tests/NuGetGallery.Facts/Controllers/AuthenticationControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/AuthenticationControllerFacts.cs index 2c4c2542dd..89589c595d 100644 --- a/tests/NuGetGallery.Facts/Controllers/AuthenticationControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/AuthenticationControllerFacts.cs @@ -16,6 +16,7 @@ using NuGetGallery.Authentication.Providers.MicrosoftAccount; using NuGetGallery.Infrastructure.Authentication; using Xunit; +using System.Collections.Generic; namespace NuGetGallery.Controllers { @@ -98,6 +99,88 @@ public void WillNotRedirectToTheReturnUrlWhenReturnUrlContainsAccount() } } + public class TheSigninAssistanceAction : TestContainer + { + [Fact] + public void NullUsernameReturnsFalse() + { + var controller = GetController(); + + var result = controller.SignInAssistance(username: null, providedEmailAddress: null); + dynamic data = result.Data; + Assert.False(data.success); + } + + [Theory] + [InlineData("random@address.com", "r**********m@address.com")] + [InlineData("rm@address.com", "r**********m@address.com")] + [InlineData("r@address.com", "r**********@address.com")] + [InlineData("random.very.long.address@address.com", "r**********s@address.com")] + public void NullProvidedEmailReturnsFormattedEmail(string email, string expectedEmail) + { + var cred = new CredentialBuilder().CreateExternalCredential("MicrosoftAccount", "blorg", identity: "John Doe "); + var existingUser = new User("existingUser") { EmailAddress = email, Credentials = new[] { cred } }; + + GetMock(); // Force a mock to be created + GetMock() + .Setup(u => u.FindByUsername(It.IsAny())) + .Returns(existingUser); + + var controller = GetController(); + + var result = controller.SignInAssistance(username: "existingUser", providedEmailAddress: null); + dynamic data = result.Data; + Assert.True(data.success); + Assert.Equal(expectedEmail, data.EmailAddress); + } + + [Theory] + [InlineData("blarg")] + [InlineData("wrong@email")] + [InlineData("nonmatching@emailaddress.com")] + public void InvalidProvidedEmailReturnsFalse(string providedEmail) + { + var cred = new CredentialBuilder().CreateExternalCredential("MicrosoftAccount", "blorg", identity: "existing@example.com"); + var existingUser = new User("existingUser") { EmailAddress = "existing@example.com", Credentials = new[] { cred } }; + + GetMock(); // Force a mock to be created + GetMock() + .Setup(u => u.FindByUsername(It.IsAny())) + .Returns(existingUser); + + var controller = GetController(); + + var result = controller.SignInAssistance(username: "existingUser", providedEmailAddress: providedEmail); + dynamic data = result.Data; + Assert.False(data.success); + } + + [Fact] + public void SendsNotificationForAssistance() + { + var email = "existing@example.com"; + var fakes = Get(); + var cred = new CredentialBuilder().CreateExternalCredential("MicrosoftAccount", "blorg", identity: "existing@example.com"); + var existingUser = new User("existingUser") { EmailAddress = email, Credentials = new[] { cred } }; + + GetMock(); // Force a mock to be created + GetMock() + .Setup(u => u.FindByUsername(It.IsAny())) + .Returns(existingUser); + var messageServiceMock = GetMock(); + messageServiceMock + .Setup(m => m.SendSigninAssistanceEmail(It.IsAny(), It.IsAny>())) + .Verifiable(); + + var controller = GetController(); + + var result = controller.SignInAssistance(username: "existingUser", providedEmailAddress: email); + dynamic data = result.Data; + Assert.True(data.success); + messageServiceMock.Verify(); + } + } + public class TheSignInAction : TestContainer { [Fact] From d0e1ed919fe0adc0652798e61cedf0179691fd22 Mon Sep 17 00:00:00 2001 From: Scott Bommarito Date: Fri, 9 Mar 2018 14:32:50 -0800 Subject: [PATCH 22/24] [Organizations] Clone existing API keys to org admin on account transform (#5589) --- src/NuGetGallery/Services/IUserService.cs | 2 + src/NuGetGallery/Services/UserService.cs | 35 ++- .../Services/UserServiceFacts.cs | 202 +++++++++++++++++- 3 files changed, 230 insertions(+), 9 deletions(-) diff --git a/src/NuGetGallery/Services/IUserService.cs b/src/NuGetGallery/Services/IUserService.cs index 003202c3ff..260caab607 100644 --- a/src/NuGetGallery/Services/IUserService.cs +++ b/src/NuGetGallery/Services/IUserService.cs @@ -42,6 +42,8 @@ public interface IUserService Task TransformUserToOrganization(User accountToTransform, User adminUser, string token); + Task TransferApiKeysScopedToUser(User userWithKeys, User userToOwnKeys); + Task AddOrganizationAsync(string username, string emailAddress, User adminUser); } } \ No newline at end of file diff --git a/src/NuGetGallery/Services/UserService.cs b/src/NuGetGallery/Services/UserService.cs index 1cd0b5bdf4..20f6fa67a8 100644 --- a/src/NuGetGallery/Services/UserService.cs +++ b/src/NuGetGallery/Services/UserService.cs @@ -10,6 +10,7 @@ using System.Threading.Tasks; using NuGetGallery.Auditing; using NuGetGallery.Configuration; +using NuGetGallery.Infrastructure.Authentication; using NuGetGallery.Security; using Crypto = NuGetGallery.CryptographyService; @@ -46,7 +47,8 @@ public UserService( IEntitiesContext entitiesContext, IContentObjectService contentObjectService, ISecurityPolicyService securityPolicyService, - IDateTimeProvider dateTimeProvider) + IDateTimeProvider dateTimeProvider, + ICredentialBuilder credentialBuilder) : this() { Config = config; @@ -370,10 +372,41 @@ public async Task TransformUserToOrganization(User accountToTransform, Use { return false; } + + await TransferApiKeysScopedToUser(accountToTransform, adminUser); return await EntitiesContext.TransformUserToOrganization(accountToTransform, adminUser, token); } + public async Task TransferApiKeysScopedToUser(User userWithKeys, User userToOwnKeys) + { + var eligibleApiKeys = userWithKeys.Credentials + .Where(c => c.IsApiKey() && c.Scopes.All(k => k.Owner == null || k.Owner == userWithKeys)).ToArray(); + foreach (var originalApiKey in eligibleApiKeys) + { + var scopes = originalApiKey.Scopes.Select(s => + new Scope(userWithKeys, s.Subject, s.AllowedAction)); + + var clonedApiKey = new Credential(originalApiKey.Type, originalApiKey.Value) + { + Description = originalApiKey.Description, + ExpirationTicks = originalApiKey.ExpirationTicks, + Expires = originalApiKey.Expires, + Scopes = scopes.ToArray(), + User = userToOwnKeys, + UserKey = userToOwnKeys.Key, + Value = originalApiKey.Value + }; + + userToOwnKeys.Credentials.Add(clonedApiKey); + } + + if (eligibleApiKeys.Any()) + { + await EntitiesContext.SaveChangesAsync(); + } + } + public async Task AddOrganizationAsync(string username, string emailAddress, User adminUser) { if (!ContentObjectService.LoginDiscontinuationConfiguration.AreOrganizationsSupportedForUser(adminUser)) diff --git a/tests/NuGetGallery.Facts/Services/UserServiceFacts.cs b/tests/NuGetGallery.Facts/Services/UserServiceFacts.cs index 45093fdf48..441026488d 100644 --- a/tests/NuGetGallery.Facts/Services/UserServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/UserServiceFacts.cs @@ -2,11 +2,13 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Collections.Generic; using System.Globalization; using System.Linq; using System.Threading.Tasks; using Moq; using NuGetGallery.Auditing; +using NuGetGallery.Authentication; using NuGetGallery.Framework; using NuGetGallery.Infrastructure.Authentication; using NuGetGallery.Security; @@ -947,8 +949,192 @@ private Task InvokeTransformUserToOrganization(int affectedRecords, User a return service.TransformUserToOrganization(account, admin, "token"); } } + + public class TheTransferApiKeysScopedToUserMethod + { + public static IEnumerable TransfersApiKeysAsExpected_Data + { + get + { + foreach (var hasExternalCredential in new[] { false, true }) + { + foreach (var hasPasswordCredential in new[] { false, true }) + { + foreach (var hasUnscopedApiKeyCredential in new[] { false, true }) + { + foreach (var hasApiKeyScopedToUserCredential in new[] { false, true }) + { + foreach (var hasApiKeyScopedToDifferentUser in new[] { false, true }) + { + yield return MemberDataHelper.AsData( + hasExternalCredential, + hasPasswordCredential, + hasUnscopedApiKeyCredential, + hasApiKeyScopedToUserCredential, + hasApiKeyScopedToDifferentUser); + } + } + } + } + } + } + } + + [Theory] + [MemberData(nameof(TransfersApiKeysAsExpected_Data))] + public async Task TransfersApiKeysAsExpected( + bool hasExternalCredential, + bool hasPasswordCredential, + bool hasUnscopedApiKeyCredential, + bool hasApiKeyScopedToUserCredential, + bool hasApiKeyScopedToDifferentUser) + { + // Arrange + var originalOwner = new User("originalOwner") { Key = 11111 }; + var randomUser = new User("randomUser") { Key = 57576768 }; + var newOwner = new User("newOwner") { Key = 69785, Credentials = new List() }; + + var credentials = new List(); + + var externalCredential = TestCredentialHelper.CreateExternalCredential("cred", null); + AddFieldsToCredential(externalCredential, "externalCredential", "value1", originalOwner, expiration: null); + + var passwordCredential = TestCredentialHelper.CreateSha1Password("password"); + AddFieldsToCredential(passwordCredential, "passwordCredential", "value2", originalOwner, expiration: null); + + var unscopedApiKeyCredential = TestCredentialHelper.CreateV4ApiKey(new TimeSpan(5, 5, 5, 5), out var key1); + AddFieldsToCredential(unscopedApiKeyCredential, "unscopedApiKey", "value3", originalOwner, expiration: new DateTime(2018, 3, 9)); + + var scopedToUserApiKeyCredential = TestCredentialHelper.CreateV4ApiKey(new TimeSpan(5, 5, 5, 5), out var key2) + .WithScopes(new[] { new Scope { Owner = originalOwner, OwnerKey = originalOwner.Key } }); + AddFieldsToCredential(scopedToUserApiKeyCredential, "scopedToUserApiKey", "value4", originalOwner, expiration: new DateTime(2018, 3, 10)); + + var scopedToDifferentUserApiKeyCredential = TestCredentialHelper.CreateV4ApiKey(new TimeSpan(5, 5, 5, 5), out var key3) + .WithScopes(new[] { new Scope { Owner = randomUser, OwnerKey = randomUser.Key } }); + AddFieldsToCredential(scopedToDifferentUserApiKeyCredential, "scopedToDifferentUserApiKey", "value5", originalOwner, expiration: new DateTime(2018, 3, 11)); + + if (hasExternalCredential) + { + credentials.Add(externalCredential); + } + + if (hasPasswordCredential) + { + credentials.Add(passwordCredential); + } + + if (hasUnscopedApiKeyCredential) + { + credentials.Add(unscopedApiKeyCredential); + } + + if (hasApiKeyScopedToUserCredential) + { + credentials.Add(scopedToUserApiKeyCredential); + } + + if (hasApiKeyScopedToDifferentUser) + { + credentials.Add(scopedToDifferentUserApiKeyCredential); + } + + originalOwner.Credentials = credentials; + var originalCredentialCount = credentials.Count(); + + var service = new TestableUserService(); + + // Act + await service.TransferApiKeysScopedToUser(originalOwner, newOwner); + + // Assert + service.MockEntitiesContext.Verify( + x => x.SaveChangesAsync(), + hasUnscopedApiKeyCredential || hasApiKeyScopedToUserCredential ? Times.Once() : Times.Never()); + + Assert.Equal(originalCredentialCount, originalOwner.Credentials.Count()); + + Assert.Equal( + (hasUnscopedApiKeyCredential ? 1 : 0) + (hasApiKeyScopedToUserCredential ? 1 : 0), + newOwner.Credentials.Count()); + + AssertCredentialInOriginalOnly(externalCredential, originalOwner, newOwner, hasExternalCredential); + AssertCredentialInOriginalOnly(passwordCredential, originalOwner, newOwner, hasPasswordCredential); + AssertCredentialInOriginalOnly(scopedToDifferentUserApiKeyCredential, originalOwner, newOwner, hasApiKeyScopedToDifferentUser); + + AssertCredentialInNew(unscopedApiKeyCredential, originalOwner, newOwner, hasUnscopedApiKeyCredential); + AssertCredentialInNew(scopedToUserApiKeyCredential, originalOwner, newOwner, hasApiKeyScopedToUserCredential); + } + + private void AddFieldsToCredential(Credential credential, string description, string value, User originalOwner, DateTime? expiration) + { + credential.Description = description; + credential.Value = value; + credential.User = originalOwner; + credential.UserKey = originalOwner.Key; + + if (expiration.HasValue) + { + credential.ExpirationTicks = expiration.Value.Ticks; + credential.Expires = expiration.Value; + } + } + + private void AssertCredentialInOriginalOnly(Credential credential, User originalOwner, User newOwner, bool hasCredential) + { + var credentialEquals = CredentialEqualsFunc(credential); + Assert.Equal(hasCredential, originalOwner.Credentials.Any( + hasCredential ? CredentialEqualsWithOwnerFunc(credential, originalOwner) : CredentialEqualsFunc(credential))); + Assert.False(newOwner.Credentials.Any(CredentialEqualsFunc(credential))); + } + + private void AssertCredentialInNew(Credential credential, User originalOwner, User newOwner, bool hasCredential) + { + Assert.Equal(hasCredential, originalOwner.Credentials.Any( + hasCredential ? CredentialEqualsWithOwnerFunc(credential, originalOwner) : CredentialEqualsFunc(credential))); + Assert.Equal(hasCredential, newOwner.Credentials.Any( + hasCredential ? CredentialEqualsWithOwnerAndScopeFunc(credential, newOwner, originalOwner) : CredentialEqualsFunc(credential))); + } + + private bool CredentialEquals(Credential expected, Credential actual) + { + return + expected.Description == actual.Description && + expected.ExpirationTicks == actual.ExpirationTicks && + expected.Expires == actual.Expires && + expected.Type == actual.Type && + expected.Value == actual.Value; + } + + private bool CredentialEqualsWithOwner(Credential expected, Credential actual, User owner) + { + return CredentialEquals(expected, actual) && + owner == actual.User && + owner.Key == actual.UserKey; + } + + private bool CredentialEqualsWithOwnerAndScope(Credential expected, Credential actual, User owner, User scopeOwner) + { + return CredentialEqualsWithOwner(expected, actual, owner) && + expected.Scopes.All(s => s.Owner == scopeOwner && s.OwnerKey == scopeOwner.Key); + } + + private Func CredentialEqualsFunc(Credential expected) + { + return (c) => CredentialEquals(expected, c); + } + + private Func CredentialEqualsWithOwnerFunc(Credential expected, User owner) + { + return (c) => CredentialEqualsWithOwner(expected, c, owner); + } + + private Func CredentialEqualsWithOwnerAndScopeFunc(Credential expected, User owner, User scopeOwner) + { + return (c) => CredentialEqualsWithOwnerAndScope(expected, c, owner, scopeOwner); + } + } - public class TheCreateOrganizationAccountMethod + public class TheAddOrganizationAccountMethod { private const string OrgName = "myOrg"; private const string OrgEmail = "myOrg@myOrg.com"; @@ -962,7 +1148,7 @@ public class TheCreateOrganizationAccountMethod public async Task WithUserNotSupportedForOrganizations_ThrowsEntityException() { SetupOrganizationsSupportedForUser(supported: false); - var exception = await Assert.ThrowsAsync(() => InvokeCreateOrganization()); + var exception = await Assert.ThrowsAsync(() => InvokeAddOrganization()); Assert.Equal(String.Format(CultureInfo.CurrentCulture, Strings.Organizations_NotInDomainWhitelist, AdminName), exception.Message); _service.MockOrganizationRepository.Verify(x => x.InsertOnCommit(It.IsAny()), Times.Never()); @@ -981,7 +1167,7 @@ public async Task WithUsernameConflict_ThrowsEntityException() SetupOrganizationsSupportedForUser(); - var exception = await Assert.ThrowsAsync(() => InvokeCreateOrganization(orgName: conflictUsername)); + var exception = await Assert.ThrowsAsync(() => InvokeAddOrganization(orgName: conflictUsername)); Assert.Equal(String.Format(CultureInfo.CurrentCulture, Strings.UsernameNotAvailable, conflictUsername), exception.Message); _service.MockOrganizationRepository.Verify(x => x.InsertOnCommit(It.IsAny()), Times.Never()); @@ -1000,7 +1186,7 @@ public async Task WithEmailConflict_ThrowsEntityException() SetupOrganizationsSupportedForUser(); - var exception = await Assert.ThrowsAsync(() => InvokeCreateOrganization(orgEmail: conflictEmail)); + var exception = await Assert.ThrowsAsync(() => InvokeAddOrganization(orgEmail: conflictEmail)); Assert.Equal(String.Format(CultureInfo.CurrentCulture, Strings.EmailAddressBeingUsed, conflictEmail), exception.Message); _service.MockOrganizationRepository.Verify(x => x.InsertOnCommit(It.IsAny()), Times.Never()); @@ -1017,7 +1203,7 @@ public async Task WhenAdminHasNoTenant_ThrowsEntityException() var adminUsername = "adminWithNoTenant"; SetupOrganizationsSupportedForUser(adminUsername); - var exception = await Assert.ThrowsAsync(() => InvokeCreateOrganization(admin: new User(adminUsername))); + var exception = await Assert.ThrowsAsync(() => InvokeAddOrganization(admin: new User(adminUsername))); Assert.Equal(String.Format(CultureInfo.CurrentCulture, Strings.Organizations_AdminAccountDoesNotHaveTenant, adminUsername), exception.Message); _service.MockOrganizationRepository.Verify(x => x.InsertOnCommit(It.IsAny()), Times.Once()); @@ -1037,7 +1223,7 @@ public async Task WhenSubscribingToPolicyFails_ThrowsUserSafeException() .Returns(Task.FromResult(false)); SetupOrganizationsSupportedForUser(); - var exception = await Assert.ThrowsAsync(() => InvokeCreateOrganization()); + var exception = await Assert.ThrowsAsync(() => InvokeAddOrganization()); Assert.Equal(Strings.DefaultUserSafeExceptionMessage, exception.Message); _service.MockOrganizationRepository.Verify(x => x.InsertOnCommit(It.IsAny()), Times.Once()); @@ -1057,7 +1243,7 @@ public async Task WhenSubscribingToPolicySucceeds_ReturnsNewOrg() .Returns(Task.FromResult(true)); SetupOrganizationsSupportedForUser(); - var org = await InvokeCreateOrganization(); + var org = await InvokeAddOrganization(); Assert.Equal(OrgName, org.Username); Assert.Equal(OrgEmail, org.UnconfirmedEmailAddress); @@ -1077,7 +1263,7 @@ public async Task WhenSubscribingToPolicySucceeds_ReturnsNewOrg() _service.MockEntitiesContext.Verify(x => x.SaveChangesAsync(), Times.Once()); } - private Task InvokeCreateOrganization(string orgName = OrgName, string orgEmail = OrgEmail, User admin = null) + private Task InvokeAddOrganization(string orgName = OrgName, string orgEmail = OrgEmail, User admin = null) { // Arrange admin = admin ?? new User(AdminName) From f842ae7a3092fd45629d5e31986204cc42810df8 Mon Sep 17 00:00:00 2001 From: Scott Bommarito Date: Fri, 9 Mar 2018 14:52:15 -0800 Subject: [PATCH 23/24] [Organizations] Use a configurable list to determine what users should be asked to transform to an organization with a deprecated password (#5588) --- src/Bootstrap/dist/css/bootstrap-theme.css | 4 +- src/Bootstrap/less/theme/modals.less | 4 +- .../Login-Discontinuation-Configuration.json | 3 + .../Controllers/PagesController.cs | 9 +- .../Extensions/ClaimsExtensions.cs | 2 +- .../Filters/UiAuthorizeAttribute.cs | 2 +- .../Services/ContentObjectService.cs | 4 +- .../LoginDiscontinuationConfiguration.cs | 38 ++++++- .../ViewModels/GalleryHomeViewModel.cs | 7 +- src/NuGetGallery/Views/Pages/Home.cshtml | 2 +- .../Views/Pages/_TransformOrLink.cshtml | 47 ++++---- .../Services/ContentObjectServiceFacts.cs | 3 +- .../LoginDiscontinuationConfigurationFacts.cs | 100 ++++++++++++++---- 13 files changed, 168 insertions(+), 57 deletions(-) diff --git a/src/Bootstrap/dist/css/bootstrap-theme.css b/src/Bootstrap/dist/css/bootstrap-theme.css index 9597ccb682..f7fd247cfb 100644 --- a/src/Bootstrap/dist/css/bootstrap-theme.css +++ b/src/Bootstrap/dist/css/bootstrap-theme.css @@ -359,7 +359,7 @@ img.reserved-indicator-icon { vertical-align: top; } .modal-container .modal-box .modal-body .action-node { - padding: 9% 6%; + padding: 0 6%; text-align: center; } .modal-container .modal-box .modal-body .action-button { @@ -392,7 +392,7 @@ img.reserved-indicator-icon { padding-top: 13px; } .modal-container .modal-box .modal-body .transform-text { - padding: 0 15px; + padding: 40px 15px; margin: 0; } #edit-metadata-form-container .loading:after { diff --git a/src/Bootstrap/less/theme/modals.less b/src/Bootstrap/less/theme/modals.less index d983d0fc15..dd3e040ae8 100644 --- a/src/Bootstrap/less/theme/modals.less +++ b/src/Bootstrap/less/theme/modals.less @@ -51,7 +51,7 @@ } .action-node { - padding: 9% 6%; + padding: 0% 6%; text-align: center; } @@ -93,7 +93,7 @@ } .transform-text { - padding: 0px 15px; + padding: 40px 15px; margin: 0px; } } diff --git a/src/NuGetGallery/App_Data/Files/Content/Login-Discontinuation-Configuration.json b/src/NuGetGallery/App_Data/Files/Content/Login-Discontinuation-Configuration.json index 770147da56..bc35f1c4cc 100644 --- a/src/NuGetGallery/App_Data/Files/Content/Login-Discontinuation-Configuration.json +++ b/src/NuGetGallery/App_Data/Files/Content/Login-Discontinuation-Configuration.json @@ -7,5 +7,8 @@ ], "ExceptionsForEmailAddresses": [ "exception@cannotUsePassword.com" + ], + "ForceTransformationToOrganizationForEmailAddresses": [ + "organization@cannotUsePassword.com" ] } \ No newline at end of file diff --git a/src/NuGetGallery/Controllers/PagesController.cs b/src/NuGetGallery/Controllers/PagesController.cs index 41e1943704..0d82cf3cda 100644 --- a/src/NuGetGallery/Controllers/PagesController.cs +++ b/src/NuGetGallery/Controllers/PagesController.cs @@ -19,15 +19,18 @@ public partial class PagesController : AppController { private readonly IContentService _contentService; + private readonly IContentObjectService _contentObjectService; private readonly IMessageService _messageService; private readonly ISupportRequestService _supportRequestService; protected PagesController() { } public PagesController(IContentService contentService, + IContentObjectService contentObjectService, IMessageService messageService, ISupportRequestService supportRequestService) { _contentService = contentService; + _contentObjectService = contentObjectService; _messageService = messageService; _supportRequestService = supportRequestService; } @@ -102,8 +105,10 @@ public virtual async Task Contact(ContactSupportViewModel contactF public virtual ActionResult Home() { var identity = OwinContext.Authentication?.User?.Identity as ClaimsIdentity; - var showTransformModal = ClaimsExtensions.HasDiscontinuedLoginCLaims(identity); - return View(new GalleryHomeViewModel(showTransformModal)); + var showTransformModal = ClaimsExtensions.HasDiscontinuedLoginClaims(identity); + var transformIntoOrganization = _contentObjectService.LoginDiscontinuationConfiguration + .ShouldUserTransformIntoOrganization(GetCurrentUser()); + return View(new GalleryHomeViewModel(showTransformModal, transformIntoOrganization)); } [HttpGet] diff --git a/src/NuGetGallery/Extensions/ClaimsExtensions.cs b/src/NuGetGallery/Extensions/ClaimsExtensions.cs index 8a1feb7293..9e23c48a20 100644 --- a/src/NuGetGallery/Extensions/ClaimsExtensions.cs +++ b/src/NuGetGallery/Extensions/ClaimsExtensions.cs @@ -33,7 +33,7 @@ public static IdentityInformation GetIdentityInformation(ClaimsIdentity claimsId return new IdentityInformation(identifierClaim.Value, nameClaim.Value, emailClaim?.Value, authType, tenantId: null); } - public static bool HasDiscontinuedLoginCLaims(ClaimsIdentity identity) + public static bool HasDiscontinuedLoginClaims(ClaimsIdentity identity) { if (identity == null || !identity.IsAuthenticated) { diff --git a/src/NuGetGallery/Filters/UiAuthorizeAttribute.cs b/src/NuGetGallery/Filters/UiAuthorizeAttribute.cs index 84a08814f4..e76be29c18 100644 --- a/src/NuGetGallery/Filters/UiAuthorizeAttribute.cs +++ b/src/NuGetGallery/Filters/UiAuthorizeAttribute.cs @@ -21,7 +21,7 @@ public override void OnAuthorization(AuthorizationContext filterContext) { // If the user has a discontinued login claim, redirect them to the homepage var identity = filterContext.HttpContext.User.Identity as ClaimsIdentity; - if (!AllowDiscontinuedLogins && ClaimsExtensions.HasDiscontinuedLoginCLaims(identity)) + if (!AllowDiscontinuedLogins && ClaimsExtensions.HasDiscontinuedLoginClaims(identity)) { filterContext.Result = new RedirectToRouteResult( new RouteValueDictionary( diff --git a/src/NuGetGallery/Services/ContentObjectService.cs b/src/NuGetGallery/Services/ContentObjectService.cs index 6fb5605098..5e60bf29d6 100644 --- a/src/NuGetGallery/Services/ContentObjectService.cs +++ b/src/NuGetGallery/Services/ContentObjectService.cs @@ -18,9 +18,7 @@ public ContentObjectService(IContentService contentService) { _contentService = contentService; - LoginDiscontinuationConfiguration = - new LoginDiscontinuationConfiguration( - Enumerable.Empty(), Enumerable.Empty(), Enumerable.Empty()); + LoginDiscontinuationConfiguration = new LoginDiscontinuationConfiguration(); } public ILoginDiscontinuationConfiguration LoginDiscontinuationConfiguration { get; set; } diff --git a/src/NuGetGallery/Services/LoginDiscontinuationConfiguration.cs b/src/NuGetGallery/Services/LoginDiscontinuationConfiguration.cs index e5c4a8bd23..6b373651dd 100644 --- a/src/NuGetGallery/Services/LoginDiscontinuationConfiguration.cs +++ b/src/NuGetGallery/Services/LoginDiscontinuationConfiguration.cs @@ -11,12 +11,16 @@ namespace NuGetGallery { public class LoginDiscontinuationConfiguration : ILoginDiscontinuationConfiguration { - public HashSet DiscontinuedForEmailAddresses { get; } - public HashSet DiscontinuedForDomains { get; } - public HashSet ExceptionsForEmailAddresses { get; } + internal HashSet DiscontinuedForEmailAddresses { get; } + internal HashSet DiscontinuedForDomains { get; } + internal HashSet ExceptionsForEmailAddresses { get; } + internal HashSet ForceTransformationToOrganizationForEmailAddresses { get; } public LoginDiscontinuationConfiguration() - : this(Enumerable.Empty(), Enumerable.Empty(), Enumerable.Empty()) + : this(Enumerable.Empty(), + Enumerable.Empty(), + Enumerable.Empty(), + Enumerable.Empty()) { } @@ -24,15 +28,22 @@ public LoginDiscontinuationConfiguration() public LoginDiscontinuationConfiguration( IEnumerable discontinuedForEmailAddresses, IEnumerable discontinuedForDomains, - IEnumerable exceptionsForEmailAddresses) + IEnumerable exceptionsForEmailAddresses, + IEnumerable forceTransformationToOrganizationForEmailAddresses) { DiscontinuedForEmailAddresses = new HashSet(discontinuedForEmailAddresses); DiscontinuedForDomains = new HashSet(discontinuedForDomains); ExceptionsForEmailAddresses = new HashSet(exceptionsForEmailAddresses); + ForceTransformationToOrganizationForEmailAddresses = new HashSet(forceTransformationToOrganizationForEmailAddresses); } public bool IsLoginDiscontinued(AuthenticatedUser authUser) { + if (authUser == null || authUser.User == null) + { + return false; + } + var email = authUser.User.ToMailAddress(); return authUser.CredentialUsed.IsPassword() && @@ -42,16 +53,33 @@ public bool IsLoginDiscontinued(AuthenticatedUser authUser) public bool AreOrganizationsSupportedForUser(User user) { + if (user == null) + { + return false; + } + var email = user.ToMailAddress(); return DiscontinuedForDomains.Contains(email.Host, StringComparer.OrdinalIgnoreCase) || DiscontinuedForEmailAddresses.Contains(email.Address); } + + public bool ShouldUserTransformIntoOrganization(User user) + { + if (user == null) + { + return false; + } + + var email = user.ToMailAddress(); + return ForceTransformationToOrganizationForEmailAddresses.Contains(email.Address); + } } public interface ILoginDiscontinuationConfiguration { bool IsLoginDiscontinued(AuthenticatedUser authUser); bool AreOrganizationsSupportedForUser(User user); + bool ShouldUserTransformIntoOrganization(User user); } } \ No newline at end of file diff --git a/src/NuGetGallery/ViewModels/GalleryHomeViewModel.cs b/src/NuGetGallery/ViewModels/GalleryHomeViewModel.cs index 68909058fd..20ca8afbca 100644 --- a/src/NuGetGallery/ViewModels/GalleryHomeViewModel.cs +++ b/src/NuGetGallery/ViewModels/GalleryHomeViewModel.cs @@ -9,13 +9,16 @@ public class GalleryHomeViewModel { public bool ShowTransformModal { get; set; } + public bool TransformIntoOrganization { get; set; } + public IList Providers { get; set; } - public GalleryHomeViewModel() : this(showTransformModal: false) { } + public GalleryHomeViewModel() : this(showTransformModal: false, transformIntoOrganization: false) { } - public GalleryHomeViewModel(bool showTransformModal) + public GalleryHomeViewModel(bool showTransformModal, bool transformIntoOrganization) { ShowTransformModal = showTransformModal; + TransformIntoOrganization = transformIntoOrganization; } } } \ No newline at end of file diff --git a/src/NuGetGallery/Views/Pages/Home.cshtml b/src/NuGetGallery/Views/Pages/Home.cshtml index 6ffb82ce1c..63b25c692a 100644 --- a/src/NuGetGallery/Views/Pages/Home.cshtml +++ b/src/NuGetGallery/Views/Pages/Home.cshtml @@ -13,7 +13,7 @@ window.showModal = true; } diff --git a/src/NuGetGallery/Views/Pages/_TransformOrLink.cshtml b/src/NuGetGallery/Views/Pages/_TransformOrLink.cshtml index d629cacb1f..44c1a58c6e 100644 --- a/src/NuGetGallery/Views/Pages/_TransformOrLink.cshtml +++ b/src/NuGetGallery/Views/Pages/_TransformOrLink.cshtml @@ -1,4 +1,6 @@ -
+@model GalleryHomeViewModel + +
Sign-in with Microsoft corporate account
-
- - - Sign in with Microsoft - -
-

If this is a team (DL) based account, you must transform it to an organization on NuGet.org:

-
- - Transform to an Organization - -
+ @if (Model.TransformIntoOrganization) + { +

We have determined this is a team (DL) based account, so you must transform it to an organization on NuGet.org:

+
+ + Transform to an Organization + +
+ } + else + { +

We have determined this is an account owned by an individual, so you must link it to a Microsoft account:

+
+ + + Sign in with Microsoft + +
+ }
diff --git a/tests/NuGetGallery.Facts/Services/ContentObjectServiceFacts.cs b/tests/NuGetGallery.Facts/Services/ContentObjectServiceFacts.cs index c9a79b5c4e..d50e3b6986 100644 --- a/tests/NuGetGallery.Facts/Services/ContentObjectServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/ContentObjectServiceFacts.cs @@ -32,8 +32,9 @@ public async Task RefreshRefreshesObject() var emails = new[] { "discontinued@different.com" }; var domains = new[] { "example.com" }; var exceptions = new[] { "exception@example.com" }; + var shouldTransforms = new[] { "transfomer@example.com" }; - var config = new LoginDiscontinuationConfiguration(emails, domains, exceptions); + var config = new LoginDiscontinuationConfiguration(emails, domains, exceptions, shouldTransforms); var configString = JsonConvert.SerializeObject(config); GetMock() diff --git a/tests/NuGetGallery.Facts/Services/LoginDiscontinuationConfigurationFacts.cs b/tests/NuGetGallery.Facts/Services/LoginDiscontinuationConfigurationFacts.cs index 7010264f90..c90882cdfc 100644 --- a/tests/NuGetGallery.Facts/Services/LoginDiscontinuationConfigurationFacts.cs +++ b/tests/NuGetGallery.Facts/Services/LoginDiscontinuationConfigurationFacts.cs @@ -10,14 +10,44 @@ namespace NuGetGallery.Services { public class LoginDiscontinuationConfigurationFacts { - public class TheIsLoginDiscontinuedMethod : TestContainer + private const string _incorrectEmail = "incorrect@notExample.com"; + private const string _incorrectDomain = "notExample.com"; + private const string _domain = "example.com"; + private const string _incorrectException = "fake@notExample.com"; + private const string _email = "test@example.com"; + + public static IEnumerable PossibleListStates + { + get + { + foreach (var isOnWhiteList in new[] { true, false }) + { + foreach (var isOnDomainList in new[] { true, false }) + { + foreach (var isOnExceptionList in new[] { true, false }) + { + foreach (var isOnTransformList in new[] { true, false }) + { + yield return MemberDataHelper.AsData(isOnWhiteList, isOnDomainList, isOnExceptionList, isOnTransformList); + } + } + } + } + } + } + + public static ILoginDiscontinuationConfiguration CreateConfiguration(bool isOnWhiteList, bool isOnDomainList, bool isOnExceptionList, bool isOnTransformList) { - private const string _incorrectEmail = "incorrect@notExample.com"; - private const string _incorrectDomain = "notExample.com"; - private const string _domain = "example.com"; - private const string _incorrectException = "fake@notExample.com"; - private const string _email = "test@example.com"; + var emails = isOnWhiteList ? new[] { _email } : new[] { _incorrectEmail }; + var domains = isOnDomainList ? new[] { _domain } : new[] { _incorrectDomain }; + var exceptions = isOnExceptionList ? new[] { _email } : new[] { _incorrectException }; + var shouldTransforms = isOnTransformList ? new[] { _email } : new[] { _incorrectException }; + return new LoginDiscontinuationConfiguration(emails, domains, exceptions, shouldTransforms); + } + + public class TheIsLoginDiscontinuedMethod + { public static IEnumerable IfPasswordLoginReturnsTrueIfOnWhitelists_Data { get @@ -33,7 +63,10 @@ public static IEnumerable IfPasswordLoginReturnsTrueIfOnWhitelists_Dat { foreach (var isOnExceptionList in new[] { true, false }) { - yield return MemberDataHelper.AsData(credentialPasswordType, isOnWhiteList, isOnDomainList, isOnExceptionList); + foreach (var isOnTransformList in new[] { true, false }) + { + yield return MemberDataHelper.AsData(credentialPasswordType, isOnWhiteList, isOnDomainList, isOnExceptionList, isOnTransformList); + } } } } @@ -43,9 +76,9 @@ public static IEnumerable IfPasswordLoginReturnsTrueIfOnWhitelists_Dat [Theory] [MemberData(nameof(IfPasswordLoginReturnsTrueIfOnWhitelists_Data))] - public void IfPasswordLoginReturnsTrueIfOnWhitelists(string credentialPasswordType, bool isOnWhiteList, bool isOnDomainList, bool isOnExceptionList) + public void IfPasswordLoginReturnsTrueIfOnWhitelists(string credentialPasswordType, bool isOnWhiteList, bool isOnDomainList, bool isOnExceptionList, bool isOnTransformList) { - TestIsLoginDiscontinued(credentialPasswordType, isOnWhiteList, isOnDomainList, isOnExceptionList, + TestIsLoginDiscontinued(credentialPasswordType, isOnWhiteList, isOnDomainList, isOnExceptionList, isOnTransformList, expectedResult: (isOnWhiteList || isOnDomainList) && !isOnExceptionList); } @@ -68,7 +101,10 @@ public static IEnumerable IfNotPasswordLoginReturnsFalse_Data { foreach (var isOnExceptionList in new[] { true, false }) { - yield return MemberDataHelper.AsData(credentialType, isOnWhiteList, isOnDomainList, isOnExceptionList); + foreach (var isOnTransformList in new[] { true, false }) + { + yield return MemberDataHelper.AsData(credentialType, isOnWhiteList, isOnDomainList, isOnExceptionList, isOnTransformList); + } } } } @@ -78,23 +114,19 @@ public static IEnumerable IfNotPasswordLoginReturnsFalse_Data [Theory] [MemberData(nameof(IfNotPasswordLoginReturnsFalse_Data))] - public void IfNotPasswordLoginReturnsFalse(string credentialType, bool isOnWhiteList, bool isOnDomainList, bool isOnExceptionList) + public void IfNotPasswordLoginReturnsFalse(string credentialType, bool isOnWhiteList, bool isOnDomainList, bool isOnExceptionList, bool isOnTransformList) { - TestIsLoginDiscontinued(credentialType, isOnWhiteList, isOnDomainList, isOnExceptionList, expectedResult: false); + TestIsLoginDiscontinued(credentialType, isOnWhiteList, isOnDomainList, isOnExceptionList, isOnTransformList, expectedResult: false); } - private void TestIsLoginDiscontinued(string credentialType, bool isOnWhiteList, bool isOnDomainList, bool isOnExceptionList, bool expectedResult) + private void TestIsLoginDiscontinued(string credentialType, bool isOnWhiteList, bool isOnDomainList, bool isOnExceptionList, bool isOnTransformList, bool expectedResult) { // Arrange var credential = new Credential(credentialType, "value"); var user = new User("test") { EmailAddress = _email, Credentials = new[] { credential } }; var authUser = new AuthenticatedUser(user, credential); - var emails = isOnWhiteList ? new[] { _email } : new[] { _incorrectEmail }; - var domains = isOnDomainList ? new[] { _domain } : new[] { _incorrectDomain }; - var exceptions = isOnExceptionList ? new[] { _email } : new[] { _incorrectException }; - - var config = new LoginDiscontinuationConfiguration(emails, domains, exceptions); + var config = CreateConfiguration(isOnWhiteList, isOnDomainList, isOnExceptionList, isOnTransformList); // Act var result = config.IsLoginDiscontinued(authUser); @@ -103,5 +135,37 @@ private void TestIsLoginDiscontinued(string credentialType, bool isOnWhiteList, Assert.Equal(expectedResult, result); } } + + public class TheSupportedForUserMethods + { + public static IEnumerable PossibleListStates => PossibleListStates; + + public void IsSupportedAsExpected(bool isOnWhiteList, bool isOnDomainList, bool isOnExceptionList, bool isOnTransformList) + { + // Arrange + var user = new User("test") { EmailAddress = _email }; + + var config = CreateConfiguration(isOnWhiteList, isOnDomainList, isOnExceptionList, isOnTransformList); + + // Act + var areOrganizationsSupported = config.AreOrganizationsSupportedForUser(user); + var shouldTransform = config.ShouldUserTransformIntoOrganization(user); + + // Assert + Assert.Equal(isOnWhiteList || isOnDomainList, areOrganizationsSupported); + Assert.Equal(isOnTransformList, shouldTransform); + } + + public void IsUnsupportedWhenNull() + { + var config = new LoginDiscontinuationConfiguration(); + + var areOrganizationsSupported = config.AreOrganizationsSupportedForUser(null); + var shouldTransform = config.ShouldUserTransformIntoOrganization(null); + + Assert.False(areOrganizationsSupported); + Assert.False(shouldTransform); + } + } } } From 6f24e48f4c1906ed8a382f02150f172003fafd21 Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 9 Mar 2018 18:06:34 -0800 Subject: [PATCH 24/24] Add destination access condition parameter to CopyFileAsync (#5594) Progress on https://github.com/NuGet/Engineering/issues/1190 --- .../NuGetGallery.Core.csproj | 2 + .../Services/AccessConditionWrapper.cs | 34 ++++++ .../CloudBlobCoreFileStorageService.cs | 31 ++++-- .../Services/IAccessCondition.cs | 14 +++ .../Services/ICoreFileStorageService.cs | 14 ++- .../Services/FileSystemFileStorageService.cs | 9 +- .../CloudBlobCoreFileStorageServiceFacts.cs | 104 +++++++++++++++++- .../FileSystemFileStorageServiceFacts.cs | 21 +++- 8 files changed, 211 insertions(+), 18 deletions(-) create mode 100644 src/NuGetGallery.Core/Services/AccessConditionWrapper.cs create mode 100644 src/NuGetGallery.Core/Services/IAccessCondition.cs diff --git a/src/NuGetGallery.Core/NuGetGallery.Core.csproj b/src/NuGetGallery.Core/NuGetGallery.Core.csproj index 0964d1bfde..ef2441500a 100644 --- a/src/NuGetGallery.Core/NuGetGallery.Core.csproj +++ b/src/NuGetGallery.Core/NuGetGallery.Core.csproj @@ -232,6 +232,7 @@ + @@ -241,6 +242,7 @@ + diff --git a/src/NuGetGallery.Core/Services/AccessConditionWrapper.cs b/src/NuGetGallery.Core/Services/AccessConditionWrapper.cs new file mode 100644 index 0000000000..8f7d08fc59 --- /dev/null +++ b/src/NuGetGallery.Core/Services/AccessConditionWrapper.cs @@ -0,0 +1,34 @@ +// 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 Microsoft.WindowsAzure.Storage; + +namespace NuGetGallery +{ + public class AccessConditionWrapper : IAccessCondition + { + private AccessConditionWrapper(string ifNoneMatchETag, string ifMatchETag) + { + IfNoneMatchETag = ifNoneMatchETag; + IfMatchETag = IfMatchETag; + } + + public string IfNoneMatchETag { get; } + + public string IfMatchETag { get; } + + public static IAccessCondition GenerateIfMatchCondition(string etag) + { + return new AccessConditionWrapper( + ifNoneMatchETag: null, + ifMatchETag: AccessCondition.GenerateIfMatchCondition(etag).IfMatchETag); + } + + public static IAccessCondition GenerateIfNotExistsCondition() + { + return new AccessConditionWrapper( + ifNoneMatchETag: AccessCondition.GenerateIfNotExistsCondition().IfNoneMatchETag, + ifMatchETag: null); + } + } +} diff --git a/src/NuGetGallery.Core/Services/CloudBlobCoreFileStorageService.cs b/src/NuGetGallery.Core/Services/CloudBlobCoreFileStorageService.cs index b6bc3e2d04..13422c1d94 100644 --- a/src/NuGetGallery.Core/Services/CloudBlobCoreFileStorageService.cs +++ b/src/NuGetGallery.Core/Services/CloudBlobCoreFileStorageService.cs @@ -110,7 +110,12 @@ public async Task GetFileReferenceAsync(string folderName, strin } } - public async Task CopyFileAsync(string srcFolderName, string srcFileName, string destFolderName, string destFileName) + public async Task CopyFileAsync( + string srcFolderName, + string srcFileName, + string destFolderName, + string destFileName, + IAccessCondition destAccessCondition) { if (srcFolderName == null) { @@ -137,36 +142,46 @@ public async Task CopyFileAsync(string srcFolderName, string srcFileName, string var destContainer = await GetContainerAsync(destFolderName); var destBlob = destContainer.GetBlobReference(destFileName); + destAccessCondition = destAccessCondition ?? AccessConditionWrapper.GenerateIfNotExistsCondition(); + var mappedDestAccessCondition = new AccessCondition + { + IfNoneMatchETag = destAccessCondition.IfNoneMatchETag, + IfMatchETag = destAccessCondition.IfMatchETag, + }; // Determine the source blob etag. await srcBlob.FetchAttributesAsync(); var srcAccessCondition = AccessCondition.GenerateIfMatchCondition(srcBlob.ETag); // Check if the destination blob already exists and fetch attributes. - var destAccessCondition = AccessCondition.GenerateIfNotExistsCondition(); if (await destBlob.ExistsAsync()) { if (destBlob.CopyState?.Status == CopyStatus.Failed) { - // If the last copy failed, allow this copy to occur. - destAccessCondition = AccessCondition.GenerateIfMatchCondition(destBlob.ETag); + // If the last copy failed, allow this copy to occur no matter what the caller's destination + // condition is. This is because the source blob is preferable over a failed copy. We use the etag + // of the failed blob to avoid inadvertently replacing a blob that is now valid (i.e. has a + // successful copy status). + mappedDestAccessCondition = AccessCondition.GenerateIfMatchCondition(destBlob.ETag); } else if ((srcBlob.Properties.ContentMD5 != null && srcBlob.Properties.ContentMD5 == destBlob.Properties.ContentMD5 && srcBlob.Properties.Length == destBlob.Properties.Length)) { // If the blob hash is the same and the length is the same, no-op the copy. - return; + return srcBlob.ETag; } } - // Start the server-side copy and wait for it to complete. + // Start the server-side copy and wait for it to complete. If "If-None-Match: *" was specified and the + // destination already exists, HTTP 409 is thrown. If "If-Match: ETAG" was specified and the destination + // has changed, HTTP 412 is thrown. try { await destBlob.StartCopyAsync( srcBlob, srcAccessCondition, - destAccessCondition); + mappedDestAccessCondition); } catch (StorageException ex) when (ex.RequestInformation?.HttpStatusCode == (int?)HttpStatusCode.Conflict) { @@ -195,6 +210,8 @@ await destBlob.StartCopyAsync( { throw new StorageException($"The blob copy operation had copy status {destBlob.CopyState.Status} ({destBlob.CopyState.StatusDescription})."); } + + return srcBlob.ETag; } public async Task SaveFileAsync(string folderName, string fileName, Stream packageFile, bool overwrite = true) diff --git a/src/NuGetGallery.Core/Services/IAccessCondition.cs b/src/NuGetGallery.Core/Services/IAccessCondition.cs new file mode 100644 index 0000000000..545751a41b --- /dev/null +++ b/src/NuGetGallery.Core/Services/IAccessCondition.cs @@ -0,0 +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. + +namespace NuGetGallery +{ + /// + /// A wrapper type around . + /// + public interface IAccessCondition + { + string IfNoneMatchETag { get; } + string IfMatchETag { get; } + } +} diff --git a/src/NuGetGallery.Core/Services/ICoreFileStorageService.cs b/src/NuGetGallery.Core/Services/ICoreFileStorageService.cs index ee4670bb47..cc47d53066 100644 --- a/src/NuGetGallery.Core/Services/ICoreFileStorageService.cs +++ b/src/NuGetGallery.Core/Services/ICoreFileStorageService.cs @@ -46,6 +46,18 @@ public interface ICoreFileStorageService /// The source file name or relative file path. /// The destination folder. /// The destination file name or relative file path. - Task CopyFileAsync(string srcFolderName, string srcFileName, string destFolderName, string destFileName); + /// + /// The access condition used to determine whether the destination is in the expected state. + /// + /// + /// The etag of the source file. This can be used if the destination file is later intended to replace + /// the source file in conjunction with . + /// + Task CopyFileAsync( + string srcFolderName, + string srcFileName, + string destFolderName, + string destFileName, + IAccessCondition destAccessCondition); } } diff --git a/src/NuGetGallery/Services/FileSystemFileStorageService.cs b/src/NuGetGallery/Services/FileSystemFileStorageService.cs index c2542b61b1..51f68f1881 100644 --- a/src/NuGetGallery/Services/FileSystemFileStorageService.cs +++ b/src/NuGetGallery/Services/FileSystemFileStorageService.cs @@ -169,7 +169,12 @@ public Task SaveFileAsync(string folderName, string fileName, Stream packageFile return Task.FromResult(0); } - public Task CopyFileAsync(string srcFolderName, string srcFileName, string destFolderName, string destFileName) + public Task CopyFileAsync( + string srcFolderName, + string srcFileName, + string destFolderName, + string destFileName, + IAccessCondition destAccessCondition) { if (srcFolderName == null) { @@ -205,7 +210,7 @@ public Task CopyFileAsync(string srcFolderName, string srcFileName, string destF throw new InvalidOperationException("Could not copy because destination file already exists", e); } - return Task.CompletedTask; + return Task.FromResult(null); } public Task IsAvailableAsync() diff --git a/tests/NuGetGallery.Core.Facts/Services/CloudBlobCoreFileStorageServiceFacts.cs b/tests/NuGetGallery.Core.Facts/Services/CloudBlobCoreFileStorageServiceFacts.cs index 1e21ae0266..c75fbb2a92 100644 --- a/tests/NuGetGallery.Core.Facts/Services/CloudBlobCoreFileStorageServiceFacts.cs +++ b/tests/NuGetGallery.Core.Facts/Services/CloudBlobCoreFileStorageServiceFacts.cs @@ -715,7 +715,12 @@ public async Task WillCopyTheFileIfDestinationDoesNotExist() }); // Act - await _target.CopyFileAsync(_srcFolderName, _srcFileName, _destFolderName, _destFileName); + await _target.CopyFileAsync( + _srcFolderName, + _srcFileName, + _destFolderName, + _destFileName, + AccessConditionWrapper.GenerateIfNotExistsCondition()); // Assert _destBlobMock.Verify( @@ -736,7 +741,12 @@ public async Task WillThrowInvalidOperationExceptionForConflict() // Act & Assert await Assert.ThrowsAsync( - () => _target.CopyFileAsync(_srcFolderName, _srcFileName, _destFolderName, _destFileName)); + () => _target.CopyFileAsync( + _srcFolderName, + _srcFileName, + _destFolderName, + _destFileName, + AccessConditionWrapper.GenerateIfNotExistsCondition())); } [Fact] @@ -770,17 +780,91 @@ public async Task WillCopyTheFileIfDestinationHasFailedCopy() .Callback(() => SetDestCopyStatus(CopyStatus.Success)); // Act - await _target.CopyFileAsync(_srcFolderName, _srcFileName, _destFolderName, _destFileName); + var srcETag = await _target.CopyFileAsync( + _srcFolderName, + _srcFileName, + _destFolderName, + _destFileName, + AccessConditionWrapper.GenerateIfNotExistsCondition()); // Assert _destBlobMock.Verify( x => x.StartCopyAsync(It.IsAny(), It.IsAny(), It.IsAny()), Times.Once); + Assert.Equal(_srcETag, srcETag); Assert.Equal(_srcFileName, srcBlob.Name); Assert.Equal(_srcETag, srcAccessCondition.IfMatchETag); Assert.Equal(_destETag, destAccessCondition.IfMatchETag); } + [Fact] + public async Task WillDefaultToIfNotExists() + { + // Arrange + AccessCondition srcAccessCondition = null; + AccessCondition destAccessCondition = null; + ISimpleCloudBlob srcBlob = null; + _destBlobMock + .Setup(x => x.StartCopyAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .Returns(Task.FromResult(0)) + .Callback((b, s, d) => + { + srcBlob = b; + srcAccessCondition = s; + destAccessCondition = d; + SetDestCopyStatus(CopyStatus.Success); + }); + + // Act + await _target.CopyFileAsync( + _srcFolderName, + _srcFileName, + _destFolderName, + _destFileName, + destAccessCondition: null); + + // Assert + _destBlobMock.Verify( + x => x.StartCopyAsync(It.IsAny(), It.IsAny(), It.IsAny()), + Times.Once); + Assert.Null(destAccessCondition.IfMatchETag); + Assert.Equal("*", destAccessCondition.IfNoneMatchETag); + } + + [Fact] + public async Task UsesProvidedMatchETag() + { + // Arrange + AccessCondition srcAccessCondition = null; + AccessCondition destAccessCondition = null; + ISimpleCloudBlob srcBlob = null; + _destBlobMock + .Setup(x => x.StartCopyAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .Returns(Task.FromResult(0)) + .Callback((b, s, d) => + { + srcBlob = b; + srcAccessCondition = s; + destAccessCondition = d; + SetDestCopyStatus(CopyStatus.Success); + }); + + // Act + await _target.CopyFileAsync( + _srcFolderName, + _srcFileName, + _destFolderName, + _destFileName, + AccessConditionWrapper.GenerateIfMatchCondition("etag!")); + + // Assert + _destBlobMock.Verify( + x => x.StartCopyAsync(It.IsAny(), It.IsAny(), It.IsAny()), + Times.Once); + Assert.NotEqual("etag!", destAccessCondition.IfMatchETag); + Assert.Null(destAccessCondition.IfNoneMatchETag); + } + [Fact] public async Task NoOpsIfPackageLengthAndHashMatch() { @@ -795,7 +879,12 @@ public async Task NoOpsIfPackageLengthAndHashMatch() .ReturnsAsync(true); // Act - await _target.CopyFileAsync(_srcFolderName, _srcFileName, _destFolderName, _destFileName); + await _target.CopyFileAsync( + _srcFolderName, + _srcFileName, + _destFolderName, + _destFileName, + AccessConditionWrapper.GenerateIfNotExistsCondition()); // Assert _destBlobMock.Verify( @@ -817,7 +906,12 @@ public async Task ThrowsIfCopyOperationFails() // Act & Assert var ex = await Assert.ThrowsAsync( - () => _target.CopyFileAsync(_srcFolderName, _srcFileName, _destFolderName, _destFileName)); + () => _target.CopyFileAsync( + _srcFolderName, + _srcFileName, + _destFolderName, + _destFileName, + AccessConditionWrapper.GenerateIfNotExistsCondition())); Assert.Contains("The blob copy operation had copy status Failed", ex.Message); } diff --git a/tests/NuGetGallery.Facts/Services/FileSystemFileStorageServiceFacts.cs b/tests/NuGetGallery.Facts/Services/FileSystemFileStorageServiceFacts.cs index a95314dcc9..f3002b835b 100644 --- a/tests/NuGetGallery.Facts/Services/FileSystemFileStorageServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/FileSystemFileStorageServiceFacts.cs @@ -333,7 +333,12 @@ await _target.SaveFileAsync( new MemoryStream(Encoding.ASCII.GetBytes(content))); // Act - await _target.CopyFileAsync(_srcFolderName, _srcFileName, _destFolderName, _destFileName); + await _target.CopyFileAsync( + _srcFolderName, + _srcFileName, + _destFolderName, + _destFileName, + destAccessCondition: null); // Assert using (var destStream = await _target.GetFileAsync(_destFolderName, _destFileName)) @@ -358,7 +363,12 @@ await _target.SaveFileAsync( new MemoryStream(Encoding.ASCII.GetBytes(content))); await Assert.ThrowsAsync( - () => _target.CopyFileAsync(_srcFolderName, _srcFileName, _destFolderName, _destFileName)); + () => _target.CopyFileAsync( + _srcFolderName, + _srcFileName, + _destFolderName, + _destFileName, + destAccessCondition: null)); } [Fact] @@ -377,7 +387,12 @@ await _target.SaveFileAsync( // Act & Assert await Assert.ThrowsAsync( - () => _target.CopyFileAsync(_srcFolderName, _srcFileName, _destFolderName, _destFileName)); + () => _target.CopyFileAsync( + _srcFolderName, + _srcFileName, + _destFolderName, + _destFileName, + destAccessCondition: null)); } public void Dispose()