diff --git a/mateclaw-server/src/main/java/vip/mate/workspace/conversation/ConversationService.java b/mateclaw-server/src/main/java/vip/mate/workspace/conversation/ConversationService.java index 0961110f..5e7171e2 100644 --- a/mateclaw-server/src/main/java/vip/mate/workspace/conversation/ConversationService.java +++ b/mateclaw-server/src/main/java/vip/mate/workspace/conversation/ConversationService.java @@ -120,7 +120,7 @@ public class ConversationService { /** * Workspace-scoped variant of {@link #listConversations(String)}. * - *

Strict ownership: only the user's own + {@code system} rows. Used by + *

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:}) 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. * *

控制台变体：includeChannelPrincipals 为 true 时额外纳入 webchat 访客会话。 */ public List 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 wrapper = new LambdaQueryWrapper() - .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 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. * *

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 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 wrapper = new LambdaQueryWrapper() .and(w -> applyOwnerScope(w, username, isGlobalAdmin(username))) .and(this::applyMalformedIdGuard) @@ -475,17 +469,17 @@ public class ConversationService { /** * Get-or-create a shared channel conversation. * - *

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 + *

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. * - *

获取或创建共享渠道会话。IM 渠道（飞书 / 钉钉 / 企微等）的会话需要在控制台中 - * 对登录用户可见，因此统一使用 {@code system} 作为 owner。对于历史上已写成发送者 - * 昵称 / open_id 的会话，这里会自动修正为 {@code system}，避免控制台列表和 - * 消息接口因权限校验而不可见。 + *

获取或创建共享渠道会话。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:} 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:} principals) are + * restricted to global admins, with legacy system-owner fallbacks preserved + * for rows/endpoints that cannot resolve an authenticated user. * *

Cross-workspace guard (issue #344). 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. * *

校验用户是否拥有该会话。直属会话直接放行;共享会话(system / IM / webchat) - * 额外要求请求者是该会话所属 workspace 的成员。 + * 仅允许全局管理员查看，避免同 workspace 成员互相看到对话。 * *

分支: *

* *

调用方签名不变;调用方若需在不查 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; } /** diff --git a/mateclaw-server/src/test/java/vip/mate/workspace/conversation/ConversationServiceOwnershipWorkspaceTest.java b/mateclaw-server/src/test/java/vip/mate/workspace/conversation/ConversationServiceOwnershipWorkspaceTest.java index 97b9a121..73213b5a 100644 --- a/mateclaw-server/src/test/java/vip/mate/workspace/conversation/ConversationServiceOwnershipWorkspaceTest.java +++ b/mateclaw-server/src/test/java/vip/mate/workspace/conversation/ConversationServiceOwnershipWorkspaceTest.java @@ -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. * - *

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). + *

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. * *

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()); } // ------------------------------------------------------------------ diff --git a/mateclaw-server/src/test/java/vip/mate/workspace/conversation/ConversationServiceWebchatVisibilityTest.java b/mateclaw-server/src/test/java/vip/mate/workspace/conversation/ConversationServiceWebchatVisibilityTest.java index b37e54e3..3a48631e 100644 --- a/mateclaw-server/src/test/java/vip/mate/workspace/conversation/ConversationServiceWebchatVisibilityTest.java +++ b/mateclaw-server/src/test/java/vip/mate/workspace/conversation/ConversationServiceWebchatVisibilityTest.java @@ -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> 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> 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