From 4ec05c56a9ce114c5d6f557c0d8a680b345f0da9 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sat, 29 Aug 2026 23:53:38 +0100 Subject: [PATCH 01/12] create: keep ids server-owned --- .github/workflows/ci.yml | 9 +++ CHANGELOG.md | 1 + docker/Dockerfile | 6 +- docker/docker-compose.yml | 2 + lib/group/store/file.js | 6 +- lib/nameserver/store/file.js | 7 ++- lib/session/store/file.js | 7 +-- lib/store/file.js | 4 ++ lib/store/file.test.js | 7 ++- lib/user/store/file.js | 6 +- lib/zone/store/file.js | 7 ++- lib/zone_record/store/file.js | 7 +-- routes/create_id.test.js | 106 ++++++++++++++++++++++++++++++++++ routes/group.test.js | 31 +++++++--- routes/nameserver.test.js | 36 ++++++------ routes/permission.test.js | 16 ++--- routes/user.test.js | 31 +++++++--- routes/zone.test.js | 39 +++++++------ routes/zone_record.test.js | 13 +++++ 19 files changed, 264 insertions(+), 77 deletions(-) create mode 100644 routes/create_id.test.js diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f8c75d7..3e5de11 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,6 +9,9 @@ on: permissions: contents: read +env: + NICTOOL_VALIDATE_SPEC: https://github.com/NicTool/validate/archive/c7d2776fd465b4118eb6d90449d1ecae42220dd5.tar.gz + jobs: lint: uses: NicTool/.github/.github/workflows/lint.yml@main @@ -43,6 +46,8 @@ jobs: node-version: ${{ matrix.node-version }} - run: sh sql/init-mysql.sh - run: npm install + - name: Install proposed validate source + run: npm install --no-save ${{ env.NICTOOL_VALIDATE_SPEC }} - run: npm test test-mac: @@ -63,6 +68,8 @@ jobs: node-version: ${{ matrix.node-version }} - run: sh sql/init-mysql.sh - run: npm install + - name: Install proposed validate source + run: npm install --no-save ${{ env.NICTOOL_VALIDATE_SPEC }} - run: npm test test-docker: @@ -100,4 +107,6 @@ jobs: node-version: ${{ matrix.node-version }} - run: sh sql/init-mysql.sh - run: npm install + - name: Install proposed validate source + run: npm install --no-save ${{ env.NICTOOL_VALIDATE_SPEC }} - run: sh test/run.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index b71da26..915db80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - fix: GET /zone/{id}/ns reads nameservers through the zone store - fix(mysql): read v2 group permission rows, and save partial permission changes - fix: file store login checks the group in user@group +- create: allocate ids internally and reject caller-supplied ids ### [3.0.3] - 2026-07-27 diff --git a/docker/Dockerfile b/docker/Dockerfile index 0fea311..88437ff 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -2,7 +2,11 @@ FROM node:22-trixie-slim WORKDIR /app COPY package*.json . # --omit=dev is safe: tests use node:test (stdlib), devDeps are only eslint/prettier -RUN npm install --omit=dev +ARG NICTOOL_VALIDATE_SPEC +RUN npm install --omit=dev \ + && if [ -n "$NICTOOL_VALIDATE_SPEC" ]; then \ + npm install --omit=dev --no-save "$NICTOOL_VALIDATE_SPEC"; \ + fi COPY . . EXPOSE 3000 CMD ["node", "server.js"] diff --git a/docker/docker-compose.yml b/docker/docker-compose.yml index 505c3aa..60e55dc 100644 --- a/docker/docker-compose.yml +++ b/docker/docker-compose.yml @@ -24,6 +24,8 @@ services: build: context: .. dockerfile: docker/Dockerfile + args: + NICTOOL_VALIDATE_SPEC: ${NICTOOL_VALIDATE_SPEC:-} ports: - '${API_PORT:-3000}:3000' depends_on: diff --git a/lib/group/store/file.js b/lib/group/store/file.js index b2680cd..49418b9 100644 --- a/lib/group/store/file.js +++ b/lib/group/store/file.js @@ -1,4 +1,4 @@ -import FileStore from '../../store/file.js' +import FileStore, { nextId } from '../../store/file.js' import { idConflict } from '../../store/error.js' import GroupBase from './base.js' @@ -72,6 +72,9 @@ class GroupRepoFile extends GroupBase { const usable_ns = args.usable_ns ?? [] delete args.usable_ns + const groups = await this._load() + if (args.id === undefined) args.id = nextId(groups) + const gid = args.id args.permissions = { ...JSON.parse(JSON.stringify(defaultPermissions)), @@ -87,7 +90,6 @@ class GroupRepoFile extends GroupBase { }, } - const groups = await this._load() groups.push(args) await this._save(groups) return gid diff --git a/lib/nameserver/store/file.js b/lib/nameserver/store/file.js index 40c16b0..a9d4ba2 100644 --- a/lib/nameserver/store/file.js +++ b/lib/nameserver/store/file.js @@ -1,4 +1,4 @@ -import FileStore from '../../store/file.js' +import FileStore, { nextId } from '../../store/file.js' import { idConflict } from '../../store/error.js' import NameserverBase from './base.js' @@ -40,6 +40,8 @@ class NameserverRepoFile extends NameserverBase { } async create(args, options) { + args = JSON.parse(JSON.stringify(args)) + if (args.id !== undefined) { const existing = [ ...(await this.get({ id: args.id })), @@ -49,7 +51,8 @@ class NameserverRepoFile extends NameserverBase { } const nameservers = await this._load() - nameservers.push(JSON.parse(JSON.stringify(args))) + if (args.id === undefined) args.id = nextId(nameservers) + nameservers.push(args) await this._save(nameservers) return args.id } diff --git a/lib/session/store/file.js b/lib/session/store/file.js index d3d959f..cb5c15e 100644 --- a/lib/session/store/file.js +++ b/lib/session/store/file.js @@ -1,4 +1,4 @@ -import FileStore from '../../store/file.js' +import FileStore, { nextId } from '../../store/file.js' // Map legacy nt_* column names to the friendly API names used throughout. function normalizeArgs(args) { @@ -37,11 +37,10 @@ class SessionRepoFile { if (existing) return existing.id const sessions = await this._load() - const nextId = sessions.reduce((max, s) => Math.max(max, s.id ?? 0), 0) + 1 - args.id = nextId + args.id = nextId(sessions) sessions.push(args) await this._save(sessions) - return nextId + return args.id } async get(args) { diff --git a/lib/store/file.js b/lib/store/file.js index 8ec2c9e..cff052d 100644 --- a/lib/store/file.js +++ b/lib/store/file.js @@ -24,6 +24,10 @@ export function resolveCodec(type = storeConfig().type) { return codecs[type] ?? jsonCodec } +export function nextId(rows) { + return rows.reduce((max, row) => Math.max(max, row.id ?? 0), 0) + 1 +} + /** * One file per entity, holding an array of rows under a single top-level key. * The codec is chosen by store.type; everything above this layer is identical diff --git a/lib/store/file.test.js b/lib/store/file.test.js index c76b6e2..33c003b 100644 --- a/lib/store/file.test.js +++ b/lib/store/file.test.js @@ -4,7 +4,7 @@ import os from 'node:os' import path from 'node:path' import { describe, it, before, after, beforeEach } from 'node:test' -import FileStore, { resolveCodec } from './file.js' +import FileStore, { nextId, resolveCodec } from './file.js' const envKeys = ['NICTOOL_DATA_STORE', 'NICTOOL_DATA_STORE_PATH', 'NICTOOL_CONF_DIR'] @@ -12,6 +12,11 @@ describe('file store', () => { const savedEnv = {} let tmp + it('allocates after the highest existing id', () => { + assert.equal(nextId([]), 1) + assert.equal(nextId([{ id: 9 }, { id: 2 }, {}]), 10) + }) + before(() => { for (const key of envKeys) { savedEnv[key] = process.env[key] diff --git a/lib/user/store/file.js b/lib/user/store/file.js index 4d2c727..87a8568 100644 --- a/lib/user/store/file.js +++ b/lib/user/store/file.js @@ -1,4 +1,4 @@ -import FileStore from '../../store/file.js' +import FileStore, { nextId } from '../../store/file.js' import { idConflict } from '../../store/error.js' import Config from '../../config.js' import Credentials from '../credentials.js' @@ -152,6 +152,9 @@ class UserRepoFile extends UserBase { Object.assign(args, await Credentials.forStorage(args.password, args.pass_salt)) } + const users = await this._load() + if (args.id === undefined) args.id = nextId(users) + if (inherit === false) { args.permissions = { ...JSON.parse(JSON.stringify(defaultPermissions)), @@ -161,7 +164,6 @@ class UserRepoFile extends UserBase { } } - const users = await this._load() users.push(args) await this._save(users) return args.id diff --git a/lib/zone/store/file.js b/lib/zone/store/file.js index bc550cc..e68d865 100644 --- a/lib/zone/store/file.js +++ b/lib/zone/store/file.js @@ -1,4 +1,4 @@ -import FileStore, { resolveCodec } from '../../store/file.js' +import FileStore, { nextId, resolveCodec } from '../../store/file.js' import { idConflict } from '../../store/error.js' import ZoneBase from './base.js' @@ -36,6 +36,8 @@ class ZoneRepoFile extends ZoneBase { } async create(args, options) { + args = JSON.parse(JSON.stringify(args)) + if (args.id !== undefined) { const existing = [ ...(await this.get({ id: args.id })), @@ -45,7 +47,8 @@ class ZoneRepoFile extends ZoneBase { } const zones = await this._load() - zones.push(JSON.parse(JSON.stringify(args))) + if (args.id === undefined) args.id = nextId(zones) + zones.push(args) await this._save(zones) return args.id } diff --git a/lib/zone_record/store/file.js b/lib/zone_record/store/file.js index e05d797..1e83842 100644 --- a/lib/zone_record/store/file.js +++ b/lib/zone_record/store/file.js @@ -1,4 +1,4 @@ -import FileStore from '../../store/file.js' +import FileStore, { nextId } from '../../store/file.js' import { idConflict } from '../../store/error.js' import ZoneRecordBase from './base.js' @@ -30,10 +30,7 @@ class ZoneRecordRepoFile extends ZoneRecordBase { const records = await this._load() - if (!args.id) { - const maxId = records.reduce((max, r) => Math.max(max, r.id ?? 0), 0) - args.id = maxId + 1 - } + if (args.id === undefined) args.id = nextId(records) if (args.ttl === undefined) args.ttl = 0 diff --git a/routes/create_id.test.js b/routes/create_id.test.js new file mode 100644 index 0000000..2fc8850 --- /dev/null +++ b/routes/create_id.test.js @@ -0,0 +1,106 @@ +import assert from 'node:assert/strict' +import { after, before, describe, it } from 'node:test' + +import Group from '../lib/group/index.js' +import Nameserver from '../lib/nameserver/index.js' +import Permission from '../lib/permission/index.js' +import User from '../lib/user/index.js' +import Zone from '../lib/zone/index.js' +import ZoneRecord from '../lib/zone_record/index.js' +import { init } from './index.js' + +import groupCase from './test/group.json' with { type: 'json' } +import nameserverCase from './test/nameserver.json' with { type: 'json' } +import permissionCase from './test/permission.json' with { type: 'json' } +import userCase from './test/user.json' with { type: 'json' } +import zoneCase from './test/zone.json' with { type: 'json' } + +const chosenId = 65001 +let server +const auth = { headers: {} } + +const userPayload = { + ...userCase, + id: chosenId, + gid: groupCase.id, + email: 'caller-id@example.com', + username: 'caller-id-user', +} +delete userPayload.deleted + +const cases = [ + ['group', { ...groupCase, id: chosenId, name: 'caller-id-group' }], + ['nameserver', { ...nameserverCase, id: chosenId, gid: groupCase.id, name: 'caller-id.ns.example.com.' }], + [ + 'permission', + { + ...permissionCase, + id: chosenId, + group: { ...permissionCase.group, id: chosenId }, + user: { ...permissionCase.user, id: chosenId }, + }, + ], + ['user', userPayload], + ['zone', { ...zoneCase, id: chosenId, gid: groupCase.id, zone: 'caller-id.example.com.' }], + [ + 'zone_record', + { + id: chosenId, + zid: zoneCase.id, + owner: 'caller-id.route.example.com.', + ttl: 300, + type: 'A', + address: '203.0.113.111', + }, + ], +] + +before(async () => { + await ZoneRecord.destroy({ id: chosenId }) + await Zone.destroy({ id: chosenId }) + await User.destroy({ id: chosenId }) + await Permission.destroy({ id: chosenId }) + await Nameserver.destroy({ id: chosenId }) + await Group.destroy({ id: chosenId }) + await Group.create(groupCase) + await User.create(userCase) + await Zone.create(zoneCase) + server = await init() + + const res = await server.inject({ + method: 'POST', + url: '/session', + payload: { + username: `${userCase.username}@${groupCase.name}`, + password: userCase.password, + }, + }) + assert.equal(res.statusCode, 200) + auth.headers = { Authorization: `Bearer ${res.result.session.token}` } +}) + +after(async () => { + await ZoneRecord.destroy({ id: chosenId }) + await Zone.destroy({ id: chosenId }) + await User.destroy({ id: chosenId }) + await Permission.destroy({ id: chosenId }) + await Nameserver.destroy({ id: chosenId }) + await Group.destroy({ id: chosenId }) + await server.stop() +}) + +describe('caller-supplied create ids', () => { + for (const [entity, payload] of cases) { + it(`rejects an id on POST /${entity}`, async () => { + const res = await server.inject({ + method: 'POST', + url: `/${entity}`, + headers: auth.headers, + payload, + }) + + assert.equal(res.statusCode, 400) + assert.match(res.result.message, /"id" is not allowed/) + }) + } +}) diff --git a/routes/group.test.js b/routes/group.test.js index 8945093..f27e7a8 100644 --- a/routes/group.test.js +++ b/routes/group.test.js @@ -9,7 +9,7 @@ import groupCase from './test/group.json' with { type: 'json' } import userCase from './test/user.json' with { type: 'json' } let server -const case2Id = 4094 +let case2Id before(async () => { server = await init() @@ -18,7 +18,7 @@ before(async () => { }) after(async () => { - await Group.destroy({ id: case2Id }) + if (case2Id) await Group.destroy({ id: case2Id }) await server.stop() }) @@ -49,9 +49,9 @@ describe('group routes', () => { assert.equal(res.result.group[0].id, groupCase.id) }) - it('POST /group', async () => { + it('POST /group allocates an id', async () => { const testCase = JSON.parse(JSON.stringify(groupCase)) - testCase.id = case2Id // make it unique + delete testCase.id testCase.name = `example2.com` delete testCase.deleted @@ -62,9 +62,24 @@ describe('group routes', () => { payload: testCase, }) assert.equal(res.statusCode, 201) + assert.equal(res.result.group.length, 1) + assert.equal(res.result.group[0].name, 'example2.com') + case2Id = res.result.group[0].id + assert.ok(Number.isInteger(case2Id)) }) - it(`GET /group/${case2Id}`, async () => { + it('PUT /group/{id} keeps using the route id', async () => { + const res = await server.inject({ + method: 'PUT', + url: `/group/${case2Id}`, + headers: auth.headers, + payload: { name: 'example3.com' }, + }) + assert.equal(res.statusCode, 200) + assert.equal(res.result.group[0].name, 'example3.com') + }) + + it('GET /group/{id}', async () => { const res = await server.inject({ method: 'GET', url: `/group/${case2Id}`, @@ -74,7 +89,7 @@ describe('group routes', () => { assert.equal(res.result.group[0].id, case2Id) }) - it(`DELETE /group/${case2Id}`, async () => { + it('DELETE /group/{id}', async () => { const res = await server.inject({ method: 'DELETE', url: `/group/${case2Id}`, @@ -83,7 +98,7 @@ describe('group routes', () => { assert.equal(res.statusCode, 200) }) - it(`GET /group/${case2Id} (soft-deleted → empty array)`, async () => { + it('GET /group/{id} hides a soft-deleted group', async () => { const res = await server.inject({ method: 'GET', url: `/group/${case2Id}`, @@ -93,7 +108,7 @@ describe('group routes', () => { assert.deepEqual(res.result.group, []) }) - it(`GET /group/${case2Id} (deleted)`, async () => { + it('GET /group/{id}?deleted=true returns a soft-deleted group', async () => { const res = await server.inject({ method: 'GET', url: `/group/${case2Id}?deleted=true`, diff --git a/routes/nameserver.test.js b/routes/nameserver.test.js index b9c65d1..d9dc710 100644 --- a/routes/nameserver.test.js +++ b/routes/nameserver.test.js @@ -11,10 +11,9 @@ import userCase from './test/user.json' with { type: 'json' } import nsCase from './test/nameserver.json' with { type: 'json' } let server -let case2Id = 4094 +let case2Id before(async () => { - await Nameserver.destroy({ id: case2Id }) await Group.create(groupCase, { ifExists: 'return' }) await User.create(userCase, { ifExists: 'return' }) await Nameserver.create(nsCase, { ifExists: 'return' }) @@ -22,7 +21,7 @@ before(async () => { }) after(async () => { - await Nameserver.destroy({ id: case2Id }) + if (case2Id) await Nameserver.destroy({ id: case2Id }) await server.stop() }) @@ -53,9 +52,9 @@ describe('nameserver routes', () => { assert.equal(res.result.nameserver[0].name, nsCase.name) }) - it(`POST /nameserver (${case2Id})`, async () => { + it('POST /nameserver allocates an id', async () => { const testCase = JSON.parse(JSON.stringify(nsCase)) - testCase.id = case2Id // make it unique + delete testCase.id testCase.name = 'c.ns.example.com.' const res = await server.inject({ @@ -65,32 +64,35 @@ describe('nameserver routes', () => { payload: testCase, }) assert.equal(res.statusCode, 201) + assert.equal(res.result.nameserver.length, 1) + assert.equal(res.result.nameserver[0].name, 'c.ns.example.com.') assert.ok(res.result.nameserver[0].gid) + case2Id = res.result.nameserver[0].id + assert.ok(Number.isInteger(case2Id)) }) - it(`GET /nameserver/${case2Id}`, async () => { + it('PUT /nameserver/{id} keeps using the route id', async () => { const res = await server.inject({ - method: 'GET', + method: 'PUT', url: `/nameserver/${case2Id}`, headers: auth.headers, + payload: { description: 'updated' }, }) assert.equal(res.statusCode, 200) - assert.ok(res.result.nameserver[0].gid) + assert.equal(res.result.nameserver[0].description, 'updated') }) - it(`PUT /nameserver/${case2Id}`, async () => { + it('GET /nameserver/{id}', async () => { const res = await server.inject({ - method: 'PUT', + method: 'GET', url: `/nameserver/${case2Id}`, headers: auth.headers, - payload: { description: 'edited by the route test' }, }) - assert.equal(res.statusCode, 200) - assert.equal((await Nameserver.get({ id: case2Id }))[0].description, 'edited by the route test') + assert.ok(res.result.nameserver[0].gid) }) - it(`DELETE /nameserver/${case2Id}`, async () => { + it('DELETE /nameserver/{id}', async () => { const res = await server.inject({ method: 'DELETE', url: `/nameserver/${case2Id}`, @@ -99,7 +101,7 @@ describe('nameserver routes', () => { assert.equal(res.statusCode, 200) }) - it(`DELETE /nameserver/${case2Id}`, async () => { + it('DELETE /nameserver/{id} returns 404 when already deleted', async () => { const res = await server.inject({ method: 'DELETE', url: `/nameserver/${case2Id}`, @@ -108,7 +110,7 @@ describe('nameserver routes', () => { assert.equal(res.statusCode, 404) }) - it(`GET /nameserver/${case2Id}`, async () => { + it('GET /nameserver/{id} hides a soft-deleted nameserver', async () => { const res = await server.inject({ method: 'GET', url: `/nameserver/${case2Id}`, @@ -117,7 +119,7 @@ describe('nameserver routes', () => { assert.deepEqual(res.result.nameserver, []) }) - it(`GET /nameserver/${case2Id} (deleted)`, async () => { + it('GET /nameserver/{id}?deleted=true returns a soft-deleted nameserver', async () => { const res = await server.inject({ method: 'GET', url: `/nameserver/${case2Id}?deleted=true`, diff --git a/routes/permission.test.js b/routes/permission.test.js index 39069b8..9c9eddf 100644 --- a/routes/permission.test.js +++ b/routes/permission.test.js @@ -53,7 +53,7 @@ describe('permission routes', () => { assert.equal(res.result.permission.nameserver.create, false) }) - it('POST /permission', async () => { + it('POST /permission allocates an id', async () => { const testCase = JSON.parse(JSON.stringify(permCase)) delete testCase.id testCase.user.id = targetId @@ -68,9 +68,11 @@ describe('permission routes', () => { payload: testCase, }) assert.equal(res.statusCode, 201) - case2Id = res.result.permission.id + assert.equal(res.result.permission.name, 'Route Test Permission 2') assert.equal(res.result.permission.zone.create, true) assert.equal(res.result.permission.nameserver.create, false) + case2Id = res.result.permission.id + assert.ok(Number.isInteger(case2Id)) }) it('POST /permission rejects an existing target', async () => { @@ -94,7 +96,7 @@ describe('permission routes', () => { assert.equal(existing.name, 'Route Test Permission 2') }) - it('GET the created permission', async () => { + it('GET /permission/{id}', async () => { const res = await server.inject({ method: 'GET', url: `/permission/${case2Id}`, @@ -106,7 +108,7 @@ describe('permission routes', () => { assert.equal(res.result.permission.nameserver.create, false) }) - it('DELETE the created permission', async () => { + it('DELETE /permission/{id}', async () => { const res = await server.inject({ method: 'DELETE', url: `/permission/${case2Id}`, @@ -116,7 +118,7 @@ describe('permission routes', () => { assert.equal(res.statusCode, 200) }) - it('DELETE the created permission again', async () => { + it('DELETE /permission/{id} returns 404 when already deleted', async () => { const res = await server.inject({ method: 'DELETE', url: `/permission/${case2Id}`, @@ -126,7 +128,7 @@ describe('permission routes', () => { assert.equal(res.statusCode, 404) }) - it('GET the deleted permission', async () => { + it('GET /permission/{id} hides a soft-deleted permission', async () => { const res = await server.inject({ method: 'GET', url: `/permission/${case2Id}`, @@ -137,7 +139,7 @@ describe('permission routes', () => { assert.equal(res.result.permission, undefined) }) - it('GET the deleted permission with deleted=true', async () => { + it('GET /permission/{id}?deleted=true returns a soft-deleted permission', async () => { const res = await server.inject({ method: 'GET', url: `/permission/${case2Id}?deleted=true`, diff --git a/routes/user.test.js b/routes/user.test.js index fa97de4..b11987f 100644 --- a/routes/user.test.js +++ b/routes/user.test.js @@ -17,10 +17,10 @@ before(async () => { await User.create(userCase, { ifExists: 'return' }) }) -const userId2 = 4094 +let userId2 after(async () => { - User.destroy({ id: userId2 }) + if (userId2) await User.destroy({ id: userId2 }) await server.stop() }) @@ -57,9 +57,9 @@ describe('user routes', () => { assert.equal(res.statusCode, 200) }) - it('POST /user', async () => { + it('POST /user allocates an id', async () => { const testCase = JSON.parse(JSON.stringify(userCase)) - testCase.id = userId2 // make it unique + delete testCase.id testCase.username = `${testCase.username}2` delete testCase.deleted @@ -70,9 +70,24 @@ describe('user routes', () => { payload: testCase, }) assert.equal(res.statusCode, 201) + assert.equal(res.result.user.length, 1) + assert.equal(res.result.user[0].username, `${userCase.username}2`) + userId2 = res.result.user[0].id + assert.ok(Number.isInteger(userId2)) }) - it(`GET /user/${userId2}`, async () => { + it('PUT /user/{id} keeps using the route id', async () => { + const res = await server.inject({ + method: 'PUT', + url: `/user/${userId2}`, + headers: auth.headers, + payload: { first_name: 'Updated' }, + }) + assert.equal(res.statusCode, 200) + assert.equal(res.result.user[0].first_name, 'Updated') + }) + + it('GET /user/{id}', async () => { const res = await server.inject({ method: 'GET', url: `/user/${userId2}`, @@ -81,7 +96,7 @@ describe('user routes', () => { assert.equal(res.statusCode, 200) }) - it(`DELETE /user/${userId2}`, async () => { + it('DELETE /user/{id}', async () => { const res = await server.inject({ method: 'DELETE', url: `/user/${userId2}`, @@ -90,7 +105,7 @@ describe('user routes', () => { assert.equal(res.statusCode, 200) }) - it(`GET /user/${userId2} (deleted)`, async () => { + it('GET /user/{id} hides a soft-deleted user', async () => { const res = await server.inject({ method: 'GET', url: `/user/${userId2}`, @@ -99,7 +114,7 @@ describe('user routes', () => { assert.ok([200, 204].includes(res.statusCode)) }) - it(`GET /user/${userId2}?deleted=true`, async () => { + it('GET /user/{id}?deleted=true returns a soft-deleted user', async () => { const res = await server.inject({ method: 'GET', url: `/user/${userId2}?deleted=true`, diff --git a/routes/zone.test.js b/routes/zone.test.js index 7e88339..1120ab3 100644 --- a/routes/zone.test.js +++ b/routes/zone.test.js @@ -11,14 +11,13 @@ import userCase from './test/user.json' with { type: 'json' } import nsCase from './test/zone.json' with { type: 'json' } let server -let case2Id = 4094 +let case2Id const subGroup = { id: 4090, parent_gid: groupCase.id, name: 'sub.route.example.com' } const subZone = { ...nsCase, id: 4091, gid: subGroup.id, zone: 'sub.route.example.com.' } before(async () => { await Zone.destroy({ id: nsCase.id }) - await Zone.destroy({ id: case2Id }) await Zone.destroy({ id: subZone.id }) // Destroy the subgroup before recreating it: a lingering row would make // Group.create early-return and skip addToSubgroups, leaving the @@ -33,6 +32,7 @@ before(async () => { }) after(async () => { + if (case2Id) await Zone.destroy({ id: case2Id }) await Zone.destroy({ id: subZone.id }) await Group.destroy({ id: subGroup.id }) await server.stop() @@ -85,10 +85,10 @@ describe('zone routes', () => { assert.ok(res.result.zone.some((z) => z.zone === nsCase.zone)) }) - it(`POST /zone (${case2Id})`, async () => { + it('POST /zone allocates an id', async () => { const testCase = JSON.parse(JSON.stringify(nsCase)) - testCase.id = case2Id // make it unique - testCase.gid = case2Id + delete testCase.id + testCase.gid = groupCase.id testCase.zone = 'route2.example.com.' const res = await server.inject({ @@ -99,33 +99,36 @@ describe('zone routes', () => { }) // console.log(res.result) assert.equal(res.statusCode, 201) + assert.equal(res.result.zone.length, 1) + assert.equal(res.result.zone[0].zone, 'route2.example.com.') assert.ok(res.result.zone[0].gid) + case2Id = res.result.zone[0].id + assert.ok(Number.isInteger(case2Id)) }) - it(`GET /zone/${case2Id}`, async () => { + it('PUT /zone/{id} keeps using the route id', async () => { const res = await server.inject({ - method: 'GET', + method: 'PUT', url: `/zone/${case2Id}`, headers: auth.headers, + payload: { description: 'updated' }, }) - // console.log(res.result) assert.equal(res.statusCode, 200) - assert.ok(res.result.zone[0].gid) + assert.equal(res.result.zone[0].description, 'updated') }) - it(`PUT /zone/${case2Id}`, async () => { + it('GET /zone/{id}', async () => { const res = await server.inject({ - method: 'PUT', + method: 'GET', url: `/zone/${case2Id}`, headers: auth.headers, - payload: { description: 'edited by the route test' }, }) - + // console.log(res.result) assert.equal(res.statusCode, 200) - assert.equal((await Zone.get({ id: case2Id }))[0].description, 'edited by the route test') + assert.ok(res.result.zone[0].gid) }) - it(`DELETE /zone/${case2Id}`, async () => { + it('DELETE /zone/{id}', async () => { const res = await server.inject({ method: 'DELETE', url: `/zone/${case2Id}`, @@ -135,7 +138,7 @@ describe('zone routes', () => { assert.equal(res.statusCode, 200) }) - it(`DELETE /zone/${case2Id}`, async () => { + it('DELETE /zone/{id} returns 404 when already deleted', async () => { const res = await server.inject({ method: 'DELETE', url: `/zone/${case2Id}`, @@ -145,7 +148,7 @@ describe('zone routes', () => { assert.equal(res.statusCode, 404) }) - it(`GET /zone/${case2Id}`, async () => { + it('GET /zone/{id} hides a soft-deleted zone', async () => { const res = await server.inject({ method: 'GET', url: `/zone/${case2Id}`, @@ -156,7 +159,7 @@ describe('zone routes', () => { assert.deepEqual(res.result.zone, []) }) - it(`GET /zone/${case2Id} (deleted)`, async () => { + it('GET /zone/{id}?deleted=true returns a soft-deleted zone', async () => { const res = await server.inject({ method: 'GET', url: `/zone/${case2Id}?deleted=true`, diff --git a/routes/zone_record.test.js b/routes/zone_record.test.js index 9aba2e2..8fbba1a 100644 --- a/routes/zone_record.test.js +++ b/routes/zone_record.test.js @@ -134,6 +134,19 @@ describe('zone_record routes', () => { createdZoneRecordIds.push(res.result.zone_record[0].id) }) + it('PUT /zone_record/{id} keeps using the route id', async () => { + const id = createdZoneRecordIds[0] + const res = await server.inject({ + method: 'PUT', + url: `/zone_record/${id}`, + headers: auth.headers, + payload: { description: 'updated' }, + }) + + assert.equal(res.statusCode, 200) + assert.equal(res.result.zone_record[0].description, 'updated') + }) + it('POST /zone_record accepts omitted ttl and stores 0', async () => { const res = await server.inject({ method: 'POST', From b2d076daef07c8543e3e78956ad61048d68c0519 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Mon, 31 Aug 2026 15:29:51 +0100 Subject: [PATCH 02/12] file store: serialize id allocation --- .github/workflows/ci.yml | 2 +- lib/group/store/file.js | 54 ++++++++++++++--------------- lib/nameserver/store/file.js | 23 ++++++------- lib/session/store/file.js | 19 ++++++----- lib/store/file.js | 64 ++++++++++++++++++++++++++++++----- lib/store/file.test.js | 20 +++++++++++ lib/user/store/file.js | 37 ++++++++++---------- lib/zone/store/file.js | 23 ++++++------- lib/zone_record/store/file.js | 25 ++++++-------- routes/create_id.test.js | 9 ++--- routes/zone_record.test.js | 18 ---------- 11 files changed, 165 insertions(+), 129 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3e5de11..1ac73c3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,7 +10,7 @@ permissions: contents: read env: - NICTOOL_VALIDATE_SPEC: https://github.com/NicTool/validate/archive/c7d2776fd465b4118eb6d90449d1ecae42220dd5.tar.gz + NICTOOL_VALIDATE_SPEC: https://github.com/NicTool/validate/archive/45037764d5ef420be6536e66894da02f60527e5e.tar.gz jobs: lint: diff --git a/lib/group/store/file.js b/lib/group/store/file.js index 49418b9..29a6368 100644 --- a/lib/group/store/file.js +++ b/lib/group/store/file.js @@ -60,39 +60,35 @@ class GroupRepoFile extends GroupBase { async create(args, options) { args = JSON.parse(JSON.stringify(args)) - - if (args.id !== undefined) { - const existing = [ - ...(await this.get({ id: args.id })), - ...(await this.get({ id: args.id, deleted: true })), - ] - if (existing.length > 0) return idConflict('group', args.id, options) - } - const usable_ns = args.usable_ns ?? [] delete args.usable_ns - const groups = await this._load() - if (args.id === undefined) args.id = nextId(groups) - - const gid = args.id - args.permissions = { - ...JSON.parse(JSON.stringify(defaultPermissions)), - id: gid, - name: `Group ${args.name} perms`, - user: { id: gid, create: false, write: false, delete: false }, - group: { id: gid, create: false, write: false, delete: false }, - nameserver: { - usable: Array.isArray(usable_ns) ? usable_ns : [], - create: false, - write: false, - delete: false, - }, - } + return this.file.mutate('group', (groups, data) => { + if (args.id !== undefined && groups.some((group) => group.id === args.id)) { + return idConflict('group', args.id, options) + } - groups.push(args) - await this._save(groups) - return gid + if (args.id === undefined) args.id = nextId(groups, data.last_id) + data.last_id = Math.max(data.last_id ?? 0, args.id) + + const gid = args.id + args.permissions = { + ...JSON.parse(JSON.stringify(defaultPermissions)), + id: gid, + name: `Group ${args.name} perms`, + user: { id: gid, create: false, write: false, delete: false }, + group: { id: gid, create: false, write: false, delete: false }, + nameserver: { + usable: Array.isArray(usable_ns) ? usable_ns : [], + create: false, + write: false, + delete: false, + }, + } + + groups.push(args) + return gid + }) } async get(args_orig) { diff --git a/lib/nameserver/store/file.js b/lib/nameserver/store/file.js index a9d4ba2..744d601 100644 --- a/lib/nameserver/store/file.js +++ b/lib/nameserver/store/file.js @@ -42,19 +42,16 @@ class NameserverRepoFile extends NameserverBase { async create(args, options) { args = JSON.parse(JSON.stringify(args)) - if (args.id !== undefined) { - const existing = [ - ...(await this.get({ id: args.id })), - ...(await this.get({ id: args.id, deleted: true })), - ] - if (existing.length > 0) return idConflict('nameserver', args.id, options) - } - - const nameservers = await this._load() - if (args.id === undefined) args.id = nextId(nameservers) - nameservers.push(args) - await this._save(nameservers) - return args.id + return this.file.mutate('nameserver', (nameservers, data) => { + if (args.id !== undefined && nameservers.some((nameserver) => nameserver.id === args.id)) { + return idConflict('nameserver', args.id, options) + } + + if (args.id === undefined) args.id = nextId(nameservers, data.last_id, 0xffff) + data.last_id = Math.max(data.last_id ?? 0, args.id) + nameservers.push(args) + return args.id + }) } async get(args) { diff --git a/lib/session/store/file.js b/lib/session/store/file.js index cb5c15e..ebd15cc 100644 --- a/lib/session/store/file.js +++ b/lib/session/store/file.js @@ -33,14 +33,17 @@ class SessionRepoFile { async create(args) { args = normalizeArgs(JSON.parse(JSON.stringify(args))) - const existing = await this.get({ uid: args.uid, session: args.session }) - if (existing) return existing.id - - const sessions = await this._load() - args.id = nextId(sessions) - sessions.push(args) - await this._save(sessions) - return args.id + return this.file.mutate('session', (sessions, data) => { + const existing = sessions.find( + (session) => session.uid === args.uid && session.session === args.session, + ) + if (existing) return existing.id + + args.id = nextId(sessions, data.last_id) + data.last_id = args.id + sessions.push(args) + return args.id + }) } async get(args) { diff --git a/lib/store/file.js b/lib/store/file.js index cff052d..ba1ce62 100644 --- a/lib/store/file.js +++ b/lib/store/file.js @@ -19,13 +19,36 @@ const tomlCodec = { } const codecs = { json: jsonCodec, toml: tomlCodec } +const mutations = new Map() + +async function serialize(file, callback) { + const previous = mutations.get(file) ?? Promise.resolve() + const current = previous.catch(() => {}).then(callback) + mutations.set(file, current) + + try { + return await current + } finally { + if (mutations.get(file) === current) mutations.delete(file) + } +} export function resolveCodec(type = storeConfig().type) { return codecs[type] ?? jsonCodec } -export function nextId(rows) { - return rows.reduce((max, row) => Math.max(max, row.id ?? 0), 0) + 1 +export function nextId(rows, lastId = 0, maxId = 0xffffffff) { + if (!Number.isInteger(lastId) || lastId < 0) throw new TypeError('file store last id must be unsigned') + + let highest = lastId + for (const row of rows) { + if (row.id === undefined) continue + if (!Number.isInteger(row.id) || row.id < 0) throw new TypeError('file store row id must be unsigned') + highest = Math.max(highest, row.id) + } + + if (highest >= maxId) throw new RangeError(`file store id space exhausted at ${maxId}`) + return highest + 1 } /** @@ -57,20 +80,45 @@ export class FileStore { return path.join(base, `${this.basename}.${this.codec.ext}`) } - async load(key) { + async _read(file) { try { - const data = await this.codec.parse(await fs.readFile(this.filePath, 'utf8')) - return Array.isArray(data?.[key]) ? data[key] : [] + const data = await this.codec.parse(await fs.readFile(file, 'utf8')) + return data && typeof data === 'object' && !Array.isArray(data) ? data : {} } catch (err) { - if (err.code === 'ENOENT') return [] + if (err.code === 'ENOENT') return {} throw err } } + async _write(file, data) { + await fs.mkdir(path.dirname(file), { recursive: true }) + await fs.writeFile(file, await this.codec.stringify(data)) + } + + async load(key) { + const data = await this._read(this.filePath) + return Array.isArray(data[key]) ? data[key] : [] + } + async save(key, rows) { const file = this.filePath - await fs.mkdir(path.dirname(file), { recursive: true }) - await fs.writeFile(file, await this.codec.stringify({ [key]: rows })) + return serialize(file, async () => { + const data = await this._read(file) + data[key] = rows + await this._write(file, data) + }) + } + + async mutate(key, callback) { + const file = this.filePath + return serialize(file, async () => { + const data = await this._read(file) + const rows = Array.isArray(data[key]) ? data[key] : [] + const result = await callback(rows, data) + data[key] = rows + await this._write(file, data) + return result + }) } } diff --git a/lib/store/file.test.js b/lib/store/file.test.js index 33c003b..e58515e 100644 --- a/lib/store/file.test.js +++ b/lib/store/file.test.js @@ -4,6 +4,7 @@ import os from 'node:os' import path from 'node:path' import { describe, it, before, after, beforeEach } from 'node:test' +import GroupRepoFile from '../group/store/file.js' import FileStore, { nextId, resolveCodec } from './file.js' const envKeys = ['NICTOOL_DATA_STORE', 'NICTOOL_DATA_STORE_PATH', 'NICTOOL_CONF_DIR'] @@ -15,6 +16,8 @@ describe('file store', () => { it('allocates after the highest existing id', () => { assert.equal(nextId([]), 1) assert.equal(nextId([{ id: 9 }, { id: 2 }, {}]), 10) + assert.equal(nextId([{ id: 9 }], 12), 13) + assert.throws(() => nextId([{ id: 0xffffffff }]), /id space exhausted/) }) before(() => { @@ -108,4 +111,21 @@ describe('file store', () => { process.env.NICTOOL_DATA_STORE = 'nonsense' assert.equal(resolveCodec().ext, 'json') }) + + for (const type of ['json', 'toml']) { + it(`serializes ${type} id allocation and does not reuse a destroyed id`, async () => { + process.env.NICTOOL_DATA_STORE = type + const store = new GroupRepoFile() + const ids = await Promise.all( + Array.from({ length: 20 }, (_, index) => store.create({ name: `group-${index}.example` })), + ) + + assert.deepEqual( + [...ids].sort((a, b) => a - b), + Array.from({ length: 20 }, (_, index) => index + 1), + ) + await store.destroy({ id: 20 }) + assert.equal(await store.create({ name: 'after-destroy.example' }), 21) + }) + } }) diff --git a/lib/user/store/file.js b/lib/user/store/file.js index 87a8568..c2184a5 100644 --- a/lib/user/store/file.js +++ b/lib/user/store/file.js @@ -135,14 +135,6 @@ class UserRepoFile extends UserBase { } async create(args, options) { - if (args.id !== undefined) { - const existing = [ - ...(await this.get({ id: args.id })), - ...(await this.get({ id: args.id, deleted: true })), - ] - if (existing.length > 0) return idConflict('user', args.id, options) - } - args = JSON.parse(JSON.stringify(args)) const inherit = args.inherit_group_permissions @@ -152,21 +144,26 @@ class UserRepoFile extends UserBase { Object.assign(args, await Credentials.forStorage(args.password, args.pass_salt)) } - const users = await this._load() - if (args.id === undefined) args.id = nextId(users) + return this.file.mutate('user', (users, data) => { + if (args.id !== undefined && users.some((user) => user.id === args.id)) { + return idConflict('user', args.id, options) + } - if (inherit === false) { - args.permissions = { - ...JSON.parse(JSON.stringify(defaultPermissions)), - id: args.id, - user: { id: args.id, create: false, write: false, delete: false }, - group: { id: args.gid, create: false, write: false, delete: false }, + if (args.id === undefined) args.id = nextId(users, data.last_id) + data.last_id = Math.max(data.last_id ?? 0, args.id) + + if (inherit === false) { + args.permissions = { + ...JSON.parse(JSON.stringify(defaultPermissions)), + id: args.id, + user: { id: args.id, create: false, write: false, delete: false }, + group: { id: args.gid, create: false, write: false, delete: false }, + } } - } - users.push(args) - await this._save(users) - return args.id + users.push(args) + return args.id + }) } async put(args) { diff --git a/lib/zone/store/file.js b/lib/zone/store/file.js index e68d865..ea5424d 100644 --- a/lib/zone/store/file.js +++ b/lib/zone/store/file.js @@ -38,19 +38,16 @@ class ZoneRepoFile extends ZoneBase { async create(args, options) { args = JSON.parse(JSON.stringify(args)) - if (args.id !== undefined) { - const existing = [ - ...(await this.get({ id: args.id })), - ...(await this.get({ id: args.id, deleted: true })), - ] - if (existing.length > 0) return idConflict('zone', args.id, options) - } - - const zones = await this._load() - if (args.id === undefined) args.id = nextId(zones) - zones.push(args) - await this._save(zones) - return args.id + return this.file.mutate('zone', (zones, data) => { + if (args.id !== undefined && zones.some((zone) => zone.id === args.id)) { + return idConflict('zone', args.id, options) + } + + if (args.id === undefined) args.id = nextId(zones, data.last_id) + data.last_id = Math.max(data.last_id ?? 0, args.id) + zones.push(args) + return args.id + }) } async get(args) { diff --git a/lib/zone_record/store/file.js b/lib/zone_record/store/file.js index 1e83842..d8913ba 100644 --- a/lib/zone_record/store/file.js +++ b/lib/zone_record/store/file.js @@ -20,23 +20,18 @@ class ZoneRecordRepoFile extends ZoneRecordBase { async create(args, options) { args = JSON.parse(JSON.stringify(args)) - if (args.id !== undefined) { - const existing = [ - ...(await this.get({ id: args.id })), - ...(await this.get({ id: args.id, deleted: true })), - ] - if (existing.length > 0) return idConflict('zone record', args.id, options) - } - - const records = await this._load() + return this.file.mutate('zone_record', (records, data) => { + if (args.id !== undefined && records.some((record) => record.id === args.id)) { + return idConflict('zone record', args.id, options) + } - if (args.id === undefined) args.id = nextId(records) + if (args.id === undefined) args.id = nextId(records, data.last_id) + data.last_id = Math.max(data.last_id ?? 0, args.id) + if (args.ttl === undefined) args.ttl = 0 - if (args.ttl === undefined) args.ttl = 0 - - records.push(args) - await this._save(records) - return args.id + records.push(args) + return args.id + }) } async count(args = {}) { diff --git a/routes/create_id.test.js b/routes/create_id.test.js index 2fc8850..e00994f 100644 --- a/routes/create_id.test.js +++ b/routes/create_id.test.js @@ -56,15 +56,16 @@ const cases = [ ] before(async () => { + const fixture = { ifExists: 'return' } await ZoneRecord.destroy({ id: chosenId }) await Zone.destroy({ id: chosenId }) await User.destroy({ id: chosenId }) await Permission.destroy({ id: chosenId }) await Nameserver.destroy({ id: chosenId }) await Group.destroy({ id: chosenId }) - await Group.create(groupCase) - await User.create(userCase) - await Zone.create(zoneCase) + await Group.create(groupCase, fixture) + await User.create(userCase, fixture) + await Zone.create(zoneCase, fixture) server = await init() const res = await server.inject({ @@ -86,7 +87,7 @@ after(async () => { await Permission.destroy({ id: chosenId }) await Nameserver.destroy({ id: chosenId }) await Group.destroy({ id: chosenId }) - await server.stop() + if (server) await server.stop() }) describe('caller-supplied create ids', () => { diff --git a/routes/zone_record.test.js b/routes/zone_record.test.js index 8fbba1a..d94620f 100644 --- a/routes/zone_record.test.js +++ b/routes/zone_record.test.js @@ -93,24 +93,6 @@ describe('zone_record routes', () => { auth.headers = { Authorization: `Bearer ${res.result.session.token}` } }) - it('POST /zone_record returns 409 for an existing id', async () => { - const res = await server.inject({ - method: 'POST', - url: '/zone_record', - headers: auth.headers, - payload: { - ...testZoneRecord, - owner: 'changed.route-zr-delete.example.com.', - }, - }) - - assert.equal(res.statusCode, 409) - assert.equal(res.result.message, `zone record id ${testZoneRecordId} already exists`) - - const [existing] = await ZoneRecord.get({ id: testZoneRecordId }) - assert.equal(existing.owner, testZoneRecord.owner) - }) - it('POST /zone_record creates and returns array payload', async () => { const res = await server.inject({ method: 'POST', From d8d2b0f425cb823f63b64f4ebd73832b95f57475 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Mon, 31 Aug 2026 17:41:33 +0100 Subject: [PATCH 03/12] deps: use validate 1.0.1 --- .github/workflows/ci.yml | 9 --------- docker/Dockerfile | 6 +----- docker/docker-compose.yml | 2 -- package.json | 2 +- 4 files changed, 2 insertions(+), 17 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1ac73c3..f8c75d7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,9 +9,6 @@ on: permissions: contents: read -env: - NICTOOL_VALIDATE_SPEC: https://github.com/NicTool/validate/archive/45037764d5ef420be6536e66894da02f60527e5e.tar.gz - jobs: lint: uses: NicTool/.github/.github/workflows/lint.yml@main @@ -46,8 +43,6 @@ jobs: node-version: ${{ matrix.node-version }} - run: sh sql/init-mysql.sh - run: npm install - - name: Install proposed validate source - run: npm install --no-save ${{ env.NICTOOL_VALIDATE_SPEC }} - run: npm test test-mac: @@ -68,8 +63,6 @@ jobs: node-version: ${{ matrix.node-version }} - run: sh sql/init-mysql.sh - run: npm install - - name: Install proposed validate source - run: npm install --no-save ${{ env.NICTOOL_VALIDATE_SPEC }} - run: npm test test-docker: @@ -107,6 +100,4 @@ jobs: node-version: ${{ matrix.node-version }} - run: sh sql/init-mysql.sh - run: npm install - - name: Install proposed validate source - run: npm install --no-save ${{ env.NICTOOL_VALIDATE_SPEC }} - run: sh test/run.sh diff --git a/docker/Dockerfile b/docker/Dockerfile index 88437ff..0fea311 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -2,11 +2,7 @@ FROM node:22-trixie-slim WORKDIR /app COPY package*.json . # --omit=dev is safe: tests use node:test (stdlib), devDeps are only eslint/prettier -ARG NICTOOL_VALIDATE_SPEC -RUN npm install --omit=dev \ - && if [ -n "$NICTOOL_VALIDATE_SPEC" ]; then \ - npm install --omit=dev --no-save "$NICTOOL_VALIDATE_SPEC"; \ - fi +RUN npm install --omit=dev COPY . . EXPOSE 3000 CMD ["node", "server.js"] diff --git a/docker/docker-compose.yml b/docker/docker-compose.yml index 60e55dc..505c3aa 100644 --- a/docker/docker-compose.yml +++ b/docker/docker-compose.yml @@ -24,8 +24,6 @@ services: build: context: .. dockerfile: docker/Dockerfile - args: - NICTOOL_VALIDATE_SPEC: ${NICTOOL_VALIDATE_SPEC:-} ports: - '${API_PORT:-3000}:3000' depends_on: diff --git a/package.json b/package.json index 0406711..b7a766a 100644 --- a/package.json +++ b/package.json @@ -61,7 +61,7 @@ "@hapi/vision": "^7.0.3", "@msimerson/hapi-openapi": "^18.0.3", "@nictool/dns-resource-record": "^1.8.2", - "@nictool/validate": "^0.9.1", + "@nictool/validate": "^1.0.1", "joi": "^18.2.3", "mysql2": "^3.23.2", "qs": "^6.15.3", From ae3c4accad45f52c77dac1c34d528a2c9a076a7e Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Tue, 1 Sep 2026 14:22:08 +0100 Subject: [PATCH 04/12] test: serialize coverage by backend --- test/backends/json.sh | 6 ++---- test/backends/mysql.sh | 2 +- test/backends/toml.sh | 6 ++---- test/run.sh | 13 ++++++------- 4 files changed, 11 insertions(+), 16 deletions(-) diff --git a/test/backends/json.sh b/test/backends/json.sh index d96349e..81ed93c 100755 --- a/test/backends/json.sh +++ b/test/backends/json.sh @@ -17,8 +17,6 @@ test_files() { } run_tests() { - # Run serially: the file store uses shared files; parallel workers cause concurrent-write corruption - for f in $(test_files); do - $NODE --test --test-reporter=spec "$f" || exit 1 - done + # shellcheck disable=SC2046 # word splitting is how the file list is passed + $NODE --test --test-concurrency=1 "$@" $(test_files) } diff --git a/test/backends/mysql.sh b/test/backends/mysql.sh index fdcaf47..fe074c6 100755 --- a/test/backends/mysql.sh +++ b/test/backends/mysql.sh @@ -23,5 +23,5 @@ test_files() { run_tests() { # shellcheck disable=SC2046 # word splitting is how the file list is passed - $NODE --test --test-reporter=spec $(test_files) + $NODE --test --test-concurrency=1 "$@" $(test_files) } diff --git a/test/backends/toml.sh b/test/backends/toml.sh index 345cdf3..e09436e 100755 --- a/test/backends/toml.sh +++ b/test/backends/toml.sh @@ -17,8 +17,6 @@ test_files() { } run_tests() { - # Run serially: TOML uses shared files; parallel workers cause concurrent-write corruption - for f in $(test_files); do - $NODE --test --test-reporter=spec "$f" || exit 1 - done + # shellcheck disable=SC2046 # word splitting is how the file list is passed + $NODE --test --test-concurrency=1 "$@" $(test_files) } diff --git a/test/run.sh b/test/run.sh index 5c22c5d..48ae343 100755 --- a/test/run.sh +++ b/test/run.sh @@ -39,19 +39,18 @@ if [ $# -ge 1 ]; then if [ "$1" = "watch" ]; then $NODE --test --watch elif [ "$1" = "coverage" ]; then - # shellcheck disable=SC2046 # word splitting is how the file list is passed - $NODE --test --experimental-test-coverage $COVERAGE_EXCLUDE $(test_files) + # shellcheck disable=SC2086 # word splitting passes each exclusion separately + run_tests --experimental-test-coverage $COVERAGE_EXCLUDE elif [ "$1" = "coverage:lcov" ]; then mkdir -p coverage # lcov alone writes only to the file, so a failure exits 1 with an empty log - # shellcheck disable=SC2046 # word splitting is how the file list is passed - $NODE --test --experimental-test-coverage $COVERAGE_EXCLUDE \ + # shellcheck disable=SC2086 # word splitting passes each exclusion separately + run_tests --experimental-test-coverage $COVERAGE_EXCLUDE \ --test-reporter=lcov --test-reporter-destination=coverage/lcov.info \ - --test-reporter=spec --test-reporter-destination=stdout \ - $(test_files) + --test-reporter=spec --test-reporter-destination=stdout else $NODE --test --test-reporter=spec "$1" fi else - run_tests + run_tests --test-reporter=spec fi From d9c67f9b2da78c7d1f1d29e226604242615920bd Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Tue, 1 Sep 2026 14:22:12 +0100 Subject: [PATCH 05/12] cov: measure mysql and json stores --- .github/workflows/coverage.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index 695f13c..ca0aa54 100644 --- a/.github/workflows/coverage.yml +++ b/.github/workflows/coverage.yml @@ -14,3 +14,4 @@ jobs: contents: read with: mysql: true + data-stores: mysql,json From 60fb79734cb0729d6c8d25dc8fdf6c7850b0e844 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Wed, 14 Oct 2026 21:01:46 +0100 Subject: [PATCH 06/12] permission: create under the file lock A permission create loaded the user file, then saved it outside the per-file lock, so users created meanwhile vanished. --- lib/permission/store/file.js | 62 +++++++++++++++++------------------- lib/store/file.test.js | 23 +++++++++++++ 2 files changed, 52 insertions(+), 33 deletions(-) diff --git a/lib/permission/store/file.js b/lib/permission/store/file.js index 44fc215..5a4d98f 100644 --- a/lib/permission/store/file.js +++ b/lib/permission/store/file.js @@ -3,6 +3,8 @@ import { idConflict } from '../../store/error.js' import PermissionBase from './base.js' +const missing = Symbol('missing') + /** * File-backed permission store (JSON or TOML, per store.type). * @@ -108,57 +110,51 @@ class PermissionRepoFile extends PermissionBase { } if (uid !== undefined) { - const users = await this._loadUsers() - const idx = users.findIndex((u) => u.id === uid) - - if (idx !== -1) { - if (users[idx].permissions) return idConflict('permission', users[idx].permissions.id, options) + const created = await this.userFile.mutate('user', (users) => { + const user = users.find((u) => u.id === uid) + if (!user) return missing + if (user.permissions) return idConflict('permission', user.permissions.id, options) const perm = JSON.parse(JSON.stringify(args)) perm.id = args.id ?? uid if (!perm.user) perm.user = {} perm.user.id = uid if (!perm.group) perm.group = {} - perm.group.id = gid ?? users[idx].gid - users[idx].permissions = perm - await this._saveUsers(users) - return users[idx].permissions.id - } - - // User not found — fall through to standalone storage + perm.group.id = gid ?? user.gid + user.permissions = perm + return perm.id + }) + if (created !== missing) return created } if (gid !== undefined && uid === undefined) { - const groups = await this._loadGroups() - const idx = groups.findIndex((g) => g.id === gid) - - if (idx !== -1) { - if (groups[idx].permissions) return idConflict('permission', groups[idx].permissions.id, options) + const created = await this.groupFile.mutate('group', (groups) => { + const group = groups.find((g) => g.id === gid) + if (!group) return missing + if (group.permissions) return idConflict('permission', group.permissions.id, options) const perm = JSON.parse(JSON.stringify(args)) perm.id = args.id ?? gid if (!perm.group) perm.group = {} perm.group.id = gid - groups[idx].permissions = perm - await this._saveGroups(groups) - return groups[idx].permissions.id - } - - // Group not found — fall through to standalone storage + group.permissions = perm + return perm.id + }) + if (created !== missing) return created } // Standalone fallback: neither user nor group record found const permId = args.id ?? uid ?? gid if (permId === undefined) return undefined - const perms = await this._loadStandalone() - if (perms.some((permission) => permission.id === permId)) { - return idConflict('permission', permId, options) - } - const perm = { ...args, id: permId } - if (uid !== undefined) perm.uid = uid - if (gid !== undefined) perm.gid = gid - perms.push(perm) - await this._saveStandalone(perms) - return permId + return this.standaloneFile.mutate('permission', (perms) => { + if (perms.some((permission) => permission.id === permId)) { + return idConflict('permission', permId, options) + } + const perm = { ...args, id: permId } + if (uid !== undefined) perm.uid = uid + if (gid !== undefined) perm.gid = gid + perms.push(perm) + return permId + }) } async get(args) { diff --git a/lib/store/file.test.js b/lib/store/file.test.js index e58515e..a0a2a3d 100644 --- a/lib/store/file.test.js +++ b/lib/store/file.test.js @@ -5,6 +5,8 @@ import path from 'node:path' import { describe, it, before, after, beforeEach } from 'node:test' import GroupRepoFile from '../group/store/file.js' +import PermissionRepoFile from '../permission/store/file.js' +import UserRepoFile from '../user/store/file.js' import FileStore, { nextId, resolveCodec } from './file.js' const envKeys = ['NICTOOL_DATA_STORE', 'NICTOOL_DATA_STORE_PATH', 'NICTOOL_CONF_DIR'] @@ -127,5 +129,26 @@ describe('file store', () => { await store.destroy({ id: 20 }) assert.equal(await store.create({ name: 'after-destroy.example' }), 21) }) + + it(`keeps ${type} users created while a permission is added to a user`, async () => { + process.env.NICTOOL_DATA_STORE = type + fs.rmSync(path.join(tmp, `user.${type}`), { force: true }) + const users = new UserRepoFile() + const uid = await users.create({ gid: 1, username: 'holder', email: 'holder@example.com' }) + + const [permId, ...created] = await Promise.all([ + new PermissionRepoFile().create({ uid, gid: 1, zone: { create: true } }), + ...Array.from({ length: 5 }, (_, index) => + users.create({ gid: 1, username: `racer-${index}`, email: `racer-${index}@example.com` }), + ), + ]) + + const stored = await new FileStore('user').load('user') + assert.deepEqual( + stored.map((user) => user.id).sort((a, b) => a - b), + [uid, ...created].sort((a, b) => a - b), + ) + assert.equal(stored.find((user) => user.id === uid).permissions.id, permId) + }) } }) From da3fb1130d01745437cf45d6e6372eaea01e2f18 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Wed, 14 Oct 2026 21:04:29 +0100 Subject: [PATCH 07/12] user: upgrade a login's hash under the file lock authenticate() saved the user list it loaded before hashing, so users created during a login's hash upgrade vanished. --- lib/store/file.test.js | 28 ++++++++++++++++++++++++++++ lib/user/store/file.js | 7 +++++-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/lib/store/file.test.js b/lib/store/file.test.js index a0a2a3d..b5dca6a 100644 --- a/lib/store/file.test.js +++ b/lib/store/file.test.js @@ -6,6 +6,7 @@ import { describe, it, before, after, beforeEach } from 'node:test' import GroupRepoFile from '../group/store/file.js' import PermissionRepoFile from '../permission/store/file.js' +import Credentials from '../user/credentials.js' import UserRepoFile from '../user/store/file.js' import FileStore, { nextId, resolveCodec } from './file.js' @@ -150,5 +151,32 @@ describe('file store', () => { ) assert.equal(stored.find((user) => user.id === uid).permissions.id, permId) }) + + it(`keeps ${type} users created while a login upgrades its password hash`, async (t) => { + process.env.NICTOOL_DATA_STORE = type + await new FileStore('group').save('group', [{ id: 1, name: 'NicTool' }]) + await new FileStore('user').save('user', [{ id: 1, gid: 1, username: 'legacy', password: 'plain' }]) + const users = new UserRepoFile() + const forStorage = Credentials.forStorage.bind(Credentials) + const created = [] + t.mock.method(Credentials, 'forStorage', async (pass) => { + for (let index = 0; index < 5; index++) { + created.push( + await users.create({ gid: 1, username: `racer-${index}`, email: `racer-${index}@example.com` }), + ) + } + return forStorage(pass) + }) + + const login = await users.authenticate({ username: 'legacy', password: 'plain' }) + + assert.equal(login.user.id, 1) + const stored = await new FileStore('user').load('user') + assert.deepEqual( + stored.map((user) => user.id).sort((a, b) => a - b), + [1, ...created].sort((a, b) => a - b), + ) + assert.ok(stored.find((user) => user.id === 1).pass_salt, 'the hash was upgraded') + }) } }) diff --git a/lib/user/store/file.js b/lib/user/store/file.js index c2184a5..7da8d42 100644 --- a/lib/user/store/file.js +++ b/lib/user/store/file.js @@ -75,8 +75,11 @@ class UserRepoFile extends UserBase { // the user their login if (needsUpgrade) { try { - Object.assign(u, await Credentials.forStorage(authTry.password)) - await this._save(users) + const upgraded = await Credentials.forStorage(authTry.password) + await this.file.mutate('user', (current) => { + const row = current.find((c) => c.id === u.id) + if (row?.password === u.password && row.pass_salt === u.pass_salt) Object.assign(row, upgraded) + }) } catch (err) { console.warn(`password hash upgrade failed for user ${authTry.username}`, err) } From 63c57d0db3ead33b3b5f1a1ea9a961953471b2a1 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Wed, 14 Oct 2026 21:05:54 +0100 Subject: [PATCH 08/12] test: force the permission create race --- lib/store/file.test.js | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/lib/store/file.test.js b/lib/store/file.test.js index b5dca6a..525255e 100644 --- a/lib/store/file.test.js +++ b/lib/store/file.test.js @@ -131,18 +131,30 @@ describe('file store', () => { assert.equal(await store.create({ name: 'after-destroy.example' }), 21) }) - it(`keeps ${type} users created while a permission is added to a user`, async () => { + it(`keeps ${type} users created while a permission is added to a user`, async (t) => { process.env.NICTOOL_DATA_STORE = type fs.rmSync(path.join(tmp, `user.${type}`), { force: true }) const users = new UserRepoFile() const uid = await users.create({ gid: 1, username: 'holder', email: 'holder@example.com' }) - const [permId, ...created] = await Promise.all([ - new PermissionRepoFile().create({ uid, gid: 1, zone: { create: true } }), - ...Array.from({ length: 5 }, (_, index) => - users.create({ gid: 1, username: `racer-${index}`, email: `racer-${index}@example.com` }), - ), - ]) + // the users are created once the permission create has read the user file + const read = FileStore.prototype._read + const racers = [] + t.mock.method(FileStore.prototype, '_read', async function (file) { + const data = await read.call(this, file) + if (racers.length === 0 && file.endsWith(`user.${type}`)) { + for (let index = 0; index < 5; index++) { + racers.push( + users.create({ gid: 1, username: `racer-${index}`, email: `racer-${index}@example.com` }), + ) + } + } + return data + }) + + const permId = await new PermissionRepoFile().create({ uid, gid: 1, zone: { create: true } }) + const created = await Promise.all(racers) + t.mock.restoreAll() const stored = await new FileStore('user').load('user') assert.deepEqual( From 4f766548db234599713dcca9aa0751f8900643e1 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Wed, 14 Oct 2026 21:44:57 +0100 Subject: [PATCH 09/12] store: change file-store rows under the file lock put, delete and destroy saved a list they had loaded earlier, so rows created in between vanished. They now change rows inside mutate(), which skips the write when the callback returns false. Nothing writes outside the lock, so save() and the _save helpers are gone. A file without last_id now records its highest id before a change, so destroying that row can't free its id for reuse. A permission with no user or group fails on the file stores instead of storing nothing: inline permission ids are user and group ids, so there is no free id. --- lib/group/store/file.js | 53 ++++++------- lib/nameserver/store/file.js | 42 +++++------ lib/permission/store/file.js | 135 +++++++++------------------------- lib/session/store/file.js | 55 ++++++-------- lib/store/file.js | 33 ++++++--- lib/store/file.test.js | 80 +++++++++++++++++--- lib/user/store/file.js | 69 ++++++++--------- lib/zone/store/file.js | 39 ++++------ lib/zone/store/file.test.js | 7 +- lib/zone_record/store/file.js | 39 ++++------ test/seed.js | 7 ++ 11 files changed, 259 insertions(+), 300 deletions(-) create mode 100644 test/seed.js diff --git a/lib/group/store/file.js b/lib/group/store/file.js index 29a6368..f3411ac 100644 --- a/lib/group/store/file.js +++ b/lib/group/store/file.js @@ -1,4 +1,4 @@ -import FileStore, { nextId } from '../../store/file.js' +import FileStore, { nextId, removeRows } from '../../store/file.js' import { idConflict } from '../../store/error.js' import GroupBase from './base.js' @@ -23,10 +23,6 @@ class GroupRepoFile extends GroupBase { return this.file.load('group') } - async _save(groups) { - return this.file.save('group', groups) - } - _postProcess(row, deletedArg) { const r = JSON.parse(JSON.stringify(row)) r.deleted = Boolean(r.deleted) @@ -127,42 +123,35 @@ class GroupRepoFile extends GroupBase { const usable_ns = args.usable_ns delete args.usable_ns - const groups = await this._load() - const idx = groups.findIndex((g) => g.id === id) - if (idx === -1) return false + return this.file.mutate('group', (groups) => { + const idx = groups.findIndex((g) => g.id === id) + if (idx === -1) return false - if (usable_ns !== undefined && groups[idx].permissions) { - groups[idx].permissions.nameserver = { - ...groups[idx].permissions.nameserver, - usable: Array.isArray(usable_ns) ? usable_ns : [], + if (usable_ns !== undefined && groups[idx].permissions) { + groups[idx].permissions.nameserver = { + ...groups[idx].permissions.nameserver, + usable: Array.isArray(usable_ns) ? usable_ns : [], + } } - } - - if (Object.keys(args).length > 0) { - groups[idx] = { ...groups[idx], ...args } - } - await this._save(groups) - return true + if (Object.keys(args).length > 0) { + groups[idx] = { ...groups[idx], ...args } + } + return true + }) } async delete(args) { - const groups = await this._load() - const idx = groups.findIndex((g) => g.id === args.id) - if (idx === -1) return false - - groups[idx].deleted = args.deleted ?? true - await this._save(groups) - return true + return this.file.mutate('group', (groups) => { + const row = groups.find((g) => g.id === args.id) + if (!row) return false + row.deleted = args.deleted ?? true + return true + }) } async destroy(args) { - const groups = await this._load() - const before = groups.length - const filtered = groups.filter((g) => g.id !== args.id) - if (filtered.length === before) return false - await this._save(filtered) - return true + return this.file.mutate('group', (groups) => removeRows(groups, (g) => g.id === args.id) > 0) } } diff --git a/lib/nameserver/store/file.js b/lib/nameserver/store/file.js index 744d601..4f4e32e 100644 --- a/lib/nameserver/store/file.js +++ b/lib/nameserver/store/file.js @@ -1,4 +1,4 @@ -import FileStore, { nextId } from '../../store/file.js' +import FileStore, { nextId, removeRows } from '../../store/file.js' import { idConflict } from '../../store/error.js' import NameserverBase from './base.js' @@ -18,10 +18,6 @@ class NameserverRepoFile extends NameserverBase { return this.file.load('nameserver') } - async _save(nameservers) { - return this.file.save('nameserver', nameservers) - } - _postProcess(row, deletedArg) { const r = JSON.parse(JSON.stringify(row)) @@ -72,32 +68,28 @@ class NameserverRepoFile extends NameserverBase { async put(args) { if (!args.id) return false - const nameservers = await this._load() - const idx = nameservers.findIndex((n) => n.id === args.id) - if (idx === -1) return false - - nameservers[idx] = { ...nameservers[idx], ...JSON.parse(JSON.stringify(args)) } - await this._save(nameservers) - return true + return this.file.mutate('nameserver', (nameservers) => { + const idx = nameservers.findIndex((n) => n.id === args.id) + if (idx === -1) return false + nameservers[idx] = { ...nameservers[idx], ...JSON.parse(JSON.stringify(args)) } + return true + }) } async delete(args) { - const nameservers = await this._load() - const idx = nameservers.findIndex((n) => n.id === args.id) - if (idx === -1) return false - - nameservers[idx].deleted = args.deleted ?? true - await this._save(nameservers) - return true + return this.file.mutate('nameserver', (nameservers) => { + const row = nameservers.find((n) => n.id === args.id) + if (!row) return false + row.deleted = args.deleted ?? true + return true + }) } async destroy(args) { - const nameservers = await this._load() - const before = nameservers.length - const filtered = nameservers.filter((n) => n.id !== args.id) - if (filtered.length === before) return false - await this._save(filtered) - return true + return this.file.mutate( + 'nameserver', + (nameservers) => removeRows(nameservers, (n) => n.id === args.id) > 0, + ) } } diff --git a/lib/permission/store/file.js b/lib/permission/store/file.js index 5a4d98f..ac45cfb 100644 --- a/lib/permission/store/file.js +++ b/lib/permission/store/file.js @@ -3,8 +3,6 @@ import { idConflict } from '../../store/error.js' import PermissionBase from './base.js' -const missing = Symbol('missing') - /** * File-backed permission store (JSON or TOML, per store.type). * @@ -36,26 +34,14 @@ class PermissionRepoFile extends PermissionBase { return this.userFile.load('user') } - async _saveUsers(users) { - return this.userFile.save('user', users) - } - async _loadGroups() { return this.groupFile.load('group') } - async _saveGroups(groups) { - return this.groupFile.save('group', groups) - } - async _loadStandalone() { return this.standaloneFile.load('permission') } - async _saveStandalone(permissions) { - return this.standaloneFile.save('permission', permissions) - } - // --------------------------------------------------------------------------- // Post-processing // --------------------------------------------------------------------------- @@ -112,7 +98,7 @@ class PermissionRepoFile extends PermissionBase { if (uid !== undefined) { const created = await this.userFile.mutate('user', (users) => { const user = users.find((u) => u.id === uid) - if (!user) return missing + if (!user) return false if (user.permissions) return idConflict('permission', user.permissions.id, options) const perm = JSON.parse(JSON.stringify(args)) perm.id = args.id ?? uid @@ -123,13 +109,13 @@ class PermissionRepoFile extends PermissionBase { user.permissions = perm return perm.id }) - if (created !== missing) return created + if (created !== false) return created } if (gid !== undefined && uid === undefined) { const created = await this.groupFile.mutate('group', (groups) => { const group = groups.find((g) => g.id === gid) - if (!group) return missing + if (!group) return false if (group.permissions) return idConflict('permission', group.permissions.id, options) const perm = JSON.parse(JSON.stringify(args)) perm.id = args.id ?? gid @@ -138,12 +124,13 @@ class PermissionRepoFile extends PermissionBase { group.permissions = perm return perm.id }) - if (created !== missing) return created + if (created !== false) return created } // Standalone fallback: neither user nor group record found const permId = args.id ?? uid ?? gid - if (permId === undefined) return undefined + // inline permission ids are user and group ids, so there is no free id to give + if (permId === undefined) throw new Error('a file-store permission needs a user or group') return this.standaloneFile.mutate('permission', (perms) => { if (perms.some((permission) => permission.id === permId)) { @@ -221,70 +208,41 @@ class PermissionRepoFile extends PermissionBase { return this._postProcess(group.permissions, deletedArg) } + // Runs change(holder, key) on the permission with this id, under the lock + // of whichever file holds it. Returns false when no file does. + async _change(id, change) { + const inline = (rows) => { + const row = rows.find((r) => r.permissions?.id === id) + if (!row) return false + change(row, 'permissions') + return true + } + if (await this.userFile.mutate('user', inline)) return true + if (await this.groupFile.mutate('group', inline)) return true + return this.standaloneFile.mutate('permission', (perms) => { + const idx = perms.findIndex((p) => p.id === id) + if (idx === -1) return false + change(perms, idx) + return true + }) + } + async put(args) { args = JSON.parse(JSON.stringify(args)) if (!args.id) return false const id = args.id delete args.id - - const users = await this._loadUsers() - const uidx = users.findIndex((u) => u.permissions?.id === id) - if (uidx !== -1) { - users[uidx].permissions = deepMerge(users[uidx].permissions, args) - await this._saveUsers(users) - return true - } - - const groups = await this._loadGroups() - const gidx = groups.findIndex((g) => g.permissions?.id === id) - if (gidx !== -1) { - groups[gidx].permissions = deepMerge(groups[gidx].permissions, args) - await this._saveGroups(groups) - return true - } - - // Check standalone - const perms = await this._loadStandalone() - const pidx = perms.findIndex((p) => p.id === id) - if (pidx !== -1) { - perms[pidx] = deepMerge(perms[pidx], args) - await this._saveStandalone(perms) - return true - } - - return false + return this._change(id, (holder, key) => { + holder[key] = deepMerge(holder[key], args) + }) } async delete(args) { if (!args.id) return false const deletedVal = args.deleted ?? true - - const users = await this._loadUsers() - const uidx = users.findIndex((u) => u.permissions?.id === args.id) - if (uidx !== -1) { - users[uidx].permissions.deleted = deletedVal - await this._saveUsers(users) - return true - } - - const groups = await this._loadGroups() - const gidx = groups.findIndex((g) => g.permissions?.id === args.id) - if (gidx !== -1) { - groups[gidx].permissions.deleted = deletedVal - await this._saveGroups(groups) - return true - } - - // Check standalone - const perms = await this._loadStandalone() - const pidx = perms.findIndex((p) => p.id === args.id) - if (pidx !== -1) { - perms[pidx].deleted = deletedVal - await this._saveStandalone(perms) - return true - } - - return false + return this._change(args.id, (holder, key) => { + holder[key].deleted = deletedVal + }) } disconnect() { @@ -293,33 +251,10 @@ class PermissionRepoFile extends PermissionBase { async destroy(args) { if (!args.id) return false - - const users = await this._loadUsers() - const uidx = users.findIndex((u) => u.permissions?.id === args.id) - if (uidx !== -1) { - delete users[uidx].permissions - await this._saveUsers(users) - return true - } - - const groups = await this._loadGroups() - const gidx = groups.findIndex((g) => g.permissions?.id === args.id) - if (gidx !== -1) { - delete groups[gidx].permissions - await this._saveGroups(groups) - return true - } - - // Check standalone - const perms = await this._loadStandalone() - const before = perms.length - const filtered = perms.filter((p) => p.id !== args.id) - if (filtered.length < before) { - await this._saveStandalone(filtered) - return true - } - - return false + return this._change(args.id, (holder, key) => { + if (Array.isArray(holder)) holder.splice(key, 1) + else delete holder[key] + }) } } diff --git a/lib/session/store/file.js b/lib/session/store/file.js index ebd15cc..4e97c07 100644 --- a/lib/session/store/file.js +++ b/lib/session/store/file.js @@ -1,4 +1,4 @@ -import FileStore, { nextId } from '../../store/file.js' +import FileStore, { nextId, removeRows } from '../../store/file.js' // Map legacy nt_* column names to the friendly API names used throughout. function normalizeArgs(args) { @@ -26,10 +26,6 @@ class SessionRepoFile { return this.file.load('session') } - async _save(sessions) { - return this.file.save('session', sessions) - } - async create(args) { args = normalizeArgs(JSON.parse(JSON.stringify(args))) @@ -70,22 +66,22 @@ class SessionRepoFile { const now = parseInt(Date.now() / 1000, 10) if (s.last_access > now - 60) return true - const sessions = await this._load() - const idx = sessions.findIndex((s) => s.id === args.id) - if (idx === -1) return false - sessions[idx].last_access = now - await this._save(sessions) - return true + return this.file.mutate('session', (sessions) => { + const row = sessions.find((s) => s.id === args.id) + if (!row) return false + row.last_access = now + return true + }) } - const sessions = await this._load() - const idx = sessions.findIndex((s) => s.id === args.id) - if (idx === -1) return false const id = args.id delete args.id - sessions[idx] = { ...sessions[idx], ...args, id } - await this._save(sessions) - return true + return this.file.mutate('session', (sessions) => { + const idx = sessions.findIndex((s) => s.id === id) + if (idx === -1) return false + sessions[idx] = { ...sessions[idx], ...args, id } + return true + }) } /** @@ -95,20 +91,17 @@ class SessionRepoFile { async delete(args) { args = normalizeArgs(JSON.parse(JSON.stringify(args))) - const sessions = await this._load() - const before = sessions.length - - const filtered = sessions.filter((s) => { - // Keep this session unless every provided filter matches it - if (args.id !== undefined && s.id !== args.id) return true - if (args.uid !== undefined && s.uid !== args.uid) return true - if (args.session !== undefined && s.session !== args.session) return true - return false // all conditions matched → remove - }) - - if (filtered.length === before) return false - await this._save(filtered) - return true + return this.file.mutate( + 'session', + (sessions) => + removeRows(sessions, (s) => { + // Remove this session only when every provided filter matches it + if (args.id !== undefined && s.id !== args.id) return false + if (args.uid !== undefined && s.uid !== args.uid) return false + if (args.session !== undefined && s.session !== args.session) return false + return true + }) > 0, + ) } disconnect() { diff --git a/lib/store/file.js b/lib/store/file.js index ba1ce62..88f3a09 100644 --- a/lib/store/file.js +++ b/lib/store/file.js @@ -100,26 +100,37 @@ export class FileStore { return Array.isArray(data[key]) ? data[key] : [] } - async save(key, rows) { - const file = this.filePath - return serialize(file, async () => { - const data = await this._read(file) - data[key] = rows - await this._write(file, data) - }) - } - async mutate(key, callback) { const file = this.filePath return serialize(file, async () => { const data = await this._read(file) const rows = Array.isArray(data[key]) ? data[key] : [] - const result = await callback(rows, data) data[key] = rows - await this._write(file, data) + // a file from before last_id existed mustn't reuse a destroyed id + if (data.last_id === undefined) { + const highest = rows.reduce( + (max, row) => (Number.isInteger(row.id) && row.id > max ? row.id : max), + 0, + ) + if (highest > 0) data.last_id = highest + } + const result = await callback(rows, data) + // false means the callback changed nothing + if (result !== false) await this._write(file, data) return result }) } } +// in place, so a mutate() callback can drop rows +export function removeRows(rows, match) { + let kept = 0 + for (const row of rows) { + if (!match(row)) rows[kept++] = row + } + const removed = rows.length - kept + rows.length = kept + return removed +} + export default FileStore diff --git a/lib/store/file.test.js b/lib/store/file.test.js index 525255e..8fcb7e3 100644 --- a/lib/store/file.test.js +++ b/lib/store/file.test.js @@ -9,6 +9,7 @@ import PermissionRepoFile from '../permission/store/file.js' import Credentials from '../user/credentials.js' import UserRepoFile from '../user/store/file.js' import FileStore, { nextId, resolveCodec } from './file.js' +import { seed } from '../../test/seed.js' const envKeys = ['NICTOOL_DATA_STORE', 'NICTOOL_DATA_STORE_PATH', 'NICTOOL_CONF_DIR'] @@ -49,11 +50,11 @@ describe('file store', () => { it(`writes .json for a json store and .toml for a toml store`, async () => { process.env.NICTOOL_DATA_STORE = 'json' - await new FileStore('zone').save('zone', [{ id: 1 }]) + await seed(new FileStore('zone'), 'zone', [{ id: 1 }]) assert.equal(fs.existsSync(path.join(tmp, 'zone.json')), true) process.env.NICTOOL_DATA_STORE = 'toml' - await new FileStore('zone').save('zone', [{ id: 1 }]) + await seed(new FileStore('zone'), 'zone', [{ id: 1 }]) assert.equal(fs.existsSync(path.join(tmp, 'zone.toml')), true) }) @@ -61,7 +62,7 @@ describe('file store', () => { process.env.NICTOOL_DATA_STORE = 'json' const store = new FileStore('rt') - await store.save('rt', [{ id: 1, zone: 'example.com', deleted: false }]) + await seed(store, 'rt', [{ id: 1, zone: 'example.com', deleted: false }]) assert.deepEqual(await store.load('rt'), [{ id: 1, zone: 'example.com', deleted: false }]) }) @@ -69,14 +70,14 @@ describe('file store', () => { it(`preserves null through json, which TOML cannot`, async () => { process.env.NICTOOL_DATA_STORE = 'json' const jsonStore = new FileStore('nulls') - await jsonStore.save('nulls', [{ id: 1, last_publish: null }]) + await seed(jsonStore, 'nulls', [{ id: 1, last_publish: null }]) const [jsonRow] = await jsonStore.load('nulls') assert.equal(jsonRow.last_publish, null, 'json keeps the null') process.env.NICTOOL_DATA_STORE = 'toml' const tomlStore = new FileStore('nulls') - await tomlStore.save('nulls', [{ id: 1, last_publish: null }]) + await seed(tomlStore, 'nulls', [{ id: 1, last_publish: null }]) const [tomlRow] = await tomlStore.load('nulls') assert.equal(tomlRow.last_publish, undefined, 'toml drops it — hence the repair in the zone store') @@ -94,11 +95,11 @@ describe('file store', () => { assert.deepEqual(await new FileStore('odd').load('odd'), []) }) - it(`creates missing parent directories on save`, async () => { + it(`creates missing parent directories on write`, async () => { process.env.NICTOOL_DATA_STORE = 'json' process.env.NICTOOL_DATA_STORE_PATH = path.join(tmp, 'deep', 'nested') - await new FileStore('zone').save('zone', [{ id: 1 }]) + await seed(new FileStore('zone'), 'zone', [{ id: 1 }]) assert.equal(fs.existsSync(path.join(tmp, 'deep', 'nested', 'zone.json')), true) }) @@ -164,10 +165,71 @@ describe('file store', () => { assert.equal(stored.find((user) => user.id === uid).permissions.id, permId) }) + for (const [label, change] of [ + ['put', (id) => new GroupRepoFile().put({ id, name: 'renamed.example' })], + ['delete', (id) => new GroupRepoFile().delete({ id })], + ['destroy', (id) => new GroupRepoFile().destroy({ id })], + ['permission put', (id) => new PermissionRepoFile().put({ id, zone: { create: true } })], + ]) { + it(`keeps ${type} groups created during a group ${label}`, async (t) => { + process.env.NICTOOL_DATA_STORE = type + for (const name of ['group', 'user', 'permission']) { + fs.rmSync(path.join(tmp, `${name}.${type}`), { force: true }) + } + const groups = new GroupRepoFile() + const gid = await groups.create({ name: 'target.example' }) + + // the groups are created once the change has read the group file + const read = FileStore.prototype._read + const racers = [] + t.mock.method(FileStore.prototype, '_read', async function (file) { + const data = await read.call(this, file) + if (racers.length === 0 && file.endsWith(`group.${type}`)) { + for (let index = 0; index < 5; index++) { + racers.push(groups.create({ name: `racer-${index}.example` })) + } + } + return data + }) + + assert.equal(await change(gid), true) + assert.equal(racers.length, 5) + const created = await Promise.all(racers) + t.mock.restoreAll() + + const stored = (await new FileStore('group').load('group')).map((group) => group.id) + for (const id of created) assert.ok(stored.includes(id), `group ${id} survives`) + }) + } + + it(`does not reuse a ${type} id destroyed before last_id was recorded`, async () => { + process.env.NICTOOL_DATA_STORE = type + fs.rmSync(path.join(tmp, `group.${type}`), { force: true }) + await seed(new FileStore('group'), 'group', [ + { id: 19, name: 'nineteen.example' }, + { id: 20, name: 'twenty.example' }, + ]) + const groups = new GroupRepoFile() + await groups.destroy({ id: 20 }) + assert.equal(await groups.create({ name: 'next.example' }), 21) + }) + + it(`refuses a ${type} permission with no user or group`, async () => { + process.env.NICTOOL_DATA_STORE = type + await assert.rejects(new PermissionRepoFile().create({ name: 'no target' }), /needs a user or group/) + }) + + it(`does not write ${type} when a change finds nothing`, async () => { + process.env.NICTOOL_DATA_STORE = type + fs.rmSync(path.join(tmp, `zone.${type}`), { force: true }) + assert.equal(await new FileStore('zone').mutate('zone', () => false), false) + assert.equal(fs.existsSync(path.join(tmp, `zone.${type}`)), false) + }) + it(`keeps ${type} users created while a login upgrades its password hash`, async (t) => { process.env.NICTOOL_DATA_STORE = type - await new FileStore('group').save('group', [{ id: 1, name: 'NicTool' }]) - await new FileStore('user').save('user', [{ id: 1, gid: 1, username: 'legacy', password: 'plain' }]) + await seed(new FileStore('group'), 'group', [{ id: 1, name: 'NicTool' }]) + await seed(new FileStore('user'), 'user', [{ id: 1, gid: 1, username: 'legacy', password: 'plain' }]) const users = new UserRepoFile() const forStorage = Credentials.forStorage.bind(Credentials) const created = [] diff --git a/lib/user/store/file.js b/lib/user/store/file.js index 7da8d42..414d66e 100644 --- a/lib/user/store/file.js +++ b/lib/user/store/file.js @@ -1,4 +1,4 @@ -import FileStore, { nextId } from '../../store/file.js' +import FileStore, { nextId, removeRows } from '../../store/file.js' import { idConflict } from '../../store/error.js' import Config from '../../config.js' import Credentials from '../credentials.js' @@ -35,10 +35,6 @@ class UserRepoFile extends UserBase { return this.file.load('user') } - async _save(users) { - return this.file.save('user', users) - } - _postProcess(u, deletedArg) { const r = { ...u } // Remove sensitive credential fields — these are stored internally but never @@ -78,7 +74,8 @@ class UserRepoFile extends UserBase { const upgraded = await Credentials.forStorage(authTry.password) await this.file.mutate('user', (current) => { const row = current.find((c) => c.id === u.id) - if (row?.password === u.password && row.pass_salt === u.pass_salt) Object.assign(row, upgraded) + if (row?.password !== u.password || row.pass_salt !== u.pass_salt) return false + Object.assign(row, upgraded) }) } catch (err) { console.warn(`password hash upgrade failed for user ${authTry.username}`, err) @@ -173,50 +170,44 @@ class UserRepoFile extends UserBase { if (!args.id) return false args = JSON.parse(JSON.stringify(args)) - const users = await this._load() - const idx = users.findIndex((u) => u.id === args.id) - if (idx === -1) return false - const inherit = args.inherit_group_permissions delete args.inherit_group_permissions - if (inherit === true) { - // Switch to inherited: remove explicit permissions - delete users[idx].permissions - } else if (inherit === false && !users[idx].permissions) { - // Switch to explicit: create default permission entry - users[idx].permissions = { - ...JSON.parse(JSON.stringify(defaultPermissions)), - id: users[idx].id, - user: { id: users[idx].id, create: false, write: false, delete: false }, - group: { id: users[idx].gid, create: false, write: false, delete: false }, + return this.file.mutate('user', (users) => { + const idx = users.findIndex((u) => u.id === args.id) + if (idx === -1) return false + + if (inherit === true) { + // Switch to inherited: remove explicit permissions + delete users[idx].permissions + } else if (inherit === false && !users[idx].permissions) { + // Switch to explicit: create default permission entry + users[idx].permissions = { + ...JSON.parse(JSON.stringify(defaultPermissions)), + id: users[idx].id, + user: { id: users[idx].id, create: false, write: false, delete: false }, + group: { id: users[idx].gid, create: false, write: false, delete: false }, + } + } else if (inherit === false && users[idx].permissions) { + users[idx].permissions.inherit = false } - } else if (inherit === false && users[idx].permissions) { - users[idx].permissions.inherit = false - } - users[idx] = { ...users[idx], ...args } - await this._save(users) - return true + users[idx] = { ...users[idx], ...args } + return true + }) } async delete(args) { - const users = await this._load() - const idx = users.findIndex((u) => u.id === args.id) - if (idx === -1) return false - - users[idx].deleted = args.deleted ?? true - await this._save(users) - return true + return this.file.mutate('user', (users) => { + const row = users.find((u) => u.id === args.id) + if (!row) return false + row.deleted = args.deleted ?? true + return true + }) } async destroy(args) { - const users = await this._load() - const before = users.length - const filtered = users.filter((u) => u.id !== args.id) - if (filtered.length === before) return false - await this._save(filtered) - return true + return this.file.mutate('user', (users) => removeRows(users, (u) => u.id === args.id) > 0) } } diff --git a/lib/zone/store/file.js b/lib/zone/store/file.js index ea5424d..cd073c8 100644 --- a/lib/zone/store/file.js +++ b/lib/zone/store/file.js @@ -1,4 +1,4 @@ -import FileStore, { nextId, resolveCodec } from '../../store/file.js' +import FileStore, { nextId, removeRows, resolveCodec } from '../../store/file.js' import { idConflict } from '../../store/error.js' import ZoneBase from './base.js' @@ -15,10 +15,6 @@ class ZoneRepoFile extends ZoneBase { return this.file.load('zone') } - async _save(zones) { - return this.file.save('zone', zones) - } - _postProcess(row, deletedArg) { const r = { ...row } r.deleted = Boolean(r.deleted) @@ -141,32 +137,25 @@ class ZoneRepoFile extends ZoneBase { async put(args) { if (!args.id) return false - const zones = await this._load() - const idx = zones.findIndex((z) => z.id === args.id) - if (idx === -1) return false - - zones[idx] = { ...zones[idx], ...args } - await this._save(zones) - return true + return this.file.mutate('zone', (zones) => { + const idx = zones.findIndex((z) => z.id === args.id) + if (idx === -1) return false + zones[idx] = { ...zones[idx], ...args } + return true + }) } async delete(args) { - const zones = await this._load() - const idx = zones.findIndex((z) => z.id === args.id) - if (idx === -1) return false - - zones[idx].deleted = args.deleted ?? true - await this._save(zones) - return true + return this.file.mutate('zone', (zones) => { + const row = zones.find((z) => z.id === args.id) + if (!row) return false + row.deleted = args.deleted ?? true + return true + }) } async destroy(args) { - const zones = await this._load() - const before = zones.length - const filtered = zones.filter((z) => z.id !== args.id) - if (filtered.length === before) return false - await this._save(filtered) - return true + return this.file.mutate('zone', (zones) => removeRows(zones, (z) => z.id === args.id) > 0) } async nameserversFor(zid) { diff --git a/lib/zone/store/file.test.js b/lib/zone/store/file.test.js index 25a79cf..4f3bcfc 100644 --- a/lib/zone/store/file.test.js +++ b/lib/zone/store/file.test.js @@ -6,6 +6,7 @@ import { describe, it, before, after } from 'node:test' import FileStore from '../../store/file.js' import ZoneRepoFile from './file.js' +import { seed } from '../../../test/seed.js' const envKeys = ['NICTOOL_DATA_STORE', 'NICTOOL_DATA_STORE_PATH', 'NICTOOL_CONF_DIR'] @@ -19,9 +20,9 @@ describe('zone file store nameservers', () => { process.env.NICTOOL_CONF_DIR = path.join(tmp, 'no-such-conf') process.env.NICTOOL_DATA_STORE = 'json' process.env.NICTOOL_DATA_STORE_PATH = tmp - await new FileStore('zone').save('zone', [{ id: 1, zone: 'example.com' }]) - await new FileStore('nameserver').save('nameserver', [{ id: 5, name: 'ns1.example.net.', ttl: 3600 }]) - await new FileStore('zone_nameserver').save('zone_nameserver', [ + await seed(new FileStore('zone'), 'zone', [{ id: 1, zone: 'example.com' }]) + await seed(new FileStore('nameserver'), 'nameserver', [{ id: 5, name: 'ns1.example.net.', ttl: 3600 }]) + await seed(new FileStore('zone_nameserver'), 'zone_nameserver', [ { zid: 1, nid: 5 }, { zid: 2, nid: 5 }, ]) diff --git a/lib/zone_record/store/file.js b/lib/zone_record/store/file.js index d8913ba..0362a4c 100644 --- a/lib/zone_record/store/file.js +++ b/lib/zone_record/store/file.js @@ -1,4 +1,4 @@ -import FileStore, { nextId } from '../../store/file.js' +import FileStore, { nextId, removeRows } from '../../store/file.js' import { idConflict } from '../../store/error.js' import ZoneRecordBase from './base.js' @@ -13,10 +13,6 @@ class ZoneRecordRepoFile extends ZoneRecordBase { return this.file.load('zone_record') } - async _save(records) { - return this.file.save('zone_record', records) - } - async create(args, options) { args = JSON.parse(JSON.stringify(args)) @@ -99,32 +95,25 @@ class ZoneRecordRepoFile extends ZoneRecordBase { async put(args) { if (!args.id) return false - const records = await this._load() - const idx = records.findIndex((r) => r.id === args.id) - if (idx === -1) return false - - records[idx] = { ...records[idx], ...args } - await this._save(records) - return true + return this.file.mutate('zone_record', (records) => { + const idx = records.findIndex((r) => r.id === args.id) + if (idx === -1) return false + records[idx] = { ...records[idx], ...args } + return true + }) } async delete(args) { - const records = await this._load() - const idx = records.findIndex((r) => r.id === args.id) - if (idx === -1) return false - - records[idx].deleted = args.deleted ?? true - await this._save(records) - return true + return this.file.mutate('zone_record', (records) => { + const row = records.find((r) => r.id === args.id) + if (!row) return false + row.deleted = args.deleted ?? true + return true + }) } async destroy(args) { - const records = await this._load() - const before = records.length - const filtered = records.filter((r) => r.id !== args.id) - if (filtered.length === before) return false - await this._save(filtered) - return true + return this.file.mutate('zone_record', (records) => removeRows(records, (r) => r.id === args.id) > 0) } } diff --git a/test/seed.js b/test/seed.js new file mode 100644 index 0000000..1aa0fc6 --- /dev/null +++ b/test/seed.js @@ -0,0 +1,7 @@ +// replaces a file store's rows, and forgets its last_id, as a fixture +export function seed(store, key, rows) { + return store.mutate(key, (current, data) => { + current.splice(0, current.length, ...rows) + delete data.last_id + }) +} From 33c148322f2c443e5d8f58965f02fbbf12d1673f Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Wed, 14 Oct 2026 21:53:00 +0100 Subject: [PATCH 10/12] store: write file stores atomically A load during a write could read half a file and fail to parse. _write now writes a copy with the old file's mode and renames it over the file. --- lib/store/file.js | 16 +++++++++++++++- lib/store/file.test.js | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/lib/store/file.js b/lib/store/file.js index 88f3a09..2e43d02 100644 --- a/lib/store/file.js +++ b/lib/store/file.js @@ -1,3 +1,4 @@ +import { randomBytes } from 'node:crypto' import fs from 'node:fs/promises' import path from 'node:path' @@ -90,9 +91,22 @@ export class FileStore { } } + // a copy renamed over the file, so a load never sees half of it async _write(file, data) { await fs.mkdir(path.dirname(file), { recursive: true }) - await fs.writeFile(file, await this.codec.stringify(data)) + const old = await fs.stat(file).catch((err) => { + if (err.code !== 'ENOENT') throw err + }) + const tmp = `${file}.${randomBytes(6).toString('hex')}.tmp` + try { + const mode = old ? old.mode & 0o777 : 0o666 + await fs.writeFile(tmp, await this.codec.stringify(data), { flag: 'wx', mode }) + if (old) await fs.chmod(tmp, mode) + await fs.rename(tmp, file) + } catch (err) { + if (err.code !== 'EEXIST') await fs.rm(tmp, { force: true }).catch(() => {}) + throw err + } } async load(key) { diff --git a/lib/store/file.test.js b/lib/store/file.test.js index 8fcb7e3..8a8c4cf 100644 --- a/lib/store/file.test.js +++ b/lib/store/file.test.js @@ -1,5 +1,6 @@ import assert from 'node:assert/strict' import fs from 'node:fs' +import fsp from 'node:fs/promises' import os from 'node:os' import path from 'node:path' import { describe, it, before, after, beforeEach } from 'node:test' @@ -117,6 +118,40 @@ describe('file store', () => { }) for (const type of ['json', 'toml']) { + it(`never lets a ${type} load read a half-written file`, async (t) => { + process.env.NICTOOL_DATA_STORE = type + const store = new FileStore('torn') + const rows = Array.from({ length: 50 }, (_, id) => ({ id, owner: `host${id}.example.com.` })) + await seed(store, 'torn', rows) + + const rename = fsp.rename + let seen + t.mock.method(fsp, 'rename', async (from, to) => { + seen = await store.load('torn').then( + (r) => r.length, + (err) => err.message, + ) + await rename(from, to) + }) + await seed(store, 'torn', [...rows, { id: 50, owner: 'new.example.com.' }]) + + assert.equal(seen, rows.length) + assert.equal((await store.load('torn')).length, rows.length + 1) + assert.deepEqual( + fs.readdirSync(tmp).filter((f) => f.endsWith('.tmp')), + [], + ) + }) + + it(`keeps the mode of an existing ${type} file`, { skip: process.platform === 'win32' }, async () => { + process.env.NICTOOL_DATA_STORE = type + const store = new FileStore('moded') + await seed(store, 'moded', [{ id: 1 }]) + fs.chmodSync(store.filePath, 0o664) + await seed(store, 'moded', [{ id: 2 }]) + assert.equal(fs.statSync(store.filePath).mode & 0o777, 0o664) + }) + it(`serializes ${type} id allocation and does not reuse a destroyed id`, async () => { process.env.NICTOOL_DATA_STORE = type const store = new GroupRepoFile() From ee6b1181fc183b12b6b80944b270ffdca8ae3afb Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Wed, 14 Oct 2026 22:56:50 +0100 Subject: [PATCH 11/12] store: answer the file-store review - A permission with no user or group answers 400 on every store; over HTTP mysql answered 500 too, from the response check. - get({ id }) and a permission put, delete or destroy now look in the same order: users, standalone, then groups. A null user or group id counts as absent on both stores. - nextId skips a row id that isn't an unsigned integer, as mutate()'s last_id scan does, and one claimId() helper allocates for every store. - File-store permission creates run one at a time, so two creates with one explicit id can't both pass the free-id check. - mutate() hands callbacks keep(), so an existing session or an ifExists return leaves the file alone. - Race tests cover put, delete and destroy on every file store. - The FileStore comment names last_id, and three comments are gone. --- lib/group/store/file.js | 9 +- lib/nameserver/store/file.js | 9 +- lib/permission/store/file.js | 55 +++++++----- lib/permission/store/mysql.js | 7 +- lib/permission/test/mysql.js | 6 ++ lib/session/store/file.js | 10 +-- lib/store/error.js | 12 ++- lib/store/file.js | 43 ++++++---- lib/store/file.test.js | 153 +++++++++++++++++++++++++++------- lib/user/store/file.js | 11 +-- lib/zone/store/file.js | 9 +- lib/zone_record/store/file.js | 9 +- routes/index.js | 3 + routes/permission.test.js | 11 +++ test/seed.js | 1 - 15 files changed, 239 insertions(+), 109 deletions(-) diff --git a/lib/group/store/file.js b/lib/group/store/file.js index f3411ac..76f244c 100644 --- a/lib/group/store/file.js +++ b/lib/group/store/file.js @@ -1,4 +1,4 @@ -import FileStore, { nextId, removeRows } from '../../store/file.js' +import FileStore, { claimId, removeRows } from '../../store/file.js' import { idConflict } from '../../store/error.js' import GroupBase from './base.js' @@ -59,13 +59,12 @@ class GroupRepoFile extends GroupBase { const usable_ns = args.usable_ns ?? [] delete args.usable_ns - return this.file.mutate('group', (groups, data) => { + return this.file.mutate('group', (groups, data, keep) => { if (args.id !== undefined && groups.some((group) => group.id === args.id)) { - return idConflict('group', args.id, options) + return keep(idConflict('group', args.id, options)) } - if (args.id === undefined) args.id = nextId(groups, data.last_id) - data.last_id = Math.max(data.last_id ?? 0, args.id) + args.id = claimId(groups, data, args.id) const gid = args.id args.permissions = { diff --git a/lib/nameserver/store/file.js b/lib/nameserver/store/file.js index 4f4e32e..8598c41 100644 --- a/lib/nameserver/store/file.js +++ b/lib/nameserver/store/file.js @@ -1,4 +1,4 @@ -import FileStore, { nextId, removeRows } from '../../store/file.js' +import FileStore, { claimId, removeRows } from '../../store/file.js' import { idConflict } from '../../store/error.js' import NameserverBase from './base.js' @@ -38,13 +38,12 @@ class NameserverRepoFile extends NameserverBase { async create(args, options) { args = JSON.parse(JSON.stringify(args)) - return this.file.mutate('nameserver', (nameservers, data) => { + return this.file.mutate('nameserver', (nameservers, data, keep) => { if (args.id !== undefined && nameservers.some((nameserver) => nameserver.id === args.id)) { - return idConflict('nameserver', args.id, options) + return keep(idConflict('nameserver', args.id, options)) } - if (args.id === undefined) args.id = nextId(nameservers, data.last_id, 0xffff) - data.last_id = Math.max(data.last_id ?? 0, args.id) + args.id = claimId(nameservers, data, args.id, 0xffff) nameservers.push(args) return args.id }) diff --git a/lib/permission/store/file.js b/lib/permission/store/file.js index ac45cfb..9ad2a86 100644 --- a/lib/permission/store/file.js +++ b/lib/permission/store/file.js @@ -1,5 +1,5 @@ -import FileStore from '../../store/file.js' -import { idConflict } from '../../store/error.js' +import FileStore, { serialize } from '../../store/file.js' +import { idConflict, StoreInvalidError } from '../../store/error.js' import PermissionBase from './base.js' @@ -62,12 +62,19 @@ class PermissionRepoFile extends PermissionBase { // CRUD // --------------------------------------------------------------------------- + // one at a time, so an id checked as free is still free when it's written async create(args, options) { + return serialize('permission create', () => this._create(args, options)) + } + + async _create(args, options) { args = JSON.parse(JSON.stringify(args)) - const uid = args.uid ?? args.user?.id - const gid = args.gid ?? args.group?.id + const uid = args.uid ?? args.user?.id ?? undefined + const gid = args.gid ?? args.group?.id ?? undefined delete args.uid delete args.gid + if (uid === undefined && gid === undefined) + throw new StoreInvalidError('a permission needs a user or group') if (args.id !== undefined) { const [users, groups, permissions] = await Promise.all([ @@ -80,8 +87,8 @@ class PermissionRepoFile extends PermissionBase { ...groups.map((group) => ({ permission: group.permissions, gid: group.id })), ...permissions.map((permission) => ({ permission, - uid: permission.uid ?? permission.user?.id, - gid: permission.gid ?? permission.group?.id, + uid: permission.uid ?? permission.user?.id ?? undefined, + gid: permission.gid ?? permission.group?.id ?? undefined, })), ].find((entry) => entry.permission?.id === args.id) if (existing) { @@ -96,10 +103,10 @@ class PermissionRepoFile extends PermissionBase { } if (uid !== undefined) { - const created = await this.userFile.mutate('user', (users) => { + const created = await this.userFile.mutate('user', (users, data, keep) => { const user = users.find((u) => u.id === uid) if (!user) return false - if (user.permissions) return idConflict('permission', user.permissions.id, options) + if (user.permissions) return keep(idConflict('permission', user.permissions.id, options)) const perm = JSON.parse(JSON.stringify(args)) perm.id = args.id ?? uid if (!perm.user) perm.user = {} @@ -113,10 +120,10 @@ class PermissionRepoFile extends PermissionBase { } if (gid !== undefined && uid === undefined) { - const created = await this.groupFile.mutate('group', (groups) => { + const created = await this.groupFile.mutate('group', (groups, data, keep) => { const group = groups.find((g) => g.id === gid) if (!group) return false - if (group.permissions) return idConflict('permission', group.permissions.id, options) + if (group.permissions) return keep(idConflict('permission', group.permissions.id, options)) const perm = JSON.parse(JSON.stringify(args)) perm.id = args.id ?? gid if (!perm.group) perm.group = {} @@ -129,12 +136,10 @@ class PermissionRepoFile extends PermissionBase { // Standalone fallback: neither user nor group record found const permId = args.id ?? uid ?? gid - // inline permission ids are user and group ids, so there is no free id to give - if (permId === undefined) throw new Error('a file-store permission needs a user or group') - return this.standaloneFile.mutate('permission', (perms) => { + return this.standaloneFile.mutate('permission', (perms, data, keep) => { if (perms.some((permission) => permission.id === permId)) { - return idConflict('permission', permId, options) + return keep(idConflict('permission', permId, options)) } const perm = { ...args, id: permId } if (uid !== undefined) perm.uid = uid @@ -168,11 +173,7 @@ class PermissionRepoFile extends PermissionBase { } if (args.id !== undefined) { - // Search user.toml by permissions.id. - // NOTE: group.toml is intentionally NOT searched here — group permissions are - // accessed via { gid }. Searching groups would cause false positives after a - // user permission is destroyed, because user and group share the same numeric - // id space (both user 4096 and group 4096 set permissions.id = 4096). + // users, then standalone, then groups, the order _change() writes in const users = await this._loadUsers() const user = users.find((u) => u.permissions?.id === args.id) if (user?.permissions) { @@ -190,6 +191,14 @@ class PermissionRepoFile extends PermissionBase { if (isDeleted !== wantDeleted) return undefined return this._postProcess(found, deletedArg) } + + const groups = await this._loadGroups() + const group = groups.find((g) => g.permissions?.id === args.id) + if (group) { + const perm = this._postProcess(group.permissions, deletedArg) + if (deletedArg === true && perm.deleted !== true) return undefined + return perm + } } return undefined @@ -208,8 +217,8 @@ class PermissionRepoFile extends PermissionBase { return this._postProcess(group.permissions, deletedArg) } - // Runs change(holder, key) on the permission with this id, under the lock - // of whichever file holds it. Returns false when no file does. + // users, then standalone, then groups, as get({ id }) looks; each change + // runs under its file's lock async _change(id, change) { const inline = (rows) => { const row = rows.find((r) => r.permissions?.id === id) @@ -218,13 +227,13 @@ class PermissionRepoFile extends PermissionBase { return true } if (await this.userFile.mutate('user', inline)) return true - if (await this.groupFile.mutate('group', inline)) return true - return this.standaloneFile.mutate('permission', (perms) => { + const standalone = await this.standaloneFile.mutate('permission', (perms) => { const idx = perms.findIndex((p) => p.id === id) if (idx === -1) return false change(perms, idx) return true }) + return standalone || this.groupFile.mutate('group', inline) } async put(args) { diff --git a/lib/permission/store/mysql.js b/lib/permission/store/mysql.js index f5e73b6..a0281f2 100644 --- a/lib/permission/store/mysql.js +++ b/lib/permission/store/mysql.js @@ -1,5 +1,5 @@ import Mysql from '../../mysql.js' -import { idConflict } from '../../store/error.js' +import { idConflict, StoreInvalidError } from '../../store/error.js' import { mapToDbColumn } from '../../util.js' import PermissionBase from './base.js' @@ -19,6 +19,11 @@ class PermissionRepoMySQL extends PermissionBase { async create(args, options) { args = objectToDb(args) + if (args.uid === null) delete args.uid + if (args.gid === null) delete args.gid + if (args.uid === undefined && args.gid === undefined) { + throw new StoreInvalidError('a permission needs a user or group') + } if (args.id !== undefined) { const p = (await this.get({ id: args.id })) ?? (await this.get({ id: args.id, deleted: true })) diff --git a/lib/permission/test/mysql.js b/lib/permission/test/mysql.js index 5e322a1..9297c92 100644 --- a/lib/permission/test/mysql.js +++ b/lib/permission/test/mysql.js @@ -16,4 +16,10 @@ describe('permission (mysql)', function () { assert.equal(p?.id, 1) assert.equal(p.user.id, null) }) + + it('refuses a second group permission sent with a null user id', async () => { + await assert.rejects(Permission.create({ user: { id: null }, group: { id: 1 }, name: 'twice' }), { + code: 'STORE_CONFLICT', + }) + }) }) diff --git a/lib/session/store/file.js b/lib/session/store/file.js index 4e97c07..2c2eb16 100644 --- a/lib/session/store/file.js +++ b/lib/session/store/file.js @@ -1,4 +1,4 @@ -import FileStore, { nextId, removeRows } from '../../store/file.js' +import FileStore, { claimId, removeRows } from '../../store/file.js' // Map legacy nt_* column names to the friendly API names used throughout. function normalizeArgs(args) { @@ -29,14 +29,13 @@ class SessionRepoFile { async create(args) { args = normalizeArgs(JSON.parse(JSON.stringify(args))) - return this.file.mutate('session', (sessions, data) => { + return this.file.mutate('session', (sessions, data, keep) => { const existing = sessions.find( (session) => session.uid === args.uid && session.session === args.session, ) - if (existing) return existing.id + if (existing) return keep(existing.id) - args.id = nextId(sessions, data.last_id) - data.last_id = args.id + args.id = claimId(sessions, data) sessions.push(args) return args.id }) @@ -95,7 +94,6 @@ class SessionRepoFile { 'session', (sessions) => removeRows(sessions, (s) => { - // Remove this session only when every provided filter matches it if (args.id !== undefined && s.id !== args.id) return false if (args.uid !== undefined && s.uid !== args.uid) return false if (args.session !== undefined && s.session !== args.session) return false diff --git a/lib/store/error.js b/lib/store/error.js index bdd3339..83eec87 100644 --- a/lib/store/error.js +++ b/lib/store/error.js @@ -1,11 +1,19 @@ export class StoreConflictError extends Error { - constructor(entity, id) { - super(`${entity} id ${id} already exists`) + constructor(entity, id, message = `${entity} id ${id} already exists`) { + super(message) this.name = 'StoreConflictError' this.code = 'STORE_CONFLICT' } } +export class StoreInvalidError extends Error { + constructor(message) { + super(message) + this.name = 'StoreInvalidError' + this.code = 'STORE_INVALID' + } +} + // ifExists keeps the old return-the-existing-id behaviour for fixtures, so // create() is not an accidental upsert everywhere else. export function idConflict(entity, id, options = {}) { diff --git a/lib/store/file.js b/lib/store/file.js index 2e43d02..f0ab9bd 100644 --- a/lib/store/file.js +++ b/lib/store/file.js @@ -22,7 +22,7 @@ const tomlCodec = { const codecs = { json: jsonCodec, toml: tomlCodec } const mutations = new Map() -async function serialize(file, callback) { +export async function serialize(file, callback) { const previous = mutations.get(file) ?? Promise.resolve() const current = previous.catch(() => {}).then(callback) mutations.set(file, current) @@ -38,24 +38,29 @@ export function resolveCodec(type = storeConfig().type) { return codecs[type] ?? jsonCodec } +export function highestId(rows) { + return rows.reduce((max, row) => (Number.isInteger(row.id) && row.id > max ? row.id : max), 0) +} + export function nextId(rows, lastId = 0, maxId = 0xffffffff) { if (!Number.isInteger(lastId) || lastId < 0) throw new TypeError('file store last id must be unsigned') - let highest = lastId - for (const row of rows) { - if (row.id === undefined) continue - if (!Number.isInteger(row.id) || row.id < 0) throw new TypeError('file store row id must be unsigned') - highest = Math.max(highest, row.id) - } - + const highest = Math.max(lastId, highestId(rows)) if (highest >= maxId) throw new RangeError(`file store id space exhausted at ${maxId}`) return highest + 1 } +// the id a create gets, recorded as the file's high-water mark +export function claimId(rows, data, id, maxId) { + if (id === undefined) id = nextId(rows, data.last_id, maxId) + data.last_id = Math.max(data.last_id ?? 0, id) + return id +} + /** - * One file per entity, holding an array of rows under a single top-level key. - * The codec is chosen by store.type; everything above this layer is identical - * for JSON and TOML. + * One file per entity, holding an array of rows under a single top-level key, + * plus last_id, the highest id ever allocated. The codec is chosen by + * store.type; everything above this layer is identical for JSON and TOML. */ export class FileStore { constructor(basename) { @@ -122,15 +127,17 @@ export class FileStore { data[key] = rows // a file from before last_id existed mustn't reuse a destroyed id if (data.last_id === undefined) { - const highest = rows.reduce( - (max, row) => (Number.isInteger(row.id) && row.id > max ? row.id : max), - 0, - ) + const highest = highestId(rows) if (highest > 0) data.last_id = highest } - const result = await callback(rows, data) - // false means the callback changed nothing - if (result !== false) await this._write(file, data) + // a callback that changed nothing returns false, or its result via keep() + let changed = true + const keep = (value) => { + changed = false + return value + } + const result = await callback(rows, data, keep) + if (changed && result !== false) await this._write(file, data) return result }) } diff --git a/lib/store/file.test.js b/lib/store/file.test.js index 8a8c4cf..ea8416e 100644 --- a/lib/store/file.test.js +++ b/lib/store/file.test.js @@ -6,9 +6,13 @@ import path from 'node:path' import { describe, it, before, after, beforeEach } from 'node:test' import GroupRepoFile from '../group/store/file.js' +import NameserverRepoFile from '../nameserver/store/file.js' import PermissionRepoFile from '../permission/store/file.js' +import SessionRepoFile from '../session/store/file.js' import Credentials from '../user/credentials.js' import UserRepoFile from '../user/store/file.js' +import ZoneRepoFile from '../zone/store/file.js' +import ZoneRecordRepoFile from '../zone_record/store/file.js' import FileStore, { nextId, resolveCodec } from './file.js' import { seed } from '../../test/seed.js' @@ -22,6 +26,7 @@ describe('file store', () => { assert.equal(nextId([]), 1) assert.equal(nextId([{ id: 9 }, { id: 2 }, {}]), 10) assert.equal(nextId([{ id: 9 }], 12), 13) + assert.equal(nextId([{ id: null }, { id: 'x' }, { id: 4 }]), 5) assert.throws(() => nextId([{ id: 0xffffffff }]), /id space exhausted/) }) @@ -200,43 +205,128 @@ describe('file store', () => { assert.equal(stored.find((user) => user.id === uid).permissions.id, permId) }) - for (const [label, change] of [ - ['put', (id) => new GroupRepoFile().put({ id, name: 'renamed.example' })], - ['delete', (id) => new GroupRepoFile().delete({ id })], - ['destroy', (id) => new GroupRepoFile().destroy({ id })], - ['permission put', (id) => new PermissionRepoFile().put({ id, zone: { create: true } })], + for (const [entity, Repo, make, changes] of [ + ['group', GroupRepoFile, (i) => ({ name: `racer-${i}.example` }), { put: { name: 'renamed.example' } }], + ['nameserver', NameserverRepoFile, (i) => ({ gid: 1, name: `ns${i}.example.` }), { put: { ttl: 7 } }], + [ + 'session', + SessionRepoFile, + (i) => ({ uid: 1, session: `racer-${i}` }), + { put: { session: 'renamed' } }, + ], + [ + 'user', + UserRepoFile, + (i) => ({ gid: 1, username: `racer-${i}`, email: `racer-${i}@example.com` }), + { put: { email: 'renamed@example.com' } }, + ], + [ + 'zone', + ZoneRepoFile, + (i) => ({ gid: 1, zone: `racer${i}.example.` }), + { put: { description: 'renamed' } }, + ], + [ + 'zone_record', + ZoneRecordRepoFile, + (i) => ({ zid: 1, owner: `racer${i}.example.`, type: 'A', address: '192.0.2.1' }), + { put: { ttl: 9 } }, + ], ]) { - it(`keeps ${type} groups created during a group ${label}`, async (t) => { - process.env.NICTOOL_DATA_STORE = type - for (const name of ['group', 'user', 'permission']) { - fs.rmSync(path.join(tmp, `${name}.${type}`), { force: true }) - } - const groups = new GroupRepoFile() - const gid = await groups.create({ name: 'target.example' }) - - // the groups are created once the change has read the group file - const read = FileStore.prototype._read - const racers = [] - t.mock.method(FileStore.prototype, '_read', async function (file) { - const data = await read.call(this, file) - if (racers.length === 0 && file.endsWith(`group.${type}`)) { - for (let index = 0; index < 5; index++) { - racers.push(groups.create({ name: `racer-${index}.example` })) + const ops = [ + ['put', (repo, id) => repo.put({ id, ...changes.put })], + ['delete', (repo, id) => repo.delete({ id })], + ] + if (entity !== 'session') ops.push(['destroy', (repo, id) => repo.destroy({ id })]) + + for (const [label, change] of ops) { + it(`keeps ${type} ${entity} rows created during a ${label}`, async (t) => { + process.env.NICTOOL_DATA_STORE = type + fs.rmSync(path.join(tmp, `${entity}.${type}`), { force: true }) + const repo = new Repo() + const id = await repo.create(make('target')) + + // the rows are created once the change has read the file + const read = FileStore.prototype._read + const racers = [] + t.mock.method(FileStore.prototype, '_read', async function (file) { + const data = await read.call(this, file) + if (racers.length === 0 && file.endsWith(`${entity}.${type}`)) { + for (let index = 0; index < 5; index++) racers.push(repo.create(make(index))) } - } - return data - }) + return data + }) - assert.equal(await change(gid), true) - assert.equal(racers.length, 5) - const created = await Promise.all(racers) - t.mock.restoreAll() + assert.equal(await change(repo, id), true) + assert.equal(racers.length, 5) + const created = await Promise.all(racers) + t.mock.restoreAll() - const stored = (await new FileStore('group').load('group')).map((group) => group.id) - for (const id of created) assert.ok(stored.includes(id), `group ${id} survives`) - }) + const stored = (await new FileStore(entity).load(entity)).map((row) => row.id) + for (const rowId of created) assert.ok(stored.includes(rowId), `${entity} ${rowId} survives`) + }) + } } + it(`changes a ${type} group's permission found by gid`, async () => { + process.env.NICTOOL_DATA_STORE = type + for (const name of ['user', 'permission']) fs.rmSync(path.join(tmp, `${name}.${type}`), { force: true }) + await seed(new FileStore('group'), 'group', [ + { id: 8, name: 'eight', permissions: { id: 8, zone: { create: true } } }, + ]) + const permissions = new PermissionRepoFile() + const { id } = await permissions.get({ gid: 8 }) + + assert.ok(await permissions.put({ id, zone: { create: false } })) + assert.equal((await permissions.get({ gid: 8 })).zone.create, false) + assert.ok(await permissions.destroy({ id })) + assert.equal(await permissions.get({ gid: 8 }), undefined) + }) + + it(`stores a ${type} group permission whose user id is null`, async () => { + process.env.NICTOOL_DATA_STORE = type + for (const name of ['user', 'permission']) fs.rmSync(path.join(tmp, `${name}.${type}`), { force: true }) + await seed(new FileStore('group'), 'group', [{ id: 3, name: 'three' }]) + const permissions = new PermissionRepoFile() + + assert.equal(await permissions.create({ user: { id: null }, group: { id: 3 }, name: 'grp' }), 3) + assert.equal((await permissions.get({ gid: 3 })).name, 'grp') + assert.equal((await permissions.get({ id: 3 })).name, 'grp') + await assert.rejects(permissions.create({ id: 900, name: 'no target' }), /needs a user or group/) + }) + + it(`gives one of two ${type} permission creates for one explicit id a conflict`, async () => { + process.env.NICTOOL_DATA_STORE = type + for (const name of ['group', 'permission']) + fs.rmSync(path.join(tmp, `${name}.${type}`), { force: true }) + await seed(new FileStore('user'), 'user', [ + { id: 1, gid: 1, username: 'one' }, + { id: 2, gid: 1, username: 'two' }, + ]) + const permissions = new PermissionRepoFile() + + const results = await Promise.allSettled([ + permissions.create({ id: 77, uid: 1, name: 'first' }), + permissions.create({ id: 77, uid: 2, name: 'second' }), + ]) + assert.deepEqual(results.map((r) => r.status).sort(), ['fulfilled', 'rejected']) + assert.equal(results.find((r) => r.status === 'rejected').reason.code, 'STORE_CONFLICT') + }) + + it(`changes the ${type} permission get returns, not a group's with the same id`, async () => { + process.env.NICTOOL_DATA_STORE = type + fs.rmSync(path.join(tmp, `user.${type}`), { force: true }) + await seed(new FileStore('group'), 'group', [ + { id: 7, name: 'seven', permissions: { id: 7, name: 'group' } }, + ]) + await seed(new FileStore('permission'), 'permission', [{ id: 7, name: 'standalone' }]) + const permissions = new PermissionRepoFile() + + assert.ok(await permissions.put({ id: 7, name: 'changed' })) + assert.equal((await permissions.get({ id: 7 })).name, 'changed') + assert.equal((await new FileStore('group').load('group'))[0].permissions.name, 'group') + }) + it(`does not reuse a ${type} id destroyed before last_id was recorded`, async () => { process.env.NICTOOL_DATA_STORE = type fs.rmSync(path.join(tmp, `group.${type}`), { force: true }) @@ -258,6 +348,7 @@ describe('file store', () => { process.env.NICTOOL_DATA_STORE = type fs.rmSync(path.join(tmp, `zone.${type}`), { force: true }) assert.equal(await new FileStore('zone').mutate('zone', () => false), false) + assert.equal(await new FileStore('zone').mutate('zone', (rows, data, keep) => keep(3)), 3) assert.equal(fs.existsSync(path.join(tmp, `zone.${type}`)), false) }) diff --git a/lib/user/store/file.js b/lib/user/store/file.js index 414d66e..e6f8d08 100644 --- a/lib/user/store/file.js +++ b/lib/user/store/file.js @@ -1,4 +1,4 @@ -import FileStore, { nextId, removeRows } from '../../store/file.js' +import FileStore, { claimId, removeRows } from '../../store/file.js' import { idConflict } from '../../store/error.js' import Config from '../../config.js' import Credentials from '../credentials.js' @@ -144,13 +144,12 @@ class UserRepoFile extends UserBase { Object.assign(args, await Credentials.forStorage(args.password, args.pass_salt)) } - return this.file.mutate('user', (users, data) => { + return this.file.mutate('user', (users, data, keep) => { if (args.id !== undefined && users.some((user) => user.id === args.id)) { - return idConflict('user', args.id, options) + return keep(idConflict('user', args.id, options)) } - if (args.id === undefined) args.id = nextId(users, data.last_id) - data.last_id = Math.max(data.last_id ?? 0, args.id) + args.id = claimId(users, data, args.id) if (inherit === false) { args.permissions = { @@ -178,10 +177,8 @@ class UserRepoFile extends UserBase { if (idx === -1) return false if (inherit === true) { - // Switch to inherited: remove explicit permissions delete users[idx].permissions } else if (inherit === false && !users[idx].permissions) { - // Switch to explicit: create default permission entry users[idx].permissions = { ...JSON.parse(JSON.stringify(defaultPermissions)), id: users[idx].id, diff --git a/lib/zone/store/file.js b/lib/zone/store/file.js index cd073c8..9f2b47b 100644 --- a/lib/zone/store/file.js +++ b/lib/zone/store/file.js @@ -1,4 +1,4 @@ -import FileStore, { nextId, removeRows, resolveCodec } from '../../store/file.js' +import FileStore, { claimId, removeRows, resolveCodec } from '../../store/file.js' import { idConflict } from '../../store/error.js' import ZoneBase from './base.js' @@ -34,13 +34,12 @@ class ZoneRepoFile extends ZoneBase { async create(args, options) { args = JSON.parse(JSON.stringify(args)) - return this.file.mutate('zone', (zones, data) => { + return this.file.mutate('zone', (zones, data, keep) => { if (args.id !== undefined && zones.some((zone) => zone.id === args.id)) { - return idConflict('zone', args.id, options) + return keep(idConflict('zone', args.id, options)) } - if (args.id === undefined) args.id = nextId(zones, data.last_id) - data.last_id = Math.max(data.last_id ?? 0, args.id) + args.id = claimId(zones, data, args.id) zones.push(args) return args.id }) diff --git a/lib/zone_record/store/file.js b/lib/zone_record/store/file.js index 0362a4c..c7306c1 100644 --- a/lib/zone_record/store/file.js +++ b/lib/zone_record/store/file.js @@ -1,4 +1,4 @@ -import FileStore, { nextId, removeRows } from '../../store/file.js' +import FileStore, { claimId, removeRows } from '../../store/file.js' import { idConflict } from '../../store/error.js' import ZoneRecordBase from './base.js' @@ -16,13 +16,12 @@ class ZoneRecordRepoFile extends ZoneRecordBase { async create(args, options) { args = JSON.parse(JSON.stringify(args)) - return this.file.mutate('zone_record', (records, data) => { + return this.file.mutate('zone_record', (records, data, keep) => { if (args.id !== undefined && records.some((record) => record.id === args.id)) { - return idConflict('zone record', args.id, options) + return keep(idConflict('zone record', args.id, options)) } - if (args.id === undefined) args.id = nextId(records, data.last_id) - data.last_id = Math.max(data.last_id ?? 0, args.id) + args.id = claimId(records, data, args.id) if (args.ttl === undefined) args.ttl = 0 records.push(args) diff --git a/routes/index.js b/routes/index.js index 590e51d..f158135 100644 --- a/routes/index.js +++ b/routes/index.js @@ -108,6 +108,9 @@ async function setup() { server.ext('onPreResponse', (request, h) => { const response = request.response + if (response?.code === 'STORE_INVALID') { + return h.response({ statusCode: 400, error: 'Bad Request', message: response.message }).code(400) + } if (!(response instanceof StoreConflictError) && response?.code !== 'STORE_CONFLICT') { return h.continue } diff --git a/routes/permission.test.js b/routes/permission.test.js index 9c9eddf..eafac7c 100644 --- a/routes/permission.test.js +++ b/routes/permission.test.js @@ -75,6 +75,17 @@ describe('permission routes', () => { assert.ok(Number.isInteger(case2Id)) }) + it('POST /permission with no user or group', async () => { + const res = await server.inject({ + method: 'POST', + url: '/permission', + headers: auth.headers, + payload: { name: 'No Target Route Test' }, + }) + assert.equal(res.statusCode, 400) + assert.match(res.result.message, /needs a user or group/) + }) + it('POST /permission rejects an existing target', async () => { const testCase = JSON.parse(JSON.stringify(permCase)) delete testCase.id diff --git a/test/seed.js b/test/seed.js index 1aa0fc6..0a2bd12 100644 --- a/test/seed.js +++ b/test/seed.js @@ -1,4 +1,3 @@ -// replaces a file store's rows, and forgets its last_id, as a fixture export function seed(store, key, rows) { return store.mutate(key, (current, data) => { current.splice(0, current.length, ...rows) From fe5b78a971bdd727e5db2c0306198789d0d7c4d7 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Thu, 15 Oct 2026 07:14:18 +0100 Subject: [PATCH 12/12] permission: keep groups out of id lookups User and group permissions share ids in the file stores. With groups in get({ id }) and _change(), destroying a user's permission left the group's with the same id reachable, so DELETE /permission/{id} removed it. Group permissions are reached by gid again. --- lib/permission/store/file.js | 33 ++++++++++++--------------------- lib/store/file.test.js | 22 +++++++++++++--------- 2 files changed, 25 insertions(+), 30 deletions(-) diff --git a/lib/permission/store/file.js b/lib/permission/store/file.js index 9ad2a86..8c68da9 100644 --- a/lib/permission/store/file.js +++ b/lib/permission/store/file.js @@ -12,14 +12,14 @@ import PermissionBase from './base.js' * record. Looked up via users[i].permissions.id === N. * * 2. Inline in the group file — group-level permissions created by the group - * store. Looked up via groups[i].permissions.id === N. + * store. Looked up by gid only, as their ids collide with users'. * * 3. Standalone permission file — fallback for permission IDs that reference * users/groups not present in the user / group files. * * get({ uid }) → inline permissions of that user * get({ gid }) → inline permissions of that group (uid absent) - * get({ id }) → search user → group → standalone by permissions.id + * get({ id }) → search user → standalone by permissions.id * getGroup({ uid }) → permissions of the group the user belongs to */ class PermissionRepoFile extends PermissionBase { @@ -173,7 +173,8 @@ class PermissionRepoFile extends PermissionBase { } if (args.id !== undefined) { - // users, then standalone, then groups, the order _change() writes in + // not groups: user and group permissions share ids, so a group's would + // answer for a destroyed user's. Group permissions are reached by gid. const users = await this._loadUsers() const user = users.find((u) => u.permissions?.id === args.id) if (user?.permissions) { @@ -191,14 +192,6 @@ class PermissionRepoFile extends PermissionBase { if (isDeleted !== wantDeleted) return undefined return this._postProcess(found, deletedArg) } - - const groups = await this._loadGroups() - const group = groups.find((g) => g.permissions?.id === args.id) - if (group) { - const perm = this._postProcess(group.permissions, deletedArg) - if (deletedArg === true && perm.deleted !== true) return undefined - return perm - } } return undefined @@ -217,23 +210,21 @@ class PermissionRepoFile extends PermissionBase { return this._postProcess(group.permissions, deletedArg) } - // users, then standalone, then groups, as get({ id }) looks; each change - // runs under its file's lock + // finds the permission where get({ id }) does, under that file's lock async _change(id, change) { - const inline = (rows) => { - const row = rows.find((r) => r.permissions?.id === id) - if (!row) return false - change(row, 'permissions') + const changed = await this.userFile.mutate('user', (users) => { + const user = users.find((u) => u.permissions?.id === id) + if (!user) return false + change(user, 'permissions') return true - } - if (await this.userFile.mutate('user', inline)) return true - const standalone = await this.standaloneFile.mutate('permission', (perms) => { + }) + if (changed) return true + return this.standaloneFile.mutate('permission', (perms) => { const idx = perms.findIndex((p) => p.id === id) if (idx === -1) return false change(perms, idx) return true }) - return standalone || this.groupFile.mutate('group', inline) } async put(args) { diff --git a/lib/store/file.test.js b/lib/store/file.test.js index ea8416e..011de9c 100644 --- a/lib/store/file.test.js +++ b/lib/store/file.test.js @@ -268,19 +268,24 @@ describe('file store', () => { } } - it(`changes a ${type} group's permission found by gid`, async () => { + it(`leaves a ${type} group's permission alone once a user's with its id is gone`, async () => { process.env.NICTOOL_DATA_STORE = type - for (const name of ['user', 'permission']) fs.rmSync(path.join(tmp, `${name}.${type}`), { force: true }) + fs.rmSync(path.join(tmp, `permission.${type}`), { force: true }) await seed(new FileStore('group'), 'group', [ - { id: 8, name: 'eight', permissions: { id: 8, zone: { create: true } } }, + { id: 1, name: 'one', permissions: { id: 1, name: 'group' } }, + ]) + await seed(new FileStore('user'), 'user', [ + { id: 1, gid: 1, username: 'one', permissions: { id: 1, name: 'user' } }, ]) const permissions = new PermissionRepoFile() - const { id } = await permissions.get({ gid: 8 }) - assert.ok(await permissions.put({ id, zone: { create: false } })) - assert.equal((await permissions.get({ gid: 8 })).zone.create, false) - assert.ok(await permissions.destroy({ id })) - assert.equal(await permissions.get({ gid: 8 }), undefined) + assert.ok(await permissions.destroy({ id: 1 })) + assert.equal(await permissions.get({ id: 1 }), undefined) + assert.equal(await permissions.put({ id: 1, name: 'changed' }), false) + assert.equal(await permissions.delete({ id: 1 }), false) + assert.equal(await permissions.destroy({ id: 1 }), false) + const [group] = await new FileStore('group').load('group') + assert.deepEqual({ ...group.permissions }, { id: 1, name: 'group' }) }) it(`stores a ${type} group permission whose user id is null`, async () => { @@ -291,7 +296,6 @@ describe('file store', () => { assert.equal(await permissions.create({ user: { id: null }, group: { id: 3 }, name: 'grp' }), 3) assert.equal((await permissions.get({ gid: 3 })).name, 'grp') - assert.equal((await permissions.get({ id: 3 })).name, 'grp') await assert.rejects(permissions.create({ id: 900, name: 'no target' }), /needs a user or group/) })