BREAKING CHANGE: SqlLogin: Parameters no longer enforce default values

[bozho]

Pull Request (PR) description

• SqlLogin
  • LoginMustChangePassword, LoginPasswordExpirationEnabled and LoginPasswordPolicyEnforced
    parameters no longer enforce default values (issue BREAKING CHANGE: SqlLogin: Enforces parameters even if they are not in the configuration #1669).

This Pull Request (PR) fixes the following issues

• Fixes BREAKING CHANGE: SqlLogin: Enforces parameters even if they are not in the configuration #1669

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 

[codecov]

Codecov Report

Merging #1696 (477b768) into main (7907181) will increase coverage by 0%.
The diff coverage is 100%.

@@         Coverage Diff          @@
##           main   #1696   +/-   ##
====================================
  Coverage    97%     97%           
====================================
  Files        38      38           
  Lines      6331    6334    +3     
====================================
+ Hits       6175    6183    +8     
+ Misses      156     151    -5     

Flag	Coverage Δ
unit	97% <100%> (+<1%)	⬆️

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

[johlju]

This is still on the todo list. Just a lot of work getting DnsServerDsc out the door. I will swing back here soon. I will try to get to the backlog of all reviews during Easter. Same for the new PR.

[bozho]

No rush - I create our own "inter-version" builds for modules I've submitted PRs for and host them on a private PS module repo, so I can keep working on my stuff :-)

[johlju]

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

a discussion (no related file):
It seems with this change we are missing unit tests, would you mind add unit tests to cover the code path that are not tested? 🙂

---

[bozho]

@johlju Ok, I think I've added missing code coverage tests. I do have a few questions.

I had to add a new Connect-SQL mock ($mockConnectSQL_SQLLogin_PasswordFlagsFalse), which also made me realise that mocking a command is not scoped inside each It block - I had to "reset" Connect-SQL mock on line 752 in DSC_SqlLogin.Tests.ps1.

I also think that mock on line 808 is wrong:

Mock -CommandName Connect-SQL -MockWith { throw } -Verifiable -ParameterFilter { $SetupCredential }

ParameterFilter will always return $false, since $SetupCredential variable is not declared in DSC_SqlLogin.Tests.ps1.

Would it be a good idea to add an explicit Mock for Connect-SQL inside each It block for Test-TargetResource context, like it's already done for Set-TargetResource context?

[bozho]

It seems that SQL2017 integration test failed because there was a problem with the test host, not because tests failed.

[johlju]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[johlju]

@bozho when you have time can you please rebase this, then when we have the latest code in the PR I will take a look at your question regarding the tests.

Sorry for taking so long until getting to this PR. 😞

[johlju]

I have this on the todo - it was a crazy week at work, but this is next in the todo.

[johlju]

I also think that mock on line 808 is wrong ... ParameterFilter will always return $false, since $SetupCredential variable is not declared in DSC_SqlLogin.Tests.ps1.

I variable is using the parameter from the function Connect-Sql - it evaluates if SetupCredential is set. But $null -ne $SetupCredential or $PSBoundParameters.ContainsKey('SetupCredential') would have been more easy to read.

Would it be a good idea to add an explicit Mock for Connect-SQL inside each It block for Test-TargetResource context, like it's already done for Set-TargetResource context?

The test need to be refactored anyway for Pester 5, so lets leave it as-is. But then we should make sure the mocks are as close to the It-bocks as possible (in BeforeAll- or BeforeEach-blocks, could also be in It-blocks).

[johlju]

Reviewed 1 of 2 files at r2, 1 of 1 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @bozho)

---

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

