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.
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 support for authenticating using azureauth cli #1464
Add support for authenticating using azureauth cli #1464
Changes from 3 commits
7c192e8
6a09a23
7780f0e
40c0186
d8d1c77
69eb5a5
3d18828
642ef26
7b017d6
54ae30f
7cf30e6
7a52b53
1e0ed15
f11f043
bd58766
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Broker would also need it's own
cfg
atribute right?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.
@demoray ping.
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.
Could we take
scopes: &Vec<String>
or similar here instead of a singleresource
?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.
@demoray After reading the
TokenCredential
trait source, I see thatresource
here is actually a space separated list of scopes, or potentially the concept of a resource as parsed by theaz CLI
. But MSAL only takes a list of scopes, which AzureAuth will turn --resource into, but we recommend passing--scope
repeatedly for each scope needed.I think we need to test this a little further and likely split this "resource" (maybe rename it scopes), and pass each value as it's own
--scope
for azureauth to correctly handle them.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.
I would prefer to do this as a follow-up PR, as I think the underlying trait should be updated to take a list of scopes.
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.
So long as this doesn't ship, a follow up PR seems okay. I do not think this will work as is.