Bed 6619 collection of objectguid #253

hotnops · 2025-10-15T16:20:51Z

Description

Motivation and Context

How Has This Been Tested?

Locally in a hybrid lab with both Entra and AD devices. I ran SharpHound
in an AD environment and observed the ObjectGuid being populated in the
collect and eventually in the BH info panel. If a device does not have an ObjectGUID,
it will simply ignore and fail gracefully.

Screenshots (if appropriate):

Types of changes

[ x] Chore (a change that does not modify the application functionality)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

Documentation updates are needed, and have been made accordingly.
[ X] I have added and/or updated tests to cover my changes.
[ X] All new and existing tests passed.
My changes include a database migration.

Summary by CodeRabbit

New Features
- Computer objects now include their Object GUID in collected properties and outputs when available.
Bug Fixes
- Malformed or unexpected GUID data is safely ignored to prevent processing errors.
Tests
- Unit tests updated to validate presence, absence, normalization, and error handling for Object GUID data.

coderabbitai · 2025-10-15T16:21:02Z

Walkthrough

Adds ObjectGUID handling for computer objects: includes LDAPProperties.ObjectGUID in CommonProperties.ComputerMethodProps, reads and validates 16-byte objectguid in LdapPropertyProcessor.ReadComputerProperties, converts to a GUID string (uppercase) when valid, and updates unit tests to include and assert ObjectGUID.

Changes

Cohort / File(s)	Summary
LDAP common properties `src/CommonLib/LdapQueries/CommonProperties.cs`	Added `LDAPProperties.ObjectGUID` to the `ComputerMethodProps` initializer; public field signature unchanged, initializer extended.
LDAP processor enhancement `src/CommonLib/Processors/LdapPropertyProcessor.cs`	`ReadComputerProperties` now reads optional `objectguid` byte[] from LDAP entry, validates length == 16, constructs a `Guid` inside try/catch, and stores the uppercase GUID string in `props["ObjectGUID"]` when valid; malformed or missing bytes are skipped.
Unit tests updates `test/unit/LdapPropertyTests.cs`	Test fixtures updated to include `objectguid` byte arrays (e.g., `Guid.Parse(...).ToByteArray()`); assertions added/adjusted to expect `ObjectGUID` in positive cases and absence in negative cases.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant Processor as LdapPropertyProcessor
  participant LDAP as LDAP Entry

  Caller->>Processor: ReadComputerProperties(entry)
  Processor->>LDAP: Read standard computer properties
  Processor->>LDAP: Read `objectguid` bytes (optional)
  alt objectguid present && bytes length == 16
    Note right of Processor #DDEEFF: try/catch GUID construction
    Processor->>Processor: Byte[] -> Guid -> ToString().ToUpperInvariant()
    Processor->>Processor: props["ObjectGUID"] = guidString
  else missing or invalid
    Note right of Processor #F6F6F6: Skip ObjectGUID population
  end
  Processor-->>Caller: props dictionary

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I hopped through bytes of shiny green,
Found a GUID tucked in a shimmering seam,
I turned its bits to letters bright,
Uppercased, tidy, safe from bite.
Carrot cheers for tests that gleam. 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Title Check	⚠️ Warning	The title uses the internal ticket identifier “Bed 6619” and a vague phrase “collection of objectguid,” which does not clearly describe the change or its context. It also omits proper casing and key details needed to convey that ObjectGUID support is being added to the computer property collection. A concise title should focus on the primary change without internal ticket numbers or awkward phrasing.	Consider revising the title to remove the ticket reference and clearly state the change, for example “Add ObjectGUID to computer properties collection,” using proper casing and concise wording so teammates can immediately grasp the primary update.
Description Check	⚠️ Warning	The description omits content in the required “Description” and “Motivation and Context” sections, leaving only placeholder comments, which means it lacks a detailed summary of the changes and the rationale behind them. While testing details and change types are partially provided, the core template sections remain unfilled, preventing a clear understanding of the change. A complete PR description must populate all required template headings with relevant details.	Populate the “Description” section with a detailed overview of the code modifications and fill in the “Motivation and Context” section with reasons for the change and any related issue references to fully comply with the repository’s PR template.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c72ac72 and d80db80.

