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 2021/02/03 01:55:55 UTC
[GitHub] [airflow] jhtimmins opened a new pull request #14042: Allow anon users to have arbitrary roles.
jhtimmins opened a new pull request #14042:
URL: https://github.com/apache/airflow/pull/14042
Fixes the issue wherein regardless of what role anonymous users are assigned (via the `AUTH_ROLE_PUBLIC` env var), they can't see any DAGs.
Current behavior causes:
Anonymous users are handled as a special case by Airflow's DAG-related security methods (`.has_access()` and `.get_accessible_dags()`). Rather than checking the `AUTH_ROLE_PUBLIC` value to check for role permissions, the methods reject access to view or edit any DAGs.
Changes in this PR:
Rather than hardcoding permission rules inside the security methods, this change checks the `AUTH_ROLE_PUBLIC` value and gives anonymous users all permissions linked to the designated role.
**This places security in the hands of the Airflow users. If the value is set to `Admin`, anonymous users will have full admin functionality.**
This also changes how the `Public` role is created. Currently, the `Public` role is created automatically by Flask App Builder. This PR explicitly declares `Public` as a default role with no permissions in `security.py`. This change makes it easier to test.
closes: #13340
----------------------------------------------------------------
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 #14042: Make the role assigned to anonymous users customizable
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #14042:
URL: https://github.com/apache/airflow/pull/14042#issuecomment-772846689
@ashb @kaxil Made the suggested updates
----------------------------------------------------------------
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] AmarEL commented on pull request #14042: Make the role assigned to anonymous users customizable
Posted by GitBox <gi...@apache.org>.
AmarEL commented on pull request #14042:
URL: https://github.com/apache/airflow/pull/14042#issuecomment-772470571
Looks perfect, I did some similar tests here and this should work
----------------------------------------------------------------
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 #14042: Make the role assigned to anonymous users customizable
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #14042:
URL: https://github.com/apache/airflow/pull/14042#issuecomment-772150430
@AmarEL @ashb @kaxil If y'all wouldn't mind taking a look at 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] github-actions[bot] commented on pull request #14042: Make the role assigned to anonymous users customizable
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14042:
URL: https://github.com/apache/airflow/pull/14042#issuecomment-772603279
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
----------------------------------------------------------------
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 #14042: Make the role assigned to anonymous users customizable
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #14042:
URL: https://github.com/apache/airflow/pull/14042#discussion_r569432133
##########
File path: tests/www/test_security.py
##########
@@ -170,10 +176,74 @@ def test_update_and_verify_permission_role(self):
assert role_perms_len == new_role_perms_len
def test_verify_public_role_has_no_permissions(self):
+ public = self.appbuilder.sm.find_role("Public")
+
+ assert public.permissions == []
+
+ def test_verify_default_anon_user_has_no_accessible_dag_ids(self):
with self.app.app_context():
- public = self.appbuilder.sm.find_role("Public")
+ user = mock.MagicMock()
+ user.is_anonymous = True
+ self.app.config['AUTH_ROLE_PUBLIC'] = 'Public'
+ assert self.app.appbuilder.sm.get_user_roles(user) == [self.app.appbuilder.sm.find_role("Public")]
+
+ self._create_dag("test_dag_id")
+ self.security_manager.sync_roles()
+
+ assert self.security_manager.get_accessible_dag_ids(user) == set()
+
+ def test_verify_default_anon_user_has_no_access_to_specific_dag(self):
+ with self.app.app_context():
+ user = mock.MagicMock()
+ user.is_anonymous = True
+ self.app.config['AUTH_ROLE_PUBLIC'] = 'Public'
+ assert self.app.appbuilder.sm.get_user_roles(user) == [self.app.appbuilder.sm.find_role("Public")]
Review comment:
```suggestion
assert self.app.appbuilder.sm.get_user_roles(user) == [self.app.appbuilder.sm.get_public_role()]
```
----------------------------------------------------------------
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 #14042: Make the role assigned to anonymous users customizable
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #14042:
URL: https://github.com/apache/airflow/pull/14042
----------------------------------------------------------------
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