From 0e244f831a73e4423560f78ad4f1aead4d49aee4 Mon Sep 17 00:00:00 2001 From: Steven Weerdenburg Date: Wed, 17 Oct 2018 16:56:00 -0400 Subject: [PATCH 01/20] Add setting 'Gallery.FileStorageDirectory' (#6569) --- src/NuGetGallery/Web.config | 1 + 1 file changed, 1 insertion(+) diff --git a/src/NuGetGallery/Web.config b/src/NuGetGallery/Web.config index 35e3ba7a69..ab92d12119 100644 --- a/src/NuGetGallery/Web.config +++ b/src/NuGetGallery/Web.config @@ -144,6 +144,7 @@ + From cdbfae0ea7c09cc873297bc3d009edb3183f847a Mon Sep 17 00:00:00 2001 From: Shishir H Date: Thu, 18 Oct 2018 12:12:18 -0700 Subject: [PATCH 02/20] Symbols UI Fixes (#6576) --- .../Extensions/PackageExtensions.cs | 11 +++++++++ .../Controllers/PackagesController.cs | 23 +++++++++++++++---- .../RequestModels/VerifyPackageRequest.cs | 1 + .../Scripts/gallery/async-file-upload.js | 9 +++++++- .../Views/Packages/DeleteSymbols.cshtml | 9 ++++---- .../Views/Packages/UploadPackage.cshtml | 9 ++++++-- .../Views/Packages/_VerifyMetadata.cshtml | 2 +- 7 files changed, 51 insertions(+), 13 deletions(-) diff --git a/src/NuGetGallery.Core/Extensions/PackageExtensions.cs b/src/NuGetGallery.Core/Extensions/PackageExtensions.cs index 1e4b202007..b29884c612 100644 --- a/src/NuGetGallery.Core/Extensions/PackageExtensions.cs +++ b/src/NuGetGallery.Core/Extensions/PackageExtensions.cs @@ -19,5 +19,16 @@ public static SymbolPackage LatestSymbolPackage(this Package package) .OrderByDescending(sp => sp.Created) .FirstOrDefault(); } + + /// + /// Returns true if there exists a symbol package which is latest and is in + /// available state, false otherwise. + /// + public static bool IsLatestSymbolPackageAvailable(this Package package) + { + var latestSymbolPackage = package.LatestSymbolPackage(); + return latestSymbolPackage != null + && latestSymbolPackage.StatusKey == PackageStatus.Available; + } } } \ No newline at end of file diff --git a/src/NuGetGallery/Controllers/PackagesController.cs b/src/NuGetGallery/Controllers/PackagesController.cs index 40ec10d4a0..b41966e067 100644 --- a/src/NuGetGallery/Controllers/PackagesController.cs +++ b/src/NuGetGallery/Controllers/PackagesController.cs @@ -237,6 +237,8 @@ private async Task UploadSymbolsPackageInternal(SubmitPackageReque var verifyRequest = new VerifyPackageRequest(packageMetadata, accountsAllowedOnBehalfOf, existingPackageRegistration); verifyRequest.IsSymbolsPackage = true; + verifyRequest.HasExistingAvailableSymbols = packageForUploadingSymbols.IsLatestSymbolPackageAvailable(); + model.InProgressUpload = verifyRequest; return View(model); @@ -386,7 +388,12 @@ private async Task UploadSymbolsPackageInternal(PackageArchiveReader // Save the uploaded file await _uploadFileService.SaveUploadFileAsync(currentUser.Key, uploadStream); - return await GetVerifyPackageView(currentUser, packageMetadata, accountsAllowedOnBehalfOf, existingPackageRegistration, isSymbolsPackageUpload: true); + return await GetVerifyPackageView(currentUser, + packageMetadata, + accountsAllowedOnBehalfOf, + existingPackageRegistration, + isSymbolsPackageUpload: true, + hasExistingSymbolsPackageAvailable: packageForUploadingSymbols.IsLatestSymbolPackageAvailable()); } private async Task UploadPackageInternal(PackageArchiveReader packageArchiveReader, Stream uploadStream, NuspecReader nuspec, PackageMetadata packageMetadata) @@ -495,14 +502,22 @@ await _packageDeleteService.HardDeletePackagesAsync( await _uploadFileService.SaveUploadFileAsync(currentUser.Key, uploadStream); - return await GetVerifyPackageView(currentUser, packageMetadata, accountsAllowedOnBehalfOf, existingPackageRegistration, isSymbolsPackageUpload: false); + var hasExistingSymbolsPackageAvailable = existingPackage != null && existingPackage.IsLatestSymbolPackageAvailable(); + + return await GetVerifyPackageView(currentUser, + packageMetadata, + accountsAllowedOnBehalfOf, + existingPackageRegistration, + isSymbolsPackageUpload: false, + hasExistingSymbolsPackageAvailable: hasExistingSymbolsPackageAvailable); } private async Task GetVerifyPackageView(User currentUser, PackageMetadata packageMetadata, IEnumerable accountsAllowedOnBehalfOf, PackageRegistration existingPackageRegistration, - bool isSymbolsPackageUpload) + bool isSymbolsPackageUpload, + bool hasExistingSymbolsPackageAvailable) { IReadOnlyList warnings = new List(); using (Stream uploadedFile = await _uploadFileService.GetUploadFileAsync(currentUser.Key)) @@ -546,8 +561,8 @@ private async Task GetVerifyPackageView(User currentUser, var model = new VerifyPackageRequest(packageMetadata, accountsAllowedOnBehalfOf, existingPackageRegistration); model.IsSymbolsPackage = isSymbolsPackageUpload; + model.HasExistingAvailableSymbols = hasExistingSymbolsPackageAvailable; model.Warnings.AddRange(warnings); - return Json(model); } diff --git a/src/NuGetGallery/RequestModels/VerifyPackageRequest.cs b/src/NuGetGallery/RequestModels/VerifyPackageRequest.cs index fc4c62f945..64bbe83803 100644 --- a/src/NuGetGallery/RequestModels/VerifyPackageRequest.cs +++ b/src/NuGetGallery/RequestModels/VerifyPackageRequest.cs @@ -117,6 +117,7 @@ public VerifyPackageRequest(PackageMetadata packageMetadata, IEnumerable p public string Tags { get; set; } public string Title { get; set; } public bool IsSymbolsPackage { get; set; } + public bool HasExistingAvailableSymbols { get; set; } public List Warnings { get; set; } = new List(); diff --git a/src/NuGetGallery/Scripts/gallery/async-file-upload.js b/src/NuGetGallery/Scripts/gallery/async-file-upload.js index c5319e98d6..6eebf213e5 100644 --- a/src/NuGetGallery/Scripts/gallery/async-file-upload.js +++ b/src/NuGetGallery/Scripts/gallery/async-file-upload.js @@ -84,7 +84,7 @@ }; function resetFileSelectFeedback() { - $('#file-select-feedback').attr('value', 'Browse or Drop files to select a package or symbols package...'); + $('#file-select-feedback').attr('value', 'Browse or Drop files to select a package (.nupkg) or symbols package (.snupkg)...'); } function prepareUploadFormData() { @@ -232,6 +232,7 @@ function clearErrors() { $("#validation-failure-container").addClass("hidden"); $("#validation-failure-list").remove(); + $('#symbols-replace-warning-container').addClass('hidden'); var warnings = $('#warning-container'); warnings.addClass("hidden"); @@ -271,6 +272,12 @@ cancelUploadAsync(); }); + if (model.IsSymbolsPackage && model.HasExistingAvailableSymbols) { + $('#symbols-replace-warning-container').removeClass('hidden'); + } else { + $('#symbols-replace-warning-container').addClass('hidden'); + } + $('#verify-submit-button').on('click', function () { $('#verify-cancel-button').attr('disabled', 'disabled'); $('#verify-submit-button').attr('disabled', 'disabled'); diff --git a/src/NuGetGallery/Views/Packages/DeleteSymbols.cshtml b/src/NuGetGallery/Views/Packages/DeleteSymbols.cshtml index dc4fb2230b..ca926a881f 100644 --- a/src/NuGetGallery/Views/Packages/DeleteSymbols.cshtml +++ b/src/NuGetGallery/Views/Packages/DeleteSymbols.cshtml @@ -61,16 +61,15 @@

- + Delete Symbols Package Version

-
+
@ViewHelpers.AlertDanger( @ - Deleting this symbols package will remove all the symbols from the symbol server and make them unavailable.
+ Deleting this symbols package will remove all symbols in this package from the symbol server and make them unavailable.
) @using (Html.BeginForm("DeleteSymbolsPackage", "Packages", FormMethod.Post, new { id = "delete-symbols-form" })) diff --git a/src/NuGetGallery/Views/Packages/UploadPackage.cshtml b/src/NuGetGallery/Views/Packages/UploadPackage.cshtml index a371e7d587..b78ff8e890 100644 --- a/src/NuGetGallery/Views/Packages/UploadPackage.cshtml +++ b/src/NuGetGallery/Views/Packages/UploadPackage.cshtml @@ -3,8 +3,8 @@ ViewBag.Title = "Upload Package"; ViewBag.Tab = "Upload"; var placeholder = Model.IsSymbolsUploadEnabled - ? "Browse or Drop files to select a package or symbols package..." - : "Browse or Drop files to select a package"; + ? "Browse or Drop files to select a package (.nupkg) or symbols package (.snupkg)..." + : "Browse or Drop files to select a package (.nupkg)"; var acceptExtensions = Model.IsSymbolsUploadEnabled ? ".nupkg,.snupkg" : ".nupkg"; @@ -58,6 +58,11 @@
} + + @Html.Partial("_VerifyForm")
diff --git a/src/NuGetGallery/Views/Packages/_VerifyMetadata.cshtml b/src/NuGetGallery/Views/Packages/_VerifyMetadata.cshtml index 6dfdc707c2..ce4c4422f7 100644 --- a/src/NuGetGallery/Views/Packages/_VerifyMetadata.cshtml +++ b/src/NuGetGallery/Views/Packages/_VerifyMetadata.cshtml @@ -121,6 +121,7 @@ +
@@ -136,7 +137,6 @@
-
(for this version)
From fa40f54d2ce81d582bc5bbbee50bba0fc00a9745 Mon Sep 17 00:00:00 2001 From: cmanu Date: Thu, 18 Oct 2018 13:08:57 -0700 Subject: [PATCH 03/20] GetEtagAsync (#6577) Add GetEtagAsync. --- .../CloudBlobCoreFileStorageService.cs | 21 +++++++ .../Services/ICoreFileStorageService.cs | 10 ++++ .../Services/FileSystemFileStorageService.cs | 7 +++ .../CloudBlobCoreFileStorageServiceFacts.cs | 56 +++++++++++++++++++ 4 files changed, 94 insertions(+) diff --git a/src/NuGetGallery.Core/Services/CloudBlobCoreFileStorageService.cs b/src/NuGetGallery.Core/Services/CloudBlobCoreFileStorageService.cs index 95c91c214a..c709e9ceab 100644 --- a/src/NuGetGallery.Core/Services/CloudBlobCoreFileStorageService.cs +++ b/src/NuGetGallery.Core/Services/CloudBlobCoreFileStorageService.cs @@ -426,6 +426,27 @@ public async Task SetMetadataAsync( } } + public async Task GetETagOrNullAsync( + string folderName, + string fileName) + { + folderName = folderName ?? throw new ArgumentNullException(nameof(folderName)); + fileName = fileName ?? throw new ArgumentNullException(nameof(fileName)); + + var container = await GetContainerAsync(folderName); + var blob = container.GetBlobReference(fileName); + try + { + await blob.FetchAttributesAsync(); + return blob.ETag; + } + // In case that the blob does not exist return null. + catch (StorageException) + { + return null; + } + } + private static SharedAccessBlobPermissions MapFileUriPermissions(FileUriPermissions permissions) { return (SharedAccessBlobPermissions)permissions; diff --git a/src/NuGetGallery.Core/Services/ICoreFileStorageService.cs b/src/NuGetGallery.Core/Services/ICoreFileStorageService.cs index 184eff8367..0914455990 100644 --- a/src/NuGetGallery.Core/Services/ICoreFileStorageService.cs +++ b/src/NuGetGallery.Core/Services/ICoreFileStorageService.cs @@ -120,5 +120,15 @@ Task SetMetadataAsync( string folderName, string fileName, Func>, IDictionary, Task> updateMetadataAsync); + + /// + /// Returns the etag value for the specified blob. If the blob does not exists it will return null. + /// + /// The folder name. + /// The file name. + /// The etag of the specified file. + Task GetETagOrNullAsync( + string folderName, + string fileName); } } \ No newline at end of file diff --git a/src/NuGetGallery/Services/FileSystemFileStorageService.cs b/src/NuGetGallery/Services/FileSystemFileStorageService.cs index c35369e7d0..2c9a2fb04b 100644 --- a/src/NuGetGallery/Services/FileSystemFileStorageService.cs +++ b/src/NuGetGallery/Services/FileSystemFileStorageService.cs @@ -273,6 +273,13 @@ public static string ResolvePath(string fileStorageDirectory) return fileStorageDirectory; } + public Task GetETagOrNullAsync( + string folderName, + string fileName) + { + throw new NotImplementedException(nameof(GetETagOrNullAsync)); + } + private static string GetContentType(string folderName) { switch (folderName) diff --git a/tests/NuGetGallery.Core.Facts/Services/CloudBlobCoreFileStorageServiceFacts.cs b/tests/NuGetGallery.Core.Facts/Services/CloudBlobCoreFileStorageServiceFacts.cs index 33b6bf2a3b..25a211b407 100644 --- a/tests/NuGetGallery.Core.Facts/Services/CloudBlobCoreFileStorageServiceFacts.cs +++ b/tests/NuGetGallery.Core.Facts/Services/CloudBlobCoreFileStorageServiceFacts.cs @@ -1410,5 +1410,61 @@ await _service.SetMetadataAsync( _blobClient.VerifyAll(); } } + + public class TheGetETagMethod + { + private const string _etag = "dummy_etag"; + + private readonly Mock _blobClient; + private readonly Mock _blobContainer; + private readonly Mock _blob; + private readonly CloudBlobCoreFileStorageService _service; + + public TheGetETagMethod() + { + _blobClient = new Mock(); + _blobContainer = new Mock(); + _blob = new Mock(); + + _blobClient.Setup(x => x.GetContainerReference(It.IsAny())) + .Returns(_blobContainer.Object); + _blobContainer.Setup(x => x.CreateIfNotExistAsync()) + .Returns(Task.FromResult(0)); + _blobContainer.Setup(x => x.SetPermissionsAsync(It.IsAny())) + .Returns(Task.FromResult(0)); + _blobContainer.Setup(x => x.GetBlobReference(It.IsAny())) + .Returns(_blob.Object); + + _service = CreateService(fakeBlobClient: _blobClient); + } + + [Fact] + public async Task VerifyTheETagValue() + { + // Arrange + _blob.SetupGet(x => x.ETag).Returns(_etag); + + // Act + var etagValue = await _service.GetETagOrNullAsync(folderName: CoreConstants.PackagesFolderName, fileName: "a"); + + // Assert + Assert.Equal(_etag, etagValue); + } + + + [Fact] + public async Task VerifyETagIsNullWhenBlobDoesNotExist() + { + // Arrange + _blob.Setup(x => x.FetchAttributesAsync()).ThrowsAsync(new StorageException("Boo")); + + // Act + var etagValue = await _service.GetETagOrNullAsync(folderName: CoreConstants.PackagesFolderName, fileName: "a"); + + // Assert + Assert.Null(etagValue); + } + } + } } \ No newline at end of file From 88588f88def779f34ffb5b939a78aacfb10d8246 Mon Sep 17 00:00:00 2001 From: Shishir H Date: Fri, 19 Oct 2018 15:57:44 -0700 Subject: [PATCH 04/20] [MSA] Pass the error messages from AADv2 to gallery (#6581) --- .../AzureActiveDirectoryV2Authenticator.cs | 30 ++++++++++----- .../Controllers/AuthenticationController.cs | 38 ++++++++++++++++--- src/NuGetGallery/Helpers/UriExtensions.cs | 16 ++++++++ src/NuGetGallery/Strings.Designer.cs | 9 +++++ src/NuGetGallery/Strings.resx | 3 ++ .../AuthenticationControllerFacts.cs | 34 ++++++++++++++--- .../NuGetGallery.Facts.csproj | 1 + .../NuGetGallery.Facts/UriExtensionsFacts.cs | 38 +++++++++++++++++++ 8 files changed, 149 insertions(+), 20 deletions(-) create mode 100644 tests/NuGetGallery.Facts/UriExtensionsFacts.cs diff --git a/src/NuGetGallery/Authentication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2Authenticator.cs b/src/NuGetGallery/Authentication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2Authenticator.cs index 0e79a4dbe4..640d3902bc 100644 --- a/src/NuGetGallery/Authentication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2Authenticator.cs +++ b/src/NuGetGallery/Authentication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2Authenticator.cs @@ -45,10 +45,15 @@ public static class AuthenticationType public static readonly string Authority = "https://login.microsoftonline.com/{0}/v2.0"; private static string _callbackPath = "users/account/authenticate/return"; - private static HashSet _errorMessageList = new HashSet { "access_denied", "consent_required" }; private static HashSet _alternateSiteRootList; private const string SELECT_ACCOUNT = "select_account"; + private static class ErrorQueryKeys + { + public const string Error = "error"; + public const string ErrorDescription = "errorDescription"; + } + /// /// The possible values returned by claim, and also the possible token values to be sent /// for authentication to the common endpoint. @@ -200,16 +205,23 @@ public override bool SupportsMultiFactorAuthentication() // error handling is done. private Task AuthenticationFailed(AuthenticationFailedNotification notification) { - if (_errorMessageList.Contains(notification.Exception.Message)) + // For every 'Challenge' sent to the external providers, we pass the 'State' + // with the redirect uri where we intend to return after successful authentication. + // Extract this "RedirectUri" property from this "State" object for redirecting on failed authentication as well. + var authenticationProperties = GetAuthenticationPropertiesFromProtocolMessage(notification.ProtocolMessage, notification.Options); + + // All authentication related exceptions will be handled. + notification.HandleResponse(); + + // Pass the errors as the query string to show appropriate message to the user. + var queryString = new List>() { - // For every 'Challenge' sent to the external providers, we store the 'State' - // with the redirect uri where we intend to return after successful authentication. - // Extract this "RedirectUri" property from this "State" object for redirecting on failed authentication as well. - var authenticationProperties = GetAuthenticationPropertiesFromProtocolMessage(notification.ProtocolMessage, notification.Options); + new KeyValuePair(ErrorQueryKeys.Error, notification.ProtocolMessage.Error), + new KeyValuePair(ErrorQueryKeys.ErrorDescription, notification.ProtocolMessage.ErrorDescription) + }; - notification.HandleResponse(); - notification.Response.Redirect(authenticationProperties.RedirectUri); - } + var redirectUri = UriExtensions.AppendQueryStringToRelativeUri(authenticationProperties.RedirectUri, queryString); + notification.Response.Redirect(redirectUri); return Task.FromResult(0); } diff --git a/src/NuGetGallery/Controllers/AuthenticationController.cs b/src/NuGetGallery/Controllers/AuthenticationController.cs index c97eb8c862..d0368b5225 100644 --- a/src/NuGetGallery/Controllers/AuthenticationController.cs +++ b/src/NuGetGallery/Controllers/AuthenticationController.cs @@ -22,6 +22,12 @@ namespace NuGetGallery { + public static class AuthenticationFailureErrors + { + public const string ACCESSS_DENIED = "access_denied"; + public const string CONSENT_REQUIRED = "consent_required"; + } + public partial class AuthenticationController : AppController { @@ -172,7 +178,7 @@ public virtual async Task SignIn(LogOnViewModel model, string retu authenticatedUser = loginUserDetails?.AuthenticatedUser; if (authenticatedUser == null) { - return ExternalLinkExpired(); + return AuthenticationFailureOrExternalLinkExpired(); } usedMultiFactorAuthentication = loginUserDetails.UsedMultiFactorAuthentication; @@ -264,7 +270,7 @@ public virtual async Task Register(LogOnViewModel model, string re var result = await _authService.ReadExternalLoginCredential(OwinContext); if (result.ExternalIdentity == null) { - return ExternalLinkExpired(); + return AuthenticationFailureOrExternalLinkExpired(); } usedMultiFactorAuthentication = result.LoginDetails?.WasMultiFactorAuthenticated ?? false; @@ -497,7 +503,7 @@ public virtual async Task LinkOrChangeExternalCredential(string re return SafeRedirect(returnUrl); } - public virtual async Task LinkExternalAccount(string returnUrl) + public virtual async Task LinkExternalAccount(string returnUrl, string error = null, string errorDescription = null) { // Extract the external login info var result = await _authService.AuthenticateExternalLogin(OwinContext); @@ -505,7 +511,8 @@ public virtual async Task LinkExternalAccount(string returnUrl) { // User got here without an external login cookie (or an expired one) // Send them to the logon action - return ExternalLinkExpired(); + string errorMessage = GetAuthenticationFailureMessage(error, errorDescription); + return AuthenticationFailureOrExternalLinkExpired(errorMessage); } if (result.Authentication != null) @@ -749,11 +756,11 @@ private string GetEmailAddressFromExternalLoginResult(AuthenticateExternalLoginR } } - private ActionResult ExternalLinkExpired() + private ActionResult AuthenticationFailureOrExternalLinkExpired(string errorMessage = null) { // User got here without an external login cookie (or an expired one) // Send them to the logon action with a message - TempData["Message"] = Strings.ExternalAccountLinkExpired; + TempData["Message"] = string.IsNullOrEmpty(errorMessage) ? Strings.ExternalAccountLinkExpired : errorMessage; return Redirect(Url.LogOn(null, relativeUrl: false)); } @@ -815,5 +822,24 @@ private ActionResult AuthenticationView(string viewName, LogOnViewModel existing return View(viewName, existingModel); } + + private string GetAuthenticationFailureMessage(string error, string errorDescription) + { + if (string.IsNullOrEmpty(error)) + { + return Strings.AuthenticationFailure_UnkownError; + } + + switch (error) + { + case AuthenticationFailureErrors.ACCESSS_DENIED: + case AuthenticationFailureErrors.CONSENT_REQUIRED: + return Strings.ExternalAccountLinkExpired; + default: + return string.IsNullOrEmpty(errorDescription) + ? error + : errorDescription; + } + } } } diff --git a/src/NuGetGallery/Helpers/UriExtensions.cs b/src/NuGetGallery/Helpers/UriExtensions.cs index 7e96409499..a75a3dca23 100644 --- a/src/NuGetGallery/Helpers/UriExtensions.cs +++ b/src/NuGetGallery/Helpers/UriExtensions.cs @@ -2,6 +2,8 @@ // 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.Web; namespace NuGetGallery { @@ -57,5 +59,19 @@ public static Uri ToHttps(this Uri uri) return uriBuilder.Uri; } + + public static string AppendQueryStringToRelativeUri(string relativeUrl, IReadOnlyCollection> queryStringCollection) + { + var tempUri = new Uri("http://www.nuget.org/"); + var builder = new UriBuilder(new Uri(tempUri, relativeUrl)); + var query = HttpUtility.ParseQueryString(builder.Query); + foreach (var pair in queryStringCollection) + { + query[pair.Key] = pair.Value; + } + + builder.Query = query.ToString(); + return builder.Uri.PathAndQuery; + } } } \ No newline at end of file diff --git a/src/NuGetGallery/Strings.Designer.cs b/src/NuGetGallery/Strings.Designer.cs index 13c64eb9d3..7d344a1a2d 100644 --- a/src/NuGetGallery/Strings.Designer.cs +++ b/src/NuGetGallery/Strings.Designer.cs @@ -486,6 +486,15 @@ public static string ArgumentCannotBeNullOrEmpty { } } + /// + /// Looks up a localized string similar to Unknown error!. + /// + public static string AuthenticationFailure_UnkownError { + get { + return ResourceManager.GetString("AuthenticationFailure_UnkownError", resourceCulture); + } + } + /// /// Looks up a localized string similar to The '{0}' authentication provider is disabled and cannot be used to authenticate ///. diff --git a/src/NuGetGallery/Strings.resx b/src/NuGetGallery/Strings.resx index 037dbadfa2..f140383239 100644 --- a/src/NuGetGallery/Strings.resx +++ b/src/NuGetGallery/Strings.resx @@ -1002,4 +1002,7 @@ The {1} Team This package contains a <license> metadata which is currently not supported. + + Unknown error! + \ No newline at end of file diff --git a/tests/NuGetGallery.Facts/Controllers/AuthenticationControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/AuthenticationControllerFacts.cs index 9577a2af05..1b725535d8 100644 --- a/tests/NuGetGallery.Facts/Controllers/AuthenticationControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/AuthenticationControllerFacts.cs @@ -1457,8 +1457,10 @@ public void WillCallChallengeAuthenticationForAADv2Provider() public class TheLinkExternalAccountAction : TestContainer { - [Fact] - public async Task GivenExpiredExternalAuth_ItRedirectsBackToLogOnWithExternalAuthExpiredMessage() + [Theory] + [InlineData("access_denied")] + [InlineData("consent_required")] + public async Task GivenExpiredExternalAuth_ItRedirectsBackToLogOnWithExternalAuthExpiredMessage(string error) { // Arrange GetMock(); // Force a mock to be created @@ -1468,12 +1470,33 @@ public async Task GivenExpiredExternalAuth_ItRedirectsBackToLogOnWithExternalAut .CompletesWith(new AuthenticateExternalLoginResult()); // Act - var result = await controller.LinkExternalAccount("theReturnUrl"); + var result = await controller.LinkExternalAccount("theReturnUrl", error); // Assert VerifyExternalLinkExpiredResult(controller, result); } + [Theory] + [InlineData("server_error", "The server encountered an unexpected error.")] + [InlineData("temporarily_unavailable", "The server is temporarily too busy to handle the request.")] + [InlineData("invalid_resource", "The target resource is invalid because either it does not exist, Azure AD cannot find it, or it is not correctly configured.")] + [InlineData("invalid_request", "Protocol error, such as a missing, required parameter.")] + public async Task GivenExpiredExternalAuth_ItRedirectsBackToLogOnWithPassedErrorMessage(string error, string errorDescription) + { + // Arrange + GetMock(); // Force a mock to be created + var controller = GetController(); + GetMock() + .Setup(x => x.AuthenticateExternalLogin(controller.OwinContext)) + .CompletesWith(new AuthenticateExternalLoginResult()); + + // Act + var result = await controller.LinkExternalAccount("theReturnUrl", error, errorDescription); + + // Assert + VerifyExternalLinkExpiredResult(controller, result, errorDescription); + } + [Fact] public async Task GivenAssociatedLocalUser_ItCreatesASessionAndSafeRedirectsToReturnUrl() { @@ -2213,10 +2236,11 @@ public void VerifyShouldChallenge(string providerUsedForLogin, bool shouldChalle } } - public static void VerifyExternalLinkExpiredResult(AuthenticationController controller, ActionResult result) + public static void VerifyExternalLinkExpiredResult(AuthenticationController controller, ActionResult result, string expectedMessage = null) { + expectedMessage = expectedMessage ?? Strings.ExternalAccountLinkExpired; ResultAssert.IsRedirect(result, permanent: false, url: controller.Url.LogOn(relativeUrl: false)); - Assert.Equal(Strings.ExternalAccountLinkExpired, controller.TempData["Message"]); + Assert.Equal(expectedMessage, controller.TempData["Message"]); } private static void EnableAllAuthenticators(AuthenticationService authService) diff --git a/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj b/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj index f3789b5360..d24ca7cbeb 100644 --- a/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj +++ b/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj @@ -261,6 +261,7 @@ + diff --git a/tests/NuGetGallery.Facts/UriExtensionsFacts.cs b/tests/NuGetGallery.Facts/UriExtensionsFacts.cs new file mode 100644 index 0000000000..37abeb162f --- /dev/null +++ b/tests/NuGetGallery.Facts/UriExtensionsFacts.cs @@ -0,0 +1,38 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Collections.Generic; +using System.Linq; +using Xunit; + +namespace NuGetGallery +{ + public class UriExtensionsFacts + { + public class TheAppendQueryStringToRelativeUriMethod + { + [Theory] + [InlineData("/abac/something?q1=123", "q2=ads&q3=4324", "/abac/something?q1=123&q2=ads&q3=4324")] + [InlineData("/abac/something?q1=123", "q2=ads", "/abac/something?q1=123&q2=ads")] + [InlineData("/abac/something", "q2=ads", "/abac/something?q2=ads")] + [InlineData("/abac/something/", "", "/abac/something/")] + public void ReturnsExpectedUrl(string url, string queryParameters, string expectedUrl) + { + // Arrange + var queryString = string.IsNullOrEmpty(queryParameters) + ? new List>() + : queryParameters + .Split('&') + .Select(qv => qv.Split('=')) + .Select(qv => new KeyValuePair(qv[0], qv[1])) + .ToList(); + + // Act + var returnUrl = UriExtensions.AppendQueryStringToRelativeUri(url, queryString); + + // Assert + Assert.Equal(expectedUrl, returnUrl); + } + } + } +} From 9411c6943ddbd170387322a7e2bdb534bb0647fe Mon Sep 17 00:00:00 2001 From: Xavier Decoster Date: Mon, 22 Oct 2018 10:08:30 +0200 Subject: [PATCH 05/20] Emails should not be sent synchronously (#6555) --- .../mail/AsynchronousEmailMessageService.cs | 116 ++++++++++ .../Infrastructure/mail/EmailBuilder.cs | 3 + .../Infrastructure/mail/EmailFormat.cs | 7 +- .../Infrastructure/mail/EmailMessageFooter.cs | 62 ++++++ .../mail/MarkdownEmailBuilder.cs | 10 + .../mail/Messages/PackageAddedMessage.cs | 62 ++++-- .../Messages/SymbolPackageAddedMessage.cs | 62 ++++-- .../NuGetGallery.Core.csproj | 9 +- .../App_Start/DefaultDependenciesModule.cs | 58 ++++- .../Configuration/AppConfiguration.cs | 4 +- .../Configuration/IAppConfiguration.cs | 7 +- .../Configuration/IServiceBusConfiguration.cs | 14 ++ .../Configuration/ServiceBusConfiguration.cs | 6 + .../Mail/Messages/ContactOwnersMessage.cs | 63 +++--- src/NuGetGallery/NuGetGallery.csproj | 3 + src/NuGetGallery/Web.config | 3 + .../AsynchronousEmailMessageServiceFacts.cs | 208 ++++++++++++++++++ .../Messages/ContactSupportMessageFacts.cs | 8 + .../Mail/Messages/PackageAddedMessageFacts.cs | 11 +- .../PackageAddedWithWarningsMessageFacts.cs | 5 + .../PackageValidationFailedMessageFacts.cs | 9 + ...kageValidationTakingTooLongMessageFacts.cs | 6 + .../SymbolPackageAddedMessageFacts.cs | 11 +- ...mbolPackageValidationFailedMessageFacts.cs | 9 + ...kageValidationTakingTooLongMessageFacts.cs | 6 + .../NuGetGallery.Core.Facts.csproj | 1 + .../AccountDeleteNoticeMessageFacts.cs | 13 ++ .../Messages/ContactOwnersMessageFacts.cs | 10 + .../Messages/CredentialAddedMessageFacts.cs | 2 + .../Messages/CredentialRemovedMessageFacts.cs | 2 + .../EmailChangeConfirmationMessageFacts.cs | 14 ++ ...oticeToPreviousEmailAddressMessageFacts.cs | 6 + .../Mail/Messages/NewAccountMessageFacts.cs | 16 ++ .../OrganizationMemberRemovedMessageFacts.cs | 6 + .../OrganizationMemberUpdatedMessageFacts.cs | 12 + ...onMembershipRequestCanceledMessageFacts.cs | 6 + ...onMembershipRequestDeclinedMessageFacts.cs | 6 + ...nMembershipRequestInitiatedMessageFacts.cs | 12 + ...ganizationMembershipRequestMessageFacts.cs | 22 ++ ...ganizationTransformAcceptedMessageFacts.cs | 6 + ...anizationTransformInitiatedMessageFacts.cs | 9 + ...ganizationTransformRejectedMessageFacts.cs | 6 + ...rganizationTransformRequestMessageFacts.cs | 10 + .../PackageDeletedNoticeMessageFacts.cs | 6 + .../Messages/PackageOwnerAddedMessageFacts.cs | 6 + .../PackageOwnerRemovedMessageFacts.cs | 12 + ...ageOwnershipRequestCanceledMessageFacts.cs | 12 + ...ageOwnershipRequestDeclinedMessageFacts.cs | 12 + ...geOwnershipRequestInitiatedMessageFacts.cs | 8 + .../PackageOwnershipRequestMessageFacts.cs | 26 +++ .../Mail/Messages/ReportAbuseMessageFacts.cs | 18 ++ .../Messages/ReportMyPackageMessageFacts.cs | 15 ++ .../Messages/SigninAssistanceMessageFacts.cs | 16 ++ 53 files changed, 965 insertions(+), 87 deletions(-) create mode 100644 src/NuGetGallery.Core/Infrastructure/mail/AsynchronousEmailMessageService.cs create mode 100644 src/NuGetGallery.Core/Infrastructure/mail/EmailMessageFooter.cs create mode 100644 tests/NuGetGallery.Core.Facts/Infrastructure/Mail/AsynchronousEmailMessageServiceFacts.cs diff --git a/src/NuGetGallery.Core/Infrastructure/mail/AsynchronousEmailMessageService.cs b/src/NuGetGallery.Core/Infrastructure/mail/AsynchronousEmailMessageService.cs new file mode 100644 index 0000000000..16b2a26343 --- /dev/null +++ b/src/NuGetGallery.Core/Infrastructure/mail/AsynchronousEmailMessageService.cs @@ -0,0 +1,116 @@ +// 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.Threading.Tasks; +using NuGet.Services.Messaging; + +namespace NuGetGallery.Infrastructure.Mail +{ + public class AsynchronousEmailMessageService : IMessageService + { + private readonly IMessageServiceConfiguration _configuration; + private readonly IEmailMessageEnqueuer _emailMessageEnqueuer; + + public AsynchronousEmailMessageService( + IMessageServiceConfiguration configuration, + IEmailMessageEnqueuer emailMessageEnqueuer) + { + _configuration = configuration ?? throw new ArgumentNullException(nameof(configuration)); + _emailMessageEnqueuer = emailMessageEnqueuer ?? throw new ArgumentNullException(nameof(emailMessageEnqueuer)); + } + + public Task SendMessageAsync( + IEmailBuilder emailBuilder, + bool copySender = false, + bool discloseSenderAddress = false) + { + if (emailBuilder == null) + { + throw new ArgumentNullException(nameof(emailBuilder)); + } + + var message = CreateMessage( + emailBuilder, + copySender, + discloseSenderAddress); + + return EnqueueMessageAsync(message); + } + + private static EmailMessageData CreateMessage( + IEmailBuilder emailBuilder, + bool copySender = false, + bool discloseSenderAddress = false) + { + var recipients = emailBuilder.GetRecipients(); + + if (recipients == EmailRecipients.None) + { + // Optimization: no need to construct message body when no recipients. + return null; + } + + if (emailBuilder.Sender == null) + { + throw new ArgumentException( + $"No sender defined for message of type '{emailBuilder.GetType()}'.", + nameof(emailBuilder.Sender)); + } + + return new EmailMessageData( + emailBuilder.GetSubject(), + emailBuilder.GetBody(EmailFormat.PlainText), + emailBuilder.GetBody(EmailFormat.Html), + emailBuilder.Sender.Address, + to: recipients.To.Select(e => e.Address).ToList(), + cc: GenerateCC( + emailBuilder.Sender.Address, + recipients.CC.Select(e => e.Address).ToList(), + copySender, + discloseSenderAddress), + bcc: recipients.Bcc.Select(e => e.Address).ToList(), + replyTo: recipients.ReplyTo.Select(e => e.Address).ToList(), + messageTrackingId: Guid.NewGuid()); + } + + private static IReadOnlyList GenerateCC( + string fromAddress, + IReadOnlyList cc, bool copySender, + bool discloseSenderAddress) + { + var ccList = new List(); + if (cc != null) + { + ccList.AddRange(cc); + } + + if (copySender && discloseSenderAddress && !ccList.Contains(fromAddress)) + { + ccList.Add(fromAddress); + } + + return ccList; + } + + private Task EnqueueMessageAsync(EmailMessageData message) + { + if (message == null || !message.To.Any()) + { + return Task.CompletedTask; + } + + if (string.IsNullOrEmpty(message.HtmlBody) + && string.IsNullOrEmpty(message.PlainTextBody)) + { + throw new ArgumentException( + "No message body defined. Both plain-text and html bodies are empty.", + nameof(message)); + } + + return _emailMessageEnqueuer.SendEmailMessageAsync(message); + } + } +} diff --git a/src/NuGetGallery.Core/Infrastructure/mail/EmailBuilder.cs b/src/NuGetGallery.Core/Infrastructure/mail/EmailBuilder.cs index 45fb00b189..0e57ed8b35 100644 --- a/src/NuGetGallery.Core/Infrastructure/mail/EmailBuilder.cs +++ b/src/NuGetGallery.Core/Infrastructure/mail/EmailBuilder.cs @@ -22,6 +22,8 @@ public string GetBody(EmailFormat format) return GetPlainTextBody(); case EmailFormat.Markdown: return GetMarkdownBody(); + case EmailFormat.Html: + return GetHtmlBody(); default: throw new ArgumentOutOfRangeException(nameof(format)); } @@ -33,6 +35,7 @@ public string GetBody(EmailFormat format) protected abstract string GetPlainTextBody(); protected abstract string GetMarkdownBody(); + protected abstract string GetHtmlBody(); /// /// Markdown sees the underscore as italics indicator, so underscores are stripped in the message. diff --git a/src/NuGetGallery.Core/Infrastructure/mail/EmailFormat.cs b/src/NuGetGallery.Core/Infrastructure/mail/EmailFormat.cs index a50338483f..3df3a1e699 100644 --- a/src/NuGetGallery.Core/Infrastructure/mail/EmailFormat.cs +++ b/src/NuGetGallery.Core/Infrastructure/mail/EmailFormat.cs @@ -18,6 +18,11 @@ public enum EmailFormat /// Indicates that will create email messages using Markdown formatting, /// which will be rendered as HTML by email clients. /// - Markdown + Markdown, + + /// + /// Indicates that will create email messages using HTML formatting. + /// + Html } } diff --git a/src/NuGetGallery.Core/Infrastructure/mail/EmailMessageFooter.cs b/src/NuGetGallery.Core/Infrastructure/mail/EmailMessageFooter.cs new file mode 100644 index 0000000000..f2d94af2fc --- /dev/null +++ b/src/NuGetGallery.Core/Infrastructure/mail/EmailMessageFooter.cs @@ -0,0 +1,62 @@ +// 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; + +namespace NuGetGallery.Infrastructure.Mail +{ + /// + /// Utility class to construct common email message footers. + /// + public static class EmailMessageFooter + { + public static string ForPackageOwnerNotifications(EmailFormat format, string galleryOwnerDisplayName, string emailSettingsUrl) + { + return ForReason( + format, + galleryOwnerDisplayName, + emailSettingsUrl, + "To stop receiving emails as an owner of this package"); + } + + public static string ForContactOwnerNotifications(EmailFormat format, string galleryOwnerDisplayName, string emailSettingsUrl) + { + return ForReason( + format, + galleryOwnerDisplayName, + emailSettingsUrl, + "To stop receiving contact emails as an owner of this package"); + } + + private static string ForReason(EmailFormat format, string galleryOwnerDisplayName, string emailSettingsUrl, string reason) + { + switch (format) + { + case EmailFormat.PlainText: + // Hyperlinks within an HTML emphasis tag are not supported by the Plain Text renderer in Markdig. + return $@" + +----------------------------------------------- + {reason}, sign in to the {galleryOwnerDisplayName} and + change your email notification settings ({emailSettingsUrl})."; + case EmailFormat.Html: + // Hyperlinks within an HTML emphasis tag are not supported by the HTML renderer in Markdig. + return $@"
+ + {reason}, sign in to the {galleryOwnerDisplayName} and + change your email notification settings. +"; + case EmailFormat.Markdown: + return $@" + +----------------------------------------------- + + {reason}, sign in to the {galleryOwnerDisplayName} and + [change your email notification settings]({emailSettingsUrl}). +"; + default: + throw new ArgumentOutOfRangeException(nameof(format)); + } + } + } +} \ No newline at end of file diff --git a/src/NuGetGallery.Core/Infrastructure/mail/MarkdownEmailBuilder.cs b/src/NuGetGallery.Core/Infrastructure/mail/MarkdownEmailBuilder.cs index 961993c7f7..4554b9dfaf 100644 --- a/src/NuGetGallery.Core/Infrastructure/mail/MarkdownEmailBuilder.cs +++ b/src/NuGetGallery.Core/Infrastructure/mail/MarkdownEmailBuilder.cs @@ -15,6 +15,11 @@ protected override string GetPlainTextBody() { var markdown = GetMarkdownBody(); + return ToPlainText(markdown); + } + + protected string ToPlainText(string markdown) + { var writer = new StringWriter(); var pipeline = new MarkdownPipelineBuilder().Build(); @@ -29,5 +34,10 @@ protected override string GetPlainTextBody() return writer.ToString(); } + + protected override string GetHtmlBody() + { + return Markdown.ToHtml(GetMarkdownBody()); + } } } \ No newline at end of file diff --git a/src/NuGetGallery.Core/Infrastructure/mail/Messages/PackageAddedMessage.cs b/src/NuGetGallery.Core/Infrastructure/mail/Messages/PackageAddedMessage.cs index 4a0163ea02..3e31ec5aa6 100644 --- a/src/NuGetGallery.Core/Infrastructure/mail/Messages/PackageAddedMessage.cs +++ b/src/NuGetGallery.Core/Infrastructure/mail/Messages/PackageAddedMessage.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Linq; using System.Net.Mail; +using Markdig; namespace NuGetGallery.Infrastructure.Mail.Messages { @@ -58,38 +59,57 @@ public override string GetSubject() protected override string GetMarkdownBody() { - var warningMessagesPlaceholder = string.Empty; - if (_hasWarnings) + return GetBodyInternal(EmailFormat.Markdown); + } + + protected override string GetPlainTextBody() + { + return GetBodyInternal(EmailFormat.PlainText); + } + + protected override string GetHtmlBody() + { + return GetBodyInternal(EmailFormat.Html); + } + + private string GetBodyInternal(EmailFormat format) + { + var warningMessages = GetWarningMessages(); + + var markdown = $@"The package [{Package.PackageRegistration.Id} {Package.Version}]({_packageUrl}) was recently published on {_configuration.GalleryOwner.DisplayName} by {Package.User.Username}. If this was not intended, please [contact support]({_packageSupportUrl})."; + + if (!string.IsNullOrEmpty(warningMessages)) { - warningMessagesPlaceholder = Environment.NewLine + string.Join(Environment.NewLine, _warningMessages); + markdown += warningMessages; } - return $@"The package [{Package.PackageRegistration.Id} {Package.Version}]({_packageUrl}) was recently published on {_configuration.GalleryOwner.DisplayName} by {Package.User.Username}. If this was not intended, please [contact support]({_packageSupportUrl}). -{warningMessagesPlaceholder} + string body; + switch (format) + { + case EmailFormat.PlainText: + body = ToPlainText(markdown); + break; + case EmailFormat.Markdown: + body = markdown; + break; + case EmailFormat.Html: + body = Markdown.ToHtml(markdown); + break; + default: + throw new ArgumentOutOfRangeException(nameof(format)); + } ------------------------------------------------ - - To stop receiving emails as an owner of this package, sign in to the {_configuration.GalleryOwner.DisplayName} and - [change your email notification settings]({_emailSettingsUrl}). -"; + return body + EmailMessageFooter.ForPackageOwnerNotifications(format, _configuration.GalleryOwner.DisplayName, _emailSettingsUrl); } - protected override string GetPlainTextBody() + private string GetWarningMessages() { - // The HTML emphasis tag is not supported by the Plain Text renderer in Markdig. - // Manually overriding this one. var warningMessagesPlaceholder = string.Empty; if (_hasWarnings) { - warningMessagesPlaceholder = Environment.NewLine + string.Join(Environment.NewLine, _warningMessages); + warningMessagesPlaceholder = Environment.NewLine + Environment.NewLine + string.Join(Environment.NewLine, _warningMessages); } - - return $@"The package {Package.PackageRegistration.Id} {Package.Version} ({_packageUrl}) was recently published on {_configuration.GalleryOwner.DisplayName} by {Package.User.Username}. If this was not intended, please contact support ({_packageSupportUrl}). -{warningMessagesPlaceholder} - ------------------------------------------------ - To stop receiving emails as an owner of this package, sign in to the {_configuration.GalleryOwner.DisplayName} and - change your email notification settings ({_emailSettingsUrl})."; + return warningMessagesPlaceholder; } } } diff --git a/src/NuGetGallery.Core/Infrastructure/mail/Messages/SymbolPackageAddedMessage.cs b/src/NuGetGallery.Core/Infrastructure/mail/Messages/SymbolPackageAddedMessage.cs index 1899e9daf7..cb249cf74e 100644 --- a/src/NuGetGallery.Core/Infrastructure/mail/Messages/SymbolPackageAddedMessage.cs +++ b/src/NuGetGallery.Core/Infrastructure/mail/Messages/SymbolPackageAddedMessage.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Linq; using System.Net.Mail; +using Markdig; namespace NuGetGallery.Infrastructure.Mail.Messages { @@ -57,38 +58,57 @@ public override string GetSubject() protected override string GetMarkdownBody() { - var warningMessagesPlaceholder = string.Empty; - if (_hasWarnings) + return GetBodyInternal(EmailFormat.Markdown); + } + + protected override string GetPlainTextBody() + { + return GetBodyInternal(EmailFormat.PlainText); + } + + protected override string GetHtmlBody() + { + return GetBodyInternal(EmailFormat.Html); + } + + private string GetBodyInternal(EmailFormat format) + { + var warningMessages = GetWarningMessages(); + + var markdown = $@"The symbol package [{_symbolPackage.Id} {_symbolPackage.Version}]({_packageUrl}) was recently published on {_configuration.GalleryOwner.DisplayName} by {_symbolPackage.Package.User.Username}. If this was not intended, please [contact support]({_packageSupportUrl})."; + + if (!string.IsNullOrEmpty(warningMessages)) { - warningMessagesPlaceholder = Environment.NewLine + string.Join(Environment.NewLine, _warningMessages); + markdown += warningMessages; } - return $@"The symbol package [{_symbolPackage.Id} {_symbolPackage.Version}]({_packageUrl}) was recently published on {_configuration.GalleryOwner.DisplayName} by {_symbolPackage.Package.User.Username}. If this was not intended, please [contact support]({_packageSupportUrl}). -{warningMessagesPlaceholder} + string body; + switch (format) + { + case EmailFormat.PlainText: + body = ToPlainText(markdown); + break; + case EmailFormat.Markdown: + body = markdown; + break; + case EmailFormat.Html: + body = Markdown.ToHtml(markdown); + break; + default: + throw new ArgumentOutOfRangeException(nameof(format)); + } ------------------------------------------------ - - To stop receiving emails as an owner of this package, sign in to the {_configuration.GalleryOwner.DisplayName} and - [change your email notification settings]({_emailSettingsUrl}). -"; + return body + EmailMessageFooter.ForPackageOwnerNotifications(format, _configuration.GalleryOwner.DisplayName, _emailSettingsUrl); } - protected override string GetPlainTextBody() + private string GetWarningMessages() { - // The HTML emphasis tag is not supported by the Plain Text renderer in Markdig. - // Manually overriding this one. var warningMessagesPlaceholder = string.Empty; if (_hasWarnings) { - warningMessagesPlaceholder = Environment.NewLine + string.Join(Environment.NewLine, _warningMessages); + warningMessagesPlaceholder = Environment.NewLine + Environment.NewLine + string.Join(Environment.NewLine, _warningMessages); } - - return $@"The symbol package {_symbolPackage.Id} {_symbolPackage.Version} ({_packageUrl}) was recently published on {_configuration.GalleryOwner.DisplayName} by {_symbolPackage.Package.User.Username}. If this was not intended, please contact support ({_packageSupportUrl}). -{warningMessagesPlaceholder} - ------------------------------------------------ - To stop receiving emails as an owner of this package, sign in to the {_configuration.GalleryOwner.DisplayName} and - change your email notification settings ({_emailSettingsUrl})."; + return warningMessagesPlaceholder; } } } diff --git a/src/NuGetGallery.Core/NuGetGallery.Core.csproj b/src/NuGetGallery.Core/NuGetGallery.Core.csproj index 86de77f526..ffd09c1299 100644 --- a/src/NuGetGallery.Core/NuGetGallery.Core.csproj +++ b/src/NuGetGallery.Core/NuGetGallery.Core.csproj @@ -169,9 +169,11 @@ + + @@ -263,6 +265,9 @@ 0.15.3 + + 2.30.0-master-42538 + 1.2.0 @@ -297,10 +302,10 @@ 4.8.0-preview4.5287 - 2.29.0 + 2.30.0-master-42538 - 2.29.0 + 2.30.0-master-42538 7.1.2 diff --git a/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs b/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs index bbe5871368..87f172e374 100644 --- a/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs +++ b/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs @@ -18,8 +18,11 @@ using Autofac; using Autofac.Core; using Elmah; +using Microsoft.Extensions.Logging; using Microsoft.WindowsAzure.ServiceRuntime; using NuGet.Services.KeyVault; +using NuGet.Services.Logging; +using NuGet.Services.Messaging; using NuGet.Services.ServiceBus; using NuGet.Services.Sql; using NuGet.Services.Validation; @@ -48,6 +51,7 @@ public static class BindingKeys public const string SymbolsPackageValidationTopic = "SymbolsPackageValidationBindingKey"; public const string PackageValidationEnqueuer = "PackageValidationEnqueuerBindingKey"; public const string SymbolsPackageValidationEnqueuer = "SymbolsPackageValidationEnqueuerBindingKey"; + public const string EmailPublisherTopic = "EmailPublisherBindingKey"; } protected override void Load(ContainerBuilder builder) @@ -376,6 +380,20 @@ protected override void Load(ContainerBuilder builder) } private static void RegisterMessagingService(ContainerBuilder builder, ConfigurationService configuration) + { + if (configuration.Current.AsynchronousEmailServiceEnabled) + { + // Register NuGet.Services.Messaging infrastructure + RegisterAsynchronousEmailMessagingService(builder, configuration); + } + else + { + // Register legacy SMTP messaging infrastructure + RegisterSmtpEmailMessagingService(builder, configuration); + } + } + + private static void RegisterSmtpEmailMessagingService(ContainerBuilder builder, ConfigurationService configuration) { MailSender mailSenderFactory() { @@ -425,6 +443,42 @@ MailSender mailSenderFactory() .InstancePerDependency(); } + private static void RegisterAsynchronousEmailMessagingService(ContainerBuilder builder, ConfigurationService configuration) + { + builder + .RegisterType() + .As(); + + var emailPublisherConnectionString = configuration.ServiceBus.EmailPublisher_ConnectionString; + var emailPublisherTopicName = configuration.ServiceBus.EmailPublisher_TopicName; + + builder + .Register(c => new TopicClientWrapper(emailPublisherConnectionString, emailPublisherTopicName)) + .As() + .SingleInstance() + .Keyed(BindingKeys.EmailPublisherTopic) + .OnRelease(x => x.Close()); + + // Create an ILoggerFactory + var loggerConfiguration = LoggingSetup.CreateDefaultLoggerConfiguration(withConsoleLogger: false); + var loggerFactory = LoggingSetup.CreateLoggerFactory(loggerConfiguration); + + builder + .RegisterType() + .WithParameter(new ResolvedParameter( + (pi, ctx) => pi.ParameterType == typeof(ITopicClient), + (pi, ctx) => ctx.ResolveKeyed(BindingKeys.EmailPublisherTopic))) + .WithParameter(new ResolvedParameter( + (pi, ctx) => pi.ParameterType == typeof(ILogger), + (pi, ctx) => loggerFactory.CreateLogger())) + .As(); + + builder.RegisterType() + .AsSelf() + .As() + .InstancePerDependency(); + } + private static ISqlConnectionFactory CreateDbConnectionFactory(IDiagnosticsService diagnostics, string name, string connectionString, ISecretInjector secretInjector) { @@ -467,8 +521,8 @@ private void RegisterAsynchronousValidation(ContainerBuilder builder, IDiagnosti ConfigurationService configuration, ISecretInjector secretInjector) { builder - .RegisterType() - .As(); + .RegisterType() + .As(); // We need to setup two enqueuers for Package validation and symbol validation each publishes // to a different topic for validation. diff --git a/src/NuGetGallery/Configuration/AppConfiguration.cs b/src/NuGetGallery/Configuration/AppConfiguration.cs index ef6a1809e5..bb89130271 100644 --- a/src/NuGetGallery/Configuration/AppConfiguration.cs +++ b/src/NuGetGallery/Configuration/AppConfiguration.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// 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; @@ -353,6 +353,8 @@ public string ExternalBrandingMessage [TypeConverter(typeof(StringArrayConverter))] public string[] RedirectedCuratedFeeds { get; set; } + public bool AsynchronousEmailServiceEnabled { get; set; } + [DefaultValue(false)] public bool RejectPackagesWithLicense { get; set; } } diff --git a/src/NuGetGallery/Configuration/IAppConfiguration.cs b/src/NuGetGallery/Configuration/IAppConfiguration.cs index 1d422c4358..57caa1596b 100644 --- a/src/NuGetGallery/Configuration/IAppConfiguration.cs +++ b/src/NuGetGallery/Configuration/IAppConfiguration.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// 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; @@ -361,6 +361,11 @@ public interface IAppConfiguration : IMessageServiceConfiguration /// string[] RedirectedCuratedFeeds { get; set; } + /// + /// Gets or sets a flag indicating whether asynchronous email service is enabled. + /// + bool AsynchronousEmailServiceEnabled { get; set; } + /// /// Flag that indicates whether packages with `license` node in them should be rejected. /// diff --git a/src/NuGetGallery/Configuration/IServiceBusConfiguration.cs b/src/NuGetGallery/Configuration/IServiceBusConfiguration.cs index e169e47d1f..55a71b657e 100644 --- a/src/NuGetGallery/Configuration/IServiceBusConfiguration.cs +++ b/src/NuGetGallery/Configuration/IServiceBusConfiguration.cs @@ -35,5 +35,19 @@ public interface IServiceBusConfiguration /// time as the . /// string SymbolsValidation_TopicName { get; set; } + + /// + /// The connection string to use when connecting to the email publisher topic. This connection string should not + /// contain the topic name as the name is explicitly specified by . This + /// connection string only needs to have the "Send" privilege. This topic is used for requesting asynchronous + /// publishing of email messages. + /// + string EmailPublisher_ConnectionString { get; set; } + + /// + /// The name of the Azure Service Bus topic to send email messages to. This topic name is used at the same + /// time as the . + /// + string EmailPublisher_TopicName { get; set; } } } \ No newline at end of file diff --git a/src/NuGetGallery/Configuration/ServiceBusConfiguration.cs b/src/NuGetGallery/Configuration/ServiceBusConfiguration.cs index 5ef8e336f7..9087013df0 100644 --- a/src/NuGetGallery/Configuration/ServiceBusConfiguration.cs +++ b/src/NuGetGallery/Configuration/ServiceBusConfiguration.cs @@ -18,5 +18,11 @@ public class ServiceBusConfiguration : IServiceBusConfiguration [DisplayName("SymbolsValidation.TopicName")] public string SymbolsValidation_TopicName { get; set; } + + [DisplayName("EmailPublisher.ConnectionString")] + public string EmailPublisher_ConnectionString { get; set; } + + [DisplayName("EmailPublisher.TopicName")] + public string EmailPublisher_TopicName { get; set; } } } \ No newline at end of file diff --git a/src/NuGetGallery/Infrastructure/Mail/Messages/ContactOwnersMessage.cs b/src/NuGetGallery/Infrastructure/Mail/Messages/ContactOwnersMessage.cs index 7d07d6ce87..4c85e7bda9 100644 --- a/src/NuGetGallery/Infrastructure/Mail/Messages/ContactOwnersMessage.cs +++ b/src/NuGetGallery/Infrastructure/Mail/Messages/ContactOwnersMessage.cs @@ -2,8 +2,8 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Globalization; using System.Net.Mail; +using Markdig; namespace NuGetGallery.Infrastructure.Mail.Messages { @@ -55,47 +55,42 @@ public override string GetSubject() protected override string GetMarkdownBody() { - var bodyTemplate = @"_User {0} <{1}> sends the following message to the owners of Package '[{2} {3}]({4})'._ - -{5} - ------------------------------------------------ - - To stop receiving contact emails as an owner of this package, sign in to the {6} and - [change your email notification settings]({7}). -"; - - return GetBodyInternal(bodyTemplate); + return GetBodyInternal(EmailFormat.Markdown); } protected override string GetPlainTextBody() { - // The HTML emphasis tag is not supported by the Plain Text renderer in Markdig. - // Manually overriding this one. - var bodyTemplate = @"User {0} <{1}> sends the following message to the owners of Package '{2} {3} ({4})'. - -{5} - ------------------------------------------------ - To stop receiving contact emails as an owner of this package, sign in to the {6} and - change your email notification settings ({7})."; + return GetBodyInternal(EmailFormat.PlainText); + } - return GetBodyInternal(bodyTemplate); + protected override string GetHtmlBody() + { + return GetBodyInternal(EmailFormat.Html); } - private string GetBodyInternal(string template) + private string GetBodyInternal(EmailFormat format) { - return string.Format( - CultureInfo.CurrentCulture, - template, - FromAddress.DisplayName, - FromAddress.Address, - Package.PackageRegistration.Id, - Package.Version, - PackageUrl, - HtmlEncodedMessage, - _configuration.GalleryOwner.DisplayName, - EmailSettingsUrl); + var markdown = $@"_User {FromAddress.DisplayName} <{FromAddress.Address}> sends the following message to the owners of Package '[{Package.PackageRegistration.Id} {Package.Version}]({PackageUrl})'._ + +{HtmlEncodedMessage}"; + + string body; + switch (format) + { + case EmailFormat.PlainText: + body = ToPlainText(markdown); + break; + case EmailFormat.Markdown: + body = markdown; + break; + case EmailFormat.Html: + body = Markdown.ToHtml(markdown); + break; + default: + throw new ArgumentOutOfRangeException(nameof(format)); + } + + return body + EmailMessageFooter.ForContactOwnerNotifications(format, _configuration.GalleryOwner.DisplayName, EmailSettingsUrl); } } } diff --git a/src/NuGetGallery/NuGetGallery.csproj b/src/NuGetGallery/NuGetGallery.csproj index c28742b65c..9dab7102e6 100644 --- a/src/NuGetGallery/NuGetGallery.csproj +++ b/src/NuGetGallery/NuGetGallery.csproj @@ -1975,6 +1975,9 @@ 0.15.3 + + 2.30.0-master-42538 + 1.2.0 diff --git a/src/NuGetGallery/Web.config b/src/NuGetGallery/Web.config index ab92d12119..4a1907fe20 100644 --- a/src/NuGetGallery/Web.config +++ b/src/NuGetGallery/Web.config @@ -45,6 +45,8 @@ + + + + diff --git a/src/NuGetGallery/Areas/Admin/DynamicData/web.config b/src/NuGetGallery/Areas/Admin/DynamicData/web.config index 698932606a..197dadb820 100644 --- a/src/NuGetGallery/Areas/Admin/DynamicData/web.config +++ b/src/NuGetGallery/Areas/Admin/DynamicData/web.config @@ -5,6 +5,7 @@ --> + diff --git a/src/NuGetGallery/Areas/Admin/Views/Web.config b/src/NuGetGallery/Areas/Admin/Views/Web.config index 0ae4425dd6..728a77dd4c 100644 --- a/src/NuGetGallery/Areas/Admin/Views/Web.config +++ b/src/NuGetGallery/Areas/Admin/Views/Web.config @@ -28,6 +28,7 @@ + diff --git a/src/NuGetGallery/Branding/Views/web.config b/src/NuGetGallery/Branding/Views/web.config index cd9418fabf..90a74379c7 100644 --- a/src/NuGetGallery/Branding/Views/web.config +++ b/src/NuGetGallery/Branding/Views/web.config @@ -29,6 +29,7 @@ + diff --git a/src/NuGetGallery/Views/web.config b/src/NuGetGallery/Views/web.config index 64ba5773a8..6f4697f730 100644 --- a/src/NuGetGallery/Views/web.config +++ b/src/NuGetGallery/Views/web.config @@ -30,6 +30,7 @@ + diff --git a/src/NuGetGallery/Web.config b/src/NuGetGallery/Web.config index 9f378de4a2..23e89ca392 100644 --- a/src/NuGetGallery/Web.config +++ b/src/NuGetGallery/Web.config @@ -292,7 +292,7 @@ --> - + diff --git a/src/NuGetGallery/api/Web.config b/src/NuGetGallery/api/Web.config index dd1ecb20e7..8221d55123 100644 --- a/src/NuGetGallery/api/Web.config +++ b/src/NuGetGallery/api/Web.config @@ -3,4 +3,7 @@ + + + From 75d53d361a9a4677d87a1b8a9101b85e2291db69 Mon Sep 17 00:00:00 2001 From: Xavier Decoster Date: Wed, 24 Oct 2018 20:12:12 +0200 Subject: [PATCH 08/20] Aligning all ServerCommon dependencies to the same version to fix assembly version loading issues (#6589) --- src/NuGetGallery.Core/NuGetGallery.Core.csproj | 4 ++-- src/NuGetGallery/NuGetGallery.csproj | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/NuGetGallery.Core/NuGetGallery.Core.csproj b/src/NuGetGallery.Core/NuGetGallery.Core.csproj index 018491b94f..9714e81006 100644 --- a/src/NuGetGallery.Core/NuGetGallery.Core.csproj +++ b/src/NuGetGallery.Core/NuGetGallery.Core.csproj @@ -241,10 +241,10 @@ 4.8.0 - 2.30.0-master-42538 + 2.30.0-master-42731 - 2.30.0-master-42538 + 2.30.0-master-42731 7.1.2 diff --git a/src/NuGetGallery/NuGetGallery.csproj b/src/NuGetGallery/NuGetGallery.csproj index 4ec8a96739..eafcecb976 100644 --- a/src/NuGetGallery/NuGetGallery.csproj +++ b/src/NuGetGallery/NuGetGallery.csproj @@ -2210,16 +2210,16 @@ 4.8.0 - 2.27.0 + 2.30.0-master-42731 - 2.2.3 + 2.30.0-master-42731 - 2.2.3 + 2.30.0-master-42731 - 2.27.0 + 2.30.0-master-42731 1.0.0 From 166be050eae38e264c056328436e7d788b374e3f Mon Sep 17 00:00:00 2001 From: Shishir H Date: Thu, 25 Oct 2018 11:14:47 -0700 Subject: [PATCH 09/20] Fix versioning issue (#6595) --- src/NuGetGallery/Web.config | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/NuGetGallery/Web.config b/src/NuGetGallery/Web.config index 23e89ca392..73e41f0e01 100644 --- a/src/NuGetGallery/Web.config +++ b/src/NuGetGallery/Web.config @@ -115,7 +115,7 @@ - + + @@ -366,8 +367,10 @@ + + From dc86fe8cf6edd140340fce1244ff1be4596029d1 Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Wed, 24 Oct 2018 08:47:50 -0700 Subject: [PATCH 13/20] Add custom query telemetry and response header to OData endpoint (#6583) Progress on https://github.com/NuGet/Engineering/issues/1798 --- .../Controllers/ODataV1FeedController.cs | 48 ++- .../ODataV2CuratedFeedController.cs | 74 ++-- .../Controllers/ODataV2FeedController.cs | 99 +++-- src/NuGetGallery/GalleryConstants.cs | 11 + .../OData/NuGetODataController.cs | 31 +- .../Services/ITelemetryService.cs | 2 + src/NuGetGallery/Services/TelemetryService.cs | 14 +- src/NuGetGallery/WebApi/QueryResult.cs | 35 +- .../ODataFeedControllerFactsBase.cs | 10 +- .../Controllers/ODataV1FeedControllerFacts.cs | 13 +- .../ODataV2CuratedFeedControllerFacts.cs | 11 +- .../Controllers/ODataV2FeedControllerFacts.cs | 9 +- .../Services/FeedServiceFacts.cs | 374 +++++++++++++++++- .../Services/TelemetryServiceFacts.cs | 25 ++ .../Infrastructure/TestableV1Feed.cs | 18 +- .../Infrastructure/TestableV2Feed.cs | 12 +- 16 files changed, 687 insertions(+), 99 deletions(-) diff --git a/src/NuGetGallery/Controllers/ODataV1FeedController.cs b/src/NuGetGallery/Controllers/ODataV1FeedController.cs index 33af4002a6..0beaa0393e 100644 --- a/src/NuGetGallery/Controllers/ODataV1FeedController.cs +++ b/src/NuGetGallery/Controllers/ODataV1FeedController.cs @@ -30,8 +30,9 @@ public class ODataV1FeedController public ODataV1FeedController( IEntityRepository packagesRepository, IGalleryConfigurationService configurationService, - ISearchService searchService) - : base(configurationService) + ISearchService searchService, + ITelemetryService telemetryService) + : base(configurationService, telemetryService) { _packagesRepository = packagesRepository; _configurationService = configurationService; @@ -56,7 +57,7 @@ public IHttpActionResult Get(ODataQueryOptions options) .WithoutSortOnColumn(Id, ShouldIgnoreOrderById(options)) .ToV1FeedPackageQuery(_configurationService.GetSiteRoot(UseHttps())); - return QueryResult(options, queryable, MaxPageSize); + return TrackedQueryResult(options, queryable, MaxPageSize, customQuery: true); } // /api/v1/Packages/$count @@ -108,6 +109,8 @@ private async Task GetCore(ODataQueryOptions o packages = packages.Where(p => p.Version == version); } + bool? customQuery = null; + // try the search service try { @@ -123,6 +126,8 @@ private async Task GetCore(ODataQueryOptions o // If intercepted, create a paged queryresult if (searchAdaptorResult.ResultsAreProvidedBySearchService) { + customQuery = false; + // Packages provided by search service packages = searchAdaptorResult.Packages; @@ -131,6 +136,7 @@ private async Task GetCore(ODataQueryOptions o if (return404NotFoundWhenNoResults && totalHits == 0) { + _telemetryService.TrackODataCustomQuery(customQuery); return NotFound(); } @@ -138,8 +144,17 @@ private async Task GetCore(ODataQueryOptions o .Take(options.Top != null ? Math.Min(options.Top.Value, MaxPageSize) : MaxPageSize) .ToV1FeedPackageQuery(GetSiteRoot()); - return QueryResult(options, pagedQueryable, MaxPageSize, totalHits, (o, s, resultCount) => - SearchAdaptor.GetNextLink(Request.RequestUri, resultCount, new { id }, o, s)); + return TrackedQueryResult( + options, + pagedQueryable, + MaxPageSize, + totalHits, + (o, s, resultCount) => SearchAdaptor.GetNextLink(Request.RequestUri, resultCount, new { id }, o, s), + customQuery); + } + else + { + customQuery = true; } } catch (Exception ex) @@ -151,11 +166,12 @@ private async Task GetCore(ODataQueryOptions o if (return404NotFoundWhenNoResults && !packages.Any()) { + _telemetryService.TrackODataCustomQuery(customQuery); return NotFound(); } var queryable = packages.ToV1FeedPackageQuery(GetSiteRoot()); - return QueryResult(options, queryable, MaxPageSize); + return TrackedQueryResult(options, queryable, MaxPageSize, customQuery); } // /api/v1/Packages(Id=,Version=)/propertyName @@ -213,24 +229,36 @@ public async Task Search( packages, searchTerm, targetFramework, - false, + includePrerelease: false, curatedFeed: null, semVerLevel: null); // Packages provided by search service (even when not hijacked) var query = searchAdaptorResult.Packages; + bool? customQuery = null; // If intercepted, create a paged queryresult if (searchAdaptorResult.ResultsAreProvidedBySearchService) { + customQuery = false; + // Add explicit Take() needed to limit search hijack result set size if $top is specified var totalHits = query.LongCount(); var pagedQueryable = query .Take(options.Top != null ? Math.Min(options.Top.Value, MaxPageSize) : MaxPageSize) .ToV1FeedPackageQuery(GetSiteRoot()); - return QueryResult(options, pagedQueryable, MaxPageSize, totalHits, (o, s, resultCount) => - SearchAdaptor.GetNextLink(Request.RequestUri, resultCount, new { searchTerm, targetFramework }, o, s)); + return TrackedQueryResult( + options, + pagedQueryable, + MaxPageSize, + totalHits, + (o, s, resultCount) => SearchAdaptor.GetNextLink(Request.RequestUri, resultCount, new { searchTerm, targetFramework }, o, s), + customQuery); + } + else + { + customQuery = true; } if (!ODataQueryVerifier.AreODataOptionsAllowed(options, ODataQueryVerifier.V1Search, @@ -241,7 +269,7 @@ public async Task Search( // If not, just let OData handle things var queryable = query.ToV1FeedPackageQuery(GetSiteRoot()); - return QueryResult(options, queryable, MaxPageSize); + return TrackedQueryResult(options, queryable, MaxPageSize, customQuery); } // /api/v1/Search()/$count?searchTerm=&targetFramework=&includePrerelease= diff --git a/src/NuGetGallery/Controllers/ODataV2CuratedFeedController.cs b/src/NuGetGallery/Controllers/ODataV2CuratedFeedController.cs index 9c704fa928..7854c912ac 100644 --- a/src/NuGetGallery/Controllers/ODataV2CuratedFeedController.cs +++ b/src/NuGetGallery/Controllers/ODataV2CuratedFeedController.cs @@ -32,8 +32,9 @@ public ODataV2CuratedFeedController( IGalleryConfigurationService configurationService, ISearchService searchService, ICuratedFeedService curatedFeedService, - IEntityRepository packagesRepository) - : base(configurationService) + IEntityRepository packagesRepository, + ITelemetryService telemetryService) + : base(configurationService, telemetryService) { _configurationService = configurationService; _searchService = searchService; @@ -68,7 +69,7 @@ public IHttpActionResult Get( semVerLevelKey) .InterceptWith(new NormalizeVersionInterceptor()); - return QueryResult(options, queryable, MaxPageSize); + return TrackedQueryResult(options, queryable, MaxPageSize, customQuery: true); } // /api/v2/curated-feed/curatedFeedName/Packages/$count?semVerLevel= @@ -108,7 +109,7 @@ public async Task FindPackagesById( var emptyResult = Enumerable.Empty().AsQueryable() .ToV2FeedPackageQuery(GetSiteRoot(), _configurationService.Features.FriendlyLicenses, semVerLevelKey); - return QueryResult(options, emptyResult, MaxPageSize); + return TrackedQueryResult(options, emptyResult, MaxPageSize, customQuery: false); } return await GetCore(options, curatedFeedName, id, normalizedVersion: null, return404NotFoundWhenNoResults: false, semVerLevel: semVerLevel); @@ -153,6 +154,7 @@ private async Task GetCore( } var semVerLevelKey = SemVerLevelKey.ForSemVerLevel(semVerLevel); + bool? customQuery = null; // try the search service try @@ -169,6 +171,8 @@ private async Task GetCore( // If intercepted, create a paged queryresult if (searchAdaptorResult.ResultsAreProvidedBySearchService) { + customQuery = false; + // Packages provided by search service packages = searchAdaptorResult.Packages; @@ -177,6 +181,7 @@ private async Task GetCore( if (return404NotFoundWhenNoResults && totalHits == 0) { + _telemetryService.TrackODataCustomQuery(customQuery); return NotFound(); } @@ -184,8 +189,17 @@ private async Task GetCore( .Take(options.Top != null ? Math.Min(options.Top.Value, MaxPageSize) : MaxPageSize) .ToV2FeedPackageQuery(GetSiteRoot(), _configurationService.Features.FriendlyLicenses, semVerLevelKey); - return QueryResult(options, pagedQueryable, MaxPageSize, totalHits, (o, s, resultCount) => - SearchAdaptor.GetNextLink(Request.RequestUri, resultCount, new { id }, o, s, semVerLevelKey)); + return TrackedQueryResult( + options, + pagedQueryable, + MaxPageSize, + totalHits, + (o, s, resultCount) => SearchAdaptor.GetNextLink(Request.RequestUri, resultCount, new { id }, o, s, semVerLevelKey), + customQuery); + } + else + { + customQuery = true; } } catch (Exception ex) @@ -197,6 +211,7 @@ private async Task GetCore( if (return404NotFoundWhenNoResults && !packages.Any()) { + _telemetryService.TrackODataCustomQuery(customQuery); return NotFound(); } @@ -205,7 +220,7 @@ private async Task GetCore( _configurationService.Features.FriendlyLicenses, semVerLevelKey); - return QueryResult(options, queryable, MaxPageSize); + return TrackedQueryResult(options, queryable, MaxPageSize, customQuery); } // /api/v2/curated-feed/curatedFeedName/Packages(Id=,Version=)/propertyName @@ -278,10 +293,13 @@ public async Task Search( var query = searchAdaptorResult.Packages; var semVerLevelKey = SemVerLevelKey.ForSemVerLevel(semVerLevel); + bool? customQuery = null; // If intercepted, create a paged queryresult if (searchAdaptorResult.ResultsAreProvidedBySearchService) { + customQuery = false; + // Add explicit Take() needed to limit search hijack result set size if $top is specified var totalHits = query.LongCount(); var pagedQueryable = query @@ -291,22 +309,32 @@ public async Task Search( _configurationService.Features.FriendlyLicenses, semVerLevelKey); - return QueryResult(options, pagedQueryable, MaxPageSize, totalHits, (o, s, resultCount) => - { - // The nuget.exe 2.x list command does not like the next link at the bottom when a $top is passed. - // Strip it of for backward compatibility. - if (o.Top == null || (resultCount.HasValue && o.Top.Value >= resultCount.Value)) + return TrackedQueryResult( + options, + pagedQueryable, + MaxPageSize, + totalHits, + (o, s, resultCount) => { - return SearchAdaptor.GetNextLink( - Request.RequestUri, - resultCount, - new { searchTerm, targetFramework, includePrerelease }, - o, - s, - semVerLevelKey); - } - return null; - }); + // The nuget.exe 2.x list command does not like the next link at the bottom when a $top is passed. + // Strip it of for backward compatibility. + if (o.Top == null || (resultCount.HasValue && o.Top.Value >= resultCount.Value)) + { + return SearchAdaptor.GetNextLink( + Request.RequestUri, + resultCount, + new { searchTerm, targetFramework, includePrerelease }, + o, + s, + semVerLevelKey); + } + return null; + }, + customQuery); + } + else + { + customQuery = true; } // If not, just let OData handle things @@ -315,7 +343,7 @@ public async Task Search( _configurationService.Features.FriendlyLicenses, semVerLevelKey); - return QueryResult(options, queryable, MaxPageSize); + return TrackedQueryResult(options, queryable, MaxPageSize, customQuery); } // /api/v2/curated-feed/curatedFeedName/Search()/$count?searchTerm=&targetFramework=&includePrerelease= diff --git a/src/NuGetGallery/Controllers/ODataV2FeedController.cs b/src/NuGetGallery/Controllers/ODataV2FeedController.cs index 5bb80ab714..72a311d9b2 100644 --- a/src/NuGetGallery/Controllers/ODataV2FeedController.cs +++ b/src/NuGetGallery/Controllers/ODataV2FeedController.cs @@ -36,8 +36,9 @@ public class ODataV2FeedController public ODataV2FeedController( IEntityRepository packagesRepository, IGalleryConfigurationService configurationService, - ISearchService searchService) - : base(configurationService) + ISearchService searchService, + ITelemetryService telemetryService) + : base(configurationService, telemetryService) { _packagesRepository = packagesRepository; _configurationService = configurationService; @@ -61,6 +62,7 @@ public async Task Get( .InterceptWith(new NormalizeVersionInterceptor()); var semVerLevelKey = SemVerLevelKey.ForSemVerLevel(semVerLevel); + bool? customQuery = null; // Try the search service try @@ -80,6 +82,8 @@ public async Task Get( // If intercepted, create a paged queryresult if (searchAdaptorResult.ResultsAreProvidedBySearchService) { + customQuery = false; + // Packages provided by search service packages = searchAdaptorResult.Packages; @@ -92,10 +96,23 @@ public async Task Get( _configurationService.Features.FriendlyLicenses, semVerLevelKey); - return QueryResult(options, pagedQueryable, MaxPageSize, totalHits, (o, s, resultCount) => - SearchAdaptor.GetNextLink(Request.RequestUri, resultCount, null, o, s, semVerLevelKey)); + return TrackedQueryResult( + options, + pagedQueryable, + MaxPageSize, + totalHits, + (o, s, resultCount) => SearchAdaptor.GetNextLink(Request.RequestUri, resultCount, null, o, s, semVerLevelKey), + customQuery); + } + else + { + customQuery = true; } } + else + { + customQuery = true; + } } catch (Exception ex) { @@ -116,7 +133,7 @@ public async Task Get( _configurationService.Features.FriendlyLicenses, semVerLevelKey); - return QueryResult(options, queryable, MaxPageSize); + return TrackedQueryResult(options, queryable, MaxPageSize, customQuery); } // /api/v2/Packages/$count?semVerLevel= @@ -169,7 +186,7 @@ public async Task FindPackagesById( _configurationService.Features.FriendlyLicenses, semVerLevelKey); - return QueryResult(options, emptyResult, MaxPageSize); + return TrackedQueryResult(options, emptyResult, MaxPageSize, customQuery: false); } return await GetCore( @@ -217,6 +234,7 @@ private async Task GetCore( } var semVerLevelKey = SemVerLevelKey.ForSemVerLevel(semVerLevel); + bool? customQuery = null; // try the search service try @@ -233,6 +251,8 @@ private async Task GetCore( // If intercepted, create a paged queryresult if (searchAdaptorResult.ResultsAreProvidedBySearchService) { + customQuery = false; + // Packages provided by search service packages = searchAdaptorResult.Packages; @@ -241,6 +261,7 @@ private async Task GetCore( if (totalHits == 0 && return404NotFoundWhenNoResults) { + _telemetryService.TrackODataCustomQuery(customQuery); return NotFound(); } @@ -251,8 +272,17 @@ private async Task GetCore( _configurationService.Features.FriendlyLicenses, semVerLevelKey); - return QueryResult(options, pagedQueryable, MaxPageSize, totalHits, (o, s, resultCount) => - SearchAdaptor.GetNextLink(Request.RequestUri, resultCount, new { id }, o, s, semVerLevelKey)); + return TrackedQueryResult( + options, + pagedQueryable, + MaxPageSize, + totalHits, + (o, s, resultCount) => SearchAdaptor.GetNextLink(Request.RequestUri, resultCount, new { id }, o, s, semVerLevelKey), + customQuery); + } + else + { + customQuery = true; } } catch (Exception ex) @@ -264,6 +294,7 @@ private async Task GetCore( if (return404NotFoundWhenNoResults && !packages.Any()) { + _telemetryService.TrackODataCustomQuery(customQuery); return NotFound(); } @@ -272,7 +303,7 @@ private async Task GetCore( _configurationService.Features.FriendlyLicenses, semVerLevelKey); - return QueryResult(options, queryable, MaxPageSize); + return TrackedQueryResult(options, queryable, MaxPageSize, customQuery); } // /api/v2/Packages(Id=,Version=)/propertyName @@ -340,10 +371,13 @@ public async Task Search( var query = searchAdaptorResult.Packages; var semVerLevelKey = SemVerLevelKey.ForSemVerLevel(semVerLevel); + bool? customQuery = null; // If intercepted, create a paged queryresult if (searchAdaptorResult.ResultsAreProvidedBySearchService) { + customQuery = false; + // Add explicit Take() needed to limit search hijack result set size if $top is specified var totalHits = query.LongCount(); var pagedQueryable = query @@ -353,17 +387,28 @@ public async Task Search( _configurationService.Features.FriendlyLicenses, semVerLevelKey); - return QueryResult(options, pagedQueryable, MaxPageSize, totalHits, (o, s, resultCount) => - { - // The nuget.exe 2.x list command does not like the next link at the bottom when a $top is passed. - // Strip it of for backward compatibility. - if (o.Top == null || (resultCount.HasValue && o.Top.Value >= resultCount.Value)) - { - return SearchAdaptor.GetNextLink(Request.RequestUri, resultCount, new { searchTerm, targetFramework, includePrerelease }, o, s, semVerLevelKey); - } - return null; - }); + return TrackedQueryResult( + options, + pagedQueryable, + MaxPageSize, + totalHits, + (o, s, resultCount) => + { + // The nuget.exe 2.x list command does not like the next link at the bottom when a $top is passed. + // Strip it of for backward compatibility. + if (o.Top == null || (resultCount.HasValue && o.Top.Value >= resultCount.Value)) + { + return SearchAdaptor.GetNextLink(Request.RequestUri, resultCount, new { searchTerm, targetFramework, includePrerelease }, o, s, semVerLevelKey); + } + return null; + }, + customQuery); + } + else + { + customQuery = true; } + //Reject only when try to reach database. if (!ODataQueryVerifier.AreODataOptionsAllowed(options, ODataQueryVerifier.V2Search, _configurationService.Current.IsODataFilterEnabled, nameof(Search))) @@ -377,7 +422,7 @@ public async Task Search( _configurationService.Features.FriendlyLicenses, semVerLevelKey); - return QueryResult(options, queryable, MaxPageSize); + return TrackedQueryResult(options, queryable, MaxPageSize, customQuery); } // /api/v2/Search()/$count?searchTerm=&targetFramework=&includePrerelease=&semVerLevel= @@ -414,7 +459,11 @@ public IHttpActionResult GetUpdates( { if (string.IsNullOrEmpty(packageIds) || string.IsNullOrEmpty(versions)) { - return Ok(Enumerable.Empty().AsQueryable()); + return TrackedQueryResult( + options, + Enumerable.Empty().AsQueryable(), + MaxPageSize, + customQuery: false); } if (!ODataQueryVerifier.AreODataOptionsAllowed(options, ODataQueryVerifier.V2GetUpdates, @@ -444,7 +493,11 @@ public IHttpActionResult GetUpdates( if (idValues.Length == 0 || idValues.Length != versionValues.Length || idValues.Length != versionConstraintValues.Length) { // Exit early if the request looks invalid - return Ok(Enumerable.Empty().AsQueryable()); + return TrackedQueryResult( + options, + Enumerable.Empty().AsQueryable(), + MaxPageSize, + customQuery: false); } var versionLookup = idValues.Select((id, i) => @@ -483,7 +536,7 @@ public IHttpActionResult GetUpdates( _configurationService.Features.FriendlyLicenses, semVerLevelKey); - return QueryResult(options, queryable, MaxPageSize); + return TrackedQueryResult(options, queryable, MaxPageSize, customQuery: false); } // /api/v2/GetUpdates()/$count?packageIds=&versions=&includePrerelease=&includeAllVersions=&targetFrameworks=&versionConstraints=&semVerLevel= diff --git a/src/NuGetGallery/GalleryConstants.cs b/src/NuGetGallery/GalleryConstants.cs index 3035853d28..fb472f53e6 100644 --- a/src/NuGetGallery/GalleryConstants.cs +++ b/src/NuGetGallery/GalleryConstants.cs @@ -87,6 +87,17 @@ public static class GalleryConstants internal const string NuGetProtocolHeaderName = "X-NuGet-Protocol-Version"; internal const string WarningHeaderName = "X-NuGet-Warning"; internal const string UserAgentHeaderName = "User-Agent"; + + /// + /// This header is for internal use only. It indicates whether an OData query is "custom". Custom means not + /// not optimized for search hijacking. Queries made by the official client should be optimized and therefore + /// not marked as custom queries (as to not overload the database). The value is either "true" or "false". If + /// the header is not present on an OData query response, that means that the search hijack detection is + /// failing, perhaps due to search service outage. The value of this header corresponds to the + /// telemetry emitted while generating the + /// response. + /// + internal const string CustomQueryHeaderName = "X-NuGet-CustomQuery"; public static readonly string ReturnUrlParameterName = "ReturnUrl"; public static readonly string CurrentUserOwinEnvironmentKey = "nuget.user"; diff --git a/src/NuGetGallery/OData/NuGetODataController.cs b/src/NuGetGallery/OData/NuGetODataController.cs index 1d6c4cb683..66683bb797 100644 --- a/src/NuGetGallery/OData/NuGetODataController.cs +++ b/src/NuGetGallery/OData/NuGetODataController.cs @@ -16,12 +16,17 @@ public abstract class NuGetODataController : ODataController { private readonly IGalleryConfigurationService _configurationService; + protected readonly ITelemetryService _telemetryService; + protected const string Id = "Id"; protected const string Version = "Version"; - protected NuGetODataController(IGalleryConfigurationService configurationService) + protected NuGetODataController( + IGalleryConfigurationService configurationService, + ITelemetryService telemetryService) { - _configurationService = configurationService; + _configurationService = configurationService ?? throw new ArgumentNullException(nameof(configurationService)); + _telemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService)); } protected virtual HttpContextBase GetTraditionalHttpContext() @@ -60,9 +65,15 @@ protected virtual string GetSiteRoot() /// Queryable to build QueryResult from. /// Maximum page size. /// A QueryResult instance. - protected virtual IHttpActionResult QueryResult(ODataQueryOptions options, IQueryable queryable, int maxPageSize) + protected virtual IHttpActionResult TrackedQueryResult( + ODataQueryOptions options, + IQueryable queryable, + int maxPageSize, + bool? customQuery) { - return new QueryResult(options, queryable, this, maxPageSize); + _telemetryService.TrackODataCustomQuery(customQuery); + + return new QueryResult(options, queryable, this, maxPageSize, customQuery); } /// @@ -76,9 +87,17 @@ protected virtual IHttpActionResult QueryResult(ODataQueryOptionsThe total number of results. This number can be larger than the size of the page being served. /// Function that generates a next link. /// A QueryResult instance. - protected virtual IHttpActionResult QueryResult(ODataQueryOptions options, IQueryable queryable, int maxPageSize, long totalResults, Func, ODataQuerySettings, long?, Uri> generateNextLink) + protected virtual IHttpActionResult TrackedQueryResult( + ODataQueryOptions options, + IQueryable queryable, + int maxPageSize, + long totalResults, + Func, ODataQuerySettings, long?, Uri> generateNextLink, + bool? customQuery) { - return new QueryResult(options, queryable, this, maxPageSize, totalResults, generateNextLink); + _telemetryService.TrackODataCustomQuery(customQuery); + + return new QueryResult(options, queryable, this, maxPageSize, totalResults, generateNextLink, customQuery); } /// diff --git a/src/NuGetGallery/Services/ITelemetryService.cs b/src/NuGetGallery/Services/ITelemetryService.cs index 999e5c1f45..c005fef6b7 100644 --- a/src/NuGetGallery/Services/ITelemetryService.cs +++ b/src/NuGetGallery/Services/ITelemetryService.cs @@ -13,6 +13,8 @@ public interface ITelemetryService { void TrackODataQueryFilterEvent(string callContext, bool isEnabled, bool isAllowed, string queryPattern); + void TrackODataCustomQuery(bool? customQuery); + void TrackPackagePushEvent(Package package, User user, IIdentity identity); void TrackPackagePushFailureEvent(string id, NuGetVersion version); diff --git a/src/NuGetGallery/Services/TelemetryService.cs b/src/NuGetGallery/Services/TelemetryService.cs index c275640b04..895a70c49b 100644 --- a/src/NuGetGallery/Services/TelemetryService.cs +++ b/src/NuGetGallery/Services/TelemetryService.cs @@ -19,6 +19,7 @@ public class TelemetryService : ITelemetryService internal class Events { public const string ODataQueryFilter = "ODataQueryFilter"; + public const string ODataCustomQuery = "ODataCustomQuery"; public const string PackagePush = "PackagePush"; public const string PackagePushFailure = "PackagePushFailure"; public const string CreatePackageVerificationKey = "CreatePackageVerificationKey"; @@ -72,13 +73,16 @@ internal class Events ReferenceLoopHandling = ReferenceLoopHandling.Ignore, Formatting = Formatting.None }; - + // ODataQueryFilter properties public const string CallContext = "CallContext"; public const string IsEnabled = "IsEnabled"; public const string IsAllowed = "IsAllowed"; public const string QueryPattern = "QueryPattern"; + // ODataCustomQuery properties + public const string IsCustomQuery = "IsCustomQuery"; + // Package push properties public const string AuthenticationMethod = "AuthenticationMethod"; public const string ClientVersion = "ClientVersion"; @@ -186,6 +190,14 @@ public void TrackODataQueryFilterEvent(string callContext, bool isEnabled, bool }); } + public void TrackODataCustomQuery(bool? customQuery) + { + TrackMetric(Events.ODataCustomQuery, 1, properties => + { + properties.Add(IsCustomQuery, customQuery?.ToString() ?? "Unknown"); + }); + } + public void TrackPackageReadMeChangeEvent(Package package, string readMeSourceType, PackageEditReadMeState readMeState) { if (package == null) diff --git a/src/NuGetGallery/WebApi/QueryResult.cs b/src/NuGetGallery/WebApi/QueryResult.cs index 5c38bf7e1b..b5425d8a44 100644 --- a/src/NuGetGallery/WebApi/QueryResult.cs +++ b/src/NuGetGallery/WebApi/QueryResult.cs @@ -15,6 +15,7 @@ using System.Web.Http.Results; using Microsoft.Data.OData; using Microsoft.Data.OData.Query; +using NuGet.Services.Entities; using NuGetGallery.OData.QueryFilter; namespace NuGetGallery.WebApi @@ -28,6 +29,7 @@ public class QueryResult private readonly System.Web.Http.ApiController _controller; private readonly long? _totalResults; private readonly Func, ODataQuerySettings, long?, Uri> _generateNextLink; + private readonly bool? _customQuery; private readonly bool _isPagedResult; private readonly int? _semVerLevelKey; @@ -37,20 +39,31 @@ public class QueryResult public bool FormatAsCountResult { get; set; } public bool FormatAsSingleResult { get; set; } - public QueryResult(ODataQueryOptions queryOptions, IQueryable queryable, System.Web.Http.ApiController controller, int maxPageSize) - : this(queryOptions, queryable, controller, maxPageSize, null, null) + public QueryResult( + ODataQueryOptions queryOptions, + IQueryable queryable, + System.Web.Http.ApiController controller, + int maxPageSize, + bool? customQuery) + : this(queryOptions, queryable, controller, maxPageSize, null, null, customQuery) { } public QueryResult( - ODataQueryOptions queryOptions, IQueryable queryable, System.Web.Http.ApiController controller, int maxPageSize, - long? totalResults, Func, ODataQuerySettings, long?, Uri> generateNextLink) + ODataQueryOptions queryOptions, + IQueryable queryable, + System.Web.Http.ApiController controller, + int maxPageSize, + long? totalResults, + Func, ODataQuerySettings, long?, Uri> generateNextLink, + bool? customQuery) { _queryOptions = queryOptions; _queryable = queryable; _controller = controller; _totalResults = totalResults; _generateNextLink = generateNextLink; + _customQuery = customQuery; var queryDictionary = HttpUtility.ParseQueryString(queryOptions.Request.RequestUri.Query); _semVerLevelKey = SemVerLevelKey.ForSemVerLevel(queryDictionary["semVerLevel"]); @@ -75,24 +88,30 @@ public QueryResult( public async Task ExecuteAsync(CancellationToken cancellationToken) { + HttpResponseMessage response; try { - return await GetInnerResult().ExecuteAsync(cancellationToken); + response = await GetInnerResult().ExecuteAsync(cancellationToken); } catch (ODataException e) { - var response = _controller.Request.CreateErrorResponse( + response = _controller.Request.CreateErrorResponse( HttpStatusCode.BadRequest, string.Format(CultureInfo.InvariantCulture, "URI or query string invalid. {0}", e.Message), e); - - return response; } catch (Exception e) { QuietLog.LogHandledException(e); throw; } + + if (_customQuery.HasValue) + { + response.Headers.Add(GalleryConstants.CustomQueryHeaderName, _customQuery.Value ? "true" : "false"); + } + + return response; } public IHttpActionResult GetInnerResult() diff --git a/tests/NuGetGallery.Facts/Controllers/ODataFeedControllerFactsBase.cs b/tests/NuGetGallery.Facts/Controllers/ODataFeedControllerFactsBase.cs index 296e2398a7..296ea4fa92 100644 --- a/tests/NuGetGallery.Facts/Controllers/ODataFeedControllerFactsBase.cs +++ b/tests/NuGetGallery.Facts/Controllers/ODataFeedControllerFactsBase.cs @@ -52,15 +52,21 @@ protected ODataFeedControllerFactsBase() protected abstract TController CreateController( IEntityRepository packagesRepository, IGalleryConfigurationService configurationService, - ISearchService searchService); + ISearchService searchService, + ITelemetryService telemetryService); protected TController CreateTestableODataFeedController(HttpRequestMessage request) { var searchService = new Mock().Object; var configurationService = new TestGalleryConfigurationService(); configurationService.Current.SiteRoot = _siteRoot; + var telemetryService = new Mock(); - var controller = CreateController(PackagesRepository, configurationService, searchService); + var controller = CreateController( + PackagesRepository, + configurationService, + searchService, + telemetryService.Object); AddRequestToController(request, controller); diff --git a/tests/NuGetGallery.Facts/Controllers/ODataV1FeedControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/ODataV1FeedControllerFacts.cs index 1b6fee3016..359e888367 100644 --- a/tests/NuGetGallery.Facts/Controllers/ODataV1FeedControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/ODataV1FeedControllerFacts.cs @@ -89,10 +89,17 @@ public async Task SearchCount_FiltersSemVerV2PackageVersions() Assert.Equal(NonSemVer2Packages.Where(p => !p.IsPrerelease).Count(), searchCount); } - protected override ODataV1FeedController CreateController(IEntityRepository packagesRepository, - IGalleryConfigurationService configurationService, ISearchService searchService) + protected override ODataV1FeedController CreateController( + IEntityRepository packagesRepository, + IGalleryConfigurationService configurationService, + ISearchService searchService, + ITelemetryService telemetryService) { - return new ODataV1FeedController(packagesRepository, configurationService, searchService); + return new ODataV1FeedController( + packagesRepository, + configurationService, + searchService, + telemetryService); } private void AssertSemVer2PackagesFilteredFromResult(IEnumerable resultSet) diff --git a/tests/NuGetGallery.Facts/Controllers/ODataV2CuratedFeedControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/ODataV2CuratedFeedControllerFacts.cs index a66fa6dc9c..c177bf68f5 100644 --- a/tests/NuGetGallery.Facts/Controllers/ODataV2CuratedFeedControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/ODataV2CuratedFeedControllerFacts.cs @@ -35,6 +35,7 @@ public class TheGetMethod private readonly Mock _searchService; private readonly Mock _curatedFeedService; private readonly Mock> _packages; + private readonly Mock _telemetryService; private readonly ODataV2CuratedFeedController _target; private readonly HttpRequestMessage _request; private readonly ODataQueryOptions _options; @@ -63,6 +64,7 @@ public TheGetMethod() _searchService = new Mock(); _curatedFeedService = new Mock(); _packages = new Mock>(); + _telemetryService = new Mock(); _config .Setup(x => x.Current) @@ -87,7 +89,8 @@ public TheGetMethod() _config.Object, _searchService.Object, _curatedFeedService.Object, - _packages.Object); + _packages.Object, + _telemetryService.Object); _request = new HttpRequestMessage(HttpMethod.Get, $"{_siteRoot}/api/v2/curated-feed/{_curatedFeedName}/Packages"); _options = new ODataQueryOptions(CreateODataQueryContext(), _request); @@ -475,7 +478,8 @@ public async Task SearchCount_IncludesSemVerV2PackageVersionsWhenSemVerLevel2OrH protected override ODataV2CuratedFeedController CreateController( IEntityRepository packagesRepository, IGalleryConfigurationService configurationService, - ISearchService searchService) + ISearchService searchService, + ITelemetryService telemetryService) { var curatedFeed = new CuratedFeed { Name = _curatedFeedName }; @@ -487,7 +491,8 @@ protected override ODataV2CuratedFeedController CreateController( configurationService, searchService, curatedFeedServiceMock.Object, - packagesRepository); + packagesRepository, + telemetryService); } private static IDbSet GetQueryableMockDbSet(params T[] sourceList) where T : class diff --git a/tests/NuGetGallery.Facts/Controllers/ODataV2FeedControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/ODataV2FeedControllerFacts.cs index c13c39b3f8..a6e39ef55c 100644 --- a/tests/NuGetGallery.Facts/Controllers/ODataV2FeedControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/ODataV2FeedControllerFacts.cs @@ -405,9 +405,14 @@ public async Task GetUpdatesCount_IncludesSemVerV2PackageVersionsWhenSemVerLevel protected override ODataV2FeedController CreateController( IEntityRepository packagesRepository, IGalleryConfigurationService configurationService, - ISearchService searchService) + ISearchService searchService, + ITelemetryService telemetryService) { - return new ODataV2FeedController(packagesRepository, configurationService, searchService); + return new ODataV2FeedController( + packagesRepository, + configurationService, + searchService, + telemetryService); } private void AssertSemVer2PackagesFilteredFromResult(IEnumerable resultSet) diff --git a/tests/NuGetGallery.Facts/Services/FeedServiceFacts.cs b/tests/NuGetGallery.Facts/Services/FeedServiceFacts.cs index f7e38b06ed..9123c1cb3c 100644 --- a/tests/NuGetGallery.Facts/Services/FeedServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/FeedServiceFacts.cs @@ -1,11 +1,14 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Linq; using System.Net; using System.Net.Http; using System.Net.Http.Headers; +using System.Threading; using System.Threading.Tasks; +using System.Web.Http; using System.Web.Http.OData; using System.Web.Http.OData.Query; using System.Web.Http.Results; @@ -227,7 +230,12 @@ public async Task V1FeedSearchDoesNotReturnPrereleasePackages() configuration.Setup(c => c.Current).Returns(new AppConfiguration() { IsODataFilterEnabled = false }); var searchService = new Mock(MockBehavior.Strict); searchService.Setup(s => s.ContainsAllVersions).Returns(false); - var v1Service = new TestableV1Feed(repo.Object, configuration.Object, searchService.Object); + var telemetryService = new Mock(); + var v1Service = new TestableV1Feed( + repo.Object, + configuration.Object, + searchService.Object, + telemetryService.Object); v1Service.Request = new HttpRequestMessage(HttpMethod.Get, "https://localhost:8081/"); // Act @@ -244,8 +252,54 @@ public async Task V1FeedSearchDoesNotReturnPrereleasePackages() Assert.Equal("Foo", result.First().Id); Assert.Equal("1.0.0", result.First().Version); Assert.Equal("https://localhost:8081/packages/Foo/1.0.0", result.First().GalleryDetailsUrl); + telemetryService.Verify(x => x.TrackODataCustomQuery(true), Times.Once); + telemetryService.Verify(x => x.TrackODataCustomQuery(It.IsAny()), Times.Once); } + [Fact] + public async Task V1FeedSearchCanUseSearchService() + { + // Arrange + var repo = new Mock>(MockBehavior.Strict); + repo.Setup(r => r.GetAll()).Returns(Enumerable.Empty().AsQueryable()); + var configuration = new Mock(MockBehavior.Strict); + configuration.Setup(c => c.GetSiteRoot(It.IsAny())).Returns("https://localhost:8081/"); + configuration.Setup(c => c.Current).Returns(new AppConfiguration() { IsODataFilterEnabled = false }); + var searchService = new Mock(MockBehavior.Strict); + searchService.Setup(s => s.ContainsAllVersions).Returns(true); + searchService + .Setup(s => s.Search(It.IsAny())) + .ReturnsAsync(new SearchResults(0, indexTimestampUtc: null)); + var telemetryService = new Mock(); + var v1Service = new TestableV1Feed( + repo.Object, + configuration.Object, + searchService.Object, + telemetryService.Object); + v1Service.RawUrl = "https://localhost:8081/"; + v1Service.Request = new HttpRequestMessage(HttpMethod.Get, v1Service.RawUrl); + v1Service.Configuration = new HttpConfiguration(); + var options = new ODataQueryOptions( + new ODataQueryContext(NuGetODataV1FeedConfig.GetEdmModel(), typeof(V1FeedPackage)), + v1Service.Request); + + // Act + var genericResult = await v1Service.Search(options, null, null); + var result = genericResult + .ExpectQueryResult() + .GetInnerResult() + .ExpectOkNegotiatedContentResult>(); + + // Assert + Assert.Empty(result); + telemetryService.Verify(x => x.TrackODataCustomQuery(false), Times.Once); + telemetryService.Verify(x => x.TrackODataCustomQuery(It.IsAny()), Times.Once); + var response = await genericResult.ExecuteAsync(CancellationToken.None); + Assert.Contains(GalleryConstants.CustomQueryHeaderName, response.Headers.Select(x => x.Key)); + Assert.Equal( + new[] { "false" }, + response.Headers.GetValues(GalleryConstants.CustomQueryHeaderName).ToArray()); + } [Fact] public async Task V1FeedSearchDoesNotReturnDeletedPackages() @@ -488,6 +542,8 @@ public async Task V2FeedPackagesReturnsCollection(string filter, int top, int ex [InlineData("Id eq 'Foo'")] [InlineData("(Id eq 'Foo')")] [InlineData("Id eq 'Bar' and Version eq '1.0.0'")] + [InlineData("Id eq 'Foo' and true")] + [InlineData("Id eq 'Foo' and false")] public async Task V2FeedPackagesUsesSearchHijackForIdOrIdVersionQueries(string filter) { // Arrange @@ -500,30 +556,52 @@ public async Task V2FeedPackagesUsesSearchHijackForIdOrIdVersionQueries(string f var searchService = new Mock(MockBehavior.Loose); searchService.CallBase = true; + searchService + .Setup(x => x.RawSearch(It.IsAny())) + .ReturnsAsync(new SearchResults(0, indexTimestampUtc: null)); + + var telemetryService = new Mock(); string rawUrl = "https://localhost:8081/api/v2/Packages?$filter=" + filter + "&$top=10&$orderby=DownloadCount desc"; - var v2Service = new TestableV2Feed(repo.Object, configuration.Object, searchService.Object); + var v2Service = new TestableV2Feed( + repo.Object, + configuration.Object, + searchService.Object, + telemetryService.Object); v2Service.Request = new HttpRequestMessage(HttpMethod.Get, rawUrl); v2Service.RawUrl = rawUrl; + v2Service.Configuration = new HttpConfiguration(); + var options = new ODataQueryOptions( + new ODataQueryContext(NuGetODataV2FeedConfig.GetEdmModel(), typeof(V2FeedPackage)), + v2Service.Request); // Act - var result = (await v2Service.Get(new ODataQueryOptions(new ODataQueryContext(NuGetODataV2FeedConfig.GetEdmModel(), typeof(V2FeedPackage)), v2Service.Request))) + var genericResult = await v2Service.Get(options); + var result = genericResult .ExpectQueryResult() .GetInnerResult() - .ExpectOkNegotiatedContentResult>() + .ExpectOkNegotiatedContentResult>() .ToArray(); // Assert Assert.NotNull(result); searchService.Verify(); + telemetryService.Verify(x => x.TrackODataCustomQuery(false), Times.Once); + telemetryService.Verify(x => x.TrackODataCustomQuery(It.IsAny()), Times.Once); + var response = await genericResult.ExecuteAsync(CancellationToken.None); + Assert.Contains(GalleryConstants.CustomQueryHeaderName, response.Headers.Select(x => x.Key)); + Assert.Equal( + new[] { "false" }, + response.Headers.GetValues(GalleryConstants.CustomQueryHeaderName).ToArray()); } [Theory] [InlineData("Id eq 'Bar' and IsPrerelease eq true")] [InlineData("Id eq 'Bar' or Id eq 'Foo'")] [InlineData("(Id eq 'Foo' and Version eq '1.0.0') or (Id eq 'Bar' and Version eq '1.0.0')")] - [InlineData("Id eq 'NotBar' and true")] + [InlineData("Id eq 'NotBar' and Version eq '1.0.0' and true")] + [InlineData("Id eq 'NotBar' and Version eq '1.0.0' and false")] [InlineData("true")] public async Task V2FeedPackagesDoesNotUseSearchHijackForFunkyQueries(string filter) { @@ -537,16 +615,30 @@ public async Task V2FeedPackagesDoesNotUseSearchHijackForFunkyQueries(string fil bool called = false; var searchService = new Mock(MockBehavior.Loose); + searchService + .Setup(x => x.RawSearch(It.IsAny())) + .ReturnsAsync(new SearchResults(0, indexTimestampUtc: null)); searchService.CallBase = true; + var telemetryService = new Mock(); + string rawUrl = "https://localhost:8081/api/v2/Packages?$filter=" + filter + "&$top=10"; - var v2Service = new TestableV2Feed(repo.Object, configuration.Object, searchService.Object); + var v2Service = new TestableV2Feed( + repo.Object, + configuration.Object, + searchService.Object, + telemetryService.Object); v2Service.Request = new HttpRequestMessage(HttpMethod.Get, rawUrl); v2Service.RawUrl = rawUrl; + v2Service.Configuration = new HttpConfiguration(); + var options = new ODataQueryOptions( + new ODataQueryContext(NuGetODataV2FeedConfig.GetEdmModel(), typeof(V2FeedPackage)), + v2Service.Request); // Act - var result = (await v2Service.Get(new ODataQueryOptions(new ODataQueryContext(NuGetODataV2FeedConfig.GetEdmModel(), typeof(V2FeedPackage)), v2Service.Request))) + var genericResult = await v2Service.Get(options); + var result = genericResult .ExpectQueryResult() .GetInnerResult() .ExpectOkNegotiatedContentResult>() @@ -556,6 +648,13 @@ public async Task V2FeedPackagesDoesNotUseSearchHijackForFunkyQueries(string fil Assert.NotNull(result); Assert.False(called); // Hijack was performed and it should not have been. searchService.Verify(); + telemetryService.Verify(x => x.TrackODataCustomQuery(true), Times.Once); + telemetryService.Verify(x => x.TrackODataCustomQuery(It.IsAny()), Times.Once); + var response = await genericResult.ExecuteAsync(CancellationToken.None); + Assert.Contains(GalleryConstants.CustomQueryHeaderName, response.Headers.Select(x => x.Key)); + Assert.Equal( + new[] { "true" }, + response.Headers.GetValues(GalleryConstants.CustomQueryHeaderName).ToArray()); } [Theory] @@ -615,7 +714,13 @@ public async Task V2FeedPackagesByIdAndVersionReturnsPackage(string expectedId, var searchService = new Mock(MockBehavior.Strict); searchService.Setup(s => s.ContainsAllVersions).Returns(false); - var v2Service = new TestableV2Feed(repo.Object, configuration.Object, searchService.Object); + var telemetryService = new Mock(); + + var v2Service = new TestableV2Feed( + repo.Object, + configuration.Object, + searchService.Object, + telemetryService.Object); v2Service.Request = new HttpRequestMessage( HttpMethod.Get, $"https://localhost:8081/api/v2/Packages(Id=\'{expectedId}\', Version=\'{expectedVersion}\')"); @@ -629,6 +734,53 @@ public async Task V2FeedPackagesByIdAndVersionReturnsPackage(string expectedId, // Assert Assert.Equal(expectedId, result.Id); Assert.Equal(expectedVersion, result.Version); + telemetryService.Verify(x => x.TrackODataCustomQuery(true), Times.Once); + telemetryService.Verify(x => x.TrackODataCustomQuery(It.IsAny()), Times.Once); + } + + [Fact] + public async Task V2FeedPackagesByIdAndVersionCanUseTheSearchService() + { + // Arrange + var repo = FeedServiceHelpers.SetupTestPackageRepository(); + + var configuration = new Mock(MockBehavior.Strict); + configuration.Setup(c => c.GetSiteRoot(It.IsAny())).Returns("https://localhost:8081/"); + configuration.Setup(c => c.Features).Returns(new FeatureConfiguration() { FriendlyLicenses = true }); + configuration.Setup(c => c.Current).Returns(new AppConfiguration() { IsODataFilterEnabled = false }); + + var searchService = new Mock(MockBehavior.Strict); + searchService.Setup(s => s.ContainsAllVersions).Returns(true); + var data = repo.Object.GetAll().Take(1).ToArray().AsQueryable(); + searchService + .Setup(s => s.Search(It.IsAny())) + .ReturnsAsync(new SearchResults( + hits: 1, + indexTimestampUtc: null, + data: data)); + + var telemetryService = new Mock(); + + var v2Service = new TestableV2Feed( + repo.Object, + configuration.Object, + searchService.Object, + telemetryService.Object); + v2Service.RawUrl = $"https://localhost:8081/api/v2/Packages(Id=\'Foo\', Version=\'1.0.0\')"; + v2Service.Request = new HttpRequestMessage(HttpMethod.Get, v2Service.RawUrl); + var options = new ODataQueryOptions( + new ODataQueryContext(NuGetODataV2FeedConfig.GetEdmModel(), typeof(V2FeedPackage)), + v2Service.Request); + + // Act + var result = (await v2Service.Get(options, "Foo", "1.0.0")) + .ExpectQueryResult() + .GetInnerResult() + .ExpectOkNegotiatedContentResult(); + + // Assert + telemetryService.Verify(x => x.TrackODataCustomQuery(false), Times.Once); + telemetryService.Verify(x => x.TrackODataCustomQuery(It.IsAny()), Times.Once); } [Theory] @@ -731,7 +883,12 @@ public async Task V2FeedFindPackagesByIdReturnsUnlistedAndPrereleasePackages() configuration.Setup(c => c.Features).Returns(new FeatureConfiguration() { FriendlyLicenses = true }); var searchService = new Mock(MockBehavior.Strict); searchService.Setup(s => s.ContainsAllVersions).Returns(false); - var v2Service = new TestableV2Feed(repo.Object, configuration.Object, searchService.Object); + var telemetryService = new Mock(); + var v2Service = new TestableV2Feed( + repo.Object, + configuration.Object, + searchService.Object, + telemetryService.Object); v2Service.Request = new HttpRequestMessage(HttpMethod.Get, "https://localhost:8081/"); // Act @@ -749,6 +906,9 @@ public async Task V2FeedFindPackagesByIdReturnsUnlistedAndPrereleasePackages() Assert.Equal("Foo", result.Last().Id); Assert.Equal("1.0.1-a", result.Last().Version); + + telemetryService.Verify(x => x.TrackODataCustomQuery(true), Times.Once); + telemetryService.Verify(x => x.TrackODataCustomQuery(It.IsAny()), Times.Once); } [Fact] @@ -765,7 +925,13 @@ public async Task V2FeedFindPackagesByIdReturnsEmptyCollectionWhenNoPackages() var searchService = new Mock(MockBehavior.Strict); searchService.Setup(s => s.ContainsAllVersions).Returns(false); - var v2Service = new TestableV2Feed(repo.Object, configuration.Object, searchService.Object); + var telemetryService = new Mock(); + + var v2Service = new TestableV2Feed( + repo.Object, + configuration.Object, + searchService.Object, + telemetryService.Object); v2Service.Request = new HttpRequestMessage(HttpMethod.Get, "https://localhost:8081/"); // Act @@ -778,6 +944,116 @@ public async Task V2FeedFindPackagesByIdReturnsEmptyCollectionWhenNoPackages() // Assert Assert.Equal(0, result.Count()); + telemetryService.Verify(x => x.TrackODataCustomQuery(true), Times.Once); + telemetryService.Verify(x => x.TrackODataCustomQuery(It.IsAny()), Times.Once); + } + + [Fact] + public async Task V2FeedFindPackagesByIdCanUseSearchService() + { + // Arrange + var repo = new Mock>(MockBehavior.Strict); + repo.Setup(r => r.GetAll()).Returns(() => Enumerable.Empty().AsQueryable()); + + var configuration = new Mock(MockBehavior.Strict); + configuration.Setup(c => c.GetSiteRoot(It.IsAny())).Returns("https://localhost:8081/"); + configuration.Setup(c => c.Features).Returns(new FeatureConfiguration() { FriendlyLicenses = true }); + + var searchService = new Mock(MockBehavior.Strict); + searchService.Setup(s => s.ContainsAllVersions).Returns(true); + searchService + .Setup(s => s.Search(It.IsAny())) + .ReturnsAsync(new SearchResults(0, indexTimestampUtc: null)); + + var telemetryService = new Mock(); + + var rawUrl = "https://localhost:8081/"; + + var v2Service = new TestableV2Feed( + repo.Object, + configuration.Object, + searchService.Object, + telemetryService.Object); + v2Service.Request = new HttpRequestMessage(HttpMethod.Get, rawUrl); + v2Service.RawUrl = rawUrl; + var options = new ODataQueryOptions( + new ODataQueryContext(NuGetODataV2FeedConfig.GetEdmModel(), typeof(V2FeedPackage)), + v2Service.Request); + + // Act + var result = (await v2Service.FindPackagesById(options, "Foo")) + .ExpectQueryResult() + .GetInnerResult() + .ExpectOkNegotiatedContentResult>(); + + // Assert + Assert.Equal(0, result.Count()); + telemetryService.Verify(x => x.TrackODataCustomQuery(false), Times.Once); + telemetryService.Verify(x => x.TrackODataCustomQuery(It.IsAny()), Times.Once); + } + + [Fact] + public async Task V2FeedFindPackagesConsidersSearchRequestFailureAsNonCustomQuery() + { + // Arrange + var packageRegistration = new PackageRegistration { Id = "Foo" }; + var repo = new Mock>(MockBehavior.Strict); + repo.Setup(r => r.GetAll()).Returns(new[] + { + new Package + { + PackageRegistration = packageRegistration, + Version = "1.0.0", + IsPrerelease = false, + Listed = false, + }, + new Package + { + PackageRegistration = packageRegistration, + Version = "1.0.1", + IsPrerelease = false, + Listed = true, + }, + }.AsQueryable()); + var configuration = new Mock(MockBehavior.Strict); + configuration.Setup(c => c.GetSiteRoot(It.IsAny())).Returns("https://localhost:8081/"); + configuration.Setup(c => c.Features).Returns(new FeatureConfiguration() { FriendlyLicenses = true }); + + var searchService = new Mock(MockBehavior.Strict); + searchService.Setup(s => s.ContainsAllVersions).Returns(true); + searchService + .Setup(s => s.Search(It.IsAny())) + .ThrowsAsync(new InvalidOperationException("Search is down.")); + + var telemetryService = new Mock(); + + var rawUrl = "https://localhost:8081/"; + + var v2Service = new TestableV2Feed( + repo.Object, + configuration.Object, + searchService.Object, + telemetryService.Object); + v2Service.Request = new HttpRequestMessage(HttpMethod.Get, rawUrl); + v2Service.RawUrl = rawUrl; + v2Service.Configuration = new HttpConfiguration(); + var options = new ODataQueryOptions( + new ODataQueryContext(NuGetODataV2FeedConfig.GetEdmModel(), typeof(V2FeedPackage)), + v2Service.Request); + + // Act + var genericResult = await v2Service.FindPackagesById(options, packageRegistration.Id); + var result = genericResult + .ExpectQueryResult() + .GetInnerResult() + .ExpectOkNegotiatedContentResult>(); + + // Assert + Assert.Equal(2, result.Count()); + telemetryService.Verify(x => x.TrackODataCustomQuery(null), Times.Once); + telemetryService.Verify(x => x.TrackODataCustomQuery(It.IsAny()), Times.Once); + var response = await genericResult.ExecuteAsync(CancellationToken.None); + Assert.DoesNotContain(GalleryConstants.CustomQueryHeaderName, response.Headers.Select(x => x.Key)); } [Fact] @@ -869,11 +1145,18 @@ public async Task V2FeedSearchFiltersPackagesBySearchTermAndPrereleaseFlag(strin configuration.Setup(c => c.Features).Returns(new FeatureConfiguration() { FriendlyLicenses = true }); configuration.Setup(c => c.Current).Returns(new AppConfiguration() { IsODataFilterEnabled = false }); + var telemetryService = new Mock(); + var searchService = new Mock(MockBehavior.Strict); searchService.Setup(s => s.ContainsAllVersions).Returns(false); - var v2Service = new TestableV2Feed(repo.Object, configuration.Object, searchService.Object); - v2Service.Request = new HttpRequestMessage(HttpMethod.Get, "https://localhost:8081/api/v2/Search()?searchTerm='" + searchTerm + "'&targetFramework=''&includePrerelease=false"); + var v2Service = new TestableV2Feed( + repo.Object, + configuration.Object, + searchService.Object, + telemetryService.Object); + v2Service.RawUrl = "https://localhost:8081/api/v2/Search()?searchTerm='" + searchTerm + "'&targetFramework=''&includePrerelease=false"; + v2Service.Request = new HttpRequestMessage(HttpMethod.Get, v2Service.RawUrl); // Act var result = (await v2Service.Search( @@ -895,6 +1178,54 @@ public async Task V2FeedSearchFiltersPackagesBySearchTermAndPrereleaseFlag(strin Assert.True(result.Any(p => p.Id == expectedId && p.Version == expectedVersion), string.Format("Search results did not contain {0} {1}", expectedId, expectedVersion)); } + + telemetryService.Verify(x => x.TrackODataCustomQuery(true), Times.Once); + telemetryService.Verify(x => x.TrackODataCustomQuery(It.IsAny()), Times.Once); + } + + [Fact] + public async Task V2FeedSearchUsesSearchService() + { + // Arrange + var repo = FeedServiceHelpers.SetupTestPackageRepository(); + var searchTerm = "foo"; + var includePrerelease = true; + + var configuration = new Mock(MockBehavior.Strict); + configuration.Setup(c => c.GetSiteRoot(It.IsAny())).Returns("https://localhost:8081/"); + configuration.Setup(c => c.Features).Returns(new FeatureConfiguration() { FriendlyLicenses = true }); + configuration.Setup(c => c.Current).Returns(new AppConfiguration() { IsODataFilterEnabled = false }); + + var telemetryService = new Mock(); + + var searchService = new Mock(MockBehavior.Strict); + searchService.Setup(s => s.ContainsAllVersions).Returns(true); + searchService + .Setup(s => s.Search(It.IsAny())) + .ReturnsAsync(new SearchResults(0, indexTimestampUtc: null)); + + var v2Service = new TestableV2Feed( + repo.Object, + configuration.Object, + searchService.Object, + telemetryService.Object); + v2Service.RawUrl = "https://localhost:8081/api/v2/Search()?searchTerm='" + searchTerm + "'&targetFramework=''&includePrerelease=false"; + v2Service.Request = new HttpRequestMessage(HttpMethod.Get, v2Service.RawUrl); + + // Act + var result = (await v2Service.Search( + new ODataQueryOptions(new ODataQueryContext(NuGetODataV2FeedConfig.GetEdmModel(), typeof(V2FeedPackage)), v2Service.Request), + searchTerm: searchTerm, + targetFramework: null, + includePrerelease: includePrerelease)) + .ExpectQueryResult() + .GetInnerResult() + .ExpectOkNegotiatedContentResult>() + .ToArray(); + + // Assert + telemetryService.Verify(x => x.TrackODataCustomQuery(false), Times.Once); + telemetryService.Verify(x => x.TrackODataCustomQuery(It.IsAny()), Times.Once); } [Theory] @@ -958,7 +1289,8 @@ public void V2FeedGetUpdatesReturnsEmptyResultsIfInputIsMalformed(string id, str var repo = Mock.Of>(); var configuration = new Mock(MockBehavior.Default); configuration.Setup(c => c.Current).Returns(new AppConfiguration() { IsODataFilterEnabled = false }); - var v2Service = new TestableV2Feed(repo, configuration.Object, null); + var telemetryService = new Mock(); + var v2Service = new TestableV2Feed(repo, configuration.Object, null, telemetryService.Object); v2Service.Request = new HttpRequestMessage(HttpMethod.Get, "https://localhost:8081/"); // Act @@ -970,10 +1302,14 @@ public void V2FeedGetUpdatesReturnsEmptyResultsIfInputIsMalformed(string id, str includeAllVersions: true, targetFrameworks: null, versionConstraints: null) + .ExpectQueryResult() + .GetInnerResult() .ExpectOkNegotiatedContentResult>(); // Assert Assert.Empty(result); + telemetryService.Verify(x => x.TrackODataCustomQuery(false), Times.Once); + telemetryService.Verify(x => x.TrackODataCustomQuery(It.IsAny()), Times.Once); } [Fact] @@ -1037,7 +1373,8 @@ public void V2FeedGetUpdatesReturnsVersionsNewerThanListedVersion() configuration.Setup(c => c.GetSiteRoot(It.IsAny())).Returns("https://localhost:8081/"); configuration.Setup(c => c.Features).Returns(new FeatureConfiguration() { FriendlyLicenses = true }); configuration.Setup(c => c.Current).Returns(new AppConfiguration() { IsODataFilterEnabled = false }); - var v2Service = new TestableV2Feed(repo.Object, configuration.Object, null); + var telemetryService = new Mock(); + var v2Service = new TestableV2Feed(repo.Object, configuration.Object, null, telemetryService.Object); v2Service.Request = new HttpRequestMessage(HttpMethod.Get, "https://localhost:8081/"); // Act @@ -1059,6 +1396,8 @@ public void V2FeedGetUpdatesReturnsVersionsNewerThanListedVersion() AssertPackage(new { Id = "Foo", Version = "1.1.0" }, result[0]); AssertPackage(new { Id = "Foo", Version = "1.2.0" }, result[1]); AssertPackage(new { Id = "Qux", Version = "2.0" }, result[2]); + telemetryService.Verify(x => x.TrackODataCustomQuery(false), Times.Once); + telemetryService.Verify(x => x.TrackODataCustomQuery(It.IsAny()), Times.Once); } [Theory] @@ -1099,10 +1438,13 @@ public void V2FeedGetUpdatesReturnsEmptyIfVersionConstraintsContainWrongNumberOf includeAllVersions: true, targetFrameworks: null, versionConstraints: constraintString) - .ExpectOkNegotiatedContentResult>(); + .ExpectQueryResult() + .GetInnerResult() + .ExpectOkNegotiatedContentResult>() + .ToArray(); // Assert - Assert.Equal(0, result.Count()); + Assert.Empty(result); } [Fact] diff --git a/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs b/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs index 18216f0f04..6f063c7c8d 100644 --- a/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs @@ -57,6 +57,10 @@ public static IEnumerable TrackMetricNames_Data (TrackAction)(s => s.TrackODataQueryFilterEvent("callContext", true, true, "queryPattern")) }; + yield return new object[] { "ODataCustomQuery", + (TrackAction)(s => s.TrackODataCustomQuery(true)) + }; + yield return new object[] { "PackagePush", (TrackAction)(s => s.TrackPackagePushEvent(package, fakes.User, identity)) }; @@ -631,6 +635,27 @@ public void TrackRequestForAccountDeletedAddsCorrectData() service.TelemetryClient.VerifyAll(); } + [Theory] + [InlineData(false)] + [InlineData(true)] + public void TrackODataCustomQueryAddsCorrectData(bool customQuery) + { + var service = CreateService(); + var allProperties = new List>(); + service.TelemetryClient + .Setup(x => x.TrackMetric(It.IsAny(), It.IsAny(), It.IsAny>())) + .Callback>((_, __, p) => allProperties.Add(p)); + + service.TrackODataCustomQuery(customQuery); + + service.TelemetryClient.Verify( + x => x.TrackMetric("ODataCustomQuery", 1, It.IsAny>()), + Times.Once); + var properties = Assert.Single(allProperties); + Assert.Contains("IsCustomQuery", properties.Keys); + Assert.Equal(customQuery.ToString(), properties["IsCustomQuery"]); + } + private TelemetryServiceWrapper CreateServiceForCertificateTelemetry(string metricName, string thumbprint) { var service = CreateService(); diff --git a/tests/NuGetGallery.Facts/TestUtils/Infrastructure/TestableV1Feed.cs b/tests/NuGetGallery.Facts/TestUtils/Infrastructure/TestableV1Feed.cs index d2a39257c5..4dd3989d47 100644 --- a/tests/NuGetGallery.Facts/TestUtils/Infrastructure/TestableV1Feed.cs +++ b/tests/NuGetGallery.Facts/TestUtils/Infrastructure/TestableV1Feed.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.Web; +using Moq; using NuGet.Services.Entities; using NuGetGallery.Configuration; using NuGetGallery.Controllers; @@ -14,12 +15,27 @@ public TestableV1Feed( IEntityRepository repo, IGalleryConfigurationService configuration, ISearchService searchService) - : base(repo, configuration, searchService) + : base(repo, configuration, searchService, Mock.Of()) { } + public TestableV1Feed( + IEntityRepository repo, + IGalleryConfigurationService configuration, + ISearchService searchService, + ITelemetryService telemetryService) + : base(repo, configuration, searchService, telemetryService) + { + } + + public string RawUrl { get; set; } + protected override HttpContextBase GetTraditionalHttpContext() { + if (!string.IsNullOrEmpty(RawUrl)) + { + return FeedServiceHelpers.GetMockContext(RawUrl.StartsWith("https"), RawUrl); + } return FeedServiceHelpers.GetMockContext(); } diff --git a/tests/NuGetGallery.Facts/TestUtils/Infrastructure/TestableV2Feed.cs b/tests/NuGetGallery.Facts/TestUtils/Infrastructure/TestableV2Feed.cs index 04a34f37b4..cb07cf95a7 100644 --- a/tests/NuGetGallery.Facts/TestUtils/Infrastructure/TestableV2Feed.cs +++ b/tests/NuGetGallery.Facts/TestUtils/Infrastructure/TestableV2Feed.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.Web; +using Moq; using NuGet.Services.Entities; using NuGetGallery.Configuration; using NuGetGallery.Controllers; @@ -14,7 +15,16 @@ public TestableV2Feed( IEntityRepository repo, IGalleryConfigurationService configuration, ISearchService searchService) - : base(repo, configuration, searchService) + : base(repo, configuration, searchService, Mock.Of()) + { + } + + public TestableV2Feed( + IEntityRepository repo, + IGalleryConfigurationService configuration, + ISearchService searchService, + ITelemetryService telemetryService) + : base(repo, configuration, searchService, telemetryService) { } From 6ebb7a2666b76a992de1927a0e5910fb99e06815 Mon Sep 17 00:00:00 2001 From: Shishir H Date: Tue, 30 Oct 2018 14:09:51 -0700 Subject: [PATCH 14/20] Fix concurrency/consistency issues in Gallery API push (#6600) --- .../NuGetGallery.Core.csproj | 1 + .../Services/PackageAlreadyExistsException.cs | 15 ++++++ src/NuGetGallery/Controllers/ApiController.cs | 6 ++- src/NuGetGallery/Services/PackageService.cs | 4 +- .../Services/PackageUploadService.cs | 32 ++++++++++++- .../Controllers/ApiControllerFacts.cs | 46 +++++++++++++++++++ .../Services/PackageServiceFacts.cs | 20 ++++++++ .../Services/PackageUploadServiceFacts.cs | 22 +++++++++ 8 files changed, 140 insertions(+), 6 deletions(-) create mode 100644 src/NuGetGallery.Core/Services/PackageAlreadyExistsException.cs diff --git a/src/NuGetGallery.Core/NuGetGallery.Core.csproj b/src/NuGetGallery.Core/NuGetGallery.Core.csproj index a6c30c559a..00b6654a50 100644 --- a/src/NuGetGallery.Core/NuGetGallery.Core.csproj +++ b/src/NuGetGallery.Core/NuGetGallery.Core.csproj @@ -161,6 +161,7 @@ + diff --git a/src/NuGetGallery.Core/Services/PackageAlreadyExistsException.cs b/src/NuGetGallery.Core/Services/PackageAlreadyExistsException.cs new file mode 100644 index 0000000000..6748c7d5a4 --- /dev/null +++ b/src/NuGetGallery.Core/Services/PackageAlreadyExistsException.cs @@ -0,0 +1,15 @@ +// 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; + +namespace NuGetGallery +{ + [Serializable] + public sealed class PackageAlreadyExistsException : Exception + { + public PackageAlreadyExistsException() { } + public PackageAlreadyExistsException(string message) : base(message) { } + public PackageAlreadyExistsException(string message, Exception inner) : base(message, inner) { } + } +} \ No newline at end of file diff --git a/src/NuGetGallery/Controllers/ApiController.cs b/src/NuGetGallery/Controllers/ApiController.cs index 69ec973713..dccd155681 100644 --- a/src/NuGetGallery/Controllers/ApiController.cs +++ b/src/NuGetGallery/Controllers/ApiController.cs @@ -25,8 +25,6 @@ using NuGetGallery.Configuration; using NuGetGallery.Filters; using NuGetGallery.Infrastructure.Authentication; -using NuGetGallery.Infrastructure.Mail; -using NuGetGallery.Infrastructure.Mail.Messages; using NuGetGallery.Packaging; using NuGetGallery.Security; using PackageIdValidator = NuGetGallery.Packaging.PackageIdValidator; @@ -731,6 +729,10 @@ await AuditingService.SaveAuditRecordAsync( { return BadRequestForExceptionMessage(ex); } + catch (PackageAlreadyExistsException ex) + { + return new HttpStatusCodeWithBodyResult(HttpStatusCode.Conflict, ex.Message); + } catch (EntityException ex) { return BadRequestForExceptionMessage(ex); diff --git a/src/NuGetGallery/Services/PackageService.cs b/src/NuGetGallery/Services/PackageService.cs index 14656e9ff4..0e0f79b0c7 100644 --- a/src/NuGetGallery/Services/PackageService.cs +++ b/src/NuGetGallery/Services/PackageService.cs @@ -479,8 +479,8 @@ private Package CreatePackageFromNuGetPackage(PackageRegistration packageRegistr if (package != null) { - throw new EntityException( - "A package with identifier '{0}' and version '{1}' already exists.", packageRegistration.Id, package.Version); + throw new PackageAlreadyExistsException( + string.Format(Strings.PackageExistsAndCannotBeModified, packageRegistration.Id, package.Version)); } package = new Package(); diff --git a/src/NuGetGallery/Services/PackageUploadService.cs b/src/NuGetGallery/Services/PackageUploadService.cs index 1c7e643d00..1b840e0d73 100644 --- a/src/NuGetGallery/Services/PackageUploadService.cs +++ b/src/NuGetGallery/Services/PackageUploadService.cs @@ -3,6 +3,8 @@ using System; using System.Collections.Generic; +using System.Data.Entity.Infrastructure; +using System.Data.SqlClient; using System.IO; using System.Linq; using System.Threading; @@ -427,7 +429,7 @@ await _packageFileService.DeleteValidationPackageFileAsync( // commit all changes to database as an atomic transaction await _entitiesContext.SaveChangesAsync(); } - catch + catch (Exception ex) { // If saving to the DB fails for any reason we need to delete the package we just saved. if (package.PackageStatusKey == PackageStatus.Validating) @@ -443,10 +445,36 @@ await _packageFileService.DeletePackageFileAsync( package.Version); } - throw; + return ReturnConflictOrThrow(ex); } return PackageCommitResult.Success; } + + private PackageCommitResult ReturnConflictOrThrow(Exception ex) + { + if (ex is DbUpdateConcurrencyException concurrencyEx) + { + return PackageCommitResult.Conflict; + } + else if (ex is DbUpdateException dbUpdateEx) + { + if (dbUpdateEx.InnerException?.InnerException != null) + { + if (dbUpdateEx.InnerException.InnerException is SqlException sqlException) + { + switch (sqlException.Number) + { + case 547: // Constraint check violation + case 2601: // Duplicated key row error + case 2627: // Unique constraint error + return PackageCommitResult.Conflict; + } + } + } + } + + throw ex; + } } } diff --git a/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs index b2a9ee0bfc..407d889ab2 100644 --- a/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs @@ -843,6 +843,52 @@ public async Task WillReturnConflictIfCommittingPackageReturnsConflict() controller.MockEntitiesContext.VerifyCommitted(Times.Never()); } + [Fact] + public async Task WillReturnConflictIfGeneratePackageThrowsPackageAlreadyExistsException() + { + // Arrange + var packageId = "theId"; + var nuGetPackage = TestPackage.CreateTestPackageStream(packageId, "1.0.42"); + + var currentUser = new User("currentUser") { Key = 1, EmailAddress = "currentUser@confirmed.com" }; + var controller = new TestableApiController(GetConfigurationService()); + controller.SetCurrentUser(currentUser); + controller.SetupPackageFromInputStream(nuGetPackage); + + var owner = new User("owner") { Key = 2, EmailAddress = "org@confirmed.com" }; + + Expression> evaluateApiScope = + x => x.Evaluate( + currentUser, + It.IsAny>(), + ActionsRequiringPermissions.UploadNewPackageId, + It.Is((context) => context.PackageId == packageId), + NuGetScopes.PackagePush); + + controller.MockApiScopeEvaluator + .Setup(evaluateApiScope) + .Returns(new ApiScopeEvaluationResult(owner, PermissionsCheckResult.Allowed, scopesAreValid: true)); + controller + .MockPackageUploadService + .Setup(x => x.GeneratePackageAsync(It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .Throws(new PackageAlreadyExistsException("Package exists")); + + // Act + var result = await controller.CreatePackagePut(); + + // Assert + ResultAssert.IsStatusCode(result, HttpStatusCode.Conflict); + controller.MockPackageUploadService.Verify(x => x.GeneratePackageAsync(It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny()), Times.Once); + } + [Fact] public async Task WillReturnValidationWarnings() { diff --git a/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs index 0809dc8d01..5e73609a59 100644 --- a/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs @@ -423,6 +423,26 @@ private async Task WillSaveTheCreatedPackageWhenThePackageRegistrationAlreadyExi Assert.Same(packageRegistration.Packages.ElementAt(0), package); } + [Fact] + private async Task WillThrowWhenThePackageRegistrationAndVersionAlreadyExists() + { + var currentUser = new User(); + var packageId = "theId"; + var packageVersion = "1.0.32"; + var nugetPackage = PackageServiceUtility.CreateNuGetPackage(packageId, packageVersion); + var packageRegistration = new PackageRegistration + { + Id = packageId, + Owners = new HashSet { currentUser }, + }; + packageRegistration.Packages.Add(new Package() { Version = packageVersion }); + var packageRegistrationRepository = new Mock>(); + var service = CreateService(packageRegistrationRepository: packageRegistrationRepository, setup: + mockPackageService => { mockPackageService.Setup(x => x.FindPackageRegistrationById(It.IsAny())).Returns(packageRegistration); }); + + await Assert.ThrowsAsync(async () => await service.CreatePackageAsync(nugetPackage.Object, new PackageStreamMetadata(), currentUser, currentUser, isVerified: false)); + } + [Fact] private async Task WillThrowIfTheNuGetPackageIdIsLongerThanMaxPackageIdLength() { diff --git a/tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs index bf0e5e3f53..e9c2b7b2a2 100644 --- a/tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Data.Entity.Infrastructure; using System.IO; using System.Linq; using System.Threading; @@ -1037,6 +1038,27 @@ public async Task DeletesPackageIfDatabaseCommitFailsWhenAvailable() Assert.Same(_unexpectedException, exception); } + [Fact] + public async Task ReturnsConflictWhenDBCommitThrowsConcurrencyViolations() + { + _package.PackageStatusKey = PackageStatus.Available; + var ex = new DbUpdateConcurrencyException("whoops!"); + _entitiesContext + .Setup(x => x.SaveChangesAsync()) + .Throws(ex); + + var result = await _target.CommitPackageAsync(_package, _packageFile); + + _packageFileService.Verify( + x => x.DeletePackageFileAsync(Id, Version), + Times.Once); + _packageFileService.Verify( + x => x.DeletePackageFileAsync(It.IsAny(), It.IsAny()), + Times.Once); + + Assert.Equal(PackageCommitResult.Conflict, result); + } + [Fact] public async Task DeletesPackageIfDatabaseCommitFailsWhenValidating() { From c6e3c4729eec3aa764e1f6bc086673cacb47a570 Mon Sep 17 00:00:00 2001 From: Scott Bommarito Date: Tue, 30 Oct 2018 16:00:28 -0700 Subject: [PATCH 15/20] Do not CC sender for Contact Owners emails --- src/NuGetGallery.Core/NuGetGallery.Core.csproj | 8 ++++---- .../Mail/BackgroundMarkdownMessageService.cs | 6 +++--- src/NuGetGallery/NuGetGallery.csproj | 14 +++++++------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/NuGetGallery.Core/NuGetGallery.Core.csproj b/src/NuGetGallery.Core/NuGetGallery.Core.csproj index 00b6654a50..c901f46416 100644 --- a/src/NuGetGallery.Core/NuGetGallery.Core.csproj +++ b/src/NuGetGallery.Core/NuGetGallery.Core.csproj @@ -203,10 +203,10 @@ - 2.30.0 + 2.31.0 - 2.30.0 + 2.31.0 1.2.0 @@ -242,10 +242,10 @@ 4.8.0 - 2.30.0 + 2.31.0 - 2.30.0 + 2.31.0 7.1.2 diff --git a/src/NuGetGallery/Infrastructure/Mail/BackgroundMarkdownMessageService.cs b/src/NuGetGallery/Infrastructure/Mail/BackgroundMarkdownMessageService.cs index ce47b656c7..49c3daca15 100644 --- a/src/NuGetGallery/Infrastructure/Mail/BackgroundMarkdownMessageService.cs +++ b/src/NuGetGallery/Infrastructure/Mail/BackgroundMarkdownMessageService.cs @@ -30,7 +30,7 @@ public BackgroundMarkdownMessageService( private Func _messageServiceFactory; private bool _sentMessage; - protected override Task SendMessageInternalAsync(MailMessage mailMessage, bool copySender = false, bool discloseSenderAddress = false) + protected override Task SendMessageInternalAsync(MailMessage mailMessage) { // Some MVC controller actions send more than one message. Since this method sends // the message async, we need a new IMessageService per email, to avoid calling @@ -39,7 +39,7 @@ protected override Task SendMessageInternalAsync(MailMessage mailMessage, bool c if (_sentMessage) { var newMessageService = _messageServiceFactory.Invoke(); - return newMessageService.SendMessageInternalAsync(mailMessage, copySender, discloseSenderAddress); + return newMessageService.SendMessageInternalAsync(mailMessage); } else { @@ -55,7 +55,7 @@ protected override Task SendMessageInternalAsync(MailMessage mailMessage, bool c { try { - await base.SendMessageInternalAsync(messageCopy, copySender, discloseSenderAddress); + await base.SendMessageInternalAsync(messageCopy); } catch (Exception ex) { diff --git a/src/NuGetGallery/NuGetGallery.csproj b/src/NuGetGallery/NuGetGallery.csproj index 87ac42dc56..796ba5b68d 100644 --- a/src/NuGetGallery/NuGetGallery.csproj +++ b/src/NuGetGallery/NuGetGallery.csproj @@ -1977,13 +1977,13 @@ 0.15.4 - 2.30.0 + 2.31.0 - 2.30.0 + 2.31.0 - 2.30.0 + 2.31.0 1.2.0 @@ -2210,16 +2210,16 @@ 4.8.0 - 2.30.0 + 2.31.0 - 2.30.0 + 2.31.0 - 2.30.0 + 2.31.0 - 2.30.0 + 2.31.0 1.0.0 From 7e155f7a657f45cfebfbf1aa8baa59b610cbc911 Mon Sep 17 00:00:00 2001 From: Andrei Grigorev Date: Wed, 31 Oct 2018 10:00:52 -0700 Subject: [PATCH 16/20] Validation message enhancements (#6608) * Package validation returns error and warning messages as IValidationMessage. PacakgesController converts those to JsonValidationMessage for Json responses and passes through IValidationMessage to Views for rendering. All PackageController Json responses use JsonValidationMessage in case of failure for consistency. ApiController always uses plain text version of the IValidationMessage. Updated tests to accomodate for the changes. * Initial view change * Safety fallbacks for error communication to the upload page. * Organized usings. * Making sure `PackageShouldNotBeSignedUserFixableValidationMessage` does not emit raw HTML. * Made type checks consistent * may it * PR feedback. --- src/NuGetGallery/Controllers/ApiController.cs | 8 +- .../Controllers/PackagesController.cs | 129 +++++++++--------- .../HttpStatusCodeWithServerWarningResult.cs | 5 + src/NuGetGallery/NuGetGallery.csproj | 4 + .../RequestModels/VerifyPackageRequest.cs | 2 +- .../Services/IValidationMessage.cs | 32 +++++ .../Services/JsonValidationMessage.cs | 41 ++++++ ...NotBeSignedUserFixableValidationMessage.cs | 25 ++++ .../Services/PackageUploadService.cs | 33 ++--- .../Services/PackageValidationResult.cs | 20 ++- .../Services/PackageValidationResultType.cs | 12 -- .../PlainTextOnlyValidationMessage.cs | 23 ++++ .../Views/Packages/_VerifyMetadata.cshtml | 43 ++++-- .../Controllers/ApiControllerFacts.cs | 6 +- .../Controllers/PackagesControllerFacts.cs | 114 +++++++++------- .../Services/PackageUploadServiceFacts.cs | 37 +++-- 16 files changed, 351 insertions(+), 183 deletions(-) create mode 100644 src/NuGetGallery/Services/IValidationMessage.cs create mode 100644 src/NuGetGallery/Services/JsonValidationMessage.cs create mode 100644 src/NuGetGallery/Services/PackageShouldNotBeSignedUserFixableValidationMessage.cs create mode 100644 src/NuGetGallery/Services/PlainTextOnlyValidationMessage.cs diff --git a/src/NuGetGallery/Controllers/ApiController.cs b/src/NuGetGallery/Controllers/ApiController.cs index dccd155681..b963c622b0 100644 --- a/src/NuGetGallery/Controllers/ApiController.cs +++ b/src/NuGetGallery/Controllers/ApiController.cs @@ -710,12 +710,12 @@ await AuditingService.SaveAuditRecordAsync( TelemetryService.TrackPackagePushEvent(package, currentUser, User.Identity); - var warnings = new List(); + var warnings = new List(); warnings.AddRange(beforeValidationResult.Warnings); warnings.AddRange(afterValidationResult.Warnings); if (package.SemVerLevelKey == SemVerLevelKey.SemVer2) { - warnings.Add(Strings.WarningSemVer2PackagePushed); + warnings.Add(new PlainTextOnlyValidationMessage(Strings.WarningSemVer2PackagePushed)); } return new HttpStatusCodeWithServerWarningResult(HttpStatusCode.Created, warnings); @@ -764,9 +764,7 @@ private static ActionResult GetActionResultOrNull(PackageValidationResult valida case PackageValidationResultType.Accepted: return null; case PackageValidationResultType.Invalid: - case PackageValidationResultType.PackageShouldNotBeSigned: - case PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates: - return new HttpStatusCodeWithBodyResult(HttpStatusCode.BadRequest, validationResult.Message); + return new HttpStatusCodeWithBodyResult(HttpStatusCode.BadRequest, validationResult.Message.PlainTextMessage); default: throw new NotImplementedException($"The package validation result type {validationResult.Type} is not supported."); } diff --git a/src/NuGetGallery/Controllers/PackagesController.cs b/src/NuGetGallery/Controllers/PackagesController.cs index 004ca349af..d5d084a707 100644 --- a/src/NuGetGallery/Controllers/PackagesController.cs +++ b/src/NuGetGallery/Controllers/PackagesController.cs @@ -253,7 +253,7 @@ private async Task UploadPackageInternal(SubmitPackageRequest mode var validationErrorMessage = GetErrorMessageOrNull(validationResult); if (validationErrorMessage != null) { - TempData["Message"] = validationErrorMessage; + TempData["Message"] = validationErrorMessage.PlainTextMessage; return View(model); } @@ -280,7 +280,7 @@ private async Task UploadPackageInternal(SubmitPackageRequest mode } var verifyRequest = new VerifyPackageRequest(packageMetadata, accountsAllowedOnBehalfOf, existingPackageRegistration); - verifyRequest.Warnings.AddRange(validationResult.Warnings); + verifyRequest.Warnings.AddRange(validationResult.Warnings.Select(w => new JsonValidationMessage(w))); verifyRequest.IsSymbolsPackage = false; model.InProgressUpload = verifyRequest; @@ -301,18 +301,18 @@ public virtual async Task UploadPackage(HttpPostedFileBase uploadFil { if (existingUploadFile != null) { - return Json(HttpStatusCode.Conflict, new[] { Strings.UploadPackage_UploadInProgress }); + return Json(HttpStatusCode.Conflict, new[] { new JsonValidationMessage(Strings.UploadPackage_UploadInProgress) }); } } if (uploadFile == null) { - return Json(HttpStatusCode.BadRequest, new[] { Strings.UploadFileIsRequired }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(Strings.UploadFileIsRequired) }); } if (!AllowedPackageExtentions.Contains(Path.GetExtension(uploadFile.FileName))) { - return Json(HttpStatusCode.BadRequest, new[] { Strings.UploadFileMustBeNuGetPackage }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(Strings.UploadFileMustBeNuGetPackage) }); } using (var uploadStream = uploadFile.InputStream) @@ -325,10 +325,10 @@ public virtual async Task UploadPackage(HttpPostedFileBase uploadFil var errors = ManifestValidator.Validate(packageArchiveReader.GetNuspec(), out nuspec, out packageMetadata).ToArray(); if (errors.Length > 0) { - var errorStrings = new List(); + var errorStrings = new List(); foreach (var error in errors) { - errorStrings.Add(error.ErrorMessage); + errorStrings.Add(new JsonValidationMessage(error.ErrorMessage)); } return Json(HttpStatusCode.BadRequest, errorStrings.ToArray()); @@ -377,12 +377,14 @@ private async Task UploadSymbolsPackageInternal(PackageArchiveReader if (ActionsRequiringPermissions.UploadSymbolPackage.CheckPermissionsOnBehalfOfAnyAccount( currentUser, existingPackageRegistration, out accountsAllowedOnBehalfOf) != PermissionsCheckResult.Allowed) { - return Json(HttpStatusCode.Conflict, new[] { string.Format(CultureInfo.CurrentCulture, Strings.PackageIdNotAvailable, existingPackageRegistration.Id) }); + return Json(HttpStatusCode.Conflict, new[] { + new JsonValidationMessage(string.Format(CultureInfo.CurrentCulture, Strings.PackageIdNotAvailable, existingPackageRegistration.Id)) }); } if (existingPackageRegistration.IsLocked) { - return Json(HttpStatusCode.Forbidden, new[] { string.Format(CultureInfo.CurrentCulture, Strings.PackageIsLocked, existingPackageRegistration.Id) }); + return Json(HttpStatusCode.Forbidden, new[] { + new JsonValidationMessage(string.Format(CultureInfo.CurrentCulture, Strings.PackageIsLocked, existingPackageRegistration.Id)) }); } // Save the uploaded file @@ -409,7 +411,7 @@ private async Task UploadPackageInternal(PackageArchiveReader packag if (foundEntryInFuture) { return Json(HttpStatusCode.BadRequest, new[] { - string.Format(CultureInfo.CurrentCulture, Strings.PackageEntryFromTheFuture, entryInTheFuture.Name) }); + new JsonValidationMessage(string.Format(CultureInfo.CurrentCulture, Strings.PackageEntryFromTheFuture, entryInTheFuture.Name)) }); } try @@ -425,7 +427,8 @@ private async Task UploadPackageInternal(PackageArchiveReader packag if (nuspec.GetMinClientVersion() > GalleryConstants.MaxSupportedMinClientVersion) { return Json(HttpStatusCode.BadRequest, new[] { - string.Format(CultureInfo.CurrentCulture, Strings.UploadPackage_MinClientVersionOutOfRange, nuspec.GetMinClientVersion()) }); + new JsonValidationMessage( + string.Format(CultureInfo.CurrentCulture, Strings.UploadPackage_MinClientVersionOutOfRange, nuspec.GetMinClientVersion())) }); } var id = nuspec.GetId(); @@ -438,7 +441,8 @@ private async Task UploadPackageInternal(PackageArchiveReader packag var version = nuspec.GetVersion().ToNormalizedString(); _telemetryService.TrackPackagePushNamespaceConflictEvent(id, version, currentUser, User.Identity); - return Json(HttpStatusCode.Conflict, new string[] { string.Format(CultureInfo.CurrentCulture, Strings.UploadPackage_IdNamespaceConflict) }); + return Json(HttpStatusCode.Conflict, new[] { + new JsonValidationMessage(string.Format(CultureInfo.CurrentCulture, Strings.UploadPackage_IdNamespaceConflict)) }); } // For existing package id verify if it is owned by the current user @@ -447,12 +451,16 @@ private async Task UploadPackageInternal(PackageArchiveReader packag if (ActionsRequiringPermissions.UploadNewPackageVersion.CheckPermissionsOnBehalfOfAnyAccount( currentUser, existingPackageRegistration, out accountsAllowedOnBehalfOf) != PermissionsCheckResult.Allowed) { - return Json(HttpStatusCode.Conflict, new[] { string.Format(CultureInfo.CurrentCulture, Strings.PackageIdNotAvailable, existingPackageRegistration.Id) }); + return Json(HttpStatusCode.Conflict, new[] { + new JsonValidationMessage( + string.Format(CultureInfo.CurrentCulture, Strings.PackageIdNotAvailable, existingPackageRegistration.Id)) }); } if (existingPackageRegistration.IsLocked) { - return Json(HttpStatusCode.Forbidden, new[] { string.Format(CultureInfo.CurrentCulture, Strings.PackageIsLocked, existingPackageRegistration.Id) }); + return Json(HttpStatusCode.Forbidden, new[] { + new JsonValidationMessage( + string.Format(CultureInfo.CurrentCulture, Strings.PackageIsLocked, existingPackageRegistration.Id)) }); } } @@ -496,7 +504,7 @@ await _packageDeleteService.HardDeletePackagesAsync( existingPackage.Version); } - return Json(HttpStatusCode.Conflict, new[] { message }); + return Json(HttpStatusCode.Conflict, new[] { new JsonValidationMessage(message) }); } } @@ -519,18 +527,18 @@ private async Task GetVerifyPackageView(User currentUser, bool isSymbolsPackageUpload, bool hasExistingSymbolsPackageAvailable) { - IReadOnlyList warnings = new List(); + IReadOnlyList warnings = new List(); using (Stream uploadedFile = await _uploadFileService.GetUploadFileAsync(currentUser.Key)) { if (uploadedFile == null) { - return Json(HttpStatusCode.BadRequest, new[] { Strings.UploadFileIsRequired }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(Strings.UploadFileIsRequired) }); } var packageArchiveReader = await SafeCreatePackage(currentUser, uploadedFile); if (packageArchiveReader == null) { - return Json(HttpStatusCode.BadRequest, new[] { Strings.UploadFileIsRequired }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(Strings.UploadFileIsRequired) }); } try @@ -543,7 +551,7 @@ private async Task GetVerifyPackageView(User currentUser, { _telemetryService.TraceException(ex); - return Json(HttpStatusCode.BadRequest, new[] { ex.GetUserSafeMessage() }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(ex.GetUserSafeMessage()) }); } if (!isSymbolsPackageUpload) @@ -562,7 +570,7 @@ private async Task GetVerifyPackageView(User currentUser, var model = new VerifyPackageRequest(packageMetadata, accountsAllowedOnBehalfOf, existingPackageRegistration); model.IsSymbolsPackage = isSymbolsPackageUpload; model.HasExistingAvailableSymbols = hasExistingSymbolsPackageAvailable; - model.Warnings.AddRange(warnings); + model.Warnings.AddRange(warnings.Select(w => new JsonValidationMessage(w))); return Json(model); } @@ -1534,22 +1542,22 @@ public virtual async Task Edit(string id, string version, VerifyPack var package = _packageService.FindPackageByIdAndVersion(id, version); if (package == null) { - return Json(HttpStatusCode.NotFound, new[] { string.Format(Strings.PackageWithIdAndVersionNotFound, id, version) }); + return Json(HttpStatusCode.NotFound, new[] { new JsonValidationMessage(string.Format(Strings.PackageWithIdAndVersionNotFound, id, version)) }); } if (ActionsRequiringPermissions.EditPackage.CheckPermissionsOnBehalfOfAnyAccount(GetCurrentUser(), package) != PermissionsCheckResult.Allowed) { - return Json(HttpStatusCode.Forbidden, new[] { Strings.Unauthorized }); + return Json(HttpStatusCode.Forbidden, new[] { new JsonValidationMessage(Strings.Unauthorized) }); } if (package.PackageRegistration.IsLocked) { - return Json(HttpStatusCode.Forbidden, new[] { string.Format(CultureInfo.CurrentCulture, Strings.PackageIsLocked, package.PackageRegistration.Id) }); + return Json(HttpStatusCode.Forbidden, new[] { new JsonValidationMessage(string.Format(CultureInfo.CurrentCulture, Strings.PackageIsLocked, package.PackageRegistration.Id)) }); } if (!ModelState.IsValid) { - var errorMessages = ModelState.Values.SelectMany(v => v.Errors.Select(e => e.ErrorMessage)); + var errorMessages = ModelState.Values.SelectMany(v => v.Errors.Select(e => new JsonValidationMessage(e.ErrorMessage))); return Json(HttpStatusCode.BadRequest, errorMessages); } @@ -1575,12 +1583,12 @@ public virtual async Task Edit(string id, string version, VerifyPack catch (ArgumentException ex) when (ex.Message.Contains(Strings.ReadMeUrlHostInvalid)) { // Thrown when ReadmeUrlHost is invalid. - return Json(HttpStatusCode.BadRequest, new[] { Strings.ReadMeUrlHostInvalid }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(Strings.ReadMeUrlHostInvalid) }); } catch (InvalidOperationException ex) { // Thrown when readme max length exceeded, or unexpected file extension. - return Json(HttpStatusCode.BadRequest, new[] { ex.Message }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(ex.Message) }); } } @@ -1771,7 +1779,7 @@ public virtual async Task VerifyPackage(VerifyPackageRequest formDat { if (!ModelState.IsValid) { - var errorMessages = ModelState.Values.SelectMany(v => v.Errors.Select(e => e.ErrorMessage)); + var errorMessages = ModelState.Values.SelectMany(v => v.Errors.Select(e => new JsonValidationMessage(e.ErrorMessage))); return Json(HttpStatusCode.BadRequest, errorMessages); } @@ -1782,13 +1790,13 @@ public virtual async Task VerifyPackage(VerifyPackageRequest formDat if (owner == null) { - var message = string.Format(CultureInfo.CurrentCulture, Strings.VerifyPackage_UserNonExistent, formData.Owner); + var message = new JsonValidationMessage(string.Format(CultureInfo.CurrentCulture, Strings.VerifyPackage_UserNonExistent, formData.Owner)); return Json(HttpStatusCode.BadRequest, new[] { message }); } if (!owner.Confirmed) { - var message = string.Format(CultureInfo.CurrentCulture, Strings.VerifyPackage_OwnerUnconfirmed, formData.Owner); + var message = new JsonValidationMessage(string.Format(CultureInfo.CurrentCulture, Strings.VerifyPackage_OwnerUnconfirmed, formData.Owner)); return Json(HttpStatusCode.BadRequest, new[] { message }); } @@ -1796,14 +1804,14 @@ public virtual async Task VerifyPackage(VerifyPackageRequest formDat { if (uploadFile == null) { - return Json(HttpStatusCode.BadRequest, new[] { Strings.VerifyPackage_UploadNotFound }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(Strings.VerifyPackage_UploadNotFound) }); } var packageArchiveReader = await SafeCreatePackage(currentUser, uploadFile); if (packageArchiveReader == null) { // Send the user back - return Json(HttpStatusCode.BadRequest, new[] { Strings.VerifyPackage_UnexpectedError }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(Strings.VerifyPackage_UnexpectedError) }); } Debug.Assert(packageArchiveReader != null); @@ -1820,7 +1828,7 @@ public virtual async Task VerifyPackage(VerifyPackageRequest formDat && string.Equals(packageMetadata.Version.ToFullStringSafe(), formData.Version, StringComparison.OrdinalIgnoreCase) && string.Equals(packageMetadata.Version.OriginalVersion, formData.OriginalVersion, StringComparison.OrdinalIgnoreCase))) { - return Json(HttpStatusCode.BadRequest, new[] { Strings.VerifyPackage_PackageFileModified }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(Strings.VerifyPackage_PackageFileModified) }); } } @@ -1882,7 +1890,9 @@ public virtual async Task VerifySymbolsPackageInternal( if (existingPackageRegistration.IsLocked) { - return Json(HttpStatusCode.Forbidden, new[] { string.Format(CultureInfo.CurrentCulture, Strings.PackageIsLocked, existingPackageRegistration.Id) }); + return Json(HttpStatusCode.Forbidden, new[] { + new JsonValidationMessage( + string.Format(CultureInfo.CurrentCulture, Strings.PackageIsLocked, existingPackageRegistration.Id)) }); } // Evaluate the permissions for the owner, the permissions for uploading a symbols should be same as that of @@ -1896,7 +1906,7 @@ public virtual async Task VerifySymbolsPackageInternal( var message = string.Format(CultureInfo.CurrentCulture, Strings.UploadPackage_NewVersionOnBehalfOfUserNotAllowed, currentUser.Username, owner.Username); - return Json(HttpStatusCode.BadRequest, new[] { message }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(message) }); } if (checkPermissionsOfUploadNewVersion == PermissionsCheckResult.PackageRegistrationFailure) @@ -1905,11 +1915,11 @@ public virtual async Task VerifySymbolsPackageInternal( var message = string.Format(CultureInfo.CurrentCulture, Strings.VerifyPackage_OwnerInvalid, owner.Username, existingPackageRegistration.Id); - return Json(HttpStatusCode.BadRequest, new[] { message }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(message) }); } // An unknown error occurred. - return Json(HttpStatusCode.BadRequest, new[] { Strings.VerifyPackage_UnexpectedError }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(Strings.VerifyPackage_UnexpectedError) }); } var commitResult = await _symbolPackageUploadService.CreateAndUploadSymbolsPackage( @@ -1922,7 +1932,7 @@ public virtual async Task VerifySymbolsPackageInternal( break; case PackageCommitResult.Conflict: TempData["Message"] = Strings.SymbolsPackage_ConflictValidating; - return Json(HttpStatusCode.Conflict, new[] { Strings.SymbolsPackage_ConflictValidating }); + return Json(HttpStatusCode.Conflict, new[] { new JsonValidationMessage(Strings.SymbolsPackage_ConflictValidating) }); default: throw new NotImplementedException($"The symbols package commit result {commitResult} is not supported."); } @@ -1948,7 +1958,7 @@ await _auditingService.SaveAuditRecordAsync( { ex.Log(); _telemetryService.TrackSymbolPackagePushFailureEvent(packageId, packageVersion); - return Json(HttpStatusCode.BadRequest, new[] { Strings.VerifyPackage_UnexpectedError }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(Strings.VerifyPackage_UnexpectedError) }); } } @@ -1990,7 +2000,7 @@ public virtual async Task VerifyPackageInternal( var message = string.Format(CultureInfo.CurrentCulture, Strings.UploadPackage_NewIdOnBehalfOfUserNotAllowed, currentUser.Username, owner.Username); - return Json(HttpStatusCode.BadRequest, new[] { message }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(message) }); } else if (checkPermissionsOfUploadNewId == PermissionsCheckResult.ReservedNamespaceFailure) { @@ -1999,11 +2009,11 @@ public virtual async Task VerifyPackageInternal( _telemetryService.TrackPackagePushNamespaceConflictEvent(packageId, version, currentUser, User.Identity); var message = string.Format(CultureInfo.CurrentCulture, Strings.UploadPackage_IdNamespaceConflict); - return Json(HttpStatusCode.Conflict, new string[] { message }); + return Json(HttpStatusCode.Conflict, new[] { new JsonValidationMessage(message) }); } // An unknown error occurred. - return Json(HttpStatusCode.BadRequest, new[] { Strings.VerifyPackage_UnexpectedError }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(Strings.VerifyPackage_UnexpectedError) }); } } else @@ -2017,7 +2027,7 @@ public virtual async Task VerifyPackageInternal( var message = string.Format(CultureInfo.CurrentCulture, Strings.UploadPackage_NewVersionOnBehalfOfUserNotAllowed, currentUser.Username, owner.Username); - return Json(HttpStatusCode.BadRequest, new[] { message }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(message) }); } if (checkPermissionsOfUploadNewVersion == PermissionsCheckResult.PackageRegistrationFailure) @@ -2026,11 +2036,11 @@ public virtual async Task VerifyPackageInternal( var message = string.Format(CultureInfo.CurrentCulture, Strings.VerifyPackage_OwnerInvalid, owner.Username, existingPackageRegistration.Id); - return Json(HttpStatusCode.BadRequest, new[] { message }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(message) }); } // An unknown error occurred. - return Json(HttpStatusCode.BadRequest, new[] { Strings.VerifyPackage_UnexpectedError }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(Strings.VerifyPackage_UnexpectedError) }); } } @@ -2057,7 +2067,7 @@ public virtual async Task VerifyPackageInternal( { _telemetryService.TraceException(ex); - return Json(HttpStatusCode.BadRequest, new[] { ex.Message }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(ex.Message) }); } var packagePolicyResult = await _securityPolicyService.EvaluatePackagePoliciesAsync( @@ -2069,7 +2079,7 @@ public virtual async Task VerifyPackageInternal( if (!packagePolicyResult.Success) { - return Json(HttpStatusCode.BadRequest, new[] { packagePolicyResult.ErrorMessage }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(packagePolicyResult.ErrorMessage) }); } // Perform validations that require the package already being in the entity context. @@ -2102,12 +2112,12 @@ public virtual async Task VerifyPackageInternal( catch (ArgumentException ex) when (ex.Message.Contains(Strings.ReadMeUrlHostInvalid)) { // Thrown when ReadmeUrlHost is invalid. - return Json(HttpStatusCode.BadRequest, new[] { Strings.ReadMeUrlHostInvalid }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(Strings.ReadMeUrlHostInvalid) }); } catch (InvalidOperationException ex) { // Thrown when readme max length exceeded, or unexpected file extension. - return Json(HttpStatusCode.BadRequest, new[] { ex.Message }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(ex.Message) }); } } @@ -2132,7 +2142,7 @@ public virtual async Task VerifyPackageInternal( break; case PackageCommitResult.Conflict: TempData["Message"] = Strings.UploadPackage_IdVersionConflict; - return Json(HttpStatusCode.Conflict, new[] { Strings.UploadPackage_IdVersionConflict }); + return Json(HttpStatusCode.Conflict, new[] { new JsonValidationMessage(Strings.UploadPackage_IdVersionConflict) }); default: throw new NotImplementedException($"The package commit result {commitResult} is not supported."); } @@ -2166,7 +2176,7 @@ await _auditingService.SaveAuditRecordAsync( catch (Exception e) { e.Log(); - return Json(HttpStatusCode.BadRequest, new[] { Strings.VerifyPackage_UnexpectedError }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(Strings.VerifyPackage_UnexpectedError) }); } await DeleteUploadedFileForUser(currentUser, uploadFile); @@ -2212,7 +2222,7 @@ private JsonResult GetJsonResultOrNull(PackageValidationResult validationResult) return null; } - return Json(HttpStatusCode.BadRequest, new[] { errorMessage }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(errorMessage) }); } private JsonResult GetJsonResultOrNull(SymbolPackageValidationResult validationResult) @@ -2236,21 +2246,17 @@ private JsonResult GetJsonResultOrNull(SymbolPackageValidationResult validationR throw new NotImplementedException($"The symbol package validation result type {validationResult.Type} is not supported."); } - return Json(httpStatusCode, new[] { validationResult.Message }); + return Json(httpStatusCode, new[] { new JsonValidationMessage(validationResult.Message) }); } - private static string GetErrorMessageOrNull(PackageValidationResult validationResult) + private static IValidationMessage GetErrorMessageOrNull(PackageValidationResult validationResult) { switch (validationResult.Type) { case PackageValidationResultType.Accepted: return null; case PackageValidationResultType.Invalid: - case PackageValidationResultType.PackageShouldNotBeSigned: return validationResult.Message; - case PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates: - return validationResult.Message + " " + - Strings.UploadPackage_PackageIsSignedButMissingCertificate_ManageCertificate; default: throw new NotImplementedException($"The package validation result type {validationResult.Type} is not supported."); } @@ -2314,17 +2320,18 @@ public virtual async Task PreviewReadMe(ReadMeRequest formData) { if (formData == null || !_readMeService.HasReadMeSource(formData)) { - return Json(HttpStatusCode.BadRequest, new[] { Strings.PreviewReadMe_ReadMeMissing }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(Strings.PreviewReadMe_ReadMeMissing) }); } try { var readMeHtml = await _readMeService.GetReadMeHtmlAsync(formData, Request.ContentEncoding); - return Json(new[] { readMeHtml }); + return Json(new[] { new JsonValidationMessage(readMeHtml) }); } catch (Exception ex) { - return Json(HttpStatusCode.BadRequest, new[] { string.Format(CultureInfo.CurrentCulture, Strings.PreviewReadMe_ConversionFailed, ex.Message) }); + return Json(HttpStatusCode.BadRequest, new[] { + new JsonValidationMessage(string.Format(CultureInfo.CurrentCulture, Strings.PreviewReadMe_ConversionFailed, ex.Message)) }); } } @@ -2412,7 +2419,7 @@ private JsonResult FailedToReadFile(Exception ex) message = ex.Message; } - return Json(HttpStatusCode.BadRequest, new[] { message }); + return Json(HttpStatusCode.BadRequest, new[] { new JsonValidationMessage(message) }); } // this method exists to make unit testing easier diff --git a/src/NuGetGallery/Infrastructure/HttpStatusCodeWithServerWarningResult.cs b/src/NuGetGallery/Infrastructure/HttpStatusCodeWithServerWarningResult.cs index 43dae46abb..f87f766885 100644 --- a/src/NuGetGallery/Infrastructure/HttpStatusCodeWithServerWarningResult.cs +++ b/src/NuGetGallery/Infrastructure/HttpStatusCodeWithServerWarningResult.cs @@ -18,6 +18,11 @@ public HttpStatusCodeWithServerWarningResult(HttpStatusCode statusCode, IReadOnl Warnings = warnings ?? new string[0]; } + public HttpStatusCodeWithServerWarningResult(HttpStatusCode statusCode, IReadOnlyList warnings) + : this(statusCode, warnings.Select(w => w.PlainTextMessage).ToList()) + { + } + public override void ExecuteResult(ControllerContext context) { var response = context.RequestContext.HttpContext.Response; diff --git a/src/NuGetGallery/NuGetGallery.csproj b/src/NuGetGallery/NuGetGallery.csproj index 87ac42dc56..4dbb2a82a7 100644 --- a/src/NuGetGallery/NuGetGallery.csproj +++ b/src/NuGetGallery/NuGetGallery.csproj @@ -426,6 +426,10 @@ + + + + diff --git a/src/NuGetGallery/RequestModels/VerifyPackageRequest.cs b/src/NuGetGallery/RequestModels/VerifyPackageRequest.cs index c4619a1e10..212a33ab24 100644 --- a/src/NuGetGallery/RequestModels/VerifyPackageRequest.cs +++ b/src/NuGetGallery/RequestModels/VerifyPackageRequest.cs @@ -120,7 +120,7 @@ public VerifyPackageRequest(PackageMetadata packageMetadata, IEnumerable p public bool IsSymbolsPackage { get; set; } public bool HasExistingAvailableSymbols { get; set; } - public List Warnings { get; set; } = new List(); + public List Warnings { get; set; } = new List(); private static IReadOnlyCollection ParseUserList(IEnumerable users) { diff --git a/src/NuGetGallery/Services/IValidationMessage.cs b/src/NuGetGallery/Services/IValidationMessage.cs new file mode 100644 index 0000000000..0d4f117320 --- /dev/null +++ b/src/NuGetGallery/Services/IValidationMessage.cs @@ -0,0 +1,32 @@ +// 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 +{ + /// + /// Represents a validation message that may include a raw HTML message (if we want to present some links in the message, for example). + /// + /// + /// Implementations should sanitize all input used to generate value. + /// We should never have a generic implementation that would accept a raw HTML message as a constructor argument and + /// simply returns it as a value of . + /// + public interface IValidationMessage + { + /// + /// Plain text representation of the validation message. If pasted into HTML, will be html encoded. + /// + string PlainTextMessage { get; } + + /// + /// An indicator that raw HTML representation is available. + /// + bool HasRawHtmlRepresentation { get; } + + /// + /// HANDLE WITH EXTREME CARE. Raw HTML representation of the message. + /// Under no conditions may it contain unvalidated user data. + /// + string RawHtmlMessage { get; } + } +} diff --git a/src/NuGetGallery/Services/JsonValidationMessage.cs b/src/NuGetGallery/Services/JsonValidationMessage.cs new file mode 100644 index 0000000000..42758e7e20 --- /dev/null +++ b/src/NuGetGallery/Services/JsonValidationMessage.cs @@ -0,0 +1,41 @@ +// 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; + +namespace NuGetGallery +{ + /// + /// Validation message representation for returning in Json responses for Gallery web pages. + /// + public class JsonValidationMessage + { + public JsonValidationMessage(string message) + { + PlainTextMessage = message ?? throw new ArgumentNullException(nameof(message)); + RawHtmlMessage = null; + } + + public JsonValidationMessage(IValidationMessage message) + { + if (message == null) + { + throw new ArgumentNullException(nameof(message)); + } + + if (message.HasRawHtmlRepresentation) + { + PlainTextMessage = null; + RawHtmlMessage = message.RawHtmlMessage; + } + else + { + PlainTextMessage = message.PlainTextMessage; + RawHtmlMessage = null; + } + } + + public string PlainTextMessage { get; } + public string RawHtmlMessage { get; } + } +} \ No newline at end of file diff --git a/src/NuGetGallery/Services/PackageShouldNotBeSignedUserFixableValidationMessage.cs b/src/NuGetGallery/Services/PackageShouldNotBeSignedUserFixableValidationMessage.cs new file mode 100644 index 0000000000..8787ac691a --- /dev/null +++ b/src/NuGetGallery/Services/PackageShouldNotBeSignedUserFixableValidationMessage.cs @@ -0,0 +1,25 @@ +// 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.Web; + +namespace NuGetGallery +{ + /// + /// Represents an error message to be displayed when user uploads a signed package but theirs account + /// was not setup to accept signed packages. We want to display some additional text when the error is + /// presented on a web page, so this class abuses the messaging a little, no actual HTML is generated. + /// + public class PackageShouldNotBeSignedUserFixableValidationMessage : IValidationMessage + { + public string PlainTextMessage => Strings.UploadPackage_PackageIsSignedButMissingCertificate_CurrentUserCanManageCertificates; + + public bool HasRawHtmlRepresentation => true; + + public string RawHtmlMessage + => HttpUtility.HtmlEncode( + Strings.UploadPackage_PackageIsSignedButMissingCertificate_CurrentUserCanManageCertificates + + " " + + Strings.UploadPackage_PackageIsSignedButMissingCertificate_ManageCertificate); + } +} \ No newline at end of file diff --git a/src/NuGetGallery/Services/PackageUploadService.cs b/src/NuGetGallery/Services/PackageUploadService.cs index 1b840e0d73..8a363ae83f 100644 --- a/src/NuGetGallery/Services/PackageUploadService.cs +++ b/src/NuGetGallery/Services/PackageUploadService.cs @@ -48,7 +48,7 @@ public PackageUploadService( public async Task ValidateBeforeGeneratePackageAsync(PackageArchiveReader nuGetPackage, PackageMetadata packageMetadata) { - var warnings = new List(); + var warnings = new List(); var result = await CheckPackageEntryCountAsync(nuGetPackage, warnings); @@ -121,7 +121,7 @@ private PackageValidationResult CheckLicenseMetadata(PackageArchiveReader nuGetP private async Task CheckPackageEntryCountAsync( PackageArchiveReader nuGetPackage, - List warnings) + List warnings) { if (!_config.RejectPackagesWithTooManyPackageEntries) { @@ -152,7 +152,7 @@ private async Task CheckPackageEntryCountAsync( /// 1. If the type is "git" - allow the URL scheme "git://" or "https://". We will translate "git://" to "https://" at display time for known domains. /// 2. For types other then "git" - URL scheme should be "https://" /// - private PackageValidationResult CheckRepositoryMetadata(PackageMetadata packageMetadata, List warnings) + private PackageValidationResult CheckRepositoryMetadata(PackageMetadata packageMetadata, List warnings) { if (packageMetadata.RepositoryUrl == null) { @@ -164,14 +164,14 @@ private PackageValidationResult CheckRepositoryMetadata(PackageMetadata packageM { if (!packageMetadata.RepositoryUrl.IsGitProtocol() && !packageMetadata.RepositoryUrl.IsHttpsProtocol()) { - warnings.Add(Strings.WarningNotHttpsOrGitRepositoryUrlScheme); + warnings.Add(new PlainTextOnlyValidationMessage(Strings.WarningNotHttpsOrGitRepositoryUrlScheme)); } } else { if (!packageMetadata.RepositoryUrl.IsHttpsProtocol()) { - warnings.Add(Strings.WarningNotHttpsRepositoryUrlScheme); + warnings.Add(new PlainTextOnlyValidationMessage(Strings.WarningNotHttpsRepositoryUrlScheme)); } } @@ -189,7 +189,7 @@ private PackageValidationResult CheckRepositoryMetadata(PackageMetadata packageM /// The package validation result or null. private async Task CheckForUnsignedPushAfterAuthorSignedAsync( PackageArchiveReader nuGetPackage, - List warnings) + List warnings) { // If the package is signed, there's no problem. if (await nuGetPackage.IsSignedAsync(CancellationToken.None)) @@ -220,9 +220,10 @@ private async Task CheckForUnsignedPushAfterAuthorSigne if (previousPackage != null && previousPackage.CertificateKey.HasValue) { - warnings.Add(string.Format( - Strings.UploadPackage_SignedToUnsignedTransition, - previousPackage.Version.ToNormalizedString())); + warnings.Add(new PlainTextOnlyValidationMessage( + string.Format( + Strings.UploadPackage_SignedToUnsignedTransition, + previousPackage.Version.ToNormalizedString()))); } return null; @@ -271,14 +272,11 @@ private async Task ValidateSignatureFilePresenceAsync( { if (requiredSigner == currentUser) { - return new PackageValidationResult( - PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates, - Strings.UploadPackage_PackageIsSignedButMissingCertificate_CurrentUserCanManageCertificates); + return PackageValidationResult.Invalid(new PackageShouldNotBeSignedUserFixableValidationMessage()); } else { - return new PackageValidationResult( - PackageValidationResultType.PackageShouldNotBeSigned, + return PackageValidationResult.Invalid( string.Format( Strings.UploadPackage_PackageIsSignedButMissingCertificate_RequiredSigner, requiredSigner.Username)); @@ -295,14 +293,11 @@ private async Task ValidateSignatureFilePresenceAsync( // someone else for help. if (isCurrentUserAnOwner) { - return new PackageValidationResult( - PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates, - Strings.UploadPackage_PackageIsSignedButMissingCertificate_CurrentUserCanManageCertificates); + return PackageValidationResult.Invalid(new PackageShouldNotBeSignedUserFixableValidationMessage()); } else { - return new PackageValidationResult( - PackageValidationResultType.PackageShouldNotBeSigned, + return PackageValidationResult.Invalid( string.Format( Strings.UploadPackage_PackageIsSignedButMissingCertificate_RequiredSigner, owner.Username)); diff --git a/src/NuGetGallery/Services/PackageValidationResult.cs b/src/NuGetGallery/Services/PackageValidationResult.cs index b132a8dd05..30ba7c2565 100644 --- a/src/NuGetGallery/Services/PackageValidationResult.cs +++ b/src/NuGetGallery/Services/PackageValidationResult.cs @@ -12,14 +12,19 @@ namespace NuGetGallery /// public class PackageValidationResult { - private static readonly IReadOnlyList EmptyList = new string[0]; + private static readonly IReadOnlyList EmptyList = new IValidationMessage[0]; - public PackageValidationResult(PackageValidationResultType type, string message) + public PackageValidationResult(PackageValidationResultType type, IValidationMessage message) : this(type, message, warnings: null) { } - public PackageValidationResult(PackageValidationResultType type, string message, IReadOnlyList warnings) + public PackageValidationResult(PackageValidationResultType type, string message) + : this(type, new PlainTextOnlyValidationMessage(message)) + { + } + + public PackageValidationResult(PackageValidationResultType type, IValidationMessage message, IReadOnlyList warnings) { if (type != PackageValidationResultType.Accepted && message == null) { @@ -32,8 +37,8 @@ public PackageValidationResult(PackageValidationResultType type, string message, } public PackageValidationResultType Type { get; } - public string Message { get; } - public IReadOnlyList Warnings { get; } + public IValidationMessage Message { get; } + public IReadOnlyList Warnings { get; } public static PackageValidationResult Accepted() { @@ -43,7 +48,7 @@ public static PackageValidationResult Accepted() warnings: null); } - public static PackageValidationResult AcceptedWithWarnings(IReadOnlyList warnings) + public static PackageValidationResult AcceptedWithWarnings(IReadOnlyList warnings) { return new PackageValidationResult( PackageValidationResultType.Accepted, @@ -52,6 +57,9 @@ public static PackageValidationResult AcceptedWithWarnings(IReadOnlyList } public static PackageValidationResult Invalid(string message) + => Invalid(new PlainTextOnlyValidationMessage(message)); + + public static PackageValidationResult Invalid(IValidationMessage message) { if (message == null) { diff --git a/src/NuGetGallery/Services/PackageValidationResultType.cs b/src/NuGetGallery/Services/PackageValidationResultType.cs index 8215cc61e7..568d64f7a7 100644 --- a/src/NuGetGallery/Services/PackageValidationResultType.cs +++ b/src/NuGetGallery/Services/PackageValidationResultType.cs @@ -18,17 +18,5 @@ public enum PackageValidationResultType /// The package is invalid based on the package content. /// Invalid, - - /// - /// The package is invalid because it should not be signed given the current required signer certificates (or - /// lack thereof). - /// - PackageShouldNotBeSigned, - - /// - /// Similar to but the current user can manage certificates and - /// potentially remediate the situation. - /// - PackageShouldNotBeSignedButCanManageCertificates, } } \ No newline at end of file diff --git a/src/NuGetGallery/Services/PlainTextOnlyValidationMessage.cs b/src/NuGetGallery/Services/PlainTextOnlyValidationMessage.cs new file mode 100644 index 0000000000..a15dd2360c --- /dev/null +++ b/src/NuGetGallery/Services/PlainTextOnlyValidationMessage.cs @@ -0,0 +1,23 @@ +// 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 +{ + /// + /// Cautious default implementation of that does not allow + /// user to specify raw html response. + /// + public class PlainTextOnlyValidationMessage : IValidationMessage + { + public PlainTextOnlyValidationMessage(string validationMessage) + { + PlainTextMessage = validationMessage; + } + + public string PlainTextMessage { get; } + + public bool HasRawHtmlRepresentation => false; + + public string RawHtmlMessage => null; + } +} \ No newline at end of file diff --git a/src/NuGetGallery/Views/Packages/_VerifyMetadata.cshtml b/src/NuGetGallery/Views/Packages/_VerifyMetadata.cshtml index ce4c4422f7..07b10a16b5 100644 --- a/src/NuGetGallery/Views/Packages/_VerifyMetadata.cshtml +++ b/src/NuGetGallery/Views/Packages/_VerifyMetadata.cshtml @@ -1,6 +1,19 @@  @@ -46,14 +59,24 @@
@ViewHelpers.AlertWarning( - @ - We found the following issue(s): -
    -
    • -
- We recommend that you fix these issues and upload a new package. - Read more
- ) + @ + We found the following issue(s): +
    + +
    • + + + +
    • + + +
    • + + +
+ We recommend that you fix these issues and upload a new package. + Read more
+ )
diff --git a/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs index 407d889ab2..3d36ff7fb3 100644 --- a/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs @@ -905,7 +905,7 @@ public async Task WillReturnValidationWarnings() .MockPackageUploadService .Setup(x => x.ValidateBeforeGeneratePackageAsync( It.IsAny(), It.IsAny())) - .ReturnsAsync(PackageValidationResult.AcceptedWithWarnings(new[] { messageA })); + .ReturnsAsync(PackageValidationResult.AcceptedWithWarnings(new[] { new PlainTextOnlyValidationMessage(messageA) })); controller .MockPackageUploadService .Setup(x => x.ValidateAfterGeneratePackageAsync( @@ -914,7 +914,7 @@ public async Task WillReturnValidationWarnings() It.IsAny(), It.IsAny(), It.IsAny())) - .ReturnsAsync(PackageValidationResult.AcceptedWithWarnings(new[] { messageB })); + .ReturnsAsync(PackageValidationResult.AcceptedWithWarnings(new[] { new PlainTextOnlyValidationMessage(messageB) })); // Act ActionResult result = await controller.CreatePackagePut(); @@ -929,8 +929,6 @@ public async Task WillReturnValidationWarnings() [Theory] [InlineData(PackageValidationResultType.Invalid)] - [InlineData(PackageValidationResultType.PackageShouldNotBeSigned)] - [InlineData(PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates)] public async Task WillReturnValidationMessageWhenValidationFails(PackageValidationResultType type) { // Arrange diff --git a/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs index 6ae3f21e5c..338225a3fd 100644 --- a/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs @@ -275,7 +275,7 @@ private static Mock GetValidPackageUploadService(string p It.IsAny(), It.IsAny())) .ReturnsAsync(new PackageValidationResult( - type, message: "Something", warnings: null)); + type, message: new PlainTextOnlyValidationMessage("Something"), warnings: null)); return fakePackageUploadService; } @@ -4081,7 +4081,7 @@ public async Task WillSetShowWarningsFromValidationBeforeGeneratePackage() var fakePackageUploadService = new Mock(); fakePackageUploadService .Setup(x => x.ValidateBeforeGeneratePackageAsync(It.IsAny(), It.IsAny())) - .ReturnsAsync(PackageValidationResult.AcceptedWithWarnings(new[] { expectedMessage })); + .ReturnsAsync(PackageValidationResult.AcceptedWithWarnings(new[] { new PlainTextOnlyValidationMessage(expectedMessage) })); var controller = CreateController( GetConfigurationService(), @@ -4096,7 +4096,7 @@ public async Task WillSetShowWarningsFromValidationBeforeGeneratePackage() Assert.NotNull(result.InProgressUpload); Assert.False(controller.TempData.ContainsKey("Message")); var actualMessage = Assert.Single(result.InProgressUpload.Warnings); - Assert.Equal(expectedMessage, actualMessage); + Assert.Equal(expectedMessage, actualMessage.PlainTextMessage); } [Fact] @@ -4177,7 +4177,7 @@ public async Task WillShowViewWithErrorsIfPackageFileIsNull() var result = await controller.UploadPackage(null) as JsonResult; Assert.NotNull(result); - Assert.Equal(Strings.UploadFileIsRequired, (result.Data as string[])[0]); + Assert.Equal(Strings.UploadFileIsRequired, (result.Data as JsonValidationMessage[])[0].PlainTextMessage); } [Fact] @@ -4191,7 +4191,7 @@ public async Task WillShowViewWithErrorsIfFileIsNotANuGetPackage() var result = await controller.UploadPackage(fakeUploadedFile.Object) as JsonResult; Assert.NotNull(result); - Assert.Equal(Strings.UploadFileMustBeNuGetPackage, (result.Data as string[])[0]); + Assert.Equal(Strings.UploadFileMustBeNuGetPackage, (result.Data as JsonValidationMessage[])[0].PlainTextMessage); } [Fact] @@ -4211,7 +4211,7 @@ public async Task WillShowViewWithErrorsIfEnsureValidThrowsException() var result = await controller.UploadPackage(fakeUploadedFile.Object) as JsonResult; Assert.NotNull(result); - Assert.Equal(Strings.FailedToReadUploadFile, (result.Data as string[])[0]); + Assert.Equal(Strings.FailedToReadUploadFile, (result.Data as JsonValidationMessage[])[0].PlainTextMessage); } private const string EnsureValidExceptionMessage = "naughty package"; @@ -4239,7 +4239,9 @@ public async Task WillShowViewWithErrorsIfEnsureValidThrowsExceptionMessage(Type var result = await controller.UploadPackage(fakeUploadedFile.Object) as JsonResult; Assert.NotNull(result); - Assert.Equal(expectExceptionMessageInResponse ? EnsureValidExceptionMessage : Strings.FailedToReadUploadFile, (result.Data as string[])[0]); + Assert.Equal( + expectExceptionMessageInResponse ? EnsureValidExceptionMessage : Strings.FailedToReadUploadFile, + (result.Data as JsonValidationMessage[])[0].PlainTextMessage); } [Theory] @@ -4265,7 +4267,7 @@ public async Task WillShowViewWithErrorsIfPackageIdIsInvalid(string packageId) var result = await controller.UploadPackage(fakeUploadedFile.Object) as JsonResult; Assert.NotNull(result); - Assert.Equal($"The package manifest contains an invalid ID: '{packageId}'", (result.Data as string[])[0]); + Assert.Equal($"The package manifest contains an invalid ID: '{packageId}'", (result.Data as JsonValidationMessage[])[0].PlainTextMessage); } [Theory] @@ -4287,7 +4289,7 @@ public async Task WillShowViewWithErrorsIfPackageIdIsBreaksParsing(string packag var result = await controller.UploadPackage(fakeUploadedFile.Object) as JsonResult; Assert.NotNull(result); - Assert.StartsWith($"An error occurred while parsing EntityName.", (result.Data as string[])[0]); + Assert.StartsWith($"An error occurred while parsing EntityName.", (result.Data as JsonValidationMessage[])[0].PlainTextMessage); } public static IEnumerable WillShowTheViewWithErrorsWhenThePackageIdIsAlreadyBeingUsed_Data @@ -4318,7 +4320,7 @@ public async Task WillShowTheViewWithErrorsWhenThePackageIdIsAlreadyBeingUsed(Us var result = await controller.UploadPackage(fakeUploadedFile.Object) as JsonResult; Assert.NotNull(result); - Assert.Equal(String.Format(Strings.PackageIdNotAvailable, "theId"), (result.Data as string[])[0]); + Assert.Equal(String.Format(Strings.PackageIdNotAvailable, "theId"), (result.Data as JsonValidationMessage[])[0].PlainTextMessage); } public static IEnumerable WillShowTheViewWithErrorsWhenThePackageIdMatchesUnownedNamespace_Data @@ -4368,7 +4370,7 @@ public async Task WillShowTheViewWithErrorsWhenThePackageIdMatchesUnownedNamespa var result = await controller.UploadPackage(fakeUploadedFile.Object) as JsonResult; Assert.NotNull(result); - Assert.Equal(String.Format(Strings.UploadPackage_IdNamespaceConflict), (result.Data as string[])[0]); + Assert.Equal(String.Format(Strings.UploadPackage_IdNamespaceConflict), (result.Data as JsonValidationMessage[])[0].PlainTextMessage); fakeTelemetryService.Verify( x => x.TrackPackagePushNamespaceConflictEvent( It.IsAny(), @@ -4518,7 +4520,7 @@ public async Task WillShowTheViewWithErrorsWhenThePackageAlreadyExists(User curr Assert.NotNull(result); Assert.Equal( String.Format(Strings.PackageExistsAndCannotBeModified, "theId", "1.0.0"), - (result.Data as string[])[0]); + (result.Data as JsonValidationMessage[])[0].PlainTextMessage); fakePackageDeleteService.Verify( x => x.HardDeletePackagesAsync( It.IsAny>(), @@ -4560,7 +4562,7 @@ public async Task WillShowTheViewWithErrorsWhenThePackageAlreadyExistsAndOnlyDif Assert.NotNull(result); Assert.Equal( String.Format(Strings.PackageVersionDiffersOnlyByMetadataAndCannotBeModified, "theId", "1.0.0+metadata"), - (result.Data as string[])[0]); + (result.Data as JsonValidationMessage[])[0].PlainTextMessage); fakePackageDeleteService.Verify( x => x.HardDeletePackagesAsync( It.IsAny>(), @@ -4732,7 +4734,7 @@ public async Task WillShowValidationErrorsFoundBeforeGeneratePackage() Assert.NotNull(result); Assert.Equal((int)HttpStatusCode.BadRequest, controller.Response.StatusCode); - Assert.Equal(expectedMessage, (result.Data as string[])[0]); + Assert.Equal(expectedMessage, (result.Data as JsonValidationMessage[])[0].PlainTextMessage); } @@ -4754,7 +4756,7 @@ public async Task WillShowValidationWarningsFoundBeforeGeneratePackage() var fakePackageUploadService = GetValidPackageUploadService(PackageId, PackageVersion); fakePackageUploadService .Setup(x => x.ValidateBeforeGeneratePackageAsync(It.IsAny(), It.IsAny())) - .ReturnsAsync(PackageValidationResult.AcceptedWithWarnings(new[] { expectedMessage })); + .ReturnsAsync(PackageValidationResult.AcceptedWithWarnings(new[] { new PlainTextOnlyValidationMessage(expectedMessage) })); var controller = CreateController( GetConfigurationService(), @@ -4767,7 +4769,7 @@ public async Task WillShowValidationWarningsFoundBeforeGeneratePackage() Assert.NotNull(result); var data = Assert.IsAssignableFrom(result.Data); var actualMessage = Assert.Single(data.Warnings); - Assert.Equal(expectedMessage, actualMessage); + Assert.Equal(expectedMessage, actualMessage.PlainTextMessage); } [Fact] @@ -4791,7 +4793,7 @@ public async Task WillShowViewWithErrorsIfSymbolsPackageValidationThrowsExceptio var result = await controller.UploadPackage(fakeUploadedFile.Object) as JsonResult; Assert.NotNull(result); - Assert.Equal(Strings.FailedToReadUploadFile, (result.Data as string[])[0]); + Assert.Equal(Strings.FailedToReadUploadFile, (result.Data as JsonValidationMessage[])[0].PlainTextMessage); } [Fact] @@ -4826,7 +4828,7 @@ public async Task WillReturnConflictWhenUserDoesNotHaveOwnPackage() Assert.NotNull(result); Assert.Equal((int)HttpStatusCode.Conflict, controller.Response.StatusCode); - Assert.Equal(string.Format(Strings.PackageIdNotAvailable, packageId), (result.Data as string[])[0]); + Assert.Equal(string.Format(Strings.PackageIdNotAvailable, packageId), (result.Data as JsonValidationMessage[])[0].PlainTextMessage); } [Fact] @@ -4862,7 +4864,7 @@ public async Task WillPreventSymbolsUploadIfOriginalPackageIsLocked() Assert.NotNull(result); Assert.Equal((int)HttpStatusCode.Forbidden, controller.Response.StatusCode); - Assert.Equal(string.Format(Strings.PackageIsLocked, packageId), (result.Data as string[])[0]); + Assert.Equal(string.Format(Strings.PackageIsLocked, packageId), (result.Data as JsonValidationMessage[])[0].PlainTextMessage); } public static IEnumerable SymbolValidationResultTypes => Enum @@ -5468,21 +5470,31 @@ public async Task WillShowTheValidationMessageWhenPackageSecurityPolicyCreatesEr var jsonResult = Assert.IsType(result); Assert.Equal((int)HttpStatusCode.BadRequest, controller.Response.StatusCode); - var message = (jsonResult.Data as string[])[0]; - Assert.Equal(expectedMessage, message); + var message = (jsonResult.Data as JsonValidationMessage[])[0]; + Assert.Equal(expectedMessage, message.PlainTextMessage); } } [Theory] - [InlineData(PackageValidationResultType.Invalid)] - [InlineData(PackageValidationResultType.PackageShouldNotBeSigned)] - [InlineData(PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates)] - public async Task WillShowTheValidationMessageWhenValidationAfterGenerateFails(PackageValidationResultType type) + [InlineData(PackageValidationResultType.Invalid, false)] + [InlineData(PackageValidationResultType.Invalid, true)] + public async Task WillShowTheValidationMessageWhenValidationAfterGenerateFails(PackageValidationResultType type, bool hasRawHtml) { var fakeUploadFileService = new Mock(); using (var fakeFileStream = new MemoryStream()) { - var expectedMessage = "The package is just bad."; + const string expectedMessage = "The package is just bad."; + var messageMock = new Mock(); + messageMock + .SetupGet(m => m.PlainTextMessage) + .Returns(expectedMessage); + messageMock + .SetupGet(m => m.HasRawHtmlRepresentation) + .Returns(hasRawHtml); + messageMock + .SetupGet(m => m.RawHtmlMessage) + .Returns(hasRawHtml ? null : expectedMessage); + var expectedResult = new PackageValidationResult(type, messageMock.Object); fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(fakeFileStream)); fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); @@ -5494,7 +5506,7 @@ public async Task WillShowTheValidationMessageWhenValidationAfterGenerateFails(P It.IsAny(), It.IsAny(), It.IsAny())) - .ReturnsAsync(new PackageValidationResult(type, expectedMessage)); + .ReturnsAsync(expectedResult); var fakeNuGetPackage = TestPackage.CreateTestPackageStream(PackageId, PackageVersion); var fakeUserService = new Mock(); @@ -5510,7 +5522,7 @@ public async Task WillShowTheValidationMessageWhenValidationAfterGenerateFails(P var result = await controller.VerifyPackage(new VerifyPackageRequest() { Listed = true, Owner = TestUtility.FakeUser.Username }); - VerifyPackageValidationResultMessage(type, expectedMessage, controller, result); + VerifyPackageValidationResultMessage(type, expectedResult, controller, result); fakePackageUploadService.Verify( x => x.GeneratePackageAsync( It.IsAny(), @@ -5523,15 +5535,25 @@ public async Task WillShowTheValidationMessageWhenValidationAfterGenerateFails(P } [Theory] - [InlineData(PackageValidationResultType.Invalid)] - [InlineData(PackageValidationResultType.PackageShouldNotBeSigned)] - [InlineData(PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates)] - public async Task WillShowTheValidationMessageWhenValidationBeforeGenerateFails(PackageValidationResultType type) + [InlineData(PackageValidationResultType.Invalid, false)] + [InlineData(PackageValidationResultType.Invalid, true)] + public async Task WillShowTheValidationMessageWhenValidationBeforeGenerateFails(PackageValidationResultType type, bool hasRawHtml) { var fakeUploadFileService = new Mock(); using (var fakeFileStream = new MemoryStream()) { - var expectedMessage = "The package is just bad."; + const string expectedMessage = "The package is just bad."; + var messageMock = new Mock(); + messageMock + .SetupGet(m => m.PlainTextMessage) + .Returns(expectedMessage); + messageMock + .SetupGet(m => m.HasRawHtmlRepresentation) + .Returns(hasRawHtml); + messageMock + .SetupGet(m => m.RawHtmlMessage) + .Returns(hasRawHtml ? null : expectedMessage); + var expectedResult = new PackageValidationResult(type, messageMock.Object); fakeUploadFileService.Setup(x => x.GetUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(fakeFileStream)); fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(TestUtility.FakeUser.Key)).Returns(Task.FromResult(0)); @@ -5540,7 +5562,7 @@ public async Task WillShowTheValidationMessageWhenValidationBeforeGenerateFails( .Setup(x => x.ValidateBeforeGeneratePackageAsync( It.IsAny(), It.IsAny())) - .ReturnsAsync(new PackageValidationResult(type, expectedMessage)); + .ReturnsAsync(expectedResult); var fakeNuGetPackage = TestPackage.CreateTestPackageStream(PackageId, PackageVersion); var fakeUserService = new Mock(); @@ -5556,7 +5578,7 @@ public async Task WillShowTheValidationMessageWhenValidationBeforeGenerateFails( var result = await controller.VerifyPackage(new VerifyPackageRequest() { Listed = true, Owner = TestUtility.FakeUser.Username }); - VerifyPackageValidationResultMessage(type, expectedMessage, controller, result); + VerifyPackageValidationResultMessage(type, expectedResult, controller, result); fakePackageUploadService.Verify( x => x.GeneratePackageAsync( It.IsAny(), @@ -5568,21 +5590,21 @@ public async Task WillShowTheValidationMessageWhenValidationBeforeGenerateFails( } } - private static void VerifyPackageValidationResultMessage(PackageValidationResultType type, string expectedMessage, PackagesController controller, JsonResult result) + private static void VerifyPackageValidationResultMessage(PackageValidationResultType type, PackageValidationResult expectedResult, PackagesController controller, JsonResult result) { var jsonResult = Assert.IsType(result); Assert.Equal((int)HttpStatusCode.BadRequest, controller.Response.StatusCode); - var message = (jsonResult.Data as string[])[0]; + var message = (jsonResult.Data as JsonValidationMessage[])[0]; - if (type == PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates) + if (!expectedResult.Message.HasRawHtmlRepresentation) { - Assert.Equal( - expectedMessage + " You can manage your certificates on the Account Settings page.", - message); + Assert.Null(message.RawHtmlMessage); + Assert.Equal(expectedResult.Message.PlainTextMessage, message.PlainTextMessage); } else { - Assert.Equal(expectedMessage, message); + Assert.Null(message.PlainTextMessage); + Assert.Equal(expectedResult.Message.RawHtmlMessage, message.RawHtmlMessage); } } @@ -5640,8 +5662,8 @@ private async Task VerifyCreateThePackage(User currentUser, User ownerInForm, bo var jsonResult = Assert.IsType(result); if (expectedMessage != null) { - var message = (jsonResult.Data as string[])[0]; - Assert.Equal(expectedMessage, message); + var message = (jsonResult.Data as JsonValidationMessage[])[0]; + Assert.Equal(expectedMessage, message.PlainTextMessage); } } } @@ -6337,7 +6359,7 @@ public async Task WillShowErrorMessageWhenFailedValidations() var result = await controller.VerifyPackage(new VerifyPackageRequest() { Owner = TestUtility.FakeUser.Username }); Assert.NotNull(result); - Assert.Equal(message, (result.Data as string[])[0]); + Assert.Equal(message, (result.Data as JsonValidationMessage[])[0].PlainTextMessage); } } @@ -6374,7 +6396,7 @@ public async Task WillReturnUnexpectedErrorWhenSymbolsCreationFails() var result = await controller.VerifyPackage(new VerifyPackageRequest() { Owner = TestUtility.FakeUser.Username }); Assert.NotNull(result); - Assert.Equal(Strings.VerifyPackage_UnexpectedError, (result.Data as string[])[0]); + Assert.Equal(Strings.VerifyPackage_UnexpectedError, (result.Data as JsonValidationMessage[])[0].PlainTextMessage); telemetryService .Verify(x => x.TrackSymbolPackagePushFailureEvent(PackageId, PackageVersion), Times.Once); } diff --git a/tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs index e9c2b7b2a2..a6e6ae507c 100644 --- a/tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs @@ -202,7 +202,7 @@ public async Task WarnsWhenPreviousVersionWasSignedButPushedVersionIsUnsigned() Assert.Equal( $"The previous package version '{previous.NormalizedVersion}' is author signed but the uploaded " + $"package is unsigned. To avoid this warning, sign the package before uploading.", - Assert.Single(result.Warnings)); + Assert.Single(result.Warnings).PlainTextMessage); _packageService.Verify( x => x.FindPackageRegistrationById(It.IsAny()), Times.Once); @@ -346,7 +346,7 @@ public async Task WarnsOnMalformedRepositoryMetadata(string repositoryType, stri else { Assert.Equal(1, result.Warnings.Count()); - Assert.Equal(expectedWarning, result.Warnings.First()); + Assert.Equal(expectedWarning, result.Warnings.First().PlainTextMessage); } } @@ -417,7 +417,7 @@ public async Task WithTooManyPackageEntries_WhenRejectPackagesWithTooManyPackage GetPackageMetadata(_nuGetPackage)); Assert.Equal(PackageValidationResultType.Invalid, result.Type); - Assert.Equal("The package contains too many files and/or folders.", result.Message); + Assert.Equal("The package contains too many files and/or folders.", result.Message.PlainTextMessage); Assert.Empty(result.Warnings); } @@ -480,7 +480,7 @@ public async Task RejectsLicensedPackagesWhenConfigured(string licenseNode, bool else { Assert.Equal(PackageValidationResultType.Invalid, result.Type); - Assert.Contains("license", result.Message); + Assert.Contains("license", result.Message.PlainTextMessage); Assert.Empty(result.Warnings); } } @@ -531,8 +531,8 @@ public async Task RejectsSignedPackageIfSigningIsNotAllowed_HasAnySignerAndOneOw _currentUser, _isNewPackageRegistration); - Assert.Equal(PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates, result.Type); - Assert.Equal(Strings.UploadPackage_PackageIsSignedButMissingCertificate_CurrentUserCanManageCertificates, result.Message); + Assert.Equal(PackageValidationResultType.Invalid, result.Type); + Assert.IsType(result.Message); Assert.Empty(result.Warnings); } @@ -549,8 +549,8 @@ public async Task RejectsSignedPackageIfSigningIsNotAllowed_HasAnySignerAndMulti _currentUser, _isNewPackageRegistration); - Assert.Equal(PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates, result.Type); - Assert.Equal(Strings.UploadPackage_PackageIsSignedButMissingCertificate_CurrentUserCanManageCertificates, result.Message); + Assert.Equal(PackageValidationResultType.Invalid, result.Type); + Assert.IsType(result.Message); Assert.Empty(result.Warnings); } @@ -567,8 +567,8 @@ public async Task RejectsSignedPackageIfSigningIsNotAllowed_HasRequiredSignerTha _currentUser, _isNewPackageRegistration); - Assert.Equal(PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates, result.Type); - Assert.Equal(Strings.UploadPackage_PackageIsSignedButMissingCertificate_CurrentUserCanManageCertificates, result.Message); + Assert.Equal(PackageValidationResultType.Invalid, result.Type); + Assert.IsType(result.Message); Assert.Empty(result.Warnings); } @@ -590,10 +590,10 @@ public async Task RejectsSignedPackageIfSigningIsNotAllowed_HasRequiredSignerTha _currentUser, _isNewPackageRegistration); - Assert.Equal(PackageValidationResultType.PackageShouldNotBeSigned, result.Type); + Assert.Equal(PackageValidationResultType.Invalid, result.Type); Assert.Equal( string.Format(Strings.UploadPackage_PackageIsSignedButMissingCertificate_RequiredSigner, otherUser.Username), - result.Message); + result.Message.PlainTextMessage); Assert.Empty(result.Warnings); } @@ -621,10 +621,10 @@ public async Task RejectsSignedPackageIfSigningIsNotAllowed_HasAnySignerAndOwner _currentUser, _isNewPackageRegistration); - Assert.Equal(PackageValidationResultType.PackageShouldNotBeSigned, result.Type); + Assert.Equal(PackageValidationResultType.Invalid, result.Type); Assert.Equal( string.Format(Strings.UploadPackage_PackageIsSignedButMissingCertificate_RequiredSigner, _owner.Username), - result.Message); + result.Message.PlainTextMessage); Assert.Empty(result.Warnings); } @@ -653,10 +653,10 @@ public async Task RejectsSignedPackageIfSigningIsNotAllowed_HasRequiredSignerAnd _currentUser, _isNewPackageRegistration); - Assert.Equal(PackageValidationResultType.PackageShouldNotBeSigned, result.Type); + Assert.Equal(PackageValidationResultType.Invalid, result.Type); Assert.Equal( string.Format(Strings.UploadPackage_PackageIsSignedButMissingCertificate_RequiredSigner, ownerB.Username), - result.Message); + result.Message.PlainTextMessage); Assert.Empty(result.Warnings); } @@ -713,7 +713,7 @@ public async Task RejectsUnsignedPackageIfSigningIsRequiredIrrespectiveOfConfig( _isNewPackageRegistration); Assert.Equal(PackageValidationResultType.Invalid, result.Type); - Assert.Equal(Strings.UploadPackage_PackageIsNotSigned, result.Message); + Assert.Equal(Strings.UploadPackage_PackageIsNotSigned, result.Message.PlainTextMessage); Assert.Empty(result.Warnings); } @@ -813,8 +813,7 @@ public async Task RejectIsTyposquattingNewVersion() _isNewPackageRegistration); Assert.Equal(PackageValidationResultType.Invalid, result.Type); - Assert.Equal(string.Format(Strings.TyposquattingCheckFails, string.Join(",", _typosquattingCheckCollisionIds)), result.Message); - Assert.Contains("similar", result.Message); + Assert.Equal(string.Format(Strings.TyposquattingCheckFails, string.Join(",", _typosquattingCheckCollisionIds)), result.Message.PlainTextMessage); Assert.Empty(result.Warnings); } } From a053203d06cdb246f10e2de84e9c087c77bce0b5 Mon Sep 17 00:00:00 2001 From: zhhyu <41028779+zhhyu@users.noreply.github.com> Date: Thu, 1 Nov 2018 10:24:47 -0700 Subject: [PATCH 17/20] Typosquatting optimization: add caching (#6597) * Typosquatting optimization: add caching * update * Move locker to cache class * Refactor, Decouple and Update a lot of things * Update * Fix unit test * Update again * Set default cache expire time as 24 hours * Some changes * Use TimeSpan for expire time * Add some tests for cache --- .../App_Start/DefaultDependenciesModule.cs | 5 + src/NuGetGallery/NuGetGallery.csproj | 2 + .../Services/ITelemetryService.cs | 6 +- .../ITyposquattingCheckListCacheService.cs | 22 ++ .../Services/ITyposquattingConfiguration.cs | 1 + src/NuGetGallery/Services/TelemetryService.cs | 7 +- .../TyposquattingCheckListCacheService.cs | 73 +++++++ .../Services/TyposquattingConfiguration.cs | 10 +- .../Services/TyposquattingService.cs | 53 +++-- .../NuGetGallery.Facts.csproj | 1 + .../Services/TelemetryServiceFacts.cs | 2 +- ...TyposquattingCheckListCacheServiceFacts.cs | 164 +++++++++++++++ .../Services/TyposquattingServiceFacts.cs | 191 ++++++++++++------ 13 files changed, 443 insertions(+), 94 deletions(-) create mode 100644 src/NuGetGallery/Services/ITyposquattingCheckListCacheService.cs create mode 100644 src/NuGetGallery/Services/TyposquattingCheckListCacheService.cs create mode 100644 tests/NuGetGallery.Facts/Services/TyposquattingCheckListCacheServiceFacts.cs diff --git a/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs b/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs index 910bec41f3..6bd9fcbbc4 100644 --- a/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs +++ b/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs @@ -333,6 +333,11 @@ protected override void Load(ContainerBuilder builder) .As() .InstancePerLifetimeScope(); + builder.RegisterType() + .AsSelf() + .As() + .SingleInstance(); + RegisterMessagingService(builder, configuration); builder.Register(c => HttpContext.Current.User) diff --git a/src/NuGetGallery/NuGetGallery.csproj b/src/NuGetGallery/NuGetGallery.csproj index 56cfea672e..686caab8c1 100644 --- a/src/NuGetGallery/NuGetGallery.csproj +++ b/src/NuGetGallery/NuGetGallery.csproj @@ -423,6 +423,7 @@ + @@ -435,6 +436,7 @@ + diff --git a/src/NuGetGallery/Services/ITelemetryService.cs b/src/NuGetGallery/Services/ITelemetryService.cs index c005fef6b7..3b4a21109f 100644 --- a/src/NuGetGallery/Services/ITelemetryService.cs +++ b/src/NuGetGallery/Services/ITelemetryService.cs @@ -203,13 +203,15 @@ public interface ITelemetryService /// The total time for the typosquatting check logic /// Whether the uploaded package is blocked because of typosquatting check. /// The list of collision package Ids for this uploaded package. - /// The length of the checklist for typosquatting check + /// The length of the checklist for typosquatting check + /// The expire time for checklist caching void TrackMetricForTyposquattingCheckResultAndTotalTime( string packageId, TimeSpan totalTime, bool wasUploadBlocked, List collisionPackageIds, - int checklistLength); + int checkListLength, + TimeSpan checkListCacheExpireTime); /// /// The retrieval time to get the checklist for typosquatting check. diff --git a/src/NuGetGallery/Services/ITyposquattingCheckListCacheService.cs b/src/NuGetGallery/Services/ITyposquattingCheckListCacheService.cs new file mode 100644 index 0000000000..71d7d17f86 --- /dev/null +++ b/src/NuGetGallery/Services/ITyposquattingCheckListCacheService.cs @@ -0,0 +1,22 @@ +// 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; + +namespace NuGetGallery +{ + /// + /// This interface is used to implement a basic caching for typosquatting checklist. + /// /// + public interface ITyposquattingCheckListCacheService + { + /// + /// The function is used to get the checklist from the cache for typosquatting + /// + /// The length of the checklist for typosquatting + /// The expire time for checklist caching + /// The package service for checklist retrieval from database + IReadOnlyCollection GetTyposquattingCheckList(int checkListLength, TimeSpan checkListExpireTime, IPackageService packageService); + } +} diff --git a/src/NuGetGallery/Services/ITyposquattingConfiguration.cs b/src/NuGetGallery/Services/ITyposquattingConfiguration.cs index ab6aa625f8..7e01b70d6a 100644 --- a/src/NuGetGallery/Services/ITyposquattingConfiguration.cs +++ b/src/NuGetGallery/Services/ITyposquattingConfiguration.cs @@ -8,5 +8,6 @@ public interface ITyposquattingConfiguration int PackageIdChecklistLength { get; } bool IsCheckEnabled { get; } bool IsBlockUsersEnabled { get; } + double PackageIdChecklistCacheExpireTimeInHours { get; } } } \ No newline at end of file diff --git a/src/NuGetGallery/Services/TelemetryService.cs b/src/NuGetGallery/Services/TelemetryService.cs index 895a70c49b..68fc927bfe 100644 --- a/src/NuGetGallery/Services/TelemetryService.cs +++ b/src/NuGetGallery/Services/TelemetryService.cs @@ -150,6 +150,7 @@ internal class Events public const string CollisionPackageIdsCount = "CollisionPackageIdsCount"; public const string CheckListLength = "CheckListLength"; public const string HasExtraCollisionPackageIds = "HasExtraCollisionPackageIds"; + public const string CheckListCacheExpireTimeInHours = "CheckListCacheExpireTimeInHours"; public TelemetryService(IDiagnosticsService diagnosticsService, ITelemetryClient telemetryClient = null) { @@ -722,15 +723,17 @@ public void TrackMetricForTyposquattingCheckResultAndTotalTime( TimeSpan totalTime, bool wasUploadBlocked, List collisionPackageIds, - int checklistLength) + int checkListLength, + TimeSpan checkListCacheExpireTime) { TrackMetric(Events.TyposquattingCheckResultAndTotalTimeInMs, totalTime.TotalMilliseconds, properties => { properties.Add(PackageId, packageId); properties.Add(WasUploadBlocked, wasUploadBlocked.ToString()); properties.Add(CollisionPackageIds, string.Join(",", collisionPackageIds.Take(TyposquattingCollisionIdsMaxPropertyValue))); properties.Add(CollisionPackageIdsCount, collisionPackageIds.Count.ToString()); - properties.Add(CheckListLength, checklistLength.ToString()); + properties.Add(CheckListLength, checkListLength.ToString()); properties.Add(HasExtraCollisionPackageIds, (collisionPackageIds.Count > TyposquattingCollisionIdsMaxPropertyValue).ToString()); + properties.Add(CheckListCacheExpireTimeInHours, checkListCacheExpireTime.ToString()); }); } diff --git a/src/NuGetGallery/Services/TyposquattingCheckListCacheService.cs b/src/NuGetGallery/Services/TyposquattingCheckListCacheService.cs new file mode 100644 index 0000000000..06d42fb991 --- /dev/null +++ b/src/NuGetGallery/Services/TyposquattingCheckListCacheService.cs @@ -0,0 +1,73 @@ +// 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; + +namespace NuGetGallery +{ + public class TyposquattingCheckListCacheService : ITyposquattingCheckListCacheService + { + private readonly object Locker = new object(); + + private List Cache; + private DateTime LastRefreshTime; + + private int TyposquattingCheckListConfiguredLength; + + public TyposquattingCheckListCacheService() + { + TyposquattingCheckListConfiguredLength = -1; + LastRefreshTime = DateTime.MinValue; + } + + public IReadOnlyCollection GetTyposquattingCheckList(int checkListConfiguredLength, TimeSpan checkListExpireTime, IPackageService packageService) + { + if (packageService == null) + { + throw new ArgumentNullException(nameof(packageService)); + } + if (checkListConfiguredLength < 0) + { + throw new ArgumentOutOfRangeException(nameof(checkListConfiguredLength), "Negative values are not supported."); + } + if (checkListExpireTime.CompareTo(TimeSpan.Zero) < 0) + { + throw new ArgumentOutOfRangeException(nameof(checkListExpireTime), "Negative values are not supported."); + } + + if (ShouldCacheBeUpdated(checkListConfiguredLength, checkListExpireTime)) + { + lock (Locker) + { + if (ShouldCacheBeUpdated(checkListConfiguredLength, checkListExpireTime)) + { + TyposquattingCheckListConfiguredLength = checkListConfiguredLength; + + Cache = packageService.GetAllPackageRegistrations() + .OrderByDescending(pr => pr.IsVerified) + .ThenByDescending(pr => pr.DownloadCount) + .Select(pr => pr.Id) + .Take(TyposquattingCheckListConfiguredLength) + .ToList(); + + LastRefreshTime = DateTime.UtcNow; + } + } + } + + return Cache; + } + + private bool ShouldCacheBeUpdated(int checkListConfiguredLength, TimeSpan checkListExpireTime) + { + return Cache == null || checkListConfiguredLength != TyposquattingCheckListConfiguredLength || IsCheckListCacheExpired(checkListExpireTime); + } + + private bool IsCheckListCacheExpired(TimeSpan checkListExpireTime) + { + return DateTime.UtcNow >= LastRefreshTime.Add(checkListExpireTime); + } + } +} \ No newline at end of file diff --git a/src/NuGetGallery/Services/TyposquattingConfiguration.cs b/src/NuGetGallery/Services/TyposquattingConfiguration.cs index ff04e68ff6..6ff9bc6de5 100644 --- a/src/NuGetGallery/Services/TyposquattingConfiguration.cs +++ b/src/NuGetGallery/Services/TyposquattingConfiguration.cs @@ -8,13 +8,17 @@ namespace NuGetGallery.Services public sealed class TyposquattingConfiguration : ITyposquattingConfiguration { private const int DefaultPackageIdCheckListLength = 20000; + private const double DefaultPackageIdChecklistCacheExpireTimeInHours = 24; public int PackageIdChecklistLength { get; } public bool IsCheckEnabled { get; } public bool IsBlockUsersEnabled { get; } + public double PackageIdChecklistCacheExpireTimeInHours { get; } + public TyposquattingConfiguration() : this(packageIdChecklistLength: DefaultPackageIdCheckListLength, isCheckEnabled: false, - isBlockUsersEnabled: false) + isBlockUsersEnabled: false, + packageIdChecklistCacheExpireTimeInHours: DefaultPackageIdChecklistCacheExpireTimeInHours) { } @@ -22,11 +26,13 @@ public TyposquattingConfiguration() public TyposquattingConfiguration( int packageIdChecklistLength, bool isCheckEnabled, - bool isBlockUsersEnabled) + bool isBlockUsersEnabled, + double packageIdChecklistCacheExpireTimeInHours) { PackageIdChecklistLength = packageIdChecklistLength; IsCheckEnabled = isCheckEnabled; IsBlockUsersEnabled = isBlockUsersEnabled; + PackageIdChecklistCacheExpireTimeInHours = packageIdChecklistCacheExpireTimeInHours; } } } \ No newline at end of file diff --git a/src/NuGetGallery/Services/TyposquattingService.cs b/src/NuGetGallery/Services/TyposquattingService.cs index 658563eeb9..c7211ded49 100644 --- a/src/NuGetGallery/Services/TyposquattingService.cs +++ b/src/NuGetGallery/Services/TyposquattingService.cs @@ -20,58 +20,57 @@ public class TyposquattingService : ITyposquattingService new ThresholdInfo (lowerBound: 50, upperBound: 129, threshold: 2) }; - private static int TyposquattingCheckListLength; - private readonly IContentObjectService _contentObjectService; private readonly IPackageService _packageService; private readonly IReservedNamespaceService _reservedNamespaceService; private readonly ITelemetryService _telemetryService; + private readonly ITyposquattingCheckListCacheService _typosquattingCheckListCacheService; - public TyposquattingService(IContentObjectService contentObjectService, IPackageService packageService, IReservedNamespaceService reservedNamespaceService, ITelemetryService telemetryService) + public TyposquattingService(IContentObjectService contentObjectService, + IPackageService packageService, + IReservedNamespaceService reservedNamespaceService, + ITelemetryService telemetryService, + ITyposquattingCheckListCacheService typosquattingCheckListCacheService) { _contentObjectService = contentObjectService ?? throw new ArgumentNullException(nameof(contentObjectService)); _packageService = packageService ?? throw new ArgumentNullException(nameof(packageService)); _reservedNamespaceService = reservedNamespaceService ?? throw new ArgumentNullException(nameof(reservedNamespaceService)); _telemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService)); - - TyposquattingCheckListLength = _contentObjectService.TyposquattingConfiguration.PackageIdChecklistLength; + _typosquattingCheckListCacheService = typosquattingCheckListCacheService ?? throw new ArgumentNullException(nameof(typosquattingCheckListCacheService)); } public bool IsUploadedPackageIdTyposquatting(string uploadedPackageId, User uploadedPackageOwner, out List typosquattingCheckCollisionIds) { + var checkListConfiguredLength = _contentObjectService.TyposquattingConfiguration.PackageIdChecklistLength; + var checkListExpireTimeInHours = TimeSpan.FromHours(_contentObjectService.TyposquattingConfiguration.PackageIdChecklistCacheExpireTimeInHours); typosquattingCheckCollisionIds = new List(); var wasUploadBlocked = false; + if (!_contentObjectService.TyposquattingConfiguration.IsCheckEnabled || _reservedNamespaceService.GetReservedNamespacesForId(uploadedPackageId).Any()) { return wasUploadBlocked; } - if (uploadedPackageId == null) { throw new ArgumentNullException(nameof(uploadedPackageId)); } - if (uploadedPackageOwner == null) { throw new ArgumentNullException(nameof(uploadedPackageOwner)); } + var totalTimeStopwatch = Stopwatch.StartNew(); var checklistRetrievalStopwatch = Stopwatch.StartNew(); - var packageRegistrations = _packageService.GetAllPackageRegistrations(); - var packagesCheckList = packageRegistrations - .OrderByDescending(pr => pr.IsVerified) - .ThenByDescending(pr => pr.DownloadCount) - .Select(pr => pr.Id) - .Take(TyposquattingCheckListLength) - .ToList(); + var packageIdsCheckList = _typosquattingCheckListCacheService.GetTyposquattingCheckList(checkListConfiguredLength, checkListExpireTimeInHours, _packageService); checklistRetrievalStopwatch.Stop(); + _telemetryService.TrackMetricForTyposquattingChecklistRetrievalTime(uploadedPackageId, checklistRetrievalStopwatch.Elapsed); + var algorithmProcessingStopwatch = Stopwatch.StartNew(); var threshold = GetThreshold(uploadedPackageId); var normalizedUploadedPackageId = TyposquattingStringNormalization.NormalizeString(uploadedPackageId); - var collisionIds = new ConcurrentBag(); - Parallel.ForEach(packagesCheckList, (packageId, loopState) => + Parallel.ForEach(packageIdsCheckList, (packageId, loopState) => { string normalizedPackageId = TyposquattingStringNormalization.NormalizeString(packageId); if (TyposquattingDistanceCalculation.IsDistanceLessThanThreshold(normalizedUploadedPackageId, normalizedPackageId, threshold)) @@ -79,27 +78,26 @@ public bool IsUploadedPackageIdTyposquatting(string uploadedPackageId, User uplo collisionIds.Add(packageId); } }); - algorithmProcessingStopwatch.Stop(); - var totalTime = checklistRetrievalStopwatch.Elapsed.Add(algorithmProcessingStopwatch.Elapsed); - _telemetryService.TrackMetricForTyposquattingChecklistRetrievalTime(uploadedPackageId, checklistRetrievalStopwatch.Elapsed); _telemetryService.TrackMetricForTyposquattingAlgorithmProcessingTime(uploadedPackageId, algorithmProcessingStopwatch.Elapsed); if (collisionIds.Count == 0) { + totalTimeStopwatch.Stop(); _telemetryService.TrackMetricForTyposquattingCheckResultAndTotalTime( uploadedPackageId, - totalTime, + totalTimeStopwatch.Elapsed, wasUploadBlocked, typosquattingCheckCollisionIds, - TyposquattingCheckListLength); + packageIdsCheckList.Count, + checkListExpireTimeInHours); return false; } var ownersCheckStopwatch = Stopwatch.StartNew(); - var collisionPackagesIdAndOwners = packageRegistrations + var collisionPackagesIdAndOwners = _packageService.GetAllPackageRegistrations() .Where(pr => collisionIds.Contains(pr.Id)) .Select(pr => new { Id = pr.Id, Owners = pr.Owners.Select(x => x.Key).ToList() }) .ToList(); @@ -122,21 +120,21 @@ public bool IsUploadedPackageIdTyposquatting(string uploadedPackageId, User uplo .Any(pio => pio.Owners.Any(k => k == uploadedPackageOwner.Key)); wasUploadBlocked = _contentObjectService.TyposquattingConfiguration.IsBlockUsersEnabled && !isUserAllowedTyposquatting; - ownersCheckStopwatch.Stop(); - totalTime = totalTime.Add(ownersCheckStopwatch.Elapsed); _telemetryService.TrackMetricForTyposquattingOwnersCheckTime(uploadedPackageId, ownersCheckStopwatch.Elapsed); + + totalTimeStopwatch.Stop(); _telemetryService.TrackMetricForTyposquattingCheckResultAndTotalTime( uploadedPackageId, - totalTime, + totalTimeStopwatch.Elapsed, wasUploadBlocked, typosquattingCheckCollisionIds, - TyposquattingCheckListLength); + packageIdsCheckList.Count, + checkListExpireTimeInHours); return wasUploadBlocked; } - private static int GetThreshold(string packageId) { foreach (var thresholdInfo in ThresholdsList) @@ -150,7 +148,6 @@ private static int GetThreshold(string packageId) throw new ArgumentException(String.Format("There is no predefined typo-squatting threshold for this package Id: {0}", packageId)); } } - public class ThresholdInfo { public int LowerBound { get; } diff --git a/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj b/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj index 28f1c676d3..b4fbb22f71 100644 --- a/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj +++ b/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj @@ -157,6 +157,7 @@ + diff --git a/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs b/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs index 6f063c7c8d..10553c66f9 100644 --- a/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs @@ -218,7 +218,7 @@ public static IEnumerable TrackMetricNames_Data }; yield return new object[] { "TyposquattingCheckResultAndTotalTimeInMs", - (TrackAction)(s => s.TrackMetricForTyposquattingCheckResultAndTotalTime(fakes.Package.Id, TimeSpan.FromMilliseconds(100), true, new List{"newtonsoft-json" }, 10000)) + (TrackAction)(s => s.TrackMetricForTyposquattingCheckResultAndTotalTime(fakes.Package.Id, TimeSpan.FromMilliseconds(100), true, new List{"newtonsoft-json" }, 10000, TimeSpan.FromHours(24))) }; yield return new object[] { "TyposquattingChecklistRetrievalTimeInMs", diff --git a/tests/NuGetGallery.Facts/Services/TyposquattingCheckListCacheServiceFacts.cs b/tests/NuGetGallery.Facts/Services/TyposquattingCheckListCacheServiceFacts.cs new file mode 100644 index 0000000000..12bea71dc0 --- /dev/null +++ b/tests/NuGetGallery.Facts/Services/TyposquattingCheckListCacheServiceFacts.cs @@ -0,0 +1,164 @@ +// 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.Threading; +using System.Threading.Tasks; +using Moq; +using NuGet.Services.Entities; +using Xunit; + +namespace NuGetGallery +{ + public class TyposquattingCheckListCacheServiceFacts + { + private static List _packageIds = new List + { + "microsoft_netframework_v1", + "WindowsAzure.Caching", + "SinglePageApplication", + "PoliteCaptcha", + "AspNetRazor.Core", + "System.Json", + "System.Spatial" + }; + + private static IQueryable PacakgeRegistrationsList = Enumerable.Range(0, _packageIds.Count()).Select(i => + new PackageRegistration() + { + Id = _packageIds[i], + DownloadCount = new Random().Next(0, 10000), + IsVerified = true, + Owners = new List { new User() { Username = string.Format("owner{0}", i + 1), Key = i + 1 } } + }).AsQueryable(); + + [Fact] + public void WhenGettingCheckListFromMultipleThreadsItIsInitializedOnce() + { + // Arrange + var mockPackageService = new Mock(); + mockPackageService + .Setup(x => x.GetAllPackageRegistrations()) + .Returns(PacakgeRegistrationsList); + + var newService = new TyposquattingCheckListCacheService(); + + // Act + int tasksNum = 3; + Task[] tasks = new Task[tasksNum]; + for (int i = 0; i < tasksNum; i++) + { + tasks[i] = Task.Factory.StartNew(() => + { + newService.GetTyposquattingCheckList(_packageIds.Count, TimeSpan.FromHours(24), mockPackageService.Object); + newService.GetTyposquattingCheckList(_packageIds.Count, TimeSpan.FromHours(24), mockPackageService.Object); + }); + } + Task.WaitAll(tasks); + + // Assert + mockPackageService.Verify( + x => x.GetAllPackageRegistrations(), + Times.Once); + } + + [Fact] + public void WhenExceedExpireTimeRefreshCheckListCache() + { + // Arrange + var mockPackageService = new Mock(); + mockPackageService + .Setup(x => x.GetAllPackageRegistrations()) + .Returns(PacakgeRegistrationsList); + + var newService = new TyposquattingCheckListCacheService(); + + // Act + newService.GetTyposquattingCheckList(_packageIds.Count, TimeSpan.FromHours(0), mockPackageService.Object); + Thread.Sleep(1); + newService.GetTyposquattingCheckList(_packageIds.Count, TimeSpan.FromHours(0), mockPackageService.Object); + + // Assert + mockPackageService.Verify( + x => x.GetAllPackageRegistrations(), + Times.Exactly(2)); + } + + [Fact] + public void WhenNotEqualConfiguredLengthRefreshCheckListCache() + { + // Arrange + var mockPackageService = new Mock(); + mockPackageService + .Setup(x => x.GetAllPackageRegistrations()) + .Returns(PacakgeRegistrationsList); + + var newService = new TyposquattingCheckListCacheService(); + + // Act + newService.GetTyposquattingCheckList(_packageIds.Count, TimeSpan.FromHours(24), mockPackageService.Object); + newService.GetTyposquattingCheckList(_packageIds.Count - 1, TimeSpan.FromHours(24), mockPackageService.Object); + + // Assert + mockPackageService.Verify( + x => x.GetAllPackageRegistrations(), + Times.Exactly(2)); + } + + [Fact] + public void CheckConfiguredLengthNotAllowed() + { + // Arrange + var mockPackageService = new Mock(); + mockPackageService + .Setup(x => x.GetAllPackageRegistrations()) + .Returns(PacakgeRegistrationsList); + var checkListConfiguredLength = -1; + + var newService = new TyposquattingCheckListCacheService(); + + // Act + var exception = Assert.Throws( + () => newService.GetTyposquattingCheckList(checkListConfiguredLength, TimeSpan.FromHours(24), mockPackageService.Object)); + + // Assert + Assert.Equal(nameof(checkListConfiguredLength), exception.ParamName); + } + + [Fact] + public void CheckExpireTimeNotAllowed() + { + // Arrange + var checkListExpireTime = TimeSpan.FromHours(-1); + var mockPackageService = new Mock(); + mockPackageService + .Setup(x => x.GetAllPackageRegistrations()) + .Returns(PacakgeRegistrationsList); + + var newService = new TyposquattingCheckListCacheService(); + + // Act + var exception = Assert.Throws( + () => newService.GetTyposquattingCheckList(_packageIds.Count, checkListExpireTime, mockPackageService.Object)); + + // Assert + Assert.Equal(nameof(checkListExpireTime), exception.ParamName); + } + + [Fact] + public void CheckNullPackageService() + { + // Arrange + var newService = new TyposquattingCheckListCacheService(); + + // Act + var exception = Assert.Throws( + () => newService.GetTyposquattingCheckList(_packageIds.Count, TimeSpan.FromHours(24), null)); + + // Assert + Assert.Equal("packageService", exception.ParamName); + } + } +} \ No newline at end of file diff --git a/tests/NuGetGallery.Facts/Services/TyposquattingServiceFacts.cs b/tests/NuGetGallery.Facts/Services/TyposquattingServiceFacts.cs index a616bec20b..88c1140994 100644 --- a/tests/NuGetGallery.Facts/Services/TyposquattingServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/TyposquattingServiceFacts.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Threading.Tasks; using Moq; using NuGet.Services.Entities; using Xunit; @@ -12,11 +13,6 @@ namespace NuGetGallery { public class TyposquattingServiceFacts { - private static Mock _packageService = new Mock(); - private static Mock _contentObjectService = new Mock(); - private static Mock _reservedNamespaceService = new Mock(); - private static Mock _telemetryService = new Mock(); - private static List _packageIds = new List { "microsoft_netframework_v1", @@ -28,7 +24,7 @@ public class TyposquattingServiceFacts "System.Spatial" }; - private IQueryable _pacakgeRegistrationsList = Enumerable.Range(0, _packageIds.Count()).Select(i => + private static IQueryable PacakgeRegistrationsList = Enumerable.Range(0, _packageIds.Count()).Select(i => new PackageRegistration() { Id = _packageIds[i], @@ -39,27 +35,60 @@ public class TyposquattingServiceFacts private User _uploadedPackageOwner = new User() { Username = string.Format("owner{0}", _packageIds.Count() + 1), Key = _packageIds.Count() + 1 }; - public TyposquattingServiceFacts() + private static ITyposquattingService CreateService( + Mock packageService = null, + Mock contentObjectService = null, + Mock reservedNamespaceService = null, + Mock telemetryService = null, + Mock typosquattingCheckListCacheService = null) { - _packageService - .Setup(x => x.GetAllPackageRegistrations()) - .Returns(_pacakgeRegistrationsList); - - _contentObjectService - .Setup(x => x.TyposquattingConfiguration.PackageIdChecklistLength) - .Returns(20000); + if (packageService == null) + { + packageService = new Mock(); + packageService + .Setup(x => x.GetAllPackageRegistrations()) + .Returns(PacakgeRegistrationsList); + } - _contentObjectService + if (contentObjectService == null) + { + contentObjectService = new Mock(); + contentObjectService + .Setup(x => x.TyposquattingConfiguration.PackageIdChecklistLength) + .Returns(20000); + contentObjectService .Setup(x => x.TyposquattingConfiguration.IsCheckEnabled) .Returns(true); + contentObjectService + .Setup(x => x.TyposquattingConfiguration.IsBlockUsersEnabled) + .Returns(true); + contentObjectService + .Setup(x => x.TyposquattingConfiguration.PackageIdChecklistCacheExpireTimeInHours) + .Returns(24); + } + + if (reservedNamespaceService == null) + { + reservedNamespaceService = new Mock(); + reservedNamespaceService + .Setup(x => x.GetReservedNamespacesForId(It.IsAny())) + .Returns(new List()); + } - _contentObjectService - .Setup(x => x.TyposquattingConfiguration.IsBlockUsersEnabled) - .Returns(true); + if (telemetryService == null) + { + telemetryService = new Mock(); + } - _reservedNamespaceService - .Setup(x => x.GetReservedNamespacesForId(It.IsAny())) - .Returns(new List()); + if (typosquattingCheckListCacheService == null) + { + typosquattingCheckListCacheService = new Mock(); + typosquattingCheckListCacheService + .Setup(x => x.GetTyposquattingCheckList(It.IsAny(), It.IsAny(), It.IsAny())) + .Returns(PacakgeRegistrationsList.Select(pr => pr.Id).ToList()); + } + + return new TyposquattingService(contentObjectService.Object, packageService.Object, reservedNamespaceService.Object, telemetryService.Object, typosquattingCheckListCacheService.Object); } [Fact] @@ -67,8 +96,9 @@ public void CheckNotTyposquattingByDifferentOwnersTest() { // Arrange var uploadedPackageId = "new_package_for_testing"; - var newService = new TyposquattingService(_contentObjectService.Object, _packageService.Object, _reservedNamespaceService.Object, _telemetryService.Object); + var newService = CreateService(); + // Act var typosquattingCheckResult = newService.IsUploadedPackageIdTyposquatting(uploadedPackageId, _uploadedPackageOwner, out List typosquattingCheckCollisionIds); @@ -85,7 +115,7 @@ public void CheckNotTyposquattingBySameOwnersTest() _uploadedPackageOwner.Key = 1; var uploadedPackageId = "microsoft_netframework.v1"; - var newService = new TyposquattingService(_contentObjectService.Object, _packageService.Object, _reservedNamespaceService.Object, _telemetryService.Object); + var newService = CreateService(); // Act var typosquattingCheckResult = newService.IsUploadedPackageIdTyposquatting(uploadedPackageId, _uploadedPackageOwner, out List typosquattingCheckCollisionIds); @@ -100,8 +130,8 @@ public void CheckIsTyposquattingByDifferentOwnersTest() { // Arrange var uploadedPackageId = "Mícrosoft.NetFramew0rk.v1"; - var newService = new TyposquattingService(_contentObjectService.Object, _packageService.Object, _reservedNamespaceService.Object, _telemetryService.Object); - + var newService = CreateService(); + // Act var typosquattingCheckResult = newService.IsUploadedPackageIdTyposquatting(uploadedPackageId, _uploadedPackageOwner, out List typosquattingCheckCollisionIds); @@ -111,13 +141,12 @@ public void CheckIsTyposquattingByDifferentOwnersTest() Assert.Equal("microsoft_netframework_v1", typosquattingCheckCollisionIds[0]); } - [Fact] public void CheckIsTyposquattingMultiCollisionsWithoutSameUser() { // Arrange var uploadedPackageId = "microsoft_netframework.v1"; - _pacakgeRegistrationsList = _pacakgeRegistrationsList.Concat(new PackageRegistration[] + var pacakgeRegistrationsList = PacakgeRegistrationsList.Concat(new PackageRegistration[] { new PackageRegistration { Id = "microsoft-netframework-v1", @@ -126,11 +155,16 @@ public void CheckIsTyposquattingMultiCollisionsWithoutSameUser() Owners = new List { new User() { Username = string.Format("owner{0}", _packageIds.Count() + 2), Key = _packageIds.Count() + 2} } } }); - _packageService - .Setup(x => x.GetAllPackageRegistrations()) - .Returns(_pacakgeRegistrationsList); + var mockPackageService = new Mock(); + mockPackageService + .Setup(x => x.GetAllPackageRegistrations()) + .Returns(pacakgeRegistrationsList); + var mockTyposquattingCheckListCacheService = new Mock(); + mockTyposquattingCheckListCacheService + .Setup(x => x.GetTyposquattingCheckList(It.IsAny(), It.IsAny(), It.IsAny())) + .Returns(pacakgeRegistrationsList.Select(pr => pr.Id).ToList()); - var newService = new TyposquattingService(_contentObjectService.Object, _packageService.Object, _reservedNamespaceService.Object, _telemetryService.Object); + var newService = CreateService(packageService: mockPackageService, typosquattingCheckListCacheService: mockTyposquattingCheckListCacheService); // Act var typosquattingCheckResult = newService.IsUploadedPackageIdTyposquatting(uploadedPackageId, _uploadedPackageOwner, out List typosquattingCheckCollisionIds); @@ -147,7 +181,7 @@ public void CheckNotTyposquattingMultiCollisionsWithSameUsers() var uploadedPackageId = "microsoft_netframework.v1"; _uploadedPackageOwner.Username = "owner1"; _uploadedPackageOwner.Key = 1; - _pacakgeRegistrationsList = _pacakgeRegistrationsList.Concat(new PackageRegistration[] + var pacakgeRegistrationsList = PacakgeRegistrationsList.Concat(new PackageRegistration[] { new PackageRegistration() { @@ -157,11 +191,17 @@ public void CheckNotTyposquattingMultiCollisionsWithSameUsers() Owners = new List { new User() { Username = string.Format("owner{0}", _packageIds.Count() + 2), Key = _packageIds.Count() + 2 } } } }); - _packageService - .Setup(x => x.GetAllPackageRegistrations()) - .Returns(_pacakgeRegistrationsList); - var newService = new TyposquattingService(_contentObjectService.Object, _packageService.Object, _reservedNamespaceService.Object, _telemetryService.Object); + var mockPackageService = new Mock(); + mockPackageService + .Setup(x => x.GetAllPackageRegistrations()) + .Returns(pacakgeRegistrationsList); + var mockTyposquattingCheckListCacheService = new Mock(); + mockTyposquattingCheckListCacheService + .Setup(x => x.GetTyposquattingCheckList(It.IsAny(), It.IsAny(), It.IsAny())) + .Returns(pacakgeRegistrationsList.Select(pr => pr.Id).ToList()); + + var newService = CreateService(packageService: mockPackageService, typosquattingCheckListCacheService: mockTyposquattingCheckListCacheService); // Act var typosquattingCheckResult = newService.IsUploadedPackageIdTyposquatting(uploadedPackageId, _uploadedPackageOwner, out List typosquattingCheckCollisionIds); @@ -177,11 +217,13 @@ public void CheckNotTyposquattingWithinReservedNameSpace() { // Arrange var uploadedPackageId = "microsoft_netframework.v1"; - _reservedNamespaceService + + var mockReservedNamespaceService = new Mock(); + mockReservedNamespaceService .Setup(x => x.GetReservedNamespacesForId(It.IsAny())) .Returns(new List { new ReservedNamespace()}); - var newService = new TyposquattingService(_contentObjectService.Object, _packageService.Object, _reservedNamespaceService.Object, _telemetryService.Object); + var newService = CreateService(reservedNamespaceService: mockReservedNamespaceService); // Act var typosquattingCheckResult = newService.IsUploadedPackageIdTyposquatting(uploadedPackageId, _uploadedPackageOwner, out List typosquattingCheckCollisionIds); @@ -197,7 +239,7 @@ public void CheckTyposquattingNullUploadedPackageId() // Arrange string uploadedPackageId = null; - var newService = new TyposquattingService(_contentObjectService.Object, _packageService.Object, _reservedNamespaceService.Object, _telemetryService.Object); + var newService = CreateService(); // Act var exception = Assert.Throws( @@ -214,7 +256,7 @@ public void CheckTyposquattingNullUploadedPackageOwner() _uploadedPackageOwner = null; var uploadedPackageId = "microsoft_netframework_v1"; - var newService = new TyposquattingService(_contentObjectService.Object, _packageService.Object, _reservedNamespaceService.Object, _telemetryService.Object); + var newService = CreateService(); // Act var exception = Assert.Throws( @@ -230,7 +272,7 @@ public void CheckTyposquattingEmptyUploadedPackageId() // Arrange var uploadedPackageId = ""; - var newService = new TyposquattingService(_contentObjectService.Object, _packageService.Object, _reservedNamespaceService.Object, _telemetryService.Object); + var newService = CreateService(); // Act var typosquattingCheckResult = newService.IsUploadedPackageIdTyposquatting(uploadedPackageId, _uploadedPackageOwner, out List typosquattingCheckCollisionIds); @@ -245,12 +287,17 @@ public void CheckTyposquattingEmptyChecklist() { // Arrange var uploadedPackageId = "microsoft_netframework_v1"; - _packageService + var mockPackageService = new Mock(); + mockPackageService .Setup(x => x.GetAllPackageRegistrations()) .Returns(new List().AsQueryable()); + var mockTyposquattingCheckListCacheService = new Mock(); + mockTyposquattingCheckListCacheService + .Setup(x => x.GetTyposquattingCheckList(It.IsAny(), It.IsAny(), It.IsAny())) + .Returns(new List()); - var newService = new TyposquattingService(_contentObjectService.Object, _packageService.Object, _reservedNamespaceService.Object, _telemetryService.Object); - + var newService = CreateService(packageService: mockPackageService, typosquattingCheckListCacheService: mockTyposquattingCheckListCacheService); + // Act var typosquattingCheckResult = newService.IsUploadedPackageIdTyposquatting(uploadedPackageId, _uploadedPackageOwner, out List typosquattingCheckCollisionIds); @@ -266,11 +313,19 @@ public void CheckTyposquattingNotEnabled(string packageId) { // Arrange var uploadedPackageId = packageId; - _contentObjectService + + var mockContentObjectService = new Mock(); + mockContentObjectService + .Setup(x => x.TyposquattingConfiguration.PackageIdChecklistLength) + .Returns(20000); + mockContentObjectService .Setup(x => x.TyposquattingConfiguration.IsCheckEnabled) .Returns(false); + mockContentObjectService + .Setup(x => x.TyposquattingConfiguration.IsBlockUsersEnabled) + .Returns(true); - var newService = new TyposquattingService(_contentObjectService.Object, _packageService.Object, _reservedNamespaceService.Object, _telemetryService.Object); + var newService = CreateService(contentObjectService: mockContentObjectService); // Act var typosquattingCheckResult = newService.IsUploadedPackageIdTyposquatting(uploadedPackageId, _uploadedPackageOwner, out List typosquattingCheckCollisionIds); @@ -285,12 +340,20 @@ public void CheckNotTyposquattingBlockUserNotEnabled() { // Arrange var uploadedPackageId = "new_package_for_testing"; - _contentObjectService + + var mockContentObjectService = new Mock(); + mockContentObjectService + .Setup(x => x.TyposquattingConfiguration.PackageIdChecklistLength) + .Returns(20000); + mockContentObjectService + .Setup(x => x.TyposquattingConfiguration.IsCheckEnabled) + .Returns(false); + mockContentObjectService .Setup(x => x.TyposquattingConfiguration.IsBlockUsersEnabled) .Returns(false); - var newService = new TyposquattingService(_contentObjectService.Object, _packageService.Object, _reservedNamespaceService.Object, _telemetryService.Object); - + var newService = CreateService(contentObjectService: mockContentObjectService); + // Act var typosquattingCheckResult = newService.IsUploadedPackageIdTyposquatting(uploadedPackageId, _uploadedPackageOwner, out List typosquattingCheckCollisionIds); @@ -304,12 +367,19 @@ public void CheckIsTyposquattingBlockUserNotEnabled() { // Arrange var uploadedPackageId = "Microsoft_NetFramework_v1"; - _contentObjectService + var mockContentObjectService = new Mock(); + mockContentObjectService + .Setup(x => x.TyposquattingConfiguration.PackageIdChecklistLength) + .Returns(20000); + mockContentObjectService + .Setup(x => x.TyposquattingConfiguration.IsCheckEnabled) + .Returns(true); + mockContentObjectService .Setup(x => x.TyposquattingConfiguration.IsBlockUsersEnabled) .Returns(false); - var newService = new TyposquattingService(_contentObjectService.Object, _packageService.Object, _reservedNamespaceService.Object, _telemetryService.Object); - + var newService = CreateService(contentObjectService: mockContentObjectService); + // Act var typosquattingCheckResult = newService.IsUploadedPackageIdTyposquatting(uploadedPackageId, _uploadedPackageOwner, out List typosquattingCheckCollisionIds); @@ -324,34 +394,37 @@ public void CheckTelemetryServiceLogOriginalUploadedPackageId() { // Arrange var uploadedPackageId = "microsoft_netframework.v1"; - var newService = new TyposquattingService(_contentObjectService.Object, _packageService.Object, _reservedNamespaceService.Object, _telemetryService.Object); + var mockTelemetryService = new Mock(); + + var newService = CreateService(telemetryService: mockTelemetryService); // Act var typosquattingCheckResult = newService.IsUploadedPackageIdTyposquatting(uploadedPackageId, _uploadedPackageOwner, out List typosquattingCheckCollisionIds); // Assert - _telemetryService.Verify( + mockTelemetryService.Verify( x => x.TrackMetricForTyposquattingChecklistRetrievalTime(uploadedPackageId, It.IsAny()), Times.Once); - _telemetryService.Verify( + mockTelemetryService.Verify( x => x.TrackMetricForTyposquattingAlgorithmProcessingTime(uploadedPackageId, It.IsAny()), Times.Once); - _telemetryService.Verify( + mockTelemetryService.Verify( x => x.TrackMetricForTyposquattingCheckResultAndTotalTime( uploadedPackageId, It.IsAny(), It.IsAny(), It.IsAny>(), - It.IsAny()), + It.IsAny(), + It.IsAny()), Times.Once); - _telemetryService.Verify( + mockTelemetryService.Verify( x => x.TrackMetricForTyposquattingOwnersCheckTime(uploadedPackageId, It.IsAny()), Times.Once); } - + [Theory] [InlineData("Microsoft_NetFramework_v1", "Microsoft.NetFramework.v1", 0)] [InlineData("Microsoft_NetFramework_v1", "microsoft-netframework-v1", 0)] From 2cf936fe74ccf11f10c404d8609b90a03f7d869b Mon Sep 17 00:00:00 2001 From: Shishir H Date: Mon, 5 Nov 2018 17:11:00 -0800 Subject: [PATCH 18/20] [Symbol server]Address UI fixes and fix re-upload on failed validation issues for Symbol Server (#6627) --- .../Services/SymbolPackageUploadService.cs | 19 ++++++- .../Views/Packages/DisplayPackage.cshtml | 16 +++--- .../SymbolPackageUploadServiceFacts.cs | 53 +++++++++++++++++++ 3 files changed, 80 insertions(+), 8 deletions(-) diff --git a/src/NuGetGallery/Services/SymbolPackageUploadService.cs b/src/NuGetGallery/Services/SymbolPackageUploadService.cs index 1dbaf994da..24f0c58747 100644 --- a/src/NuGetGallery/Services/SymbolPackageUploadService.cs +++ b/src/NuGetGallery/Services/SymbolPackageUploadService.cs @@ -146,6 +146,7 @@ public async Task CreateAndUploadSymbolsPackage(Package pac Stream symbolPackageFile = symbolPackageStream.AsSeekableStream(); + var previousSymbolsPackage = package.LatestSymbolPackage(); var symbolPackage = _symbolPackageService.CreateSymbolPackage(package, packageStreamMetadata); await _validationService.StartValidationAsync(symbolPackage); @@ -161,6 +162,16 @@ public async Task CreateAndUploadSymbolsPackage(Package pac { if (symbolPackage.StatusKey == PackageStatus.Validating) { + // If the last uploaded symbol package has failed validation, it will leave the snupkg in the + // validations container. We could possibly overwrite it, but that might introduce a concurrency + // issue on multiple snupkg uploads with a prior failed validation. The best thing to do would be + // to delete the failed validation snupkg from validations container and then proceed with normal + // upload. + if (previousSymbolsPackage != null && previousSymbolsPackage.StatusKey == PackageStatus.FailedValidation) + { + await DeleteSymbolsPackageAsync(previousSymbolsPackage); + } + await _symbolPackageFileService.SaveValidationPackageFileAsync(symbolPackage.Package, symbolPackageFile); } else if (symbolPackage.StatusKey == PackageStatus.Available) @@ -239,7 +250,13 @@ public async Task DeleteSymbolsPackageAsync(SymbolPackage symbolPackage) throw new ArgumentNullException(nameof(symbolPackage)); } - if (await _symbolPackageFileService.DoesPackageFileExistAsync(symbolPackage.Package)) + if (symbolPackage.StatusKey == PackageStatus.FailedValidation + && await _symbolPackageFileService.DoesValidationPackageFileExistAsync(symbolPackage.Package)) + { + await _symbolPackageFileService.DeleteValidationPackageFileAsync(symbolPackage.Id, symbolPackage.Version); + } + else if (symbolPackage.StatusKey == PackageStatus.Available + && await _symbolPackageFileService.DoesPackageFileExistAsync(symbolPackage.Package)) { await _symbolPackageFileService.DeletePackageFileAsync(symbolPackage.Id, symbolPackage.Version); } diff --git a/src/NuGetGallery/Views/Packages/DisplayPackage.cshtml b/src/NuGetGallery/Views/Packages/DisplayPackage.cshtml index 6e615a174c..9968291441 100644 --- a/src/NuGetGallery/Views/Packages/DisplayPackage.cshtml +++ b/src/NuGetGallery/Views/Packages/DisplayPackage.cshtml @@ -275,9 +275,8 @@ { @ViewHelpers.AlertWarning( @ - The symbols for this package have not been indexed yet. They are not available - for download from the NuGet.org symbol server. Symbols will be indexed and will - be available for download after both validation and indexing are complete. + The symbols for this package have not been indexed yet. They are not available + for download from the NuGet.org symbol server. Symbols will be available for download after both validation and indexing are complete. Symbols validation and indexing may take up to an hour. Read more. ) @@ -721,10 +720,13 @@ Revalidate package -
  • - - Revalidate symbols -
    • + if (Model.LatestSymbolsPackage != null) + { +
  • + + Revalidate symbols +
    • + } } @if (User.IsAdministrator() && Config.Current.AsynchronousPackageValidationEnabled) diff --git a/tests/NuGetGallery.Facts/Services/SymbolPackageUploadServiceFacts.cs b/tests/NuGetGallery.Facts/Services/SymbolPackageUploadServiceFacts.cs index 7a60eccb20..4c48e8f8f6 100644 --- a/tests/NuGetGallery.Facts/Services/SymbolPackageUploadServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/SymbolPackageUploadServiceFacts.cs @@ -264,6 +264,59 @@ public async Task WillReturnConflictIfFileExistsInContainer() symbolPackageFileService.Verify(x => x.SavePackageFileAsync(package, It.IsAny(), It.IsAny()), Times.Once); } + [Fact] + public async Task WillDeleteFailedValidationSnupkg() + { + // Arrange + var symbolPackageService = new Mock(); + symbolPackageService + .Setup(x => x.CreateSymbolPackage(It.IsAny(), It.IsAny())) + .Returns((Package package1, PackageStreamMetadata streamMetadata) => + { + var symbolPackage = new SymbolPackage() + { + Package = package1, + PackageKey = package1.Key, + Created = DateTime.UtcNow, + StatusKey = PackageStatus.Validating + }; + + return symbolPackage; + }) + .Verifiable(); + + var symbolPackageFileService = new Mock(); + symbolPackageFileService + .Setup(x => x.DoesValidationPackageFileExistAsync(It.IsAny())) + .ReturnsAsync(true) + .Verifiable(); + symbolPackageFileService + .Setup(x => x.DeleteValidationPackageFileAsync(It.IsAny(), It.IsAny())) + .Completes() + .Verifiable(); + symbolPackageFileService + .Setup(x => x.SaveValidationPackageFileAsync(It.IsAny(), It.IsAny())) + .Completes() + .Verifiable(); + + var service = CreateService(symbolPackageService: symbolPackageService, symbolPackageFileService: symbolPackageFileService); + var package = new Package() { PackageRegistration = new PackageRegistration() { Id = "theId" }, Version = "1.0.23" }; + package.SymbolPackages.Add(new SymbolPackage() + { + StatusKey = PackageStatus.FailedValidation, + Key = 1232, + Package = package + }); + + // Act + var result = await service.CreateAndUploadSymbolsPackage(package, new MemoryStream()); + + // Assert + Assert.NotNull(result); + Assert.Equal(PackageCommitResult.Success, result); + symbolPackageFileService.VerifyAll(); + } + [Fact] public async Task WillDeleteSavedFileAndThrowWhenDBWriteFails() { From 56126b148da9954dfbc559c7d0d9d8e591d785ac Mon Sep 17 00:00:00 2001 From: zhhyu <41028779+zhhyu@users.noreply.github.com> Date: Tue, 6 Nov 2018 14:06:01 -0800 Subject: [PATCH 19/20] Fix default value bug and add tests (#6630) --- .../Services/TyposquattingConfiguration.cs | 8 +++--- .../Services/ContentObjectServiceFacts.cs | 27 +++++++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/NuGetGallery/Services/TyposquattingConfiguration.cs b/src/NuGetGallery/Services/TyposquattingConfiguration.cs index 6ff9bc6de5..02cccb939d 100644 --- a/src/NuGetGallery/Services/TyposquattingConfiguration.cs +++ b/src/NuGetGallery/Services/TyposquattingConfiguration.cs @@ -7,8 +7,8 @@ namespace NuGetGallery.Services { public sealed class TyposquattingConfiguration : ITyposquattingConfiguration { - private const int DefaultPackageIdCheckListLength = 20000; - private const double DefaultPackageIdChecklistCacheExpireTimeInHours = 24; + public const int DefaultPackageIdCheckListLength = 10000; + public const double DefaultPackageIdChecklistCacheExpireTimeInHours = 24.0; public int PackageIdChecklistLength { get; } public bool IsCheckEnabled { get; } public bool IsBlockUsersEnabled { get; } @@ -29,10 +29,10 @@ public TyposquattingConfiguration( bool isBlockUsersEnabled, double packageIdChecklistCacheExpireTimeInHours) { - PackageIdChecklistLength = packageIdChecklistLength; + PackageIdChecklistLength = packageIdChecklistLength == default(int) ? DefaultPackageIdCheckListLength : packageIdChecklistLength; IsCheckEnabled = isCheckEnabled; IsBlockUsersEnabled = isBlockUsersEnabled; - PackageIdChecklistCacheExpireTimeInHours = packageIdChecklistCacheExpireTimeInHours; + PackageIdChecklistCacheExpireTimeInHours = packageIdChecklistCacheExpireTimeInHours == default(double) ? DefaultPackageIdChecklistCacheExpireTimeInHours : packageIdChecklistCacheExpireTimeInHours; } } } \ No newline at end of file diff --git a/tests/NuGetGallery.Facts/Services/ContentObjectServiceFacts.cs b/tests/NuGetGallery.Facts/Services/ContentObjectServiceFacts.cs index 48dcfc1bac..557c2aca06 100644 --- a/tests/NuGetGallery.Facts/Services/ContentObjectServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/ContentObjectServiceFacts.cs @@ -36,6 +36,13 @@ public void ObjectsHaveDefaultStateIfNotRefreshed() Assert.False(symbolsConfiguration.IsSymbolsUploadEnabledForAll); Assert.Empty(symbolsConfiguration.AlwaysEnabledForDomains); Assert.Empty(symbolsConfiguration.AlwaysEnabledForEmailAddresses); + + var typosquattingConfiguration = service.TyposquattingConfiguration as TyposquattingConfiguration; + + Assert.Equal(TyposquattingConfiguration.DefaultPackageIdCheckListLength, typosquattingConfiguration.PackageIdChecklistLength); + Assert.False(typosquattingConfiguration.IsCheckEnabled); + Assert.False(typosquattingConfiguration.IsBlockUsersEnabled); + Assert.Equal(TyposquattingConfiguration.DefaultPackageIdChecklistCacheExpireTimeInHours, typosquattingConfiguration.PackageIdChecklistCacheExpireTimeInHours); } [Fact] @@ -55,6 +62,9 @@ public async Task RefreshRefreshesObject() var alwaysEnabledForDomains = new[] { "a" }; var alwaysEnabledForEmailAddresses = new[] { "b" }; + var packageIdChecklistLength = 20000; + var packageIdChecklistCacheExpireTimeInHours = 12.0; + var certificatesConfiguration = new CertificatesConfiguration( isUIEnabledByDefault, alwaysEnabledForDomains, @@ -67,6 +77,13 @@ public async Task RefreshRefreshesObject() alwaysEnabledForEmailAddresses: alwaysEnabledForEmailAddresses); var symbolsJson = JsonConvert.SerializeObject(symbolsConfiguration); + var typosquattingConfiguration = new TyposquattingConfiguration( + packageIdChecklistLength: packageIdChecklistLength, + isCheckEnabled: true, + isBlockUsersEnabled: true, + packageIdChecklistCacheExpireTimeInHours: packageIdChecklistCacheExpireTimeInHours); + var typosquattingJson = JsonConvert.SerializeObject(typosquattingConfiguration); + var contentService = GetMock(); contentService @@ -81,6 +98,10 @@ public async Task RefreshRefreshesObject() .Setup(x => x.GetContentItemAsync(GalleryConstants.ContentNames.SymbolsConfiguration, It.IsAny())) .Returns(Task.FromResult(new HtmlString(symbolsJson))); + contentService + .Setup(x => x.GetContentItemAsync(GalleryConstants.ContentNames.TyposquattingConfiguration, It.IsAny())) + .Returns(Task.FromResult(new HtmlString(typosquattingJson))); + var service = GetService(); // Act @@ -89,6 +110,7 @@ public async Task RefreshRefreshesObject() loginDiscontinuationConfiguration = service.LoginDiscontinuationConfiguration as LoginDiscontinuationConfiguration; certificatesConfiguration = service.CertificatesConfiguration as CertificatesConfiguration; symbolsConfiguration = service.SymbolsConfiguration as SymbolsConfiguration; + typosquattingConfiguration = service.TyposquattingConfiguration as TyposquattingConfiguration; // Assert Assert.True(loginDiscontinuationConfiguration.DiscontinuedForEmailAddresses.SequenceEqual(emails)); @@ -103,6 +125,11 @@ public async Task RefreshRefreshesObject() Assert.True(symbolsConfiguration.IsSymbolsUploadEnabledForAll); Assert.Equal(alwaysEnabledForDomains, symbolsConfiguration.AlwaysEnabledForDomains); Assert.Equal(alwaysEnabledForEmailAddresses, symbolsConfiguration.AlwaysEnabledForEmailAddresses); + + Assert.Equal(packageIdChecklistLength, typosquattingConfiguration.PackageIdChecklistLength); + Assert.True(typosquattingConfiguration.IsCheckEnabled); + Assert.True(typosquattingConfiguration.IsBlockUsersEnabled); + Assert.Equal(packageIdChecklistCacheExpireTimeInHours, typosquattingConfiguration.PackageIdChecklistCacheExpireTimeInHours); } } } From 663cd345d93b57b9b7801a1fd99e38d2a1edbe47 Mon Sep 17 00:00:00 2001 From: Svetlana Kofman Date: Tue, 6 Nov 2018 14:15:14 -0800 Subject: [PATCH 20/20] Http to https translation for GitHub pages urls (#6626) --- src/NuGetGallery/Helpers/UriExtensions.cs | 8 +++++++- tests/NuGetGallery.Facts/Helpers/HtmlExtensionsFacts.cs | 1 + .../ViewModels/DisplayPackageViewModelFacts.cs | 4 ++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/NuGetGallery/Helpers/UriExtensions.cs b/src/NuGetGallery/Helpers/UriExtensions.cs index e982b5b609..e16c92ed98 100644 --- a/src/NuGetGallery/Helpers/UriExtensions.cs +++ b/src/NuGetGallery/Helpers/UriExtensions.cs @@ -26,7 +26,7 @@ public static bool IsGitProtocol(this Uri uri) public static bool IsDomainWithHttpsSupport(this Uri uri) { - return IsGitHubUri(uri) || IsCodeplexUri(uri) || IsMicrosoftUri(uri); + return IsGitHubUri(uri) || IsGitHubPagerUri(uri) || IsCodeplexUri(uri) || IsMicrosoftUri(uri); } public static bool IsGitHubUri(this Uri uri) @@ -35,6 +35,12 @@ public static bool IsGitHubUri(this Uri uri) string.Equals(uri.Host, "github.com", StringComparison.OrdinalIgnoreCase); } + public static bool IsGitHubPagerUri(this Uri uri) + { + return uri.Authority.EndsWith(".github.com", StringComparison.OrdinalIgnoreCase) || + uri.Authority.EndsWith(".github.io", StringComparison.OrdinalIgnoreCase); + } + private static bool IsCodeplexUri(this Uri uri) { return uri.Authority.EndsWith(".codeplex.com", StringComparison.OrdinalIgnoreCase) || diff --git a/tests/NuGetGallery.Facts/Helpers/HtmlExtensionsFacts.cs b/tests/NuGetGallery.Facts/Helpers/HtmlExtensionsFacts.cs index 774e0fa565..c78fae1ad7 100644 --- a/tests/NuGetGallery.Facts/Helpers/HtmlExtensionsFacts.cs +++ b/tests/NuGetGallery.Facts/Helpers/HtmlExtensionsFacts.cs @@ -78,6 +78,7 @@ public void EncodesHtml() [InlineData("http://www.github.com/nuget is my site.", "https://www.github.com/nuget is my site.")] [InlineData("My site is http://www.asp.net best site ever!", "My site is https://www.asp.net/ best site ever!")] [InlineData("My site is http:////github.com bad url", "My site is http:////github.com bad url")] + [InlineData("Im using github pages! http://mypage.github.com/stuff.", "Im using github pages! https://mypage.github.com/stuff.")] public void ConvertsUrlsToLinks(string input, string expected) { // Arrange diff --git a/tests/NuGetGallery.Facts/ViewModels/DisplayPackageViewModelFacts.cs b/tests/NuGetGallery.Facts/ViewModels/DisplayPackageViewModelFacts.cs index c566cda78c..e8445fc96b 100644 --- a/tests/NuGetGallery.Facts/ViewModels/DisplayPackageViewModelFacts.cs +++ b/tests/NuGetGallery.Facts/ViewModels/DisplayPackageViewModelFacts.cs @@ -78,6 +78,8 @@ public void ItDeterminesRepositoryKind(string repoUrl, string repoType, Reposito [InlineData("http://msdn.microsoft.com/en-us/library/vstudio/hh191443.aspx", "https://msdn.microsoft.com/en-us/library/vstudio/hh191443.aspx")] [InlineData("http://microsoft.com/iconurl/9594202", "https://microsoft.com/iconurl/9594202")] [InlineData("http://microsoft.com:80/", "https://microsoft.com/")] + [InlineData("http://githubpages.github.io/my.page", "https://githubpages.github.io/my.page")] + [InlineData("http://githubpages.github.com", "https://githubpages.github.com/")] public void ItInitializesProjectUrl(string projectUrl, string expected) { var package = new Package @@ -99,6 +101,8 @@ public void ItInitializesProjectUrl(string projectUrl, string expected) [InlineData("http://aspnetwebstack.codeplex.com/license", "https://aspnetwebstack.codeplex.com/license")] [InlineData("http://go.microsoft.com/?linkid=9809688", "https://go.microsoft.com/?linkid=9809688")] [InlineData("http://github.com/url", "https://github.com/url")] + [InlineData("http://githubpages.github.io/my.page/license.html", "https://githubpages.github.io/my.page/license.html")] + [InlineData("http://githubpages.github.com", "https://githubpages.github.com/")] public void ItInitializesLicenseUrl(string licenseUrl, string expected) { var package = new Package