From 4b2bf385d81c45f3b9ca00eb76cfbebcbfb33c32 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 03:34:04 +0000 Subject: [PATCH] fix(user): scope filter-based bulk delete to match listing filters DELETE /orchestrator/user/bulk with a listingRequest filter (instead of explicit ids) resolved the delete target list via the same query builder the listing endpoint uses, but skipped the normalization the listing endpoint applies first (SetDefaultValuesIfNotPresent, setStatusFilterType). With no default page size and no status filtering applied, a filter such as {"searchKey":"","status":["inactive"]} resolved to every active user instead of the intended filtered subset, causing a bulk delete of far more users than intended. getUserIdsHonoringFilters now applies the same normalization the listing path (GetAllWithFilters) applies before building the query, and rejects a filter-based bulk delete that carries no concrete filter criteria instead of letting it silently resolve to "every user". The explicit ids-based delete path is unchanged. Fixes #7020 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01J5DesTG5XvPdmPCDuL6izq --- pkg/auth/user/UserService.go | 9 ++ .../UserService_bulkdelete_filter_test.go | 100 ++++++++++++++++++ pkg/auth/user/UserService_ent.go | 7 ++ 3 files changed, 116 insertions(+) create mode 100644 pkg/auth/user/UserService_bulkdelete_filter_test.go diff --git a/pkg/auth/user/UserService.go b/pkg/auth/user/UserService.go index 21320edd23..a7724ecc69 100644 --- a/pkg/auth/user/UserService.go +++ b/pkg/auth/user/UserService.go @@ -1480,6 +1480,15 @@ func (impl *UserServiceImpl) BulkDeleteUsers(request *userBean.BulkDeleteRequest // getUserIdsHonoringFilters get the filtered user ids according to the request filters and returns userIds and error(not nil) if any exception is caught. func (impl *UserServiceImpl) getUserIdsHonoringFilters(request *userBean.ListingRequest) ([]int32, error) { + // a filter based bulk delete must always be scoped by at least one concrete filter; an empty/default + // filter must never be allowed to silently widen into "delete every user" (no filter => no filter based bulk delete). + if len(strings.TrimSpace(request.SearchKey)) == 0 && !isStatusFilterApplied(request) { + return nil, &util.ApiError{Code: "400", HttpStatusCode: http.StatusBadRequest, UserMessage: "at least one filter criteria (searchKey or status) is required for a filter based bulk delete"} + } + // applying the same normalization the listing path (GetAllWithFilters) applies before resolving the + // query, so that filter resolution for delete matches filter resolution for listing exactly. + impl.userCommonService.SetDefaultValuesIfNotPresent(request, false) + setStatusFilterType(request) //query to get particular models respecting filters query, queryParams := helper.GetQueryForUserListingWithFilters(request) models, err := impl.userRepository.GetAllExecutingQuery(query, queryParams) diff --git a/pkg/auth/user/UserService_bulkdelete_filter_test.go b/pkg/auth/user/UserService_bulkdelete_filter_test.go new file mode 100644 index 0000000000..85aecd4654 --- /dev/null +++ b/pkg/auth/user/UserService_bulkdelete_filter_test.go @@ -0,0 +1,100 @@ +/* + * Copyright (c) 2024. Devtron Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package user + +import ( + "testing" + + "github.com/devtron-labs/devtron/pkg/auth/user/bean" + "github.com/devtron-labs/devtron/pkg/auth/user/repository" + "github.com/devtron-labs/devtron/pkg/auth/user/repository/helper" + "github.com/stretchr/testify/assert" +) + +// fakeUserRepoForFilterTest is a minimal repository.UserRepository fake that only answers +// GetAllExecutingQuery - the single repository call getUserIdsHonoringFilters makes. Every other +// method is left to panic on the embedded nil interface, since these tests never expect them to run. +type fakeUserRepoForFilterTest struct { + repository.UserRepository + called bool + gotQuery string + gotParams []interface{} + modelsToRet []repository.UserModel +} + +func (f *fakeUserRepoForFilterTest) GetAllExecutingQuery(query string, queryParams []interface{}) ([]repository.UserModel, error) { + f.called = true + f.gotQuery = query + f.gotParams = queryParams + return f.modelsToRet, nil +} + +func newTestUserServiceForFilterTest(repo *fakeUserRepoForFilterTest) *UserServiceImpl { + return &UserServiceImpl{ + userRepository: repo, + userCommonService: &UserCommonServiceImpl{}, + } +} + +// TestGetUserIdsHonoringFilters_RejectsUnscopedFilter is a regression test for the production incident +// where `DELETE /orchestrator/user/bulk` with body {"listingRequest":{"searchKey":"","status":["inactive"]}} +// (an empty search key, intended to be narrowed down by status) deleted every active user on the platform +// instead of only the intended inactive ones. The delete path resolved the filter directly through the +// query builder without the normalization/bound the listing endpoint applies, so an effectively empty +// filter silently widened into "every user". With the fix, a listingRequest carrying no concrete filter +// (status isn't a filterable field in this OSS build) is rejected outright instead of being resolved at all. +func TestGetUserIdsHonoringFilters_RejectsUnscopedFilter(t *testing.T) { + repo := &fakeUserRepoForFilterTest{} + impl := newTestUserServiceForFilterTest(repo) + + // mirrors the exact incident payload once decoded: SearchKey is empty and there is no other + // concrete filter field available on this build's ListingRequest. + request := &bean.ListingRequest{SearchKey: ""} + + ids, err := impl.getUserIdsHonoringFilters(request) + + assert.Error(t, err) + assert.Nil(t, ids) + assert.False(t, repo.called, "no query should ever be executed for an unscoped filter based bulk delete") +} + +// TestGetUserIdsHonoringFilters_MatchesListingQuery asserts that once a concrete filter is supplied, the +// query getUserIdsHonoringFilters resolves is identical - query text and params - to the query the listing +// endpoint (GetAllWithFilters) would build for the exact same input. This is the core of the fix: delete-side +// filter resolution can no longer diverge from listing-side filter resolution (e.g. via a missing default +// page size), so the delete path's resolved id set only ever contains records actually matching the filter. +func TestGetUserIdsHonoringFilters_MatchesListingQuery(t *testing.T) { + matchingUser := repository.UserModel{Id: 42, EmailId: "inactive-user@example.com"} + repo := &fakeUserRepoForFilterTest{modelsToRet: []repository.UserModel{matchingUser}} + impl := newTestUserServiceForFilterTest(repo) + + deleteRequest := &bean.ListingRequest{SearchKey: "inactive-user"} + ids, err := impl.getUserIdsHonoringFilters(deleteRequest) + assert.NoError(t, err) + assert.Equal(t, []int32{42}, ids) + assert.True(t, repo.called) + + // Build the query the listing endpoint would run for the exact same input, applying the exact + // same normalization GetAllWithFilters applies before calling the shared query builder. + listingRequest := &bean.ListingRequest{SearchKey: "inactive-user"} + (&UserCommonServiceImpl{}).SetDefaultValuesIfNotPresent(listingRequest, false) + setStatusFilterType(listingRequest) + listingQuery, listingParams := helper.GetQueryForUserListingWithFilters(listingRequest) + + assert.Equal(t, listingQuery, repo.gotQuery, "delete path query must match listing path query for the same filter") + assert.Equal(t, listingParams, repo.gotParams, "delete path query params must match listing path query params for the same filter") +} diff --git a/pkg/auth/user/UserService_ent.go b/pkg/auth/user/UserService_ent.go index f576f49da3..ab1dc4e6b8 100644 --- a/pkg/auth/user/UserService_ent.go +++ b/pkg/auth/user/UserService_ent.go @@ -119,6 +119,13 @@ func setStatusFilterType(request *userBean.ListingRequest) { return } +// isStatusFilterApplied reports whether the request carries a concrete status filter. +// the OSS ListingRequest has no status filtering concept, so this is always false here; +// an enterprise build that adds status filtering should override this accordingly. +func isStatusFilterApplied(request *userBean.ListingRequest) bool { + return false +} + func setCurrentTimeInUserInfo(request *userBean.ListingRequest) { return }