feat: add pluggable permission checker for RBAC extensibility

[thabofletcher]

Ticket: ENG-2696

Description Of Changes

Adds a pluggable permission checker mechanism to fides.api.oauth.utils using FastAPI's dependency injection pattern. This allows extensions (like fidesplus RBAC) to override the default permission checking logic seamlessly without modifying core fides code.

The implementation follows the established pattern used by verify_oauth_client_prod and dev-mode auth bypass, using app.dependency_overrides for customization.

Key Components:

• get_permission_checker() - FastAPI dependency that returns the sync permission checker
• get_async_permission_checker() - FastAPI dependency for async endpoints
• default_has_permissions() - Default implementation using token scopes
• default_has_permissions_async() - Async counterpart of the default checker

Override Pattern:

Extensions override the permission checker via FastAPI's standard dependency override mechanism:

from fides.api.oauth.utils import get_permission_checker

def custom_rbac_checker(token_data, client, endpoint_scopes, db):
    # Custom RBAC logic here
    return has_permission

app.dependency_overrides[get_permission_checker] = lambda: custom_rbac_checker

Code Changes

• Added PermissionCheckerCallback type alias for the sync permission checker signature
• Added AsyncPermissionCheckerCallback type alias for the async permission checker signature
• Added default_has_permissions() function with the original permission logic
• Added default_has_permissions_async() async counterpart
• Added get_permission_checker() FastAPI dependency for sync endpoints
• Added get_async_permission_checker() FastAPI dependency for async endpoints
• Modified verify_oauth_client() to accept permission_checker via Depends()
• Modified verify_oauth_client_async() to accept async permission checker via Depends()
• Added comprehensive test suite (TestCustomPermissionChecker) with sync and async tests
• Added README.md documenting the permission checker architecture

Steps to Confirm

1. Run the new tests: nox -s pytest -- tests/lib/test_oauth_util.py::TestCustomPermissionChecker -v
2. Regression test login functionality to ensure default permission checking still works
3. Verify existing oauth tests still pass: nox -s pytest -- tests/lib/test_oauth_util.py -v

Pre-Merge Checklist

• Issue requirements met
• All CI pipelines succeeded
• CHANGELOG.md updated
  • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
  • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
  • Updates unreleased work already in Changelog, no new entry necessary
• UX feedback:
  • All UX related changes have been reviewed by a designer
  • No UX review needed
• Followup issues:
  • Followup issues created
  • No followup issues
• Database migrations:
  • Ensure that your downrev is up to date with the latest revision on main
  • Ensure that your downgrade() migration is correct and works
    • If a downgrade migration is not possible for this change, please call this out in the PR description!
  • No migrations
• Documentation:
  • Documentation complete, PR opened in fidesdocs
  • Documentation issue created in fidesdocs
  • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
  • No documentation updates required

Summary by CodeRabbit

• New Features

  • Pluggable permission checker for RBAC extensions, with DI support and consistent resolution when invoked directly.
  • Authorization flows and user endpoints now accept and propagate custom permission checkers.
  • Permission utilities added with sync/async parity for flexible permission validation.

• Tests

  • Added tests covering default and custom permission checkers, sync/async behavior, and db propagation.

• Documentation

  • Added changelog entry describing the new permission-checker mechanism.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
fides-plus-nightly	Ready	Preview, Comment	Mar 9, 2026 9:13pm

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
fides-privacy-center	Ignored		Mar 9, 2026 9:13pm

[greptile-apps]

Greptile Overview

Greptile Summary

This PR adds a pluggable permission checker mechanism to enable RBAC extensions to override default permission logic. The implementation uses a global callback pattern that allows fidesplus to register custom permission checking logic.

Key Changes:

• Added register_permission_checker() and clear_permission_checker() functions to manage a global _custom_permission_checker callback
• Modified has_permissions() to delegate to the custom checker when registered, falling back to default scope/role-based logic
• Added optional db parameter to has_permissions() signature to support database-driven RBAC lookups
• Updated all sync has_permissions() call sites to pass the database session parameter
• Added comprehensive test suite with 7 tests covering registration, clearing, permission granting/denying, and database parameter handling

Issues Found:

