From 430fc2673ce7f21bf546e64492abf92b4c95dc59 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Sun, 13 Sep 2026 15:56:16 -0700 Subject: [PATCH] Filter accounts on @type, the name Stalwart uses The Accounts list sent x:Account/query with {"type": "User"}, and a live 0.16 server refuses it: "unsupportedFilter - type". The registry keys a filter by the property's name on the object, which for the discriminator is @type, so the whole list failed to load. {"@type": "User"} is accepted. The mock took the wrong name without complaint, which is how it shipped. It now refuses any filter name the real server does not index for that object, answering the way Stalwart does. --- server/src/mock/directory.test.ts | 15 ++++++++++---- server/src/mock/directory.ts | 21 ++++++++++++++------ web/src/lib/__tests__/adminDirectory.test.ts | 16 +++++++++++++-- web/src/lib/adminDirectory.ts | 7 +++++-- 4 files changed, 45 insertions(+), 14 deletions(-) diff --git a/server/src/mock/directory.test.ts b/server/src/mock/directory.test.ts index 01c4fe8..8809495 100644 --- a/server/src/mock/directory.test.ts +++ b/server/src/mock/directory.test.ts @@ -21,7 +21,7 @@ test("an ordinary user is refused the directory outright", () => { test("helpdesk may read and edit but not create or delete", () => { const dir = make("helpdesk"); - const { ids } = dir.handlers["x:Account/query"]!({ filter: { type: "User" } }) as { ids: string[] }; + const { ids } = dir.handlers["x:Account/query"]!({ filter: { "@type": "User" } }) as { ids: string[] }; assert.ok(ids.length > 20); assert.throws(() => dir.handlers["x:Account/set"]!({ create: { n: { name: "x", domainId: "d1" } } }), (e: Refused) => e.type === "forbidden"); assert.throws(() => dir.handlers["x:Account/set"]!({ destroy: [ids[0]] }), (e: Refused) => e.type === "forbidden"); @@ -29,11 +29,11 @@ test("helpdesk may read and edit but not create or delete", () => { test("queries page, count and match text the way the client asks", () => { const dir = make("admin"); - const all = dir.handlers["x:Account/query"]!({ filter: { type: "User" }, calculateTotal: true }) as { ids: string[]; total: number }; - const page = dir.handlers["x:Account/query"]!({ filter: { type: "User" }, position: 10, limit: 5, calculateTotal: true }) as { ids: string[]; total: number }; + const all = dir.handlers["x:Account/query"]!({ filter: { "@type": "User" }, calculateTotal: true }) as { ids: string[]; total: number }; + const page = dir.handlers["x:Account/query"]!({ filter: { "@type": "User" }, position: 10, limit: 5, calculateTotal: true }) as { ids: string[]; total: number }; assert.equal(page.total, all.total); assert.deepEqual(page.ids, all.ids.slice(10, 15)); - const ada = dir.handlers["x:Account/query"]!({ filter: { type: "User", text: "lovelace" } }) as { ids: string[] }; + const ada = dir.handlers["x:Account/query"]!({ filter: { "@type": "User", text: "lovelace" } }) as { ids: string[] }; assert.equal(ada.ids.length, 1); assert.throws(() => dir.handlers["x:Account/query"]!({ filter: { operator: "OR", conditions: [] } }), (e: Refused) => e.type === "unsupportedFilter"); }); @@ -93,3 +93,10 @@ test("a domain's zone file is computed on read, with long keys split as the serv assert.match(zone, /IN MX 10 /); assert.match(zone, /_domainkey\.example\.com\. IN TXT \(\n {4}"/); }); + +test("a filter on a name the registry does not index is refused, as the live server refuses it", () => { + const dir = make("admin"); + // Seen on a live 0.16 server: "x:Account/query: unsupportedFilter - type". + assert.throws(() => dir.handlers["x:Account/query"]!({ filter: { type: "User" } }), (e: Refused) => e.type === "unsupportedFilter" && e.message === "type"); + assert.doesNotThrow(() => dir.handlers["x:Account/query"]!({ filter: { "@type": "Group", domainId: "d1", text: "x" } })); +}); diff --git a/server/src/mock/directory.ts b/server/src/mock/directory.ts index 1a9d4f7..b65379a 100644 --- a/server/src/mock/directory.ts +++ b/server/src/mock/directory.ts @@ -197,10 +197,19 @@ export function createDirectory(opts: Options) { return { accountId: opts.accountId, state: "1", list: found.map((x) => view(x, a.properties)), notFound: ids ? ids.filter((id) => !list.some((x) => x.id === id)) : [] }; }; - const query = (list: () => Obj[], perm: string, match: (o: Obj, filter: Obj) => boolean) => (a: Obj) => { + /** + * A query, filtered only on what the real server indexes for that object. + * Any other name is refused the way Stalwart refuses it -- `unsupportedFilter` + * with the name as the whole description -- because a mock that took + * `{"type": "User"}` let exactly that ship, and the live server answers it + * with "unsupportedFilter - type". + */ + const query = (list: () => Obj[], perm: string, filterable: string[], match: (o: Obj, filter: Obj) => boolean) => (a: Obj) => { demand(perm); const filter = (a.filter as Obj | undefined) ?? {}; if ("operator" in filter) throw opts.fail("unsupportedFilter", "Only AND is supported in filters"); + const unknown = Object.keys(filter).find((k) => !filterable.includes(k)); + if (unknown) throw opts.fail("unsupportedFilter", unknown); // Stalwart's default order is newest first, by id. const rows = list().filter((o) => match(o, filter)).sort((x, y) => String(y.id).localeCompare(String(x.id), undefined, { numeric: true })); const position = Math.max(0, Number(a.position ?? 0)); @@ -245,8 +254,8 @@ export function createDirectory(opts: Options) { const handlers: Record Obj> = { "x:Account/get": get(accounts, "sysAccountGet"), - "x:Account/query": query(() => accounts, "sysAccountQuery", (o, f) => - (f.type === undefined || o["@type"] === f.type) && (f.domainId === undefined || o.domainId === f.domainId) && matchText(o, f.text) && matchText(o, f.name)), + "x:Account/query": query(() => accounts, "sysAccountQuery", ["text", "@type", "domainId", "externalId", "memberGroupIds", "memberTenantId", "name"], (o, f) => + (f["@type"] === undefined || o["@type"] === f["@type"]) && (f.domainId === undefined || o.domainId === f.domainId) && matchText(o, f.text) && matchText(o, f.name)), "x:Account/set": (a) => { const created: Obj = {}; const notCreated: Obj = {}; @@ -319,7 +328,7 @@ export function createDirectory(opts: Options) { return { accountId: opts.accountId, oldState: "1", newState: "2", created, updated, destroyed, ...(Object.keys(notCreated).length ? { notCreated } : {}), ...(Object.keys(notUpdated).length ? { notUpdated } : {}), ...(Object.keys(notDestroyed).length ? { notDestroyed } : {}) }; }, "x:Domain/get": get(domains, "sysDomainGet"), - "x:Domain/query": query(() => domains, "sysDomainQuery", (o, f) => matchText(o, f.text) && matchText(o, f.name)), + "x:Domain/query": query(() => domains, "sysDomainQuery", ["text", "aliases", "memberTenantId", "name"], (o, f) => matchText(o, f.text) && matchText(o, f.name)), "x:Domain/set": (a) => { const created: Obj = {}; const notCreated: Obj = {}; @@ -366,7 +375,7 @@ export function createDirectory(opts: Options) { return { accountId: opts.accountId, oldState: "1", newState: "2", created, updated, destroyed, ...(Object.keys(notCreated).length ? { notCreated } : {}), ...(Object.keys(notUpdated).length ? { notUpdated } : {}), ...(Object.keys(notDestroyed).length ? { notDestroyed } : {}) }; }, "x:DkimSignature/get": get(dkimKeys, "sysDkimSignatureGet"), - "x:DkimSignature/query": query(() => dkimKeys, "sysDkimSignatureQuery", (o, f) => f.domainId === undefined || o.domainId === f.domainId), + "x:DkimSignature/query": query(() => dkimKeys, "sysDkimSignatureQuery", ["domainId", "memberTenantId"], (o, f) => f.domainId === undefined || o.domainId === f.domainId), "x:DkimSignature/set": (a) => { const destroyed: string[] = []; for (const id of (a.destroy as string[]) ?? []) { @@ -382,7 +391,7 @@ export function createDirectory(opts: Options) { return { accountId: opts.accountId, state: "1", list: ((a.ids as string[]) ?? ["ns1"]).filter((id) => id === "ns1").map((id) => ({ id, "@type": "Cloudflare", description: "Cloudflare (main zone)" })), notFound: [] }; }, "x:Role/get": get(roles, "sysRoleGet"), - "x:Role/query": query(() => roles, "sysRoleQuery", (o, f) => matchText(o, f.description)), + "x:Role/query": query(() => roles, "sysRoleQuery", ["text", "description", "memberTenantId"], (o, f) => matchText(o, f.description)), }; return { handlers, permissions: [...permissions], accounts }; diff --git a/web/src/lib/__tests__/adminDirectory.test.ts b/web/src/lib/__tests__/adminDirectory.test.ts index 8443570..98af899 100644 --- a/web/src/lib/__tests__/adminDirectory.test.ts +++ b/web/src/lib/__tests__/adminDirectory.test.ts @@ -1,5 +1,6 @@ -import { describe, expect, it } from "vitest"; -import { aliasList, describeDirectoryError, DirectoryError, hasPassword, passwordPatch, quotasWithDisk } from "@/lib/adminDirectory"; +import { describe, expect, it, vi } from "vitest"; +import { client } from "@/jmap/client"; +import { aliasList, describeDirectoryError, DirectoryError, hasPassword, passwordPatch, queryAccounts, quotasWithDisk } from "@/lib/adminDirectory"; describe("setting a password", () => { it("writes into the existing password credential, keeping its place", () => { @@ -43,3 +44,14 @@ describe("explaining a refusal", () => { expect(describeDirectoryError({ type: "forbidden", message: "x:Account/set: forbidden" })).toMatch(/refused/); }); }); + +describe("the account query", () => { + it("filters on @type, the property's name on the object", async () => { + // A live 0.16 server answers a plain `type` with "unsupportedFilter - type" + // and fails the whole list, which is how this was found. + const call = vi.spyOn(client, "call").mockResolvedValue({ ids: [], total: 0 }); + await queryAccounts({ type: "User", text: " ada ", position: 50, limit: 50 }); + expect(call).toHaveBeenCalledWith("x:Account/query", { filter: { "@type": "User", text: "ada" }, position: 50, limit: 50, calculateTotal: true }); + call.mockRestore(); + }); +}); diff --git a/web/src/lib/adminDirectory.ts b/web/src/lib/adminDirectory.ts index c2cd38a..e24e60c 100644 --- a/web/src/lib/adminDirectory.ts +++ b/web/src/lib/adminDirectory.ts @@ -19,7 +19,8 @@ import type { PermissionsMode, RoleDef, UserRoles } from "@/lib/adminAccess"; * `emailAddress` and `usedDiskQuota` are computed by the server. * - Secrets read back masked. A new password is written to the existing * password credential, so its id -- which OAuth tokens are tied to -- stays. - * - Filters are AND only, and the default order is newest first. + * - Filters are AND only, keyed by property name as it appears on the object + * (`@type`, not `type`), and the default order is newest first. * * Query and get are two requests rather than one with a result reference. * Whether the registry methods resolve back-references has not been checked on @@ -89,7 +90,9 @@ interface QueryResult { } export async function queryAccounts(opts: { type: "User" | "Group"; text?: string; position?: number; limit?: number }): Promise<{ ids: string[]; total: number }> { - const filter: Record = { type: opts.type }; + // The registry names the discriminator `@type`, as it is on the object. A + // plain `type` is not a property it knows and fails the whole query. + const filter: Record = { "@type": opts.type }; if (opts.text?.trim()) filter.text = opts.text.trim(); const res = await client.call("x:Account/query", { filter,