refactor PII Logging behavior (#23698)

christothes · web-flow · commit 65c81953e963 · 2021-08-31T19:36:22.000-05:00
diff --git a/sdk/identity/Azure.Identity/src/AzureIdentityEventSource.cs b/sdk/identity/Azure.Identity/src/AzureIdentityEventSource.cs
@@ -162,25 +162,22 @@ private static string FormatException(Exception ex)
         }
 
         [NonEvent]
-        public void LogMsal(Microsoft.Identity.Client.LogLevel level, string message, bool containsPii)
+        public void LogMsal(Microsoft.Identity.Client.LogLevel level, string message)
         {
-            if (!containsPii)
+            switch (level)
             {
-                switch (level)
-                {
-                    case Microsoft.Identity.Client.LogLevel.Error when IsEnabled(EventLevel.Error, EventKeywords.All):
-                        LogMsalError(message);
-                        break;
-                    case Microsoft.Identity.Client.LogLevel.Warning when IsEnabled(EventLevel.Warning, EventKeywords.All):
-                        LogMsalWarning(message);
-                        break;
-                    case Microsoft.Identity.Client.LogLevel.Info when IsEnabled(EventLevel.Informational, EventKeywords.All):
-                        LogMsalInformational(message);
-                        break;
-                    case Microsoft.Identity.Client.LogLevel.Verbose when IsEnabled(EventLevel.Verbose, EventKeywords.All):
-                        LogMsalVerbose(message);
-                        break;
-                }
+                case Microsoft.Identity.Client.LogLevel.Error when IsEnabled(EventLevel.Error, EventKeywords.All):
+                    LogMsalError(message);
+                    break;
+                case Microsoft.Identity.Client.LogLevel.Warning when IsEnabled(EventLevel.Warning, EventKeywords.All):
+                    LogMsalWarning(message);
+                    break;
+                case Microsoft.Identity.Client.LogLevel.Info when IsEnabled(EventLevel.Informational, EventKeywords.All):
+                    LogMsalInformational(message);
+                    break;
+                case Microsoft.Identity.Client.LogLevel.Verbose when IsEnabled(EventLevel.Verbose, EventKeywords.All):
+                    LogMsalVerbose(message);
+                    break;
             }
         }
 
diff --git a/sdk/identity/Azure.Identity/src/MsalClientBase.cs b/sdk/identity/Azure.Identity/src/MsalClientBase.cs
@@ -11,7 +11,7 @@ internal abstract class MsalClientBase<TClient>
         where TClient : IClientApplicationBase
     {
         private readonly AsyncLockWithValue<TClient> _clientAsyncLock;
-        internal protected bool LogPII { get; protected set; }
+        internal protected bool IsPiiLoggingEnabled { get; }
 
         /// <summary>
         /// For mocking purposes only.
@@ -20,7 +20,7 @@ protected MsalClientBase()
         {
         }
 
-        protected MsalClientBase(CredentialPipeline pipeline, string tenantId, string clientId, ITokenCacheOptions cacheOptions)
+        protected MsalClientBase(CredentialPipeline pipeline, string tenantId, string clientId, bool isPiiLoggingEnabled, ITokenCacheOptions cacheOptions)
         {
             // This validation is performed as a backstop. Validation in TokenCredentialOptions.AuthorityHost prevents users from explicitly
             // setting AuthorityHost to a non TLS endpoint. However, the AuthorityHost can also be set by the AZURE_AUTHORITY_HOST environment
@@ -29,14 +29,11 @@ protected MsalClientBase(CredentialPipeline pipeline, string tenantId, string cl
             // we validate here as all other credentials will create an MSAL client.
             Validations.ValidateAuthorityHost(pipeline.AuthorityHost);
 
+            IsPiiLoggingEnabled = isPiiLoggingEnabled;
             Pipeline = pipeline;
-
             TenantId = tenantId;
-
             ClientId = clientId;
-
             TokenCache = cacheOptions?.TokenCachePersistenceOptions == null ? null : new TokenCache(cacheOptions?.TokenCachePersistenceOptions);
-
             _clientAsyncLock = new AsyncLockWithValue<TClient>();
         }
 
@@ -73,5 +70,13 @@ protected async ValueTask<TClient> GetClientAsync(bool async, CancellationToken
             asyncLock.SetValue(client);
             return client;
         }
+
+        protected void LogMsal(LogLevel level, string message, bool isPii)
+        {
+            if (!isPii || IsPiiLoggingEnabled)
+            {
+                AzureIdentityEventSource.Singleton.LogMsal(level, message);
+            }
+        }
     }
 }
