permission: file stores draw one id sequence

CHANGELOG.md

@@ -6,6 +6,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 
 ### Unreleased
 
+- fix: file stores draw permission ids from one sequence, and renumber repeated ids at startup
+- fix: file store user, group and permission reads hide revoked permissions, and a regrant gets a new id
 - fix: reject create conflicts and duplicate permission targets
 - fix(sql): seed ns export types coredns & native for 2.x upgrade
 - fix: server stop waits for the stores to disconnect

lib/group/store/file.js

@@ -1,10 +1,13 @@
-import FileStore, { claimId, removeRows } from '../../store/file.js'
+import FileStore, { claimId, removeRows, serialize } 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,
@@ -28,6 +31,7 @@ class GroupRepoFile extends GroupBase {
   _postProcess(row, deletedArg) {
     const r = JSON.parse(JSON.stringify(row))
     r.deleted = Boolean(r.deleted)
+    if (r.permissions?.deleted) delete r.permissions
     if (r.permissions?.nameserver && !Array.isArray(r.permissions.nameserver.usable)) {
       r.permissions.nameserver.usable = []
     }
@@ -61,31 +65,33 @@ class GroupRepoFile extends GroupBase {
     const usable_ns = args.usable_ns ?? []
     delete args.usable_ns
 
-    return this.file.mutate('group', (groups, data, keep) => {
-      if (args.id !== undefined && groups.some((group) => group.id === args.id)) {
-        return keep(idConflict('group', args.id, options))
-      }
-
-      args.id = claimId(groups, data, args.id)
+    return serialize('permission create', () =>
+      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))
+        }
 
-      const gid = args.id
-      args.permissions = {
-        ...JSON.parse(JSON.stringify(defaultPermissions)),
-        id: gid,
-        name: `Group ${args.name} perms`,
-        user: { id: gid, create: false, write: false, delete: false },
-        group: { id: gid, create: false, write: false, delete: false },
-        nameserver: {
-          usable: Array.isArray(usable_ns) ? usable_ns : [],
-          create: false,
-          write: false,
-          delete: false,
-        },
-      }
+        args.id = claimId(groups, data, args.id)
+
+        const gid = args.id
+        args.permissions = {
+          ...JSON.parse(JSON.stringify(defaultPermissions)),
+          id: await permissions.allocateId(),
+          name: `Group ${args.name} perms`,
+          user: { create: false, write: false, delete: false },
+          group: { id: gid, create: false, write: false, delete: false },
+          nameserver: {
+            usable: Array.isArray(usable_ns) ? usable_ns : [],
+            create: false,
+            write: false,
+            delete: false,
+          },
+        }
 
-      groups.push(args)
-      return gid
-    })
+        groups.push(args)
+        return gid
+      }),
+    )
   }
 
   async get(args_orig) {
@@ -128,7 +134,7 @@ class GroupRepoFile extends GroupBase {
       const idx = groups.findIndex((g) => g.id === id)
       if (idx === -1) return false
 
-      if (usable_ns !== undefined && groups[idx].permissions) {
+      if (usable_ns !== undefined && groups[idx].permissions && !groups[idx].permissions.deleted) {
         groups[idx].permissions.nameserver = {
           ...groups[idx].permissions.nameserver,
           usable: Array.isArray(usable_ns) ? usable_ns : [],

lib/permission/store/base.js

@@ -11,6 +11,7 @@
  *   put(args)          → boolean
  *   delete(args)       → boolean
  *   destroy(args)      → boolean
+ *   migrate()          → runs once before the server takes requests
  */
 class PermissionBase {
   constructor(args = {}) {
@@ -41,6 +42,8 @@ class PermissionBase {
     throw new Error('destroy() not implemented by this store')
   }
 
+  async migrate() {}
+
   /**
    * Returns the effective permissions for a user:
    * – If the user has their own permission row with inherit=false, return it.

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, { highestId, nextId, serialize } from '../../store/file.js'
+import { idConflict, StoreConflictError, StoreInvalidError } from '../../store/error.js'
 
 import PermissionBase from './base.js'
 
@@ -12,15 +12,17 @@ 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
  * getGroup({ uid }) → permissions of the group the user belongs to
+ *
+ * All three draw ids from one sequence, kept as the permission file's last_id.
  */
 class PermissionRepoFile extends PermissionBase {
   constructor(args = {}) {
@@ -42,6 +44,27 @@ 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 _held(standalone) {
+    const [users, groups] = await Promise.all([this._loadUsers(), this._loadGroups()])
+    return [...users, ...groups]
+      .map((row) => row.permissions)
+      .filter(Boolean)
+      .concat(standalone)
+  }
+
+  async _claimId(standalone, data, id) {
+    const held = await this._held(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, highestId(held), id)
+    return id
+  }
+
   // ---------------------------------------------------------------------------
   // Post-processing
   // ---------------------------------------------------------------------------
@@ -58,6 +81,11 @@ class PermissionRepoFile extends PermissionBase {
     return r
   }
 
+  _matching(perm, deletedArg) {
+    if (Boolean(perm.deleted) !== Boolean(deletedArg)) return undefined
+    return this._postProcess(perm, deletedArg)
+  }
+
   // ---------------------------------------------------------------------------
   // CRUD
   // ---------------------------------------------------------------------------
@@ -103,12 +131,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 +148,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 +163,13 @@ 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) => {
+      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
@@ -149,6 +178,58 @@ class PermissionRepoFile extends PermissionBase {
     })
   }
 
+  // Old stores took ids from owners, and reached a group's permission by gid alone.
+  // So groups get new ids, and of the rest the row get({ id }) answered keeps its id.
+  async migrate() {
+    return serialize('permission create', async () => {
+      if (await this.standaloneFile.mutate('permission', (perms, data, keep) => keep(data.ids_migrated))) {
+        return
+      }
+
+      const seen = new Set()
+      const renumber = async (perms, allocate) => {
+        let moved = false
+        for (const perm of perms) {
+          if (perm?.id === undefined) continue
+          if (seen.has(perm.id)) {
+            perm.id = await allocate()
+            moved = true
+          }
+          seen.add(perm.id)
+        }
+        return moved
+      }
+      const inline = (rows) => rows.map((row) => row.permissions)
+
+      await this.standaloneFile.mutate('permission', async (perms, data) => {
+        const lastIds = await Promise.all([this.userFile.lastId('user'), this.groupFile.lastId('group')])
+        const highest = Math.max(highestId(await this._held(perms)), ...lastIds)
+        if (highest <= (data.last_id ?? 0)) return false
+        data.last_id = highest
+      })
+      await this.userFile.mutate('user', (users) => renumber(inline(users), () => this.allocateId()))
+      await this.standaloneFile.mutate('permission', (perms, data) =>
+        renumber(perms, () => this._claimId(perms, data)),
+      )
+      await this.groupFile.mutate('group', async (groups) => {
+        const perms = inline(groups).filter(Boolean)
+        if (perms.length === 0) return false
+        let id = await this.standaloneFile.mutate('permission', (standalone, data) => {
+          const first = data.last_id ?? 0
+          data.last_id = nextId([], first + perms.length - 1)
+          return first
+        })
+        for (const perm of perms) {
+          perm.id = ++id
+          if (perm.user) delete perm.user.id
+        }
+      })
+      await this.standaloneFile.mutate('permission', (perms, data) => {
+        data.ids_migrated = true
+      })
+    })
+  }
+
   async get(args) {
     args = JSON.parse(JSON.stringify(args))
     const deletedArg = args.deleted ?? false
@@ -157,41 +238,25 @@ class PermissionRepoFile extends PermissionBase {
       const users = await this._loadUsers()
       const user = users.find((u) => u.id === args.uid)
       if (!user?.permissions) return undefined
-      const perm = this._postProcess(user.permissions, deletedArg)
-      if (deletedArg === true && perm.deleted !== true) return undefined
-      return perm
+      return this._matching(user.permissions, deletedArg)
     }
 
     if (args.gid !== undefined) {
       // group-level lookup: no uid qualifier
       const groups = await this._loadGroups()
       const group = groups.find((g) => g.id === args.gid)
       if (!group?.permissions) return undefined
-      const perm = this._postProcess(group.permissions, deletedArg)
-      if (deletedArg === true && perm.deleted !== true) return undefined
-      return perm
+      return this._matching(group.permissions, deletedArg)
     }
 
     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)
-        if (deletedArg === true && perm.deleted !== true) return undefined
-        return perm
-      }
+      const [users, groups] = await Promise.all([this._loadUsers(), this._loadGroups()])
+      const holder = [...users, ...groups].find((row) => row.permissions?.id === args.id)
+      if (holder) return this._matching(holder.permissions, deletedArg)
 
-      // Check standalone permission.toml
       const perms = await this._loadStandalone()
       const found = perms.find((p) => p.id === args.id)
-      if (found) {
-        const isDeleted = Boolean(found.deleted)
-        const wantDeleted = Boolean(deletedArg)
-        if (isDeleted !== wantDeleted) return undefined
-        return this._postProcess(found, deletedArg)
-      }
+      if (found) return this._matching(found, deletedArg)
     }
 
     return undefined
@@ -207,6 +272,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 +285,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