perf: reduce per-assignment Casbin calls in visible assignments fast path#278
Draft
mariajgrimaldi wants to merge 12 commits into
Draft
perf: reduce per-assignment Casbin calls in visible assignments fast path#278mariajgrimaldi wants to merge 12 commits into
mariajgrimaldi wants to merge 12 commits into
Conversation
…path Three changes to cut latency in get_visible_role_assignments_for_user: 1. Pre-filter by org/scope/role before the authorization pass so Casbin never evaluates assignments that would be filtered out anyway. 2. Replace the per-assignment enforcer.enforce() loop in _filter_allowed_assignments with a single batch_enforce() call. 3. Memoize get_permissions_for_single_role within get_role_assignments so the same role key is not expanded multiple times in one call. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @mariajgrimaldi! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
… assignments Bypass Casbin enforce() entirely in _filter_allowed_assignments: - Check staff/superuser once via Django User query (early return) - Call get_scopes_for_subject_and_permission once per distinct permission type (typically 1-2 calls total) instead of N enforce() calls - Filter in Python using key_match_func for glob scope support This eliminates the is_admin_or_superuser_check overhead (ScopeData/UserData construction per item, RequestCache lookups) that dominated the previous batch_enforce path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ser_staff_or_superuser - Add filter_role_assignments_visible_to_subject to roles.py as a public API function for the scope-based visibility check previously inlined in _filter_allowed_assignments. - Add is_user_staff_or_superuser to utils.py, using RequestCache to avoid repeated DB lookups within the same request and falling back to a DB query for non-request contexts. Both users.py and matcher.py now delegate to it instead of duplicating the cache and lookup logic. - Move the staff/superuser shortcut out of _filter_allowed_assignments and up to the public callers (get_visible_role_assignments_for_user and get_visible_user_role_assignments_filtered_by_current_user). - Rename _prefilter_assignments to _filter_assignments_by_params. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove the is_user_staff_or_superuser guard from get_visible_role_assignments_for_user and get_visible_user_role_assignments_filtered_by_current_user. The scope-based approach introduced in the previous commit is fast enough for the common case; the short-circuit can be added later with an explicit test for the staff/superuser path. Also remove the corresponding short-circuit section from ADR 0014. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…le pass Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Revert engine/matcher.py to the original RequestCache-based approach. The is_user_staff_or_superuser utility was introduced alongside the short-circuit that was dropped, so remove it from utils.py as well. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t_visible_role_assignments_for_user - Rewrite TestGetVisibleRoleAssignmentsForUser with proper Expected result docstrings, AAA structure, and a setUpClass that creates Django User records so assertions are not vacuously true. - Add TestFilterRoleAssignmentsVisibleToSubject in test_roles.py to cover the new public filter_role_assignments_visible_to_subject function directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d_assignments filter_role_assignments_visible_to_subject uses key_match instead of enforce(), so the enforcer never runs is_admin_or_superuser_check. Without the early return, staff and superusers silently lose the access that check would have granted them. is_user_staff_or_superuser is added to utils with RequestCache to avoid repeated DB lookups within the same request. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Cut latency in
get_visible_role_assignments_for_userandget_visible_user_role_assignments_filtered_by_current_userfor the admin console assignments view, as described in ADR 0014.Three independent changes:
Pre-filter before authorization: A new
_filter_assignments_by_paramsfunction applies org, scope, and role filters on the flat assignment list before any authorization check. Previously, the full list was authorized first and filtered afterward on the grouped result. Assignments dropped by the filters are never evaluated for visibility.Scope-based visibility check to avoid calling enforcer() N times:
_filter_allowed_assignmentsnow delegates tofilter_role_assignments_visible_to_subject(inopenedx_authz.api.roles). Instead of callingenforcer.enforce()once per assignment, it callsget_scopes_for_subject_and_permissiononce per distinct permission type (one enforcer lookup per type, not one per assignment), then useskey_match_functo match each assignment's scope against the viewer's accessible scopes. Becausekey_match_funcbypasses the enforcer entirely,is_admin_or_superuser_checknever runs on this path. Staff and superusers return early before the filter to preserve the access that check would have granted them.Memoize role permission lookups in
get_role_assignments:get_permissions_for_single_rolewas called once per policy entry even when multiple assignments shared the same role. A local_perm_cachedict skips the lookup for role keys already resolved in the same call.Performance reports
Profiled with pyinstrument on muscat (
GET /api/authz/v1/assignments/). Reports attached below.Each
enforce()call costs 20 to 80ms due to Casbin evaluating the full policy graph per (subject, action, object) triple, including role graph traversal viahas_linkand the custom matcher function._filter_allowed_assignments/filter_role_assignments_visible_to_subject:mainenforce()calls)mainenforce()calls)MJG/perf-visible-assignmentsMJG/perf-visible-assignmentsget_visible_role_assignments_for_usertotal:mainmainMJG/perf-visible-assignmentsMJG/perf-visible-assignmentsTotal request wall time (muscat):
mainmainMJG/perf-visible-assignmentsMJG/perf-visible-assignmentsBoth branches returned identical results on muscat against the same dataset (25 assignments, same content).
Pyinstrument reports:
Testing instructions
Use the pyinstrument-instrumented endpoint to reproduce. Requires
ProfilerMiddlewareactive in the LMS settings:GET https://muscat.eu1.edunext.cloud/api/authz/v1/assignments/?profileget_visible_role_assignments_for_useris no longer the dominant cost and_filter_allowed_assignmentsdoes not appear in the trace.library_adminin one scope. Confirm the response contains only assignments within that scope andfilter_role_assignments_visible_to_subjectreplaces the oldis_user_allowedloop.?org=OrgXor?role=library_adminquery params and confirm the pre-filter is applied before the authorization pass.Merge checklist: