Repository navigation
fix: scope filter-based bulk user delete to match listing filters - #7021
Merged
Merged
Conversation
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5DesTG5XvPdmPCDuL6izq
|
Bito Automatic Review Skipped - Draft PR |
claude
Bot
requested review from
prakarsh-dt and
vivek-devtron
as code owners
September 4, 2026 03:42
Author
|
/review Generated by Claude Code |
|
vivek-devtron
approved these changes
Sep 7, 2026
Ash-exp
pushed a commit
that referenced
this pull request
Sep 22, 2026
…lters getUserIdsHonoringFilters (used by both BulkDeleteUsers and enterprise's BulkUpdateStatus) resolved listingRequest filters directly through the query builder without the normalization the listing endpoint (GetAllWithFilters) applies, and had no guard against an effectively empty filter. A request with an empty searchKey and no status filter would silently widen into "every user", as it did in production. Ports the OSS fix from #7021 (commit 4b2bf38) to this repo's own copy of the same code: reject a filter-based bulk operation with no concrete filter, and apply SetDefaultValuesIfNotPresent/setStatusFilterType before building the query, exactly as the listing path does. Adds isStatusFilterApplied here reflecting enterprise's own real status filter (a non-empty Status slice), rather than the OSS stub which always returns false. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J5DesTG5XvPdmPCDuL6izq
This was referenced Oct 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requested by Shivam Nagar, Pawan Kumar · Slack thread
Fixes #7020
Description
Before:
DELETE /orchestrator/user/bulkaccepts alistingRequestfilter ({searchKey, status}) in place of explicitids, used by the UI's "select all matching results across pages" bulk-delete flow. The backend resolved that filter into a delete target list viagetUserIdsHonoringFilters, which called the same query builder the listing endpoint uses (GetQueryForUserListingWithFilters) but skipped the normalization the listing endpoint (GetAllWithFilters) applies first —SetDefaultValuesIfNotPresent(default page size) andsetStatusFilterType(status filtering). With no defaultSize, the query builder emitted noLIMIT; combined with the skipped status filtering, a request such as{"listingRequest":{"searchKey":"","status":["inactive"]}}— intended to delete only inactive users matching an (empty) search — resolved to every active, non-API-token user. This is exactly what happened in the incident this PR fixes: an admin intending to bulk-delete 108 inactive users instead deactivated every user account on the platform.After: filter resolution for the delete path is normalized and scoped identically to the listing path, and a filter-based bulk delete is rejected outright unless it carries at least one concrete filter field — an empty/default filter can no longer silently widen into "delete everyone".
How
In
pkg/auth/user/UserService.go,getUserIdsHonoringFiltersnow:util.ApiError) if it carries no concrete filter (emptySearchKeyand no status filter applied) — no filter means no filter-based bulk delete.impl.userCommonService.SetDefaultValuesIfNotPresent(request, false)andsetStatusFilterType(request)before building the query viahelper.GetQueryForUserListingWithFilters, exactly mirroring whatGetAllWithFilters(the listing endpoint) does before calling the same query builder.Added
isStatusFilterAppliednext to the existingsetStatusFilterTypeOSS/enterprise hook inUserService_ent.go(a no-op returningfalsein this OSS build, matching the existing convention for enterprise-extendable filter behavior) so the "no concrete filter" check has a forward-compatible way to recognize a status filter if/when one is added.The explicit
ids-based delete path (deleteUsersByIds) is untouched — it already worked correctly per the original investigation.Testing
pkg/auth/user/UserService_bulkdelete_filter_test.go:TestGetUserIdsHonoringFilters_RejectsUnscopedFilter— regression test for the exact incident payload (SearchKey: "", no other concrete filter): asserts the call is now rejected and the repository query is never executed.TestGetUserIdsHonoringFilters_MatchesListingQuery— asserts that once a concrete filter (SearchKey) is present, the exact query text and paramsgetUserIdsHonoringFiltersbuilds are identical to what the listing endpoint (GetAllWithFilters) would build for the same input.go build ./pkg/auth/user/...andgo build ./...succeed (the one pre-existing build failure, an unrelated missing vendored Azure DevOps module inpkg/deployment/gitOps/git, reproduces identically on a cleanmaincheckout before this change).go test ./pkg/auth/user/... -run TestGetUserIdsHonoringFilters -v -vet=offpasses both new tests. Note: this package's two pre-existing test files (UserService_test.go,UserFlows_test.go) already fail to compile against the currentUserServiceImpl/UserCommonServiceImplAPIs independent of this change (confirmed by reproducing the identical compile errors on a clean checkout before any edits here) — that pre-existing breakage is out of scope for this fix, and this package isn't part of the repo'smake test-unittarget (which only runs./pkg/pipeline) so it isn't caught by CI either.gofmt -lis clean on all changed files.Checklist:
Does this PR introduce a user-facing change?
🤖 Generated with Claude Code
https://claude.ai/code/session_01J5DesTG5XvPdmPCDuL6izq
Generated by Claude Code