Skip to content

Honour direct user grants in every permission check - #398

Open
antosubash wants to merge 1 commit into
mainfrom
ccr-3799e40b-9ryt6r
Open

antosubash wants to merge 1 commit into
mainfrom
ccr-3799e40b-9ryt6r

Conversation

@antosubash

Copy link
Copy Markdown
Owner

Fixes #337.

Problem

simple_module_hosting.permissions.RequiresPermission resolved roles only. permissions.deps.RequiresPermission also read permissions_user_permission. Every module importing the framework class ignored a grant made on the permissions screen: the PUT returned 200, and the user still got 403. In this repo that covered users, settings, file_storage, feature_flags, audit_log, tenants and others. The menu filter and the frontend's auth.permissions couldn't see direct grants either.

A third gate had a worse version of the same bug. auth.deps.require_permission called get_permissions_for_roles(user.roles) without the registry's role map, so every non-admin role resolved to no permissions at all.

Approach: option 2 from the issue (fold grants into the resolved set)

  • core: PermissionRegistry.add_grant_source(source) adds an async (request, user) -> keys seam. Hosting never imports the plugin (SM009).
  • hosting: a single resolve_principal_permissions() combines roles and grant sources. It's used by both InertiaLayoutDataMiddleware and RequiresPermission, which is now async. If a source raises, it contributes nothing and is logged, so the request fails closed with a 403 instead of a 500. Sources are skipped for principals that already hold *. ensure_resolved_permissions() covers bare routers that run without the middleware.
  • permissions: permissions.grants.direct_grant_source reads direct grants through a per-process TTLCache (30 s). Saving a user's grants publishes permissions.user_grants on the InvalidationBus from an on_commit callback, following the same pattern as users.session_version_cache. permissions.deps.RequiresPermission is now an alias of the framework class, so the two can't drift apart again.
  • auth: require_permission reads the same resolved set, with any-of semantics unchanged.
  • docs: docs/modules/permissions.md and docs/modules/auth.md and the permissions README now name a single RequiresPermission.

Trade-offs for review

  1. Revocation latency on permissions' own routes. These used to read direct grants live from the DB. They now share the cache. If another worker revokes a grant and no invalidation transport (Redis via background_tasks) is installed, the change can take up to 30 s to apply. This is the same window users.session_version_cache already accepts. The worker that made the change sees it on the next request.
  2. Cost: one indexed query per user per cache miss, on an authenticated request.
  3. Not fixed here (follow-up): set_role_permissions only updates the in-memory role map of the process that handled the save, so other workers stay stale until restart. It's the same class of bug but a separate fix.

Tests

  • modules/permissions/tests/test_direct_grants_everywhere.py covers the issue's reproduction. A user with no roles gets 403, then 200 after a direct grant (which also proves the cache eviction), then 403 after revoking. It also checks that the grant appears in Inertia auth.permissions. Both tests fail with the grant source switched off.
  • framework/hosting/tests/test_grant_sources.py checks merging grants with roles, failing closed when a source raises, skipping sources for wildcard holders, the middleware passing grants to the frontend, and RequiresPermission with no middleware present.
  • modules/auth/tests/test_deps.py adds a regression test for a mapped non-admin role.
  • Full Python suite: 3586 passed, 2 skipped. ruff format, ruff check and ty are clean, and the 300-line check passes.

https://claude.ai/code/session_01RqQH2V6szjHKpQxSg29LqP


Generated by Claude Code

The framework's RequiresPermission resolved roles only, while
permissions.deps.RequiresPermission also read the direct-grant table. Every
module importing the framework class (users, settings, file_storage,
feature_flags, audit_log, tenants, ...) ignored a grant made on the
permissions screen: the PUT returned 200 and the user still got 403.

- core: PermissionRegistry.add_grant_source(), an async per-principal seam,
  so hosting never imports the plugin (SM009).
- hosting: one async resolve_principal_permissions() (roles + sources, fail
  closed on a raising source, skipped for wildcard holders) used by both
  InertiaLayoutDataMiddleware and RequiresPermission, so the door, the menu
  filter and auth.permissions agree.
- permissions: registers a per-process TTL-cached grant source, evicted via
  the InvalidationBus from an on_commit callback when grants are saved;
  permissions.deps.RequiresPermission is now an alias of the framework class.
- auth: require_permission resolved roles without the registry's role map,
  so every non-admin role held nothing; it now reads the same set.

Claude-Session: https://claude.ai/code/session_01RqQH2V6szjHKpQxSg29LqP
@antosubash
antosubash marked this pull request as ready for review October 3, 2026 17:52
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
🔒 Security Review ✅ Completed 2026-10-03T18:02:47.803439Z db802aa Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Two RequiresPermission classes with different semantics: the hosting one silently ignores per-user grants

2 participants