Don't add User role perms to custom roles. (#13856)

closes: #9245 #13511
(cherry picked from commit 35b5a38)

airflow/www/security.py

@@ -43,7 +43,7 @@
 }
 
 
-class AirflowSecurityManager(SecurityManager, LoggingMixin):
+class AirflowSecurityManager(SecurityManager, LoggingMixin):  # pylint: disable=too-many-public-methods
     """Custom security manager, which introduces an permission model adapted to Airflow"""
 
     ###########################################################################
@@ -220,8 +220,8 @@ def get_user_roles(user=None):
             return [current_app.appbuilder.sm.find_role(public_role)] if public_role else []
         return user.roles
 
-    def get_all_permissions_views(self):
-        """Returns a set of tuples with the perm name and view menu name"""
+    def get_current_user_permissions(self):
+        """Returns permissions for logged in user as a set of tuples with the perm name and view menu name"""
         perms_views = set()
         for role in self.get_user_roles():
             perms_views.update(
@@ -363,7 +363,7 @@ def has_access(self, permission, resource, user=None) -> bool:
 
     def _get_and_cache_perms(self):
         """Cache permissions-views"""
-        self.perms = self.get_all_permissions_views()
+        self.perms = self.get_current_user_permissions()
 
     def _has_role(self, role_name_or_list):
         """Whether the user has this role name"""
@@ -439,89 +439,48 @@ def _merge_perm(self, permission_name, view_menu_name):
         if not permission_view and permission_name and view_menu_name:
             self.add_permission_view_menu(permission_name, view_menu_name)
 
-    @provide_session
-    def create_custom_dag_permission_view(self, session=None):
+    def add_homepage_access_to_custom_roles(self):
         """
-        Workflow:
-        1. Fetch all the existing (permissions, view-menu) from Airflow DB.
-        2. Fetch all the existing dag models that are either active or paused.
-        3. Create both read and write permission view-menus relation for every dags from step 2
-        4. Find out all the dag specific roles(excluded pubic, admin, viewer, op, user)
-        5. Get all the permission-vm owned by the user role.
-        6. Grant all the user role's permission-vm except the all-dag view-menus to the dag roles.
-        7. Commit the updated permission-vm-role into db
+        Add Website.can_read access to all roles.
 
         :return: None.
         """
-        self.log.debug('Fetching a set of all permission, view_menu from FAB meta-table')
+        website_permission = self.add_permission_view_menu(
+            permissions.ACTION_CAN_READ, permissions.RESOURCE_WEBSITE
+        )
+        for role in self.get_all_roles():
+            self.add_permission_role(role, website_permission)
 
-        def merge_pv(perm, view_menu):
-            """Create permission view menu only if it doesn't exist"""
-            if view_menu and perm and (view_menu, perm) not in all_permission_views:
-                self._merge_perm(perm, view_menu)
+        self.get_session.commit()
 
-        all_permission_views = set()
+    def get_all_permissions(self):
+        """Returns all permissions as a set of tuples with the perm name and view menu name"""
+        perms = set()
         for permission_view in self.get_session.query(self.permissionview_model).all():
             if permission_view.permission and permission_view.view_menu:
-                all_permission_views.add((permission_view.permission.name, permission_view.view_menu.name))
+                perms.add((permission_view.permission.name, permission_view.view_menu.name))
+
+        return perms
+
+    @provide_session
+    def create_dag_specific_permissions(self, session=None):
+        """
+        Creates 'can_read' and 'can_edit' permissions for all active and paused DAGs.
 
-        # Get all the active / paused dags and insert them into a set
-        all_dags_models = (
+        :return: None.
+        """
+        perms = self.get_all_permissions()
+        dag_models = (
             session.query(models.DagModel)
             .filter(or_(models.DagModel.is_active, models.DagModel.is_paused))
             .all()
         )
 
-        # create can_edit and can_read permissions for every dag(vm)
-        for dag in all_dags_models:
-            for perm in self.DAG_PERMS:
-                merge_pv(perm, self.prefixed_dag_id(dag.dag_id))
-
-        # for all the dag-level role, add the permission of viewer
-        # with the dag view to ab_permission_view
-        all_roles = self.get_all_roles()
-        user_role = self.find_role('User')
-
-        dag_role = [role for role in all_roles if role.name not in EXISTING_ROLES]
-        update_perm_views = []
-
-        # need to remove all_dag vm from all the existing view-menus
-        dag_vm = self.find_view_menu(permissions.RESOURCE_DAG)
-        ab_perm_view_role = sqla_models.assoc_permissionview_role
-        perm_view = self.permissionview_model
-        view_menu = self.viewmenu_model
-
-        all_perm_view_by_user = (
-            session.query(ab_perm_view_role)
-            .join(
-                perm_view,
-                perm_view.id == ab_perm_view_role.columns.permission_view_id,  # pylint: disable=no-member
-            )
-            .filter(ab_perm_view_role.columns.role_id == user_role.id)  # pylint: disable=no-member
-            .join(view_menu)
-            .filter(perm_view.view_menu_id != dag_vm.id)
-        )
-        all_perm_views = {role.permission_view_id for role in all_perm_view_by_user}
-
-        for role in dag_role:
-            # pylint: disable=no-member
-            # Get all the perm-view of the role
-
-            existing_perm_view_by_user = self.get_session.query(ab_perm_view_role).filter(
-                ab_perm_view_role.columns.role_id == role.id
-            )
-
-            existing_perms_views = {pv.permission_view_id for pv in existing_perm_view_by_user}
-            missing_perm_views = all_perm_views - existing_perms_views
-
-            for perm_view_id in missing_perm_views:
-                update_perm_views.append({'permission_view_id': perm_view_id, 'role_id': role.id})
-
-        if update_perm_views:
-            self.get_session.execute(
-                ab_perm_view_role.insert(), update_perm_views  # pylint: disable=no-value-for-parameter
-            )
-        self.get_session.commit()
+        for dag in dag_models:
+            for perm_name in self.DAG_PERMS:
+                dag_resource_name = self.prefixed_dag_id(dag.dag_id)
+                if dag_resource_name and perm_name and (dag_resource_name, perm_name) not in perms:
+                    self._merge_perm(perm_name, dag_resource_name)
 
     def update_admin_perm_view(self):
         """
@@ -560,13 +519,14 @@ def sync_roles(self):
         """
         # Create global all-dag VM
         self.create_perm_vm_for_all_dag()
+        self.create_dag_specific_permissions()
 
         # Create default user role.
         for config in self.ROLE_CONFIGS:
             role = config['role']
             perms = config['perms']
             self.init_role(role, perms)
-        self.create_custom_dag_permission_view()
+        self.add_homepage_access_to_custom_roles()
         # init existing roles, the rest role could be created through UI.
         self.update_admin_perm_view()
         self.clean_perms()

airflow/www/views.py

@@ -528,7 +528,7 @@ def index(self):
                 .all()
             )
 
