From 2385b5cf5d959a7581bf968f15f346d9a0ff816b Mon Sep 17 00:00:00 2001 From: Howard Wolosky Date: Sat, 20 Jun 2020 19:03:49 -0700 Subject: [PATCH] Fix usage of Resolve-RepositoryElements -DisableValidation (#243) There are only four legitimate instances in the module as of today where we should be disabling the validation that Resolve-RepositoryElements provides. All other instances were likely introduced due to mistaken copy/paste issues. The only reason we'd want to disable the validation is if there are multiple parametersets that need to be considered for the function, where at least one _would_ require `OwnerName`/`RepositoryName` while at least one other one _would not_. This would have meant that we would have attempted to call those GitHub endpoints with incomplete path information which would have most likely resulted in an error that we could have caught sooner without needing to invoke a web request. I've fixed up all of these incorrect instances, and added a comment to the two remaining ones to remind others later on why that switch is being used, and help prevent incorrect usage in the future. I've also cleaned up unnecessary passing of `BoundParameters` since it already does the correct thing by default. --- GitHubContents.ps1 | 2 +- GitHubIssues.ps1 | 4 ++++ GitHubMiscellaneous.ps1 | 24 ++++++++++++++++++-- GitHubReleases.ps1 | 2 +- GitHubRepositories.ps1 | 35 +++++++++++++++++++---------- GitHubRepositoryForks.ps1 | 4 ++-- Tests/GitHubMiscellaneous.tests.ps1 | 20 +++++++++++++++++ 7 files changed, 73 insertions(+), 18 deletions(-) diff --git a/GitHubContents.ps1 b/GitHubContents.ps1 index 1514382e..780a538d 100644 --- a/GitHubContents.ps1 +++ b/GitHubContents.ps1 @@ -138,7 +138,7 @@ Write-InvocationLog - $elements = Resolve-RepositoryElements -DisableValidation + $elements = Resolve-RepositoryElements $OwnerName = $elements.ownerName $RepositoryName = $elements.repositoryName diff --git a/GitHubIssues.ps1 b/GitHubIssues.ps1 index 8e903c00..fa043e3d 100644 --- a/GitHubIssues.ps1 +++ b/GitHubIssues.ps1 @@ -219,6 +219,9 @@ filter Get-GitHubIssue Write-InvocationLog + # Intentionally disabling validation here because parameter sets exist that do not require + # an OwnerName and RepositoryName. Therefore, we will do futher parameter validation further + # into the function. $elements = Resolve-RepositoryElements -DisableValidation $OwnerName = $elements.ownerName $RepositoryName = $elements.repositoryName @@ -226,6 +229,7 @@ filter Get-GitHubIssue $telemetryProperties = @{ 'OwnerName' = (Get-PiiSafeString -PlainText $OwnerName) 'RepositoryName' = (Get-PiiSafeString -PlainText $RepositoryName) + 'OrganizationName' = (Get-PiiSafeString -PlainText $OrganizationName) 'ProvidedIssue' = $PSBoundParameters.ContainsKey('Issue') } diff --git a/GitHubMiscellaneous.ps1 b/GitHubMiscellaneous.ps1 index b137d6ea..10f98fb5 100644 --- a/GitHubMiscellaneous.ps1 +++ b/GitHubMiscellaneous.ps1 @@ -310,14 +310,18 @@ filter Get-GitHubLicense Write-InvocationLog + $telemetryProperties = @{} + + # Intentionally disabling validation because parameter sets exist that both require + # OwnerName/RepositoryName (to get that repo's License) as well as don't (to get + # all known Licenses). We'll do additional parameter validation within the function. $elements = Resolve-RepositoryElements -DisableValidation $OwnerName = $elements.ownerName $RepositoryName = $elements.repositoryName - $telemetryProperties = @{} - $uriFragment = 'licenses' $description = 'Getting all licenses' + if ($PSBoundParameters.ContainsKey('Key')) { $telemetryProperties['Key'] = $Name @@ -331,6 +335,12 @@ filter Get-GitHubLicense $uriFragment = "repos/$OwnerName/$RepositoryName/license" $description = "Getting the license for $RepositoryName" } + elseif ([String]::IsNullOrEmpty($OwnerName) -xor [String]::IsNullOrEmpty($RepositoryName)) + { + $message = 'When specifying OwnerName and/or RepositorName, BOTH must be specified.' + Write-Log -Message $message -Level Error + throw $message + } $params = @{ 'UriFragment' = $uriFragment @@ -537,6 +547,9 @@ filter Get-GitHubCodeOfConduct Write-InvocationLog + # Intentionally disabling validation because parameter sets exist that both require + # OwnerName/RepositoryName (to get that repo's Code of Conduct) as well as don't (to get + # all known Codes of Conduct). We'll do additional parameter validation within the function. $elements = Resolve-RepositoryElements -DisableValidation $OwnerName = $elements.ownerName $RepositoryName = $elements.repositoryName @@ -545,6 +558,7 @@ filter Get-GitHubCodeOfConduct $uriFragment = 'codes_of_conduct' $description = 'Getting all Codes of Conduct' + if ($PSBoundParameters.ContainsKey('Key')) { $telemetryProperties['Key'] = $Name @@ -558,6 +572,12 @@ filter Get-GitHubCodeOfConduct $uriFragment = "repos/$OwnerName/$RepositoryName/community/code_of_conduct" $description = "Getting the Code of Conduct for $RepositoryName" } + elseif ([String]::IsNullOrEmpty($OwnerName) -xor [String]::IsNullOrEmpty($RepositoryName)) + { + $message = 'When specifying OwnerName and/or RepositorName, BOTH must be specified.' + Write-Log -Message $message -Level Error + throw $message + } $params = @{ 'UriFragment' = $uriFragment diff --git a/GitHubReleases.ps1 b/GitHubReleases.ps1 index 6083f669..b4024828 100644 --- a/GitHubReleases.ps1 +++ b/GitHubReleases.ps1 @@ -176,7 +176,7 @@ filter Get-GitHubRelease Write-InvocationLog -Invocation $MyInvocation - $elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters -DisableValidation + $elements = Resolve-RepositoryElements $OwnerName = $elements.ownerName $RepositoryName = $elements.repositoryName diff --git a/GitHubRepositories.ps1 b/GitHubRepositories.ps1 index c59e33e5..e61bc1d5 100644 --- a/GitHubRepositories.ps1 +++ b/GitHubRepositories.ps1 @@ -306,6 +306,7 @@ filter Remove-GitHubRepository DefaultParameterSetName='Elements', ConfirmImpact="High")] [Alias('Delete-GitHubRepository')] + [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSReviewUnusedParameter", "", Justification="One or more parameters (like NoStatus) are only referenced by helper methods which get access to it from the stack via Get-Variable -Scope 1.")] param( [Parameter(ParameterSetName='Elements')] [string] $OwnerName, @@ -329,7 +330,7 @@ filter Remove-GitHubRepository Write-InvocationLog -Invocation $MyInvocation - $elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters + $elements = Resolve-RepositoryElements $OwnerName = $elements.ownerName $RepositoryName = $elements.repositoryName @@ -548,7 +549,11 @@ filter Get-GitHubRepository Write-InvocationLog -Invocation $MyInvocation - $elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters -DisableValidation + # We are explicitly disabling validation here because a valid parameter set for this function + # allows the OwnerName to be passed in, but not the RepositoryName. That would allow the caller + # to get all of the repositories owned by a specific username. Therefore, we don't want to fail + # if both have not been supplied...we'll do the extra validation within the function. + $elements = Resolve-RepositoryElements -DisableValidation $OwnerName = $elements.ownerName $RepositoryName = $elements.repositoryName @@ -1012,7 +1017,7 @@ filter Update-GitHubRepository Write-InvocationLog -Invocation $MyInvocation - $elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters + $elements = Resolve-RepositoryElements $OwnerName = $elements.ownerName $RepositoryName = $elements.repositoryName @@ -1131,6 +1136,7 @@ filter Get-GitHubRepositoryTopic DefaultParameterSetName='Elements')] [OutputType({$script:GitHubRepositoryTopicTypeName})] [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")] + [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSReviewUnusedParameter", "", Justification="One or more parameters (like NoStatus) are only referenced by helper methods which get access to it from the stack via Get-Variable -Scope 1.")] param( [Parameter(ParameterSetName='Elements')] [string] $OwnerName, @@ -1152,7 +1158,7 @@ filter Get-GitHubRepositoryTopic Write-InvocationLog -Invocation $MyInvocation - $elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters + $elements = Resolve-RepositoryElements $OwnerName = $elements.ownerName $RepositoryName = $elements.repositoryName @@ -1316,7 +1322,7 @@ function Set-GitHubRepositoryTopic { Write-InvocationLog -Invocation $MyInvocation - $elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters + $elements = Resolve-RepositoryElements $OwnerName = $elements.ownerName $RepositoryName = $elements.repositoryName @@ -1434,6 +1440,7 @@ filter Get-GitHubRepositoryContributor [OutputType({$script:GitHubUserTypeName})] [OutputType({$script:GitHubRepositoryContributorStatisticsTypeName})] [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")] + [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSReviewUnusedParameter", "", Justification="One or more parameters (like NoStatus) are only referenced by helper methods which get access to it from the stack via Get-Variable -Scope 1.")] param( [Parameter(ParameterSetName='Elements')] [string] $OwnerName, @@ -1459,7 +1466,7 @@ filter Get-GitHubRepositoryContributor Write-InvocationLog -Invocation $MyInvocation - $elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters + $elements = Resolve-RepositoryElements $OwnerName = $elements.ownerName $RepositoryName = $elements.repositoryName @@ -1577,8 +1584,9 @@ filter Get-GitHubRepositoryCollaborator [CmdletBinding( SupportsShouldProcess, DefaultParameterSetName='Elements')] - [OutputType({$script:GitHubUserTypeName})] - [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")] + [OutputType({$script:GitHubUserTypeName})] + [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")] + [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSReviewUnusedParameter", "", Justification="One or more parameters (like NoStatus) are only referenced by helper methods which get access to it from the stack via Get-Variable -Scope 1.")] param( [Parameter(ParameterSetName='Elements')] [string] $OwnerName, @@ -1603,7 +1611,7 @@ filter Get-GitHubRepositoryCollaborator Write-InvocationLog -Invocation $MyInvocation - $elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters + $elements = Resolve-RepositoryElements $OwnerName = $elements.ownerName $RepositoryName = $elements.repositoryName @@ -1692,6 +1700,7 @@ filter Get-GitHubRepositoryLanguage DefaultParameterSetName='Elements')] [OutputType({$script:GitHubRepositoryLanguageTypeName})] [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")] + [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSReviewUnusedParameter", "", Justification="One or more parameters (like NoStatus) are only referenced by helper methods which get access to it from the stack via Get-Variable -Scope 1.")] param( [Parameter(ParameterSetName='Elements')] [string] $OwnerName, @@ -1713,7 +1722,7 @@ filter Get-GitHubRepositoryLanguage Write-InvocationLog -Invocation $MyInvocation - $elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters + $elements = Resolve-RepositoryElements $OwnerName = $elements.ownerName $RepositoryName = $elements.repositoryName @@ -1798,6 +1807,7 @@ filter Get-GitHubRepositoryTag DefaultParameterSetName='Elements')] [OutputType({$script:GitHubRepositoryTagTypeName})] [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")] + [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSReviewUnusedParameter", "", Justification="One or more parameters (like NoStatus) are only referenced by helper methods which get access to it from the stack via Get-Variable -Scope 1.")] param( [Parameter(ParameterSetName='Elements')] [string] $OwnerName, @@ -1819,7 +1829,7 @@ filter Get-GitHubRepositoryTag Write-InvocationLog -Invocation $MyInvocation - $elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters + $elements = Resolve-RepositoryElements $OwnerName = $elements.ownerName $RepositoryName = $elements.repositoryName @@ -1909,6 +1919,7 @@ filter Move-GitHubRepositoryOwnership [OutputType({$script:GitHubRepositoryTypeName})] [Alias('Transfer-GitHubRepositoryOwnership')] [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSShouldProcess", "", Justification="Methods called within here make use of PSShouldProcess, and the switch is passed on to them inherently.")] + [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSReviewUnusedParameter", "", Justification="One or more parameters (like NoStatus) are only referenced by helper methods which get access to it from the stack via Get-Variable -Scope 1.")] param( [Parameter(ParameterSetName='Elements')] [string] $OwnerName, @@ -1936,7 +1947,7 @@ filter Move-GitHubRepositoryOwnership Write-InvocationLog -Invocation $MyInvocation - $elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters + $elements = Resolve-RepositoryElements $OwnerName = $elements.ownerName $RepositoryName = $elements.repositoryName diff --git a/GitHubRepositoryForks.ps1 b/GitHubRepositoryForks.ps1 index 3322c726..457ee90d 100644 --- a/GitHubRepositoryForks.ps1 +++ b/GitHubRepositoryForks.ps1 @@ -91,7 +91,7 @@ filter Get-GitHubRepositoryFork Write-InvocationLog - $elements = Resolve-RepositoryElements -DisableValidation + $elements = Resolve-RepositoryElements $OwnerName = $elements.ownerName $RepositoryName = $elements.repositoryName @@ -211,7 +211,7 @@ filter New-GitHubRepositoryFork Write-InvocationLog - $elements = Resolve-RepositoryElements -DisableValidation + $elements = Resolve-RepositoryElements $OwnerName = $elements.ownerName $RepositoryName = $elements.repositoryName diff --git a/Tests/GitHubMiscellaneous.tests.ps1 b/Tests/GitHubMiscellaneous.tests.ps1 index 7be6eb96..9d9e64dd 100644 --- a/Tests/GitHubMiscellaneous.tests.ps1 +++ b/Tests/GitHubMiscellaneous.tests.ps1 @@ -99,6 +99,16 @@ Describe 'Get-GitHubLicense' { } } + Context 'Will fail if not provided both OwnerName and RepositoryName' { + It 'Should fail if only OwnerName is specified' { + { Get-GitHubLicense -OwnerName 'PowerShell' } | Should -Throw + } + + It 'Should fail if only RepositoryName is specified' { + { Get-GitHubLicense -RepositoryName 'PowerShell' } | Should -Throw + } + } + Context 'Can get the license for a repo with the repo on the pipeline' { BeforeAll { $result = Get-GitHubRepository -OwnerName 'PowerShell' -RepositoryName 'PowerShell' | Get-GitHubLicense @@ -213,6 +223,16 @@ Describe 'Get-GitHubCodeOfConduct' { } } + Context 'Will fail if not provided both OwnerName and RepositoryName' { + It 'Should fail if only OwnerName is specified' { + { Get-GitHubCodeOfConduct -OwnerName 'PowerShell' } | Should -Throw + } + + It 'Should fail if only RepositoryName is specified' { + { Get-GitHubCodeOfConduct -RepositoryName 'PowerShell' } | Should -Throw + } + } + Context 'Can get the code of conduct for a repo with the repo on the pipeline' { BeforeAll { $result = Get-GitHubRepository -OwnerName 'PowerShell' -RepositoryName 'PowerShell' | Get-GitHubCodeOfConduct