Audit add and remove ownership request action (#8764)

joelverhagen · web-flow · commit c98395ffeebe · 2021-08-27T11:02:07.000-07:00
Progress on NuGet/Engineering#3994
diff --git a/src/NuGetGallery.Core/Auditing/AuditedPackageRegistrationAction.cs b/src/NuGetGallery.Core/Auditing/AuditedPackageRegistrationAction.cs
@@ -9,6 +9,8 @@ public enum AuditedPackageRegistrationAction
         RemoveOwner,
         MarkVerified,
         MarkUnverified,
-        SetRequiredSigner
+        SetRequiredSigner,
+        AddOwnershipRequest,
+        DeleteOwnershipRequest,
     }
 }
diff --git a/src/NuGetGallery.Core/Auditing/PackageRegistrationAuditRecord.cs b/src/NuGetGallery.Core/Auditing/PackageRegistrationAuditRecord.cs
@@ -14,6 +14,8 @@ public class PackageRegistrationAuditRecord : AuditRecord<AuditedPackageRegistra
         public string Owner { get; }
         public string PreviousRequiredSigner { get; private set; }
         public string NewRequiredSigner { get; private set; }
+        public string RequestingOwner { get; private set; }
+        public string NewOwner { get; private set; }
 
         public PackageRegistrationAuditRecord(
             string id, AuditedPackageRegistration registrationRecord, AuditedPackageRegistrationAction action, string owner)
@@ -55,5 +57,58 @@ public static PackageRegistrationAuditRecord CreateForSetRequiredSigner(
 
             return record;
         }
+
+        private static PackageRegistrationAuditRecord CreateForOwnerRequest(
+            PackageRegistration registration,
+            string requestingOwner,
+            string newOwner,
+            AuditedPackageRegistrationAction action)
+        {
+            if (registration == null)
+            {
+                throw new ArgumentNullException(nameof(registration));
+            }
+
+            if (requestingOwner == null)
+            {
+                throw new ArgumentNullException(nameof(requestingOwner));
+            }
+
+            if (newOwner == null)
+            {
+                throw new ArgumentNullException(nameof(newOwner));
+            }
+
+            var record = new PackageRegistrationAuditRecord(registration, action, owner: null);
+
+            record.RequestingOwner = requestingOwner;
+            record.NewOwner = newOwner;
+
+            return record;
+        }
+
+        public static PackageRegistrationAuditRecord CreateForAddOwnershipRequest(
+            PackageRegistration registration,
+            string requestingOwner,
+            string newOwner)
+        {
+            return CreateForOwnerRequest(
+                registration,
+                requestingOwner,
+                newOwner,
+                AuditedPackageRegistrationAction.AddOwnershipRequest);
+        }
+
+        public static PackageRegistrationAuditRecord CreateForDeleteOwnershipRequest(
+            PackageRegistration registration,
+            string requestingOwner,
+            string newOwner)
+        {
+            return CreateForOwnerRequest(
+                registration,
+                requestingOwner,
+                newOwner,
+                AuditedPackageRegistrationAction.DeleteOwnershipRequest);
+        }
     }
 }
diff --git a/src/NuGetGallery.Core/NuGetGallery.Core.csproj b/src/NuGetGallery.Core/NuGetGallery.Core.csproj
@@ -43,6 +43,9 @@
   </ItemGroup>
 
   <ItemGroup>
+    <PackageReference Include="Microsoft.IdentityModel.Clients.ActiveDirectory">
+      <Version>5.2.6</Version>
+    </PackageReference>
     <PackageReference Include="NuGet.Packaging">
       <Version>5.9.0</Version>
     </PackageReference>
