Add telemetry for auto-add package owner feature (#6374)

xavierdecoster · web-flow · commit c4ad812356ab · 2018-09-07T19:20:40.000+02:00
diff --git a/src/NuGetGallery/Security/MicrosoftTeamSubscription.cs b/src/NuGetGallery/Security/MicrosoftTeamSubscription.cs
@@ -13,6 +13,23 @@ public class MicrosoftTeamSubscription : IUserSecurityPolicySubscription
 
         internal const string MicrosoftUsername = "Microsoft";
         internal const string Name = "MicrosoftTeamSubscription";
+        internal static readonly string[] AllowedCopyrightNotices = new string[]
+        {
+            "(c) Microsoft Corporation. All rights reserved.",
+            "&#169; Microsoft Corporation. All rights reserved.",
+            "© Microsoft Corporation. All rights reserved.",
+            "© Microsoft Corporation. Tüm hakları saklıdır.",
+            "© Microsoft Corporation. Todos os direitos reservados.",
+            "© Microsoft Corporation. Alle Rechte vorbehalten.",
+            "© Microsoft Corporation. 保留所有权利.",
+            "© Microsoft Corporation. Všechna práva vyhrazena.",
+            "© Microsoft Corporation. Reservados todos los derechos.",
+            "© Microsoft Corporation. Wszelkie prawa zastrzeżone.",
+            "© Microsoft Corporation. Tous droits réservés.",
+            "© Microsoft Corporation. 著作權所有，並保留一切權利。",
+            "© Microsoft Corporation. Tutti i diritti sono riservati.",
+            "© Корпорация Майкрософт (Microsoft Corporation). Все права защищены."
+        };
 
         public string SubscriptionName => Name;
 
@@ -40,24 +57,9 @@ private static List<UserSecurityPolicy> InitializePoliciesList()
             return new List<UserSecurityPolicy>()
             {
                 RequirePackageMetadataCompliancePolicy.CreatePolicy(
-                    Name, 
+                    Name,
                     MicrosoftUsername,
-                    allowedCopyrightNotices: new string[] 
-                    {
-                        "(c) Microsoft Corporation. All rights reserved.",
-                        "© Microsoft Corporation. All rights reserved.",
-                        "© Microsoft Corporation. Tüm hakları saklıdır.",
-                        "© Microsoft Corporation. Todos os direitos reservados.",
-                        "© Microsoft Corporation. Alle Rechte vorbehalten.",
-                        "© Microsoft Corporation. 保留所有权利.",
-                        "© Microsoft Corporation. Všechna práva vyhrazena.",
-                        "© Microsoft Corporation. Reservados todos los derechos.",
-                        "© Microsoft Corporation. Wszelkie prawa zastrzeżone.",
-                        "© Microsoft Corporation. Tous droits réservés.",
-                        "© Microsoft Corporation. 著作權所有，並保留一切權利。",
-                        "© Microsoft Corporation. Tutti i diritti sono riservati.",
-                        "© Корпорация Майкрософт (Microsoft Corporation). Все права защищены."
-                    },
+                    allowedCopyrightNotices: AllowedCopyrightNotices,
                     isLicenseUrlRequired: true,
                     isProjectUrlRequired: true,
                     errorMessageFormat: Strings.SecurityPolicy_RequireMicrosoftPackageMetadataComplianceForPush)
diff --git a/src/NuGetGallery/Security/PackageSecurityPolicyEvaluationContext.cs b/src/NuGetGallery/Security/PackageSecurityPolicyEvaluationContext.cs
@@ -12,19 +12,22 @@ public class PackageSecurityPolicyEvaluationContext : UserSecurityPolicyEvaluati
         public PackageSecurityPolicyEvaluationContext(
             IUserService userService,
             IPackageOwnershipManagementService packageOwnershipManagementService,
+            ITelemetryService telemetryService,
             IEnumerable<UserSecurityPolicy> policies,
             Package package,
             HttpContextBase httpContext)
             : base(policies, httpContext)
         {
             Package = package ?? throw new ArgumentNullException(nameof(package));
             UserService = userService ?? throw new ArgumentNullException(nameof(userService));
+            TelemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService));
             PackageOwnershipManagementService = packageOwnershipManagementService ?? throw new ArgumentNullException(nameof(packageOwnershipManagementService));
         }
 
         public PackageSecurityPolicyEvaluationContext(
             IUserService userService,
             IPackageOwnershipManagementService packageOwnershipManagementService,
+            ITelemetryService telemetryService,
             IEnumerable<UserSecurityPolicy> policies,
             Package package,
             User sourceAccount,
@@ -34,6 +37,7 @@ public PackageSecurityPolicyEvaluationContext(
         {
             Package = package ?? throw new ArgumentNullException(nameof(package));
             UserService = userService ?? throw new ArgumentNullException(nameof(userService));
+            TelemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService));
             PackageOwnershipManagementService = packageOwnershipManagementService ?? throw new ArgumentNullException(nameof(packageOwnershipManagementService));
         }
 
@@ -44,6 +48,8 @@ public PackageSecurityPolicyEvaluationContext(
 
         public IUserService UserService { get; }
 
+        public ITelemetryService TelemetryService { get; }
+
         public IPackageOwnershipManagementService PackageOwnershipManagementService { get; }
     }
 }
diff --git a/src/NuGetGallery/Security/RequirePackageMetadataCompliancePolicy.cs b/src/NuGetGallery/Security/RequirePackageMetadataCompliancePolicy.cs
@@ -49,6 +49,11 @@ public override async Task<SecurityPolicyResult> EvaluateAsync(PackageSecurityPo
             // Evaluate package metadata validations
             if (!IsPackageMetadataCompliant(context.Package, state, out var complianceFailures))
             {
+                context.TelemetryService.TrackPackageMetadataComplianceError(
+                    context.Package.Id,
+                    context.Package.NormalizedVersion,
+                    complianceFailures);
+
                 // Package policy not met.
                 return SecurityPolicyResult.CreateErrorResult(
                     string.Format(
@@ -63,12 +68,21 @@ public override async Task<SecurityPolicyResult> EvaluateAsync(PackageSecurityPo
                 // This will also mark the package as verified if the prefix has been reserved by the co-owner.
                 // The entities context is committed later as a single atomic transaction (see PackageUploadService).
                 await context.PackageOwnershipManagementService.AddPackageOwnerAsync(context.Package.PackageRegistration, requiredCoOwner, commitChanges: false);
+
+                context.TelemetryService.TrackPackageOwnershipAutomaticallyAdded(
+                    context.Package.Id,
+                    context.Package.NormalizedVersion);
             }
 
             // If the PackageRegistration is not marked as verified,
             // the account pushing the package has not registered the prefix yet.
             if (!context.Package.PackageRegistration.IsVerified)
             {
+                context.TelemetryService.TrackPackageMetadataComplianceWarning(
+                    context.Package.Id,
+                    context.Package.NormalizedVersion,
+                    complianceWarnings: new[] { Strings.SecurityPolicy_RequirePackagePrefixReserved });
+
                 return SecurityPolicyResult.CreateWarningResult(Strings.SecurityPolicy_RequirePackagePrefixReserved);
             }
 
@@ -77,8 +91,8 @@ public override async Task<SecurityPolicyResult> EvaluateAsync(PackageSecurityPo
         }
 
         public static UserSecurityPolicy CreatePolicy(
-            string subscription, 
-            string requiredCoOwnerUsername, 
+            string subscription,
+            string requiredCoOwnerUsername,
             string[] allowedCopyrightNotices,
             bool isLicenseUrlRequired,
             bool isProjectUrlRequired,
@@ -153,7 +167,7 @@ public class State
 
             [JsonProperty("licUrlReq")]
             public bool IsLicenseUrlRequired { get; set; }
-            
+
             [JsonProperty("projUrlReq")]
             public bool IsProjectUrlRequired { get; set; }
 
diff --git a/src/NuGetGallery/Security/SecurityPolicyService.cs b/src/NuGetGallery/Security/SecurityPolicyService.cs
@@ -32,6 +32,7 @@ private static readonly RequireOrganizationTenantPolicy _organizationTenantPolic
 
         private readonly Lazy<IUserService> _userService;
         private readonly Lazy<IPackageOwnershipManagementService> _packageOwnershipManagementService;
+        private readonly ITelemetryService _telemetryService;
 
         protected IEntitiesContext EntitiesContext { get; set; }
 
@@ -56,6 +57,7 @@ public SecurityPolicyService(
             IAppConfiguration configuration,
             Lazy<IUserService> userService,
             Lazy<IPackageOwnershipManagementService> packageOwnershipManagementService,
+            ITelemetryService telemetryService,
             MicrosoftTeamSubscription microsoftTeamSubscription = null)
         {
             EntitiesContext = entitiesContext ?? throw new ArgumentNullException(nameof(entitiesContext));
@@ -72,6 +74,7 @@ public SecurityPolicyService(
             MicrosoftTeamSubscription = microsoftTeamSubscription;
             _packageOwnershipManagementService = packageOwnershipManagementService ?? throw new ArgumentNullException(nameof(packageOwnershipManagementService));
             _userService = userService ?? throw new ArgumentNullException(nameof(userService));
+            _telemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService));
         }
 
         /// <summary>
@@ -204,6 +207,7 @@ private async Task<SecurityPolicyResult> EvaluatePackagePoliciesInternalAsync(
                     var context = new PackageSecurityPolicyEvaluationContext(
                         _userService.Value,
                         _packageOwnershipManagementService.Value,
+                        _telemetryService,
                         foundPolicies,
                         package,
                         sourceAccount,
diff --git a/src/NuGetGallery/Services/ITelemetryService.cs b/src/NuGetGallery/Services/ITelemetryService.cs
@@ -51,6 +51,10 @@ public interface ITelemetryService
         /// </summary>
         void TrackUserPackageDeleteChecked(UserPackageDeleteEvent details, UserPackageDeleteOutcome outcome);
 
+        void TrackPackageMetadataComplianceError(string packageId, string packageVersion, IEnumerable<string> complianceFailures);
+
+        void TrackPackageMetadataComplianceWarning(string packageId, string packageVersion, IEnumerable<string> complianceWarnings);
+
         /// <summary>
         /// A telemetry event emitted when a user package delete is executed.
         /// </summary>
@@ -64,6 +68,13 @@ public interface ITelemetryService
         /// or empty.</exception>
         void TrackCertificateAdded(string thumbprint);
 
+        /// <summary>
+        /// A telemetry event emitted when a package owner was automatically added to a package registration.
+        /// </summary>
+        /// <param name="packageId">The package registration id.</param>
+        /// <param name="packageVersion">The normalized package version.</param>
+        void TrackPackageOwnershipAutomaticallyAdded(string packageId, string packageVersion);
+
         /// <summary>
         /// A telemetry event emitted when a certificate is activated for an account.
         /// </summary>
diff --git a/src/NuGetGallery/Services/TelemetryService.cs b/src/NuGetGallery/Services/TelemetryService.cs
@@ -6,6 +6,7 @@
 using System.Linq;
 using System.Security.Principal;
 using System.Web;
+using Newtonsoft.Json;
 using NuGet.Versioning;
 using NuGetGallery.Authentication;
 using NuGetGallery.Diagnostics;
@@ -51,11 +52,20 @@ internal class Events
             public const string SymbolPackagePush = "SymbolPackagePush";
             public const string SymbolPackagePushFailure = "SymbolPackagePushFailure";
             public const string SymbolPackageGalleryValidation = "SymbolPackageGalleryValidation";
+            public const string PackageMetadataComplianceError = "PackageMetadataComplianceError";
+            public const string PackageMetadataComplianceWarning = "PackageMetadataComplianceWarning";
+            public const string PackageOwnershipAutomaticallyAdded = "PackageOwnershipAutomaticallyAdded";
         }
 
         private IDiagnosticsSource _diagnosticsSource;
         private ITelemetryClient _telemetryClient;
 
+        private readonly JsonSerializerSettings _defaultJsonSerializerSettings = new JsonSerializerSettings
+        {
+            ReferenceLoopHandling = ReferenceLoopHandling.Ignore,
+            Formatting = Formatting.None
+        };
+
         // ODataQueryFilter properties
         public const string CallContext = "CallContext";
         public const string IsEnabled = "IsEnabled";
@@ -116,6 +126,10 @@ internal class Events
         public const string CreatedDateForAccountToBeDeleted = "CreatedDateForAccountToBeDeleted";
         public const string AccountDeleteSucceeded = "AccountDeleteSucceeded";
 
+        // Package metadata compliance properties
+        public const string ComplianceFailures = "ComplianceFailures";
+        public const string ComplianceWarnings = "ComplianceWarnings";
+
         public const string ValueUnknown = "Unknown";
 
         public TelemetryService(IDiagnosticsService diagnosticsService, ITelemetryClient telemetryClient = null)
@@ -313,6 +327,36 @@ public void TrackPackageRevalidate(Package package)
             TrackMetricForPackage(Events.PackageRevalidate, package);
         }
 
+        public void TrackPackageMetadataComplianceError(string packageId, string packageVersion, IEnumerable<string> complianceFailures)
+        {
+            TrackMetricForPackage(
+                Events.PackageMetadataComplianceError, 
+                packageId, 
+                packageVersion,
+                properties => {
+                    properties.Add(ComplianceFailures, JsonConvert.SerializeObject(complianceFailures, _defaultJsonSerializerSettings));
+                });
+        }
+
+        public void TrackPackageMetadataComplianceWarning(string packageId, string packageVersion, IEnumerable<string> complianceWarnings)
+        {
+            TrackMetricForPackage(
+                Events.PackageMetadataComplianceWarning,
+                packageId,
+                packageVersion,
+                properties => {
+                    properties.Add(ComplianceWarnings, JsonConvert.SerializeObject(complianceWarnings, _defaultJsonSerializerSettings));
+                });
+        }
+
+        public void TrackPackageOwnershipAutomaticallyAdded(string packageId, string packageVersion)
+        {
+            TrackMetricForPackage(
+                Events.PackageOwnershipAutomaticallyAdded, 
+                packageId, 
+                packageVersion);
+        }
+
         public void TrackCertificateAdded(string thumbprint)
         {
             TrackMetricForCertificateActivity(Events.CertificateAdded, thumbprint);
diff --git a/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs
@@ -1580,13 +1580,16 @@ private static ISecurityPolicyService CreateSecurityPolicyService(
                     var configurationMock = new Mock<IAppConfiguration>(MockBehavior.Strict);
                     configurationMock.SetupGet(m => m.EnforceDefaultSecurityPolicies).Returns(false);
 
+                    var telemetryServiceMock = new Mock<ITelemetryService>();
+
                     return new SecurityPolicyService(
                         entitiesContext,
                         auditing,
                         diagnostics,
                         configurationMock.Object,
                         userServiceFactory,
-                        packageOwnershipManagementServiceFactory);
+                        packageOwnershipManagementServiceFactory,
+                        telemetryServiceMock.Object);
                 }
 
                 private static Mock<TestPackageReader> CreatePackage(
diff --git a/tests/NuGetGallery.Facts/Security/MicrosoftTeamSubscriptionFacts.cs b/tests/NuGetGallery.Facts/Security/MicrosoftTeamSubscriptionFacts.cs
@@ -26,6 +26,7 @@ public void Policies_ReturnsMicrosoftTeamSubscriptionPolicies()
                 "\"copy\":" +
                 "[" +
                 "\"(c) Microsoft Corporation. All rights reserved.\"," +
+                "\"&#169; Microsoft Corporation. All rights reserved.\"," +
                 "\"© Microsoft Corporation. All rights reserved.\"," +
                 "\"© Microsoft Corporation. Tüm hakları saklıdır.\"," +
                 "\"© Microsoft Corporation. Todos os direitos reservados.\"," +
diff --git a/tests/NuGetGallery.Facts/Security/RequirePackageMetadataCompliancePolicyFacts.cs b/tests/NuGetGallery.Facts/Security/RequirePackageMetadataCompliancePolicyFacts.cs
diff --git a/tests/NuGetGallery.Facts/Security/SecurityPolicyServiceFacts.cs b/tests/NuGetGallery.Facts/Security/SecurityPolicyServiceFacts.cs
diff --git a/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs b/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs

Original file line number	Diff line number	Diff line change
`@@ -12,19 +12,22 @@ public class PackageSecurityPolicyEvaluationContext : UserSecurityPolicyEvaluati`
`12`	`12`	`public PackageSecurityPolicyEvaluationContext(`
`13`	`13`	`IUserService userService,`
`14`	`14`	`IPackageOwnershipManagementService packageOwnershipManagementService,`
	`15`	`+ ITelemetryService telemetryService,`
`15`	`16`	`IEnumerable<UserSecurityPolicy> policies,`
`16`	`17`	`Package package,`
`17`	`18`	`HttpContextBase httpContext)`
`18`	`19`	`: base(policies, httpContext)`
`19`	`20`	`{`
`20`	`21`	`Package = package ?? throw new ArgumentNullException(nameof(package));`
`21`	`22`	`UserService = userService ?? throw new ArgumentNullException(nameof(userService));`
	`23`	`+ TelemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService));`
`22`	`24`	`PackageOwnershipManagementService = packageOwnershipManagementService ?? throw new ArgumentNullException(nameof(packageOwnershipManagementService));`
`23`	`25`	`}`
`24`	`26`
`25`	`27`	`public PackageSecurityPolicyEvaluationContext(`
`26`	`28`	`IUserService userService,`
`27`	`29`	`IPackageOwnershipManagementService packageOwnershipManagementService,`
	`30`	`+ ITelemetryService telemetryService,`
`28`	`31`	`IEnumerable<UserSecurityPolicy> policies,`
`29`	`32`	`Package package,`
`30`	`33`	`User sourceAccount,`
`@@ -34,6 +37,7 @@ public PackageSecurityPolicyEvaluationContext(`
`34`	`37`	`{`
`35`	`38`	`Package = package ?? throw new ArgumentNullException(nameof(package));`
`36`	`39`	`UserService = userService ?? throw new ArgumentNullException(nameof(userService));`
	`40`	`+ TelemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService));`
`37`	`41`	`PackageOwnershipManagementService = packageOwnershipManagementService ?? throw new ArgumentNullException(nameof(packageOwnershipManagementService));`
`38`	`42`	`}`
`39`	`43`
`@@ -44,6 +48,8 @@ public PackageSecurityPolicyEvaluationContext(`
`44`	`48`
`45`	`49`	`public IUserService UserService { get; }`
`46`	`50`
	`51`	`+ public ITelemetryService TelemetryService { get; }`
	`52`	`+`
`47`	`53`	`public IPackageOwnershipManagementService PackageOwnershipManagementService { get; }`
`48`	`54`	`}`
`49`	`55`	`}`