Repository navigation
fix(auth): prevent custom permission checks racing policy refresh - #2125
Conversation
Reproduced from PR #2123 post-merge QA at 3371fd4: concurrent custom-permission checks raised a Casbin TypeError when authentication removed rows during enforcement. Use the existing synchronized enforcer and preserve unchanged grants. Cover HTTP concurrency, denial, revocation, and an explicitly held in-flight read.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Reconcile API key permissions api/src/unraid-api/auth/auth.service.ts, .limetech/ai-review-markers/fix-casbin-permission-race-4f61c40b0160.json |
Permission synchronization retains requested grants, removes stale grants, and adds only missing grants. A review marker records the final-stage disposition. |
Synchronize enforcement and test permission updates api/src/unraid-api/auth/casbin/casbin.service.ts, api/src/unraid-api/auth/casbin/permission-sync.integration.spec.ts |
Casbin initialization now uses a synchronized enforcer. Integration tests cover concurrent GraphQL authorization, changed and wildcard permissions, in-flight checks during removal, and recovery after a failed role lookup. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Possibly related PRs
- unraid/api#2123: Its mixed GraphQL query scenario exposed the authorization race addressed here.
Suggested reviewers: pujitm
Merge Risk: 
c014b
-
No actionable issue remains identified in the permission-race fix; it is ready for normal merge checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title names the main change: it fixes a race in the auth layer where custom permission checks could run during a policy refresh. The changeset supports this. The auth service now syncs permissions… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the main change: it fixes a race in the auth layer where custom permission checks could run during a policy refresh. The changeset supports this. The auth service now syncs permissions without deleting all grants first, and the Casbin service uses a synchronized enforcer that serializes checks with policy mutations. The title uses the conventional commit form and stays under 72 characters. It does not name the synchronized enforcer or the permission-preserving sync, but those are secondary details.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks each grant in line,
Keeps the matches, drops the stale.
Casbin guards the checks in time,
While tests run requests without fail.
Fresh permissions take their place,
And carrots wait beside the case.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2125 +/- ##
==========================================
+ Coverage 53.90% 53.94% +0.03%
==========================================
Files 1046 1046
Lines 73596 73606 +10
Branches 8818 8828 +10
==========================================
+ Hits 39675 39707 +32
+ Misses 33793 33771 -22
Partials 128 128 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
🔄 PR Merged - Plugin Redirected to StagingThis PR has been merged and the preview plugin has been updated to redirect to the staging version. For users testing this PR:
Staging URL: Thank you for testing! 🚀 |
🤖 I have created a release *beep* *boop* --- ## [4.37.8](v4.37.7...v4.37.8) (2026-10-26) ### Bug Fixes * **api:** preserve partial results and accurate notification counts ([#2123](#2123)) ([3371fd4](3371fd4)) * **auth:** limit repeated authorization requests ([#2094](#2094)) ([e160a4e](e160a4e)) * **auth:** prevent custom permission checks racing policy refresh ([#2125](#2125)) ([05ba1bf](05ba1bf)) * **graphql:** bound request size and expanded selections ([#2121](#2121)) ([0c60d9d](0c60d9d)) * **graphql:** filter schema metadata requests ([#2095](#2095)) ([dcd396e](dcd396e)) * **settings:** omit internal provider values ([#2096](#2096)) ([30e8d6a](30e8d6a)) * **updates:** embed internal OS checks on Tools page ([#2020](#2020)) ([b95ae92](b95ae92)) * **web:** restore right-side banner fade on legacy headers ([#2126](#2126)) ([da7b87d](da7b87d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Eli Bosley <11823237+elibosley@users.noreply.github.com>
Custom API keys could fail mixed GraphQL queries with
Cannot read properties of undefined (reading '0')inside Casbin. Authentication cleared and rebuilt the shared permission rows while another authorization check was iterating them. This was discovered during post-merge QA of #2123 at 3371fd4.This uses Casbin’s existing synchronized enforcer to serialize checks with policy mutations. Permission synchronization now removes obsolete grants and adds missing grants, preserving unchanged grants during repeated authentication. Permission changes still apply on the next authentication; no permission cache, schema change or migration is introduced.
Reviewer consideration: authorization checks now share Casbin’s dependency-owned lock. This prevents concurrent policy mutation; unchanged keys avoid policy writes. An in-flight check finishes before revocation, and later checks observe the revoked permission.
Validation: all 366 authentication tests, API type-check, API lint and authorization coverage lint passed locally. Six integration regressions cover 48 concurrent HTTP queries with granted/denied fields, unchanged refreshes, changed permissions, wildcard/empty permissions, a deterministically paused authorization check overlapping policy removal, and lock recovery after a failed check. The deterministic overlap reproduced the exact exception before the fix. Exact-head CI build/test and CodeQL passed; the advisory dependency audit still fails. The packaged plugin from run 40244541250 was installed on a managed Unraid 7.4.0-rc.1 QA VM, and all 32 packaged API dist files matched the downloaded CI payload.
Appliance QA passed 300 real HTTP requests, including 288 requests at concurrency 24 across two distinct GUEST keys. Cases covered stable grants and cross-key denial, actual unavailable Docker/libvirt backends with healthy notification data retained, denied API-key mutation without side effects, revocation and restoration on subsequent requests, empty custom grants with an explicit GUEST role, ADMIN/VIEWER compatibility, and persisted grants/revocations after service restart. Zero Casbin type errors were logged. Observed p95 request latency was 49 ms (diagnostic, not a comparative performance claim).
All five temporary keys were deleted and verified absent through the native CLI. Notification configuration remained byte-for-byte unchanged. Provider rollback completed, and a fresh check confirmed the reservation absent and the capacity available and powered off. PR remains unmerged.
Summary by CodeRabbit