fix(nextly): a super-admin's key copies the catalogue, and a plugin caller has roles - #1783
Merged
Merged
Conversation
…aller has roles A read-only or full-access key copies its owner's permission rows, and a super-admin's power is a bypass rather than rows: the role is granted the permissions that exist at setup and nothing grants it the ones a later collection adds, while the session never needs them. So a super-admin's key held a stale subset at best and, on an install whose first user came before the grant, nothing, which refused every request with the first key an operator mints. Such a key copies the catalogue now, every permission the install declares with orphans left out, bounded by the key's kind like anyone else's: read-only stays read-only. The super-admin check the service already ran for role assignment is the one it runs here. On the plugin path, resolveServiceOpts built the caller with role: "", a documented limitation, so a code-defined rule reading req.user.role refused every caller while the same caller's own request passed, and a negative rule granted what it was written to refuse. The roles are resolved and the caller is built by buildUserContext, the constructor every other authenticated path uses; the lookup is injectable, as resolve-run-as already has it, so the translation is tested without a database. Proven end to end through the real catch-all: a super-admin's own read-only key reads through a plugin route and cannot write through one.
Both were closed literals of services/lib/permissions, and the plugin facade now imports one more export from it, listRoleSlugsForUser, which vitest refused as undefined on the mock. Each spreads the real module and overrides only what it stubs, so the next export the subject gains is there too.
Collaborator
Author
|
@codex review |
|
Warning Review limit reachedNext included review available in 49 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change makes plugin caller role resolution asynchronous and injectable. It updates super-admin API key grants to use the permission catalogue, supports inherited super-admin status, expands route tests, and documents the resulting token behavior. ChangesPermission resolution and API key scopes
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Client
participant PluginRoute
participant APIKeyService
participant PermissionCatalogue
Client->>PluginRoute: request with API key
PluginRoute->>APIKeyService: resolve API key grants
APIKeyService->>PermissionCatalogue: select non-orphaned catalogue permissions
PermissionCatalogue-->>APIKeyService: permission rows
APIKeyService-->>PluginRoute: token-type-scoped grants
PluginRoute-->>Client: allow read or reject write
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48e457d7fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…oles A super-admin by INHERITANCE was not one to the key service. The catalogue branch read `user_roles` directly, so an operator who builds a role on top of Super Admin is a super-admin to the session bypass, the admin's checks and the key ceiling, and an ordinary user to the key they mint. `isSuperAdmin` resolves the inherited set and is what every other gate asks; the key service asks it too, and the ceiling check that carried the same direct query is derived from it rather than repeating it. A plugin caller that arrived on an API key is judged on the KEY's roles. A stored role rule reads `user.roles` with no scope in front of it, so resolving the owner's roles let a viewer-scoped key minted by an administrator satisfy an administrators-only rule, and refused a key holding the very role a rule names because its owner did not hold it. The REST path answers this with `resolveRoleSlugs`; the facade now answers it the same way, falling back to the account only for a scope that carries no roles, as `apiKeyWriteAllowed` does. Controls: an integration case whose owner holds super-admin ONLY by inheritance, asserting their read-only key holds a catalogue permission no role of theirs grants (the one thing the role-rows branch cannot produce); a unit case asserting the resolver is called, for the owner and nobody else; and four cases pinning the key's roles in both directions, including an emptied role list that must not fall back to the owner.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5722a7097a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5722a7097a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ange `isSuperAdmin` answers from a process-wide map with a 60-second life, and `invalidatePermissionCache` cleared the two permission tiers beside it and not that map. A user removed from the role kept the session bypass until the entry aged out. Reading a key's grants through that answer extends it: the grants are cached for five minutes of their own, so a stale `true` could be copied into a key's grants after the demotion and outlive it by both windows together. The cause is the missing eviction, not the new reader, and it is fixed there, which closes the same window for the session bypass that has always had it. A `userId` hint clears that user. A `roleId` hint clears the map: it names no user, the map does not record which users a role reaches, and the in-memory permission keys only name users that happen to hold a cached entry, so the subset cannot be derived. The map is bounded at 1,000 entries with a 60-second life and a role change is rare, so re-asking costs one indexed query. Controls, on a real database with the role removed by a raw row delete so only the call under test can clear anything: the answer is still cached after the demotion, which is what makes the other three mean something; a userId invalidation clears that user; a roleId invalidation clears them too; and an unrelated user's answer survives a userId invalidation, which separates this from clearing the map on every hint.
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f9e46c747
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`resolveServiceOpts` gained locale forwarding on main while this branch made it async and gave it the caller's roles. Both are kept: main's `carried` object and its locale refusal stand unchanged, and the caller inside the `user` branch is built by `buildUserContext` from the key's roles or the account's. The wrapper takes the injectable deps again and awaits the call, which the merge had dropped. One test of main's asserted the caller's exact shape and now names `roles` beside `role`, which is what this branch adds to it.
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
@nextlyhq/builder
create-nextly-app
@nextlyhq/eslint-plugin
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
Contributor
…olds-the-catalogue
Collaborator
Author
|
Merged |
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
mobeenabdullah
added a commit
that referenced
this pull request
Sep 12, 2026
…y grants (#1794) * fix(nextly): fail closed on an unread role set, and retire derived key grants Follow-up to #1783, whose last review round landed after it merged. The facade resolved roles through `listRoleSlugsForUser`, which degrades a failed query to an empty set. That is the safe direction for a rule granting on a role and the wrong one for a rule withholding on it: `user.role !== "suspended"` admits a caller the database declined to answer for, which is the same empty-role grant #1783 exists to remove arriving by another door. Its strict sibling exists for exactly this and says so in its own documentation. A throw refuses the plugin's call, which is the correct direction: a decision taken on roles nobody read is not a decision. An API key's grants are DERIVED from the same RBAC rows and cached for five minutes under the key's id, and nothing retired them when a ROLE changed. `UserRoleService` evicts when a role is assigned to or removed from a USER; the role services only call `invalidatePermissionCache`, which knows nothing about keys. So revoking a role's inherited `super-admin` left that user's key holding the whole catalogue for the rest of the TTL, and changing a role's permissions left a role-based key holding the old set, which a comment in `UserRoleService` claimed was handled elsewhere and was not. Fixed by reversing the direction rather than by enumerating keys, which the permissions module cannot do and which a path written later would have to remember: `invalidatePermissionCache` counts its invalidations, and the key cache refuses an entry resolved under an older count. Blunt on purpose. A role change is rare and re-resolving a key's grants is a couple of indexed queries; a stale grant is the catalogue in the hands of somebody who no longer holds the role that granted it. Two suites had mocked `services/lib/permissions` as closed literals and refused the new import; both derive from the real module now, and their comments say why. Controls: a caller whose role lookup throws is refused, and a case asserting the STRICT resolver was called and the swallowing one was not, since a facade wired to either would pass the first alone. On a real database, a key holding a catalogue permission its owner's own role does not grant, which keeps holding it after a silent revocation and loses it once the role change is announced, by either hint, while its own role's grant survives so the difference is a re-resolve and not a wipe. * fix(nextly): invalidate on a permission change, and stamp the revision honestly Three of the four findings in this round, each real. `PermissionService.updatePermission`, `deletePermissionById` and `deletePermission` called no invalidation at all, and neither existing one can express what they do: a permission row belongs to no user and no role, so there is no id to scope by. A role-based key therefore kept a renamed slug and a super-admin's key kept a deleted grant until their entries aged out. `invalidateAllPermissionCaches` clears the process-local tiers, tombstones the shared one through a new `PermissionCacheService.invalidateAll`, and advances the revision; all three mutations call it after a successful write. The revision was read AFTER the grant queries, so an invalidation landing while they were in flight was stamped onto rows read under the old one, and the next request reused grants the change was meant to retire for the whole TTL. It is captured before the reads now, which leaves such an entry already behind when it is written. The strict resolver propagates the driver's exception by design, and this facade had no catch around it, so a plugin route saw a raw database error with no `code` to branch on. Wrapped in `NextlyError.internal`, cause kept. The refusal is unchanged; only its shape is. The fourth finding, that this counter is process-local, is answered on the thread rather than here: every cache it governs is already process-local, the shared tier this module has is a database table rather than a signal, and a shared generation read per request would put a query on the hot path the cache exists to remove. That is a design decision, not a patch. Controls. A permission-row invalidation retires a key's catalogue grant. For the race, a resolution is started, the caches are invalidated while it is in flight, a new permission row is added, and the next read must SEE it — observed on the catalogue rather than on the super-admin answer, which has a cache of its own and made the first version of this case measure the wrong mechanism. Its control is the same sequence without the race, where the row must NOT appear, since otherwise the case passes on an implementation that caches nothing. And the refusal is typed, with a second case on the cause, since an implementation that threw a fresh error and discarded the original passes the first alone. * test(nextly): reach the delete through the service, not the primitive The case for a permission change called `invalidateAllPermissionCaches` directly, so it proved the primitive works and said nothing about whether anything calls it. Measured: removing the call from the delete path left it green, which is a fixture that never reaches the mechanism it is named for. The new case deletes through `PermissionService.deletePermissionById` and requires the key's grants to lose the row. The direct case stays, because the two claims are different and only one of them is about the service. * fix(nextly): no cache may accept an answer resolved before an invalidation Three findings, all real, and the third is the same race one level in: it defeats the fix from the last round on its own. `isSuperAdmin` awaits two queries and then writes its cache, so a lookup started before a demotion completes after it and puts `true` back into the map the invalidation had just cleared. The next key request then correctly rejects the outer entry, consumes that stale inner one, and caches catalogue-wide grants under the NEW revision for another five minutes. Two instances of one shape is a coincidence; three is the design. So the rule is stated once, as `resolvedUnderCurrentRevision`, and asked at every cache write here rather than patched per cache: the super-admin answer, both tiers behind `hasPermission`, and the key grants already fixed. The database tier was the worst of them, because its entries are shared across instances and live for a day, so a decision written after a tombstone outlived everything else. Nine methods write permission rows and none invalidated anything. Review found them three at a time, twice, which is what a rule with nothing enforcing it looks like, so there is a check now. It reads the SYNTAX TREE: its first version found method bodies by counting braces from the signature, and a signature carrying an object type gave it the return type's brace, so `ensurePermission` mutates the table twice and read as compliant. That version also named six writers. The tree version named nine, and the three nobody had mentioned are `markOrphanedPermissions`, `normalizeReversedSlugs` and `returnPermissionToPresets` — the first of which decides whether a row is in the catalogue at all. The fourth finding, that the counter is process-local, is still open on the thread. It is a design decision with a cost either way, and the founder has the options. Controls: a lookup in flight across a revocation must not restore the answer, asserted both on the map directly and through the key grants that consume it; and the writer check names its nine rather than counting them, with a case requiring the tree to have parsed anything at all. * fix(nextly): one invalidation per batch, and a guard that reads every file Five findings, all real, and two of them were about this branch's own controls. The writer check named two service files, so it reported a third as compliant without opening it — the failure it exists to catch, one level up. It reads the package's whole source tree now, and immediately found two more writers in `role-permission-service.ts`. It also resolves one file's own helpers as a fixed point, because a writer is allowed to delegate: four seeder passes share one "did this write anything" helper rather than asking it four times, and a check that only looked for a direct call would have called all four of them silent. It accepts either invalidator too: a scoped `invalidatePermissionCache({ roleId })` is right where the write is additive and confined to one role, and demanding the table-wide form there would turn a correct cheap call into an unfiltered rewrite. The table-wide tombstone was paid per permission. A seeder ensures one at a time, so a single new collection rewrote and locked the whole cache table once per permission, each pass expiring rows the previous had already expired. Each seeding method runs as one sweep now: the revision still advances per write, so nothing in flight can file a result as current, and only the table write is deferred. The flush happens on the way out even when the batch throws, since a partial write still changed rows. The Tier-2 lookup is awaited, so an invalidation can land while it is outstanding; the row it returns predates the change and was being promoted into Tier 1 for that tier's whole life, having just been tombstoned. It recomputes instead. The fire-and-forget Tier-2 write had the mirror of that: checked only before launching, it could land behind a tombstone with a fresh expiry, into the tier that is shared between instances and lives for a day. It re-checks after the upsert and tombstones this user's rows if the revision moved, so either the invalidation caught the row or this does. Both delete paths in `PermissionService` reached the same statement by different routes and now share one, which is also where the invalidation lives, and the seeder's four passes share the helper above. The comment on the writer check describes the invariant rather than how it was arrived at. Controls: the sweep advances the revision per write while deferring the flush, clears by the time the batch returns, and flushes when the batch throws. * test(nextly): drop a shared-cache case the harness cannot reach The two cases added for the tier-2 promotion re-check could not run: their own control showed the shared tier was never consulted, and a probe found why — `setCachedPermission` does not take effect under `createTestNextly`, where the table is created and a write followed by a read returns null. A green from a case that cannot reach its branch is worse than no case, so they are removed and the branch says in the source that it is uncovered, and why. The predicate it uses is covered by the sweep and race cases; the call site is not. * fix(nextly): write a permission row through one gate that retires the caches A permission row belongs to no user and no role, so neither scoped invalidation can express a change to one. Every writer therefore carried its own table-wide invalidation, which is a step each new writer has to remember and whose omission is invisible where it happens: the writer's own tests assert on rows, not on what still answers from cache. `writingPermissions` makes the two acts one. The table arrives as the callback's parameter, so a write is written inside the gate as a matter of course, and the guard holds every other write in the package to the same rule by asking a property of the write itself rather than recognising the method around it. Where the write is inside a transaction the gate wraps the transaction, so the caches are retired after the commit rather than before it. Three further cases where a retired answer could come back: - the zero-role denial was stored without the verification its sibling branch does, so a first role granted while that write was in flight left a stored `false` for a permission the user now holds; - clearing the shared tier is itself an awaited write, and a check beginning and finishing inside that window read a row the clearing had not reached and promoted it into a tier that outlives the clearing; - the seeding batch was counted for the process, so a revocation raised while any seeder was awaiting was deferred until that seeder finished. It is scoped to the operation now, which is what `AsyncLocalStorage` is for. The stored tier's default life drops from a day to five minutes. It is shared between instances and the signal that retires it is held in memory, so the TTL is the only bound on how long an instance that did not handle the change goes on serving what it stored. Storing that signal beside the rows is the fix for that, and is not in this change. * test(nextly): a revocation raised outside a batch is not deferred by it Two cases, and the second is what makes the first mean anything. Holding a batch open and raising an unrelated revocation from outside it distinguishes a batch scoped to its operation from one counted for the process; on its own it is also satisfied by a sweep that defers nothing at all, so the control asserts that the batch's own writes are still coalesced. Verified by reproducing the process-wide read: the first case fails and the control stays green. * fix(nextly): an API key's roles refuse when they cannot be read, as a plugin call does The strict resolver only ran where `authenticatedScope.roles` was absent, and an API-key request always populates that scope: a read-only or full-access key resolved its owner's roles through the swallowing lookup, so a failed query arrived as an empty set in the very field every later role rule reads directly. Nothing downstream can tell that from an owner who holds no roles, and a rule that WITHHOLDS on one — `!roles.includes("suspended")` — then authorizes the request it exists to refuse. The door was closed on one path and left open on the reachable one. Both paths now ask `listRoleSlugsForUserOrRefuse`. Written once because it is one question: asked separately, the two would drift, and a caller left on the swallowing resolver is invisible, since an empty role set is an ordinary answer. Its contract is tested against the real function rather than through either caller's mock, and without mocking anything: an empty id returns before any query, and a lookup with no adapter registered fails for real. An earlier version of that file mocked the strict lookup through the module's exports, which does not intercept a call the wrapper makes by its internal name — it passed the real lookup straight through while appearing to control it. The documented default for PERMISSION_CACHE_TTL_SECONDS follows the code to 300 seconds, and says why the value is what bounds cross-instance staleness. * test(nextly): name which role resolver the key path asks, not only that it refuses The refusal case mocked whichever resolver the service called, so it passed on either wiring: the swallowing sibling rejects too, once a test tells it to. Reverting the fix left it green. Both resolvers are mocked now and the case asserts the door, which is the property — an unreadable role set refusing rather than arriving as none.
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
1 participant
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.
Decision D12. Two pre-existing defects on the API-key and plugin paths, parked since Phase 1, that block Phase 5's auth tests and any real test of an MCP server with an operator's own key.
A super-admin's key held nothing, or a stale subset
A
read-onlyorfull-accesskey copies its owner's permission rows. A super-admin's power is a bypass, not rows:/admin/setupgrants the Super Admin role every permission that exists at that moment, and nothing grants it the ones a collection added later brings, while the session never needs the rows. So a super-admin's key held the setup-time subset at best and, on an install whose first user came before the grant, nothing: every request refused, starting with the first key an operator mints to try an integration with. The Phase 1 suites worked around it with a viewer-role key; the comment that recorded the workaround is gone.The key copies the catalogue now: every permission the install declares (
permissionsrows with noorphaned_at), bounded by the key's kind like anyone else's, so read-only stays read-only. A permission a package stopped declaring is not inherited. The super-admin check the service already ran inline for role assignment is one private method now, used by both.A plugin caller had no roles
resolveServiceOptsbuilt the caller foras: "user"as{ id, email, role: "", permissions: [] }, a documented v1 limitation. A code-defined rule such asreq.user?.role === "editor"refused every caller on the plugin path while the same caller's own request passed it, and a negative rule (role !== "suspended") granted what it was written to refuse. The roles are resolved (listRoleSlugsForUser) and the caller is built bybuildUserContext, the one constructor every other authenticated path uses; the lookup is injectable, the wayresolve-run-asalready takes its deps, so the translation stays testable without a database.resolveServiceOptsis async now; its one production caller, the facade wrapper, already was.Verified
api-key-token-types.test.ts+6 (a super-admin role holding no rows: read-only holds everyread-*in the catalogue and nothing writable; full-access holds all; a permission added after the grant is held; an orphaned one is not; the editor's key is the control that an ordinary creator still copies their own rows).plugin-route-key-scope.integration.test.ts+2, through the real catch-all dispatcher: a super-admin's own read-only key reads through a plugin route and cannot write through one.service-opts.test.ts+1 (roles reachuser.roleanduser.roles); every case awaits the async resolver with a fake lookup.nextlyunit suites 754/754 acrossdomains/auth,plugins,auth; lint clean. Typecheck fails on one line from docs(nextly): field hooks run last and see the whole record #1754 (collection-mutation.test.ts:345), which test(nextly): type the hook-order callback so check-types passes on main #1780 fixes; nothing here.docs/guides/authentication.mdxstates the rule beside the token-type table.services/lib/permissionsas closed literals and refused the facade's new import; both derive from the real module now (48e457d), which is this repository's own rule for mocks.Summary by CodeRabbit
New Features
Documentation