uranusjr commented on code in PR #34317:
URL: https://github.com/apache/airflow/pull/34317#discussion_r1347905048
##########
airflow/www/security_manager.py:
##########
@@ -268,125 +271,53 @@ def get_user_roles(user=None):
user = g.user
return user.roles
- def get_readable_dags(self, user) -> Iterable[DagModel]:
- """Gets the DAGs readable by authenticated user."""
- warnings.warn(
- "`get_readable_dags` has been deprecated. Please use
`get_readable_dag_ids` instead.",
- RemovedInAirflow3Warning,
- stacklevel=2,
- )
- with warnings.catch_warnings():
- warnings.simplefilter("ignore", RemovedInAirflow3Warning)
- return self.get_accessible_dags([permissions.ACTION_CAN_READ],
user)
-
- def get_editable_dags(self, user) -> Iterable[DagModel]:
- """Gets the DAGs editable by authenticated user."""
- warnings.warn(
- "`get_editable_dags` has been deprecated. Please use
`get_editable_dag_ids` instead.",
- RemovedInAirflow3Warning,
- stacklevel=2,
- )
- with warnings.catch_warnings():
- warnings.simplefilter("ignore", RemovedInAirflow3Warning)
- return self.get_accessible_dags([permissions.ACTION_CAN_EDIT],
user)
-
- @provide_session
- def get_accessible_dags(
- self,
- user_actions: Container[str] | None,
- user,
- session: Session = NEW_SESSION,
- ) -> Iterable[DagModel]:
- warnings.warn(
- "`get_accessible_dags` has been deprecated. Please use
`get_accessible_dag_ids` instead.",
- RemovedInAirflow3Warning,
- stacklevel=3,
- )
- dag_ids = self.get_accessible_dag_ids(user, user_actions, session)
- return
session.scalars(select(DagModel).where(DagModel.dag_id.in_(dag_ids)))
-
- def get_readable_dag_ids(self, user) -> set[str]:
+ def get_readable_dag_ids(self, user=None) -> set[str]:
"""Gets the DAG IDs readable by authenticated user."""
- return self.get_accessible_dag_ids(user, [permissions.ACTION_CAN_READ])
+ return self.get_permitted_dag_ids(methods=["GET"], user=user)
- def get_editable_dag_ids(self, user) -> set[str]:
+ def get_editable_dag_ids(self, user=None) -> set[str]:
"""Gets the DAG IDs editable by authenticated user."""
- return self.get_accessible_dag_ids(user, [permissions.ACTION_CAN_EDIT])
+ return self.get_permitted_dag_ids(methods=["PUT"], user=user)
@provide_session
- def get_accessible_dag_ids(
+ def get_permitted_dag_ids(
self,
- user,
- user_actions: Container[str] | None = None,
+ *,
+ methods: Container[ResourceMethod] | None = None,
+ user=None,
session: Session = NEW_SESSION,
) -> set[str]:
"""Generic function to get readable or writable DAGs for user."""
- if not user_actions:
- user_actions = [permissions.ACTION_CAN_EDIT,
permissions.ACTION_CAN_READ]
+ if not methods:
+ methods = ["PUT", "GET"]
- if not get_auth_manager().is_logged_in():
- roles = user.roles
- else:
- if (permissions.ACTION_CAN_EDIT in user_actions and
self.can_edit_all_dags(user)) or (
- permissions.ACTION_CAN_READ in user_actions and
self.can_read_all_dags(user)
- ):
- return {dag.dag_id for dag in
session.execute(select(DagModel.dag_id))}
- user_query = session.scalar(
- select(User)
- .options(
- joinedload(User.roles)
- .subqueryload(Role.permissions)
- .options(joinedload(Permission.action),
joinedload(Permission.resource))
+ dag_ids = {dag.dag_id for dag in
session.execute(select(DagModel.dag_id))}
+
+ if ("GET" in methods and
get_auth_manager().is_authorized_dag(method="GET", user=user)) or (
+ "PUT" in methods and
get_auth_manager().is_authorized_dag(method="PUT", user=user)
+ ):
+ return dag_ids
+
+ return {
+ dag_id
+ for dag_id in dag_ids
+ if (
+ "GET" in methods
+ and get_auth_manager().is_authorized_dag(
+ method="GET", details=DagDetails(id=dag_id), user=user
+ )
+ )
+ or (
+ "PUT" in methods
+ and get_auth_manager().is_authorized_dag(
+ method="PUT", details=DagDetails(id=dag_id), user=user
)
Review Comment:
Can we split this up? Maybe with a local function. This is very out of hand.
##########
airflow/www/views.py:
##########
@@ -3908,9 +3909,11 @@ class DagFilter(BaseFilter):
"""Filter using DagIDs."""
def apply(self, query, func):
- if get_airflow_app().appbuilder.sm.has_all_dags_access(g.user):
+ if get_auth_manager().is_authorized_dag(
+ method="GET", user=g.user
+ ) or get_auth_manager().is_authorized_dag(method="PUT", user=g.user):
return query
Review Comment:
```suggestion
if get_auth_manager().is_authorized_dag(method="GET", user=g.user):
return query
if get_auth_manager().is_authorized_dag(method="PUT", user=g.user):
return query
```
I’m not a fan very long if statements, especially with how Black tends to
wrap it very awkwardly.
##########
airflow/www/security_manager.py:
##########
@@ -738,24 +639,13 @@ def create_perm_vm_for_all_dag(self) -> None:
def check_authorization(
self,
perms: Sequence[tuple[str, str]] | None = None,
- dag_id: str | None = None,
) -> bool:
"""Checks that the logged in user has the specified permissions."""
if not perms:
return True
for perm in perms:
- if perm in (
- (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
- (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG),
- (permissions.ACTION_CAN_DELETE, permissions.RESOURCE_DAG),
- ):
- can_access_all_dags = self.has_access(*perm)
- if not can_access_all_dags:
- action = perm[0]
- if not self.can_access_some_dags(action, dag_id):
- return False
- elif not self.has_access(*perm):
+ if not self.has_access(*perm):
return False
return True
Review Comment:
```python
return all(self.has_access(*perm) for perm in perms)
```
##########
airflow/www/views.py:
##########
@@ -3908,9 +3909,11 @@ class DagFilter(BaseFilter):
"""Filter using DagIDs."""
def apply(self, query, func):
- if get_airflow_app().appbuilder.sm.has_all_dags_access(g.user):
+ if get_auth_manager().is_authorized_dag(
+ method="GET", user=g.user
+ ) or get_auth_manager().is_authorized_dag(method="PUT", user=g.user):
return query
Review Comment:
```suggestion
if get_auth_manager().is_authorized_dag(method="GET", user=g.user):
return query
if get_auth_manager().is_authorized_dag(method="PUT", user=g.user):
return query
```
I’m not a fan very long if statements, especially with how Black tends to
wrap it very awkwardly.
##########
airflow/www/extensions/init_jinja_globals.py:
##########
@@ -69,10 +70,17 @@ def prepare_jinja_globals():
"git_version": git_version,
"k8s_or_k8scelery_executor": IS_K8S_OR_K8SCELERY_EXECUTOR,
"rest_api_enabled": False,
- "auth_manager": get_auth_manager(),
"config_test_connection": conf.get("core", "test_connection",
fallback="Disabled"),
}
+ # Extra global specific to auth manager
+ extra_globals.update(
+ {
+ "auth_manager": get_auth_manager(),
+ "DagDetails": DagDetails,
+ }
+ )
Review Comment:
```suggestion
extra_globals["auth_manager"] = get_auth_manager()
extra_globals["DagDetails"] = DagDetails
```
Feels easier to read and actually shorter.
--
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]
