GetNetworkCredential() returns null if PSCredential has null or empty user name

[iSazonov]

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

[PaulHigin]

I agree that this should return $null rather than throw a NR exception. I have just one minor test comment.

[PaulHigin]

Shouldn't the test be: "[PSCredential]::Empty.GetNetworkCredential() | Should Be $Null"?

[TravisEz13]

A agree this would make any error, have more information to debug the issue. Therefore making a rerun less likely to be needed.

[iSazonov]

Fixed.

[TravisEz13]

@Indhukrishna FYI

[TravisEz13]

Left a comment. I think at least one other test case needs to be added.

[TravisEz13]

Should we add a case for an empty username? How do you get the password from the object if GetNetworkCredential returns $null

[iSazonov]

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.

[daxian-dbw]

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.

[iSazonov]

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?

[daxian-dbw]

Why we allow the sample?

Do you mean the XML comment? I missed the comment, and it looks totally fine to return null then :)

[iSazonov]

I meant your sample:

$psobj = [pscustomobject] @{}
$cred = [pscredential]::new($psobj)

Why the constructor don't throw? It seems the constructor should throw.

[daxian-dbw]

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.

[iSazonov]

For the PR I think we can leave the constructor as is.

Please clarify what is your conclusion about the PR fix?

[daxian-dbw]

Based on the existing XML comments for GetNetworkCredential(), I withdraw my comment about making GetNetworkCredential throw. Returning null is totally fine.

[PaulHigin]

LGTM

Agree with the consensus