From 09595092c4aaf5dbc4146d0a412445aa69c17087 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sun, 30 Aug 2026 00:15:00 +0100 Subject: [PATCH] store: reject create conflicts --- CHANGELOG.md | 2 + lib/group/store/base.js | 4 +- lib/group/store/file.js | 12 ++- lib/group/store/mysql.js | 9 +- lib/group/test/index.js | 2 +- lib/nameserver/store/base.js | 4 +- lib/nameserver/store/file.js | 12 ++- lib/nameserver/store/mysql.js | 9 +- lib/permission/store/base.js | 4 +- lib/permission/store/file.js | 74 ++++++++++------ lib/permission/store/mysql.js | 37 +++++--- lib/permission/test/index.js | 6 +- lib/permission/test/permission.json | 2 +- lib/session/test/index.js | 2 +- lib/store/create_conflict.test.js | 129 ++++++++++++++++++++++++++++ lib/store/error.js | 14 +++ lib/user/store/base.js | 4 +- lib/user/store/file.js | 12 ++- lib/user/store/mysql.js | 9 +- lib/user/test/index.js | 4 +- lib/user/test/mysql.js | 2 +- lib/zone/store/base.js | 4 +- lib/zone/store/file.js | 12 ++- lib/zone/store/mysql.js | 9 +- lib/zone_record/store/base.js | 4 +- lib/zone_record/store/file.js | 12 ++- lib/zone_record/store/mysql.js | 9 +- routes/group.test.js | 4 +- routes/index.js | 16 ++++ routes/nameserver.test.js | 6 +- routes/permission.test.js | 57 ++++++++---- routes/session.test.js | 6 +- routes/test/permission.json | 2 +- routes/user.test.js | 4 +- routes/zone.test.js | 6 +- routes/zone_record.test.js | 22 ++++- test/fixtures.js | 14 ++- 37 files changed, 403 insertions(+), 137 deletions(-) create mode 100644 lib/store/create_conflict.test.js create mode 100644 lib/store/error.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 3151a4c..e03ac1e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Unreleased +- fix: reject create conflicts and duplicate permission targets + ### [3.0.3] - 2026-07-27 - many updates for data stores and NS backends diff --git a/lib/group/store/base.js b/lib/group/store/base.js index 11bfd56..e8cfee9 100644 --- a/lib/group/store/base.js +++ b/lib/group/store/base.js @@ -6,7 +6,7 @@ * * Repo contract: * get(args) → object[] - * create(args) → number (groupId) + * create(args, options) → number (groupId) * put(args) → boolean * delete(args) → boolean * destroy(args) → boolean @@ -29,7 +29,7 @@ class GroupBase { throw new Error('get() not implemented by this repo') } - async create(_args) { + async create(_args, _options) { throw new Error('create() not implemented by this repo') } diff --git a/lib/group/store/file.js b/lib/group/store/file.js index a5044c4..b2680cd 100644 --- a/lib/group/store/file.js +++ b/lib/group/store/file.js @@ -1,4 +1,5 @@ import FileStore from '../../store/file.js' +import { idConflict } from '../../store/error.js' import GroupBase from './base.js' @@ -57,12 +58,15 @@ class GroupRepoFile extends GroupBase { return [rootGid, ...this._collectSubgroupIds(groups, rootGid)] } - async create(args) { + async create(args, options) { args = JSON.parse(JSON.stringify(args)) - if (args.id) { - const existing = await this.get({ id: args.id }) - if (existing.length === 1) return existing[0].id + 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 ?? [] diff --git a/lib/group/store/mysql.js b/lib/group/store/mysql.js index 611810e..cb9422c 100644 --- a/lib/group/store/mysql.js +++ b/lib/group/store/mysql.js @@ -1,6 +1,7 @@ import Mysql from '../../mysql.js' import GroupBase from './base.js' import Permission from '../../permission/index.js' +import { idConflict } from '../../store/error.js' import { mapToDbColumn } from '../../util.js' const groupDbMap = { id: 'nt_group_id', parent_gid: 'parent_group_id' } @@ -12,10 +13,10 @@ class Group extends GroupBase { this.mysql = Mysql } - async create(args) { - if (args.id) { - const g = await this.get({ id: args.id }) - if (g.length === 1) return g[0].id + async create(args, options) { + if (args.id !== undefined) { + const g = [...(await this.get({ id: args.id })), ...(await this.get({ id: args.id, deleted: true }))] + if (g.length > 0) return idConflict('group', args.id, options) } const usable_ns = args.usable_ns diff --git a/lib/group/test/index.js b/lib/group/test/index.js index d39ab54..369c996 100644 --- a/lib/group/test/index.js +++ b/lib/group/test/index.js @@ -17,7 +17,7 @@ after(async () => { describe('group', function () { before(async () => { - await Group.create(testCase) + await Group.create(testCase, { ifExists: 'return' }) }) it('gets group by id', async () => { diff --git a/lib/nameserver/store/base.js b/lib/nameserver/store/base.js index d10d33b..ae2fd83 100644 --- a/lib/nameserver/store/base.js +++ b/lib/nameserver/store/base.js @@ -6,7 +6,7 @@ * * Repo contract: * get(args) → object[] - * create(args) → number (zoneId) + * create(args, options) → number (zoneId) * put(args) → boolean * delete(args) → boolean * destroy(args) → boolean @@ -24,7 +24,7 @@ class NameserverBase { throw new Error('get() not implemented by this repo') } - async create(_args) { + async create(_args, _options) { throw new Error('create() not implemented by this repo') } diff --git a/lib/nameserver/store/file.js b/lib/nameserver/store/file.js index 587a125..40c16b0 100644 --- a/lib/nameserver/store/file.js +++ b/lib/nameserver/store/file.js @@ -1,4 +1,5 @@ import FileStore from '../../store/file.js' +import { idConflict } from '../../store/error.js' import NameserverBase from './base.js' @@ -38,10 +39,13 @@ class NameserverRepoFile extends NameserverBase { return r } - async create(args) { - if (args.id) { - const existing = await this.get({ id: args.id }) - if (existing.length === 1) return existing[0].id + 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('nameserver', args.id, options) } const nameservers = await this._load() diff --git a/lib/nameserver/store/mysql.js b/lib/nameserver/store/mysql.js index 8ef94ad..1246c97 100644 --- a/lib/nameserver/store/mysql.js +++ b/lib/nameserver/store/mysql.js @@ -1,5 +1,6 @@ import Mysql from '../../mysql.js' import NameserverBase from './base.js' +import { idConflict } from '../../store/error.js' import { mapToDbColumn } from '../../util.js' const nsDbMap = { id: 'nt_nameserver_id', gid: 'nt_group_id' } @@ -15,10 +16,10 @@ class Nameserver extends NameserverBase { this.mysql = Mysql } - async create(args) { - if (args.id) { - const g = await this.get({ id: args.id }) - if (g.length === 1) return g[0].id + async create(args, options) { + if (args.id !== undefined) { + const g = [...(await this.get({ id: args.id })), ...(await this.get({ id: args.id, deleted: true }))] + if (g.length > 0) return idConflict('nameserver', args.id, options) } args = await resolveType(args) diff --git a/lib/permission/store/base.js b/lib/permission/store/base.js index ac59a10..94a2769 100644 --- a/lib/permission/store/base.js +++ b/lib/permission/store/base.js @@ -5,7 +5,7 @@ * repository classes must extend this class and implement the repo contract. * * Repo contract: - * create(args) → id + * create(args, options) → id * get(args) → object | undefined * getGroup(args) → object | undefined * put(args) → boolean @@ -17,7 +17,7 @@ class PermissionBase { this.debug = args?.debug ?? false } - async create(_args) { + async create(_args, _options) { throw new Error('create() not implemented by this store') } diff --git a/lib/permission/store/file.js b/lib/permission/store/file.js index 15e7cdd..44fc215 100644 --- a/lib/permission/store/file.js +++ b/lib/permission/store/file.js @@ -1,4 +1,5 @@ import FileStore from '../../store/file.js' +import { idConflict } from '../../store/error.js' import PermissionBase from './base.js' @@ -73,28 +74,52 @@ class PermissionRepoFile extends PermissionBase { // CRUD // --------------------------------------------------------------------------- - async create(args) { + async create(args, options) { args = JSON.parse(JSON.stringify(args)) const uid = args.uid ?? args.user?.id const gid = args.gid ?? args.group?.id delete args.uid delete args.gid + if (args.id !== undefined) { + const [users, groups, permissions] = await Promise.all([ + this._loadUsers(), + this._loadGroups(), + this._loadStandalone(), + ]) + const existing = [ + ...users.map((user) => ({ permission: user.permissions, uid: user.id, gid: user.gid })), + ...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, + })), + ].find((entry) => entry.permission?.id === args.id) + if (existing) { + const sameTarget = + uid !== undefined + ? existing.uid === uid + : gid !== undefined + ? existing.uid === undefined && existing.gid === gid + : true + return idConflict('permission', args.id, sameTarget ? options : undefined) + } + } + if (uid !== undefined) { const users = await this._loadUsers() const idx = users.findIndex((u) => u.id === uid) if (idx !== -1) { - // Store inline in user.toml using the actual permission data from args - if (!users[idx].permissions) { - const perm = JSON.parse(JSON.stringify(args)) - perm.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 - } + if (users[idx].permissions) return idConflict('permission', users[idx].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 } @@ -107,14 +132,12 @@ class PermissionRepoFile extends PermissionBase { const idx = groups.findIndex((g) => g.id === gid) if (idx !== -1) { - // Store inline in group.toml - if (!groups[idx].permissions) { - const perm = JSON.parse(JSON.stringify(args)) - perm.id = gid - if (!perm.group) perm.group = {} - perm.group.id = gid - groups[idx].permissions = perm - } + if (groups[idx].permissions) return idConflict('permission', groups[idx].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 } @@ -127,13 +150,14 @@ class PermissionRepoFile extends PermissionBase { if (permId === undefined) return undefined const perms = await this._loadStandalone() - if (!perms.find((p) => p.id === permId)) { - const perm = { ...args, id: permId } - if (uid !== undefined) perm.uid = uid - if (gid !== undefined) perm.gid = gid - perms.push(perm) - await this._saveStandalone(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) + await this._saveStandalone(perms) return permId } diff --git a/lib/permission/store/mysql.js b/lib/permission/store/mysql.js index 3f673c2..92b0350 100644 --- a/lib/permission/store/mysql.js +++ b/lib/permission/store/mysql.js @@ -1,4 +1,5 @@ import Mysql from '../../mysql.js' +import { idConflict } from '../../store/error.js' import { mapToDbColumn } from '../../util.js' import PermissionBase from './base.js' @@ -16,22 +17,38 @@ class PermissionRepoMySQL extends PermissionBase { this.mysql = Mysql } - async create(args) { - if (args.id) { - const p = await this.get({ id: args.id }) - if (p) return p.id + async create(args, options) { + args = objectToDb(args) + + if (args.id !== undefined) { + const p = (await this.get({ id: args.id })) ?? (await this.get({ id: args.id, deleted: true })) + if (p) { + // The fixture opt-out covers re-creating the same permission, not another + // target that happens to hold this id. + const sameTarget = + args.uid !== undefined + ? p.user?.id === args.uid + : args.gid !== undefined + ? p.user?.id === undefined && p.group?.id === args.gid + : true + return idConflict('permission', args.id, sameTarget ? options : undefined) + } } - // Deduplicate group-level permission rows (uid IS NULL) to prevent accumulation - if (args.gid !== undefined && args.uid === undefined) { - const rows = await Mysql.execute( - `SELECT nt_perm_id FROM nt_perm WHERE nt_group_id = ? AND nt_user_id IS NULL LIMIT 1`, + // nt_perm carries at most one row per user and one per group; a second makes + // every later read of that permission ambiguous. + let rows = [] + if (args.uid !== undefined) { + rows = await Mysql.execute('SELECT nt_perm_id FROM nt_perm WHERE nt_user_id = ? LIMIT 1', [args.uid]) + } else if (args.gid !== undefined) { + rows = await Mysql.execute( + 'SELECT nt_perm_id FROM nt_perm WHERE nt_group_id = ? AND nt_user_id IS NULL LIMIT 1', [args.gid], ) - if (rows.length > 0) return rows[0].nt_perm_id } + if (rows.length > 0) return idConflict('permission', rows[0].nt_perm_id, options) - return await Mysql.execute(...Mysql.insert(`nt_perm`, mapToDbColumn(objectToDb(args), permDbMap))) + return await Mysql.execute(...Mysql.insert(`nt_perm`, mapToDbColumn(args, permDbMap))) } async get(args) { diff --git a/lib/permission/test/index.js b/lib/permission/test/index.js index 45dd96f..2797219 100644 --- a/lib/permission/test/index.js +++ b/lib/permission/test/index.js @@ -10,8 +10,8 @@ import userTestCase from '../../user/test/user.json' with { type: 'json' } import permTestCase from './permission.json' with { type: 'json' } before(async () => { - await Group.create(groupTestCase) - await User.create(userTestCase) + await Group.create(groupTestCase, { ifExists: 'return' }) + await User.create(userTestCase, { ifExists: 'return' }) }) after(async () => { @@ -20,7 +20,7 @@ after(async () => { describe('permission', function () { it('creates a permission', async () => { - assert.ok(await Permission.create(permTestCase)) + assert.ok(await Permission.create(permTestCase, { ifExists: 'return' })) }) it('get: by id', async () => { diff --git a/lib/permission/test/permission.json b/lib/permission/test/permission.json index dc09a85..754a102 100644 --- a/lib/permission/test/permission.json +++ b/lib/permission/test/permission.json @@ -1,5 +1,5 @@ { - "id": 4096, + "id": 63096, "inherit": true, "name": "Test Permission", "self_write": false, diff --git a/lib/session/test/index.js b/lib/session/test/index.js index baeed87..db09f60 100644 --- a/lib/session/test/index.js +++ b/lib/session/test/index.js @@ -13,7 +13,7 @@ const sessionUser = { } before(async () => { - await User.create(sessionUser) + await User.create(sessionUser, { ifExists: 'return' }) }) after(async () => { diff --git a/lib/store/create_conflict.test.js b/lib/store/create_conflict.test.js new file mode 100644 index 0000000..0b3d811 --- /dev/null +++ b/lib/store/create_conflict.test.js @@ -0,0 +1,129 @@ +import assert from 'node:assert/strict' +import { after, before, describe, it } from 'node:test' + +import Group from '../group/index.js' +import Nameserver from '../nameserver/index.js' +import Permission from '../permission/index.js' +import User from '../user/index.js' +import Zone from '../zone/index.js' +import ZoneRecord from '../zone_record/index.js' + +const group = { id: 62000, parent_gid: 0, name: 'conflict-group' } +const nameserver = { + id: 62001, + gid: group.id, + name: 'conflict.ns.example.com.', + address: '203.0.113.1', + ttl: 3600, + type: 'nsd', +} +const user = { + id: 62002, + gid: group.id, + username: 'conflict-user', + email: 'conflict@example.com', + password: 'Tw0-G00d#Passwords', + first_name: 'Conflict', + last_name: 'User', +} +const zone = { + id: 62003, + gid: group.id, + zone: 'conflict.example.com.', + mailaddr: 'hostmaster.conflict.example.com.', + serial: 2026082901, + refresh: 3600, + retry: 600, + expire: 86400, + minimum: 300, + ttl: 300, +} +const zoneRecord = { + id: 62004, + zid: zone.id, + owner: 'www.conflict.example.com.', + ttl: 300, + type: 'A', + address: '203.0.113.2', +} +const permission = { + id: 62005, + name: 'conflict permission', + group: { id: 62999 }, + user: { id: 62999 }, +} + +const cases = [ + ['group', Group, group, 'name', 'changed-group'], + ['nameserver', Nameserver, nameserver, 'name', 'changed.ns.example.com.'], + ['user', User, user, 'username', 'changed-user'], + ['zone', Zone, zone, 'zone', 'changed.example.com.'], + ['zone record', ZoneRecord, zoneRecord, 'owner', 'changed.conflict.example.com.'], + ['permission', Permission, permission, 'name', 'changed permission'], +] + +let userPermissionId + +before(async () => { + await Permission.destroy({ id: permission.id }) + await ZoneRecord.destroy({ id: zoneRecord.id }) + await Zone.destroy({ id: zone.id }) + await User.destroy({ id: user.id }) + await Nameserver.destroy({ id: nameserver.id }) + await Group.destroy({ id: group.id }) + + for (const [, store, original] of cases) await store.create(structuredClone(original)) +}) + +after(async () => { + if (userPermissionId !== undefined) await Permission.destroy({ id: userPermissionId }) + await Permission.destroy({ id: permission.id }) + await ZoneRecord.destroy({ id: zoneRecord.id }) + await Zone.destroy({ id: zone.id }) + await User.destroy({ id: user.id }) + await Nameserver.destroy({ id: nameserver.id }) + await Group.destroy({ id: group.id }) + await Group.disconnect() +}) + +describe('create id conflicts', () => { + for (const [entity, store, original, field, changed] of cases) { + it(`${entity} rejects the collision and leaves the existing row untouched`, async () => { + await assert.rejects(store.create({ ...structuredClone(original), [field]: changed }), /already exists/) + + const result = await store.get({ id: original.id }) + const existing = Array.isArray(result) ? result[0] : result + assert.equal(existing[field], original[field]) + }) + } + + it('permission rejects a second create for the same user', async () => { + const original = { + name: 'user permission', + user: { id: user.id }, + group: { id: group.id }, + } + userPermissionId = await Permission.create(structuredClone(original)) + + await assert.rejects( + Permission.create({ ...structuredClone(original), name: 'changed user permission' }), + /already exists/, + ) + + const existing = await Permission.get({ id: userPermissionId }) + assert.equal(existing.name, original.name) + }) + + it('rejects an id collision with a deleted row', async () => { + await ZoneRecord.delete({ id: zoneRecord.id }) + + await assert.rejects( + ZoneRecord.create({ ...structuredClone(zoneRecord), owner: 'changed.conflict.example.com.' }), + /already exists/, + ) + + const [existing] = await ZoneRecord.get({ id: zoneRecord.id, deleted: true }) + assert.equal(existing.owner, zoneRecord.owner) + await ZoneRecord.delete({ id: zoneRecord.id, deleted: false }) + }) +}) diff --git a/lib/store/error.js b/lib/store/error.js new file mode 100644 index 0000000..bdd3339 --- /dev/null +++ b/lib/store/error.js @@ -0,0 +1,14 @@ +export class StoreConflictError extends Error { + constructor(entity, id) { + super(`${entity} id ${id} already exists`) + this.name = 'StoreConflictError' + this.code = 'STORE_CONFLICT' + } +} + +// 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 = {}) { + if (options.ifExists === 'return') return id + throw new StoreConflictError(entity, id) +} diff --git a/lib/user/store/base.js b/lib/user/store/base.js index 2190215..7a752a2 100644 --- a/lib/user/store/base.js +++ b/lib/user/store/base.js @@ -8,7 +8,7 @@ * Repo contract: * authenticate(authTry) → { user, group } | undefined * get(args) → object[] - * create(args) → number (userId) + * create(args, options) → number (userId) * put(args) → boolean * delete(args) → boolean * destroy(args) → boolean @@ -34,7 +34,7 @@ class UserBase { throw new Error('get() not implemented by this repo') } - async create(_args) { + async create(_args, _options) { throw new Error('create() not implemented by this repo') } diff --git a/lib/user/store/file.js b/lib/user/store/file.js index 4ee232e..ec31b84 100644 --- a/lib/user/store/file.js +++ b/lib/user/store/file.js @@ -1,4 +1,5 @@ import FileStore from '../../store/file.js' +import { idConflict } from '../../store/error.js' import Config from '../../config.js' import Credentials from '../credentials.js' import UserBase from './base.js' @@ -130,10 +131,13 @@ class UserRepoFile extends UserBase { return users.length } - async create(args) { - if (args.id) { - const existing = await this.get({ id: args.id }) - if (existing.length === 1) return existing[0].id + 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)) diff --git a/lib/user/store/mysql.js b/lib/user/store/mysql.js index 85b55e6..aa1384e 100644 --- a/lib/user/store/mysql.js +++ b/lib/user/store/mysql.js @@ -3,6 +3,7 @@ import Config from '../../config.js' import Credentials from '../credentials.js' import UserBase from './base.js' import Permission from '../../permission/index.js' +import { idConflict } from '../../store/error.js' import { mapToDbColumn } from '../../util.js' const userDbMap = { id: 'nt_user_id', gid: 'nt_group_id' } @@ -71,9 +72,11 @@ class UserRepoMySQL extends UserBase { } } - async create(args) { - const u = await this.get({ id: args.id, gid: args.gid }) - if (u.length === 1) return u[0].id + async create(args, options) { + if (args.id !== undefined) { + const u = [...(await this.get({ id: args.id })), ...(await this.get({ id: args.id, deleted: true }))] + if (u.length > 0) return idConflict('user', args.id, options) + } args = JSON.parse(JSON.stringify(args)) diff --git a/lib/user/test/index.js b/lib/user/test/index.js index 4a25f52..4b5cbb1 100644 --- a/lib/user/test/index.js +++ b/lib/user/test/index.js @@ -21,7 +21,7 @@ const userCase = { const groupCase = { ...groupJson, id: 4085, name: 'usertest.example.com' } before(async () => { - await Group.create(groupCase) + await Group.create(groupCase, { ifExists: 'return' }) }) after(async () => { @@ -49,7 +49,7 @@ function sanitizeActual(u) { describe('user', function () { describe('POST', function () { it('creates a user', async () => { - assert.ok(await User.create(userCase)) + assert.ok(await User.create(userCase, { ifExists: 'return' })) let users = await User.get({ id: userCase.id }) assert.deepEqual(sanitizeActual(users[0]), sanitize(userCase)) assert.ok(users[0].permissions, 'user has permissions') diff --git a/lib/user/test/mysql.js b/lib/user/test/mysql.js index a52cc75..e0254ad 100644 --- a/lib/user/test/mysql.js +++ b/lib/user/test/mysql.js @@ -28,7 +28,7 @@ const authCreds = { const SELF_DESCRIBING = /^\d+\$[0-9a-f]{64}$/ before(async () => { - await Group.create(groupCase) + await Group.create(groupCase, { ifExists: 'return' }) }) after(async () => { diff --git a/lib/zone/store/base.js b/lib/zone/store/base.js index f65a539..4ac3e1f 100644 --- a/lib/zone/store/base.js +++ b/lib/zone/store/base.js @@ -4,7 +4,7 @@ * All zone repository classes must extend this class and implement: * get(args) → object[] * count(args) → number - * create(args) → number (zoneId) + * create(args, options) → number (zoneId) * put(args) → boolean * delete(args) → boolean * destroy(args) → boolean @@ -22,7 +22,7 @@ class ZoneBase { throw new Error('count() not implemented by this repo') } - async create(_args) { + async create(_args, _options) { throw new Error('create() not implemented by this repo') } diff --git a/lib/zone/store/file.js b/lib/zone/store/file.js index 2db3eed..28be839 100644 --- a/lib/zone/store/file.js +++ b/lib/zone/store/file.js @@ -1,4 +1,5 @@ import FileStore, { resolveCodec } from '../../store/file.js' +import { idConflict } from '../../store/error.js' import ZoneBase from './base.js' @@ -34,10 +35,13 @@ class ZoneRepoFile extends ZoneBase { return r } - async create(args) { - if (args.id) { - const existing = await this.get({ id: args.id }) - if (existing.length === 1) return existing[0].id + 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('zone', args.id, options) } const zones = await this._load() diff --git a/lib/zone/store/mysql.js b/lib/zone/store/mysql.js index d6a1b78..a9141fa 100644 --- a/lib/zone/store/mysql.js +++ b/lib/zone/store/mysql.js @@ -1,5 +1,6 @@ import Mysql from '../../mysql.js' import ZoneBase from './base.js' +import { idConflict } from '../../store/error.js' import { mapToDbColumn } from '../../util.js' const zoneDbMap = { id: 'nt_zone_id', gid: 'nt_group_id' } @@ -50,10 +51,10 @@ class ZoneRepoMySQL extends ZoneBase { this.mysql = Mysql } - async create(args) { - if (args.id) { - const g = await this.get({ id: args.id }) - if (g.length === 1) return g[0].id + async create(args, options) { + if (args.id !== undefined) { + const g = [...(await this.get({ id: args.id })), ...(await this.get({ id: args.id, deleted: true }))] + if (g.length > 0) return idConflict('zone', args.id, options) } return await Mysql.execute(...Mysql.insert(`nt_zone`, mapToDbColumn(args, zoneDbMap))) diff --git a/lib/zone_record/store/base.js b/lib/zone_record/store/base.js index 4afc5e6..2d9e2fc 100644 --- a/lib/zone_record/store/base.js +++ b/lib/zone_record/store/base.js @@ -4,7 +4,7 @@ * All zone repository classes must extend this class and implement: * get(args) → object[] * count(args) → number - * create(args) → number (zoneId) + * create(args, options) → number (zoneId) * put(args) → boolean * delete(args) → boolean * destroy(args) → boolean @@ -22,7 +22,7 @@ class ZoneRecordBase { throw new Error('count() not implemented by this repo') } - async create(_args) { + async create(_args, _options) { throw new Error('create() not implemented by this repo') } diff --git a/lib/zone_record/store/file.js b/lib/zone_record/store/file.js index 4ff5651..e05d797 100644 --- a/lib/zone_record/store/file.js +++ b/lib/zone_record/store/file.js @@ -1,4 +1,5 @@ import FileStore from '../../store/file.js' +import { idConflict } from '../../store/error.js' import ZoneRecordBase from './base.js' @@ -16,12 +17,15 @@ class ZoneRecordRepoFile extends ZoneRecordBase { return this.file.save('zone_record', records) } - async create(args) { + async create(args, options) { args = JSON.parse(JSON.stringify(args)) - if (args.id) { - const existing = await this.get({ id: args.id }) - if (existing.length === 1) return existing[0].id + 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() diff --git a/lib/zone_record/store/mysql.js b/lib/zone_record/store/mysql.js index 3b4ed24..2732d2e 100644 --- a/lib/zone_record/store/mysql.js +++ b/lib/zone_record/store/mysql.js @@ -3,6 +3,7 @@ import { applyMap, getMap, unApplyMap } from '@nictool/dns-resource-record' import ZoneRecordBase from './base.js' import Mysql from '../../mysql.js' +import { idConflict } from '../../store/error.js' import { mapToDbColumn } from '../../util.js' const zrDbMap = { id: 'nt_zone_record_id', zid: 'nt_zone_id', owner: 'name' } @@ -26,10 +27,10 @@ class ZoneRecordMySQL extends ZoneRecordBase { this.mysql = Mysql } - async create(args) { - if (args.id) { - const g = await this.get({ id: args.id }) - if (g.length === 1) return g[0].id + async create(args, options) { + if (args.id !== undefined) { + const g = [...(await this.get({ id: args.id })), ...(await this.get({ id: args.id, deleted: true }))] + if (g.length > 0) return idConflict('zone record', args.id, options) } const rrArgs = args.ttl === undefined ? { ...args, default: { ttl: 0 } } : args diff --git a/routes/group.test.js b/routes/group.test.js index 8b83199..8945093 100644 --- a/routes/group.test.js +++ b/routes/group.test.js @@ -13,8 +13,8 @@ const case2Id = 4094 before(async () => { server = await init() - await Group.create(groupCase) - await User.create(userCase) + await Group.create(groupCase, { ifExists: 'return' }) + await User.create(userCase, { ifExists: 'return' }) }) after(async () => { diff --git a/routes/index.js b/routes/index.js index 598f092..89b4887 100644 --- a/routes/index.js +++ b/routes/index.js @@ -14,6 +14,7 @@ import HapiSwagger from '@msimerson/hapi-openapi' import qs from 'qs' import Config from '../lib/config.js' +import { StoreConflictError } from '../lib/store/error.js' import pkgJson from '../package.json' with { type: 'json' } @@ -105,6 +106,21 @@ async function setup() { server.auth.default('nt_jwt_strategy') + server.ext('onPreResponse', (request, h) => { + const response = request.response + if (!(response instanceof StoreConflictError) && response?.code !== 'STORE_CONFLICT') { + return h.continue + } + + return h + .response({ + statusCode: 409, + error: 'Conflict', + message: response.message, + }) + .code(409) + }) + server.route({ method: 'GET', path: '/', diff --git a/routes/nameserver.test.js b/routes/nameserver.test.js index ef80f95..77d3e6e 100644 --- a/routes/nameserver.test.js +++ b/routes/nameserver.test.js @@ -15,9 +15,9 @@ let case2Id = 4094 before(async () => { await Nameserver.destroy({ id: case2Id }) - await Group.create(groupCase) - await User.create(userCase) - await Nameserver.create(nsCase) + await Group.create(groupCase, { ifExists: 'return' }) + await User.create(userCase, { ifExists: 'return' }) + await Nameserver.create(nsCase, { ifExists: 'return' }) server = await init() }) diff --git a/routes/permission.test.js b/routes/permission.test.js index 18089f4..39069b8 100644 --- a/routes/permission.test.js +++ b/routes/permission.test.js @@ -11,17 +11,18 @@ import userCase from './test/user.json' with { type: 'json' } import permCase from './test/permission.json' with { type: 'json' } let server -let case2Id = 4094 +let case2Id +const targetId = 63094 before(async () => { server = await init() - await Group.create(groupCase) - await User.create(userCase) - await Permission.create(permCase) + await Group.create(groupCase, { ifExists: 'return' }) + await User.create(userCase, { ifExists: 'return' }) + await Permission.create(permCase, { ifExists: 'return' }) }) after(async () => { - Permission.destroy({ id: case2Id }) + if (case2Id !== undefined) await Permission.destroy({ id: case2Id }) await server.stop() }) @@ -41,10 +42,10 @@ describe('permission routes', () => { auth.headers = { Authorization: `Bearer ${res.result.session.token}` } }) - it(`GET /permission/${userCase.id}`, async () => { + it(`GET /permission/${permCase.id}`, async () => { const res = await server.inject({ method: 'GET', - url: `/permission/${userCase.id}`, + url: `/permission/${permCase.id}`, headers: auth.headers, }) assert.equal(res.statusCode, 200) @@ -52,14 +53,13 @@ describe('permission routes', () => { assert.equal(res.result.permission.nameserver.create, false) }) - it(`POST /permission (${case2Id})`, async () => { + it('POST /permission', async () => { const testCase = JSON.parse(JSON.stringify(permCase)) - testCase.id = case2Id // make it unique - testCase.user.id = case2Id - testCase.group.id = case2Id + delete testCase.id + testCase.user.id = targetId + testCase.group.id = targetId testCase.name = `Route Test Permission 2` delete testCase.deleted - // console.log(testCase) const res = await server.inject({ method: 'POST', @@ -67,13 +67,34 @@ describe('permission routes', () => { headers: auth.headers, payload: testCase, }) - // console.log(res.result) assert.equal(res.statusCode, 201) + case2Id = res.result.permission.id assert.equal(res.result.permission.zone.create, true) assert.equal(res.result.permission.nameserver.create, false) }) - it(`GET /permission/${case2Id}`, async () => { + it('POST /permission rejects an existing target', async () => { + const testCase = JSON.parse(JSON.stringify(permCase)) + delete testCase.id + testCase.user.id = targetId + testCase.group.id = targetId + testCase.name = 'Changed Route Test Permission' + delete testCase.deleted + + const res = await server.inject({ + method: 'POST', + url: '/permission', + headers: auth.headers, + payload: testCase, + }) + assert.equal(res.statusCode, 409) + assert.match(res.result.message, /permission id .* already exists/) + + const existing = await Permission.get({ id: case2Id }) + assert.equal(existing.name, 'Route Test Permission 2') + }) + + it('GET the created permission', async () => { const res = await server.inject({ method: 'GET', url: `/permission/${case2Id}`, @@ -85,7 +106,7 @@ describe('permission routes', () => { assert.equal(res.result.permission.nameserver.create, false) }) - it(`DELETE /permission/${case2Id}`, async () => { + it('DELETE the created permission', async () => { const res = await server.inject({ method: 'DELETE', url: `/permission/${case2Id}`, @@ -95,7 +116,7 @@ describe('permission routes', () => { assert.equal(res.statusCode, 200) }) - it(`DELETE /permission/${case2Id}`, async () => { + it('DELETE the created permission again', async () => { const res = await server.inject({ method: 'DELETE', url: `/permission/${case2Id}`, @@ -105,7 +126,7 @@ describe('permission routes', () => { assert.equal(res.statusCode, 404) }) - it(`GET /permission/${case2Id}`, async () => { + it('GET the deleted permission', async () => { const res = await server.inject({ method: 'GET', url: `/permission/${case2Id}`, @@ -116,7 +137,7 @@ describe('permission routes', () => { assert.equal(res.result.permission, undefined) }) - it(`GET /permission/${case2Id} (deleted)`, async () => { + it('GET the deleted permission with deleted=true', async () => { const res = await server.inject({ method: 'GET', url: `/permission/${case2Id}?deleted=true`, diff --git a/routes/session.test.js b/routes/session.test.js index a49f1ee..8c1ffc2 100644 --- a/routes/session.test.js +++ b/routes/session.test.js @@ -13,9 +13,9 @@ import Permission from '../lib/permission/index.js' let server before(async () => { - await Group.create(groupCase) - await User.create(userCase) - await Permission.create(permCase) + await Group.create(groupCase, { ifExists: 'return' }) + await User.create(userCase, { ifExists: 'return' }) + await Permission.create(permCase, { ifExists: 'return' }) server = await init() }) diff --git a/routes/test/permission.json b/routes/test/permission.json index e9a58bc..bdd35e5 100644 --- a/routes/test/permission.json +++ b/routes/test/permission.json @@ -1,5 +1,5 @@ { - "id": 4095, + "id": 63095, "inherit": true, "name": "Test Permission", "self_write": false, diff --git a/routes/user.test.js b/routes/user.test.js index da88079..fa97de4 100644 --- a/routes/user.test.js +++ b/routes/user.test.js @@ -13,8 +13,8 @@ let server, before(async () => { server = await init() - await Group.create(groupCase) - await User.create(userCase) + await Group.create(groupCase, { ifExists: 'return' }) + await User.create(userCase, { ifExists: 'return' }) }) const userId2 = 4094 diff --git a/routes/zone.test.js b/routes/zone.test.js index 86c3eb7..8c70cff 100644 --- a/routes/zone.test.js +++ b/routes/zone.test.js @@ -24,9 +24,9 @@ before(async () => { // Group.create early-return and skip addToSubgroups, leaving the // nt_group_subgroups closure row (and thus the include_subgroups query) empty. await Group.destroy({ id: subGroup.id }) - await Group.create(groupCase) - await User.create(userCase) - await Zone.create(nsCase) + await Group.create(groupCase, { ifExists: 'return' }) + await User.create(userCase, { ifExists: 'return' }) + await Zone.create(nsCase, { ifExists: 'return' }) await Group.create(subGroup) await Zone.create(subZone) server = await init() diff --git a/routes/zone_record.test.js b/routes/zone_record.test.js index 5e96845..6445ac2 100644 --- a/routes/zone_record.test.js +++ b/routes/zone_record.test.js @@ -47,8 +47,8 @@ before(async () => { username: `route-zr-delete-${testGroupId}`, } - await Group.create(testGroup) - await User.create(testUser) + await Group.create(testGroup, { ifExists: 'return' }) + await User.create(testUser, { ifExists: 'return' }) await Zone.create(testZone) await ZoneRecord.create(testZoneRecord) @@ -82,6 +82,24 @@ 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', diff --git a/test/fixtures.js b/test/fixtures.js index 5e8b4e1..788be8e 100644 --- a/test/fixtures.js +++ b/test/fixtures.js @@ -31,14 +31,12 @@ switch (process.argv[2]) { } async function setup() { - await Group.create(groupCase) - await Group.create(groupCaseR) - await User.create(userCase) - await User.create(userCaseR) - // Seed the shared route permission so the permission and session suites, - // which both create it in their before hooks, early-return instead of racing - // two concurrent INSERTs of the same explicit id. - await Permission.create(permCaseR) + const fixture = { ifExists: 'return' } + await Group.create(groupCase, fixture) + await Group.create(groupCaseR, fixture) + await User.create(userCase, fixture) + await User.create(userCaseR, fixture) + await Permission.create(permCaseR, fixture) // await createTestSession() await Permission.disconnect() await User.disconnect()