Validation message enhancements

src/NuGetGallery/Controllers/ApiController.cs

@@ -712,12 +712,12 @@ await AuditingService.SaveAuditRecordAsync(
 
                             TelemetryService.TrackPackagePushEvent(package, currentUser, User.Identity);
 
-                            var warnings = new List<string>();
+                            var warnings = new List<IValidationMessage>();
                             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);
@@ -762,9 +762,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.");
             }

src/NuGetGallery/Infrastructure/HttpStatusCodeWithServerWarningResult.cs

@@ -18,6 +18,12 @@ public HttpStatusCodeWithServerWarningResult(HttpStatusCode statusCode, IReadOnl
             Warnings = warnings ?? new string[0];
         }
 
+        public HttpStatusCodeWithServerWarningResult(HttpStatusCode statusCode, IReadOnlyList<IValidationMessage> warnings)
+            : this(statusCode, warnings.Select(w => w.PlainTextMessage).ToList())
+        {
+
+        }
+
         public override void ExecuteResult(ControllerContext context)
         {
             var response = context.RequestContext.HttpContext.Response;

src/NuGetGallery/NuGetGallery.csproj

@@ -426,6 +426,10 @@
     <Compile Include="Services\ITyposquattingConfiguration.cs" />
     <Compile Include="Services\ISymbolsConfiguration.cs" />
     <Compile Include="Services\ITyposquattingService.cs" />
+    <Compile Include="Services\IValidationMessage.cs" />
+    <Compile Include="Services\JsonValidationMessage.cs" />
+    <Compile Include="Services\PackageShouldNotBeSignedUserFixableValidationMessage.cs" />
+    <Compile Include="Services\PlainTextOnlyValidationMessage.cs" />
     <Compile Include="Services\SymbolPackageValidationResult.cs" />
     <Compile Include="Infrastructure\Mail\MarkdownMessageService.cs" />
     <Compile Include="Services\SymbolPackageUploadService.cs" />

src/NuGetGallery/RequestModels/VerifyPackageRequest.cs

@@ -120,7 +120,7 @@ public VerifyPackageRequest(PackageMetadata packageMetadata, IEnumerable<User> p
         public bool IsSymbolsPackage { get; set; }
         public bool HasExistingAvailableSymbols { get; set; }
 
-        public List<string> Warnings { get; set; } = new List<string>();
+        public List<JsonValidationMessage> Warnings { get; set; } = new List<JsonValidationMessage>();
 
         private static IReadOnlyCollection<string> ParseUserList(IEnumerable<User> users)
         {

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
+{
+    /// <summary>
+    /// Represents a validation message that may include a raw HTML message (if we want to present some links in the message, for example).
+    /// </summary>
+    /// <remarks>
+    /// Implementations should sanitize all input used to generate <see cref="IValidationMessage.RawHtmlMessage"/> 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 <see cref="IValidationMessage.RawHtmlMessage"/>.
+    /// </remarks>
+    public interface IValidationMessage
+    {
+        /// <summary>
+        /// Plain text representation of the validation message. If pasted into HTML, will be html encoded.
+        /// </summary>
+        string PlainTextMessage { get; }
+
+        /// <summary>
+        /// An indicator that raw HTML representation is available.
+        /// </summary>
+        bool HasRawHtmlRepresentation { get; }
+
+        /// <summary>
+        /// HANDLE WITH EXTREME CARE. Raw HTML representation of the message.
+        /// Under no conditions it may contain unvalidated user data.
+        /// </summary>
+        string RawHtmlMessage { get; }
+    }
+}

src/NuGetGallery/Services/JsonValidationMessage.cs

@@ -0,0 +1,34 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+namespace NuGetGallery
+{
+    /// <summary>
+    /// Validation message representation for returning in Json responses for Gallery web pages.
+    /// </summary>
+    public class JsonValidationMessage
+    {
+        public JsonValidationMessage(string message)
+        {
+            PlainTextMessage = message;
+            RawHtmlMessage = null;
+        }
+
+        public JsonValidationMessage(IValidationMessage message)
+        {
+            if (message.HasRawHtmlRepresentation)
+            {
+                PlainTextMessage = null;
+                RawHtmlMessage = message.RawHtmlMessage;
+            }
+            else
+            {
+                PlainTextMessage = message.PlainTextMessage;
+                RawHtmlMessage = null;
+            }
+        }
+
+        public string PlainTextMessage { get; }
+        public string RawHtmlMessage { get; }
+    }
+}

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
+{
+    /// <summary>
+    /// 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.
+    /// </summary>
+    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);
+    }
+}

src/NuGetGallery/Services/PackageUploadService.cs

@@ -46,7 +46,7 @@ public PackageUploadService(
 
         public async Task<PackageValidationResult> ValidateBeforeGeneratePackageAsync(PackageArchiveReader nuGetPackage, PackageMetadata packageMetadata)
         {
-            var warnings = new List<string>();
+            var warnings = new List<IValidationMessage>();
 
             var result = await CheckPackageEntryCountAsync(nuGetPackage, warnings);
 
@@ -119,7 +119,7 @@ private PackageValidationResult CheckLicenseMetadata(PackageArchiveReader nuGetP
 
         private async Task<PackageValidationResult> CheckPackageEntryCountAsync(
             PackageArchiveReader nuGetPackage,
-            List<string> warnings)
+            List<IValidationMessage> warnings)
         {
             if (!_config.RejectPackagesWithTooManyPackageEntries)
             {
@@ -150,7 +150,7 @@ private async Task<PackageValidationResult> 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://"
         /// </summary>
-        private PackageValidationResult CheckRepositoryMetadata(PackageMetadata packageMetadata, List<string> warnings)
+        private PackageValidationResult CheckRepositoryMetadata(PackageMetadata packageMetadata, List<IValidationMessage> warnings)
         {
             if (packageMetadata.RepositoryUrl == null)
             {
@@ -162,14 +162,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));
                 }
             }
 
@@ -187,7 +187,7 @@ private PackageValidationResult CheckRepositoryMetadata(PackageMetadata packageM
         /// <returns>The package validation result or null.</returns>
         private async Task<PackageValidationResult> CheckForUnsignedPushAfterAuthorSignedAsync(
             PackageArchiveReader nuGetPackage,
-            List<string> warnings)
+            List<IValidationMessage> warnings)
         {
             // If the package is signed, there's no problem.
             if (await nuGetPackage.IsSignedAsync(CancellationToken.None))
@@ -218,9 +218,10 @@ private async Task<PackageValidationResult> 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;
@@ -269,14 +270,11 @@ private async Task<PackageValidationResult> 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));
@@ -293,14 +291,11 @@ private async Task<PackageValidationResult> 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));

