-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[Identity] Enable brokered auth in DAC #40335
base: main
Are you sure you want to change the base?
Conversation
API change check API changes are not detected in this pull request. |
Signed-off-by: Paul Van Eck <paulvaneck@microsoft.com>
bf20396
to
e24dd7e
Compare
@xiangyan99 This is the preliminary implementation. Would be good to get your thoughts on:
|
Signed-off-by: Paul Van Eck <paulvaneck@microsoft.com>
Signed-off-by: Paul Van Eck <paulvaneck@microsoft.com>
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.
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
Files not reviewed (1)
- sdk/identity/azure-identity/dev_requirements.txt: Language not supported
sdk/identity/azure-identity/azure/identity/_credentials/default.py
Outdated
Show resolved
Hide resolved
def _get_broker_credential() -> Optional[Type["InteractiveBrowserBrokerCredential"]]: | ||
# Get the broker credential if available | ||
try: | ||
from azure.identity.broker import InteractiveBrowserBrokerCredential |
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.
We need to double check the behavior of get_token in the brokered credential.
Does it raise AuthFail or AuthUnavailable?
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.
If within DAC, the broker credential token request methods will raise CredentialUnavailableError
. However, I am noticing that if silent auth fails, we fall back to interactive sign-in. If we want this to be purely silent auth, we'd likely have to introduce some kwarg to signal the credential not to try interactive auth. WDYT?
Signed-off-by: Paul Van Eck <paulvaneck@microsoft.com>
Description
This adds
InteractiveBrowserBrokerCredential
silent flow authentication to the end of theDefaultAzureCredential
chain. Here, we opt to conditionally add it to the chain only if the broker package is installed.Additional context: https://gist.github.com/christothes/f8a0dc6249261cb36a6c452717c4e932
Closes: #39966