Fix bug with namespace reservation and snupkg push with older clients (#6768)

shishirx34 · web-flow · commit 24fb2a3a5c76 · 2018-12-18T16:46:05.000-08:00
diff --git a/src/NuGetGallery/Services/PackageService.cs b/src/NuGetGallery/Services/PackageService.cs
@@ -56,6 +56,11 @@ public async Task EnsureValid(PackageArchiveReader packageArchiveReader)
                     packageArchiveReader.GetNuspecReader(),
                     strict: true);
 
+                if (packageMetadata.IsSymbolsPackage())
+                {
+                    throw new InvalidPackageException(Strings.UploadPackage_SymbolsPackageNotAllowed);
+                }
+
                 PackageHelper.ValidateNuGetPackageMetadata(packageMetadata);
 
                 var supportedFrameworks = GetSupportedFrameworks(packageArchiveReader).Select(fn => fn.ToShortNameOrNull()).ToArray();
diff --git a/src/NuGetGallery/Services/ReservedNamespaceService.cs b/src/NuGetGallery/Services/ReservedNamespaceService.cs
@@ -15,7 +15,7 @@ namespace NuGetGallery
 {
     public class ReservedNamespaceService : IReservedNamespaceService
     {
-        private static readonly Regex NamespaceRegex = new Regex(@"^\w+([_.-]\w+)*[.]?$", RegexOptions.Compiled | RegexOptions.ExplicitCapture);
+        private static readonly Regex NamespaceRegex = new Regex(@"^\w+([_.-]\w+)*[.-]?$", RegexOptions.Compiled | RegexOptions.ExplicitCapture);
 
         public IEntitiesContext EntitiesContext { get; protected set; }
         public IEntityRepository<ReservedNamespace> ReservedNamespaceRepository { get; protected set; }
@@ -47,7 +47,14 @@ public async Task AddReservedNamespaceAsync(ReservedNamespace newNamespace)
                 throw new ArgumentNullException(nameof(newNamespace));
             }
 
-            ValidateNamespace(newNamespace.Value);
+            try
+            {
+                ValidateNamespace(newNamespace.Value);
+            }
+            catch (ArgumentException ex)
+            {
+                throw new InvalidOperationException(ex.Message, ex);
+            }
 
             var matchingReservedNamespaces = FindAllReservedNamespacesForPrefix(prefix: newNamespace.Value, getExactMatches: !newNamespace.IsPrefix);
             if (matchingReservedNamespaces.Any())
@@ -348,12 +355,12 @@ public static void ValidateNamespace(string value)
                 throw new ArgumentException(Strings.ReservedNamespace_InvalidNamespace);
             }
 
-            if (value.Length > NuGet.Services.Entities.Constants.MaxPackageIdLength)
+            if (value.Length > Constants.MaxPackageIdLength)
             {
                 throw new ArgumentException(string.Format(
                     CultureInfo.CurrentCulture,
                     Strings.ReservedNamespace_NamespaceExceedsLength,
-                    NuGet.Services.Entities.Constants.MaxPackageIdLength));
+                    Constants.MaxPackageIdLength));
             }
 
             if (!NamespaceRegex.IsMatch(value))
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
@@ -491,7 +491,7 @@ For more information, please contact '{2}'.</value>
     <value>This package will only be available to download with SemVer 2.0.0 compatible NuGet clients, such as Visual Studio 2017 (version 15.3) and above or NuGet client 4.3 and above. For more information, see https://go.microsoft.com/fwlink/?linkid=852248.</value>
   </data>
   <data name="ReservedNamespace_InvalidCharactersInNamespace" xml:space="preserve">
-    <value>The namespace '{0}' contains invalid characters. Examples of valid namespaces include 'MyNamespace' and 'MyNamespace.'.</value>
+    <value>The namespace '{0}' contains invalid characters. Examples of valid namespaces include 'MyNamespace', 'MyNamespace.' or 'MyNamespace-' etc.</value>
   </data>
   <data name="ReservedNamespace_InvalidNamespace" xml:space="preserve">
     <value>Invalid namespace specified</value>
