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,