Skip to content

Commit 435aaed

Browse files
committed
fix: bound a bulk user action
Closes the half I left open when the last review pointed out that the honest bound for a bulk action is on the caller's id list. Nothing upstream provided one: the ids arrive straight off a JSON decode from the panel, /v1 and the post_users_bulk MCP tool. Each id costs a row read, and a delete costs a webhook delivery per subscriber on a 512-slot queue that drops when full — so an unbounded list quietly lost the very notifications it generated, while holding the single DB connection for as long as it took. Capped at 1000, which no operator meets working through the panel. The test is worth a note. The first version built a bare Manager and asserted "some error", and it passed with the bound removed — the nil store was erroring on its own. That is exactly the tautological-test trap the previous pass caught twice elsewhere, so this one runs against a real store, checks an ordinary selection still goes through, and asserts the specific error code. Mutation-checked: deleting the bound fails it.
1 parent a1375b3 commit 435aaed

5 files changed

Lines changed: 65 additions & 0 deletions

File tree

internal/core/manager_users.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,11 @@ func (m *Manager) SetUserLimits(ctx context.Context, id, dataLimit, expireAt int
198198
return err
199199
}
200200

201+
// maxBulkUsers caps one bulk action. High enough that no operator meets it working
202+
// through the panel, low enough that a single call cannot monopolise the one DB
203+
// connection or overrun the webhook queue.
204+
const maxBulkUsers = 1000
205+
201206
// BulkUserAction applies one action to many users with a SINGLE config sync at the
202207
// end (instead of one per user), returning how many users were actually affected.
203208
// Actions: "enable", "disable", "delete", "reset" (traffic), "extend" (push expiry
@@ -207,6 +212,16 @@ func (m *Manager) BulkUserAction(ctx context.Context, ids []int64, action string
207212
if len(ids) == 0 {
208213
return 0, invalidCode("err.noUsersSelected", "не выбрано ни одного пользователя")
209214
}
215+
// A bound, because nothing upstream provides one: the id list arrives straight off a
216+
// JSON decode from the panel, /v1 and the post_users_bulk MCP tool. Every id costs a
217+
// row read, and a delete costs a webhook delivery per subscriber on a 512-slot queue
218+
// that drops when full — so an unbounded list quietly loses the very notifications it
219+
// generates, and holds the single DB connection for as long as it takes.
220+
if len(ids) > maxBulkUsers {
221+
return 0, invalidCode("err.tooManyUsersSelected",
222+
"за один раз можно обработать не больше {{max}} пользователей",
223+
map[string]any{"max": maxBulkUsers})
224+
}
210225
// Snapshot the users up front. The audit rows for a bulk DELETE can't look their
211226
// names up afterwards; an id that isn't in the snapshot doesn't exist (so this
212227
// doubles as the "which ids are real" filter); and the prior state is what tells a
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
package core
2+
3+
import (
4+
"context"
5+
"errors"
6+
"path/filepath"
7+
"testing"
8+
9+
"github.com/AppsGanin/rospanel/internal/store"
10+
)
11+
12+
// A bulk action is reachable from the panel, /v1 and the post_users_bulk MCP tool, and
13+
// the id list arrives straight off a JSON decode with no bound anywhere upstream. Every
14+
// id costs a row read, and a delete costs a webhook delivery per subscriber on a queue
15+
// that drops when full — so an unbounded list silently loses the notifications it
16+
// generates while holding the single DB connection.
17+
func TestBulkUserActionIsBounded(t *testing.T) {
18+
// A real store, because the point is that the BOUND refuses the call — with a bare
19+
// Manager the nil store errors on its own and the test would pass either way. (It
20+
// did, the first time I wrote it.)
21+
st, err := store.Open(filepath.Join(t.TempDir(), "bulk.db"))
22+
if err != nil {
23+
t.Fatalf("open: %v", err)
24+
}
25+
t.Cleanup(func() { st.Close() })
26+
m := &Manager{store: st}
27+
28+
u, err := st.CreateUser("bulk-one", "uuid", "pw", "tok", 0, 0, 0)
29+
if err != nil {
30+
t.Fatalf("create user: %v", err)
31+
}
32+
33+
// An ordinary selection goes through: the bound must not refuse real work.
34+
if _, err := m.BulkUserAction(context.Background(), []int64{u.ID}, "disable", 0); err != nil {
35+
t.Fatalf("an ordinary bulk action was refused: %v", err)
36+
}
37+
38+
ids := make([]int64, maxBulkUsers+1)
39+
for i := range ids {
40+
ids[i] = int64(i + 1)
41+
}
42+
_, err = m.BulkUserAction(context.Background(), ids, "disable", 0)
43+
var ve *ValidationError
44+
if !errors.As(err, &ve) || ve.Code != "err.tooManyUsersSelected" {
45+
t.Errorf("%d ids were accepted (or refused for the wrong reason): %v", len(ids), err)
46+
}
47+
}

internal/i18n/errcatalog_gen.go

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

web/src/i18n/en.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -405,6 +405,7 @@ const en: Dict = {
405405
loginCharset: "username: 3–32 characters, Latin letters, digits, dot, hyphen or underscore",
406406
nameRequired: "enter a name",
407407
noFailedToRetry: "there are no failed deliveries to retry",
408+
tooManyUsersSelected: "at most {{max}} users can be processed in one call",
408409
noUsersSelected: "no users selected",
409410
nothingToSend: "nothing to send — add text or an attachment",
410411
nothingToUpdate: "nothing to update",

web/src/i18n/ru.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,6 +408,7 @@ const ru = {
408408
loginCharset: "логин: 3–32 символа, латиница, цифры, точка, дефис или подчёркивание",
409409
nameRequired: "укажите имя",
410410
noFailedToRetry: "нет неудачных отправок для повтора",
411+
tooManyUsersSelected: "за один раз можно обработать не больше {{max}} пользователей",
411412
noUsersSelected: "не выбрано ни одного пользователя",
412413
nothingToSend: "нечего отправлять — добавьте текст или вложение",
413414
nothingToUpdate: "нечего обновлять",

0 commit comments

Comments
 (0)