Skip to content

ENG-2696: Fix permission checker resolution in user endpoints#7607

Merged
thabofletcher merged 1 commit intomainfrom
ENG-2696-fix-user-endpoint-permission-checker
Mar 10, 2026
Merged

ENG-2696: Fix permission checker resolution in user endpoints#7607
thabofletcher merged 1 commit intomainfrom
ENG-2696-fix-user-endpoint-permission-checker

Conversation

@thabofletcher
Copy link
Contributor

@thabofletcher thabofletcher commented Mar 10, 2026

Ticket ENG-2696

Description Of Changes

Restores _resolve_depends calls in get_user and get_users route handlers to ensure the permission_checker dependency is properly resolved when fidesplus overrides the dependency via dependency_overrides.

This fixes a 500 error on GET /api/v1/user/{id} when running with fidesplus RBAC enabled.

Code Changes

  • Added _resolve_depends(permission_checker, get_permission_checker) to get_user endpoint
  • Added _resolve_depends(permission_checker, get_permission_checker) to get_users endpoint

Steps to Confirm

  1. Run fidesplus with RBAC enabled (FIDESPLUS__RBAC__ENABLED=true)
  2. Navigate to User Management > click on a user profile
  3. Confirm the page loads without a 500 error

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 (covered by feat: add pluggable permission checker for RBAC extensibility #7296)
  • 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

🤖 Generated with Claude Code

Restore _resolve_depends calls in get_user and get_users route handlers
to ensure the permission_checker dependency is properly resolved when
fidesplus overrides the dependency via dependency_overrides.

This fixes a 500 error on GET /api/v1/user/{id} when running with
fidesplus RBAC enabled.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@thabofletcher thabofletcher requested a review from a team as a code owner March 10, 2026 00:52
@thabofletcher thabofletcher requested review from johnewart and removed request for a team March 10, 2026 00:52
@vercel
Copy link
Contributor

vercel bot commented Mar 10, 2026

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

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
fides-plus-nightly Ignored Ignored Mar 10, 2026 0:52am
fides-privacy-center Ignored Ignored Mar 10, 2026 0:52am

Request Review

@coderabbitai
Copy link

coderabbitai bot commented Mar 10, 2026

📝 Walkthrough

Walkthrough

This pull request adds runtime resolution of FastAPI dependencies to three user endpoint functions. The change ensures the permission_checker dependency is properly initialized when functions are called directly, independent of FastAPI's dependency injection system.

Changes

Cohort / File(s) Summary
Dependency Resolution in User Endpoints
src/fides/api/v1/endpoints/user_endpoints.py
Added runtime dependency resolution in get_user, get_users, and verify_user_read_scopes functions using _resolve_depends(permission_checker, get_permission_checker) to ensure permission_checker is available when functions are invoked directly rather than through FastAPI's DI.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 Dependencies now resolve with care,
When called directly, they're always there!
Three functions fixed, a steady hand,
FastAPI's magic, properly planned.
Permission checks shall never disappear. ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main change: restoring permission checker resolution in user endpoints to fix a dependency resolution issue.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description check ✅ Passed The pull request description is comprehensive and follows the template structure with all required sections completed.

✏️ 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 ENG-2696-fix-user-endpoint-permission-checker

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

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 10, 2026

Greptile Summary

This PR adds two _resolve_depends(permission_checker, get_permission_checker) calls — one each in get_user and get_users — to ensure the permission_checker dependency is properly resolved when those endpoint functions are called directly (i.e., outside FastAPI's DI request lifecycle), which was causing a TypeError/500 error when fidesplus invoked them with an unresolved DependsClass default.

  • Root cause: PR feat: add pluggable permission checker for RBAC extensibility #7296 added permission_checker: PermissionCheckerCallback = Depends(get_permission_checker) to both handlers and passed it to has_permissions, but omitted the _resolve_depends guard that exists in every other function following this pattern (verify_user_read_scopes, verify_oauth_client, system_manager_oauth_util.py).
  • Fix: Two targeted _resolve_depends(permission_checker, get_permission_checker) calls restore the expected behavior. In the normal FastAPI HTTP path these calls are a no-op (FastAPI already resolved the value); they only matter when the functions are called directly without going through the DI layer.
  • Scope: All other endpoints in the file that accept permission_checker as a parameter (update_user, get_managed_systems, get_managed_system_details) pass it along to verify_oauth_client, which already contains its own _resolve_depends call, so they are not affected.
  • Caveat: When called directly and _resolve_depends takes effect, it falls back to get_permission_checker()default_has_permissions rather than any fidesplus RBAC override registered via app.dependency_overrides. This is expected behaviour for code that bypasses the DI layer, and the primary goal of the fix (preventing a 500/TypeError) is achieved.

Confidence Score: 4/5

  • This PR is safe to merge — it makes a targeted, additive fix that is a no-op in the normal FastAPI HTTP path and only activates for direct function calls.
  • The change is minimal (4 lines), consistent with the established _resolve_depends pattern across the codebase, and does not alter behaviour for the standard HTTP request flow. The only uncertainty is that when _resolve_depends does take effect (direct call path), it falls back to default_has_permissions rather than any registered RBAC override, which is a known and acceptable limitation of the pattern.
  • No files require special attention.

Important Files Changed

Filename Overview
src/fides/api/v1/endpoints/user_endpoints.py Adds _resolve_depends(permission_checker, get_permission_checker) to get_user and get_users endpoint handlers, restoring consistency with the existing pattern used in verify_user_read_scopes and verify_oauth_client. The fix prevents a TypeError when these functions are called directly (bypassing FastAPI's DI) and the permission_checker parameter holds an unresolved DependsClass object.

Last reviewed commit: 5f5c8da

Copy link
Collaborator

@johnewart johnewart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼 Though really we should probably not have _resolve_depends in the first place 😄

@thabofletcher thabofletcher added this pull request to the merge queue Mar 10, 2026
Merged via the queue into main with commit 5acf39d Mar 10, 2026
57 of 58 checks passed
@thabofletcher thabofletcher deleted the ENG-2696-fix-user-endpoint-permission-checker branch March 10, 2026 16:25
mfbrown pushed a commit that referenced this pull request Mar 12, 2026
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants