feat: implementing AZAddOwner relationship BED-4754#2649
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Azure integration test helpers and node constructors, introduces AddOwner role IDs and a new post-processing flow that clears existing edges, discovers tenant roles and targets, enqueues azure.AddOwner relationship jobs, and adds unit and integration tests validating AddOwner edge creation. Changes
Sequence Diagram(s)sequenceDiagram
actor Test
participant CreateAZAddOwnerEdge as CreateAZAddOwnerEdge()
participant GraphDB as GraphDB
participant JobQueue as EnsureRelationshipJobQueue
Test->>CreateAZAddOwnerEdge: Invoke CreateAZAddOwnerEdge(ctx, db)
CreateAZAddOwnerEdge->>GraphDB: Clear existing azure.AddOwner edges
GraphDB-->>CreateAZAddOwnerEdge: Acknowledgement
CreateAZAddOwnerEdge->>GraphDB: Query tenants
GraphDB-->>CreateAZAddOwnerEdge: tenant list
loop per tenant
CreateAZAddOwnerEdge->>GraphDB: Query roles by AddOwnerRoleIDs()
GraphDB-->>CreateAZAddOwnerEdge: role nodes
CreateAZAddOwnerEdge->>GraphDB: Query tenant applications & service principals
GraphDB-->>CreateAZAddOwnerEdge: app/SP nodes
loop for each role × target
CreateAZAddOwnerEdge->>JobQueue: Enqueue EnsureRelationshipJob(kind=azure.AddOwner, start=role, end=target)
JobQueue-->>CreateAZAddOwnerEdge: Enqueue ack
end
end
CreateAZAddOwnerEdge-->>Test: Return stats / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/go/analysis/azure/azure_integration_test.go (1)
43-44: Defer suite teardown immediately after setup.If a
require.*fails before Line 108, cleanup is skipped and DB resources can leak for the test run.♻️ Proposed change
func TestAZAddOwner(t *testing.T) { t.Parallel() //#region Setup for test var ( suite = setupIntegrationTestSuite(t) @@ ) + defer teardownIntegrationTestSuite(t, &suite) @@ - teardownIntegrationTestSuite(t, &suite) }Also applies to: 108-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/analysis/azure/azure_integration_test.go` around lines 43 - 44, After calling setupIntegrationTestSuite(t) assign its result to suite and immediately defer suite teardown to avoid leaking DB resources if a require.* assertion fails; specifically, right after setupIntegrationTestSuite(t) add a deferred call to the suite's teardown method (e.g., defer suite.TearDownSuite() or defer suite.TearDown()) so teardown always runs, and apply the same change to the other setup call around lines 108-109 that also instantiates a suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/go/analysis/azure/azure_integration_test.go`:
- Around line 83-95: The test builds ambiguous keys by concatenating IDs into
expected map entries (see expected map population) and when iterating
addOwnerEdges using edge.StartID and edge.EndID; replace the fragile
concatenation with a safe composite key (e.g., use a tuple-like struct key or a
delimiter/format that cannot collide such as fmt.Sprintf("%s|%s", start, end))
so each pair is uniquely represented; update both the expected map keys (where
HybridIdentityAdminRole.ID, AZApp.ID, etc. are inserted) and the lookup used for
addOwnerEdges to use the new composite key type/format.
---
Nitpick comments:
In `@packages/go/analysis/azure/azure_integration_test.go`:
- Around line 43-44: After calling setupIntegrationTestSuite(t) assign its
result to suite and immediately defer suite teardown to avoid leaking DB
resources if a require.* assertion fails; specifically, right after
setupIntegrationTestSuite(t) add a deferred call to the suite's teardown method
(e.g., defer suite.TearDownSuite() or defer suite.TearDown()) so teardown always
runs, and apply the same change to the other setup call around lines 108-109
that also instantiates a suite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b7de2860-a302-4f30-92b1-b99f42d129db
📒 Files selected for processing (5)
packages/go/analysis/azure/azure_integration_suite_test.gopackages/go/analysis/azure/azure_integration_test.gopackages/go/analysis/azure/post.gopackages/go/analysis/azure/post_test.gopackages/go/graphschema/azure/roles.go
bsheth711
commented
Apr 14, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/go/analysis/azure/azure_integration_test.go (1)
77-82: Return transaction fetch errors instead of asserting and continuing.At Line 81,
assert.Nil(t, err)allows the callback to continue with invalid state and returnsnil, which can blur the root cause.🔧 Fail-fast transaction callback
addOwnerEdges, err := ops.FetchRelationships(tx.Relationships().Filterf(func() graph.Criteria { return query.Kind(query.Relationship(), graphAzure.AddOwner) })) - assert.Nil(t, err) + if err != nil { + return err + } assert.Len(t, addOwnerEdges, 8)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/analysis/azure/azure_integration_test.go` around lines 77 - 82, The transaction callback passed to GraphDB.ReadTransaction uses assert.Nil(t, err) after calling ops.FetchRelationships which lets the callback continue on failure; change the callback to fail fast by returning the encountered error (or wrapping it) instead of asserting and continuing. Specifically, in the function passed to GraphDB.ReadTransaction, replace the assert.Nil(t, err) after ops.FetchRelationships(tx.Relationships().Filterf(...)) with a direct return of err (or use require on the test caller and return err), so the ReadTransaction call sees and propagates the fetch error related to graphAzure.AddOwner.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/go/analysis/azure/azure_integration_test.go`:
- Around line 73-75: The test risks a panic when dereferencing
postProcessingStats.RelationshipsCreated[graphAzure.AddOwner]; change the
assertion flow so you first ensure the map entry exists and is non-nil before
dereferencing—either replace assert.NotNil with require.NotNil for
postProcessingStats.RelationshipsCreated[graphAzure.AddOwner] or explicitly
fetch the entry (v, ok :=
postProcessingStats.RelationshipsCreated[graphAzure.AddOwner]) and
assert.True(ok) and assert.NotNil(v) before calling assert.Equal on int(*v);
reference the symbols postProcessingStats.RelationshipsCreated and
graphAzure.AddOwner to locate the check in azure_integration_test.go.
---
Nitpick comments:
In `@packages/go/analysis/azure/azure_integration_test.go`:
- Around line 77-82: The transaction callback passed to GraphDB.ReadTransaction
uses assert.Nil(t, err) after calling ops.FetchRelationships which lets the
callback continue on failure; change the callback to fail fast by returning the
encountered error (or wrapping it) instead of asserting and continuing.
Specifically, in the function passed to GraphDB.ReadTransaction, replace the
assert.Nil(t, err) after ops.FetchRelationships(tx.Relationships().Filterf(...))
with a direct return of err (or use require on the test caller and return err),
so the ReadTransaction call sees and propagates the fetch error related to
graphAzure.AddOwner.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: cf0a1a1f-f679-43b6-8454-9ca4aa0ad2a1
📒 Files selected for processing (1)
packages/go/analysis/azure/azure_integration_test.go
cweidenkeller
approved these changes
Apr 15, 2026
StephenHinck
requested changes
Apr 15, 2026
Contributor
StephenHinck
left a comment
StephenHinck
left a comment
There was a problem hiding this comment.
This appears to utilize our legacy "delete -> recreate" pattern for post-processed edges. We now have the delta-change-analysis pattern utilized by the the user role assignment post processing package. See https://github.com/SpecterOps/BloodHound/blob/main/packages/go/analysis/azure/post.go#L927 for reference.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/go/analysis/azure/post.go (1)
751-753: Consider graceful termination onSubmitfailure for consistency.The error handling here differs from
addSecret(lines 680-681) which returnsnilon Submit failure, allowing graceful termination when context is cancelled. This explicit error approach is more defensive but creates slightly inconsistent behavior.♻️ Optional: Match the graceful termination pattern
if !sink.Submit(ctx, nextJob) { - return fmt.Errorf("unable to submit to channel in postAzureAddOwner") + return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/analysis/azure/post.go` around lines 751 - 753, In postAzureAddOwner, make Submit failure mirror the graceful termination used in addSecret: when sink.Submit(ctx, nextJob) returns false, return nil (allowing clean exit when ctx is cancelled) instead of returning a formatted error; update the failure branch in postAzureAddOwner to match addSecret's behavior so cancellation/closed-channel shutdowns are handled consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/go/analysis/azure/post.go`:
- Around line 751-753: In postAzureAddOwner, make Submit failure mirror the
graceful termination used in addSecret: when sink.Submit(ctx, nextJob) returns
false, return nil (allowing clean exit when ctx is cancelled) instead of
returning a formatted error; update the failure branch in postAzureAddOwner to
match addSecret's behavior so cancellation/closed-channel shutdowns are handled
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: cfb87f74-ee8c-42bc-a4e9-b401ef1fca60
📒 Files selected for processing (2)
packages/go/analysis/azure/azure_integration_test.gopackages/go/analysis/azure/post.go
bsheth711
force-pushed
the
BED-4754-azaddowner-relationships
branch
from
e3bea8d to
4e17c6b
Compare
StephenHinck
approved these changes
Apr 15, 2026
Contributor
StephenHinck
left a comment
StephenHinck
left a comment
There was a problem hiding this comment.
Thank you very much for that refactor to use DCA. I did pull it down and validate it runs as expected.
bsheth711
force-pushed
the
BED-4754-azaddowner-relationships
branch
from
4e17c6b to
48eed3b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR implements the AZAddOwner relationship in Azure post processing. Previously this relationship was unimplemented. The PR also includes an integration test for the new functionality.
The HybridIdentityAdministratorRole literal was found from: https://learn.microsoft.com/en-us/entra/identity/role-based-access-control/permissions-reference.
Motivation and Context
Resolves BED-4754
Why is this change required? What problem does it solve?
How Has This Been Tested?
I created a new integration test for post processing to test the creation of the AZAddOwner relationship. All new and existing tests compile and pass locally. Additionally, creation of relationships with sample data has been verified.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests