Repository navigation
Conversation
User, group and standalone permissions took their ids from the user or
group id, so a group's permission collided with the same-numbered user's.
get({ id }) skipped groups to avoid answering for the wrong one, which
left group permissions unreachable by id.
They now share one sequence kept as the permission file's last_id, as
nt_perm does in mysql. getGroup honours deleted, and a standalone
create refuses a second permission for one target.
There was a problem hiding this comment.
🟡 Changes recommended
Allocation races, legacy-ID retargeting, incomplete high-water tracking, and revoked-permission reads remain unresolved.
4 open issues
What changed in this PR
This PR introduces a shared permission-ID sequence for JSON and TOML stores, aligning allocation with MySQL.
Changes:
- Routes user, group, and standalone permission allocation through one sequence.
- Adds group permission lookup by ID, revoked-group filtering, and duplicate-owner checks.
- Expands permission-ID and file-store regression tests.
| File | Description |
|---|---|
| lib/user/store/file.js | Uses shared allocation for explicit user permissions. |
| lib/store/file.test.js | Updates fixtures and tests explicit-ID tracking. |
| lib/permission/test/index.js | Adds shared permission identity and revocation tests. |
| lib/permission/store/file.js | Implements shared allocation and group-ID access. |
| lib/group/store/file.js | Uses shared allocation for group permissions. |
Review effort: Balanced
Comment on lines
+56
to
+58
| 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) |
Comment on lines
+242
to
+245
| const changedGroup = await this.groupFile.mutate('group', (groups) => { | ||
| const group = groups.find((g) => g.permissions?.id === id) | ||
| if (!group) return false | ||
| change(group, 'permissions') |
Contributor
Author
There was a problem hiding this comment.
fixed in 798f8cc. On stores written by main, a repeated destroy now returns false and leaves the group's permission alone
| 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) |
Comment on lines
200
to
201
| const perm = this._postProcess(holder.permissions, deletedArg) | ||
| if (deletedArg === true && perm.deleted !== true) return undefined |
Migration ran on the first id-based permission call, and only for a user
and group sharing an id. A group read handed out the old id, and a user
destroyed or inheriting first hid the duplicate. Main also left ids
repeated between standalone rows and owners, and between two users.
migrate() now runs once before the server takes requests, and the
permission file records that it has. It starts the sequence past the
owners' last_ids and the highest id held, then renumbers every repeat
among users and standalone rows. The row main's get({ id }) answered
with keeps the id, so ids clients hold still mean the same row. Every
group permission gets a new id: main never reached one by id, so the id
may be one a user's permission held before it was removed.
get({ uid }) and get({ gid }) filter revoked permissions the way
get({ id }) does, matching mysql. The race regression covers user create,
user going explicit and migration, with a racer that takes the group
path, since a user's racer waits on the user file lock.
Group.get and User.get drop a revoked permission as well, so a zone no
longer seeds nameservers from one, and Group.put leaves it alone. A
regrant takes a new id, as mysql's delete and insert does, so a stale
delete can't revoke it. A group's permission no longer names its gid as
a user.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On json and toml, a permission took its id from its user or group. Group 5 and user 5 both got permission 5, so
GET /permission/5could only answer for one of them.get({ id })skipped groups to avoid returning the wrong one. A group permission was then unreachable by id, and a user going explicit could reuse an id another permission held.User, group and standalone permissions now draw from one sequence, kept as the permission file's
last_id. That is whatnt_perm's auto increment gives mysql. An id given explicitly is recorded too, so it is never handed out again after a destroy.Stores written before this change repeat ids: a user and a group, a standalone row and an owner, and with an explicit id two users. The server now calls
Permission.migrate()before it takes requests, once per store. It starts the sequence past the owners'last_ids and renumbers repeats among users and standalone rows. The rowget({ id })answered on main keeps the id, so an id a client holds still means the same row. Every group permission gets a new id, since main never reached one by id and its old id may be one a removed user permission held. mysql has nothing to migrate.File stores now hide revoked permissions the way mysql does.
get({ uid }),get({ gid }),getGroup,Group.getandUser.getall skip them, soPOST /zoneno longer takes default nameservers from one. A regrant takes a new id, as mysql's delete and insert does.A standalone create refuses a second permission for one user or group, where mysql returns a conflict. A group's permission no longer names its gid as
user.id.