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 7b8c6ffbbc7 Add order_by parameter to GET /permissions for pagination
consistency (#63418)
7b8c6ffbbc7 is described below
commit 7b8c6ffbbc7741ad2c063355888ee6ba6475377f
Author: Henry Chen <[email protected]>
AuthorDate: Thu Mar 12 23:31:34 2026 +0800
Add order_by parameter to GET /permissions for pagination consistency
(#63418)
GET /permissions lacked the order_by parameter present on other list
endpoints (e.g. GET /roles), causing non-deterministic pagination.
---
.../openapi/v2-fab-auth-manager-generated.yaml | 15 +++++
.../fab/auth_manager/api_fastapi/routes/roles.py | 6 +-
.../fab/auth_manager/api_fastapi/services/roles.py | 11 +++-
.../auth_manager/api_fastapi/routes/test_roles.py | 71 +++++++++++++++++++++-
.../api_fastapi/services/test_roles.py | 46 +++++++++++++-
5 files changed, 142 insertions(+), 7 deletions(-)
diff --git
a/providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/openapi/v2-fab-auth-manager-generated.yaml
b/providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/openapi/v2-fab-auth-manager-generated.yaml
index 9ad5472e709..5d81b0b416f 100644
---
a/providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/openapi/v2-fab-auth-manager-generated.yaml
+++
b/providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/openapi/v2-fab-auth-manager-generated.yaml
@@ -409,6 +409,15 @@ paths:
- OAuth2PasswordBearer: []
- HTTPBearer: []
parameters:
+ - name: order_by
+ in: query
+ required: false
+ schema:
+ type: string
+ description: Field to order by. Prefix with '-' for descending.
+ default: id
+ title: Order By
+ description: Field to order by. Prefix with '-' for descending.
- name: offset
in: query
required: false
@@ -434,6 +443,12 @@ paths:
application/json:
schema:
$ref: '#/components/schemas/PermissionCollectionResponse'
+ '400':
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/HTTPExceptionResponse'
+ description: Bad Request
'401':
content:
application/json:
diff --git
a/providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/routes/roles.py
b/providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/routes/roles.py
index 64668b56659..3b799905a56 100644
---
a/providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/routes/roles.py
+++
b/providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/routes/roles.py
@@ -132,6 +132,7 @@ def patch_role(
response_model=PermissionCollectionResponse,
responses=create_openapi_http_exception_doc(
[
+ status.HTTP_400_BAD_REQUEST,
status.HTTP_401_UNAUTHORIZED,
status.HTTP_403_FORBIDDEN,
status.HTTP_500_INTERNAL_SERVER_ERROR,
@@ -140,9 +141,10 @@ def patch_role(
dependencies=[Depends(requires_fab_custom_view("GET",
permissions.RESOURCE_ROLE))],
)
def get_permissions(
+ order_by: str = Query("id", description="Field to order by. Prefix with
'-' for descending."),
limit: int = Depends(get_effective_limit()),
offset: int = Query(0, ge=0, description="Number of items to skip before
starting to collect results."),
-):
+) -> PermissionCollectionResponse:
"""List all action-resource (permission) pairs."""
with get_application_builder():
- return FABAuthManagerRoles.get_permissions(limit=limit, offset=offset)
+ return FABAuthManagerRoles.get_permissions(order_by=order_by,
limit=limit, offset=offset)
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 44f82caa95b..dda6515c265 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
@@ -164,13 +164,22 @@ class FABAuthManagerRoles:
return RoleResponse.model_validate(update_data)
@classmethod
- def get_permissions(cls, *, limit: int, offset: int) ->
PermissionCollectionResponse:
+ def get_permissions(cls, *, order_by: str, limit: int, offset: int) ->
PermissionCollectionResponse:
security_manager = get_fab_auth_manager().security_manager
session = security_manager.session
total_entries =
session.scalars(select(func.count(Permission.id))).one()
+ ordering = build_ordering(
+ order_by,
+ allowed={
+ "id": Permission.id,
+ "action_id": Permission.action_id,
+ "resource_id": Permission.resource_id,
+ },
+ )
query = (
select(Permission)
.options(joinedload(Permission.action),
joinedload(Permission.resource))
+ .order_by(ordering)
.offset(offset)
.limit(limit)
)
diff --git
a/providers/fab/tests/unit/fab/auth_manager/api_fastapi/routes/test_roles.py
b/providers/fab/tests/unit/fab/auth_manager/api_fastapi/routes/test_roles.py
index bdfdf5ac99a..a1ed2e453f7 100644
--- a/providers/fab/tests/unit/fab/auth_manager/api_fastapi/routes/test_roles.py
+++ b/providers/fab/tests/unit/fab/auth_manager/api_fastapi/routes/test_roles.py
@@ -580,7 +580,7 @@ class TestRoles:
resp = test_client.get("/fab/v1/permissions")
assert resp.status_code == 200
assert resp.json() == dummy.model_dump(by_alias=True)
-
mock_permissions.get_permissions.assert_called_once_with(limit=100, offset=0)
+
mock_permissions.get_permissions.assert_called_once_with(order_by="id",
limit=100, offset=0)
@patch("airflow.providers.fab.auth_manager.api_fastapi.routes.roles.FABAuthManagerRoles")
@patch("airflow.providers.fab.auth_manager.api_fastapi.security.get_auth_manager")
@@ -599,3 +599,72 @@ class TestRoles:
resp = test_client.get("/fab/v1/permissions")
assert resp.status_code == 403
mock_permissions.get_permissions.assert_not_called()
+
+
@patch("airflow.providers.fab.auth_manager.api_fastapi.routes.roles.FABAuthManagerRoles")
+
@patch("airflow.providers.fab.auth_manager.api_fastapi.security.get_auth_manager")
+ @patch(
+
"airflow.providers.fab.auth_manager.api_fastapi.routes.roles.get_application_builder",
+ return_value=_noop_cm(),
+ )
+ @patch("airflow.providers.fab.auth_manager.api_fastapi.parameters.conf")
+ def test_get_permissions_passes_params_and_clamps_limit(
+ self,
+ conf_mock,
+ mock_get_application_builder,
+ mock_get_auth_manager,
+ mock_permissions,
+ test_client,
+ as_user,
+ ):
+ conf_mock.getint.side_effect = lambda section, option: {
+ "maximum_page_limit": 50,
+ "fallback_page_limit": 20,
+ }[option]
+
+ mgr = MagicMock()
+ mgr.is_authorized_custom_view.return_value = True
+ mock_get_auth_manager.return_value = mgr
+
+ dummy = PermissionCollectionResponse(permissions=[], total_entries=0)
+ mock_permissions.get_permissions.return_value = dummy
+
+ with as_user():
+ resp = test_client.get(
+ "/fab/v1/permissions", params={"order_by": "-id", "limit":
1000, "offset": 5}
+ )
+ assert resp.status_code == 200
+ assert resp.json() == dummy.model_dump(by_alias=True)
+
mock_permissions.get_permissions.assert_called_once_with(order_by="-id",
limit=50, offset=5)
+
+
@patch("airflow.providers.fab.auth_manager.api_fastapi.routes.roles.FABAuthManagerRoles")
+
@patch("airflow.providers.fab.auth_manager.api_fastapi.security.get_auth_manager")
+ @patch(
+
"airflow.providers.fab.auth_manager.api_fastapi.routes.roles.get_application_builder",
+ return_value=_noop_cm(),
+ )
+ @patch("airflow.providers.fab.auth_manager.api_fastapi.parameters.conf")
+ def test_get_permissions_uses_fallback_when_limit_zero(
+ self,
+ conf_mock,
+ mock_get_application_builder,
+ mock_get_auth_manager,
+ mock_permissions,
+ test_client,
+ as_user,
+ ):
+ conf_mock.getint.side_effect = lambda section, option: {
+ "maximum_page_limit": 100,
+ "fallback_page_limit": 33,
+ }[option]
+
+ mgr = MagicMock()
+ mgr.is_authorized_custom_view.return_value = True
+ mock_get_auth_manager.return_value = mgr
+
+ dummy = PermissionCollectionResponse(permissions=[], total_entries=0)
+ mock_permissions.get_permissions.return_value = dummy
+
+ with as_user():
+ resp = test_client.get("/fab/v1/permissions", params={"limit": 0})
+ assert resp.status_code == 200
+
mock_permissions.get_permissions.assert_called_once_with(order_by="id",
limit=33, offset=0)
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 1dec2954b60..88620b80b23 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
@@ -384,7 +384,7 @@ class TestRolesService:
fab_auth_manager.security_manager = MagicMock(session=session)
get_fab_auth_manager.return_value = fab_auth_manager
- out = FABAuthManagerRoles.get_permissions(limit=10, offset=0)
+ out = FABAuthManagerRoles.get_permissions(order_by="id", limit=10,
offset=0)
assert isinstance(out, PermissionCollectionResponse)
assert out.total_entries == 1
assert len(out.permissions) == 1
@@ -402,7 +402,7 @@ class TestRolesService:
fab_auth_manager.security_manager = MagicMock(session=session)
get_fab_auth_manager.return_value = fab_auth_manager
- out = FABAuthManagerRoles.get_permissions(limit=10, offset=0)
+ out = FABAuthManagerRoles.get_permissions(order_by="id", limit=10,
offset=0)
assert out.total_entries == 0
assert out.permissions == []
@@ -426,7 +426,7 @@ class TestRolesService:
fab_auth_manager.security_manager = MagicMock(session=session)
get_fab_auth_manager.return_value = fab_auth_manager
- out = FABAuthManagerRoles.get_permissions(limit=10, offset=0)
+ out = FABAuthManagerRoles.get_permissions(order_by="id", limit=10,
offset=0)
assert isinstance(out, PermissionCollectionResponse)
assert out.total_entries == 2
assert len(out.permissions) == 2
@@ -436,3 +436,43 @@ class TestRolesService:
assert out.permissions[1] == ActionResource(
action=Action(name="can_edit"), resource=Resource(name="DAG")
)
+
+
@patch("airflow.providers.fab.auth_manager.api_fastapi.services.roles.build_ordering")
+ def test_get_permissions_ordering_happy_path(self, build_ordering,
get_fab_auth_manager):
+ perm_obj = types.SimpleNamespace(
+ action=types.SimpleNamespace(name="can_read"),
+ resource=types.SimpleNamespace(name="DAG"),
+ )
+ session = MagicMock()
+ session.scalars.side_effect = [
+ types.SimpleNamespace(one=lambda: 1),
+ types.SimpleNamespace(all=lambda: [perm_obj]),
+ ]
+ fab_auth_manager = MagicMock()
+ fab_auth_manager.security_manager = MagicMock(session=session)
+ get_fab_auth_manager.return_value = fab_auth_manager
+
+ build_ordering.return_value = column("id").desc()
+
+ out = FABAuthManagerRoles.get_permissions(order_by="-id", limit=10,
offset=0)
+
+ assert out.total_entries == 1
+ assert len(out.permissions) == 1
+
+ build_ordering.assert_called_once()
+ args, kwargs = build_ordering.call_args
+ assert args[0] == "-id"
+ assert set(kwargs["allowed"].keys()) == {"id", "action_id",
"resource_id"}
+
+
@patch("airflow.providers.fab.auth_manager.api_fastapi.services.roles.build_ordering")
+ def test_get_permissions_invalid_order_by_bubbles_400(self,
build_ordering, get_fab_auth_manager):
+ session = MagicMock()
+ fab_auth_manager = MagicMock()
+ fab_auth_manager.security_manager = MagicMock(session=session)
+ get_fab_auth_manager.return_value = fab_auth_manager
+
+ build_ordering.side_effect = HTTPException(status_code=400,
detail="disallowed")
+
+ with pytest.raises(HTTPException) as ex:
+ FABAuthManagerRoles.get_permissions(order_by="nope", limit=10,
offset=0)
+ assert ex.value.status_code == 400
