Add GPO Status Property and Pretty Resolution #1825
Conversation
mykeelium
added
enhancement
WalkthroughAdds GPO status parsing and exposure across schema, ingestion, converter, UI, and tests: new ParseGPOData returns GPO node props (raw and pretty status), graph schema and SDKs gain Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Conv as Graphify Converter
participant EIN as ein.ParseGPOData
participant Schema as Graph Schema
Conv->>EIN: ParseGPOData(gpo)
EIN->>EIN: read gpo.Properties[ad.GPOStatus.String()]
alt Status present ("0".."3")
EIN->>EIN: set GPOStatusRaw and map to PrettyGPOStatus
EIN-->>Conv: IngestibleNode{ObjectID, Labels:[ad.GPO], Props:{gpostatusraw,gpostatus}}
else Status missing/unknown
EIN-->>Conv: IngestibleNode{ObjectID, Labels:[ad.GPO], Props:{} or {gpostatusraw}}
end
Conv->>Conv: append node props + ACE-derived rel props
Note over Conv: NodeProps now include GPO status fields
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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)
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: 3
🧹 Nitpick comments (3)
packages/go/ein/ad_test.go (2)
358-359: Rename for accuracy: TestParseGroupGPOData → TestParseGPOData.The test exercises GPO parsing, not Group. A small rename improves discoverability.
Apply:
-func TestParseGroupGPOData(t *testing.T) { +func TestParseGPOData(t *testing.T) {
369-453: Use ad constants for property keys in expectations to prevent drift.Hard-coded "gpostatus"/"gpostatusraw" make tests brittle if keys change. Prefer ad.GPOStatus(.String()) and ad.GPOStatusRaw(.String()) like other tests in this file.
Apply:
- Properties: map[string]any{"gpostatus": "0"}, + Properties: map[string]any{ad.GPOStatus.String(): "0"}, ... - PropertyMap: map[string]any{"gpostatusraw": "0", "gpostatus": ein.PrettyGPOStatusEnabled}, + PropertyMap: map[string]any{ad.GPOStatusRaw.String(): "0", ad.GPOStatus.String(): ein.PrettyGPOStatusEnabled}, ... - Properties: map[string]any{"gpostatus": "1"}, + Properties: map[string]any{ad.GPOStatus.String(): "1"}, ... - PropertyMap: map[string]any{"gpostatusraw": "1", "gpostatus": ein.PrettyGPOStatusUserConfigurationDisabled}, + PropertyMap: map[string]any{ad.GPOStatusRaw.String(): "1", ad.GPOStatus.String(): ein.PrettyGPOStatusUserConfigurationDisabled}, ... - Properties: map[string]any{"gpostatus": "2"}, + Properties: map[string]any{ad.GPOStatus.String(): "2"}, ... - PropertyMap: map[string]any{"gpostatusraw": "2", "gpostatus": ein.PrettyGPOStatusComputerConfigurationDisabled}, + PropertyMap: map[string]any{ad.GPOStatusRaw.String(): "2", ad.GPOStatus.String(): ein.PrettyGPOStatusComputerConfigurationDisabled}, ... - Properties: map[string]any{"gpostatus": "3"}, + Properties: map[string]any{ad.GPOStatus.String(): "3"}, ... - PropertyMap: map[string]any{"gpostatusraw": "3", "gpostatus": ein.PrettyGPOStatusDisabled}, + PropertyMap: map[string]any{ad.GPOStatusRaw.String(): "3", ad.GPOStatus.String(): ein.PrettyGPOStatusDisabled}, ... - Properties: map[string]any{"gpostatus": "4"}, + Properties: map[string]any{ad.GPOStatus.String(): "4"}, ... - PropertyMap: map[string]any{"gpostatusraw": "4", "gpostatus": ein.PrettyGPOStatusNotExisting}, + PropertyMap: map[string]any{ad.GPOStatusRaw.String(): "4", ad.GPOStatus.String(): ein.PrettyGPOStatusNotExisting},packages/go/ein/ad.go (1)
1542-1544: Remove empty var() block.The no-op var block adds noise; other new funcs avoid it. Safe to remove.
Apply:
- var ()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
cmd/api/src/services/graphify/convertors.go(1 hunks)packages/go/ein/ad.go(1 hunks)packages/go/ein/ad_test.go(1 hunks)packages/go/graphschema/ad/ad.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
cmd/api/src/services/graphify/convertors.go (1)
packages/go/ein/ad.go (1)
ParseGPOData(1541-1568)
packages/go/ein/ad_test.go (3)
packages/go/graphschema/ad/ad.go (1)
GPO(32-32)packages/go/ein/models.go (1)
IngestibleNode(77-81)packages/go/ein/ad.go (6)
PrettyGPOStatusEnabled(1535-1535)PrettyGPOStatusUserConfigurationDisabled(1536-1536)PrettyGPOStatusComputerConfigurationDisabled(1537-1537)PrettyGPOStatusDisabled(1538-1538)PrettyGPOStatusNotExisting(1534-1534)ParseGPOData(1541-1568)
packages/go/graphschema/ad/ad.go (1)
packages/go/graphschema/common/common.go (1)
Property(49-49)
packages/go/ein/ad.go (2)
packages/go/graphschema/ad/ad.go (3)
GPO(32-32)GPOStatus(270-270)GPOStatusRaw(269-269)packages/go/ein/models.go (1)
IngestibleNode(77-81)
⏰ 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). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: build-ui
🔇 Additional comments (1)
cmd/api/src/services/graphify/convertors.go (1)
176-177: Nice: Append parsed GPO node data to NodeProps for merge-on-ingest.This mirrors the DC registry pattern and should merge cleanly on ObjectID. No concerns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/go/ein/ad.go (1)
1549-1564: Minor clarity/perf: switch on a local ‘raw’ var instead of re‑indexing the map.Reduces interface comparisons and makes intent explicit.
Apply:
- if status, ok := gpo.Properties[ad.GPOStatus.String()]; ok { - propMap[ad.GPOStatusRaw.String()] = strings.TrimSpace(fmt.Sprint(status)) - - switch propMap[ad.GPOStatusRaw.String()] { + if status, ok := gpo.Properties[ad.GPOStatus.String()]; ok { + raw := strings.TrimSpace(fmt.Sprint(status)) + propMap[ad.GPOStatusRaw.String()] = raw + + switch raw { case "0": propMap[ad.GPOStatus.String()] = PrettyGPOStatusEnabled case "1": propMap[ad.GPOStatus.String()] = PrettyGPOStatusUserConfigurationDisabled case "2": propMap[ad.GPOStatus.String()] = PrettyGPOStatusComputerConfigurationDisabled case "3": propMap[ad.GPOStatus.String()] = PrettyGPOStatusDisabled default: propMap[ad.GPOStatus.String()] = PrettyGPOStatusNotExisting } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/csharp/graphschema/PropertyNames.cs(2 hunks)packages/cue/bh/ad/ad.cue(2 hunks)packages/go/ein/ad.go(1 hunks)packages/go/ein/ad_test.go(1 hunks)packages/go/graphschema/ad/ad.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cue/bh/ad/ad.cue
- packages/go/ein/ad_test.go
- packages/csharp/graphschema/PropertyNames.cs
🧰 Additional context used
🧬 Code graph analysis (2)
packages/go/graphschema/ad/ad.go (3)
cmd/api/src/test/integration/harnesses.go (1)
Property(6383-6383)packages/go/graphschema/azure/azure.go (1)
Property(99-99)packages/go/graphschema/common/common.go (1)
Property(49-49)
packages/go/ein/ad.go (4)
packages/go/graphschema/ad/ad.go (3)
GPO(32-32)GPOStatus(274-274)GPOStatusRaw(273-273)packages/go/ein/incoming_models.go (1)
GPO(168-168)packages/go/ein/models.go (1)
IngestibleNode(77-81)packages/go/graphschema/common/common.go (1)
ObjectID(52-52)
⏰ 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). (2)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
🔇 Additional comments (8)
packages/go/graphschema/ad/ad.go (5)
273-274: New GPO properties wired — ensure they’re schema‑generated.Additions look correct. Please confirm these were produced via CUE codegen to avoid drift.
Run to verify CUE, JS, and C# schemas also define these properties:
#!/bin/bash set -euo pipefail # CUE schemas rg -n -C3 -S '\bgpostatus(raw)?\b' packages/cue || true # Go generated file sanity (present here) rg -n -C1 -S 'GPOStatus(Raw)?|\"gpostatus(raw)?\"' packages/go/graphschema/ad/ad.go # JS UI schema rg -n -C3 -S '\bgpostatus(raw)?\b' packages/javascript || true # C# names rg -n -C3 -S 'GPOStatus(Raw)?|gpostatus(raw)?' packages/csharp || true
277-279: AllProperties() includes new keys.Slice updated correctly to include GPOStatusRaw and GPOStatus.
558-561: ParseProperty covers both new literals.String→enum mapping looks correct.
845-847: String() mapping added for both keys.Consistent with other entries.
1131-1133: Name() entries read well.Labels are clear and match naming conventions.
packages/go/ein/ad.go (3)
1539-1544: Prettified GPO status constants look good.Values are concise and user‑friendly.
1546-1571: Nice: status normalization avoids panics on non‑string inputs.Converting via fmt.Sprint + TrimSpace fixes the earlier crash risk.
1546-1553: ParseGPOData is invoked and its props are included in GPO ingestion
cmd/api/src/services/graphify/convertors.go appends ein.ParseGPOData(gpo) to converted.NodeProps (line ~176); unit tests in packages/go/ein/ad_test.go cover the pretty-status mappings.
Description
Handles new collection properties on a GPO. The status of the GPO is collected as "gpostatus" and this is ingested as a "gpostatusraw" value. Properties were added for simple text explanations of what the status is.
Motivation and Context
Resolves BED-6027
How Has This Been Tested?
This change has been tested by ingesting data with this new field, as well as creating unit tests around the functionality.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Schema
UI
Tests