BED-6418: Completeness Metrics Update

[maffkipp]

Description

This PR updates our rules for determining which users or computers to consider "active" when calculating Local Group Completeness and Session Completeness metrics. Both should now use the following criteria:

• user or computer must have the enabled property set to true
• user or computer must have a lastlogontimestamp property within the last 14 days

Motivation and Context

Resolves BED-6418

Previously, we were not checking the enabled property and we had a expiration window of 90 days, which is too long to provide helpful results and makes this metric appear lower than needed.

How Has This Been Tested?

• Updated testing harness to check the new behavior works as expected

Screenshots (optional):

Types of changes

• New feature (non-breaking change which adds functionality)

Checklist:

• I have met the contributing prerequisites
  • Assigned myself to this PR
  • Added the appropriate labels
  • Associated an issue: Issues and Pull Requests README #672
  • Read the Contributing guide: https://github.com/SpecterOps/BloodHound/wiki/Contributing
• I have ensured that related documentation is up-to-date
  • Open API docs
  • Code comments (GoDocs / JSDocs)
• I have followed proper test practices
  • Added/updated tests to cover my changes
  • All new and existing tests passed

Summary by CodeRabbit

• Bug Fixes
  • Updated activity detection logic to measure system activity based on logon timestamps instead of password change dates for improved accuracy.
  • Reduced activity threshold from 90 to 14 days to better identify inactive systems.
  • Enhanced completeness reports to exclude disabled systems and require valid logon data.

[coderabbitai]

Walkthrough

The changes shift user activity filtering logic in Active Directory completeness analysis from PasswordLastSet to LastLogonTimestamp. A test harness is updated to include a disabled user node with corresponding relationship adjustments. Query functions are refactored to enforce the Enabled flag and reduce the activity threshold from ninety to fourteen days.

Changes

Cohort / File(s)	Summary
Test harness updates cmd/api/src/test/integration/harnesses/completenessharness.json	Introduces new CUserDisabled node (n10) with enabled:false and lastlogontimestamp. Adds enabled flags to existing node properties. Reworks relationship definitions with explicit fromId/toId pairs and adds new MemberOf relationships referencing the new node. Adjusts node y-coordinates for layout reordering.
Query logic refactoring packages/go/analysis/ad/queries.go	Replaces ninetyDays with new fourteenDays constant. In FetchLocalGroupCompleteness and FetchUserSessionCompleteness, shifts from PasswordLastSet to LastLogonTimestamp for activity filtering. Adds Enabled == true requirement and updates activity threshold calculation to use fourteenDays based on most recent logon timestamp.

Sequence Diagram

sequenceDiagram
    participant Filter as Activity Filter Logic
    participant OldPath as Old Path (PasswordLastSet)
    participant NewPath as New Path (LastLogonTimestamp)
    participant Result as Filter Result

    rect rgb(240, 248, 255)
    Note over OldPath: Previous Approach
    Filter->>OldPath: Check PasswordLastSet > 90 days ago
    OldPath->>Result: Include/Exclude based on pwd change
    end

    rect rgb(230, 245, 230)
    Note over NewPath: New Approach
    Filter->>NewPath: Check Enabled == true
    NewPath->>NewPath: Check LastLogonTimestamp exists
    NewPath->>NewPath: Check LastLogonTimestamp > 14 days ago
    NewPath->>Result: Include/Exclude based on recent activity
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Harness file: Verify the new CUserDisabled node structure is correct and all relationship updates reference valid node IDs
• Query functions: Trace through both FetchLocalGroupCompleteness and FetchUserSessionCompleteness to ensure LastLogonTimestamp logic is correctly applied; validate that the 14-day threshold is intentional and doesn't inadvertently filter out expected records; confirm Enabled flag filtering is applied consistently

Poem

