Audit for DeleteAccount #5297
Audit for DeleteAccount #5297
Conversation
cristinamanum
requested review from
shishirx34,
agr,
chenriksson,
joelverhagen,
ryuyu,
scottbommarito,
skofman1 and
loic-sharma
| public enum AuditedDeleteAccountAction | ||
| { | ||
| DeleteAccount, | ||
| RequestAccountDeletion |
There was a problem hiding this comment.
RequestAccountDeletion seems like it should go on AuditedUserAction, since it's an action that the user is taking.
Since DeleteAccount is an admin action, then it seems like maybe we should introduce an AuditedAdminAction for it? It could be reused by any admin-level actions that we want to audit.
There was a problem hiding this comment.
I thought about it however I decided to group the actions on scenario as the other Audit cases.
|
|
||
| public string AdminUserName; | ||
|
|
||
| public ActionStatus Status; |
There was a problem hiding this comment.
Status w/o reason isn't too useful. I would remove.
Also, we don't track status w/ other audit operations. Generally we audit first, then attempt the SQL/Storage/etc action. It could lead to audits that don't match the data, but that's better than missing audit logs.
There was a problem hiding this comment.
I think the this depends on scenario. In this case i think that it is useful to know if an user tried to request delete account and it was not successful.
There was a problem hiding this comment.
I agree with @chenriksson that a failure without a reason is not great. However, I see that the support request service just returns null on failure...
| TempData["RequestFailedMessage"] = Strings.AccountDelete_CreateSupportRequestFails; | ||
| return RedirectToAction("DeleteRequest"); | ||
| } | ||
| _messageService.SendAccountDeleteNotice(user.ToMailAddress(), user.Username); | ||
|
|
||
| await _auditingService.SaveAuditRecordAsync(new DeleteAccountAuditRecord(userName: user.Username, |
There was a problem hiding this comment.
Could we move all auditing to the DeleteAccountService, to avoid cluttering the controller? I would also move the bulk of the RequestAccountDeletion logic (and SupportRequestService) there as well, which should make this possible.
There was a problem hiding this comment.
Moved the auditing out of the Controller.
| var auditingService = new Mock<IAuditingService>(); | ||
| return auditingService; | ||
| } | ||
|
|
There was a problem hiding this comment.
Add tests to verify that the delete operations write audit logs? There are similar test cases for packages, I think.
| @@ -24,6 +24,7 @@ public class DeleteAccountService : IDeleteAccountService | |||
| private readonly AuthenticationService _authService; | |||
| private readonly IEntityRepository<User> _userRepository; | |||
| private readonly ISupportRequestService _supportRequestService; | |||
| private IAuditingService _auditingService { get; } | |||
There was a problem hiding this comment.
_ prefix is non idiomatic for properties
There was a problem hiding this comment.
Updated for both comments.
| await _auditingService.SaveAuditRecordAsync(new DeleteAccountAuditRecord(userName: userToBeDeleted.Username, | ||
| status: DeleteAccountAuditRecord.ActionStatus.Success, | ||
| action: AuditedDeleteAccountAction.DeleteAccount, | ||
| adminUserName: admin.Username)); |
There was a problem hiding this comment.
inconsistent casing of Username.
There was a problem hiding this comment.
(I think Username is preferred over UserName)
There was a problem hiding this comment.
Updated to user Username.
| Assert.Equal("NuGetGallery.Auditing.UserAuditRecord", actualAuditRecordTypeNames[4]); | ||
| Assert.Equal("NuGetGallery.Auditing.UserSecurityPolicyAuditRecord", actualAuditRecordTypeNames[5]); | ||
|
|
||
| Assert.Equal("NuGetGallery.Auditing.DeleteAccountAuditRecord", actualAuditRecordTypeNames[0]); |
There was a problem hiding this comment.
Can we refactor this test to have an expected list of strings so that the count is implied? And we don't need to write the indexes ourselves?
| @@ -0,0 +1,36 @@ | |||
| // 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. | |||
| namespace NuGetGallery.Auditing | |||
There was a problem hiding this comment.
nit: blank line between copyright and namespace
Add audit for DeleteAccount. Changes in the IFX shim will be in a different PR.