[issue #1669](https://github.com/dsccommunity/SqlServerDsc/issues/1669).

Lets add parenthesis around this to make it like the others ([issue #1669](https://github.com/dsccommunity/SqlServerDsc/issues/1669)).

---

source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1, line 209 at r4 (raw file):

Quoted 6 lines of code…

                        Write-Verbose -Message (
                            $script:localizedData.SetPasswordPolicyEnforced -f $LoginPasswordPolicyEnforced, $Name, $ServerName, $InstanceName
                        )
                        Write-Verbose -Message (
                            $script:localizedData.SetPasswordExpirationEnabled -f $LoginPasswordExpirationEnabled, $Name, $ServerName, $InstanceName
                        )

These verbose statements should be moved into each if-block below.

---

source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1, line 215 at r4 (raw file):

Quoted 9 lines of code…

                        if ( $PSBoundParameters.ContainsKey('LoginPasswordPolicyEnforced') )
                        {
                            $login.PasswordPolicyEnforced = $LoginPasswordPolicyEnforced
                        }

                        if ( $PSBoundParameters.ContainsKey('LoginPasswordExpirationEnabled') )
                        {
                            $login.PasswordExpirationEnabled = $LoginPasswordExpirationEnabled
                        }

Looking at comment on line 197, please add an integration tests that verifies this do work
See example integration test config:

SqlServerDsc/tests/Integration/DSC_SqlLogin.config.ps1

Line 207 in 8aa0a4c

Configuration DSC_SqlLogin_AddLoginDscUser4_Config

[bozho]

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

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

It seems with this change we are missing unit tests, would you mind add unit tests to cover the code path that are not tested? 🙂

I've only seen this comment now. I've added unit tests back in May, hope they're good enough.

---

---

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

Previously, johlju (Johan Ljunggren) wrote…

[issue #1669](https://github.com/dsccommunity/SqlServerDsc/issues/1669).

Lets add parenthesis around this to make it like the others ([issue #1669](https://github.com/dsccommunity/SqlServerDsc/issues/1669)).

Done.

---

source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1, line 209 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

                        Write-Verbose -Message (
                            $script:localizedData.SetPasswordPolicyEnforced -f $LoginPasswordPolicyEnforced, $Name, $ServerName, $InstanceName
                        )
                        Write-Verbose -Message (
                            $script:localizedData.SetPasswordExpirationEnabled -f $LoginPasswordExpirationEnabled, $Name, $ServerName, $InstanceName
                        )

These verbose statements should be moved into each if-block below.

Done.

---

source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1, line 215 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

                        if ( $PSBoundParameters.ContainsKey('LoginPasswordPolicyEnforced') )
                        {
                            $login.PasswordPolicyEnforced = $LoginPasswordPolicyEnforced
                        }

                        if ( $PSBoundParameters.ContainsKey('LoginPasswordExpirationEnabled') )
                        {
                            $login.PasswordExpirationEnabled = $LoginPasswordExpirationEnabled
                        }

Looking at comment on line 197, please add an integration tests that verifies this do work
See example integration test config:

SqlServerDsc/tests/Integration/DSC_SqlLogin.config.ps1

Line 207 in 8aa0a4c

Configuration DSC_SqlLogin_AddLoginDscUser4_Config

Done.

[johlju]

Reviewed 3 of 3 files at r5.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @bozho)

---

source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1, line 214 at r5 (raw file):

                            Write-Verbose -Message (
                                $script:localizedData.SetPasswordExpirationEnabled -f $LoginPasswordExpirationEnabled, $Name, $ServerName, $InstanceName
                            )

Sorry, my English wasn't clear enough. I meant that the corresponding verbose message should go into the respective if-blocks.
We can remove this verbose message here.

---

source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1, line 222 at r5 (raw file):

                            Write-Verbose -Message (
                                $script:localizedData.SetPasswordPolicyEnforced -f $LoginPasswordPolicyEnforced, $Name, $ServerName, $InstanceName
                            )

We can remove this verbose message here.

---

tests/Integration/DSC_SqlLogin.config.ps1, line 317 at r5 (raw file):

Configuration DSC_SqlLogin_UpdateLoginDscUser4_Config_LoginPasswordExpirationEnabled

Please add a section in the integration test as well, see this:

SqlServerDsc/tests/Integration/DSC_SqlLogin.Integration.Tests.ps1

Lines 430 to 483 in 8aa0a4c

	$configurationName = "$($script:dscResourceName)_AddLoginDscUser4_Config"
	Context ('When using configuration {0} (to update back to original password)' -f $configurationName) {
	It 'Should re-compile and re-apply the MOF without throwing' {
	{
	$configurationParameters = @{
	OutputPath = $TestDrive
	# The variable $ConfigurationData was dot-sourced above.
	ConfigurationData = $ConfigurationData
	}
	& $configurationName @configurationParameters
	$startDscConfigurationParameters = @{
	Path = $TestDrive
	ComputerName = 'localhost'
	Wait = $true
	Verbose = $true
	Force = $true
	ErrorAction = 'Stop'
	}
	Start-DscConfiguration @startDscConfigurationParameters
	} | Should -Not -Throw
	}
	It 'Should be able to call Get-DscConfiguration without throwing' {
	{
	$script:currentConfiguration = Get-DscConfiguration -Verbose -ErrorAction Stop
	} | Should -Not -Throw
	}
	It 'Should return $true when Test-DscConfiguration is run' {
	Test-DscConfiguration -Verbose | Should -Be 'True'
	}
	It 'Should allow SQL Server, login username and password to connect to SQL Instance (using SqlConnection.Open())' {
	$serverName = $ConfigurationData.AllNodes.ServerName
	$instanceName = $ConfigurationData.AllNodes.InstanceName
	$databaseName = $ConfigurationData.AllNodes.DefaultDbName
	$userName = $ConfigurationData.AllNodes.DscUser4Name
	$password = $ConfigurationData.AllNodes.DscUser4Pass1 # Original password
	$sqlConnectionString = 'Data Source={0}\{1};User ID={2};Password={3};Connect Timeout=5;Database={4};' -f $serverName, $instanceName, $userName, $password, $databaseName
	{
	$sqlConnection = New-Object System.Data.SqlClient.SqlConnection $sqlConnectionString
	$sqlConnection.Open()
	$sqlConnection.Close()
	} | Should -Not -Throw
	}
	}
	Wait-ForIdleLcm -Clear

You can copy the marked code and add a test after the one that is marked (marked in the link above).

---

tests/Integration/DSC_SqlLogin.config.ps1, line 343 at r5 (raw file):

Configuration DSC_SqlLogin_UpdateLoginDscUser4_Config_LoginPasswordPolicyEnforced

Same as the previous comment.

[bozho]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @bozho and @johlju)

---

source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1, line 214 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

                            Write-Verbose -Message (
                                $script:localizedData.SetPasswordExpirationEnabled -f $LoginPasswordExpirationEnabled, $Name, $ServerName, $InstanceName
                            )

Sorry, my English wasn't clear enough. I meant that the corresponding verbose message should go into the respective if-blocks.
We can remove this verbose message here.

Oh, no, your English was clear. I copy/pasted both messages into both blocks and meant to delete the unrelated one in each, got distracted and then committed like this, sorry :-)

---

source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1, line 222 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

                            Write-Verbose -Message (
                                $script:localizedData.SetPasswordPolicyEnforced -f $LoginPasswordPolicyEnforced, $Name, $ServerName, $InstanceName
                            )

We can remove this verbose message here.

Same here :)

[bozho]

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

---

tests/Integration/DSC_SqlLogin.config.ps1, line 317 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Configuration DSC_SqlLogin_UpdateLoginDscUser4_Config_LoginPasswordExpirationEnabled

Please add a section in the integration test as well, see this:

SqlServerDsc/tests/Integration/DSC_SqlLogin.Integration.Tests.ps1

Lines 430 to 483 in 8aa0a4c

	$configurationName = "$($script:dscResourceName)_AddLoginDscUser4_Config"
	Context ('When using configuration {0} (to update back to original password)' -f $configurationName) {
	It 'Should re-compile and re-apply the MOF without throwing' {
	{
	$configurationParameters = @{
	OutputPath = $TestDrive
	# The variable $ConfigurationData was dot-sourced above.
	ConfigurationData = $ConfigurationData
	}
	& $configurationName @configurationParameters
	$startDscConfigurationParameters = @{
	Path = $TestDrive
	ComputerName = 'localhost'
	Wait = $true
	Verbose = $true
	Force = $true
	ErrorAction = 'Stop'
	}
	Start-DscConfiguration @startDscConfigurationParameters
	} | Should -Not -Throw
	}
	It 'Should be able to call Get-DscConfiguration without throwing' {
	{
	$script:currentConfiguration = Get-DscConfiguration -Verbose -ErrorAction Stop
	} | Should -Not -Throw
	}
	It 'Should return $true when Test-DscConfiguration is run' {
	Test-DscConfiguration -Verbose | Should -Be 'True'
	}
	It 'Should allow SQL Server, login username and password to connect to SQL Instance (using SqlConnection.Open())' {
	$serverName = $ConfigurationData.AllNodes.ServerName
	$instanceName = $ConfigurationData.AllNodes.InstanceName
	$databaseName = $ConfigurationData.AllNodes.DefaultDbName
	$userName = $ConfigurationData.AllNodes.DscUser4Name
	$password = $ConfigurationData.AllNodes.DscUser4Pass1 # Original password
	$sqlConnectionString = 'Data Source={0}\{1};User ID={2};Password={3};Connect Timeout=5;Database={4};' -f $serverName, $instanceName, $userName, $password, $databaseName
	{
	$sqlConnection = New-Object System.Data.SqlClient.SqlConnection $sqlConnectionString
	$sqlConnection.Open()
	$sqlConnection.Close()
	} | Should -Not -Throw
	}
	}
	Wait-ForIdleLcm -Clear

You can copy the marked code and add a test after the one that is marked (marked in the link above).

Done.

---

tests/Integration/DSC_SqlLogin.config.ps1, line 343 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Configuration DSC_SqlLogin_UpdateLoginDscUser4_Config_LoginPasswordPolicyEnforced

Same as the previous comment.

Done.

[johlju]

Reviewed 2 of 3 files at r7, 1 of 1 files at r8.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @bozho)

---

source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1, line 300 at r8 (raw file):

                        $login.PasswordPolicyEnforced = $LoginPasswordPolicyEnforced
                        $login.PasswordExpirationEnabled = $LoginPasswordExpirationEnabled

Here we still enforce the values (when the login does not exists).

Code quote:

                        $login.PasswordPolicyEnforced = $LoginPasswordPolicyEnforced
                        $login.PasswordExpirationEnabled = $LoginPasswordExpirationEnabled
                        if ( $LoginMustChangePassword )
                        {
                            $LoginCreateOptions = [Microsoft.SqlServer.Management.Smo.LoginCreateOptions]::MustChange
                        }
                        else
                        {
                            $LoginCreateOptions = [Microsoft.SqlServer.Management.Smo.LoginCreateOptions]::None
                        }

---

tests/Integration/DSC_SqlLogin.config.ps1, line 218 at r8 (raw file):

            Name                           = $Node.DscUser4Name
            LoginType                      = $Node.DscUser4Type
            LoginMustChangePassword        = $false

As per other comment, when we no longer have these values to $true as the default value, then I think we need to add another integration tests that adds another SQL login where these values are not set to verify it works not specifying these values.

Code quote:

            LoginMustChangePassword        = $false
            LoginPasswordExpirationEnabled = $true
            LoginPasswordPolicyEnforced    = $true

[johlju]

@bozho sorry to taking so long to get back to this - but now I'm on it. We just need to fix these two last review comments and then I will merge this one. Let me know if you have time to work on this. Again, sorry for the delay.

[johlju]

@bozho just make sure the unit tests passes (so I can merge) - don't bother with adding more unit tests for any new code paths. I'm currently refactoring the unit tests for Pester 5 in another PR so I have to "rewrite" most tests anyway. I will make sure any new code paths are covered by the Pester 5 tests.

[bozho]

No worries, I'll just need to refresh my memory a bit :-)

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

---

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

                    }

                    # `PasswordPolicyEnforced and `PasswordExpirationEnabled` must be updated together (if one or both are not in the desired state)