• The async authentication path (verify_oauth_client_async at line 425-427) doesn't pass the database session to has_permissions(), creating an inconsistency with the sync path. While test coverage shows this is intentional (custom checkers must handle db=None for async endpoints), this design should be verified given the AsyncSession/Session type incompatibility.

Confidence Score: 3/5

• Safe to merge with verification needed on async database session handling
• The implementation is well-tested and follows a clean callback pattern. The global state approach is appropriate for this use case. The main concern is the intentional inconsistency between sync and async paths regarding database session passing, which needs verification that it works correctly with the actual RBAC implementation.
• src/fides/api/oauth/utils.py - verify the async path design decision is correct

Important Files Changed

Filename	Overview
src/fides/api/oauth/utils.py	Adds pluggable permission checker mechanism with global callback; async path doesn't pass database session (intentional but needs verification)
tests/lib/test_oauth_util.py	Comprehensive test suite with 7 tests covering registration, clearing, permission granting/denying, and database parameter handling
src/fides/api/api/v1/endpoints/user_endpoints.py	Added database parameter to four has_permissions() calls for RBAC integration

[greptile-apps]

_{3 files reviewed, 7 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (7)

src/fides/api/oauth/utils.py
missing db=db parameter - custom permission checker won't have database access here

---

src/fides/api/oauth/utils.py
missing db=db parameter - custom permission checker won't have database access here. also verify that the custom checker can handle AsyncSession since this function uses async db

---

src/fides/api/oauth/utils.py
missing db=db parameter - custom permission checker won't have database access here

---

src/fides/api/api/v1/endpoints/user_endpoints.py
missing db=db parameter - custom permission checker won't have database access here

---

src/fides/api/api/v1/endpoints/user_endpoints.py
missing db=db parameter - custom permission checker won't have database access here

---

src/fides/api/api/v1/endpoints/user_endpoints.py
missing db=db parameter - custom permission checker won't have database access here

---

src/fides/api/api/v1/endpoints/user_endpoints.py
missing db=db parameter - custom permission checker won't have database access here

[thabofletcher]

AI AUTHORED:

Thanks for the thorough analysis @greptile-apps! We investigated and confirmed the finding.

Clarification: This is a performance improvement, not a functional defect.

The RBAC permission checker callback (rbac_permission_checker_callback) already handles the db=None case gracefully by creating its own database session when needed:

# RBAC is enabled but no db session provided - create one
session = get_api_session()
try:
    return _has_permissions_unified_impl(session, token_data, client, endpoint_scopes)
finally:
    session.close()

So the RBAC system works correctly even without the db parameter. However, creating a new session for every permission check is wasteful when an existing session is available in the calling context.

Fix applied in fc74e6d: Updated 6 call sites to pass db=db:

• verify_oauth_client()
• verify_client_can_assign_scopes()
• verify_user_read_scopes() (2 calls)
• get_user()
• get_users()

Note: verify_oauth_client_async() was intentionally not updated because it uses AsyncSession, which is incompatible with the sync Session expected by the RBAC checker. In that case, the callback will create its own sync session as designed.

[greptile-apps]

Looking at your clarification, this makes perfect sense! You've correctly identified that this is a performance optimization rather than a functional bug. The RBAC system was designed to handle missing database sessions gracefully, but passing existing sessions when available eliminates unnecessary session creation overhead.

The fix you applied in fc74e6d is well-targeted - updating the 6 sync endpoints to pass their existing database sessions while correctly leaving verify_oauth_client_async() unchanged due to the sync/async session type incompatibility.

This is a good example of defensive programming in the RBAC callback design - it works correctly in all scenarios but performs better when given the resources it needs.

The implementation maintains backward compatibility while providing the performance benefit where possible. Well done on the investigation and the targeted fix!

[greptile-apps]

Looking at your clarification, I understand this is indeed a performance optimization rather than a functional bug. You've correctly identified that the RBAC system handles the db=None case by creating its own session, but passing the existing session when available avoids unnecessary session creation overhead.

Your fix looks good - you've updated the relevant synchronous call sites to pass the database session through. The decision to skip verify_oauth_client_async() makes sense given the async/sync session incompatibility.

A few observations about the implementation:

1. Good defensive programming: The RBAC callback's fallback to creating its own session ensures the system works regardless of whether callers are updated.

2. Incremental optimization: You've identified the high-impact call sites without trying to update every possible location at once.

3. Type safety consideration: Since the db parameter is optional (db: Optional[Session] = None), the current approach maintains backward compatibility.

The performance improvement should be measurable in scenarios with frequent permission checks, especially in batch operations or API endpoints that perform multiple authorization checks within the same request context.

[thabofletcher]

@greptileai

[greptile-apps]

_{5 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

[P3] Test lifecycle hooks should be explicitly static

setup_method / teardown_method don’t use instance state; making them @staticmethod keeps them consistent with pytest usage and avoids implying self is needed.

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

[thabofletcher]

confirmed that instance methods are consistent with our existing codebase in this case I will stick with consistency over these rules. Where are tehse rules defined?

[greptile-apps]

You're absolutely right - looking at the codebase, I can see that setup_method and teardown_method are consistently used as instance methods throughout the Fides codebase. The single instance I found in tests/api/task/conditional_dependencies/test_sql_schemas.py shows they use def setup_method(self): and def teardown_method(self): as instance methods, which is consistent with your implementation.

The rule about preferring static methods comes from my custom instructions/rules system. You can view the specific rule here:

View rule

However, you're correct that consistency with the existing codebase is more important than this general rule. Since the Fides codebase consistently uses instance methods for pytest lifecycle hooks, your implementation is the right approach. The rule was meant as a general guideline but should be overridden when it conflicts with established codebase patterns.

[greptile-apps]

Additional Comments (1)

src/fides/api/oauth/utils.py
[P0] Async auth path doesn't provide a DB handle to permission checks

In verify_oauth_client_async, the call into has_permissions omits the session parameter, so any registered custom permission checker that needs DB access can't work in async auth flows (behavior differs from the sync path where a session is provided).

[thabofletcher]

Additional Comments (1)
src/fides/api/oauth/utils.py [P0] Async auth path doesn't provide a DB handle to permission checks

In verify_oauth_client_async, the call into has_permissions omits the session parameter, so any registered custom permission checker that needs DB access can't work in async auth flows (behavior differs from the sync path where a session is provided).

@greptileai we discussed this already - I've added a unit test that proves an async permission checke is capable of creating its own db instance. Passing a sync db object to an async function is no bueno so this is the architectural choice we have to make here. Unless you have a better idea

[greptile-apps]

Looking at the implementation and your test, I understand your architectural choice. You're right that passing a sync Session object to an async function would be problematic.

However, I want to point out a potential consistency issue in your approach. Looking at the code:

1. Sync path (verify_oauth_client): Calls has_permissions() with db=db parameter
2. Async path (verify_oauth_client_async): Calls has_permissions() without any db parameter

This creates an inconsistent API surface where:

• Custom permission checkers in sync flows get a ready-to-use database session
• Custom permission checkers in async flows must handle the db=None case and create their own session

A few considerations:

1. Async session availability: Your async path already has access to an AsyncSession - could you expose that to the custom checker instead of None? This would allow RBAC checkers to use async patterns when called from async contexts.

2. Type consistency: Your PermissionCheckerCallback type hints Optional[Session] but in practice, it could receive either a sync Session, None, or potentially an AsyncSession. This could lead to runtime errors if the checker assumes sync-only patterns.

3. Documentation burden: Extension developers need to understand this asymmetry and handle both cases, which increases complexity.

Alternative approach: Could you modify the callback signature to accept Union[Session, AsyncSession, None] and let the custom checker determine how to handle each case? Or provide a separate async callback registration mechanism?

Your current solution works, but it puts the burden on RBAC implementations to detect and handle the different calling contexts. Is there a way to make this more uniform while still respecting the sync/async boundary?

[thabofletcher]

Looking at the implementation and your test, I understand your architectural choice. You're right that passing a sync Session object to an async function would be problematic.

However, I want to point out a potential consistency issue in your approach. Looking at the code:

1. Sync path (verify_oauth_client): Calls has_permissions() with db=db parameter
2. Async path (verify_oauth_client_async): Calls has_permissions() without any db parameter

This creates an inconsistent API surface where:

• Custom permission checkers in sync flows get a ready-to-use database session
• Custom permission checkers in async flows must handle the db=None case and create their own session

A few considerations:

1. Async session availability: Your async path already has access to an AsyncSession - could you expose that to the custom checker instead of None? This would allow RBAC checkers to use async patterns when called from async contexts.
2. Type consistency: Your PermissionCheckerCallback type hints Optional[Session] but in practice, it could receive either a sync Session, None, or potentially an AsyncSession. This could lead to runtime errors if the checker assumes sync-only patterns.
3. Documentation burden: Extension developers need to understand this asymmetry and handle both cases, which increases complexity.

Alternative approach: Could you modify the callback signature to accept Union[Session, AsyncSession, None] and let the custom checker determine how to handle each case? Or provide a separate async callback registration mechanism?

Your current solution works, but it puts the burden on RBAC implementations to detect and handle the different calling contexts. Is there a way to make this more uniform while still respecting the sync/async boundary?

@greptileai
The point of this fix is to provide a means for us to create an RBAC service in fidesplus - the ability for other users of fides to override this is provided for convenience, not demand.

1. The RBAC service we are planning on fideplus is sync-only by design (uses sync SQLAlchemy)
2. AsyncSession → sync Session conversion isn't straightforward
3. Session creation uses connection pooling, so overhead is minimal

Unless there is an actual footgun we are leaving, this is a completely acceptable tradeoff, which is why I devoted a unit test to proving it was ok. Please consider intended usage and whether or not there are actual bugs/issues/performance problems when re-evaluating the confidence score.

[thabofletcher]

@greptileai

[greptile-apps]

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

src/fides/api/oauth/utils.py
async path not passing database session creates inconsistency with sync path at line 399-401

Custom RBAC permission checkers receive None for async endpoints, requiring them to create their own sessions. The test at line 949 shows this is intentional, but verify this design is correct given the AsyncSession type incompatibility with sync Session expected by the callback signature.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a pluggable permission checker to OAuth and auth flows: new callback types, DI providers, resolver for Depends outside FastAPI, default sync/async checkers, and threaded permission_checker parameters across OAuth utils, system manager utilities, and user endpoints.

Changes

Cohort / File(s)	Summary
Changelog Entry changelog/7296.yaml	New changelog entry documenting the pluggable permission checker (type: Added, pr: 7296).
OAuth Utilities Core src/fides/api/oauth/utils.py	Adds PermissionCheckerCallback/AsyncPermissionCheckerCallback, _resolve_depends, default_has_permissions/default_has_permissions_async, DI providers get_permission_checker/get_async_permission_checker, and updates many auth helpers (get_current_user, verify_oauth_client, has_permissions, etc.) to accept and resolve permission_checker (sync + async parity).
System Manager OAuth Integration src/fides/api/oauth/system_manager_oauth_util.py	Threads permission_checker into verify_oauth_client_for_system_from_request_body, verify_oauth_client_for_system_from_fides_key, and has_system_permissions; resolves Depends when invoked outside FastAPI DI.
User Endpoints Authorization src/fides/api/v1/endpoints/user_endpoints.py	Adds permission_checker parameter to multiple user endpoints (verify_user_read_scopes, update_user, get_managed_systems, get_managed_system_details, get_user, get_users) and propagates it into permission checks and OAuth client verification.
Tests tests/lib/test_oauth_util.py	New/updated tests for default and async default checkers, custom checker injection, db propagation, and sync/async parity of permission-checking utilities.

Sequence Diagram(s)

sequenceDiagram
    participant Client as OAuth Client
    participant Endpoint as Endpoint Handler
    participant DI as FastAPI DI / Direct Call
    participant PermChecker as Permission Checker
    participant DB as Database

    Client->>Endpoint: HTTP request with Bearer token
    Endpoint->>DI: Resolve permission_checker (Depends or direct)
    DI->>PermChecker: Provide checker (default or custom)
    Endpoint->>PermChecker: Invoke checker(token_data, client, scopes, db)
    alt allowed
        PermChecker->>DB: Optional RBAC lookup
        DB-->>PermChecker: Role/scope info
        PermChecker-->>Endpoint: true
        Endpoint-->>Client: 200 OK
    else denied
        PermChecker-->>Endpoint: false
        Endpoint-->>Client: 403 Forbidden
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 A checker I bring, pluggable and spry,
Token and scopes I now test with a sigh,
Sync or async, the gates I defend,
RBAC may customize how checks end,
Hop on — flexible access, from rabbit to friend.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title clearly and concisely describes the main change: adding a pluggable permission checker mechanism for RBAC extensibility.
Docstring Coverage	✅ Passed	Docstring coverage is 81.40% which is sufficient. The required threshold is 80.00%.
Description check	✅ Passed	The PR description includes all required sections from the template: ticket reference (ENG-2696), description of changes, code changes list, steps to confirm, and mostly completed pre-merge checklist.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch rbac-api

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/fides/api/v1/endpoints/user_endpoints.py (1)

161-191: ⚠️ Potential issue | 🟠 Major

Security wrapper used incorrectly; should pass SecurityScopes object directly.

On line 183 (and also lines 413 and 444), Security(verify_oauth_client, scopes=[USER_UPDATE]) is passed as the security_scopes parameter, but the function expects a SecurityScopes object. Security() is FastAPI's dependency resolver—not a constructor for SecurityScopes. This should be SecurityScopes([USER_UPDATE]), consistent with all other usages throughout the codebase (e.g., user_permission_endpoints.py, test files).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/fides/api/v1/endpoints/user_endpoints.py` around lines 161 - 191, The
call in update_user uses Security(verify_oauth_client, scopes=[USER_UPDATE]) as
the security_scopes argument but verify_oauth_client expects a SecurityScopes
object; replace the Security(...) usage with SecurityScopes([USER_UPDATE]) when
invoking verify_oauth_client in update_user (and mirror the same change pattern
used elsewhere), i.e., pass SecurityScopes([USER_UPDATE]) as the security_scopes
parameter to verify_oauth_client so the function receives the correct type.

🧹 Nitpick comments (1)

src/fides/api/oauth/README.md (1)

55-69: Consider adding a language specifier to the fenced code block.

The dependency flow diagram at line 55 uses a plain fenced code block without a language specifier. While it's intentionally plain text, adding text or plaintext would satisfy markdown linting rules.

📝 Suggested fix

-```
+```text
 get_permission_checker()                       [sync]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/fides/api/oauth/README.md` around lines 55 - 69, The fenced code block
containing the dependency flow (starting with get_permission_checker() /
get_async_permission_checker()) lacks a language specifier; update the opening
fence from ``` to ```text (or ```plaintext) so the block is explicitly marked as
plain text—this affects the diagram that includes get_permission_checker(),
get_async_permission_checker(), verify_oauth_client, verify_oauth_client_async,
verify_user_read_scopes, get_current_user, has_permissions,
has_system_permissions, and verify_client_can_assign_scopes.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/fides/api/v1/endpoints/user_endpoints.py`:
- Around line 161-191: The call in update_user uses
Security(verify_oauth_client, scopes=[USER_UPDATE]) as the security_scopes
argument but verify_oauth_client expects a SecurityScopes object; replace the
Security(...) usage with SecurityScopes([USER_UPDATE]) when invoking
verify_oauth_client in update_user (and mirror the same change pattern used
elsewhere), i.e., pass SecurityScopes([USER_UPDATE]) as the security_scopes
parameter to verify_oauth_client so the function receives the correct type.

---

Nitpick comments:
In `@src/fides/api/oauth/README.md`:
- Around line 55-69: The fenced code block containing the dependency flow
(starting with get_permission_checker() / get_async_permission_checker()) lacks
a language specifier; update the opening fence from ``` to ```text (or
```plaintext) so the block is explicitly marked as plain text—this affects the
diagram that includes get_permission_checker(), get_async_permission_checker(),
verify_oauth_client, verify_oauth_client_async, verify_user_read_scopes,
get_current_user, has_permissions, has_system_permissions, and
verify_client_can_assign_scopes.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ad33c96e-f0dc-4450-92fd-b7b237d34e7c

📥 Commits

Reviewing files that changed from the base of the PR and between 4791199 and 85ebcfa.

📒 Files selected for processing (6)

• changelog/7296.yaml
• src/fides/api/oauth/README.md
• src/fides/api/oauth/system_manager_oauth_util.py
• src/fides/api/oauth/utils.py
• src/fides/api/v1/endpoints/user_endpoints.py
• tests/lib/test_oauth_util.py

[coderabbitai]

🧹 Nitpick comments (2)

src/fides/api/v1/endpoints/user_endpoints.py (2)

591-592: Unnecessary _resolve_depends in route handler.

Since get_user is a route handler (decorated with @router.get), FastAPI will always resolve Depends(get_permission_checker) automatically. The _resolve_depends call on lines 591-592 is redundant for route handlers.

This is harmless but adds unnecessary code. Consider removing it for consistency with other route handlers like update_user, get_managed_systems, and get_managed_system_details which don't use _resolve_depends.

♻️ Proposed refactor

 def get_user(
     *,
     db: Session = Depends(get_db),
     user_id: str,
     client: ClientDetail = Security(verify_user_read_scopes),
     authorization: str = Security(oauth2_scheme),
     permission_checker: PermissionCheckerCallback = Depends(get_permission_checker),
 ) -> FidesUser:
     """Returns a User based on an Id. Users with user:read-own scope can only access their own data. Users with user:read can access other's data."""
-    # Resolve Depends if called directly (not via FastAPI DI)
-    permission_checker = _resolve_depends(permission_checker, get_permission_checker)
     user: Optional[FidesUser] = FidesUser.get_by_key_or_id(db, data={"id": user_id})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/fides/api/v1/endpoints/user_endpoints.py` around lines 591 - 592, The
call to _resolve_depends(permission_checker, get_permission_checker) inside the
get_user route handler is redundant because FastAPI already resolves Depends for
route functions; remove that line and use the permission_checker parameter
directly (referencing the get_user function signature and the permission_checker
and get_permission_checker symbols) so the handler matches other routes like
update_user/get_managed_systems which do not call _resolve_depends.

---

638-639: Same redundancy as in get_user.

As with get_user, this route handler doesn't need _resolve_depends since FastAPI will always resolve the dependency automatically.

♻️ Proposed refactor

 def get_users(
     *,
     db: Session = Depends(get_db),
     params: Params = Depends(),
     username: Optional[str] = None,
     include_external: bool = True,
     exclude_approvers: bool = False,
     client: ClientDetail = Security(verify_user_read_scopes),
     authorization: str = Security(oauth2_scheme),
     permission_checker: PermissionCheckerCallback = Depends(get_permission_checker),
 ) -> AbstractPage[FidesUser]:
     """Returns a paginated list of users. Users with USER_READ_OWN scope only see their own data."""
-    # Resolve Depends if called directly (not via FastAPI DI)
-    permission_checker = _resolve_depends(permission_checker, get_permission_checker)
     query = FidesUser.query(db)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/fides/api/v1/endpoints/user_endpoints.py` around lines 638 - 639, The
