Send auth flow events before early exiting when no success events exist (#145)

Author: kyle-rader-msft

src/AzureAuth.Test/CommandMainTest.cs

@@ -11,6 +11,7 @@ namespace Microsoft.Authentication.AzureAuth.Test
     using System.Runtime.InteropServices;
     using FluentAssertions;
     using Microsoft.Authentication.MSALWrapper;
+    using Microsoft.Authentication.MSALWrapper.AuthFlow;
     using Microsoft.Extensions.DependencyInjection;
     using Microsoft.Extensions.Logging;
     using Microsoft.Identity.Client;
@@ -61,9 +62,9 @@ internal class CommandMainTest
         private IFileSystem fileSystem;
         private IServiceProvider serviceProvider;
         private MemoryTarget logTarget;
-        private Mock<ITokenFetcher> tokenFetcherMock;
         private Mock<IEnv> envMock;
         private Mock<ITelemetryService> telemetryServiceMock;
+        private Mock<IAuthFlow> authFlowMock;
 
         /// <summary>
         /// The setup.
@@ -82,14 +83,9 @@ public void Setup()
             loggingConfig.AddTarget(this.logTarget);
             loggingConfig.AddRuleForAllLevels(this.logTarget);
 
-            // Setup moq token fetcher
-            // When registering the token fetcher here the DI framework will match against
-            // the most specific constructor (i.e. most params) that it knows how to construct.
-            // Meaning all param types are also registered with the DI service provider.
-            this.tokenFetcherMock = new Mock<ITokenFetcher>(MockBehavior.Strict);
-
             this.envMock = new Mock<IEnv>(MockBehavior.Strict);
             this.telemetryServiceMock = new Mock<ITelemetryService>(MockBehavior.Strict);
+            this.authFlowMock = new Mock<IAuthFlow>(MockBehavior.Strict);
 
             // Environment variables should be null by default.
             this.envMock.Setup(env => env.Get(It.IsAny<string>())).Returns((string)null);
@@ -104,9 +100,9 @@ public void Setup()
                 })
                 .AddSingleton(this.eventData)
                 .AddSingleton(this.fileSystem)
-                .AddSingleton<ITokenFetcher>(this.tokenFetcherMock.Object)
                 .AddSingleton<IEnv>(this.envMock.Object)
                 .AddSingleton<ITelemetryService>(this.telemetryServiceMock.Object)
+                .AddSingleton<IAuthFlow>(this.authFlowMock.Object)
                 .AddTransient<CommandMain>()
                 .BuildServiceProvider();
         }
@@ -575,6 +571,39 @@ public void TestGenerateEvent_From_AuthFlowResult_With_Errors_And_Null_TokenResu
             eventData.Measures.Should().ContainKey("duration_milliseconds");
         }
 
+        [Test]
+        public void TestSendEvent_From_AuthFlowResult_With_Errors_And_Null_TokenResult()
+        {
+            var errors = new[]
+            {
+                new Exception("Exception 1."),
+            };
+
+            AuthFlowResult authFlowResult = new AuthFlowResult(null, errors, "Sample");
+
+            this.authFlowMock.Setup((a) => a.GetTokenAsync()).ReturnsAsync(authFlowResult);
+
+            // It would be nice to increase the assertion level here to include specific attributes on the eventData sent.
+            // The mock setup matching with strict behavior is difficult to get right, and we have other tests already validating the contents
+            // of events generated have what we expect. This validates we are in fact sending them.
+            this.telemetryServiceMock.Setup(s => s.SendEvent("authflow_Sample", It.IsAny<EventData>()));
+
+            var subject = this.serviceProvider.GetService<CommandMain>();
+
+            // mock valid args
+            subject.Resource = "f0e8d801-3a50-48fd-b2da-6476d6e832a2";
+            subject.Client = "e19f71ed-3b14-448d-9346-9eff9753646b";
+            subject.Tenant = "9f6227ee-3d14-473e-8bed-1281171ef8c9";
+            subject.EvaluateOptions().Should().BeTrue();
+
+            // Act
+            // Note: If the mock telem service matching fails, the OnExecute's global try-catch will hide that failure.
+            subject.OnExecute().Should().Be(1);
+
+            // Assert
+            this.telemetryServiceMock.VerifyAll();
+        }
+
         /// <summary>
         /// Test to generate event data from an authflow result with token result and msal errors.
         /// </summary>

src/AzureAuth/CommandMain.cs

@@ -483,6 +483,9 @@ private int GetToken()
                 this.eventData.Add("error_count", errors.Length);
                 this.eventData.Add("authflow_count", results.Length);
 
+                // Send custom telemetry events for each authflow result.
+                this.SendAuthFlowTelemetryEvents(results);
+
                 if (successfulResult == null)
                 {
                     this.logger.LogError("Authentication failed. Re-run with '--verbosity debug' to get see more info.");
@@ -507,9 +510,6 @@ private int GetToken()
                     case OutputMode.None:
                         break;
                 }
-
-                // Send custom telemetry events for each authflow result.
-                this.SendAuthFlowTelemetryEvents(results);
             }
             catch (Exception ex)
             {
@@ -535,24 +535,31 @@ private void SendAuthFlowTelemetryEvents(AuthFlowResult[] results)
 
         private AuthFlowExecutor AuthFlowExecutor()
         {
-            if (this.authFlowExecutor == null)
+            // TODO: Really we need to get rid of Resource
+            var scopes = this.Scopes ?? new string[] { $"{this.authSettings.Resource}/.default" };
+
+            IEnumerable<IAuthFlow> authFlows = null;
+            if (this.authFlow != null)
             {
-                // TODO: Really we need to get rid of Resource
-                var scopes = this.Scopes ?? new string[] { $"{this.authSettings.Resource}/.default" };
-
-                var authFlows = AuthFlowFactory.Create(
-                    this.logger,
-                    this.CombinedAuthMode,
-                    new Guid(this.authSettings.Client),
-                    new Guid(this.authSettings.Tenant),
-                    scopes,
-                    this.CacheFilePath,
-                    this.PreferredDomain,
-                    PrefixedPromptHint(this.authSettings.PromptHint),
-                    Constants.AuthOSXKeyChainSuffix);
-
-                this.authFlowExecutor = new AuthFlowExecutor(this.logger, authFlows, this.StopwatchTracker());
+                // if this.authFlow has been injected - use that.
+                authFlows = new[] { this.authFlow };
             }
+            else
+            {
+                // Normal production flow
+                authFlows = AuthFlowFactory.Create(
+                this.logger,
+                this.CombinedAuthMode,
+                new Guid(this.authSettings.Client),
+                new Guid(this.authSettings.Tenant),
+                scopes,
+                this.CacheFilePath,
+                this.PreferredDomain,
+                PrefixedPromptHint(this.authSettings.PromptHint),
+                Constants.AuthOSXKeyChainSuffix);
+            }
+
+            this.authFlowExecutor = new AuthFlowExecutor(this.logger, authFlows, this.StopwatchTracker());
 
             return this.authFlowExecutor;
         }