@@ -1085,4 +1085,7 @@ The {1} Team</value>
     <value>The package metadata defines '{0}' as author more than once, which is not allowed by policy.</value>
     <comment>{0} is the list of duplicate package authors</comment>
   </data>
+  <data name="UploadPackage_SymbolsPackageNotAllowed" xml:space="preserve">
+    <value>Snupkg upload failed. Please use latest NuGet clients (v 4.9.0 or above) and a V3 endpoint to push Symbol packages. For details, refer https://docs.microsoft.com/nuget/create-packages/symbol-packages-snupkg</value>
+  </data>
 </root>
diff --git a/tests/NuGetGallery.Core.Facts/TestUtils/PackageServiceUtility.cs b/tests/NuGetGallery.Core.Facts/TestUtils/PackageServiceUtility.cs
@@ -190,5 +190,15 @@ public static MemoryStream CreateNuGetPackageStream(string id = "theId",
                 licenseFilename: licenseFilename,
                 licenseFileContents: licenseFileContents);
         }
+
+        public static PackageArchiveReader CreateArchiveReader(Stream stream)
+        {
+            if (stream == null)
+            {
+                stream = TestPackage.CreateTestPackageStream("theId", "1.0.42");
+            }
+
+            return new PackageArchiveReader(stream, leaveStreamOpen: true);
+        }
     }
 }
diff --git a/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs
@@ -2059,6 +2059,21 @@ public async Task WillThrowIfThePackageIsNull()
             }
         }
 
