Add specific error message for reservations with no owner

joelverhagen · joelverhagen · commit daf417280827 · 2021-02-09T16:37:34.000-08:00
diff --git a/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs b/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs
@@ -256,6 +256,11 @@ public void TrackPackagePushNamespaceConflictEvent(string packageId, string pack
             throw new NotImplementedException();
         }
 
+        public void TrackPackagePushOwnerlessNamespaceConflictEvent(string packageId, string packageVersion, User user, IIdentity identity)
+        {
+            throw new NotImplementedException();
+        }
+
         public void TrackPackageReadMeChangeEvent(Package package, string readMeSourceType, PackageEditReadMeState readMeState)
         {
             throw new NotImplementedException();
diff --git a/src/NuGetGallery.Services/Permissions/ActionRequiringReservedNamespacePermissions.cs b/src/NuGetGallery.Services/Permissions/ActionRequiringReservedNamespacePermissions.cs
@@ -57,9 +57,21 @@ protected override PermissionsCheckResult CheckPermissionsForEntity(User account
                 return PermissionsCheckResult.Allowed;
             }
 
+            var hasAnyOwners = reservedNamespaces.Any(rn => rn.Owners.Any());
+
             // Permissions on only a single namespace are required to perform the action.
-            return reservedNamespaces.Any(rn => PermissionsHelpers.IsRequirementSatisfied(ReservedNamespacePermissionsRequirement, account, rn)) ?
-                PermissionsCheckResult.Allowed : PermissionsCheckResult.ReservedNamespaceFailure;
+            if (reservedNamespaces.Any(rn => PermissionsHelpers.IsRequirementSatisfied(ReservedNamespacePermissionsRequirement, account, rn)))
+            {
+                return PermissionsCheckResult.Allowed;
+            }
+            else if (hasAnyOwners)
+            {
+                return PermissionsCheckResult.ReservedNamespaceFailure;
+            }
+            else
+            {
+                return PermissionsCheckResult.OwnerlessReservedNamespaceFailure;
+            }
         }
 
         public PermissionsCheckResult CheckPermissionsOnBehalfOfAnyAccount(User currentUser, ActionOnNewPackageContext newPackageContext)
diff --git a/src/NuGetGallery.Services/Permissions/PermissionsCheckResult.cs b/src/NuGetGallery.Services/Permissions/PermissionsCheckResult.cs
@@ -31,6 +31,12 @@ public enum PermissionsCheckResult
         /// <summary>
         /// The current user does not have permissions to perform the action on the <see cref="ReservedNamespace"/> on behalf of another <see cref="User"/>.
         /// </summary>
-        ReservedNamespaceFailure
+        ReservedNamespaceFailure,
+
+        /// <summary>
+        /// The current user does not have permissions to perform the action on the <see cref="ReservedNamespace"/> on behalf of another <see cref="User"/>
+        /// but none of the namespaces currently have owners.
+        /// </summary>
+        OwnerlessReservedNamespaceFailure,
     }
 }
diff --git a/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs b/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs
@@ -61,6 +61,8 @@ void TrackPackageDeprecate(
 
         void TrackPackagePushNamespaceConflictEvent(string packageId, string packageVersion, User user, IIdentity identity);
 
+        void TrackPackagePushOwnerlessNamespaceConflictEvent(string packageId, string packageVersion, User user, IIdentity identity);
+
         void TrackVerifyPackageKeyEvent(string packageId, string packageVersion, User user, IIdentity identity, int statusCode);
 
         void TrackNewUserRegistrationEvent(User user, Credential identity);
diff --git a/src/NuGetGallery.Services/Telemetry/TelemetryService.cs b/src/NuGetGallery.Services/Telemetry/TelemetryService.cs
@@ -28,6 +28,7 @@ public class Events
             public const string VerifyPackageKey = "VerifyPackageKey";
             public const string PackageReadMeChanged = "PackageReadMeChanged";
             public const string PackagePushNamespaceConflict = "PackagePushNamespaceConflict";
+            public const string PackagePushOwnerlessNamespaceConflict = "PackagePushOwnerlessNamespaceConflict";
             public const string NewUserRegistration = "NewUserRegistration";
             public const string CredentialAdded = "CredentialAdded";
             public const string CredentialUsed = "CredentialUsed";
@@ -364,6 +365,11 @@ public void TrackPackagePushNamespaceConflictEvent(string packageId, string pack
             TrackMetricForPackage(Events.PackagePushNamespaceConflict, packageId, packageVersion, user, identity);
         }
 
+        public void TrackPackagePushOwnerlessNamespaceConflictEvent(string packageId, string packageVersion, User user, IIdentity identity)
+        {
+            TrackMetricForPackage(Events.PackagePushOwnerlessNamespaceConflict, packageId, packageVersion, user, identity);
+        }
+
         public void TrackCreatePackageVerificationKeyEvent(string packageId, string packageVersion, User user, IIdentity identity)
         {
             TrackMetricForPackage(Events.CreatePackageVerificationKey, packageId, packageVersion, user, identity);
diff --git a/src/NuGetGallery/Controllers/ApiController.cs b/src/NuGetGallery/Controllers/ApiController.cs
@@ -1195,6 +1195,11 @@ private HttpStatusCodeWithBodyResult GetHttpResultFromFailedApiScopeEvaluationHe
                 TelemetryService.TrackPackagePushNamespaceConflictEvent(id, versionString, GetCurrentUser(), User.Identity);
                 return new HttpStatusCodeWithBodyResult(HttpStatusCode.Conflict, Strings.UploadPackage_IdNamespaceConflict);
             }
+            else if (result.PermissionsCheckResult == PermissionsCheckResult.OwnerlessReservedNamespaceFailure)
+            {
+                TelemetryService.TrackPackagePushOwnerlessNamespaceConflictEvent(id, versionString, GetCurrentUser(), User.Identity);
+                return new HttpStatusCodeWithBodyResult(HttpStatusCode.Conflict, Strings.UploadPackage_OwnerlessIdNamespaceConflict);
+            }
 
             var message = result.PermissionsCheckResult == PermissionsCheckResult.Allowed && !result.IsOwnerConfirmed ?
                 Strings.ApiKeyOwnerUnconfirmed : Strings.ApiKeyNotAuthorized;
diff --git a/src/NuGetGallery/Controllers/PackagesController.cs b/src/NuGetGallery/Controllers/PackagesController.cs
@@ -507,15 +507,29 @@ private async Task<JsonResult> UploadPackageInternal(PackageArchiveReader packag
             var id = nuspec.GetId();
             existingPackageRegistration = _packageService.FindPackageRegistrationById(id);
             // For a new package id verify if the user is allowed to use it.
-            if (existingPackageRegistration == null &&
-                ActionsRequiringPermissions.UploadNewPackageId.CheckPermissionsOnBehalfOfAnyAccount(
-                    currentUser, new ActionOnNewPackageContext(id, _reservedNamespaceService), out accountsAllowedOnBehalfOf) != PermissionsCheckResult.Allowed)
+            if (existingPackageRegistration == null)
             {
-                var version = nuspec.GetVersion().ToNormalizedString();
-                _telemetryService.TrackPackagePushNamespaceConflictEvent(id, version, currentUser, User.Identity);
+                var result = ActionsRequiringPermissions.UploadNewPackageId.CheckPermissionsOnBehalfOfAnyAccount(
+                    currentUser,
+                    new ActionOnNewPackageContext(id, _reservedNamespaceService),
+                    out accountsAllowedOnBehalfOf);
 
-                return Json(HttpStatusCode.Conflict, new[] {
-                    new JsonValidationMessage(string.Format(CultureInfo.CurrentCulture, Strings.UploadPackage_IdNamespaceConflict)) });
+                if (result == PermissionsCheckResult.ReservedNamespaceFailure)
+                {
+                    var version = nuspec.GetVersion().ToNormalizedString();
+                    _telemetryService.TrackPackagePushNamespaceConflictEvent(id, version, currentUser, User.Identity);
+                    return Json(HttpStatusCode.Conflict, new[] { new JsonValidationMessage(Strings.UploadPackage_IdNamespaceConflict) });
+                }
+                else if (result == PermissionsCheckResult.OwnerlessReservedNamespaceFailure)
+                {
+                    var version = nuspec.GetVersion().ToNormalizedString();
+                    _telemetryService.TrackPackagePushOwnerlessNamespaceConflictEvent(id, version, currentUser, User.Identity);
+                    return Json(HttpStatusCode.Conflict, new[] { new JsonValidationMessage(new OwnerlessNamespaceIdConflictMessage()) });
+                }
+                else if (result != PermissionsCheckResult.Allowed)
+                {
+                    throw new InvalidOperationException($"When checking if a new package ID can be created, result '{result}' is not expected.");
+                }
             }
 
             // For existing package id verify if it is owned by the current user
@@ -2577,9 +2591,13 @@ protected virtual async Task<JsonResult> VerifyPackageInternal(
                             // The owner specified in the form is not allowed to push to a reserved namespace matching the new ID
                             var version = packageVersion.ToNormalizedString();
                             _telemetryService.TrackPackagePushNamespaceConflictEvent(packageId, version, currentUser, User.Identity);
-
-                            var message = string.Format(CultureInfo.CurrentCulture, Strings.UploadPackage_IdNamespaceConflict);
-                            return Json(HttpStatusCode.Conflict, new[] { new JsonValidationMessage(message) });
+                            return Json(HttpStatusCode.Conflict, new[] { new JsonValidationMessage(Strings.UploadPackage_IdNamespaceConflict) });
+                        }
+                        else if (checkPermissionsOfUploadNewId == PermissionsCheckResult.OwnerlessReservedNamespaceFailure)
+                        {
+                            var version = packageVersion.ToNormalizedString();
+                            _telemetryService.TrackPackagePushOwnerlessNamespaceConflictEvent(packageId, version, currentUser, User.Identity);
+                            return Json(HttpStatusCode.Conflict, new[] { new JsonValidationMessage(new OwnerlessNamespaceIdConflictMessage()) });
                         }
 
                         // An unknown error occurred.
diff --git a/src/NuGetGallery/NuGetGallery.csproj b/src/NuGetGallery/NuGetGallery.csproj
@@ -308,6 +308,7 @@
     <Compile Include="Services\IPackageVulnerabilitiesService.cs" />
     <Compile Include="Services\IPackageMetadataValidationService.cs" />
     <Compile Include="Services\MarkdownService.cs" />
+    <Compile Include="Services\OwnerlessNamespaceIdConflictMessage.cs" />
     <Compile Include="Services\PackageVulnerabilitiesService.cs" />
     <Compile Include="Services\PackageMetadataValidationService.cs" />
     <Compile Include="Services\ConfigurationIconFileProvider.cs" />
diff --git a/src/NuGetGallery/Services/OwnerlessNamespaceIdConflictMessage.cs b/src/NuGetGallery/Services/OwnerlessNamespaceIdConflictMessage.cs
@@ -0,0 +1,16 @@
+﻿// 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 package ID reservation conflict, but all of the namespaces that prevented the upload have no
+    /// owners.
+    /// </summary>
+    public class OwnerlessNamespaceIdConflictMessage : IValidationMessage
+    {
+        public bool HasRawHtmlRepresentation => true;
+        public string PlainTextMessage => Strings.UploadPackage_OwnerlessIdNamespaceConflict;
+        public string RawHtmlMessage => Strings.UploadPackage_OwnerlessIdNamespaceConflictHtml;
+    }
+}
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
@@ -1209,4 +1209,10 @@ The {1} Team</value>
     <value>The provided package has more than {0} downloads and therefore cannot be deleted via API.</value>
     <comment>{0} is the download count limit.</comment>
   </data>
+  <data name="UploadPackage_OwnerlessIdNamespaceConflict" xml:space="preserve">
+    <value>The package ID is reserved. You can upload your package with a different package ID. Reach out to support@nuget.org if you have questions.</value>
+  </data>
+  <data name="UploadPackage_OwnerlessIdNamespaceConflictHtml" xml:space="preserve">
+    <value>The package ID is reserved. You can upload your package with a different package ID. Reach out to &lt;a href="mailto:support@nuget.org"&gt;support@nuget.org&lt;/a&gt; if you have questions.</value>
+  </data>
 </root>
diff --git a/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs b/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs
@@ -253,6 +253,11 @@ public void TrackPackagePushNamespaceConflictEvent(string packageId, string pack
             throw new NotImplementedException();
         }
 
+        public void TrackPackagePushOwnerlessNamespaceConflictEvent(string packageId, string packageVersion, User user, IIdentity identity)
+        {
+            throw new NotImplementedException();
+        }
+
         public void TrackPackageReadMeChangeEvent(Package package, string readMeSourceType, PackageEditReadMeState readMeState)
         {
             throw new NotImplementedException();
diff --git a/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs
@@ -222,9 +222,24 @@ public static IEnumerable<object[]> InvalidScopes_Data
                         continue;
                     }
 
-                    var isReservedNamespaceConflict = result == PermissionsCheckResult.ReservedNamespaceFailure;
-                    var statusCode = isReservedNamespaceConflict ? HttpStatusCode.Conflict : HttpStatusCode.Forbidden;
-                    var description = isReservedNamespaceConflict ? Strings.UploadPackage_IdNamespaceConflict : Strings.ApiKeyNotAuthorized;
+                    HttpStatusCode statusCode;
+                    string description;
+                    if (result == PermissionsCheckResult.ReservedNamespaceFailure)
+                    {
+                        statusCode = HttpStatusCode.Conflict;
+                        description = Strings.UploadPackage_IdNamespaceConflict;
+                    }
+                    else if (result == PermissionsCheckResult.OwnerlessReservedNamespaceFailure)
+                    {
+                        statusCode = HttpStatusCode.Conflict;
+                        description = Strings.UploadPackage_OwnerlessIdNamespaceConflict;
+                    }
+                    else
+                    {
+                        statusCode = HttpStatusCode.Forbidden;
+                        description = Strings.ApiKeyNotAuthorized;
+                    }
+
                     yield return MemberDataHelper.AsData(new ApiScopeEvaluationResult(null, result, scopesAreValid: true), statusCode, description);
                 }
             }
