You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by ka...@apache.org on 2021/09/10 13:24:28 UTC
[airflow] 04/04: Avoid redirect loop for users with no permissions
(#17838)
This is an automated email from the ASF dual-hosted git repository.
kaxilnaik pushed a commit to branch v2-1-test
in repository https://gitbox.apache.org/repos/asf/airflow.git
commit 8a1990b305e9b3e2a9ffcef895f2cda5d8c1d1dd
Author: Jed Cunningham <66...@users.noreply.github.com>
AuthorDate: Thu Aug 26 14:59:30 2021 -0600
Avoid redirect loop for users with no permissions (#17838)
Like we recently did for users with no roles, also handle it gracefully
when users have no permissions instead of letting them get stuck in a
redirect loop.
This also changes the approach to rendering the template as a 403 for
the originally requested URI instead of redirecting to a separate endpoint.
Closes: #16587
(cherry picked from commit e18b6a6d19f9ea0d8fe760ba00adf38810f0e510)
---
airflow/www/auth.py | 18 ++++++++++--
airflow/www/security.py | 6 ++++
.../{no_roles.html => no_roles_permissions.html} | 2 +-
airflow/www/views.py | 14 ---------
tests/www/test_security.py | 21 +++++++++++++
tests/www/views/test_views_acl.py | 34 ++++++++++++++++------
tests/www/views/test_views_base.py | 2 +-
7 files changed, 69 insertions(+), 28 deletions(-)
diff --git a/airflow/www/auth.py b/airflow/www/auth.py
index b1218e0..0b1d9ed 100644
--- a/airflow/www/auth.py
+++ b/airflow/www/auth.py
@@ -15,10 +15,13 @@
# specific language governing permissions and limitations
# under the License.
+import socket
from functools import wraps
from typing import Callable, Optional, Sequence, Tuple, TypeVar, cast
-from flask import current_app, flash, g, redirect, request, url_for
+from flask import current_app, flash, g, redirect, render_template, request, url_for
+
+from airflow.configuration import conf
T = TypeVar("T", bound=Callable)
@@ -30,8 +33,17 @@ def has_access(permissions: Optional[Sequence[Tuple[str, str]]] = None) -> Calla
@wraps(func)
def decorated(*args, **kwargs):
appbuilder = current_app.appbuilder
- if not g.user.is_anonymous and not g.user.roles:
- return redirect(url_for("Airflow.no_roles"))
+ if not g.user.is_anonymous and not appbuilder.sm.current_user_has_permissions():
+ return (
+ render_template(
+ 'airflow/no_roles_permissions.html',
+ hostname=socket.getfqdn()
+ if conf.getboolean('webserver', 'EXPOSE_HOSTNAME', fallback=True)
+ else 'redact',
+ logout_url=appbuilder.get_url_for_logout,
+ ),
+ 403,
+ )
if appbuilder.sm.check_authorization(permissions, request.args.get('dag_id', None)):
return func(*args, **kwargs)
diff --git a/airflow/www/security.py b/airflow/www/security.py
index 9259043..a47a6a9 100644
--- a/airflow/www/security.py
+++ b/airflow/www/security.py
@@ -274,6 +274,12 @@ class AirflowSecurityManager(SecurityManager, LoggingMixin):
)
return perms_views
+ def current_user_has_permissions(self) -> bool:
+ for role in self.get_user_roles():
+ if role.permissions:
+ return True
+ return False
+
def get_readable_dags(self, user):
"""Gets the DAGs readable by authenticated user."""
return self.get_accessible_dags([permissions.ACTION_CAN_READ], user)
diff --git a/airflow/www/templates/airflow/no_roles.html b/airflow/www/templates/airflow/no_roles_permissions.html
similarity index 96%
rename from airflow/www/templates/airflow/no_roles.html
rename to airflow/www/templates/airflow/no_roles_permissions.html
index 20f5eab..eaa54c0 100644
--- a/airflow/www/templates/airflow/no_roles.html
+++ b/airflow/www/templates/airflow/no_roles_permissions.html
@@ -26,7 +26,7 @@
<body>
<div style="font-family: verdana; text-align: center; margin-top: 200px;">
<img src="{{ url_for('static', filename='pin_100.png') }}" width="50px" alt="pin-logo" />
- <h1>Your user has no roles!</h1>
+ <h1>Your user has no roles and/or permissions!</h1>
<p>Unfortunately your user has no roles, and therefore you cannot use Airflow.</p>
<p>Please contact your Airflow administrator
(<a href="https://airflow.apache.org/docs/apache-airflow/stable/security/webserver.html#web-authentication">authentication</a>
diff --git a/airflow/www/views.py b/airflow/www/views.py
index 6296058..b26df70 100644
--- a/airflow/www/views.py
+++ b/airflow/www/views.py
@@ -500,20 +500,6 @@ class Airflow(AirflowBaseView):
return wwwutils.json_response(payload)
- @expose('/no_roles')
- def no_roles(self):
- """Show 'user has no roles' on screen (instead of an endless redirect loop)"""
- if g.user.is_anonymous or g.user.roles:
- return redirect(url_for("Airflow.index"))
-
- return render_template(
- 'airflow/no_roles.html',
- hostname=socket.getfqdn()
- if conf.getboolean('webserver', 'EXPOSE_HOSTNAME', fallback=True)
- else 'redact',
- logout_url=current_app.appbuilder.get_url_for_logout,
- )
-
@expose('/home')
@auth.has_access(
[
diff --git a/tests/www/test_security.py b/tests/www/test_security.py
index 358387b..02a00ef 100644
--- a/tests/www/test_security.py
+++ b/tests/www/test_security.py
@@ -345,6 +345,27 @@ class TestSecurity(unittest.TestCase):
mock_get_user_roles.return_value = []
assert len(self.security_manager.get_current_user_permissions()) == 0
+ @mock.patch('airflow.www.security.g')
+ def test_current_user_has_permissions(self, mock_g):
+ with self.app.app_context():
+ user = api_connexion_utils.create_user(
+ self.app,
+ "current_user_has_permissions",
+ "current_user_has_permissions",
+ permissions=[("can_some_action", "SomeBaseView")],
+ )
+ mock_g.user = user
+ assert self.security_manager.current_user_has_permissions()
+
+ # Role, but no permissions
+ role = user.roles[0]
+ role.permissions = []
+ assert not self.security_manager.current_user_has_permissions()
+
+ # No role
+ user.roles = []
+ assert not self.security_manager.current_user_has_permissions()
+
def test_get_accessible_dag_ids(self):
role_name = 'MyRole1'
permission_action = [permissions.ACTION_CAN_READ]
diff --git a/tests/www/views/test_views_acl.py b/tests/www/views/test_views_acl.py
index b2d97c8..0341a4b 100644
--- a/tests/www/views/test_views_acl.py
+++ b/tests/www/views/test_views_acl.py
@@ -782,22 +782,38 @@ def client_no_roles(acl_app, user_no_roles):
)
+@pytest.fixture(scope="module")
+def user_no_permissions(acl_app):
+ return create_user(
+ acl_app,
+ username="no_permissions_user",
+ role_name="no_permissions_role",
+ )
+
+
+@pytest.fixture()
+def client_no_permissions(acl_app, user_no_permissions):
+ return client_with_login(
+ acl_app,
+ username="no_permissions_user",
+ password="no_permissions_user",
+ )
+
+
@pytest.fixture()
def client_anonymous(acl_app):
return acl_app.test_client()
@pytest.mark.parametrize(
- "client, url, expected_content",
+ "client, url, status_code, expected_content",
[
- ["client_no_roles", "/home", "Your user has no roles!"],
- ["client_no_roles", "/no_roles", "Your user has no roles!"],
- ["client_all_dags", "/home", "DAGs - Airflow"],
- ["client_all_dags", "/no_roles", "DAGs - Airflow"],
- ["client_anonymous", "/home", "Sign In"],
- ["client_anonymous", "/no_roles", "Sign In"],
+ ["client_no_roles", "/home", 403, "Your user has no roles and/or permissions!"],
+ ["client_no_permissions", "/home", 403, "Your user has no roles and/or permissions!"],
+ ["client_all_dags", "/home", 200, "DAGs - Airflow"],
+ ["client_anonymous", "/home", 200, "Sign In"],
],
)
-def test_no_roles_redirect(request, client, url, expected_content):
+def test_no_roles_permissions(request, client, url, status_code, expected_content):
resp = request.getfixturevalue(client).get(url, follow_redirects=True)
- check_content_in_response(expected_content, resp)
+ check_content_in_response(expected_content, resp, status_code)
diff --git a/tests/www/views/test_views_base.py b/tests/www/views/test_views_base.py
index f16e17a..b071a4c 100644
--- a/tests/www/views/test_views_base.py
+++ b/tests/www/views/test_views_base.py
@@ -33,7 +33,7 @@ from tests.test_utils.www import check_content_in_response, check_content_not_in
def test_index(admin_client):
- with assert_queries_count(44):
+ with assert_queries_count(45):
resp = admin_client.get('/', follow_redirects=True)
check_content_in_response('DAGs', resp)