📒 Files selected for processing (1)

test/unit/LdapPropertyTests.cs (6 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

test/unit/LdapPropertyTests.cs (1)

test/unit/Utils.cs (2)

Utils (11-23)

B64ToBytes (13-16)

⏰ 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: build

🔇 Additional comments (3)

test/unit/LdapPropertyTests.cs (3)

449-449: LGTM! ObjectGUID test coverage properly implemented.

The test correctly adds objectguid test data using Guid.Parse().ToByteArray() and includes proper assertions to verify the extracted value matches the expected uppercase GUID string format.

Also applies to: 546-548

604-604: LGTM! Bad path test correctly verifies objectguid absence.

The assertion properly verifies that objectguid is not present in the result when it's not provided in the test data, confirming graceful handling of missing objectguid.

1387-1387: LGTM! ObjectGUID test coverage properly implemented.

The test correctly includes objectguid test data and assertions to verify extraction, properly addressing the previous review comment.

Also applies to: 1434-1435

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 4

🧹 Nitpick comments (1)

test/unit/LdapPropertyTests.cs (1)

439-603: Add test cases for ObjectGUID edge cases.

Consider adding dedicated test cases to cover:

Computer with null objectguid (graceful skip)

Computer with empty byte array for objectguid

Computer with malformed objectguid bytes (not 16 bytes)

These tests would verify the defensive handling suggested in the processor review and ensure devices without valid ObjectGUIDs "fail gracefully" as mentioned in the PR description.

Do you want me to generate example test cases for these scenarios?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1ec7cba and 9eadfb9.

📒 Files selected for processing (3)

src/CommonLib/LdapQueries/CommonProperties.cs (1 hunks)
src/CommonLib/Processors/LdapPropertyProcessor.cs (1 hunks)
test/unit/LdapPropertyTests.cs (6 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

test/unit/LdapPropertyTests.cs (1)

test/unit/Utils.cs (2)

Utils (11-23)

B64ToBytes (13-16)

src/CommonLib/LdapQueries/CommonProperties.cs (1)

src/CommonLib/Enums/LDAPProperties.cs (1)

LDAPProperties (3-99)

src/CommonLib/Processors/LdapPropertyProcessor.cs (4)

src/CommonLib/DirectoryObjects/DirectoryEntryWrapper.cs (1)

GetByteProperty (177-181)

src/CommonLib/DirectoryObjects/IDirectoryObject.cs (1)

GetByteProperty (17-17)

src/CommonLib/DirectoryObjects/SearchResultEntryWrapper.cs (1)

GetByteProperty (177-191)

src/CommonLib/Enums/LDAPProperties.cs (1)

LDAPProperties (3-99)

⏰ 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: build

🔇 Additional comments (2)

test/unit/LdapPropertyTests.cs (1)

395-395: ObjectGUID is not processed for user objects.

The objectguid test data added here won't be processed because ReadUserProperties in LdapPropertyProcessor.cs does not extract the ObjectGUID. Only ReadComputerProperties includes ObjectGUID extraction logic (lines 439-444).

Consider removing this test data or clarifying why it's included if it's for future use.

src/CommonLib/LdapQueries/CommonProperties.cs (1)

34-34: LGTM! Correctly adds ObjectGUID to computer method properties.

This change is necessary to ensure ObjectGUID is fetched during computer object queries, enabling the extraction logic in ReadComputerProperties. The placement is appropriate and consistent with existing property declarations.

src/CommonLib/Processors/LdapPropertyProcessor.cs

test/unit/LdapPropertyTests.cs

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (3)

src/CommonLib/Processors/LdapPropertyProcessor.cs (1)
447-450: Consider catching ArgumentException specifically.

The general catch block works but could mask unexpected exceptions. Since new Guid(byte[]) throws ArgumentException for invalid input, catching it specifically improves clarity and error handling.

Apply this diff:
-                catch
+                catch (ArgumentException)
                 {
                     // Skip malformed GUID bytes
                 }
test/unit/LdapPropertyTests.cs (2)
395-395: Remove unused objectguid test data.

This test is for ReadUserProperties, which doesn't process ObjectGUID (only ReadComputerProperties does, per the PR scope). The objectguid entry is unused and could be removed for clarity.

1459-1459: Add assertions to verify ObjectGUID extraction.

The test includes objectguid in the test data but doesn't assert it's correctly processed. Add assertions after line 1490 to verify extraction, similar to other computer property tests.

Add these assertions:
Assert.Contains("objectguid", keys);
Assert.Equal("A6F75BA4-F1AE-4B47-A606-E3A0A69AEC83", props["objectguid"]);
Note: You'll need to extract props and keys from the test result, similar to how it's done in other tests around lines 1422-1423.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 40bef5c and c72ac72.

📒 Files selected for processing (2)

src/CommonLib/Processors/LdapPropertyProcessor.cs (1 hunks)
test/unit/LdapPropertyTests.cs (8 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

test/unit/LdapPropertyTests.cs (1)

test/unit/Utils.cs (2)

Utils (11-23)

B64ToBytes (13-16)

src/CommonLib/Processors/LdapPropertyProcessor.cs (5)

src/CommonLib/DirectoryObjects/DirectoryEntryWrapper.cs (1)

GetByteProperty (177-181)

src/CommonLib/DirectoryObjects/IDirectoryObject.cs (1)

GetByteProperty (17-17)

src/CommonLib/DirectoryObjects/SearchResultEntryWrapper.cs (1)

GetByteProperty (177-191)

test/unit/Facades/MockDirectoryObject.cs (1)

GetByteProperty (151-153)

src/CommonLib/Enums/LDAPProperties.cs (1)

LDAPProperties (3-99)

⏰ 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: build

🔇 Additional comments (1)

test/unit/LdapPropertyTests.cs (1)

547-548: LGTM!

The assertions correctly verify ObjectGUID extraction with proper uppercase formatting matching the implementation.

hotnops added 2 commits October 15, 2025 08:39

Adding object guid to collection

cfb5b80

Fixing up unit tests

9eadfb9

coderabbitai bot reviewed Oct 15, 2025

View reviewed changes

src/CommonLib/Processors/LdapPropertyProcessor.cs Show resolved Hide resolved

test/unit/LdapPropertyTests.cs Show resolved Hide resolved

test/unit/LdapPropertyTests.cs Show resolved Hide resolved

test/unit/LdapPropertyTests.cs Show resolved Hide resolved

hotnops added 2 commits October 15, 2025 09:26

Ensuring that GUID as all uppercase

40bef5c

Adding more robust unit tests to appease code rabbit

c72ac72

coderabbitai bot reviewed Oct 15, 2025

View reviewed changes

MikeX777 assigned hotnops Oct 16, 2025

MikeX777 approved these changes Oct 16, 2025

View reviewed changes

Removing unncessary property adds

d80db80

definitelynotagoblin approved these changes Oct 16, 2025

View reviewed changes

MikeX777 merged commit 6d95d6a into SpecterOps:v4 Oct 16, 2025
3 checks passed

github-actions bot locked and limited conversation to collaborators Oct 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Bed 6619 collection of objectguid #253

Bed 6619 collection of objectguid #253

Uh oh!

hotnops commented Oct 15, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Oct 15, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Bed 6619 collection of objectguid #253

Bed 6619 collection of objectguid #253

Uh oh!

Conversation

hotnops commented Oct 15, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hotnops commented Oct 15, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 15, 2025 •

edited

Loading