Repository navigation
Sync-DbaAvailabilityGroup - Add -UpdateExistingLogin pass-through to Copy-DbaLogin - #10802
andreasjordan wants to merge 12 commits into
Conversation
potatoqualitee
left a comment
There was a problem hiding this comment.
Reviewed at 62d22b9.
The AG-specific parameter forwarding and integration test are sound. This stacked head still contains the material permission-sync defect in the shared in-place login implementation: when the source login has no direct server permissions, destination-only direct grants are never enumerated or revoked. A login with inherited CONNECT SQL can retain a destination-only direct CONTROL SERVER grant after an UpdateExistingLogin sync. The later LoginPermissions step calls the same helper and does not correct it.
The three shared implementation/test files are byte-identical to the reviewed #10801 head. I independently validated the helper's skipped reconciliation in a PowerShell 7.6.6 probe and traced SMO's direct-grantee permission enumeration. The inline finding gives the concrete setup and required correction. No live SQL Server reproduction was run locally.
Reviewed the complete five-file patch, surrounding source, tests, intended behavior, discussions, and exact-head CI. All 20 executed checks passed; one generic Windows check was skipped. The COPY job explicitly ran and passed both Copy-DbaLogin.Tests.ps1 and Sync-DbaAvailabilityGroup.Tests.ps1.
| ObjectLevel = $ObjectLevel | ||
| ExcludeDatabaseMapping = $ExcludeDatabaseMapping | ||
| } | ||
| Update-SqlPermission @splatPermission |
There was a problem hiding this comment.
[P1] Reconcile destination permissions when the source list is empty
This new in-place call preserves the destination principal, but Update-SqlPermission only enumerates and revokes destination server permissions inside foreach ($perm in $SourceServer.EnumServerPermissions($loginName)). When the source has no direct server permissions, that entire reconciliation is skipped.
A concrete case is a source login that receives CONNECT SQL through a user-defined server role: grant CONNECT SQL to the role, add app_login, then REVOKE CONNECT SQL FROM app_login. Create the same-SID login and corresponding role on the destination, with an additional direct GRANT CONTROL SERVER TO app_login. Copy-DbaLogin -UpdateExisting -Force updates the existing login but leaves CONTROL SERVER in place. Force-only replacement removes that grant when it drops the principal.
SMO's server-permission enumeration filters sys.server_permissions by the named grantee; it does not include inherited role permissions. I independently executed this exact helper under PowerShell 7.6.6 with recording server objects: the empty source list was read once, the destination list was never read, and no revoke occurred. This was a control-flow probe, not a live SQL Server integration run.
Move destination permission reconciliation outside the source-permission loop so it runs even for an empty source list. Add a regression asserting removal of the extra direct destination grant while preserving the intended role-based access.
This same implementation is included in this stacked PR. Sync-DbaAvailabilityGroup -UpdateExistingLogin selects it, and the subsequent LoginPermissions step also uses Update-SqlPermission, so that later step does not repair the excess grant.
There was a problem hiding this comment.
Confirmed, the same fix as in #10801: b3c77091f is cherry-picked here as 3e194823f, so the LoginPermissions step, which calls the same helper, no longer leaves such a grant either. The regression test is the new context in Copy-DbaLogin.Tests.ps1 described in the reply on #10801.
Lab: Sync-DbaAvailabilityGroup.Tests.ps1 11 of 11 on Windows PowerShell 5.1 and on PowerShell 7.6.
This text was created by Claude and reviewed by Andreas Jordan.
potatoqualitee
left a comment
There was a problem hiding this comment.
Reviewed exact stacked head 3e194823f900f0339545e090f347cc00fee846f4, including the AG-specific forwarding, its integration test, and the inherited Copy-DbaLogin implementation; passing CI confirmed. The prior destination-only permission grant finding is fixed.
[P1] Inherited destructive login replacement when the source password hash is unreadable (public/Copy-DbaLogin.ps1, near lines 352–383). This branch includes the same failing pre-drop guard as #10801: when the source login can be enumerated but its password hash cannot be read (for example, insufficient source permissions on SQL Server 2025), $sourceHash is null, the incompatibility checks do not match, and -Force can drop an existing destination login. The subsequent New-DbaLogin attempt also cannot obtain the hash and fails, leaving the login missing. Sync-DbaAvailabilityGroup -UpdateExistingLogin -Force reaches this exact path for an existing secondary login with a different SID, so the new feature inherits the destructive failure. Without -UpdateExistingLogin, AG sync with -Force can reach it too.
Required correction: carry over a fail-closed guard from #10801 that checks hash availability before any drop, not just recognized unsupported hash prefixes, and test that an unreadable source hash leaves the destination/secondary login intact. This must be corrected in the stacked head before approval.
|
Confirmed, the same fix as in #10801: Lab: This text was created by Claude and reviewed by Andreas Jordan. |
Without -Force an existing login was skipped, so a password changed on the source never reached the destination. With -Force it was dropped and recreated after its databases and jobs were given to sa. -UpdateExisting compares the password hash, CHECK_POLICY, CHECK_EXPIRATION, default database, default language and disabled state, and applies the differences with ALTER LOGIN in one transaction on the SMO connection. SQL Server refuses a hashed password while CHECK_POLICY is on (error 33020), so the policy is turned off and set back around it. SID, ownerships and sessions stay; permissions are synced as after a copy. A login with a different SID is skipped with a warning unless -Force is also given, and the Windows group lockout check of #8572 applies as for a drop. Also fixes two cases where -Force dropped a login it could not recreate, now one check per SQL login before anything is dropped: - 2012+ source to an older destination: the existing check called Stop-Function without -Continue, so the copy went on after the warning. - SQL Server 2025 hashes (0x0300), which SQL Server 2019 refuses. Windows logins are still copied to older destinations. Part of #10772 (do Copy-DbaLogin) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…Copy-DbaLogin The Logins step called Copy-DbaLogin -Force:$Force only, so an AG sync either left stale passwords on the secondaries or dropped and recreated every login on every run. -UpdateExistingLogin is passed through as -UpdateExisting, so logins that differ from the primary are updated in place and identical ones are left alone. Named for the Logins step only, like -UseJobLastModified. The Copy-DbaLogin call moves into a splat, as it now has six parameters. Fixes #10772 (do Sync-DbaAvailabilityGroup) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…login with the source name Update-SqlPermission checked whether the destination login was in a server role the source login is not in, and then dropped the name of the source login from that role. Without a rename both names are the same. With -LoginRenameHashtable they differ, and -UpdateExisting reaches this with the memberships the existing login already has: the renamed login stayed in the role, and a different login that happens to have the name of the source login lost it. The role messages named the undefined $destRoleName and said "Adding" for the removal. The new context renames a login that is in no server role to one that is a sysadmin on the destination, next to another sysadmin with the name of the source login. Part of #10772 (do Copy-DbaLogin) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ace fails The destination can be a server object of the caller with a transaction open on its connection. The update then began a nested transaction, and after a failing statement its rollback, which SMO sends as an unnamed ROLLBACK TRANSACTION, took back the whole transaction of the caller with the work done in it. The update now begins a transaction only when none is open. Inside a transaction of the caller it sets a savepoint, takes its statements back to it after a failure as long as the transaction can still commit, and leaves the commit to the caller. The new context fails the last statement of an update inside a transaction of the test and checks on that connection that the transaction and its temporary table are still there and the login is as it was. Part of #10772 (do Copy-DbaLogin) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ion older than SQL Server 2005 The check for database mappings on a destination older than SQL Server 2005 ended with a bare continue in a function without a loop of its own, so it went on with the next iteration of the loop of the caller. Sync-DbaLoginPermission, which accepts SQL Server 2000 destinations, calls it in its loop over the logins: the rest of the iteration was skipped, and every login came without a result row, neither Successful nor Failed. Copy-DbaLogin calls it in a loop of its own around the call and was not affected. It is a return now. No lab or CI instance runs SQL Server 2000, so there is no regression test; the binding was checked with a probe of the same shape on Windows PowerShell 5.1 and PowerShell 7.6. (do Copy-DbaLogin, Sync-DbaLoginPermission) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… does not have Update-SqlPermission walked the server roles of the source only. A user-defined server role that exists on the destination only was never visited, so a login updated in place with -UpdateExisting kept its membership there, and with it a permission like CONTROL SERVER of that role, while a login that -Force drops and recreates loses it. The helper now also walks the roles of the destination that the source does not have and takes the login out of them; the role and its other members stay. Sync-DbaLoginPermission uses the same helper and removes such a membership now as well, as its description of a sync says. The new context creates such a role with CONTROL SERVER on the destination, with the login and a second login as members, and checks that only the updated login left it. Part of #10772 (do Copy-DbaLogin, Sync-DbaLoginPermission) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… login has none Update-SqlPermission compared the direct server permissions of the destination login with those of the source inside the loop over the source permissions. For a source login without any direct server permission, one that gets CONNECT SQL through a server role for example, that loop never ran, so a login updated in place with -UpdateExisting kept every direct grant it had on the destination, CONTROL SERVER included, while a login that -Force drops and recreates loses it. The comparison runs once after that loop now. Sync-DbaLoginPermission uses the same helper and revokes such a grant now as well, and a recreated login loses the CONNECT SQL of CREATE LOGIN when the source login has none, as the helper already did for a source login with other permissions. The new context revokes CONNECT SQL from the source login, which connects through a user-defined server role, grants CONNECT SQL and CONTROL SERVER directly to the destination login, and checks that both are gone after the update while the login connects through the role. Part of #10772 (do Copy-DbaLogin, Sync-DbaLoginPermission) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The check before the drop read the password hash of a SQL login only for a source of SQL Server 2012 or later and a destination older than SQL Server 2022, and it treated a hash it could not read as compatible. LOGINPROPERTY returns no hash to a source connection without CONTROL SERVER, even when that connection can see the logins, so -Force, and -UpdateExisting -Force for a login with a different SID, dropped the destination login, and New-DbaLogin then failed with "Invalid cast from 'System.DBNull' to 'System.Byte[]'" and left the login missing. Development does the same with -Force. The hash is now read for every SQL login before anything is dropped, and a hash that could not be read gives a Failed row with a warning. The check of the hash format still runs for the same versions as before. Sync-DbaAvailabilityGroup and Start-DbaMigration call Copy-DbaLogin and get the same check. The new context connects to the source as a login with VIEW ANY DEFINITION only and checks that -Force and -UpdateExisting -Force leave the destination login with its SID. Part of #10772 (do Copy-DbaLogin, Sync-DbaAvailabilityGroup, Start-DbaMigration) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r than SQL Server 2005 -UpdateExisting skipped a destination older than SQL Server 2005, because ALTER LOGIN came with 2005. Since #10813, Connect-DbaInstance refuses every instance older than SQL Server 2008, so the check can no longer be reached, and the version guide asks for no new SQL Server 2000 or 2005 code paths. Part of #10772 (do Copy-DbaLogin) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ffab219 to
5237070
Compare
potatoqualitee
left a comment
There was a problem hiding this comment.
Reviewed head 52370703046196bd341f0934e6490c59a63ff667.
Re-reviewed the current stacked head, including the inherited Copy-DbaLogin corrections and the availability-group forwarding test. The previously identified unreadable-hash drop path is guarded before replacement, including the -Force path. No remaining material defect identified. Depends on #10801.
potatoqualitee
left a comment
There was a problem hiding this comment.
Fresh full review of head 52370703046196bd341f0934e6490c59a63ff667.
Requesting changes for two material defects in the newly reachable in-place permission synchronization, detailed inline:
- Destination-only permissions on server securables, including IMPERSONATE on a privileged login, survive reconciliation.
- GrantWithGrant -> Grant can revoke the desired permission itself, including CONNECT SQL.
The earlier unreadable-password-hash and caller-transaction fixes are present. These remaining cases are not covered by the new integration contexts. I independently validated both findings against the current source, SMO's enumeration/overload contracts and permission semantics, then executed exact-helper recording probes. Those probes establish the calls/arguments; they are not live SQL integration tests. The current import/smoke/security checks pass, while the main ci-azure run remains pending.
The three inherited implementation/test files are byte-identical to #10801. The new AG flag forwards correctly, but it exposes these defects on secondaries. For the grant-option defect, the AG's later LoginPermissions pass may restore a removed grant; the supported configuration excluding LoginPermissions leaves the login broken. The impersonation grant survives either way.
| ObjectLevel = $ObjectLevel | ||
| ExcludeDatabaseMapping = $ExcludeDatabaseMapping | ||
| } | ||
| Update-SqlPermission @splatPermission |
There was a problem hiding this comment.
[P1] Reconcile permissions on server securables before treating an in-place login as synchronized
With the same-SID app_login on both instances, give only the destination GRANT IMPERSONATE ON LOGIN::sa TO app_login, then run Copy-DbaLogin -Source src -Destination dst -Login app_login -UpdateExisting -Force. This branch retains the principal and calls Update-SqlPermission, but that helper only calls Server.EnumServerPermissions for server permission reconciliation. SMO's SrvYukonPerm.xml explicitly restricts those results to prmssn.class = 100; the impersonation grant is class 101 and belongs to the separate object-permission enumeration. No such enumeration/removal occurs here. The login therefore retains the ability to impersonate sa, whereas the previous Force replacement removes the grant when the principal is dropped. -ObjectLevel in the helper only handles database-user objects and does not cover this.
Please reconcile destination-only permissions on server securables as well as class-100 permissions, matching the securable and mapped grantee correctly, and add a real SQL regression asserting that a matching-SID destination login loses its extra impersonation grant while unrelated principals are preserved.
Independently checked against Microsoft's server-permission enumerator and object-permission enumerator. An exact-helper recording probe also confirms zero calls to object-permission enumeration; this probe was not a live SQL integration run.
There was a problem hiding this comment.
Confirmed, the same fix as in #10801: 19a92e973 is cherry-picked here as e81ea4b47. A login on a secondary loses the permissions on logins and server roles that it does not have on the primary, IMPERSONATE included, with the LoginPermissions step and without it.
Permissions on endpoints and availability groups stay on purpose, and here that matters most: when the replicas run under different service accounts, a secondary grants CONNECT on its endpoint to the service account of the primary, which has no such permission on the primary. Revoking it would stop the data movement to that secondary. The reply on #10801 has the details and the regression test.
This text was created by Claude and reviewed by Andreas Jordan.
| $grantWithGrant = $false | ||
| } | ||
|
|
||
| $DestServer.PSObject.Methods["Revoke"].Invoke($permSet, $newLoginName, $false, $grantWithGrant) |
There was a problem hiding this comment.
[P1] Preserve the permission when downgrading GrantWithGrant to Grant
A matching-SID login can have ordinary CONNECT SQL on the source and CONNECT SQL WITH GRANT OPTION on the destination. The grant loop first applies ordinary GRANT, which does not remove an existing grant option. This comparison then sees destination GrantWithGrant versus source Grant and reaches this call with revokeGrant = false, cascade = true (verified against the actual SMO overload). That revokes CONNECT SQL itself, and this helper never grants it again. With no role providing alternative CONNECT SQL, the new in-place path leaves the login unable to connect even though the source login still can. The same problem removes other desired permissions, such as VIEW SERVER STATE.
Please explicitly handle the grant-option downgrade while retaining the source permission, or perform reconciliation/revocation before applying the final desired grants. Add a real SQL regression that starts with destination WITH GRANT OPTION, expects final state GRANT and verifies continued login connectivity.
I traced the exact current helper: Grant CONNECT SQL (false) is followed by Revoke CONNECT SQL (revokeGrant=false, cascade=true). This was a recording-boundary probe, not live SQL. Microsoft's REVOKE server permissions documentation distinguishes revoking the permission from GRANT OPTION FOR; the existing tests do not cover this transition.
In this stacked PR, Sync-DbaAvailabilityGroup -UpdateExistingLogin reaches the same helper. Excluding the separate LoginPermissions step (as the new logins-only test does) leaves the permission missing; the default later permission pass can mask it by granting it again.
There was a problem hiding this comment.
Confirmed, the same fixes as in #10801: 5f8f777ef (only the grant option is revoked when the primary has the permission without it) and f69a33f00 (a DENY of a permission the login has with grant option runs with CASCADE) are cherry-picked here as 644d67cda and 6295754a7. With -UpdateExistingLogin and the LoginPermissions step excluded, a login on a secondary now keeps CONNECT SQL and the like, so it no longer depends on that step to get them back. The regression test is the new context in Copy-DbaLogin.Tests.ps1 that the reply on #10801 describes.
Lab: Sync-DbaAvailabilityGroup.Tests.ps1 12 of 12 on Windows PowerShell 5.1 and on PowerShell 7.6.
This text was created by Claude and reviewed by Andreas Jordan.
… has the server permission without it A login updated in place can have a server permission WITH GRANT OPTION on the destination and without it on the source. The GRANT of the source permission leaves the grant option in place, and the sync of Update-SqlPermission then found no source permission with the same state and revoked the permission itself with CASCADE. It never granted it again, so the login lost CONNECT SQL, or VIEW SERVER STATE and the like, which the source login still has. When the source login has the permission without the grant option, only the grant option is now revoked (REVOKE GRANT OPTION FOR ... CASCADE). Update-SqlPermission is shared: Copy-DbaLogin calls it for created and updated logins, and Sync-DbaLoginPermission for every existing login, which had the same defect. The new context gives the destination login CONNECT SQL and VIEW SERVER STATE with grant option, checks that both stay granted without it, and that the login can still connect. Part of #10772 (do Copy-DbaLogin, Sync-DbaLoginPermission) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ith grant option When the source login is denied a server permission that the existing destination login has WITH GRANT OPTION, the DENY of Update-SqlPermission failed with "To revoke or deny grantable privileges, specify the CASCADE option." and a warning. The sync then revoked the permission with CASCADE, so the login ended with neither the grant nor the deny. The DENY now runs with CASCADE, which SQL Server also accepts for a login without the permission. Found while fixing the grant option finding of the review. Update-SqlPermission is shared: Copy-DbaLogin calls it for created and updated logins, and Sync-DbaLoginPermission for every existing login, which had the same defect. The grant option context now also denies ALTER TRACE on the source and grants it with grant option on the destination, and checks that it ends as DENY without a warning. Part of #10772 (do Copy-DbaLogin, Sync-DbaLoginPermission) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t the source login does not have Update-SqlPermission synced the permissions on the server only: Server.EnumServerPermissions returns class 100 and nothing else. A login updated in place therefore kept a permission like IMPERSONATE ON LOGIN::sa or CONTROL ON SERVER ROLE that only the destination login had, while -Force drops the login and the permission with it. The permissions of the login on logins and server roles (Server.EnumObjectPermissions, class 101) are now compared with those of the source login by securable and permission, and those the source login does not have are revoked. As for the server permissions, only the grant option goes when the source login has the permission without it. A created login gets none of these permissions from the source, so nothing is granted. Permissions on endpoints and availability groups stay: they belong to the replica, and a secondary grants CONNECT on its endpoint to the service account of the primary, which has no such permission on the primary. The help of Copy-DbaLogin -UpdateExisting and of Sync-DbaLoginPermission says so. Update-SqlPermission is shared: Copy-DbaLogin calls it for created and updated logins, and Sync-DbaLoginPermission for every existing login, which now revokes these permissions as well. The new context gives the destination login IMPERSONATE on another login, CONTROL on a server role with grant option and CONNECT on an endpoint, and gives a second login IMPERSONATE on the same login. Part of #10772 (do Copy-DbaLogin, Sync-DbaLoginPermission) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #10772
Depends on #10801 (
Copy-DbaLogin -UpdateExisting). This branch is stacked on it, so the diff shows the commits of #10801 until that PR is merged. Only the commit "Sync-DbaAvailabilityGroup - Add -UpdateExistingLogin pass-through to Copy-DbaLogin" belongs to this PR. The ten commits #10801 got after its reviews were added on top of it. Chrissy rebased both branches ontodevelopmenton November 11, so the SHAs in the paragraphs of "After review" before the last one are from before that rebase; the commits kept their titles.Problem
The Logins step of
Sync-DbaAvailabilityGroupcallsCopy-DbaLogin -Force:$Forceand nothing else. A login that already exists on a secondary is therefore either skipped, so a password changed on the primary never reaches the secondaries, or, with-Force, dropped and recreated on every run after its databases and jobs were given tosa.Change
New switch
-UpdateExistingLogin, passed through as-UpdateExistingtoCopy-DbaLoginfor the Logins step only. Logins whose password hash, password policy, default database, default language or disabled state differs from the primary are updated in place. Identical ones are left alone, and SIDs, ownerships and open sessions on the secondaries are kept. With-Forceas well, only logins with a different SID are dropped and recreated, while-Forcekeeps dropping and recreating all other object types.The name follows
-UseJobLastModifiedfrom #10768, so the parameter list shows that it affects the Logins step only. TheCopy-DbaLogincall moved into a splat, because it now has six parameters. Without the new switch nothing changes.Tests
UpdateExistingLoginwas added to the parameter validation test.-UseJobLastModifiedcontext. A login with the same SID but another password on the secondary gets the primary's hash, and itsprincipal_idstays the same, so it was not dropped. Only the Logins step runs.The file passes in the lab under Windows PowerShell 5.1: 11 tests, 11 passed, also after the two review fixes of #10801 were added.
After review
Direct server permissions when the source login has none (3e19482). The review of this head found the defect of the shared
Update-SqlPermissionthat the latest review of #10801 found: for a source login without any direct server permission, the direct permissions of the destination login were never compared, so a login updated in place kept a directCONTROL SERVER, and the LoginPermissions step, which calls the same helper, did not take it away either. The fix of #10801 (b3c7709) is cherry-picked here; its new context is inCopy-DbaLogin.Tests.ps1, see #10801.Sync-DbaAvailabilityGroup.Tests.ps111 of 11 on Windows PowerShell 5.1 and on PowerShell 7.6.A password hash that cannot be read (4500376). The review of this head found the destructive path of the check before the drop that the latest review of #10801 found: when the password hash of the source login cannot be read,
-Force, and-UpdateExistingLogin -Forcefor a login with a different SID on a secondary, dropped the login, and Copy-DbaLogin then could not create it again. The fix of #10801 (0177c40) is cherry-picked here, and so is 2c3bfc3 as ffab219 (the skip for a destination older than SQL Server 2005 is gone, Connect-DbaInstance refuses those since #10813). The new context is inCopy-DbaLogin.Tests.ps1, see #10801.Sync-DbaAvailabilityGroup.Tests.ps111 of 11 on Windows PowerShell 5.1 and on PowerShell 7.6.The grant option, DENY and permissions on logins and server roles (644d67c, 6295754, e81ea4b). The review of
523707030found the two defects of the sharedUpdate-SqlPermissionthat the latest review of #10801 found, here reachable on every secondary with-UpdateExistingLogin: a permission with grant option on the secondary and without it on the primary was revoked completely,CONNECT SQLincluded, and a permission likeIMPERSONATEon a login that only the secondary login had survived. The fixes of #10801 (5f8f777, f69a33f, 19a92e9) are cherry-picked here, including theDENY ... CASCADEfound on the way. Permissions on endpoints and availability groups stay, because a secondary grantsCONNECTon its endpoint to the service account of the primary. The new contexts are inCopy-DbaLogin.Tests.ps1, see #10801.Sync-DbaAvailabilityGroup.Tests.ps112 of 12 on Windows PowerShell 5.1 and on PowerShell 7.6.🤖 Generated with Claude Code