+        public class TheEnsureValidMethod
+        {
+            [Fact]
+            public async Task EnsureValidThrowsForSymbolsPackage()
+            {
+                // Arrange
+                var service = CreateService();
+                var packageStream = TestPackage.CreateTestSymbolPackageStream();
+                var packageArchiveReader = PackageServiceUtility.CreateArchiveReader(packageStream);
+
+                // Act and Assert
+                await Assert.ThrowsAsync<InvalidPackageException>(async () => await service.EnsureValid(packageArchiveReader));
+            }
+        }
+
         public class TheRemovePackageOwnerMethod
         {
             [Fact]
diff --git a/tests/NuGetGallery.Facts/Services/ReservedNamespaceServiceFacts.cs b/tests/NuGetGallery.Facts/Services/ReservedNamespaceServiceFacts.cs
@@ -99,7 +99,7 @@ public async Task EmptyOrNullNamespaceThrowsException(string value)
             {
                 var service = new TestableReservedNamespaceService();
                 var addNamespace = new ReservedNamespace(value, isSharedNamespace: false, isPrefix: true);
-                await Assert.ThrowsAsync<ArgumentException>(async () => await service.AddReservedNamespaceAsync(addNamespace));
+                await Assert.ThrowsAsync<InvalidOperationException>(async () => await service.AddReservedNamespaceAsync(addNamespace));
             }
 
             [Theory]
@@ -110,7 +110,7 @@ public async Task InvalidNamespaceThrowsException(string value)
             {
                 var service = new TestableReservedNamespaceService();
                 var addNamespace = new ReservedNamespace(value, isSharedNamespace: false, isPrefix: true);
-                await Assert.ThrowsAsync<ArgumentException>(async () => await service.AddReservedNamespaceAsync(addNamespace));
+                await Assert.ThrowsAsync<InvalidOperationException>(async () => await service.AddReservedNamespaceAsync(addNamespace));
             }
 
             [Fact]
@@ -969,6 +969,7 @@ public class ValidateNamespaceMethod
             [InlineData("Cont@ins$pecia|C#aracters")]
             [InlineData("Endswithperods..")]
             [InlineData("Multiple.Sequential..Periods.")]
+            [InlineData("Multiple-Sequential--hyphens")]
             public void InvalidNamespacesThrowsException(string value)
             {
                 Assert.Throws<ArgumentException>(() => ReservedNamespaceService.ValidateNamespace(value));
@@ -980,6 +981,9 @@ public void InvalidNamespacesThrowsException(string value)
             [InlineData("Name.Space.")]
             [InlineData("123_Name.space.")]
             [InlineData("123-Namespace.")]
+            [InlineData("123-Namespace-endswith-hyphen-")]
+            [InlineData("123_Namespace_endswith_Underscores_")]
+            [InlineData("Multiple_Sequential__Underscores")]
             public void ValidNamespacesDontThrowException(string value)
             {
                 var ex = Record.Exception(() => ReservedNamespaceService.ValidateNamespace(value));
diff --git a/tests/NuGetGallery.Facts/Services/SymbolPackageServiceFacts.cs b/tests/NuGetGallery.Facts/Services/SymbolPackageServiceFacts.cs
@@ -7,9 +7,9 @@
 using System.IO.Compression;
 using System.Threading.Tasks;
 using Moq;
-using NuGet.Packaging;
 using NuGet.Services.Entities;
 using NuGetGallery.Packaging;
+using NuGetGallery.TestUtils;
 using Xunit;
 using ClientPackageType = NuGet.Packaging.Core.PackageType;
 
@@ -39,16 +39,6 @@ private static ISymbolPackageService CreateService(
             return symbolPackageService.Object;
         }
 
-        private static PackageArchiveReader CreateArchiveReader(Stream stream)
-        {
-            if (stream == null)
-            {
-                stream = TestPackage.CreateTestPackageStream("theId", "1.0.42");
-            }
-
-            return new PackageArchiveReader(stream, leaveStreamOpen: true);
-        }
-
         private static List<ClientPackageType> CreateSymbolPackageTypesObject()
         {
             return new List<ClientPackageType>()
@@ -91,7 +81,7 @@ public async Task WillThrowForMissingSymbolsPackageType()
             {
                 var service = CreateService();
                 var invalidSymbolPackageStream = TestPackage.CreateTestPackageStream("theId", "1.0.42");
-                var packageArchiveReader = CreateArchiveReader(invalidSymbolPackageStream);
+                var packageArchiveReader = PackageServiceUtility.CreateArchiveReader(invalidSymbolPackageStream);
 
                 await Assert.ThrowsAsync<InvalidPackageException>(async () => await service.EnsureValidAsync(packageArchiveReader));
             }
@@ -105,7 +95,7 @@ public async Task WillThrowForIncorrectSymbolsPackageTypeVersion()
                     new ClientPackageType("SymbolsPackage", new Version("1.1"))
                 };
                 var invalidSymbolPackageStream = TestPackage.CreateTestPackageStream("theId", "1.0.42", packageTypes: packageTypes);
-                var packageArchiveReader = CreateArchiveReader(invalidSymbolPackageStream);
+                var packageArchiveReader = PackageServiceUtility.CreateArchiveReader(invalidSymbolPackageStream);
 
                 await Assert.ThrowsAsync<InvalidPackageException>(async () => await service.EnsureValidAsync(packageArchiveReader));
             }
@@ -118,7 +108,7 @@ public async Task WillThrowForMultiplePackageTypesInNuspec()
                 packageTypes.Add(new ClientPackageType("RandomPackageType", ClientPackageType.EmptyVersion));
 
                 var invalidSymbolPackageStream = TestPackage.CreateTestPackageStream("theId", "1.0.42", packageTypes: packageTypes);
-                var packageArchiveReader = CreateArchiveReader(invalidSymbolPackageStream);
+                var packageArchiveReader = PackageServiceUtility.CreateArchiveReader(invalidSymbolPackageStream);
 
                 await Assert.ThrowsAsync<InvalidPackageException>(async () => await service.EnsureValidAsync(packageArchiveReader));
             }
@@ -130,7 +120,7 @@ public async Task WillThrowForAuthorsMetadataInNuspec()
                 var packageTypes = CreateSymbolPackageTypesObject();
 
                 var invalidSymbolPackageStream = TestPackage.CreateTestPackageStream("theId", "1.0.42", authors: "Random authors", packageTypes: packageTypes);
-                var packageArchiveReader = CreateArchiveReader(invalidSymbolPackageStream);
+                var packageArchiveReader = PackageServiceUtility.CreateArchiveReader(invalidSymbolPackageStream);
 
                 await Assert.ThrowsAsync<InvalidDataException>(async () => await service.EnsureValidAsync(packageArchiveReader));
             }
