Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IsSupportLoggingEnabled property to TokenCredentialOptions #37463

Merged
merged 6 commits into from
Jul 14, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sdk/identity/Azure.Identity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

### Features Added
- Continuous Access Evaluation (CAE) is now configurable per-request by setting the `IsCaeEnabled` property of `TokenRequestContext` via its constructor.
- Added `IsSupportLoggingEnabled` property to `TokenCredentialOptions` which equates to the MSAL property `IsPIILoggingEnabled`
christothes marked this conversation as resolved.
Show resolved Hide resolved

### Bugs Fixed
- Fixed an issue with `TokenCachePersistenceOptions` where credentials in the same process would share the same cache, even if they had different configured names.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,7 @@ public partial class TokenCredentialOptions : Azure.Core.ClientOptions
public TokenCredentialOptions() { }
public System.Uri AuthorityHost { get { throw null; } set { } }
public new Azure.Identity.TokenCredentialDiagnosticsOptions Diagnostics { get { throw null; } }
public bool IsSupportLoggingEnabled { get { throw null; } set { } }
}
public abstract partial class UnsafeTokenCacheOptions : Azure.Identity.TokenCachePersistenceOptions
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public AzureCliCredential(AzureCliCredentialOptions options)

internal AzureCliCredential(CredentialPipeline pipeline, IProcessService processService, AzureCliCredentialOptions options = null)
{
_logPII = options?.IsLoggingPIIEnabled ?? false;
_logPII = options?.IsSupportLoggingEnabled ?? false;
_logAccountDetails = options?.Diagnostics?.IsAccountIdentifierLoggingEnabled ?? false;
_pipeline = pipeline;
_path = !string.IsNullOrEmpty(EnvironmentVariables.Path) ? EnvironmentVariables.Path : DefaultPath;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public AzureDeveloperCliCredential(AzureDeveloperCliCredentialOptions options)

internal AzureDeveloperCliCredential(CredentialPipeline pipeline, IProcessService processService, AzureDeveloperCliCredentialOptions options = null)
{
_logPII = options?.IsLoggingPIIEnabled ?? false;
_logPII = options?.IsSupportLoggingEnabled ?? false;
_logAccountDetails = options?.Diagnostics?.IsAccountIdentifierLoggingEnabled ?? false;
_pipeline = pipeline;
_processService = processService ?? ProcessService.Default;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public AzurePowerShellCredential(AzurePowerShellCredentialOptions options) : thi
internal AzurePowerShellCredential(AzurePowerShellCredentialOptions options, CredentialPipeline pipeline, IProcessService processService)
{
UseLegacyPowerShell = false;
_logPII = options?.IsLoggingPIIEnabled ?? false;
_logPII = options?.IsSupportLoggingEnabled ?? false;
_logAccountDetails = options?.Diagnostics?.IsAccountIdentifierLoggingEnabled ?? false;
TenantId = options?.TenantId;
_pipeline = pipeline ?? CredentialPipeline.GetInstance(options);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace Azure.Identity
public class ClientCertificateCredentialOptions : TokenCredentialOptions, ISupportsTokenCachePersistenceOptions, ISupportsDisableInstanceDiscovery, ISupportsAdditionallyAllowedTenants
{
/// <summary>
/// Specifies the <see cref="TokenCachePersistenceOptions"/> to be used by the credential. If not options are specified, the token cache will not be persisted to disk.
/// Specifies the <see cref="TokenCachePersistenceOptions"/> to be used by the credential. If no options are specified, the token cache will not be persisted to disk.
/// </summary>
public TokenCachePersistenceOptions TokenCachePersistenceOptions { get; set; }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,11 @@ public Uri AuthorityHost
}

/// <summary>
/// Gets or sets value indicating if ETW logging that contains PII content should be logged.
/// Setting this property will not disable redaction of <see cref="Request"/> Content. To enable logging of sensitive <see cref="Request.Content"/>
/// Gets or sets value indicating if ETW logging that contains potentially sensitive content should be logged.
/// Setting this property to true will not disable redaction of <see cref="Request"/> Content. To enable logging of sensitive <see cref="Request.Content"/>
christothes marked this conversation as resolved.
Show resolved Hide resolved
/// the <see cref="DiagnosticsOptions.IsLoggingContentEnabled"/> property must be set to <c>true</c>.
/// </summary>
internal bool IsLoggingPIIEnabled { get; set; }
public bool IsSupportLoggingEnabled { get; set; }

internal virtual T Clone<T>()
where T : TokenCredentialOptions, new()
Expand All @@ -46,7 +46,7 @@ internal virtual T Clone<T>()
// copy TokenCredentialOptions Properties
clone.AuthorityHost = AuthorityHost;

clone.IsLoggingPIIEnabled = IsLoggingPIIEnabled;
clone.IsSupportLoggingEnabled = IsSupportLoggingEnabled;

// copy TokenCredentialDiagnosticsOptions specific options
clone.Diagnostics.IsAccountIdentifierLoggingEnabled = Diagnostics.IsAccountIdentifierLoggingEnabled;
Expand All @@ -57,7 +57,7 @@ internal virtual T Clone<T>()
// copy ISupportsTokenCachePersistenceOptions
CloneIfImplemented<ISupportsTokenCachePersistenceOptions>(this, clone, (o, c) => c.TokenCachePersistenceOptions = o.TokenCachePersistenceOptions);

// copy ISupportsAdditinallyAllowedTenants
// copy ISupportsAdditionallyAllowedTenants
CloneIfImplemented<ISupportsAdditionallyAllowedTenants>(this, clone, (o, c) => CloneListItems(o.AdditionallyAllowedTenants, c.AdditionallyAllowedTenants));

// copy base ClientOptions properties, this would be replaced by a similar method on the base class
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public VisualStudioCredential(VisualStudioCredentialOptions options) : this(opti

internal VisualStudioCredential(string tenantId, CredentialPipeline pipeline, IFileSystemService fileSystem, IProcessService processService, VisualStudioCredentialOptions options = null)
{
_logPII = options?.IsLoggingPIIEnabled ?? false;
_logPII = options?.IsSupportLoggingEnabled ?? false;
_logAccountDetails = options?.Diagnostics?.IsAccountIdentifierLoggingEnabled ?? false;
TenantId = tenantId;
_pipeline = pipeline ?? CredentialPipeline.GetInstance(null);
Expand Down
6 changes: 3 additions & 3 deletions sdk/identity/Azure.Identity/src/MsalClientBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ internal abstract class MsalClientBase<TClient>
private readonly AsyncLockWithValue<(TClient Client, TokenCache Cache)> _clientWithCaeAsyncLock;
private readonly bool _logAccountDetails;
private readonly TokenCachePersistenceOptions _tokenCachePersistenceOptions;
protected internal bool IsPiiLoggingEnabled { get; }
protected internal bool IsSupportLoggingEnabled { get; }
protected internal bool DisableInstanceDiscovery { get; }
protected string[] cp1Capabilities = new[] { "CP1" };
protected internal CredentialPipeline Pipeline { get; }
Expand Down Expand Up @@ -44,7 +44,7 @@ protected MsalClientBase(CredentialPipeline pipeline, string tenantId, string cl
DisableInstanceDiscovery = options is ISupportsDisableInstanceDiscovery supportsDisableInstanceDiscovery && supportsDisableInstanceDiscovery.DisableInstanceDiscovery;
ISupportsTokenCachePersistenceOptions cacheOptions = options as ISupportsTokenCachePersistenceOptions;
_tokenCachePersistenceOptions = cacheOptions?.TokenCachePersistenceOptions;
IsPiiLoggingEnabled = options?.IsLoggingPIIEnabled ?? false;
IsSupportLoggingEnabled = options?.IsSupportLoggingEnabled ?? false;
Pipeline = pipeline;
TenantId = tenantId;
ClientId = clientId;
Expand Down Expand Up @@ -85,7 +85,7 @@ await _clientWithCaeAsyncLock.GetLockOrValueAsync(async, cancellationToken).Conf

protected void LogMsal(LogLevel level, string message, bool isPii)
{
if (!isPii || IsPiiLoggingEnabled)
if (!isPii || IsSupportLoggingEnabled)
{
AzureIdentityEventSource.Singleton.LogMsal(level, message);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ protected virtual async ValueTask<IConfidentialClientApplication> CreateClientCo

ConfidentialClientApplicationBuilder confClientBuilder = ConfidentialClientApplicationBuilder.Create(ClientId)
.WithHttpClientFactory(new HttpPipelineClientFactory(Pipeline.HttpPipeline))
.WithLogging(LogMsal, enablePiiLogging: IsPiiLoggingEnabled);
.WithLogging(LogMsal, enablePiiLogging: IsSupportLoggingEnabled);

// Special case for using appTokenProviderCallback, authority validation and instance metadata discovery should be disabled since we're not calling the STS
// The authority matches the one configured in the CredentialOptions.
Expand Down
2 changes: 1 addition & 1 deletion sdk/identity/Azure.Identity/src/MsalPublicClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ protected virtual ValueTask<IPublicClientApplication> CreateClientCoreAsync(bool
.Create(ClientId)
.WithAuthority(authorityUri)
.WithHttpClientFactory(new HttpPipelineClientFactory(Pipeline.HttpPipeline))
.WithLogging(LogMsal, enablePiiLogging: IsPiiLoggingEnabled);
.WithLogging(LogMsal, enablePiiLogging: IsSupportLoggingEnabled);

if (!string.IsNullOrEmpty(RedirectUrl))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ public override TokenCredential GetTokenCredential(CommonCredentialTestConfig co
{
Transport = config.Transport,
DisableInstanceDiscovery = config.DisableInstanceDiscovery,
AdditionallyAllowedTenants = config.AdditionallyAllowedTenants
AdditionallyAllowedTenants = config.AdditionallyAllowedTenants,
IsSupportLoggingEnabled = config.IsSupportLoggingEnabled,
};
var pipeline = CredentialPipeline.GetInstance(options);
return InstrumentClient(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ public override TokenCredential GetTokenCredential(CommonCredentialTestConfig co
{
AdditionallyAllowedTenants = config.AdditionallyAllowedTenants,
TenantId = config.TenantId,
IsSupportLoggingEnabled = config.IsSupportLoggingEnabled,
};
var (_, _, processOutput) = CredentialTestHelpers.CreateTokenForAzureCli();
var testProcess = new TestProcess { Output = processOutput };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ public override TokenCredential GetTokenCredential(CommonCredentialTestConfig co
{
AdditionallyAllowedTenants = config.AdditionallyAllowedTenants,
TenantId = config.TenantId,
IsSupportLoggingEnabled = config.IsSupportLoggingEnabled,
};
var (_, _, processOutput) = CredentialTestHelpers.CreateTokenForAzureDeveloperCli();
var testProcess = new TestProcess { Output = processOutput };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public override TokenCredential GetTokenCredential(CommonCredentialTestConfig co
{
AdditionallyAllowedTenants = config.AdditionallyAllowedTenants,
TenantId = config.TenantId,
IsSupportLoggingEnabled = config.IsSupportLoggingEnabled,
};
var (_, _, processOutput) = CredentialTestHelpers.CreateTokenForAzurePowerShell(TimeSpan.FromSeconds(30));
var testProcess = new TestProcess { Output = processOutput };
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Threading.Tasks;
using Azure.Core;
using Azure.Identity.Tests.Mock;
using NUnit.Framework;

namespace Azure.Identity.Tests
Expand Down Expand Up @@ -32,11 +29,14 @@ public override TokenCredential GetTokenCredential(CommonCredentialTestConfig co
{
Transport = config.Transport,
DisableInstanceDiscovery = config.DisableInstanceDiscovery,
AdditionallyAllowedTenants = config.AdditionallyAllowedTenants
AdditionallyAllowedTenants = config.AdditionallyAllowedTenants,
IsSupportLoggingEnabled = config.IsSupportLoggingEnabled,
};
var pipeline = CredentialPipeline.GetInstance(options);
options.Pipeline = pipeline;
return InstrumentClient(new ClientAssertionCredential(config.TenantId, ClientId, () => "assertion", options));
var cred = new ClientAssertionCredential(config.TenantId, ClientId, () => "assertion", options);
var instrumented = InstrumentClient(cred);
return instrumented;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ public override TokenCredential GetTokenCredential(CommonCredentialTestConfig co
{
Transport = config.Transport,
DisableInstanceDiscovery = config.DisableInstanceDiscovery,
AdditionallyAllowedTenants = config.AdditionallyAllowedTenants
AdditionallyAllowedTenants = config.AdditionallyAllowedTenants,
IsSupportLoggingEnabled = config.IsSupportLoggingEnabled,
};
var pipeline = CredentialPipeline.GetInstance(options);
var certificatePath = Path.Combine(TestContext.CurrentContext.TestDirectory, "Data", "cert.pfx");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ public override TokenCredential GetTokenCredential(CommonCredentialTestConfig co
{
Transport = config.Transport,
DisableInstanceDiscovery = config.DisableInstanceDiscovery,
AdditionallyAllowedTenants = config.AdditionallyAllowedTenants
AdditionallyAllowedTenants = config.AdditionallyAllowedTenants,
IsSupportLoggingEnabled = config.IsSupportLoggingEnabled,
};
var pipeline = CredentialPipeline.GetInstance(options);
return InstrumentClient(new ClientSecretCredential(config.TenantId, ClientId, "secret", options, pipeline, null));
Expand Down
57 changes: 57 additions & 0 deletions sdk/identity/Azure.Identity/tests/CredentialTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,63 @@ public async Task IsAccountIdentifierLoggingEnabled([Values(true, false)] bool i
}
}

[Test]
public async Task RespectsIsSupportLoggingEnabled([Values(true, false)] bool isSupportLoggingEnabled)
{
using var _listener = new TestEventListener();
_listener.EnableEvents(AzureIdentityEventSource.Singleton, EventLevel.Verbose);

var token = Guid.NewGuid().ToString();
var idToken = CredentialTestHelpers.CreateMsalIdToken(Guid.NewGuid().ToString(), "userName", TenantId);
bool calledDiscoveryEndpoint = false;
bool isPubClient = false;
var mockTransport = new MockTransport(req =>
{
calledDiscoveryEndpoint |= req.Uri.Path.Contains("discovery/instance");

MockResponse response = new(200);
if (req.Uri.Path.EndsWith("/devicecode"))
{
response = CredentialTestHelpers.CreateMockMsalDeviceCodeResponse();
}
else if (req.Uri.Path.Contains("/userrealm/"))
{
response.SetContent(UserrealmResponse);
}
else
{
if (isPubClient || typeof(TCredOptions) == typeof(AuthorizationCodeCredentialOptions))
{
response = CredentialTestHelpers.CreateMockMsalTokenResponse(200, token, TenantId, ExpectedUsername, ObjectId);
}
else
{
response.SetContent($"{{\"token_type\": \"Bearer\",\"expires_in\": 9999,\"ext_expires_in\": 9999,\"access_token\": \"{token}\" }}");
}
}

return response;
});

var config = new CommonCredentialTestConfig()
{
Transport = mockTransport,
TenantId = TenantId,
IsSupportLoggingEnabled = isSupportLoggingEnabled
};
var credential = GetTokenCredential(config);
if (!CredentialTestHelpers.IsMsalCredential(credential))
{
Assert.Ignore($"{credential.GetType().Name} is not an MSAL credential.");
}
isPubClient = CredentialTestHelpers.IsCredentialTypePubClient(credential);
AccessToken actualToken = await credential.GetTokenAsync(new TokenRequestContext(MockScopes.Default, null), default);

Assert.AreEqual(token, actualToken.Token);
string expectedPrefix = isSupportLoggingEnabled ? "True" : "False";
Assert.True(_listener.EventData.Any(d => d.Payload.Any(p => p.ToString().StartsWith($"{expectedPrefix} MSAL"))));
}

[Test]
[NonParallelizable]
public async Task DisableInstanceMetadataDiscovery([Values(true, false)] bool disable)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
using Azure.Identity.Tests.Mock;
using Microsoft.Identity.Client;
using NUnit.Framework;
using Castle.DynamicProxy;

namespace Azure.Identity.Tests
{
Expand Down
12 changes: 2 additions & 10 deletions sdk/identity/Azure.Identity/tests/DeviceCodeCredentialTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ public override TokenCredential GetTokenCredential(CommonCredentialTestConfig co
{
Transport = config.Transport,
AdditionallyAllowedTenants = config.AdditionallyAllowedTenants,
DisableInstanceDiscovery = config.DisableInstanceDiscovery
DisableInstanceDiscovery = config.DisableInstanceDiscovery,
IsSupportLoggingEnabled = config.IsSupportLoggingEnabled,
};
var pipeline = CredentialPipeline.GetInstance(options);
return InstrumentClient(new DeviceCodeCredential((code, _) =>
Expand Down Expand Up @@ -97,15 +98,6 @@ public async Task AuthenticateWithDeviceCodeMockAsync([Values(null, TenantIdHint
Assert.AreEqual(token.Token, expectedToken);
}

[Test]
public void RespectsIsPIILoggingEnabled([Values(true, false)] bool isLoggingPIIEnabled)
{
var credential = new DeviceCodeCredential(new DeviceCodeCredentialOptions { IsLoggingPIIEnabled = isLoggingPIIEnabled });

Assert.NotNull(credential.Client);
Assert.AreEqual(isLoggingPIIEnabled, credential.Client.IsPiiLoggingEnabled);
}

[Test]
[NonParallelizable]
public async Task AuthenticateWithDeviceCodeNoCallback()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ public override TokenCredential GetTokenCredential(CommonCredentialTestConfig co
var options = new EnvironmentCredentialOptions
{
Transport = config.Transport,
DisableInstanceDiscovery = config.DisableInstanceDiscovery
DisableInstanceDiscovery = config.DisableInstanceDiscovery,
IsSupportLoggingEnabled = config.IsSupportLoggingEnabled,
};

var pipeline = CredentialPipeline.GetInstance(options);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ public override TokenCredential GetTokenCredential(CommonCredentialTestConfig co
var options = new EnvironmentCredentialOptions
{
Transport = config.Transport,
DisableInstanceDiscovery = config.DisableInstanceDiscovery
DisableInstanceDiscovery = config.DisableInstanceDiscovery,
IsSupportLoggingEnabled = config.IsSupportLoggingEnabled,
};

var pipeline = CredentialPipeline.GetInstance(options);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public override TokenCredential GetTokenCredential(CommonCredentialTestConfig co
{
Transport = config.Transport,
DisableInstanceDiscovery = config.DisableInstanceDiscovery,
IsSupportLoggingEnabled = config.IsSupportLoggingEnabled,
};

return InstrumentClient(new EnvironmentCredential(options));
Expand Down
Loading