Make the FAB roles PATCH endpoint replace permissions, not just add them - #71933
Merged
vincbeck merged 2 commits intoAug 24, 2026
Merged
Conversation
PATCH /roles/{name} only ever added permissions found in the request
body to a role; it never revoked permissions that were on the role but
missing from the body, so there was no way to remove a permission from
a role through the stable REST API (apache#18714). The endpoint already
required "PUT"-level authorization, and every other PATCH endpoint in
this API (connections, dags, dag runs, pools, variables) treats a field
as fully replaced once it is present in the request (via
model_fields_set) or named in update_mask -- roles just never extended
that convention to the "actions" list. This also fixes update_mask
silently letting stray "actions" in the body get applied even when the
mask didn't request it.
A 2023 attempt (apache#30193) added a separate POST
.../actions/revoke endpoint instead of fixing PATCH, which required
callers to compute their own diff and stalled on bikeshedding over new
exception classes and HTTP status codes; it went stale and was closed
unmerged. This instead makes PATCH itself diff against the role's
current permissions and apply add/remove using the security manager's
existing single-item methods, so the request body can just state the
desired end state.
vincbeck
approved these changes
Aug 21, 2026
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
12 tasks
imrichardwu
pushed a commit
to imrichardwu/airflow
that referenced
this pull request
Sep 11, 2026
…hem (apache#71933) PATCH /roles/{name} only ever added permissions found in the request body to a role; it never revoked permissions that were on the role but missing from the body, so there was no way to remove a permission from a role through the stable REST API (apache#18714). The endpoint already required "PUT"-level authorization, and every other PATCH endpoint in this API (connections, dags, dag runs, pools, variables) treats a field as fully replaced once it is present in the request (via model_fields_set) or named in update_mask -- roles just never extended that convention to the "actions" list. This also fixes update_mask silently letting stray "actions" in the body get applied even when the mask didn't request it. A 2023 attempt (apache#30193) added a separate POST .../actions/revoke endpoint instead of fixing PATCH, which required callers to compute their own diff and stalled on bikeshedding over new exception classes and HTTP status codes; it went stale and was closed unmerged. This instead makes PATCH itself diff against the role's current permissions and apply add/remove using the security manager's existing single-item methods, so the request body can just state the desired end state.
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
2 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.
PATCH /roles/{name} only ever added permissions found in the request body to a role; it never revoked permissions that were on the role but missing from the body, so there was no way to remove a permission from a role through the stable REST API. The endpoint already required "PUT"-level authorization, and every other PATCH endpoint in this API (connections, dags, dag runs, pools, variables) treats a field as fully replaced once it is present in the request (via model_fields_set) or named in update_mask -- roles just never extended that convention to the "actions" list. This also fixes update_mask silently letting stray "actions" in the body get applied even when the mask didn't request it.
A 2023 attempt (#30193) added a separate POST .../actions/revoke endpoint instead of fixing PATCH, which required callers to compute their own diff and stalled on bikeshedding over new exception classes and HTTP status codes; it went stale and was closed unmerged. This instead makes PATCH itself diff against the role's current permissions and apply add/remove using the security manager's existing single-item methods, so the request body can just state the desired end state.
closes: #18714
related: #30193
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Sonnet 5) following the guidelines