Merge pull request #336 from Coffey-Labs/fix/account-type-filter

Filter accounts on @type, the name Stalwart uses

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 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 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" } }));
+});

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<string, (a: Obj) => Obj> = {
     "x:Account/get": get(accounts, "sysAccountGet"),
-    "x:Account/query": query(() => accounts, "sysAccountQuery", (o, f) =>
+    "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)),
+      (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 };

web/src/lib/__tests__/adminDirectory.test.ts

@@ -1,5 +1,6 @@
-import { describe, expect, it } from "vitest";
+import { describe, expect, it, vi } from "vitest";
-import { aliasList, describeDirectoryError, DirectoryError, hasPassword, passwordPatch, quotasWithDisk } from "@/lib/adminDirectory";
+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();
+  });
+});

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<string, unknown> = { 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<string, unknown> = { "@type": opts.type };
   if (opts.text?.trim()) filter.text = opts.text.trim();
   const res = await client.call<QueryResult>("x:Account/query", {
     filter,