Cli credentials#7637
Conversation
daviwil
left a comment
daviwil
left a comment
There was a problem hiding this comment.
Thanks for continuing the work on this credential! Looks good overall aside from some possible improvements to the CredentialClient design.
|
|
||
| import * as child_process from "child_process"; | ||
|
|
||
| export interface CredentialClient { |
There was a problem hiding this comment.
Since this is specific to the AzureCliCredential I'd call this AzureCliCredentialClient and call the implementation DefaultAzureCliCredentialClient. Since CredentialClient is being exposed on the public interface of this library, it'll be confusing to see something with a more general name than the usage pattern implies.
There was a problem hiding this comment.
This also needs some basic documentation to make api-extractor happy.
There was a problem hiding this comment.
Fixed with the refactor
|
|
||
| /** | ||
| * Provides the user access token and expire time | ||
| * with azure cli command "az account get-access-token" in powershell. |
There was a problem hiding this comment.
Just some nits here on casing and removing an inaccuracy about it being invoked via PowerShell.
| * with azure cli command "az account get-access-token" in powershell. | |
| * with Azure CLI command "az account get-access-token". |
| .getAzureCliAccessToken(resource) | ||
| .then((obj: any) => { | ||
| if (obj.stderr) { | ||
| let isLoginError = obj.stderr.match("Please run 'az login' to setup account"); |
There was a problem hiding this comment.
| let isLoginError = obj.stderr.match("Please run 'az login' to setup account"); | |
| let isLoginError = obj.stderr.match("Please run 'az login' from a command prompt to authenticate before using this credential."); |
| obj.stderr.match("az:(.*)not found") || | ||
| obj.stderr.startsWith("'az' is not recognized"); | ||
| if (isNotInstallError) { | ||
| throw new Error("Azure CLI not Installed"); |
There was a problem hiding this comment.
| throw new Error("Azure CLI not Installed"); | |
| throw new Error("Azure CLI could not be found. Please visit https://aka.ms/azure-cli for installation instructions and then, once installed, authenticate to your Azure account using 'az login'."); |
There was a problem hiding this comment.
Yeah that's definitely better. Updating
| try { | ||
| child_process.exec( | ||
| `az account get-access-token --output json --resource ${resource}`, | ||
| (error, stdout, stderr) => { |
There was a problem hiding this comment.
I think this interface and class should return an AccessToken, not the pipes from running the command. It'd make mocking and usage easier if it was just handled in the "client" class. What do you think?
There was a problem hiding this comment.
I don't mind the stdout/stderr approach since it's mocking around the call out to the az.
daviwil
left a comment
daviwil
left a comment
There was a problem hiding this comment.
The new design is much better, thanks a bunch for doing that!
| this.getAzureCliAccessToken(resource) | ||
| .then((obj: any) => { | ||
| if (obj.stderr) { | ||
| let isLoginError = obj.stderr.match("Please run 'az login' from a command prompt to authenticate before using this credential."); |
There was a problem hiding this comment.
I don't know this API, so I have to ask: is this checking whether stderr contains that literal?
There was a problem hiding this comment.
Looks like it's a regex match. I might just change it to look for the 'az login' part.
| protected async getAzureCliAccessToken(resource: string) { | ||
| return new Promise((resolve, reject) => { | ||
| try { | ||
| child_process.exec( |
There was a problem hiding this comment.
To address security concerns, in .NET and Python we use a working directory on the path.
There was a problem hiding this comment.
I missed this comment, but I can do a follow-up PR
7 participants
Taking #6521 and building with the latest master.
With this credential, it's now possible to configure your app to get the logged in credentials via
az. Ifazcan't be found, or if the user is not logged in, then the user will get the appropriate error.Note: this is will become part of the DefaultAzureCredentials