diff --git a/sdk/identity/Azure.Identity/src/MsalConfidentialClient.cs b/sdk/identity/Azure.Identity/src/MsalConfidentialClient.cs
@@ -18,25 +18,21 @@ internal class MsalConfidentialClient : MsalClientBase<IConfidentialClientApplic
         /// For mocking purposes only.
         /// </summary>
         protected MsalConfidentialClient()
-            : base()
-        {
-        }
+        { }
 
-        public MsalConfidentialClient(CredentialPipeline pipeline, string tenantId, string clientId, string clientSecret, ITokenCacheOptions cacheOptions, RegionalAuthority? regionalAuthority, bool logPii)
-            : base(pipeline, tenantId, clientId, cacheOptions)
+        public MsalConfidentialClient(CredentialPipeline pipeline, string tenantId, string clientId, string clientSecret, ITokenCacheOptions cacheOptions, RegionalAuthority? regionalAuthority, bool isPiiLoggingEnabled)
+            : base(pipeline, tenantId, clientId, isPiiLoggingEnabled, cacheOptions)
         {
             _clientSecret = clientSecret;
             RegionalAuthority = regionalAuthority;
-            LogPII = logPii;
         }
 
-        public MsalConfidentialClient(CredentialPipeline pipeline, string tenantId, string clientId, ClientCertificateCredential.IX509Certificate2Provider certificateProvider, bool includeX5CClaimHeader, ITokenCacheOptions cacheOptions, RegionalAuthority? regionalAuthority, bool logPii)
-            : base(pipeline, tenantId, clientId, cacheOptions)
+        public MsalConfidentialClient(CredentialPipeline pipeline, string tenantId, string clientId, ClientCertificateCredential.IX509Certificate2Provider certificateProvider, bool includeX5CClaimHeader, ITokenCacheOptions cacheOptions, RegionalAuthority? regionalAuthority, bool isPiiLoggingEnabled)
+            : base(pipeline, tenantId, clientId, isPiiLoggingEnabled, cacheOptions)
         {
             _includeX5CClaimHeader = includeX5CClaimHeader;
             _certificateProvider = certificateProvider;
             RegionalAuthority = regionalAuthority;
-            LogPII = logPii;
         }
 
         internal RegionalAuthority? RegionalAuthority { get; }
@@ -46,7 +42,7 @@ protected override async ValueTask<IConfidentialClientApplication> CreateClientA
             ConfidentialClientApplicationBuilder confClientBuilder = ConfidentialClientApplicationBuilder.Create(ClientId)
                 .WithAuthority(Pipeline.AuthorityHost.AbsoluteUri, TenantId)
                 .WithHttpClientFactory(new HttpPipelineClientFactory(Pipeline.HttpPipeline))