diff --git a/src/NuGetGallery.Services/PackageManagement/IPackageOwnerRequestService.cs b/src/NuGetGallery.Services/PackageManagement/IPackageOwnerRequestService.cs
@@ -10,14 +10,23 @@ namespace NuGetGallery
     public interface IPackageOwnerRequestService
     {
         /// <summary>
-        /// Gets <see cref="PackageOwnerRequest"/>s that match the provided conditions.
+        /// Gets <see cref="PackageOwnerRequest"/>s that match the provided conditions. User entities on the returned requests are not populated.
         /// </summary>
         /// <param name="package">If nonnull, only returns <see cref="PackageOwnerRequest"/>s that are for this package.</param>
         /// <param name="requestingOwner">If nonnull, only returns <see cref="PackageOwnerRequest"/>s that were requested by this owner.</param>
         /// <param name="newOwner">If nonnull, only returns <see cref="PackageOwnerRequest"/>s that are for this user to become an owner.</param>
         /// <returns>An <see cref="IEnumerable{PackageOwnerRequest}"/> containing all objects that matched the conditions.</returns>
         IEnumerable<PackageOwnerRequest> GetPackageOwnershipRequests(PackageRegistration package = null, User requestingOwner = null, User newOwner = null);
 
+        /// <summary>
+        /// Gets <see cref="PackageOwnerRequest"/>s that match the provided conditions. User entities on the returned requests are populated.
+        /// </summary>
+        /// <param name="package">If nonnull, only returns <see cref="PackageOwnerRequest"/>s that are for this package.</param>
+        /// <param name="requestingOwner">If nonnull, only returns <see cref="PackageOwnerRequest"/>s that were requested by this owner.</param>
+        /// <param name="newOwner">If nonnull, only returns <see cref="PackageOwnerRequest"/>s that are for this user to become an owner.</param>
+        /// <returns>An <see cref="IEnumerable{PackageOwnerRequest}"/> containing all objects that matched the conditions.</returns>
+        IEnumerable<PackageOwnerRequest> GetPackageOwnershipRequestsWithUsers(PackageRegistration package = null, User requestingOwner = null, User newOwner = null);
+
         /// <summary>
         /// Checks if the pending owner has a request for this package which matches the specified token.
         /// </summary>
diff --git a/src/NuGetGallery.Services/PackageManagement/PackageOwnerRequestService.cs b/src/NuGetGallery.Services/PackageManagement/PackageOwnerRequestService.cs
@@ -45,10 +45,37 @@ public PackageOwnerRequest GetPackageOwnershipRequest(PackageRegistration packag
             return request.ConfirmationCode == token ? request : null;
         }
 
-        public IEnumerable<PackageOwnerRequest> GetPackageOwnershipRequests(PackageRegistration package = null, User requestingOwner = null, User newOwner = null)
+        public IEnumerable<PackageOwnerRequest> GetPackageOwnershipRequests(
+            PackageRegistration package = null,
+            User requestingOwner = null,
+            User newOwner = null)
+        {
+            return GetPackageOwnershipRequests(includeUsers: false, package, requestingOwner, newOwner);
+        }
+
+        public IEnumerable<PackageOwnerRequest> GetPackageOwnershipRequestsWithUsers(
+            PackageRegistration package = null,
+            User requestingOwner = null,
+            User newOwner = null)
+        {
+            return GetPackageOwnershipRequests(includeUsers: true, package, requestingOwner, newOwner);
+        }
+
+        private IEnumerable<PackageOwnerRequest> GetPackageOwnershipRequests(
+            bool includeUsers,
+            PackageRegistration package,
+            User requestingOwner,
+            User newOwner)
         {
             var query = _packageOwnerRequestRepository.GetAll().Include(e => e.PackageRegistration);
 
+            if (includeUsers)
+            {
+                query = query
+                    .Include(x => x.RequestingOwner)
+                    .Include(x => x.NewOwner);
+            }
+
             if (package != null)
             {
                 query = query.Where(r => r.PackageRegistrationKey == package.Key);
@@ -101,18 +128,23 @@ public async Task<PackageOwnerRequest> AddPackageOwnershipRequest(PackageRegistr
 
             _packageOwnerRequestRepository.InsertOnCommit(newRequest);
             await _packageOwnerRequestRepository.CommitChangesAsync();
+
             return newRequest;
         }
 
         public async Task DeletePackageOwnershipRequest(PackageOwnerRequest request, bool commitChanges = true)
         {
+            if (request == null)
+            {
+                throw new ArgumentNullException(nameof(request));
+            }
+
             _packageOwnerRequestRepository.DeleteOnCommit(request);
 
             if (commitChanges)
             {
                 await _packageOwnerRequestRepository.CommitChangesAsync();
             }
         }
-        
     }
 }
diff --git a/src/NuGetGallery.Services/PackageManagement/PackageOwnershipManagementService.cs b/src/NuGetGallery.Services/PackageManagement/PackageOwnershipManagementService.cs
@@ -95,12 +95,22 @@ private async Task AddPackageOwnerTask(PackageRegistration packageRegistration,
 
             await _packageService.AddPackageOwnerAsync(packageRegistration, user, commitChanges);
 
-            await DeletePackageOwnershipRequestAsync(packageRegistration, user, commitChanges);
+            // Delete any ownership request related to this new owner, since the work is done. Don't audit since the
+            // "add owner" operation is audited itself. Having a "delete package ownership" audit here would be confusing
+            // because the intended user action was to add an owner, not to reject (delete) an ownership request.
+            await DeletePackageOwnershipRequestAsync(packageRegistration, user, commitChanges, saveAudit: false);
         }
 
         public async Task<PackageOwnerRequest> AddPackageOwnershipRequestAsync(PackageRegistration packageRegistration, User requestingOwner, User newOwner)
         {
-            return await _packageOwnerRequestService.AddPackageOwnershipRequest(packageRegistration, requestingOwner, newOwner);
+            var request = await _packageOwnerRequestService.AddPackageOwnershipRequest(packageRegistration, requestingOwner, newOwner);
+
+            await _auditingService.SaveAuditRecordAsync(PackageRegistrationAuditRecord.CreateForAddOwnershipRequest(
+                packageRegistration,
+                requestingOwner.Username,
+                newOwner.Username));
+
+            return request;
         }
 
         public PackageOwnerRequest GetPackageOwnershipRequest(PackageRegistration package, User pendingOwner, string token)
@@ -184,6 +194,11 @@ private async Task RemovePackageOwnerImplAsync(PackageRegistration packageRegist
         }
 
         public async Task DeletePackageOwnershipRequestAsync(PackageRegistration packageRegistration, User newOwner, bool commitChanges = true)
+        {
+            await DeletePackageOwnershipRequestAsync(packageRegistration, newOwner, commitChanges, saveAudit: true);
+        }
+
+        private async Task DeletePackageOwnershipRequestAsync(PackageRegistration packageRegistration, User newOwner, bool commitChanges, bool saveAudit)
         {
             if (packageRegistration == null)
             {
@@ -195,10 +210,25 @@ public async Task DeletePackageOwnershipRequestAsync(PackageRegistration package
                 throw new ArgumentNullException(nameof(newOwner));
             }
 
-            var request = _packageOwnerRequestService.GetPackageOwnershipRequests(package: packageRegistration, newOwner: newOwner).FirstOrDefault();
+            var request = _packageOwnerRequestService
+                .GetPackageOwnershipRequestsWithUsers(package: packageRegistration, newOwner: newOwner)
+                .FirstOrDefault();
             if (request != null)
             {
+                // We must capture this audit record prior to the actual deletion operation. Deletion of an entity in
+                // Entity Framework clears the relationship properties cause us to lose the information needed to create
+                // the audit record. The package ID and usernames become unavailable.
+                var auditRecord = PackageRegistrationAuditRecord.CreateForDeleteOwnershipRequest(
+                    request.PackageRegistration,
+                    request.RequestingOwner.Username,
+                    request.NewOwner.Username);
+
                 await _packageOwnerRequestService.DeletePackageOwnershipRequest(request, commitChanges);
+
+                if (saveAudit)
+                {
+                    await _auditingService.SaveAuditRecordAsync(auditRecord);
+                }
             }
         }
 
diff --git a/src/NuGetGallery/NuGetGallery.csproj b/src/NuGetGallery/NuGetGallery.csproj
@@ -2212,9 +2212,6 @@
     <PackageReference Include="Microsoft.Extensions.Logging.Abstractions">
       <Version>2.2.0</Version>
     </PackageReference>
-    <PackageReference Include="Microsoft.IdentityModel.Clients.ActiveDirectory">
-      <Version>5.2.6</Version>
-    </PackageReference>
     <PackageReference Include="Microsoft.Net.Http">
       <Version>2.2.29</Version>
     </PackageReference>
diff --git a/tests/NuGetGallery.Core.Facts/Auditing/AuditedPackageRegistrationActionTests.cs b/tests/NuGetGallery.Core.Facts/Auditing/AuditedPackageRegistrationActionTests.cs
@@ -16,7 +16,9 @@ public void Definition_HasNotChanged()
                 "RemoveOwner",
                 "MarkVerified",
                 "MarkUnverified",
-                "SetRequiredSigner"
+                "SetRequiredSigner",
+                "AddOwnershipRequest",
+                "DeleteOwnershipRequest",
             };
 
             Verify(typeof(AuditedPackageRegistrationAction), expectedNames);
diff --git a/tests/NuGetGallery.Core.Facts/Auditing/PackageRegistrationAuditRecordTests.cs b/tests/NuGetGallery.Core.Facts/Auditing/PackageRegistrationAuditRecordTests.cs
@@ -38,6 +38,34 @@ public void GetPath_ReturnsLowerCasedId()
             Assert.Equal("a", actualPath);
         }
 