@johlju Re-reading this comment, it doesn't match with what if below checks.

If PasswordPolicyEnforced and PasswordExpirationEnabled must always be set/updated together, shouldn't the if check that either both or neither of these parameters are specified and then set/update each based on their current settings?

---

source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1, line 300 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Here we still enforce the values (when the login does not exists).

Whoops, missed that one. Let's sort out my comment for line 204 above, then we copy the solution here.

---

tests/Integration/DSC_SqlLogin.config.ps1, line 218 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

As per other comment, when we no longer have these values to $true as the default value, then I think we need to add another integration tests that adds another SQL login where these values are not set to verify it works not specifying these values.

Ok. Should I do anything here, then?

[johlju]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @bozho and @johlju)

---

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

Previously, bozho (Marko Bozikovic) wrote…

@johlju Re-reading this comment, it doesn't match with what if below checks.

If PasswordPolicyEnforced and PasswordExpirationEnabled must always be set/updated together, shouldn't the if check that either both or neither of these parameters are specified and then set/update each based on their current settings?

This PR proposed to change so these two properties can be set individually. You created integration tests to verify that these properties could be set individually, once those new integration tests passes then this comment can be removed as it is wrong.

---

tests/Integration/DSC_SqlLogin.config.ps1, line 218 at r8 (raw file):

