feat(graphschema): Add Environment principal kinds BED-7076 #2209
Conversation
WalkthroughThis PR adds transaction support and comprehensive CRUD operations for schema relationship findings, remediations, and environment principal kinds. It introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
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 |
cweidenkeller
force-pushed
the
BED-7076
branch
from
01019ad to
b09718c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/api/src/database/graphschema.go (2)
619-648: CreateRemediations lacks duplicate key handling.Unlike
CreateSchemaRelationshipFindingwhich maps duplicate key errors to a specific error type,CreateRemediationsdoesn't handle the case where remediations for the samefinding_idandcontent_typealready exist. This could result in a raw database error being returned to callers.Consider whether this method should:
- Return a specific duplicate error like other create methods
- Use the upsert pattern from
UpdateRemediationsinstead🔎 Option: Add duplicate key error handling
if result := s.db.WithContext(ctx).Raw(` WITH inserted AS ( INSERT INTO schema_remediations (finding_id, content_type, content) VALUES (?, 'short_description', ?), (?, 'long_description', ?), (?, 'short_remediation', ?), (?, 'long_remediation', ?) RETURNING finding_id, content_type, content ) SELECT finding_id, MAX(content) FILTER (WHERE content_type = 'short_description') as short_description, MAX(content) FILTER (WHERE content_type = 'long_description') as long_description, MAX(content) FILTER (WHERE content_type = 'short_remediation') as short_remediation, MAX(content) FILTER (WHERE content_type = 'long_remediation') as long_remediation FROM inserted GROUP BY finding_id`, findingId, shortDescription, findingId, longDescription, findingId, shortRemediation, findingId, longRemediation).Scan(&remediations); result.Error != nil { + if strings.Contains(result.Error.Error(), DuplicateKeyValueErrorString) { + return model.Remediations{}, fmt.Errorf("remediations already exist for finding: %w", result.Error) + } return model.Remediations{}, CheckError(result) }
714-726: CreateSchemaEnvironmentPrincipalKind lacks duplicate key handling.Similar to
CreateRemediations, this method doesn't handle duplicate key violations. Given the learning thatprincipal_kindis a primary key enforcing global uniqueness, attempting to create the same principal kind for a different environment would fail with a raw database error.🔎 Add duplicate key error handling
func (s *BloodhoundDB) CreateSchemaEnvironmentPrincipalKind(ctx context.Context, environmentId int32, principalKind int32) (model.SchemaEnvironmentPrincipalKind, error) { var envPrincipalKind model.SchemaEnvironmentPrincipalKind if result := s.db.WithContext(ctx).Raw(` INSERT INTO schema_environments_principal_kinds (environment_id, principal_kind) VALUES (?, ?) RETURNING environment_id, principal_kind`, environmentId, principalKind).Scan(&envPrincipalKind); result.Error != nil { + if strings.Contains(result.Error.Error(), DuplicateKeyValueErrorString) { + return model.SchemaEnvironmentPrincipalKind{}, fmt.Errorf("principal kind already exists: %w", result.Error) + } return model.SchemaEnvironmentPrincipalKind{}, CheckError(result) } return envPrincipalKind, nil }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/api/src/database/database_integration_test.gocmd/api/src/database/db.gocmd/api/src/database/graphschema.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/mocks/db.gocmd/api/src/model/graphschema.go
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 2105
File: cmd/api/src/database/migration/migrations/v8.5.0.sql:139-143
Timestamp: 2025-12-10T20:16:54.652Z
Learning: In the BloodHound schema_environments_principal_kinds table (cmd/api/src/database/migration/migrations/v8.5.0.sql), the PRIMARY KEY(principal_kind) design is intentional and enforces that each principal kind is globally unique and exclusively owned by a single environment. This creates a one-to-many relationship from environments to principal kinds where an environment can have multiple principal kinds, but a principal kind can never be shared between environments.
📚 Learning: 2025-11-25T22:11:53.518Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 2107
File: cmd/api/src/database/graphschema.go:86-100
Timestamp: 2025-11-25T22:11:53.518Z
Learning: In cmd/api/src/database/graphschema.go, the CreateSchemaEdgeKind method intentionally does not use AuditableTransaction or audit logging because it would create too much noise in the audit log, unlike CreateGraphSchemaExtension which does use auditing.
Applied to files:
cmd/api/src/database/graphschema_integration_test.gocmd/api/src/model/graphschema.gocmd/api/src/database/db.gocmd/api/src/database/graphschema.gocmd/api/src/database/mocks/db.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 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.
Applied to files:
cmd/api/src/database/graphschema_integration_test.gocmd/api/src/model/graphschema.gocmd/api/src/database/graphschema.go
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/database_integration_test.go
📚 Learning: 2025-12-10T20:16:54.652Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 2105
File: cmd/api/src/database/migration/migrations/v8.5.0.sql:139-143
Timestamp: 2025-12-10T20:16:54.652Z
Learning: In the BloodHound schema_environments_principal_kinds table (cmd/api/src/database/migration/migrations/v8.5.0.sql), the PRIMARY KEY(principal_kind) design is intentional and enforces that each principal kind is globally unique and exclusively owned by a single environment. This creates a one-to-many relationship from environments to principal kinds where an environment can have multiple principal kinds, but a principal kind can never be shared between environments.
Applied to files:
cmd/api/src/database/graphschema_integration_test.go
📚 Learning: 2025-08-12T15:30:00.877Z
Learnt from: bsheth711
Repo: SpecterOps/BloodHound PR: 1766
File: cmd/api/src/database/access_control_list.go:55-103
Timestamp: 2025-08-12T15:30:00.877Z
Learning: In BloodHound's database layer, s.UpdateUser uses nested transactions within GORM. When called inside an AuditableTransaction, error propagation from the nested transaction causes rollback of the parent transaction, maintaining atomicity across the entire operation.
Applied to files:
cmd/api/src/database/database_integration_test.gocmd/api/src/database/db.go
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
cmd/api/src/database/db.go
📚 Learning: 2025-05-29T18:24:23.227Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:24:23.227Z
Learning: In the stbernard package, use slog for all logging prints instead of fmt.Printf/fmt.Fprintf to maintain consistency with the codebase's logging standards.
Applied to files:
cmd/api/src/database/db.go
📚 Learning: 2025-09-02T16:46:30.895Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: cmd/api/src/api/v2/auth/auth.go:559-573
Timestamp: 2025-09-02T16:46:30.895Z
Learning: In the BloodHound codebase, when updating user ETAC (Environment Access Control) lists in the UpdateUser function, the approach is to let GORM handle the creation/persistence of the environment access records through model associations rather than using explicit database helper methods like UpdateEnvironmentListForUser.
Applied to files:
cmd/api/src/database/graphschema.go
🧬 Code graph analysis (3)
cmd/api/src/database/database_integration_test.go (2)
packages/go/graphschema/common/common.go (1)
Enabled(64-64)cmd/api/src/database/db.go (1)
BloodhoundDB(194-197)
cmd/api/src/database/graphschema.go (3)
cmd/api/src/model/graphschema.go (9)
SchemaEnvironment(101-106)SchemaEnvironment(108-110)SchemaRelationshipFinding(113-121)SchemaRelationshipFinding(123-125)Remediations(127-133)Remediations(135-137)SchemaEnvironmentPrincipalKind(141-144)SchemaEnvironmentPrincipalKind(146-148)SchemaEnvironmentPrincipalKinds(139-139)cmd/api/src/database/helper.go (1)
CheckError(39-45)cmd/api/src/database/db.go (2)
ErrNotFound(42-42)ErrDuplicateSchemaRelationshipFindingName(61-61)
cmd/api/src/database/mocks/db.go (1)
cmd/api/src/model/graphschema.go (9)
Remediations(127-133)Remediations(135-137)SchemaEnvironmentPrincipalKind(141-144)SchemaEnvironmentPrincipalKind(146-148)SchemaRelationshipFinding(113-121)SchemaRelationshipFinding(123-125)SchemaEnvironment(101-106)SchemaEnvironment(108-110)SchemaEnvironmentPrincipalKinds(139-139)
⏰ 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: build-ui
- GitHub Check: run-tests
- GitHub Check: run-analysis
🔇 Additional comments (12)
cmd/api/src/database/db.go (1)
229-239: Well-designed transaction support with proper identity resolver propagation.The
Transactionmethod correctly wraps GORM's transaction handling while preserving theidResolverin the transaction-scopedBloodhoundDBinstance. This ensures that all existing methods can participate in transactions without modification.cmd/api/src/database/database_integration_test.go (1)
120-238: Comprehensive transaction tests with good isolation and coverage.The test suite effectively covers commit/rollback semantics, nested method calls, isolation levels, and read-only transactions. Each subtest properly creates its own isolated database instance, ensuring test independence.
cmd/api/src/database/graphschema_integration_test.go (3)
2364-2417: Excellent cascade deletion test covering the full dependency chain.This test comprehensively verifies that deleting a schema extension cascades to all dependent entities: node kinds, properties, edge kinds, environments, relationship findings, remediations, and principal kinds. This is critical for maintaining referential integrity.
1482-1583: Thorough transaction tests for schema environment operations.The tests effectively validate multi-operation commits, rollbacks on application errors, rollbacks on database constraint violations, and create-then-delete within a single transaction. This provides strong confidence in transactional integrity.
2038-2070: Verify upsert behavior aligns with intended semantics.The "upsert creates new remediations" test case for
UpdateRemediationsdemonstrates that calling update without an existing record creates one. Confirm this is the intended behavior, as some APIs distinguish between create and update operations.cmd/api/src/model/graphschema.go (3)
112-125: Model correctly defined for relationship findings.The
SchemaRelationshipFindingstruct has appropriate field mappings and JSON tags. TheTableName()method correctly maps toschema_relationship_findings.
127-137: Remediations model uses aggregated view pattern.The
Remediationsstruct represents an aggregated view of multiple rows inschema_remediations(keyed byfinding_idandcontent_type). This design allows storing different content types as separate rows while presenting a unified struct to the application layer.
139-148: Principal kinds model aligns with intentional schema design.The
SchemaEnvironmentPrincipalKindstructure correctly reflects the schema whereprincipal_kindis globally unique across all environments. Based on learnings, thePRIMARY KEY(principal_kind)design intentionally enforces that each principal kind is exclusively owned by a single environment.cmd/api/src/database/graphschema.go (3)
541-556: Consistent pattern for GetSchemaEnvironmentById.The method correctly uses
.Scan()withRowsAffectedcheck for not-found handling, consistent with other methods in this file.
672-702: UpdateRemediations implements proper upsert semantics.The
ON CONFLICT DO UPDATEclause correctly handles both update and insert cases. The CTE pattern atomically performs the upsert and returns the aggregated result.
55-70: Interface extension is well-organized and follows existing patterns.The new interface methods are grouped logically by entity type and follow the existing naming conventions (Create/Get/Delete patterns).
cmd/api/src/database/mocks/db.go (1)
464-477: LGTM! Auto-generated mocks are correctly structured.All 12 new mock methods (CreateRemediations, CreateSchemaEnvironmentPrincipalKind, CreateSchemaRelationshipFinding, DeleteRemediations, DeleteSchemaEnvironment, DeleteSchemaEnvironmentPrincipalKind, DeleteSchemaRelationshipFinding, GetRemediationsByFindingId, GetSchemaEnvironmentById, GetSchemaEnvironmentPrincipalKindsByEnvironmentId, GetSchemaRelationshipFindingById, UpdateRemediations) follow the standard gomock patterns and match the corresponding model types.
Also applies to: 588-616, 929-941, 990-1030, 2010-2023, 2175-2203, 2220-2233, 2986-2999
Description
Describe your changes in detail
This adds Env principal kinds table for the extensions feature
Motivation and Context
Resolves 7076
Why is this change required? What problem does it solve?
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Added integration tests
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.