SqlAGDatabase: Fix impersonate permission test

[codykonior]

Pull Request (PR) description

When adding a database to an AG using SqlAGDatabase and the MatchDatabaseOwner switch, it checks for an IMPERSONATE ANY LOGIN permission.

That permission doesn't exist < SQL 2014. There are two alternatives:

• CONTROL SERVER (also granted as part of the sysadmin server role) permission
• IMPERSONATE permission

This change adds those two checks. It also fixes the help text that said MatchDatabaseOwner is true by default, when it was always false by default.

This Pull Request (PR) fixes the following issues

• Fixes SqlAGDatabase: Impersonate permission incorrectly checked on SQL 2012 #1237

Task list

• Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
  Entry should say what was changed, and how that affects users (if applicable).
• Resource documentation added/updated in README.md.
• Resource parameter descriptions added/updated in README.md, schema.mof
  and comment-based help.
• Comment-based help added/updated.
• Localization strings added/updated in all localization files as appropriate.
• Examples appropriately added/updated.
• Unit tests added/updated. See DSC Resource Testing Guidelines.
• Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
• New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

---

This change is 

[codecov-io]

Codecov Report

Merging #1239 into dev will increase coverage by <1%.
The diff coverage is 96%.

@@          Coverage Diff          @@
##            dev   #1239    +/-   ##
=====================================
+ Coverage    97%     98%   +<1%     
=====================================
  Files        35      34     -1     
  Lines      4323    4190   -133     
=====================================
- Hits       4220    4131    -89     
+ Misses      103      59    -44

[codykonior]

The CI failure seem to be unrelated to my CR.

I can attempt to deal with the codecov/patch stuff though.

[johlju]

@codykonior Yes, it seems there is a intermittent error that occurs sometimes. Hopefully it works on the next push. It is also possible to kick off the tests again by closing and then reopening this PR. But let's wait for next push if you can look at getting more coverage of the changed code.

[johlju]

@codykonior I wait to review this until you resolved the missing code coverage. 🙂

[codykonior]

That second build failed for unrelated reasons again but this one passed.

I haven’t been able to find the last few % of code coverage, the reporting doesn’t seem to indicate where it is.

I put in some extra properties to the AGDatabase test file. These are needed if the tests are expanded to remove the Test-EffectivePermissions mock in favour of an Invoke-Query mock. I tried it out but didn’t modify the tests because it should already be covered, and doing so would create a dependency for that test on the SQLPS/SqlServer module that wasn’t there before (not sure why, sends it down more code paths I guess).

So basically let me know if you know what’s not being tested and if you need more.

[johlju]

Sorry it took so long to review this. :/

Reviewed 1 of 3 files at r1, 4 of 4 files at r2.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @codykonior)

---

SqlServerDscHelper.psm1, line 1087 at r2 (raw file):

            'LOGIN', 'MESSAGE TYPE', 'OBJECT', 'REMOTE SERVICE BINDING', 'ROLE', 'ROUTE', 'SCHEMA', 'SERVER', 'SERVICE', 'SYMMETRIC KEY',
            'TYPE', 'USER', 'XML SCHEMA COLLECTION')]
        [System.String] $SecurableClass = 'SERVER',

We should use [Parameter()] on non-mandatory parameters.

---

SqlServerDscHelper.psm1, line 1087 at r2 (raw file):

            'LOGIN', 'MESSAGE TYPE', 'OBJECT', 'REMOTE SERVICE BINDING', 'ROLE', 'ROUTE', 'SCHEMA', 'SERVER', 'SERVICE', 'SYMMETRIC KEY',
            'TYPE', 'USER', 'XML SCHEMA COLLECTION')]
        [System.String] $SecurableClass = 'SERVER',

For parameter variables, then parameter variable name should be on a separate row (see other parameters).

---

SqlServerDscHelper.psm1, line 1089 at r2 (raw file):

        [System.String] $SecurableClass = 'SERVER',

        [System.String] $Securable

We should use [Parameter()] on non-mandatory parameters.

---

SqlServerDscHelper.psm1, line 1089 at r2 (raw file):

        [System.String] $SecurableClass = 'SERVER',

        [System.String] $Securable

For parameter variables, then parameter variable name should be on a separate row (see other parameters).

---

SqlServerDscHelper.psm1, line 1102 at r2 (raw file):

