Skip to content

feat: collect msDS-aADObjectID for users and groups - BED-9245 - #308

Open
martinsohn wants to merge 1 commit into
v4from
microsoft-entra-domain-services
Open

feat: collect msDS-aADObjectID for users and groups - BED-9245#308
martinsohn wants to merge 1 commit into
v4from
microsoft-entra-domain-services

Conversation

@martinsohn

@martinsohn martinsohn commented Aug 11, 2026

Copy link
Copy Markdown

Description

Collects msDS-aADObjectID for users and groups in Microsoft Entra Domain Services and emits the normalized value as aadobjectid.

Motivation and Context

BloodHound uses aadobjectid to correlate Microsoft Entra ID identities with their synchronized Entra DS counterparts.

This PR is part of: BED-9245

How Has This Been Tested?

  • Added unit coverage for user and group collection, normalization, and blank values.
  • Ran dotnet test test/unit/CommonLibTest.csproj: 430 of 431 tests passed.
  • One unrelated timestamp assertion failed locally by one day in ConvertFileTimeToUnixEpoch_ValidTimestamp_ValidUnixEpoch.
  • Built SharpHound with this SharpHoundCommon revision and confirmed aadobjectid in the lab collection for 5 users and 7 groups.

Screenshots (if appropriate):

N/A

Types of changes

  • Chore
  • Bug fix
  • New feature
  • Breaking change

Checklist:

  • I have met the contributing prerequisites
  • I have ensured that related documentation is up-to-date
  • I have followed proper test practices
    • Added/updated tests to cover these changes.
    • One unrelated existing test failure is documented above.

Summary by CodeRabbit

  • New Features
    • Added support for reading Azure AD object IDs from directory records.
    • Object IDs are normalized for consistent use across user and group properties.
    • The original directory-specific attribute is excluded from generic property results.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The LDAP property model now includes msds-aadobjectid. Group and user property processing stores valid values as trimmed, uppercase aadobjectid entries. Tests verify normalization, source-key removal, and reserved-property filtering.

Changes

AAD object ID LDAP support

Layer / File(s) Summary
LDAP property contract
src/CommonLib/Enums/LDAPProperties.cs, src/CommonLib/LdapQueries/CommonProperties.cs
Adds the AADObjectID LDAP constant and includes it in the requested object properties list.
AAD object ID processing and validation
src/CommonLib/Processors/LdapPropertyProcessor.cs, test/unit/LdapPropertyTests.cs
Normalizes valid values for group and user properties. Tests verify uppercase output, source-key removal, and reserved-property filtering.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: lrfalslev

Poem

A rabbit found an ID in the LDAP hay,
Trimmed its whitespace and hopped away.
Uppercase letters lined up bright,
Group and user paths now read it right.
The source key rests, filtered from sight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the new collection of msDS-aADObjectID values for users and groups.
Description check ✅ Passed The description covers the change, motivation, testing, change type, and documented unrelated test failure.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch microsoft-entra-domain-services

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
test/unit/LdapPropertyTests.cs (1)

129-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for trimming and blank values.

The group and user fixtures use already trimmed, non-empty values. The assertions verify uppercase output, but they do not exercise Trim() or the IsNullOrWhiteSpace omission path in AddAADObjectIDProperty. Add one input with surrounding whitespace and one whitespace-only input. Assert the normalized value in the first case and the absence of aadobjectid in the second case.

Also applies to: 140-142, 326-326, 363-365

🤖 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 `@test/unit/LdapPropertyTests.cs` at line 129, Add test coverage in the group
and user LDAP property fixtures around the existing msds-aadobjectid cases:
include a value with surrounding whitespace and assert it is trimmed and
normalized, then include a whitespace-only value and assert aadobjectid is
omitted. Apply the same coverage to the referenced fixture cases while
preserving the existing uppercase assertions.
🤖 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.

Nitpick comments:
In `@test/unit/LdapPropertyTests.cs`:
- Line 129: Add test coverage in the group and user LDAP property fixtures
around the existing msds-aadobjectid cases: include a value with surrounding
whitespace and assert it is trimmed and normalized, then include a
whitespace-only value and assert aadobjectid is omitted. Apply the same coverage
to the referenced fixture cases while preserving the existing uppercase
assertions.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b91909b6-7174-4ebb-ab82-07c20d294057

📥 Commits

Reviewing files that changed from the base of the PR and between fcb6cb9 and f24ce18.

📒 Files selected for processing (4)
  • src/CommonLib/Enums/LDAPProperties.cs
  • src/CommonLib/LdapQueries/CommonProperties.cs
  • src/CommonLib/Processors/LdapPropertyProcessor.cs
  • test/unit/LdapPropertyTests.cs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant