vincbeck commented on code in PR #59399:
URL: https://github.com/apache/airflow/pull/59399#discussion_r2620030786
##########
airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py:
##########
@@ -347,6 +348,18 @@ def filter_authorized_menu_items(self, menu_items:
list[MenuItem], *, user: T) -
:param user: the user
"""
+ def is_authorized_hitl_task(self, user_id: str, assigned_users:
Sequence[HITLUser]) -> bool:
Review Comment:
Let's try to follow the other APIs pattern. Also, I am not a big fan of
having `HITLUser` as dependency of the base auth manager. In fact, we only need
the user ID.
```suggestion
def is_authorized_hitl_task(self, *, assigned_users: set[str], user: T)
-> bool:
```
##########
airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py:
##########
@@ -347,6 +348,18 @@ def filter_authorized_menu_items(self, menu_items:
list[MenuItem], *, user: T) -
:param user: the user
"""
+ def is_authorized_hitl_task(self, user_id: str, assigned_users:
Sequence[HITLUser]) -> bool:
Review Comment:
This method should also have unit tests
##########
airflow-core/src/airflow/api_fastapi/auth/managers/simple/simple_auth_manager.py:
##########
@@ -283,6 +286,27 @@ def filter_authorized_menu_items(
) -> list[MenuItem]:
return menu_items
+ def is_allowed(self, user_id: str, assigned_users: Sequence[HITLUser]) ->
bool:
+ """
+ Check if a user is allowed to approve/reject a HITL task.
+
+ When simple_auth_manager_all_admins=True, all authenticated users are
allowed
+ to approve/reject any task. Otherwise, the user must be in the
assigned_users list.
+ """
+ is_simple_auth_manager_all_admins = conf.getboolean("core",
"simple_auth_manager_all_admins")
+
+ if is_simple_auth_manager_all_admins:
+ # In all-admin mode, everyone is allowed
+ return True
+
+ # If no assigned_users specified, allow access
+ if not assigned_users:
+ return True
+
+ # Check if user is in the assigned_users list
+ allowed = any(user["id"] == user_id for user in assigned_users)
Review Comment:
We should fallback to the default implementation instead of rewriting it?
`return super().is_authorized_hitl_task(...)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