-                .WithLogging(AzureIdentityEventSource.Singleton.LogMsal, enablePiiLogging: LogPII);
+                .WithLogging(LogMsal, enablePiiLogging: IsPiiLoggingEnabled);
 
             if (_clientSecret != null)
             {
diff --git a/sdk/identity/Azure.Identity/src/MsalPublicClient.cs b/sdk/identity/Azure.Identity/src/MsalPublicClient.cs
@@ -18,11 +18,10 @@ internal class MsalPublicClient : MsalClientBase<IPublicClientApplication>
         protected MsalPublicClient()
         { }
 
-        public MsalPublicClient(CredentialPipeline pipeline, string tenantId, string clientId, string redirectUrl, ITokenCacheOptions cacheOptions, bool logPII)
-            : base(pipeline, tenantId, clientId, cacheOptions)
+        public MsalPublicClient(CredentialPipeline pipeline, string tenantId, string clientId, string redirectUrl, ITokenCacheOptions cacheOptions, bool isPiiLoggingEnabled)
+            : base(pipeline, tenantId, clientId, isPiiLoggingEnabled, cacheOptions)
         {
             RedirectUrl = redirectUrl;
-            LogPII = logPII;
         }
 
         protected override ValueTask<IPublicClientApplication> CreateClientAsync(bool async, CancellationToken cancellationToken)
@@ -42,7 +41,7 @@ protected virtual ValueTask<IPublicClientApplication> CreateClientCoreAsync(stri
                 .Create(ClientId)
                 .WithAuthority(authorityUri)
                 .WithHttpClientFactory(new HttpPipelineClientFactory(Pipeline.HttpPipeline))
-                .WithLogging(AzureIdentityEventSource.Singleton.LogMsal, enablePiiLogging: LogPII);
+                .WithLogging(LogMsal, enablePiiLogging: IsPiiLoggingEnabled);
 
             if (!string.IsNullOrEmpty(RedirectUrl))
             {
diff --git a/sdk/identity/Azure.Identity/tests/DeviceCodeCredentialTests.cs b/sdk/identity/Azure.Identity/tests/DeviceCodeCredentialTests.cs
@@ -127,7 +127,7 @@ public void RespectsIsPIILoggingEnabled([Values(true, false)] bool isLoggingPIIE
             var credential = new DeviceCodeCredential(new DeviceCodeCredentialOptions { IsLoggingPIIEnabled = isLoggingPIIEnabled});
 
             Assert.NotNull(credential.Client);
-            Assert.AreEqual(isLoggingPIIEnabled, credential.Client.LogPII);
+            Assert.AreEqual(isLoggingPIIEnabled, credential.Client.IsPiiLoggingEnabled);
         }
 
         [Test]
diff --git a/sdk/identity/Azure.Identity/tests/InteractiveBrowserCredentialTests.cs b/sdk/identity/Azure.Identity/tests/InteractiveBrowserCredentialTests.cs
@@ -55,7 +55,7 @@ public void RespectsIsPIILoggingEnabled([Values(true, false)] bool isLoggingPIIE
             var credential = new InteractiveBrowserCredential(new InteractiveBrowserCredentialOptions { IsLoggingPIIEnabled = isLoggingPIIEnabled});
 
             Assert.NotNull(credential.Client);
-            Assert.AreEqual(isLoggingPIIEnabled, credential.Client.LogPII);
+            Assert.AreEqual(isLoggingPIIEnabled, credential.Client.IsPiiLoggingEnabled);
         }
 
         [Test]
diff --git a/sdk/identity/Azure.Identity/tests/MsalClientBaseTests.cs b/sdk/identity/Azure.Identity/tests/MsalClientBaseTests.cs
@@ -0,0 +1,88 @@
+﻿// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System;
+using System.Collections.Generic;
+using System.Diagnostics.Tracing;
+using System.Threading;
+using System.Threading.Tasks;
+using Azure.Core;
+using Azure.Core.Diagnostics;
+using Azure.Core.Pipeline;
+using Microsoft.Identity.Client;
+using NUnit.Framework;
+using Azure.Core.TestFramework;
+
+namespace Azure.Identity.Tests
+{
+    public class MsalClientBaseTests
+    {
+        [Test]
+        [NonParallelizable]
+        public void LogPiiIsEnforcedPerInstance([Values(true, false)] bool logPii)
+        {
+            string client1Message = "client1";
+            string client2Message = "client2";
+            List<string> messages = new();
+
+            using AzureEventSourceListener listener = new(
+                (args, _) =>
+                {
+                    if (args.EventName.StartsWith(nameof(AzureIdentityEventSource.LogMsal)))
+                    {
+                        messages.Add(args.GetProperty<string>("message"));
+                    }
+                },
+                EventLevel.Verbose);
+
+            var client_1 = new MockMsalClient(
+                new CredentialPipeline(new Uri("https://w.com"), new HttpPipeline(new MockTransport()), new ClientDiagnostics(Moq.Mock.Of<ClientOptions>())),
+                "tenant",
+                "client",
+                logPii,
+                new InteractiveBrowserCredentialOptions());
+
+            var client_2 = new MockMsalClient(
+                new CredentialPipeline(new Uri("https://w.com"), new HttpPipeline(new MockTransport()), new ClientDiagnostics(Moq.Mock.Of<ClientOptions>())),
+                "tenant",
+                "client",
+                false, // never log PII
+                new InteractiveBrowserCredentialOptions());
+
+            Assert.AreEqual(logPii, client_1.IsPiiLoggingEnabled);
+
+            client_1.Log(client1Message, true);
+            client_2.Log(client2Message, true);
+
+            if (logPii)
+            {
+                Assert.Contains(client1Message, messages);
+            }
+            Assert.That(messages, Does.Not.Contain(client2Message));
+            messages.Clear();
+
+            client_1.Log(client1Message, false);
+            client_2.Log(client2Message, false);
+
+            Assert.That(messages, Contains.Item(client1Message));
+            Assert.That(messages, Contains.Item(client2Message));
+        }
+
+        private class MockMsalClient : MsalClientBase<IClientApplicationBase>
+        {
+            public MockMsalClient(CredentialPipeline pipeline, string tenantId, string clientId, bool isPiiLoggingEnabled, ITokenCacheOptions cacheOptions)
+                : base(pipeline, tenantId, clientId, isPiiLoggingEnabled, cacheOptions)
+            { }
+
+            public ManualResetEventSlim Evt { get; set; }
+
+            protected override ValueTask<IClientApplicationBase> CreateClientAsync(bool async, CancellationToken cancellationToken)
+                => throw new NotImplementedException();
+
+            public void Log(string message, bool isPii)
+            {
+                LogMsal(LogLevel.Error, message, isPii);
+            }
+        }
+    }
+}
diff --git a/sdk/identity/Azure.Identity/tests/SharedTokenCacheCredentialTests.cs b/sdk/identity/Azure.Identity/tests/SharedTokenCacheCredentialTests.cs
@@ -78,7 +78,7 @@ public void RespectsIsPIILoggingEnabled([Values(true, false)] bool isLoggingPIIE
             var credential = new SharedTokenCacheCredential(new SharedTokenCacheCredentialOptions{ IsLoggingPIIEnabled = isLoggingPIIEnabled});
 
             Assert.NotNull(credential.Client);
-            Assert.AreEqual(isLoggingPIIEnabled, credential.Client.LogPII);
+            Assert.AreEqual(isLoggingPIIEnabled, credential.Client.IsPiiLoggingEnabled);
         }
 
         [Test]
diff --git a/sdk/identity/Azure.Identity/tests/UsernamePasswordCredentialTests.cs b/sdk/identity/Azure.Identity/tests/UsernamePasswordCredentialTests.cs
@@ -72,7 +72,7 @@ public void RespectsIsPIILoggingEnabled([Values(true, false)] bool isLoggingPIIE
                 null);
 
             Assert.NotNull(credential.Client);
-            Assert.AreEqual(isLoggingPIIEnabled, credential.Client.LogPII);
+            Assert.AreEqual(isLoggingPIIEnabled, credential.Client.IsPiiLoggingEnabled);
         }
 
         [Test]
diff --git a/sdk/identity/Azure.Identity/tests/VisualStudioCodeCredentialTests.cs b/sdk/identity/Azure.Identity/tests/VisualStudioCodeCredentialTests.cs
@@ -88,7 +88,7 @@ public void RespectsIsPIILoggingEnabled([Values(true, false)] bool isLoggingPIIE
             var credential = new VisualStudioCodeCredential(new VisualStudioCodeCredentialOptions{ IsLoggingPIIEnabled = isLoggingPIIEnabled});
 
             Assert.NotNull(credential.Client);
-            Assert.AreEqual(isLoggingPIIEnabled, credential.Client.LogPII);
+            Assert.AreEqual(isLoggingPIIEnabled, credential.Client.IsPiiLoggingEnabled);
         }
 
         [Test]

Original file line number	Diff line number	Diff line change
`@@ -11,7 +11,7 @@ internal abstract class MsalClientBase<TClient>`
`11`	`11`	`where TClient : IClientApplicationBase`
`12`	`12`	`{`
`13`	`13`	`private readonly AsyncLockWithValue<TClient> _clientAsyncLock;`
`14`		`- internal protected bool LogPII { get; protected set; }`
	`14`	`+ internal protected bool IsPiiLoggingEnabled { get; }`
`15`	`15`
`16`	`16`	`/// <summary>`
`17`	`17`	`/// For mocking purposes only.`
`@@ -20,7 +20,7 @@ protected MsalClientBase()`
`20`	`20`	`{`
`21`	`21`	`}`
`22`	`22`
`23`		`- protected MsalClientBase(CredentialPipeline pipeline, string tenantId, string clientId, ITokenCacheOptions cacheOptions)`
	`23`	`+ protected MsalClientBase(CredentialPipeline pipeline, string tenantId, string clientId, bool isPiiLoggingEnabled, ITokenCacheOptions cacheOptions)`
`24`	`24`	`{`
`25`	`25`	`// This validation is performed as a backstop. Validation in TokenCredentialOptions.AuthorityHost prevents users from explicitly`
`26`	`26`	`// setting AuthorityHost to a non TLS endpoint. However, the AuthorityHost can also be set by the AZURE_AUTHORITY_HOST environment`
`@@ -29,14 +29,11 @@ protected MsalClientBase(CredentialPipeline pipeline, string tenantId, string cl`
`29`	`29`	`// we validate here as all other credentials will create an MSAL client.`
`30`	`30`	`Validations.ValidateAuthorityHost(pipeline.AuthorityHost);`
`31`	`31`
	`32`	`+ IsPiiLoggingEnabled = isPiiLoggingEnabled;`
`32`	`33`	`Pipeline = pipeline;`
`33`		`-`
`34`	`34`	`TenantId = tenantId;`
`35`		`-`
`36`	`35`	`ClientId = clientId;`
`37`		`-`
`38`	`36`	`TokenCache = cacheOptions?.TokenCachePersistenceOptions == null ? null : new TokenCache(cacheOptions?.TokenCachePersistenceOptions);`
`39`		`-`
`40`	`37`	`_clientAsyncLock = new AsyncLockWithValue<TClient>();`
`41`	`38`	`}`
`42`	`39`
`@@ -73,5 +70,13 @@ protected async ValueTask<TClient> GetClientAsync(bool async, CancellationToken`
`73`	`70`	`asyncLock.SetValue(client);`
`74`	`71`	`return client;`
`75`	`72`	`}`
	`73`	`+`
	`74`	`+ protected void LogMsal(LogLevel level, string message, bool isPii)`
	`75`	`+ {`
	`76`	`+ if (!isPii \|\| IsPiiLoggingEnabled)`
	`77`	`+ {`
	`78`	`+ AzureIdentityEventSource.Singleton.LogMsal(level, message);`
	`79`	`+ }`
	`80`	`+ }`
`76`	`81`	`}`
`77`	`82`	`}`
Original file line number	Diff line number	Diff line change
`@@ -18,11 +18,10 @@ internal class MsalPublicClient : MsalClientBase<IPublicClientApplication>`
`18`	`18`	`protected MsalPublicClient()`
`19`	`19`	`{ }`
`20`	`20`
`21`		`- public MsalPublicClient(CredentialPipeline pipeline, string tenantId, string clientId, string redirectUrl, ITokenCacheOptions cacheOptions, bool logPII)`
`22`		`- : base(pipeline, tenantId, clientId, cacheOptions)`
	`21`	`+ public MsalPublicClient(CredentialPipeline pipeline, string tenantId, string clientId, string redirectUrl, ITokenCacheOptions cacheOptions, bool isPiiLoggingEnabled)`
	`22`	`+ : base(pipeline, tenantId, clientId, isPiiLoggingEnabled, cacheOptions)`
`23`	`23`	`{`
`24`	`24`	`RedirectUrl = redirectUrl;`
`25`		`- LogPII = logPII;`
`26`	`25`	`}`
`27`	`26`
`28`	`27`	`protected override ValueTask<IPublicClientApplication> CreateClientAsync(bool async, CancellationToken cancellationToken)`
`@@ -42,7 +41,7 @@ protected virtual ValueTask<IPublicClientApplication> CreateClientCoreAsync(stri`
`42`	`41`	`.Create(ClientId)`
`43`	`42`	`.WithAuthority(authorityUri)`
`44`	`43`	`.WithHttpClientFactory(new HttpPipelineClientFactory(Pipeline.HttpPipeline))`
`45`		`- .WithLogging(AzureIdentityEventSource.Singleton.LogMsal, enablePiiLogging: LogPII);`
	`44`	`+ .WithLogging(LogMsal, enablePiiLogging: IsPiiLoggingEnabled);`
`46`	`45`
`47`	`46`	`if (!string.IsNullOrEmpty(RedirectUrl))`
`48`	`47`	`{`
Original file line number	Diff line number	Diff line change
`@@ -127,7 +127,7 @@ public void RespectsIsPIILoggingEnabled([Values(true, false)] bool isLoggingPIIE`
`127`	`127`	`var credential = new DeviceCodeCredential(new DeviceCodeCredentialOptions { IsLoggingPIIEnabled = isLoggingPIIEnabled});`
`128`	`128`
`129`	`129`	`Assert.NotNull(credential.Client);`
`130`		`- Assert.AreEqual(isLoggingPIIEnabled, credential.Client.LogPII);`
	`130`	`+ Assert.AreEqual(isLoggingPIIEnabled, credential.Client.IsPiiLoggingEnabled);`
`131`	`131`	`}`
`132`	`132`
`133`	`133`	`[Test]`
Original file line number	Diff line number	Diff line change
`@@ -55,7 +55,7 @@ public void RespectsIsPIILoggingEnabled([Values(true, false)] bool isLoggingPIIE`
`55`	`55`	`var credential = new InteractiveBrowserCredential(new InteractiveBrowserCredentialOptions { IsLoggingPIIEnabled = isLoggingPIIEnabled});`
`56`	`56`
`57`	`57`	`Assert.NotNull(credential.Client);`
`58`		`- Assert.AreEqual(isLoggingPIIEnabled, credential.Client.LogPII);`
	`58`	`+ Assert.AreEqual(isLoggingPIIEnabled, credential.Client.IsPiiLoggingEnabled);`
`59`	`59`	`}`
`60`	`60`
`61`	`61`	`[Test]`
Original file line number	Diff line number	Diff line change
`@@ -78,7 +78,7 @@ public void RespectsIsPIILoggingEnabled([Values(true, false)] bool isLoggingPIIE`
`78`	`78`	`var credential = new SharedTokenCacheCredential(new SharedTokenCacheCredentialOptions{ IsLoggingPIIEnabled = isLoggingPIIEnabled});`
`79`	`79`
`80`	`80`	`Assert.NotNull(credential.Client);`
`81`		`- Assert.AreEqual(isLoggingPIIEnabled, credential.Client.LogPII);`
	`81`	`+ Assert.AreEqual(isLoggingPIIEnabled, credential.Client.IsPiiLoggingEnabled);`
`82`	`82`	`}`
`83`	`83`
`84`	`84`	`[Test]`
Original file line number	Diff line number	Diff line change
`@@ -72,7 +72,7 @@ public void RespectsIsPIILoggingEnabled([Values(true, false)] bool isLoggingPIIE`
`72`	`72`	`null);`
`73`	`73`
`74`	`74`	`Assert.NotNull(credential.Client);`
`75`		`- Assert.AreEqual(isLoggingPIIEnabled, credential.Client.LogPII);`
	`75`	`+ Assert.AreEqual(isLoggingPIIEnabled, credential.Client.IsPiiLoggingEnabled);`
`76`	`76`	`}`
`77`	`77`
`78`	`78`	`[Test]`
Original file line number	Diff line number	Diff line change
`@@ -88,7 +88,7 @@ public void RespectsIsPIILoggingEnabled([Values(true, false)] bool isLoggingPIIE`
`88`	`88`	`var credential = new VisualStudioCodeCredential(new VisualStudioCodeCredentialOptions{ IsLoggingPIIEnabled = isLoggingPIIEnabled});`
`89`	`89`
`90`	`90`	`Assert.NotNull(credential.Client);`
`91`		`- Assert.AreEqual(isLoggingPIIEnabled, credential.Client.LogPII);`
	`91`	`+ Assert.AreEqual(isLoggingPIIEnabled, credential.Client.IsPiiLoggingEnabled);`
`92`	`92`	`}`
`93`	`93`
`94`	`94`	`[Test]`