Putting self-contained icon upload behind feature flag (#7116)

agr · web-flow · commit e7cc39debc92 · 2019-05-02T17:13:54.000-07:00
* Feature flagged icon upload. Updated tests to accomodate.
Fixed the misreported nuspec file size test.
diff --git a/src/NuGetGallery/Controllers/ApiController.cs b/src/NuGetGallery/Controllers/ApiController.cs
@@ -620,7 +620,7 @@ await PackageDeleteService.HardDeletePackagesAsync(
                             }
 
                             // Perform all the validations we can before adding the package to the entity context.
-                            var beforeValidationResult = await PackageUploadService.ValidateBeforeGeneratePackageAsync(packageToPush, packageMetadata);
+                            var beforeValidationResult = await PackageUploadService.ValidateBeforeGeneratePackageAsync(packageToPush, packageMetadata, currentUser);
                             var beforeValidationActionResult = GetActionResultOrNull(beforeValidationResult);
                             if (beforeValidationActionResult != null)
                             {
diff --git a/src/NuGetGallery/Controllers/PackagesController.cs b/src/NuGetGallery/Controllers/PackagesController.cs
@@ -274,7 +274,7 @@ private async Task<ActionResult> UploadPackageInternal(SubmitPackageRequest mode
             PackageMetadata packageMetadata,
             User currentUser)
         {
-            var validationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(packageArchiveReader, packageMetadata);
+            var validationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(packageArchiveReader, packageMetadata, currentUser);
             var validationErrorMessage = GetErrorMessageOrNull(validationResult);
             if (validationErrorMessage != null)
             {
@@ -644,7 +644,7 @@ private async Task<PackageContentData> ValidateAndProcessPackageContents(User cu
 
                 if (!isSymbolsPackageUpload)
                 {
-                    var validationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(packageArchiveReader, packageMetadata);
+                    var validationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(packageArchiveReader, packageMetadata, currentUser);
                     var validationJsonResult = GetJsonResultOrNull(validationResult);
                     if (validationJsonResult != null)
                     {
@@ -2325,7 +2325,7 @@ protected virtual async Task<JsonResult> VerifyPackageInternal(
                 }
 
                 // Perform all the validations we can before adding the package to the entity context.
-                var beforeValidationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(packageArchiveReader, packageMetadata);
+                var beforeValidationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(packageArchiveReader, packageMetadata, currentUser);
                 var beforeValidationJsonResult = GetJsonResultOrNull(beforeValidationResult);
                 if (beforeValidationJsonResult != null)
                 {
diff --git a/src/NuGetGallery/Services/FeatureFlagService.cs b/src/NuGetGallery/Services/FeatureFlagService.cs
@@ -15,6 +15,7 @@ public class FeatureFlagService : IFeatureFlagService
         // Typosquatting detection
         private const string TyposquattingFeatureName = GalleryPrefix + "Typosquatting";
         private const string TyposquattingFlightName = GalleryPrefix + "TyposquattingFlight";
+        private const string EmbeddedIconFlightName = GalleryPrefix + "EmbeddedIcons";
 
         private const string PackagesAtomFeedFeatureName = GalleryPrefix + "PackagesAtomFeed";
 
@@ -48,6 +49,11 @@ public bool IsManageDeprecationEnabled(User user)
             return _client.IsEnabled(ManageDeprecationFeatureName, user, defaultValue: false);
         }
 
+        public bool AreEmbeddedIconsEnabled(User user)
+        {
+            return _client.IsEnabled(EmbeddedIconFlightName, user, defaultValue: false);
+        }
+
         private bool IsEnabled(string flight, User user, bool defaultValue)
         {
             return _client.IsEnabled(flight, user, defaultValue);
diff --git a/src/NuGetGallery/Services/IFeatureFlagService.cs b/src/NuGetGallery/Services/IFeatureFlagService.cs
@@ -42,5 +42,10 @@ public interface IFeatureFlagService
         /// </summary>
         /// <returns></returns>
         bool IsSearchCircuitBreakerEnabled();
+
+        /// <summary>
+        /// Whether the user is allowed to publish packages with an embedded icon.
+        /// </summary>
+        bool AreEmbeddedIconsEnabled(User user);
     }
 }
diff --git a/src/NuGetGallery/Services/IPackageUploadService.cs b/src/NuGetGallery/Services/IPackageUploadService.cs
@@ -18,8 +18,13 @@ public interface IPackageUploadService
         /// UI package upload.
         /// </summary>
         /// <param name="nuGetPackage">The package archive reader.</param>
+        /// <param name="packageMetadata">The package metadata.</param>
+        /// <param name="currentUser">The user who pushed the package.</param>
         /// <returns>The package validation result.</returns>
-        Task<PackageValidationResult> ValidateBeforeGeneratePackageAsync(PackageArchiveReader nuGetPackage, PackageMetadata packageMetadata);
+        Task<PackageValidationResult> ValidateBeforeGeneratePackageAsync(
+            PackageArchiveReader nuGetPackage,
+            PackageMetadata packageMetadata,
+            User currentUser);
 
         Task<Package> GeneratePackageAsync(
             string id,
diff --git a/src/NuGetGallery/Services/PackageUploadService.cs b/src/NuGetGallery/Services/PackageUploadService.cs
@@ -47,6 +47,7 @@ public class PackageUploadService : IPackageUploadService
         private const long MaxAllowedLicenseLengthForUploading = 1024 * 1024; // 1 MB
         private const int MaxAllowedLicenseNodeValueLength = 500;
         private const string LicenseNodeName = "license";
+        private const string IconNodeName = "icon";
         private const string AllowedLicenseVersion = "1.0.0";
         private const string Unlicensed = "UNLICENSED";
 
@@ -60,6 +61,7 @@ public class PackageUploadService : IPackageUploadService
         private readonly ITelemetryService _telemetryService;
         private readonly ICoreLicenseFileService _coreLicenseFileService;
         private readonly IDiagnosticsSource _trace;
+        private readonly IFeatureFlagService _featureFlagService;
 
         public PackageUploadService(
             IPackageService packageService,
@@ -71,7 +73,8 @@ public PackageUploadService(
             ITyposquattingService typosquattingService,
             ITelemetryService telemetryService,
             ICoreLicenseFileService coreLicenseFileService,
-            IDiagnosticsService diagnosticsService)
+            IDiagnosticsService diagnosticsService,
+            IFeatureFlagService featureFlagService)
         {
             _packageService = packageService ?? throw new ArgumentNullException(nameof(packageService));
             _packageFileService = packageFileService ?? throw new ArgumentNullException(nameof(packageFileService));
@@ -87,9 +90,13 @@ public PackageUploadService(
                 throw new ArgumentNullException(nameof(diagnosticsService));
             }
             _trace = diagnosticsService.GetSource(nameof(PackageUploadService));
+            _featureFlagService = featureFlagService ?? throw new ArgumentNullException(nameof(featureFlagService));
         }
 
-        public async Task<PackageValidationResult> ValidateBeforeGeneratePackageAsync(PackageArchiveReader nuGetPackage, PackageMetadata packageMetadata)
+        public async Task<PackageValidationResult> ValidateBeforeGeneratePackageAsync(
+            PackageArchiveReader nuGetPackage,
+            PackageMetadata packageMetadata,
+            User currentUser)
         {
             var warnings = new List<IValidationMessage>();
 
@@ -125,7 +132,7 @@ public async Task<PackageValidationResult> ValidateBeforeGeneratePackageAsync(Pa
                 return result;
             }
 
-            result = await CheckLicenseMetadataAsync(nuGetPackage, warnings);
+            result = await CheckLicenseMetadataAsync(nuGetPackage, warnings, currentUser);
             if (result != null)
             {
                 _telemetryService.TrackLicenseValidationFailure();
@@ -135,13 +142,9 @@ public async Task<PackageValidationResult> ValidateBeforeGeneratePackageAsync(Pa
             return PackageValidationResult.AcceptedWithWarnings(warnings);
         }
 
-        private async Task<PackageValidationResult> CheckLicenseMetadataAsync(PackageArchiveReader nuGetPackage, List<IValidationMessage> warnings)
+        private async Task<PackageValidationResult> CheckLicenseMetadataAsync(PackageArchiveReader nuGetPackage, List<IValidationMessage> warnings, User user)
         {
-            LicenseCheckingNuspecReader nuspecReader = null;
-            using (var nuspec = nuGetPackage.GetNuspec())
-            {
-                nuspecReader = new LicenseCheckingNuspecReader(nuspec);
-            }
+            var nuspecReader = GetNuspecReader(nuGetPackage);
 
             var licenseElement = nuspecReader.LicenseElement;
 
@@ -184,6 +187,12 @@ private async Task<PackageValidationResult> CheckLicenseMetadataAsync(PackageArc
             var licenseMetadata = nuspecReader.GetLicenseMetadata();
             var licenseDeprecationUrl = GetExpectedLicenseUrl(licenseMetadata);
 
+            // TODO: move out when full blown validation is implemented https://github.com/nuget/nugetgallery/issues/7063
+            if (nuspecReader.IconExists && !_featureFlagService.AreEmbeddedIconsEnabled(user))
+            {
+                return PackageValidationResult.Invalid(Strings.UploadPackage_EmbeddedIconNotAccepted);
+            }
+
             if (licenseMetadata == null)
             {
                 if (string.IsNullOrWhiteSpace(licenseUrl))
@@ -329,6 +338,14 @@ private async Task<PackageValidationResult> CheckLicenseMetadataAsync(PackageArc
             return null;
         }
 
+        private static UserContentEnabledNuspecReader GetNuspecReader(PackageArchiveReader nuGetPackage)
+        {
+            using (var nuspec = nuGetPackage.GetNuspec())
+            {
+                return new UserContentEnabledNuspecReader(nuspec);
+            }
+        }
+
         private bool IsMalformedDeprecationUrl(string licenseUrl)
         {
             // nuget.exe 4.9.0 and its dotnet and msbuild counterparts encode spaces as "+"
@@ -435,14 +452,15 @@ private static string GetLicenseType(XElement licenseElement)
             => licenseElement
                 .GetOptionalAttributeValue("type");
 
-        private class LicenseCheckingNuspecReader : NuspecReader
+        private class UserContentEnabledNuspecReader : NuspecReader
         {
-            public LicenseCheckingNuspecReader(Stream stream)
+            public UserContentEnabledNuspecReader(Stream stream)
                 : base(stream)
             {
             }
 
             public XElement LicenseElement => MetadataNode.Element(MetadataNode.Name.Namespace + LicenseNodeName);
+            public bool IconExists => MetadataNode.Element(MetadataNode.Name.Namespace + IconNodeName) != null;
         }
 
         private async Task<PackageValidationResult> CheckPackageEntryCountAsync(
diff --git a/src/NuGetGallery/Strings.Designer.cs b/src/NuGetGallery/Strings.Designer.cs
diff --git a/src/NuGetGallery/Strings.resx b/src/NuGetGallery/Strings.resx
@@ -1118,4 +1118,7 @@ The {1} Team</value>
   <data name="SymbolsPackage_NoSymbols" xml:space="preserve">
     <value>The package does not contain any symbol (.pdb) files.</value>
   </data>
+  <data name="UploadPackage_EmbeddedIconNotAccepted" xml:space="preserve">
+    <value>The &lt;icon&gt; element is not currently supported.</value>
+  </data>
 </root>
diff --git a/tests/NuGetGallery.Core.Facts/TestUtils/PackageServiceUtility.cs b/tests/NuGetGallery.Core.Facts/TestUtils/PackageServiceUtility.cs
@@ -126,7 +126,9 @@ public static MemoryStream CreateNuGetPackageStream(string id = "theId",
             Func<string> getCustomNuspecNodes = null,
             string licenseExpression = null,
             string licenseFilename = null,
-            byte[] licenseFileContents = null)
+            byte[] licenseFileContents = null,
+            string iconFilename = null,
+            byte[] iconFileBinaryContents = null)
         {
             if (packageDependencyGroups == null)
             {
@@ -188,7 +190,9 @@ public static MemoryStream CreateNuGetPackageStream(string id = "theId",
                 getCustomNuspecNodes: getCustomNuspecNodes,
                 licenseExpression: licenseExpression,
                 licenseFilename: licenseFilename,
-                licenseFileContents: licenseFileContents);
+                licenseFileContents: licenseFileContents,
+                iconFilename: iconFilename,
+                iconFileContents: iconFileBinaryContents);
         }
 
         public static PackageArchiveReader CreateArchiveReader(Stream stream)
diff --git a/tests/NuGetGallery.Core.Facts/TestUtils/TestPackage.cs b/tests/NuGetGallery.Core.Facts/TestUtils/TestPackage.cs
@@ -41,7 +41,8 @@ public static void WriteNuspec(
             RepositoryMetadata repositoryMetadata = null,
             Func<string> getCustomNodes = null,
             string licenseExpression = null,
-            string licenseFilename = null)
+            string licenseFilename = null,
+            string iconFilename = null)
         {
             var fullNuspec = (@"<?xml version=""1.0""?>
                 <package xmlns=""http://schemas.microsoft.com/packaging/2011/08/nuspec.xsd"">
@@ -60,6 +61,7 @@ public static void WriteNuspec(
                         <releaseNotes>" + (releaseNotes ?? string.Empty) + @"</releaseNotes>
                         <licenseUrl>" + (licenseUrl?.AbsoluteUri ?? string.Empty) + @"</licenseUrl>
                         " + WriteLicense(licenseExpression, licenseFilename) + @"
+                        " + WriteIcon(iconFilename) + @"
                         <projectUrl>" + (projectUrl?.ToString() ?? string.Empty) + @"</projectUrl>
                         <iconUrl>" + (iconUrl?.ToString() ?? string.Empty) + @"</iconUrl>
                         <packageTypes>" + WritePackageTypes(packageTypes) + @"</packageTypes>
@@ -102,6 +104,16 @@ private static string WriteLicense(string licenseExpression, string licenseFilen
             return stringBuilder.ToString();
         }
 
+        private static string WriteIcon(string iconFilename)
+        {
+            if (iconFilename != null)
+            {
+                return $"<icon>{iconFilename}</icon>";
+            }
+
+            return string.Empty;
+        }
+
         private static string WriteRepositoryMetadata(RepositoryMetadata repositoryMetadata)
         {
             return repositoryMetadata == null
@@ -198,7 +210,9 @@ public static MemoryStream CreateTestPackageStream(
             Func<string> getCustomNuspecNodes = null,
             string licenseExpression = null,
             string licenseFilename = null,
-            byte[] licenseFileContents = null)
+            byte[] licenseFileContents = null,
+            string iconFilename = null,
+            byte[] iconFileContents = null)
         {
             return CreateTestPackageStream(packageArchive =>
             {
@@ -208,19 +222,11 @@ public static MemoryStream CreateTestPackageStream(
                     WriteNuspec(stream, true, id, version, title, summary, authors, owners, description, tags, language,
                         copyright, releaseNotes, minClientVersion, licenseUrl, projectUrl, iconUrl,
                         requireLicenseAcceptance, packageDependencyGroups, packageTypes, isSymbolPackage, repositoryMetadata,
-                        getCustomNuspecNodes, licenseExpression, licenseFilename);
+                        getCustomNuspecNodes, licenseExpression, licenseFilename, iconFilename);
                 }
 
-                if (licenseFileContents != null && licenseFilename != null)
-                {
-                    // enforce directory separators the same way as the client (see PackageArchiveReader.GetStream)
-                    licenseFilename = PathUtility.StripLeadingDirectorySeparators(licenseFilename);
-                    var licenseEntry = packageArchive.CreateEntry(licenseFilename, CompressionLevel.Fastest);
-                    using (var licenseStream = licenseEntry.Open())
-                    {
-                        licenseStream.Write(licenseFileContents, 0, licenseFileContents.Length);
-                    }
-                }
+                licenseFilename = AddBinaryFile(packageArchive, licenseFilename, licenseFileContents);
+                iconFilename = AddBinaryFile(packageArchive, iconFilename, iconFileContents);
 
                 if (populatePackage != null)
                 {
@@ -229,6 +235,22 @@ public static MemoryStream CreateTestPackageStream(
             }, desiredTotalEntryCount);
         }
 
+        private static string AddBinaryFile(ZipArchive archive, string filename, byte[] fileContents)
+        {
+            if (fileContents != null && filename != null)
+            {
+                // enforce directory separators the same way as the client (see PackageArchiveReader.GetStream)
+                filename = PathUtility.StripLeadingDirectorySeparators(filename);
+                var fileEntry = archive.CreateEntry(filename, CompressionLevel.Fastest);
+                using (var fileStream = fileEntry.Open())
+                {
+                    fileStream.Write(fileContents, 0, fileContents.Length);
+                }
+            }
+
+            return filename;
+        }
+
         public static MemoryStream CreateTestSymbolPackageStream(string id = "theId", string version = "1.0.42", Action<ZipArchive> populatePackage = null)
         {
             var packageTypes = new List<ClientPackageType>();
diff --git a/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs
@@ -113,7 +113,7 @@ public TestableApiController(
 
             MockPackageUploadService
                 .Setup(x => x.ValidateBeforeGeneratePackageAsync(
-                    It.IsAny<PackageArchiveReader>(), It.IsAny<PackageMetadata>()))
+                    It.IsAny<PackageArchiveReader>(), It.IsAny<PackageMetadata>(), It.IsAny<User>()))
                 .ReturnsAsync(PackageValidationResult.Accepted());
 
             MockPackageUploadService.Setup(x => x.GeneratePackageAsync(It.IsAny<string>(), It.IsAny<PackageArchiveReader>(), It.IsAny<PackageStreamMetadata>(), It.IsAny<User>(), It.IsAny<User>()))
@@ -905,7 +905,7 @@ public async Task WillReturnValidationWarnings()
                 controller
                     .MockPackageUploadService
                     .Setup(x => x.ValidateBeforeGeneratePackageAsync(
-                        It.IsAny<PackageArchiveReader>(), It.IsAny<PackageMetadata>()))
+                        It.IsAny<PackageArchiveReader>(), It.IsAny<PackageMetadata>(), It.IsAny<User>()))
                     .ReturnsAsync(PackageValidationResult.AcceptedWithWarnings(new[] { new PlainTextOnlyValidationMessage(messageA) }));
                 controller
                     .MockPackageUploadService
@@ -974,14 +974,14 @@ public async Task DoesNotThrowForAnyPackageValidationResultType(PackageValidatio
                 controller.SetupPackageFromInputStream(nuGetPackage);
                 controller.MockPackageUploadService
                     .Setup(x => x.ValidateBeforeGeneratePackageAsync(
-                        It.IsAny<PackageArchiveReader>(), It.IsAny<PackageMetadata>()))
+                        It.IsAny<PackageArchiveReader>(), It.IsAny<PackageMetadata>(), It.IsAny<User>()))
                     .ReturnsAsync(new PackageValidationResult(type, string.Empty));
 
                 await controller.CreatePackagePut();
 
                 controller.MockPackageUploadService.Verify(
                     x => x.ValidateBeforeGeneratePackageAsync(
-                        It.IsAny<PackageArchiveReader>(), It.IsAny<PackageMetadata>()),
+                        It.IsAny<PackageArchiveReader>(), It.IsAny<PackageMetadata>(), It.IsAny<User>()),
                     Times.Once);
             }
 
diff --git a/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs
diff --git a/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj b/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj
diff --git a/tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs
diff --git a/tests/NuGetGallery.Facts/TestData/theId.nuspec b/tests/NuGetGallery.Facts/TestData/theId.nuspec

Original file line number	Diff line number	Diff line change
`@@ -620,7 +620,7 @@ await PackageDeleteService.HardDeletePackagesAsync(`
`620`	`620`	`}`
`621`	`621`
`622`	`622`	`// Perform all the validations we can before adding the package to the entity context.`
`623`		`- var beforeValidationResult = await PackageUploadService.ValidateBeforeGeneratePackageAsync(packageToPush, packageMetadata);`
	`623`	`+ var beforeValidationResult = await PackageUploadService.ValidateBeforeGeneratePackageAsync(packageToPush, packageMetadata, currentUser);`
`624`	`624`	`var beforeValidationActionResult = GetActionResultOrNull(beforeValidationResult);`
`625`	`625`	`if (beforeValidationActionResult != null)`
`626`	`626`	`{`
Original file line number	Diff line number	Diff line change
`@@ -274,7 +274,7 @@ private async Task<ActionResult> UploadPackageInternal(SubmitPackageRequest mode`
`274`	`274`	`PackageMetadata packageMetadata,`
`275`	`275`	`User currentUser)`
`276`	`276`	`{`
`277`		`- var validationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(packageArchiveReader, packageMetadata);`
	`277`	`+ var validationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(packageArchiveReader, packageMetadata, currentUser);`
`278`	`278`	`var validationErrorMessage = GetErrorMessageOrNull(validationResult);`
`279`	`279`	`if (validationErrorMessage != null)`
`280`	`280`	`{`
`@@ -644,7 +644,7 @@ private async Task<PackageContentData> ValidateAndProcessPackageContents(User cu`
`644`	`644`
`645`	`645`	`if (!isSymbolsPackageUpload)`
`646`	`646`	`{`
`647`		`- var validationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(packageArchiveReader, packageMetadata);`
	`647`	`+ var validationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(packageArchiveReader, packageMetadata, currentUser);`
`648`	`648`	`var validationJsonResult = GetJsonResultOrNull(validationResult);`
`649`	`649`	`if (validationJsonResult != null)`
`650`	`650`	`{`
`@@ -2325,7 +2325,7 @@ protected virtual async Task<JsonResult> VerifyPackageInternal(`
`2325`	`2325`	`}`
`2326`	`2326`
`2327`	`2327`	`// Perform all the validations we can before adding the package to the entity context.`
`2328`		`- var beforeValidationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(packageArchiveReader, packageMetadata);`
	`2328`	`+ var beforeValidationResult = await _packageUploadService.ValidateBeforeGeneratePackageAsync(packageArchiveReader, packageMetadata, currentUser);`
`2329`	`2329`	`var beforeValidationJsonResult = GetJsonResultOrNull(beforeValidationResult);`
`2330`	`2330`	`if (beforeValidationJsonResult != null)`
`2331`	`2331`	`{`
Original file line number	Diff line number	Diff line change
`@@ -42,5 +42,10 @@ public interface IFeatureFlagService`
`42`	`42`	`/// </summary>`
`43`	`43`	`/// <returns></returns>`
`44`	`44`	`bool IsSearchCircuitBreakerEnabled();`
	`45`	`+`
	`46`	`+ /// <summary>`
	`47`	`+ /// Whether the user is allowed to publish packages with an embedded icon.`
	`48`	`+ /// </summary>`
	`49`	`+ bool AreEmbeddedIconsEnabled(User user);`
`45`	`50`	`}`
`46`	`51`	`}`