[Identity] Always include the Broker as part of DAC #52189
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JonathanCrd
requested review from
maorleger,
pvaneck,
chlowell,
xiangyan99,
anannya03 and
minhanh-phan
github-project-automation
bot
moved this to Untriaged
in Azure Identity SDK Improvements
There was a problem hiding this comment.
Pull Request Overview
This PR changes the DefaultAzureCredential (DAC) factory to always include the Broker credential in the credential chain, even when the Azure.Identity.Broker package is not installed. Instead of throwing an error during DAC construction, the error is now deferred until GetToken() is called, matching the behavior of VisualStudioCodeCredential.
Key changes:
- Introduces a new internal
BrokerCredentialtype that handles broker availability checks at token request time - Updates the credential factory to unconditionally include broker credentials in the chain
- Modifies tests to account for the additional credential in the chain
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
DefaultAzureCredentialTests.cs |
Updates test assertions to expect 9/10 credentials instead of 8/9, adds broker credential exclusion option |
DefaultAzureCredentialFactoryTests.cs |
Adds broker credential to test matrices, removes broker package availability checks, updates credential count expectations |
DefaultAzureCredentialFactory.cs |
Simplifies broker credential creation by always including it in the chain, removes package availability checks |
BrokerCredential.cs |
New internal credential type that defers broker availability checking to token request time |
DevelopmentBrokerOptionsTests.cs |
Updates test to use simplified broker credential creation method |
DefaultAzureCredentialFactoryTests.cs (Broker package) |
Updates test assertion to expect BrokerCredential type instead of InteractiveBrowserCredential |
sdk/identity/Azure.Identity/src/Credentials/BrokerCredential.cs
Outdated
Show resolved
Hide resolved
sdk/identity/Azure.Identity/src/Credentials/BrokerCredential.cs
Outdated
Show resolved
Hide resolved
sdk/identity/Azure.Identity/src/Credentials/BrokerCredential.cs
Outdated
Show resolved
Hide resolved
christothes
reviewed
Aug 22, 2025
sdk/identity/Azure.Identity/src/Credentials/BrokerCredential.cs
Outdated
Show resolved
Hide resolved
christothes
reviewed
Aug 22, 2025
sdk/identity/Azure.Identity/src/Credentials/BrokerCredential.cs
Outdated
Show resolved
Hide resolved
christothes
reviewed
Aug 22, 2025
sdk/identity/Azure.Identity/src/Credentials/BrokerCredential.cs
Outdated
Show resolved
Hide resolved
scottaddie
reviewed
Aug 22, 2025
sdk/identity/Azure.Identity.Broker/tests/DefaultAzureCredentialFactoryTests.cs
Outdated
Show resolved
Hide resolved
Moved logic for mapping credential options to broker options from BrokerCredential to a new CredentialOptionsMapper class. This improves code organization and reusability by centralizing the mapping logic.
christothes
reviewed
Aug 29, 2025
sdk/identity/Azure.Identity.Broker/tests/DevelopmentBrokerOptionsTests.cs
Outdated
Show resolved
Hide resolved
christothes
reviewed
Aug 29, 2025
sdk/identity/Azure.Identity/src/Credentials/CredentialOptionsMapper.cs
Outdated
Show resolved
Hide resolved
christothes
reviewed
Aug 29, 2025
sdk/identity/Azure.Identity/src/Credentials/CredentialOptionsMapper.cs
Outdated
Show resolved
Hide resolved
christothes
reviewed
Aug 29, 2025
sdk/identity/Azure.Identity/src/Credentials/ISupportsTenantId.cs
Outdated
Show resolved
Hide resolved
christothes
reviewed
Aug 29, 2025
| @@ -343,22 +332,14 @@ public virtual TokenCredential CreateInteractiveBrowserCredential() | |||
| Pipeline); | |||
| } | |||
|
|
|||
| public TokenCredential CreateBrokerCredential(InteractiveBrowserCredentialOptions brokerOptions) | |||
| internal TokenCredential CreateBrokerCredential() | |||
There was a problem hiding this comment.
Yes, it's used in this factory class to add the Broker into the chain
There was a problem hiding this comment.
What I meant was all it does is return new BrokerCredential(options) - can callers just do that themselves?
christothes
reviewed
Aug 29, 2025
sdk/identity/Azure.Identity/tests/TokenCredentialOptionsTests.cs
Outdated
Show resolved
Hide resolved
christothes
reviewed
Aug 29, 2025
sdk/identity/Azure.Identity/tests/TokenCredentialOptionsTests.cs
Outdated
Show resolved
Hide resolved
christothes
reviewed
Sep 4, 2025
| @@ -5,13 +5,14 @@ | |||
| using System.Collections.Generic; | |||
| using System.Threading; | |||
| using System.Threading.Tasks; | |||
| using Azure.Identity.Credentials; | |||
There was a problem hiding this comment.
All these added using statements can be removed.
christothes
approved these changes
Sep 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #52138
This PR changes the DAC Factory to always include the Broker even if the Azure.Identity.Broker package is not installed. In order to do this, a new internal
BrokerCredentialtype is used, so instead of throwing an error during the construction of DAC, an error will be thrown whenGetToken()is called.This is the same Behaviour as
VisualStudioCodeCredential.