if ($null -eq $Securable) {

We should have opening brace on a separate row.

---

SqlServerDscHelper.psm1, line 1103 at r2 (raw file):

Quoted 8 lines of code…

    if ($null -eq $Securable) {
        $queryToGetEffectivePermissionsForLogin = "
            EXECUTE AS LOGIN = '$LoginName'
            SELECT DISTINCT permission_name
            FROM fn_my_permissions(null, '$SecurableClass')
            REVERT
        "
    } else {

It seems the missing row is the new code path in line 1103 in SqlServerDscHelper.psm1. It never get to line 1103 because there is no unit test that sets $Securable to $null that is evaluated on line 1102.

You can verify this locally using

# Make sure the framework is cloned and the correct types are loaded.
.\Assert-TestEnvironment.ps1 
# Runs the unit test with code coverage.
Invoke-Pester .\Tests\Unit\SqlServerDSCHelper.Tests.ps1 -CodeCoverage .\SqlServerDscHelper.psm1

---

SqlServerDscHelper.psm1, line 1109 at r2 (raw file):

} else {

We should have closing and opening brace on a separate rows.

---

SqlServerDscHelper.psm1, line 1253 at r2 (raw file):

tihs

typo: this

---

SqlServerDscHelper.psm1, line 1264 at r2 (raw file):

        $ServerObject,

        [System.String] $Securable

We should use [Parameter()] on non-mandatory parameters.

---

SqlServerDscHelper.psm1, line 1264 at r2 (raw file):

        $ServerObject,

        [System.String] $Securable

For parameter variables, then parameter variable name should be on a separate row (see other parameters).

---

SqlServerDscHelper.psm1, line 1269 at r2 (raw file):

# Only exists on this is SQL 2014 or newer

Can we rephrase this to what does only exists in SQL 2014?

---

SqlServerDscHelper.psm1, line 1276 at r2 (raw file):

Quoted 7 lines of code…

    $testLoginEffectivePermissionsParams = @{
        SQLServer       = $ServerObject.ComputerNamePhysicalNetBIOS
        SQLInstanceName = $ServerObject.ServiceName
        LoginName       = $ServerObject.ConnectionContext.TrueLogin
        Permissions     = @('IMPERSONATE ANY LOGIN')
    }
    $impersonatePermissionsPresent = Test-LoginEffectivePermissions @testLoginEffectivePermissionsParams

Since we have a $ServerObject parameter, then we now the instance name (or default instance). Could we actually call the existing helper function Get-SqlInstanceMajorVersion to know what the major version is and only call this if the version is 2014 or newer?

---

SqlServerDscHelper.psm1, line 1276 at r2 (raw file):

        Permissions     = @('IMPERSONATE ANY LOGIN')
    }
    $impersonatePermissionsPresent = Test-LoginEffectivePermissions @testLoginEffectivePermissionsParams

Since we are making three different permission tests, could we add appropriate verbose message to each Test-LoginEffectivePermissions so the user can now what permission is actually found/used? It would ease problem debugging too.

---

SqlServerDscHelper.psm1, line 1283 at r2 (raw file):

$ServerObject.ServiceName

We should not be using $ServerObject.InstanceName?

---

SqlServerDscHelper.psm1, line 1294 at r2 (raw file):

$ServerObject.ServiceName

We should not be using $ServerObject.InstanceName?

---

SqlServerDscHelper.psm1, line 1305 at r2 (raw file):

    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 )

Would it be appropriate that we add what permission (either one) is expected if no permission is found? Would it be to complex, better in the docs? 🤔

---

DSCResources/MSFT_SqlAGDatabase/MSFT_SqlAGDatabase.psm1, line 145 at r2 (raw file):

 The default is '$true'.

This should change to $false as well.

---

DSCResources/MSFT_SqlAGDatabase/MSFT_SqlAGDatabase.psm1, line 249 at r2 (raw file):

                    $impersonatePermissionsStatus.Add(
                        $availabilityGroupReplica.Name,
                        ( Test-ImpersonatePermissions -ServerObject $currentAvailabilityGroupReplicaServerObject -Securable $databaseObject.Owner )

Curious. Did the MatchDatabaseOwner parameter even work before since it never referenced the actual owner of the databases?

---

DSCResources/MSFT_SqlAGDatabase/MSFT_SqlAGDatabase.psm1, line 611 at r2 (raw file):

        If set to $false, the owner of the database will be the PSDscRunAsCredential.

        The default is '$false'.

Please make this change in the README.md for the parameter as well.

---

Tests/Unit/SqlServerDSCHelper.Tests.ps1, line 1262 at r2 (raw file):

present'{

Missing a space before the open brace.

---

Tests/Unit/SqlServerDSCHelper.Tests.ps1, line 1267 at r2 (raw file):

                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

If we instead mock the call to Test-LoginEffectivePermissions then we should be able to use a ParameterFilter on two different Assert-MockCalled to filter out that we actually call 'Impersonate..' and 'COntrol...' once each.

I think mocking Test-LoginEffectivePermissions would also simplify this test, and give better control testing this helper function?

[stale]

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

[codykonior]

Trying to rebase has broken this. I'll re-raise it.