PR feedback

NuGet · Jun 12, 2017 · 7afc8df · 7afc8df
1 parent 54c8e5a
commit 7afc8df
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 22 deletions.
diff --git a/src/NuGetGallery/Areas/Admin/Controllers/SecurityPolicyController.cs b/src/NuGetGallery/Areas/Admin/Controllers/SecurityPolicyController.cs
@@ -48,7 +48,9 @@ public virtual JsonResult Search(string query)
         {
             // Parse query and look for users in the DB.
             var usernames = GetUsernamesFromQuery(query ?? "");
-            var users = FindUsers(usernames);
+            var users = EntitiesContext.Users
+                .Where(u => usernames.Any(name => u.Username == name))
+                .ToList();
             var usersNotFound = usernames.Except(users.Select(u => u.Username));
 
             var results = new UserSecurityPolicySearchResult()
@@ -89,19 +91,25 @@ public async Task<JsonResult> Update(List<string> subscriptionsJson)
 
             foreach (var r in subscribeRequests)
             {
-                var user = FindUser(r.Key);
-                foreach (var subscription in r.Value)
+                var user = EntitiesContext.Users.FirstOrDefault(u => u.Username == r.Key);
+                if (user != null)
                 {
-                    await PolicyService.SubscribeAsync(user, subscription);
+                    foreach (var subscription in r.Value)
+                    {
+                        await PolicyService.SubscribeAsync(user, subscription);
+                    }
                 }
             }
 
             foreach (var r in unsubscribeRequests)
             {
-                var user = FindUser(r.Key);
-                foreach (var subscription in r.Value)
+                var user = EntitiesContext.Users.FirstOrDefault(u => u.Username == r.Key);
+                if (user != null)
                 {
-                    await PolicyService.UnsubscribeAsync(user, subscription);
+                    foreach (var subscription in r.Value)
+                    {
+                        await PolicyService.UnsubscribeAsync(user, subscription);
+                    }
                 }
             }
 
@@ -114,18 +122,5 @@ private static string[] GetUsernamesFromQuery(string query)
                 .Select(username => username.Trim())
                 .Where(username => !string.IsNullOrEmpty(username)).ToArray();
         }
-
-        private User FindUser(string username)
-        {
-            return EntitiesContext.Users
-                .FirstOrDefault(u => username.Equals(u.Username, StringComparison.OrdinalIgnoreCase));
-        }
-
-        private IEnumerable<User> FindUsers(string[] usernames)
-        {
-            return EntitiesContext.Users
-                .Where(u => usernames.Any(name => u.Username.Equals(name, StringComparison.OrdinalIgnoreCase)))
-                .ToList();
-        }
     }
 }
diff --git a/src/NuGetGallery/Security/SecurityPolicyService.cs b/src/NuGetGallery/Security/SecurityPolicyService.cs
@@ -165,7 +165,7 @@ public Task<bool> SubscribeAsync(User user, string subscriptionName)
         {
             if (string.IsNullOrEmpty(subscriptionName))
             {
-                throw new ArgumentNullException(nameof(subscriptionName));
+                throw new ArgumentException(nameof(subscriptionName));
             }
 
             var subscription = UserSubscriptions.FirstOrDefault(s => s.SubscriptionName.Equals(subscriptionName, StringComparison.OrdinalIgnoreCase));
@@ -225,7 +225,7 @@ public Task UnsubscribeAsync(User user, string subscriptionName)
         {
             if (string.IsNullOrEmpty(subscriptionName))
             {
-                throw new ArgumentNullException(nameof(subscriptionName));
+                throw new ArgumentException(nameof(subscriptionName));
             }
 
             var subscription = UserSubscriptions.FirstOrDefault(s => s.SubscriptionName.Equals(subscriptionName, StringComparison.OrdinalIgnoreCase));

diff --git a/tests/NuGetGallery.Facts/Areas/Admin/Controllers/SecurityPolicyControllerFacts.cs b/tests/NuGetGallery.Facts/Areas/Admin/Controllers/SecurityPolicyControllerFacts.cs
@@ -187,6 +187,28 @@ public async Task UpdateUnsubscribesUsers()
             policyService.MockEntitiesContext.Verify(c => c.SaveChangesAsync(), Times.Exactly(2));
         }
 
+        [Fact]
+        public async Task UpdateIgnoresBadUsers()
+        {
+            // Arrange.
+            var users = TestUsers.ToList();
+            var policyService = new TestSecurityPolicyService();
+            var entitiesMock = policyService.MockEntitiesContext;
+            entitiesMock.Setup(c => c.Users).Returns(users.MockDbSet().Object);
+            var controller = new SecurityPolicyController(entitiesMock.Object, policyService);
+            var subscription = policyService.Mocks.Subscription.Object;
+
+            // Act.
+            var model = new List<string>
+            {
+                $"{{\"u\":\"D\",\"g\":\"{subscription.SubscriptionName}\",\"v\":\"false\"}}"
+            };
+            var result = await controller.Update(model);
+
+            // Assert.
+            policyService.MockEntitiesContext.Verify(c => c.SaveChangesAsync(), Times.Never);
+        }
+
         private IEnumerable<User> TestUsers
         {
             get