Computer: Remove deletion of AD computer object when using PasswordPass and UnsecuredJoin options or JoinReadOnly option #447
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ecuredJoin options
webalexeu
changed the title
WalkthroughThe changes modify the DSC_Computer resource to conditionally preserve pre-created Active Directory computer objects when using specific options (PasswordPass and UnsecuredJoin or JoinReadOnly), preventing deletion during domain join operations that expect the object to already exist. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as DSC Configuration
participant Resource as DSC_Computer<br/>(Set-TargetResource)
participant AD as Active Directory
participant Error as Error Handler
Caller->>Resource: Invoke Set-TargetResource
Note over Resource: Detect precreated requirement<br/>($requiresPrecreated = JoinReadOnly OR<br/>(PasswordPass AND UnsecuredJoin))
Resource->>AD: Query for existing computer object
alt Object Exists
alt Precreation NOT Required
Resource->>AD: Delete computer object
Resource->>Caller: ✓ Proceed with join
else Precreation Required
Resource->>Caller: ✓ Object exists, continue
end
else Object Does NOT Exist
alt Precreation Required
Resource->>Error: Raise ObjectNotFoundException
Error->>Caller: ✗ Computer object not found error
else Precreation NOT Required
Resource->>Caller: ✓ Proceed (no deletion needed)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes introduce a new conditional control flow with multiple branches based on precreation detection and object existence, requiring careful verification that the option combinations and error conditions are correct. The logic is focused but depends on accurate interpretation of the PasswordPass, UnsecuredJoin, and JoinReadOnly options. Pre-merge checks✅ Passed checks (5 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (5)**⚙️ CodeRabbit configuration file
Files:
{**/*.ps1,**/*.psm1,**/*.psd1}⚙️ CodeRabbit configuration file
Files:
source/DSCResources/**/*.psm1⚙️ CodeRabbit configuration file
Files:
**/*.md⚙️ CodeRabbit configuration file
Files:
CHANGELOG.md⚙️ CodeRabbit configuration file
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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: 2
🧹 Nitpick comments (2)
source/DSCResources/DSC_Computer/DSC_Computer.psm1 (2)
271-271: Normalize variable casing for consistency ($Options vs $options).
PowerShell is case-insensitive, but consistent casing improves readability.- if ( -not (($Options -contains 'PasswordPass' -and $options -contains 'UnsecuredJoin') -or ($Options -contains 'JoinReadOnly'))) + if ( -not (($Options -contains 'PasswordPass' -and $Options -contains 'UnsecuredJoin') -or ($Options -contains 'JoinReadOnly'))) ... - if (($Options -contains 'PasswordPass' -and $options -contains 'UnsecuredJoin') -or ($Options -contains 'JoinReadOnly')) + if (($Options -contains 'PasswordPass' -and $Options -contains 'UnsecuredJoin') -or ($Options -contains 'JoinReadOnly'))Also applies to: 280-280
265-285: DRY the option checks and make intent explicit.
Compute the “uses pre-created account” condition once and reuse.# Check for existing computer object using ADSI without ActiveDirectory module $computerObject = Get-ADSIComputer -Name $Name -DomainName $DomainName -Credential $Credential + $usesPrecreatedAccount = ( + ($Options -contains 'PasswordPass' -and $Options -contains 'UnsecuredJoin') -or + ($Options -contains 'JoinReadOnly') + ) if ($computerObject) { - # 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'))) + # UnsecuredJoin+PasswordPass or JoinReadOnly use an existing machine account; do not delete it. + if (-not $usesPrecreatedAccount) { 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')) + # Require a pre-created computer object for UnsecuredJoin+PasswordPass or JoinReadOnly. + if ($usesPrecreatedAccount) { $errorMessage = $script:localizedData.ComputerObjectNotFound -f $Name,$DomainName New-ObjectNotFoundException -Message $errorMessage } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)source/DSCResources/DSC_Computer/DSC_Computer.psm1(1 hunks)source/DSCResources/DSC_Computer/en-US/DSC_Computer.strings.psd1(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**
⚙️ CodeRabbit configuration file
**: # DSC Community GuidelinesTerminology
- Command: Public command
- Function: Private function
- Resource: DSC class-based resource
Build & Test Workflow
- Run in PowerShell, from repository root
- Build before running tests:
.\build.ps1 -Tasks build- Always run tests in new PowerShell session:
Invoke-Pester -Path @({test paths}) -Output DetailedFile Organization
- Public commands:
source/Public/{CommandName}.ps1- Private functions:
source/Private/{FunctionName}.ps1- Unit tests:
tests/Unit/{Classes|Public|Private}/{Name}.Tests.ps1- Integration tests:
tests/Integration/Commands/{CommandName}.Integration.Tests.ps1Requirements
- Follow guidelines over existing code patterns
- Always update CHANGELOG.md Unreleased section
- Localize all strings using string keys; remove any orphaned string keys
- Check DscResource.Common before creating private functions
- Separate reusable logic into private functions
- Add unit tests for all commands/functions/resources
- Add integration tests for all public commands and resources
Files:
source/DSCResources/DSC_Computer/en-US/DSC_Computer.strings.psd1source/DSCResources/DSC_Computer/DSC_Computer.psm1CHANGELOG.md
**/*.ps?(m|d)1
⚙️ CodeRabbit configuration file
**/*.ps?(m|d)1: # PowerShell GuidelinesNaming
- Use descriptive names (3+ characters, no abbreviations)
- Functions: PascalCase with Verb-Noun format using approved verbs
- Parameters: PascalCase
- Variables: camelCase
- Keywords: lower-case
- Classes: PascalCase
- Include scope for script/global/environment variables:
$script:,$global:,$env:File naming
- Class files:
###.ClassName.ps1format (e.g.001.SqlReason.ps1,004.StartupParameters.ps1)Formatting
Indentation & Spacing
- Use 4 spaces (no tabs)
- One space around operators:
$a = 1 + 2- One space between type and variable:
[String] $name- One space between keyword and parenthesis:
if ($condition)- No spaces on empty lines
- Try to limit lines to 120 characters
Braces
- Newline before opening brace (except variable assignments)
- One newline after opening brace
- Two newlines after closing brace (one if followed by another brace or continuation)
Quotes
- Use single quotes unless variable expansion is needed:
'text'vs"text $variable"Arrays
- Single line:
@('one', 'two', 'three')- Multi-line: each element on separate line with proper indentation
- Do not use the unary comma operator (
,) in return statements to force
an arrayHashtables
- Empty:
@{}- Each property on separate line with proper indentation
- Properties: Use PascalCase
Comments
- Single line:
# Comment(capitalized, on own line)- Multi-line:
<# Comment #>format (opening and closing brackets on own line), and indent text- No commented-out code
Comment-based help
- Always add comment-based help to all functions and scripts
- Comment-based help: SYNOPSIS, DESCRIPTION (40+ chars), PARAMETER, EXAMPLE sections before function/class
- Comment-based help indentation: keywords 4 spaces, text 8 spaces
- Include examples for all parameter sets and combinations
- INPUTS: List each pipeline‑accepted type (one per line) with a 1‑line description.
- OUTPUTS: Lis...
Files:
source/DSCResources/DSC_Computer/en-US/DSC_Computer.strings.psd1source/DSCResources/DSC_Computer/DSC_Computer.psm1
source/DSCResources/**/*.psm1
⚙️ CodeRabbit configuration file
source/DSCResources/**/*.psm1: # MOF-based Desired State Configuration (DSC) Resources GuidelinesRequired Functions
- Every DSC resource must define:
Get-TargetResource,Set-TargetResource,Test-TargetResource- Export using
*-TargetResourcepatternFunction Return Types
Get-TargetResource: Must return hashtable with all resource propertiesTest-TargetResource: Must return boolean ($true/$false)Set-TargetResource: Must not return anything (void)Parameter Guidelines
Get-TargetResource: Only include parameters needed to retrieve actual current state valuesGet-TargetResource: Remove non-mandatory parameters that are never usedSet-TargetResourceandTest-TargetResource: Must have identical parametersSet-TargetResourceandTest-TargetResource: Unused mandatory parameters: Add "Not used in <function_name>" to help commentRequired Elements
- Each function must include
Write-Verboseat least once
Get-TargetResource: Use verbose message starting with "Getting the current state of..."Set-TargetResource: Use verbose message starting with "Setting the desired state of..."Test-TargetResource: Use verbose message starting with "Determining the current state of..."- Use localized strings for all messages (Write-Verbose, Write-Error, etc.)
- Import localized strings using
Get-LocalizedDataat module topError Handling
- Do not use
throwfor terminating errors- Use
try/catchblocks to handle exceptions- Throw localized exceptions using the appropriate
New-*Exceptioncmdlet:
New‑InvalidDataExceptionNew-ArgumentExceptionNew-InvalidOperationException- [
New-ObjectNotFoundException](https:/...
Files:
source/DSCResources/DSC_Computer/DSC_Computer.psm1
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: # Markdown Style Guidelines
- Wrap lines at word boundaries when over 80 characters (except tables/code blocks)
- Use 2 spaces for indentation
- Use '1.' for all items in ordered lists (1/1/1 numbering style)
- Disable
MD013rule by adding a comment for tables/code blocks exceeding 80 characters- Empty lines required before/after code blocks and headings (except before line 1)
- Escape backslashes in file paths only (not in code blocks)
- Code blocks must specify language identifiers
Text Formatting
- Parameters: bold
- Values/literals:
inline code- Resource/module/product names: italic
- Commands/files/paths:
inline code
Files:
CHANGELOG.md
CHANGELOG.md
⚙️ CodeRabbit configuration file
CHANGELOG.md: # Changelog Guidelines
- Always update the Unreleased section in CHANGELOG.md
- Use Keep a Changelog format
- Describe notable changes briefly, ≤2 items per change type
- Reference issues using format issue #<issue_number>
- No empty lines between list items in same section
- Do not add item if there are already an existing item for the same change
Files:
CHANGELOG.md
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
17-17: Line length
Expected: 80; Actual: 108
(MD013, line-length)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: dsccommunity.ComputerManagementDsc (Build Package Module)
🔇 Additional comments (2)
source/DSCResources/DSC_Computer/en-US/DSC_Computer.strings.psd1 (1)
20-20: LGTM: New localized error string added and consistent with existing keys.
Message text is clear and parameterized as required.source/DSCResources/DSC_Computer/DSC_Computer.psm1 (1)
265-276: LGTM: Correctly avoids deleting pre-created AD computer objects for specific join modes.
Logic aligns with Add-Computer semantics for PasswordPass+UnsecuredJoin and JoinReadOnly.
Comment on lines
+17
to
+18
| - 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). |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Rather see this resolved at the same time as a test is commited.
Comment on lines
265
to
285
| # 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 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-ADSIComputerto return a non-null object and assertRemove-ADSIObjectis not invoked whenOptionscontainsPasswordPass&UnsecuredJoinorJoinReadOnly. - Stub
Get-ADSIComputerto return$nulland assertNew-ObjectNotFoundExceptionis 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.
There was a problem hiding this comment.
@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. 😄
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
We need a unit test to cover the new code paths.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #447 +/- ##
===================================
- Coverage 86% 86% -1%
===================================
Files 21 21
Lines 2083 2088 +5
===================================
+ Hits 1805 1808 +3
- Misses 278 280 +2
🚀 New features to boost your workflow:
|
This comment was marked as resolved.
This comment was marked as resolved.
johlju
requested changes
Sep 4, 2025
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
johlju
added
the
waiting for code fix
johlju
requested changes
Oct 16, 2025
There was a problem hiding this comment.
@johlju reviewed 1 of 3 files at r1, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @webalexeu)
Comment on lines
+17
to
+18
| - 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). |
There was a problem hiding this comment.
Rather see this resolved at the same time as a test is commited.
Comment on lines
265
to
285
| # 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
We need a unit test to cover the new code paths.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request (PR) description
When using PasswordPass and UnsecuredJoin options or JoinReadOnly option means AD computer object has been pre-created within Active Directory prior to domain join and should not be deleted
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).
and comment-based help.
This change is