From 4cbbd0773774fd22655f53cbc3257b25a5fa3fb5 Mon Sep 17 00:00:00 2001 From: Christy Henriksson Date: Wed, 15 Mar 2017 18:33:52 -0700 Subject: [PATCH 1/3] Temp keys telemetry --- src/NuGetGallery/Constants.cs | 1 + src/NuGetGallery/Controllers/ApiController.cs | 15 +++-- .../Controllers/PackagesController.cs | 2 +- src/NuGetGallery/ExtensionMethods.cs | 20 +++++-- .../Services/ITelemetryService.cs | 6 +- src/NuGetGallery/Services/TelemetryService.cs | 56 ++++++++++++++++--- .../Controllers/ApiControllerFacts.cs | 29 +++++++++- .../Controllers/PackagesControllerFacts.cs | 2 +- 8 files changed, 108 insertions(+), 23 deletions(-) diff --git a/src/NuGetGallery/Constants.cs b/src/NuGetGallery/Constants.cs index 0083223d78..3cd3044230 100644 --- a/src/NuGetGallery/Constants.cs +++ b/src/NuGetGallery/Constants.cs @@ -52,6 +52,7 @@ public static class Constants public const string UrlValidationErrorMessage = "This doesn't appear to be a valid HTTP/HTTPS URL"; internal const string ApiKeyHeaderName = "X-NuGet-ApiKey"; + internal const string ClientVersionHeaderName = "X-NuGet-Client-Version"; internal const string WarningHeaderName = "X-NuGet-Warning"; public static readonly string ReturnUrlParameterName = "ReturnUrl"; diff --git a/src/NuGetGallery/Controllers/ApiController.cs b/src/NuGetGallery/Controllers/ApiController.cs index 434d3c68dc..0707cb7d63 100644 --- a/src/NuGetGallery/Controllers/ApiController.cs +++ b/src/NuGetGallery/Controllers/ApiController.cs @@ -219,7 +219,10 @@ public async virtual Task CreatePackageVerificationKeyAsync(string // validation until the VerifyPackageKey call. var credential = CredentialBuilder.CreatePackageVerificationApiKey(id); - await AuthenticationService.AddCredential(GetCurrentUser(), credential); + var user = GetCurrentUser(); + await AuthenticationService.AddCredential(user, credential); + + TelemetryService.TrackSymbolsPushEvent(id, version, user, User.Identity); return Json(new { @@ -245,11 +248,13 @@ public async virtual Task VerifyPackageKeyAsync(string id, string { await AuthenticationService.RemoveCredential(user, credential); } + + TelemetryService.TrackSymbolsPushCallbackEvent(id, version, user, User.Identity, result?.StatusCode ?? 200); - return result; + return (ActionResult)result ?? new EmptyResult(); } - private ActionResult VerifyPackageKeyInternal(User user, Credential credential, string id, string version) + private HttpStatusCodeWithBodyResult VerifyPackageKeyInternal(User user, Credential credential, string id, string version) { // Verify that the user has permission to push for the specific Id \ version combination. var package = PackageService.FindPackageByIdAndVersion(id, version); @@ -281,7 +286,7 @@ private ActionResult VerifyPackageKeyInternal(User user, Credential credential, } } - return new EmptyResult(); + return null; } [HttpPut] @@ -483,7 +488,7 @@ await AuditingService.SaveAuditRecordAsync( Url.Action("ReportMyPackage", "Packages", routeValues: new { id = package.PackageRegistration.Id, version = package.Version }, protocol: Request.Url.Scheme), Url.Action("Account", "Users", routeValues: null, protocol: Request.Url.Scheme)); - TelemetryService.TrackPackagePushEvent(user, User.Identity); + TelemetryService.TrackPackagePushEvent(package, user, User.Identity); return new HttpStatusCodeResult(HttpStatusCode.Created); } diff --git a/src/NuGetGallery/Controllers/PackagesController.cs b/src/NuGetGallery/Controllers/PackagesController.cs index 007ae7ac17..3e07e669a3 100644 --- a/src/NuGetGallery/Controllers/PackagesController.cs +++ b/src/NuGetGallery/Controllers/PackagesController.cs @@ -1224,7 +1224,7 @@ await _auditingService.SaveAuditRecordAsync( // delete the uploaded binary in the Uploads container await _uploadFileService.DeleteUploadFileAsync(currentUser.Key); - _telemetryService.TrackPackagePushEvent(currentUser, User.Identity); + _telemetryService.TrackPackagePushEvent(package, currentUser, User.Identity); TempData["Message"] = String.Format( CultureInfo.CurrentCulture, Strings.SuccessfullyUploadedPackage, package.PackageRegistration.Id, package.Version); diff --git a/src/NuGetGallery/ExtensionMethods.cs b/src/NuGetGallery/ExtensionMethods.cs index e48a92fea5..9592895e7a 100644 --- a/src/NuGetGallery/ExtensionMethods.cs +++ b/src/NuGetGallery/ExtensionMethods.cs @@ -389,20 +389,28 @@ public static string GetAuthenticationType(this IIdentity self) return identity?.GetClaimOrDefault(ClaimTypes.AuthenticationMethod); } - public static bool IsScopedAuthentication(this IIdentity self) + private static string GetScopeClaim(this IIdentity self) { var identity = self as ClaimsIdentity; - if (identity == null) - { - return false; - } + return identity?.GetClaimOrDefault(NuGetClaims.Scope); + } - var scopeClaim = identity.GetClaimOrDefault(NuGetClaims.Scope); + public static bool IsScopedAuthentication(this IIdentity self) + { + var scopeClaim = self.GetScopeClaim(); return !ScopeEvaluator.IsEmptyScopeClaim(scopeClaim); } + public static bool HasVerifyScope(this IIdentity self) + { + var scopeClaim = self.GetScopeClaim(); + + return ScopeEvaluator.ScopeClaimsAllowsActionForSubject(scopeClaim, null, + new [] { NuGetScopes.PackageVerify }); + } + // This is a method because the first call will perform a database call /// /// Get the current user, from the database, or if someone in this request has already diff --git a/src/NuGetGallery/Services/ITelemetryService.cs b/src/NuGetGallery/Services/ITelemetryService.cs index 5c6bf586fd..5bd76bba55 100644 --- a/src/NuGetGallery/Services/ITelemetryService.cs +++ b/src/NuGetGallery/Services/ITelemetryService.cs @@ -9,6 +9,10 @@ public interface ITelemetryService { void TrackODataQueryFilterEvent(string callContext, bool isEnabled, bool isAllowed, string queryPattern); - void TrackPackagePushEvent(User user, IIdentity identity); + void TrackPackagePushEvent(Package package, User user, IIdentity identity); + + void TrackSymbolsPushEvent(string packageId, string packageVersion, User user, IIdentity identity); + + void TrackSymbolsPushCallbackEvent(string packageId, string packageVersion, User user, IIdentity identity, int statusCode); } } \ No newline at end of file diff --git a/src/NuGetGallery/Services/TelemetryService.cs b/src/NuGetGallery/Services/TelemetryService.cs index 44af1f30ff..f5061f5cbe 100644 --- a/src/NuGetGallery/Services/TelemetryService.cs +++ b/src/NuGetGallery/Services/TelemetryService.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Security.Principal; +using System.Web; namespace NuGetGallery { @@ -16,11 +17,20 @@ public class TelemetryService : ITelemetryService public const string IsAllowed = "IsAllowed"; public const string QueryPattern = "QueryPattern"; - // Package push - public const string PackagePush = "PackagePush"; - public const string AuthenticatinMethod = "AuthenticationMethod"; + // Package events + public const string PackagePushEvent = "PackagePush"; + public const string SymbolsPushEvent = "SymbolsPush"; + public const string SymbolsPushCallbackEvent = "SymbolsPushCallback"; + + // Package event properties + public const string AuthenticationMethod = "AuthenticationMethod"; public const string AccountCreationDate = "AccountCreationDate"; + public const string ClientVersion = "ClientVersion"; public const string IsScoped = "IsScoped"; + public const string HasVerifyScope = "HasVerifyScope"; + public const string PackageId = "PackageId"; + public const string PackageVersion = "PackageVersion"; + public const string SymbolsStatusCode = "SymbolsStatusCode"; public void TrackODataQueryFilterEvent(string callContext, bool isEnabled, bool isAllowed, string queryPattern) { @@ -35,7 +45,29 @@ public void TrackODataQueryFilterEvent(string callContext, bool isEnabled, bool Telemetry.TrackEvent(ODataQueryFilter, telemetryProperties, metrics: null); } - public void TrackPackagePushEvent(User user, IIdentity identity) + public void TrackPackagePushEvent(Package package, User user, IIdentity identity) + { + if (package == null) + { + throw new ArgumentNullException(nameof(package)); + } + + TrackPackageEvent(PackagePushEvent, package.PackageRegistration.Id, package.NormalizedVersion, user, identity); + } + + public void TrackSymbolsPushEvent(string packageId, string packageVersion, User user, IIdentity identity) + { + TrackPackageEvent(SymbolsPushEvent, packageId, packageVersion, user, identity); + } + + public void TrackSymbolsPushCallbackEvent(string packageId, string packageVersion, User user, IIdentity identity, int statusCode) + { + TrackPackageEvent(SymbolsPushCallbackEvent, packageId, packageVersion, user, identity, + properties => properties.Add(SymbolsStatusCode, statusCode.ToString())); + } + + private void TrackPackageEvent(string eventName, string packageId, string packageVersion, User user, IIdentity identity, + Action> addCustomProperties = null) { if (user == null) { @@ -47,15 +79,23 @@ public void TrackPackagePushEvent(User user, IIdentity identity) throw new ArgumentNullException(nameof(identity)); } - string authenticationMethod = identity.GetAuthenticationType(); - bool isScoped = identity.IsScopedAuthentication(); + var authenticationMethod = identity.GetAuthenticationType(); + var isScoped = identity.IsScopedAuthentication(); + var hasVerifyScope = isScoped && identity.HasVerifyScope(); + var clientVersion = HttpContext.Current?.Request?.Headers[Constants.ClientVersionHeaderName]; var telemetryProperties = new Dictionary(); - telemetryProperties.Add(AuthenticatinMethod, authenticationMethod); + telemetryProperties.Add(ClientVersion, clientVersion); + telemetryProperties.Add(PackageId, packageId); + telemetryProperties.Add(PackageVersion, packageVersion); + telemetryProperties.Add(AuthenticationMethod, authenticationMethod); telemetryProperties.Add(AccountCreationDate, user.CreatedUtc != null ? user.CreatedUtc.Value.ToString("d") : "N/A"); telemetryProperties.Add(IsScoped, isScoped.ToString()); + telemetryProperties.Add(HasVerifyScope, hasVerifyScope.ToString()); + + addCustomProperties?.Invoke(telemetryProperties); - Telemetry.TrackEvent(PackagePush, telemetryProperties, metrics: null); + Telemetry.TrackEvent(eventName, telemetryProperties, metrics: null); } } } \ No newline at end of file diff --git a/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs index bc45af459c..d575200e3d 100644 --- a/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs @@ -631,7 +631,7 @@ public async Task WillSendPackagePushEvent() await controller.CreatePackagePut(); - controller.MockTelemetryService.Verify(x => x.TrackPackagePushEvent(user, controller.OwinContext.Request.User.Identity), Times.Once); + controller.MockTelemetryService.Verify(x => x.TrackPackagePushEvent(It.IsAny(), user, controller.OwinContext.Request.User.Identity), Times.Once); } } @@ -1059,6 +1059,9 @@ public async void CreatePackageKeyReturnsPackageVerificationKey(string scope) Assert.True(DateTime.TryParse(json.Expires, out expires)); controller.MockAuthenticationService.Verify(s => s.AddCredential(It.IsAny(), It.IsAny()), Times.Once); + + controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushEvent("foo", "1.0.0", + It.IsAny(), controller.OwinContext.Request.User.Identity), Times.Once); } } @@ -1083,6 +1086,9 @@ public async void VerifyPackageKeyReturns404IfPackageDoesNotExist_ApiKeyV2(strin String.Format(CultureInfo.CurrentCulture, Strings.PackageWithIdAndVersionNotFound, "foo", "1.0.0")); controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Never); + + controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushCallbackEvent("foo", "1.0.0", + It.IsAny(), controller.OwinContext.Request.User.Identity, 404), Times.Once); } [Theory] @@ -1102,6 +1108,9 @@ public async void VerifyPackageKeyReturns404IfPackageDoesNotExist_ApiKeyVerifyV1 String.Format(CultureInfo.CurrentCulture, Strings.PackageWithIdAndVersionNotFound, "foo", "1.0.0")); controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Once); + + controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushCallbackEvent("foo", "1.0.0", + It.IsAny(), controller.OwinContext.Request.User.Identity, 404), Times.Once); } [Theory] @@ -1128,6 +1137,9 @@ public async void VerifyPackageKeyReturns403IfUserIsNotAnOwner_ApiKeyV2(string s Strings.ApiKeyNotAuthorized); controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Never); + + controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushCallbackEvent("foo", "1.0.0", + It.IsAny(), controller.OwinContext.Request.User.Identity, 403), Times.Once); } [Theory] @@ -1152,6 +1164,9 @@ public async void VerifyPackageKeyReturns403IfUserIsNotAnOwner_ApiKeyVerifyV1(st Strings.ApiKeyNotAuthorized); controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Once); + + controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushCallbackEvent("foo", "1.0.0", + It.IsAny(), controller.OwinContext.Request.User.Identity, 403), Times.Once); } [Theory] @@ -1181,6 +1196,9 @@ public async void VerifyPackageKeyReturns403IfScopeDoesNotMatch_ApiKeyV2(string Strings.ApiKeyNotAuthorized); controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Never); + + controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushCallbackEvent("foo", "1.0.0", + It.IsAny(), controller.OwinContext.Request.User.Identity, 403), Times.Once); } [Theory] @@ -1210,6 +1228,9 @@ public async void VerifyPackageKeyReturns403IfScopeDoesNotMatch_ApiKeyVerifyV1(s Strings.ApiKeyNotAuthorized); controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Once); + + controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushCallbackEvent("foo", "1.0.0", + It.IsAny(), controller.OwinContext.Request.User.Identity, 403), Times.Once); } [Theory] @@ -1233,6 +1254,9 @@ public async void VerifyPackageKeyReturns200_ApiKeyV2(string scope) ResultAssert.IsEmpty(result); controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Never); + + controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushCallbackEvent("foo", "1.0.0", + It.IsAny(), controller.OwinContext.Request.User.Identity, 200), Times.Once); } [Theory] @@ -1254,6 +1278,9 @@ public async void VerifyPackageKeyReturns200_ApiKeyVerifyV1(string scope) ResultAssert.IsEmpty(result); controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Once); + + controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushCallbackEvent("foo", "1.0.0", + It.IsAny(), controller.OwinContext.Request.User.Identity, 200), Times.Once); } } diff --git a/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs index 8eef4805f6..353adab7a4 100644 --- a/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs @@ -1879,7 +1879,7 @@ public async Task WillSendPackagePublishedEvent() await controller.VerifyPackage(new VerifyPackageRequest { Listed = true, Edit = null }); // Assert - fakeTelemetryService.Verify(x => x.TrackPackagePushEvent(TestUtility.FakeUser, controller.OwinContext.Request.User.Identity), Times.Once); + fakeTelemetryService.Verify(x => x.TrackPackagePushEvent(It.IsAny(), TestUtility.FakeUser, controller.OwinContext.Request.User.Identity), Times.Once); } } } From 4a5af43aa8d0f3260b55b84c7de6b1ea0b85eb40 Mon Sep 17 00:00:00 2001 From: Christy Henriksson Date: Thu, 16 Mar 2017 07:03:49 -0700 Subject: [PATCH 2/3] PR feedback --- src/NuGetGallery/Controllers/ApiController.cs | 4 +- .../Services/ITelemetryService.cs | 4 +- src/NuGetGallery/Services/TelemetryService.cs | 118 ++++++++++++------ .../Controllers/ApiControllerFacts.cs | 18 +-- 4 files changed, 90 insertions(+), 54 deletions(-) diff --git a/src/NuGetGallery/Controllers/ApiController.cs b/src/NuGetGallery/Controllers/ApiController.cs index 0707cb7d63..fb716a9262 100644 --- a/src/NuGetGallery/Controllers/ApiController.cs +++ b/src/NuGetGallery/Controllers/ApiController.cs @@ -222,7 +222,7 @@ public async virtual Task CreatePackageVerificationKeyAsync(string var user = GetCurrentUser(); await AuthenticationService.AddCredential(user, credential); - TelemetryService.TrackSymbolsPushEvent(id, version, user, User.Identity); + TelemetryService.TrackCreatePackageVerificationKeyEvent(id, version, user, User.Identity); return Json(new { @@ -249,7 +249,7 @@ public async virtual Task VerifyPackageKeyAsync(string id, string await AuthenticationService.RemoveCredential(user, credential); } - TelemetryService.TrackSymbolsPushCallbackEvent(id, version, user, User.Identity, result?.StatusCode ?? 200); + TelemetryService.TrackVerifyPackageKeyEvent(id, version, user, User.Identity, result?.StatusCode ?? 200); return (ActionResult)result ?? new EmptyResult(); } diff --git a/src/NuGetGallery/Services/ITelemetryService.cs b/src/NuGetGallery/Services/ITelemetryService.cs index 5bd76bba55..306afcc5f2 100644 --- a/src/NuGetGallery/Services/ITelemetryService.cs +++ b/src/NuGetGallery/Services/ITelemetryService.cs @@ -11,8 +11,8 @@ public interface ITelemetryService void TrackPackagePushEvent(Package package, User user, IIdentity identity); - void TrackSymbolsPushEvent(string packageId, string packageVersion, User user, IIdentity identity); + void TrackCreatePackageVerificationKeyEvent(string packageId, string packageVersion, User user, IIdentity identity); - void TrackSymbolsPushCallbackEvent(string packageId, string packageVersion, User user, IIdentity identity, int statusCode); + void TrackVerifyPackageKeyEvent(string packageId, string packageVersion, User user, IIdentity identity, int statusCode); } } \ No newline at end of file diff --git a/src/NuGetGallery/Services/TelemetryService.cs b/src/NuGetGallery/Services/TelemetryService.cs index f5061f5cbe..84c77186d6 100644 --- a/src/NuGetGallery/Services/TelemetryService.cs +++ b/src/NuGetGallery/Services/TelemetryService.cs @@ -10,39 +10,40 @@ namespace NuGetGallery { public class TelemetryService : ITelemetryService { - // ODataQueryFilter - public const string ODataQueryFilter = "ODataQueryFilter"; + // Event types + public const string ODataQueryFilterEvent = "ODataQueryFilter"; + public const string PackagePushEvent = "PackagePush"; + public const string CreatePackageVerificationKeyEvent = "CreatePackageVerificationKeyEvent"; + public const string VerifyPackageKeyEvent = "VerifyPackageKeyEvent"; + + // ODataQueryFilter properties public const string CallContext = "CallContext"; public const string IsEnabled = "IsEnabled"; public const string IsAllowed = "IsAllowed"; public const string QueryPattern = "QueryPattern"; - // Package events - public const string PackagePushEvent = "PackagePush"; - public const string SymbolsPushEvent = "SymbolsPush"; - public const string SymbolsPushCallbackEvent = "SymbolsPushCallback"; - - // Package event properties + // Package push properties public const string AuthenticationMethod = "AuthenticationMethod"; public const string AccountCreationDate = "AccountCreationDate"; public const string ClientVersion = "ClientVersion"; public const string IsScoped = "IsScoped"; - public const string HasVerifyScope = "HasVerifyScope"; public const string PackageId = "PackageId"; public const string PackageVersion = "PackageVersion"; - public const string SymbolsStatusCode = "SymbolsStatusCode"; + + // Verify package properties + public const string HasVerifyScope = "HasVerifyScope"; + public const string VerifyPackageKeyStatusCode = "VerifyPackageKeyStatusCode"; public void TrackODataQueryFilterEvent(string callContext, bool isEnabled, bool isAllowed, string queryPattern) { - var telemetryProperties = new Dictionary(); - - telemetryProperties.Add(CallContext, callContext); - telemetryProperties.Add(IsEnabled, $"{isEnabled}"); - - telemetryProperties.Add(IsAllowed, $"{isAllowed}"); - telemetryProperties.Add(QueryPattern, queryPattern); + TrackEvent(ODataQueryFilterEvent, properties => + { + properties.Add(CallContext, callContext); + properties.Add(IsEnabled, $"{isEnabled}"); - Telemetry.TrackEvent(ODataQueryFilter, telemetryProperties, metrics: null); + properties.Add(IsAllowed, $"{isAllowed}"); + properties.Add(QueryPattern, queryPattern); + }); } public void TrackPackagePushEvent(Package package, User user, IIdentity identity) @@ -52,22 +53,48 @@ public void TrackPackagePushEvent(Package package, User user, IIdentity identity throw new ArgumentNullException(nameof(package)); } - TrackPackageEvent(PackagePushEvent, package.PackageRegistration.Id, package.NormalizedVersion, user, identity); - } + if (user == null) + { + throw new ArgumentNullException(nameof(user)); + } - public void TrackSymbolsPushEvent(string packageId, string packageVersion, User user, IIdentity identity) - { - TrackPackageEvent(SymbolsPushEvent, packageId, packageVersion, user, identity); + if (identity == null) + { + throw new ArgumentNullException(nameof(identity)); + } + + TrackEvent(PackagePushEvent, properties => { + properties.Add(ClientVersion, GetClientVersion()); + properties.Add(PackageId, package.PackageRegistration.Id); + properties.Add(PackageVersion, package.Version); + properties.Add(AuthenticationMethod, identity.GetAuthenticationType()); + properties.Add(AccountCreationDate, GetAccountCreationDate(user)); + properties.Add(IsScoped, identity.IsScopedAuthentication().ToString()); + }); } - public void TrackSymbolsPushCallbackEvent(string packageId, string packageVersion, User user, IIdentity identity, int statusCode) + public void TrackCreatePackageVerificationKeyEvent(string packageId, string packageVersion, User user, IIdentity identity) { - TrackPackageEvent(SymbolsPushCallbackEvent, packageId, packageVersion, user, identity, - properties => properties.Add(SymbolsStatusCode, statusCode.ToString())); + if (user == null) + { + throw new ArgumentNullException(nameof(user)); + } + + if (identity == null) + { + throw new ArgumentNullException(nameof(identity)); + } + + TrackEvent(CreatePackageVerificationKeyEvent, properties => { + properties.Add(ClientVersion, GetClientVersion()); + properties.Add(PackageId, packageId); + properties.Add(PackageVersion, packageVersion); + properties.Add(AccountCreationDate, GetAccountCreationDate(user)); + properties.Add(IsScoped, identity.IsScopedAuthentication().ToString()); + }); } - private void TrackPackageEvent(string eventName, string packageId, string packageVersion, User user, IIdentity identity, - Action> addCustomProperties = null) + public void TrackVerifyPackageKeyEvent(string packageId, string packageVersion, User user, IIdentity identity, int statusCode) { if (user == null) { @@ -79,21 +106,30 @@ private void TrackPackageEvent(string eventName, string packageId, string packag throw new ArgumentNullException(nameof(identity)); } - var authenticationMethod = identity.GetAuthenticationType(); - var isScoped = identity.IsScopedAuthentication(); - var hasVerifyScope = isScoped && identity.HasVerifyScope(); - var clientVersion = HttpContext.Current?.Request?.Headers[Constants.ClientVersionHeaderName]; + TrackEvent(VerifyPackageKeyEvent, properties => + { + properties.Add(PackageId, packageId); + properties.Add(PackageVersion, packageVersion); + properties.Add(HasVerifyScope, identity.HasVerifyScope().ToString()); + properties.Add(VerifyPackageKeyStatusCode, statusCode.ToString()); + }); + } + + private static string GetClientVersion() + { + return HttpContext.Current?.Request?.Headers[Constants.ClientVersionHeaderName]; + } + private static string GetAccountCreationDate(User user) + { + return user.CreatedUtc != null ? user.CreatedUtc.Value.ToString("d") : "N/A"; + } + + private static void TrackEvent(string eventName, Action> addProperties) + { var telemetryProperties = new Dictionary(); - telemetryProperties.Add(ClientVersion, clientVersion); - telemetryProperties.Add(PackageId, packageId); - telemetryProperties.Add(PackageVersion, packageVersion); - telemetryProperties.Add(AuthenticationMethod, authenticationMethod); - telemetryProperties.Add(AccountCreationDate, user.CreatedUtc != null ? user.CreatedUtc.Value.ToString("d") : "N/A"); - telemetryProperties.Add(IsScoped, isScoped.ToString()); - telemetryProperties.Add(HasVerifyScope, hasVerifyScope.ToString()); - - addCustomProperties?.Invoke(telemetryProperties); + + addProperties(telemetryProperties); Telemetry.TrackEvent(eventName, telemetryProperties, metrics: null); } diff --git a/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs index d575200e3d..ee6f312aa0 100644 --- a/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs @@ -1060,7 +1060,7 @@ public async void CreatePackageKeyReturnsPackageVerificationKey(string scope) controller.MockAuthenticationService.Verify(s => s.AddCredential(It.IsAny(), It.IsAny()), Times.Once); - controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushEvent("foo", "1.0.0", + controller.MockTelemetryService.Verify(x => x.TrackCreatePackageVerificationKeyEvent("foo", "1.0.0", It.IsAny(), controller.OwinContext.Request.User.Identity), Times.Once); } } @@ -1087,7 +1087,7 @@ public async void VerifyPackageKeyReturns404IfPackageDoesNotExist_ApiKeyV2(strin controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Never); - controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushCallbackEvent("foo", "1.0.0", + controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent("foo", "1.0.0", It.IsAny(), controller.OwinContext.Request.User.Identity, 404), Times.Once); } @@ -1109,7 +1109,7 @@ public async void VerifyPackageKeyReturns404IfPackageDoesNotExist_ApiKeyVerifyV1 controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Once); - controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushCallbackEvent("foo", "1.0.0", + controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent("foo", "1.0.0", It.IsAny(), controller.OwinContext.Request.User.Identity, 404), Times.Once); } @@ -1138,7 +1138,7 @@ public async void VerifyPackageKeyReturns403IfUserIsNotAnOwner_ApiKeyV2(string s controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Never); - controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushCallbackEvent("foo", "1.0.0", + controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent("foo", "1.0.0", It.IsAny(), controller.OwinContext.Request.User.Identity, 403), Times.Once); } @@ -1165,7 +1165,7 @@ public async void VerifyPackageKeyReturns403IfUserIsNotAnOwner_ApiKeyVerifyV1(st controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Once); - controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushCallbackEvent("foo", "1.0.0", + controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent("foo", "1.0.0", It.IsAny(), controller.OwinContext.Request.User.Identity, 403), Times.Once); } @@ -1197,7 +1197,7 @@ public async void VerifyPackageKeyReturns403IfScopeDoesNotMatch_ApiKeyV2(string controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Never); - controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushCallbackEvent("foo", "1.0.0", + controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent("foo", "1.0.0", It.IsAny(), controller.OwinContext.Request.User.Identity, 403), Times.Once); } @@ -1229,7 +1229,7 @@ public async void VerifyPackageKeyReturns403IfScopeDoesNotMatch_ApiKeyVerifyV1(s controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Once); - controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushCallbackEvent("foo", "1.0.0", + controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent("foo", "1.0.0", It.IsAny(), controller.OwinContext.Request.User.Identity, 403), Times.Once); } @@ -1255,7 +1255,7 @@ public async void VerifyPackageKeyReturns200_ApiKeyV2(string scope) controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Never); - controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushCallbackEvent("foo", "1.0.0", + controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent("foo", "1.0.0", It.IsAny(), controller.OwinContext.Request.User.Identity, 200), Times.Once); } @@ -1279,7 +1279,7 @@ public async void VerifyPackageKeyReturns200_ApiKeyVerifyV1(string scope) controller.MockAuthenticationService.Verify(s => s.RemoveCredential(It.IsAny(), It.IsAny()), Times.Once); - controller.MockTelemetryService.Verify(x => x.TrackSymbolsPushCallbackEvent("foo", "1.0.0", + controller.MockTelemetryService.Verify(x => x.TrackVerifyPackageKeyEvent("foo", "1.0.0", It.IsAny(), controller.OwinContext.Request.User.Identity, 200), Times.Once); } } From 00c1c928e453431f5d0d26be2cd5aa22662f3399 Mon Sep 17 00:00:00 2001 From: Christy Henriksson Date: Thu, 16 Mar 2017 08:47:03 -0700 Subject: [PATCH 3/3] PR feedback #2 --- src/NuGetGallery/ExtensionMethods.cs | 4 ++-- src/NuGetGallery/Services/TelemetryService.cs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/NuGetGallery/ExtensionMethods.cs b/src/NuGetGallery/ExtensionMethods.cs index 9592895e7a..08ad599abd 100644 --- a/src/NuGetGallery/ExtensionMethods.cs +++ b/src/NuGetGallery/ExtensionMethods.cs @@ -407,8 +407,8 @@ public static bool HasVerifyScope(this IIdentity self) { var scopeClaim = self.GetScopeClaim(); - return ScopeEvaluator.ScopeClaimsAllowsActionForSubject(scopeClaim, null, - new [] { NuGetScopes.PackageVerify }); + return ScopeEvaluator.ScopeClaimsAllowsActionForSubject(scopeClaim, subject: null, + requestedActions: new [] { NuGetScopes.PackageVerify }); } // This is a method because the first call will perform a database call diff --git a/src/NuGetGallery/Services/TelemetryService.cs b/src/NuGetGallery/Services/TelemetryService.cs index 84c77186d6..84c24f9158 100644 --- a/src/NuGetGallery/Services/TelemetryService.cs +++ b/src/NuGetGallery/Services/TelemetryService.cs @@ -31,7 +31,7 @@ public class TelemetryService : ITelemetryService public const string PackageVersion = "PackageVersion"; // Verify package properties - public const string HasVerifyScope = "HasVerifyScope"; + public const string IsVerificationKeyUsed = "IsVerificationKeyUsed"; public const string VerifyPackageKeyStatusCode = "VerifyPackageKeyStatusCode"; public void TrackODataQueryFilterEvent(string callContext, bool isEnabled, bool isAllowed, string queryPattern) @@ -110,7 +110,7 @@ public void TrackVerifyPackageKeyEvent(string packageId, string packageVersion, { properties.Add(PackageId, packageId); properties.Add(PackageVersion, packageVersion); - properties.Add(HasVerifyScope, identity.HasVerifyScope().ToString()); + properties.Add(IsVerificationKeyUsed, identity.HasVerifyScope().ToString()); properties.Add(VerifyPackageKeyStatusCode, statusCode.ToString()); }); }