feat: Available Domains updated to support OG Environments: BED-6761 #2269
Conversation
WalkthroughThis pull request refactors domain handling to support generic environments. It renames Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIHandler as ListAvailableEnvironments<br/>Handler
participant DB as Database
participant Graph as Graph Engine
Client->>APIHandler: GET /api/v2/available-environments
APIHandler->>DB: GetEnvironments(ctx)
DB-->>APIHandler: []SchemaEnvironment
APIHandler->>DB: GetFlagByKey(ctx, "opengraph_findings")
DB-->>APIHandler: FeatureFlag
APIHandler->>APIHandler: BuildEnvironmentFilter(ctx, db, request)
APIHandler->>APIHandler: Parse sort parameters with EnvironmentSelectors
APIHandler->>Graph: GraphQuery(filter criteria)
Graph-->>APIHandler: []*graph.Node
APIHandler->>APIHandler: BuildEnvironmentSelectors(nodes, kindToDisplayName)
APIHandler->>APIHandler: Resolve type & collected status per node
APIHandler-->>Client: JSON array of EnvironmentSelectors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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
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: 2
🤖 Fix all issues with AI agents
In `@cmd/api/src/api/v2/search.go`:
- Around line 150-168: The resolveEnvType function currently returns an empty
string when no kind matches, which can break consumers; update resolveEnvType to
return a stable fallback (e.g., "unknown") instead of "" and emit a warning via
the existing logger or a passed-in logger if available; locate resolveEnvType
and modify its final return path (after checking node.Kinds and
kindToDisplayName) to log a warning mentioning node.ID or node.Kinds and then
return "unknown" so API/UI consumers always receive a non-empty environment
type.
In `@cmd/api/src/database/graphschema.go`:
- Around line 869-894: Delete the dead code: remove the Kind struct declaration
and the GetKindByName method on BloodhoundDB (including the TODO comment above
it) since they are unused; ensure no remaining references to Kind or
GetKindByName remain (and remove any now-unused ErrNotFound or other
imports/vars only referenced by that method) and run tests/compile to confirm
nothing else depended on these symbols.
🧹 Nitpick comments (4)
cmd/api/src/model/search_test.go (1)
30-34: Consider renaming test functions to match type name.The test function names retain
DomainSelectorsprefix (e.g.,TestDomainSelectors_TestIsSortable) while the type is nowEnvironmentSelectors. Consider renaming for consistency—e.g.,TestEnvironmentSelectors_IsSortable.cmd/api/src/api/v2/search_test.go (1)
236-249: Consider adding a test case forFeatureOpenGraphFindingsenabled.All test cases mock
FeatureOpenGraphFindingsas disabled. Consider adding a test case where this flag is enabled to verify the alternate code path behavior.Also applies to: 268-269, 298-299
cmd/api/src/model/search.go (1)
89-95: Consider using the sharedGetFilterableColumnshelper fromcmd/api/src/api/filters.go.The relevant code snippets show there's already a
GetFilterableColumnsfunction incmd/api/src/api/filters.gothat performs the same logic. This implementation duplicates that code. However, sinceEnvironmentSelectorsmay need to implement a specific interface, this might be intentional.cmd/api/src/api/v2/search.go (1)
139-148: Consider adding a comment explaining the rationale for defaulting OpenGraph extensions tocollected=true.The logic is clear for built-ins (respect the stored property), but the decision to default OpenGraph extensions to
truemay warrant a brief explanation beyond "for now" to help future maintainers understand the business requirement.
| func resolveEnvType(node *graph.Node, kindToDisplayName map[string]string) string { | ||
| // TODO: Remove hardcoded built-in types once they are saved in DB and not CUE | ||
| if node.Kinds.ContainsOneOf(azure.Tenant) { | ||
| return "azure" | ||
| } | ||
| if node.Kinds.ContainsOneOf(ad.Domain) { | ||
| return "active-directory" | ||
| } | ||
|
|
||
| // For custom extensions, use the display name from the schema extension | ||
| // Note: Nodes should only have one environment kind. In the edge case where there are multiple, we take the first. | ||
| for _, kind := range node.Kinds { | ||
| if displayName, ok := kindToDisplayName[kind.String()]; ok { | ||
| return displayName | ||
| } | ||
| } | ||
|
|
||
| return "" | ||
| } |
There was a problem hiding this comment.
Empty string fallback for unknown environment types may cause UI/API issues.
When a node doesn't match any known kind in kindToDisplayName, the function returns an empty string. This could result in confusing API responses where type is blank. Consider logging a warning or returning a fallback like "unknown".
Suggested improvement
// For custom extensions, use the display name from the schema extension
// Note: Nodes should only have one environment kind. In the edge case where there are multiple, we take the first.
for _, kind := range node.Kinds {
if displayName, ok := kindToDisplayName[kind.String()]; ok {
return displayName
}
}
- return ""
+ return "unknown"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func resolveEnvType(node *graph.Node, kindToDisplayName map[string]string) string { | |
| // TODO: Remove hardcoded built-in types once they are saved in DB and not CUE | |
| if node.Kinds.ContainsOneOf(azure.Tenant) { | |
| return "azure" | |
| } | |
| if node.Kinds.ContainsOneOf(ad.Domain) { | |
| return "active-directory" | |
| } | |
| // For custom extensions, use the display name from the schema extension | |
| // Note: Nodes should only have one environment kind. In the edge case where there are multiple, we take the first. | |
| for _, kind := range node.Kinds { | |
| if displayName, ok := kindToDisplayName[kind.String()]; ok { | |
| return displayName | |
| } | |
| } | |
| return "" | |
| } | |
| func resolveEnvType(node *graph.Node, kindToDisplayName map[string]string) string { | |
| // TODO: Remove hardcoded built-in types once they are saved in DB and not CUE | |
| if node.Kinds.ContainsOneOf(azure.Tenant) { | |
| return "azure" | |
| } | |
| if node.Kinds.ContainsOneOf(ad.Domain) { | |
| return "active-directory" | |
| } | |
| // For custom extensions, use the display name from the schema extension | |
| // Note: Nodes should only have one environment kind. In the edge case where there are multiple, we take the first. | |
| for _, kind := range node.Kinds { | |
| if displayName, ok := kindToDisplayName[kind.String()]; ok { | |
| return displayName | |
| } | |
| } | |
| return "unknown" | |
| } |
🤖 Prompt for AI Agents
In `@cmd/api/src/api/v2/search.go` around lines 150 - 168, The resolveEnvType
function currently returns an empty string when no kind matches, which can break
consumers; update resolveEnvType to return a stable fallback (e.g., "unknown")
instead of "" and emit a warning via the existing logger or a passed-in logger
if available; locate resolveEnvType and modify its final return path (after
checking node.Kinds and kindToDisplayName) to log a warning mentioning node.ID
or node.Kinds and then return "unknown" so API/UI consumers always receive a
non-empty environment type.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/api/src/api/v2/search.go`:
- Around line 86-116: The error branch in ListAvailableEnvironments is inverted:
when BuildEnvironmentFilter returns database.ErrNotFound treat it as a
server/configuration issue (respond with an internal server error or not-found
status) instead of calling api.HandleDatabaseError for other errors; update the
logic in ListAvailableEnvironments so that errors.Is(err, database.ErrNotFound)
triggers api.WriteErrorResponse with http.StatusInternalServerError (including
err.Error() in the message) and all other filter errors return
http.StatusBadRequest via api.WriteErrorResponse; reference
BuildEnvironmentFilter, database.ErrNotFound, ListAvailableEnvironments,
api.WriteErrorResponse and ensure the response uses the actual error text for
debugging.
♻️ Duplicate comments (1)
cmd/api/src/api/v2/search.go (1)
150-168: Empty string fallback for unknown environment types may cause issues.When a node doesn't match any known kind in
kindToDisplayName, the function returns an empty string. This could result in confusing API responses wheretypeis blank. Consider returning a fallback like"unknown"or logging a warning.Suggested improvement
// For custom extensions, use the display name from the schema extension // Note: Nodes should only have one environment kind. In the edge case where there are multiple, we take the first. for _, kind := range node.Kinds { if displayName, ok := kindToDisplayName[kind.String()]; ok { return displayName } } - return "" + return "unknown"
🧹 Nitpick comments (2)
cmd/api/src/api/v2/search_test.go (1)
234-243: Consider adding a test case forGetEnvironmentsdatabase error.The current error test only covers
GetFilteredAndSortedNodesfailure. A test case for whenGetEnvironmentsreturns an error would improve coverage of the error handling path inBuildEnvironmentFilter.Suggested test case
{ Name: "GetEnvironmentsError", Setup: func() { mockDB.EXPECT().GetEnvironments(gomock.Any()).Return(nil, fmt.Errorf("database error")) }, Test: func(output apitest.Output) { apitest.StatusCode(output, http.StatusBadRequest) }, },cmd/api/src/api/v2/search.go (1)
139-148: Clarify the rationale for OpenGraph extensions defaulting tocollected: true.The comment says "all OpenGraph extensions default to true for now" but doesn't explain why. This behavior differs from built-ins which respect the actual
collectedproperty. Consider adding a brief explanation or TODO for future alignment.
| func (s *Resources) ListAvailableEnvironments(response http.ResponseWriter, request *http.Request) { | ||
| ctx := request.Context() | ||
|
|
||
| if sortItems, err := api.ParseGraphSortParameters(domains, request.URL.Query()); err != nil { | ||
| api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusBadRequest, api.ErrorResponseDetailsNotSortable, request), response) | ||
| } else if filterCriteria, err := domains.GetFilterCriteria(request); err != nil { | ||
| api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusBadRequest, err.Error(), request), response) | ||
| } else if nodes, err := s.GraphQuery.GetFilteredAndSortedNodes(sortItems, filterCriteria); err != nil { | ||
| api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, fmt.Sprintf("%s: %s", api.ErrorResponseDetailsInternalServerError, err), request), response) | ||
| } else { | ||
| api.WriteBasicResponse(request.Context(), setNodeProperties(nodes), http.StatusOK, response) | ||
| sortItems, err := api.ParseGraphSortParameters(model.EnvironmentSelectors{}, request.URL.Query()) | ||
| if err != nil { | ||
| api.WriteErrorResponse(ctx, api.BuildErrorResponse(http.StatusBadRequest, api.ErrorResponseDetailsNotSortable, request), response) | ||
| return | ||
| } | ||
|
|
||
| filterResult, err := BuildEnvironmentFilter(ctx, s.DB, request) | ||
| if err != nil { | ||
| if errors.Is(err, database.ErrNotFound) { | ||
| api.HandleDatabaseError(request, response, err) | ||
| } else { | ||
| api.WriteErrorResponse(ctx, api.BuildErrorResponse(http.StatusBadRequest, err.Error(), request), response) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| // Fetch and filter domain nodes | ||
| nodes, err := s.GraphQuery.GetFilteredAndSortedNodes(sortItems, filterResult.FilterCriteria) | ||
| if err != nil { | ||
| api.WriteErrorResponse(ctx, api.BuildErrorResponse(http.StatusInternalServerError, err.Error(), request), response) | ||
| return | ||
| } | ||
|
|
||
| // Build response with domain type display names | ||
| responseData := BuildEnvironmentSelectors(nodes, filterResult.KindToDisplayName) | ||
|
|
||
| api.WriteBasicResponse(ctx, responseData, http.StatusOK, response) | ||
| } |
There was a problem hiding this comment.
Error handling for ErrNotFound appears inverted.
When BuildEnvironmentFilter returns database.ErrNotFound, the code calls HandleDatabaseError, but for other errors it writes a StatusBadRequest. This seems backward - a database "not found" error during filter building typically indicates a configuration issue (not a client error), while filter parsing errors (like invalid query params) would be bad requests.
Consider whether the logic should be:
ErrNotFound→StatusInternalServerErrororStatusNotFound(server/config issue)- Other filter errors →
StatusBadRequest(client input issue)
🔧 Suggested fix
filterResult, err := BuildEnvironmentFilter(ctx, s.DB, request)
if err != nil {
- if errors.Is(err, database.ErrNotFound) {
- api.HandleDatabaseError(request, response, err)
- } else {
+ if errors.Is(err, database.ErrNotFound) {
+ // Environment configuration not found - server/config issue
api.WriteErrorResponse(ctx, api.BuildErrorResponse(http.StatusBadRequest, err.Error(), request), response)
+ } else {
+ api.HandleDatabaseError(request, response, err)
}
return
}🤖 Prompt for AI Agents
In `@cmd/api/src/api/v2/search.go` around lines 86 - 116, The error branch in
ListAvailableEnvironments is inverted: when BuildEnvironmentFilter returns
database.ErrNotFound treat it as a server/configuration issue (respond with an
internal server error or not-found status) instead of calling
api.HandleDatabaseError for other errors; update the logic in
ListAvailableEnvironments so that errors.Is(err, database.ErrNotFound) triggers
api.WriteErrorResponse with http.StatusInternalServerError (including
err.Error() in the message) and all other filter errors return
http.StatusBadRequest via api.WriteErrorResponse; reference
BuildEnvironmentFilter, database.ErrNotFound, ListAvailableEnvironments,
api.WriteErrorResponse and ensure the response uses the actual error text for
debugging.
brandonshearin
changed the title
Description
ListAvailableEnvironmentsresources handler lives. This function was previously calledGetAvailableDomainsand the verbiage has been updated from domains -> environments to better reflect its purpose.opengraph_findingsfeature flag, non user-updatable. When on,api/v2/available-domainswill return any environments registered through OpenGraph in addition to existing ad domains and azure tenants.model.SchemaEnvironmentresult of theGetEnvironmentsDB method with 2 virtual fields-- extension name and environment kind name.Motivation and Context
BED-6761
We want environments collected under a registered OpenGraph extension to be selectable in the UI. This requires updates to the existing
api/v2/available-domainsendpoint.How Has This Been Tested?
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.