Set-DbaLogin - Return the login as it is after unlocking it with a new password - #10537
Draft
andreasjordan wants to merge 2 commits into
Draft
Set-DbaLogin - Return the login as it is after unlocking it with a new password#10537andreasjordan wants to merge 2 commits into
andreasjordan wants to merge 2 commits into
Conversation
…w 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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
This is a draft on purpose. It restores #10530, which was squash merged by mistake and then reverted
in fc09467. It is kept as a draft so it cannot land again before it has been reviewed. Mark it ready
when you want it.
Important
One test in this PR will fail on CI, on purpose. The runners have no account lockout threshold, so a
login cannot be locked out there and
unlocks a login that was locked outcannot pass. See #10529 for whathas to change on the runner image. Skipping the test instead would hide the very regression this PR fixes,
so it is left enabled and red until the runners are configured. Everything else in the file passes.
The change is the original commit of #10530, cherry-picked onto current
developmentand unmodified.The bug
Unlocking a login by setting a new password calls
Login.ChangePassword($NewSecurePassword, $Unlock, $PasswordMustChange), which really does clear the lock on the instance. But the login object was only refreshed when-PasswordMustChangewas bound, so in every other case the object returned still reportedIsLockedas$true:Anyone checking the result had to conclude the unlock had failed while the instance said it had worked. The object is refreshed in both cases now.
Why nobody noticed
The test file was invisible. Its integration
Contextwas skipped with a name of"???"and a# TODO: Fix later, and its-TestCaseswere built in aBeforeAll, which runs after discovery, so the cases were empty. Pester 5 silently produced no tests from an empty case list; Pester 6 fails the file during discovery instead, which is how this surfaced.With the cases built in
BeforeDiscoveryand theContextnamed and enabled, the file runs 45 tests instead of 1.The lockout tests needed a second fix
Two tests lock a login out by failing five logons. After a failed logon SqlClient blocks the connection pool for a growing number of seconds and answers the following attempts itself, so most of those logons never reached the instance.
BadPasswordCountplateaued at 3 no matter how many attempts were made, even with theStart-Sleep -s 5the test already had, so the login was never locked.They use
-NonPooledConnectionnow. The bad password count then climbs one per attempt and the login locks exactly at the threshold, which also makes the sleeps unnecessary and the file faster.Testing
Re-verified on 2026-08-08 against current
development, on a lab whose hosts have an account lockout threshold of 5: 45 passed, 0 failed, 0 skipped, 0 warnings.The precondition is only the threshold. SQL Server reads it from the local policy of the host running the instance, not from the domain policy - verified in a lab where the domain has no threshold and the local one has five, and the lockout follows the local value.
One note for #10529, and the scope matters. On a domain member,
net accounts /lockoutthreshold:5does not survive: the Default Domain Policy re-applies its own account policy to the local account database at boot and every 16 hours, so the value silently goes back toNever. That is how the lab hosts lost it after a reboot, and there it had to move into a GPO linked to the OU the machines live in. The CI runners are not domain joined, so none of that applies to them - there is no domain policy to overwrite the value and setting it on the image is enough.After #10529
Once the runners have a threshold, this test passes on CI unchanged. No follow-up change to this file is needed.
🤖 Generated with Claude Code