Skip to content

Bed 6420 etac #2021

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
stephanieslamb wants to merge 15 commits into
base: main
Choose a base branch
from
+389 −34
Draft

Bed 6420 etac #2021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2bc3f12
started new branch. handlers updated, unit tests working, still work…
stephanieslamb Oct 23, 2025
6fa906e
BED-6420 reworked ETAC logic, started working on unit tests
mvlipka Oct 24, 2025
ea86c8e
working on additional unit tests
stephanieslamb Oct 27, 2025
48ea175
added comparing node counts to members/counts endpoint HS tests. wor…
stephanieslamb Oct 29, 2025
2b28d78
applying additional filters in graph query. stubbing out HS tests
stephanieslamb Nov 3, 2025
aa8ba9c
Merge branch 'main' into BED-6420-etac
stephanieslamb Nov 3, 2025
0b8e038
pfc
stephanieslamb Nov 3, 2025
41ef145
fixing unit test
stephanieslamb Nov 3, 2025
10790b9
fixed broken unit tests
stephanieslamb Nov 3, 2025
f9c7018
updated graph query
stephanieslamb Nov 3, 2025
b779d6a
missed log
stephanieslamb Nov 3, 2025
8aa1456
updates from peer review. moving some logic out of for loop
stephanieslamb Nov 5, 2025
01ab874
clean up
stephanieslamb Nov 5, 2025
4e088d5
rabbit suggestion
stephanieslamb Nov 5, 2025
79c3220
moved building filters into check for consistency
stephanieslamb Nov 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 72 additions & 6 deletions cmd/api/src/api/v2/assetgrouptags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,11 @@ type assetGroupTagSelectorRequest struct {

func (s Resources) GetAssetGroupTags(response http.ResponseWriter, request *http.Request) {
var rCtx = request.Context()

if paramIncludeCounts, err := api.ParseOptionalBool(request.URL.Query().Get(api.QueryParameterIncludeCounts), false); err != nil {
if user, isUser := auth.GetUserFromAuthCtx(ctx.FromRequest(request).AuthCtx); !isUser {
slog.Error("Unable to get user from auth context")
api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, "unknown user", request), response)
return
} else if paramIncludeCounts, err := api.ParseOptionalBool(request.URL.Query().Get(api.QueryParameterIncludeCounts), false); err != nil {
api.WriteErrorResponse(rCtx, api.BuildErrorResponse(http.StatusBadRequest, "Invalid value specified for include counts", request), response)
} else if queryFilters, err := model.NewQueryParameterFilterParser().ParseQueryParameterFilters(request); err != nil {
api.WriteErrorResponse(rCtx, api.BuildErrorResponse(http.StatusBadRequest, api.ErrorResponseDetailsBadQueryParameterFilters, request), response)
Expand Down Expand Up @@ -112,6 +115,8 @@ func (s Resources) GetAssetGroupTags(response http.ResponseWriter, request *http
Tags: make([]AssetGroupTagView, 0, len(tags)),
}
selectorCounts map[int]int
accessList = []string{}
accessFilters = []graph.Criteria{}
)

if paramIncludeCounts {
Expand All @@ -123,12 +128,46 @@ func (s Resources) GetAssetGroupTags(response http.ResponseWriter, request *http
api.HandleDatabaseError(request, response, err)
return
}

}

etacFlag, err := s.DB.GetFlagByKey(request.Context(), appcfg.FeatureETAC)
if err != nil {
api.HandleDatabaseError(request, response, err)
return
}

if paramIncludeCounts && etacFlag.Enabled {
accessList = ExtractEnvironmentIDsFromUser(&user)

// only build accessFilters if user has some targeted access and is allEnvironments is false
if !user.AllEnvironments && len(accessList) > 0 {
accessFilters = []graph.Criteria{
query.Or(
query.In(query.NodeProperty(ad.DomainSID.String()), accessList),
query.In(query.NodeProperty(azure.TenantID.String()), accessList),
),
}
}
}
for _, tag := range tags {
tview := AssetGroupTagView{AssetGroupTag: tag}

if paramIncludeCounts {
if n, err := s.GraphQuery.CountNodesByKind(rCtx, tag.ToKind()); err != nil {
// user has no access therefore should see no nodes
if etacFlag.Enabled && !user.AllEnvironments && len(accessList) == 0 {
tview.Counts = &AssetGroupTagCounts{
Selectors: selectorCounts[tag.ID],
Members: 0,
}
resp.Tags = append(resp.Tags, tview)
continue
}

// clone accessFilters so each tag call receives its own slice
filters := append([]graph.Criteria{}, accessFilters...)

if n, err := s.GraphQuery.CountNodesByKind(rCtx, filters, tag.ToKind()); err != nil {
api.HandleDatabaseError(request, response, err)
return
} else {
Expand All @@ -137,7 +176,9 @@ func (s Resources) GetAssetGroupTags(response http.ResponseWriter, request *http
Members: n,
}
}

}

resp.Tags = append(resp.Tags, tview)
}
api.WriteBasicResponse(rCtx, resp, http.StatusOK, response)
Expand Down Expand Up @@ -568,21 +609,46 @@ type GetAssetGroupTagMemberCountsResponse struct {
}

func (s *Resources) GetAssetGroupTagMemberCountsByKind(response http.ResponseWriter, request *http.Request) {
environmentIds := request.URL.Query()[api.QueryParameterEnvironments]
var (
environmentIds = request.URL.Query()[api.QueryParameterEnvironments]
filters []graph.Criteria
)

if tagId, err := strconv.Atoi(mux.Vars(request)[api.URIPathVariableAssetGroupTagID]); err != nil {
if user, isUser := auth.GetUserFromAuthCtx(ctx.FromRequest(request).AuthCtx); !isUser {
slog.Error("Unable to get user from auth context")
api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, "unknown user", request), response)
return
} else if tagId, err := strconv.Atoi(mux.Vars(request)[api.URIPathVariableAssetGroupTagID]); err != nil {
api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusNotFound, api.ErrorResponseDetailsIDMalformed, request), response)
} else if tag, err := s.DB.GetAssetGroupTag(request.Context(), tagId); err != nil {
api.HandleDatabaseError(request, response, err)
} else {
filters := []graph.Criteria{}
if len(environmentIds) > 0 {
filters = append(filters, query.Or(
query.In(query.NodeProperty(ad.DomainSID.String()), environmentIds),
query.In(query.NodeProperty(azure.TenantID.String()), environmentIds),
))
}

// ETAC feature etacFlag
if etacFlag, err := s.DB.GetFlagByKey(request.Context(), appcfg.FeatureETAC); err != nil {
api.HandleDatabaseError(request, response, err)
return
} else {
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),
),
)
}
}
}
Comment on lines +638 to +650
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


primaryNodeKindsCounts, err := s.GraphQuery.GetPrimaryNodeKindCounts(request.Context(), tag.ToKind(), filters...)
if err != nil {
api.HandleDatabaseError(request, response, err)
Expand Down
Loading