allow for explicit login credentials #161
Conversation
|
@jaredcnance, It will cover your contributions to all .NET Foundation-managed open source projects. |
| { | ||
| try | ||
| { | ||
| await _armManager.Initialize(); |
There was a problem hiding this comment.
Under the hood, this calls ArmManager.SelectTenantAsync, which calls _authHelper.GetToken and _authHelper.AcquireTokens() which I don't think makes much sense if there is a login command. this should also resolve #112
There was a problem hiding this comment.
the reason for having this is to prompt to login if the user never actually called func azure login, or if the cached token has expired. otherwise everytime you run any other command it'll fail until you run login again.
There was a problem hiding this comment.
added an optional requiresLogin param to the constructor.
| if (string.IsNullOrWhiteSpace(_username)) | ||
| await _armManager.LoginAsync(); | ||
| else | ||
| await _armManager.LoginAsync(_username, _password); |
There was a problem hiding this comment.
may want to prompt if the username is provided but the password is empty?
also, it may be convenient to check the environment as well, where the priority order is something like:
- use parameters
- no parameters, use environment (
AZURE_USERandAZURE_PASSWORD) - no env vars, prompt with interactive dialog, logging a message to the console to indicate action is required
|
@jaredcnance, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request. |
this removes the requirement that a CLI only login makes the password visible on the display. the password can still be supplied using the (-w) option
|
I'm very sorry, some how I missed the notification for this PR, and just saw it now. I'll take a look at it today. |
| password.Append(key.KeyChar); | ||
| } | ||
| Console.Write(Environment.NewLine); | ||
| return password.ToString(); |
There was a problem hiding this comment.
There is SecurityHelpers.ReadPassword() which is similar to this method.
|
|
||
| Parser.Setup<string>('w') | ||
| .WithDescription("password") | ||
| .Callback(password => _password = password); |
There was a problem hiding this comment.
Can you add a long option for these as well for consistency with other flags?
Maybe the description can be Username to use for non-interactive login. Note that accounts with 2 factor-auth require interactive login. Default: interactive and Password to use for non-interactive login only in conjunction with -u. Default: prompt
|
@ahmelsayed any thoughts on the updates? |
|
@jaredcnance sorry about the delay. I fixed my notifications for this repo which weren't making it to my email for some reason. Sorry I don't mean to keep pushing back, but why not just make |
|
pushback is good 😄 the only reason is so it can call |
|
ah, you're right. I forgot about that. Your change actually makes more sense now as login is just one form of initialization. |
|
Merged - Thank you very much for your contributions, @jaredcnance :) |
Closes #160
If you guys are cool with this, let me know and I'll get some tests written.