src/NuGetGallery/Services/PackageValidationResult.cs

@@ -12,14 +12,19 @@ namespace NuGetGallery
     /// </summary>
     public class PackageValidationResult
     {
-        private static readonly IReadOnlyList<string> EmptyList = new string[0];
+        private static readonly IReadOnlyList<IValidationMessage> 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<string> warnings)
+        public PackageValidationResult(PackageValidationResultType type, string message)
+            : this(type, new PlainTextOnlyValidationMessage(message))
+        {
+        }
+
+        public PackageValidationResult(PackageValidationResultType type, IValidationMessage message, IReadOnlyList<IValidationMessage> 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<string> Warnings { get; }
+        public IValidationMessage Message { get; }
+        public IReadOnlyList<IValidationMessage> Warnings { get; }
 
         public static PackageValidationResult Accepted()
         {
@@ -43,7 +48,7 @@ public static PackageValidationResult Accepted()
                 warnings: null);
         }
 
-        public static PackageValidationResult AcceptedWithWarnings(IReadOnlyList<string> warnings)
+        public static PackageValidationResult AcceptedWithWarnings(IReadOnlyList<IValidationMessage> warnings)
         {
             return new PackageValidationResult(
                 PackageValidationResultType.Accepted,
@@ -52,6 +57,9 @@ public static PackageValidationResult AcceptedWithWarnings(IReadOnlyList<string>
         }
 
         public static PackageValidationResult Invalid(string message)
+            => Invalid(new PlainTextOnlyValidationMessage(message));
+
+        public static PackageValidationResult Invalid(IValidationMessage message)
         {
             if (message == null)
             {

src/NuGetGallery/Services/PackageValidationResultType.cs

@@ -18,17 +18,5 @@ public enum PackageValidationResultType
         /// The package is invalid based on the package content.
         /// </summary>
         Invalid,
-
-        /// <summary>
-        /// The package is invalid because it should not be signed given the current required signer certificates (or
-        /// lack thereof).
-        /// </summary>
-        PackageShouldNotBeSigned,
-
-        /// <summary>
-        /// Similar to <see cref="PackageShouldNotBeSigned"/> but the current user can manage certificates and
-        /// potentially remediate the situation.
-        /// </summary>
-        PackageShouldNotBeSignedButCanManageCertificates,
     }
 }

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
+{
+    /// <summary>
+    /// Cautious default implementation of <see cref="IValidationMessage"/> that does not allow
+    /// user to specify raw html response.
+    /// </summary>
+    public class PlainTextOnlyValidationMessage : IValidationMessage
+    {
+        public PlainTextOnlyValidationMessage(string validationMessage)
+        {
+            PlainTextMessage = validationMessage;
+        }
+
+        public string PlainTextMessage { get; }
+
+        public bool HasRawHtmlRepresentation => false;
+
+        public string RawHtmlMessage => null;
+    }
+}