diff --git a/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs
@@ -6505,7 +6505,7 @@ public async Task WillShowTheViewWithErrorsWhenThePackageIdIsAlreadyBeingUsed(Us
                 Assert.Equal(String.Format(Strings.PackageIdNotAvailable, "theId"), (result.Data as JsonValidationMessage[])[0].PlainTextMessage);
             }
 
-            public static IEnumerable<object[]> WillShowTheViewWithErrorsWhenThePackageIdMatchesUnownedNamespace_Data
+            public static IEnumerable<object[]> WillShowTheViewWithErrorsWhenThePackageIdIsBlockedByReservedNamespace_Data
             {
                 get
                 {
@@ -6516,8 +6516,8 @@ public static IEnumerable<object[]> WillShowTheViewWithErrorsWhenThePackageIdMat
             }
 
             [Theory]
-            [MemberData(nameof(WillShowTheViewWithErrorsWhenThePackageIdMatchesUnownedNamespace_Data))]
-            public async Task WillShowTheViewWithErrorsWhenThePackageIdMatchesUnownedNamespace(User currentUser, User reservedNamespaceOwner)
+            [MemberData(nameof(WillShowTheViewWithErrorsWhenThePackageIdIsBlockedByReservedNamespace_Data))]
+            public async Task WillShowTheViewWithErrorsWhenThePackageIdMatchesOwnedByOtherUserNamespace(User currentUser, User reservedNamespaceOwner)
             {
                 var fakeUploadedFile = new Mock<HttpPostedFileBase>();
                 fakeUploadedFile.Setup(x => x.FileName).Returns("theFile.nupkg");
@@ -6552,7 +6552,8 @@ 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 JsonValidationMessage[])[0].PlainTextMessage);
+                Assert.Equal(Strings.UploadPackage_IdNamespaceConflict, (result.Data as JsonValidationMessage[])[0].PlainTextMessage);
+                Assert.Null((result.Data as JsonValidationMessage[])[0].RawHtmlMessage);
                 fakeTelemetryService.Verify(
                     x => x.TrackPackagePushNamespaceConflictEvent(
                         It.IsAny<string>(),
@@ -6562,6 +6563,54 @@ public async Task WillShowTheViewWithErrorsWhenThePackageIdMatchesUnownedNamespa
                     Times.Once);
             }
 
