fix(api): Fixes bug in BED-6515 #2242
Open
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
- Fixes Permission Denied! error for User and Power User roles for editing their own cypher queries - The permission is extended to User and Power User roles from the Administrator role according to permission requirements documentation in https://specterops.atlassian.net/wiki/spaces/~712020bb8a7f8d20094d79bf4a232719fb4cba/pages/1336901652/User+Permissions+Grid - Adds unit tests for the backend changes Fix: BED-6515
Contributor
WalkthroughBroadened privileged-role check in saved query sharing to treat Administrator, User, and Power User as privileged; adjusted control flow accordingly. Tests refactored into a table-driven suite with a new helper and fixtures, expanding coverage across ownership, public/shared scopes, and role permutations. No public signatures changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (7)📓 Common learnings📚 Learning: 2025-07-22T20:30:34.839ZApplied to files:
📚 Learning: 2025-09-08T19:01:53.112ZApplied to files:
📚 Learning: 2025-08-27T19:15:54.854ZApplied to files:
📚 Learning: 2025-08-27T15:12:49.129ZApplied to files:
📚 Learning: 2025-07-14T22:41:57.442ZApplied to files:
📚 Learning: 2025-11-05T21:13:08.542ZApplied to files:
🧬 Code graph analysis (1)cmd/api/src/api/v2/saved_queries_permissions_test.go (6)
⏰ 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)
🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting 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
🤖 Fix all issues with AI agents
In @cmd/api/src/api/v2/saved_queries_permissions_test.go:
- Around line 680-694: Tests allow Public=true with non-empty UserIDs, but the
HTTP handler in saved_queries_permissions.go explicitly rejects that
combination; update CanUpdateSavedQueriesPermission to mirror the handler
validation by returning an error when payload.Public is true and payload.UserIDs
is non-empty (use the same message "Public cannot be true while user_ids is
populated" or equivalent) so permission logic and handler behavior are
consistent; alternatively, remove/adjust the test cases that assert allowing
Public=true with UserIDs if you prefer keeping the current permission function
semantics.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/api/src/api/v2/saved_queries_permissions.gocmd/api/src/api/v2/saved_queries_permissions_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:108-148
Timestamp: 2025-09-08T19:01:53.112Z
Learning: In BloodHound's CypherSearch component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the sharing state reset for sharedIds and isPublic after the two-step permissions update is handled elsewhere in the codebase, so additional state reset callbacks in the updateQueryPermissions function are not needed.
📚 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/saved_queries_permissions_test.go
🧬 Code graph analysis (2)
cmd/api/src/api/v2/saved_queries_permissions.go (1)
cmd/api/src/auth/role.go (3)
RoleAdministrator(29-29)RoleUser(27-27)RolePowerUser(28-28)
cmd/api/src/api/v2/saved_queries_permissions_test.go (4)
cmd/api/src/auth/role.go (2)
RoleUser(27-27)RolePowerUser(28-28)cmd/api/src/api/v2/saved_queries_permissions.go (3)
SavedQueryPermissionRequest(39-42)CanUpdateSavedQueriesPermission(50-86)ErrInvalidSelfShare(45-45)cmd/api/src/database/saved_queries_permissions.go (1)
SavedQueryScopeMap(42-42)cmd/api/src/model/saved_queries_permissions.go (3)
SavedQueryScopeOwned(27-27)SavedQueryScopePublic(29-29)SavedQueryScopeShared(28-28)
⏰ 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: build-ui
- GitHub Check: run-go-unit-tests
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (3)
cmd/api/src/api/v2/saved_queries_permissions.go (1)
51-57: LGTM! Permission extension is correctly implemented.The change successfully extends edit permissions for saved queries from Administrator-only to include User and Power User roles. The boolean aggregation is clear and the comment explains the intent well.
cmd/api/src/api/v2/saved_queries_permissions_test.go (2)
89-115: Test fixtures correctly configured.The new
userRoleUserandpowerUserfixtures are properly set up with appropriate role names and permissions, matching the pattern of the existingadminUserfixture.
600-614: Comprehensive test coverage for new role permissions.These test cases provide thorough coverage of the permission changes, including:
- Self-share error validation for User and Power User roles
- Public-to-private query transitions
- Idempotent operations (setting private queries to private)
- Converting public+shared queries to private
The tests follow established patterns and effectively validate that User and Power User roles now have the same privileged permissions as Administrators for managing their own saved queries.
Also applies to: 616-630, 632-646, 648-662, 664-678, 696-710, 712-726, 728-742
Comment on lines
680
to
694
| t.Run("User-owned, public & shared query shared to other users is allowed", func(t *testing.T) { | ||
| payload := v2.SavedQueryPermissionRequest{ | ||
| UserIDs: []uuid.UUID{nonAdminUser1.ID, nonAdminUser2.ID}, | ||
| Public: true, // keep it public | ||
| } | ||
|
|
||
| dbSavedQueryScope := database.SavedQueryScopeMap{ | ||
| model.SavedQueryScopeOwned: true, // belongs to RoleUser | ||
| model.SavedQueryScopePublic: true, // currently public | ||
| model.SavedQueryScopeShared: true, // and shared to others | ||
| } | ||
|
|
||
| err := v2.CanUpdateSavedQueriesPermission(userRoleUser, true, payload, dbSavedQueryScope) | ||
| require.Nil(t, err) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check handler validation for Public:true with UserIDs
rg -n "Public cannot be true while user_ids is populated" cmd/api/src/api/v2/saved_queries_permissions.goRepository: SpecterOps/BloodHound
Length of output: 231
🏁 Script executed:
# Check CanUpdateSavedQueriesPermission function logic
rg -n -B5 -A10 "func CanUpdateSavedQueriesPermission" cmd/api/src/api/v2/saved_queries_permissions.goRepository: SpecterOps/BloodHound
Length of output: 973
🏁 Script executed:
# Look at the test file to understand the test structure
fd saved_queries_permissions_test.go cmd/api/src/api/v2/Repository: SpecterOps/BloodHound
Length of output: 117
🏁 Script executed:
# Check lines 680-694 and 744-758 in the test file
sed -n '680,694p; 744,758p' cmd/api/src/api/v2/saved_queries_permissions_test.goRepository: SpecterOps/BloodHound
Length of output: 1344
Tests verify behavior that the handler explicitly rejects.
The permission function CanUpdateSavedQueriesPermission allows setting Public: true while providing UserIDs, but the handler at line 162 of saved_queries_permissions.go explicitly rejects this combination:
"Public cannot be true while user_ids is populated"These test cases bypass the handler and verify only the permission function's logic, which permits the scenario. This creates an inconsistency: the handler will reject valid permission function outputs. Additionally, sharing a public query to specific users is logically redundant since public queries are accessible to everyone.
Consider either removing these test cases or adding validation in CanUpdateSavedQueriesPermission to reject Public: true with non-empty UserIDs for consistency with handler validation.
Also applies to: 744-758
🤖 Prompt for AI Agents
In @cmd/api/src/api/v2/saved_queries_permissions_test.go around lines 680 - 694,
Tests allow Public=true with non-empty UserIDs, but the HTTP handler in
saved_queries_permissions.go explicitly rejects that combination; update
CanUpdateSavedQueriesPermission to mirror the handler validation by returning an
error when payload.Public is true and payload.UserIDs is non-empty (use the same
message "Public cannot be true while user_ids is populated" or equivalent) so
permission logic and handler behavior are consistent; alternatively,
remove/adjust the test cases that assert allowing Public=true with UserIDs if
you prefer keeping the current permission function semantics.
…ueriesPermission to a reusable table-drive test suite that loops over various scenarios
- Makes reusable helper function newSavedQueryScope to make custom values and combinations of SavedQueryScopeOwned vs. SavedQueryScopePublic vs. SavedQueryScopeShared
- Makes reusable function runCanUpdateSavedQueriesPermissionTest to loop over json table of various scenarios with custom parameters for:
- test name
- test description
- user role
- whether user owns the query
- permission request for userId's to share with
- permission request for if query will be public
- previously saved states for whether
- query was owned by user
- query was public
- query was shared to others
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/api/src/api/v2/saved_queries_permissions_test.go (1)
536-646: Consider expanding test coverage for User and PowerUser roles.The User role and PowerUser test cases primarily cover self-share errors and public→private transitions. While these roles share the same privileged code path as Admin, adding a few more cases (e.g., sharing a private query to other users, making a private query public) would provide stronger regression coverage for the bug fix.
This is optional since the core logic is tested via the Admin-owned cases.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/api/v2/saved_queries_permissions_test.go
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:108-148
Timestamp: 2025-09-08T19:01:53.112Z
Learning: In BloodHound's CypherSearch component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the sharing state reset for sharedIds and isPublic after the two-step permissions update is handled elsewhere in the codebase, so additional state reset callbacks in the updateQueryPermissions function are not needed.
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
📚 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/saved_queries_permissions_test.go
📚 Learning: 2025-09-08T19:01:53.112Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:108-148
Timestamp: 2025-09-08T19:01:53.112Z
Learning: In BloodHound's CypherSearch component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the sharing state reset for sharedIds and isPublic after the two-step permissions update is handled elsewhere in the codebase, so additional state reset callbacks in the updateQueryPermissions function are not needed.
Applied to files:
cmd/api/src/api/v2/saved_queries_permissions_test.go
📚 Learning: 2025-08-27T19:15:54.854Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SavedQueryPermissions.tsx:53-54
Timestamp: 2025-08-27T19:15:54.854Z
Learning: In the BloodHound codebase, the team allows the useQueryPermissions hook to run even with undefined queryId because the API is designed to return errors for empty states, and they prefer to handle these error cases naturally rather than preventing the queries from executing.
Applied to files:
cmd/api/src/api/v2/saved_queries_permissions_test.go
📚 Learning: 2025-08-27T15:12:49.129Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts:98-105
Timestamp: 2025-08-27T15:12:49.129Z
Learning: In the BloodHound codebase, for the `useDeleteQueryPermissions` hook in `packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts`, the team prefers to use broad cache invalidation with `queryClient.invalidateQueries(savedQueryKeys.permissions)` rather than targeted invalidation for specific permission entries.
Applied to files:
cmd/api/src/api/v2/saved_queries_permissions_test.go
📚 Learning: 2025-07-14T22:41:57.442Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1562
File: cmd/api/src/daemons/datapipe/pipeline.go:177-185
Timestamp: 2025-07-14T22:41:57.442Z
Learning: In BloodHound's datapipe pipeline, when checking feature flags before performing actions (like cache reset), the correct pattern is to verify both that the flag retrieval succeeds AND that the flag is enabled. Performing the action when flag retrieval fails is incorrect logic.
Applied to files:
cmd/api/src/api/v2/saved_queries_permissions_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/saved_queries_permissions_test.go
🧬 Code graph analysis (1)
cmd/api/src/api/v2/saved_queries_permissions_test.go (4)
cmd/api/src/database/saved_queries_permissions.go (1)
SavedQueryScopeMap(42-42)cmd/api/src/model/saved_queries_permissions.go (3)
SavedQueryScopeOwned(27-27)SavedQueryScopePublic(29-29)SavedQueryScopeShared(28-28)cmd/api/src/api/v2/saved_queries_permissions.go (5)
SavedQueryPermissionRequest(39-42)CanUpdateSavedQueriesPermission(50-86)ErrForbidden(46-46)ErrInvalidSelfShare(45-45)ErrInvalidPublicShare(47-47)cmd/api/src/auth/role.go (2)
RoleUser(27-27)RolePowerUser(28-28)
⏰ 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
🔇 Additional comments (4)
cmd/api/src/api/v2/saved_queries_permissions_test.go (4)
53-60: LGTM!Clean helper that reduces boilerplate for constructing scope maps in tests.
62-83: LGTM!Well-structured test helper with proper
t.Helper()usage and clear parameter organization.
121-147: LGTM!New user fixtures for
RoleUserandRolePowerUserare consistent with existing patterns and correctly support testing the extended privilege model.
649-660: LGTM!The test execution loop correctly iterates over all cases and invokes the helper with appropriate arguments.
…_CanUpdateSavedQueriesPermission to get rid of extra unnecessary wrapper runCanUpdateSavedQueriesPermissionTest
merging with main
…anUpdateSavedQueriesPermission in saved_queries_permissions_test.go for the case, non admin Non-privileged user cannot make someone else's public query private
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/api/src/api/v2/saved_queries_permissions_test.go`:
- Around line 268-279: The test suite contains a duplicate test case named
"Non-admin owned, public query set to public error" (same payload, user
nonAdminUser1, scope from newSavedQueryScope(true, true, false), and expectedErr
v2.ErrForbidden); remove the duplicate entry or replace it with a distinct
scenario (e.g., change the payload, ownership flag, user, or expectedErr) so
each test case in saved_queries_permissions_test.go is unique; locate the
duplicate by searching for the test name and the v2.SavedQueryPermissionRequest
payload and delete or modify the redundant block.
♻️ Duplicate comments (1)
cmd/api/src/api/v2/saved_queries_permissions_test.go (1)
574-585: Test verifies behavior that the handler explicitly rejects.This test case passes
Public: truewith a non-emptyUserIDslist, but the inline comment acknowledges that the "HTTP handler will reject this combination." Testing permission logic for an input that can never reach this code path provides limited value and may give false confidence about test coverage.
🧹 Nitpick comments (1)
cmd/api/src/api/v2/saved_queries_permissions_test.go (1)
650-660: LGTM!The test loop correctly captures the range variable and iterates through all test cases. Since these subtests are self-contained with no shared mocks or state, you could optionally add
t.Parallel()after thet.Runline to speed up test execution. Based on learnings, this pattern is acceptable for self-contained subtests.♻️ Optional: Add subtest parallelization
for _, tc := range tests { tc := tc // capture range variable t.Run(tc.name, func(t *testing.T) { + t.Parallel() if tc.comment != "" { t.Log(tc.comment) } err := v2.CanUpdateSavedQueriesPermission(tc.user, tc.savedQueryBelongsToUser, tc.payload, tc.scope) require.Equal(t, tc.expectedErr, err) }) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/api/v2/saved_queries_permissions_test.go
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:108-148
Timestamp: 2025-09-08T19:01:53.112Z
Learning: In BloodHound's CypherSearch component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the sharing state reset for sharedIds and isPublic after the two-step permissions update is handled elsewhere in the codebase, so additional state reset callbacks in the updateQueryPermissions function are not needed.
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
📚 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/saved_queries_permissions_test.go
📚 Learning: 2025-09-08T19:01:53.112Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:108-148
Timestamp: 2025-09-08T19:01:53.112Z
Learning: In BloodHound's CypherSearch component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the sharing state reset for sharedIds and isPublic after the two-step permissions update is handled elsewhere in the codebase, so additional state reset callbacks in the updateQueryPermissions function are not needed.
Applied to files:
cmd/api/src/api/v2/saved_queries_permissions_test.go
📚 Learning: 2025-08-27T19:15:54.854Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SavedQueryPermissions.tsx:53-54
Timestamp: 2025-08-27T19:15:54.854Z
Learning: In the BloodHound codebase, the team allows the useQueryPermissions hook to run even with undefined queryId because the API is designed to return errors for empty states, and they prefer to handle these error cases naturally rather than preventing the queries from executing.
Applied to files:
cmd/api/src/api/v2/saved_queries_permissions_test.go
📚 Learning: 2025-08-27T15:12:49.129Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts:98-105
Timestamp: 2025-08-27T15:12:49.129Z
Learning: In the BloodHound codebase, for the `useDeleteQueryPermissions` hook in `packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts`, the team prefers to use broad cache invalidation with `queryClient.invalidateQueries(savedQueryKeys.permissions)` rather than targeted invalidation for specific permission entries.
Applied to files:
cmd/api/src/api/v2/saved_queries_permissions_test.go
📚 Learning: 2025-07-14T22:41:57.442Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1562
File: cmd/api/src/daemons/datapipe/pipeline.go:177-185
Timestamp: 2025-07-14T22:41:57.442Z
Learning: In BloodHound's datapipe pipeline, when checking feature flags before performing actions (like cache reset), the correct pattern is to verify both that the flag retrieval succeeds AND that the flag is enabled. Performing the action when flag retrieval fails is incorrect logic.
Applied to files:
cmd/api/src/api/v2/saved_queries_permissions_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/saved_queries_permissions_test.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: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (2)
cmd/api/src/api/v2/saved_queries_permissions_test.go (2)
53-60: LGTM!Clean helper function that improves test readability by succinctly constructing scope maps.
98-124: LGTM!Test fixtures for
userRoleUserandpowerUserare properly constructed with appropriate roles and permissions to exercise the expanded privilege checks.
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.
Fix: BED-6515
Description
Describe your changes in detail
Motivation and Context
Resolves BED-6515
Why is this change required? What problem does it solve?
To get rid of unexpected Permission Denied! error or "User" and "Power User" roles when they try to Edit/Share their own cypher queries
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.
Manually on the browser for the users with role "Administrator", "User" and "Power User" and by unit testing the code
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.