This is an automated email from the ASF dual-hosted git repository.
vincbeck pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/airflow.git
The following commit(s) were added to refs/heads/main by this push:
new 9b9fc0eec64 Make the FAB roles PATCH endpoint replace permissions, not
just add them (#71933)
9b9fc0eec64 is described below
commit 9b9fc0eec643a3dadb4951d64eb6ddbd3f1a57fc
Author: Dheeren Mohta <[email protected]>
AuthorDate: Mon Aug 24 22:03:03 2026 +0530
Make the FAB roles PATCH endpoint replace permissions, not just add them
(#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 (#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/airflow#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.
---
.../fab/auth_manager/api_fastapi/services/roles.py | 67 ++++--
.../api_fastapi/services/test_roles.py | 228 ++++++++++++++++-----
2 files changed, 225 insertions(+), 70 deletions(-)
diff --git
a/providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/services/roles.py
b/providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/services/roles.py
index dda6515c265..8d64a650da6 100644
---
a/providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/services/roles.py
+++
b/providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/services/roles.py
@@ -137,31 +137,62 @@ class FABAuthManagerRoles:
detail=f"Role with name {name!r} does not exist.",
)
+ # A field is only touched if the client actually sent it (tracked by
pydantic's
+ # `model_fields_set`, independent of default values). With no
update_mask that means
+ # every field present in the request body -- consistent with this
endpoint requiring
+ # "PUT"-level authorization and with how the other PATCH endpoints in
this API
+ # (connections, dags, dag runs, pools, variables) resolve which fields
to replace.
+ fields_to_update = set(body.model_fields_set)
if update_mask:
- fields_to_update = {f.strip() for f in update_mask.split(",") if
f.strip()}
- update_data = RoleResponse.model_validate(existing)
-
- for field in fields_to_update:
- if field == "actions":
- update_data.permissions = body.permissions
- elif hasattr(body, field):
- setattr(update_data, field, getattr(body, field))
- else:
+ requested_fields = {f.strip() for f in update_mask.split(",") if
f.strip()}
+ for field in requested_fields:
+ if field != "actions" and not hasattr(body, field):
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail=f"'{field}' in update_mask is unknown",
)
- else:
- update_data = RoleResponse(name=body.name,
permissions=body.permissions or [])
+ # "actions" is the external (JSON) name for the "permissions"
attribute.
+ normalized_fields = {"permissions" if field == "actions" else
field for field in requested_fields}
+ fields_to_update &= normalized_fields
- perms: list[tuple[str, str]] = [(ar.action.name, ar.resource.name) for
ar in (body.permissions or [])]
- cls._check_action_and_resource(security_manager, perms)
- security_manager.bulk_sync_roles([{"role": name, "perms": perms}])
+ update_data = RoleResponse.model_validate(existing)
+ if "permissions" in fields_to_update:
+ cls._replace_role_permissions(security_manager, existing,
body.permissions or [])
+ update_data.permissions = body.permissions or []
+ if "name" in fields_to_update:
+ update_data.name = body.name
+
+ if update_data.name != existing.name:
+ security_manager.update_role(role_id=existing.id,
name=update_data.name)
+ return update_data
- new_name = update_data.name
- if new_name and new_name != existing.name:
- security_manager.update_role(role_id=existing.id, name=new_name)
- return RoleResponse.model_validate(update_data)
+ @classmethod
+ def _replace_role_permissions(
+ cls,
+ security_manager: FabAirflowSecurityManagerOverride,
+ role: Role,
+ permissions: list[ActionResource],
+ ) -> None:
+ """
+ Make the role's permissions match `permissions` exactly.
+
+ Unlike the additive sync used on role creation, a PATCH that touches
the permission
+ set must also revoke permissions currently on the role that are absent
from the
+ request -- otherwise permissions could be added but never removed via
the API.
+ """
+ target_pairs = {(ar.action.name, ar.resource.name) for ar in
permissions}
+ cls._check_action_and_resource(security_manager, list(target_pairs))
+
+ current_permissions = {(p.action.name, p.resource.name): p for p in
role.permissions}
+
+ for action_name, resource_name in target_pairs -
current_permissions.keys():
+ permission = security_manager.get_permission(
+ action_name, resource_name
+ ) or security_manager.create_permission(action_name, resource_name)
+ security_manager.add_permission_to_role(role, permission)
+
+ for pair in current_permissions.keys() - target_pairs:
+ security_manager.remove_permission_from_role(role,
current_permissions[pair])
@classmethod
def get_permissions(cls, *, order_by: str, limit: int, offset: int) ->
PermissionCollectionResponse:
diff --git
a/providers/fab/tests/unit/fab/auth_manager/api_fastapi/services/test_roles.py
b/providers/fab/tests/unit/fab/auth_manager/api_fastapi/services/test_roles.py
index 88620b80b23..9bdd0235417 100644
---
a/providers/fab/tests/unit/fab/auth_manager/api_fastapi/services/test_roles.py
+++
b/providers/fab/tests/unit/fab/auth_manager/api_fastapi/services/test_roles.py
@@ -29,6 +29,7 @@ from
airflow.providers.fab.auth_manager.api_fastapi.datamodels.roles import (
ActionResource,
PermissionCollectionResponse,
Resource,
+ RoleBody,
)
from airflow.providers.fab.auth_manager.api_fastapi.services.roles import (
FABAuthManagerRoles,
@@ -59,6 +60,16 @@ def _make_role_obj(name: str, perms: list[tuple[str, str]]):
return types.SimpleNamespace(id=1, name=name, permissions=perm_objs)
+def _make_role_body(name: str, actions: list[tuple[str, str]] | None = None,
*, include_actions: bool = True):
+ """Build a real `RoleBody` so `model_fields_set` reflects which fields the
caller supplied."""
+ kwargs: dict = {"name": name}
+ if include_actions:
+ kwargs["actions"] = [
+ ActionResource(action=Action(name=a), resource=Resource(name=r))
for (a, r) in (actions or [])
+ ]
+ return RoleBody(**kwargs)
+
+
class _FakeScalarCount:
def __init__(self, value: int):
self._value = value
@@ -263,60 +274,168 @@ class TestRolesService:
# PATCH /roles/{name}
- def test_patch_role_success(self, get_fab_auth_manager, fab_auth_manager,
security_manager):
- role = _make_role_obj("viewer", [("can_read", "DAG")])
+ def test_patch_role_rename_success(self, get_fab_auth_manager,
fab_auth_manager, security_manager):
+ role = _make_role_obj("viewer", [("can_edit", "DAG")])
security_manager.find_role.return_value = role
+ security_manager.get_permission.return_value = types.SimpleNamespace(
+ action=types.SimpleNamespace(name="can_edit"),
resource=types.SimpleNamespace(name="DAG")
+ )
fab_auth_manager.security_manager = security_manager
get_fab_auth_manager.return_value = fab_auth_manager
- body = types.SimpleNamespace(
- name="viewer",
- permissions=[
- types.SimpleNamespace(
- action=types.SimpleNamespace(name="can_edit"),
- resource=types.SimpleNamespace(name="DAG"),
- )
- ],
- )
+ body = _make_role_body("editor", [("can_edit", "DAG")])
+
out = FABAuthManagerRoles.patch_role(body=body, name="viewer")
- assert out.name == "viewer"
+
+ assert out.name == "editor"
assert out.permissions
assert out.permissions[0].action.name == "can_edit"
assert out.permissions[0].resource.name == "DAG"
+ # The permission set is unchanged, so nothing should be added or
removed.
+ security_manager.add_permission_to_role.assert_not_called()
+ security_manager.remove_permission_from_role.assert_not_called()
+ security_manager.update_role.assert_called_once_with(role_id=role.id,
name="editor")
- def test_patch_role_rename_success(self, get_fab_auth_manager,
fab_auth_manager, security_manager):
- role = _make_role_obj("viewer", [("can_edit", "DAG")])
+ def test_patch_role_adds_missing_permission(
+ self, get_fab_auth_manager, fab_auth_manager, security_manager
+ ):
+ """Regression test: permissions present in the body but not yet on the
role are added."""
+ role = _make_role_obj("viewer", [])
security_manager.find_role.return_value = role
+ new_permission = types.SimpleNamespace(
+ action=types.SimpleNamespace(name="can_edit"),
resource=types.SimpleNamespace(name="DAG")
+ )
+ security_manager.get_permission.return_value = new_permission
fab_auth_manager.security_manager = security_manager
get_fab_auth_manager.return_value = fab_auth_manager
- body = types.SimpleNamespace(
- name="editor",
- permissions=[
- types.SimpleNamespace(
- action=types.SimpleNamespace(name="can_edit"),
- resource=types.SimpleNamespace(name="DAG"),
- )
- ],
- )
+ body = _make_role_body("viewer", [("can_edit", "DAG")])
+
out = FABAuthManagerRoles.patch_role(body=body, name="viewer")
- assert out.name == "editor"
+
assert out.permissions
assert out.permissions[0].action.name == "can_edit"
assert out.permissions[0].resource.name == "DAG"
+ security_manager.get_permission.assert_called_once_with("can_edit",
"DAG")
+ security_manager.create_permission.assert_not_called()
+ security_manager.add_permission_to_role.assert_called_once_with(role,
new_permission)
+ security_manager.remove_permission_from_role.assert_not_called()
- def test_patch_role_with_update_mask(self, get_fab_auth_manager,
fab_auth_manager, security_manager):
+ def test_patch_role_creates_permission_when_missing_from_db(
+ self, get_fab_auth_manager, fab_auth_manager, security_manager
+ ):
+ """When the (action, resource) pair has no Permission row yet, one is
created before adding."""
+ role = _make_role_obj("viewer", [])
+ security_manager.find_role.return_value = role
+ security_manager.get_permission.return_value = None
+ created_permission = types.SimpleNamespace(
+ action=types.SimpleNamespace(name="can_edit"),
resource=types.SimpleNamespace(name="DAG")
+ )
+ security_manager.create_permission.return_value = created_permission
+ fab_auth_manager.security_manager = security_manager
+ get_fab_auth_manager.return_value = fab_auth_manager
+ body = _make_role_body("viewer", [("can_edit", "DAG")])
+
+ FABAuthManagerRoles.patch_role(body=body, name="viewer")
+
+ security_manager.create_permission.assert_called_once_with("can_edit",
"DAG")
+ security_manager.add_permission_to_role.assert_called_once_with(role,
created_permission)
+
+ def test_patch_role_removes_permission_absent_from_body(
+ self, get_fab_auth_manager, fab_auth_manager, security_manager
+ ):
+ """The core issue #18714 regression test: a permission missing from
the PATCH body
+ must be revoked from the role, not silently kept."""
+ role = _make_role_obj("viewer", [("can_read", "DAG"), ("can_edit",
"DAG")])
+ security_manager.find_role.return_value = role
+ security_manager.get_permission.return_value = types.SimpleNamespace(
+ action=types.SimpleNamespace(name="can_edit"),
resource=types.SimpleNamespace(name="DAG")
+ )
+ fab_auth_manager.security_manager = security_manager
+ get_fab_auth_manager.return_value = fab_auth_manager
+ body = _make_role_body("viewer", [("can_edit", "DAG")])
+
+ out = FABAuthManagerRoles.patch_role(body=body, name="viewer")
+
+ assert {(p.action.name, p.resource.name) for p in out.permissions} ==
{("can_edit", "DAG")}
+ security_manager.add_permission_to_role.assert_not_called()
+ removed_role, removed_permission =
security_manager.remove_permission_from_role.call_args.args
+ assert removed_role is role
+ assert (removed_permission.action.name,
removed_permission.resource.name) == ("can_read", "DAG")
+
+ def test_patch_role_adds_and_removes_permissions_together(
+ self, get_fab_auth_manager, fab_auth_manager, security_manager
+ ):
+ role = _make_role_obj("viewer", [("can_read", "DAG"), ("can_edit",
"DAG")])
+ security_manager.find_role.return_value = role
+ security_manager.get_permission.return_value = None
+ created_permission = types.SimpleNamespace(
+ action=types.SimpleNamespace(name="can_read"),
resource=types.SimpleNamespace(name="Connections")
+ )
+ security_manager.create_permission.return_value = created_permission
+ security_manager.get_resource.side_effect = lambda n: (
+ object() if n in {"DAG", "Connections"} else None
+ )
+ fab_auth_manager.security_manager = security_manager
+ get_fab_auth_manager.return_value = fab_auth_manager
+ # can_edit/DAG is kept as-is, can_read/DAG is dropped,
can_read/Connections is added.
+ body = _make_role_body("viewer", [("can_edit", "DAG"), ("can_read",
"Connections")])
+
+ out = FABAuthManagerRoles.patch_role(body=body, name="viewer")
+
+ assert {(p.action.name, p.resource.name) for p in out.permissions} == {
+ ("can_edit", "DAG"),
+ ("can_read", "Connections"),
+ }
+ security_manager.add_permission_to_role.assert_called_once_with(role,
created_permission)
+ removed_role, removed_permission =
security_manager.remove_permission_from_role.call_args.args
+ assert removed_role is role
+ assert (removed_permission.action.name,
removed_permission.resource.name) == ("can_read", "DAG")
+
+ def test_patch_role_explicit_empty_actions_removes_all_permissions(
+ self, get_fab_auth_manager, fab_auth_manager, security_manager
+ ):
+ """Sending `"actions": []` is an explicit instruction to clear the
permission set."""
+ role = _make_role_obj("viewer", [("can_read", "DAG"), ("can_edit",
"DAG")])
+ security_manager.find_role.return_value = role
+ fab_auth_manager.security_manager = security_manager
+ get_fab_auth_manager.return_value = fab_auth_manager
+ body = _make_role_body("viewer", [])
+
+ out = FABAuthManagerRoles.patch_role(body=body, name="viewer")
+
+ assert out.permissions == []
+ assert security_manager.remove_permission_from_role.call_count == 2
+ security_manager.add_permission_to_role.assert_not_called()
+
+ def test_patch_role_without_actions_key_leaves_permissions_untouched(
+ self, get_fab_auth_manager, fab_auth_manager, security_manager
+ ):
+ """A rename-only PATCH that never mentions "actions" must not touch
permissions at
+ all -- omitting the field is not the same as sending an empty list."""
role = _make_role_obj("viewer", [("can_read", "DAG")])
security_manager.find_role.return_value = role
fab_auth_manager.security_manager = security_manager
get_fab_auth_manager.return_value = fab_auth_manager
- body = types.SimpleNamespace(
- name="viewer1",
- permissions=[
- types.SimpleNamespace(
- action=types.SimpleNamespace(name="can_edit"),
- resource=types.SimpleNamespace(name="DAG"),
- )
- ],
+ body = _make_role_body("editor", include_actions=False)
+
+ out = FABAuthManagerRoles.patch_role(body=body, name="viewer")
+
+ assert out.name == "editor"
+ assert [(p.action.name, p.resource.name) for p in out.permissions] ==
[("can_read", "DAG")]
+ security_manager.add_permission_to_role.assert_not_called()
+ security_manager.remove_permission_from_role.assert_not_called()
+ security_manager.get_permission.assert_not_called()
+
+ def test_patch_role_with_update_mask(self, get_fab_auth_manager,
fab_auth_manager, security_manager):
+ role = _make_role_obj("viewer", [("can_read", "DAG")])
+ security_manager.find_role.return_value = role
+ security_manager.get_permission.return_value = types.SimpleNamespace(
+ action=types.SimpleNamespace(name="can_edit"),
resource=types.SimpleNamespace(name="DAG")
)
+ fab_auth_manager.security_manager = security_manager
+ get_fab_auth_manager.return_value = fab_auth_manager
+ # "name" differs from the role's current name but is excluded by the
mask.
+ body = _make_role_body("viewer1", [("can_edit", "DAG")])
+
out = FABAuthManagerRoles.patch_role(
body=body,
name="viewer",
@@ -326,23 +445,19 @@ class TestRolesService:
assert out.permissions
assert out.permissions[0].action.name == "can_edit"
assert out.permissions[0].resource.name == "DAG"
+ security_manager.update_role.assert_not_called()
- def test_patch_role_rename_with_update_mask(
+ def test_patch_role_rename_with_update_mask_leaves_permissions_untouched(
self, get_fab_auth_manager, fab_auth_manager, security_manager
):
+ """update_mask=name must not apply the body's `actions`, even though
the body
+ includes a different permission set (the pre-fix code ignored the mask
here)."""
role = _make_role_obj("viewer", [("can_read", "DAG")])
security_manager.find_role.return_value = role
fab_auth_manager.security_manager = security_manager
get_fab_auth_manager.return_value = fab_auth_manager
- body = types.SimpleNamespace(
- name="viewer1",
- permissions=[
- types.SimpleNamespace(
- action=types.SimpleNamespace(name="can_edit"),
- resource=types.SimpleNamespace(name="DAG"),
- )
- ],
- )
+ body = _make_role_body("viewer1", [("can_edit", "DAG")])
+
out = FABAuthManagerRoles.patch_role(
body=body,
name="viewer",
@@ -352,20 +467,29 @@ class TestRolesService:
assert out.permissions
assert out.permissions[0].action.name == "can_read"
assert out.permissions[0].resource.name == "DAG"
+ security_manager.add_permission_to_role.assert_not_called()
+ security_manager.remove_permission_from_role.assert_not_called()
+ security_manager.get_permission.assert_not_called()
+
+ def test_patch_role_unknown_update_mask_field(
+ self, get_fab_auth_manager, fab_auth_manager, security_manager
+ ):
+ role = _make_role_obj("viewer", [])
+ security_manager.find_role.return_value = role
+ fab_auth_manager.security_manager = security_manager
+ get_fab_auth_manager.return_value = fab_auth_manager
+ body = _make_role_body("viewer", [])
+
+ with pytest.raises(HTTPException) as ex:
+ FABAuthManagerRoles.patch_role(body=body, name="viewer",
update_mask="unknown_field")
+ assert ex.value.status_code == 400
+ assert ex.value.detail == "'unknown_field' in update_mask is unknown"
def test_patch_role_not_found(self, get_fab_auth_manager,
fab_auth_manager, security_manager):
security_manager.find_role.return_value = None
fab_auth_manager.security_manager = security_manager
get_fab_auth_manager.return_value = fab_auth_manager
- body = types.SimpleNamespace(
- name="viewer",
- permissions=[
- types.SimpleNamespace(
- action=types.SimpleNamespace(name="can_edit"),
- resource=types.SimpleNamespace(name="DAG"),
- )
- ],
- )
+ body = _make_role_body("viewer", [("can_edit", "DAG")])
with pytest.raises(HTTPException) as ex:
FABAuthManagerRoles.patch_role(body=body, name="viewer")
assert ex.value.status_code == 404
