The Wayback Machine - https://web.archive.org/web/20200923073229/https://github.com/PowerShell/PowerShell/pull/1904
Skip to content
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

Added support for title in get-Credential, and message set to optional. #1904

Merged
merged 14 commits into from Sep 16, 2016

Conversation

@McAndersDK
Copy link
Contributor

McAndersDK commented Aug 18, 2016

Added possibility to add title to the credential prompt window.
Added possibility to only supply username without giving a message.

Added possibility to add title to the credential prompt window.
Added possibility to only supply username without giving a message.
@msftclas
Copy link

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!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@msftclas
Copy link

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.

Thanks, MSBOT;

@msftclas msftclas added cla-signed and removed cla-required labels Aug 18, 2016
Missing ) and changed or to ||
@McAndersDK McAndersDK changed the title Update CredentialCommands.cs Added support for title in get-Credential, and message set to optional. Aug 18, 2016
@lzybkr
Copy link
Member

lzybkr commented Aug 18, 2016

Your code does not compile. Please fix before we review this PR.

@McAndersDK
Copy link
Contributor Author

McAndersDK commented Aug 19, 2016

it build on my machine?
I cant see in the build details what is wrong? @lzybkr

@vors
Copy link
Collaborator

vors commented Aug 19, 2016

@McAndersDK you can see logs from the build server by clicking on the build status in your PR (red cross)

For example, here
https://ci.appveyor.com/project/PowerShell/powershell/build/6.0.0-alpha.9-36#L276

The compilation error message

C:\projects\powershell\src\Microsoft.PowerShell.Security\security\CredentialCommands.cs(122,83): error CS1061: 'GetCredentialCommand' does not contain a definition for 'Username' and no extension method 'Username' accepting a 
first argument of type 'GetCre
dentialCommand' could be found (are you missing a using directive or an assembly reference?)
C:\projects\powershell\src\Microsoft.PowerShell.Security\security\CredentialCommands.cs(124,40): error CS1061: 'GetCredentialCommand' does not contain a definition for 'Username' and no extension method 'Username' accepting a 
first argument of type 'GetCre
dentialCommand' could be found (are you missing a using directive or an assembly reference?)
C:\projects\powershell\src\Microsoft.PowerShell.Security\security\CredentialCommands.cs(129,67): error CS0103: The name 'caption' does not exist in the current context
Fixed wrong caps in username
@McAndersDK
Copy link
Contributor Author

McAndersDK commented Aug 19, 2016

Thanks @vors for some reason appveyor only showed the first 200 lines here.
I had to download the log to see the complete output.

Fix Scoping on caption.
@vors
Copy link
Collaborator

vors commented Aug 19, 2016

@McAndersDK it should not be the case in general, but could be a one-off problem with AppVeyor.

@lzybkr
Copy link
Member

lzybkr commented Aug 19, 2016

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 Get-Credential is a bit harder to test than some cmdlets, but...

@JamesWTruher has ported our Get-Credential to Pester, hopefully we'll see a PR from him today.

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.

@daxian-dbw

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.

@daxian-dbw

daxian-dbw Aug 19, 2016 Member

please fix the indention before string caption

@@ -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.

@daxian-dbw

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.

@daxian-dbw

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; 
@McAndersDK
Copy link
Contributor Author

McAndersDK commented Aug 19, 2016

@daxian-dbw Thanks for input, I will make sure this will go into the PR with the test for the changes.

@jpsnover
Copy link
Contributor

jpsnover commented Aug 20, 2016

We should have done this a long time ago. Thanks!
542

@@ -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.

@rkeithhill

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.

@lzybkr
Copy link
Member

lzybkr commented Aug 22, 2016

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).

@McAndersDK
Copy link
Contributor Author

McAndersDK commented Aug 22, 2016

yeah I will. Should I create a new PR with both file in it or just create a new one with the test separate?

@McAndersDK
Copy link
Contributor Author

McAndersDK commented Aug 25, 2016

Yeah i need to add some more tests with the streams.

Please give me some days to add them in.

@lzybkr
Copy link
Member

lzybkr commented Aug 25, 2016

Awesome - thanks for being responsive to our feedback.

Also added test for empty username.
@McAndersDK
Copy link
Contributor Author

McAndersDK commented Aug 29, 2016

Travis just timed out?
How do we rerun travis? except to do a push?

$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.

@lzybkr

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.

@lzybkr

lzybkr Aug 30, 2016 Member

A 'should throw' test always has pipes a scriptblock, e.g.

{ $ps.AddScript("Get-Credential -UserName ''").Invoke() } | should throw
@McAndersDK
Copy link
Contributor Author

McAndersDK commented Sep 1, 2016

@lzybkr That did not work. we need to test the invoke for credential object?, if none throw?
but why change this? it worked before? atleast the test did not fail, I have not tryed the other way around

@lzybkr
Copy link
Member

lzybkr commented Sep 1, 2016

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.

lzybkr and others added 2 commits Sep 15, 2016
Change test to work on any culture
@lzybkr
lzybkr approved these changes Sep 16, 2016
@lzybkr lzybkr merged commit 5738f37 into PowerShell:master Sep 16, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lzybkr lzybkr added this to the 6.0.0-alpha.11 milestone Sep 16, 2016
@iSazonov
Copy link
Collaborator

iSazonov commented Sep 19, 2016

Set "message" to optional is not best practice, from https://msdn.microsoft.com/en-us/library/dd878348%28v=vs.85%29.aspx :
•Each parameter set must have at least one unique parameter. If possible, make this parameter a mandatory parameter.

Set both "message" and "title" to optional open a way to "anonymous" request of credential. That is bad. This provokes writing bad code.

@lzybkr
Copy link
Member

lzybkr commented Sep 19, 2016

Before this change, you could call Get-Credential with no parameters, so I thought the change was necessary to keep that behavior. Maybe it wasn't.

The current experience (after this PR) with Get-Credential and no additional parameters is broken - it prompts twice (see #2306). So maybe we need to revert the change on the Message parameter.

@iSazonov
Copy link
Collaborator

iSazonov commented Sep 22, 2016

Before this change, you could call Get-Credential with no parameters

Get-Credential with no parameters set message to default "Enter you credentials". Message is in another parameter set and that is another experience.
So I think we should follow the msdn recommendations above mentioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.
Morty Proxy This is a proxified and sanitized view of the page, visit original site.