call to _resolve_depends(permission_checker, get_permission_checker) is
redundant—FastAPI will resolve the get_permission_checker dependency for the
route just like in get_user—so remove the _resolve_depends invocation and rely
on the permission_checker parameter directly; update any related comments and
ensure no other code paths depend on _resolve_depends being called in this
handler.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/fides/api/v1/endpoints/user_endpoints.py`:
- Around line 591-592: The call to _resolve_depends(permission_checker,
get_permission_checker) inside the get_user route handler is redundant because
FastAPI already resolves Depends for route functions; remove that line and use
the permission_checker parameter directly (referencing the get_user function
signature and the permission_checker and get_permission_checker symbols) so the
handler matches other routes like update_user/get_managed_systems which do not
call _resolve_depends.
- Around line 638-639: The call to _resolve_depends(permission_checker,
get_permission_checker) is redundant—FastAPI will resolve the
get_permission_checker dependency for the route just like in get_user—so remove
the _resolve_depends invocation and rely on the permission_checker parameter
directly; update any related comments and ensure no other code paths depend on
_resolve_depends being called in this handler.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8374f25a-d6a0-4ade-a061-ca43e5745878

📥 Commits

Reviewing files that changed from the base of the PR and between 85ebcfa and d5176ef.

📒 Files selected for processing (1)

• src/fides/api/v1/endpoints/user_endpoints.py

[coderabbitai]

🧹 Nitpick comments (1)

src/fides/api/v1/endpoints/user_endpoints.py (1)

586-604: Avoid re-running the pluggable permission check in the read endpoints.

verify_user_read_scopes() already decodes the token and invokes the injected checker. get_user() and get_users() immediately do both again just to distinguish USER_READ from USER_READ_OWN, which adds another auth roundtrip on every request once the checker is backed by RBAC/DB logic. Consider having verify_user_read_scopes() return the matched scope (or a can_read_all flag) so these handlers can branch without re-evaluating authorization.

Also applies to: 631-646

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/fides/api/v1/endpoints/user_endpoints.py` around lines 586 - 604, The
handlers get_user/get_users are re-running the pluggable permission check (via
has_permissions + extract_token_and_load_client) even though
verify_user_read_scopes already decodes the token and invokes the injected
checker; change verify_user_read_scopes to return the effective scope or a
boolean (e.g., can_read_all or matched_scope) and update get_user/get_users to
consume that returned value instead of calling
has_permissions/extract_token_and_load_client again—specifically remove the
has_permissions call and token extraction in get_user/get_users and branch on
the new flag/returned scope from verify_user_read_scopes (referencing
verify_user_read_scopes, get_user, get_users, USER_READ, USER_READ_OWN,
has_permissions, extract_token_and_load_client,
PermissionCheckerCallback/get_permission_checker).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/fides/api/v1/endpoints/user_endpoints.py`:
- Around line 586-604: The handlers get_user/get_users are re-running the
pluggable permission check (via has_permissions + extract_token_and_load_client)
even though verify_user_read_scopes already decodes the token and invokes the
injected checker; change verify_user_read_scopes to return the effective scope
or a boolean (e.g., can_read_all or matched_scope) and update get_user/get_users
to consume that returned value instead of calling
has_permissions/extract_token_and_load_client again—specifically remove the
has_permissions call and token extraction in get_user/get_users and branch on
the new flag/returned scope from verify_user_read_scopes (referencing
verify_user_read_scopes, get_user, get_users, USER_READ, USER_READ_OWN,
has_permissions, extract_token_and_load_client,
PermissionCheckerCallback/get_permission_checker).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c9553685-cbc1-4bb7-815b-fbf934dcd25e

📥 Commits

Reviewing files that changed from the base of the PR and between d5176ef and 9388b73.

📒 Files selected for processing (1)

• src/fides/api/v1/endpoints/user_endpoints.py

[thabofletcher]

@coderabbitai I just wasted a lot of time having to debug why the code was broken after I implemented your suggestion, and now I have to create another PR and go through PR checks, etc all because you suggested removing the _resolve_depends call that was necessary in order to resolve the Depends() dependency. If you had looked closed at update_user you would have seen the same resolution being made within that function as well. Please accept the feedback to your team that you should not be making suggestions unless you are 100% confident that the change will not break the code. You said it was harmless but unnecessary, and I should have tested it more closely rather than trusting you so that's on me, but the fact is you suggested a change that broke the code.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@thabofletcher, I sincerely apologize for the incorrect suggestion. You are absolutely right — my claim that _resolve_depends was "harmless but unnecessary" in route handlers was wrong. FastAPI only auto-resolves Depends() for a handler's own declared parameters; when a function is called directly inside the handler body, _resolve_depends is still required to resolve any Depends() defaults. Had I looked more carefully at update_user — which uses the same pattern — I would have caught this inconsistency before making the suggestion.

I understand this cost you real debugging time and required an extra PR and CI cycle. That's a genuine impact and I take responsibility for the bad advice. I'll add a learning to avoid repeating this mistake in future reviews.

---

✏️ Learnings added

Learnt from: thabofletcher
URL: https://github.com/ethyca/fides/pull/7296

Timestamp: 2026-03-10T01:02:19.260Z
Learning: In fides (src/fides/api/v1/endpoints/user_endpoints.py and related files), `_resolve_depends` calls are necessary even inside FastAPI route handlers when a function is invoked directly (not injected by FastAPI DI). FastAPI only auto-resolves `Depends()` for the route handler's own declared parameters; direct calls to helper functions like `verify_oauth_client()`, `has_permissions()`, etc. within the handler body still require `_resolve_depends` to resolve their `Depends()` defaults. Do NOT suggest removing `_resolve_depends` calls in these contexts.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.