mirror of
https://gitee.com/mateos/mateclaw.git
synced 2026-09-13 03:13:41 +08:00
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.
This commit is contained in:
parent
8f8d43d693
commit
03a4cbd3e1
@ -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<ConversationEntity> wrapper = new LambdaQueryWrapper<ConversationEntity>()
|
||||
.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<ConversationEntity> 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<ConversationEntity> wrapper = new LambdaQueryWrapper<ConversationEntity>()
|
||||
.and(w -> applyOwnerScope(w, username, true))
|
||||
.and(w -> applyOwnerScope(w, username, isGlobalAdmin(username)))
|
||||
.isNull(ConversationEntity::getParentConversationId)
|
||||
.orderByDesc(ConversationEntity::getPinned)
|
||||
.orderByDesc(ConversationEntity::getLastActiveTime);
|
||||
|
||||
@ -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.
|
||||
*
|
||||
* <p>WebChat threads are owned by an external visitor principal
|
||||
* ({@code webchat:<visitorId>}), 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:<visitorId>}), 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.
|
||||
*
|
||||
* <p>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<LambdaQueryWrapper<ConversationEntity>> 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<LambdaQueryWrapper<ConversationEntity>> 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<LambdaQueryWrapper<ConversationEntity>> 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<LambdaQueryWrapper<ConversationEntity>> 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<LambdaQueryWrapper<ConversationEntity>> 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;
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user