feat(bed-5656): create AZRoleApprover edge for post processing #1561
Conversation
|
Warning Rate limit exceeded@neumachen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis change introduces new functionality to the Azure analysis package, enabling the creation of "approver" edges in the graph database for Azure roles. It adds a function to iterate over all tenants and delegates the creation of approver edges for each role, handling explicit and default approvers. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Post as CreateAZRoleApproverEdge
participant DB as Graph Database
participant Edge as CreateApproverEdge
Caller->>Post: Call CreateAZRoleApproverEdge(ctx, db)
Post->>DB: Fetch all AZTenant nodes
loop For each tenant
Post->>Edge: CreateApproverEdge(ctx, db, tenantNode, operation)
Edge->>DB: Query roles and approvers for tenant
alt Explicit approvers exist
Edge->>DB: Fetch approver entity nodes
Edge->>DB: Create approver edges
else No explicit approvers
Edge->>DB: Fetch default admin roles
Edge->>DB: Create approver edges from admin roles
end
end
Post-->>Caller: Return stats and error (if any)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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 (
|
a1c5a8c to
02ba7d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/go/analysis/azure/post.go (2)
988-997:⚠️ Potential issueMissing EndUserAssignmentUserApprovers handling
The code only reads
EndUserAssignmentGroupApproversbut according to the past review comments and the query logic, you should also handleEndUserAssignmentUserApprovers.
1018-1023:⚠️ Potential issueCritical: Uninitialized channel will cause nil pointer panic
The
outCchannel is declared but never initialized, which will cause a panic when trying to submit. Additionally, the function doesn't follow the established post-processing pattern used by other functions in this file.You need to follow the pattern shown in past review comments and other functions like
AppRoleAssignments. Here's an example from the file:operation := analysis.NewPostRelationshipOperation(ctx, db, "Azure Role Approver Post Processing") // ... then use operation.Operation.SubmitReader to properly handle channelsAlso applies to: 1063-1068
🧹 Nitpick comments (1)
packages/go/analysis/azure/post.go (1)
948-949: Improve error message with tenant contextThe error message should include which tenant node had the missing property for better debugging.
-return nil, fmt.Errorf("read tenant ID property value is nil") +return nil, fmt.Errorf("tenant node %d is missing %s property", tenantNode.ID, azure.TenantID)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/go/analysis/azure/post.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/go/analysis/azure/post.go (9)
packages/go/dawgs/graph/node.go (1)
NodeSet(133-133)packages/go/analysis/azure/tenant.go (1)
FetchTenants(86-102)packages/go/graphschema/azure/azure.go (7)
TenantID(107-107)EndUserAssignmentRequiresApproval(135-135)EndUserAssignmentUserApprovers(137-137)EndUserAssignmentGroupApprovers(138-138)GlobalAdmin(65-65)PrivilegedRoleAdmin(69-69)AZRoleApprover(96-96)packages/go/dawgs/graph/graph.go (2)
Transaction(340-378)ID(113-113)packages/go/dawgs/ops/ops.go (1)
FetchNodeSet(104-114)packages/go/dawgs/query/model.go (3)
NodeProperty(159-161)Or(201-205)IsNotNull(553-555)packages/go/analysis/post.go (1)
CreatePostRelationshipJob(151-157)packages/go/graphschema/common/common.go (1)
ObjectID(52-52)packages/go/dawgs/graph/properties.go (2)
AsProperties(492-511)PropertyMap(480-480)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: build-ui
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
packages/go/analysis/azure/post.go (3)
1005-1012: Impossible AND of multipleKindfiltersA node cannot simultaneously be
Role,GlobalAdmin, andPrivilegedRoleAdmin.
Previous review already pointed this out. The correct filter is byRoleTemplateIDOR.- query.Kind(query.Node(), azure.Role), - query.Kind(query.Node(), azure.GlobalAdmin), - query.Kind(query.Node(), azure.PrivilegedRoleAdmin), + query.Kind(query.Node(), azure.Role), + query.Or( + query.Equals(query.NodeProperty(azure.RoleTemplateID.String()), azure.CompanyAdministratorRole), + query.Equals(query.NodeProperty(azure.RoleTemplateID.String()), azure.PrivilegedRoleAdministratorRole), + ),
1063-1069: Second occurrence of niloutCSame issue as lines 1018-1025. Edge is never scheduled.
938-944: 🛠️ Refactor suggestion
⚠️ Potential issueMissing
analysis.NewPostRelationshipOperation– no jobs ever leave this functionEvery other post-processing routine in this file creates an
analysis.NewPostRelationshipOperation(seeAppRoleAssignments,ExecuteCommand, etc.).
Without anoperation, noSubmitReaderpipe exists and the channels on which you attempt to enqueue jobs are nil (see lines 1019 and 1064). Consequently:
- No edges are written.
processingStatsis never mutated.- The caller will believe post-processing succeeded while the graph is unchanged.
Refactor to the established pattern:
-func CreateAZRoleApproverEdge(ctx context.Context, db graph.Database) (*analysis.AtomicPostProcessingStats, error) { - processingStats := &analysis.AtomicPostProcessingStats{} +func CreateAZRoleApproverEdge(ctx context.Context, db graph.Database) (*analysis.AtomicPostProcessingStats, error) { + operation := analysis.NewPostRelationshipOperation(ctx, db, "Azure Role Approver Post Processing")…and replace all manual
db.ReadTransactionsections withoperation.Operation.SubmitReader(...), then return&operation.Stats, operation.Done().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/go/analysis/azure/post.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (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 (1)
packages/go/analysis/azure/post.go (1)
957-978:⚠️ Potential issueFilter for approval-required roles ignores
EndUserAssignmentUserApproversThe spec requires “at least one of the user or group approver lists is non-null”.
The current criteria only check for GroupApprovers.Add the missing predicate:
- query.Or( - query.IsNotNull(query.NodeProperty(azure.EndUserAssignmentUserApprovers.String())), - query.IsNotNull(query.NodeProperty(azure.EndUserAssignmentGroupApprovers.String())), - ), + query.Or( + query.IsNotNull(query.NodeProperty(azure.EndUserAssignmentUserApprovers.String())), + query.IsNotNull(query.NodeProperty(azure.EndUserAssignmentGroupApprovers.String())), + ),Later logic must iterate over both slices, not just the group one (see next comment).
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/go/analysis/azure/post.go (5)
1011-1018: Impossible role query – node cannot beRoleandGlobalAdminandPrivilegedRoleAdminSame issue raised in previous review – a node cannot hold three distinct kinds simultaneously.
Switch to a single
azure.Rolekind check and OR onRoleTemplateIDvalues instead.
1025-1031:outCis nil – edge creation never reaches the workerDeclaring
var outC chan<- analysis.CreatePostRelationshipJobwithout initialising it means everychannels.Submitcall is discarded (it immediately returnsfalse).This will be fixed automatically once the function is refactored to use
operation.Operation.SubmitReader(...).
1043-1064: Node creation inside a read-only transaction violates the contract
db.ReadTransactionforbids writes. Callingtx.CreateNodehere can panic or be silently ignored depending on backend.Move the node-creation logic into a writer obtained inside
operation.Operation.SubmitReader(...)or emit aCreateNodeJob.
1070-1075: Second occurrence of niloutC(see earlier comment)Edge submission inside the approver loop also uses an uninitialised channel.
937-945: 🛠️ Refactor suggestionPost-processing operation is never initialised – stats stay empty and edges cannot be emitted
processingStatsis created manually but you never spin up ananalysis.NewPostRelationshipOperation.
Consequences:
processingStatsis never mutated – callers see zero work done.- All subsequent
channels.Submitcalls target a nil channel (see comments below), becauseoutCis only handed out by the operation.Refactor along the same pattern used by
AppRoleAssignmentsorExecuteCommand:-func CreateAZRoleApproverEdge(ctx context.Context, db graph.Database) (*analysis.AtomicPostProcessingStats, error) { - processingStats := &analysis.AtomicPostProcessingStats{} +func CreateAZRoleApproverEdge(ctx context.Context, db graph.Database) (*analysis.AtomicPostProcessingStats, error) { + operation := analysis.NewPostRelationshipOperation(ctx, db, "AZRoleApprover Post Processing")…and return
&operation.Stats, operation.Done()at the end.
All edge creation must then happen insideoperation.Operation.SubmitReader(...)closures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/go/analysis/azure/post.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
🔇 Additional comments (1)
packages/go/analysis/azure/post.go (1)
959-977: Possible tenant-ID comparison mismatch
query.Equals(query.NodeProperty(azure.TenantID.String()), tenantID)compares a property to a raw string.
If the query builder expects agraph.PropertyValue, the comparison will never match, silently returning zero roles.Please confirm the expected type; if a
PropertyValueis required, wrap the string:query.Equals(query.NodeProperty(azure.TenantID.String()), graph.StringProperty(tenantID)),
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/go/analysis/azure/azrole/approver.go (3)
17-17: Add a meaningful package-level doc comment
// CreateApproverEdge ...does not describe behaviour or expectations. Replace with a GoDoc-style sentence so the exported symbol is discoverable ingo doc.
93-93: RenameprincipalIDparameterThe argument actually carries the tenant ID, not a principal ID. Misnaming
confuses readers and future maintainers.-func handleDefaultAdminRoles(ctx context.Context, tx graph.Transaction, outC chan<- analysis.CreatePostRelationshipJob, principalID string, fetchedAZRole *graph.Node) error { +func handleDefaultAdminRoles(ctx context.Context, tx graph.Transaction, outC chan<- analysis.CreatePostRelationshipJob, tenantID string, fetchedAZRole *graph.Node) error {
120-148: Batch principal lookup to cut N round-trips
handlePrincipalApproversperforms one DB query per principal ID. For large
approver lists this becomes O(n) round-trips. Consider fetching all at once:nodeSet, err := ops.FetchNodeSet(tx.Nodes().Filterf(func() graph.Criteria { return query.And( query.Kind(query.Node(), azure.Entity), query.In(query.NodeProperty(common.ObjectID.String()), principalIDs...), ) }))Then iterate the result map for edge creation and log missing IDs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/go/analysis/azure/azrole/approver.go(1 hunks)packages/go/analysis/azure/post.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/go/analysis/azure/post.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/go/analysis/azure/azrole/approver.go (2)
66-92: Loop variable captured - all edges may reference the last roleThe closure passed to
SubmitReadercapturesfetchedAZRolefrom the outerforloop. Because the loop variable is reused, every asynchronous reader may see the value from the final iteration.Apply this fix to capture the loop variable correctly:
-for _, fetchedAZRole := range fetchedAZRoles { +for _, role := range fetchedAZRoles { + role := role // capture loop variable if err := operation.Operation.SubmitReader(func(ctx context.Context, tx graph.Transaction, outC chan<- analysis.CreatePostRelationshipJob) error { - userApproversID, err := fetchedAZRole.Properties.Get( + userApproversID, err := role.Properties.Get( azure.EndUserAssignmentUserApprovers.String(), ).StringSlice() if err != nil { return err } - groupApproversID, err := fetchedAZRole.Properties.Get( + groupApproversID, err := role.Properties.Get( azure.EndUserAssignmentGroupApprovers.String(), ).StringSlice() if err != nil { return err } principalIDs := append(userApproversID, groupApproversID...) if len(principalIDs) == 0 { - return handleDefaultAdminRoles(ctx, db, tx, outC, tenantID, fetchedAZRole) + return handleDefaultAdminRoles(ctx, db, tx, outC, tenantID, role) } else { - return handlePrincipalApprovers(ctx, db, tx, outC, principalIDs, fetchedAZRole) + return handlePrincipalApprovers(ctx, db, tx, outC, principalIDs, role) } }); err != nil { return err } }
101-108: Query can never match: a node cannot satisfy multipleKindpredicates
query.Kind(query.Node(), azure.Role)is combined with anORthat checksazure.GlobalAdminandazure.PrivilegedRoleAdmin. A node can only have one kind, so the outerKind(Role)makes the OR branch unreachable.Remove the conflicting
Kind(Role)predicate:return query.And( query.Equals(query.NodeProperty(azure.TenantID.String()), tenantID), - query.Kind(query.Node(), azure.Role), query.Or( query.Kind(query.Node(), azure.GlobalAdmin), query.Kind(query.Node(), azure.PrivilegedRoleAdmin), ), )
🧹 Nitpick comments (1)
packages/go/analysis/azure/azrole/approver.go (1)
155-156: Merge variable declaration with assignmentThe variable declaration and assignment can be combined for cleaner code.
-var nodeID graph.ID -nodeID = fetchedNode.ID +nodeID := fetchedNode.ID
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/go/analysis/azure/azrole/approver.go(1 hunks)packages/go/analysis/azure/post.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/go/analysis/azure/post.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
packages/go/analysis/azure/azrole/approver.go
[error] 155-155: S1021: should merge variable declaration with assignment on next line
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (2)
packages/go/analysis/azure/azrole/approver.go (2)
17-31: LGTM - Proper tenant property extractionThe function correctly extracts both
tenantIDandtenantObjectIDfrom the tenant node properties with appropriate error handling.
131-167: LGTM - Robust principal approver handlingThe function properly handles entity node lookup with appropriate error handling, including graceful handling of missing nodes with warning logs and context cancellation support.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/go/analysis/azure/role_approver.go (2)
140-154: Performance concern: Multiple database transactions for principal lookupEach principal ID requires a separate database transaction, which could be inefficient for roles with many approvers. Consider batching the lookups into a single transaction.
Optimize by fetching all principals in a single transaction:
+var fetchedNodes map[string]*graph.Node +err := db.ReadTransaction(ctx, func(tx graph.Transaction) error { + nodes, err := ops.FetchNodeSet(tx.Nodes().Filterf(func() graph.Criteria { + return query.And( + query.Kind(query.Node(), azure.Entity), + query.In(query.NodeProperty(common.ObjectID.String()), principalIDs), + ) + })) + if err != nil { + return err + } + + fetchedNodes = make(map[string]*graph.Node) + for _, node := range nodes { + if objID, err := node.Properties.Get(common.ObjectID.String()).String(); err == nil { + fetchedNodes[objID] = node + } + } + return nil +}) +if err != nil { + return err +} + for _, principalID := range principalIDs { - var fetchedNode *graph.Node - err := db.ReadTransaction(ctx, func(tx graph.Transaction) error { - // ... transaction code - }) - if err != nil { - if graph.IsErrNotFound(err) { - slog.WarnContext(...) - continue - } else { - return err - } - } + fetchedNode, exists := fetchedNodes[principalID] + if !exists { + slog.WarnContext( + ctx, + fmt.Sprintf( + "Entity node not found for principal ID: %s, skipping edge creation", + principalID, + ), + ) + continue + }
170-176: Channel submission failure handling should be documentedThe function returns
nilwhenchannels.Submitreturnsfalse, which suggests the channel is closed or full. This behavior should be documented as it exits the loop early, potentially leaving some edges uncreated.Add a comment explaining the early return behavior:
if !channels.Submit(ctx, outC, analysis.CreatePostRelationshipJob{ FromID: fetchedNode.ID, ToID: fetchedAZRole.ID, Kind: azure.AZRoleApprover, }) { + // Channel is closed or context cancelled, stop processing return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/go/analysis/azure/post.go(1 hunks)packages/go/analysis/azure/role_approver.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/go/analysis/azure/post.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: build-ui
🔇 Additional comments (3)
packages/go/analysis/azure/role_approver.go (3)
1-15: LGTM: Clean package structure and importsThe package declaration and imports are well-organized and appropriate for the Azure analysis functionality.
17-23: LGTM: Well-structured function signatureThe function signature is well-designed with proper context handling, database interface, and operation tracking. The parameters are appropriately typed and named.
105-110: Verify TenantRoles function availabilityThe code calls
TenantRolesfunction which is not defined in this file. Ensure this function is properly imported and available in the azure package.#!/bin/bash # Description: Verify TenantRoles function exists and is accessible # Expected: Find the TenantRoles function definition in the azure package ast-grep --pattern $'func TenantRoles($$$) ($$$) { $$$ }'
…tions Refactor CreateAZRoleApproverEdge to return (*analysis.AtomicPostProcessingStats, error) instead of (graph.NodeSet, error), following the established pattern of other post-processing functions like AppRoleAssignments and ExecuteCommand.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/go/analysis/azure/post.go(2 hunks)packages/go/analysis/azure/role_approver.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/go/analysis/azure/role_approver.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
packages/go/analysis/azure/post.go
[error] 26-26: other declaration of cardinality
(typecheck)
[error] 27-27: other declaration of graph
(typecheck)
[error] 28-28: other declaration of ops
(typecheck)
[error] 29-29: other declaration of query
(typecheck)
[error] 30-30: other declaration of channels
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-tests
- GitHub Check: run-analysis
🔇 Additional comments (1)
packages/go/analysis/azure/post.go (1)
938-964: Well-structured post-processing functionThe function correctly implements the post-processing pattern used throughout this file and addresses the key concerns from previous reviews:
- ✅ Proper return type
(*analysis.AtomicPostProcessingStats, error)- ✅ Uses
analysis.NewPostRelationshipOperationframework- ✅ Follows established error handling patterns
- ✅ Good separation of concerns by delegating to
CreateApproverEdgeThe implementation is consistent with other post-processing functions like
AppRoleAssignments()andExecuteCommand().
Description
Describe your changes in detail
Motivation and Context
Resolves BED-5656
Why is this change required? What problem does it solve?
How Has This Been Tested?
This was tested locally by ingesting data from our SpecterOps Test environment and comparing the results before/and after post processing was added
With just ingest and no post processing:
After post processing was ran, we can see the new
AZRoleApproverrelationships created between theAZRolenodesScreenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit