Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ETAC-backed node authorization, environment ID extraction, handler enforcement with 403 responses, user-resolution contracts, and dependency wiring through GraphDB registration. ChangesGraph node authorization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GetNodeByID
participant GraphDB
participant NodeAuthorizer
participant ETACService
participant AppDB
Client->>GetNodeByID: Request node by ID
GetNodeByID->>GraphDB: Load node
GraphDB-->>GetNodeByID: Node
GetNodeByID->>NodeAuthorizer: CanAccessNode(context, node)
NodeAuthorizer->>ETACService: CheckUserAccess(user, environmentID)
ETACService->>AppDB: Query allowed environments
AppDB-->>ETACService: Environment access records
ETACService-->>NodeAuthorizer: Access decision
NodeAuthorizer-->>GetNodeByID: Allow or deny
GetNodeByID-->>Client: 200 node or 403 forbidden
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/graphdb/internal/handlers/handlers.go`:
- Line 45: Update the remaining NewHandlersContainer caller in
graphdb_e2e_test.go to pass a NodeAuthorizer implementation, matching the setup
used by the updated handler tests, so the server/graphdb test package compiles
with the new constructor signature.
In `@server/graphdb/internal/handlers/mocks/nodeauthorizer.go`:
- Line 1: Update the mockery configuration or template used to generate
nodeauthorizer mocks so output includes LICENSE.header, uses the repository’s
approved receiver naming instead of _m/_mock, and replaces interface{} with the
preferred Go type syntax in helper signatures; regenerate nodeauthorizer.go and
verify the generated file follows repository Go policies.
In `@server/graphdb/internal/routes/routes_test.go`:
- Around line 48-53: The remaining handlers.NewHandlersContainer call in
graphdb_e2e_test.go must be updated to match its new two-argument signature.
Pass the existing services.NewService(store) GraphDB value and provide an
appropriate NodeAuthorizer implementation or mock, consistent with the setup in
routes_test.go, so the test typechecks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5ec9909a-f796-4d84-8cda-68f11f46556a
📒 Files selected for processing (12)
server/graphdb/graphdb.goserver/graphdb/internal/authz/authorizer.goserver/graphdb/internal/authz/authorizer_test.goserver/graphdb/internal/handlers/handlers.goserver/graphdb/internal/handlers/mocks/nodeauthorizer.goserver/graphdb/internal/handlers/node.goserver/graphdb/internal/handlers/node_test.goserver/graphdb/internal/handlers/relationship_test.goserver/graphdb/internal/routes/routes_test.goserver/graphdb/internal/services/node.goserver/modules/modules.goserver/modules/modules_test.go
Rewrote authorizer to use the ETAC slice rather than the old helper
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/graphdb/internal/authz/authorizer_test.go (1)
80-198: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a test case for the ETAC error path.
None of the table cases mock
CheckUserAccessto return an error, so the deny-on-error branch (authorizer.golines 51-53: log +return false) is untested. Given this is the default-deny security path, it's worth covering explicitly.✅ Suggested additional case
{ name: "ETAC service error should be denied", ctx: ctxWithUser(userWithEnvironments(false, testDomainSID)), node: newTestNode(ad.Entity.String(), ad.DomainSID.String(), testDomainSID), want: false, mockSetup: func(m *etacMocks.MockService, ctx context.Context, node services.Node) { m.EXPECT().CheckUserAccess(ctx, &model.User{ AllEnvironments: false, EnvironmentTargetedAccessControl: []model.EnvironmentTargetedAccessControl{ {EnvironmentID: testDomainSID}, }, }, testDomainSID).Return(false, errors.New("etac unavailable")) }, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/graphdb/internal/authz/authorizer_test.go` around lines 80 - 198, Add a table entry to TestNodeAuthorizer_CanAccessNode named “ETAC service error should be denied,” using the existing user, AD node, and testDomainSID setup; configure CheckUserAccess to return false with an errors.New error and assert want is false, importing errors if needed.
🧹 Nitpick comments (3)
server/etac/internal/services/service.go (2)
73-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment casing doesn't match the unexported identifier.
The comment refers to
ShouldFilterForETACbut the method is unexportedshouldFilterForETAC.✏️ Fix comment casing
-// ShouldFilterForETAC determines whether ETAC filtering should be applied based +// shouldFilterForETAC determines whether ETAC filtering should be applied based // on the feature flag and the user's environment access. Filtering is skipped // when the feature is disabled or when the user has access to all environments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/etac/internal/services/service.go` around lines 73 - 76, Update the doc comment above shouldFilterForETAC to begin with the exact unexported method name, shouldFilterForETAC, instead of ShouldFilterForETAC.
86-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment casing doesn't match the exported identifier; also reconsider exporting this method solely for test access.
The comment says
checkUserAccessToEnvironmentsbut the method is exported asCheckUserAccessToEnvironments. Additionally, this method appears exported only soservice_test.go(externalservices_testpackage) can exercise the otherwise-unreachable all-environments short-circuit branch — this expands the public API surface for test convenience. Consider keeping it unexported and covering that branch via an internalpackage servicestest file instead, per the coding guideline that internal-logic tests may live in the same package as the code under test.As per coding guidelines, "If a test file is testing internal (non-exported) logic, the file may be included in the same package as the code being tested."✏️ Fix comment casing (minimal fix if keeping it exported)
-// checkUserAccessToEnvironments reports whether the user is permitted to access +// CheckUserAccessToEnvironments reports whether the user is permitted to access // every environment in the supplied list. Users with access to all environments // are always permitted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/etac/internal/services/service.go` around lines 86 - 89, Keep CheckUserAccessToEnvironments unexported as checkUserAccessToEnvironments unless it is genuinely required by production callers, and move the short-circuit coverage into an internal package services test so service_test.go does not require the exported API. Update all internal call sites and tests accordingly; if the method must remain exported, change its doc comment to begin with CheckUserAccessToEnvironments.Source: Coding guidelines
server/etac/internal/appdb/appdb_test.go (1)
82-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCoverage gap: the row-scan error branch (
fmt.Errorf("reading rows: ...")) is untested.
WillReturnErrorfails atQuery, never reachingpgx.CollectRows/RowToStructByName. Consider a case that returns malformed rows (e.g., wrong column types) to exercise that wrap path, especially once switched to%w.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/etac/internal/appdb/appdb_test.go` around lines 82 - 90, Extend the “propagates database errors” tests for GetEnvironmentTargetedAccessControlForUser with a malformed successful query result that causes pgx.CollectRows/RowToStructByName to fail during scanning, then assert the returned error wraps the scan error (including via errors.Is if applicable) and verify pool expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/etac/internal/appdb/appdb.go`:
- Around line 82-88: In the row collection error handling within the appdb query
function, change the fmt.Errorf wrapper from the non-wrapping %s verb to %w so
callers can inspect the underlying error via errors.Is or errors.As, matching
the existing Query error behavior.
In `@server/graphdb/internal/authz/authorizer.go`:
- Around line 34-38: Update the e2e harness setup in graphdb_e2e_test.go to
construct a NodeAuthorizer via NewNodeAuthorizer using the available ETAC
service, then pass it to handlers.NewHandlersContainer alongside
services.NewService(store), matching the constructor’s new signature.
---
Outside diff comments:
In `@server/graphdb/internal/authz/authorizer_test.go`:
- Around line 80-198: Add a table entry to TestNodeAuthorizer_CanAccessNode
named “ETAC service error should be denied,” using the existing user, AD node,
and testDomainSID setup; configure CheckUserAccess to return false with an
errors.New error and assert want is false, importing errors if needed.
---
Nitpick comments:
In `@server/etac/internal/appdb/appdb_test.go`:
- Around line 82-90: Extend the “propagates database errors” tests for
GetEnvironmentTargetedAccessControlForUser with a malformed successful query
result that causes pgx.CollectRows/RowToStructByName to fail during scanning,
then assert the returned error wraps the scan error (including via errors.Is if
applicable) and verify pool expectations.
In `@server/etac/internal/services/service.go`:
- Around line 73-76: Update the doc comment above shouldFilterForETAC to begin
with the exact unexported method name, shouldFilterForETAC, instead of
ShouldFilterForETAC.
- Around line 86-89: Keep CheckUserAccessToEnvironments unexported as
checkUserAccessToEnvironments unless it is genuinely required by production
callers, and move the short-circuit coverage into an internal package services
test so service_test.go does not require the exported API. Update all internal
call sites and tests accordingly; if the method must remain exported, change its
doc comment to begin with CheckUserAccessToEnvironments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d107f7df-3fbc-4fee-a8e0-32224fc625b7
📒 Files selected for processing (15)
server/etac/etac.goserver/etac/internal/appdb/appdb.goserver/etac/internal/appdb/appdb_integration_test.goserver/etac/internal/appdb/appdb_test.goserver/etac/internal/services/mocks/appdatabase.goserver/etac/internal/services/service.goserver/etac/internal/services/service_test.goserver/etac/mocks/service.goserver/graphdb/graphdb.goserver/graphdb/internal/authz/authorizer.goserver/graphdb/internal/authz/authorizer_test.goserver/graphdb/internal/handlers/mocks/nodeauthorizer.goserver/users/mocks/resolver.goserver/users/mocks/user.goserver/users/users.go
✅ Files skipped from review due to trivial changes (5)
- server/users/mocks/resolver.go
- server/graphdb/internal/handlers/mocks/nodeauthorizer.go
- server/users/mocks/user.go
- server/etac/mocks/service.go
- server/etac/internal/services/mocks/appdatabase.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/graphdb/graphdb.go
| accessControlList, err := pgx.CollectRows(rows, func(row pgx.CollectableRow) (services.EnvironmentTargetedAccessControl, error) { | ||
| scanned, err := pgx.RowToStructByName[etacRow](row) | ||
| return toEnvironmentTargetedAccessControl(scanned), err | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("reading rows: %s", err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use %w to preserve the error chain when wrapping row-scan errors.
fmt.Errorf("reading rows: %s", err) discards the underlying error, so callers can't errors.Is/errors.As against it — unlike the raw Query error returned a few lines above.
🐛 Proposed fix
- return nil, fmt.Errorf("reading rows: %s", err)
+ return nil, fmt.Errorf("reading rows: %w", err)📝 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.
| accessControlList, err := pgx.CollectRows(rows, func(row pgx.CollectableRow) (services.EnvironmentTargetedAccessControl, error) { | |
| scanned, err := pgx.RowToStructByName[etacRow](row) | |
| return toEnvironmentTargetedAccessControl(scanned), err | |
| }) | |
| if err != nil { | |
| return nil, fmt.Errorf("reading rows: %s", err) | |
| } | |
| accessControlList, err := pgx.CollectRows(rows, func(row pgx.CollectableRow) (services.EnvironmentTargetedAccessControl, error) { | |
| scanned, err := pgx.RowToStructByName[etacRow](row) | |
| return toEnvironmentTargetedAccessControl(scanned), err | |
| }) | |
| if err != nil { | |
| return nil, fmt.Errorf("reading rows: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/etac/internal/appdb/appdb.go` around lines 82 - 88, In the row
collection error handling within the appdb query function, change the fmt.Errorf
wrapper from the non-wrapping %s verb to %w so callers can inspect the
underlying error via errors.Is or errors.As, matching the existing Query error
behavior.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/graphdb/graphdb_e2e_test.go (2)
106-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo e2e coverage for the 403 forbidden path introduced by this PR.
withAuthenticatedUseralways injects a user withAllEnvironments: true, soCanAccessNodealways allows access in this harness.TestGetNodeByIDonly exercises 200/400/404; there is no test assertingGetNodeByIDreturns 403 when the ETAC authorizer denies access — the exact behavior this PR wires end-to-end.Consider adding a sub-middleware/test case using a restricted user (or a second harness variant) to assert the 403 response from
s.nodeAuthorizer.CanAccessNodedenial, since unit tests onNodeAuthorizerdon't verify the full HTTP-level enforcement.Also applies to: 291-359
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/graphdb/graphdb_e2e_test.go` around lines 106 - 119, The graph database e2e tests lack HTTP-level coverage for authorization denial. Add a restricted-user middleware or harness variant alongside withAuthenticatedUser, configure the ETAC authorizer to deny access, and extend TestGetNodeByID with a case asserting GetNodeByID returns HTTP 403.
160-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist new variable declarations into a
var(...)block.
etacService,nodeAuthorizer, andhandlerSetaren't error-checked immediately, so they can be grouped into avar(...)block per the repo's Go style.As per coding guidelines, "Group variable initializations in a `var ( ... )` block and hoist them to the top of the function when possible."♻️ Proposed grouping
- store := appdb.NewStore(graphDatabase, dbPool) - etacService := etac.Register(dbPool, dogtags.NewDefaultService()) - nodeAuthorizer := authz.NewNodeAuthorizer(etacService) - handlerSet := handlers.NewHandlersContainer(services.NewService(store), nodeAuthorizer) + var ( + store = appdb.NewStore(graphDatabase, dbPool) + etacService = etac.Register(dbPool, dogtags.NewDefaultService()) + nodeAuthorizer = authz.NewNodeAuthorizer(etacService) + handlerSet = handlers.NewHandlersContainer(services.NewService(store), nodeAuthorizer) + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/graphdb/graphdb_e2e_test.go` around lines 160 - 163, Group the `etacService`, `nodeAuthorizer`, and `handlerSet` declarations into a single var(...) block near the top of the containing test function, preserving their existing initialization expressions and dependency order; leave the error-checked `store` initialization separate if needed.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/graphdb/graphdb_e2e_test.go`:
- Around line 106-119: The graph database e2e tests lack HTTP-level coverage for
authorization denial. Add a restricted-user middleware or harness variant
alongside withAuthenticatedUser, configure the ETAC authorizer to deny access,
and extend TestGetNodeByID with a case asserting GetNodeByID returns HTTP 403.
- Around line 160-163: Group the `etacService`, `nodeAuthorizer`, and
`handlerSet` declarations into a single var(...) block near the top of the
containing test function, preserving their existing initialization expressions
and dependency order; leave the error-checked `store` initialization separate if
needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4f85cb5a-9031-4236-9af4-eac0ccdebb0b
📒 Files selected for processing (2)
server/graphdb/graphdb_e2e_test.goserver/graphdb/internal/authz/authorizer_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/graphdb/internal/authz/authorizer_test.go
There was a problem hiding this comment.
The ETAC service has been migrated from BHE
| @@ -0,0 +1,61 @@ | |||
| // Copyright 2026 Specter Ops, Inc. | |||
There was a problem hiding this comment.
The user service has been migrated from BHE
urangel
left a comment
urangel
left a comment
There was a problem hiding this comment.
Thank you both for the huge lift! @ddlees and @superlinkx
3 participants
Description
Motivation and Context
Resolves BED-8945
How Has This Been Tested?
Added and updated all unit tests in graphdb onion
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
"forbidden".