Psscriptanalyzer: Credential Parameter Rules

[mattmcnabb]

There are two rules, UsePSCredentialType and AvoidUserNameAndPasswordParams that need to be reworked. While it is good practice to use the "PSCredential" type for parameters that take a credential, a better practice is to use the provided parameter attribute "CredentialAttribute." Both rules previously mentioned flag parameters that use this attribute.

Example:

[System.Management.Automation.CredentialAttribute()]
$Credential

[KirkMunro]

FYI, since you're working on this right now, the best practice way to implement a Credential parameter so that it supports both credential objects as well as string objects, is as follows:

[System.Management.Automation.PSCredential]
[System.Management.Automation.Credential()]
$Credential = [System.Management.Automation.PSCredential]::Empty

This still uses strong typing, while leveraging the credential attribute (it doesn't need "Attribute" in the name) to convert from string to credential with a prompt. It also defaults to Empty, which I would only expect from a command where the credential was not mandatory.

Using this model encourages (and reminds) authors that there is such thing as an Empty credential object, which someone can pass in if they want, and which a function should handle accordingly. Most script authors don't get this implementation right, either forgetting the attribute or not supporting the Empty credential.

[mattmcnabb]

Kirk,

Can you clarify this usage a bit? For instance, why do you need two type attributes when one will work just fine?

Also, what would be a use case for an empty credential? I've seen the above default value before, but struggle to see where it would be useful.

Thanks!

[KirkMunro]

They aren't two type attributes. S.M.A.Credential() is a type converter attribute. It provides metadata to PowerShell telling it that whatever value is passed into the parameter will be converted using the specified type converter attribute. S.M.A.PSCredential is a type. It tells PowerShell to add an argument type converter attribute that will try to coerce whatever it receives into a S.M.A.PSCredential. If the value it receives cannot be coerced into that type, an error is displayed. Minor detail, but I thought it should be raised for anyone reading this thread.

While using only S.M.A.Credential() works, the parameter ends up missing the argument type converter attribute that is used by Intellisense. Adding the actual type to the parameter definition ensures that this type converter attribute is present, allowing Intellisense to work for that parameter. It also ensures the appropriate metadata is there for anything that is looking for that metadata. For example, if you run Get-Command on a command with a credential parameter that is missing the type, and then you inspect the parameters (which tools do, as does PowerShell), the type of the credential parameter will appear as System.Object (untyped).

I personally think it's a best practice to list the type because it clearly identifies what the type of the resulting object will be to someone looking at the code. In either case they may have to look up what S.M.A.Credential does, but they may be able to guess if they are familiar with the concept of converter attributes. I also think having S.M.A.Credential() by itself will likely confuse others (why does that "type" have round brackets but others do not?).

The empty credential bit is more of my personal reminder that it should be supported when PSCredential parameters are used because it is a valid value. This dates back to pre-splatting, when functions may use conditional logic to define what parameter values to pass in, and based on the condition a credential parameter may be passed to something as [S.M.A.PSCredential]::Empty, just so that the author wouldn't have to create the function invocation twice in one script.

Regardless of the past though, since it can be passed in, it should be supported, so I took to default assignment to [S.M.A.PSCredential]::Empty and then internally checking specifically for that value instead of checking for null as a best practice so that whatever I would do with my Credential parameter would work. e.g.

if ($Credential -eq [System.Management.Automation.PSCredential]::Empty) {
    $PSBoundParameters.Remove('Credential') > $null
}
<# Now PSBoundParameters can be splatted #>

Does that clear it up?

[mattmcnabb]

It does clear things up. Thanks for the detailed response! It's funny how you never really know when or where you'll find a good trick or good advice!

So by defaulting to an empty credential, we can splat $PSBoundParameters without having to check if the $Credential parameter was passed in? That would make things easier.

Now I have a bunch of scripts to re-write :(

[KirkMunro]

Yes, by defaulting you can splat directly, but you need to make sure that the function/cmdlet you're splatting into supports [S.M.A.PSCredential]::Empty (test it first!). If not, you need to use the conditional logic I provided to remove the Credential parameter if it is [S.M.A.PSCredential]::Empty first. If you don't want to test what you're calling to see if Empty is supported, then use that conditional logic every time.

[mattmcnabb]

Well, if I update a function to use Kirk's credential parameter method I no longer get flagged by the two rules mentioned. Should this issue be closed or does it need to remain open? Thanks again, Kirk!

[raghushantha]

Thanks for the awesome write-up.

Fix is available in development branch:
https://github.com/PowerShell/PSScriptAnalyzer/pull/391