Skip to content

Commit 7e155f7

Browse files
agragr
authored
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.
1 parent 6ebb7a2 commit 7e155f7

16 files changed

Lines changed: 351 additions & 183 deletions

‎src/NuGetGallery/Controllers/ApiController.cs‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -710,12 +710,12 @@ await AuditingService.SaveAuditRecordAsync(
710710

711711
TelemetryService.TrackPackagePushEvent(package, currentUser, User.Identity);
712712

713-
var warnings = new List<string>();
713+
var warnings = new List<IValidationMessage>();
714714
warnings.AddRange(beforeValidationResult.Warnings);
715715
warnings.AddRange(afterValidationResult.Warnings);
716716
if (package.SemVerLevelKey == SemVerLevelKey.SemVer2)
717717
{
718-
warnings.Add(Strings.WarningSemVer2PackagePushed);
718+
warnings.Add(new PlainTextOnlyValidationMessage(Strings.WarningSemVer2PackagePushed));
719719
}
720720

721721
return new HttpStatusCodeWithServerWarningResult(HttpStatusCode.Created, warnings);
@@ -764,9 +764,7 @@ private static ActionResult GetActionResultOrNull(PackageValidationResult valida
764764
case PackageValidationResultType.Accepted:
765765
return null;
766766
case PackageValidationResultType.Invalid:
767-
case PackageValidationResultType.PackageShouldNotBeSigned:
768-
case PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates:
769-
return new HttpStatusCodeWithBodyResult(HttpStatusCode.BadRequest, validationResult.Message);
767+
return new HttpStatusCodeWithBodyResult(HttpStatusCode.BadRequest, validationResult.Message.PlainTextMessage);
770768
default:
771769
throw new NotImplementedException($"The package validation result type {validationResult.Type} is not supported.");
772770
}

‎src/NuGetGallery/Controllers/PackagesController.cs‎

Lines changed: 68 additions & 61 deletions
Large diffs are not rendered by default.

‎src/NuGetGallery/Infrastructure/HttpStatusCodeWithServerWarningResult.cs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,11 @@ public HttpStatusCodeWithServerWarningResult(HttpStatusCode statusCode, IReadOnl
1818
Warnings = warnings ?? new string[0];
1919
}
2020

21+
public HttpStatusCodeWithServerWarningResult(HttpStatusCode statusCode, IReadOnlyList<IValidationMessage> warnings)
22+
: this(statusCode, warnings.Select(w => w.PlainTextMessage).ToList())
23+
{
24+
}
25+
2126
public override void ExecuteResult(ControllerContext context)
2227
{
2328
var response = context.RequestContext.HttpContext.Response;

‎src/NuGetGallery/NuGetGallery.csproj‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -426,6 +426,10 @@
426426
<Compile Include="Services\ITyposquattingConfiguration.cs" />
427427
<Compile Include="Services\ISymbolsConfiguration.cs" />
428428
<Compile Include="Services\ITyposquattingService.cs" />
429+
<Compile Include="Services\IValidationMessage.cs" />
430+
<Compile Include="Services\JsonValidationMessage.cs" />
431+
<Compile Include="Services\PackageShouldNotBeSignedUserFixableValidationMessage.cs" />
432+
<Compile Include="Services\PlainTextOnlyValidationMessage.cs" />
429433
<Compile Include="Services\SymbolPackageValidationResult.cs" />
430434
<Compile Include="Infrastructure\Mail\MarkdownMessageService.cs" />
431435
<Compile Include="Services\SymbolPackageUploadService.cs" />

‎src/NuGetGallery/RequestModels/VerifyPackageRequest.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ public VerifyPackageRequest(PackageMetadata packageMetadata, IEnumerable<User> p
120120
public bool IsSymbolsPackage { get; set; }
121121
public bool HasExistingAvailableSymbols { get; set; }
122122

123-
public List<string> Warnings { get; set; } = new List<string>();
123+
public List<JsonValidationMessage> Warnings { get; set; } = new List<JsonValidationMessage>();
124124

125125
private static IReadOnlyCollection<string> ParseUserList(IEnumerable<User> users)
126126
{

‎src/NuGetGallery/Services/IValidationMessage.cs‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
// Copyright (c) .NET Foundation. All rights reserved.
2+
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
3+
4+
namespace NuGetGallery
5+
{
6+
/// <summary>
7+
/// Represents a validation message that may include a raw HTML message (if we want to present some links in the message, for example).
8+
/// </summary>
9+
/// <remarks>
10+
/// Implementations should sanitize all input used to generate <see cref="IValidationMessage.RawHtmlMessage"/> value.
11+
/// We should never have a generic implementation that would accept a raw HTML message as a constructor argument and
12+
/// simply returns it as a value of <see cref="IValidationMessage.RawHtmlMessage"/>.
13+
/// </remarks>
14+
public interface IValidationMessage
15+
{
16+
/// <summary>
17+
/// Plain text representation of the validation message. If pasted into HTML, will be html encoded.
18+
/// </summary>
19+
string PlainTextMessage { get; }
20+
21+
/// <summary>
22+
/// An indicator that raw HTML representation is available.
23+
/// </summary>
24+
bool HasRawHtmlRepresentation { get; }
25+
26+
/// <summary>
27+
/// HANDLE WITH EXTREME CARE. Raw HTML representation of the message.
28+
/// Under no conditions may it contain unvalidated user data.
29+
/// </summary>
30+
string RawHtmlMessage { get; }
31+
}
32+
}

‎src/NuGetGallery/Services/JsonValidationMessage.cs‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
// Copyright (c) .NET Foundation. All rights reserved.
2+
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
3+
4+
using System;
5+
6+
namespace NuGetGallery
7+
{
8+
/// <summary>
9+
/// Validation message representation for returning in Json responses for Gallery web pages.
10+
/// </summary>
11+
public class JsonValidationMessage
12+
{
13+
public JsonValidationMessage(string message)
14+
{
15+
PlainTextMessage = message ?? throw new ArgumentNullException(nameof(message));
16+
RawHtmlMessage = null;
17+
}
18+
19+
public JsonValidationMessage(IValidationMessage message)
20+
{
21+
if (message == null)
22+
{
23+
throw new ArgumentNullException(nameof(message));
24+
}
25+
26+
if (message.HasRawHtmlRepresentation)
27+
{
28+
PlainTextMessage = null;
29+
RawHtmlMessage = message.RawHtmlMessage;
30+
}
31+
else
32+
{
33+
PlainTextMessage = message.PlainTextMessage;
34+
RawHtmlMessage = null;
35+
}
36+
}
37+
38+
public string PlainTextMessage { get; }
39+
public string RawHtmlMessage { get; }
40+
}
41+
}

‎src/NuGetGallery/Services/PackageShouldNotBeSignedUserFixableValidationMessage.cs‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// Copyright (c) .NET Foundation. All rights reserved.
2+
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
3+
4+
using System.Web;
5+
6+
namespace NuGetGallery
7+
{
8+
/// <summary>
9+
/// Represents an error message to be displayed when user uploads a signed package but theirs account
10+
/// was not setup to accept signed packages. We want to display some additional text when the error is
11+
/// presented on a web page, so this class abuses the messaging a little, no actual HTML is generated.
12+
/// </summary>
13+
public class PackageShouldNotBeSignedUserFixableValidationMessage : IValidationMessage
14+
{
15+
public string PlainTextMessage => Strings.UploadPackage_PackageIsSignedButMissingCertificate_CurrentUserCanManageCertificates;
16+
17+
public bool HasRawHtmlRepresentation => true;
18+
19+
public string RawHtmlMessage
20+
=> HttpUtility.HtmlEncode(
21+
Strings.UploadPackage_PackageIsSignedButMissingCertificate_CurrentUserCanManageCertificates
22+
+ " "
23+
+ Strings.UploadPackage_PackageIsSignedButMissingCertificate_ManageCertificate);
24+
}
25+
}

‎src/NuGetGallery/Services/PackageUploadService.cs‎

Lines changed: 14 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ public PackageUploadService(
4848

4949
public async Task<PackageValidationResult> ValidateBeforeGeneratePackageAsync(PackageArchiveReader nuGetPackage, PackageMetadata packageMetadata)
5050
{
51-
var warnings = new List<string>();
51+
var warnings = new List<IValidationMessage>();
5252

5353
var result = await CheckPackageEntryCountAsync(nuGetPackage, warnings);
5454

@@ -121,7 +121,7 @@ private PackageValidationResult CheckLicenseMetadata(PackageArchiveReader nuGetP
121121

122122
private async Task<PackageValidationResult> CheckPackageEntryCountAsync(
123123
PackageArchiveReader nuGetPackage,
124-
List<string> warnings)
124+
List<IValidationMessage> warnings)
125125
{
126126
if (!_config.RejectPackagesWithTooManyPackageEntries)
127127
{
@@ -152,7 +152,7 @@ private async Task<PackageValidationResult> CheckPackageEntryCountAsync(
152152
/// 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.
153153
/// 2. For types other then "git" - URL scheme should be "https://"
154154
/// </summary>
155-
private PackageValidationResult CheckRepositoryMetadata(PackageMetadata packageMetadata, List<string> warnings)
155+
private PackageValidationResult CheckRepositoryMetadata(PackageMetadata packageMetadata, List<IValidationMessage> warnings)
156156
{
157157
if (packageMetadata.RepositoryUrl == null)
158158
{
@@ -164,14 +164,14 @@ private PackageValidationResult CheckRepositoryMetadata(PackageMetadata packageM
164164
{
165165
if (!packageMetadata.RepositoryUrl.IsGitProtocol() && !packageMetadata.RepositoryUrl.IsHttpsProtocol())
166166
{
167-
warnings.Add(Strings.WarningNotHttpsOrGitRepositoryUrlScheme);
167+
warnings.Add(new PlainTextOnlyValidationMessage(Strings.WarningNotHttpsOrGitRepositoryUrlScheme));
168168
}
169169
}
170170
else
171171
{
172172
if (!packageMetadata.RepositoryUrl.IsHttpsProtocol())
173173
{
174-
warnings.Add(Strings.WarningNotHttpsRepositoryUrlScheme);
174+
warnings.Add(new PlainTextOnlyValidationMessage(Strings.WarningNotHttpsRepositoryUrlScheme));
175175
}
176176
}
177177

@@ -189,7 +189,7 @@ private PackageValidationResult CheckRepositoryMetadata(PackageMetadata packageM
189189
/// <returns>The package validation result or null.</returns>
190190
private async Task<PackageValidationResult> CheckForUnsignedPushAfterAuthorSignedAsync(
191191
PackageArchiveReader nuGetPackage,
192-
List<string> warnings)
192+
List<IValidationMessage> warnings)
193193
{
194194
// If the package is signed, there's no problem.
195195
if (await nuGetPackage.IsSignedAsync(CancellationToken.None))
@@ -220,9 +220,10 @@ private async Task<PackageValidationResult> CheckForUnsignedPushAfterAuthorSigne
220220

221221
if (previousPackage != null && previousPackage.CertificateKey.HasValue)
222222
{
223-
warnings.Add(string.Format(
224-
Strings.UploadPackage_SignedToUnsignedTransition,
225-
previousPackage.Version.ToNormalizedString()));
223+
warnings.Add(new PlainTextOnlyValidationMessage(
224+
string.Format(
225+
Strings.UploadPackage_SignedToUnsignedTransition,
226+
previousPackage.Version.ToNormalizedString())));
226227
}
227228

228229
return null;
@@ -271,14 +272,11 @@ private async Task<PackageValidationResult> ValidateSignatureFilePresenceAsync(
271272
{
272273
if (requiredSigner == currentUser)
273274
{
274-
return new PackageValidationResult(
275-
PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates,
276-
Strings.UploadPackage_PackageIsSignedButMissingCertificate_CurrentUserCanManageCertificates);
275+
return PackageValidationResult.Invalid(new PackageShouldNotBeSignedUserFixableValidationMessage());
277276
}
278277
else
279278
{
280-
return new PackageValidationResult(
281-
PackageValidationResultType.PackageShouldNotBeSigned,
279+
return PackageValidationResult.Invalid(
282280
string.Format(
283281
Strings.UploadPackage_PackageIsSignedButMissingCertificate_RequiredSigner,
284282
requiredSigner.Username));
@@ -295,14 +293,11 @@ private async Task<PackageValidationResult> ValidateSignatureFilePresenceAsync(
295293
// someone else for help.
296294
if (isCurrentUserAnOwner)
297295
{
298-
return new PackageValidationResult(
299-
PackageValidationResultType.PackageShouldNotBeSignedButCanManageCertificates,
300-
Strings.UploadPackage_PackageIsSignedButMissingCertificate_CurrentUserCanManageCertificates);
296+
return PackageValidationResult.Invalid(new PackageShouldNotBeSignedUserFixableValidationMessage());
301297
}
302298
else
303299
{
304-
return new PackageValidationResult(
305-
PackageValidationResultType.PackageShouldNotBeSigned,
300+
return PackageValidationResult.Invalid(
306301
string.Format(
307302
Strings.UploadPackage_PackageIsSignedButMissingCertificate_RequiredSigner,
308303
owner.Username));

‎src/NuGetGallery/Services/PackageValidationResult.cs‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,19 @@ namespace NuGetGallery
1212
/// </summary>
1313
public class PackageValidationResult
1414
{
15-
private static readonly IReadOnlyList<string> EmptyList = new string[0];
15+
private static readonly IReadOnlyList<IValidationMessage> EmptyList = new IValidationMessage[0];
1616

17-
public PackageValidationResult(PackageValidationResultType type, string message)
17+
public PackageValidationResult(PackageValidationResultType type, IValidationMessage message)
1818
: this(type, message, warnings: null)
1919
{
2020
}
2121

22-
public PackageValidationResult(PackageValidationResultType type, string message, IReadOnlyList<string> warnings)
22+
public PackageValidationResult(PackageValidationResultType type, string message)
23+
: this(type, new PlainTextOnlyValidationMessage(message))
24+
{
25+
}
26+
27+
public PackageValidationResult(PackageValidationResultType type, IValidationMessage message, IReadOnlyList<IValidationMessage> warnings)
2328
{
2429
if (type != PackageValidationResultType.Accepted && message == null)
2530
{
@@ -32,8 +37,8 @@ public PackageValidationResult(PackageValidationResultType type, string message,
3237
}
3338

3439
public PackageValidationResultType Type { get; }
35-
public string Message { get; }
36-
public IReadOnlyList<string> Warnings { get; }
40+
public IValidationMessage Message { get; }
41+
public IReadOnlyList<IValidationMessage> Warnings { get; }
3742

3843
public static PackageValidationResult Accepted()
3944
{
@@ -43,7 +48,7 @@ public static PackageValidationResult Accepted()
4348
warnings: null);
4449
}
4550

46-
public static PackageValidationResult AcceptedWithWarnings(IReadOnlyList<string> warnings)
51+
public static PackageValidationResult AcceptedWithWarnings(IReadOnlyList<IValidationMessage> warnings)
4752
{
4853
return new PackageValidationResult(
4954
PackageValidationResultType.Accepted,
@@ -52,6 +57,9 @@ public static PackageValidationResult AcceptedWithWarnings(IReadOnlyList<string>
5257
}
5358

5459
public static PackageValidationResult Invalid(string message)
60+
=> Invalid(new PlainTextOnlyValidationMessage(message));
61+
62+
public static PackageValidationResult Invalid(IValidationMessage message)
5563
{
5664
if (message == null)
5765
{

0 commit comments

Comments
 (0)