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 ChangesETAC filtering and attack-path API
Go module updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ServiceTests
participant ETACService
participant AppDatabase
ServiceTests->>ETACService: FilterEnvironmentsByAccess(user, requestedIDs)
ETACService->>AppDatabase: Fetch allowed environment access
AppDatabase-->>ETACService: Allowed IDs or database error
ETACService-->>ServiceTests: Filtered IDs or error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
api
… input parameter validation/translation to exist in the services package
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/etac/internal/services/service.go (2)
86-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment omits the sentinel "deny all" return behavior.
The comment only describes the
nil"no filtering" case. It doesn't mention that the function returns[]string{""}when the user has no allowed environments or none of the requested IDs are permitted — a subtle contract that callers building SQL filters need to know about.📝 Proposed doc update
// FilterEnvironmentsByAccess returns the environment IDs the user is allowed to query. // It returns nil when ETAC filtering does not apply, such as when the user has // access to all environments. +// When ETAC filtering applies and the user has no accessible environments, or none +// of the requested IDs are accessible, it returns the sentinel []string{""} so that +// callers can build a filter that matches no results. func (s *Service) FilterEnvironmentsByAccess(ctx context.Context, user users.User, requestedIDs []string) ([]string, error) {🤖 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, Update the doc comment for Service.FilterEnvironmentsByAccess to document both sentinel outcomes: nil when ETAC filtering does not apply, and []string{""} when the user has no allowed environments or none of the requested IDs are permitted. Preserve the existing description of returning permitted environment IDs.
86-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate allow-list construction with
CheckUserAccessToEnvironments.Both
FilterEnvironmentsByAccess(lines 97-118) andCheckUserAccessToEnvironments(lines 142-150, shown as context) callGetEnvironmentTargetedAccessControlForUserand then build a set/map of allowed environment IDs from the result. Consider extracting a small private helper (e.g.,allowedEnvironmentSet(ctx, userID) (map[string]struct{}, error)) that both methods can share, to avoid the two implementations drifting apart over time.♻️ Proposed refactor sketch
+func (s *Service) allowedEnvironmentSet(ctx context.Context, userID uuid.UUID) (map[string]struct{}, error) { + allowedList, err := s.appdb.GetEnvironmentTargetedAccessControlForUser(ctx, userID) + if err != nil { + return nil, err + } + + allowedSet := make(map[string]struct{}, len(allowedList)) + for _, envAccess := range allowedList { + allowedSet[envAccess.EnvironmentID] = struct{}{} + } + return allowedSet, nil +} + func (s *Service) FilterEnvironmentsByAccess(ctx context.Context, user users.User, requestedIDs []string) ([]string, error) { if !s.ShouldFilterForETAC(user) { if len(requestedIDs) == 0 { return nil, nil } return requestedIDs, nil } - allowedList, err := s.appdb.GetEnvironmentTargetedAccessControlForUser(ctx, user.GetID()) + allowedSet, err := s.allowedEnvironmentSet(ctx, user.GetID()) if err != nil { return nil, err } - var allowlist []string - for _, envAccess := range allowedList { - allowlist = append(allowlist, envAccess.EnvironmentID) - } - - if len(allowlist) == 0 { + if len(allowedSet) == 0 { return []string{""}, nil } if len(requestedIDs) == 0 { - return allowlist, nil - } - - allowedSet := make(map[string]struct{}, len(allowlist)) - for _, environmentID := range allowlist { - allowedSet[environmentID] = struct{}{} + allowlist := make([]string, 0, len(allowedSet)) + for environmentID := range allowedSet { + allowlist = append(allowlist, environmentID) + } + return allowlist, nil } ...Note: switching to a map-first approach changes iteration order for the "no requestedIDs" full-allowlist case (map iteration is unordered), which would break the existing
TestService_FilterEnvironmentsByAccessassertion expecting[]string{"env-1", "env-2"}in order. If you adopt this refactor, either keep a parallel ordered slice or update that test to useElementsMatch.🤖 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 - 132, Extract the shared allow-list retrieval and environment-ID set construction from FilterEnvironmentsByAccess and CheckUserAccessToEnvironments into a private helper such as allowedEnvironmentSet, then use it in both methods. Preserve the existing ordered allowlist slice in FilterEnvironmentsByAccess for the no-requestedIDs case so its output order remains unchanged, while retaining current empty-access and intersection behavior.
🤖 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/etac/internal/services/service.go`:
- Around line 86-89: Update the doc comment for
Service.FilterEnvironmentsByAccess to document both sentinel outcomes: nil when
ETAC filtering does not apply, and []string{""} when the user has no allowed
environments or none of the requested IDs are permitted. Preserve the existing
description of returning permitted environment IDs.
- Around line 86-132: Extract the shared allow-list retrieval and environment-ID
set construction from FilterEnvironmentsByAccess and
CheckUserAccessToEnvironments into a private helper such as
allowedEnvironmentSet, then use it in both methods. Preserve the existing
ordered allowlist slice in FilterEnvironmentsByAccess for the no-requestedIDs
case so its output order remains unchanged, while retaining current empty-access
and intersection behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1150e6d8-4425-4358-be07-95238e936d09
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
server/etac/etac.goserver/etac/internal/services/service.goserver/etac/internal/services/service_test.goserver/etac/mocks/service.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/go/openapi/src/paths/attack-paths.attack-paths-finding-types.yaml`:
- Around line 39-44: Align the asset_group_tag_id filter description and
referenced predicate schema in
packages/go/openapi/src/paths/attack-paths.attack-paths-finding-types.yaml:39-44
by either documenting ~eq among the supported predicates or constraining the
schema to eq and neq, then regenerate
packages/go/openapi/doc/openapi.json:18667-18674 so the generated artifact
matches the corrected source.
🪄 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: 6e7ac8df-5457-43bd-8e8a-ea924477d200
📒 Files selected for processing (3)
packages/go/openapi/doc/openapi.jsonpackages/go/openapi/src/openapi.yamlpackages/go/openapi/src/paths/attack-paths.attack-paths-finding-types.yaml
3 participants
Description
This PR adds a new
GET /api/v2/attack-paths/finding-typesendpoint to support the attack path type filter in the findings table UI.The endpoint returns only attack path types that currently have findings associated with them, rather than all attack path types. It also respects environment scoping, ETAC access, asset group tag filtering, and the OpenGraph findings feature flag.
Motivation and Context
Resolves BED-8673
How Has This Been Tested?
Unit, Integration, Manual Testing
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
GET /api/v2/attack-paths/finding-typesto list distinct attack path finding types with environment and asset group tag filtering.