vincbeck commented on code in PR #42473:
URL: https://github.com/apache/airflow/pull/42473#discussion_r1775693901
##########
airflow/api_connexion/security.py:
##########
@@ -126,13 +126,14 @@ def callback():
if dag_id or access or access_entity:
return access
- # No DAG id is provided, the user is not authorized to access all
DAGs and authorization is done
+ # No DAG id is provided: the user is not authorized to access all
DAGs and authorization is done
# on DAG level
- # If method is "GET", return whether the user has read access to
any DAGs
- # If method is "PUT", return whether the user has edit access to
any DAGs
- return (method == "GET" and
any(get_auth_manager().get_permitted_dag_ids(methods=["GET"]))) or (
- method == "PUT" and
any(get_auth_manager().get_permitted_dag_ids(methods=["PUT"]))
- )
+ if method == "GET":
+ return
any(get_auth_manager().get_permitted_dag_ids(methods=["GET"]))
+ elif method == "PUT":
+ return
any(get_auth_manager().get_permitted_dag_ids(methods=["PUT"]))
+ else:
+ raise RuntimeError("Unexpected")
Review Comment:
```suggestion
raise False
```
##########
airflow/api_connexion/security.py:
##########
@@ -126,13 +126,14 @@ def callback():
if dag_id or access or access_entity:
return access
- # No DAG id is provided, the user is not authorized to access all
DAGs and authorization is done
+ # No DAG id is provided: the user is not authorized to access all
DAGs and authorization is done
Review Comment:
Yeah I dont understand my own comment myself xD We might want to rephrase
it. What I meant is, if no DAG id is provided and user does not have
permissions to read all DAGs, we check whether the user have permission to read
at least one DAG.
Example: list dags page.
The user does not have permissions to read all DAGs. We want to check if the
user has permissions to read at least one DAG. If not, they will have denied
access.
To be honest I hate that logic and I was planning to modify it at some
point. I think we should introduce a new action `LIST` which gives access to a
user to list given resources. That would simplify the logic, if the user has
`LIST` permissions on DAGs, then they are authorized to list DAGs. Of course
the list of DAGs will be filtered to only contain DAGs that the user has access
to (that is already done today). Today we check whether the user has
permission to access one dag to check whether the user has permissions to see
the list, and then when listing the DAGs, we check for each DAG, whether they
have access to it.
Also, this is a legacy from FAB because we assume here that DAGs are the
only type of resources that a user can have resource-specific permission (user
X can read on DAG Y). But this is no longer true with the introduction of auth
manager where any type of resource can be used to create resource-specific
permission. Example: user X can read on connection Z.
--
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]
