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..b0684f8 100644 --- a/lib/store/file.test.js +++ b/lib/store/file.test.js @@ -268,6 +268,41 @@ 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(`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 f0b6318..bc2aafb 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, + id: users[idx].permissions?.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..3830045 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,62 @@ 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('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) + 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]) + }) })