chore: Configure ProcessACL for async enumerable ACE collection #220
Conversation
WalkthroughThe changes introduce two new boolean properties to the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ACLProcessor
participant ACE
Caller->>ACLProcessor: ProcessACL(...)
loop For each access rule in security descriptor
ACLProcessor->>ACE: Create ACE with owner rights flags
ACLProcessor-->>Caller: yield ACE
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/CommonLib/OutputTypes/ACE.cs (1)
17-37: UpdateEqualsandGetHashCodemethods to include new properties.The new properties
IsPermissionForOwnerRightsSidandIsInheritedPermissionForOwnerRightsSidare not included in the equality comparison or hash code calculation. This could lead to incorrect behavior when comparing ACE objects or using them in collections.Apply this diff to include the new properties in equality and hash code calculations:
protected bool Equals(ACE other) { return PrincipalSID == other.PrincipalSID && PrincipalType == other.PrincipalType && - RightName == other.RightName && IsInherited == other.IsInherited; + RightName == other.RightName && IsInherited == other.IsInherited && + IsPermissionForOwnerRightsSid == other.IsPermissionForOwnerRightsSid && + IsInheritedPermissionForOwnerRightsSid == other.IsInheritedPermissionForOwnerRightsSid; } public override int GetHashCode() { unchecked { var hashCode = PrincipalSID != null ? PrincipalSID.GetHashCode() : 0; hashCode = (hashCode * 397) ^ (int)PrincipalType; hashCode = (hashCode * 397) ^ (RightName != null ? RightName.GetHashCode() : 0); hashCode = (hashCode * 397) ^ IsInherited.GetHashCode(); + hashCode = (hashCode * 397) ^ IsPermissionForOwnerRightsSid.GetHashCode(); + hashCode = (hashCode * 397) ^ IsInheritedPermissionForOwnerRightsSid.GetHashCode(); return hashCode; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/CommonLib/OutputTypes/ACE.cs(1 hunks)src/CommonLib/OutputTypes/OutputBase.cs(1 hunks)src/CommonLib/Processors/ACLProcessor.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
src/CommonLib/OutputTypes/OutputBase.cs (1)
12-12: Good architectural choice for supporting async streaming.Changing from
ACE[]toIEnumerable<ACE>provides better flexibility and aligns well with the async enumerable pattern introduced inACLProcessor. The initialization withArray.Empty<ACE>()is appropriate.src/CommonLib/Processors/ACLProcessor.cs (2)
223-239: Well-implemented async enumerable pattern.The conversion to
IAsyncEnumerable<ACE>is properly implemented with appropriate use ofAsyncEnumerable.Empty<ACE>()for empty results and proper forwarding to the core processing method.
301-342: Correct implementation of per-ACE owner rights tracking.The logic properly identifies Owner Rights SIDs (ending with "S-1-3-4") and tracks both direct and inherited permissions using the new ACE properties. This successfully replaces the previous tuple-based approach.
Description
Returning ProcessACL to return an IAsyncEnumerable for deferred collection processing as it had been before.
This may help resolves issues of memory spikes.
Additionally, moving the aggregate property
doesanyacegrantownerrightsfrom the parent domain object and attaching to the ACE instead asIsPermissionForOwnerRightsSidas this is more reflective of an ACE's properties than of the parent.Motivation and Context
Investigating a large memory spike https://bloodhoundhq.slack.com/archives/C20NG2L87/p1751305729336129
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Refactor