test(auth): assert the role literal each authorization wrapper enforces - #726
Conversation
The shared `ensure_partition_role` helper was well covered, but every existing test of the thin wrappers replaced them via `dependency_overrides`, so the wrapper body — and the role string it hardcodes — never executed. Mutation testing showed five of six wrappers could be made allow-all with zero failures; `require_admin_or_self`, which guards token regeneration, had zero references in tests/ at all. Two adjacent gaps had the same shape: dropping the `SUPER_ADMIN_MODE` conjunct (making super-admin permanently on) failed no test, and forcing `pending_task_count=0` in the quota check failed no test — every enforcing case already passed 0, leaving the in-flight half (the #664 race) unverified. Call each wrapper directly and spy on the role it forwards; add the missing SUPER_ADMIN_MODE deny case and a pending-tasks-alone quota case. The quota tests use the real AuthService rather than a fake so they assert production semantics. Verified by re-running the mutations, each of which previously passed: require_partition_owner "owner" -> "viewer" 1 failed require_partition_editor "editor" -> "viewer" 1 failed SUPER_ADMIN_MODE conjunct dropped 1 failed pending_task_count forced to 0 1 failed require_admin_or_self allow-all 3 failed Tests only; no production change. 1815 passed. Closes #725
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughExpanded authentication dependency unit coverage for partition role forwarding, administrator authorization, super-admin mode behavior, and file quota enforcement including pending tasks. ChangesAuthentication dependency coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
hedhoud
left a comment
There was a problem hiding this comment.
The new tests cover the authorization decisions that were previously unprotected, including role forwarding, admin boundaries, and pending-task quota enforcement. I verified them against the latest develop branch and the full unit suite passes. Approved.
Closes #725.
Tests only — no production change.
Problem
The shared
ensure_partition_rolehelper is well covered (5 tests), but the role literal inside each wrapper is not. Every existing test of the wrappers replaces them viadependency_overrides(e.g.test_phase14_partition_routes.py:59-60), so the wrapper body never executes.Mutation testing against the full suite:
require_partitions_viewerSUPER_ADMIN_MODEconjunct (super-admin permanently on)pending_task_count = 0in the quota checkSo
require_partition_owner,_editor,_viewer,require_adminandrequire_admin_or_selfcould each be made allow-all with zero failures.require_admin_or_self— which guards token regeneration — had zero references intests/.Change
ensure_partition_role, asserting the role it forwards (viewer/editor/owner, andviewerper-partition for the plural variant)require_adminand a 4-case matrix forrequire_admin_or_selfSUPER_ADMIN_MODE=False, is_admin=Truedeny case (only the allow side was tested)The quota tests use the real
AuthService, not a fake, so they assert production semantics rather than a stand-in's.Verification
Each mutation from the issue, re-run against these tests:
Full suite: 1815 passed.
ruff check/ruff format --check/check_layer_imports.pygreen.Why this one matters
It's the only issue in the v2.0.1 batch that prevents bugs rather than fixing one. A one-word slip (
"owner"→"viewer") across three byte-identical wrappers currently ships green — this makes that impossible.Summary by CodeRabbit