@@ -142,7 +132,7 @@ public async Task WillThrowForOwnersMetadataInNuspec()
                 var packageTypes = CreateSymbolPackageTypesObject();
 
                 var invalidSymbolPackageStream = TestPackage.CreateTestPackageStream("theId", "1.0.42", owners: "Random owners", packageTypes: packageTypes);
-                var packageArchiveReader = CreateArchiveReader(invalidSymbolPackageStream);
+                var packageArchiveReader = PackageServiceUtility.CreateArchiveReader(invalidSymbolPackageStream);
 
                 await Assert.ThrowsAsync<InvalidDataException>(async () => await service.EnsureValidAsync(packageArchiveReader));
             }
@@ -158,7 +148,7 @@ public async Task WillThrowForInvalidFilesInSnupkg(string extension)
                 var action = CreatePopulatePackageAction(extension);
 
                 var invalidSymbolPackageStream = TestPackage.CreateTestSymbolPackageStream("theId", "1.0.42", populatePackage: action);
-                var packageArchiveReader = CreateArchiveReader(invalidSymbolPackageStream);
+                var packageArchiveReader = PackageServiceUtility.CreateArchiveReader(invalidSymbolPackageStream);
 
                 await Assert.ThrowsAsync<InvalidDataException>(async () => await service.EnsureValidAsync(packageArchiveReader));
             }
@@ -175,7 +165,7 @@ public async Task WillNotThrowForValidSnupkgFile(string extension)
                 var action = CreatePopulatePackageAction(extension);
 
                 var validSymbolPackageStream = TestPackage.CreateTestSymbolPackageStream("theId", "1.0.42", populatePackage: action);
-                var packageArchiveReader = CreateArchiveReader(validSymbolPackageStream);
+                var packageArchiveReader = PackageServiceUtility.CreateArchiveReader(validSymbolPackageStream);
 
                 await service.EnsureValidAsync(packageArchiveReader);
             }

Original file line number	Diff line number	Diff line change
`@@ -15,7 +15,7 @@ namespace NuGetGallery`
`15`	`15`	`{`
`16`	`16`	`public class ReservedNamespaceService : IReservedNamespaceService`
`17`	`17`	`{`
`18`		`- private static readonly Regex NamespaceRegex = new Regex(@"^\w+([_.-]\w+)*[.]?$", RegexOptions.Compiled \| RegexOptions.ExplicitCapture);`
	`18`	`+ private static readonly Regex NamespaceRegex = new Regex(@"^\w+([_.-]\w+)*[.-]?$", RegexOptions.Compiled \| RegexOptions.ExplicitCapture);`
`19`	`19`
`20`	`20`	`public IEntitiesContext EntitiesContext { get; protected set; }`
`21`	`21`	`public IEntityRepository<ReservedNamespace> ReservedNamespaceRepository { get; protected set; }`
`@@ -47,7 +47,14 @@ public async Task AddReservedNamespaceAsync(ReservedNamespace newNamespace)`
`47`	`47`	`throw new ArgumentNullException(nameof(newNamespace));`
`48`	`48`	`}`
`49`	`49`
`50`		`- ValidateNamespace(newNamespace.Value);`
	`50`	`+ try`
	`51`	`+ {`
	`52`	`+ ValidateNamespace(newNamespace.Value);`
	`53`	`+ }`
	`54`	`+ catch (ArgumentException ex)`
	`55`	`+ {`
	`56`	`+ throw new InvalidOperationException(ex.Message, ex);`
	`57`	`+ }`
`51`	`58`
`52`	`59`	`var matchingReservedNamespaces = FindAllReservedNamespacesForPrefix(prefix: newNamespace.Value, getExactMatches: !newNamespace.IsPrefix);`
`53`	`60`	`if (matchingReservedNamespaces.Any())`
`@@ -348,12 +355,12 @@ public static void ValidateNamespace(string value)`
`348`	`355`	`throw new ArgumentException(Strings.ReservedNamespace_InvalidNamespace);`
`349`	`356`	`}`
`350`	`357`
`351`		`- if (value.Length > NuGet.Services.Entities.Constants.MaxPackageIdLength)`
	`358`	`+ if (value.Length > Constants.MaxPackageIdLength)`
