You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/08/27 05:47:10 UTC
[GitHub] [airflow] jhtimmins opened a new pull request #10594: Add permissions for stable API
jhtimmins opened a new pull request #10594:
URL: https://github.com/apache/airflow/pull/10594
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #10594:
URL: https://github.com/apache/airflow/pull/10594
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r487682606
##########
File path: docs/security/access-control.rst
##########
@@ -114,3 +114,61 @@ using the ``airflow roles create`` command, e.g.:
And we could assign the given role to a new user using the ``airflow
users add-role`` CLI command.
+
+Permissions
+'''''''''''
+
+Resource-Based permissions
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+Starting with version 2.0, permissions are based on individual resources and a small subset of actions on those resources. Resources match standard Airflow concepts, such as ``Dag``, ``DagRun``, ``Task``, and ``Connection``. Actions include ``can_create``, ``can_read``, ``can_edit``, and ``can_delete``. Permissions (each consistint of a resource + action pair) are then added to roles.
+
+Simple table:
+
+================================================================================== ====== ====================================================================================
+ Inputs
+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+View Method Permissions
+================================================================================== ====== ====================================================================================
+/config GET Config.can_read
+/connections GET Connection.can_read
+/connections POST Connection.can_create
+/connections/{connection_id} DELETE Connection.can_delete
+/connections/{connection_id} GET Connection.can_read
+/connections/{connection_id} PATCH Connection.can_edit
+/dagSources/{file_token} GET DagCode.can_read
+/dags GET Dag.can_read
+/dags/{dag_id} GET Dag.can_read
+/dags/{dag_id} PATCH Dag.can_edit
+/dags/{dag_id}/clearTaskInstances POST Dag.can_read, DagRun.can_read, TaskInstance.can_edit
+/dags/{dag_id}/details GET Dag.can_read
+/dags/{dag_id}/tasks GET Dag.can_read, Task.can_read
+/dags/{dag_id}/tasks/{task_id} GET Dag.can_read, Task.can_read
Review comment:
I agree, it's better to start without it and add it later if it turned out to be something that's actually needed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-695977861
@mik-laj @ashb Ok I've incorporated both of your requested changes. Mind letting me know if anything is missing? If it looks good, feel free to merge.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r486463936
##########
File path: docs/security/access-control.rst
##########
@@ -114,3 +114,61 @@ using the ``airflow roles create`` command, e.g.:
And we could assign the given role to a new user using the ``airflow
users add-role`` CLI command.
+
+Permissions
+'''''''''''
+
+Resource-Based permissions
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+Starting with version 2.0, permissions are based on individual resources and a small subset of actions on those resources. Resources match standard Airflow concepts, such as ``Dag``, ``DagRun``, ``Task``, and ``Connection``. Actions include ``can_create``, ``can_read``, ``can_edit``, and ``can_delete``. Permissions (each consistint of a resource + action pair) are then added to roles.
+
+Simple table:
+
+================================================================================== ====== ====================================================================================
+ Inputs
+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+View Method Permissions
+================================================================================== ====== ====================================================================================
+/config GET Config.can_read
+/connections GET Connection.can_read
+/connections POST Connection.can_create
+/connections/{connection_id} DELETE Connection.can_delete
+/connections/{connection_id} GET Connection.can_read
+/connections/{connection_id} PATCH Connection.can_edit
+/dagSources/{file_token} GET DagCode.can_read
+/dags GET Dag.can_read
+/dags/{dag_id} GET Dag.can_read
+/dags/{dag_id} PATCH Dag.can_edit
+/dags/{dag_id}/clearTaskInstances POST Dag.can_read, DagRun.can_read, TaskInstance.can_edit
+/dags/{dag_id}/details GET Dag.can_read
+/dags/{dag_id}/tasks GET Dag.can_read, Task.can_read
+/dags/{dag_id}/tasks/{task_id} GET Dag.can_read, Task.can_read
Review comment:
I don't think we need Task as a separate permission entity -- it's not separately useful enough I don't think
##########
File path: docs/security/access-control.rst
##########
@@ -114,3 +114,61 @@ using the ``airflow roles create`` command, e.g.:
And we could assign the given role to a new user using the ``airflow
users add-role`` CLI command.
+
+Permissions
+'''''''''''
+
+Resource-Based permissions
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+Starting with version 2.0, permissions are based on individual resources and a small subset of actions on those resources. Resources match standard Airflow concepts, such as ``Dag``, ``DagRun``, ``Task``, and ``Connection``. Actions include ``can_create``, ``can_read``, ``can_edit``, and ``can_delete``. Permissions (each consistint of a resource + action pair) are then added to roles.
+
+Simple table:
+
+================================================================================== ====== ====================================================================================
+ Inputs
+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+View Method Permissions
+================================================================================== ====== ====================================================================================
+/config GET Config.can_read
+/connections GET Connection.can_read
+/connections POST Connection.can_create
+/connections/{connection_id} DELETE Connection.can_delete
+/connections/{connection_id} GET Connection.can_read
+/connections/{connection_id} PATCH Connection.can_edit
+/dagSources/{file_token} GET DagCode.can_read
+/dags GET Dag.can_read
+/dags/{dag_id} GET Dag.can_read
+/dags/{dag_id} PATCH Dag.can_edit
+/dags/{dag_id}/clearTaskInstances POST Dag.can_read, DagRun.can_read, TaskInstance.can_edit
+/dags/{dag_id}/details GET Dag.can_read
+/dags/{dag_id}/tasks GET Dag.can_read, Task.can_read
+/dags/{dag_id}/tasks/{task_id} GET Dag.can_read, Task.can_read
Review comment:
```suggestion
/dags/{dag_id}/tasks GET Dag.can_read
/dags/{dag_id}/tasks/{task_id} GET Dag.can_read
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-695977861
@mik-laj @ashb Ok I've incorporated both of your requested changes. Mind letting me know if anything is missing? If it looks good, feel free to merge.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r492218445
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
This would filter before any date limits are applied, and thus would get an incorrect count.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-693365767
Have you looked at this issue in other views as well? For example: Endpoint - "List XCom entries" allows specifying `~` as the dag_id, dag_run_id, task_id to retrieve XCOM entries for all DAGs, DAG runs and task instances.
This address URL returns all entries for all DAG in the database.
https://localhostot/api/v1/dags/~/dagRuns/~/taskInstances/~/xcomEntries
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r483761513
##########
File path: airflow/api_connexion/security.py
##########
@@ -16,25 +16,63 @@
# under the License.
from functools import wraps
-from typing import Callable, TypeVar, cast
+from typing import Callable, Optional, Sequence, Tuple, TypeVar, cast
from flask import Response, current_app
-from airflow.api_connexion.exceptions import Unauthenticated
+from airflow.api_connexion.exceptions import PermissionDenied, Unauthenticated
T = TypeVar("T", bound=Callable) # pylint: disable=invalid-name
-def requires_authentication(function: T):
- """Decorator for functions that require authentication"""
+def check_authentication():
+ """Checks that the request has valid authorization information."""
+ response = current_app.api_auth.requires_authentication(Response)()
+ if response.status_code != 200:
+ # since this handler only checks authentication, not authorization,
+ # we should always return 401
+ raise Unauthenticated(headers=response.headers)
- @wraps(function)
- def decorated(*args, **kwargs):
- response = current_app.api_auth.requires_authentication(Response)()
- if response.status_code != 200:
- # since this handler only checks authentication, not authorization,
- # we should always return 401
- raise Unauthenticated(headers=response.headers)
- return function(*args, **kwargs)
- return cast(T, decorated)
+def check_authorization(
+ permissions: Optional[Sequence[Tuple[str, str]]] = None, dag_id: Optional[int] = None
+):
+ """Checks that the logged in user has the specified permissions."""
+
+ if not permissions:
+ return
+
+ appbuilder = current_app.appbuilder
+ for permission in permissions:
+ if permission in (('can_read', 'Dag'), ('can_edit', 'Dag')):
Review comment:
Should we check dag level permissions for DagRun/TaskInstance too?
Perhaps anywhere we have a `dag_id` named parameter?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r492438024
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
Good count. I think the original behavior included a bug. @mik-laj can you confirm that the count should be taken after filtering by date?
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
Good catch. I think the original behavior included a bug. @mik-laj can you confirm that the count should be taken after filtering by date?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r482787734
##########
File path: airflow/api_connexion/endpoints/version_endpoint.py
##########
@@ -32,6 +33,7 @@ class VersionInfo(NamedTuple):
git_version: Optional[str]
+@security.requires_access([("can_read", "Version")])
def get_version():
Review comment:
This will allow the client's behavior to be changed before authorization is performed. I am able to imagine that other authorization methods may be required depending on the version.
Kubernetes API don't have aaccesss control on /version endpoint
```
$ ENDPOINT=$(gcloud container clusters describe airflow-cluster --zone us-central1 --format='value(endpoint)')
$ curl --insecure https://$ENDPOINT/version
{
"major": "1",
"minor": "16+",
"gitVersion": "v1.16.13-gke.1",
"gitCommit": "688c6543aa4b285355723f100302d80431e411cc",
"gitTreeState": "clean",
"buildDate": "2020-07-21T02:37:26Z",
"goVersion": "go1.13.9b4",
"compiler": "gc",
"platform": "linux/amd64"
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-687496912
@ashb @mik-laj @houqp Ok I think I've addressed or included all of your edits.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-690017142
@mik-laj @ashb Ok I've added docs that show the permissions for each endpoint. https://github.com/apache/airflow/pull/10594/files#diff-1da5e874639ba7d6631e109fd81e9dadR130
This is also a good place to review the permissions I've added to the endpoints, in case y'all disagree with my choices. Thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r490144910
##########
File path: airflow/models/dag.py
##########
@@ -1664,6 +1665,38 @@ def deactivate_stale_dags(expiration_date, session=None):
session.merge(dag)
session.commit()
+ @classmethod
+ def get_readable_dags(cls, user):
+ """Gets the DAGs readable by authenticated user."""
+ return cls.get_accessible_dags(security.CAN_READ, user)
+
+ @classmethod
+ def get_editable_dags(cls, user):
+ """Gets the DAGs editable by authenticated user."""
+ return cls.get_accessible_dags(security.CAN_EDIT, user)
+
+ @staticmethod
+ @provide_session
+ def get_accessible_dags(user_action, user, session=None):
+ """Generic function to get readable or writable DAGs for authenticated user."""
+
+ if user.is_anonymous or 'Public' in user.roles:
+ # return an empty set if the role is public
+ return set()
+
+ resources = set()
+ for role in user.roles:
+ for permission in role.permissions:
Review comment:
Won't this also include things like "can_index on Airflow", resulting in `WHERE dag_id IN (..., "Airflow", ...)` etc?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r490330627
##########
File path: airflow/models/dag.py
##########
@@ -1664,6 +1665,38 @@ def deactivate_stale_dags(expiration_date, session=None):
session.merge(dag)
session.commit()
+ @classmethod
+ def get_readable_dags(cls, user):
+ """Gets the DAGs readable by authenticated user."""
+ return cls.get_accessible_dags(security.CAN_READ, user)
+
+ @classmethod
+ def get_editable_dags(cls, user):
+ """Gets the DAGs editable by authenticated user."""
+ return cls.get_accessible_dags(security.CAN_EDIT, user)
+
+ @staticmethod
+ @provide_session
+ def get_accessible_dags(user_action, user, session=None):
+ """Generic function to get readable or writable DAGs for authenticated user."""
+
+ if user.is_anonymous or 'Public' in user.roles:
+ # return an empty set if the role is public
+ return set()
+
+ resources = set()
+ for role in user.roles:
+ for permission in role.permissions:
+ resource = permission.view_menu.name
+ action = permission.permission.name
+ if action == user_action:
+ resources.add(resource)
+
+ if 'Dag' in resources:
Review comment:
Affirmative
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj edited a comment on pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-687500056
I think it is worth adding a description of the required permissions for each endpoint to the OpenAPI specification. What do you think about it? Currently, this information is only available in code. It is not always obvious what permissions you need to have to read extra links.
I will look at it on Monday again.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins edited a comment on pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins edited a comment on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-693154312
Good catch @mik-laj. I've updated the authorization code to properly handle cases where a user has access to some dags but not all. The endpoint is also updated to only return the proper results. https://github.com/apache/airflow/pull/10594/files#diff-9982141783adf996be12cffcf84d6fe5R59
@ashb you may be curious as well
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r490335548
##########
File path: airflow/models/dag.py
##########
@@ -1664,6 +1665,38 @@ def deactivate_stale_dags(expiration_date, session=None):
session.merge(dag)
session.commit()
+ @classmethod
+ def get_readable_dags(cls, user):
+ """Gets the DAGs readable by authenticated user."""
+ return cls.get_accessible_dags(security.CAN_READ, user)
+
+ @classmethod
+ def get_editable_dags(cls, user):
+ """Gets the DAGs editable by authenticated user."""
+ return cls.get_accessible_dags(security.CAN_EDIT, user)
+
+ @staticmethod
+ @provide_session
+ def get_accessible_dags(user_action, user, session=None):
+ """Generic function to get readable or writable DAGs for authenticated user."""
+
+ if user.is_anonymous or 'Public' in user.roles:
+ # return an empty set if the role is public
+ return set()
+
+ resources = set()
+ for role in user.roles:
+ for permission in role.permissions:
Review comment:
This will look at that permission, but the check on 1692 will only filter on permissions with an action (`can_read`), matching the specified action from the function args.
However, it will grab all resources with matching actions. So it would grab `Airflow.can_read`, if `action == 'can_read'`. I figure that was fine though, since it isn't an issue unless users are naming dags `Airflow`, `TaskInstance`, etc.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r492438024
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
Good count. I think the original behavior included a bug. @mik-laj can you confirm that the count should be taken after filtering by date?
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
Good catch. I think the original behavior included a bug. @mik-laj can you confirm that the count should be taken after filtering by date?
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -158,26 +158,29 @@ def _apply_date_filters_to_query(
return query
-@security.requires_authentication
+@security.requires_access([("can_read", "Dag"), ("can_read", "DagRun")])
@provide_session
def get_dag_runs_batch(session):
"""
Get list of DAG Runs
"""
body = request.get_json()
- try:
+ try: # TODO: Handle filtering.
data = dagruns_batch_form_schema.load(body)
except ValidationError as err:
raise BadRequest(detail=str(err.messages))
+ appbuilder = current_app.appbuilder
+ readable_dag_ids = appbuilder.sm.get_readable_dag_ids(g.user)
query = session.query(DagRun)
-
- if data["dag_ids"]:
- query = query.filter(DagRun.dag_id.in_(data["dag_ids"]))
+ if data.get("dag_ids"):
+ dag_ids = set(data["dag_ids"]) & set(readable_dag_ids)
+ query = query.filter(DagRun.dag_id.in_(dag_ids))
+ else:
+ query = query.filter(DagRun.dag_id.in_(readable_dag_ids))
dag_runs, total_entries = _fetch_dag_runs(
query,
- session,
Review comment:
It has to do with how the count is getting made, as seen in your previous comment. It used to use `session`, but it doesn't need to now.
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
Actually, I think we should maintain the existing behavior here and do the count prior to the date filtering. Just to keep scope limited. If we decide that the count should take place after the date filtering, let's make that behavioral change in a separate PR, just to reduce scope for this one somewhat.
##########
File path: airflow/api_connexion/endpoints/xcom_endpoint.py
##########
@@ -49,22 +52,28 @@ def get_xcom_entries(
"""
query = session.query(XCom)
- if dag_id != '~':
+ if dag_id == '~':
+ appbuilder = current_app.appbuilder
+ readable_dag_ids = appbuilder.sm.get_readable_dag_ids(g.user)
+ query = query.filter(XCom.dag_id.in_(readable_dag_ids))
+ query.join(DR, and_(XCom.dag_id.in_(readable_dag_ids), XCom.execution_date == DR.execution_date))
+ else:
query = query.filter(XCom.dag_id == dag_id)
Review comment:
Correct
##########
File path: airflow/api_connexion/endpoints/xcom_endpoint.py
##########
@@ -49,22 +52,28 @@ def get_xcom_entries(
"""
query = session.query(XCom)
- if dag_id != '~':
+ if dag_id == '~':
+ appbuilder = current_app.appbuilder
+ readable_dag_ids = appbuilder.sm.get_readable_dag_ids(g.user)
+ query = query.filter(XCom.dag_id.in_(readable_dag_ids))
+ query.join(DR, and_(XCom.dag_id.in_(readable_dag_ids), XCom.execution_date == DR.execution_date))
Review comment:
I've made the update, and created an issue to add a test for this
https://github.com/apache/airflow/issues/11073
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
I've created an issue to change how the total_entries are counted. #11074
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-692141260
If there are 3 DAGs in the database: ``DAG_A``, ``DAG_B``, and you have permissions - ``[can_read, DAG_A]``, then you will send a request - `GET /api/v1/dags`, what DAGs will you get? I guess you will either get information on all DAGs, or you will not get information on any.
Your database query should be different depending on the accessible DAGs for the user.
https://github.com/apache/airflow/blob/eaa49b2257913c34b15408a14e445f6106e691ee/airflow/www/views.py#L287
https://github.com/apache/airflow/blob/eaa49b2257913c34b15408a14e445f6106e691ee/airflow/www/views.py#L305-L306
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r490143642
##########
File path: airflow/models/dag.py
##########
@@ -1664,6 +1665,38 @@ def deactivate_stale_dags(expiration_date, session=None):
session.merge(dag)
session.commit()
+ @classmethod
+ def get_readable_dags(cls, user):
+ """Gets the DAGs readable by authenticated user."""
+ return cls.get_accessible_dags(security.CAN_READ, user)
+
+ @classmethod
+ def get_editable_dags(cls, user):
+ """Gets the DAGs editable by authenticated user."""
+ return cls.get_accessible_dags(security.CAN_EDIT, user)
+
+ @staticmethod
+ @provide_session
+ def get_accessible_dags(user_action, user, session=None):
+ """Generic function to get readable or writable DAGs for authenticated user."""
+
+ if user.is_anonymous or 'Public' in user.roles:
+ # return an empty set if the role is public
+ return set()
+
+ resources = set()
+ for role in user.roles:
+ for permission in role.permissions:
+ resource = permission.view_menu.name
+ action = permission.permission.name
+ if action == user_action:
+ resources.add(resource)
+
+ if 'Dag' in resources:
+ return session.query(DagModel)
+
+ return session.query(DagModel).filter(DagModel.dag_id.in_(resources))
+
Review comment:
This feels out of place here in the DAG class -- it leaks FAB into Airflow core, which so far has almost entirely managed to mot be tied together.
I think this would be better if we kept that separation, so is it possible to keep this in the SecurityManager class?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r481849731
##########
File path: airflow/api_connexion/security.py
##########
@@ -37,3 +37,32 @@ def decorated(*args, **kwargs):
return function(*args, **kwargs)
return cast(T, decorated)
+
+
+def requires_access(permissions: Sequence[Tuple[str, str]]) -> Callable[[T], T]:
Review comment:
For endpoints that need to be guarded by authentication but not authorization, one solution we can adopt is to pass in an empty permissions list, or `None`, then in the decorator skip the role check entirely if that's the case.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r481490675
##########
File path: airflow/api_connexion/security.py
##########
@@ -37,3 +37,32 @@ def decorated(*args, **kwargs):
return function(*args, **kwargs)
return cast(T, decorated)
+
+
+def requires_access(permissions: Sequence[Tuple[str, str]]) -> Callable[[T], T]:
Review comment:
@houqp we could do that, but there are times when authentication is needed, but we don't tie it to a specific permission. Such as the Health and Configs endpoints. We could add a semi-arbitrary permission for those cases (`('Health', 'can_read')`), or similar with reading configs. Or we could allow `requires_access()` to work without any permissions, in which case it only requires authentication.
What do you think? @mik-laj curious about your thoughts as well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-696472970
@ashb I don't have any major comments. It looks good now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-692122198
@mik-laj Ok this is ready to merge, if you wouldn't mind approving/merging
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r492218445
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
This would filter before any date limits are applied, and thus would get an incorrect count.
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -158,26 +158,29 @@ def _apply_date_filters_to_query(
return query
-@security.requires_authentication
+@security.requires_access([("can_read", "Dag"), ("can_read", "DagRun")])
@provide_session
def get_dag_runs_batch(session):
"""
Get list of DAG Runs
"""
body = request.get_json()
- try:
+ try: # TODO: Handle filtering.
data = dagruns_batch_form_schema.load(body)
except ValidationError as err:
raise BadRequest(detail=str(err.messages))
+ appbuilder = current_app.appbuilder
+ readable_dag_ids = appbuilder.sm.get_readable_dag_ids(g.user)
query = session.query(DagRun)
-
- if data["dag_ids"]:
- query = query.filter(DagRun.dag_id.in_(data["dag_ids"]))
+ if data.get("dag_ids"):
+ dag_ids = set(data["dag_ids"]) & set(readable_dag_ids)
+ query = query.filter(DagRun.dag_id.in_(dag_ids))
+ else:
+ query = query.filter(DagRun.dag_id.in_(readable_dag_ids))
dag_runs, total_entries = _fetch_dag_runs(
query,
- session,
Review comment:
What was the reason for removing the session here?
##########
File path: airflow/api_connexion/endpoints/xcom_endpoint.py
##########
@@ -49,22 +52,28 @@ def get_xcom_entries(
"""
query = session.query(XCom)
- if dag_id != '~':
+ if dag_id == '~':
+ appbuilder = current_app.appbuilder
+ readable_dag_ids = appbuilder.sm.get_readable_dag_ids(g.user)
+ query = query.filter(XCom.dag_id.in_(readable_dag_ids))
+ query.join(DR, and_(XCom.dag_id.in_(readable_dag_ids), XCom.execution_date == DR.execution_date))
Review comment:
```suggestion
query.join(DR, and_(XCom.dag_id == DR.dag_id, XCom.execution_date == DR.execution_date))
```
This is not your PR causing this -- but I also think that this is missing a `query = ` at the start -- I don't _think_ `.join()` mutates the query, but returns a new one. I could be wrong on this. (Separate PR to fix it if this _doesn't_ work as is)
##########
File path: airflow/api_connexion/endpoints/xcom_endpoint.py
##########
@@ -49,22 +52,28 @@ def get_xcom_entries(
"""
query = session.query(XCom)
- if dag_id != '~':
+ if dag_id == '~':
+ appbuilder = current_app.appbuilder
+ readable_dag_ids = appbuilder.sm.get_readable_dag_ids(g.user)
+ query = query.filter(XCom.dag_id.in_(readable_dag_ids))
+ query.join(DR, and_(XCom.dag_id.in_(readable_dag_ids), XCom.execution_date == DR.execution_date))
+ else:
query = query.filter(XCom.dag_id == dag_id)
Review comment:
When there's a single dag_id the permissions decorator will validate access to that dag, correct?
##########
File path: airflow/api_connexion/endpoints/xcom_endpoint.py
##########
@@ -49,22 +52,28 @@ def get_xcom_entries(
"""
query = session.query(XCom)
- if dag_id != '~':
+ if dag_id == '~':
+ appbuilder = current_app.appbuilder
+ readable_dag_ids = appbuilder.sm.get_readable_dag_ids(g.user)
+ query = query.filter(XCom.dag_id.in_(readable_dag_ids))
+ query.join(DR, and_(XCom.dag_id.in_(readable_dag_ids), XCom.execution_date == DR.execution_date))
Review comment:
```suggestion
query.join(DR, and_(XCom.dag_id == DR.dag_id, XCom.execution_date == DR.execution_date))
```
This is not your PR causing this -- but I also think that this is missing a `query = ` at the start -- I don't _think_ `.join()` mutates the query, but returns a new one. I could be wrong on this. (Separate PR to fix it if this _doesn't_ work as is)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r483298085
##########
File path: airflow/api_connexion/security.py
##########
@@ -16,25 +16,61 @@
# under the License.
from functools import wraps
-from typing import Callable, TypeVar, cast
+from typing import Callable, Optional, Sequence, Tuple, TypeVar, cast
from flask import Response, current_app
-from airflow.api_connexion.exceptions import Unauthenticated
+from airflow.api_connexion.exceptions import PermissionDenied, Unauthenticated
T = TypeVar("T", bound=Callable) # pylint: disable=invalid-name
-def requires_authentication(function: T):
- """Decorator for functions that require authentication"""
+def check_authentication():
+ """Checks that the request has valid authorization information."""
+ response = current_app.api_auth.requires_authentication(Response)()
+ if response.status_code != 200:
+ # since this handler only checks authentication, not authorization,
+ # we should always return 401
+ raise Unauthenticated(headers=response.headers)
- @wraps(function)
- def decorated(*args, **kwargs):
- response = current_app.api_auth.requires_authentication(Response)()
- if response.status_code != 200:
- # since this handler only checks authentication, not authorization,
- # we should always return 401
- raise Unauthenticated(headers=response.headers)
- return function(*args, **kwargs)
- return cast(T, decorated)
+def check_authorization(permissions=None, dag_id=None):
Review comment:
nitpick, could you add type annotations to this function?
##########
File path: airflow/api_connexion/security.py
##########
@@ -16,25 +16,61 @@
# under the License.
from functools import wraps
-from typing import Callable, TypeVar, cast
+from typing import Callable, Optional, Sequence, Tuple, TypeVar, cast
from flask import Response, current_app
-from airflow.api_connexion.exceptions import Unauthenticated
+from airflow.api_connexion.exceptions import PermissionDenied, Unauthenticated
T = TypeVar("T", bound=Callable) # pylint: disable=invalid-name
-def requires_authentication(function: T):
- """Decorator for functions that require authentication"""
+def check_authentication():
+ """Checks that the request has valid authorization information."""
+ response = current_app.api_auth.requires_authentication(Response)()
+ if response.status_code != 200:
+ # since this handler only checks authentication, not authorization,
+ # we should always return 401
+ raise Unauthenticated(headers=response.headers)
- @wraps(function)
- def decorated(*args, **kwargs):
- response = current_app.api_auth.requires_authentication(Response)()
- if response.status_code != 200:
- # since this handler only checks authentication, not authorization,
- # we should always return 401
- raise Unauthenticated(headers=response.headers)
- return function(*args, **kwargs)
- return cast(T, decorated)
+def check_authorization(permissions=None, dag_id=None):
+ """Checks that the logged in user has the specified permissions."""
+
+ if not permissions:
+ permissions = []
Review comment:
shouldn't we doing an early return here instead of creating an empty permissions list?
##########
File path: airflow/www/security.py
##########
@@ -521,6 +524,17 @@ def sync_roles(self):
self.update_admin_perm_view()
self.clean_perms()
+ def sync_resource_permissions(self, permissions=None):
+ """
+ Populates resource-based permissions.
+ """
+ if not permissions:
+ permissions = []
Review comment:
same as my previous comment, seems like we can just do early return here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r480818779
##########
File path: airflow/www/security.py
##########
@@ -521,6 +524,18 @@ def sync_roles(self):
self.update_admin_perm_view()
self.clean_perms()
+ def sync_resource_permissions(self):
+ """
+ Populates resource-based permissions.
+ """
+ resources = ['Pool', 'Connection', 'Dag', 'DagBag', 'DagRun',
+ 'DagCode', 'Log', 'Task', 'ImportError', 'Variable', 'XCom']
+ actions = ['can_create', 'can_read', 'can_edit', 'can_delete']
Review comment:
Good point @houqp. I've updated it so that the resource is created dynamically when the decorator is called on the function.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r480108441
##########
File path: tests/test_utils/api_connexion_utils.py
##########
@@ -30,6 +30,19 @@ def create_user(app, username, role):
)
+def create_role(app, name, permissions=None):
Review comment:
What do you think about extending the create_user method to accept a list of permissions?
```python
def create_user(app, username, role=None, permissions=None):
appbuilder = app.appbuilder
current_role = None
if role and permission:
raise IllegalArgument("role and permission are muttually exclussive")
elif role:
current_role = appbuilder.sm.find_role(role)
elif permission:
ranadom_suffix = get_random_string()
current_role = create_role(app, get_random_string("test-role-{ranadom_suffix}"))
tester = appbuilder.sm.find_user(username=username)
if not tester:
appbuilder.sm.add_user(
username=username,
first_name=username,
last_name=username,
email=f"{username}@fab.org",
role=current_role,
password=username,
)
```
This will simplify testing as we won't have to worry so much about side effects as each role will only be used once. This will also explicitly associate the user with a specific permission set.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins edited a comment on pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins edited a comment on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-693154312
Good catch @mik-laj. I've updated the authorization code to properly handle cases where a user has access to some dags but not all. The endpoint is also updated to only return the proper results. Tests are included as well. https://github.com/apache/airflow/pull/10594/files#diff-9982141783adf996be12cffcf84d6fe5R59
@ashb you may be curious as well
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj edited a comment on pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-692141260
If there are 2 DAGs in the database: ``DAG_A``, ``DAG_B``, and you have permission - ``[can_read, DAG_A]``, then you will send a request - `GET /api/v1/dags`, what DAGs will you get? I guess you will either get information on all DAGs, or you will not get information on any. While you should only get information about ``DAG_A``. ``DAG_B`` should be hidden Your database query should be different depending on the accessible DAGs for the user.
https://github.com/apache/airflow/blob/eaa49b2257913c34b15408a14e445f6106e691ee/airflow/www/views.py#L287
https://github.com/apache/airflow/blob/eaa49b2257913c34b15408a14e445f6106e691ee/airflow/www/views.py#L305-L306
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r482790126
##########
File path: airflow/api_connexion/security.py
##########
@@ -37,3 +37,32 @@ def decorated(*args, **kwargs):
return function(*args, **kwargs)
return cast(T, decorated)
+
+
+def requires_access(permissions: Sequence[Tuple[str, str]]) -> Callable[[T], T]:
Review comment:
Config endpoint should have authentication and authorization.
Health and version endpoint don't need any protection. There are no sensitive data there and for operational reasons it is better not to add them. This will greatly facilitate monitoring, autodiscovery and API client development.
##########
File path: airflow/api_connexion/security.py
##########
@@ -37,3 +37,32 @@ def decorated(*args, **kwargs):
return function(*args, **kwargs)
return cast(T, decorated)
+
+
+def requires_access(permissions: Sequence[Tuple[str, str]]) -> Callable[[T], T]:
Review comment:
Config endpoint should have authentication and authorization.
Health and version endpoint don't need any protection. There are no sensitive data there and for operational reasons it is better not to add them. This will greatly facilitate monitoring, service autodiscovery and API client development.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r482814012
##########
File path: airflow/api_connexion/security.py
##########
@@ -37,3 +37,32 @@ def decorated(*args, **kwargs):
return function(*args, **kwargs)
return cast(T, decorated)
+
+
+def requires_access(permissions: Sequence[Tuple[str, str]]) -> Callable[[T], T]:
Review comment:
Thanks. Good to know. I've removed the Health and Version authorization.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r483712499
##########
File path: airflow/www/security.py
##########
@@ -151,10 +152,12 @@ class AirflowSecurityManager(SecurityManager, LoggingMixin):
WRITE_DAG_PERMS = {
'can_dag_edit',
+ 'can_edit',
}
READ_DAG_PERMS = {
'can_dag_read',
+ 'can_read',
Review comment:
Where is `can_dag_read` still used?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r492439912
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
Can you create a issues about it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-692132874
@jhtimmins I am starting to look at it and there seems to be one problem so please hold off the merge.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r486466062
##########
File path: docs/security/access-control.rst
##########
@@ -114,3 +114,61 @@ using the ``airflow roles create`` command, e.g.:
And we could assign the given role to a new user using the ``airflow
users add-role`` CLI command.
+
+Permissions
+'''''''''''
+
+Resource-Based permissions
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+Starting with version 2.0, permissions are based on individual resources and a small subset of actions on those resources. Resources match standard Airflow concepts, such as ``Dag``, ``DagRun``, ``Task``, and ``Connection``. Actions include ``can_create``, ``can_read``, ``can_edit``, and ``can_delete``. Permissions (each consistint of a resource + action pair) are then added to roles.
+
+Simple table:
+
Review comment:
```suggestion
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r482785240
##########
File path: airflow/api_connexion/endpoints/health_endpoint.py
##########
@@ -14,13 +14,15 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+from airflow.api_connexion import security
from airflow.api_connexion.schemas.health_schema import health_schema
from airflow.jobs.scheduler_job import SchedulerJob
HEALTHY = "healthy"
UNHEALTHY = "unhealthy"
+@security.requires_access([("can_read", "Health")])
Review comment:
This endpoint does not need authorization as it will be used to verify that the instance is working properly even if no account has been created yet.
The Kubernetes API also don't have authorization for health endpoint.
```
$ ENDPOINT=$(gcloud container clusters describe airflow-cluster --zone us-central1 --format='value(endpoint)')
$ curl --insecure https://$ENDPOINT/readyz?verbose
[+]ping ok
[+]log ok
[+]etcd ok
[+]poststarthook/generic-apiserver-start-informers ok
[+]poststarthook/start-apiextensions-informers ok
[+]poststarthook/start-apiextensions-controllers ok
[+]poststarthook/crd-informer-synced ok
[+]poststarthook/bootstrap-controller ok
[+]poststarthook/rbac/bootstrap-roles ok
[+]poststarthook/scheduling/bootstrap-system-priority-classes ok
[+]SSH Tunnel Check ok
[+]poststarthook/ca-registration ok
[+]poststarthook/start-kube-apiserver-admission-initializer ok
[+]poststarthook/start-kube-aggregator-informers ok
[+]poststarthook/apiservice-registration-controller ok
[+]poststarthook/apiservice-status-available-controller ok
[+]poststarthook/kube-apiserver-autoregistration ok
[+]autoregister-completion ok
[+]poststarthook/apiservice-openapi-controller ok
[+]shutdown ok
healthz check passed
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r490145964
##########
File path: airflow/models/dag.py
##########
@@ -1664,6 +1665,38 @@ def deactivate_stale_dags(expiration_date, session=None):
session.merge(dag)
session.commit()
+ @classmethod
+ def get_readable_dags(cls, user):
+ """Gets the DAGs readable by authenticated user."""
+ return cls.get_accessible_dags(security.CAN_READ, user)
+
+ @classmethod
+ def get_editable_dags(cls, user):
+ """Gets the DAGs editable by authenticated user."""
+ return cls.get_accessible_dags(security.CAN_EDIT, user)
+
+ @staticmethod
+ @provide_session
+ def get_accessible_dags(user_action, user, session=None):
+ """Generic function to get readable or writable DAGs for authenticated user."""
+
+ if user.is_anonymous or 'Public' in user.roles:
+ # return an empty set if the role is public
+ return set()
+
+ resources = set()
+ for role in user.roles:
+ for permission in role.permissions:
+ resource = permission.view_menu.name
+ action = permission.permission.name
+ if action == user_action:
+ resources.add(resource)
+
+ if 'Dag' in resources:
Review comment:
This is the new "all_dags" permission right?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-693154312
Good catch @mik-laj. I've updated the authorization code to properly handle cases where a user has access to some dags but not all. The endpoint is also updated to only return the proper results. https://github.com/apache/airflow/pull/10594/files#diff-9982141783adf996be12cffcf84d6fe5R59
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-696472970
@ashb I don't have any major comments. It looks good now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r483762556
##########
File path: airflow/api_connexion/security.py
##########
@@ -16,25 +16,63 @@
# under the License.
from functools import wraps
-from typing import Callable, TypeVar, cast
+from typing import Callable, Optional, Sequence, Tuple, TypeVar, cast
from flask import Response, current_app
-from airflow.api_connexion.exceptions import Unauthenticated
+from airflow.api_connexion.exceptions import PermissionDenied, Unauthenticated
T = TypeVar("T", bound=Callable) # pylint: disable=invalid-name
-def requires_authentication(function: T):
- """Decorator for functions that require authentication"""
+def check_authentication():
+ """Checks that the request has valid authorization information."""
+ response = current_app.api_auth.requires_authentication(Response)()
+ if response.status_code != 200:
+ # since this handler only checks authentication, not authorization,
+ # we should always return 401
+ raise Unauthenticated(headers=response.headers)
- @wraps(function)
- def decorated(*args, **kwargs):
- response = current_app.api_auth.requires_authentication(Response)()
- if response.status_code != 200:
- # since this handler only checks authentication, not authorization,
- # we should always return 401
- raise Unauthenticated(headers=response.headers)
- return function(*args, **kwargs)
- return cast(T, decorated)
+def check_authorization(
+ permissions: Optional[Sequence[Tuple[str, str]]] = None, dag_id: Optional[int] = None
+):
+ """Checks that the logged in user has the specified permissions."""
+
+ if not permissions:
+ return
+
+ appbuilder = current_app.appbuilder
+ for permission in permissions:
+ if permission in (('can_read', 'Dag'), ('can_edit', 'Dag')):
Review comment:
Ah, no, this is fine, we just need to add `('can_read', 'Dag')` to the listed permissions in dagrun end points.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r492218992
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -158,26 +158,29 @@ def _apply_date_filters_to_query(
return query
-@security.requires_authentication
+@security.requires_access([("can_read", "Dag"), ("can_read", "DagRun")])
@provide_session
def get_dag_runs_batch(session):
"""
Get list of DAG Runs
"""
body = request.get_json()
- try:
+ try: # TODO: Handle filtering.
data = dagruns_batch_form_schema.load(body)
except ValidationError as err:
raise BadRequest(detail=str(err.messages))
+ appbuilder = current_app.appbuilder
+ readable_dag_ids = appbuilder.sm.get_readable_dag_ids(g.user)
query = session.query(DagRun)
-
- if data["dag_ids"]:
- query = query.filter(DagRun.dag_id.in_(data["dag_ids"]))
+ if data.get("dag_ids"):
+ dag_ids = set(data["dag_ids"]) & set(readable_dag_ids)
+ query = query.filter(DagRun.dag_id.in_(dag_ids))
+ else:
+ query = query.filter(DagRun.dag_id.in_(readable_dag_ids))
dag_runs, total_entries = _fetch_dag_runs(
query,
- session,
Review comment:
What was the reason for removing the session here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r486465179
##########
File path: docs/security/access-control.rst
##########
@@ -114,3 +114,61 @@ using the ``airflow roles create`` command, e.g.:
And we could assign the given role to a new user using the ``airflow
users add-role`` CLI command.
+
+Permissions
+'''''''''''
+
+Resource-Based permissions
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+Starting with version 2.0, permissions are based on individual resources and a small subset of actions on those resources. Resources match standard Airflow concepts, such as ``Dag``, ``DagRun``, ``Task``, and ``Connection``. Actions include ``can_create``, ``can_read``, ``can_edit``, and ``can_delete``. Permissions (each consistint of a resource + action pair) are then added to roles.
+
+Simple table:
+
+================================================================================== ====== ====================================================================================
+ Inputs
+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+View Method Permissions
+================================================================================== ====== ====================================================================================
+/config GET Config.can_read
+/connections GET Connection.can_read
+/connections POST Connection.can_create
+/connections/{connection_id} DELETE Connection.can_delete
+/connections/{connection_id} GET Connection.can_read
+/connections/{connection_id} PATCH Connection.can_edit
+/dagSources/{file_token} GET DagCode.can_read
+/dags GET Dag.can_read
+/dags/{dag_id} GET Dag.can_read
+/dags/{dag_id} PATCH Dag.can_edit
+/dags/{dag_id}/clearTaskInstances POST Dag.can_read, DagRun.can_read, TaskInstance.can_edit
+/dags/{dag_id}/details GET Dag.can_read
+/dags/{dag_id}/tasks GET Dag.can_read, Task.can_read
+/dags/{dag_id}/tasks/{task_id} GET Dag.can_read, Task.can_read
+/dags/{dag_id}/dagRuns GET Dag.can_read, DagRun.can_read
+/dags/{dag_id}/dagRuns POST Dag.can_read, DagRun.can_create
+/dags/{dag_id}/dagRuns/{dag_run_id} DELETE Dag.can_read, DagRun.can_delete
+/dags/{dag_id}/dagRuns/{dag_run_id} GET Dag.can_read, DagRun.can_read
+/dags/{dag_id}/dagRuns/list POST Dag.can_read, DagRun.can_read
+/eventLogs GET Log.can_read
+/eventLogs/{event_log_id} GET Log.can_read
+/importErrors GET ImportError.can_read
+/importErrors/{import_error_id} GET ImportError.can_read
+/health GET None
+/version GET None
+/pools GET Pool.can_read
+/pools POST Pool.can_create
+/pools/{pool_name} DELETE Pool.can_delete
+/pools/{pool_name} GET Pool.can_read
+/pools/{pool_name} PATCH Pool.can_edit
+/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances GET Dag.can_read, DagRun.can_read, TaskInstance.can_read
+/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id} GET Dag.can_read, DagRun.can_read, TaskInstance.can_read
+/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/links GET DagBag.can_read, Dag.can_read, DagRun.can_read, Task.can_read, TaskInstance.can_read
+/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/logs/{task_try_number} GET Dag.can_read, DagRun.can_read, Task.can_read
+/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/list POST DagBag.can_read, Dag.can_read, DagRun.can_read, Task.can_read, TaskInstance.can_read
Review comment:
```suggestion
/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/list POST Dag.can_read, DagRun.can_read, Task.can_read, TaskInstance.can_read
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-696472970
@ashb I don't have any major comments. It looks good now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r492439376
##########
File path: docs/security/access-control.rst
##########
@@ -114,3 +118,69 @@ using the ``airflow roles create`` command, e.g.:
And we could assign the given role to a new user using the ``airflow
users add-role`` CLI command.
+
+
+Permissions
+'''''''''''
+
+Resource-Based permissions
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Starting with version 2.0, permissions are based on individual resources and a small subset of actions on those
+resources. Resources match standard Airflow concepts, such as ``Dag``, ``DagRun``, ``Task``, and
+``Connection``. Actions include ``can_create``, ``can_read``, ``can_edit``, and ``can_delete``.
+
+Permissions (each consistent of a resource + action pair) are then added to roles.
+
+Starting with version 2.0, permissions are based on individual resources and a small subset of actions on those resources. Resources match standard Airflow concepts, such as ``Dag``, ``DagRun``, ``Task``, and ``Connection``. Actions include ``can_create``, ``can_read``, ``can_edit``, and ``can_delete``. Permissions (each consisting of a resource + action pair) are then added to roles.
Review comment:
The beginning of this paragraph is repeated.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r490144910
##########
File path: airflow/models/dag.py
##########
@@ -1664,6 +1665,38 @@ def deactivate_stale_dags(expiration_date, session=None):
session.merge(dag)
session.commit()
+ @classmethod
+ def get_readable_dags(cls, user):
+ """Gets the DAGs readable by authenticated user."""
+ return cls.get_accessible_dags(security.CAN_READ, user)
+
+ @classmethod
+ def get_editable_dags(cls, user):
+ """Gets the DAGs editable by authenticated user."""
+ return cls.get_accessible_dags(security.CAN_EDIT, user)
+
+ @staticmethod
+ @provide_session
+ def get_accessible_dags(user_action, user, session=None):
+ """Generic function to get readable or writable DAGs for authenticated user."""
+
+ if user.is_anonymous or 'Public' in user.roles:
+ # return an empty set if the role is public
+ return set()
+
+ resources = set()
+ for role in user.roles:
+ for permission in role.permissions:
Review comment:
Won't this also include things like "can_index on Airflow"?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r486500224
##########
File path: docs/security/access-control.rst
##########
@@ -96,21 +96,69 @@ DAG Level Role
is treated as a ``View`` which has two permissions associated with it (``can_dag_read`` and ``can_dag_edit``). There is a special view called ``all_dags`` which
allows the role to access all the dags. The default ``Admin``, ``Viewer``, ``User``, ``Op`` roles can all access ``all_dags`` view.
-Add a new role
-''''''''''''''
-To configure a new role, go to **Security** tab and click **List Roles** in the new UI.
+Permissions
+'''''''''''
+
+Resource-Based permissions
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Starting with version 2.0, permissions are based on individual resources and a small subset of actions on those
+resources. Resources match standard Airflow concepts, such as ``Dag``, ``DagRun``, ``Task``, and
+``Connection``. Actions include ``can_create``, ``can_read``, ``can_edit``, and ``can_delete``.
+
+Permissions (each consistent of a resource + action pair) are then added to roles.
+
+Starting with version 2.0, permissions are based on individual resources and a small subset of actions on those resources. Resources match standard Airflow concepts, such as ``Dag``, ``DagRun``, ``Task``, and ``Connection``. Actions include ``can_create``, ``can_read``, ``can_edit``, and ``can_delete``. Permissions (each consistint of a resource + action pair) are then added to roles.
+
+Simple table:
+
+================================================================================== ====== ====================================================================================
+ Inputs
+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+View Method Permissions
+================================================================================== ====== ====================================================================================
+/config GET Config.can_read
+/connections GET Connection.can_read
+/connections POST Connection.can_create
+/connections/{connection_id} DELETE Connection.can_delete
+/connections/{connection_id} GET Connection.can_read
+/connections/{connection_id} PATCH Connection.can_edit
+/dagSources/{file_token} GET DagCode.can_read
+/dags GET Dag.can_read
+/dags/{dag_id} GET Dag.can_read
+/dags/{dag_id} PATCH Dag.can_edit
+/dags/{dag_id}/clearTaskInstances POST Dag.can_read, DagRun.can_read, TaskInstance.can_edit
+/dags/{dag_id}/details GET Dag.can_read
+/dags/{dag_id}/tasks GET Dag.can_read, Task.can_read
+/dags/{dag_id}/tasks/{task_id} GET Dag.can_read, Task.can_read
+/dags/{dag_id}/dagRuns GET Dag.can_read, DagRun.can_read
+/dags/{dag_id}/dagRuns POST Dag.can_read, DagRun.can_create
+/dags/{dag_id}/dagRuns/{dag_run_id} DELETE Dag.can_read, DagRun.can_delete
+/dags/{dag_id}/dagRuns/{dag_run_id} GET Dag.can_read, DagRun.can_read
+/dags/{dag_id}/dagRuns/list POST Dag.can_read, DagRun.can_read
+/eventLogs GET Log.can_read
+/eventLogs/{event_log_id} GET Log.can_read
+/importErrors GET ImportError.can_read
+/importErrors/{import_error_id} GET ImportError.can_read
+/health GET None
+/version GET None
+/pools GET Pool.can_read
+/pools POST Pool.can_create
+/pools/{pool_name} DELETE Pool.can_delete
+/pools/{pool_name} GET Pool.can_read
+/pools/{pool_name} PATCH Pool.can_edit
+/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances GET Dag.can_read, DagRun.can_read, TaskInstance.can_read
+/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id} GET Dag.can_read, DagRun.can_read, TaskInstance.can_read
+/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/links GET DagBag.can_read, Dag.can_read, DagRun.can_read, Task.can_read, TaskInstance.can_read
+/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/logs/{task_try_number} GET Dag.can_read, DagRun.can_read, Task.can_read
+/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/list POST DagBag.can_read, Dag.can_read, DagRun.can_read, Task.can_read, TaskInstance.can_read
+/variables GET Variable.can_read
+/variables POST Variable.can_create
+/variables/{variable_key} DELETE Variable.can_delete
+/variables/{variable_key} GET Variable.can_read
+/variables/{variable_key} PATCH Variable.can_edit
+/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/xcomEntries GET Dag.can_read, DagRun.can_read, Task.can_read, XCom.can_read
+/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/xcomEntries/{xcom_key} GET Dag.can_read, DagRun.can_read, Task.can_read, XCom.can_read
+================================================================================== ====== ====================================================================================
-.. image:: /img/add-role.png
-.. image:: /img/new-role.png
-
-The image shows the creation of a role which can only write to
-``example_python_operator``. You can also create roles via the CLI
-using the ``airflow roles create`` command, e.g.:
-
-.. code-block:: bash
-
- airflow roles create Role1 Role2
-
-And we could assign the given role to a new user using the ``airflow
-users add-role`` CLI command.
Review comment:
This might be a rebase deletion! Or was this intentional?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-696253413
@mik-laj Any final comments? James has addressed the per-dag access controls now.
There's one or two small things here to fix, but if you have no further comments I'll merge this tomorrow lunchtime.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r482904458
##########
File path: airflow/www/app.py
##########
@@ -58,6 +58,7 @@ def sync_appbuilder_roles(flask_app):
if conf.getboolean('webserver', 'UPDATE_FAB_PERMS'):
security_manager = flask_app.appbuilder.sm
security_manager.sync_roles()
+ security_manager.sync_resource_permissions()
Review comment:
This won't do anything if we dont' pass any permissions in.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r480819521
##########
File path: airflow/www/security.py
##########
@@ -505,6 +507,7 @@ def sync_roles(self):
:return: None.
"""
+ # breakpoint()
Review comment:
Removed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r492439382
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
Actually, I think we should maintain the existing behavior here and do the count prior to the date filtering. Just to keep scope limited. If we decide that the count should take place after the date filtering, let's make that behavioral change in a separate PR, just to reduce scope for this one somewhat.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj edited a comment on pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-693365767
Have you looked at this issue in other views as well?
For example Endpoint - "List XCom entries" allows specifying `~` as the dag_id, dag_run_id, task_id to retrieve XCOM entries for all DAGs, DAG runs and task instances.
This address URL returns all entries for all DAG in the database.
```
https://localhostot/api/v1/dags/~/dagRuns/~/taskInstances/~/xcomEntries
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #10594:
URL: https://github.com/apache/airflow/pull/10594
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r479838304
##########
File path: airflow/www/security.py
##########
@@ -521,6 +524,18 @@ def sync_roles(self):
self.update_admin_perm_view()
self.clean_perms()
+ def sync_resource_permissions(self):
+ """
+ Populates resource-based permissions.
+ """
+ resources = ['Pool', 'Connection', 'Dag', 'DagBag', 'DagRun',
+ 'DagCode', 'Log', 'Task', 'ImportError', 'Variable', 'XCom']
+ actions = ['can_create', 'can_read', 'can_edit', 'can_delete']
Review comment:
some of the resources here like ImportError doesn't have edit/delete/create endpoints right?
##########
File path: airflow/www/security.py
##########
@@ -505,6 +507,7 @@ def sync_roles(self):
:return: None.
"""
+ # breakpoint()
Review comment:
is it expected to keep this comment here?
##########
File path: airflow/api_connexion/security.py
##########
@@ -37,3 +37,32 @@ def decorated(*args, **kwargs):
return function(*args, **kwargs)
return cast(T, decorated)
+
+
+def requires_access(permissions: Sequence[Tuple[str, str]]) -> Callable[[T], T]:
Review comment:
should we combine @security.requires_access and @security.requires_authentication into one decorator instead? will there be a case where one would only use require_access without requires_authentication? this doesn't make sense semantically right?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r492438593
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
`total_entries` should return the number of elements that are available on all the page of the current request.
##########
File path: docs/security/access-control.rst
##########
@@ -114,3 +118,69 @@ using the ``airflow roles create`` command, e.g.:
And we could assign the given role to a new user using the ``airflow
users add-role`` CLI command.
+
+
+Permissions
+'''''''''''
+
+Resource-Based permissions
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Starting with version 2.0, permissions are based on individual resources and a small subset of actions on those
+resources. Resources match standard Airflow concepts, such as ``Dag``, ``DagRun``, ``Task``, and
+``Connection``. Actions include ``can_create``, ``can_read``, ``can_edit``, and ``can_delete``.
+
+Permissions (each consistent of a resource + action pair) are then added to roles.
+
+Starting with version 2.0, permissions are based on individual resources and a small subset of actions on those resources. Resources match standard Airflow concepts, such as ``Dag``, ``DagRun``, ``Task``, and ``Connection``. Actions include ``can_create``, ``can_read``, ``can_edit``, and ``can_delete``. Permissions (each consisting of a resource + action pair) are then added to roles.
Review comment:
The beginning of this paragraph is repeated.
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
Can you create a issues about it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r492438593
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
`total_entries` should return the number of elements that are available on all the page of the current request.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj edited a comment on pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-692141260
If there are 2 DAGs in the database: ``DAG_A``, ``DAG_B``, and you have permission - ``[can_read, DAG_A]``, then you will send a request - `GET /api/v1/dags`, what DAGs will you get? I guess you will either get information on all DAGs, or you will not get information on any.
Your database query should be different depending on the accessible DAGs for the user.
https://github.com/apache/airflow/blob/eaa49b2257913c34b15408a14e445f6106e691ee/airflow/www/views.py#L287
https://github.com/apache/airflow/blob/eaa49b2257913c34b15408a14e445f6106e691ee/airflow/www/views.py#L305-L306
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r492438024
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
Good count. I think the original behavior included a bug. @mik-laj can you confirm that the count should be taken after filtering by date?
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
Good catch. I think the original behavior included a bug. @mik-laj can you confirm that the count should be taken after filtering by date?
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -158,26 +158,29 @@ def _apply_date_filters_to_query(
return query
-@security.requires_authentication
+@security.requires_access([("can_read", "Dag"), ("can_read", "DagRun")])
@provide_session
def get_dag_runs_batch(session):
"""
Get list of DAG Runs
"""
body = request.get_json()
- try:
+ try: # TODO: Handle filtering.
data = dagruns_batch_form_schema.load(body)
except ValidationError as err:
raise BadRequest(detail=str(err.messages))
+ appbuilder = current_app.appbuilder
+ readable_dag_ids = appbuilder.sm.get_readable_dag_ids(g.user)
query = session.query(DagRun)
-
- if data["dag_ids"]:
- query = query.filter(DagRun.dag_id.in_(data["dag_ids"]))
+ if data.get("dag_ids"):
+ dag_ids = set(data["dag_ids"]) & set(readable_dag_ids)
+ query = query.filter(DagRun.dag_id.in_(dag_ids))
+ else:
+ query = query.filter(DagRun.dag_id.in_(readable_dag_ids))
dag_runs, total_entries = _fetch_dag_runs(
query,
- session,
Review comment:
It has to do with how the count is getting made, as seen in your previous comment. It used to use `session`, but it doesn't need to now.
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
Actually, I think we should maintain the existing behavior here and do the count prior to the date filtering. Just to keep scope limited. If we decide that the count should take place after the date filtering, let's make that behavioral change in a separate PR, just to reduce scope for this one somewhat.
##########
File path: airflow/api_connexion/endpoints/xcom_endpoint.py
##########
@@ -49,22 +52,28 @@ def get_xcom_entries(
"""
query = session.query(XCom)
- if dag_id != '~':
+ if dag_id == '~':
+ appbuilder = current_app.appbuilder
+ readable_dag_ids = appbuilder.sm.get_readable_dag_ids(g.user)
+ query = query.filter(XCom.dag_id.in_(readable_dag_ids))
+ query.join(DR, and_(XCom.dag_id.in_(readable_dag_ids), XCom.execution_date == DR.execution_date))
+ else:
query = query.filter(XCom.dag_id == dag_id)
Review comment:
Correct
##########
File path: airflow/api_connexion/endpoints/xcom_endpoint.py
##########
@@ -49,22 +52,28 @@ def get_xcom_entries(
"""
query = session.query(XCom)
- if dag_id != '~':
+ if dag_id == '~':
+ appbuilder = current_app.appbuilder
+ readable_dag_ids = appbuilder.sm.get_readable_dag_ids(g.user)
+ query = query.filter(XCom.dag_id.in_(readable_dag_ids))
+ query.join(DR, and_(XCom.dag_id.in_(readable_dag_ids), XCom.execution_date == DR.execution_date))
Review comment:
I've made the update, and created an issue to add a test for this
https://github.com/apache/airflow/issues/11073
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
I've created an issue to change how the total_entries are counted. #11074
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-696253413
@mik-laj Any final comments? James has addressed the per-dag access controls now.
There's one or two small things here to fix, but if you have no further comments I'll merge this tomorrow lunchtime.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-683761806
Can you add some docs? We have docs for access control, but that doesn't describe the permission in detail.
https://airflow.readthedocs.io/en/latest/security/access-control.html
Here is similar documentation on access control on other services.
https://cloud.google.com/composer/docs/how-to/access-control
https://cloud.google.com/workflows/docs/access-control
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r492221129
##########
File path: airflow/api_connexion/endpoints/xcom_endpoint.py
##########
@@ -49,22 +52,28 @@ def get_xcom_entries(
"""
query = session.query(XCom)
- if dag_id != '~':
+ if dag_id == '~':
+ appbuilder = current_app.appbuilder
+ readable_dag_ids = appbuilder.sm.get_readable_dag_ids(g.user)
+ query = query.filter(XCom.dag_id.in_(readable_dag_ids))
+ query.join(DR, and_(XCom.dag_id.in_(readable_dag_ids), XCom.execution_date == DR.execution_date))
+ else:
query = query.filter(XCom.dag_id == dag_id)
Review comment:
When there's a single dag_id the permissions decorator will validate access to that dag, correct?
##########
File path: airflow/api_connexion/endpoints/xcom_endpoint.py
##########
@@ -49,22 +52,28 @@ def get_xcom_entries(
"""
query = session.query(XCom)
- if dag_id != '~':
+ if dag_id == '~':
+ appbuilder = current_app.appbuilder
+ readable_dag_ids = appbuilder.sm.get_readable_dag_ids(g.user)
+ query = query.filter(XCom.dag_id.in_(readable_dag_ids))
+ query.join(DR, and_(XCom.dag_id.in_(readable_dag_ids), XCom.execution_date == DR.execution_date))
Review comment:
```suggestion
query.join(DR, and_(XCom.dag_id == DR.dag_id, XCom.execution_date == DR.execution_date))
```
This is not your PR causing this -- but I also think that this is missing a `query = ` at the start -- I don't _think_ `.join()` mutates the query, but returns a new one. I could be wrong on this. (Separate PR to fix it if this _doesn't_ work as is)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj edited a comment on pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-687500056
I think it is worth adding a description of the required permissions for each endpoint to the OpenAPI specification. What do you think about it? Currently, this information is only available in code.
I will look at it on Monday again.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r482584110
##########
File path: airflow/api_connexion/security.py
##########
@@ -37,3 +37,32 @@ def decorated(*args, **kwargs):
return function(*args, **kwargs)
return cast(T, decorated)
+
+
+def requires_access(permissions: Sequence[Tuple[str, str]]) -> Callable[[T], T]:
Review comment:
yeah that isnt particularly difficult, but I think it complicates the interface somewhat. I think I'll just create resources that aren't based on a specific model. So 'Health' or 'Configuration', just to keep the format consistent
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-687500056
I think it is worth adding a description of the required permissions for each endpoint to the OpenAPI specification. What do you think about it? Currently, this information is only available in code.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r490332164
##########
File path: airflow/models/dag.py
##########
@@ -1664,6 +1665,38 @@ def deactivate_stale_dags(expiration_date, session=None):
session.merge(dag)
session.commit()
+ @classmethod
+ def get_readable_dags(cls, user):
+ """Gets the DAGs readable by authenticated user."""
+ return cls.get_accessible_dags(security.CAN_READ, user)
+
+ @classmethod
+ def get_editable_dags(cls, user):
+ """Gets the DAGs editable by authenticated user."""
+ return cls.get_accessible_dags(security.CAN_EDIT, user)
+
+ @staticmethod
+ @provide_session
+ def get_accessible_dags(user_action, user, session=None):
+ """Generic function to get readable or writable DAGs for authenticated user."""
+
+ if user.is_anonymous or 'Public' in user.roles:
+ # return an empty set if the role is public
+ return set()
+
+ resources = set()
+ for role in user.roles:
+ for permission in role.permissions:
+ resource = permission.view_menu.name
+ action = permission.permission.name
+ if action == user_action:
+ resources.add(resource)
+
+ if 'Dag' in resources:
+ return session.query(DagModel)
+
+ return session.query(DagModel).filter(DagModel.dag_id.in_(resources))
+
Review comment:
@ashb It's certainly possible. I was deciding between letting flask code leak into Airflow core and putting core code into the security manager. I chose the former, but I can switch back to the latter.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r492220849
##########
File path: airflow/api_connexion/endpoints/xcom_endpoint.py
##########
@@ -49,22 +52,28 @@ def get_xcom_entries(
"""
query = session.query(XCom)
- if dag_id != '~':
+ if dag_id == '~':
+ appbuilder = current_app.appbuilder
+ readable_dag_ids = appbuilder.sm.get_readable_dag_ids(g.user)
+ query = query.filter(XCom.dag_id.in_(readable_dag_ids))
+ query.join(DR, and_(XCom.dag_id.in_(readable_dag_ids), XCom.execution_date == DR.execution_date))
Review comment:
```suggestion
query.join(DR, and_(XCom.dag_id == DR.dag_id, XCom.execution_date == DR.execution_date))
```
This is not your PR causing this -- but I also think that this is missing a `query = ` at the start -- I don't _think_ `.join()` mutates the query, but returns a new one. I could be wrong on this. (Separate PR to fix it if this _doesn't_ work as is)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r481490675
##########
File path: airflow/api_connexion/security.py
##########
@@ -37,3 +37,32 @@ def decorated(*args, **kwargs):
return function(*args, **kwargs)
return cast(T, decorated)
+
+
+def requires_access(permissions: Sequence[Tuple[str, str]]) -> Callable[[T], T]:
Review comment:
@houqp we do that, but there are times when authentication is needed, but we don't tie it to a specific permission. Such as the Health and Configs endpoints. We could add a semi-arbitrary permission for those cases (`('Health', 'can_read')`), or similar with reading configs. Or we could allow `requires_access()` to work without any permissions, in which case it only requires authentication.
What do you think? @mik-laj curious about your thoughts as well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r483749253
##########
File path: airflow/api_connexion/security.py
##########
@@ -16,25 +16,63 @@
# under the License.
from functools import wraps
-from typing import Callable, TypeVar, cast
+from typing import Callable, Optional, Sequence, Tuple, TypeVar, cast
from flask import Response, current_app
-from airflow.api_connexion.exceptions import Unauthenticated
+from airflow.api_connexion.exceptions import PermissionDenied, Unauthenticated
T = TypeVar("T", bound=Callable) # pylint: disable=invalid-name
-def requires_authentication(function: T):
- """Decorator for functions that require authentication"""
+def check_authentication():
+ """Checks that the request has valid authorization information."""
+ response = current_app.api_auth.requires_authentication(Response)()
+ if response.status_code != 200:
+ # since this handler only checks authentication, not authorization,
+ # we should always return 401
+ raise Unauthenticated(headers=response.headers)
- @wraps(function)
- def decorated(*args, **kwargs):
- response = current_app.api_auth.requires_authentication(Response)()
- if response.status_code != 200:
- # since this handler only checks authentication, not authorization,
- # we should always return 401
- raise Unauthenticated(headers=response.headers)
- return function(*args, **kwargs)
- return cast(T, decorated)
+def check_authorization(
+ permissions: Optional[Sequence[Tuple[str, str]]] = None, dag_id: Optional[int] = None
+):
Review comment:
could you annotate the return type as well?
```suggestion
) -> None:
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r483844302
##########
File path: airflow/www/security.py
##########
@@ -151,10 +152,12 @@ class AirflowSecurityManager(SecurityManager, LoggingMixin):
WRITE_DAG_PERMS = {
'can_dag_edit',
+ 'can_edit',
}
READ_DAG_PERMS = {
'can_dag_read',
+ 'can_read',
Review comment:
Currently, the UI is still using `can_dag_read`. When I backport the new permissions I can remove this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#issuecomment-687503868
Alternatively, you can add the tables to the documentation and that will be enough for me too.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r480108441
##########
File path: tests/test_utils/api_connexion_utils.py
##########
@@ -30,6 +30,19 @@ def create_user(app, username, role):
)
+def create_role(app, name, permissions=None):
Review comment:
What do you think about extending the create_user method to accept a list of permissions?
```
def create_user(app, username, role=None, permissions=None):
appbuilder = app.appbuilder
current_role = None
if role and permission:
raise IllegalArgument("role and permission are muttually exclussive")
elif role:
current_role = appbuilder.sm.find_role(role)
elif permission:
ranadom_suffix = get_random_string()
current_role = create_role(app, get_random_string("test-role-{ranadom_suffix}"))
tester = appbuilder.sm.find_user(username=username)
if not tester:
appbuilder.sm.add_user(
username=username,
first_name=username,
last_name=username,
email=f"{username}@fab.org",
role=current_role,
password=username,
)
```
This will simplify testing as we won't have to worry so much about side effects as each role will only be used once. This will also explicitly associate the user with a specific permission set.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r492439548
##########
File path: airflow/api_connexion/endpoints/xcom_endpoint.py
##########
@@ -49,22 +52,28 @@ def get_xcom_entries(
"""
query = session.query(XCom)
- if dag_id != '~':
+ if dag_id == '~':
+ appbuilder = current_app.appbuilder
+ readable_dag_ids = appbuilder.sm.get_readable_dag_ids(g.user)
+ query = query.filter(XCom.dag_id.in_(readable_dag_ids))
+ query.join(DR, and_(XCom.dag_id.in_(readable_dag_ids), XCom.execution_date == DR.execution_date))
+ else:
query = query.filter(XCom.dag_id == dag_id)
Review comment:
Correct
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r492438593
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
`total_entries` should return the number of elements that are available on all the page of the current request.
##########
File path: docs/security/access-control.rst
##########
@@ -114,3 +118,69 @@ using the ``airflow roles create`` command, e.g.:
And we could assign the given role to a new user using the ``airflow
users add-role`` CLI command.
+
+
+Permissions
+'''''''''''
+
+Resource-Based permissions
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Starting with version 2.0, permissions are based on individual resources and a small subset of actions on those
+resources. Resources match standard Airflow concepts, such as ``Dag``, ``DagRun``, ``Task``, and
+``Connection``. Actions include ``can_create``, ``can_read``, ``can_edit``, and ``can_delete``.
+
+Permissions (each consistent of a resource + action pair) are then added to roles.
+
+Starting with version 2.0, permissions are based on individual resources and a small subset of actions on those resources. Resources match standard Airflow concepts, such as ``Dag``, ``DagRun``, ``Task``, and ``Connection``. Actions include ``can_create``, ``can_read``, ``can_edit``, and ``can_delete``. Permissions (each consisting of a resource + action pair) are then added to roles.
Review comment:
The beginning of this paragraph is repeated.
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -122,6 +122,7 @@ def _fetch_dag_runs(
limit,
offset,
):
+ total_entries = query.count()
Review comment:
Can you create a issues about it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #10594: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r492439144
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -158,26 +158,29 @@ def _apply_date_filters_to_query(
return query
-@security.requires_authentication
+@security.requires_access([("can_read", "Dag"), ("can_read", "DagRun")])
@provide_session
def get_dag_runs_batch(session):
"""
Get list of DAG Runs
"""
body = request.get_json()
- try:
+ try: # TODO: Handle filtering.
data = dagruns_batch_form_schema.load(body)
except ValidationError as err:
raise BadRequest(detail=str(err.messages))
+ appbuilder = current_app.appbuilder
+ readable_dag_ids = appbuilder.sm.get_readable_dag_ids(g.user)
query = session.query(DagRun)
-
- if data["dag_ids"]:
- query = query.filter(DagRun.dag_id.in_(data["dag_ids"]))
+ if data.get("dag_ids"):
+ dag_ids = set(data["dag_ids"]) & set(readable_dag_ids)
+ query = query.filter(DagRun.dag_id.in_(dag_ids))
+ else:
+ query = query.filter(DagRun.dag_id.in_(readable_dag_ids))
dag_runs, total_entries = _fetch_dag_runs(
query,
- session,
Review comment:
It has to do with how the count is getting made, as seen in your previous comment. It used to use `session`, but it doesn't need to now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10594: WIP: Add permissions for stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10594:
URL: https://github.com/apache/airflow/pull/10594#discussion_r486465035
##########
File path: docs/security/access-control.rst
##########
@@ -114,3 +114,61 @@ using the ``airflow roles create`` command, e.g.:
And we could assign the given role to a new user using the ``airflow
users add-role`` CLI command.
+
+Permissions
+'''''''''''
+
+Resource-Based permissions
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+Starting with version 2.0, permissions are based on individual resources and a small subset of actions on those resources. Resources match standard Airflow concepts, such as ``Dag``, ``DagRun``, ``Task``, and ``Connection``. Actions include ``can_create``, ``can_read``, ``can_edit``, and ``can_delete``. Permissions (each consistint of a resource + action pair) are then added to roles.
+
+Simple table:
+
+================================================================================== ====== ====================================================================================
+ Inputs
+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+View Method Permissions
+================================================================================== ====== ====================================================================================
+/config GET Config.can_read
+/connections GET Connection.can_read
+/connections POST Connection.can_create
+/connections/{connection_id} DELETE Connection.can_delete
+/connections/{connection_id} GET Connection.can_read
+/connections/{connection_id} PATCH Connection.can_edit
+/dagSources/{file_token} GET DagCode.can_read
+/dags GET Dag.can_read
+/dags/{dag_id} GET Dag.can_read
+/dags/{dag_id} PATCH Dag.can_edit
+/dags/{dag_id}/clearTaskInstances POST Dag.can_read, DagRun.can_read, TaskInstance.can_edit
+/dags/{dag_id}/details GET Dag.can_read
+/dags/{dag_id}/tasks GET Dag.can_read, Task.can_read
+/dags/{dag_id}/tasks/{task_id} GET Dag.can_read, Task.can_read
+/dags/{dag_id}/dagRuns GET Dag.can_read, DagRun.can_read
+/dags/{dag_id}/dagRuns POST Dag.can_read, DagRun.can_create
+/dags/{dag_id}/dagRuns/{dag_run_id} DELETE Dag.can_read, DagRun.can_delete
+/dags/{dag_id}/dagRuns/{dag_run_id} GET Dag.can_read, DagRun.can_read
+/dags/{dag_id}/dagRuns/list POST Dag.can_read, DagRun.can_read
+/eventLogs GET Log.can_read
+/eventLogs/{event_log_id} GET Log.can_read
+/importErrors GET ImportError.can_read
+/importErrors/{import_error_id} GET ImportError.can_read
+/health GET None
+/version GET None
+/pools GET Pool.can_read
+/pools POST Pool.can_create
+/pools/{pool_name} DELETE Pool.can_delete
+/pools/{pool_name} GET Pool.can_read
+/pools/{pool_name} PATCH Pool.can_edit
+/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances GET Dag.can_read, DagRun.can_read, TaskInstance.can_read
+/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id} GET Dag.can_read, DagRun.can_read, TaskInstance.can_read
+/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/links GET DagBag.can_read, Dag.can_read, DagRun.can_read, Task.can_read, TaskInstance.can_read
Review comment:
```suggestion
/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/links GET Dag.can_read, DagRun.can_read, Task.can_read, TaskInstance.can_read
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org