+            [Theory]
+            [MemberData(nameof(WillShowTheViewWithErrorsWhenThePackageIdIsBlockedByReservedNamespace_Data))]
+            public async Task WillShowTheViewWithErrorsWhenThePackageIdMatchesUnownedNamespace(User currentUser, User reservedNamespaceOwner)
+            {
+                var fakeUploadedFile = new Mock<HttpPostedFileBase>();
+                fakeUploadedFile.Setup(x => x.FileName).Returns("theFile.nupkg");
+                Stream fakeFileStream = TestPackage.CreateTestPackageStream("Random.Package1", "1.0.0");
+                fakeUploadedFile.Setup(x => x.InputStream).Returns(fakeFileStream);
+
+                var fakeUploadFileService = new Mock<IUploadFileService>();
+                fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(currentUser.Key)).Returns(Task.FromResult(0));
+                fakeUploadFileService.SetupSequence(x => x.GetUploadFileAsync(currentUser.Key))
+                    .Returns(Task.FromResult<Stream>(null))
+                    .Returns(Task.FromResult(fakeFileStream));
+
+                var fakePackageService = new Mock<IPackageService>();
+                fakePackageService.Setup(x => x.FindPackageRegistrationById(It.IsAny<string>())).Returns(() => null);
+
+                var fakeReservedNamespaceService = new Mock<IReservedNamespaceService>();
+                fakeReservedNamespaceService
+                    .Setup(r => r.GetReservedNamespacesForId(It.IsAny<string>()))
+                    .Returns(new[] { new ReservedNamespace() });
+
+                var fakeTelemetryService = new Mock<ITelemetryService>();
+
+                var controller = CreateController(
+                    GetConfigurationService(),
+                    uploadFileService: fakeUploadFileService,
+                    packageService: fakePackageService,
+                    fakeNuGetPackage: fakeFileStream,
+                    reservedNamespaceService: fakeReservedNamespaceService,
+                    telemetryService: fakeTelemetryService);
+                controller.SetCurrentUser(currentUser);
+
+                var result = await controller.UploadPackage(fakeUploadedFile.Object) as JsonResult;
+
+                Assert.NotNull(result);
+                Assert.Null((result.Data as JsonValidationMessage[])[0].PlainTextMessage);
+                Assert.Equal(Strings.UploadPackage_OwnerlessIdNamespaceConflictHtml, (result.Data as JsonValidationMessage[])[0].RawHtmlMessage);
+                fakeTelemetryService.Verify(
+                    x => x.TrackPackagePushOwnerlessNamespaceConflictEvent(
+                        It.IsAny<string>(),
+                        It.IsAny<string>(),
+                        currentUser,
+                        controller.OwinContext.Request.User.Identity),
+                    Times.Once);
+            }
+
             public static IEnumerable<object[]> WillUploadThePackageWhenIdMatchesOwnedNamespace_Data
             {
                 get
diff --git a/tests/NuGetGallery.Facts/Services/ReservedNamespaceServiceFacts.cs b/tests/NuGetGallery.Facts/Services/ReservedNamespaceServiceFacts.cs
diff --git a/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs b/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs

Original file line number	Diff line number	Diff line change
`@@ -256,6 +256,11 @@ public void TrackPackagePushNamespaceConflictEvent(string packageId, string pack`
`256`	`256`	`throw new NotImplementedException();`
`257`	`257`	`}`
`258`	`258`
	`259`	`+ public void TrackPackagePushOwnerlessNamespaceConflictEvent(string packageId, string packageVersion, User user, IIdentity identity)`
	`260`	`+ {`
	`261`	`+ throw new NotImplementedException();`
	`262`	`+ }`
	`263`	`+`
`259`	`264`	`public void TrackPackageReadMeChangeEvent(Package package, string readMeSourceType, PackageEditReadMeState readMeState)`
`260`	`265`	`{`
`261`	`266`	`throw new NotImplementedException();`
Original file line number	Diff line number	Diff line change
`@@ -31,6 +31,12 @@ public enum PermissionsCheckResult`
`31`	`31`	`/// <summary>`
`32`	`32`	`/// The current user does not have permissions to perform the action on the <see cref="ReservedNamespace"/> on behalf of another <see cref="User"/>.`
`33`	`33`	`/// </summary>`
`34`		`- ReservedNamespaceFailure`
	`34`	`+ ReservedNamespaceFailure,`
	`35`	`+`
	`36`	`+ /// <summary>`
	`37`	`+ /// The current user does not have permissions to perform the action on the <see cref="ReservedNamespace"/> on behalf of another <see cref="User"/>`
	`38`	`+ /// but none of the namespaces currently have owners.`
	`39`	`+ /// </summary>`
	`40`	`+ OwnerlessReservedNamespaceFailure,`
`35`	`41`	`}`
`36`	`42`	`}`