Previously, bozho (Marko Bozikovic) wrote…

Ok. Should I do anything here, then?

We need another test after you resolve one of the other comments.

As per current code in this PR when a SQL Login is created the two properties LoginPasswordExpirationEnabled and LoginPasswordPolicyEnforced are always set to $false for a new SQL login.

When you resolve the code at line 300 in DSC_SqlLogin.psm1 (see other comment), then we need another integration tests that adds a new SQL login and do not pass the parameters LoginPasswordExpirationEnabled and LoginPasswordPolicyEnforced to make sure we can create a SQL login without those properties.

[johlju]

No worries, I'll just need to refresh my memory a bit :-)

@bozho awesome! Thank you 🙇‍♂️

[bozho]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @bozho and @johlju)

---

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

Previously, johlju (Johan Ljunggren) wrote…

This PR proposed to change so these two properties can be set individually. You created integration tests to verify that these properties could be set individually, once those new integration tests passes then this comment can be removed as it is wrong.

Ah, I understand what the comment is trying to say here: if either or both parameters are specified and either is not in desired state, we need to use Update-SQLServerLogin to update login data, not that both parameters have to be specified.

The if block staring on line 205 allows us to avoid calling Update-SQLServerLogin twice if both parameters have changed (i.e. calling it for each change).

