From 03a4cbd3e1476053b1c02e5a36723af3a94a522f Mon Sep 17 00:00:00 2001 From: matevip Date: Thu, 18 Jun 2026 07:01:24 +0800 Subject: [PATCH] fix(webchat): gate admin-console webchat list visibility on global admin Align the conversation list/page with the isConversationOwner cross-workspace guard: only a global admin can open a webchat-owned conversation, so only admins should see those rows. Previously listConversations / pageConversations surfaced webchat principals to every authenticated user, who would then 403 on opening them (list-vs-access asymmetry). Also fixes ConversationServiceWebchatVisibilityTest, which still asserted the pre-guard owner behavior and never mocked AuthService, so it threw NPE at runtime once isConversationOwner started resolving the requester. The owner matrix is covered by ConversationServiceOwnershipWorkspaceTest; this test now pins the admin-gated list visibility for both admin and non-admin callers. --- .../conversation/ConversationService.java | 27 ++++++- ...versationServiceWebchatVisibilityTest.java | 77 ++++++++++++------- 2 files changed, 71 insertions(+), 33 deletions(-) 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 a49b597d..c578f3d5 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 @@ -142,8 +142,14 @@ public class ConversationService { // // 同时返回当前用户的会话和定时任务（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); LambdaQueryWrapper wrapper = new LambdaQueryWrapper() - .and(w -> applyOwnerScope(w, username, includeChannelPrincipals)) + .and(w -> applyOwnerScope(w, username, includeWebchat)) .isNull(ConversationEntity::getParentConversationId) .orderByDesc(ConversationEntity::getPinned) .orderByDesc(ConversationEntity::getLastActiveTime); @@ -197,6 +203,18 @@ public class ConversationService { } } + /** + * Whether the user is a global admin (role=admin), resolved from the DB — + * never from client-controlled data. Gates webchat row visibility in the + * admin-console list/page: {@link #isConversationOwner} only lets a global + * admin open a webchat-owned conversation, so only admins should see those + * rows — otherwise the console lists threads it would then 403 on. + */ + private boolean isGlobalAdmin(String username) { + UserEntity u = authService.findByUsername(username); + return u != null && "admin".equalsIgnoreCase(u.getRole()); + } + /** * Paginated variant used by the Sessions admin page. * @@ -216,10 +234,11 @@ 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, so include - // external webchat principals alongside the user's own + system rows. + // 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. LambdaQueryWrapper wrapper = new LambdaQueryWrapper() - .and(w -> applyOwnerScope(w, username, true)) + .and(w -> applyOwnerScope(w, username, isGlobalAdmin(username))) .isNull(ConversationEntity::getParentConversationId) .orderByDesc(ConversationEntity::getPinned) .orderByDesc(ConversationEntity::getLastActiveTime); 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 13d6b660..355a44f8 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 @@ -14,6 +14,8 @@ import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import vip.mate.agent.repository.AgentMapper; +import vip.mate.auth.model.UserEntity; +import vip.mate.auth.service.AuthService; import vip.mate.workspace.conversation.model.ConversationEntity; import vip.mate.workspace.conversation.repository.ConversationMapper; @@ -27,17 +29,23 @@ import static org.mockito.Mockito.when; * Pin the admin-console visibility of webchat conversations. * *

WebChat threads are owned by an external visitor principal - * ({@code webchat:}), not a MateClaw account. They must surface in - * the console list / page / owner-check the same way {@code system}-owned IM - * conversations do — otherwise they are silently invisible (the reported bug). - * The strict overload (used by the visitor self-service path) must NOT widen to - * other principals. + * ({@code webchat:}), not a MateClaw account. The admin-console + * list / page surface them alongside {@code system}-owned IM rows — but only + * for a global admin, because per the cross-workspace guard (issue #344) only a + * global admin can actually open a webchat-owned conversation. Listing them to + * a non-admin would show rows the caller would then 403 on, so the webchat + * clause is gated on the requester's role. The strict overload (visitor + * self-service path) never widens to other principals. + * + *

The owner-check matrix itself is covered by + * {@link ConversationServiceOwnershipWorkspaceTest}. */ @ExtendWith(MockitoExtension.class) class ConversationServiceWebchatVisibilityTest { @Mock private ConversationMapper conversationMapper; @Mock private AgentMapper agentMapper; + @Mock private AuthService authService; @InjectMocks private ConversationService service; @@ -55,8 +63,9 @@ class ConversationServiceWebchatVisibilityTest { } @Test - @DisplayName("lenient list includes webchat principals (username LIKE 'webchat:%')") - void lenientListIncludesWebchat() { + @DisplayName("lenient list, global admin: includes webchat principals (username LIKE 'webchat:%')") + void lenientListAdminIncludesWebchat() { + when(authService.findByUsername("admin")).thenReturn(user("admin")); ArgumentCaptor> captor = ArgumentCaptor.forClass(LambdaQueryWrapper.class); when(conversationMapper.selectList(captor.capture())).thenReturn(List.of()); @@ -70,7 +79,21 @@ class ConversationServiceWebchatVisibilityTest { } @Test - @DisplayName("strict list excludes webchat principals (no LIKE clause)") + @DisplayName("lenient list, non-admin: excludes webchat principals (no LIKE clause)") + void lenientListNonAdminExcludesWebchat() { + when(authService.findByUsername("alice")).thenReturn(user("member")); + ArgumentCaptor> captor = + ArgumentCaptor.forClass(LambdaQueryWrapper.class); + when(conversationMapper.selectList(captor.capture())).thenReturn(List.of()); + + service.listConversations("alice", 1L, true); + + String sql = captor.getValue().getTargetSql(); + assertThat(sql).doesNotContainIgnoringCase("like"); + } + + @Test + @DisplayName("strict list excludes webchat principals (no LIKE clause, no role lookup)") void strictListExcludesWebchat() { ArgumentCaptor> captor = ArgumentCaptor.forClass(LambdaQueryWrapper.class); @@ -83,8 +106,9 @@ class ConversationServiceWebchatVisibilityTest { } @Test - @DisplayName("page query includes webchat principals") - void pageIncludesWebchat() { + @DisplayName("page query, global admin: includes webchat principals") + void pageAdminIncludesWebchat() { + when(authService.findByUsername("admin")).thenReturn(user("admin")); ArgumentCaptor> captor = ArgumentCaptor.forClass(LambdaQueryWrapper.class); when(conversationMapper.selectPage(any(Page.class), captor.capture())) @@ -99,28 +123,23 @@ class ConversationServiceWebchatVisibilityTest { } @Test - @DisplayName("isConversationOwner: webchat + system + self visible; foreign user not") - void ownerCheckRecognizesWebchat() { - when(conversationMapper.selectOne(any(LambdaQueryWrapper.class))) - .thenReturn(ownedBy("webchat:visitor-1")); - assertThat(service.isConversationOwner("webchat:k:visitor-1", "admin")).isTrue(); + @DisplayName("page query, non-admin: excludes webchat principals") + void pageNonAdminExcludesWebchat() { + when(authService.findByUsername("alice")).thenReturn(user("member")); + ArgumentCaptor> captor = + ArgumentCaptor.forClass(LambdaQueryWrapper.class); + when(conversationMapper.selectPage(any(Page.class), captor.capture())) + .thenReturn(new Page<>()); - when(conversationMapper.selectOne(any(LambdaQueryWrapper.class))) - .thenReturn(ownedBy("system")); - assertThat(service.isConversationOwner("feishu:ou_x", "admin")).isTrue(); + service.pageConversations("alice", 1L, 1, 20, null); - when(conversationMapper.selectOne(any(LambdaQueryWrapper.class))) - .thenReturn(ownedBy("admin")); - assertThat(service.isConversationOwner("c1", "admin")).isTrue(); - - when(conversationMapper.selectOne(any(LambdaQueryWrapper.class))) - .thenReturn(ownedBy("bob")); - assertThat(service.isConversationOwner("c2", "admin")).isFalse(); + String sql = captor.getValue().getTargetSql(); + assertThat(sql).doesNotContainIgnoringCase("like"); } - private static ConversationEntity ownedBy(String username) { - ConversationEntity conv = new ConversationEntity(); - conv.setUsername(username); - return conv; + private static UserEntity user(String role) { + UserEntity u = new UserEntity(); + u.setRole(role); + return u; } }