`352`	`359`	`{`
`353`	`360`	`throw new ArgumentException(string.Format(`
`354`	`361`	`CultureInfo.CurrentCulture,`
`355`	`362`	`Strings.ReservedNamespace_NamespaceExceedsLength,`
`356`		`- NuGet.Services.Entities.Constants.MaxPackageIdLength));`
	`363`	`+ Constants.MaxPackageIdLength));`
`357`	`364`	`}`
`358`	`365`
`359`	`366`	`if (!NamespaceRegex.IsMatch(value))`
Original file line number	Diff line number	Diff line change
`@@ -190,5 +190,15 @@ public static MemoryStream CreateNuGetPackageStream(string id = "theId",`
`190`	`190`	`licenseFilename: licenseFilename,`
`191`	`191`	`licenseFileContents: licenseFileContents);`
`192`	`192`	`}`
	`193`	`+`
	`194`	`+ public static PackageArchiveReader CreateArchiveReader(Stream stream)`
	`195`	`+ {`
	`196`	`+ if (stream == null)`
	`197`	`+ {`
	`198`	`+ stream = TestPackage.CreateTestPackageStream("theId", "1.0.42");`
	`199`	`+ }`
	`200`	`+`
	`201`	`+ return new PackageArchiveReader(stream, leaveStreamOpen: true);`
	`202`	`+ }`
`193`	`203`	`}`
`194`	`204`	`}`
Original file line number	Diff line number	Diff line change
`@@ -99,7 +99,7 @@ public async Task EmptyOrNullNamespaceThrowsException(string value)`
`99`	`99`	`{`
`100`	`100`	`var service = new TestableReservedNamespaceService();`
`101`	`101`	`var addNamespace = new ReservedNamespace(value, isSharedNamespace: false, isPrefix: true);`
`102`		`- await Assert.ThrowsAsync<ArgumentException>(async () => await service.AddReservedNamespaceAsync(addNamespace));`
	`102`	`+ await Assert.ThrowsAsync<InvalidOperationException>(async () => await service.AddReservedNamespaceAsync(addNamespace));`
`103`	`103`	`}`
`104`	`104`
`105`	`105`	`[Theory]`
`@@ -110,7 +110,7 @@ public async Task InvalidNamespaceThrowsException(string value)`
`110`	`110`	`{`
`111`	`111`	`var service = new TestableReservedNamespaceService();`
`112`	`112`	`var addNamespace = new ReservedNamespace(value, isSharedNamespace: false, isPrefix: true);`
`113`		`- await Assert.ThrowsAsync<ArgumentException>(async () => await service.AddReservedNamespaceAsync(addNamespace));`
	`113`	`+ await Assert.ThrowsAsync<InvalidOperationException>(async () => await service.AddReservedNamespaceAsync(addNamespace));`
`114`	`114`	`}`
`115`	`115`
`116`	`116`	`[Fact]`
`@@ -969,6 +969,7 @@ public class ValidateNamespaceMethod`
`969`	`969`	`[InlineData("Cont@ins$pecia\|C#aracters")]`
`970`	`970`	`[InlineData("Endswithperods..")]`
`971`	`971`	`[InlineData("Multiple.Sequential..Periods.")]`
	`972`	`+ [InlineData("Multiple-Sequential--hyphens")]`
`972`	`973`	`public void InvalidNamespacesThrowsException(string value)`
`973`	`974`	`{`
`974`	`975`	`Assert.Throws<ArgumentException>(() => ReservedNamespaceService.ValidateNamespace(value));`
`@@ -980,6 +981,9 @@ public void InvalidNamespacesThrowsException(string value)`
`980`	`981`	`[InlineData("Name.Space.")]`
`981`	`982`	`[InlineData("123_Name.space.")]`
`982`	`983`	`[InlineData("123-Namespace.")]`
	`984`	`+ [InlineData("123-Namespace-endswith-hyphen-")]`
	`985`	`+ [InlineData("123_Namespace_endswith_Underscores_")]`
	`986`	`+ [InlineData("Multiple_Sequential__Underscores")]`
`983`	`987`	`public void ValidNamespacesDontThrowException(string value)`
`984`	`988`	`{`
`985`	`989`	`var ex = Record.Exception(() => ReservedNamespaceService.ValidateNamespace(value));`