mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-09 19:24:31 +00:00
fix(synology-chat): resolve Chat API user_id for reply delivery (#23709)
* fix(synology-chat): resolve Chat API user_id for reply delivery Synology Chat outgoing webhooks use a per-integration user_id that differs from the global Chat API user_id required by method=chatbot. This caused reply messages to fail silently when the IDs diverged. Changes: - Add fetchChatUsers() and resolveChatUserId() to resolve the correct Chat API user_id via the user_list endpoint (cached 5min) - Use resolved user_id for all sendMessage() calls in webhook handler and channel dispatcher - Add Provider field to MsgContext so the agent runner correctly identifies the message channel (was "unknown", now "synology-chat") - Log warnings when user_list API fails or when falling back to unresolved webhook user_id - Add 5 tests for user_id resolution (nickname, username, case, not-found, URL rewrite) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(synology-chat): use Readable stream in integration test for Windows compat Replace EventEmitter + process.nextTick with Readable stream for request body simulation. The process.nextTick approach caused the test to hang on Windows CI (120s timeout) because events were not reliably delivered to readBody() listeners. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: harden synology reply user resolution and cache scope (#23709) (thanks @druide67) --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
@@ -4,16 +4,18 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||
// Mock http and https modules before importing the client
|
||||
vi.mock("node:https", () => {
|
||||
const mockRequest = vi.fn();
|
||||
return { default: { request: mockRequest }, request: mockRequest };
|
||||
const mockGet = vi.fn();
|
||||
return { default: { request: mockRequest, get: mockGet }, request: mockRequest, get: mockGet };
|
||||
});
|
||||
|
||||
vi.mock("node:http", () => {
|
||||
const mockRequest = vi.fn();
|
||||
return { default: { request: mockRequest }, request: mockRequest };
|
||||
const mockGet = vi.fn();
|
||||
return { default: { request: mockRequest, get: mockGet }, request: mockRequest, get: mockGet };
|
||||
});
|
||||
|
||||
// Import after mocks are set up
|
||||
const { sendMessage, sendFileUrl } = await import("./client.js");
|
||||
const { sendMessage, sendFileUrl, fetchChatUsers, resolveChatUserId } = await import("./client.js");
|
||||
const https = await import("node:https");
|
||||
let fakeNowMs = 1_700_000_000_000;
|
||||
|
||||
@@ -111,3 +113,122 @@ describe("sendFileUrl", () => {
|
||||
expect(result).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
// Helper to mock the user_list API response for fetchChatUsers / resolveChatUserId
|
||||
function mockUserListResponse(
|
||||
users: Array<{ user_id: number; username: string; nickname: string }>,
|
||||
) {
|
||||
const httpsGet = vi.mocked((https as any).get);
|
||||
httpsGet.mockImplementation((_url: any, _opts: any, callback: any) => {
|
||||
const res = new EventEmitter() as any;
|
||||
res.statusCode = 200;
|
||||
process.nextTick(() => {
|
||||
callback(res);
|
||||
res.emit("data", Buffer.from(JSON.stringify({ success: true, data: { users } })));
|
||||
res.emit("end");
|
||||
});
|
||||
const req = new EventEmitter() as any;
|
||||
req.destroy = vi.fn();
|
||||
return req;
|
||||
});
|
||||
}
|
||||
|
||||
function mockUserListResponseOnce(
|
||||
users: Array<{ user_id: number; username: string; nickname: string }>,
|
||||
) {
|
||||
const httpsGet = vi.mocked((https as any).get);
|
||||
httpsGet.mockImplementationOnce((_url: any, _opts: any, callback: any) => {
|
||||
const res = new EventEmitter() as any;
|
||||
res.statusCode = 200;
|
||||
process.nextTick(() => {
|
||||
callback(res);
|
||||
res.emit("data", Buffer.from(JSON.stringify({ success: true, data: { users } })));
|
||||
res.emit("end");
|
||||
});
|
||||
const req = new EventEmitter() as any;
|
||||
req.destroy = vi.fn();
|
||||
return req;
|
||||
});
|
||||
}
|
||||
|
||||
describe("resolveChatUserId", () => {
|
||||
const baseUrl =
|
||||
"https://nas.example.com/webapi/entry.cgi?api=SYNO.Chat.External&method=chatbot&version=2&token=%22test%22";
|
||||
const baseUrl2 =
|
||||
"https://nas2.example.com/webapi/entry.cgi?api=SYNO.Chat.External&method=chatbot&version=2&token=%22test-2%22";
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
vi.useFakeTimers();
|
||||
// Advance time to invalidate any cached user list from previous tests
|
||||
fakeNowMs += 10 * 60 * 1000;
|
||||
vi.setSystemTime(fakeNowMs);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.useRealTimers();
|
||||
});
|
||||
|
||||
it("resolves user by nickname (webhook username = Chat nickname)", async () => {
|
||||
mockUserListResponse([
|
||||
{ user_id: 4, username: "jmn67", nickname: "jmn" },
|
||||
{ user_id: 7, username: "she67", nickname: "sarah" },
|
||||
]);
|
||||
const result = await resolveChatUserId(baseUrl, "jmn");
|
||||
expect(result).toBe(4);
|
||||
});
|
||||
|
||||
it("resolves user by username when nickname does not match", async () => {
|
||||
mockUserListResponse([
|
||||
{ user_id: 4, username: "jmn67", nickname: "" },
|
||||
{ user_id: 7, username: "she67", nickname: "sarah" },
|
||||
]);
|
||||
// Advance time to invalidate cache
|
||||
fakeNowMs += 10 * 60 * 1000;
|
||||
vi.setSystemTime(fakeNowMs);
|
||||
const result = await resolveChatUserId(baseUrl, "jmn67");
|
||||
expect(result).toBe(4);
|
||||
});
|
||||
|
||||
it("is case-insensitive", async () => {
|
||||
mockUserListResponse([{ user_id: 4, username: "JMN67", nickname: "JMN" }]);
|
||||
fakeNowMs += 10 * 60 * 1000;
|
||||
vi.setSystemTime(fakeNowMs);
|
||||
const result = await resolveChatUserId(baseUrl, "jmn");
|
||||
expect(result).toBe(4);
|
||||
});
|
||||
|
||||
it("returns undefined when user is not found", async () => {
|
||||
mockUserListResponse([{ user_id: 4, username: "jmn67", nickname: "jmn" }]);
|
||||
fakeNowMs += 10 * 60 * 1000;
|
||||
vi.setSystemTime(fakeNowMs);
|
||||
const result = await resolveChatUserId(baseUrl, "unknown_user");
|
||||
expect(result).toBeUndefined();
|
||||
});
|
||||
|
||||
it("uses method=user_list instead of method=chatbot in the API URL", async () => {
|
||||
mockUserListResponse([]);
|
||||
fakeNowMs += 10 * 60 * 1000;
|
||||
vi.setSystemTime(fakeNowMs);
|
||||
await resolveChatUserId(baseUrl, "anyone");
|
||||
const httpsGet = vi.mocked((https as any).get);
|
||||
expect(httpsGet).toHaveBeenCalledWith(
|
||||
expect.stringContaining("method=user_list"),
|
||||
expect.any(Object),
|
||||
expect.any(Function),
|
||||
);
|
||||
});
|
||||
|
||||
it("keeps user cache scoped per incoming URL", async () => {
|
||||
mockUserListResponseOnce([{ user_id: 4, username: "jmn67", nickname: "jmn" }]);
|
||||
mockUserListResponseOnce([{ user_id: 9, username: "jmn67", nickname: "jmn" }]);
|
||||
|
||||
const result1 = await resolveChatUserId(baseUrl, "jmn");
|
||||
const result2 = await resolveChatUserId(baseUrl2, "jmn");
|
||||
|
||||
expect(result1).toBe(4);
|
||||
expect(result2).toBe(9);
|
||||
const httpsGet = vi.mocked((https as any).get);
|
||||
expect(httpsGet).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user