RequirePackageMetadataCompliancePolicy: allow exceptions for authors (#6728)

Author: xavierdecoster

src/NuGetGallery/Security/MicrosoftTeamSubscription.cs

@@ -61,6 +61,7 @@ private static List<UserSecurityPolicy> InitializePoliciesList()
                     Name,
                     MicrosoftUsername,
                     allowedCopyrightNotices: AllowedCopyrightNotices,
+                    allowedAuthors: new[] { MicrosoftUsername },
                     isLicenseUrlRequired: true,
                     isProjectUrlRequired: true,
                     errorMessageFormat: Strings.SecurityPolicy_RequireMicrosoftPackageMetadataComplianceForPush)

src/NuGetGallery/Security/RequirePackageMetadataCompliancePolicy.cs

@@ -95,6 +95,7 @@ public static UserSecurityPolicy CreatePolicy(
             string subscription,
             string requiredCoOwnerUsername,
             string[] allowedCopyrightNotices,
+            string[] allowedAuthors,
             bool isLicenseUrlRequired,
             bool isProjectUrlRequired,
             string errorMessageFormat)
@@ -103,6 +104,7 @@ public static UserSecurityPolicy CreatePolicy(
             {
                 RequiredCoOwnerUsername = requiredCoOwnerUsername,
                 AllowedCopyrightNotices = allowedCopyrightNotices,
+                AllowedAuthors = allowedAuthors,
                 IsLicenseUrlRequired = isLicenseUrlRequired,
                 IsProjectUrlRequired = isProjectUrlRequired,
                 ErrorMessageFormat = errorMessageFormat
@@ -116,13 +118,7 @@ private bool IsPackageMetadataCompliant(Package package, State state, out IList<
             complianceFailures = new List<string>();
 
             // Author validation
-            if (!package.FlattenedAuthors
-                .Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries)
-                .Select(s => s.Trim())
-                .Contains(state.RequiredCoOwnerUsername, StringComparer.InvariantCultureIgnoreCase))
-            {
-                complianceFailures.Add(string.Format(CultureInfo.CurrentCulture, Strings.SecurityPolicy_RequiredAuthorMissing, state.RequiredCoOwnerUsername));
-            }
+            ValidatePackageAuthors(package, state, complianceFailures);
 
             // Copyright validation
             if (!state.AllowedCopyrightNotices.Contains(package.Copyright))
@@ -145,6 +141,47 @@ private bool IsPackageMetadataCompliant(Package package, State state, out IList<
             return !complianceFailures.Any();
         }
 
+        private static void ValidatePackageAuthors(Package package, State state, IList<string> complianceFailures)
+        {
+            var packageAuthors = package.FlattenedAuthors
+                .Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries)
+                .Select(s => s.Trim())
+                .ToList();
+
+            // Check for duplicate entries
+            var duplicateAuthors = packageAuthors
+                .GroupBy(x => x)
+                .Where(group => group.Count() > 1)
+                .Select(group => group.Key)
+                .ToList();
+
+            if (duplicateAuthors.Any())
+            {
+                complianceFailures.Add(string.Format(CultureInfo.CurrentCulture, Strings.SecurityPolicy_PackageAuthorDuplicatesNotAllowed, string.Join(",", duplicateAuthors)));
+            }
+            else
+            {
+                if (state.AllowedAuthors?.Length > 0)
+                {
+                    foreach (var packageAuthor in packageAuthors)
+                    {
+                        if (!state.AllowedAuthors.Contains(packageAuthor))
+                        {
+                            complianceFailures.Add(string.Format(CultureInfo.CurrentCulture, Strings.SecurityPolicy_PackageAuthorNotAllowed, packageAuthor));
+                        }
+                    }
+                }
+                else
+                {
+                    // No list of allowed authors is defined for this policy.
+                    // We require the required co-owner to be defined as the only package author.
+                    if (packageAuthors.Count() > 1 || packageAuthors.Single() != state.RequiredCoOwnerUsername)
+                    {
+                        complianceFailures.Add(string.Format(CultureInfo.CurrentCulture, Strings.SecurityPolicy_RequiredAuthorMissing, state.RequiredCoOwnerUsername));
+                    }
+                }
+            }
+        }
 
         /// <summary>
         /// Retrieve the policy state.
@@ -167,6 +204,9 @@ public class State
             [JsonProperty("copy")]
             public string[] AllowedCopyrightNotices { get; set; }
 
+            [JsonProperty("authors")]
+            public string[] AllowedAuthors { get; set; }
+
             [JsonProperty("licUrlReq")]
             public bool IsLicenseUrlRequired { get; set; }
 

src/NuGetGallery/Strings.Designer.cs

@@ -1077,4 +1077,12 @@ The {1} Team</value>
   <data name="UploadPackage_MalformedLicenseUrl" xml:space="preserve">
     <value>The package contains a malformed license URL.</value>
   </data>
+  <data name="SecurityPolicy_PackageAuthorNotAllowed" xml:space="preserve">
+    <value>The package metadata defines '{0}' as one of the authors which is not allowed by policy.</value>
+    <comment>{0} is the author that is not in the list of allowed authors for this policy.</comment>
+  </data>
+  <data name="SecurityPolicy_PackageAuthorDuplicatesNotAllowed" xml:space="preserve">
+    <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>
 </root>

src/NuGetGallery/Strings.resx

@@ -1391,7 +1391,7 @@ public async Task AddsRequiredCoOwnerWhenPackageWithNewRegistrationIdIsCompliant
                         _packageId,
                         "1.0.0",
                         isSigned: true,
-                        authors: $"{_user.Username},{_requiredCoOwner.Username}",
+                        authors: $"{_requiredCoOwner.Username}",
                         licenseUrl: new Uri("https://github.com/NuGet/NuGetGallery/blob/master/LICENSE.txt"),
                         projectUrl: new Uri("https://www.nuget.org"));
 

tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs

@@ -40,6 +40,7 @@ public void Policies_ReturnsMicrosoftTeamSubscriptionPolicies()
                 "\"© корпорация Майкрософт. Все права защищены.\"," +
                 "\"© Microsoft Corporation。 著作權所有，並保留一切權利。\"" +
                 "]," +
+                "\"authors\":[\"Microsoft\"]," +
                 "\"licUrlReq\":true," +
                 "\"projUrlReq\":true," +
                 "\"error\":\"The package is not compliant with metadata requirements for Microsoft packages on NuGet.org. Go to https://aka.ms/Microsoft-NuGet-Compliance for more information.\\r\\nPolicy violations: {0}\"}", 

tests/NuGetGallery.Facts/Security/MicrosoftTeamSubscriptionFacts.cs

@@ -44,7 +44,6 @@ public void Evaluate_ThrowsForNullArgument()
             Assert.ThrowsAsync<ArgumentNullException>(() => policyHandler.EvaluateAsync(null));
         }
 
-
         [Fact]
         public async Task Evaluate_DoesNotCommitChangesToEntityContext()
         {
@@ -71,13 +70,13 @@ public async Task Evaluate_DoesNotCommitChangesToEntityContext()
             var telemetryService = new Mock<ITelemetryService>().Object;
 
             var context = new PackageSecurityPolicyEvaluationContext(
-                userService.Object, 
-                packageOwnershipManagementService.Object, 
+                userService.Object,
+                packageOwnershipManagementService.Object,
                 telemetryService,
-                subscription.Policies, 
+                subscription.Policies,
                 newMicrosoftCompliantPackage,
                 sourceAccount: nugetUser,
-                targetAccount: nugetUser, 
+                targetAccount: nugetUser,
                 httpContext: It.IsAny<HttpContextBase>());
 
             // Act
@@ -97,9 +96,9 @@ public async Task Evaluate_SilentlySucceedsWhenRequiredCoOwnerDoesNotExist()
             var policyHandler = new RequirePackageMetadataCompliancePolicy();
             var fakes = new Fakes();
             var context = CreateTestContext(
-                false, 
-                subscription.Policies, 
-                fakes.NewPackageVersion, 
+                false,
+                subscription.Policies,
+                fakes.NewPackageVersion,
                 packageRegistrationAlreadyExists: false,
                 sourceAccount: nugetUser,
                 targetAccount: nugetUser);
@@ -206,6 +205,120 @@ public async Task Evaluate_NonCompliantPackage_CreatesErrorResult(Package nonCom
             Assert.False(newPackageRegistration.IsVerified);
         }
 
+        [Fact]
+        public async Task Evaluate_NonCompliantPackageAuthor_CreatesErrorResult()
+        {
+            // Arrange
+            var nugetUser = new User("NuGet");
+            var newPackageRegistration = new PackageRegistration { Id = "NewPackageId", Owners = new List<User> { nugetUser } };
+            var packageAuthors = new[] { MicrosoftTeamSubscription.MicrosoftUsername, "The Not-Allowed Package Authors" };
+            var nonCompliantPackage = Fakes.CreateCompliantPackage("1.0.0", newPackageRegistration, packageAuthors);
+
+            var policy = RequirePackageMetadataCompliancePolicy.CreatePolicy(
+                    MicrosoftTeamSubscription.Name,
+                    MicrosoftTeamSubscription.MicrosoftUsername,
+                    allowedCopyrightNotices: MicrosoftTeamSubscription.AllowedCopyrightNotices,
+                    allowedAuthors: new[] { MicrosoftTeamSubscription.MicrosoftUsername },
+                    isLicenseUrlRequired: true,
+                    isProjectUrlRequired: true,
+                    errorMessageFormat: Strings.SecurityPolicy_RequireMicrosoftPackageMetadataComplianceForPush);
+
+            var policyHandler = new RequirePackageMetadataCompliancePolicy();
+
+            var context = CreateTestContext(
+                true,
+                new[] { policy },
+                nonCompliantPackage,
+                packageRegistrationAlreadyExists: false,
+                sourceAccount: nugetUser,
+                targetAccount: nugetUser);
+
+            // Act
+            var result = await policyHandler.EvaluateAsync(context);
+
+            // Assert
+            Assert.False(result.Success);
+            Assert.Null(newPackageRegistration.Owners.SingleOrDefault(u => u.Username == MicrosoftTeamSubscription.MicrosoftUsername));
+            Assert.False(newPackageRegistration.IsVerified);
+        }
+
+        [Fact]
+        public async Task Evaluate_DuplicatePackageAuthor_CreatesErrorResult()
+        {
+            // Arrange
+            var nugetUser = new User("NuGet");
+            var newPackageRegistration = new PackageRegistration { Id = "NewPackageId", Owners = new List<User> { nugetUser } };
+            var packageAuthors = new[] { MicrosoftTeamSubscription.MicrosoftUsername, MicrosoftTeamSubscription.MicrosoftUsername };
+            var nonCompliantPackage = Fakes.CreateCompliantPackage("1.0.0", newPackageRegistration, packageAuthors);
+
+            var policy = RequirePackageMetadataCompliancePolicy.CreatePolicy(
+                    MicrosoftTeamSubscription.Name,
+                    MicrosoftTeamSubscription.MicrosoftUsername,
+                    allowedCopyrightNotices: MicrosoftTeamSubscription.AllowedCopyrightNotices,
+                    allowedAuthors: new[] { MicrosoftTeamSubscription.MicrosoftUsername },
+                    isLicenseUrlRequired: true,
+                    isProjectUrlRequired: true,
+                    errorMessageFormat: Strings.SecurityPolicy_RequireMicrosoftPackageMetadataComplianceForPush);
+
+            var policyHandler = new RequirePackageMetadataCompliancePolicy();
+
+            var context = CreateTestContext(
+                true,
+                new[] { policy },
+                nonCompliantPackage,
+                packageRegistrationAlreadyExists: false,
+                sourceAccount: nugetUser,
+                targetAccount: nugetUser);
+
+            // Act
+            var result = await policyHandler.EvaluateAsync(context);
+
+            // Assert
+            Assert.False(result.Success);
+            Assert.Null(newPackageRegistration.Owners.SingleOrDefault(u => u.Username == MicrosoftTeamSubscription.MicrosoftUsername));
+            Assert.False(newPackageRegistration.IsVerified);
+        }
+
+        [Fact]
+        public async Task Evaluate_CompliantPackageAuthors_CreatesSuccessResult()
+        {
+            // Arrange
+            var nugetUser = new User("NuGet");
+            var newPackageRegistration = new PackageRegistration { Id = "NewPackageId", Owners = new List<User> { nugetUser } };
+            var packageAuthors = new[] { MicrosoftTeamSubscription.MicrosoftUsername, "The Most-Awesome Package Authors" };
+            var compliantPackage = Fakes.CreateCompliantPackage("1.0.0", newPackageRegistration, packageAuthors);
+
+            var policy = RequirePackageMetadataCompliancePolicy.CreatePolicy(
+                    MicrosoftTeamSubscription.Name,
+                    MicrosoftTeamSubscription.MicrosoftUsername,
+                    allowedCopyrightNotices: MicrosoftTeamSubscription.AllowedCopyrightNotices,
+                    allowedAuthors: packageAuthors,
+                    isLicenseUrlRequired: true,
+                    isProjectUrlRequired: true,
+                    errorMessageFormat: Strings.SecurityPolicy_RequireMicrosoftPackageMetadataComplianceForPush);
+
+            var policyHandler = new RequirePackageMetadataCompliancePolicy();
+
+            var packageOwnershipManagementService = new Mock<IPackageOwnershipManagementService>();
+            packageOwnershipManagementService.Setup(m => m.AddPackageOwnerAsync(newPackageRegistration, It.IsAny<User>(), false)).Returns(Task.CompletedTask);
+
+            var context = CreateTestContext(
+                true,
+                new[] { policy },
+                compliantPackage,
+                packageRegistrationAlreadyExists: false,
+                sourceAccount: nugetUser,
+                targetAccount: nugetUser,
+                packageOwnershipManagementService: packageOwnershipManagementService.Object);
+
+            // Act
+            var result = await policyHandler.EvaluateAsync(context);
+
+            // Assert
+            Assert.True(result.Success);
+            packageOwnershipManagementService.Verify(s => s.AddPackageOwnerAsync(newPackageRegistration, Fakes.RequiredCoOwner, false), Times.Once);
+        }
+
         private static PackageSecurityPolicyEvaluationContext CreateTestContext(
             bool microsoftUserExists,
             IEnumerable<UserSecurityPolicy> policies,
@@ -298,7 +411,7 @@ public Fakes(
                 };
             }
 
-            public static Package CreateCompliantPackage(string version, PackageRegistration packageRegistration)
+            public static Package CreateCompliantPackage(string version, PackageRegistration packageRegistration, string[] allowedAuthors = null)
             {
                 return new Package
                 {
@@ -307,7 +420,7 @@ public static Package CreateCompliantPackage(string version, PackageRegistration
                     Copyright = "(c) Microsoft Corporation. All rights reserved.",
                     ProjectUrl = "https://github.com/NuGet/NuGetGallery",
                     LicenseUrl = "https://github.com/NuGet/NuGetGallery/blob/master/LICENSE.txt",
-                    FlattenedAuthors = "NuGet, Microsoft"
+                    FlattenedAuthors = allowedAuthors == null ? "Microsoft" : string.Join(",", allowedAuthors)
                 };
             }
 
@@ -345,7 +458,7 @@ public static IReadOnlyCollection<Package> CreateNonCompliantPackages()
             public User Owner { get; }
 
             public Package NewPackageVersion { get; }
-            
+
             public PackageRegistration ExistingPackageRegistration { get; }
 
             public static User RequiredCoOwner { get; }