Skip to content

Commit fde79cf

Browse files
authored
fix(group): bots always follow their owner — no exit/kick role exceptions, blacklist cascades to owner's bots (#355)
Fixes #354 Product decision: **a user's bots always follow the user — no role exceptions.** When a user leaves, is kicked, or is blacklisted, every bot they created and invited into the group must follow them. This closes the two gaps left after the D-2 cascade (#1186 / #1189). ## Gap 1 — creator/manager exceptions on the exit/kick cascade - **`groupExit`**: removed the `loginMember.Role != MemberRoleCreator` guard. A group owner who exits (after ownership transfer to the second-oldest member) now cascades exactly like a normal member: bot members removed in the same transaction, plus the existing thread_member / IM subscription / pinned / conv_ext cleanup and the system tip. - New-owner selection now uses `QuerySecondOldestMemberExcludingBotsOf`, which excludes the leaving owner's own bots — otherwise a bot could be promoted to creator and cascade-deleted in the same transaction, leaving the group ownerless. Other users' bots keep the old selection semantics. - **`RemoveGroupMembers`**: dropped the manager exception from the removable filter and from the per-member cascade guard. A kicked manager leaves together with their bots. Creator keeps the belt-and-suspenders exemption (cannot be kicked; API-level `memberRemove` already restricts manager removal to the owner). ## Gap 2 — blacklist had no cascade at all Previously `blacklist add` only flipped the target user's member status + IM blacklist. Their bots stayed `status=Normal`, so a blacklisted user could keep reading group/subchannel content **through their own bot**, bypassing the entire `ExistMemberActive` hardening line (#343 / #345). - On blacklist add/remove, the target list is expanded with the users' in-group bots (`robot.creator_uid` match, same fail-closed ownership semantics as the D-2 cascade: orphan/disabled bots are not anyone's bot). - The bots then go through the same pipeline as the user: member status flip, IM blacklist add/remove, parent-group + subchannel subscription teardown (YUJ-4185 line), and symmetric restore on un-blacklist. - The ownership query intentionally does not filter `group_member.status`, so un-blacklist finds the bots that are currently in `Blacklist` state and restores them. ## Tests - `TestQuitGroup_CreatorCascadesBots` — owner exits → ownership transferred to a human (bot excluded from selection), owner's bot cascade-removed. - `TestQuerySecondOldestMemberExcludingBotsOf_OnlyBotsLeft` — no new owner when only the leaver's bots remain; other users' bots unaffected. - `TestRemoveGroupMembers_ManagerKickCascadesBots` — kicked manager + their bot both removed; creator still unkickable. - `TestQueryBotUIDsOwnedByUIDs_BasicMatch` / `TestExpandBlacklistTargetsWithOwnedBots` — cascade target expansion semantics (ownership scoping, orphan/disabled exclusion, dedupe). - `TestBlacklistCascade_BotBlockedByExistMemberActive` — blacklisted user's bot fails `ExistMemberActive` (subchannel read/write gate denies); un-blacklist symmetrically restores; bystanders untouched. - Existing D-2 cascade suites (`bot_cascade_test.go`, `api_groupexit_cascade_thread_test.go`) pass unchanged except the obsolete creator-skip case, which now asserts the new product decision. Full `modules/group`, `modules/bot_api`, `modules/thread`, `modules/message`, `modules/user`, `modules/robot`, `modules/webhook` suites pass locally. <!-- sprint-set-retrigger -->
1 parent 7ade30d commit fde79cf

13 files changed

Lines changed: 596 additions & 52 deletions

File tree

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
package bot_api
2+
3+
import (
4+
"encoding/json"
5+
"net/http"
6+
"strings"
7+
"testing"
8+
9+
"github.com/Mininglamp-OSS/octo-lib/pkg/util"
10+
"github.com/Mininglamp-OSS/octo-lib/testutil"
11+
"github.com/stretchr/testify/assert"
12+
"github.com/stretchr/testify/require"
13+
)
14+
15+
// PR#355 review 守卫:bot_admin 与人类管理员同权,POST
16+
// /v1/bot/groups/:group_no/members/remove 不得移除群主/管理员。#354 把
17+
// service 层 RemoveGroupMembers 的 manager 豁免下沉移除后,目标角色校验由
18+
// 调用方负责——Web API memberRemove 有 creator/manager 守卫,这里验证 Bot
19+
// API 路径的对等守卫:
20+
// - 目标含 manager → 403 cannot_remove_privileged,且整个请求拒绝(混合
21+
// 列表里的普通成员也不能被"顺带"移除);
22+
// - 目标含 creator → 403 cannot_remove_privileged;
23+
// - 目标全为普通成员 → 正常移除(守卫不误伤)。
24+
25+
const (
26+
rmGuardGroupNo = "g_rm_guard_1"
27+
rmGuardBotID = "bot_rm_guard"
28+
rmGuardBotToken = "bf_rm_guard_token"
29+
rmGuardCreator = "u_rm_creator"
30+
rmGuardManager = "u_rm_manager"
31+
rmGuardCommon = "u_rm_common"
32+
)
33+
34+
func setupRemoveGuardEnv(t *testing.T) http.Handler {
35+
t.Helper()
36+
s, ctx := testutil.NewTestServer()
37+
require.NoError(t, testutil.CleanAllTables(ctx))
38+
39+
_, err := ctx.DB().InsertBySql(
40+
"INSERT INTO robot (robot_id, status, creator_uid, bot_token) VALUES (?, 1, ?, ?)",
41+
rmGuardBotID, rmGuardCreator, rmGuardBotToken).Exec()
42+
require.NoError(t, err)
43+
44+
_, err = ctx.DB().InsertBySql(
45+
"INSERT INTO `group` (group_no, name, status, version) VALUES (?, ?, 1, 1)",
46+
rmGuardGroupNo, "remove guard group").Exec()
47+
require.NoError(t, err)
48+
49+
// creator(role=1) / manager(role=2) / 普通成员(role=0) / bot 管理员(bot_admin=1)
50+
for _, m := range []struct {
51+
uid string
52+
role, robot, adm int
53+
}{
54+
{rmGuardCreator, 1, 0, 0},
55+
{rmGuardManager, 2, 0, 0},
56+
{rmGuardCommon, 0, 0, 0},
57+
{rmGuardBotID, 0, 1, 1},
58+
} {
59+
_, err = ctx.DB().InsertBySql(
60+
"INSERT INTO group_member (group_no, uid, role, robot, bot_admin, vercode, is_deleted, status, version) VALUES (?, ?, ?, ?, ?, ?, 0, 1, 1)",
61+
rmGuardGroupNo, m.uid, m.role, m.robot, m.adm, util.GenerUUID()).Exec()
62+
require.NoError(t, err)
63+
}
64+
65+
return s.GetRoute()
66+
}
67+
68+
func rmGuardIsActiveMember(t *testing.T, handler http.Handler, uid string) bool {
69+
t.Helper()
70+
w := doBot(handler, botReq(t, "GET", "/v1/bot/groups/"+rmGuardGroupNo+"/members", rmGuardBotToken, nil))
71+
require.Equalf(t, http.StatusOK, w.Code, "list members body: %s", w.Body.String())
72+
// GET /members 返回成员数组;uid 可能出现在响应的其他字段里,所以逐项
73+
// 比对而不是对响应体做字符串包含判断。
74+
var members []struct {
75+
UID string `json:"uid"`
76+
}
77+
require.NoErrorf(t, json.Unmarshal(w.Body.Bytes(), &members), "list members body: %s", w.Body.String())
78+
for _, m := range members {
79+
if m.UID == uid {
80+
return true
81+
}
82+
}
83+
return false
84+
}
85+
86+
func TestBotGroupMemberRemove_ManagerTargetForbidden(t *testing.T) {
87+
handler := setupRemoveGuardEnv(t)
88+
89+
w := doBot(handler, botReq(t, "POST", "/v1/bot/groups/"+rmGuardGroupNo+"/members/remove", rmGuardBotToken,
90+
map[string]interface{}{"members": []string{rmGuardManager}}))
91+
assert.Equalf(t, http.StatusForbidden, w.Code, "body: %s", w.Body.String())
92+
assert.Contains(t, w.Body.String(), "cannot be removed through the bot API")
93+
assert.True(t, rmGuardIsActiveMember(t, handler, rmGuardManager), "manager must remain a member")
94+
}
95+
96+
func TestBotGroupMemberRemove_CreatorTargetForbidden(t *testing.T) {
97+
handler := setupRemoveGuardEnv(t)
98+
99+
w := doBot(handler, botReq(t, "POST", "/v1/bot/groups/"+rmGuardGroupNo+"/members/remove", rmGuardBotToken,
100+
map[string]interface{}{"members": []string{rmGuardCreator}}))
101+
assert.Equalf(t, http.StatusForbidden, w.Code, "body: %s", w.Body.String())
102+
assert.Contains(t, w.Body.String(), "cannot be removed through the bot API")
103+
assert.True(t, rmGuardIsActiveMember(t, handler, rmGuardCreator), "creator must remain a member")
104+
}
105+
106+
func TestBotGroupMemberRemove_MixedListRejectedAtomically(t *testing.T) {
107+
handler := setupRemoveGuardEnv(t)
108+
109+
// 混合列表(普通成员 + 管理员)→ 整个请求 403,普通成员也不能被顺带移除。
110+
w := doBot(handler, botReq(t, "POST", "/v1/bot/groups/"+rmGuardGroupNo+"/members/remove", rmGuardBotToken,
111+
map[string]interface{}{"members": []string{rmGuardCommon, rmGuardManager}}))
112+
assert.Equalf(t, http.StatusForbidden, w.Code, "body: %s", w.Body.String())
113+
assert.Contains(t, w.Body.String(), "cannot be removed through the bot API")
114+
assert.True(t, rmGuardIsActiveMember(t, handler, rmGuardManager), "manager must remain a member")
115+
assert.True(t, rmGuardIsActiveMember(t, handler, rmGuardCommon), "common member must not be removed when the request is rejected")
116+
}
117+
118+
func TestBotGroupMemberRemove_ManagerTargetCaseVariantForbidden(t *testing.T) {
119+
handler := setupRemoveGuardEnv(t)
120+
121+
// MySQL utf8mb4_*_ci collation 下 uid 匹配大小写不敏感:大小写变体在
122+
// service 层仍会命中真实 manager 行,所以守卫必须按 DB 解析行的角色
123+
// 拦截,而不是在 Go 里做大小写敏感的字符串比对(codex review P1 回归)。
124+
w := doBot(handler, botReq(t, "POST", "/v1/bot/groups/"+rmGuardGroupNo+"/members/remove", rmGuardBotToken,
125+
map[string]interface{}{"members": []string{strings.ToUpper(rmGuardManager)}}))
126+
assert.Equalf(t, http.StatusForbidden, w.Code, "body: %s", w.Body.String())
127+
assert.Contains(t, w.Body.String(), "cannot be removed through the bot API")
128+
assert.True(t, rmGuardIsActiveMember(t, handler, rmGuardManager), "manager must remain a member")
129+
}
130+
131+
func TestBotGroupMemberRemove_CommonTargetStillWorks(t *testing.T) {
132+
handler := setupRemoveGuardEnv(t)
133+
134+
w := doBot(handler, botReq(t, "POST", "/v1/bot/groups/"+rmGuardGroupNo+"/members/remove", rmGuardBotToken,
135+
map[string]interface{}{"members": []string{rmGuardCommon}}))
136+
require.Equalf(t, http.StatusOK, w.Code, "body: %s", w.Body.String())
137+
resp := decodeBody(t, w)
138+
assert.Equal(t, true, resp["ok"])
139+
assert.Equal(t, float64(1), resp["removed"])
140+
assert.False(t, rmGuardIsActiveMember(t, handler, rmGuardCommon), "common member should be removed")
141+
}

modules/bot_api/groups.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -656,6 +656,35 @@ func (ba *BotAPI) botGroupMemberRemove(c *wkhttp.Context) {
656656
return
657657
}
658658

659+
// PR#355 review: bot_admin 与人类管理员同权,不能踢群主/管理员——对齐
660+
// Web API memberRemove 的角色守卫(manager 不可移除 manager/creator)。
661+
// #354 把 service 层 RemoveGroupMembers 的 manager 豁免去掉后,目标角色
662+
// 校验完全由调用方负责;不在这里拦截的话,bot_admin=1 的 bot 可以越权
663+
// 踢任意管理员并级联带走其 bot。
664+
//
665+
// 角色校验必须基于 DB 解析后的目标行:WHERE 条件与 service 层
666+
// QueryMembersWithUids 完全一致,由 MySQL collation 决定 uid 匹配
667+
// (utf8mb4_*_ci 大小写不敏感)。如果改在 Go 里做大小写敏感的字符串
668+
// 比对,请求方用 uid 的大小写变体即可绕过守卫、却仍命中 service 层的
669+
// 真实 manager 行。
670+
var targetRows []struct {
671+
UID string `db:"uid"`
672+
Role int `db:"role"`
673+
}
674+
_, err = ba.db.session.Select("uid", "role").From("group_member").
675+
Where("uid in ? and group_no=? and is_deleted=0", filteredMembers, groupNo).Load(&targetRows)
676+
if err != nil {
677+
ba.Error("query target members failed", zap.Error(err), zap.String("groupNo", groupNo))
678+
httperr.ResponseErrorL(c, errcode.ErrBotAPIQueryFailed, nil, nil)
679+
return
680+
}
681+
for _, row := range targetRows {
682+
if row.Role != group.MemberRoleCommon {
683+
httperr.ResponseErrorLWithStatus(c, errcode.ErrBotAPICannotRemovePrivileged, nil, i18n.Details{"uid": row.UID})
684+
return
685+
}
686+
}
687+
659688
removeResp, err := ba.groupService.RemoveGroupMembers(&group.RemoveGroupMembersServiceReq{
660689
GroupNo: groupNo,
661690
Members: filteredMembers,

modules/botfather/api_bot_group_test.go

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -351,9 +351,14 @@ func TestBotGroupMemberRemove_CannotRemoveCreator(t *testing.T) {
351351
"members": []string{testutil.UID},
352352
}))
353353

354-
assert.Equal(t, http.StatusOK, w.Code)
355-
result := jsonResult(t, w)
356-
assert.Equal(t, float64(0), result["removed"])
354+
// PR#355 review (Jerry-Xin): the bot member-remove role guard now rejects
355+
// the request outright instead of silently skipping the creator. This
356+
// mirrors the Web API memberRemove rule (a manager cannot kick the
357+
// creator/managers) — see modules/bot_api/groups.go and the matching
358+
// err.server.bot_api.cannot_remove_privileged 403 in
359+
// modules/bot_api/group_member_remove_guard_test.go.
360+
assert.Equalf(t, http.StatusForbidden, w.Code, "body: %s", w.Body.String())
361+
assert.Contains(t, w.Body.String(), "cannot be removed through the bot API")
357362
}
358363

359364
func TestBotGroupMemberRemove_NotBotAdmin(t *testing.T) {

modules/group/api.go

Lines changed: 32 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2833,8 +2833,9 @@ func (g *Group) groupExit(c *wkhttp.Context) {
28332833
**/
28342834
var newGrouper *MemberModel // 新群主
28352835
if loginMember.Role == MemberRoleCreator {
2836-
// 查询第二老成员
2837-
newGrouper, err = g.db.QuerySecondOldestMember(groupNo)
2836+
// 查询第二老成员。#354:排除退群群主名下的 bot——它们会在下方被级联带走,
2837+
// 不能被选为新群主(否则新群主在同一事务内即被删除)。
2838+
newGrouper, err = g.db.QuerySecondOldestMemberExcludingBotsOf(groupNo, loginUID)
28382839
if err != nil {
28392840
g.Error("查询第二元老成员失败!", zap.Error(err))
28402841
httperr.ResponseErrorL(c, errcode.ErrGroupQueryFailed, nil, nil)
@@ -2896,21 +2897,19 @@ func (g *Group) groupExit(c *wkhttp.Context) {
28962897
}
28972898

28982899
// D-2 · 级联带走 inviter 拉入的 bot(YUJ-49 / Mininglamp-OSS/octo-server#1186)。
2899-
// Edge case: 群主退群时此逻辑不触发(角色转让由上方 newGrouper 兜底;
2900-
// 若确要销群应走 DismissGroup)——保持 bot 跟随新群主留在群中
2900+
// #354 产品决策:bot 永远跟随其主人,无角色例外——群主退群(角色已由上方
2901+
// newGrouper 完成转让)同样级联带走自己名下的 bot,和普通成员一致
29012902
var cascadedBotUsers []*user.Model
2902-
if loginMember.Role != MemberRoleCreator {
2903-
cascadedUIDs, cerr := cascadeRemoveBotsInvitedByUIDTx(g.db, g.ctx, groupNo, loginUID, tx)
2904-
if cerr != nil {
2905-
tx.Rollback()
2906-
g.Error("级联移除 bot 成员失败", zap.Error(cerr))
2907-
httperr.ResponseErrorL(c, errcode.ErrGroupStoreFailed, nil, nil)
2908-
return
2909-
}
2910-
for _, botUID := range cascadedUIDs {
2911-
botUser, _ := g.userDB.QueryByUID(botUID)
2912-
cascadedBotUsers = append(cascadedBotUsers, botUser)
2913-
}
2903+
cascadedUIDs, cerr := cascadeRemoveBotsInvitedByUIDTx(g.db, g.ctx, groupNo, loginUID, tx)
2904+
if cerr != nil {
2905+
tx.Rollback()
2906+
g.Error("级联移除 bot 成员失败", zap.Error(cerr))
2907+
httperr.ResponseErrorL(c, errcode.ErrGroupStoreFailed, nil, nil)
2908+
return
2909+
}
2910+
for _, botUID := range cascadedUIDs {
2911+
botUser, _ := g.userDB.QueryByUID(botUID)
2912+
cascadedBotUsers = append(cascadedBotUsers, botUser)
29142913
}
29152914

29162915
// 若退群者是外部成员且当前群是外部群,检查是否需要恢复普通群
@@ -3128,6 +3127,16 @@ func (g *Group) blacklist(c *wkhttp.Context) {
31283127
httperr.ResponseErrorL(c, errcode.ErrGroupManagerOnly, nil, nil)
31293128
return
31303129
}
3130+
// #354 · Bot 跟人走:拉黑/解除拉黑级联到目标用户名下在群的 bot
3131+
// (robot.creator_uid 命中)。旧行为只动用户本人,其 bot 仍 status=Normal,
3132+
// 被拉黑用户可经自己的 bot 旁路读群/子区内容,绕过 ExistMemberActive
3133+
// 加固线(#343/#345)。解除拉黑走同一扩展,对称恢复。
3134+
targetUIDs, err := expandBlacklistTargetsWithOwnedBots(g.db, groupNo, req.Uids)
3135+
if err != nil {
3136+
g.Error("查询拉黑级联 bot 失败", zap.Error(err))
3137+
httperr.ResponseErrorL(c, errcode.ErrGroupQueryFailed, nil, nil)
3138+
return
3139+
}
31313140
status := 0
31323141
if action == "add" {
31333142
status = int(common.GroupMemberStatusBlacklist)
@@ -3141,14 +3150,14 @@ func (g *Group) blacklist(c *wkhttp.Context) {
31413150
httperr.ResponseErrorL(c, errcode.ErrGroupStoreFailed, nil, nil)
31423151
return
31433152
}
3144-
err = g.db.updateMembersStatus(version, groupNo, status, req.Uids)
3153+
err = g.db.updateMembersStatus(version, groupNo, status, targetUIDs)
31453154
if err != nil {
31463155
g.Error("添加或移除群成员黑名单错误", zap.Error(err))
31473156
httperr.ResponseErrorL(c, errcode.ErrGroupStoreFailed, nil, nil)
31483157
return
31493158
}
31503159
if status == int(common.GroupMemberStatusBlacklist) {
3151-
err = g.setGroupBlacklist(groupNo, req.Uids, status == int(common.GroupMemberStatusBlacklist))
3160+
err = g.setGroupBlacklist(groupNo, targetUIDs, status == int(common.GroupMemberStatusBlacklist))
31523161
if err != nil {
31533162
g.Error("添加IM黑名单错误", zap.Error(err))
31543163
httperr.ResponseErrorL(c, errcode.ErrGroupStoreFailed, nil, nil)
@@ -3158,18 +3167,19 @@ func (g *Group) blacklist(c *wkhttp.Context) {
31583167
// 仅靠 setGroupBlacklist(IMBlacklistAdd) 只挡“发送”,不断“接收”——被拉黑者仍
31593168
// 通过 WuKongIM WS 收子区/父群实时消息(越权读 P0)。子区订阅复用 #27/#332 统一
31603169
// helper;父群订阅显式 IMRemoveSubscriber。best-effort:失败只记日志、不回滚拉黑。
3161-
for _, uid := range req.Uids {
3170+
// #354:级联拉黑的 bot 同样摘除订阅(targetUIDs 已含 bot)。
3171+
for _, uid := range targetUIDs {
31623172
g.removeUserFromGroupThreads(groupNo, uid, group.SpaceID)
31633173
}
31643174
if rmErr := g.ctx.IMRemoveSubscriber(&config.SubscriberRemoveReq{
31653175
ChannelID: groupNo,
31663176
ChannelType: common.ChannelTypeGroup.Uint8(),
3167-
Subscribers: req.Uids,
3177+
Subscribers: targetUIDs,
31683178
}); rmErr != nil {
31693179
g.Error("拉黑摘除父群IM订阅失败", zap.Error(rmErr), zap.String("groupNo", groupNo))
31703180
}
31713181
} else {
3172-
members, err := g.db.QueryMembersWithUids(req.Uids, groupNo)
3182+
members, err := g.db.QueryMembersWithUids(targetUIDs, groupNo)
31733183
if err != nil {
31743184
g.Error("查询移除黑名单成员错误", zap.Error(err))
31753185
httperr.ResponseErrorL(c, errcode.ErrGroupQueryFailed, nil, nil)
@@ -3222,7 +3232,7 @@ func (g *Group) blacklist(c *wkhttp.Context) {
32223232
return
32233233
}
32243234
} else {
3225-
for _, uid := range req.Uids {
3235+
for _, uid := range targetUIDs {
32263236
// 发送群成员更新命令
32273237
err = g.ctx.SendCMD(config.MsgCMDReq{
32283238
ChannelID: groupNo,

modules/group/api_groupexit_cascade_thread_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ func TestGroupExit_CascadeBot_AlsoRemovesFromThread(t *testing.T) {
7777
f := New(s.ctx)
7878
ensureThreadTables(t, f)
7979

80-
// --- 1. 用户:群主 owner、退群者 leaver(非群主,cascade 前置条件)、bot 本体 ---
80+
// --- 1. 用户:群主 owner、退群者 leaver(普通成员场景;#354 起群主退群同样 cascade)、bot 本体 ---
8181
insertTestUsers(t, userDB, "owner", "leaver")
8282
botUID := "yuj52_bot_by_leaver"
8383
require.NoError(t, userDB.Insert(&user.Model{UID: botUID, Name: "bot-by-leaver", ShortNo: "sn_" + botUID, Robot: 1}))

0 commit comments

Comments
 (0)