feat: add pluggable permission checker for RBAC extensibility

[thabofletcher]

Ticket: N/A - Internal infrastructure work

Description Of Changes

Adds a pluggable permission checker mechanism to fides.api.oauth.utils that allows extensions (like fidesplus RBAC) to override the default permission checking logic. This enables the upcoming database-driven RBAC system to integrate seamlessly without modifying core fides code.

The implementation adds:

• register_permission_checker(callback) - Register a custom permission checker callback
• clear_permission_checker() - Revert to default permission behavior
• Modified has_permissions() to use the custom checker if registered

Code Changes

• Added PermissionCheckerCallback type alias for the custom checker signature
• Added _custom_permission_checker global to store the registered callback
• Added register_permission_checker() function to register custom checkers
• Added clear_permission_checker() function to revert to default behavior
• Modified has_permissions() to delegate to custom checker when registered
• Added optional db parameter to has_permissions() for RBAC database lookups
• Added comprehensive test suite (TestCustomPermissionChecker) with 7 tests

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

[vercel]

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

2 Skipped Deployments

Project	Deployment	Actions	Updated (UTC)
fides-plus-nightly	Ignored	Preview	Feb 3, 2026 5:18am
fides-privacy-center	Ignored		Feb 3, 2026 5:18am

[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.