Validate the members file fully before an access change writes it

CHANGELOG.md

@@ -12,6 +12,7 @@ Until 1.0.0, releases stay on 0.x: `Added` bumps minor, `Fixed`/`Changed` (and t
 ### Fixed
 
 - `/spacestate`: the Sentry context for a failed call now says why the network failed (`errorCause`, such as `ENOTFOUND`, `ECONNREFUSED` or `UNEXPECTED_REDIRECT`), and shows the address without its query. Sentry was replacing the old redacted address with `[Filtered]`.
+- `/admin level set` and capability changes no longer write `members.yaml` when the file was hand-edited into a state that fails the startup cross-checks (an id in two entries, or an admin with no entry). Before, the file was saved but the in-memory view and the `access.changed` audit record were not, and the admin got a generic error. Now the change is refused with "The members file is invalid", and nothing is written.
 - `/schedule`: a schedule deleted, paused or edited while another one due at the same time was posting no longer goes out as it was. Pixel reads each schedule again right before posting it.
 - `/schedule`: a message or poll that `{{tokens}}` would push past Discord's length limits (2000 characters for a message, 300 for a question, 55 for an answer) is now refused when it's created or edited, instead of failing at every occurrence. One that still comes out too long when it's posted is cut to fit and logged as `schedule.clipped`.
 - `/schedule edit`: changing `when`, `repeat` or `days` now counts from the edit. Before, a weekly post that last went out on Monday and was moved to Thursday at 19:20 could post straight away for that Thursday 19:00, while the reply said next week.

docs/identity-and-access.md

@@ -81,9 +81,9 @@ Both files are loaded at startup into an in-memory view that is replaced as a wh
 
 **Changes go through the access store** (`core/ports/access-store.ts`, implemented by `services/access-store.ts`). One change at a time it:
 
-1. re-reads `members.yaml`, so edits made by hand aren't lost, and refuses if it has become invalid,
+1. re-reads `members.yaml`, so edits made by hand aren't lost, and refuses if it has become invalid (the same checks as at startup, including an id in two entries or an admin with no entry),
 2. edits the YAML document, so comments, order and quoting survive (IDs are written as quoted strings),
-3. re-validates the result and writes it to a temp file next to the original, then flushes it to disk,
+3. re-validates the result, builds the new view, and writes it to a temp file next to the original, then flushes it to disk,
 4. checks the file wasn't edited in the meantime, keeps the old one as `members.yaml.bak`, and renames the temp file over the original (atomic),
 5. only then swaps the in-memory view and records the change.
 

src/services/access-store.test.ts

@@ -401,6 +401,33 @@ describe("safe writes", () => {
 		expect(existsSync(`${membersFile}.bak`)).toBe(false);
 	});
 
+	it.each([
+		[
+			"an id in two entries",
+			`members:\n  - ids: ["discord:${IDS.member}"]\n    tier: member\n  - ids: ["discord:${IDS.member}"]\n    tier: friend\n${ADMIN_ENTRY}`,
+		],
+		["an admin with no entry", `members:\n  - ids: ["discord:${IDS.member}"]\n    tier: member\n`],
+	])("refuses a file that fails the cross-checks (%s), and changes nothing", async (_, broken) => {
+		const store = open();
+		writeFileSync(membersFile, broken);
+		const before = store.view;
+		for (const change of [
+			{ kind: "set-tier", id: NEW_ID, tier: "friend" },
+			{ kind: "set-tier", id: IDS.member, tier: "friend" },
+			{ kind: "set-capabilities", id: IDS.member, capabilities: ["back-door"] },
+		] as const) {
+			const error = await store.apply(change, by).catch((e: unknown) => e as Error);
+			expect(error).toBeInstanceOf(AccessStoreError);
+			expect((error as Error).message).toMatch(/invalid.*\/admin reload/s);
+			expect((error as Error).message).not.toContain(IDS.member);
+		}
+		expect(read()).toBe(broken);
+		expect(store.view).toBe(before);
+		expect(existsSync(`${membersFile}.bak`)).toBe(false);
+		expect(entries("access.changed")).toHaveLength(0);
+		expect(entries("access.file_invalid").length).toBeGreaterThan(0);
+	});
+
 	it("reports an unreadable file clearly, without its path", async () => {
 		const store = open();
 		rmSync(membersFile);

src/services/access-store.ts

@@ -41,9 +41,10 @@ import {
 /**
  * The bot-managed members file. Every change goes through `apply`, which:
  *
- *  1. re-reads the file (so hand edits aren't lost) and checks it is valid,
+ *  1. re-reads the file (so hand edits aren't lost) and checks it is valid, together
+ *     with the admins (the same checks as at startup),
  *  2. edits the YAML document, so comments and order survive,
- *  3. re-validates the result, writes it to a temp file and fsyncs it,
+ *  3. re-validates the result, builds the new view, writes it to a temp file and fsyncs it,
  *  4. checks the file wasn't edited meanwhile, keeps the old one as `.bak`,
  *  5. renames the temp file over the original (atomic on a normal filesystem),
  *  6. only then swaps the in-memory view and writes the audit record.
@@ -233,12 +234,15 @@ export class FileAccessStore implements AccessStore {
 		const next = doc.toString({ flowCollectionPadding: false });
 		// Our own edit must produce a valid file. If it doesn't, that is a bug, not user error.
 		const nextEntries = parseMembers(next, file);
+		// Build the new view before writing, so a failing cross-file check can't leave the file
+		// changed while the view and the audit record are not.
+		const nextView = buildAccessConfig(this.#admins, nextEntries, this.#paths);
 
 		this.#writeFile(file, raw, next);
 
 		const updated = nextEntries.find((entry) => entry.ids.includes(ref));
 		const after = toRecord(updated as NonNullable<typeof updated>);
-		this.#view = buildAccessConfig(this.#admins, nextEntries, this.#paths);
+		this.#view = nextView;
 		this.#audit(change, by, before, after);
 		return { before, after };
 	}
@@ -262,7 +266,10 @@ export class FileAccessStore implements AccessStore {
 
 	#parseCurrent(raw: string, file: string) {
 		try {
-			return parseMembers(raw, file);
+			const entries = parseMembers(raw, file);
+			// The same cross-file checks as startup: an id in two entries, an admin with no entry.
+			buildAccessConfig(this.#admins, entries, this.#paths);
+			return entries;
 		} catch (error) {
 			if (!(error instanceof AccessConfigError)) throw error;
 			this.#logger.warn({ event: "access.file_invalid" }, error.message);