+        [Fact]
+        public void CreateForAddOwnershipRequest_InitializesProperties()
+        {
+            var record = PackageRegistrationAuditRecord.CreateForAddOwnershipRequest(
+                new PackageRegistration { Id = "NuGet.Versioning" },
+                requestingOwner: "NuGet",
+                newOwner: "Microsoft");
+
+            Assert.Equal("NuGet.Versioning", record.Id);
+            Assert.Equal("NuGet", record.RequestingOwner);
+            Assert.Equal("Microsoft", record.NewOwner);
+            Assert.Equal(AuditedPackageRegistrationAction.AddOwnershipRequest, record.Action);
+        }
+
+        [Fact]
+        public void CreateForDeleteOwnershipRequest_InitializesProperties()
+        {
+            var record = PackageRegistrationAuditRecord.CreateForDeleteOwnershipRequest(
+                new PackageRegistration { Id = "NuGet.Versioning" },
+                requestingOwner: "NuGet",
+                newOwner: "Microsoft");
+
+            Assert.Equal("NuGet.Versioning", record.Id);
+            Assert.Equal("NuGet", record.RequestingOwner);
+            Assert.Equal("Microsoft", record.NewOwner);
+            Assert.Equal(AuditedPackageRegistrationAction.DeleteOwnershipRequest, record.Action);
+        }
+
         [Fact]
         public void CreateForSetRequiredSigner_WhenRegistrationIsNull_Throws()
         {
diff --git a/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs
@@ -2659,10 +2659,10 @@ public async Task ReturnsProperResult(
 
         public class PackageVerificationKeyContainer : TestContainer
         {
-            public static int UserKey = 1234;
-            public static string Username = "testuser";
-            public static string PackageId = "foo";
-            public static string PackageVersion = "1.0.0";
+            public const int UserKey = 1234;
+            public const string Username = "testuser";
+            public const string PackageId = "foo";
+            public string PackageVersion = "1.0.0";
 
             internal TestableApiController SetupController(string keyType, Scope scope, Package package, bool isOwner = true)
             {
diff --git a/tests/NuGetGallery.Facts/Extensions/RouteExtensionsFacts.cs b/tests/NuGetGallery.Facts/Extensions/RouteExtensionsFacts.cs
@@ -10,10 +10,10 @@ namespace NuGetGallery.Extensions
 {
     public class RouteExtensionsFacts
     {
-        private static string _routeUrl = "test/{user}";
-        private static string _url = "test/user1";
-        private static int _segment = 1;
-        private static string _obfuscatedValue = "obfuscatedData";
+        private const string _routeUrl = "test/{user}";
+        private const string _url = "test/user1";
+        private const int _segment = 1;
+        private const string _obfuscatedValue = "obfuscatedData";
 
         [Fact]
         public void MapRoute_WithoutConstraints_AddsObfuscation()
diff --git a/tests/NuGetGallery.Facts/Services/DeleteAccountServiceFacts.cs b/tests/NuGetGallery.Facts/Services/DeleteAccountServiceFacts.cs
@@ -21,7 +21,7 @@ public class DeleteAccountServiceFacts
     {
         public class TheDeleteGalleryUserAccountAsyncMethod
         {
-            private static int Key = -1;
+            private int Key = -1;
 
             [Theory]
             [InlineData(false)]
@@ -429,7 +429,7 @@ await deleteAccountService.DeleteAccountAsync(
             }
 
 
-            private static User CreateTestUserWithRegistration(ref PackageRegistration registration)
+            private User CreateTestUserWithRegistration(ref PackageRegistration registration)
             {
                 var testUser = new User("TestUser") { Key = Key++ };
                 testUser.EmailAddress = "user@test.com";
@@ -439,7 +439,7 @@ private static User CreateTestUserWithRegistration(ref PackageRegistration regis
                 return testUser;
             }
 
-            private static User CreateTestUser(ref PackageRegistration registration)
+            private User CreateTestUser(ref PackageRegistration registration)
             {
                 var testUser = new User("TestUser") { Key = Key++ };
                 testUser.EmailAddress = "user@test.com";
@@ -473,7 +473,7 @@ private static User CreateTestUser(ref PackageRegistration registration)
                 return testUser;
             }
 
-            private static void AddOrganizationMigrationRequest(User testUser)
+            private void AddOrganizationMigrationRequest(User testUser)
             {
                 var testOrganizationAdmin = new User("TestOrganizationAdmin") { Key = Key++ };
 
@@ -482,7 +482,7 @@ private static void AddOrganizationMigrationRequest(User testUser)
                 testOrganizationAdmin.OrganizationMigrationRequests.Add(request);
             }
 
-            private static void AddOrganizationMigrationRequests(User testUser)
+            private void AddOrganizationMigrationRequests(User testUser)
             {
                 var testOrganization = new Organization("testOrganization") { Key = Key++ };
 
@@ -491,7 +491,7 @@ private static void AddOrganizationMigrationRequests(User testUser)
                 testUser.OrganizationMigrationRequests.Add(request);
             }
 
-            private static void AddOrganizationRequests(User testUser, bool isAdmin)
+            private void AddOrganizationRequests(User testUser, bool isAdmin)
             {
                 var testOrganization = new Organization("testOrganization") { Key = Key++ };
 
@@ -500,7 +500,7 @@ private static void AddOrganizationRequests(User testUser, bool isAdmin)
                 testUser.OrganizationRequests.Add(request);
             }
 
-            private static void AddOrganization(User testUser, bool isAdmin, bool hasAdminMember, bool hasCollaboratorMember)
+            private void AddOrganization(User testUser, bool isAdmin, bool hasAdminMember, bool hasCollaboratorMember)
             {
                 var testOrganization = new Organization("testOrganization") { Key = Key++ };
 
diff --git a/tests/NuGetGallery.Facts/Services/PackageOwnerRequestServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageOwnerRequestServiceFacts.cs
diff --git a/tests/NuGetGallery.Facts/Services/PackageOwnershipManagementServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageOwnershipManagementServiceFacts.cs

Original file line number	Diff line number	Diff line change
`@@ -9,6 +9,8 @@ public enum AuditedPackageRegistrationAction`
`9`	`9`	`RemoveOwner,`
`10`	`10`	`MarkVerified,`
`11`	`11`	`MarkUnverified,`
`12`		`- SetRequiredSigner`
	`12`	`+ SetRequiredSigner,`
	`13`	`+ AddOwnershipRequest,`
	`14`	`+ DeleteOwnershipRequest,`
`13`	`15`	`}`
`14`	`16`	`}`
Original file line number	Diff line number	Diff line change
`@@ -2659,10 +2659,10 @@ public async Task ReturnsProperResult(`
`2659`	`2659`
`2660`	`2660`	`public class PackageVerificationKeyContainer : TestContainer`
`2661`	`2661`	`{`
`2662`		`- public static int UserKey = 1234;`
`2663`		`- public static string Username = "testuser";`
`2664`		`- public static string PackageId = "foo";`
`2665`		`- public static string PackageVersion = "1.0.0";`
	`2662`	`+ public const int UserKey = 1234;`
	`2663`	`+ public const string Username = "testuser";`
	`2664`	`+ public const string PackageId = "foo";`
	`2665`	`+ public string PackageVersion = "1.0.0";`
`2666`	`2666`
`2667`	`2667`	`internal TestableApiController SetupController(string keyType, Scope scope, Package package, bool isOwner = true)`
`2668`	`2668`	`{`