Copy list of scopes before removing them (#10275)

A collection modified exception is thrown since internally the DbContext modifies the collection we are enumerating.

Author: joelverhagen

src/NuGetGallery.Services/Authentication/AuthenticationService.cs

@@ -1,4 +1,4 @@
-// Copyright (c) .NET Foundation. All rights reserved.
+// 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.
 
 using System;
@@ -714,7 +714,7 @@ public virtual async Task RemoveCredential(User user, Credential cred, bool comm
 
         public virtual async Task EditCredentialScopes(User user, Credential cred, ICollection<Scope> newScopes)
         {
-            foreach (var oldScope in cred.Scopes)
+            foreach (var oldScope in cred.Scopes.ToList())
             {
                 Entities.Scopes.Remove(oldScope);
             }
@@ -1053,4 +1053,4 @@ private async Task MigrateCredentials(User user, List<Credential> creds, string
             }
         }
     }
-}
+}

tests/NuGetGallery.Facts/Authentication/AuthenticationServiceFacts.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Data.Entity;
 using System.Globalization;
 using System.Linq;
 using System.Security.Claims;
@@ -2382,6 +2383,44 @@ public async Task SavesChangesInTheDataStore()
                 authService.Entities.VerifyCommitChanges();
             }
 
+            /// <summary>
+            /// Needed to avoid collection modified exception caused by the entity context.
+            /// </summary>
+            [Fact]
+            public async Task CopiesScopeCollectionForDeletion()
+            {
+                // Arrange
+                var credentialBuilder = new CredentialBuilder();
+
+                var credScopes =
+                    Enumerable.Range(0, 5)
+                        .Select(
+                            i => new Scope { AllowedAction = NuGetScopes.PackagePush, Key = i, Subject = "package" + i }).ToList();
+
+                var mockScopes = new Mock<DbSet<Scope>>();
+                var dbContext = GetMock<IEntitiesContext>();
+                dbContext.Setup(x => x.Scopes).Returns(mockScopes.Object);
+                mockScopes.Setup(x => x.Remove(It.IsAny<Scope>())).Callback<Scope>(x => credScopes.Remove(x));
+
+                var fakes = Get<Fakes>();
+                var cred = credentialBuilder.CreateApiKey(null, out string plaintextApiKey);
+                var user = fakes.CreateUser("test", credentialBuilder.CreatePasswordCredential(Fakes.Password), cred);
+                var authService = Get<AuthenticationService>();
+
+                var newScopes =
+                    Enumerable.Range(1, 2)
+                        .Select(
+                            i => new Scope { AllowedAction = NuGetScopes.PackageUnlist, Key = i * 10, Subject = "otherpackage" + i }).ToList();
+
+                cred.Scopes = credScopes;
+
+                // Act
+                await authService.EditCredentialScopes(user, cred, newScopes);
+
+                // Act
+                Assert.Empty(credScopes);
+            }
+
             [Fact]
             public async Task WritesAuditRecordForTheEditedCredential()
             {