Add commands for certificate binding in Report Server #2409
Conversation
WalkthroughFive new public PowerShell commands for managing SSL certificate bindings and listing certificates in SQL Server Reporting Services (SSRS) and Power BI Report Server (PBIRS) are introduced. These commands wrap corresponding CIM methods to provide Get, Add, Remove, and Set operations for SSL bindings, plus a command to list available SSL certificates. Comprehensive integration and unit tests are added, localized strings updated, and a pipeline stage created for secure BI Report Server testing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… readability and maintainability
…te examples; enhance integration and unit tests for SSL certificate properties
…pdate expected parameters and error IDs for consistency
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2409 +/- ##
======================================
Coverage 94% 94%
======================================
Files 210 215 +5
Lines 10754 10864 +110
======================================
+ Hits 10133 10243 +110
Misses 621 621
🚀 New features to boost your workflow:
|
…trieval and validation before execution
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
@johlju reviewed 22 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @johlju).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@source/Public/Remove-SqlDscRSSslCertificateBinding.ps1`:
- Around line 95-130: The cmdlet declaration is too loose: change the OutputType
and the $Configuration parameter from System.Object to CimInstance to tighten
typing and surface errors earlier; specifically replace
OutputType([System.Object]) with
OutputType([Microsoft.Management.Infrastructure.CimInstance]) and change the
parameter declaration for $Configuration from [System.Object] $Configuration to
[Microsoft.Management.Infrastructure.CimInstance] $Configuration (keep
ValueFromPipeline = $true and other attributes unchanged).
- Around line 152-181: Replace the direct construction of
System.Management.Automation.ErrorRecord inside the catch block with the module
helper pattern: build an error message using $instanceName and
$_.Exception.Message, create an InvalidOperation exception via
New-InvalidOperationException -Message ... -PassThru, then create an ErrorRecord
with New-ErrorRecord -Exception <that exception> -ErrorId 'RSRSSCB0001'
-ErrorCategory 'InvalidOperation' -TargetObject $Configuration and pass that to
$PSCmdlet.ThrowTerminatingError; locate this change around the
Invoke-RsCimMethod call and the existing catch that currently constructs
[System.Management.Automation.ErrorRecord]::new().
In `@source/Public/Set-SqlDscRSSslCertificateBinding.ps1`:
- Around line 162-192: The certificate-hash comparison is inconsistent because
you only lowercase the input hash ($normalizedHash) but not the hashes from
existing bindings; update the removal loop and the existence check to compare
against the lowercased binding hash (e.g., use
$binding.CertificateHash.ToLower() in the foreach where
Remove-SqlDscRSSslCertificateBinding is called and use
$_.CertificateHash.ToLower() in the Where-Object that sets $bindingExists) so
both sides use the same normalized value before calling
Remove-SqlDscRSSslCertificateBinding and Add-SqlDscRSSslCertificateBinding.
🧹 Nitpick comments (21)
azure-pipelines.yml (1)
1192-1203: Consider adding the new secure stage to Deploy dependencies.The new
Integration_Test_Commands_BIReportServer_Securestage is not listed in thedependsOnblock for the Deploy stage. If SSL/TLS functionality is critical for releases, consider adding it to ensure these tests pass before deployment.♻️ Suggested change
- stage: Deploy dependsOn: - Quality_Test_and_Unit_Test - Integration_Test_Commands_SqlServer - Integration_Test_Commands_SqlServer_PreparedImage - Integration_Test_Commands_ReportingServices - Integration_Test_Commands_BIReportServer + - Integration_Test_Commands_BIReportServer_Secure - Integration_Test_Resources_SqlServer - Integration_Test_Resources_SqlServer_dbatools - Integration_Test_Resources_ReportingServices - Integration_Test_Resources_PowerBIReportServer - Integration_Test_Resources_ReportingServices_dbatoolstests/Integration/Commands/Pre.Set-SqlDscRSUrlReservation.Integration.Tests.ps1 (1)
51-59: Consider simplifying the URL filtering logic.The loop-based filtering is duplicated between both tests. This could be simplified using
Where-Objectfor better readability, though the current approach works correctly.♻️ Simplified filtering approach
# Instead of the for loop, you could use: $currentUrls = 0..($reservations.Application.Count - 1) | Where-Object { $reservations.Application[$_] -eq 'ReportServerWebService' } | ForEach-Object { $reservations.UrlString[$_] }Also applies to: 72-80
CHANGELOG.md (2)
217-223: Changelog entry is a bit verbose; consider shortening + add issue reference (if one exists).This new “Added” bullet is longer than the repo’s “briefly” guidance, and it doesn’t reference an issue (if this change originated from one). Consider collapsing to a shorter 1–2 line description (and add
[issue#NNNN](...)if applicable).
247-249: Changelog entry could be shortened for consistency with changelog guidelines.This entry is fine, but could likely be reduced to a single line (command + what it wraps) to match the “briefly” guidance.
source/Public/Get-SqlDscRSSslCertificateBinding.ps1 (1)
117-126: Consider usingNew-ErrorRecordhelper instead of direct ErrorRecord construction.Per coding guidelines, use
New-ErrorRecordinstead of[System.Management.Automation.ErrorRecord]::new(...)for error handling. This also applies toNew-Exceptionfor creating the exception.♻️ Suggested refactor
catch { + $errorMessage = $script:localizedData.Get_SqlDscRSSslCertificateBinding_FailedToGet -f $instanceName, $_.Exception.Message + + $errorRecord = New-ErrorRecord -Exception (New-Exception -Message $errorMessage -InnerException $_.Exception) -ErrorId 'GSRSSCB0001' -ErrorCategory 'InvalidOperation' -TargetObject $Configuration + - $PSCmdlet.ThrowTerminatingError( - [System.Management.Automation.ErrorRecord]::new( - ($script:localizedData.Get_SqlDscRSSslCertificateBinding_FailedToGet -f $instanceName, $_.Exception.Message), - 'GSRSSCB0001', - [System.Management.Automation.ErrorCategory]::InvalidOperation, - $Configuration - ) - ) + $PSCmdlet.ThrowTerminatingError($errorRecord) }source/Public/Get-SqlDscRSSslCertificate.ps1 (1)
99-108: Consider usingNew-ErrorRecordhelper.Same as the previous file - per coding guidelines, prefer
New-ErrorRecordandNew-Exceptionhelpers over directErrorRecordconstruction.source/Public/Set-SqlDscRSSslCertificateBinding.ps1 (1)
97-97: Consider more specific OutputType.The
[OutputType([System.Object])]is generic. SincePassThrureturns theConfigurationCIM instance, consider using[OutputType([Microsoft.Management.Infrastructure.CimInstance])]or conditional OutputType attributes.tests/Unit/Public/Get-SqlDscRSSslCertificateBinding.Tests.ps1 (1)
74-173: Good test coverage for core scenarios.Tests cover:
- Successful retrieval with multiple bindings
- Empty results handling
- CIM method failure with correct error ID verification
- Configuration passed as parameter (not pipeline)
Consider adding a test that explicitly passes the
-Lcidparameter to verify the override behavior (bypassingGet-OperatingSystem).♻️ Suggested additional test
Context 'When specifying Lcid parameter explicitly' { BeforeAll { $mockCimInstance = [PSCustomObject] @{ InstanceName = 'SSRS' } Mock -CommandName Invoke-RsCimMethod -MockWith { return @{ Application = @('ReportServerWebService') CertificateHash = @('AABBCCDD') IPAddress = @('0.0.0.0') Port = @(443) Lcid = @(1031) } } } It 'Should use the specified Lcid' { $result = $mockCimInstance | Get-SqlDscRSSslCertificateBinding -Lcid 1031 $result.Lcid | Should -Be 1031 Should -Invoke -CommandName Invoke-RsCimMethod -ParameterFilter { $Arguments.Lcid -eq 1031 } -Exactly -Times 1 Should -Invoke -CommandName Get-OperatingSystem -Exactly -Times 0 } }tests/Integration/Commands/Add-SqlDscRSSslCertificateBinding.Integration.Tests.ps1 (3)
94-100: AvoidShould -Not -Throwassertion.Per testing guidelines: "No
Should -Not -Throw- invoke commands directly." Instead of wrapping the command in a scriptblock and asserting it doesn't throw, invoke the command directly. Failures will surface as test failures.♻️ Suggested refactor
It 'Should add SSL certificate binding for ReportServerWebService' { - { $script:configuration | Add-SqlDscRSSslCertificateBinding -Application 'ReportServerWebService' `@script`:addSslCertificateBindingParameters } | Should -Not -Throw + $null = $script:configuration | Add-SqlDscRSSslCertificateBinding -Application 'ReportServerWebService' `@script`:addSslCertificateBindingParameters } It 'Should add SSL certificate binding for ReportServerWebApp' { - { $script:configuration | Add-SqlDscRSSslCertificateBinding -Application 'ReportServerWebApp' `@script`:addSslCertificateBindingParameters } | Should -Not -Throw + $null = $script:configuration | Add-SqlDscRSSslCertificateBinding -Application 'ReportServerWebApp' `@script`:addSslCertificateBindingParameters }Apply the same pattern to all other
Itblocks in this file.
89-129: Context descriptions should be more distinctive.Three Context blocks have identical descriptions: "When adding SSL certificate binding for SQL Server Reporting Services". While the tags differentiate them, consider making descriptions unique for clarity in test output (e.g., include the SQL version).
♻️ Suggested improvement
- Context 'When adding SSL certificate binding for SQL Server Reporting Services' -Tag @('Integration_SQL2017_RS') { + Context 'When adding SSL certificate binding for SQL Server 2017 Reporting Services' -Tag @('Integration_SQL2017_RS') { ... - Context 'When adding SSL certificate binding for SQL Server Reporting Services' -Tag @('Integration_SQL2019_RS') { + Context 'When adding SSL certificate binding for SQL Server 2019 Reporting Services' -Tag @('Integration_SQL2019_RS') { ... - Context 'When adding SSL certificate binding for SQL Server Reporting Services' -Tag @('Integration_SQL2022_RS') { + Context 'When adding SSL certificate binding for SQL Server 2022 Reporting Services' -Tag @('Integration_SQL2022_RS') {
73-77: Consider adding cleanup in AfterAll.The comment notes the certificate is not removed after tests. Consider adding an
AfterAllblock to clean up:
- The certificate from
Cert:\LocalMachine\My- The certificate from
Cert:\LocalMachine\Root- The temporary
.cerfileThis prevents test artifacts from accumulating on the CI machine.
♻️ Suggested cleanup block
AfterAll { # Clean up the test certificate if ($script:testCertificate) { $script:testCertificate | Remove-Item -Force -ErrorAction 'SilentlyContinue' } # Remove from Trusted Root $rootCert = Get-ChildItem -Path 'Cert:\LocalMachine\Root' | Where-Object { $_.Thumbprint -eq $script:testCertificateHash } if ($rootCert) { $rootCert | Remove-Item -Force -ErrorAction 'SilentlyContinue' } # Remove temporary file if (Test-Path -Path $script:certificatePath) { Remove-Item -Path $script:certificatePath -Force -ErrorAction 'SilentlyContinue' } }tests/Integration/Commands/Remove-SqlDscRSSslCertificateBinding.Integration.Tests.ps1 (3)
61-62: Remove duplicate variable assignments.These variables are already defined in the outer
BeforeAllblock (lines 38-39). The duplicate assignments here are redundant.Suggested fix
$script:testCertificateHash = $script:testCertificate.Thumbprint - $script:testIPAddress = '0.0.0.0' - $script:testPort = 443 Write-Verbose -Message ('Using self-signed certificate ''{0}'' with thumbprint ''{1}''.' -f $script:testCertificate.Subject, $script:testCertificateHash) -Verbose
72-74: AvoidShould -Not -Throwpattern.Per coding guidelines, avoid using
Should -Not -Throw. Instead, invoke the command directly and let any exception fail the test naturally.Suggested fix
It 'Should remove SSL certificate binding' { - { $script:configuration | Remove-SqlDscRSSslCertificateBinding -Application 'ReportServerWebService' -CertificateHash $script:testCertificateHash -IPAddress $script:testIPAddress -Port $script:testPort -Force -ErrorAction 'Stop' } | Should -Not -Throw + $null = $script:configuration | Remove-SqlDscRSSslCertificateBinding -Application 'ReportServerWebService' -CertificateHash $script:testCertificateHash -IPAddress $script:testIPAddress -Port $script:testPort -Force -ErrorAction 'Stop' }This pattern applies to all similar
Itblocks in this file (lines 82-84, 92-94, 102-104).
67-75: Context descriptions are identical but test different SQL Server versions.The three Context blocks for SSRS all have the identical description "When removing SSL certificate binding for SQL Server Reporting Services" but are tagged for different SQL Server versions (2017, 2019, 2022). Consider making descriptions more specific to improve test output clarity.
Suggested fix
- Context 'When removing SSL certificate binding for SQL Server Reporting Services' -Tag @('Integration_SQL2017_RS') { + Context 'When removing SSL certificate binding for SQL Server 2017 Reporting Services' -Tag @('Integration_SQL2017_RS') {Apply similar changes to the 2019 and 2022 Context blocks.
Also applies to: 77-85, 87-95
tests/Unit/Public/Remove-SqlDscRSSslCertificateBinding.Tests.ps1 (1)
83-84: AvoidShould -Not -Throwpattern in unit tests.Per coding guidelines, avoid using
Should -Not -Throw. Invoke the command directly instead.Suggested fix
It 'Should remove SSL certificate binding without errors' { - { $mockCimInstance | Remove-SqlDscRSSslCertificateBinding -CertificateHash 'AABBCCDD' -Application 'ReportServerWebService' -Confirm:$false } | Should -Not -Throw + $null = $mockCimInstance | Remove-SqlDscRSSslCertificateBinding -CertificateHash 'AABBCCDD' -Application 'ReportServerWebService' -Confirm:$false Should -Invoke -CommandName Invoke-RsCimMethod -ParameterFilter {This pattern should be applied to all similar occurrences in this file (lines 113, 150, 198).
tests/Integration/Commands/Get-SqlDscRSSslCertificateBinding.Integration.Tests.ps1 (1)
60-98: Context descriptions are identical but test different SQL Server versions.Similar to the Remove tests, these Context blocks have identical descriptions but are tagged for different versions. Consider making them more specific for better test output clarity.
Suggested fix
- Context 'When getting SSL certificate bindings for SQL Server Reporting Services' -Tag @('Integration_SQL2017_RS') { + Context 'When getting SSL certificate bindings for SQL Server 2017 Reporting Services' -Tag @('Integration_SQL2017_RS') {Apply similar changes to the 2019 and 2022 Context blocks.
Also applies to: 100-138, 140-178
tests/Unit/Public/Set-SqlDscRSSslCertificateBinding.Tests.ps1 (1)
88-89: AvoidShould -Not -Throwpattern in unit tests.Per coding guidelines, invoke the command directly instead of wrapping in
Should -Not -Throw.Suggested fix
It 'Should add the SSL certificate binding' { - { $mockCimInstance | Set-SqlDscRSSslCertificateBinding -Application 'ReportServerWebService' -CertificateHash 'AABBCCDD' -Confirm:$false } | Should -Not -Throw + $null = $mockCimInstance | Set-SqlDscRSSslCertificateBinding -Application 'ReportServerWebService' -CertificateHash 'AABBCCDD' -Confirm:$false Should -Invoke -CommandName Add-SqlDscRSSslCertificateBinding -Exactly -Times 1This pattern should be applied to all similar occurrences in this file (lines 118, 147, 191, 234, 255).
tests/Integration/Commands/Get-SqlDscRSSslCertificate.Integration.Tests.ps1 (1)
58-94: Context descriptions are identical but test different SQL Server versions.Consider making Context descriptions more specific to distinguish between SQL Server 2017, 2019, and 2022 in test output.
Suggested fix
- Context 'When getting SSL certificates for SQL Server Reporting Services' -Tag @('Integration_SQL2017_RS') { + Context 'When getting SSL certificates for SQL Server 2017 Reporting Services' -Tag @('Integration_SQL2017_RS') {Apply similar changes to the 2019 and 2022 Context blocks.
Also applies to: 96-132, 134-170
tests/Integration/Commands/Set-SqlDscRSSslCertificateBinding.Integration.Tests.ps1 (2)
38-41: Remove duplicate variable declarations.The variables
$script:testIPAddressand$script:testPortare declared in the outerBeforeAll(lines 39-40) and then redeclared in the innerBeforeAll(lines 62-63) with the same values. Remove the duplicates from one location for clarity.♻️ Suggested fix
Remove lines 62-63 since the variables are already defined in the outer
BeforeAll:$script:testCertificateHash = $script:testCertificate.Thumbprint - $script:testIPAddress = '0.0.0.0' - $script:testPort = 443 Write-Verbose -Message ('Using self-signed certificate ''{0}'' with thumbprint ''{1}''.' -f $script:testCertificate.Subject, $script:testCertificateHash) -VerboseAlso applies to: 61-63
73-75: AvoidShould -Not -Throwper guidelines.Per coding guidelines,
Should -Not -Throwshould not be used. Instead, invoke the command directly; if it throws, the test will fail naturally.♻️ Suggested fix for all instances (lines 74, 92, 110, 128)
It 'Should set SSL certificate binding' { - { $script:configuration | Set-SqlDscRSSslCertificateBinding -Application 'ReportServerWebService' -CertificateHash $script:testCertificateHash -IPAddress $script:testIPAddress -Port $script:testPort -Force -ErrorAction 'Stop' } | Should -Not -Throw + $null = $script:configuration | Set-SqlDscRSSslCertificateBinding -Application 'ReportServerWebService' -CertificateHash $script:testCertificateHash -IPAddress $script:testIPAddress -Port $script:testPort -Force -ErrorAction 'Stop' }Apply the same pattern to lines 92, 110, and 128.
source/Public/Add-SqlDscRSSslCertificateBinding.ps1 (1)
174-181: UseNew-ErrorRecordhelper instead of direct ErrorRecord construction.Per coding guidelines, error handling should use
New-ErrorRecordinstead of[System.Management.Automation.ErrorRecord]::new(...).♻️ Suggested fix
catch { - $PSCmdlet.ThrowTerminatingError( - [System.Management.Automation.ErrorRecord]::new( - ($script:localizedData.Add_SqlDscRSSslCertificateBinding_FailedToAdd -f $instanceName, $_.Exception.Message), - 'ASRSSCB0001', - [System.Management.Automation.ErrorCategory]::InvalidOperation, - $Configuration - ) - ) + $errorRecord = New-ErrorRecord ` + -Message ($script:localizedData.Add_SqlDscRSSslCertificateBinding_FailedToAdd -f $instanceName, $_.Exception.Message) ` + -ErrorId 'ASRSSCB0001' ` + -Category 'InvalidOperation' ` + -TargetObject $Configuration ` + -Exception $_.Exception + + $PSCmdlet.ThrowTerminatingError($errorRecord) }
1 participant
Pull Request (PR) description
Get-SqlDscRSSslCertificateBinding,Add-SqlDscRSSslCertificateBinding,Remove-SqlDscRSSslCertificateBinding,and
Set-SqlDscRSSslCertificateBindingto manage SSL certificate bindingsfor SQL Server Reporting Services or Power BI Report Server. These commands
wrap the
ListSSLCertificateBindings,CreateSSLCertificateBinding, andRemoveSSLCertificateBindingCIM methods. TheSet-SqlDscRSSslCertificateBindingcommand provides a declarative approach to set SSL bindings to an exact list.
Get-SqlDscRSSslCertificateto list available SSLcertificates that can be used for Reporting Services. Wraps the
ListSSLCertificatesCIM method.This Pull Request (PR) fixes the following issues
None.
Task list
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
This change is