ManagedIdentityTokenProvider change #6637
Conversation
bainian12345
commented
Jun 19, 2019
- Allow connection string to accept both "Managed Identity" and "ManagedIdentity"
- Remove default authority for ManagedIdentityTokenProvider
Pull latest master
pull latest master
| @@ -25,7 +20,7 @@ public class AzureActiveDirectoryTokenProvider : TokenProvider | |||
| public AzureActiveDirectoryTokenProvider(AuthenticationCallback authenticationCallback, string authority, object state) | |||
| { | |||
| this.AuthCallback = authenticationCallback ?? throw Fx.Exception.ArgumentNull(nameof(authenticationCallback)); | |||
| this.authority = authority ?? CommonAuthority; | |||
| this.authority = authority; | |||
There was a problem hiding this comment.
Should we consider validating the authority as not null or empty? Seems better to catch that up front then failing when trying to acquire a token later.
| @@ -79,7 +79,7 @@ public static TokenProvider CreateSharedAccessSignatureTokenProvider(string keyN | |||
| /// <returns>The <see cref="Microsoft.ServiceBus.TokenProvider" /> for returning Json web token.</returns> | |||
| public static TokenProvider CreateAzureActiveDirectoryTokenProvider( | |||
| AzureActiveDirectoryTokenProvider.AuthenticationCallback authCallback, | |||
| string authority = AzureActiveDirectoryTokenProvider.CommonAuthority, | |||
| string authority, | |||
There was a problem hiding this comment.
Should we consider validating the authority as not null or empty? Seems better to catch that up front then failing when trying to acquire a token later.
sdk/servicebus/Microsoft.Azure.ServiceBus/src/ServiceBusConnectionStringBuilder.cs
Show resolved
Hide resolved
sdk/servicebus/Microsoft.Azure.ServiceBus/src/ServiceBusConnectionStringBuilder.cs
Show resolved
Hide resolved
sdk/servicebus/Microsoft.Azure.ServiceBus/tests/ServiceBusConnectionStringBuilderTests.cs
Show resolved
Hide resolved
| { | ||
| public const string ManagedIdentity = "Managed Identity"; | ||
| None, |
There was a problem hiding this comment.
None [](start = 12, length = 4)
This is interesting. For customers who are using SASTokens or SASKeys, this authenticationType would be None. Which is quite confusing.
There was a problem hiding this comment.
There was a problem hiding this comment.
The initial implementation is also string-based, but it is in the process of being given an overhaul, where the new API surface will simply take a token credential that the Azure.Identity library is responsible for producing. As a general rule, authentication should be an external concern; authorization against the resource is an internal one.
| { | ||
| connectionStringBuilder.Append(AuthenticationConfigName).Append(KeyValueSeparator).Append(this.Authentication).Append(KeyValuePairDelimiter); | ||
| connectionStringBuilder.Append(AuthenticationConfigName).Append(KeyValueSeparator).Append("Managed Identity").Append(KeyValuePairDelimiter); |
There was a problem hiding this comment.
"Managed Identity" [](start = 106, length = 18)
Can we convert this to a constant on top of this file?