-            user_permissions = current_app.appbuilder.sm.get_all_permissions_views()
+            user_permissions = current_app.appbuilder.sm.get_current_user_permissions()
             all_dags_editable = (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG) in user_permissions
 
             for dag in dags:

tests/www/test_security.py

@@ -226,11 +226,11 @@ def test_get_user_roles_for_anonymous_user(self):
             assert perms_views == viewer_role_perms
 
     @mock.patch('airflow.www.security.AirflowSecurityManager.get_user_roles')
-    def test_get_all_permissions_views(self, mock_get_user_roles):
+    def test_get_current_user_permissions(self, mock_get_user_roles):
         role_name = 'MyRole5'
         role_perm = 'can_some_action'
         role_vm = 'SomeBaseView'
-        username = 'get_all_permissions_views'
+        username = 'get_current_user_permissions'
 
         with self.app.app_context():
             user = fab_utils.create_user(
@@ -244,10 +244,10 @@ def test_get_all_permissions_views(self, mock_get_user_roles):
             role = user.roles[0]
             mock_get_user_roles.return_value = [role]
 
-            assert self.security_manager.get_all_permissions_views() == {(role_perm, role_vm)}
+            assert self.security_manager.get_current_user_permissions() == {(role_perm, role_vm)}
 
             mock_get_user_roles.return_value = []
-            assert len(self.security_manager.get_all_permissions_views()) == 0
+            assert len(self.security_manager.get_current_user_permissions()) == 0
 
     def test_get_accessible_dag_ids(self):
         role_name = 'MyRole1'

tests/www/test_views.py

@@ -1824,6 +1824,10 @@ def add_permission_for_role(self):
             permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG
         )
         self.appbuilder.sm.add_permission_role(all_dag_role, read_perm_on_all_dag)
+        read_perm_on_task_instance = self.appbuilder.sm.find_permission_view_menu(
+            permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE
+        )
+        self.appbuilder.sm.add_permission_role(all_dag_role, read_perm_on_task_instance)
         self.appbuilder.sm.add_permission_role(all_dag_role, website_permission)
 
         role_user = self.appbuilder.sm.find_role('User')