Bed 6420 etac #2021
Draft
Bed 6420 etac #2021
+389
−34
Conversation
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
…ng on one HS test
…ing on adding something similar to list asset group labels endpoint HS tests
stephanieslamb
added
enhancement
Contributor
WalkthroughAdds early user-auth validation and ETAC feature-flag gating to asset group tag listing and member-counting flows; updates graph.CountNodesByKind to accept filter criteria and propagates that change to mocks and tests; plus minor frontend import reorderings and license header additions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as AssetGroupTags Handler
participant Auth as Auth Resolver
participant FF as FeatureFlag
participant Graph as GraphQuery
Client->>API: GetAssetGroupTags(req)
activate API
API->>Auth: resolve user from context
alt user missing
Auth-->>API: error
API-->>Client: 500
else user present
Auth-->>API: User
API->>FF: GetFlagByKey("ETAC")
FF-->>API: flag(enabled/disabled)
alt ETAC enabled
note right of API `#e0f7fa`: derive env accessList from User\nbuild per-request and per-tag accessFilters
API->>Graph: CountNodesByKind(ctx, accessFiltersForTag, kinds...)
else ETAC disabled
note right of API `#f1f8e9`: no env accessFilters applied
API->>Graph: CountNodesByKind(ctx, [], kinds...)
end
Graph-->>API: counts
API-->>Client: tags + counts
end
deactivate API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cmd/api/src/test/integration/harnesses/completenessharness.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
cmd/api/src/api/v2/assetgrouptags.go(4 hunks)cmd/api/src/api/v2/assetgrouptags_test.go(16 hunks)cmd/api/src/queries/graph.go(2 hunks)cmd/api/src/queries/mocks/graph.go(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-08T19:24:33.396Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.test.tsx:17-20
Timestamp: 2025-09-08T19:24:33.396Z
Learning: In BloodHound codebase, prefer importing `render` and `screen` from custom test-utils (e.g., `import { render, screen } from '../../test-utils';`) instead of directly from `testing-library/react`.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.test.tsx
📚 Learning: 2025-08-28T19:26:03.304Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneAnalysisIcon.tsx:26-26
Timestamp: 2025-08-28T19:26:03.304Z
Learning: In packages/javascript/bh-shared-ui/src/hooks/, useZonePathParams is exported through the useZoneParams barrel (useZoneParams/index.ts exports it via wildcard from useZonePathParams.tsx), and usePrivilegeZoneAnalysis is exported through useConfiguration.ts. Both are available via the main hooks barrel import.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx
📚 Learning: 2025-08-25T20:12:35.629Z
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1803
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryCard.tsx:24-24
Timestamp: 2025-08-25T20:12:35.629Z
Learning: The useHighestPrivilegeTagId hook is available through the hooks barrel export in packages/javascript/bh-shared-ui/src/hooks/index.ts via the wildcard export `export * from './useAssetGroupTags'`. The import `import { useHighestPrivilegeTagId } from '../../../hooks'` works correctly and doesn't cause build failures.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsx
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx
📚 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.gocmd/api/src/api/v2/assetgrouptags_test.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 (2)
cmd/api/src/api/v2/assetgrouptags.go (9)
cmd/api/src/auth/model.go (2)
GetUserFromAuthCtx(184-191)Context(174-178)cmd/api/src/ctx/ctx.go (2)
FromRequest(70-72)Get(75-85)cmd/api/src/api/marshalling.go (1)
WriteErrorResponse(77-85)cmd/api/src/api/error.go (2)
BuildErrorResponse(131-142)HandleDatabaseError(146-155)cmd/api/src/api/constant.go (3)
QueryParameterIncludeCounts(43-43)QueryParameterEnvironments(49-49)URIPathVariableAssetGroupTagID(55-55)cmd/api/src/model/appcfg/flag.go (1)
FeatureETAC(43-43)cmd/api/src/api/v2/etac.go (1)
ExtractEnvironmentIDsFromUser(63-71)packages/go/graphschema/ad/ad.go (1)
DomainSID(155-155)packages/go/graphschema/azure/azure.go (1)
TenantID(107-107)
cmd/api/src/api/v2/assetgrouptags_test.go (5)
cmd/api/src/api/constant.go (2)
URIPathVariableAssetGroupTagID(55-55)QueryParameterEnvironments(49-49)cmd/api/src/model/appcfg/flag.go (2)
FeatureETAC(43-43)FeatureFlag(49-69)packages/go/graphschema/ad/ad.go (3)
DomainSID(155-155)Domain(35-35)User(29-29)packages/go/graphschema/azure/azure.go (2)
TenantID(107-107)User(41-41)cmd/api/src/api/v2/assetgrouptags.go (1)
GetAssetGroupTagMemberCountsResponse(601-604)
⏰ 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: build-ui
- GitHub Check: run-tests
🔇 Additional comments (5)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsx (1)
26-26: LGTM - Import reordering only.The import statement reordering has no functional impact.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx (1)
28-28: LGTM - Import reordering only.The import statement reordering has no functional impact.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.tsx (1)
1-15: LGTM - License header added.The Apache 2.0 license header addition aligns with licensing requirements and introduces no functional changes.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.test.tsx (1)
1-15: LGTM - License header added.The Apache 2.0 license header addition aligns with licensing requirements and introduces no functional changes to the tests.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx (1)
25-25: LGTM - Import reordering only.The import statement reordering has no functional impact.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/api/src/api/v2/assetgrouptags_test.go (1)
1-1: Fix mock isolation in test cases - generic expectation contaminating subsequent tests.The test has mock contamination across test cases. Both "IncludeCounts_Selector_counts" (lines 263-307) and "IncludeCounts_member_counts" (lines 310-371) register expectations on the same
mockGraphDbobject without proper isolation. The genericCountNodesByKindexpectation at line 288 (.CountNodesByKind(gomock.Any(), gomock.Any(), gomock.Any()).Return(int64(0), nil).Times(4)) matches calls in the subsequent test case and returns 0 instead of allowing the specific criteria-based mocks (lines 343, 349) to match and return 6/4.Each test case needs its own mock controller and mock instances, or mocks must be reset between cases:
// Option 1: Create mocks per test case (recommended) { Name: "IncludeCounts_member_counts", Input: func(input *apitest.Input) { mockCtrl := gomock.NewController(t) // New controller per case mockDB := mocks_db.NewMockDatabase(mockCtrl) mockGraphDb := mocks_graph.NewMockGraph(mockCtrl) // ... setup with isolated mocks }, // ... } // Option 2: Reset mocks between cases using mockCtrl.Finish() and recreating
🧹 Nitpick comments (1)
cmd/api/src/api/v2/assetgrouptags_test.go (1)
1749-1758: Consider optimizing duplicate environment filters.The test expects TWO criteria, both filtering for the same environment "testenv" (once from query params, once from ETAC list). While this is functionally correct (the intersection of "testenv" and "testenv" is "testenv"), it results in redundant filtering.
Consider whether the implementation could optimize this case by detecting when the query param environments are a subset of (or equal to) the user's ETAC environments and applying the filter only once.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/api/v2/assetgrouptags_test.go(19 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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_test.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
📚 Learning: 2025-05-27T16:58:33.295Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Applied to files:
cmd/api/src/api/v2/assetgrouptags_test.go
🧬 Code graph analysis (1)
cmd/api/src/api/v2/assetgrouptags_test.go (7)
cmd/api/src/api/constant.go (2)
URIPathVariableAssetGroupTagID(55-55)QueryParameterEnvironments(49-49)cmd/api/src/model/assetgrouptags.go (1)
AssetGroupTags(111-111)cmd/api/src/database/db.go (1)
ErrNotFound(41-41)cmd/api/src/model/appcfg/flag.go (2)
FeatureETAC(43-43)FeatureFlag(49-69)packages/go/graphschema/azure/azure.go (2)
User(41-41)TenantID(107-107)packages/go/graphschema/ad/ad.go (3)
User(29-29)DomainSID(155-155)Domain(35-35)cmd/api/src/api/v2/assetgrouptags.go (1)
GetAssetGroupTagMemberCountsResponse(601-604)
🪛 GitHub Actions: Run Go Unit Tests
cmd/api/src/api/v2/assetgrouptags_test.go
[error] 1-1: Command go test -json -coverprofile ... failed with exit status 1
⏰ 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: build-ui
- GitHub Check: run-tests
🔇 Additional comments (6)
cmd/api/src/api/v2/assetgrouptags_test.go (6)
78-88: LGTM: Proper authentication error handling.Good addition of test cases to verify that requests without valid user context are properly rejected with internal server errors. This ensures the ETAC feature fails closed when authentication context is missing.
Also applies to: 1587-1597
101-103: LGTM: Comprehensive ETAC feature flag coverage.Good addition of ETAC feature flag checks across test scenarios. The tests properly mock both enabled and disabled states, ensuring the feature can be controlled via configuration.
Also applies to: 152-154, 180-182, 211-213, 241-243, 290-292, 354-356
343-354: LGTM: Correct signature update with ETAC criteria.The updated
CountNodesByKindcall correctly includes the new criteria parameter for environment-based filtering. The expectation properly validates that ETAC environment filters are applied when a user has restricted environment access.
316-324: LGTM: Clear and explicit test user configuration.Good practice to explicitly construct user models with all relevant fields for ETAC testing. This makes the test scenarios clear and ensures different user permission configurations are properly tested.
92-95: No issues found. Helper functions are properly defined and accessible.The
setupUser()andsetupUserCtx()helper functions are defined incmd/api/src/api/v2/fileingest_test.goand are accessible toassetgrouptags_test.gosince both files are in the same package. The code will compile and function correctly.
1787-1812: Missing defensive check for empty ETAC list in GetAssetGroupTagMemberCountsByKind.When a user has
AllEnvironments: falseand an emptyEnvironmentTargetedAccessControllist,ExtractEnvironmentIDsFromUserreturns[]. The code then appends a filter withquery.In(..., []), which matches zero nodes.However, the test expects 2 results—contradicting access control semantics. Compare this to
GetAssetGroupTagsByTagNameWithMemberCounts(lines 144-155), which explicitly checksif len(accessList) == 0and returns 0 members. The same defensive logic should apply here.Action: Add the defensive check before appending the ETAC filter at lines 634-640:
if len(accessList) > 0 { filters = append(filters, query.Or(...)) }Otherwise, update the test expectation to 0 results.
⛔ Skipped due to 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.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.
bsheth711
requested changes
Nov 3, 2025
cmd/api/src/api/v2/assetgrouptags.go
Outdated
| if n, err := s.GraphQuery.CountNodesByKind(rCtx, tag.ToKind()); err != nil { | ||
| // only check user access if ETAC is enabled | ||
| if etacFlag.Enabled { | ||
| accessList := ExtractEnvironmentIDsFromUser(&user) |
Contributor
There was a problem hiding this comment.
This does not need to be in the loop
cmd/api/src/api/v2/assetgrouptags.go
Outdated
Comment on lines
157
to
162
| // user has access to specified environment | ||
| filters = append(filters, query.Or( | ||
| query.In(query.NodeProperty(ad.DomainSID.String()), accessList), | ||
| query.In(query.NodeProperty(azure.TenantID.String()), accessList), | ||
| )) | ||
| } |
Contributor
There was a problem hiding this comment.
This can also be moved out of the loop
cmd/api/src/queries/graph.go
Outdated
Comment on lines
671
to
673
| func (s *GraphQuery) CountNodesByKind(ctx context.Context, filter []graph.Criteria, kinds ...graph.Kind) (int64, error) { | ||
| combinedFilter := query.And(filter, (query.KindIn(query.Node(), kinds...))) | ||
| return s.CountFilteredNodes(ctx, combinedFilter) |
Contributor
There was a problem hiding this comment.
Since filter is a slice, it should be exploded. Also consider renaming it to the plural filters since it is a slice.
Suggested change
| func (s *GraphQuery) CountNodesByKind(ctx context.Context, filter []graph.Criteria, kinds ...graph.Kind) (int64, error) { | |
| combinedFilter := query.And(filter, (query.KindIn(query.Node(), kinds...))) | |
| return s.CountFilteredNodes(ctx, combinedFilter) | |
| func (s *GraphQuery) CountNodesByKind(ctx context.Context, filter []graph.Criteria, kinds ...graph.Kind) (int64, error) { | |
| filter = append(filter, query.KindIn(query.Node(), kinds...)) | |
| combinedFilter := query.And(filter...) | |
| return s.CountFilteredNodes(ctx, combinedFilter) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cmd/api/src/api/v2/assetgrouptags.go (1)
149-149: Move user access list extraction outside the loop.The user doesn't change between iterations. Extract the access list once before the loop begins to avoid redundant work.
Based on learnings
Apply this diff:
+ var accessList []string + if etacFlag.Enabled && paramIncludeCounts && !user.AllEnvironments { + accessList = ExtractEnvironmentIDsFromUser(&user) + } + for _, tag := range tags { filters := []graph.Criteria{} tview := AssetGroupTagView{AssetGroupTag: tag} if paramIncludeCounts { // only check user access if ETAC is enabled if etacFlag.Enabled { - accessList := ExtractEnvironmentIDsFromUser(&user) - if !user.AllEnvironments {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/api/src/api/v2/assetgrouptags.go(5 hunks)cmd/api/src/api/v2/assetgrouptags_test.go(18 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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.gocmd/api/src/api/v2/assetgrouptags_test.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 (2)
cmd/api/src/api/v2/assetgrouptags.go (9)
cmd/api/src/auth/model.go (2)
GetUserFromAuthCtx(184-191)Context(174-178)cmd/api/src/ctx/ctx.go (2)
FromRequest(70-72)Get(75-85)cmd/api/src/api/marshalling.go (1)
WriteErrorResponse(77-85)cmd/api/src/api/error.go (2)
BuildErrorResponse(131-142)HandleDatabaseError(146-155)cmd/api/src/api/constant.go (3)
QueryParameterIncludeCounts(43-43)QueryParameterEnvironments(49-49)URIPathVariableAssetGroupTagID(55-55)cmd/api/src/model/appcfg/flag.go (1)
FeatureETAC(43-43)cmd/api/src/api/v2/etac.go (1)
ExtractEnvironmentIDsFromUser(63-71)packages/go/graphschema/ad/ad.go (1)
DomainSID(155-155)packages/go/graphschema/azure/azure.go (1)
TenantID(107-107)
cmd/api/src/api/v2/assetgrouptags_test.go (7)
cmd/api/src/api/constant.go (3)
URIPathVariableAssetGroupTagID(55-55)QueryParameterEnvironments(49-49)QueryParameterIncludeCounts(43-43)cmd/api/src/model/assetgrouptags.go (5)
AssetGroupTags(111-111)AssetGroupTag(93-109)AssetGroupTag(113-115)AssetGroupTagTypeLabel(50-50)AssetGroupTagTypeTier(49-49)cmd/api/src/database/db.go (1)
ErrNotFound(41-41)cmd/api/src/model/appcfg/flag.go (2)
FeatureETAC(43-43)FeatureFlag(49-69)packages/go/graphschema/azure/azure.go (2)
User(41-41)TenantID(107-107)packages/go/graphschema/ad/ad.go (3)
User(29-29)DomainSID(155-155)Domain(35-35)cmd/api/src/api/v2/assetgrouptags.go (1)
GetAssetGroupTagMemberCountsResponse(607-610)
⏰ 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-analysis
- GitHub Check: run-tests
Contributor
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)
cmd/api/src/api/v2/assetgrouptags.go(5 hunks)cmd/api/src/queries/graph.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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
🧬 Code graph analysis (1)
cmd/api/src/api/v2/assetgrouptags.go (9)
cmd/api/src/auth/model.go (2)
GetUserFromAuthCtx(184-191)Context(174-178)cmd/api/src/ctx/ctx.go (2)
FromRequest(70-72)Get(75-85)cmd/api/src/api/marshalling.go (1)
WriteErrorResponse(77-85)cmd/api/src/api/error.go (2)
BuildErrorResponse(131-142)HandleDatabaseError(146-155)cmd/api/src/api/constant.go (3)
QueryParameterIncludeCounts(43-43)QueryParameterEnvironments(49-49)URIPathVariableAssetGroupTagID(55-55)cmd/api/src/model/appcfg/flag.go (1)
FeatureETAC(43-43)cmd/api/src/api/v2/etac.go (1)
ExtractEnvironmentIDsFromUser(63-71)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). (2)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cmd/api/src/api/v2/assetgrouptags.go (1)
71-74: Don't ship a panic-based Errorf implementation.This method still panics unconditionally, which will crash any request handler that invokes it. This issue was previously flagged but remains unaddressed.
🧹 Nitpick comments (1)
cmd/api/src/api/v2/assetgrouptags.go (1)
148-148: Fix comment typo.The comment has a duplicate "is": "is allEnvironments is false" should be "allEnvironments is false".
Apply this diff:
- // only build accessFilters if user has some targeted access and is allEnvironments is false + // only build accessFilters if user has some targeted access and allEnvironments is false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/api/src/api/v2/assetgrouptags.go(6 hunks)cmd/api/src/queries/graph.go(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
📚 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-06-25T18:24:25.014Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T18:24:25.014Z
Learning: In BloodHound Go code, for error types in slog structured logging, prefer using slog.String("key", err.Error()) over slog.Any("key", err). The explicit string conversion with err.Error() is preferred over using slog.Any() for error types.
Applied to files:
cmd/api/src/api/v2/assetgrouptags.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/api/v2/assetgrouptags.go
🧬 Code graph analysis (1)
cmd/api/src/api/v2/assetgrouptags.go (9)
cmd/api/src/auth/model.go (2)
GetUserFromAuthCtx(184-191)Context(174-178)cmd/api/src/ctx/ctx.go (2)
FromRequest(70-72)Get(75-85)cmd/api/src/api/marshalling.go (1)
WriteErrorResponse(77-85)cmd/api/src/api/error.go (2)
BuildErrorResponse(131-142)HandleDatabaseError(146-155)cmd/api/src/api/constant.go (3)
QueryParameterIncludeCounts(43-43)QueryParameterEnvironments(49-49)URIPathVariableAssetGroupTagID(55-55)cmd/api/src/model/appcfg/flag.go (1)
FeatureETAC(43-43)cmd/api/src/api/v2/etac.go (1)
ExtractEnvironmentIDsFromUser(63-71)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). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
Contributor
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)
cmd/api/src/api/v2/assetgrouptags.go(5 hunks)cmd/api/src/api/v2/assetgrouptags_test.go(18 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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.
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.
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
📚 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.gocmd/api/src/api/v2/assetgrouptags_test.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.gocmd/api/src/api/v2/assetgrouptags_test.go
📚 Learning: 2025-06-25T18:24:25.014Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T18:24:25.014Z
Learning: In BloodHound Go code, for error types in slog structured logging, prefer using slog.String("key", err.Error()) over slog.Any("key", err). The explicit string conversion with err.Error() is preferred over using slog.Any() for error types.
Applied to files:
cmd/api/src/api/v2/assetgrouptags.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/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 (2)
cmd/api/src/api/v2/assetgrouptags.go (9)
cmd/api/src/auth/model.go (2)
GetUserFromAuthCtx(184-191)Context(174-178)cmd/api/src/ctx/ctx.go (2)
FromRequest(70-72)Get(75-85)cmd/api/src/api/marshalling.go (1)
WriteErrorResponse(77-85)cmd/api/src/api/error.go (2)
BuildErrorResponse(131-142)HandleDatabaseError(146-155)cmd/api/src/api/constant.go (3)
QueryParameterIncludeCounts(43-43)QueryParameterEnvironments(49-49)URIPathVariableAssetGroupTagID(55-55)cmd/api/src/model/appcfg/flag.go (1)
FeatureETAC(43-43)cmd/api/src/api/v2/etac.go (1)
ExtractEnvironmentIDsFromUser(63-71)packages/go/graphschema/ad/ad.go (1)
DomainSID(155-155)packages/go/graphschema/azure/azure.go (1)
TenantID(107-107)
cmd/api/src/api/v2/assetgrouptags_test.go (7)
cmd/api/src/api/constant.go (3)
URIPathVariableAssetGroupTagID(55-55)QueryParameterEnvironments(49-49)QueryParameterIncludeCounts(43-43)cmd/api/src/model/assetgrouptags.go (4)
AssetGroupTag(93-109)AssetGroupTag(113-115)AssetGroupTagTypeLabel(50-50)AssetGroupTagTypeTier(49-49)cmd/api/src/database/db.go (1)
ErrNotFound(41-41)cmd/api/src/model/appcfg/flag.go (2)
FeatureETAC(43-43)FeatureFlag(49-69)packages/go/graphschema/azure/azure.go (2)
User(41-41)TenantID(107-107)packages/go/graphschema/ad/ad.go (3)
User(29-29)DomainSID(155-155)Domain(35-35)cmd/api/src/api/v2/assetgrouptags.go (1)
GetAssetGroupTagMemberCountsResponse(606-609)
⏰ 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). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-go-unit-tests
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (2)
cmd/api/src/api/v2/assetgrouptags_test.go (2)
78-88: Good coverage of auth context error paths.The new test cases properly verify that both endpoints handle missing user context correctly by returning 500 with an appropriate error message. This is good defensive coding and helps catch authentication issues early.
Also applies to: 1593-1603
314-380: Comprehensive ETAC test coverage.The test cases thoroughly cover ETAC scenarios including:
- Users with
AllEnvironmentsenabled- Users with targeted environment access
- Users with no environment access
- ETAC feature flag enabled/disabled states
The tests properly verify that
CountNodesByKindandGetPrimaryNodeKindCountsare called with appropriate filter criteria based on the user's ETAC configuration.Also applies to: 1691-1810
Comment on lines
+638
to
+650
| if etacFlag.Enabled && !user.AllEnvironments { | ||
| accessList := ExtractEnvironmentIDsFromUser(&user) | ||
|
|
||
| if len(accessList) > 0 { | ||
| filters = append(filters, | ||
| query.Or( | ||
| query.In(query.NodeProperty(ad.DomainSID.String()), accessList), | ||
| query.In(query.NodeProperty(azure.TenantID.String()), accessList), | ||
| ), | ||
| ) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent ETAC handling when user has no environment access.
When ETAC is enabled, user.AllEnvironments is false, and accessList is empty, this endpoint doesn't append any ETAC filter. This differs from GetAssetGroupTags (lines 158-165), which explicitly sets counts to 0 and skips querying when accessList is empty.
The current implementation allows users with no ETAC access to see counts for any environments they specify via the environments query parameter, potentially bypassing ETAC restrictions. When accessList is empty, no ETAC filter is added, so only the environmentIds filter (lines 626-631) applies, allowing users to query arbitrary environments.
Consider applying the same defensive pattern as GetAssetGroupTags:
if etacFlag.Enabled && !user.AllEnvironments {
accessList := ExtractEnvironmentIDsFromUser(&user)
+ if len(accessList) == 0 {
+ // User has no environment access, return zero counts
+ data := GetAssetGroupTagMemberCountsResponse{
+ Counts: map[string]int{},
+ }
+ api.WriteBasicResponse(request.Context(), data, http.StatusOK, response)
+ return
+ }
+
- if len(accessList) > 0 {
- filters = append(filters,
- query.Or(
- query.In(query.NodeProperty(ad.DomainSID.String()), accessList),
- query.In(query.NodeProperty(azure.TenantID.String()), accessList),
- ),
- )
- }
+ filters = append(filters,
+ query.Or(
+ query.In(query.NodeProperty(ad.DomainSID.String()), accessList),
+ query.In(query.NodeProperty(azure.TenantID.String()), accessList),
+ ),
+ )
}📝 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.
Suggested change
| if etacFlag.Enabled && !user.AllEnvironments { | |
| accessList := ExtractEnvironmentIDsFromUser(&user) | |
| if len(accessList) > 0 { | |
| filters = append(filters, | |
| query.Or( | |
| query.In(query.NodeProperty(ad.DomainSID.String()), accessList), | |
| query.In(query.NodeProperty(azure.TenantID.String()), accessList), | |
| ), | |
| ) | |
| } | |
| } | |
| } | |
| if etacFlag.Enabled && !user.AllEnvironments { | |
| accessList := ExtractEnvironmentIDsFromUser(&user) | |
| if len(accessList) == 0 { | |
| // User has no environment access, return zero counts | |
| data := GetAssetGroupTagMemberCountsResponse{ | |
| Counts: map[string]int{}, | |
| } | |
| api.WriteBasicResponse(request.Context(), data, http.StatusOK, response) | |
| return | |
| } | |
| filters = append(filters, | |
| query.Or( | |
| query.In(query.NodeProperty(ad.DomainSID.String()), accessList), | |
| query.In(query.NodeProperty(azure.TenantID.String()), accessList), | |
| ), | |
| ) | |
| } |
🤖 Prompt for AI Agents
In cmd/api/src/api/v2/assetgrouptags.go around lines 638 to 650, currently when
ETAC is enabled and user.AllEnvironments is false but
ExtractEnvironmentIDsFromUser returns an empty accessList no ETAC filter is
applied which lets the explicit environments query bypass ETAC; update the logic
to mirror GetAssetGroupTags (lines ~158-165) by detecting an empty accessList
and short-circuiting to return zero counts / skip executing the query (or
alternatively append a filter that matches nothing) so users with no ETAC access
cannot retrieve counts for arbitrary environments.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 tickets adds ETAC filtering for nodes on the GetAssetGroupTags and GetAssetGroupTagMemberCountsByKind endpoints.
Motivation and Context
Resolves BED-6420
Why is this change required? What problem does it solve?
How Has This Been Tested?
These changes were tested locally. Screenshots below show responses for each endpoint. To test, create two new users, both with all_environments = false. You will need to create a user(with user_id) in the ETAC table to add a single environment id to test for a single environment.
Additional unit tests added.
Screenshots (optional):
/asset-group-tags?counts=true Admin privileges
/asset-group-tags?counts=true user no access
/asset-group-tags?counts=true user all envs=false, wraith corp access
/asset-group-tags?counts=true user all envs=false, phantom corp access
/asset-group-tags/1/members/counts Admin privileges
/asset-group-tags/1/members/counts user no access
/asset-group-tags/1/members/counts user wraith corp access
/asset-group-tags/1/members/counts user phantom corp access
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Breaking Changes
Chores