mirror of
https://gitee.com/mateos/mateclaw.git
synced 2026-09-13 03:13:41 +08:00
fix(security): cross-workspace guard on isConversationOwner (#344)
Closes the authorization asymmetry between list endpoints (which filter by workspaceId) and direct-access endpoints (which did not): a logged-in user could reach another workspace's system / IM / webchat-owned conversation by id and run any of messages / delete / rename / pin / setModel / clear / chat-files download on it. Per the maintainer's guidance on issue #344, workspaces are now treated as untrusted isolation boundaries — the fix is the cross-cutting hardening, kept out of feature work. Behavior change (only for shared, non-direct convs): - requester is a global admin (user.role=admin) → pass - requester is a member of the conversation's workspace → pass - otherwise → deny Preserved to avoid regressions: - direct owner (username == conv.username) → pass without lookup - convs without workspace_id (legacy rows) → legacy system-owner check - anonymous user (authService returns null, e.g. permitAll reconnect) → legacy system-owner check Callers in ConversationController / ChatController / SubagentController / GoalController / ApprovalController (18 sites) are unchanged — the signature stays isConversationOwner(conversationId, username). The workspace membership check is done via WorkspaceService.hasPermissionCached (Caffeine-backed, same cache the WorkspaceAccessInterceptor uses) and ignores the X-Workspace-Id header, which is client-controlled. Tests: 11 cases in ConversationServiceOwnershipWorkspaceTest covering each branch of the new logic. No caller-side test changes — the 66 caller tests (ConversationService*Test, ChatController*Test, SubagentController*Test, GoalController*Test, ApprovalController*Test) still pass.
This commit is contained in:
倪程伟
matevip
parent
6bbb6489f4
commit
cf5d47d249
@ -17,6 +17,8 @@ import vip.mate.approval.MetadataDecision;
|
||||
import vip.mate.agent.repository.AgentMapper;
|
||||
import vip.mate.approval.model.ToolApprovalEntity;
|
||||
import vip.mate.approval.repository.ToolApprovalMapper;
|
||||
import vip.mate.auth.model.UserEntity;
|
||||
import vip.mate.auth.service.AuthService;
|
||||
import vip.mate.channel.model.ChannelSessionEntity;
|
||||
import vip.mate.channel.repository.ChannelSessionMapper;
|
||||
import vip.mate.task.model.AsyncTaskEntity;
|
||||
@ -29,6 +31,7 @@ import vip.mate.workspace.conversation.repository.ConversationMapper;
|
||||
import vip.mate.workspace.conversation.repository.MessageMapper;
|
||||
import vip.mate.workspace.conversation.vo.ConversationVO;
|
||||
import vip.mate.workspace.conversation.vo.MessageVO;
|
||||
import vip.mate.workspace.core.service.WorkspaceService;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.nio.file.Files;
|
||||
@ -81,6 +84,8 @@ public class ConversationService {
|
||||
private final AsyncTaskMapper asyncTaskMapper;
|
||||
private final ChannelSessionMapper channelSessionMapper;
|
||||
private final ApplicationEventPublisher eventPublisher;
|
||||
private final AuthService authService;
|
||||
private final WorkspaceService workspaceService;
|
||||
|
||||
/**
|
||||
* Optional spill store. Injected via a setter so the existing @RequiredArgsConstructor
|
||||
@ -1516,14 +1521,37 @@ public class ConversationService {
|
||||
}
|
||||
|
||||
/**
|
||||
* Check whether a user owns the conversation, treating system-owned
|
||||
* rows (e.g. from scheduled jobs / IM channels) and external channel
|
||||
* principals ({@code webchat:<visitorId>}) as visible to every
|
||||
* authenticated user — otherwise the console could list a webchat
|
||||
* conversation but 403 on opening / deleting / renaming it.
|
||||
* 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.
|
||||
*
|
||||
* <p>校验用户是否拥有该会话。定时任务产生的会话（username = system）
|
||||
* 和 webchat 访客会话（username = webchat:<visitorId>）对所有登录用户可见。
|
||||
* <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 —
|
||||
* the list endpoints filtered by {@code workspaceId} but the direct-access
|
||||
* endpoints did not. Under a multi-tenant model where workspaces are
|
||||
* 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.
|
||||
*
|
||||
* <p>校验用户是否拥有该会话。直属会话直接放行;共享会话(system / IM / webchat)
|
||||
* 额外要求请求者是该会话所属 workspace 的成员。
|
||||
*
|
||||
* <p>分支:
|
||||
* <ul>
|
||||
* <li>会话不存在 → false</li>
|
||||
* <li>请求者是该会话的直属 owner → 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>
|
||||
* </ul>
|
||||
*
|
||||
* <p>调用方签名不变;调用方若需在不查 DB 的情况下做 admin 例外,可在外层先短路,
|
||||
* 但通常让本方法统一处理以避免散落的 admin 例外逻辑。注意:本方法不读
|
||||
* {@code X-Workspace-Id} header —— 该 header 客户端可伪造,以 DB 中的成员关系为准。
|
||||
*/
|
||||
public boolean isConversationOwner(String conversationId, String username) {
|
||||
ConversationEntity conv = conversationMapper.selectOne(
|
||||
@ -1532,10 +1560,28 @@ public class ConversationService {
|
||||
if (conv == null) {
|
||||
return false;
|
||||
}
|
||||
String owner = conv.getUsername();
|
||||
return username.equals(owner)
|
||||
|| SYSTEM_USER.equals(owner)
|
||||
|| (owner != null && owner.startsWith(WEBCHAT_OWNER_PREFIX));
|
||||
// 直属 owner 一律放行:会话由该用户创建,workspace 自然一致,无需再做成员校验。
|
||||
if (username != null && username.equals(conv.getUsername())) {
|
||||
return true;
|
||||
}
|
||||
// 共享会话(system / IM / webchat owner)以下收紧。
|
||||
Long convWorkspaceId = conv.getWorkspaceId();
|
||||
UserEntity requester = authService.findByUsername(username);
|
||||
// 老数据无 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());
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@ -0,0 +1,250 @@
|
||||
package vip.mate.workspace.conversation;
|
||||
|
||||
import com.fasterxml.jackson.databind.ObjectMapper;
|
||||
import org.junit.jupiter.api.BeforeEach;
|
||||
import org.junit.jupiter.api.DisplayName;
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.junit.jupiter.api.extension.ExtendWith;
|
||||
import org.mockito.InjectMocks;
|
||||
import org.mockito.Mock;
|
||||
import org.mockito.Spy;
|
||||
import org.mockito.junit.jupiter.MockitoExtension;
|
||||
import org.springframework.context.ApplicationEventPublisher;
|
||||
import vip.mate.agent.repository.AgentMapper;
|
||||
import vip.mate.approval.repository.ToolApprovalMapper;
|
||||
import vip.mate.auth.model.UserEntity;
|
||||
import vip.mate.auth.service.AuthService;
|
||||
import vip.mate.channel.repository.ChannelSessionMapper;
|
||||
import vip.mate.task.repository.AsyncTaskMapper;
|
||||
import vip.mate.workspace.conversation.model.ConversationEntity;
|
||||
import vip.mate.workspace.conversation.repository.ConversationMapper;
|
||||
import vip.mate.workspace.conversation.repository.MessageMapper;
|
||||
import vip.mate.workspace.core.service.WorkspaceService;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
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;
|
||||
|
||||
/**
|
||||
* Pin the cross-workspace authorization guard added to
|
||||
* {@link ConversationService#isConversationOwner(String, String)} in response
|
||||
* to issue #344.
|
||||
*
|
||||
* <p>Pre-fix behavior: any logged-in user could reach a system / IM / webchat
|
||||
* -owned conversation by id, regardless of which workspace the conversation
|
||||
* lived in. List endpoints filtered by {@code workspaceId}, direct-access
|
||||
* 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>Pure-Mockito (no Spring context) so the test stays fast and isolated.
|
||||
*
|
||||
* @author MateClaw Team
|
||||
*/
|
||||
@ExtendWith(MockitoExtension.class)
|
||||
class ConversationServiceOwnershipWorkspaceTest {
|
||||
|
||||
private static final String SYSTEM_CONV = "cron:daily-report";
|
||||
private static final String ALICE_CONV = "alice-uuid-1";
|
||||
private static final String WEBCHAT_CONV = "webchat:testkey1:vA";
|
||||
private static final long WS_TENANT_A = 10L;
|
||||
private static final long WS_TENANT_B = 20L;
|
||||
private static final long ALICE_USER_ID = 1001L;
|
||||
private static final long BOB_USER_ID = 1002L;
|
||||
private static final long ADMIN_USER_ID = 1003L;
|
||||
|
||||
@Mock private ConversationMapper conversationMapper;
|
||||
@Mock private MessageMapper messageMapper;
|
||||
@Mock private AgentMapper agentMapper;
|
||||
@Spy private ObjectMapper objectMapper = new ObjectMapper();
|
||||
@Mock private ToolApprovalMapper toolApprovalMapper;
|
||||
@Mock private AsyncTaskMapper asyncTaskMapper;
|
||||
@Mock private ChannelSessionMapper channelSessionMapper;
|
||||
@Mock private ApplicationEventPublisher eventPublisher;
|
||||
@Mock private AuthService authService;
|
||||
@Mock private WorkspaceService workspaceService;
|
||||
|
||||
@InjectMocks private ConversationService service;
|
||||
|
||||
@BeforeEach
|
||||
void stubUsers() {
|
||||
// Lenient stubs — not every test needs both users (admin-only tests
|
||||
// would trip strict-stubbing otherwise).
|
||||
org.mockito.Mockito.lenient().when(authService.findByUsername("alice"))
|
||||
.thenReturn(user(ALICE_USER_ID, "user"));
|
||||
org.mockito.Mockito.lenient().when(authService.findByUsername("bob"))
|
||||
.thenReturn(user(BOB_USER_ID, "user"));
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
// 1. Direct owner — no workspace check needed
|
||||
// ------------------------------------------------------------------
|
||||
|
||||
@Test
|
||||
@DisplayName("direct owner: always allowed, no membership lookup")
|
||||
void directOwnerShortCircuits() {
|
||||
when(conversationMapper.selectOne(any())).thenReturn(conv(ALICE_CONV, "alice", WS_TENANT_A));
|
||||
|
||||
assertThat(service.isConversationOwner(ALICE_CONV, "alice")).isTrue();
|
||||
|
||||
// Workspace membership is NOT consulted — alice owns it, end of story.
|
||||
verify(workspaceService, never()).hasPermissionCached(anyLong(), anyLong(), anyString());
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
// 2. System conv, same-workspace member → allowed
|
||||
// ------------------------------------------------------------------
|
||||
|
||||
@Test
|
||||
@DisplayName("system conv in requester's workspace: member passes")
|
||||
void systemConvSameWorkspaceMember() {
|
||||
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();
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
// 3. System conv, cross-workspace user → DENIED (the #344 fix)
|
||||
// ------------------------------------------------------------------
|
||||
|
||||
@Test
|
||||
@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();
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
// 4. Global admin bypass
|
||||
// ------------------------------------------------------------------
|
||||
|
||||
@Test
|
||||
@DisplayName("system conv in another workspace: global admin bypasses")
|
||||
void systemConvAdminBypass() {
|
||||
when(authService.findByUsername("admin")).thenReturn(user(ADMIN_USER_ID, "admin"));
|
||||
when(conversationMapper.selectOne(any())).thenReturn(conv(SYSTEM_CONV, "system", WS_TENANT_A));
|
||||
|
||||
// Admin skips the membership check entirely.
|
||||
assertThat(service.isConversationOwner(SYSTEM_CONV, "admin")).isTrue();
|
||||
verify(workspaceService, never()).hasPermissionCached(anyLong(), anyLong(), anyString());
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
// 5. Anonymous / permitAll reconnect — user lookup returns null
|
||||
// ------------------------------------------------------------------
|
||||
|
||||
@Test
|
||||
@DisplayName("system conv + null user record (anonymous reconnect): legacy behavior preserved")
|
||||
void anonymousReconnectLegacyFallback() {
|
||||
when(conversationMapper.selectOne(any())).thenReturn(conv(SYSTEM_CONV, "system", WS_TENANT_A));
|
||||
when(authService.findByUsername("anonymous")).thenReturn(null);
|
||||
|
||||
// Pre-fix: anonymous could see system convs. Preserve that.
|
||||
assertThat(service.isConversationOwner(SYSTEM_CONV, "anonymous")).isTrue();
|
||||
verify(workspaceService, never()).hasPermissionCached(anyLong(), anyLong(), anyString());
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("anonymous reconnect to a non-system conv: still rejected")
|
||||
void anonymousReconnectNonSystemConv() {
|
||||
when(conversationMapper.selectOne(any())).thenReturn(conv(ALICE_CONV, "alice", WS_TENANT_A));
|
||||
when(authService.findByUsername("anonymous")).thenReturn(null);
|
||||
|
||||
assertThat(service.isConversationOwner(ALICE_CONV, "anonymous")).isFalse();
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
// 6. Legacy rows without workspace_id — fall back to old logic
|
||||
// ------------------------------------------------------------------
|
||||
|
||||
@Test
|
||||
@DisplayName("conv without workspace_id: legacy system-owner check, no membership lookup")
|
||||
void legacyConvWithoutWorkspace() {
|
||||
when(conversationMapper.selectOne(any())).thenReturn(conv(SYSTEM_CONV, "system", null));
|
||||
|
||||
assertThat(service.isConversationOwner(SYSTEM_CONV, "bob")).isTrue();
|
||||
verify(workspaceService, never()).hasPermissionCached(anyLong(), anyLong(), anyString());
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
// 7. Webchat convs — already isolated; verify the fix doesn't open them
|
||||
// ------------------------------------------------------------------
|
||||
|
||||
@Test
|
||||
@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.
|
||||
assertThat(service.isConversationOwner(WEBCHAT_CONV, "alice")).isFalse();
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("webchat conv: global admin can still reach it (consistent with system convs)")
|
||||
void webchatConvAdminBypass() {
|
||||
when(authService.findByUsername("admin")).thenReturn(user(ADMIN_USER_ID, "admin"));
|
||||
when(conversationMapper.selectOne(any())).thenReturn(conv(WEBCHAT_CONV, "webchat:vA", WS_TENANT_A));
|
||||
|
||||
assertThat(service.isConversationOwner(WEBCHAT_CONV, "admin")).isTrue();
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
// 8. Edge cases
|
||||
// ------------------------------------------------------------------
|
||||
|
||||
@Test
|
||||
@DisplayName("conversation not found: false")
|
||||
void notFound() {
|
||||
when(conversationMapper.selectOne(any())).thenReturn(null);
|
||||
|
||||
assertThat(service.isConversationOwner("missing", "alice")).isFalse();
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("system conv + same-workspace member that the workspace service lost track of: rejected")
|
||||
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();
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------
|
||||
// helpers
|
||||
// ------------------------------------------------------------------
|
||||
|
||||
private static UserEntity user(long id, String role) {
|
||||
UserEntity u = new UserEntity();
|
||||
u.setId(id);
|
||||
u.setRole(role);
|
||||
return u;
|
||||
}
|
||||
|
||||
private static ConversationEntity conv(String conversationId, String username, Long workspaceId) {
|
||||
ConversationEntity c = new ConversationEntity();
|
||||
c.setConversationId(conversationId);
|
||||
c.setUsername(username);
|
||||
c.setWorkspaceId(workspaceId);
|
||||
return c;
|
||||
}
|
||||
}
|
||||
Loading…
Reference in New Issue
Block a user