Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SqlLogin: Added and amended integration tests for logins of 'SqlLogin' type (Fixes #361, #792, #1032, #1050, #1634) #1652

Merged
merged 71 commits into from
Jan 12, 2021

Conversation

SphenicPaul
Copy link
Contributor

@SphenicPaul SphenicPaul commented Dec 14, 2020

Pull Request (PR) description

Updated the SqlLogin resource integration tests to:

  • Create a new, SqlLogin that doesn't use the same name as the SQL, admin account used in the integration tests
  • Ensure a new, SqlLogin resource of SqlLogin type (i.e. not of type Windows) can be used to connect into the database (once the login is created by the DSC configuration)
  • Ensure the default database connected to (as part of the above) is the same as specified in the resource's DefaultDatabase property/parameter (this didn't appear to be covered by any integration tests)
  • Add assertions/tests (again, to add additional test coverage that seemed to be missing) around the SqlLogin specific parameters (that don't apply to Windows types):
    • LoginPasswordExpirationEnabled
    • LoginPasswordPolicyEnforced
    • LoginMustChangePassword
      Note: I had to add some code (and add/edit related unit tests and mof/help descriptions/comments) to throw an exception if an update to LoginMustChangePassword is attempted on a pre-existing SQL login as there is no functionality with the underlying, SMO libraries to support this at present - it only seems to be available for creating a login or changing a password - and there is a pre-existing/present risk/danger of this providing a false-positive outcome suggesting the update for LoginMustChangePassword had been made when, infact, it had not been.
  • Add configuration to test the update of a SqlLogin type (including assertions of all of the above), and also including password change (to resolve SqlServerLogin: Set-SQLServerLoginPassword setting secure password string for method accepting only plain text version of password #361, SqlLogin: Issues with sql-native accounts.  #1032 and SqlLogin: Add integration test for testing changing passwords for SQL login #1050, and relating to SqlServerLogin: Password test fails for nativ sql users #1048)
  • Amended Wait-ForLCM test, helper function to add -Clear switch usage (to fix SqlLogin: Integration tests fails intermittently #1634) although this is intemittent (and I saw this in approximately 1 of 10 executions) so I can't properly test this will be a permanent solution/fix.

Also added:

This Pull Request (PR) fixes the following issues

Task list

  • Added an entry to the change log under the Unreleased section of the
    file CHANGELOG.md. Entry should say what was changed and how that
    affects users (if applicable), and reference the issue being resolved
    (if applicable).
  • Resource documentation updated in the resource's README.md.
  • Resource parameter descriptions updated in schema.mof.
  • Comment-based help updated, including parameter descriptions.
  • Localization strings updated.
  • Examples updated.
  • Unit tests updated. See DSC Community Testing Guidelines.
  • Integration tests updated (where possible). See DSC Community Testing Guidelines.
  • Code changes adheres to DSC Community Style Guidelines.

This change is Reviewable

…cUser4Name', property value, not 'Admin_UserName' property value
…User4Pass1' and 'DscUser4Pass2' properties in 'SqlLogin', integration test, configuration data.
…DscUser4_Config' and 'DSC_SqlLogin_UpdateLoginDscUser4_Config', integration test configurations
…ure new SQL login can connect into the (correct, default) database
…ar to 'AddLoginDscUser4' ones, but with new password).
… 'LoginPasswordPolicyEnforced' parameter assertions to 'SqlLogin' resource, integration tests for 'SqlLogin' login types.
…disabled' account (replacing incorrect 'if ($Disabled)' condition)
@codecov
Copy link

codecov bot commented Dec 14, 2020

Codecov Report

Merging #1652 (d0d0120) into main (23b977c) will increase coverage by 0%.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff          @@
##           main   #1652   +/-   ##
====================================
  Coverage    97%     97%           
====================================
  Files        38      38           
  Lines      6257    6258    +1     
====================================
+ Hits       6100    6101    +1     
  Misses      157     157           
Flag Coverage Δ
unit 97% <100%> (+<1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1 97% <100%> (+<1%) ⬆️

…'SqlRole' configuration to 'Sqlogin', integration config to support connection into database
…lude 'InstanceName' in connection string. Also added tests for pre/post password change using 'Connect-SQL'.
…r4_Config' configuration to leave 'LoginMustChangePassword', propert value as $false
Copy link
Contributor Author

@SphenicPaul SphenicPaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @johlju)


CHANGELOG.md, line 8 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
## [15.0.1] - 2021-01-09

This section header should be removed, guessing it was wrongly duplicated from below. 🙂

Done.


tests/Integration/DSC_SqlLogin.Integration.Tests.ps1, line 284 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
$sqlConnection = New-Object System.Data.SqlClient.SqlConnection $sqlConnectionString

Let's move this into the scriptblock below together with Open(). If the type does not exist it will throw also, then we are missing something to run the test.

Done.


tests/Integration/DSC_SqlLogin.Integration.Tests.ps1, line 303 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
                $sqlConnection = New-Object System.Data.SqlClient.SqlConnection $sqlConnectionString
                $sqlCommand = New-Object System.Data.SqlClient.SqlCommand('SELECT DB_NAME() as CurrentDatabaseName', $sqlConnection)
                $sqlConnection.Open()
                $sqlDataAdapter = New-Object System.Data.SqlClient.SqlDataAdapter $sqlCommand
                $sqlDataSet = New-Object System.Data.DataSet
                $sqlDataAdapter.Fill($sqlDataSet) | Out-Null
                $sqlConnection.Close()

We should have this in { ... } | Should -Not -Thow. Use $script: scope to pass out variables outside the scriptblock.

OK. I've added $script:CurrentDatabaseName = $null before and after the assertions (to remove risks around value impacting other current and future tests), and also added the lines you specified into a new scriptblock, whilst asserting there is no exception thrown.

Second assertion now ensures the $script:CurrentDatabaseName value assigned within the scriptblock is as expected.


tests/Integration/DSC_SqlLogin.Integration.Tests.ps1, line 373 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
$sqlConnection = New-Object System.Data.SqlClient.SqlConnection $sqlConnectionString

Let's move this into the scriptblock below together with Open().

Done.


tests/Integration/DSC_SqlLogin.Integration.Tests.ps1, line 393 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
                $sqlConnectionString = 'Data Source={0}\{1};User ID={2};Password={3};Connect Timeout=5;' -f $serverName, $instanceName, $userName, $password # Note: Not providing a database name
                $sqlConnection = New-Object System.Data.SqlClient.SqlConnection $sqlConnectionString
                $sqlCommand = New-Object System.Data.SqlClient.SqlCommand('SELECT DB_NAME() as CurrentDatabaseName', $sqlConnection)
                $sqlConnection.Open()
                $sqlDataAdapter = New-Object System.Data.SqlClient.SqlDataAdapter $sqlCommand
                $sqlDataSet = New-Object System.Data.DataSet
                $sqlDataAdapter.Fill($sqlDataSet) | Out-Null
                $sqlConnection.Close()

We should have this in { ... } | Should -Not -Thow. Use $script: scope to pass out variables outside the scriptblock.

As per other comment (re: line 303)...

I've added $script:CurrentDatabaseName = $null before and after the assertions (to remove risks around value impacting other current and future tests), and also added the lines you specified into a new scriptblock, whilst asserting there is no exception thrown.

Second assertion now ensures the $script:CurrentDatabaseName value assigned within the scriptblock is as expected.


tests/Integration/DSC_SqlLogin.Integration.Tests.ps1, line 404 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
        # Note that this configuration has already been run within these Integration tests but is
        # executed once more to reset the password back to the original one provided.

We should use comment-block when the comment is more than 1 row.

<#
    Note that this configuration has already been run within these Integration tests but is
    executed once more to reset the password back to the original one provided.
#>

Done.


tests/Integration/DSC_SqlLogin.Integration.Tests.ps1, line 450 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
$sqlConnection = New-Object System.Data.SqlClient.SqlConnection $sqlConnectionString

Let's move this into the scriptblock below together with Open().

Done.


tests/Integration/DSC_SqlLogin.Integration.Tests.ps1, line 567 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Maybe we should also move this code to its own Context-block 'When preparing the database for removal' (or someting).

OK. I've moved this functionality into the 'When preparing database, dependencies cleanup' context and separated this out from the original test/context.

The new functionality is in it's own 'It' block within a | Should -Not -Throw assertion (with the exception of the variable assignments in the 'Context', not the 'It', as I wanted to use the $defaultDbName variable value in the 'It` name/description).


tests/Integration/DSC_SqlLogin.Integration.Tests.ps1, line 576 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
"ALTER DATABASE [{0}] SET OFFLINE WITH ROLLBACK IMMEDIATE"

We can using single quotes around this string (').

Done.

…icyEnforced' within 'Set-TargetResource' in 'SqlLogin' resource.
…d' and 'PasswordPolicyEnforced' in `SqlLogin` resource.
@SphenicPaul
Copy link
Contributor Author

@johlju - I've now made these revisions.

Additionally the exception (i.e. The CHECK_EXPIRATION option cannot be used when CHECK_POLICY is OFF.) is thrown when the second update is performed on the DscUser4 login. The integration tests (across a few configurations) do this to the login...

(1) Add the login (in DSC_SqlLogin_AddLoginDscUser4_Config)...

        SqlLogin 'Integration_Test'
        {
            Ensure                         = 'Present'
            Name                           = $Node.DscUser4Name
            LoginType                      = $Node.DscUser4Type
            LoginMustChangePassword        = $false
            LoginPasswordExpirationEnabled = $true
            LoginPasswordPolicyEnforced    = $true

            <...snip...>

        }

...then (2) updates the login (i.e. the first update, in DSC_SqlLogin_UpdateLoginDscUser4_Config)...

        SqlLogin 'Integration_Test'
        {
            Ensure                         = 'Present'
            Name                           = $Node.DscUser4Name
            LoginType                      = $Node.DscUser4Type
            LoginMustChangePassword        = $false  # Left the same as this cannot be updated
            LoginPasswordExpirationEnabled = $false
            LoginPasswordPolicyEnforced    = $false

            <...snip...>

        }

... then (3) updates the login again (i.e. the second update) by using the configuration used in the original "Add" configuration (in DSC_SqlLogin_AddLoginDscUser4_Config)...

        SqlLogin 'Integration_Test'
        {
            Ensure                         = 'Present'
            Name                           = $Node.DscUser4Name
            LoginType                      = $Node.DscUser4Type
            LoginMustChangePassword        = $false
            LoginPasswordExpirationEnabled = $true
            LoginPasswordPolicyEnforced    = $true

            <...snip...>

        }

... so given the exception (The CHECK_EXPIRATION option cannot be used when CHECK_POLICY is OFF.), it looked like Set-TargetResource (in the SqlLogin resource) is setting the PasswordExpirationEnabled value before the PasswordPolicyEnforced value.

This did work in the original update (i.e. step/test (1)), but the LoginPasswordPolicyEnforced (I suspect) defaults to $true anyway so, as a result, this add/set doesn't see the exception raised like it does in step/test (3) where step/test (2) had set it to $false).

I've just pushed another change to swap the order in which these are applied within the SqlLogin resource (and updated the changelog with that update).

Not sure if there should be any additional logic/condition within the Set-TargetResource resource to cater for invalid combinations (e.g. where LoginPasswordExpirationEnabled = $true and LoginPasswordPolicyEnforced = $false) or if this should be raised by the SMO library and caught (as it seems to be) by the SqlLogin resource, and/or an additional test to ensure that the logic behaves as expected (whichever way that may be). Either way, I suspect it's a quick change/addition to make, if required. Just let me know what you think?

I'll see how this current build progresses (or doesn't) and will likely make any further changes (if needed) tomorrow.

Let me know if you've any further questions.

Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r5, 1 of 1 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SphenicPaul)


source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1, line 204 at r6 (raw file):

if ( $login.PasswordPolicyEnforced -ne $LoginPasswordPolicyEnforced )

Can we add a comment saying this must always be changed prior to PasswordExpirationEnabled?


tests/Integration/DSC_SqlLogin.Integration.Tests.ps1, line 567 at r4 (raw file):

Previously, SphenicPaul (Paul J. Wilcox) wrote…

OK. I've moved this functionality into the 'When preparing database, dependencies cleanup' context and separated this out from the original test/context.

The new functionality is in it's own 'It' block within a | Should -Not -Throw assertion (with the exception of the variable assignments in the 'Context', not the 'It', as I wanted to use the $defaultDbName variable value in the 'It` name/description).

Just for information, non-blocking. Code inside Context-blocks should be put in a BeforeAll-block, or BeforeEach-block, so that it is faster converting to Pester 5 (that requires that for correct operation). In this case I will fix it when rebasing the Pester 5 PR.

@johlju
Copy link
Member

johlju commented Jan 11, 2021

Not sure if there should be any additional logic/condition within the Set-TargetResource resource to cater for invalid combinations (e.g. where LoginPasswordExpirationEnabled = $true and LoginPasswordPolicyEnforced = $false) or if this should be raised by the SMO library and caught (as it seems to be) by the SqlLogin resource, and/or an additional test to ensure that the logic behaves as expected (whichever way that may be). Either way, I suspect it's a quick change/addition to make, if required. Just let me know what you think?

I think we let the SMO handle the error if it is wrongly configured. There could too many variations to handle them in the resource.

Also I think the root cause is having these set to default values

        [Parameter()]
        [System.Boolean]
        $LoginMustChangePassword = $true,

        [Parameter()]
        [System.Boolean]
        $LoginPasswordExpirationEnabled = $true,

        [Parameter()]
        [System.Boolean]
        $LoginPasswordPolicyEnforced = $true,

The parameters should never be enforced unless the are part of the configuration. Currently the resources enforces values even when the user did not opt-in to enforce them :/ But changing this is a breaking change.

For example, this evaluation:

if ($login.PasswordPolicyEnforced -ne $LoginPasswordPolicyEnforced)

should have been:

if ($PSBoundParameters.ContainsKey('PasswordPolicyEnforced') -and $login.PasswordPolicyEnforced -ne $LoginPasswordPolicyEnforced)

Copy link
Contributor Author

@SphenicPaul SphenicPaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @johlju)