I'll rewrite the comment to make it understandable on a first read :-)

---

source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1, line 300 at r8 (raw file):

Previously, bozho (Marko Bozikovic) wrote…

Whoops, missed that one. Let's sort out my comment for line 204 above, then we copy the solution here.

@johlju I think this is fine: we are creating a new SQL login here and if $LoginPasswordPolicyEnforced or $LoginPasswordExpirationEnabled are not specified, they'll be $false, which is the new default (changes to lines 166, 170 and 174).

I did a quick manual test. Creating a new Microsoft.SqlServer.Management.Smo.Login instance, those properties are $null. If I set the login type to SqlLogin and execute $login.Create($password), the new login is created with MustChangePassword set to $false, PasswordExpirationEnabled set to $false and PasswordPolicyEnforced set to $true - should this combination be the new default?

[johlju]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @bozho)

---

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

if either or both parameters are specified and either is not in desired state, we need to use Update-SQLServerLogin to update login data, not that both parameters have to be specified.

Exactly 🙂

I'll rewrite the comment to make it understandable on a first read :-)

Sounds good.

---

source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1, line 300 at r8 (raw file):

Previously, bozho (Marko Bozikovic) wrote…

@johlju I think this is fine: we are creating a new SQL login here and if $LoginPasswordPolicyEnforced or $LoginPasswordExpirationEnabled are not specified, they'll be $false, which is the new default (changes to lines 166, 170 and 174).

I did a quick manual test. Creating a new Microsoft.SqlServer.Management.Smo.Login instance, those properties are $null. If I set the login type to SqlLogin and execute $login.Create($password), the new login is created with MustChangePassword set to $false, PasswordExpirationEnabled set to $false and PasswordPolicyEnforced set to $true - should this combination be the new default?

Either that, or we just revert to $true on the parameters, and keep using that as the default. Then there is one less breaking change in this PR. 🤔 It will also keep the login to the most secure (?) and the user must set things to $false to be "less" secure.

But I'm also good with what you suggest above to, keeping it the same as SQL Server does.

I let you decide, I'm good with whatever you decide. :)

[bozho]

Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @johlju)

---

source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1, line 300 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Either that, or we just revert to $true on the parameters, and keep using that as the default. Then there is one less breaking change in this PR. 🤔 It will also keep the login to the most secure (?) and the user must set things to $false to be "less" secure.

But I'm also good with what you suggest above to, keeping it the same as SQL Server does.

