Skip to content

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

Merged
potatoqualitee merged 1 commit into
developmentfrom
fix-setdbalogin-unlock-refresh
Aug 8, 2026
Merged

Set-DbaLogin - Return the login as it is after unlocking it with a new password#10530
potatoqualitee merged 1 commit into
developmentfrom
fix-setdbalogin-unlock-refresh

Conversation

@andreasjordan

Copy link
Copy Markdown
Collaborator

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 out cannot pass. See #10529 for what has 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 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

Against a lab whose host has 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.

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

…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>
@potatoqualitee

Copy link
Copy Markdown
Member

Genuine failure, unfortunately (isn't it nice that we mostly get genuine errors nowadays?)

@potatoqualitee
potatoqualitee merged commit 8276aeb into development Aug 8, 2026
20 of 21 checks passed
@potatoqualitee
potatoqualitee deleted the fix-setdbalogin-unlock-refresh branch August 8, 2026 07:57
@potatoqualitee
potatoqualitee restored the fix-setdbalogin-unlock-refresh branch August 8, 2026 07:58
@potatoqualitee

Copy link
Copy Markdown
Member

oh no! undo undo

@potatoqualitee

Copy link
Copy Markdown
Member

Merged by accident, lemme see how to fix

@andreasjordan

Copy link
Copy Markdown
Collaborator Author

Just skip the one test again.

potatoqualitee pushed a commit that referenced this pull request Aug 8, 2026
…ith a new password"

This reverts commit 8276aeb, the squash merge of #10530,
which was merged by mistake. The work is unchanged on fix-setdbalogin-unlock-refresh and
can come back through a new PR.

(do Set-DbaLogin)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@potatoqualitee

Copy link
Copy Markdown
Member

i reverted it, looks like it'll need a new PR, sorry

@potatoqualitee
potatoqualitee deleted the fix-setdbalogin-unlock-refresh branch August 8, 2026 08:04
@andreasjordan

Copy link
Copy Markdown
Collaborator Author

No problem, I still have the local branch. Will open a new pr after the environment change.

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