mirror of
https://gitee.com/mateos/mateclaw.git
synced 2026-09-13 03:13:41 +08:00
fix(conversation): isolate shared conversations (#616)
This commit is contained in:
matevip
parent
c1ba390f25
commit
bdd51e7b44
@ -120,7 +120,7 @@ public class ConversationService {
|
||||
/**
|
||||
* Workspace-scoped variant of {@link #listConversations(String)}.
|
||||
*
|
||||
* <p>Strict ownership: only the user's own + {@code system} rows. Used by
|
||||
* <p>Strict ownership: only the user's own rows. Used by
|
||||
* callers that must not see other principals' conversations — notably the
|
||||
* webchat visitor self-service path, which scopes to one visitor.
|
||||
*
|
||||
@ -131,32 +131,25 @@ public class ConversationService {
|
||||
}
|
||||
|
||||
/**
|
||||
* Admin-console variant. When {@code includeChannelPrincipals} is true, also
|
||||
* returns conversations owned by external channel principals
|
||||
* ({@code webchat:<visitorId>}) so the console surfaces webchat threads
|
||||
* alongside the user's own + {@code system} rows — the same way IM-channel
|
||||
* ({@code system}-owned) conversations already appear. The visitor-facing
|
||||
* webchat endpoints keep using the strict overload, so this does not widen a
|
||||
* visitor's own access.
|
||||
* Admin-console variant. Ordinary users only see their own rows. When
|
||||
* {@code includeChannelPrincipals} is true, global admins additionally see
|
||||
* shared system/channel-principal conversations for inspection. The
|
||||
* visitor-facing webchat endpoints keep using the strict overload, so this
|
||||
* does not widen a visitor's own access.
|
||||
*
|
||||
* <p>控制台变体：includeChannelPrincipals 为 true 时额外纳入 webchat 访客会话。
|
||||
*/
|
||||
public List<ConversationVO> listConversations(String username, Long workspaceId,
|
||||
boolean includeChannelPrincipals) {
|
||||
// Return both the current user's conversations AND those created by
|
||||
// scheduled jobs (owner=system). Child conversations spawned by
|
||||
// delegation are excluded — they don't belong in the sidebar.
|
||||
// Return the current user's conversations. Shared system/channel
|
||||
// principals are only surfaced to global admins; otherwise members in
|
||||
// the same workspace can see each other's IM/cron conversations (#616).
|
||||
//
|
||||
// 同时返回当前用户的会话和定时任务（system）产生的会话；
|
||||
// 排除子会话（委派产生的子会话不在侧边栏显示）。
|
||||
//
|
||||
// External channel principals (webchat) are only surfaced to global
|
||||
// admins: per isConversationOwner they are the only ones who can open a
|
||||
// webchat-owned conversation, so listing them to anyone else would show
|
||||
// rows the caller would then 403 on (issue #344 alignment).
|
||||
boolean includeWebchat = includeChannelPrincipals && isGlobalAdmin(username);
|
||||
// 返回当前用户自己的会话。system/webchat 等共享主体仅对全局管理员展示，
|
||||
// 避免同工作区成员互相看到 IM/定时任务会话（#616）。
|
||||
boolean includeSharedPrincipals = includeChannelPrincipals && isGlobalAdmin(username);
|
||||
LambdaQueryWrapper<ConversationEntity> wrapper = new LambdaQueryWrapper<ConversationEntity>()
|
||||
.and(w -> applyOwnerScope(w, username, includeWebchat))
|
||||
.and(w -> applyOwnerScope(w, username, includeSharedPrincipals))
|
||||
.and(this::applyMalformedIdGuard)
|
||||
.and(this::applyOrdinaryConversationGuard)
|
||||
.isNull(ConversationEntity::getParentConversationId)
|
||||
@ -200,15 +193,16 @@ public class ConversationService {
|
||||
|
||||
/**
|
||||
* Apply the owner-scope predicate onto a (nested) wrapper: always the user's
|
||||
* own + {@link #SYSTEM_USER} rows; when {@code includeChannelPrincipals} is
|
||||
* true, also external channel-principal rows ({@code webchat:%}). Kept as one
|
||||
* helper so the list and page queries stay in lockstep.
|
||||
* own rows; when {@code includeSharedPrincipals} is true, also shared
|
||||
* {@link #SYSTEM_USER} and external channel-principal rows ({@code webchat:%}).
|
||||
* Kept as one helper so the list and page queries stay in lockstep.
|
||||
*/
|
||||
private void applyOwnerScope(LambdaQueryWrapper<ConversationEntity> w,
|
||||
String username, boolean includeChannelPrincipals) {
|
||||
w.in(ConversationEntity::getUsername, username, SYSTEM_USER);
|
||||
if (includeChannelPrincipals) {
|
||||
w.or().likeRight(ConversationEntity::getUsername, WEBCHAT_OWNER_PREFIX);
|
||||
String username, boolean includeSharedPrincipals) {
|
||||
w.eq(ConversationEntity::getUsername, username);
|
||||
if (includeSharedPrincipals) {
|
||||
w.or().eq(ConversationEntity::getUsername, SYSTEM_USER)
|
||||
.or().likeRight(ConversationEntity::getUsername, WEBCHAT_OWNER_PREFIX);
|
||||
}
|
||||
}
|
||||
|
||||
@ -257,7 +251,7 @@ public class ConversationService {
|
||||
* Paginated variant used by the Sessions admin page.
|
||||
*
|
||||
* <p>Mirrors {@link #listConversations(String, Long)}'s filtering (current
|
||||
* user + system rows, top-level only, optional workspace) and adds a
|
||||
* user rows, top-level only, optional workspace) and adds a
|
||||
* {@code keyword} match against title / conversationId. The keyword is
|
||||
* case-insensitive and treated as a substring.
|
||||
*
|
||||
@ -272,9 +266,9 @@ public class ConversationService {
|
||||
com.baomidou.mybatisplus.extension.plugins.pagination.Page<ConversationEntity> pager =
|
||||
new com.baomidou.mybatisplus.extension.plugins.pagination.Page<>(page, size);
|
||||
|
||||
// Admin Sessions page surfaces channel conversations too, but only to
|
||||
// global admins — they are the only ones who can open a webchat-owned
|
||||
// conversation (issue #344), so non-admins must not see those rows.
|
||||
// Admin Sessions page surfaces shared system/channel conversations too,
|
||||
// but only to global admins. Non-admins are isolated to their own rows
|
||||
// so workspace peers cannot see each other's shared-channel threads (#616).
|
||||
LambdaQueryWrapper<ConversationEntity> wrapper = new LambdaQueryWrapper<ConversationEntity>()
|
||||
.and(w -> applyOwnerScope(w, username, isGlobalAdmin(username)))
|
||||
.and(this::applyMalformedIdGuard)
|
||||
@ -475,17 +469,17 @@ public class ConversationService {
|
||||
/**
|
||||
* Get-or-create a shared channel conversation.
|
||||
*
|
||||
* <p>IM-channel (Feishu / DingTalk / WeCom / …) conversations must be
|
||||
* visible to every logged-in user in the admin console, so the owner is
|
||||
* uniformly set to {@code system}. For legacy rows whose owner was
|
||||
* <p>IM-channel (Feishu / DingTalk / WeCom / …) conversations use the
|
||||
* shared {@code system} owner and are only surfaced to global admins in the
|
||||
* admin console. For legacy rows whose owner was
|
||||
* historically written as a sender nickname / {@code open_id}, this
|
||||
* method silently rewrites it to {@code system} on read — otherwise the
|
||||
* console list and message endpoints would 403 those rows.
|
||||
* admin console list and message endpoints would 403 those rows.
|
||||
*
|
||||
* <p>获取或创建共享渠道会话。IM 渠道（飞书 / 钉钉 / 企微等）的会话需要在控制台中
|
||||
* 对登录用户可见，因此统一使用 {@code system} 作为 owner。对于历史上已写成发送者
|
||||
* 昵称 / open_id 的会话，这里会自动修正为 {@code system}，避免控制台列表和
|
||||
* 消息接口因权限校验而不可见。
|
||||
* <p>获取或创建共享渠道会话。IM 渠道（飞书 / 钉钉 / 企微等）的会话统一使用
|
||||
* {@code system} 作为 owner，并仅在全局管理员控制台中展示。对于历史上已写成
|
||||
* 发送者昵称 / open_id 的会话，这里会自动修正为 {@code system}，避免管理员
|
||||
* 控制台列表和消息接口因权限校验而不可见。
|
||||
*/
|
||||
@Transactional
|
||||
public ConversationEntity getOrCreateSharedConversation(String conversationId, Long agentId) {
|
||||
@ -1740,10 +1734,10 @@ public class ConversationService {
|
||||
}
|
||||
|
||||
/**
|
||||
* Check whether a user owns the conversation. Direct owners always pass;
|
||||
* shared rows (system / IM / {@code webchat:<visitorId>} principals) are
|
||||
* additionally gated by the requester's membership in the conversation's
|
||||
* workspace, so they are not reachable cross-workspace by id.
|
||||
* Check whether a user owns the conversation. Direct owners always pass.
|
||||
* Shared rows (system / IM / {@code webchat:<visitorId>} principals) are
|
||||
* restricted to global admins, with legacy system-owner fallbacks preserved
|
||||
* for rows/endpoints that cannot resolve an authenticated user.
|
||||
*
|
||||
* <p><b>Cross-workspace guard (issue #344).</b> The legacy contract let any
|
||||
* logged-in user reach a system / IM / webchat-owned conversation by id —
|
||||
@ -1752,20 +1746,22 @@ public class ConversationService {
|
||||
* untrusted isolation boundaries, that asymmetry is a cross-workspace
|
||||
* authorization gap. This method now also requires, for shared (non-direct)
|
||||
* conversations, that the requester actually be a member of the
|
||||
* conversation's workspace.
|
||||
* conversation's workspace. Issue #616 tightened this further: workspace
|
||||
* membership alone is not enough to read a shared system conversation,
|
||||
* because that lets peers in the same workspace see each other's channel
|
||||
* or scheduled-job conversations.
|
||||
*
|
||||
* <p>校验用户是否拥有该会话。直属会话直接放行;共享会话(system / IM / webchat)
|
||||
* 额外要求请求者是该会话所属 workspace 的成员。
|
||||
* 仅允许全局管理员查看，避免同 workspace 成员互相看到对话。
|
||||
*
|
||||
* <p>分支:
|
||||
* <ul>
|
||||
* <li>会话不存在 → false</li>
|
||||
* <li>请求者是该会话的直属 owner → true(自己的会话,workspace 隐式一致)</li>
|
||||
* <li>请求者是全局 admin(user.role=admin)→ true(横切覆盖,与具体 workspace 无关)</li>
|
||||
* <li>会话无 workspace_id(老数据)→ 仅看是否 system owner(维持旧行为,避免回归)</li>
|
||||
* <li>请求者用户记录不存在(permitAll 端点的匿名重连)→ 仅看是否 system owner(维持旧行为)</li>
|
||||
* <li>请求者是全局 admin(user.role=admin)→ true(横切覆盖,与具体 workspace 无关)</li>
|
||||
* <li>请求者非该会话 workspace 的成员 → false</li>
|
||||
* <li>否则 → system owner 检查(共享会话对本 workspace 成员可见)</li>
|
||||
* <li>否则 → false</li>
|
||||
* </ul>
|
||||
*
|
||||
* <p>调用方签名不变;调用方若需在不查 DB 的情况下做 admin 例外,可在外层先短路,
|
||||
@ -1786,21 +1782,17 @@ public class ConversationService {
|
||||
// 共享会话(system / IM / webchat owner)以下收紧。
|
||||
Long convWorkspaceId = conv.getWorkspaceId();
|
||||
UserEntity requester = authService.findByUsername(username);
|
||||
// 全局 admin 横切放行,覆盖所有 workspace。
|
||||
if (requester != null && "admin".equalsIgnoreCase(requester.getRole())) {
|
||||
return true;
|
||||
}
|
||||
// 老数据无 workspace_id,或请求者为匿名(permitAll 端点重连场景):维持旧行为,
|
||||
// 仅 system owner 可见。避免数据迁移未完成或匿名流式场景下回归。
|
||||
if (convWorkspaceId == null || requester == null) {
|
||||
return SYSTEM_USER.equals(conv.getUsername());
|
||||
}
|
||||
// 全局 admin 横切放行,覆盖所有 workspace。
|
||||
if ("admin".equalsIgnoreCase(requester.getRole())) {
|
||||
return true;
|
||||
}
|
||||
// #344 的核心守卫:必须是该会话所属 workspace 的成员(viewer 或更高)。
|
||||
// 不读 X-Workspace-Id header —— 客户端可伪造;以 DB 成员关系为准。
|
||||
if (!workspaceService.hasPermissionCached(convWorkspaceId, requester.getId(), "viewer")) {
|
||||
return false;
|
||||
}
|
||||
return SYSTEM_USER.equals(conv.getUsername());
|
||||
// #616: workspace membership alone is not ownership.
|
||||
return false;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@ -26,7 +26,6 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy;
|
||||
import static org.mockito.ArgumentMatchers.any;
|
||||
import static org.mockito.ArgumentMatchers.anyLong;
|
||||
import static org.mockito.ArgumentMatchers.anyString;
|
||||
import static org.mockito.ArgumentMatchers.eq;
|
||||
import static org.mockito.Mockito.never;
|
||||
import static org.mockito.Mockito.verify;
|
||||
import static org.mockito.Mockito.when;
|
||||
@ -42,9 +41,11 @@ import static org.mockito.Mockito.when;
|
||||
* endpoints did not — an asymmetry that becomes a cross-workspace breach once
|
||||
* workspaces are untrusted isolation boundaries.
|
||||
*
|
||||
* <p>Post-fix behavior: shared conversations are visible only to members of
|
||||
* their own workspace (plus global admins, plus the legacy escape hatches for
|
||||
* pre-workspace rows and anonymous permitAll reconnects).
|
||||
* <p>Post-fix behavior: shared conversations are visible only to global admins
|
||||
* (plus the legacy escape hatches for pre-workspace rows and anonymous
|
||||
* permitAll reconnects). Issue #616 intentionally rejects same-workspace
|
||||
* ordinary members too, because workspace membership is not conversation
|
||||
* ownership.
|
||||
*
|
||||
* <p>Pure-Mockito (no Spring context) so the test stays fast and isolated.
|
||||
*
|
||||
@ -101,17 +102,16 @@ class ConversationServiceOwnershipWorkspaceTest {
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
// 2. System conv, same-workspace member → allowed
|
||||
// 2. System conv, same-workspace member → rejected (#616)
|
||||
// ------------------------------------------------------------------
|
||||
|
||||
@Test
|
||||
@DisplayName("system conv in requester's workspace: member passes")
|
||||
void systemConvSameWorkspaceMember() {
|
||||
@DisplayName("system conv in requester's workspace: member rejected (#616)")
|
||||
void systemConvSameWorkspaceMemberRejected() {
|
||||
when(conversationMapper.selectOne(any())).thenReturn(conv(SYSTEM_CONV, "system", WS_TENANT_A));
|
||||
when(workspaceService.hasPermissionCached(WS_TENANT_A, ALICE_USER_ID, "viewer"))
|
||||
.thenReturn(true);
|
||||
|
||||
assertThat(service.isConversationOwner(SYSTEM_CONV, "alice")).isTrue();
|
||||
assertThat(service.isConversationOwner(SYSTEM_CONV, "alice")).isFalse();
|
||||
verify(workspaceService, never()).hasPermissionCached(anyLong(), anyLong(), anyString());
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
@ -122,11 +122,9 @@ class ConversationServiceOwnershipWorkspaceTest {
|
||||
@DisplayName("system conv in another workspace: non-member rejected (#344)")
|
||||
void systemConvCrossWorkspaceRejected() {
|
||||
when(conversationMapper.selectOne(any())).thenReturn(conv(SYSTEM_CONV, "system", WS_TENANT_A));
|
||||
// Bob is not a member of tenant A.
|
||||
when(workspaceService.hasPermissionCached(WS_TENANT_A, BOB_USER_ID, "viewer"))
|
||||
.thenReturn(false);
|
||||
|
||||
assertThat(service.isConversationOwner(SYSTEM_CONV, "bob")).isFalse();
|
||||
verify(workspaceService, never()).hasPermissionCached(anyLong(), anyLong(), anyString());
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
@ -189,12 +187,11 @@ class ConversationServiceOwnershipWorkspaceTest {
|
||||
@DisplayName("webchat conv: invisible to a JWT user even when they share the workspace")
|
||||
void webchatConvInvisibleToJwtUser() {
|
||||
when(conversationMapper.selectOne(any())).thenReturn(conv(WEBCHAT_CONV, "webchat:vA", WS_TENANT_A));
|
||||
when(workspaceService.hasPermissionCached(WS_TENANT_A, ALICE_USER_ID, "viewer"))
|
||||
.thenReturn(true);
|
||||
|
||||
// Alice is a member of the conv's workspace, but the conv is owned by
|
||||
// "webchat:vA" — not "system" — so the final OR-clause returns false.
|
||||
// Alice may be a member of the conv's workspace, but the conv is owned
|
||||
// by "webchat:vA" and workspace membership is not ownership.
|
||||
assertThat(service.isConversationOwner(WEBCHAT_CONV, "alice")).isFalse();
|
||||
verify(workspaceService, never()).hasPermissionCached(anyLong(), anyLong(), anyString());
|
||||
}
|
||||
|
||||
@Test
|
||||
@ -219,15 +216,12 @@ class ConversationServiceOwnershipWorkspaceTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("system conv + same-workspace member that the workspace service lost track of: rejected")
|
||||
@DisplayName("system conv + ordinary member: rejected without membership lookup (#616)")
|
||||
void systemConvMemberCacheMiss() {
|
||||
when(conversationMapper.selectOne(any())).thenReturn(conv(SYSTEM_CONV, "system", WS_TENANT_A));
|
||||
// Membership cache returns false even though we'd expect this user to
|
||||
// be a member — defense in depth: when in doubt, deny.
|
||||
when(workspaceService.hasPermissionCached(eq(WS_TENANT_A), eq(ALICE_USER_ID), eq("viewer")))
|
||||
.thenReturn(false);
|
||||
|
||||
assertThat(service.isConversationOwner(SYSTEM_CONV, "alice")).isFalse();
|
||||
verify(workspaceService, never()).hasPermissionCached(anyLong(), anyLong(), anyString());
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
|
||||
@ -79,8 +79,8 @@ class ConversationServiceWebchatVisibilityTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("lenient list, non-admin: excludes webchat principals (no 'webchat:%' param)")
|
||||
void lenientListNonAdminExcludesWebchat() {
|
||||
@DisplayName("lenient list, non-admin: excludes shared principals (#616)")
|
||||
void lenientListNonAdminExcludesSharedPrincipals() {
|
||||
when(authService.findByUsername("alice")).thenReturn(user("member"));
|
||||
ArgumentCaptor<LambdaQueryWrapper<ConversationEntity>> captor =
|
||||
ArgumentCaptor.forClass(LambdaQueryWrapper.class);
|
||||
@ -88,10 +88,12 @@ class ConversationServiceWebchatVisibilityTest {
|
||||
|
||||
service.listConversations("alice", 1L, true);
|
||||
|
||||
// The malformed-id guard still emits a NOT LIKE, so we assert on the
|
||||
// param value instead of the LIKE keyword.
|
||||
// Members should only see their own rows: not webchat, and not
|
||||
// workspace-wide system rows such as IM/cron conversations.
|
||||
assertThat(captor.getValue().getTargetSql()).containsIgnoringCase("username");
|
||||
assertThat(captor.getValue().getParamNameValuePairs().values())
|
||||
.doesNotContain("webchat:%");
|
||||
.contains("alice")
|
||||
.doesNotContain("system", "webchat:%");
|
||||
}
|
||||
|
||||
@Test
|
||||
@ -126,7 +128,7 @@ class ConversationServiceWebchatVisibilityTest {
|
||||
|
||||
@Test
|
||||
@DisplayName("page query, non-admin: excludes webchat principals")
|
||||
void pageNonAdminExcludesWebchat() {
|
||||
void pageNonAdminExcludesSharedPrincipals() {
|
||||
when(authService.findByUsername("alice")).thenReturn(user("member"));
|
||||
ArgumentCaptor<LambdaQueryWrapper<ConversationEntity>> captor =
|
||||
ArgumentCaptor.forClass(LambdaQueryWrapper.class);
|
||||
@ -135,8 +137,10 @@ class ConversationServiceWebchatVisibilityTest {
|
||||
|
||||
service.pageConversations("alice", 1L, 1, 20, null);
|
||||
|
||||
assertThat(captor.getValue().getTargetSql()).containsIgnoringCase("username");
|
||||
assertThat(captor.getValue().getParamNameValuePairs().values())
|
||||
.doesNotContain("webchat:%");
|
||||
.contains("alice")
|
||||
.doesNotContain("system", "webchat:%");
|
||||
}
|
||||
|
||||
@Test
|
||||
|
||||
Loading…
Reference in New Issue
Block a user