BED-6441 - Adding back Aces to Domain Object Collection #172
Conversation
WalkthroughThe Domain object processing now assigns the computed ACE array to the Domain’s Aces property. After collecting ACEs from _aclProcessor.ProcessACL(...).ToArrayAsync(...), the result is stored in ret.Aces. No other control-flow or flag computations were changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant OP as ObjectProcessor
participant ACL as ACLProcessor
participant D as Domain
OP->>ACL: ProcessACL(domainDn, cancellationToken)
ACL-->>OP: IAsyncEnumerable<ACE>
OP->>OP: ToArrayAsync(...) → aces[]
OP->>D: set Aces = aces[]
note right of D: Existing flag computations remain unchanged\n(e.g., owner rights, inheritance, protection)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
🧹 Nitpick comments (1)
src/Runtime/ObjectProcessors.cs (1)
493-501: Nit: keep ACL block ordering consistent with other processorsEverywhere else (User/Computer/Group/OU/GPO/Container/CA types) we add the owner-rights booleans first, then assign
ret.Aces. Consider reordering for consistency.var aces = await _aclProcessor.ProcessACL(resolvedSearchResult, entry, true) .ToArrayAsync(cancellationToken: _cancellationToken); -ret.Aces = aces; ret.Properties.Add("doesanyacegrantownerrights", aces.Any(ace => ace.IsPermissionForOwnerRightsSid)); ret.Properties.Add("doesanyinheritedacegrantownerrights", aces.Any(ace => ace.IsInheritedPermissionForOwnerRightsSid)); +ret.Aces = aces; ret.IsACLProtected = _aclProcessor.IsACLProtected(entry); ret.Properties.Add("isaclprotected", ret.IsACLProtected); ret.InheritanceHashes = _aclProcessor.GetInheritedAceHashes(entry, resolvedSearchResult).ToArray();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Runtime/ObjectProcessors.cs(1 hunks)
🔇 Additional comments (1)
src/Runtime/ObjectProcessors.cs (1)
492-501: Domain.Aces assignment restored — good fixret.Aces is set for Domain in src/Runtime/ObjectProcessors.cs (~line 495), matching other processors. Confirm the Domain type (likely in SharpHoundCommonLib.OutputTypes) declares an Aces property and that it is included in JSON serialization (check JsonDataWriter.cs and JsonExtensions.cs).
Description
A small fix to add back some logic that was accidentally removed.
Motivation and Context
Resolves BED-6441
How Has This Been Tested?
This has been tested by running collection with the current version, and a build created using this change. The Aces were then collected for domain objects
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit