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/12 23:15:47 UTC
[GitHub] [airflow] jhtimmins opened a new pull request #10308: Adds a role based authentication backend for the experimental API
jhtimmins opened a new pull request #10308:
URL: https://github.com/apache/airflow/pull/10308
<!--
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] jhtimmins edited a comment on pull request #10308: Adds a role based authentication backend for the experimental API
Posted by GitBox <gi...@apache.org>.
jhtimmins edited a comment on pull request #10308:
URL: https://github.com/apache/airflow/pull/10308#issuecomment-673736854
> What do you think to add role support for all auth backend? I don't understand why you want to do it on the authentication level and not as a separate decorator that will be responsible for permissions handling.
@mik-laj We did it this way for the sake of simplicity. Rather than adding an additional hook for an API that's going to be deprecated shortly, this lets us hook into existing functionality in a non-disruptive way.
----------------------------------------------------------------
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 #10308: Adds a role based authentication backend for the experimental API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10308:
URL: https://github.com/apache/airflow/pull/10308#issuecomment-673744325
How can I configure your auth backend and authorization with Google OpenID at the same time?
----------------------------------------------------------------
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 #10308: Adds a role based authentication backend for the experimental API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10308:
URL: https://github.com/apache/airflow/pull/10308#discussion_r469603674
##########
File path: airflow/api/auth/backend/role_based_auth.py
##########
@@ -0,0 +1,77 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Role based authentication backend - access is based on user roles."""
+from functools import wraps
+from typing import Callable, Optional, Tuple, TypeVar, Union
+
+from flask import Response
+from requests.auth import AuthBase
+
+from airflow.www.app import cached_app
+
+CLIENT_AUTH: Optional[Union[Tuple[str, str], AuthBase]] = None
+
+
+def init_app(_):
+ """Initializes authentication backend"""
+
+
+T = TypeVar("T", bound=Callable) # pylint: disable=invalid-name
+
+
+def requires_authentication(function):
+ """Decorator for functions that require authentication"""
+
+ @wraps(function)
+ def decorated(*args, **kwargs):
+ appbuilder = cached_app().appbuilder # pylint: disable=no-member
Review comment:
```suggestion
appbuilder = current_app.appbuilder # pylint: disable=no-member
```
Please use Flask.current_app. This makes it harder to create side effects.
----------------------------------------------------------------
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 #10308: Adds a role based authentication backend for the experimental API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #10308:
URL: https://github.com/apache/airflow/pull/10308#discussion_r470165669
##########
File path: tests/www/test_role_based_auth.py
##########
@@ -0,0 +1,241 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import json
+import unittest
+from datetime import datetime
+
+from flask import g
+
+from airflow import settings
+from airflow.api.common.experimental import delete_dag, pool
+from airflow.exceptions import DagNotFound, PoolNotFound
+from airflow.models.dag import DagModel
+from airflow.www import app as application
+from tests.test_utils.config import conf_vars
+
+
+class RoleBasedAuthTest(unittest.TestCase):
+
+ def setUp(self):
+ self.tearDownClass()
+ with conf_vars(
+ {("api", "auth_backend"): "airflow.api.auth.backend.role_based_auth"}
+ ):
+ settings.configure_orm()
+ self.session = settings.Session
+ self.app = application.create_app(testing=True)
+ self.admin_role = self.app.appbuilder.sm.find_role("Admin") # pylint: disable=no-member
+ admin_username = "das_admin"
+ admin_email = "das_admin@fab.org"
+ self.admin_user = self.app.appbuilder.sm.find_user( # pylint: disable=no-member
+ username=admin_username, email=admin_email
+ )
+ if not self.admin_user:
+ self.admin_user = self.app.appbuilder.sm.add_user( # pylint: disable=no-member
+ admin_username,
+ "admin",
+ "user",
+ admin_email,
+ self.admin_role,
+ "general",
+ )
+ self.admin_user.roles = [self.admin_role]
+
+ viewer_username = "das_viewer"
+ viewer_email = "das_viewer@fab.org"
+ self.viewer_role = self.app.appbuilder.sm.find_role("Viewer") # pylint: disable=no-member
+ self.viewer_user = self.app.appbuilder.sm.find_user( # pylint: disable=no-member
+ username=viewer_username, email=viewer_email
+ )
+ if not self.viewer_user:
+ self.viewer_user = self.app.appbuilder.sm.add_user( # pylint: disable=no-member
+ viewer_username,
+ "viewer",
+ "user",
+ viewer_email,
+ self.viewer_role,
+ "general",
+ )
+ self.viewer_user.roles = [self.viewer_role]
+ pool.create_pool("not_swimming_pool", 16, "a description")
+
+ @classmethod
+ def tearDownClass(cls):
+ pools = [
+ "test_delete_pool_unauthorized",
+ "test_delete_pool_authorized",
+ "test_create_pool_unauthorized",
+ "test_create_pool_authorized",
+ ]
+ for pool_name in pools:
+ try:
+ pool.delete_pool(pool_name)
+ except PoolNotFound:
+ pass
+
+ dags = [
+ "test_delete_dag_unauthorized",
+ "test_delete_dag_authorized",
+ "test_pause_dag_unauthorized",
+ "test_delete_dag_authorized",
+ "test_pause_dag_unauthorized",
+ "test_pause_dag_authorized",
+ "test_trigger_dag_unauthorized",
+ ]
+
+ for dag in dags:
+ try:
+ delete_dag.delete_dag(dag)
+ except DagNotFound:
+ pass
+
+ def create_test_dag(self, dag_id):
+ dag = DagModel(dag_id=dag_id)
+ self.session.add(dag)
+ self.session.commit()
+
+ def use_admin_role(self):
Review comment:
This isn't accessible to users. It just allows the tests to toggle between Admin access and Viewer access.
----------------------------------------------------------------
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 closed pull request #10308: Adds a role based authentication backend for the experimental API
Posted by GitBox <gi...@apache.org>.
jhtimmins closed pull request #10308:
URL: https://github.com/apache/airflow/pull/10308
----------------------------------------------------------------
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 #10308: Adds a role based authentication backend for the experimental API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10308:
URL: https://github.com/apache/airflow/pull/10308#discussion_r469606214
##########
File path: tests/www/test_role_based_auth.py
##########
@@ -0,0 +1,241 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import json
+import unittest
+from datetime import datetime
+
+from flask import g
+
+from airflow import settings
+from airflow.api.common.experimental import delete_dag, pool
+from airflow.exceptions import DagNotFound, PoolNotFound
+from airflow.models.dag import DagModel
+from airflow.www import app as application
+from tests.test_utils.config import conf_vars
+
+
+class RoleBasedAuthTest(unittest.TestCase):
+
+ def setUp(self):
+ self.tearDownClass()
+ with conf_vars(
+ {("api", "auth_backend"): "airflow.api.auth.backend.role_based_auth"}
+ ):
+ settings.configure_orm()
+ self.session = settings.Session
+ self.app = application.create_app(testing=True)
+ self.admin_role = self.app.appbuilder.sm.find_role("Admin") # pylint: disable=no-member
+ admin_username = "das_admin"
+ admin_email = "das_admin@fab.org"
+ self.admin_user = self.app.appbuilder.sm.find_user( # pylint: disable=no-member
+ username=admin_username, email=admin_email
+ )
+ if not self.admin_user:
+ self.admin_user = self.app.appbuilder.sm.add_user( # pylint: disable=no-member
+ admin_username,
+ "admin",
+ "user",
+ admin_email,
+ self.admin_role,
+ "general",
+ )
+ self.admin_user.roles = [self.admin_role]
+
+ viewer_username = "das_viewer"
+ viewer_email = "das_viewer@fab.org"
+ self.viewer_role = self.app.appbuilder.sm.find_role("Viewer") # pylint: disable=no-member
+ self.viewer_user = self.app.appbuilder.sm.find_user( # pylint: disable=no-member
+ username=viewer_username, email=viewer_email
+ )
+ if not self.viewer_user:
+ self.viewer_user = self.app.appbuilder.sm.add_user( # pylint: disable=no-member
+ viewer_username,
+ "viewer",
+ "user",
+ viewer_email,
+ self.viewer_role,
+ "general",
+ )
+ self.viewer_user.roles = [self.viewer_role]
+ pool.create_pool("not_swimming_pool", 16, "a description")
+
+ @classmethod
+ def tearDownClass(cls):
+ pools = [
+ "test_delete_pool_unauthorized",
+ "test_delete_pool_authorized",
+ "test_create_pool_unauthorized",
+ "test_create_pool_authorized",
+ ]
+ for pool_name in pools:
+ try:
+ pool.delete_pool(pool_name)
+ except PoolNotFound:
+ pass
+
+ dags = [
+ "test_delete_dag_unauthorized",
+ "test_delete_dag_authorized",
+ "test_pause_dag_unauthorized",
+ "test_delete_dag_authorized",
+ "test_pause_dag_unauthorized",
+ "test_pause_dag_authorized",
+ "test_trigger_dag_unauthorized",
+ ]
+
+ for dag in dags:
+ try:
+ delete_dag.delete_dag(dag)
+ except DagNotFound:
+ pass
+
+ def create_test_dag(self, dag_id):
+ dag = DagModel(dag_id=dag_id)
+ self.session.add(dag)
+ self.session.commit()
+
+ def use_admin_role(self):
Review comment:
How is the end user to set this attribute?
----------------------------------------------------------------
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 #10308: Adds a role based authentication backend for the experimental API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #10308:
URL: https://github.com/apache/airflow/pull/10308#issuecomment-673736854
> What do you think to add role support for all auth backend? I don't understand why you want to do it on the authentication level and not as a separate decorator that will be responsible for permissions handling.
We did it this way for the sake of simplicity. Rather than adding an additional hook for an API that's going to be deprecated shortly, this lets us hook into existing functionality in a non-disruptive way.
----------------------------------------------------------------
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 #10308: Adds a role based authentication backend for the experimental API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10308:
URL: https://github.com/apache/airflow/pull/10308#issuecomment-673161486
Can you add some documentation? https://airflow.readthedocs.io/en/latest/security.html#api-authentication
----------------------------------------------------------------
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 #10308: Adds a role based authentication backend for the experimental API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #10308:
URL: https://github.com/apache/airflow/pull/10308#issuecomment-674949577
@mik-laj Your concerns make sense. I'm going to close this ticket and will move it into an internal repo of ours. Thanks for reviewing 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 #10308: Adds a role based authentication backend for the experimental API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10308:
URL: https://github.com/apache/airflow/pull/10308#issuecomment-673161101
What do you think to add role support for all auth backend? I don't understand why you want to do it on the authentication level and not as a separate decorator that will be responsible for permissions handling.
----------------------------------------------------------------
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