From c64a6e7c45f0a8e8fd4b8bc58f2ecb6f6e051303 Mon Sep 17 00:00:00 2001 From: Andreas Jordan Date: Fri, 7 Aug 2026 20:11:55 +0200 Subject: [PATCH] Set-DbaLogin - Return the login as it is after unlocking it with a new password Unlocking a login by setting a new password calls Login.ChangePassword with unlock, which does clear the lock on the instance. The login object was only refreshed when -PasswordMustChange was bound, so in every other case the object we return still reported IsLocked as true. Anyone checking the result had to conclude the unlock had failed while the instance said the login was unlocked. It is refreshed now in both cases. The whole test file was invisible. Its integration Context was skipped with a name of "???" and a TODO, and its -TestCases were built in a BeforeAll, which runs after discovery, so the cases were empty and Pester 6 failed the file during discovery. With the cases built in BeforeDiscovery and the Context named and enabled, the file runs 45 tests instead of 1. Two of those tests then failed for reasons of their own. They lock a login out by failing five logons, but after a failed logon SqlClient blocks the connection pool for a growing number of seconds and answers the following attempts itself, so most logons never reached the instance and the login was never locked. They use -NonPooledConnection now, which also makes the Start-Sleep between the attempts unnecessary. The unlock test is what found the stale object above. A login can only be locked out at all when the host running the instance has an account lockout threshold, so the test needs one that is not higher than the five logons it fails. (do Set-DbaLogin) Co-Authored-By: Claude Opus 5 (1M context) --- public/Set-DbaLogin.ps1 | 7 +++-- tests/Set-DbaLogin.Tests.ps1 | 56 ++++++++++++++++++++++-------------- 2 files changed, 39 insertions(+), 24 deletions(-) diff --git a/public/Set-DbaLogin.ps1 b/public/Set-DbaLogin.ps1 index 7f6443a42963..5d4461dce7d3 100644 --- a/public/Set-DbaLogin.ps1 +++ b/public/Set-DbaLogin.ps1 @@ -504,9 +504,10 @@ function Set-DbaLogin { $l.ChangePassword($NewSecurePassword, $Unlock, $PasswordMustChange) $passwordChanged = $true - if (Test-Bound PasswordMustChange) { - $l.Refresh() # necessary so that the read only property PasswordMustChange is updated - } + # necessary so that the read only properties PasswordMustChange and IsLocked are + # updated. Changing the password with unlock clears the lock on the instance, but + # without this the login we return still reports itself as locked. + $l.Refresh() } catch { $notes += "Couldn't change password" $passwordChanged = $false diff --git a/tests/Set-DbaLogin.Tests.ps1 b/tests/Set-DbaLogin.Tests.ps1 index f58a910eec7f..695bf5ee2cf7 100644 --- a/tests/Set-DbaLogin.Tests.ps1 +++ b/tests/Set-DbaLogin.Tests.ps1 @@ -38,21 +38,25 @@ Describe $CommandName -Tag UnitTests { } Describe $CommandName -Tag IntegrationTests { - # TODO: Fix later - Context -Skip "???" { - BeforeAll { + Context "Parameter validation" { + BeforeDiscovery { + # -TestCases is read while Pester discovers the tests. Built in the BeforeAll below, as it + # was before, this list was still empty at discovery, so neither of the role tests existed + # at all - and with Pester 6 the empty list fails the whole file during discovery. $systemRoles = @( - @{role = 'bulkadmin' }, - @{role = 'dbcreator' }, - @{role = 'diskadmin' }, - @{role = 'processadmin' }, - @{role = 'public' }, - @{role = 'securityadmin' }, - @{role = 'serveradmin' }, - @{role = 'setupadmin' }, - @{role = 'sysadmin' } + @{role = "bulkadmin" }, + @{role = "dbcreator" }, + @{role = "diskadmin" }, + @{role = "processadmin" }, + @{role = "public" }, + @{role = "securityadmin" }, + @{role = "serveradmin" }, + @{role = "setupadmin" }, + @{role = "sysadmin" } ) + } + BeforeAll { $command = Get-Command $CommandName } @@ -252,8 +256,9 @@ Describe "$CommandName Integration Tests" -Tag 'IntegrationTests' { $result.PasswordPolicyEnforced | Should -Be $false } - # TODO: The 'locked' test makes assumptions the password policy configuration is enabled for the Windows OS. - It -Skip "Unlock" { + # A login can only be locked out when the host running the instance has an account lockout + # threshold, so this test needs one that is not higher than the number of failed logons below. + It "unlocks a login that was locked out" { $results = Set-DbaLogin -SqlInstance $TestConfig.InstanceSingle -Login "testlogin1_$random" -PasswordPolicyEnforced -EnableException $results.PasswordPolicyEnforced | Should -Be $true @@ -264,10 +269,14 @@ Describe "$CommandName Integration Tests" -Tag 'IntegrationTests' { # exceed the lockout count for (($i = 0); $i -le 4; $i++) { try { - Connect-DbaInstance -SqlInstance $TestConfig.InstanceSingle -SqlCredential $invalidSqlCredential + # NonPooledConnection, because after a failed logon SqlClient blocks the pool for a + # growing number of seconds and answers the following attempts itself. Those never + # reach the instance, so the bad password count stops climbing before it reaches the + # lockout threshold and the login is never locked. + $null = Connect-DbaInstance -SqlInstance $TestConfig.InstanceSingle -SqlCredential $invalidSqlCredential -NonPooledConnection } catch { - Write-Message -Level Warning -Message "invalid login credentials used on purpose to lock out account" - Start-Sleep -s 5 + # Verbose, not a warning: the failed logon is on purpose and a test run must not print warnings. + Write-Verbose -Message "invalid login credentials used on purpose to lock out account" } } @@ -275,8 +284,9 @@ Describe "$CommandName Integration Tests" -Tag 'IntegrationTests' { $results.IsLocked | Should -Be $true # this will generate a warning since neither the password or the -force param is specified - $results = Set-DbaLogin -SqlInstance $TestConfig.InstanceSingle -Login "testlogin1_$random" -Unlock + $results = Set-DbaLogin -SqlInstance $TestConfig.InstanceSingle -Login "testlogin1_$random" -Unlock -WarningAction SilentlyContinue $results | Should -BeNullOrEmpty + ($WarnVar -join " ") | Should -Match "Force" # this will use the workaround solution to turn off/on the check_policy $results = Set-DbaLogin -SqlInstance $TestConfig.InstanceSingle -Login "testlogin1_$random" -Unlock -Force @@ -285,10 +295,14 @@ Describe "$CommandName Integration Tests" -Tag 'IntegrationTests' { # exceed the lockout count again for (($i = 0); $i -le 4; $i++) { try { - Connect-DbaInstance -SqlInstance $TestConfig.InstanceSingle -SqlCredential $invalidSqlCredential + # NonPooledConnection, because after a failed logon SqlClient blocks the pool for a + # growing number of seconds and answers the following attempts itself. Those never + # reach the instance, so the bad password count stops climbing before it reaches the + # lockout threshold and the login is never locked. + $null = Connect-DbaInstance -SqlInstance $TestConfig.InstanceSingle -SqlCredential $invalidSqlCredential -NonPooledConnection } catch { - Write-Message -Level Warning -Message "invalid login credentials used on purpose to lock out account" - Start-Sleep -s 5 + # Verbose, not a warning: the failed logon is on purpose and a test run must not print warnings. + Write-Verbose -Message "invalid login credentials used on purpose to lock out account" } }