Changes to helper function Connect-SQL

- Connect-SQL now uses parameter sets to more intuitive evaluate that the correct parameters are used in different scenarios (issue dsccommunity#1403).
johlju · Jul 22, 2019 · a66fcde · a66fcde
1 parent adcf2bb
commit a66fcde
Show file tree

Hide file tree

Showing 3 changed files with 152 additions and 91 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -26,6 +26,9 @@
   - Changes to helper function Connect-SQL
     - When impersonating WindowsUser credential use the NetworkCredential UserName.
     - Added addtional verbose logging.
+    - Connect-SQL now uses parameter sets to more intuitive evaluate that
+      the correct parameters are used in different scenarios
+      ([issue #1403](https://github.com/PowerShell/SqlServerDsc/issues/1403)).
 - Changes to SqlServerSecureConnection
   - Forced $Thumbprint to lowercase to fix [issue #1350](https://github.com/PowerShell/SqlServerDsc/issues/1350).
   - Add parameter SuppressRestart with default value false.

diff --git a/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1 b/Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1
@@ -920,54 +920,71 @@ function Start-SqlSetupProcess
 
     .PARAMETER SQLServer
         String containing the host name of the SQL Server to connect to.
+        Defaults to $env:COMPUTERNAME.
 
     .PARAMETER SQLInstanceName
         String containing the SQL Server Database Engine instance to connect to.
+        Defaults to 'MSSQLSERVER'.
 
     .PARAMETER SetupCredential
-        PSCredential object with the credentials to use to impersonate a user when connecting.
-        If this is not provided then the current user will be used to connect to the SQL Server Database Engine instance.
+        The credentials to use to impersonate a user when connecting to the
+        SQL Server Database Engine instance. If this parameter is left out, then
+        the current user will be used to connect to the SQL Server Database Engine
+        instance using Windows Integrated authentication.
 
     .PARAMETER LoginType
         Specifies which type of logon credential should be used. The valid types
-        are Integrated, WindowsUser, or SqlLogin. If WindowsUser or SqlLogin are
-        specified then the parameter SetupCredential needs to be specified as well.
-        If set to 'Integrated' then the credentials that the resource current are
-        run with will be used.
+        are 'WindowsUser' or 'SqlLogin'. Defaults to 'WindowsUser'
         If set to 'WindowsUser' then the it will impersonate using the Windows
         login specified in the parameter SetupCredential.
         If set to 'WindowsUser' then the it will impersonate using the native SQL
         login specified in the parameter SetupCredential.
-        Default value is 'Integrated'.
 
     .PARAMETER StatementTimeout
-        Set the query StatementTimeout in seconds. Default 600 seconds (10mins).
+        Set the query StatementTimeout in seconds. Default 600 seconds (10 minutes).
+
+    .EXAMPLE
+        Connect-SQL
+
+        Connects to the default instance on the local server.
+
+    .EXAMPLE
+        Connect-SQL -InstanceName 'MyInstance'
+
+        Connects to the instance 'MyInstance' on the local server.
+
+    .EXAMPLE
+        Connect-SQL ServerName 'sql.company.local' -InstanceName 'MyInstance'
+
+        Connects to the instance 'MyInstance' on the server 'sql.company.local'.
 #>
 function Connect-SQL
 {
-    [CmdletBinding()]
+    [CmdletBinding(DefaultParameterSetName='SqlServer')]
     param
     (
-        [Parameter()]
+        [Parameter(ParameterSetName='SqlServer')]
+        [Parameter(ParameterSetName='SqlServerWithCredential')]
         [ValidateNotNull()]
         [System.String]
         $ServerName = $env:COMPUTERNAME,
 
-        [Parameter()]
+        [Parameter(ParameterSetName='SqlServer')]
+        [Parameter(ParameterSetName='SqlServerWithCredential')]
         [ValidateNotNull()]
         [System.String]
         $InstanceName = 'MSSQLSERVER',
 
-        [Parameter()]
+        [Parameter(ParameterSetName='SqlServerWithCredential', Mandatory = $true)]
         [ValidateNotNull()]
         [Alias('DatabaseCredential')]
         [System.Management.Automation.PSCredential]
         $SetupCredential,
 
-        [Parameter()]
-        [ValidateSet('Integrated', 'WindowsUser', 'SqlLogin')]
+        [Parameter(ParameterSetName='SqlServerWithCredential')]
+        [ValidateSet('WindowsUser', 'SqlLogin')]
         [System.String]
-        $LoginType = 'Integrated',
+        $LoginType = 'WindowsUser',
 
         [Parameter()]
         [ValidateNotNull()]
@@ -992,7 +1009,7 @@ function Connect-SQL
     $sqlConnectionContext.StatementTimeout = $StatementTimeout
     $sqlConnectionContext.ApplicationName = 'SqlServerDsc'
 
-    if ($LoginType -eq 'Integrated')
+    if ($PSCmdlet.ParameterSetName -eq 'SqlServer')
     {
         <#
             This is only used for verbose messaging and not for the connection
@@ -1006,33 +1023,25 @@ function Connect-SQL
     }
     else
     {
-        if ($SetupCredential)
-        {
-            $connectUserName = $SetupCredential.GetNetworkCredential().UserName
+        $connectUserName = $SetupCredential.GetNetworkCredential().UserName
 
-            Write-Verbose -Message (
-                $script:localizedData.ConnectingUsingImpersonation -f $connectUsername, $LoginType
-            ) -Verbose
-
-            if ($LoginType -eq 'SqlLogin')
-            {
-                $sqlConnectionContext.LoginSecure = $false
-                $sqlConnectionContext.Login = $connectUserName
-                $sqlConnectionContext.SecurePassword = $SetupCredential.Password
-            }
+        Write-Verbose -Message (
+            $script:localizedData.ConnectingUsingImpersonation -f $connectUsername, $LoginType
+        ) -Verbose
 
-            if ($LoginType -eq 'WindowsUser')
-            {
-                $sqlConnectionContext.LoginSecure = $true
-                $sqlConnectionContext.ConnectAsUser = $true
-                $sqlConnectionContext.ConnectAsUserName = $connectUserName
-                $sqlConnectionContext.ConnectAsUserPassword = $SetupCredential.GetNetworkCredential().Password
-            }
+        if ($LoginType -eq 'SqlLogin')
+        {
+            $sqlConnectionContext.LoginSecure = $false
+            $sqlConnectionContext.Login = $connectUserName
+            $sqlConnectionContext.SecurePassword = $SetupCredential.Password
         }
-        else
+
+        if ($LoginType -eq 'WindowsUser')
         {
-            $errorMessage = $script:localizedData.CredentialsNotSpecified -f $LoginType
-            New-InvalidArgumentException -ArgumentName 'SetupCredential' -Message $errorMessage
+            $sqlConnectionContext.LoginSecure = $true
+            $sqlConnectionContext.ConnectAsUser = $true
+            $sqlConnectionContext.ConnectAsUserName = $connectUserName
+            $sqlConnectionContext.ConnectAsUserPassword = $SetupCredential.GetNetworkCredential().Password
         }
     }
 
@@ -1698,10 +1707,14 @@ function Invoke-Query
         $connectSQLParameters = @{
             ServerName       = $SQLServer
             InstanceName     = $SQLInstanceName
-            LoginType        = $LoginType
             StatementTimeout = $StatementTimeout
         }
 
+        if ($LoginType -ne 'Integrated')
+        {
+            $connectSQLParameters['LoginType'] = $LoginType
+        }
+
         if ($PSBoundParameters.ContainsKey('DatabaseCredential'))
         {
             $connectSQLParameters.SetupCredential = $DatabaseCredential

diff --git a/Tests/Unit/SqlServerDsc.Common.Tests.ps1 b/Tests/Unit/SqlServerDsc.Common.Tests.ps1
@@ -1451,7 +1451,7 @@ InModuleScope 'SqlServerDsc.Common' {
             Mock -CommandName New-InvalidOperationException -MockWith $mockThrowLocalizedMessage -Verifiable
         }
 
-        $queryParams = @{
+        $queryParameters = @{
             SQLServer          = 'Server1'
             SQLInstanceName    = 'MSSQLSERVER'
             Database           = 'master'
@@ -1471,17 +1471,22 @@ InModuleScope 'SqlServerDsc.Common' {
             }
 
             It 'Should execute the query silently' {
-                $queryParams.Query = "EXEC sp_configure 'show advanced option', '1'"
-                $mockExpectedQuery = $queryParams.Query.Clone()
+                $queryParameters.Query = "EXEC sp_configure 'show advanced option', '1'"
+                $mockExpectedQuery = $queryParameters.Query.Clone()
 
-                { Invoke-Query @queryParams } | Should -Not -Throw
+                { Invoke-Query @queryParameters } | Should -Not -Throw
+
+                Assert-MockCalled -CommandName Connect-SQL -ParameterFilter {
+                    # Should not be called with a login type.
+                    $PSBoundParameters.ContainsKey('LoginType') -eq $false
+                } -Scope It -Times 1 -Exactly
             }
 
             It 'Should throw the correct error, ExecuteNonQueryFailed, when executing the query fails' {
-                $queryParams.Query = 'BadQuery'
+                $queryParameters.Query = 'BadQuery'
 
-                { Invoke-Query @queryParams } | Should -Throw (
-                    $script:localizedData.ExecuteNonQueryFailed -f $queryParams.Database
+                { Invoke-Query @queryParameters } | Should -Throw (
+                    $script:localizedData.ExecuteNonQueryFailed -f $queryParameters.Database
                 )
             }
 
@@ -1513,21 +1518,51 @@ InModuleScope 'SqlServerDsc.Common' {
             }
         }
 
-        Context 'When executing a query with results' {
+        Context 'When executing a query with no results using Windows impersonation' {
+            It 'Should execute the query silently' {
+                $testParameters = $queryParameters.Clone()
+                $testParameters.LoginType = 'WindowsUser'
+                $testParameters.Query = "EXEC sp_configure 'show advanced option', '1'"
+                $mockExpectedQuery = $testParameters.Query.Clone()
+
+                { Invoke-Query @testParameters } | Should -Not -Throw
+
+                Assert-MockCalled -CommandName Connect-SQL -ParameterFilter {
+                    $LoginType -eq 'WindowsUser'
+                } -Scope It -Times 1 -Exactly
+            }
+        }
+
+        Context 'when executing a query with no results using SQL impersonation' {
+            It 'Should execute the query silently' {
+                $testParameters = $queryParameters.Clone()
+                $testParameters.LoginType = 'SqlLogin'
+                $testParameters.Query = "EXEC sp_configure 'show advanced option', '1'"
+                $mockExpectedQuery = $testParameters.Query.Clone()
+
+                { Invoke-Query @testParameters } | Should -Not -Throw
+
+                Assert-MockCalled -CommandName Connect-SQL -ParameterFilter {
+                    $LoginType -eq 'SqlLogin'
+                } -Scope It -Times 1 -Exactly
+            }
+        }
+
+        Context 'when executing a query with results' {
             It 'Should execute the query and return a result set' {
-                $queryParams.Query = 'SELECT name FROM sys.databases'
-                $mockExpectedQuery = $queryParams.Query.Clone()
+                $queryParameters.Query = 'SELECT name FROM sys.databases'
+                $mockExpectedQuery = $queryParameters.Query.Clone()
 
-                Invoke-Query @queryParams -WithResults | Should -Not -BeNullOrEmpty
+                Invoke-Query @queryParameters -WithResults | Should -Not -BeNullOrEmpty
 
                 Assert-MockCalled -CommandName Connect-SQL -Scope It -Times 1 -Exactly
             }
 
             It 'Should throw the correct error, ExecuteQueryWithResultsFailed, when executing the query fails' {
-                $queryParams.Query = 'BadQuery'
+                $queryParameters.Query = 'BadQuery'
 
-                { Invoke-Query @queryParams -WithResults } | Should -Throw (
-                    $script:localizedData.ExecuteQueryWithResultsFailed -f $queryParams.Database
+                { Invoke-Query @queryParameters -WithResults } | Should -Throw (
+                    $script:localizedData.ExecuteQueryWithResultsFailed -f $queryParameters.Database
                 )
 
                 Assert-MockCalled -CommandName Connect-SQL -Scope It -Times 1 -Exactly
@@ -1576,7 +1611,7 @@ InModuleScope 'SqlServerDsc.Common' {
                     $queryParametersWithSMO.Query = 'BadQuery'
 
                     { Invoke-Query @queryParametersWithSMO } | Should -Throw (
-                        $script:localizedData.ExecuteNonQueryFailed -f $queryParams.Database
+                        $script:localizedData.ExecuteNonQueryFailed -f $queryParameters.Database
                     )
 
                     Assert-MockCalled -CommandName Connect-SQL -Scope It -Times 0 -Exactly
@@ -1597,7 +1632,7 @@ InModuleScope 'SqlServerDsc.Common' {
                     $queryParametersWithSMO.Query = 'BadQuery'
 
                     { Invoke-Query @queryParametersWithSMO -WithResults } | Should -Throw (
-                        $script:localizedData.ExecuteQueryWithResultsFailed -f $queryParams.Database
+                        $script:localizedData.ExecuteQueryWithResultsFailed -f $queryParameters.Database
                     )
 
                     Assert-MockCalled -CommandName Connect-SQL -Scope It -Times 0 -Exactly
@@ -1620,7 +1655,7 @@ InModuleScope 'SqlServerDsc.Common' {
 
                     { $mockSMOServer | Invoke-Query -Query $mockQuery -Database master -WithResults } |
                         Should -Throw (
-                            $script:localizedData.ExecuteQueryWithResultsFailed -f $queryParams.Database
+                            $script:localizedData.ExecuteQueryWithResultsFailed -f $queryParameters.Database
                         )
 
                     Assert-MockCalled -CommandName Connect-SQL -Scope It -Times 0 -Exactly
@@ -2508,27 +2543,56 @@ InModuleScope 'SqlServerDsc.Common' {
         }
 
         Context 'When connecting to the named instance using Windows Authentication impersonation' {
-            It 'Should return the correct service instance' {
+            BeforeAll {
                 $mockExpectedDatabaseEngineServer = $env:COMPUTERNAME
                 $mockExpectedDatabaseEngineInstance = $mockInstanceName
+            }
 
-                $testParameters = @{
-                    ServerName = $mockExpectedDatabaseEngineServer
-                    InstanceName = $mockExpectedDatabaseEngineInstance
-                    SetupCredential = $mockWinCredential
-                    LoginType = 'WindowsUser'
+            Context 'When using the default login type' {
+                BeforeAll {
+                    $testParameters = @{
+                        ServerName = $mockExpectedDatabaseEngineServer
+                        InstanceName = $mockExpectedDatabaseEngineInstance
+                        SetupCredential = $mockWinCredential
+                    }
                 }
 
-                $databaseEngineServerObject = Connect-SQL @testParameters
-                $databaseEngineServerObject.ConnectionContext.ServerInstance | Should -BeExactly "$mockExpectedDatabaseEngineServer\$mockExpectedDatabaseEngineInstance"
-                $databaseEngineServerObject.ConnectionContext.ConnectAsUser | Should -Be $true
-                $databaseEngineServerObject.ConnectionContext.ConnectAsUserPassword | Should -BeExactly $mockWinCredential.GetNetworkCredential().Password
-                $databaseEngineServerObject.ConnectionContext.ConnectAsUserName | Should -BeExactly $mockWinCredential.GetNetworkCredential().UserName
-                $databaseEngineServerObject.ConnectionContext.ConnectAsUser | Should -Be $true
-                $databaseEngineServerObject.ConnectionContext.LoginSecure | Should -Be $true
+                It 'Should return the correct service instance' {
+                    $databaseEngineServerObject = Connect-SQL @testParameters
+                    $databaseEngineServerObject.ConnectionContext.ServerInstance | Should -BeExactly "$mockExpectedDatabaseEngineServer\$mockExpectedDatabaseEngineInstance"
+                    $databaseEngineServerObject.ConnectionContext.ConnectAsUser | Should -Be $true
+                    $databaseEngineServerObject.ConnectionContext.ConnectAsUserPassword | Should -BeExactly $mockWinCredential.GetNetworkCredential().Password
+                    $databaseEngineServerObject.ConnectionContext.ConnectAsUserName | Should -BeExactly $mockWinCredential.GetNetworkCredential().UserName
+                    $databaseEngineServerObject.ConnectionContext.ConnectAsUser | Should -Be $true
+                    $databaseEngineServerObject.ConnectionContext.LoginSecure | Should -Be $true
 
-                Assert-MockCalled -CommandName New-Object -Exactly -Times 1 -Scope It `
-                    -ParameterFilter $mockNewObject_MicrosoftDatabaseEngine_ParameterFilter
+                    Assert-MockCalled -CommandName New-Object -Exactly -Times 1 -Scope It `
+                        -ParameterFilter $mockNewObject_MicrosoftDatabaseEngine_ParameterFilter
+                }
+            }
+
+            Context 'When using the default login type' {
+                BeforeAll {
+                    $testParameters = @{
+                        ServerName = $mockExpectedDatabaseEngineServer
+                        InstanceName = $mockExpectedDatabaseEngineInstance
+                        SetupCredential = $mockWinCredential
+                        LoginType = 'WindowsUser'
+                    }
+                }
+
+                It 'Should return the correct service instance' {
+                    $databaseEngineServerObject = Connect-SQL @testParameters
+                    $databaseEngineServerObject.ConnectionContext.ServerInstance | Should -BeExactly "$mockExpectedDatabaseEngineServer\$mockExpectedDatabaseEngineInstance"
+                    $databaseEngineServerObject.ConnectionContext.ConnectAsUser | Should -Be $true
+                    $databaseEngineServerObject.ConnectionContext.ConnectAsUserPassword | Should -BeExactly $mockWinCredential.GetNetworkCredential().Password
+                    $databaseEngineServerObject.ConnectionContext.ConnectAsUserName | Should -BeExactly $mockWinCredential.GetNetworkCredential().UserName
+                    $databaseEngineServerObject.ConnectionContext.ConnectAsUser | Should -Be $true
+                    $databaseEngineServerObject.ConnectionContext.LoginSecure | Should -Be $true
+
+                    Assert-MockCalled -CommandName New-Object -Exactly -Times 1 -Scope It `
+                        -ParameterFilter $mockNewObject_MicrosoftDatabaseEngine_ParameterFilter
+                }
             }
         }
 
@@ -2550,25 +2614,6 @@ InModuleScope 'SqlServerDsc.Common' {
             }
         }
 
-        Context 'When the logon type is WindowsUser or SqlLogin, but not credentials were passed' {
-            It 'Should throw the correct error' {
-                $mockExpectedDatabaseEngineServer = 'TestServer'
-                $mockExpectedDatabaseEngineInstance = 'MSSQLSERVER'
-
-                 $connectSqlParameters = @{
-                    ServerName      = $mockExpectedDatabaseEngineServer
-                    LoginType       = 'WindowsUser'
-                }
-
-                $mockCorrectErrorMessage = $script:localizedData.CredentialsNotSpecified -f $connectSqlParameters.LoginType
-
-                { Connect-SQL @connectSqlParameters } | Should -Throw $mockCorrectErrorMessage
-
-                Assert-MockCalled -CommandName New-Object -Exactly -Times 1 -Scope It `
-                    -ParameterFilter $mockNewObject_MicrosoftDatabaseEngine_ParameterFilter
-            }
-        }
-
         Assert-VerifiableMock
     }