From 2bb64b006ebcfa7be097c024fc275d5b57564460 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Fri, 16 Oct 2026 05:51:47 +0100 Subject: [PATCH 1/3] user: moving a user moves its permission rows A user's explicit permission row stayed in the old group after a move, so Permission.get({ uid }) reported a group the user had left. On mysql the nt_user and nt_perm updates share a transaction and every one of the user's rows moves, deleted ones too. The file stores move the inline permission with the user. An inheritance change in the same PUT runs in that transaction under a lock on the user's row, so a failure rolls back the move as well. Going explicit after a revoke gives the user fresh permissions on every store instead of a create conflict or the revoked grants. --- CHANGELOG.md | 1 + lib/store/file.test.js | 17 +++++++++ lib/user/store/file.js | 9 +++-- lib/user/store/mysql.js | 78 +++++++++++++++++++++++++++-------------- lib/user/test/index.js | 15 ++++++++ lib/user/test/mysql.js | 46 ++++++++++++++++++++++++ 6 files changed, 137 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bdcb612..a6d2da2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - fix(sql): store existing zone names lowercase without the trailing dot - fix: one random, self describing session row per login, expired ones swept at login, and mysql throttles the activity touch - fix: a token is valid only while its session row exists, so logout revokes it, and any authenticated request counts as activity +- fix: moving a user moves their permission rows to the new group ### [3.0.3] - 2026-07-27 diff --git a/lib/store/file.test.js b/lib/store/file.test.js index 011de9c..5eb659a 100644 --- a/lib/store/file.test.js +++ b/lib/store/file.test.js @@ -268,6 +268,23 @@ describe('file store', () => { } } + it(`gives a ${type} user fresh permissions when it goes explicit after a revoke`, async () => { + process.env.NICTOOL_DATA_STORE = type + fs.rmSync(path.join(tmp, `user.${type}`), { force: true }) + const users = new UserRepoFile() + const permissions = new PermissionRepoFile() + const uid = await users.create({ gid: 1, username: 'again', email: 'again@example.com' }) + assert.ok(await users.put({ id: uid, inherit_group_permissions: false })) + const { id } = await permissions.get({ uid }) + assert.ok(await permissions.put({ id, zone: { delete: true } })) + assert.ok(await permissions.delete({ id })) + + assert.ok(await users.put({ id: uid, inherit_group_permissions: false })) + const perm = await permissions.get({ uid }) + assert.equal(perm.inherit, false) + assert.equal(perm.zone.delete, false) + }) + it(`leaves a ${type} group's permission alone once a user's with its id is gone`, async () => { process.env.NICTOOL_DATA_STORE = type fs.rmSync(path.join(tmp, `permission.${type}`), { force: true }) diff --git a/lib/user/store/file.js b/lib/user/store/file.js index f0b6318..23604fc 100644 --- a/lib/user/store/file.js +++ b/lib/user/store/file.js @@ -178,17 +178,20 @@ class UserRepoFile extends UserBase { if (inherit === true) { delete users[idx].permissions - } else if (inherit === false && !users[idx].permissions) { + } else if (inherit === false && (!users[idx].permissions || users[idx].permissions.deleted)) { 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 }, + group: { id: args.gid ?? users[idx].gid, create: false, write: false, delete: false }, } - } else if (inherit === false && users[idx].permissions) { + } else if (inherit === false) { users[idx].permissions.inherit = false } + if (args.gid !== undefined && users[idx].permissions?.group) { + users[idx].permissions.group.id = args.gid + } users[idx] = { ...users[idx], ...args } return true }) diff --git a/lib/user/store/mysql.js b/lib/user/store/mysql.js index b40b6aa..24a0c58 100644 --- a/lib/user/store/mysql.js +++ b/lib/user/store/mysql.js @@ -213,35 +213,34 @@ class UserRepoMySQL extends UserBase { const id = args.id delete args.id - // Explicit Permission Management - if (args.inherit_group_permissions !== undefined) { - const inherit = args.inherit_group_permissions - delete args.inherit_group_permissions - - const userPerm = await Permission.get({ uid: id }) - if (inherit === true && userPerm) { - // Switch to inherited: delete explicit perms - await Permission.destroy({ id: userPerm.id }) - } else if (inherit === false && !userPerm) { - // Switch to explicit: create nt_perm entry - const [userData] = await this.get({ id }) - await Permission.create({ - uid: id, - inherit: false, - name: `User ${userData.username} perms`, - }) - } else if (inherit === false && userPerm) { - // Stay explicit, ensure inherit is false - await Permission.put({ id: userPerm.id, inherit: false }) - } + const inherit = args.inherit_group_permissions + delete args.inherit_group_permissions + + const update = Mysql.update(`nt_user`, { nt_user_id: id }, mapToDbColumn(args, userDbMap)) + if (args.gid === undefined && inherit === undefined) { + if (Object.keys(args).length === 0) return true + return (await Mysql.execute(...update)).changedRows === 1 } - if (Object.keys(args).length === 0) return true + // the user's row lock orders concurrent moves and permission changes + return Mysql.transaction(async (tx) => { + const [user] = await tx.execute( + 'SELECT nt_group_id AS gid, username FROM nt_user WHERE nt_user_id = ? FOR UPDATE', + [id], + ) + if (!user) return false - const r = await Mysql.execute( - ...Mysql.update(`nt_user`, { nt_user_id: id }, mapToDbColumn(args, userDbMap)), - ) - return r.changedRows === 1 + let changed = false + if (Object.keys(args).length > 0) changed = (await tx.execute(...update)).changedRows === 1 + + // a user's permission rows, deleted ones too, live in the user's group + const gid = args.gid ?? user.gid + if (gid !== user.gid) { + await tx.execute('UPDATE nt_perm SET nt_group_id = ? WHERE nt_user_id = ?', [gid, id]) + } + if (inherit !== undefined && (await setInherit(tx, id, gid, user.username, inherit))) changed = true + return changed || Object.keys(args).length === 0 + }) } async delete(args) { @@ -268,3 +267,30 @@ class UserRepoMySQL extends UserBase { } export default UserRepoMySQL + +async function setInherit(tx, uid, gid, username, inherit) { + const [live] = await tx.execute( + 'SELECT nt_perm_id AS id FROM nt_perm WHERE nt_user_id = ? AND deleted = 0', + [uid], + ) + if (inherit === false && live) { + return ( + (await tx.execute('UPDATE nt_perm SET inherit_perm = 0 WHERE nt_perm_id = ?', [live.id])) + .changedRows === 1 + ) + } + + // a soft-deleted row is a revoked one, so it never comes back with its grants + const removed = (await tx.execute('DELETE FROM nt_perm WHERE nt_user_id = ?', [uid])).affectedRows > 0 + if (inherit === true) return removed + + await tx.execute( + ...Mysql.insert(`nt_perm`, { + nt_user_id: uid, + nt_group_id: gid, + inherit_perm: 0, + perm_name: `User ${username} perms`, + }), + ) + return true +} diff --git a/lib/user/test/index.js b/lib/user/test/index.js index 7364232..a473f08 100644 --- a/lib/user/test/index.js +++ b/lib/user/test/index.js @@ -76,6 +76,21 @@ describe('user', function () { assert.equal(users[0].first_name, 'Untie') await User.put({ id: userCase.id, first_name: 'Unit' }) }) + + it('moves an explicit permission with its user', async () => { + const target = { ...groupJson, id: 4084, name: 'usermove.example.com' } + await Group.destroy({ id: target.id }) + await Group.create(target) + try { + await User.put({ id: userCase.id, inherit_group_permissions: false }) + assert.ok(await User.put({ id: userCase.id, gid: target.id })) + const [u] = await User.get({ id: userCase.id }) + assert.equal(u.permissions.group.id, target.id) + } finally { + await User.put({ id: userCase.id, gid: groupCase.id, inherit_group_permissions: true }) + await Group.destroy({ id: target.id }) + } + }) }) describe('DELETE', function () { diff --git a/lib/user/test/mysql.js b/lib/user/test/mysql.js index e0254ad..6c502e7 100644 --- a/lib/user/test/mysql.js +++ b/lib/user/test/mysql.js @@ -4,6 +4,7 @@ import { describe, it, before, after } from 'node:test' import User from '../index.js' import Group from '../../group/index.js' +import Permission from '../../permission/index.js' import Credentials from '../credentials.js' import groupJson from '../../group/test/group.json' with { type: 'json' } @@ -125,4 +126,49 @@ describe('user (mysql)', function () { assert.equal(creds.pass_salt, salt, 'salt should be unchanged') }) }) + + it('rolls back the move when its permission change fails', async (t) => { + await seedUser(testPass, null) + await User.mysql.execute('DELETE FROM nt_perm WHERE nt_user_id = ?', [upgradeUserId]) + t.mock.method(User.mysql, 'insert', () => ['INSERT INTO nt_perm (no_such_column) VALUES(?)', [1]]) + await assert.rejects(User.put({ id: upgradeUserId, gid: 1, inherit_group_permissions: false })) + t.mock.restoreAll() + assert.equal(await Permission.get({ uid: upgradeUserId }), undefined) + assert.equal((await User.get({ id: upgradeUserId }))[0].gid, groupCase.id) + }) + + it('keeps an explicit permission when the move that drops it fails', async (t) => { + await seedUser(testPass, null) + assert.ok(await User.put({ id: upgradeUserId, inherit_group_permissions: false })) + const perm = await Permission.get({ uid: upgradeUserId }) + const update = User.mysql.update.bind(User.mysql) + t.mock.method(User.mysql, 'update', (table, ...rest) => + table === 'nt_user' ? ['UPDATE nt_user SET no_such_column = 1', []] : update(table, ...rest), + ) + await assert.rejects(User.put({ id: upgradeUserId, gid: 1, inherit_group_permissions: true })) + t.mock.restoreAll() + assert.equal((await Permission.get({ uid: upgradeUserId })).id, perm.id) + await User.mysql.execute('DELETE FROM nt_perm WHERE nt_user_id = ?', [upgradeUserId]) + }) + + it('returns false for a missing user without touching permissions', async () => { + await User.destroy({ id: upgradeUserId }) + assert.equal(await User.put({ id: upgradeUserId, gid: 1, inherit_group_permissions: false }), false) + assert.equal(await Permission.get({ uid: upgradeUserId }), undefined) + }) + + it('gives a user fresh permissions when it goes explicit after a revoke', async () => { + await seedUser(testPass, null) + assert.ok(await User.put({ id: upgradeUserId, inherit_group_permissions: false })) + const perm = await Permission.get({ uid: upgradeUserId }) + await Permission.put({ id: perm.id, zone: { delete: true } }) + await Permission.delete({ id: perm.id }) + + assert.ok(await User.put({ id: upgradeUserId, inherit_group_permissions: false })) + const again = await Permission.get({ uid: upgradeUserId }) + assert.equal(again.inherit, false) + assert.equal(again.zone.delete, false) + assert.equal(await Permission.get({ uid: upgradeUserId, deleted: true }), undefined) + await User.mysql.execute('DELETE FROM nt_perm WHERE nt_user_id = ?', [upgradeUserId]) + }) }) From 831e757c3e378c278201472dcf2ddd63154b29c4 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Fri, 16 Oct 2026 06:52:19 +0100 Subject: [PATCH 2/3] test: an explicit user going explicit again keeps its grants --- lib/user/test/mysql.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/lib/user/test/mysql.js b/lib/user/test/mysql.js index 6c502e7..3830045 100644 --- a/lib/user/test/mysql.js +++ b/lib/user/test/mysql.js @@ -151,6 +151,19 @@ describe('user (mysql)', function () { await User.mysql.execute('DELETE FROM nt_perm WHERE nt_user_id = ?', [upgradeUserId]) }) + it('keeps an explicit permission and its grants when it goes explicit again', async () => { + await seedUser(testPass, null) + assert.ok(await User.put({ id: upgradeUserId, inherit_group_permissions: false })) + const perm = await Permission.get({ uid: upgradeUserId }) + await Permission.put({ id: perm.id, zone: { delete: true } }) + + assert.ok(await User.put({ id: upgradeUserId, inherit_group_permissions: false })) + const again = await Permission.get({ uid: upgradeUserId }) + assert.equal(again.id, perm.id) + assert.equal(again.zone.delete, true) + await User.mysql.execute('DELETE FROM nt_perm WHERE nt_user_id = ?', [upgradeUserId]) + }) + it('returns false for a missing user without touching permissions', async () => { await User.destroy({ id: upgradeUserId }) assert.equal(await User.put({ id: upgradeUserId, gid: 1, inherit_group_permissions: false }), false) From 9aeb619d09e5aadca70b741d2ac7ece2a7fdd94f Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Fri, 16 Oct 2026 06:57:28 +0100 Subject: [PATCH 3/3] user: a reset keeps the revoked permission's id --- lib/store/file.test.js | 18 ++++++++++++++++++ lib/user/store/file.js | 2 +- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/store/file.test.js b/lib/store/file.test.js index 5eb659a..b0684f8 100644 --- a/lib/store/file.test.js +++ b/lib/store/file.test.js @@ -285,6 +285,24 @@ describe('file store', () => { assert.equal(perm.zone.delete, false) }) + it(`keeps a ${type} user's permission id when it goes explicit after a revoke`, async () => { + process.env.NICTOOL_DATA_STORE = type + for (const name of ['user', 'group', 'permission']) { + fs.rmSync(path.join(tmp, `${name}.${type}`), { force: true }) + } + const users = new UserRepoFile() + const permissions = new PermissionRepoFile() + const first = await users.create({ gid: 1, username: 'first', email: 'first@example.com' }) + const second = await users.create({ gid: 1, username: 'second', email: 'second@example.com' }) + await permissions.create({ uid: first, id: second, inherit: false }) + const id = await permissions.create({ uid: second, id: second + 1000, inherit: false }) + assert.ok(await permissions.delete({ id })) + + assert.ok(await users.put({ id: second, inherit_group_permissions: false })) + assert.equal((await permissions.get({ uid: second })).id, id) + assert.equal((await permissions.get({ uid: first })).id, second) + }) + it(`leaves a ${type} group's permission alone once a user's with its id is gone`, async () => { process.env.NICTOOL_DATA_STORE = type fs.rmSync(path.join(tmp, `permission.${type}`), { force: true }) diff --git a/lib/user/store/file.js b/lib/user/store/file.js index 23604fc..bc2aafb 100644 --- a/lib/user/store/file.js +++ b/lib/user/store/file.js @@ -181,7 +181,7 @@ class UserRepoFile extends UserBase { } else if (inherit === false && (!users[idx].permissions || users[idx].permissions.deleted)) { users[idx].permissions = { ...JSON.parse(JSON.stringify(defaultPermissions)), - id: users[idx].id, + id: users[idx].permissions?.id ?? users[idx].id, user: { id: users[idx].id, create: false, write: false, delete: false }, group: { id: args.gid ?? users[idx].gid, create: false, write: false, delete: false }, }