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') |
| 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 |
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.Two smaller gaps the new tests hit.
getGroupreturned a revoked group permission. A standalone create allowed a second permission for one user or group, where mysql returns a conflict.Stores written before this change still give group N's permission the id N, beside user N's. Main never reached a group permission by id, so a stale id could not touch one. Now it can. No record points at a permission id, so recreating such a group's permission gives it a fresh id and orphans nothing.