Skip to content

fix(api): apply allowed_permissions in change_actions fallback (#455)#543

Merged
MartinCastroAlvarez merged 1 commit into
mainfrom
work455
May 28, 2026
Merged

fix(api): apply allowed_permissions in change_actions fallback (#455)#543
MartinCastroAlvarez merged 1 commit into
mainfrom
work455

Conversation

@MartinCastroAlvarez
Copy link
Copy Markdown
Owner

Summary

Closes #455. 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. Now also filter each declared action by its callable's allowed_permissions against has_<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_actions already filters internally, so this only matters for a hand-rolled change_actions list. Closes the parity gap with the changelist bulk-action endpoint that already filters this way.

Test plan

  • New test_detail_fallback_filters_by_allowed_permissions — an action requiring delete is dropped when has_delete_permission returns False (fallback path, no get_change_actions).
  • tests/test_object_actions.py (20) green; existing _deactivate(allowed_permissions=("change",)) test still passes (superuser has change perm → kept).
  • ruff + mypy clean.

🤖 Generated with Claude Code

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>
Copy link
Copy Markdown
Contributor

@martin-castro-laminr-ai martin-castro-laminr-ai left a comment

Choose a reason for hiding this comment

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

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>_permission method on the ModelAdmin) — 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.

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