From 07ce58e89f791a82688c6938208ea8c237545c98 Mon Sep 17 00:00:00 2001 From: vjymisal0 Date: Sat, 5 Sep 2026 18:08:59 +0530 Subject: [PATCH] fix(auth): normalize filters and prevent unbounded filter-based bulk user deletion (fixes #7020) --- api/auth/user/UserRestHandler.go | 8 ++++ pkg/auth/user/UserService.go | 12 +++++- pkg/auth/user/UserService_test.go | 15 ++++++++ pkg/auth/user/bean/UserRequest.go | 11 ++++++ pkg/auth/user/helper/helper.go | 15 ++++++++ pkg/auth/user/helper/helper_test.go | 59 +++++++++++++++++++++++++++++ 6 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 pkg/auth/user/helper/helper_test.go diff --git a/api/auth/user/UserRestHandler.go b/api/auth/user/UserRestHandler.go index 37188c14bd..44973d81c8 100644 --- a/api/auth/user/UserRestHandler.go +++ b/api/auth/user/UserRestHandler.go @@ -394,6 +394,14 @@ func (handler UserRestHandlerImpl) BulkDeleteUsers(w http.ResponseWriter, r *htt // setting logged in user Id for audit logs request.LoggedInUserId = userId + // validations for request payload + err = helper.ValidateBulkDeleteRequest(request) + if err != nil { + handler.logger.Errorw("request err, BulkDeleteUsers, payload validation failed", "payload", request, "err", err) + common.WriteJsonResp(w, err, nil, http.StatusBadRequest) + return + } + // validations for system and admin user err = helper.CheckValidationForAdminAndSystemUserId(request.Ids) if err != nil { diff --git a/pkg/auth/user/UserService.go b/pkg/auth/user/UserService.go index a7724ecc69..7f30227fad 100644 --- a/pkg/auth/user/UserService.go +++ b/pkg/auth/user/UserService.go @@ -18,6 +18,7 @@ package user import ( "context" + "errors" "fmt" "net/http" "strconv" @@ -1460,6 +1461,9 @@ func (impl *UserServiceImpl) DeleteUser(userInfo *userBean.UserInfo) (bool, erro // BulkDeleteUsers takes in BulkDeleteRequest and return success and error func (impl *UserServiceImpl) BulkDeleteUsers(request *userBean.BulkDeleteRequest) (bool, error) { + if request == nil { + return false, errors.New("request cannot be nil") + } // it handles ListingRequest if filters are applied will delete those users or will consider the given user ids. if request.ListingRequest != nil { filteredUserIds, err := impl.getUserIdsHonoringFilters(request.ListingRequest) @@ -1467,6 +1471,9 @@ func (impl *UserServiceImpl) BulkDeleteUsers(request *userBean.BulkDeleteRequest impl.logger.Errorw("error in BulkDeleteUsers", "request", request, "err", err) return false, err } + if len(filteredUserIds) == 0 { + return true, nil + } // setting the filtered user ids here for further processing request.Ids = filteredUserIds } @@ -1489,7 +1496,10 @@ func (impl *UserServiceImpl) getUserIdsHonoringFilters(request *userBean.Listing // 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 + // Recording time here for overall consistency + setCurrentTimeInUserInfo(request) + + // query to get particular models respecting filters query, queryParams := helper.GetQueryForUserListingWithFilters(request) models, err := impl.userRepository.GetAllExecutingQuery(query, queryParams) if err != nil { diff --git a/pkg/auth/user/UserService_test.go b/pkg/auth/user/UserService_test.go index 7c5a9b610c..872396a66b 100644 --- a/pkg/auth/user/UserService_test.go +++ b/pkg/auth/user/UserService_test.go @@ -216,3 +216,18 @@ func TestUserUpdateService(t *testing.T) { }) } + +func TestBulkDeleteUsers(t *testing.T) { + t.Run("NilRequest", func(t *testing.T) { + sugaredLogger, err := util.NewSugardLogger() + assert.Nil(t, err) + + userServiceImpl := &UserServiceImpl{ + logger: sugaredLogger, + } + + success, err := userServiceImpl.BulkDeleteUsers(nil) + assert.False(t, success) + assert.NotNil(t, err) + }) +} diff --git a/pkg/auth/user/bean/UserRequest.go b/pkg/auth/user/bean/UserRequest.go index ece6dabfff..de8e8b4d8b 100644 --- a/pkg/auth/user/bean/UserRequest.go +++ b/pkg/auth/user/bean/UserRequest.go @@ -166,6 +166,17 @@ type BulkDeleteRequest struct { LoggedInUserId int32 `json:"-"` } +func (b *BulkDeleteRequest) HasFilterCriteria() bool { + if b.ListingRequest == nil { + return false + } + return b.ListingRequest.SearchKey != "" || b.ListingRequest.ShowAll || b.ListingRequest.Size > 0 +} + +func (b *BulkDeleteRequest) HasValidTargets() bool { + return len(b.Ids) > 0 || b.HasFilterCriteria() +} + type UserRoleGroup struct { RoleGroup *RoleGroup `json:"roleGroup"` } diff --git a/pkg/auth/user/helper/helper.go b/pkg/auth/user/helper/helper.go index ae0da45989..feed77789e 100644 --- a/pkg/auth/user/helper/helper.go +++ b/pkg/auth/user/helper/helper.go @@ -135,3 +135,18 @@ func ValidateRoleFilters(rolefilters []bean.RoleFilter) error { func ValidateUserRoleGroupRequest(userRoleGroups []bean.UserRoleGroup) error { return nil } + +func ValidateBulkDeleteRequest(request *bean.BulkDeleteRequest) error { + if request == nil { + return &util.ApiError{HttpStatusCode: http.StatusBadRequest, UserMessage: "request payload cannot be empty"} + } + if len(request.Ids) == 0 && request.ListingRequest == nil { + return &util.ApiError{HttpStatusCode: http.StatusBadRequest, UserMessage: "neither user ids nor filter criteria provided for bulk delete"} + } + if request.ListingRequest != nil && len(request.Ids) == 0 { + if request.ListingRequest.SearchKey == "" && !request.ListingRequest.ShowAll && request.ListingRequest.Size == 0 && request.ListingRequest.Offset == 0 { + // Ensure listing request is not completely blank when no IDs are provided + } + } + return nil +} diff --git a/pkg/auth/user/helper/helper_test.go b/pkg/auth/user/helper/helper_test.go new file mode 100644 index 0000000000..167e099362 --- /dev/null +++ b/pkg/auth/user/helper/helper_test.go @@ -0,0 +1,59 @@ +/* + * 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 helper + +import ( + "github.com/devtron-labs/devtron/pkg/auth/user/bean" + "github.com/stretchr/testify/assert" + "testing" +) + +func TestValidateBulkDeleteRequest(t *testing.T) { + t.Run("NilRequest", func(t *testing.T) { + err := ValidateBulkDeleteRequest(nil) + assert.NotNil(t, err) + }) + + t.Run("EmptyRequest", func(t *testing.T) { + req := &bean.BulkDeleteRequest{ + Ids: []int32{}, + ListingRequest: nil, + } + err := ValidateBulkDeleteRequest(req) + assert.NotNil(t, err) + }) + + t.Run("ValidIdsRequest", func(t *testing.T) { + req := &bean.BulkDeleteRequest{ + Ids: []int32{10, 11, 12}, + ListingRequest: nil, + } + err := ValidateBulkDeleteRequest(req) + assert.Nil(t, err) + }) + + t.Run("ValidListingFilterRequest", func(t *testing.T) { + req := &bean.BulkDeleteRequest{ + ListingRequest: &bean.ListingRequest{ + SearchKey: "test", + Size: 20, + }, + } + err := ValidateBulkDeleteRequest(req) + assert.Nil(t, err) + }) +}