Skip to content

Modifies error messages of AvoidUsernameAndPasswordParams and UsePSCredentialType rules. #456

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

Merged
merged 4 commits into from
Mar 16, 2016

Conversation

kapilmb
Copy link

@kapilmb kapilmb commented Mar 4, 2016

This change is Review on Reviewable

@KirkMunro
Copy link
Contributor

Hi again, I have one suggestion regarding the text changes for this pull request. In PowerShell 3.0 and 4.0, the order is important. In PowerShell 5.0, the order is not important. However, in all versions, it is important that both a PSCredential type attribute and a Credential conversion attribute are used, and in the right way. With the text changed as is, I felt that last point might not be clear to an end user because it indicates it doesn't apply in PowerShell 5.0 or later. I haven't looked too deeply, so maybe this is ok, but I wanted to mention it to make sure that the text conveys (a) that the Credential conversion attribute should be used with parameters of type PSCredential, and (b) that the Credential conversion attribute must be placed after the PSCredential type attribute in PowerShell versions 4.0 and earlier.

@kapilmb
Copy link
Author

kapilmb commented Mar 5, 2016

In PowerShell 5.0, I found that the following 3 cases behave identically when a string is passed as an argument to the Credential parameter.

[PSCredential] 
$Credential

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

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

It appears that in PS 5.0 the interpreter converts a string to PSCredential type even when CredentialAttribute is not specified.

@KirkMunro
Copy link
Contributor

That must be some magic done in the PowerShell interpreter then, because I'm using PowerShell 5.0.10586.122 and when I invoke Connect-MsolService -Credential foo (a compiled cmdlet), I receive the following error:

Connect-MsolService : Cannot bind parameter 'Credential'. Cannot convert the "foo" value of type "System.String" to type "System.Management.Automation.PSCredential".
At line:1 char:33
+ connect-msolservice -Credential foo
+                                 ~~~
    + CategoryInfo          : InvalidArgument: (:) [Connect-MsolService], ParameterBindingException
    + FullyQualifiedErrorId : CannotConvertArgumentNoMessage,Microsoft.Online.Administration.Automation.ConnectMsolService

Very cool to see this being addressed in native PowerShell. It would be great if this rule (or a similar one) would warn on binary module commands in the future as well.

@kapilmb
Copy link
Author

kapilmb commented Mar 10, 2016

@KirkMunro Thank you very much for the feedback on this issue.

Since PSScriptAnalyzer doesn't support binary module analysis , do we agree on the information provided in the error message?

@@ -217,16 +217,16 @@
<value>One Char</value>
</data>
<data name="UsePSCredentialTypeDescription" xml:space="preserve">
<value>Checks that cmdlets that have a Credential parameter accept PSCredential with CredentialAttribute where PSCredential comes before CredentialAttribute.. This comes from the PowerShell teams best practices.</value>
<value>Checks if a credential parameter of type PSCredential has a CredentialAttribute attribute such that PSCredential precedes CredentialAttribute. This is not applicable to PowerShell version 5.0 or above. </value>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks if a parameter named Credential is of type PSCredential. For PowerShell 4 and earlier, also checks if there is a Credential transformation attribute defined after the PSCredential type attribute.

@KirkMunro
Copy link
Contributor

I just added line comments for each of the messages showing what I think they should look like. The general statement "This is applicable only to PowerShell version less than 5.0" was too general IMHO. Only the Credential transformation attribute details aren't necessary in PowerShell 5.0+. The other details (about SecureString and about the PSCredential type attribute) apply to all versions. With that in mind, I provided updated messages that I think make that easier to understand. That's just my opinion though.

@kapilmb
Copy link
Author

kapilmb commented Mar 11, 2016

@KirkMunro Thanks for the detailed comments. The modifications that you suggest definitely make the error messages much clearer and easier to understand. I'll make the necessary changes and finalize the pull request. Thanks again.

@raghushantha
Copy link
Member

:


Review status: 0 of 5 files reviewed at latest revision, 9 unresolved discussions.


Comments from the review on Reviewable.io

@raghushantha
Copy link
Member

:lgtm:


Comments from the review on Reviewable.io

@raghushantha
Copy link
Member

Reviewed 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 8 unresolved discussions.


Comments from the review on Reviewable.io

@KirkMunro
Copy link
Contributor

:lgtm:


Review status: 2 of 7 files reviewed at latest revision, 8 unresolved discussions.


Comments from the review on Reviewable.io

@KirkMunro
Copy link
Contributor

Reviewed 2 of 5 files at r3.
Review status: 4 of 7 files reviewed at latest revision, 8 unresolved discussions.


Comments from the review on Reviewable.io

kapilmb pushed a commit that referenced this pull request Mar 16, 2016
Modifies error messages of AvoidUsernameAndPasswordParams and UsePSCredentialType rules.
@kapilmb kapilmb merged commit d43a478 into development Mar 16, 2016
@kapilmb
Copy link
Author

kapilmb commented Mar 16, 2016

@KirkMunro and @raghushantha Thanks for reviewing!

@kapilmb kapilmb deleted the FixCredentialUsageErrorMessage branch March 16, 2016 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants