GetNetworkCredential() returns null if PSCredential has null or empty user name - #4697
Conversation
2c39ad7 to
7321221
Compare
7321221 to
a82738b
Compare
Paul Higinbotham (PaulHigin)
left a comment
There was a problem hiding this comment.
I agree that this should return $null rather than throw a NR exception. I have just one minor test comment.
| Describe "Credential tests" -Tags "CI" { | ||
| It "Explicit cast for an empty credential returns null" { | ||
| # We should explicitly check that the expression returns $null | ||
| [PSCredential]::Empty.GetNetworkCredential() -eq $null | Should Be $true |
There was a problem hiding this comment.
Shouldn't the test be: "[PSCredential]::Empty.GetNetworkCredential() | Should Be $Null"?
There was a problem hiding this comment.
A agree this would make any error, have more information to debug the issue. Therefore making a rerun less likely to be needed.
Travis Plunk (TravisEz13)
left a comment
There was a problem hiding this comment.
Left a comment. I think at least one other test case needs to be added.
| @@ -0,0 +1,6 @@ | |||
| Describe "Credential tests" -Tags "CI" { | |||
| It "Explicit cast for an empty credential returns null" { | |||
There was a problem hiding this comment.
Should we add a case for an empty username? How do you get the password from the object if GetNetworkCredential returns $null
There was a problem hiding this comment.
PSCredential constructor fails if username is empty. The only way to get an empty credentials is [PSCredential]::Empty. And if we can [PSCredential]::Empty any methods shouldn't throw for the empty credential - we're just fix the issue.
There was a problem hiding this comment.
Actually, you are able to create empty PSCredential instances with a public constructor PSCredential(psobject pso)
$psobj = [pscustomobject] @{}
$cred = [pscredential]::new($psobj)The current implementation of GetNetworkCredential() doesn't allow returning a null value -- it throws if the user name is invalid. I think we should keep it that way and throw exception when user name is null or empty.
There was a problem hiding this comment.
Why we allow the sample? It seems constructor should throw in the case.
On the other hand, [PSCredential]::Empty is a valid object - why any its method should throw?
There was a problem hiding this comment.
Why we allow the sample?
Do you mean the XML comment? I missed the comment, and it looks totally fine to return null then :)
There was a problem hiding this comment.
I meant your sample:
$psobj = [pscustomobject] @{}
$cred = [pscredential]::new($psobj)
Why the constructor don't throw? It seems the constructor should throw.
There was a problem hiding this comment.
I don't know the original intention, but changing this behavior would be a breaking change for sure -- it makes something that works stop working. It feels insignificant to me, so I don't think it's worth to be fixed. But you can go both ways -- it's insignificant so a breaking change might not matter.
There was a problem hiding this comment.
For the PR I think we can leave the constructor as is.
Please clarify what is your conclusion about the PR fix?
There was a problem hiding this comment.
Based on the existing XML comments for GetNetworkCredential(), I withdraw my comment about making GetNetworkCredential throw. Returning null is totally fine.
Paul Higinbotham (PaulHigin)
left a comment
There was a problem hiding this comment.
LGTM
Agree with the consensus
…r empty user name (PowerShell#4697) * GetNetworkCredential() returns null if PSCredential has empty user name * Fix test
Fix #4696
Problem descriptions
System.Management.Automation.PSCredential allow user name being empty but explicit cast [pscredential]::Empty.GetNetworkCredential() throws.
Fix
GetNetworkCredential() returns null if PSCredential has null or empty user name