Add computer name targeting to AD domain state - #2002
Open
soulemike wants to merge 3 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthrough
ChangesTargeted DNS collection
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@powershell/public/Get-MtADDomainState.ps1`:
- Line 75: Update the resolved computer name and DACL binding logic around
$resolvedComputerName so omitting -ComputerName preserves the serverless
LDAP://$domainDN path. Only construct an explicit LDAP://$ComputerName/$domainDN
DirectoryEntry with AuthenticationTypes.ServerBind when -ComputerName is
supplied, and apply the same adjustment to the corresponding logic around the
referenced additional locations.
- Around line 15-17: Update the ComputerName documentation in
Get-MtADDomainState to state that it must identify an Active Directory domain
controller or AD DS server, not a DNS-only server. Clarify that DNS collection
uses the same target only when it also supports the required DNS management
access, or introduce separate AD and DNS target parameters if the implementation
supports that scope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 05d0ce35-7a80-481b-98fc-22a15458718a
📒 Files selected for processing (1)
powershell/public/Get-MtADDomainState.ps1
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@powershell/public/Get-MtADDomainState.ps1`:
- Around line 70-72: Update Get-MtADDomainState to resolve the explicit
ComputerName before AD data collection and pass it to all supported AD cmdlets,
not only the Get-DnsServer* calls; ensure the LDAP searcher also targets
ComputerName when provided while retaining serverless behavior when it is
absent. Apply the related changes at powershell/public/Get-MtADDomainState.ps1
lines 56-66, 70-72, and 211, using the existing domainState collection flow and
ComputerName handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b6957cb-d5ee-40dc-83d8-959fdb15ea47
📒 Files selected for processing (1)
powershell/public/Get-MtADDomainState.ps1
Comment on lines
+70
to
+72
| if (-not $ComputerName) { | ||
| $ComputerName = $domainState.Domain.DNSRoot | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg 'Get-MtADDomainState\.ps1$' || true
echo "== relevant file around lines 1-130 =="
if [ -f powershell/public/Get-MtADDomainState.ps1 ]; then
sed -n '1,130p' powershell/public/Get-MtADDomainState.ps1 | cat -n
fi
echo "== relevant file around lines 180-230 =="
if [ -f powershell/public/Get-MtADDomainState.ps1 ]; then
sed -n '180,230p' powershell/public/Get-MtADDomainState.ps1 | cat -n
fi
echo "== full file metadata =="
wc -l powershell/public/Get-MtADDomainState.ps1
echo "== ComputerName usages =="
rg -n '\bComputerName\b|\b-\s*Server\b|Get-AD|LDAP://' powershell/public/Get-MtADDomainState.ps1Repository: maester365/maester
Length of output: 14440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p=Path('powershell/public/Get-MtADDomainState.ps1')
text=p.read_text()
print('has ComputerName param:', 'param' in text and '$ComputerName' in text)
print('computer name parameter block vicinity:', '$ComputerName' in text)
for line in enumerate(text.splitlines(), 1):
if 'ComputerName' in line[1] or 'Get-AD' in line[1] or 'SearchRoot' in line[1]:
print(f'{line[0]}: {line[1]}')
PY
# Read-only behavioral/static verifier: identify every Get-AD* and LDAP SearchRoot call
# and whether a local ComputerName variable is interpolated into its common target-oriented parameter.
import re, subprocess
from pathlib import Path
text = Path('powershell/public/Get-MtADDomainState.ps1').read_text()
lines = text.splitlines()
get_ad_calls = []
ldap_calls = []
for i,l in enumerate(lines,1):
if re.search(r'Get-AD\w+', l):
get_ad_calls.append((i,l.strip()))
if 'SearchRoot = [ADSI]' in l or 'LDAP://' in l:
ldap_calls.append((i,l.strip()))
print('== Get-AD calls ==')
for i,l in get_ad_calls: print(f'{i}: {l}')
print('== LDAP SearchRoot calls ==')
for i,l in ldap_calls: print(f'{i}: {l}')
print('== static probe: ComputerName passed to cmdlet/common target-like parameter near Get-AD calls? ==')
# Windows PowerShell Get-AD* cmdlets accept -Server as common parameter or per-cmdlet Server;
# check if any sampled call line contains "-ComputerName" or "ComputerName".
for class_ in ['Domain','Forest','Computer','User','Group','ServiceAccount','DomainController','ReplicationSite','ReplicationSubnet','RootDSE','OptionalFeature']:
found=[x for x in get_ad_calls if 'Get-AD'+class_ in x[1]]
passed=[x for x in found if re.search(r'(?i)\b(Server|Computer|Target)\b', x[1])]
print('Get-AD'+class_+' count='+str(len(found))+' explicit target-like params='+str(len(passed)))
print('static probe done.')
PYRepository: maester365/maester
Length of output: 3789
Propagate ComputerName consistently beyond DNS.
ComputerName is currently only passed to Get-DnsServer*; resolve and apply the explicit target to the supported AD cmdlets before collection, and honor it for the LDAP searcher while preserving the implicit serverless behavior when a target is not provided.
📍 Affects 1 file
powershell/public/Get-MtADDomainState.ps1#L70-L72(this comment)powershell/public/Get-MtADDomainState.ps1#L56-L66powershell/public/Get-MtADDomainState.ps1#L211-L211
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@powershell/public/Get-MtADDomainState.ps1` around lines 70 - 72, Update
Get-MtADDomainState to resolve the explicit ComputerName before AD data
collection and pass it to all supported AD cmdlets, not only the Get-DnsServer*
calls; ensure the LDAP searcher also targets ComputerName when provided while
retaining serverless behavior when it is absent. Apply the related changes at
powershell/public/Get-MtADDomainState.ps1 lines 56-66, 70-72, and 211, using the
existing domainState collection flow and ComputerName handling.
SamErde
added
enhancement
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
📑 Description
Adds an optional param for the target of the caching commands with a default of the current domain. May need future mapping in other commands.
Allow callers to target a specific domain controller or DNS server when collecting AD domain state so sub-commands that require a host can be directed explicitly.
Preserve existing behavior by falling back to the collected domain object's DNS root when no computer name is supplied.
Ensure cached domain state is separated per target host to avoid overwriting or reusing results collected for a different server.
Description
Added an optional ComputerName parameter and updated the command help with examples describing its use.
Build @adServerParameters when -ComputerName is provided and splat it to supported Get-AD* calls so those queries target the specified AD Server.
Introduced resolvedComputerName = if ($ComputerName) { $ComputerName } else { $domainState.Domain.DNSRoot } and use it for DNS and LDAP sub-commands, including Get-DnsServerZone, Get-DnsServerResourceRecord, and the LDAP:// search root for DirectorySearcher.
Use a cache key that includes the ComputerName (DomainState:$ComputerName) when a target host is specified so targeted collections do not clash with the default cache.
Closes #1974
✅ Checks
/powershell/tests/pester.ps1locally.ℹ️ Additional Information
Summary by CodeRabbit
-ComputerNameparameter toGet-MtADDomainStateto collect domain and DNS state from a specified DNS server/domain controller.ComputerNameinstead of using the local DNS context.