[Prefix] Add auditing for reserved namespaces #4940

shishirx34 · 2017-11-01T00:27:53Z

I will send out the Shim changes once I get a build out of this merge, since it requires the changes from NuGetGallery.Core for the new audit record types and actions. Addresses #4549 first part.

…ords

chenriksson · 2017-11-01T15:58:13Z

src/NuGetGallery.Core/Auditing/AuditedEntities/AuditedReservedNamespace.cs

+        public bool IsPrefix { get; }
+        public bool IsSharedNamespace { get; }
+
+        public AuditedReservedNamespace(ReservedNamespace reservedNamespace)


You should probably keep the default ctor in case we need it for deserialization.

We should use a consistent pattern across all audit entities... maybe like AuditedPackageRegistration does it (default ctor and static CreateFrom for populating from the EF entity)?

I didn't see consistency here, AuditedUserSecurityPolicy doesn't use CreateFrom uses the ctor to create audit record.

I don't mind making this to use CreateFrom instead.

chenriksson · 2017-11-01T16:03:39Z

src/NuGetGallery.Core/Auditing/AuditedPackageAction.cs

@@ -15,6 +15,7 @@ public enum AuditedPackageAction
        Edit,
        [Obsolete("Undo package edit functionality is being retired.")]
        UndoEdit,
-        Verify
+        Verify,
+        UploadFailedNamespaceConflict


We don't audit upload failures due to id/title conflict, so I don't think we should with namespace either. The only failures we currently audit are authentication related (package push attempt by non-owner, login failed).

This is as per @skofman1's comment in #4549

changed this to telemetry value rather than auditing.

chenriksson · 2017-11-01T16:24:21Z

src/NuGetGallery.Core/Auditing/AuditedPackageRegistrationAction.cs

-        RemoveOwner
+        RemoveOwner,
+        MarkVerified,
+        MarkUnverified


I don't think you need separate actions for MarkVerified/MarkUnverified. AuditedPackageRegistration.IsVerified will be logged when the action that caused verification is audited (i.e., package owner added, or push of new package).

True, will remove this.

On second thoughts, I think we should have these actions for AffectedRegistrations in the reserved namespace audit, the registration records are updated for marking verified/unverified actions, which cannot be said of using (Add/Remove)Owner see this

Good point. I was thinking the list of packages that became verified could be audited as part of the register/unregister namespace. This is probably better since the audit path will be more consistent for when packages become un-/verified.

If we want to see when a package became verified, regardless of how, do we just look under registration auditing (auditing/{package-id})?

chenriksson · 2017-11-01T16:25:53Z

src/NuGetGallery.Core/Auditing/AuditedReservedNamespaceAction.cs

