From 3f894babd7e5e15a802f55d8479e4ea44248c885 Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 9 Dec 2022 07:49:39 -0500 Subject: [PATCH 1/3] Audit the AddCredential action after we commit to the DB This allows the new DB key on the credential to be included in the audit record. --- .../Authentication/AuthenticationService.cs | 26 +++++++--- .../AuthenticationServiceFacts.cs | 51 +++++++++++++++++++ 2 files changed, 71 insertions(+), 6 deletions(-) diff --git a/src/NuGetGallery.Services/Authentication/AuthenticationService.cs b/src/NuGetGallery.Services/Authentication/AuthenticationService.cs index 673bd85613..1dc093d0e6 100644 --- a/src/NuGetGallery.Services/Authentication/AuthenticationService.cs +++ b/src/NuGetGallery.Services/Authentication/AuthenticationService.cs @@ -6,6 +6,7 @@ using System.Data.Entity; using System.Globalization; using System.Linq; +using System.Net; using System.Security.Claims; using System.Threading.Tasks; using System.Web.Mvc; @@ -473,6 +474,9 @@ public virtual async Task ReplaceCredential(User user, Credential credential) { await ReplaceCredentialInternal(user, credential); await Entities.SaveChangesAsync(); + + await Auditing.SaveAuditRecordAsync(new UserAuditRecord( + user, AuditedUserAction.AddCredential, credential)); } public virtual async Task ResetPasswordWithToken(string username, string token, string newPassword) @@ -501,6 +505,10 @@ public virtual async Task ResetPasswordWithToken(string username, st user.FailedLoginCount = 0; user.LastFailedLoginUtc = null; await Entities.SaveChangesAsync(); + + await Auditing.SaveAuditRecordAsync(new UserAuditRecord( + user, AuditedUserAction.AddCredential, cred)); + return cred; } @@ -590,6 +598,10 @@ public virtual async Task ChangePassword(User user, string oldPassword, st // Save changes await Entities.SaveChangesAsync(); + + await Auditing.SaveAuditRecordAsync(new UserAuditRecord( + user, AuditedUserAction.AddCredential, passwordCredential)); + return true; } @@ -623,10 +635,10 @@ public virtual async Task AddCredential(User user, Credential credential) throw new InvalidOperationException(ServicesStrings.OrganizationsCannotCreateCredentials); } - await Auditing.SaveAuditRecordAsync(new UserAuditRecord(user, AuditedUserAction.AddCredential, credential)); user.Credentials.Add(credential); await Entities.SaveChangesAsync(); + await Auditing.SaveAuditRecordAsync(new UserAuditRecord(user, AuditedUserAction.AddCredential, credential)); _telemetryService.TrackNewCredentialCreated(user, credential); } @@ -838,9 +850,6 @@ await Auditing.SaveAuditRecordAsync(new UserAuditRecord( } user.Credentials.Add(credential); - - await Auditing.SaveAuditRecordAsync(new UserAuditRecord( - user, AuditedUserAction.AddCredential, credential)); } private static CredentialKind GetCredentialKind(string type) @@ -1024,15 +1033,20 @@ private async Task MigrateCredentials(User user, List creds, string await Auditing.SaveAuditRecordAsync(new UserAuditRecord(user, AuditedUserAction.RemoveCredential, toRemove)); // Now add one if there are no credentials left + Credential newCred = null; if (creds.Count == 0) { - var newCred = _credentialBuilder.CreatePasswordCredential(password); - await Auditing.SaveAuditRecordAsync(new UserAuditRecord(user, AuditedUserAction.AddCredential, newCred)); + newCred = _credentialBuilder.CreatePasswordCredential(password); user.Credentials.Add(newCred); } // Save changes, if any await Entities.SaveChangesAsync(); + + if (newCred != null) + { + await Auditing.SaveAuditRecordAsync(new UserAuditRecord(user, AuditedUserAction.AddCredential, newCred)); + } } } } \ No newline at end of file diff --git a/tests/NuGetGallery.Facts/Authentication/AuthenticationServiceFacts.cs b/tests/NuGetGallery.Facts/Authentication/AuthenticationServiceFacts.cs index 0817d1a5a4..0c3742d895 100644 --- a/tests/NuGetGallery.Facts/Authentication/AuthenticationServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Authentication/AuthenticationServiceFacts.cs @@ -15,6 +15,8 @@ using NuGetGallery.Auditing; using NuGetGallery.Authentication.Providers; using NuGetGallery.Authentication.Providers.MicrosoftAccount; +using NuGetGallery.Configuration; +using NuGetGallery.Diagnostics; using NuGetGallery.Framework; using NuGetGallery.Infrastructure.Authentication; using Xunit; @@ -2070,6 +2072,55 @@ public async Task WritesAuditRecordForTheNewCredential() ar.AffectedCredential.Length == 1 && ar.AffectedCredential[0].Type == cred.Type && ar.AffectedCredential[0].Identity == cred.Identity)); + + Assert.Equal( + new List { nameof(IEntitiesContext.SaveChangesAsync), nameof(IAuditingService.SaveAuditRecordAsync) }, + operations); + } + + [Fact] + public async Task WritesAuditRecordAfterDbCommit() + { + // Arrange + var entitiesContext = new Mock(); + var auditingService = new Mock(); + var credentialBuilder = new CredentialBuilder(); + + var authService = new AuthenticationService( + entitiesContext.Object, + Get(), + Get(), + auditingService.Object, + Enumerable.Empty(), + credentialBuilder, + Get(), + Get(), + Get(), + Get(), + Get()); + var operations = new List(); + + var fakes = Get(); + var user = fakes.CreateUser("test", credentialBuilder.CreatePasswordCredential(Fakes.Password)); + var cred = credentialBuilder.CreateExternalCredential("flarg", "glarb", "blarb"); + Mock + .Get(authService.Auditing) + .Setup(x => x.SaveAuditRecordAsync(It.IsAny())) + .Returns(Task.CompletedTask) + .Callback(() => operations.Add(nameof(IAuditingService.SaveAuditRecordAsync))); + Mock + .Get(authService.Entities) + .Setup(x => x.SaveChangesAsync()) + .ReturnsAsync(() => 0) + .Callback(() => operations.Add(nameof(IEntitiesContext.SaveChangesAsync))); + + // Act + await authService.AddCredential(user, cred); + + // Assert + Assert.Equal( + new List { nameof(IEntitiesContext.SaveChangesAsync), nameof(IAuditingService.SaveAuditRecordAsync) }, + operations); } } From 49994568b6b70f4f2d52ec64da1d3bc6d5701198 Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 9 Dec 2022 08:23:21 -0500 Subject: [PATCH 2/3] Fixed a line --- .../Authentication/AuthenticationServiceFacts.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/NuGetGallery.Facts/Authentication/AuthenticationServiceFacts.cs b/tests/NuGetGallery.Facts/Authentication/AuthenticationServiceFacts.cs index 0c3742d895..bce12636d9 100644 --- a/tests/NuGetGallery.Facts/Authentication/AuthenticationServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Authentication/AuthenticationServiceFacts.cs @@ -2072,10 +2072,6 @@ public async Task WritesAuditRecordForTheNewCredential() ar.AffectedCredential.Length == 1 && ar.AffectedCredential[0].Type == cred.Type && ar.AffectedCredential[0].Identity == cred.Identity)); - - Assert.Equal( - new List { nameof(IEntitiesContext.SaveChangesAsync), nameof(IAuditingService.SaveAuditRecordAsync) }, - operations); } [Fact] From 1afb42ac5e59d7263464ae4c006b7de6679d7ec4 Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 9 Dec 2022 11:49:02 -0500 Subject: [PATCH 3/3] Remove using --- .../Authentication/AuthenticationService.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/NuGetGallery.Services/Authentication/AuthenticationService.cs b/src/NuGetGallery.Services/Authentication/AuthenticationService.cs index 1dc093d0e6..057b3190fa 100644 --- a/src/NuGetGallery.Services/Authentication/AuthenticationService.cs +++ b/src/NuGetGallery.Services/Authentication/AuthenticationService.cs @@ -6,7 +6,6 @@ using System.Data.Entity; using System.Globalization; using System.Linq; -using System.Net; using System.Security.Claims; using System.Threading.Tasks; using System.Web.Mvc;