From 0ed01cd9637df57b90a2ea39861b74c9b2149013 Mon Sep 17 00:00:00 2001 From: Cody Konior Date: Tue, 9 Oct 2018 12:24:57 +0800 Subject: [PATCH 1/5] #1237 SqlAGDatabase Fix impersonate permission test --- CHANGELOG.md | 4 ++ .../MSFT_SqlAGDatabase.psm1 | 46 ++++++------- SqlServerDscHelper.psm1 | 68 ++++++++++++++++--- 3 files changed, 85 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c30c18185..478f6e92b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,10 @@ - Fix unit tests that didn't mock some of the calls. It no longer fail when a SQL Server installation is not present on the node running the unit test ([issue #983](https://github.com/PowerShell/SqlServerDsc/issues/983)). +- Changes to SqlAGDatabase + - Fix MatchDatabaseOwner to check for CONTROL SERVER and IMPERSONATE + LOGIN permission in addition to IMPERSONATE ANY LOGIN. + - Fix MatchDatabaseOwner help text. ## 12.0.0.0 diff --git a/DSCResources/MSFT_SqlAGDatabase/MSFT_SqlAGDatabase.psm1 b/DSCResources/MSFT_SqlAGDatabase/MSFT_SqlAGDatabase.psm1 index b934b1331..75a955bc2 100644 --- a/DSCResources/MSFT_SqlAGDatabase/MSFT_SqlAGDatabase.psm1 +++ b/DSCResources/MSFT_SqlAGDatabase/MSFT_SqlAGDatabase.psm1 @@ -232,33 +232,33 @@ function Set-TargetResource # Get only the secondary replicas. Some tests do not need to be performed on the primary replica $secondaryReplicas = $availabilityGroup.AvailabilityReplicas | Where-Object -FilterScript { $_.Role -ne 'Primary' } - # Ensure the appropriate permissions are in place on all the replicas - if ( $MatchDatabaseOwner ) + foreach ( $databaseToAddToAvailabilityGroup in $databasesToAddToAvailabilityGroup ) { - $impersonatePermissionsStatus = @{} + $databaseObject = $primaryServerObject.Databases[$databaseToAddToAvailabilityGroup] - foreach ( $availabilityGroupReplica in $secondaryReplicas ) + # Ensure the appropriate permissions are in place on all the intended secondary replicas for each database + if ( $MatchDatabaseOwner ) { - $currentAvailabilityGroupReplicaServerObject = Connect-SQL -SQLServer $availabilityGroupReplica.Name - $impersonatePermissionsStatus.Add( - $availabilityGroupReplica.Name, - ( Test-ImpersonatePermissions -ServerObject $currentAvailabilityGroupReplicaServerObject ) - ) - } + $impersonatePermissionsStatus = @{} - if ( $impersonatePermissionsStatus.Values -contains $false ) - { - $impersonatePermissionsMissingParameters = @( - [System.Security.Principal.WindowsIdentity]::GetCurrent().Name, - ( ( $impersonatePermissionsStatus.GetEnumerator() | Where-Object -FilterScript { -not $_.Value } | Select-Object -ExpandProperty Key ) -join ', ' ) - ) - throw ($script:localizedData.ImpersonatePermissionsMissing -f $impersonatePermissionsMissingParameters ) - } - } + foreach ( $availabilityGroupReplica in $secondaryReplicas ) + { + $currentAvailabilityGroupReplicaServerObject = Connect-SQL -SQLServer $availabilityGroupReplica.Name + $impersonatePermissionsStatus.Add( + $availabilityGroupReplica.Name, + ( Test-ImpersonatePermissions -ServerObject $currentAvailabilityGroupReplicaServerObject -LoginName $databaseObject.Owner ) + ) + } - foreach ( $databaseToAddToAvailabilityGroup in $databasesToAddToAvailabilityGroup ) - { - $databaseObject = $primaryServerObject.Databases[$databaseToAddToAvailabilityGroup] + if ( $impersonatePermissionsStatus.Values -contains $false ) + { + $impersonatePermissionsMissingParameters = @( + [System.Security.Principal.WindowsIdentity]::GetCurrent().Name, + ( ( $impersonatePermissionsStatus.GetEnumerator() | Where-Object -FilterScript { -not $_.Value } | Select-Object -ExpandProperty Key ) -join ', ' ) + ) + throw ($script:localizedData.ImpersonatePermissionsMissing -f $impersonatePermissionsMissingParameters ) + } + } <# Verify the prerequisites prior to joining the database to the availability group @@ -608,7 +608,7 @@ function Set-TargetResource If set to $false, the owner of the database will be the PSDscRunAsCredential. - The default is '$true'. + The default is '$false'. .PARAMETER ProcessOnlyOnActiveNode Specifies that the resource will only determine if a change is needed if the target node is the active host of the SQL Server Instance. diff --git a/SqlServerDscHelper.psm1 b/SqlServerDscHelper.psm1 index 00222aeb6..f4b520d42 100644 --- a/SqlServerDscHelper.psm1 +++ b/SqlServerDscHelper.psm1 @@ -1079,7 +1079,14 @@ function Test-LoginEffectivePermissions [Parameter(Mandatory = $true)] [System.String[]] - $Permissions + $Permissions, + + [ValidateSet('APPLICATION ROLE', 'ASSEMBLY', 'ASYMMETRIC KEY', 'CERTIFICATE', 'CONTRACT', 'DATABASE', 'ENDPOINT', 'FULLTEXT CATALOG', + 'LOGIN', 'MESSAGE TYPE', 'OBJECT', 'REMOTE SERVICE BINDING', 'ROLE', 'ROUTE', 'SCHEMA', 'SERVER', 'SERVICE', 'SYMMETRIC KEY', + 'TYPE', 'USER', 'XML SCHEMA COLLECTION')] + [System.String] $SecurableClass = 'SERVER', + + [System.String] $Securable ) # Assume the permissions are not present @@ -1092,13 +1099,21 @@ function Test-LoginEffectivePermissions WithResults = $true } - $queryToGetEffectivePermissionsForLogin = " - EXECUTE AS LOGIN = '$LoginName' - SELECT DISTINCT permission_name - FROM fn_my_permissions(null,'SERVER') - REVERT - " - + if ($null -eq $Securable) { + $queryToGetEffectivePermissionsForLogin = " + EXECUTE AS LOGIN = '$LoginName' + SELECT DISTINCT permission_name + FROM fn_my_permissions(null, '$SecurableClass') + REVERT + " + } else { + $queryToGetEffectivePermissionsForLogin = " + EXECUTE AS LOGIN = '$LoginName' + SELECT DISTINCT permission_name + FROM fn_my_permissions('$Securable', '$SecurableClass') + REVERT + " + } Write-Verbose -Message ($script:localizedData.GetEffectivePermissionForLogin -f $LoginName, $sqlInstanceName) -Verbose $loginEffectivePermissionsResult = Invoke-Query @invokeQueryParameters -Query $queryToGetEffectivePermissionsForLogin @@ -1233,6 +1248,10 @@ function Get-PrimaryReplicaServerObject .PARAMETER ServerObject The server object on which to perform the test. + + .PARAMETER LoginName + If set then this specific login is also checked for the specific permission. + #> function Test-ImpersonatePermissions { @@ -1240,18 +1259,47 @@ function Test-ImpersonatePermissions ( [Parameter(Mandatory = $true)] [Microsoft.SqlServer.Management.Smo.Server] - $ServerObject + $ServerObject, + + [System.String] $LoginName ) + $impersonatePermissionsPresent = $false + + # Only exists on this is SQL 2014 or newer $testLoginEffectivePermissionsParams = @{ SQLServer = $ServerObject.ComputerNamePhysicalNetBIOS SQLInstanceName = $ServerObject.ServiceName LoginName = $ServerObject.ConnectionContext.TrueLogin Permissions = @('IMPERSONATE ANY LOGIN') } - $impersonatePermissionsPresent = Test-LoginEffectivePermissions @testLoginEffectivePermissionsParams + if ( -not $impersonatePermissionsPresent ) + { + # Check for sysadmin / control server permission which allows impersonation + $testLoginEffectivePermissionsParams = @{ + SQLServer = $ServerObject.ComputerNamePhysicalNetBIOS + SQLInstanceName = $ServerObject.ServiceName + LoginName = $ServerObject.ConnectionContext.TrueLogin + Permissions = @('CONTROL SERVER') + } + $impersonatePermissionsPresent = Test-LoginEffectivePermissions @testLoginEffectivePermissionsParams + } + + if ( -not $impersonatePermissionsPresent -and -not [System.String]::IsNullOrEmpty($LoginName) ) { + # Check for login-specific impersonation permissions + $testLoginEffectivePermissionsParams = @{ + SQLServer = $ServerObject.ComputerNamePhysicalNetBIOS + SQLInstanceName = $ServerObject.ServiceName + LoginName = $ServerObject.ConnectionContext.TrueLogin + Permissions = @('IMPERSONATE') + SecurableClass = 'LOGIN' + Securable = $LoginName + } + $impersonatePermissionsPresent = Test-LoginEffectivePermissions @testLoginEffectivePermissionsParams + } + if ( -not $impersonatePermissionsPresent ) { New-VerboseMessage -Message ( 'The login "{0}" does not have impersonate permissions on the instance "{1}\{2}".' -f $testLoginEffectivePermissionsParams.LoginName, $testLoginEffectivePermissionsParams.SQLServer, $testLoginEffectivePermissionsParams.SQLInstanceName ) From de00248eceb3c8f1aeecd69dd3b0735a61ebd1df Mon Sep 17 00:00:00 2001 From: Cody Konior Date: Fri, 12 Oct 2018 23:38:42 +0800 Subject: [PATCH 2/5] Additional unit tests for Test-ImpersonatePermissions --- Tests/Unit/SqlServerDSCHelper.Tests.ps1 | 53 +++++++++++++++++++++---- 1 file changed, 46 insertions(+), 7 deletions(-) diff --git a/Tests/Unit/SqlServerDSCHelper.Tests.ps1 b/Tests/Unit/SqlServerDSCHelper.Tests.ps1 index 371468004..0573a11b0 100644 --- a/Tests/Unit/SqlServerDSCHelper.Tests.ps1 +++ b/Tests/Unit/SqlServerDSCHelper.Tests.ps1 @@ -1219,23 +1219,62 @@ InModuleScope $script:moduleName { $mockServerObject.ServiceName = 'MSSQLSERVER' $mockServerObject.ConnectionContext = $mockConnectionContextObject + $mockImpersonateAnyLoginPermissionsPresent = @( + 'Impersonate Any Login' + ) + + $mockControlServerPermissionsPresent = @( + 'Control Server' + ) + + $mockImpersonateLoginPermissionsPresent = @( + 'Impersonate' + ) + + $mockPermissionsMissing = @( + ) + + $mockInvokeQueryImpersonatePermissionsSet = @() # Will be set dynamically in the check + + $mockInvokeQueryImpersonatePermissionsResult = { + return New-Object -TypeName PSObject -Property @{ + Tables = @{ + Rows = @{ + permission_name = $mockInvokeQueryImpersonatePermissionsSet + } + } + } + } + + BeforeEach { + Mock -CommandName Invoke-Query -MockWith $mockInvokeQueryImpersonatePermissionsResult -Verifiable + } + Context 'When impersonate permissions are present for the login' { - Mock -CommandName Test-LoginEffectivePermissions -MockWith { $true } + It 'Should return true when the impersonate any login permission is present'{ + $mockInvokeQueryImpersonatePermissionsSet = $mockImpersonateAnyLoginPermissionsPresent.Clone() - It 'Should return true when the impersonate permissions are present for the login'{ Test-ImpersonatePermissions -ServerObject $mockServerObject | Should -Be $true + } + + It 'Should return true when the control server permission is present'{ + $mockInvokeQueryImpersonatePermissionsSet = $mockControlServerPermissionsPresent.Clone() - Assert-MockCalled -CommandName Test-LoginEffectivePermissions -Scope It -Times 1 -Exactly + Test-ImpersonatePermissions -ServerObject $mockServerObject | Should -Be $true + } + + It 'Should return true when the impersonate login permission is present'{ + $mockInvokeQueryImpersonatePermissionsSet = $mockImpersonateLoginPermissionsPresent.Clone() + + Test-ImpersonatePermissions -ServerObject $mockServerObject -LoginName 'SomeLogin' | Should -Be $true } } Context 'When impersonate permissions are missing for the login' { - Mock -CommandName Test-LoginEffectivePermissions -MockWith { $false } -Verifiable - It 'Should return false when the impersonate permissions are missing for the login'{ - Test-ImpersonatePermissions -ServerObject $mockServerObject | Should -Be $false + $mockInvokeQueryImpersonatePermissionsSet = $mockPermissionsMissing.Clone() - Assert-MockCalled -CommandName Test-LoginEffectivePermissions -Scope It -Times 1 -Exactly + Test-ImpersonatePermissions -ServerObject $mockServerObject | Should -Be $false } } } From 01c939937fb14e22f6422f5127545848113b77c2 Mon Sep 17 00:00:00 2001 From: Cody Konior Date: Sat, 13 Oct 2018 19:21:38 +0800 Subject: [PATCH 3/5] Reduce confusion over Test-ImpersonatePermissions parameter Improve test --- .../MSFT_SqlAGDatabase/MSFT_SqlAGDatabase.psm1 | 2 +- SqlServerDscHelper.psm1 | 10 +++++----- Tests/Unit/SqlServerDSCHelper.Tests.ps1 | 8 +++++++- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/DSCResources/MSFT_SqlAGDatabase/MSFT_SqlAGDatabase.psm1 b/DSCResources/MSFT_SqlAGDatabase/MSFT_SqlAGDatabase.psm1 index 75a955bc2..5dcabd654 100644 --- a/DSCResources/MSFT_SqlAGDatabase/MSFT_SqlAGDatabase.psm1 +++ b/DSCResources/MSFT_SqlAGDatabase/MSFT_SqlAGDatabase.psm1 @@ -246,7 +246,7 @@ function Set-TargetResource $currentAvailabilityGroupReplicaServerObject = Connect-SQL -SQLServer $availabilityGroupReplica.Name $impersonatePermissionsStatus.Add( $availabilityGroupReplica.Name, - ( Test-ImpersonatePermissions -ServerObject $currentAvailabilityGroupReplicaServerObject -LoginName $databaseObject.Owner ) + ( Test-ImpersonatePermissions -ServerObject $currentAvailabilityGroupReplicaServerObject -Securable $databaseObject.Owner ) ) } diff --git a/SqlServerDscHelper.psm1 b/SqlServerDscHelper.psm1 index f4b520d42..c0b6a07c0 100644 --- a/SqlServerDscHelper.psm1 +++ b/SqlServerDscHelper.psm1 @@ -1249,8 +1249,8 @@ function Get-PrimaryReplicaServerObject .PARAMETER ServerObject The server object on which to perform the test. - .PARAMETER LoginName - If set then this specific login is also checked for the specific permission. + .PARAMETER Securable + If set then impersonate permission on tihs specific securable (login) is also checked. #> function Test-ImpersonatePermissions @@ -1261,7 +1261,7 @@ function Test-ImpersonatePermissions [Microsoft.SqlServer.Management.Smo.Server] $ServerObject, - [System.String] $LoginName + [System.String] $Securable ) $impersonatePermissionsPresent = $false @@ -1287,7 +1287,7 @@ function Test-ImpersonatePermissions $impersonatePermissionsPresent = Test-LoginEffectivePermissions @testLoginEffectivePermissionsParams } - if ( -not $impersonatePermissionsPresent -and -not [System.String]::IsNullOrEmpty($LoginName) ) { + if ( -not $impersonatePermissionsPresent -and -not [System.String]::IsNullOrEmpty($Securable) ) { # Check for login-specific impersonation permissions $testLoginEffectivePermissionsParams = @{ SQLServer = $ServerObject.ComputerNamePhysicalNetBIOS @@ -1295,7 +1295,7 @@ function Test-ImpersonatePermissions LoginName = $ServerObject.ConnectionContext.TrueLogin Permissions = @('IMPERSONATE') SecurableClass = 'LOGIN' - Securable = $LoginName + Securable = $Securable } $impersonatePermissionsPresent = Test-LoginEffectivePermissions @testLoginEffectivePermissionsParams } diff --git a/Tests/Unit/SqlServerDSCHelper.Tests.ps1 b/Tests/Unit/SqlServerDSCHelper.Tests.ps1 index 0573a11b0..e970e11e7 100644 --- a/Tests/Unit/SqlServerDSCHelper.Tests.ps1 +++ b/Tests/Unit/SqlServerDSCHelper.Tests.ps1 @@ -1255,18 +1255,24 @@ InModuleScope $script:moduleName { $mockInvokeQueryImpersonatePermissionsSet = $mockImpersonateAnyLoginPermissionsPresent.Clone() Test-ImpersonatePermissions -ServerObject $mockServerObject | Should -Be $true + # It's called once because it short-circuits with the IMPERSONATE ANY LOGIN persmission is present + Assert-MockCalled -CommandName Invoke-Query -Exactly -Times 1 -Scope It } It 'Should return true when the control server permission is present'{ $mockInvokeQueryImpersonatePermissionsSet = $mockControlServerPermissionsPresent.Clone() Test-ImpersonatePermissions -ServerObject $mockServerObject | Should -Be $true + # It's called twice, once for IMPERSONATE ANY LOGIN and again for CONTROL SERVER + Assert-MockCalled -CommandName Invoke-Query -Exactly -Times 2 -Scope It } It 'Should return true when the impersonate login permission is present'{ $mockInvokeQueryImpersonatePermissionsSet = $mockImpersonateLoginPermissionsPresent.Clone() - Test-ImpersonatePermissions -ServerObject $mockServerObject -LoginName 'SomeLogin' | Should -Be $true + Test-ImpersonatePermissions -ServerObject $mockServerObject -Securable 'DatabaseOwner1' | Should -Be $true + # It's called three times, IMPERSONATE ANY, CONTROL SERVER, and then a specific IMPERSONATE LOGIN + Assert-MockCalled -CommandName Invoke-Query -Exactly -Times 3 -Scope It } } From eb80785a66f9e5d749ab9275d2f8a7580bd2b631 Mon Sep 17 00:00:00 2001 From: Cody Konior Date: Sat, 13 Oct 2018 19:24:07 +0800 Subject: [PATCH 4/5] Add owner to database mock --- Tests/Unit/MSFT_SqlAGDatabase.Tests.ps1 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Tests/Unit/MSFT_SqlAGDatabase.Tests.ps1 b/Tests/Unit/MSFT_SqlAGDatabase.Tests.ps1 index 9786856db..fa3de1f51 100644 --- a/Tests/Unit/MSFT_SqlAGDatabase.Tests.ps1 +++ b/Tests/Unit/MSFT_SqlAGDatabase.Tests.ps1 @@ -235,6 +235,8 @@ try #region Database Mocks + $mockDatabaseOwner = 'DatabaseOwner1' + # The databases found on the instance $mockPresentDatabaseNames = @( 'DB1' @@ -272,6 +274,7 @@ try $newDatabaseObject.LogFiles = @{ FileName = ( [IO.Path]::Combine( $mockLogFilePath, "$($mockPresentDatabaseName).ldf" ) ) } + $newDatabaseObject.Owner = $mockDatabaseOwner # Add the database object to the database collection $mockDatabaseObjects.Add($newDatabaseObject) @@ -292,6 +295,7 @@ try $newDatabaseObject.LogFiles = @{ FileName = ( [IO.Path]::Combine( $mockLogFilePathIncorrect, "$($mockPresentDatabaseName).ldf" ) ) } + $newDatabaseObject.Owner = $mockDatabaseOwner # Add the database object to the database collection $mockDatabaseObjectsWithIncorrectFileNames.Add($newDatabaseObject) From b538792b383388c465c456bf119339c58a24f9eb Mon Sep 17 00:00:00 2001 From: Cody Konior Date: Sat, 13 Oct 2018 19:49:42 +0800 Subject: [PATCH 5/5] Add some extra mocked properties --- Tests/Unit/MSFT_SqlAGDatabase.Tests.ps1 | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Tests/Unit/MSFT_SqlAGDatabase.Tests.ps1 b/Tests/Unit/MSFT_SqlAGDatabase.Tests.ps1 index fa3de1f51..30ee8b5f4 100644 --- a/Tests/Unit/MSFT_SqlAGDatabase.Tests.ps1 +++ b/Tests/Unit/MSFT_SqlAGDatabase.Tests.ps1 @@ -76,6 +76,8 @@ try #region mock names + $mockTrueLogin = 'Login1' + $mockDatabaseOwner = 'DatabaseOwner1' $mockServerObjectDomainInstanceName = 'Server1' $mockPrimaryServerObjectDomainInstanceName = 'Server2' $mockAvailabilityGroupObjectName = 'AvailabilityGroup1' @@ -235,8 +237,6 @@ try #region Database Mocks - $mockDatabaseOwner = 'DatabaseOwner1' - # The databases found on the instance $mockPresentDatabaseNames = @( 'DB1' @@ -312,6 +312,9 @@ try $mockServerObject.AvailabilityGroups.Add($mockAvailabilityGroupObject.Clone()) $mockServerObject.AvailabilityGroups.Add($mockAvailabilityGroupWithoutDatabasesObject.Clone()) $mockServerObject.AvailabilityGroups.Add($mockAvailabilityGroupObjectWithPrimaryReplicaOnAnotherServer.Clone()) + $mockServerObject.ComputerNamePhysicalNetBIOS = $mockServerObjectDomainInstanceName + $mockServerObject.ConnectionContext = New-Object -TypeName Microsoft.SqlServer.Management.Smo.ConnectionContext + $mockServerObject.ConnectionContext.TrueLogin = $mockTrueLogin $mockServerObject.Databases = $mockDatabaseObjects $mockServerObject.DomainInstanceName = $mockServerObjectDomainInstanceName $mockServerObject.NetName = $mockServerObjectDomainInstanceName @@ -325,6 +328,9 @@ try $mockServer2Object.AvailabilityGroups.Add($mockAvailabilityGroupObject.Clone()) $mockServer2Object.AvailabilityGroups.Add($mockAvailabilityGroupWithoutDatabasesObject.Clone()) $mockServer2Object.AvailabilityGroups.Add($mockAvailabilityGroupObjectWithPrimaryReplicaOnAnotherServer.Clone()) + $mockServer2Object.ComputerNamePhysicalNetBIOS = $mockPrimaryServerObjectDomainInstanceName + $mockServer2Object.ConnectionContext = New-Object -TypeName Microsoft.SqlServer.Management.Smo.ConnectionContext + $mockServer2Object.ConnectionContext.TrueLogin = $mockTrueLogin $mockServer2Object.Databases = $mockDatabaseObjects $mockServer2Object.DomainInstanceName = $mockPrimaryServerObjectDomainInstanceName $mockServer2Object.NetName = $mockPrimaryServerObjectDomainInstanceName