permission: file stores draw one id sequence

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 },

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

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])
+  })
+})

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')
     })
 

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 },
         }