diff --git a/lib/group/store/file.js b/lib/group/store/file.js index 43961c9..1d0572c 100644 --- a/lib/group/store/file.js +++ b/lib/group/store/file.js @@ -2,9 +2,12 @@ import FileStore, { claimId, removeRows } from '../../store/file.js' import { idConflict } from '../../store/error.js' import GroupBase from './base.js' +import PermissionRepoFile from '../../permission/store/file.js' import { cascade } from '../../references.js' +const permissions = new PermissionRepoFile() + const defaultPermissions = { inherit: false, self_write: false, @@ -61,7 +64,7 @@ class GroupRepoFile extends GroupBase { const usable_ns = args.usable_ns ?? [] delete args.usable_ns - return this.file.mutate('group', (groups, data, keep) => { + return this.file.mutate('group', async (groups, data, keep) => { if (args.id !== undefined && groups.some((group) => group.id === args.id)) { return keep(idConflict('group', args.id, options)) } @@ -71,7 +74,7 @@ class GroupRepoFile extends GroupBase { const gid = args.id args.permissions = { ...JSON.parse(JSON.stringify(defaultPermissions)), - id: gid, + id: await permissions.allocateId(), name: `Group ${args.name} perms`, user: { id: gid, create: false, write: false, delete: false }, group: { id: gid, create: false, write: false, delete: false }, diff --git a/lib/permission/store/file.js b/lib/permission/store/file.js index 8c68da9..f735e61 100644 --- a/lib/permission/store/file.js +++ b/lib/permission/store/file.js @@ -1,5 +1,5 @@ -import FileStore, { serialize } from '../../store/file.js' -import { idConflict, StoreInvalidError } from '../../store/error.js' +import FileStore, { nextId, serialize } from '../../store/file.js' +import { idConflict, StoreConflictError, StoreInvalidError } from '../../store/error.js' import PermissionBase from './base.js' @@ -12,14 +12,16 @@ 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 by gid only, as their ids collide with users'. + * store. * * 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 → standalone by permissions.id + * get({ id }) → search user → group → standalone by permissions.id + * + * All three draw ids from one sequence, kept as the permission file's last_id. * getGroup({ uid }) → permissions of the group the user belongs to */ class PermissionRepoFile extends PermissionBase { @@ -42,6 +44,21 @@ class PermissionRepoFile extends PermissionBase { return this.standaloneFile.load('permission') } + // mysql's nt_perm has one sequence for user and group permissions alike + async allocateId(id) { + return this.standaloneFile.mutate('permission', (standalone, data) => this._claimId(standalone, data, id)) + } + + async _claimId(standalone, data, id) { + const [users, groups] = await Promise.all([this._loadUsers(), this._loadGroups()]) + const held = [...users, ...groups].map((row) => row.permissions).filter(Boolean) + held.push(...standalone) + if (id === undefined) id = nextId(held, data.last_id) + else if (held.some((perm) => perm.id === id)) throw new StoreConflictError('permission', id) + data.last_id = Math.max(data.last_id ?? 0, id) + return id + } + // --------------------------------------------------------------------------- // Post-processing // --------------------------------------------------------------------------- @@ -103,12 +120,12 @@ class PermissionRepoFile extends PermissionBase { } if (uid !== undefined) { - const created = await this.userFile.mutate('user', (users, data, keep) => { + const created = await this.userFile.mutate('user', async (users, data, keep) => { const user = users.find((u) => u.id === uid) if (!user) return false if (user.permissions) return keep(idConflict('permission', user.permissions.id, options)) const perm = JSON.parse(JSON.stringify(args)) - perm.id = args.id ?? uid + perm.id = await this.allocateId(args.id) if (!perm.user) perm.user = {} perm.user.id = uid if (!perm.group) perm.group = {} @@ -120,12 +137,12 @@ class PermissionRepoFile extends PermissionBase { } if (gid !== undefined && uid === undefined) { - const created = await this.groupFile.mutate('group', (groups, data, keep) => { + const created = await this.groupFile.mutate('group', async (groups, data, keep) => { const group = groups.find((g) => g.id === gid) if (!group) return false if (group.permissions) return keep(idConflict('permission', group.permissions.id, options)) const perm = JSON.parse(JSON.stringify(args)) - perm.id = args.id ?? gid + perm.id = await this.allocateId(args.id) if (!perm.group) perm.group = {} perm.group.id = gid group.permissions = perm @@ -135,12 +152,16 @@ class PermissionRepoFile extends PermissionBase { } // Standalone fallback: neither user nor group record found - const permId = args.id ?? uid ?? gid - - return this.standaloneFile.mutate('permission', (perms, data, keep) => { - if (perms.some((permission) => permission.id === permId)) { - return keep(idConflict('permission', permId, options)) + return this.standaloneFile.mutate('permission', async (perms, data, keep) => { + if (args.id !== undefined && perms.some((permission) => permission.id === args.id)) { + return keep(idConflict('permission', args.id, options)) } + const holder = perms.find((p) => { + const owner = p.uid ?? p.user?.id ?? undefined + return uid !== undefined ? owner === uid : owner === undefined && (p.gid ?? p.group?.id) === gid + }) + if (holder) return keep(idConflict('permission', holder.id, options)) + const permId = await this._claimId(perms, data, args.id) const perm = { ...args, id: permId } if (uid !== undefined) perm.uid = uid if (gid !== undefined) perm.gid = gid @@ -173,12 +194,10 @@ class PermissionRepoFile extends PermissionBase { } if (args.id !== undefined) { - // 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) { - const perm = this._postProcess(user.permissions, deletedArg) + const [users, groups] = await Promise.all([this._loadUsers(), this._loadGroups()]) + const holder = [...users, ...groups].find((row) => row.permissions?.id === args.id) + if (holder) { + const perm = this._postProcess(holder.permissions, deletedArg) if (deletedArg === true && perm.deleted !== true) return undefined return perm } @@ -207,6 +226,7 @@ class PermissionRepoFile extends PermissionBase { if (!group?.permissions) return undefined const deletedArg = args.deleted ?? false + if (Boolean(group.permissions.deleted) !== (deletedArg === true)) return undefined return this._postProcess(group.permissions, deletedArg) } @@ -219,6 +239,13 @@ class PermissionRepoFile extends PermissionBase { return true }) if (changed) return true + const changedGroup = await this.groupFile.mutate('group', (groups) => { + const group = groups.find((g) => g.permissions?.id === id) + if (!group) return false + change(group, 'permissions') + return true + }) + if (changedGroup) return true return this.standaloneFile.mutate('permission', (perms) => { const idx = perms.findIndex((p) => p.id === id) if (idx === -1) return false diff --git a/lib/permission/test/index.js b/lib/permission/test/index.js index 9ca2006..647e151 100644 --- a/lib/permission/test/index.js +++ b/lib/permission/test/index.js @@ -77,3 +77,90 @@ describe('permission', function () { assert.equal(p, undefined) }) }) + +describe('permission ids', () => { + // a user and a group may share an id, but never a permission + const gid = 47301 + const uids = [47301, 47302] + const gids = [gid, 47302] + let groupPerm + let userPerm + + const userCase = (id, inherit) => ({ + id, + gid, + username: `permids${id}`, + email: `permids${id}@example.com`, + password: 'Wh@tA-Decent#P6ssw0rd', + first_name: 'Perm', + last_name: 'Ids', + inherit_group_permissions: inherit, + }) + + async function cleanup() { + for (const uid of uids) { + for (const deleted of [false, true]) { + const p = await Permission.get({ uid, deleted }) + if (p) await Permission.destroy({ id: p.id }) + } + await User.destroy({ id: uid }) + } + for (const g of gids) { + for (const deleted of [false, true]) { + const p = await Permission.get({ gid: g, deleted }) + if (p) await Permission.destroy({ id: p.id }) + } + await Group.destroy({ id: g }) + } + } + + before(async () => { + await cleanup() + await Group.create({ id: gid, parent_gid: 0, name: 'permids.test' }) + await Group.create({ id: gids[1], parent_gid: 0, name: 'permids2.test' }) + await User.create(userCase(uids[0], false)) + groupPerm = await Permission.get({ gid }) + userPerm = await Permission.get({ uid: uids[0] }) + }) + + after(cleanup) + + it('gives a user and a group with the same id different permission ids', () => { + assert.ok(groupPerm, 'the group has no permission') + assert.ok(userPerm, 'the user has no permission') + assert.notEqual(groupPerm.id, userPerm.id) + }) + + it('get: by a group permission id', async () => { + assert.deepEqual(await Permission.get({ id: groupPerm.id }), groupPerm) + }) + + it('delete: revokes the group permission and leaves the user one', async () => { + assert.ok(await Permission.delete({ id: groupPerm.id })) + try { + assert.equal( + await Permission.getGroup({ uid: uids[0] }), + undefined, + 'getGroup read a deleted permission', + ) + assert.equal( + (await Permission.get({ uid: uids[0] }))?.id, + userPerm.id, + 'the user permission was revoked', + ) + } finally { + await Permission.delete({ id: groupPerm.id, deleted: 0 }) + } + }) + + it('a user going explicit gets an unused permission id', async () => { + await User.create(userCase(uids[1], true)) + assert.ok(await User.put({ id: uids[1], inherit_group_permissions: false })) + + const perm = await Permission.get({ uid: uids[1] }) + const taken = [groupPerm.id, userPerm.id, (await Permission.get({ gid: gids[1] })).id] + assert.ok(perm, 'the user has no permission') + assert.ok(!taken.includes(perm.id), `permission id ${perm.id} is taken`) + assert.equal((await Permission.get({ id: perm.id }))?.user?.id, uids[1]) + }) +}) diff --git a/lib/store/file.test.js b/lib/store/file.test.js index b0684f8..3607577 100644 --- a/lib/store/file.test.js +++ b/lib/store/file.test.js @@ -303,11 +303,11 @@ describe('file store', () => { 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 () => { + it(`leaves a ${type} group's permission alone once a user's is gone`, async () => { process.env.NICTOOL_DATA_STORE = type fs.rmSync(path.join(tmp, `permission.${type}`), { force: true }) await seed(new FileStore('group'), 'group', [ - { id: 1, name: 'one', permissions: { id: 1, name: 'group' } }, + { id: 1, name: 'one', permissions: { id: 2, name: 'group' } }, ]) await seed(new FileStore('user'), 'user', [ { id: 1, gid: 1, username: 'one', permissions: { id: 1, name: 'user' } }, @@ -319,8 +319,9 @@ describe('file store', () => { 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) + assert.equal((await permissions.get({ id: 2 })).name, 'group') const [group] = await new FileStore('group').load('group') - assert.deepEqual({ ...group.permissions }, { id: 1, name: 'group' }) + assert.deepEqual({ ...group.permissions }, { id: 2, name: 'group' }) }) it(`stores a ${type} group permission whose user id is null`, async () => { @@ -329,11 +330,25 @@ describe('file store', () => { 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) + const id = await permissions.create({ user: { id: null }, group: { id: 3 }, name: 'grp' }) + assert.ok(Number.isInteger(id)) assert.equal((await permissions.get({ gid: 3 })).name, 'grp') + assert.equal((await permissions.get({ id })).name, 'grp') await assert.rejects(permissions.create({ id: 900, name: 'no target' }), /needs a user or group/) }) + it(`never reissues a ${type} permission id given explicitly`, 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' }]) + const permissions = new PermissionRepoFile() + + assert.equal(await permissions.create({ id: 50, uid: 1, name: 'explicit' }), 50) + assert.ok(await permissions.destroy({ id: 50 })) + assert.equal(await permissions.create({ uid: 1, name: 'next' }), 51) + }) + 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']) @@ -352,17 +367,17 @@ describe('file store', () => { 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 () => { + it(`changes the ${type} permission put names, not a group's`, 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' }]) + await seed(new FileStore('permission'), 'permission', [{ id: 8, 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.ok(await permissions.put({ id: 8, name: 'changed' })) + assert.equal((await permissions.get({ id: 8 })).name, 'changed') assert.equal((await new FileStore('group').load('group'))[0].permissions.name, 'group') }) diff --git a/lib/user/store/file.js b/lib/user/store/file.js index 88eb9fb..6ea1ae9 100644 --- a/lib/user/store/file.js +++ b/lib/user/store/file.js @@ -3,10 +3,13 @@ import { idConflict } from '../../store/error.js' import Config from '../../config.js' import Credentials from '../credentials.js' import UserBase from './base.js' +import PermissionRepoFile from '../../permission/store/file.js' import { splitQualifiedUsername } from '../qualified.js' import { cascade } from '../../references.js' +const permissions = new PermissionRepoFile() + const boolFields = ['is_admin', 'deleted'] // Read the group file directly (never via the Group module) to avoid circular imports. @@ -146,7 +149,7 @@ class UserRepoFile extends UserBase { Object.assign(args, await Credentials.forStorage(args.password, args.pass_salt)) } - return this.file.mutate('user', (users, data, keep) => { + return this.file.mutate('user', async (users, data, keep) => { if (args.id !== undefined && users.some((user) => user.id === args.id)) { return keep(idConflict('user', args.id, options)) } @@ -156,7 +159,7 @@ class UserRepoFile extends UserBase { if (inherit === false) { args.permissions = { ...JSON.parse(JSON.stringify(defaultPermissions)), - id: args.id, + id: await permissions.allocateId(), user: { id: args.id, create: false, write: false, delete: false }, group: { id: args.gid, create: false, write: false, delete: false }, } @@ -174,7 +177,7 @@ class UserRepoFile extends UserBase { const inherit = args.inherit_group_permissions delete args.inherit_group_permissions - return this.file.mutate('user', (users) => { + return this.file.mutate('user', async (users) => { const idx = users.findIndex((u) => u.id === args.id) if (idx === -1) return false @@ -183,7 +186,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].permissions?.id ?? users[idx].id, + id: users[idx].permissions?.id ?? (await permissions.allocateId()), user: { id: users[idx].id, create: false, write: false, delete: false }, group: { id: args.gid ?? users[idx].gid, create: false, write: false, delete: false }, }