Skip to content

Computer: Remove deletion of AD computer object when using PasswordPass and UnsecuredJoin options or JoinReadOnly option #447

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Computer: Remove deletion of AD computer object when using PasswordPass and UnsecuredJoin options or JoinReadOnly option #447

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fad75ab
Remove deletion of AD computer object when using PasswordPass and Uns…
webalexeu May 16, 2025
9004dff
Merge branch 'main' into fix/computer_object
webalexeu May 16, 2025
d15afe1
Improve the check and add JoinReadOnly option
webalexeu Jun 16, 2025
861b24a
Fix syntax
webalexeu Jun 16, 2025
1cf12f8
Typo
webalexeu Sep 3, 2025
0db4bd1
Merge branch 'main' into fix/computer_object
webalexeu Sep 3, 2025
f1630a9
Merge branch 'main' into fix/computer_object
johlju Sep 4, 2025
643f5fb
Manage condition using variable
webalexeu Sep 8, 2025
06fe4cb
Merge branch 'main' into fix/computer_object
johlju Oct 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- Computer
- Fix Get-ComputerDomain function to retrieve the computer NETBIOS domain name instead of the user.
- Remove deletion of computer object when PasswordPass and UnsecuredJoin options are used in DSC_Computer.
Fixes [Issue #446](https://github.com/dsccommunity/ComputerManagementDsc/issues/446).
Comment on lines +21 to +22
Copy link

@coderabbitai coderabbitai bot Sep 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Include JoinReadOnly and wrap long line to satisfy MD013.
The code change also covers JoinReadOnly; reflect that in the changelog. Also wrap to ≤80 chars.

-  - Remove deletion of computer object when PasswordPass and UnsecuredJoin options are used in DSC_Computer.
+  - Remove deletion of computer object when PasswordPass and UnsecuredJoin
+    options, or JoinReadOnly option, are used in DSC_Computer.
   Fixes [Issue #446](https://github.com/dsccommunity/ComputerManagementDsc/issues/446).
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- Remove deletion of computer object when PasswordPass and UnsecuredJoin options are used in DSC_Computer.
Fixes [Issue #446](https://github.com/dsccommunity/ComputerManagementDsc/issues/446).
- Remove deletion of computer object when PasswordPass and UnsecuredJoin
options, or JoinReadOnly option, are used in DSC_Computer.
Fixes [Issue #446](https://github.com/dsccommunity/ComputerManagementDsc/issues/446).
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

17-17: Line length
Expected: 80; Actual: 108

(MD013, line-length)

🤖 Prompt for AI Agents 
In CHANGELOG.md around lines 17 to 18, update the entry to mention that the
removal of deletion of the computer object also applies when JoinReadOnly is
used (not just PasswordPass and UnsecuredJoin), and reflow the line so it is
wrapped at or under 80 characters per line to satisfy MD013; keep the same
wording and link to Issue #446 but split into multiple lines as needed for the
80-character limit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webalexeu can you resolve this?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather see this resolved at the same time as a test is commited.

- `UserAccountControl`
- Tests remove parameter type on `Assert-BoundParameter` mocks.

Expand Down
21 changes: 17 additions & 4 deletions source/DSCResources/DSC_Computer/DSC_Computer.psm1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,13 +262,26 @@ function Set-TargetResource
$addComputerParameters.Add("Server", $Server)
}

# Check for existing computer objecst using ADSI without ActiveDirectory module
# Check for existing computer object using ADSI without ActiveDirectory module
$computerObject = Get-ADSIComputer -Name $Name -DomainName $DomainName -Credential $Credential

if ($computerObject)
{
Remove-ADSIObject -Path $computerObject.Path -Credential $Credential
Write-Verbose -Message ($script:localizedData.DeletedExistingComputerObject -f $Name, $computerObject.Path)
# UnsecuredJoin and PasswordPass options or JoinReadOnly option uses an existing machine account
# to join the computer to the domain and should not be deleted before.
if ( -not (($Options -contains 'PasswordPass' -and $options -contains 'UnsecuredJoin') -or ($Options -contains 'JoinReadOnly')))
{
Remove-ADSIObject -Path $computerObject.Path -Credential $Credential
Write-Verbose -Message ($script:localizedData.DeletedExistingComputerObject -f $Name, $computerObject.Path)
}
}
else
{
# Check if the computer object exists in the domain when using UnsecuredJoin and PasswordPass options or JoinReadOnly option
if (($Options -contains 'PasswordPass' -and $options -contains 'UnsecuredJoin') -or ($Options -contains 'JoinReadOnly'))
{
$errorMessage = $script:localizedData.ComputerObjectNotFound -f $Name,$DomainName
New-ObjectNotFoundException -Message $errorMessage
}
}
Comment on lines 265 to 285
Copy link

@coderabbitai coderabbitai bot Sep 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Add unit tests for both branches (found vs. not found) under the new options.
Cover: 1) no deletion when object exists and options require pre-created account; 2) ObjectNotFound exception when missing.

🏁 Script executed:

#!/bin/bash
# Locate tests covering the new behaviors
rg -nC2 -g 'tests/**' -e "ComputerObjectNotFound|JoinReadOnly|UnsecuredJoin|PasswordPass"

# Ensure tests assert deletion behavior paths as expected
rg -nC3 -g 'tests/**' -e "DeletedExistingComputerObject|Remove-ADSIObject"

Length of output: 12903

Add missing unit tests for ADSI deletion logic under the new options.

  • Stub Get-ADSIComputer to return a non-null object and assert Remove-ADSIObject is not invoked when Options contains PasswordPass & UnsecuredJoin or JoinReadOnly.
  • Stub Get-ADSIComputer to return $null and assert New-ObjectNotFoundException is thrown for those same option combinations.
🤖 Prompt for AI Agents 
In source/DSCResources/DSC_Computer/DSC_Computer.psm1 around lines 265-285, add
unit tests that cover the ADSI deletion logic for the PasswordPass+UnsecuredJoin
and JoinReadOnly option combinations: create one test that stubs
Get-ADSIComputer to return a non-null object and asserts Remove-ADSIObject is
NOT invoked when Options contains both 'PasswordPass' and 'UnsecuredJoin' and
another test that stubs Get-ADSIComputer to return $null and asserts
New-ObjectNotFoundException is thrown for the same option combinations (repeat
for the 'JoinReadOnly' scenario); ensure the tests set Options appropriately,
inject a stubbed Credential/DomainName if required, and verify behavior with
appropriate mocks/spies for Remove-ADSIObject and the exception helper.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webalexeu can you add a unit test to handle the new code path that throws. I tried to get the AI to add it but that failed big time. 😄

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a unit test to cover the new code paths.


if (-not [System.String]::IsNullOrEmpty($Options))
Expand Down
1 change: 1 addition & 0 deletions source/DSCResources/DSC_Computer/en-US/DSC_Computer.strings.psd1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ ConvertFrom-StringData @'
DomainNameAndWorkgroupNameError = Only DomainName or WorkGroupName can be specified at once.
ComputerNotInDomainMessage = This machine is not a domain member.
DeletedExistingComputerObject = Deleted existing computer object with name '{0}' at path '{1}'.
ComputerObjectNotFound = Computer object with name '{0}' not found in domain '{1}'.
InvalidOptionPasswordPassUnsecuredJoin = Domain Join option 'PasswordPass' may not be specified if 'UnsecuredJoin' is specified.
InvalidOptionCredentialUnsecuredJoinNullUsername = 'Credential' username must be null if 'UnsecuredJoin' is specified.
'@