Skip to content

Set-DbaLogin - Return the login as it is after unlocking it with a new password - #10537

Draft
andreasjordan wants to merge 4 commits into
developmentfrom
restore-setdbalogin-unlock-refresh
Draft

andreasjordan wants to merge 4 commits into
developmentfrom
restore-setdbalogin-unlock-refresh

Conversation

@andreasjordan

@andreasjordan andreasjordan commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

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 fails on CI until the fleet controller is redeployed. The runners need an account
lockout threshold for unlocks a login that was locked out to pass. #10718 added net accounts /lockoutthreshold:5
to bootstrap-runner.ps1 and is merged, but the controller bundles its own copy of that script when
deploy-controller.ps1 packages it, so new runners keep the old bootstrap until the controller is deployed again
(see the comment on this PR for the evidence). Skipping the test instead would hide the very regression this PR fixes,
so it is left enabled and red until then. Everything else in the file passes.

The change is the original commit of #10530, cherry-picked onto development and unmodified. The branch has since
taken current development (v2.9.0) by merge.

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 -PasswordMustChange was bound, so in every other case the object returned still reported IsLocked as $true:

locked before unlock (server): 1
returned object IsLocked : True
server says IsLocked     : 0

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 Context was skipped with a name of "???" and a # TODO: Fix later, and its -TestCases were built in a BeforeAll, 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 BeforeDiscovery and the Context named 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. BadPasswordCount plateaued at 3 no matter how many attempts were made, even with the Start-Sleep -s 5 the test already had, so the login was never locked.

They use -NonPooledConnection now. 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-09-19 against current development (after the merge), on a lab whose hosts have an account lockout threshold of 5: 45 passed, 0 failed, 0 skipped in Windows PowerShell 5.1 and in pwsh 7.

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.

On a domain member, net accounts /lockoutthreshold:5 does 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 to Never. 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 - the bootstrap value is enough.

After the controller redeploy

Once the runners have the threshold, this test passes on CI unchanged. No follow-up change to this file is needed.

🤖 Generated with Claude Code

andreasjordan and others added 3 commits August 8, 2026 18:05
…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>
@andreasjordan

Copy link
Copy Markdown
Collaborator Author

Status after #10718 merged: the branch took current development (e648e6e), the file passes 45/45 in the lab in Windows PowerShell 5.1 and pwsh 7, and CI run 35431752494 is red on exactly the one expected test, unlocks a login that was locked out, on all three attempts on runner dbatoolsTKDT98. The login never locked, so that runner had no lockout threshold.

Why the merged #10718 did not reach it: runners are bootstrapped by the fleet controller, and the controller does not read bootstrap-runner.ps1 from the repo. deploy-controller.ps1 copies .github/runners/bootstrap-runner.ps1 into Modules/FleetCore/ at packaging time (lines 78-87 and the expected list at line 106), and FleetCore.psm1 resolves it from its own module directory (Resolve-FleetFile at line 132, read at line 1145). The GitHub runner-reconcile.yml workflow, which would fetch the raw file from development, last ran on 2026-08-02. So every runner still gets the bootstrap that was bundled into the last controller deployment, without the net accounts lines.

What is needed: one ./deploy-controller.ps1 run against dbatools-fleet-controller in dbatools-ci, which I cannot do from here (no Azure access). After that a rerun of this PR's CI should turn the test green, and I will drop the red-by-design note from the description.

created by Claude and reviewed by Andreas Jordan

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants