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

SupportsShouldProcess doesn't care for WhatIfPreference #1892

Open
PrzemyslawKlys opened this issue Feb 19, 2023 · 5 comments
Open

SupportsShouldProcess doesn't care for WhatIfPreference #1892

PrzemyslawKlys opened this issue Feb 19, 2023 · 5 comments
Labels
Resolution - By Design This is by design

Comments

@PrzemyslawKlys
Copy link
Contributor

Summary of the new feature
It seems the SupportShouldProcess rule doesn't always pick up WhatIf being passed to a function later on. It mostly does this for external modules (that do it properly), but not when the functions are within the same module (different files) that I am actually building.

image

Proposed technical implementation details (optional)
I guess a prescan should be made and if there are commands with explicit WhatIf calls or WhatIfPreference calls it should treat it as condition to ignore.

@ghost ghost added the Needs: Triage 🔍 label Feb 19, 2023
@bergmeister
Copy link
Collaborator

PowerShell passes WhatIfPreference on automatically (except for splatting with binary cmdlets due PowerShell/PowerShell#4568) so no need to pass like -WhatIf:$WhatIfPreference unless I am not aware of one of the many PowerShell gotchas.

@PrzemyslawKlys
Copy link
Contributor Author

From my own testing it doesn't work cross-modules (non-binary modules)

image

And even PSScriptAnalyzer doesn't detect ShouldProcess properly (it doesn't show that for binary modules such as AD/GPO and so on.

@SydneyhSmith
Copy link
Collaborator

The expectation is that the rule searches the function block for the should process implementation (not checking the cmdlets called within the function), the rule is just used to check the current function so the rule working as expected

@PrzemyslawKlys
Copy link
Contributor Author

PrzemyslawKlys commented Mar 1, 2023

That's not what I am seeing @SydneyhSmith

image

function Test-Me1 {
    [CmdletBinding(SupportsShouldProcess)]
    param(

    )

    Set-ADUser -Identity "CN=Test User,OU=Users,OU=Test,DC=domain,DC=com" -Replace @{description="Test"}

}

function Test-Me2 {
    [CmdletBinding(SupportsShouldProcess)]
    param(

    )

    Set-ADACL -Identity "CN=Test User,OU=Users,OU=Test,DC=domain,DC=com"
}

function Test-Me3 {
    [CmdletBinding(SupportsShouldProcess)]
    param(

    )

    Set-O365Contact -Identity "CN=Test User,OU=Users,OU=Test,DC=domain,DC=com" -WhatIf:$WhatIfPreferece
}

It is aware of functions from other modules supporting WhatIf or not supporting it, or something else is in play. In screenshot above first function is calling native Set-ADUser (Binary) and it doesn't show ShouldProcess is not called (and normally it would work). In the 2nd one it's calling function from non-binary module and it seems to "detect" it properly, but doesn't actually work as shown in example above. 3rd function tries to use WhatIf:$WhatIfPreference (to workaround issue for non-binary modules) and is not detected as something that supports anything mainly because Set-o365Contact is not available (private function).

I believe that even tho something may be working as expected in terms of detection this rules is giving a false positives that can lead to problems of users, and maybe could be improved to make a full detection.

@ArwynFr
Copy link

ArwynFr commented Nov 13, 2024

The expectation is that the rule searches the function block for the should process implementation (not checking the cmdlets called within the function), the rule is just used to check the current function so the rule working as expected

Then the rule is both badly implemented AND a problem in itself.

Consider the following code - violating the rule:

Invoke-Sub.ps1

function Invoke-Sub {
  [CmdletBinding(SupportsShouldProcess)]
  param ()

  if ($PSCmdlet.ShouldProcess('x')) { }
}

Invoke-Main.ps1

function Invoke-Main {
  [CmdletBinding(SupportsShouldProcess)]
  param ()
  
  Invoke-Sub
}

The analyzer fails correctly with this error:

# 'Invoke-Main' has the ShouldProcess attribute but does not call ShouldProcess/ShouldContinue.PSScriptAnalyzer(PSShouldProcess)

If you put both functions in the same file, however, the analyzer passes. If the rule is to simply check the function body without considering whether the function called support should process or not, then the implementation is wrong. The same apply with native cmdlets: if you declare a function with SupportsShouldProcess and it's body does not use $PSCmdlet.ShouldProcess but calls Remove-Item for instance, the analyzer passes.

The rule of checking only the body for calls to $PSCmdlet.ShouldProcess, however is problematic. In the example above, if you remove SupportsShouldProcess then you can't Invoke-Main -WhatIf anymore. If you still want to be able to run the main function in what if mode, you can add a call to $PSCmdlet.ShouldProcess around the subfunction invokation. However, this prevents from calling child functions, and propagating the $WhatIfPreference mode to display the notifications for calls in the sub-subfunctions.

I think most script writers expect the rule to accept calling a cmdlet that supports should process as a valid case. This would require to fix how the analyzers checks whether the called cmdlets actually support or not should process for cases that currently fails: functions declared in other script modules or in separate files. If you align the implementation to the rule definition instead, then I think a lot of developers will simply disable that rule check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution - By Design This is by design
Projects
None yet
Development

No branches or pull requests

4 participants