🐰 Fourteen sunsets, not ninety days,
We hop through timestamps in mystic ways,
Enabled flags now guard the gate,
LastLogonTime determines your fate,
Active hearts are all we celebrate! 🕒✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
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.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The PR title "BED-6418: Completeness Metrics Update" is partially related to the changeset. It correctly identifies that the changes concern completeness metrics and includes the relevant Jira ticket identifier, making it trackable and contextually relevant. However, the title is somewhat broad and doesn't capture the specific nature of the update—namely, the shift from a 90-day to 14-day activity window and the addition of an enabled property check. While a teammate scanning history would understand this involves completeness metrics changes, the title could be more specific about the actual behavioral changes.
Description Check	✅ Passed	The PR description is well-structured and addresses all major template sections. It includes a clear description of changes (enabled property and 14-day window requirement), explains the motivation (previous 90-day window was unhelpful and enabled property wasn't checked), documents testing approach (updated testing harness), specifies the change type (new feature), and completes all checklist items. The associated Jira ticket is properly referenced, and the description provides sufficient detail for reviewers to understand the scope and rationale of the changes.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch BED-6418--completeness-stats

---

📜 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 3432fda and 973e47f.

⛔ Files ignored due to path filters (1)

• cmd/api/src/test/integration/harnesses/completenessharness.svg is excluded by !**/*.svg

📒 Files selected for processing (2)

• cmd/api/src/test/integration/harnesses/completenessharness.json (13 hunks)
• packages/go/analysis/ad/queries.go (5 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

packages/go/analysis/ad/queries.go (2)

packages/go/graphschema/common/common.go (1)

• Enabled (64-64)

packages/go/graphschema/ad/ad.go (1)

• LastLogonTimestamp (169-169)

⏰ 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). (4)

• GitHub Check: Build BloodHound Container Image / Build and Package Container
• GitHub Check: run-analysis
• GitHub Check: build-ui
• GitHub Check: run-tests

🔇 Additional comments (5)

packages/go/analysis/ad/queries.go (2)

1508-1508: Verify the 14-day activity window is appropriate.

The activity threshold has been reduced from 90 days to 14 days. This is a significant change that will classify far fewer users and computers as "active," potentially causing a substantial drop in completeness metrics for existing deployments.

Please confirm:

1. Is 14 days the intended threshold for your Active Directory environment?
2. Have stakeholders been informed about the potential impact on existing completeness metrics?
3. Are there any environments where a configurable threshold would be more appropriate?

---

1581-1649: LGTM! User session completeness logic is correct.

The refactoring properly:

• Filters for enabled users with LastLogonTimestamp
• Calculates the activity threshold based on the most recent logon
• Applies the 14-day window consistently

Variable naming is clear and the logic matches the function in FetchLocalGroupCompleteness.

cmd/api/src/test/integration/harnesses/completenessharness.json (3)

162-166: Verify CComputerA should be excluded from completeness metrics.

CComputerA (n5) has enabled: "true" but is missing the lastlogontimestamp property. According to the query filters in FetchLocalGroupCompleteness (lines 1518-1519 in queries.go), computers must have both Enabled == true and an existing LastLogonTimestamp property.

Please confirm:

1. Is CComputerA intentionally designed to be filtered out of completeness calculations?
2. If so, are there tests that verify this computer is correctly excluded?
3. Should the test harness include a comment explaining this node's purpose?

---

243-260: Excellent addition of disabled user test case.

The new CUserDisabled node (n10) is a valuable test case. It has a recent lastlogontimestamp (NOW()) but enabled: "false", which should cause it to be filtered out by the new Enabled check in the completeness queries.

This ensures the test harness validates that disabled accounts are correctly excluded from completeness metrics regardless of their recent activity.

---

146-148: Incorrect — 10800000 is seconds (≈125 days), so the timestamp is correct.

The harness uses epoch seconds (fixtures contain integer epoch‑seconds and searches compare n.lastlogontimestamp to datetime().epochseconds). 10,800,000 seconds = 125 days, which is > 14 days (1,209,600 seconds).

Likely an incorrect or invalid review comment.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[AD7ZJ]

This looks good to me, at least as far as implementing what is described in the ticket :)

[ALCooper12]

Just wondering, but why doesn't CComputerB(Base Computer) and CGroup(Base Group) not have enabled: true or enabled: false like the other nodes on the completenesssharness.svg file?

[maffkipp]

@ALCooper12 CComputerA and CComputerB are both intended as different kinds of negative cases and should not be included in the calculation for Local Group Completeness, since they do not meet the requirements for both the enabled property and the lastlogontimestamp property. Similarly, we don't check those properties for Group nodes during these calculations and so don't need to set them for CGroup.

[ALCooper12]

Thanks for clarifying! LGTM 👍🏾