+    public enum AuditedReservednamespaceAction
+    {
+        Allocate,
+        Deallocate,


Would rename to [Reserve/Unreserve]Namespace or [Add/Remove]NamespaceReservation, which seems more specific.

chenriksson · 2017-11-01T16:26:53Z

src/NuGetGallery.Core/Auditing/AuditedReservedNamespaceAction.cs

+
+namespace NuGetGallery.Auditing
+{
+    public enum AuditedReservednamespaceAction


Casing - should be AuditedReservedNamespaceAction

joelverhagen · 2017-11-01T19:14:40Z

src/NuGetGallery.Core/Auditing/AuditedEntities/AuditedPackageRegistration.cs


        public static AuditedPackageRegistration CreateFrom(PackageRegistration packageRegistration)
        {
            return new AuditedPackageRegistration
            {
                Id = packageRegistration.Id,
                DownloadCount = packageRegistration.DownloadCount,
-                Key = packageRegistration.Key
+                Key = packageRegistration.Key,
+                IsVerified = packageRegistration.IsVerified


nit trailing comma so that future diffs only change one line

Putting a trailing comma on the last property of an object seems weird to me. Ticks my OCD :)

joelverhagen · 2017-11-01T19:15:30Z

src/NuGetGallery.Core/Auditing/PackageAuditRecord.cs

@@ -30,6 +30,9 @@ public class PackageAuditRecord : AuditRecord<AuditedPackageAction>
            Reason = reason;
        }

+        public PackageAuditRecord(string id, string version, AuditedPackageAction action, string reason)
+            : this(id, version, "", null, null, action, reason) { }


name ambiguous parameters

shishirx34 · 2017-11-01T21:54:56Z

@chenriksson - addressed feedback.

chenriksson · 2017-11-02T12:41:57Z

src/NuGetGallery.Core/Auditing/ReservedNamespaceAuditRecord.cs

+                case AuditedReservedNamespaceAction.RemoveOwner:
+                    return AuditedPackageRegistrationAction.MarkUnverified;
+                default:
+                    return null;


This default case shouldn't happen... I would have it throw instead of complicating the code to work with nullables.

Will throw and add tests.

chenriksson · 2017-11-02T12:42:38Z

src/NuGetGallery.Core/Auditing/ReservedNamespaceAuditRecord.cs

+            {
+                AffectedOwner = username;
+
+                var registrationAction = GetPackageRegistrationAction(action);


No need to call GetPackageRegistrationAction if registrations is null/empty

good catch

chenriksson · 2017-11-02T12:48:39Z

src/NuGetGallery/Services/TelemetryService.cs

@@ -216,6 +203,30 @@ private static string GetApiKeyCreationDate(User user, IIdentity identity)
            return apiKey?.Created.ToString("O") ?? "N/A";
        }

+        private void TrackPackageForEvent(string eventValue, string packageId, string packageVersion, User user, IIdentity identity)


If you add AuthenticationMethod, would also match TrackPackagePushEvent. Maybe worth doing?

sure

chenriksson · 2017-11-02T12:50:13Z

tests/NuGetGallery.Core.Facts/Auditing/ReservedNamespaceAuditRecordTests.cs

+            Assert.Equal(prefix.IsPrefix, record.AffectedReservedNamespace.IsPrefix);
+            Assert.Equal(AuditedReservedNamespaceAction.AddOwner, record.Action);
+            Assert.Equal(registrationsList.Count, record.AffectedRegistrations.Length);
+            Assert.Equal(owner.Username, record.AffectedOwner);


nit: Arrange/Act/Assert comments would be useful in a long test like this

done

chenriksson · 2017-11-02T12:50:33Z

tests/NuGetGallery.Facts/Services/ReservedNamespaceServiceFacts.cs

@@ -2,12 +2,14 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

 using Moq;
-using NuGetGallery.TestUtils;
+using Xunit;


shishirx34 added 8 commits October 30, 2017 14:58

Add base audit for reserved namespace

d4e6166

Add audit records for namespace and package registration modified rec…

3ee01b1

…ords

update package registration audit record

08daf0c

Merge remote-tracking branch 'origin/dev' into prefix-audit

8c6dd90

Add tests for audits

e7d9b55

Add record audit tests

37223d9

Add audit for failed uploads

a349b1c

add audit for upload in api controller

6c422da

shishirx34 requested review from joelverhagen, agr, xavierdecoster, ryuyu, chenriksson, skofman1 and scottbommarito November 1, 2017 00:27

dnfclas added the cla-already-signed label Nov 1, 2017

chenriksson suggested changes Nov 1, 2017

View reviewed changes

joelverhagen reviewed Nov 1, 2017

View reviewed changes

shishirx34 added 3 commits November 1, 2017 13:14

Address feedback, refactor

0e56b0f

use createFrom

6da583c

add telemetry; refactor; remove unnecessary actions

56d27cd

chenriksson reviewed Nov 2, 2017

View reviewed changes

Address comments, nit fixes, more tests

1d932a1

chenriksson approved these changes Nov 2, 2017

View reviewed changes

shishirx34 merged commit ff2cbd9 into dev Nov 2, 2017

shishirx34 mentioned this pull request Nov 3, 2017

Deployment 2017-11-30 #4950

Closed

10 tasks

joelverhagen deleted the prefix-audit branch February 26, 2018 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Prefix] Add auditing for reserved namespaces #4940

[Prefix] Add auditing for reserved namespaces #4940

shishirx34 commented Nov 1, 2017

chenriksson Nov 1, 2017

shishirx34 Nov 1, 2017

chenriksson Nov 1, 2017

shishirx34 Nov 1, 2017

shishirx34 Nov 1, 2017

chenriksson Nov 1, 2017

shishirx34 Nov 1, 2017

shishirx34 Nov 1, 2017

chenriksson Nov 2, 2017

chenriksson Nov 1, 2017

chenriksson Nov 1, 2017

joelverhagen Nov 1, 2017

shishirx34 Nov 1, 2017

joelverhagen Nov 1, 2017

shishirx34 commented Nov 1, 2017

chenriksson Nov 2, 2017

shishirx34 Nov 2, 2017

chenriksson Nov 2, 2017 •

edited by shishirx34

Loading

chenriksson Nov 2, 2017 •

edited by shishirx34

Loading

chenriksson Nov 2, 2017 •

edited by shishirx34

Loading

chenriksson Nov 2, 2017

[Prefix] Add auditing for reserved namespaces #4940

[Prefix] Add auditing for reserved namespaces #4940

Conversation

shishirx34 commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shishirx34 commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenriksson Nov 2, 2017 • edited by shishirx34 Loading

Choose a reason for hiding this comment

chenriksson Nov 2, 2017 • edited by shishirx34 Loading

Choose a reason for hiding this comment

chenriksson Nov 2, 2017 • edited by shishirx34 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenriksson Nov 2, 2017 •

edited by shishirx34

Loading

chenriksson Nov 2, 2017 •

edited by shishirx34

Loading

chenriksson Nov 2, 2017 •

edited by shishirx34

Loading