Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdded support for title in get-Credential, and message set to optional. #1904
Conversation
Added possibility to add title to the credential prompt window. Added possibility to only supply username without giving a message.
msftclas
commented
Aug 18, 2016
|
Hi @McAndersDK, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
msftclas
commented
Aug 18, 2016
|
@McAndersDK, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
Missing ) and changed or to ||
|
Your code does not compile. Please fix before we review this PR. |
|
it build on my machine? |
|
@McAndersDK you can see logs from the build server by clicking on the build status in your PR (red cross) For example, here The compilation error message
|
Fixed wrong caps in username
|
Thanks @vors for some reason appveyor only showed the first 200 lines here. |
Fix Scoping on caption.
|
@McAndersDK it should not be the case in general, but could be a one-off problem with AppVeyor. |
|
Thanks for fixing the build errors. Before accepting this PR, we'll need some tests. I was going to accept this PR without tests because @JamesWTruher has ported our After that PR is accepted, I'll ask you to add some tests for your new functionality. |
| caption = UtilsStrings.PromptForCredential_DefaultCaption; | ||
| } | ||
|
|
||
| if (!string.IsNullOrEmpty(this.Message) || !string.IsNullOrEmpty(this.UserName)) |
This comment has been minimized.
This comment has been minimized.
daxian-dbw
Aug 19, 2016
Member
if only -Title is specified, the code path seems cannot get to this.Host.UI.PromptForCredential
| @@ -86,14 +112,22 @@ public GetCredentialCommand() : base() | ||
| /// </summary> | ||
| protected override void BeginProcessing() | ||
| { | ||
| if (!string.IsNullOrEmpty(this.Message)) | ||
| string caption; |
This comment has been minimized.
This comment has been minimized.
| @@ -16,7 +16,7 @@ namespace Microsoft.PowerShell.Commands | ||
| /// operations involving security. | ||
| /// </summary> | ||
| [Cmdlet(VerbsCommon.Get, "Credential", DefaultParameterSetName = GetCredentialCommand.credentialSet, HelpUri = "http://go.microsoft.com/fwlink/?LinkID=113311")] | ||
| [OutputType(typeof(PSCredential), ParameterSetName = new string[] { GetCredentialCommand.credentialSet, GetCredentialCommand.messageSet })] | ||
| [OutputType(typeof(PSCredential), ParameterSetName = new string[] { GetCredentialCommand.credentialSet, GetCredentialCommand.messageSet, GetCredentialCommand.titleSet, GetCredentialCommand.usernameSet })] |
This comment has been minimized.
This comment has been minimized.
daxian-dbw
Aug 19, 2016
Member
It feels not necessary to create two more parameter sets.
How about making Message not mandatory? It won't be a breaking change to existing scripts.
Then you can add Title to the existing MessageSet and the ValidateNotNullOrEmpty attribute can be added to Message and Title.
| } | ||
| else { | ||
| caption = UtilsStrings.PromptForCredential_DefaultCaption; | ||
| } |
This comment has been minimized.
This comment has been minimized.
daxian-dbw
Aug 19, 2016
Member
If you add ValidateNotNullOrEmpty attribute to Title, then this can be turned to
string caption = this.Title ?? UtilsStrings.PromptForCredential_DefaultCaption;
|
@daxian-dbw Thanks for input, I will make sure this will go into the PR with the test for the changes. |
| @@ -86,14 +112,22 @@ public GetCredentialCommand() : base() | ||
| /// </summary> | ||
| protected override void BeginProcessing() | ||
| { | ||
| if (!string.IsNullOrEmpty(this.Message)) | ||
| string caption; | ||
| if(!string.IsNullOrEmpty(this.Title)) { |
This comment has been minimized.
This comment has been minimized.
rkeithhill
Aug 20, 2016
Contributor
Looks like this code based uses a space between the if keyword and the opening paren. To be consistent with the rest of the code, you should add the space.
|
Can you add a test to test\powershell\Modules\Microsoft.PowerShell.Security\GetCredential.Tests.ps1 and fix the formatting as @rkeithhill suggested (though we'd fix that with a tool later anyway). |
|
yeah I will. Should I create a new PR with both file in it or just create a new one with the test separate? |
|
Yeah i need to add some more tests with the streams. Please give me some days to add them in. |
|
Awesome - thanks for being responsive to our feedback. |
Also added test for empty username.
|
Travis just timed out? |
| $cred.gettype().FullName | Should Be "System.Management.Automation.PSCredential" | ||
| $netcred = $cred.GetNetworkCredential() | ||
| $netcred.UserName | Should be "Joe" | ||
| $netcred.Password | Should be "this is a test" | ||
| # Resx file only include english language, this should be changed to reflect Current Culture if title change by language. | ||
| $th.ui.Streams.Prompt[-1] | should be "Credential:Windows PowerShell credential request.:Foo" | ||
| } |
This comment has been minimized.
This comment has been minimized.
lzybkr
Aug 30, 2016
Member
We do run tests on localized builds of Windows, so this test would fail on those builds.
In some tests, if the message is important, we'll actually load the resource to get the text - that works on localized builds as well. We might not have any examples of that pattern in out pester tests though.
| $th.ui.Streams.Prompt[-1] | should be "Credential:CustomTitle:Foo" | ||
| } | ||
| It "Get-Credential with empty username, should throw" { | ||
| $cred = $ps.AddScript("Get-Credential -UserName ''").Invoke() | should throw | ||
| } |
This comment has been minimized.
This comment has been minimized.
lzybkr
Aug 30, 2016
Member
A 'should throw' test always has pipes a scriptblock, e.g.
{ $ps.AddScript("Get-Credential -UserName ''").Invoke() } | should throw|
@lzybkr That did not work. we need to test the invoke for credential object?, if none throw? |
|
The problem with not using a script block is that if it doesn't throw, the test passes but should fail. Adding the change I suggested exposed the fact that no exception was thrown, so failing the test is correct. I think this test might not make sense. Looking at the console host - we prompt for the username if it's null or empty. Other hosts might do that or throw, but we don't really control other hosts. I'd say just remove this test. |
|
Set "message" to optional is not best practice, from https://msdn.microsoft.com/en-us/library/dd878348%28v=vs.85%29.aspx : Set both "message" and "title" to optional open a way to "anonymous" request of credential. That is bad. This provokes writing bad code. |
|
Before this change, you could call The current experience (after this PR) with |
Get-Credential with no parameters set message to default "Enter you credentials". Message is in another parameter set and that is another experience. |



McAndersDK commentedAug 18, 2016
Added possibility to add title to the credential prompt window.
Added possibility to only supply username without giving a message.