feat(AGT): add filter support to GET /asset-group-tag/:id/members endpoint BED-6819 #2198
Conversation
WalkthroughAdds server-side validation and translation of query-parameter filters (name, object_id, primary_kind) for the AssetGroupTag members endpoint: new AssetGroupMember.ValidFilters(), parsing/validation in GetAssetGroupMembersByTag, graph-DB filter translation, tests, and OpenAPI parameter additions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "API Handler\nGetAssetGroupMembersByTag"
participant Validator as "AssetGroupMember.ValidFilters"
participant Translator as "Filter Translator\nbuildAssetGroupMembersByTagGraphDbFilters"
participant Graph as "GraphDB"
Client->>API: GET /asset-group-tags/{id}/members?name~=foo&primary_kind=eq:bar
API->>Validator: request valid filter map
Validator-->>API: allowed fields & operators
API->>API: parse & validate query filters → queryFilterMap
API->>Translator: translate queryFilterMap → graph.Criteria
Translator-->>API: graph.Criteria (Contains/Not wraps, combined)
API->>Graph: Query nodes with graph.Criteria + pagination
Graph-->>API: nodes + count
API-->>Client: 200 OK with members
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 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
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/go/openapi/doc/openapi.json (1)
5514-5546: New member filters look consistent with existing API surfaceThe added
primary_kind,name, andobject_idfilters correctly mirror the selector-members endpoint and use the standardapi.params.predicate.filter.stringschema, so the spec matches the new backend filter support.If you want to tighten the spec further, you could optionally mirror the
sort_byenum constraint used on/asset-group-tags/{asset_group_tag_id}/selectors/{asset_group_tag_selector_id}/membersso both member-listing endpoints advertise the same explicit sortable columns, but this is non-blocking.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/api/src/api/v2/assetgrouptags.go(3 hunks)cmd/api/src/api/v2/assetgrouptags_test.go(1 hunks)packages/go/openapi/doc/openapi.json(1 hunks)packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
📚 Learning: 2025-07-09T00:36:54.112Z
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
Applied to files:
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.yamlcmd/api/src/api/v2/assetgrouptags_test.gopackages/go/openapi/doc/openapi.jsoncmd/api/src/api/v2/assetgrouptags.go
📚 Learning: 2025-11-05T21:13:08.542Z
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Applied to files:
cmd/api/src/api/v2/assetgrouptags_test.gocmd/api/src/api/v2/assetgrouptags.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/api/v2/assetgrouptags_test.go
🧬 Code graph analysis (1)
cmd/api/src/api/v2/assetgrouptags_test.go (2)
cmd/api/src/api/constant.go (1)
URIPathVariableAssetGroupTagID(56-56)cmd/api/src/model/filter.go (1)
Equals(45-45)
⏰ 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: run-tests
- GitHub Check: build-ui
- GitHub Check: run-analysis
🔇 Additional comments (5)
packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.yaml (1)
45-62: LGTM! OpenAPI specification updated correctly.The three new query parameters (primary_kind, name, object_id) are well-defined with appropriate descriptions and schema references. The parameters are correctly marked as optional and use the predicate filter schema which supports the operators implemented in the backend.
cmd/api/src/api/v2/assetgrouptags_test.go (1)
2340-2420: LGTM! Comprehensive test coverage for the new filters.The three new test cases effectively validate the filtering functionality:
- primary_kind filter: Verifies kind-based filtering with equals operator
- name filter: Tests substring matching with the contains operator (~eq)
- object_id filter: Tests exclusion with the not-equals operator
Each test correctly constructs the expected graph query filter and verifies the appropriate GraphDB methods are invoked. The tests follow the established pattern and integrate well with the existing test suite.
cmd/api/src/api/v2/assetgrouptags.go (3)
828-834: LGTM! ValidFilters method correctly declares supported filters.The method appropriately specifies which operators are supported for each filter field:
nameandobject_idsupport Equals, NotEquals, and ApproximatelyEquals (for substring matching)primary_kindsupports only Equals and NotEquals (ApproximatelyEquals doesn't make sense for node kinds)This aligns well with the OpenAPI specification and the filter building logic.
881-881: LGTM! Filter validation follows best practices.The query parameter parsing and validation logic correctly:
- Parses filters from the request using the standard filter parser
- Validates filter names against
AssetGroupMember.ValidFilters()- Validates operators against the allowed operators for each field
- Returns appropriate 400 errors for invalid filters with clear error messages
The early return pattern ensures invalid requests are rejected before any processing occurs.
Also applies to: 884-914
927-958: LGTM! Graph query filter building is well-implemented.The filter building logic correctly handles the three filter types:
object_id and name: Maps to graph node properties with appropriate operator handling:
ApproximatelyEquals→StringContains(substring match)Equals→Equals(exact match)primary_kind: Uses
query.Kind()which is the correct approach for filtering by graph node type (not a property)NotEquals: Handled uniformly for all fields by wrapping the predicate in
query.Not()The combination logic is intuitive:
- Multiple filters for the same field are ORed (e.g.,
?name=eq:foo&name=eq:bar→ name matches foo OR bar)- Filters for different fields are ANDed (e.g.,
?name=eq:foo&object_id=eq:123→ name matches foo AND object_id matches 123)The comment on Line 927 helpfully explains why a bespoke approach is needed for the graph query complexity.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/api/src/api/v2/assetgrouptags.go (2)
927-930: Good integration of the new filter logic.The graph filter building and integration is correct. The comment could optionally be expanded to clarify that filters within a field group are OR'd, while different fields are AND'd together.
947-983: Verify case sensitivity for approximate matching.The filter translation logic is well-structured and correctly implements OR within field groups and AND across different fields. The handling of
NotEqualsby wrapping criteria inquery.Not()is correct.However,
ApproximatelyEqualsusesquery.StringContains(lines 960, 967), which may be case-sensitive. In contrast,SearchAssetGroupTagsusesquery.CaseInsensitiveStringContains(line 1222) for similar fuzzy matching. Consider verifying whether case-sensitive matching is intentional or if consistency with the search behavior is desired.Optional: Add defensive default case
While the validation prevents unhandled filter names from reaching the switch statement, adding a default case would make the code more defensive against future modifications:
switch queryFilter.Name { case "object_id": ref = query.NodeProperty(common.ObjectID.String()) if queryFilter.Operator == model.ApproximatelyEquals { ref = query.StringContains(ref, queryFilter.Value) } else { ref = query.Equals(ref, queryFilter.Value) } case "name": ref = query.NodeProperty(common.Name.String()) if queryFilter.Operator == model.ApproximatelyEquals { ref = query.StringContains(ref, queryFilter.Value) } else { ref = query.Equals(ref, queryFilter.Value) } case "primary_kind": ref = query.Kind(query.Node(), graph.StringKind(queryFilter.Value)) + default: + // This should never be reached due to validation, but defend against future changes + continue }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/api/v2/assetgrouptags.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
📚 Learning: 2025-07-09T00:36:54.112Z
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
Applied to files:
cmd/api/src/api/v2/assetgrouptags.go
📚 Learning: 2025-11-05T21:13:08.542Z
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Applied to files:
cmd/api/src/api/v2/assetgrouptags.go
📚 Learning: 2025-05-27T17:28:48.241Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/graphify/tasks.go:68-80
Timestamp: 2025-05-27T17:28:48.241Z
Learning: In Go, avoid using pipelined if/else statements when creating resources that need cleanup (like files, network connections, etc.). The defer semantics work best when resources are created and immediately deferred for cleanup in the same scope. Instead, extract resource management into separate functions where proper defer patterns can be used effectively.
Applied to files:
cmd/api/src/api/v2/assetgrouptags.go
📚 Learning: 2025-05-27T17:28:46.899Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/graphify/tasks.go:114-118
Timestamp: 2025-05-27T17:28:46.899Z
Learning: When processing files in a loop where resources need to be cleaned up, extract the file processing into a separate function that uses `defer` for resource management rather than trying to handle cleanup in the loop body. This ensures proper resource cleanup while maintaining error handling robustness.
Applied to files:
cmd/api/src/api/v2/assetgrouptags.go
⏰ 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: run-analysis
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (2)
cmd/api/src/api/v2/assetgrouptags.go (2)
828-834: LGTM! Well-designed filter configuration.The
ValidFiltersmethod correctly defines the supported filters and their operators. The decision to excludeApproximatelyEqualsforprimary_kindis appropriate since kind matching should be exact.
884-914: LGTM! Thorough filter validation.The filter parsing and validation logic is well-structured:
- Validates filter names against
ValidFilters()- Ensures operators are supported for each filter
- Provides clear error messages for invalid filters
- Fails fast with appropriate 400 responses
ALCooper12
left a comment
There was a problem hiding this comment.
Just needs another commit of just pfc, otherwise LGTM!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/api/src/api/v2/assetgrouptags.go (2)
947-983: Consider adding a default case to the switch statement for robustness.The switch statement (lines 956-973) handles all currently defined filterable fields from
ValidFilters(). However, if someone later adds a new field toValidFilters()but forgets to update this switch,refwould remain nil and be appended toinnerFilters, leading to subtle bugs.🔎 Proposed defensive default case
switch queryFilter.Name { case "object_id": ref = query.NodeProperty(common.ObjectID.String()) if queryFilter.Operator == model.ApproximatelyEquals { ref = query.StringContains(ref, queryFilter.Value) } else { ref = query.Equals(ref, queryFilter.Value) } case "name": ref = query.NodeProperty(common.Name.String()) if queryFilter.Operator == model.ApproximatelyEquals { ref = query.StringContains(ref, queryFilter.Value) } else { ref = query.Equals(ref, queryFilter.Value) } case "primary_kind": ref = query.Kind(query.Node(), graph.StringKind(queryFilter.Value)) + default: + // This should never happen due to validation, but guard against future changes + continue }Alternatively, add a comment explaining why a default case is not needed:
case "primary_kind": ref = query.Kind(query.Node(), graph.StringKind(queryFilter.Value)) + // No default needed: validation in GetAssetGroupMembersByTag ensures + // only fields from ValidFilters() reach this code }
957-970: Optional: Consider reducing duplication between object_id and name cases.The cases for
object_id(lines 957-963) andname(lines 964-970) are identical except for the property name. While this is clear and maintainable as-is, you could optionally extract the common logic if the pattern grows to more fields.Possible refactoring approach
+ var propertyName string switch queryFilter.Name { - case "object_id": - ref = query.NodeProperty(common.ObjectID.String()) - if queryFilter.Operator == model.ApproximatelyEquals { - ref = query.StringContains(ref, queryFilter.Value) - } else { - ref = query.Equals(ref, queryFilter.Value) - } - case "name": - ref = query.NodeProperty(common.Name.String()) + case "object_id", "name": + if queryFilter.Name == "object_id" { + propertyName = common.ObjectID.String() + } else { + propertyName = common.Name.String() + } + ref = query.NodeProperty(propertyName) if queryFilter.Operator == model.ApproximatelyEquals { ref = query.StringContains(ref, queryFilter.Value) } else { ref = query.Equals(ref, queryFilter.Value) } case "primary_kind": ref = query.Kind(query.Node(), graph.StringKind(queryFilter.Value)) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/api/src/api/v2/assetgrouptags.gocmd/api/src/api/v2/assetgrouptags_test.gopackages/go/openapi/doc/openapi.jsonpackages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/api/src/api/v2/assetgrouptags_test.go
- packages/go/openapi/src/paths/asset-isolation.asset-group-tags.id.members.yaml
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
📚 Learning: 2025-07-09T00:36:54.112Z
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
Applied to files:
cmd/api/src/api/v2/assetgrouptags.gopackages/go/openapi/doc/openapi.json
📚 Learning: 2025-11-05T21:13:08.542Z
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Applied to files:
cmd/api/src/api/v2/assetgrouptags.go
📚 Learning: 2025-05-27T17:28:48.241Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/graphify/tasks.go:68-80
Timestamp: 2025-05-27T17:28:48.241Z
Learning: In Go, avoid using pipelined if/else statements when creating resources that need cleanup (like files, network connections, etc.). The defer semantics work best when resources are created and immediately deferred for cleanup in the same scope. Instead, extract resource management into separate functions where proper defer patterns can be used effectively.
Applied to files:
cmd/api/src/api/v2/assetgrouptags.go
📚 Learning: 2025-05-27T17:28:46.899Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/graphify/tasks.go:114-118
Timestamp: 2025-05-27T17:28:46.899Z
Learning: When processing files in a loop where resources need to be cleaned up, extract the file processing into a separate function that uses `defer` for resource management rather than trying to handle cleanup in the loop body. This ensures proper resource cleanup while maintaining error handling robustness.
Applied to files:
cmd/api/src/api/v2/assetgrouptags.go
🧬 Code graph analysis (1)
cmd/api/src/api/v2/assetgrouptags.go (5)
cmd/api/src/model/filter.go (7)
ValidFilters(486-486)FilterOperator(38-38)Equals(45-45)NotEquals(46-46)ApproximatelyEquals(47-47)QueryParameterFilterMap(151-151)NewQueryParameterFilterParser(473-477)cmd/api/src/api/error.go (4)
BuildErrorResponse(134-145)ErrorResponseDetailsBadQueryParameterFilters(43-43)ErrorResponseDetailsColumnNotFilterable(44-44)ErrorResponseDetailsFilterPredicateNotSupported(45-45)cmd/api/src/api/constant.go (1)
URIPathVariableAssetGroupTagID(56-56)packages/go/graphschema/ad/ad.go (1)
DomainSID(155-155)packages/go/graphschema/azure/azure.go (1)
TenantID(107-107)
⏰ 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). (3)
- GitHub Check: build-ui
- GitHub Check: run-tests
- GitHub Check: run-analysis
🔇 Additional comments (4)
packages/go/openapi/doc/openapi.json (1)
5521-5546: New member-filter query params look correct and consistent
primary_kind,name, andobject_idare added as optional string predicate filters and match the pattern used on the deprecated/asset-groups/{asset_group_id}/membersendpoint and the selector-based members endpoint (same names andapi.params.predicate.filter.stringschema). JSON structure is valid and the docs (“Filter by …”) are clear.cmd/api/src/api/v2/assetgrouptags.go (3)
828-834: LGTM! Well-structured filter definition.The ValidFilters method correctly defines the supported filter fields and their allowed operators. The restriction of
primary_kindto exact matching (Equals/NotEquals only) is appropriate since it represents discrete node types.
884-914: LGTM! Filter validation correctly implemented.The validation loop properly checks that filter fields are valid and operators are supported before building the queryFilterMap. The approach follows the established pattern used in other endpoints (e.g., GetAssetGroupTags). Note that
IsStringDatais correctly omitted here since these filters translate to graph criteria rather than SQL.
927-930: LGTM! Clean integration of graph filter translation.The conditional append ensures empty filter results don't affect the query, and the inline comment clearly explains the need for custom translation logic.
Description
Describe your changes in detail
Added filter support to GET
/asset-group-tag/:id/membersendpoint for:Motivation and Context
Resolves BED-6819
Why is this change required? What problem does it solve?
How Has This Been Tested?
Added tests + locally hit the above endpoint with various cases to ensure it was functional
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.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.