SqlLogin: add sid parameter#2453
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Sid property/parameter to the SqlLogin DSC resource: schema, Get/Set/Test-TargetResource signatures and logic, localization string, unit tests, example, and SMO test stub to support SID propagation and verification. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as DSC Config
participant Local as Local DSC Engine
participant Resource as SqlLogin Resource
participant SMO as SMO (stub / API)
participant SQL as SQL Server
Config->>Local: Apply configuration (includes Sid)
Local->>Resource: Invoke Set-TargetResource(Name, Sid, ...)
Resource->>SMO: Connect / find existing Login(Name)
alt Login not present
Resource->>SMO: Create Login(Name) with parsed Sid (byte[])
else Login present
Resource->>SMO: Update Login.Sid if Sid provided
end
SMO->>SQL: Execute create/update with supplied SID
SQL-->>SMO: Confirm creation/update
Resource->>SMO: Query Login to verify Sid
SMO-->>Resource: Return Login.Sid (byte[])
Resource->>Local: Return success/failure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1 (1)
447-513:⚠️ Potential issue | 🟡 MinorMissing
.PARAMETER SidinTest-TargetResourcecomment-based help.The
Sidparameter was added toTest-TargetResource(line 511-513) but the comment-based help block (lines 406-446) does not include a.PARAMETER Sidentry.Set-TargetResourcehas it (line 129), so this is an oversight. As per coding guidelines: "Always add comment-based help to all functions and scripts with SYNOPSIS, DESCRIPTION, PARAMETER, and EXAMPLE sections."Proposed fix — add after the Language parameter doc
.PARAMETER Language Specifies the default language for the login. + + .PARAMETER Sid + Specifies the login Sid. #>
🤖 Fix all issues with AI agents
In `@source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1`:
- Around line 348-351: Test-TargetResource detects SID mismatches but
Set-TargetResource only assigns $login.Sid during creation (in the branch that
sets $login.Sid), causing an endless non-convergence for existing logins; update
Set-TargetResource and/or Test-TargetResource to handle this: either (A) in
Set-TargetResource when $PSBoundParameters.ContainsKey('Sid') and the login
already exists, emit a clear warning (via Write-Verbose/Write-Warning or
Write-Error as appropriate) stating SQL Server cannot change a SID on an
existing login and that manual recreate is required (reference:
Set-TargetResource and $login.Sid assignment), or (B) remove/skip SID comparison
for existing logins inside Test-TargetResource so SID mismatches do not force a
false negative for existing accounts (reference: Test-TargetResource SID check);
choose one approach and implement consistent logging/messages so users
understand no automatic SID change will occur.
- Around line 587-597: Rename the local variable $InfoSid to camelCase $infoSid
and add a null-check for $loginInfo.Sid before calling
[System.BitConverter]::ToString to avoid a null-reference; specifically in the
DSC_SqlLogin.psm1 block that compares Sid (inside the Ensure Present branch)
first verify if ($loginInfo.Sid -ne $null) then compute $infoSid = '0x' +
[System.BitConverter]::ToString($loginInfo.Sid).Replace('-', '') and perform the
comparison/Write-Verbose/$testPassed = $false as currently done, otherwise
handle the null case (e.g., Write-Verbose indicating missing Sid and set
$testPassed = $false) so no BitConverter call is made on $null.
In `@source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.schema.mof`:
- Line 18: Update the Description for the Sid property in
DSC_SqlLogin.schema.mof to use the uppercase acronym "SID", state that it
applies only to SQL Logins, and indicate the expected format (e.g., standard SID
string like "S-1-5-..." or a hexadecimal/binary representation) similar in style
to other properties such as LoginMustChangePassword; modify the Description
attribute on the Sid property (the "String Sid;" declaration) to reflect this
improved, explicit text.
In `@tests/Unit/DSC_SqlLogin.Tests.ps1`:
- Around line 1044-1062: In the Set-TargetResource test block, replace the
incorrect $mockTestTargetResourceParameters reference with
$mockSetTargetResourceParameters when constructing the PSCredential (used in
Set-TargetResource invocation) so the credential uses the correct Name; ensure
the same corrected variable is used consistently in the test scope (the
assertion that New-SQLServerLogin was invoked should continue to compare
$login.Sid against the byte[] representation of the hex literal but keep the
credential argument list as @($mockSetTargetResourceParameters.Name,
$mockPassword)).
🧹 Nitpick comments (2)
CHANGELOG.md (1)
23-24: Align Markdown formatting with repo guidelines (resource + parameter + casing + wrap).Please format the resource name in italics, the parameter in bold, and capitalize SID. Also wrap the line if it exceeds 80 chars.
Suggested tweak
-- SqlLogin - - Added parameter `Sid` to allow setting the sid of the new login. ([issue `#1470`](https://github.com/dsccommunity/SqlServerDsc/issues/1470)) +- _SqlLogin_ + - Added parameter **Sid** to allow setting the SID of the new login. + ([issue `#1470`](https://github.com/dsccommunity/SqlServerDsc/issues/1470))As per coding guidelines: “Format parameters using bold in Markdown documentation” and “Format resource/module/product names using italic in Markdown documentation.”
tests/Unit/DSC_SqlLogin.Tests.ps1 (1)
1059-1061: ComplexCompare-Objectassertion in-ParameterFilteris hard to follow.The SID comparison inside the
ParameterFilterworks but is dense. Consider extracting the expected byte array and using a simpler comparison, or at minimum adding a brief inline comment.Also,
$login.Sidon line 1060 uses lowercase$login— while PowerShell is case-insensitive, the parameter name is$Login(PascalCase) per the function signature.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1 (1)
444-446:⚠️ Potential issue | 🟡 MinorMissing
.PARAMETER SidinTest-TargetResourcecomment-based help.The
Sidparameter was added toTest-TargetResource(line 511–513) but the comment-based help block does not document it.Set-TargetResourcealready has the corresponding.PARAMETER Sidentry. As per coding guidelines, all parameters must be documented in comment-based help.Proposed fix — add after `.PARAMETER Language`
.PARAMETER Language Specifies the default language for the login. + + .PARAMETER Sid + Specifies the security identifier (SID) for the login. Only applies + to SQL Logins. #>
🧹 Nitpick comments (2)
source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1 (2)
128-131: Improve.PARAMETER Siddescription to match the MOF schema.The MOF description uses "security identifier (SID)" with format guidance and SQL Login applicability. The help text here just says "Specifies the login Sid." — keep them consistent.
Suggested improvement
.PARAMETER Sid - Specifies the login Sid. + Specifies the security identifier (SID) for the login. Only applies + to SQL Logins. The value should be a hexadecimal string (e.g. + '0x1234...').
348-351: Consider adding input validation for theSidhex format.The hex-to-byte parsing on line 350 will produce a cryptic error if
$Sidis not a well-formed hex string (e.g., odd length, non-hex characters). AValidatePatternon the parameter or an early guard with a localized error message would improve the user experience.Example: add ValidatePattern to the parameter declaration
[Parameter()] + [ValidatePattern('^0x([0-9A-Fa-f]{2})+$')] [System.String] $SidApply the same to
Test-TargetResource's$Sidparameter as well.
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
Can we also an an example how to provide the sid? Either in an existing example file or preferably a new one.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2453 +/- ##
=====================================
Coverage 94% 94%
=====================================
Files 227 227
Lines 10801 10804 +3
=====================================
+ Hits 10172 10175 +3
Misses 629 629
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@source/Examples/Resources/SqlLogin/1-AddSqlLogin.ps1`:
- Around line 1-5: Update the .DESCRIPTION block to accurately describe the
example: remove references to the Windows user 'CONTOSO\WindowsUser' and Windows
group 'CONTOSO\WindowsGroup' and instead state that the script demonstrates
creating two SqlLogin resources (one with an explicit SID) — reference the
SqlLogin resources in the file and mention the explicit SID usage so the
description matches the example content.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Unit/DSC_SqlLogin.Tests.ps1 (1)
522-538:⚠️ Potential issue | 🟡 MinorSid test case reuses a mock not designed for it — the returned hashtable has no
Sidkey.The
Get-TargetResourcemock (lines 528–538) was built for the boolean propertiesLoginPasswordPolicyEnforced/LoginPasswordExpirationEnabledand flips them with-not $MockPropertyValue. When$MockPropertyValueis the Sid string,-not '0x…'evaluates to$false, which is semantically meaningless for those properties, and the mock never returns aSidkey at all.This means the test only proves that a missing Sid triggers
$false, not that a different Sid triggers$false. Consider a dedicatedContext(or addingSidto the mock return value with a different value) so the comparison is explicit.#!/bin/bash # Verify how Test-TargetResource handles Sid comparison in the production code ast-grep --pattern $'function Test-TargetResource { $$$ }' # Also search for Sid comparison logic rg -n -C5 'Sid' --type ps1 -g '!*Tests*' -g '*SqlLogin*'
🧹 Nitpick comments (1)
tests/Unit/DSC_SqlLogin.Tests.ps1 (1)
120-120: Inconsistent hex string parsing patterns for Sid.Line 120 starts with a raw hex string (
'B76150A66B38F64FAE9470091789AA66'), while line 205 starts with0x-prefixed hex ('0xB76150A66B38F64FAE9470091789AA66') and strips it first. Both yield the same byte array, but the inconsistency across test contexts reduces readability. Consider using the same pattern everywhere for clarity.Also applies to: 205-205
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/Examples/Resources/SqlLogin/1-AddWindowsUser.ps1 (1)
15-18:⚠️ Potential issue | 🟡 Minor
$LoginCredentialparameter is unused after removing the SqlLogin resource.The SqlLogin resource block was moved to
1-AddSqlLogin.ps1, but the$LoginCredentialparameter remains. None of the WindowsUser/WindowsGroup resources in this file reference it.Proposed fix — remove the unused parameter
param ( [Parameter(Mandatory = $true)] [System.Management.Automation.PSCredential] - $SqlAdministratorCredential, - - [Parameter(Mandatory = $true)] - [System.Management.Automation.PSCredential] - $LoginCredential + $SqlAdministratorCredential )
🤖 Fix all issues with AI agents
In `@source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1`:
- Around line 590-607: In Test-TargetResource, when $loginInfo.Sid is $null the
code currently just sets $testPassed = $false; add a Write-Verbose to report the
missing SID so users can diagnose failures. Specifically, in the $null branch
for $loginInfo.Sid emit a verbose message (reuse or create a localized string)
that includes $Name and the expected $Sid (or pass 'null' as the actual SID)
before setting $testPassed = $false so behavior matches other property mismatch
branches.
In `@source/Examples/Resources/SqlLogin/1-AddSqlLogin.ps1`:
- Around line 1-5: The .DESCRIPTION comment-based help block has incorrect
indentation for the description text (the line containing 'SqlLogin2' is
indented 7 spaces); update the description lines inside the .DESCRIPTION block
so the text lines use 8-space indentation per the guideline (adjust the line
with 'SqlLogin2' to match the other description lines), ensuring the
comment-based help keywords remain at 4 spaces and description text at 8 spaces.
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
@johlju partially reviewed 8 files and all commit messages, made 2 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on cai-n).
source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1 line 611 at r6 (raw file):
$testPassed = $false } }
Maybe we can do something like this to not duplicate Write-Verbose?
Suggestion:
$infoSid = if ($null -ne $loginInfo.Sid)
{
'0x' + [System.BitConverter]::ToString($loginInfo.Sid).Replace('-', '')
}
if ($null -eq $infoSid -or $infoSid -ne $Sid)
{
Write-Verbose -Message (
$script:localizedData.WrongSid -f $Name, $infoSid, $Sid
)
$testPassed = $false
}source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1 line 612 at r6 (raw file):
} } }
If we does this it will return $false when sid mismatch on an exist login, but then there is no logic in Set to actually set the correct sid on the login. So either we have to add logic to Set or not enforce Sid in Test.
Code quote:
if ( $PSBoundParameters.ContainsKey('Sid') )
{
if ($null -eq $loginInfo.Sid)
{
Write-Verbose -Message (
$script:localizedData.WrongSid -f $Name, $null, $Sid
)
$testPassed = $false
}
else
{
$infoSid = '0x' + [System.BitConverter]::ToString($loginInfo.Sid).Replace('-', '')
if ( $infoSid -ne $Sid )
{
Write-Verbose -Message (
$script:localizedData.WrongSid -f $Name, $infoSid, $Sid
)
$testPassed = $false
}
}
}
|
I didn't really know how to tackle the tests, Sid can only be set at the creation of the login, is it alright if I just remove the Sid part in Test-TargetResource ? |
|
Yes, it is okay if we can't enforce Sid without recreating the login. If another contributor need to enforce the Sid then that can be a future PR :) |
|
I was meaning to say that it is okay to remove the Test logic 🙂 |
johlju
added
the
waiting for code fix
|
I'm still confused haha, do you mean not set the $testPassed = $false but keep the Write-Verbose -Message ? |
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
I hope this is more clear 🙂
@johlju reviewed 3 files and all commit messages, made 4 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on cai-n).
source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1 line 449 at r8 (raw file):
.PARAMETER Sid Specifies the security identifier (SID) for the login. Only applies to SQL Logins. The value should be a hexadecimal string (e.g. '0x1234...').
Add this too the comment based help.
Suggestion:
.PARAMETER Sid
Specifies the security identifier (SID) for the login. Only applies to SQL Logins. The value should be a hexadecimal string (e.g. '0x1234...').
Not currently used in Test-TargetResource to enforce Sid.source/DSCResources/DSC_SqlLogin/DSC_SqlLogin.psm1 line 607 at r8 (raw file):
$testPassed = $false } }
We can remove this entire block since there is no logic in Set-TargetResource to re-create the login to actually be able to set a Sid that mismatch.
Also remove any logic from the unit tests for Test-TargetResource.
Code quote:
if ( $PSBoundParameters.ContainsKey('Sid') )
{
$infoSid = if ($null -ne $loginInfo.Sid)
{
'0x' + [System.BitConverter]::ToString($loginInfo.Sid).Replace('-', '')
}
if ($null -eq $infoSid -or $infoSid -ne $Sid)
{
Write-Verbose -Message (
$script:localizedData.WrongSid -f $Name, $infoSid, $Sid
)
$testPassed = $false
}
}source/DSCResources/DSC_SqlLogin/en-US/DSC_SqlLogin.strings.psd1 line 23 at r8 (raw file):
WrongDefaultDatabase = The login '{0}' has the default database '{1}', but expected it to have the default database '{2}'. WrongLanguage = The login '{0}' has the default language '{1}', but expected it to have the default language '{2}'. WrongSid = The login '{0}' has the Sid '{1}', but expected it to have the Sid '{2}'.
We can remove this as we are about to remove the logic in Test-TargetResource.
Code quote:
WrongSid = The login '{0}' has the Sid '{1}', but expected it to have the Sid '{2}'.
|
Added a new issue #2455 to track the enhancement to be able to recreate a login if sid mismatch. 🙂 |
johlju
left a comment
johlju
left a comment
There was a problem hiding this comment.
@johlju reviewed 2 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on cai-n).
|
Will merge once all tests passes. There might be intermittent errors, so I will kick off any failing integration tests later. |
johlju
removed
the
waiting for code fix
Thank you, I might also take that issue since I'm already looking at sid stuff if it's alright |
2 participants
Pull Request (PR) description
Add the parameter Sid to SqlLogin to allow setting the sid for newly created login of type SqlLogin.
This Pull Request (PR) fixes the following issues
Task list
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
This change is