BED-5752 PIM Roles AZRoleApprover Post Integration & Testing #1615
Conversation
… slices in the graphdb, and call PIM Roles post
|
""" WalkthroughThis update introduces the creation and testing of Azure Privileged Identity Management (PIM) role approver edges in the analysis pipeline. It adds new integration tests, a test harness, supporting JSON configuration, and modifies the post-processing logic to include the creation of AZRoleApprover edges. Query logic for approver edge creation is also simplified. Changes
Sequence Diagram(s)sequenceDiagram
participant PostFunction as Post()
participant Hybrid as hybrid.PostHybrid
participant AzureAnalysis as azureAnalysis.CreateAZRoleApproverEdge
participant Stats as AggregateStats
PostFunction->>Hybrid: PostHybrid(...)
Hybrid-->>PostFunction: HybridStats, err
alt No error
PostFunction->>AzureAnalysis: CreateAZRoleApproverEdge(...)
AzureAnalysis-->>PostFunction: ApproverStats, err
alt No error
PostFunction->>Stats: Merge ApproverStats
PostFunction-->>Caller: AggregateStats, nil
else Error
PostFunction-->>Caller: AggregateStats, err
end
else Error
PostFunction-->>Caller: HybridStats, err
end
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🧬 Code Graph Analysis (1)packages/go/analysis/azure/post.go (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (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 (
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
cmd/api/src/analysis/azure/pimroles_integration_test.go (1)
21-79: Comprehensive integration test with detailed relationship verification.The test properly validates the
CreateAZRoleApproverEdgefunctionality with appropriate setup, execution, and verification phases. The specific assertions ensure the correct relationships are created between expected nodes.Consider adding a helper function to centralize the expected mappings for better maintainability:
+// expectedMappings defines the expected start->end node relationships +var expectedMappings = map[string][]string{ + "AZBase n5": {"PIMTestRole"}, + "AZBase n7": {"PIMTestRole", "PIMTestRole3"}, + "AZBase n10": {"PIMTestRole3"}, + // ... other mappings +}This would make the test more maintainable if the harness data changes in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/api/src/analysis/azure/pimroles_integration_test.go(1 hunks)cmd/api/src/analysis/azure/post.go(1 hunks)cmd/api/src/test/integration/graph.go(2 hunks)cmd/api/src/test/integration/harnesses.go(2 hunks)cmd/api/src/test/integration/harnesses/AZPIMRolesHarness.json(1 hunks)packages/go/analysis/azure/role_approver.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/go/analysis/azure/role_approver.go (1)
packages/go/graphschema/azure/azure.go (2)
EndUserAssignmentUserApprovers(121-121)EndUserAssignmentGroupApprovers(122-122)
cmd/api/src/test/integration/graph.go (4)
cmd/api/src/test/integration/harnesses/utils.go (1)
Node(38-44)packages/go/graphschema/common/common.go (2)
Name(37-37)ObjectID(36-36)packages/go/graphschema/azure/azure.go (3)
TenantID(91-91)License(109-109)Entity(12-12)packages/go/graphschema/ad/ad.go (1)
Entity(12-12)
cmd/api/src/analysis/azure/post.go (1)
packages/go/analysis/azure/post.go (1)
CreateAZRoleApproverEdge(955-977)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
🔇 Additional comments (8)
packages/go/analysis/azure/role_approver.go (1)
67-68: Simplified query logic for approver property checks.The removal of empty list checks broadens the selection to include roles with non-null but potentially empty approver properties. This aligns with the bug fix mentioned in the PR objectives regarding DAWGs query issues when approver slices were not set.
cmd/api/src/test/integration/graph.go (2)
334-342: Well-implemented test utility method.The
NewAzureBasemethod follows the established pattern of other node creation methods and correctly maps the required properties for Azure base entities.
582-582: Minor formatting improvements for consistency.Adding spaces after comment slashes improves readability and maintains consistent comment formatting throughout the file.
Also applies to: 585-585, 588-588
cmd/api/src/analysis/azure/post.go (1)
42-44: Proper integration into post-processing pipeline.The addition of
CreateAZRoleApproverEdgefollows the established pattern for post-processing steps with appropriate error handling and statistics merging.Also applies to: 50-50
cmd/api/src/test/integration/harnesses.go (3)
9866-9887: LGTM! Well-structured test harness definition.The
AZPIMRolesHarnessstruct is well-organized with clear separation between tenant, base nodes, and role nodes. The naming convention is consistent and descriptive.
9889-9965: LGTM! Comprehensive test data setup with good PIM configuration coverage.The Setup method creates a thorough test scenario covering various PIM configurations:
- Different approval requirements
- Mixed user and group approvers
- Various MFA, justification, and ticket requirements
- Proper tenant-role relationships
This provides excellent test coverage for the AZRoleApprover edge creation functionality.
10078-10078: LGTM! Proper integration with harness system.The harness is correctly added to the
HarnessDetailsstruct, enabling its use in integration tests.cmd/api/src/test/integration/harnesses/AZPIMRolesHarness.json (1)
140-149: Mixed list-vs-string encoding for approver fields – verify downstream parser
EndUserAssignmentUserApprovers/…GroupApproversappear in three different shapes:
- Plain GUID string – e.g. line 144
- Comma-separated list without brackets – none in this file
- Bracketed comma-separated list stored as a single string – lines 278-279 & 332-333
Unless the post-processing code normalises these patterns, only one representation will be interpreted correctly. Please confirm what
azureAnalysis/post.goexpects; if it demands an array, emit JSON arrays instead:- "EndUserAssignmentUserApprovers": "[8793f86f-aa8c-459b-968d-617e4d1e7db8, … ]" + "EndUserAssignmentUserApprovers": [ + "8793f86f-aa8c-459b-968d-617e4d1e7db8", + "7c48ffef-4f6e-4646-b261-9b41d3761127", + "b7a61883-d1d4-4197-8c40-cb258de60f58" + ]This keeps the harness strictly valid JSON and avoids brittle string-splitting downstream.
Also applies to: 274-281, 328-336
| "id": "n0", | ||
| "position": { | ||
| "x": 320.152171827136, | ||
| "y": 368.5809253615741 | ||
| }, | ||
| "caption": "", | ||
| "style": { | ||
| "outside-position": "right" | ||
| }, | ||
| "labels": [ | ||
| "AZRole" | ||
| ], | ||
| "properties": { | ||
| "name": "Global Administrator", | ||
| "objectid": "62e90394-69f5-4237-9190-012177145e10@6c12b0b0-b2cc-4a73-8252-0b94bfca2145", | ||
| "EndUserAssignmentRequiresApproval": "False", | ||
| "EndUserAssignmentRequiresCAPAuthenticationContext": "False", | ||
| "EndUserAssignmentUserApprovers": "", | ||
| "EndUserAssignmentGroupApprovers": "", | ||
| "EndUserAssignmentRequiresMFA": "True", | ||
| "EndUserAssignmentRequiresJustification": "True", | ||
| "EndUserAssignmentRequiresTicketInformation": "False" | ||
| } | ||
| }, |
There was a problem hiding this comment.
Relationship IDs collide with node IDs – must be unique
Node n0 (lines 57-80) is re-used as the ID of the first relationship (lines 372-378).
Most loaders—including the BloodHound test harness helpers—expect the id field to be globally unique across all nodes and relationships. Re-using the same identifier can silently drop elements or blow up with “duplicate key”/“already exists” errors when the harness is ingested.
- "id": "n0", // relationship
+ "id": "r0", // relationship – make all rel IDs distinct, e.g. r0…r13Update every relationship block ("id": "n0" … "id": "n14") to use a prefix that cannot clash with node IDs.
Also applies to: 372-378
🤖 Prompt for AI Agents
In cmd/api/src/test/integration/harnesses/AZPIMRolesHarness.json between lines
57-80 and 372-378, the "id" field for nodes and relationships is duplicated,
causing conflicts. To fix this, update the "id" values for all relationship
blocks by adding a unique prefix (e.g., "rel_") to ensure they do not clash with
node IDs, making all IDs globally unique across nodes and relationships.
There was a problem hiding this comment.
This can be ignored as the json is just used as a reference for writing the Go test harness
There was a problem hiding this comment.
@mvlipka, understood! Thank you for the clarification. You're absolutely right - this aligns with the existing knowledge that JSON files in the cmd/api/src/test/integration/harnesses/ directory are reference documents for writing Go test harnesses, not actual data files that get parsed by the application code. Since these files serve as visual/structural references rather than being ingested directly, the duplicate ID issue doesn't affect functionality.
I'll note this for future reviews of similar reference files in the harnesses directory.
🧠 Learnings used
Learnt from: mvlipka
PR: SpecterOps/BloodHound#1615
File: cmd/api/src/test/integration/harnesses/AZPIMRolesHarness.json:190-194
Timestamp: 2025-06-26T20:50:03.654Z
Learning: The JSON files in cmd/api/src/test/integration/harnesses/ directory (like AZPIMRolesHarness.json) are used as reference documents for writing Go test harnesses, not as actual data files that get parsed by the application code. Minor formatting issues like trailing spaces in these reference files don't affect functionality.
Learnt from: elikmiller
PR: SpecterOps/BloodHound#1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/upload/upload.go:36-67
Timestamp: 2025-05-27T17:30:55.548Z
Learning: In the BloodHound codebase, SaveIngestFile function should only handle file operations (save + validate) and should not populate RequestID/JobID fields, as these are properly handled by the API handler layer which has access to request context and URL parameters.
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1606
File: .golangci.json:3-5
Timestamp: 2025-06-25T18:23:53.103Z
Learning: In the BloodHound project's .golangci.json, errcheck linter is intentionally disabled because it was too noisy, but the severity override settings are preserved for when it gets re-enabled in the future.
| "AZBase" | ||
| ], | ||
| "properties": { | ||
| "objectid": "2c1b9e24-af0b-42e3-ac11-f70fe1f58e71 " | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Trailing space in objectid will break look-ups
The objectid of node n7 ends with a space:
"objectid": "2c1b9e24-af0b-42e3-ac11-f70fe1f58e71 "Many parsers do string-literal comparisons when mapping approvers ⇒ bases. That stray space makes the value unequal to the same GUID used elsewhere and will cause the approver edge (n5 → n14) to be skipped.
- "objectid": "2c1b9e24-af0b-42e3-ac11-f70fe1f58e71 "
+ "objectid": "2c1b9e24-af0b-42e3-ac11-f70fe1f58e71"🤖 Prompt for AI Agents
In cmd/api/src/test/integration/harnesses/AZPIMRolesHarness.json around lines
190 to 194, the "objectid" value has a trailing space that will cause string
comparison mismatches and break look-ups. Remove the trailing space from the
"objectid" string so it exactly matches the GUID used elsewhere, ensuring proper
mapping of approvers to bases.
There was a problem hiding this comment.
This can be ignored as the json is just used as a reference for writing the Go test harness
There was a problem hiding this comment.
@mvlipka Thanks for the clarification! I understand now that this JSON file serves as a reference document for writing the Go test harness rather than being parsed directly by the application code. That makes sense - the trailing space won't cause any issues since it's not being used for actual string lookups or comparisons.
✏️ Learnings added
Learnt from: mvlipka
PR: SpecterOps/BloodHound#1615
File: cmd/api/src/test/integration/harnesses/AZPIMRolesHarness.json:190-194
Timestamp: 2025-06-26T20:50:03.654Z
Learning: The JSON files in cmd/api/src/test/integration/harnesses/ directory (like AZPIMRolesHarness.json) are used as reference documents for writing Go test harnesses, not as actual data files that get parsed by the application code. Minor formatting issues like trailing spaces in these reference files don't affect functionality.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1606
File: .golangci.json:3-5
Timestamp: 2025-06-25T18:23:53.103Z
Learning: In the BloodHound project's .golangci.json, errcheck linter is intentionally disabled because it was too noisy, but the severity override settings are preserved for when it gets re-enabled in the future.
Description
This creates the test harness for post processing on the
AZRoleApproveredge as well as calls the post processing function inazureAnalysisDuring integration testing, we found a bug with the existing DAWGs query when the approvers slices wasn't set, that was also fixed
Motivation and Context
Resolves: BED-5752
How Has This Been Tested?
Integration tests were written to cover the edge creation
Screenshots (optional):
The harness was tested and written against the provided images:
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Chores