-
Notifications
You must be signed in to change notification settings - Fork 2.1k
add OS default broker auth to DAC #45891
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
Conversation
d269e1b
to
c7fc022
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds OS Broker authentication support to the DefaultAzureCredential by introducing a new OSBrokerCredential
, integrating it into the credential chains, and updating tests and documentation.
- Introduce
OSBrokerCredential
and refactorIdentityUtil
for broker availability. - Integrate
OSBrokerCredential
into both developer-only and default credential chains inDefaultAzureCredentialBuilder
. - Update existing tests to mock and assert OS broker behavior; add a new test for broker-unavailable scenarios and document troubleshooting steps.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
sdk/identity/azure-identity/src/test/java/com/azure/identity/DefaultAzureCredentialTest.java | Added OSBrokerCredential mocks and assertions to user and developer credential chain tests. |
sdk/identity/azure-identity/src/test/java/com/azure/identity/DacOsBrokerCredentialTest.java | Added new test covering the broker-unavailable error path for OSBrokerCredential . |
sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/util/IdentityUtil.java | Refactored broker availability check; added isVsCodeBrokerAuthAvailable and simplified isBrokerAvailable . |
sdk/identity/azure-identity/src/main/java/com/azure/identity/OSBrokerCredential.java | Introduced the OSBrokerCredential implementation, handling dynamic broker credential creation. |
sdk/identity/azure-identity/src/main/java/com/azure/identity/DefaultAzureCredentialBuilder.java | Integrated OSBrokerCredential into the developer credentials list. |
sdk/identity/azure-identity/TROUBLESHOOTING.md | Documented broker-authentication errors, mitigation steps, and Windows-only support note. |
Comments suppressed due to low confidence (4)
sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/util/IdentityUtil.java:182
- [nitpick] Update this comment to reflect that the method also verifies broker availability (i.e., it returns true only if the broker is available and the VS Code auth record file exists).
// Check if VS Code broker auth record file exists
sdk/identity/azure-identity/src/main/java/com/azure/identity/OSBrokerCredential.java:16
- Add JavaDoc to describe the purpose and behavior of
OSBrokerCredential
and itsgetToken
method, in line with Azure SDK public API documentation requirements.
class OSBrokerCredential implements TokenCredential {
sdk/identity/azure-identity/src/main/java/com/azure/identity/OSBrokerCredential.java:57
- [nitpick] Capitalize the beginning of this error message (e.g., "Azure-identity-broker dependency is not available...") to match sentence casing conventions used elsewhere.
new CredentialUnavailableException("azure-identity-broker dependency is not available. "
sdk/identity/azure-identity/src/test/java/com/azure/identity/DacOsBrokerCredentialTest.java:10
- Add a test for the successful (happy-path) scenario where the broker dependency is available and
getToken
returns a validAccessToken
.
public class DacOsBrokerCredentialTest {
sdk/identity/azure-identity/src/main/java/com/azure/identity/OSBrokerCredential.java
Show resolved
Hide resolved
sdk/identity/azure-identity/src/main/java/com/azure/identity/OSBrokerCredential.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
sdk/identity/azure-identity/src/test/java/com/azure/identity/DefaultAzureCredentialTest.java
Show resolved
Hide resolved
/check-enforcer override |
No description provided.