fix(api): apply allowed_permissions in change_actions fallback (#455)#543
Conversation
The fallback path of permitted_action_names (used when a ModelAdmin declares `change_actions` without a `get_change_actions` hook) surfaced declared names as-is, with only the run view's has_change_permission gating the object. Apply each declared action's `allowed_permissions` against has_<perm>_permission(request), mirroring Django's own _filter_actions_by_permissions — defense in depth so an action the user can't run is never offered. Unknown perm / raising check denies. Low severity (the standard mixin provides get_change_actions, which filters internally — the fallback only triggers for a hand-rolled change_actions list), but closes the parity gap with the changelist bulk-action endpoint that already filters this way. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
martin-castro-laminr-ai
left a comment
There was a problem hiding this comment.
Review — architect + security [S] (merging)
This is the security follow-up I queued in my earlier review of #427/#540. Reviewed as non-author.
[S] Fail-secure semantics: _user_has_action_perms denies on:
- An unknown perm (no
has_<perm>_permissionmethod on theModelAdmin) — never grant a perm we can't verify. - A raising check (
except Exception) — never grant on the optimistic path. - A method returning falsy — straight deny.
All three are the right call: an action's allowed_permissions is part of its trust contract, and the fallback path was previously trusting it implicitly. Now it's enforced, matching Django's own _filter_actions_by_permissions and the changelist bulk-action endpoint. ✅
Test covers the fallback path explicitly (_delete_action.allowed_permissions = ('delete',) + a user whose has_delete_permission returns False → action dropped from the surface). The existing _deactivate(allowed_permissions=('change',)) case stays green (superuser has change perm).
Tier 3 (defense in depth, no contract change). CI green. Note: this code is on which is transient during the #544 migration — when Phase 2/3 moves the API to the repo, this fix needs to come along. Tracked.
Summary
Closes #455. The fallback path of
permitted_action_names(used when aModelAdmindeclareschange_actionswithout aget_change_actionshook) surfaced declared names as-is, with only the run view'shas_change_permissiongating the object. Now also filter each declared action by its callable'sallowed_permissionsagainsthas_<perm>_permission(request)— mirroring Django's own_filter_actions_by_permissions. Defense in depth: an action the user can't run is never offered.Unknown perm / raising check denies (we never grant an action whose permission we can't verify).
LOW severity: the standard mixin's
get_change_actionsalready filters internally, so this only matters for a hand-rolledchange_actionslist. Closes the parity gap with the changelist bulk-action endpoint that already filters this way.Test plan
test_detail_fallback_filters_by_allowed_permissions— an action requiringdeleteis dropped whenhas_delete_permissionreturns False (fallback path, noget_change_actions).tests/test_object_actions.py(20) green; existing_deactivate(allowed_permissions=("change",))test still passes (superuser has change perm → kept).ruff+mypyclean.🤖 Generated with Claude Code