source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1, line 204 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
if ( $login.PasswordPolicyEnforced -ne $LoginPasswordPolicyEnforced )

Can we add a comment saying this must always be changed prior to PasswordExpirationEnabled?

OK. Done. I've added the equivalent comments to both sections of code (in case these ever get split up at any point)

Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r7.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@SphenicPaul
Copy link
Contributor Author

OK. All that makes sense, and latest revisions made.

We'll see how the build goes this time.

@johlju
Copy link
Member

johlju commented Jan 11, 2021

If the tests pass lets merge this. 🙂 I can create an issue to track the breaking code change.

@SphenicPaul
Copy link
Contributor Author

SphenicPaul commented Jan 11, 2021

Also, while I think about (and for the record), the tests are specifying the input/property/parameter values anyway so I’d expect the defaults set in the resource parameters would be ignored in these cases.

I guess the point/issue you raise is still a valid one though and could still occur if no parameter values were provided.

@johlju
Copy link
Member

johlju commented Jan 11, 2021

I guess the point/issue you raise is still a valid one though and could still occur if no parameter values were provided.

Yes, it only occurs for already present SQL logins if no value is provided.

…nforced' and 'PasswordExpirationEnabled' as a single update.
@SphenicPaul
Copy link
Contributor Author

It looks like the latest error is now from a different test - it's now the test that does the first update (when it is setting LoginPasswordPolicyEnforced from $true to $false... whilst LoginPasswordExpirationEnabled is still $true) - It's hitting the error updating the properties in the opposite direction.

I've updated the function once more, now to update both LoginPasswordExpirationEnabled and LoginPasswordPolicyEnforced within the same update passed to Update-SQLServerLogin.

I'll try and check back later.

@johlju
Copy link
Member

johlju commented Jan 11, 2021

Looks like SqlLogin passed now, though it seemed to fail on SqlSetup (intermittent error) so I kick off the tests again tomorrow. Can't do it until all tests finishes and unit tests run for 3 hours.

Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r8.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @SphenicPaul)

@johlju johlju added ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Jan 12, 2021
@johlju johlju merged commit 9130c05 into dsccommunity:main Jan 12, 2021
@johlju johlju removed the ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. label Jan 12, 2021
@johlju
Copy link
Member

johlju commented Jan 12, 2021

@SphenicPaul merged this now! Thank you, awesome work on this! 😃

@SphenicPaul
Copy link
Contributor Author

No worries...Although I do suspect the changlog might still need amending slightly to reflect the final change I made to bring the updates of both the SqlLogin properties into one update.

I suspect the changelog still refers to the ordering change (which is probably less relevant now as both properties now get updated together).

Let me know if you think it needs amending or not. I can raise a quick PR for that later on if needed.

@johlju
Copy link
Member

johlju commented Jan 12, 2021

@SphenicPaul Please update the change log appropriately. 🙂

@SphenicPaul
Copy link
Contributor Author

SphenicPaul commented Jan 12, 2021

OK. Raised PR #1670 to resolve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment