mirror of
https://github.com/jhd3197/ServerKit.git
synced 2026-10-23 03:27:43 +09:00
test(authz): ratchet plugin routes too, and write down why not to sweep them
PR #122 review suggested converting the serverkit-status routes from @jwt_required() + an inline is_admin check to rbac decorators. Declined: plan 76's recorded decision (2026-08-19) is that JWT-only is the deliberate default, because every rbac decorator authenticates through auth_required() and therefore GRANTS API-key callers access -- an expansion of the authenticated surface, never a mechanical sweep. Two gaps let that suggestion look uncontroversial: 1. jwt_only_census.py scanned only app/api, so the extension's 15 bare @jwt_required() routes were invisible to the ratchet. It now scans app/plugins as well; ceiling 577 -> 611 for the newly visible population (a scope widening, not a regression). Only the live copy is counted -- builtin-extensions/ is the same code twice and the drift guard pins them equal. 2. Neither CLAUDE.md nor AGENTS.md said the inline shape is already a valid explicit decision (test_route_authz_static counts .is_admin via ATTRIBUTE_GATES), or that ~91 routes across 21 files use it. Both now do, with the plan 76 quote and a note that automated reviewers raise this routinely. Audited 2026-08-31 by AST over app/api + app/plugins + builtin-extensions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
30b780f4e4
commit
a31cc5d3a8
4 files changed
+79
-10
No files matched your search
@@ -215,6 +215,42 @@ authorization decision — `@jwt_required()` alone is never sufficient
|
||||
When in doubt: admin-only for panel-wide resources, `@developer_required` for
|
||||
operational actions, the file's own inline ownership helper for user data.
|
||||
|
||||
#### Do not sweep `@jwt_required()` + inline `is_admin` to rbac decorators
|
||||
|
||||
An existing route shaped like this **already satisfies default-deny** —
|
||||
`test_route_authz_static.py` counts inspecting `.is_admin` / `.is_developer` as
|
||||
an inline role check (`ATTRIBUTE_GATES`):
|
||||
|
||||
```python
|
||||
@bp.route('/<int:page_id>', methods=['PUT'])
|
||||
@jwt_required()
|
||||
def update_page(page_id):
|
||||
user = get_current_user()
|
||||
if not user or not user.is_admin:
|
||||
return jsonify({'error': 'Admin access required'}), 403
|
||||
```
|
||||
|
||||
It is also the majority shape: ~91 routes across 21 files (audited 2026-08-31).
|
||||
|
||||
Converting one to `@admin_required` is **not** a refactor. Every rbac decorator
|
||||
authenticates through `auth_required()`, which accepts an `X-API-Key`, so the
|
||||
conversion *grants API-key callers access to that route*. The decision recorded
|
||||
when plan 76 closed (2026-08-19) still governs:
|
||||
|
||||
> JWT-only is the deliberate default. A route becomes API-key capable one at a
|
||||
> time, when an API-key use case exists for it, in a commit that says so —
|
||||
> never as a mechanical sweep.
|
||||
|
||||
`backend/tests/jwt_only_census.py` ratchets the population (`JWT_ONLY_CEILING`,
|
||||
scanning `app/api` **and** `app/plugins`): it may shrink through deliberate
|
||||
conversions and may not grow, so a NEW route must pick a policy decorator
|
||||
instead of bare `@jwt_required()`.
|
||||
|
||||
The rule above ("do not stack `@jwt_required()` outside a gate") is about not
|
||||
having *both*. It is not an instruction to convert routes that have neither.
|
||||
Automated reviewers suggest this conversion routinely — decline it unless the
|
||||
commit can name the API-key use case.
|
||||
|
||||
## Key Environment Variables
|
||||
|
||||
| Variable | Purpose |
|
||||
|
||||
@@ -129,6 +129,28 @@ outside one. `backend/tests/test_route_authz_static.py` (coverage),
|
||||
`test_route_authz_sweep.py` (enforcement) and `test_auth_decorator_boundaries.py`
|
||||
(one boundary) all fail the build otherwise. See AGENTS.md for the allowlists.
|
||||
|
||||
**`@jwt_required()` + an inline `is_admin` check is a valid explicit decision —
|
||||
do NOT sweep it to rbac decorators.** `test_route_authz_static.py` counts
|
||||
inspecting `.is_admin` / `.is_developer` as an inline role check
|
||||
(`ATTRIBUTE_GATES`), so those routes already satisfy default-deny. They are
|
||||
also the majority: ~91 routes across 21 files use exactly that shape.
|
||||
|
||||
The reason it is not a free refactor: every rbac decorator authenticates via
|
||||
`auth_required()`, which accepts an `X-API-Key` as well as a JWT. Converting a
|
||||
route therefore **grants API-key callers access to it** — an expansion of the
|
||||
authenticated surface, not a cleanup. The recorded decision (2026-08-19,
|
||||
closing plan 76) stands:
|
||||
|
||||
> JWT-only is the deliberate default. A route becomes API-key capable one at a
|
||||
> time, when an API-key use case exists for it, in a commit that says so —
|
||||
> never as a mechanical sweep.
|
||||
|
||||
`backend/tests/jwt_only_census.py` ratchets the population (`JWT_ONLY_CEILING`)
|
||||
over `app/api` **and** `app/plugins`: it may shrink as routes are deliberately
|
||||
converted, and may not grow — a NEW route picks a policy decorator rather than
|
||||
bare `@jwt_required()`. Automated reviewers (Copilot) suggest this conversion
|
||||
routinely; decline it unless the commit can name the API-key use case.
|
||||
|
||||
## Key Environment Variables
|
||||
|
||||
| Variable | Purpose |
|
||||
|
||||
@@ -1 +1 @@
|
||||
577
|
||||
611
|
||||
@@ -38,6 +38,16 @@ import sys
|
||||
HERE = os.path.dirname(os.path.abspath(__file__))
|
||||
BACKEND = os.path.dirname(HERE)
|
||||
API_DIR = os.path.join(BACKEND, 'app', 'api')
|
||||
# Extension routes are the same authenticated surface as core routes, and were
|
||||
# invisible here until 2026-08-31: the serverkit-status blueprint carried 15
|
||||
# bare-@jwt_required() routes that this census never saw, so the ratchet could
|
||||
# not object when a review suggested converting them wholesale. Plugin routes
|
||||
# are scanned too now. Only the live copy under app/plugins/ is counted --
|
||||
# builtin-extensions/ is the same code a second time on disk (the drift guard
|
||||
# in test_builtin_extension_drift.py pins them equal), and counting both would
|
||||
# double every extension route.
|
||||
PLUGIN_DIR = os.path.join(BACKEND, 'app', 'plugins')
|
||||
SCAN_DIRS = (API_DIR, PLUGIN_DIR)
|
||||
CEILING_FILE = os.path.join(HERE, 'JWT_ONLY_CEILING')
|
||||
|
||||
SKIP_DIRS = {'__pycache__'}
|
||||
@@ -72,15 +82,16 @@ def count_file(path):
|
||||
|
||||
def census():
|
||||
found = {}
|
||||
for dirpath, dirnames, filenames in os.walk(API_DIR):
|
||||
dirnames[:] = [d for d in dirnames if d not in SKIP_DIRS]
|
||||
for name in sorted(filenames):
|
||||
if not name.endswith('.py'):
|
||||
continue
|
||||
path = os.path.join(dirpath, name)
|
||||
hits = count_file(path)
|
||||
if hits:
|
||||
found[_rel(path)] = hits
|
||||
for root in SCAN_DIRS:
|
||||
for dirpath, dirnames, filenames in os.walk(root):
|
||||
dirnames[:] = [d for d in dirnames if d not in SKIP_DIRS]
|
||||
for name in sorted(filenames):
|
||||
if not name.endswith('.py'):
|
||||
continue
|
||||
path = os.path.join(dirpath, name)
|
||||
hits = count_file(path)
|
||||
if hits:
|
||||
found[_rel(path)] = hits
|
||||
return found
|
||||
|
||||
|
||||
|
||||
Reference in new issue
Block a user