I let you decide, I'm good with whatever you decide. :)

I gave this some thought and I think we should leave it like this.

Microsoft.SqlServer.Management.Smo.Login defaults seem sane for automation purposes: it's reasonable to assume the password will be a strong, auto-generated one, so having PasswordPolicyEnforced as true by default is fine.

Having LoginMustChangePassword as true by default would complicate automated deployments and PasswordExpirationEnabled being true by default might also be a bit of a faff, especially if the login is used only "internally" by deployed system. One would expect that a system will have a general password/key rotation policy for all components :-)

---

tests/Integration/DSC_SqlLogin.config.ps1, line 218 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We need another test after you resolve one of the other comments.

As per current code in this PR when a SQL Login is created the two properties LoginPasswordExpirationEnabled and LoginPasswordPolicyEnforced are always set to $false for a new SQL login.

When you resolve the code at line 300 in DSC_SqlLogin.psm1 (see other comment), then we need another integration tests that adds a new SQL login and do not pass the parameters LoginPasswordExpirationEnabled and LoginPasswordPolicyEnforced to make sure we can create a SQL login without those properties.

ah, got it.

[bozho]

@johlju Eh, we have a bit of a mess in ...LoginDscUser4... integration tests...

First, we apply DSC_SqlLogin_AddLoginDscUser4_Config configuration. That one has both password flags (LoginPasswordExpirationEnabled and LoginPasswordPolicyEnforced) set to $true.

Then, we apply DSC_SqlLogin_UpdateLoginDscUser4_Config, which sets both password flag to $false (in addition to changing the password).

Then, we apply DSC_SqlLogin_AddLoginDscUser4_Config again, which resets the password, as well as both password flags back to $true.

Those three are all pre-existing test code (when all three password params had defaults as $true).

The new tests are:
Apply DSC_SqlLogin_UpdateLoginDscUser4_Config_LoginPasswordExpirationEnabled, which only explicitly sets LoginPasswordExpirationEnabled to $true. The Get-DscConfiguration test currently fails here because it tests that LoginPasswordPolicyEnforced is $false, but it's not because it was reset by the second run of DSC_SqlLogin_AddLoginDscUser4_Config configuration. I could change the expectation there to $true, but that makes the test a bit pointless - we've just reset SQL login with DSC_SqlLogin_AddLoginDscUser4_Config and we're setting LoginPasswordExpirationEnabled using DSC_SqlLogin_UpdateLoginDscUser4_Config_LoginPasswordExpirationEnabled to $true, but the flag is already in that state.

Furthermore, I've done some manual testing and setting LoginPasswordExpirationEnabled to $true and keeping LoginPasswordPolicyEnforced=$false will cause [Microsoft.SqlServer.Management.Smo.Login]::Alter() to fail, and the resource code currently does not check for that.

I think the correct order of LoginDscUser4 tests should be:
DSC_SqlLogin_AddLoginDscUser4_Config - LoginPasswordExpirationEnabled and LoginPasswordPolicyEnforced set to $true
DSC_SqlLogin_UpdateLoginDscUser4_Config which will set both password flags to $false.
DSC_SqlLogin_UpdateLoginDscUser4_Config_LoginPasswordPolicyEnforced which would only set LoginPasswordPolicyEnforced to $true
DSC_SqlLogin_UpdateLoginDscUser4_Config_LoginPasswordExpirationEnabled which would only set LoginPasswordExpirationEnabled to $true.
DSC_SqlLogin_AddLoginDscUser4_Config to reset the password.

We should also add either parameter check for LoginPasswordExpirationEnabled and LoginPasswordPolicyEnforced to resource code to disallow the invalid $true/$false combo.

A separate ...LoginDscUser5... test(s) would then test with new defaults ($false).

[johlju]

I agree with all the above. Do as suggested. Though we could skip the extra parameter check for now and that in another PR, just to get this PR through. 🙂

[johlju]

Waiting for the tests to pass.

Reviewed 2 of 3 files at r9, 1 of 1 files at r10.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @bozho)

[johlju]

I will kick off the integration tests that failed once all tests has finished running.

[johlju]

Reviewed 2 of 2 files at r11.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @bozho)

[johlju]

Thank you @bozho!