This is an automated email from the ASF dual-hosted git repository. jhtimmins pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/airflow.git
The following commit(s) were added to refs/heads/main by this push: new 6deebec Consolidate method names between Airflow Security Manager and FAB default (#18726) 6deebec is described below commit 6deebec04c71373f5f99a14a3477fc4d6dc9bcdc Author: James Timmins <ja...@astronomer.io> AuthorDate: Thu Oct 21 22:58:38 2021 -0700 Consolidate method names between Airflow Security Manager and FAB default (#18726) * Replace add_view_menu with create_resource. * Replace add_permission_view_menu with create_permission. * Replace find_permission_view_menu with get_permission. * Replace find_view_menu with get_resource. * Replace get_all_view_menu with get_all_resources. * Replace find_permission with get_action. * Replace del_permission_view_menu with delete_permission. * Replace del_view_menu with delete_resource. * Replace del_permission with delete_action. * Replace add_permission_role with add_permission_to_role. * Replace find_permissions_view_menu with get_resource_permissions. * Replace self.del_permission_role wth remove_permission_from_role. * Replace exist_permission_on_roles with permission_exists_in_one_or_more_roles. * Replace find_roles_permission_view_menus with filter_roles_by_perm_with_action. * Replace get_db_role_permissions with get_role_permissions_from_db. * Replace add_permission with create_action. * Remove unused exist_permission_on_view function. * Rename local variables. * Remove sqla model names from SecurityManager base class. * Update names in add_permissions_view. * Flake8. * Use updated perm names. * Use updated perm names for local vars. * Use black. * Reorder import statements. * Remove accidental renaming of BaseSecurityManager.roles. --- .../2c6edca13270_resource_based_permissions.py | 4 +- ...8c147f_remove_can_read_permission_on_config_.py | 4 +- ...ad25_resource_based_permissions_for_default_.py | 4 +- airflow/www/fab_security/manager.py | 314 ++++++++++++--------- airflow/www/fab_security/sqla/manager.py | 284 +++++++++++-------- airflow/www/security.py | 130 +-------- tests/test_utils/api_connexion_utils.py | 2 +- tests/www/test_security.py | 6 +- tests/www/views/test_views_acl.py | 30 +- 9 files changed, 370 insertions(+), 408 deletions(-) diff --git a/airflow/migrations/versions/2c6edca13270_resource_based_permissions.py b/airflow/migrations/versions/2c6edca13270_resource_based_permissions.py index fc792d3..81bf3b3 100644 --- a/airflow/migrations/versions/2c6edca13270_resource_based_permissions.py +++ b/airflow/migrations/versions/2c6edca13270_resource_based_permissions.py @@ -295,7 +295,9 @@ def remap_permissions(): for new_action_name, new_resource_name in new: new_permission = appbuilder.sm.create_permission(new_action_name, new_resource_name) for role in appbuilder.sm.get_all_roles(): - if appbuilder.sm.exist_permission_on_roles(old_resource_name, old_action_name, [role.id]): + if appbuilder.sm.permission_exists_in_one_or_more_roles( + old_resource_name, old_action_name, [role.id] + ): appbuilder.sm.add_permission_to_role(role, new_permission) appbuilder.sm.remove_permission_from_role(role, old_permission) appbuilder.sm.delete_permission(old_action_name, old_resource_name) diff --git a/airflow/migrations/versions/82b7c48c147f_remove_can_read_permission_on_config_.py b/airflow/migrations/versions/82b7c48c147f_remove_can_read_permission_on_config_.py index 3cb8f24..1926e4d 100644 --- a/airflow/migrations/versions/82b7c48c147f_remove_can_read_permission_on_config_.py +++ b/airflow/migrations/versions/82b7c48c147f_remove_can_read_permission_on_config_.py @@ -47,7 +47,7 @@ def upgrade(): ) for role in roles_to_modify: - if appbuilder.sm.exist_permission_on_roles( + if appbuilder.sm.permission_exists_in_one_or_more_roles( permissions.RESOURCE_CONFIG, permissions.ACTION_CAN_READ, [role.id] ): appbuilder.sm.remove_permission_from_role(role, can_read_on_config_perm) @@ -64,7 +64,7 @@ def downgrade(): ) for role in roles_to_modify: - if not appbuilder.sm.exist_permission_on_roles( + if not appbuilder.sm.permission_exists_in_one_or_more_roles( permissions.RESOURCE_CONFIG, permissions.ACTION_CAN_READ, [role.id] ): appbuilder.sm.add_permission_to_role(role, can_read_on_config_perm) diff --git a/airflow/migrations/versions/a13f7613ad25_resource_based_permissions_for_default_.py b/airflow/migrations/versions/a13f7613ad25_resource_based_permissions_for_default_.py index 294f59c..c7eee64 100644 --- a/airflow/migrations/versions/a13f7613ad25_resource_based_permissions_for_default_.py +++ b/airflow/migrations/versions/a13f7613ad25_resource_based_permissions_for_default_.py @@ -147,7 +147,9 @@ def remap_permissions(): for new_action_name, new_resource_name in new: new_permission = appbuilder.sm.create_permission(new_action_name, new_resource_name) for role in appbuilder.sm.get_all_roles(): - if appbuilder.sm.exist_permission_on_roles(old_resource_name, old_action_name, [role.id]): + if appbuilder.sm.permission_exists_in_one_or_more_roles( + old_resource_name, old_action_name, [role.id] + ): appbuilder.sm.add_permission_to_role(role, new_permission) appbuilder.sm.remove_permission_from_role(role, old_permission) appbuilder.sm.delete_permission(old_action_name, old_resource_name) diff --git a/airflow/www/fab_security/manager.py b/airflow/www/fab_security/manager.py index 32510a6..a83cd31 100644 --- a/airflow/www/fab_security/manager.py +++ b/airflow/www/fab_security/manager.py @@ -1266,23 +1266,21 @@ class BaseSecurityManager: else: return None - def is_item_public(self, permission_name, view_name): + def is_item_public(self, action_name, resource_name): """ Check if view has public permissions - :param permission_name: - the permission: can_show, can_edit... - :param view_name: - the name of the class view (child of BaseView) + :param action_name: + the action: can_show, can_edit... + :param resource_name: + the name of the resource """ - permissions = self.get_public_permissions() - if permissions: - for i in permissions: - if (view_name == i.view_menu.name) and (permission_name == i.permission.name): + perms = self.get_public_permissions() + if perms: + for perm in perms: + if (resource_name == perm.view_menu.name) and (action_name == perm.permission.name): return True - return False - else: - return False + return False def _has_access_builtin_roles(self, role, permission_name: str, view_name: str) -> bool: """Checks permission on builtin role""" @@ -1307,7 +1305,7 @@ class BaseSecurityManager: db_role_ids.append(role.id) # If it's not a builtin role check against database store roles - return self.exist_permission_on_roles(view_name, permission_name, db_role_ids) + return self.permission_exists_in_one_or_more_roles(view_name, permission_name, db_role_ids) def get_user_roles(self, user) -> List[object]: """Get current user roles, if user is not authenticated returns the public role""" @@ -1322,7 +1320,7 @@ class BaseSecurityManager: for permission in self.builtin_roles[role.name]: result.add((permission[1], permission[0])) else: - for permission in self.get_db_role_permissions(role.id): + for permission in self.get_role_permissions_from_db(role.id): result.add((permission.permission.name, permission.view_menu.name)) return result @@ -1335,7 +1333,7 @@ class BaseSecurityManager: return result def _get_user_permission_view_menus( - self, user: object, permission_name: str, view_menus_name: List[str] + self, user: object, action_name: str, resource_names: List[str] ) -> Set[str]: """ Return a set of view menu names with a certain permission name @@ -1353,14 +1351,14 @@ class BaseSecurityManager: result = set() for role in roles: if role.name in self.builtin_roles: - for view_menu_name in view_menus_name: - if self._has_access_builtin_roles(role, permission_name, view_menu_name): - result.add(view_menu_name) + for resource_name in resource_names: + if self._has_access_builtin_roles(role, action_name, resource_name): + result.add(resource_name) else: db_role_ids.append(role.id) # Then check against database-stored roles pvms_names = [ - pvm.view_menu.name for pvm in self.find_roles_permission_view_menus(permission_name, db_role_ids) + pvm.view_menu.name for pvm in self.filter_roles_by_perm_with_action(action_name, db_role_ids) ] result.update(pvms_names) return result @@ -1376,75 +1374,75 @@ class BaseSecurityManager: def get_user_menu_access(self, menu_names: List[str] = None) -> Set[str]: if current_user.is_authenticated: - return self._get_user_permission_view_menus(g.user, "menu_access", view_menus_name=menu_names) + return self._get_user_permission_view_menus(g.user, "menu_access", resource_names=menu_names) elif current_user_jwt: return self._get_user_permission_view_menus( - current_user_jwt, "menu_access", view_menus_name=menu_names + current_user_jwt, "menu_access", resource_names=menu_names ) else: - return self._get_user_permission_view_menus(None, "menu_access", view_menus_name=menu_names) + return self._get_user_permission_view_menus(None, "menu_access", resource_names=menu_names) - def add_permissions_view(self, base_permissions, view_menu): + def add_permissions_view(self, base_action_names, resource_name): """ Adds a permission on a view menu to the backend :param base_permissions: list of permissions from view (all exposed methods): 'can_add','can_edit' etc... - :param view_menu: - name of the view or menu to add + :param resource_name: + name of the resource to add """ - view_menu_db = self.add_view_menu(view_menu) - perm_views = self.find_permissions_view_menu(view_menu_db) + resource = self.create_resource(resource_name) + perms = self.get_resource_permissions(resource) - if not perm_views: + if not perms: # No permissions yet on this view - for permission in base_permissions: - pv = self.add_permission_view_menu(permission, view_menu) + for action_name in base_action_names: + action = self.create_permission(action_name, resource_name) if self.auth_role_admin not in self.builtin_roles: - role_admin = self.find_role(self.auth_role_admin) - self.add_permission_role(role_admin, pv) + admin_role = self.find_role(self.auth_role_admin) + self.add_permission_to_role(admin_role, action) else: # Permissions on this view exist but.... - role_admin = self.find_role(self.auth_role_admin) - for permission in base_permissions: + admin_role = self.find_role(self.auth_role_admin) + for action_name in base_action_names: # Check if base view permissions exist - if not self.exist_permission_on_views(perm_views, permission): - pv = self.add_permission_view_menu(permission, view_menu) + if not self.perms_include_action(perms, action_name): + action = self.create_permission(action_name, resource_name) if self.auth_role_admin not in self.builtin_roles: - self.add_permission_role(role_admin, pv) - for perm_view in perm_views: - if perm_view.permission is None: + self.add_permission_to_role(admin_role, action) + for perm in perms: + if perm.permission is None: # Skip this perm_view, it has a null permission continue - if perm_view.permission.name not in base_permissions: + if perm.permission.name not in base_action_names: # perm to delete roles = self.get_all_roles() - perm = self.find_permission(perm_view.permission.name) + action = self.get_action(perm.permission.name) # del permission from all roles for role in roles: - self.del_permission_role(role, perm) - self.del_permission_view_menu(perm_view.permission.name, view_menu) - elif ( - self.auth_role_admin not in self.builtin_roles and perm_view not in role_admin.permissions - ): + # TODO: An action can't be removed from a role. + # This is a bug in FAB. It has been reported. + self.remove_permission_from_role(role, action) + self.delete_permission(perm.permission.name, resource_name) + elif self.auth_role_admin not in self.builtin_roles and perm not in admin_role.permissions: # Role Admin must have all permissions - self.add_permission_role(role_admin, perm_view) + self.add_permission_to_role(admin_role, perm) - def add_permissions_menu(self, view_menu_name): + def add_permissions_menu(self, resource_name): """ Adds menu_access to menu on permission_view_menu - :param view_menu_name: - The menu name + :param resource_name: + The resource name """ - self.add_view_menu(view_menu_name) - pv = self.find_permission_view_menu("menu_access", view_menu_name) - if not pv: - pv = self.add_permission_view_menu("menu_access", view_menu_name) + self.create_resource(resource_name) + perm = self.get_permission("menu_access", resource_name) + if not perm: + perm = self.create_permission("menu_access", resource_name) if self.auth_role_admin not in self.builtin_roles: role_admin = self.find_role(self.auth_role_admin) - self.add_permission_role(role_admin, pv) + self.add_permission_to_role(role_admin, perm) def security_cleanup(self, baseviews, menus): """ @@ -1453,23 +1451,23 @@ class BaseSecurityManager: :param baseviews: A list of BaseViews class :param menus: Menu class """ - viewsmenus = self.get_all_view_menu() + resources = self.get_all_resources() roles = self.get_all_roles() - for viewmenu in viewsmenus: + for resource in resources: found = False for baseview in baseviews: - if viewmenu.name == baseview.class_permission_name: + if resource.name == baseview.class_permission_name: found = True break - if menus.find(viewmenu.name): + if menus.find(resource.name): found = True if not found: - permissions = self.find_permissions_view_menu(viewmenu) + permissions = self.get_resource_permissions(resource) for permission in permissions: for role in roles: - self.del_permission_role(role, permission) - self.del_permission_view_menu(permission.permission.name, viewmenu.name) - self.del_view_menu(viewmenu.name) + self.remove_permission_from_role(role, permission) + self.delete_permission(permission.permission.name, resource.name) + self.delete_resource(resource.name) self.security_converge(baseviews, menus) @staticmethod @@ -1492,19 +1490,19 @@ class BaseSecurityManager: @staticmethod def _add_state_transition( state_transition: Dict, - old_view_name: str, - old_perm_name: str, - view_name: str, - perm_name: str, + old_resource_name: str, + old_action_name: str, + resource_name: str, + action_name: str, ) -> None: - old_pvm = state_transition["add"].get((old_view_name, old_perm_name)) - if old_pvm: - state_transition["add"][(old_view_name, old_perm_name)].add((view_name, perm_name)) + old_perm = state_transition["add"].get((old_resource_name, old_action_name)) + if old_perm: + state_transition["add"][(old_resource_name, old_action_name)].add((resource_name, action_name)) else: - state_transition["add"][(old_view_name, old_perm_name)] = {(view_name, perm_name)} - state_transition["del_role_pvm"].add((old_view_name, old_perm_name)) - state_transition["del_views"].add(old_view_name) - state_transition["del_perms"].add(old_perm_name) + state_transition["add"][(old_resource_name, old_action_name)] = {(resource_name, action_name)} + state_transition["del_role_pvm"].add((old_resource_name, old_action_name)) + state_transition["del_views"].add(old_resource_name) + state_transition["del_perms"].add(old_action_name) @staticmethod def _update_del_transitions(state_transitions: Dict, baseviews: List) -> None: @@ -1600,22 +1598,22 @@ class BaseSecurityManager: log.debug(f"State transitions: {state_transitions}") roles = self.get_all_roles() for role in roles: - permissions = list(role.permissions) - for pvm in permissions: - new_pvm_states = state_transitions["add"].get((pvm.view_menu.name, pvm.permission.name)) - if not new_pvm_states: + perms = list(role.permissions) + for perm in perms: + new_perm_states = state_transitions["add"].get((perm.view_menu.name, perm.permission.name)) + if not new_perm_states: continue - for new_pvm_state in new_pvm_states: - new_pvm = self.add_permission_view_menu(new_pvm_state[1], new_pvm_state[0]) - self.add_permission_role(role, new_pvm) - if (pvm.view_menu.name, pvm.permission.name) in state_transitions["del_role_pvm"]: - self.del_permission_role(role, pvm) - for pvm in state_transitions["del_role_pvm"]: - self.del_permission_view_menu(pvm[1], pvm[0], cascade=False) - for view_name in state_transitions["del_views"]: - self.del_view_menu(view_name) - for permission_name in state_transitions["del_perms"]: - self.del_permission(permission_name) + for new_perm_state in new_perm_states: + new_perm = self.create_permission(new_perm_state[1], new_perm_state[0]) + self.add_permission_to_role(role, new_perm) + if (perm.view_menu.name, perm.permission.name) in state_transitions["del_role_pvm"]: + self.remove_permission_from_role(role, perm) + for perm in state_transitions["del_role_pvm"]: + self.delete_permission(perm[1], perm[0], cascade=False) + for resource_name in state_transitions["del_views"]: + self.delete_resource(resource_name) + for action_name in state_transitions["del_perms"]: + self.delete_action(action_name) return state_transitions def find_register_user(self, registration_hash): @@ -1642,7 +1640,7 @@ class BaseSecurityManager: """Generic function that returns all existing users""" raise NotImplementedError - def get_db_role_permissions(self, role_id: int) -> List[object]: + def get_role_permissions_from_db(self, role_id: int) -> List[object]: """Get all DB permissions from a role id""" raise NotImplementedError @@ -1682,18 +1680,27 @@ class BaseSecurityManager: """Returns all permissions from public role""" raise NotImplementedError - def find_permission(self, name): - """Finds and returns a Permission by name""" + def get_action(self, name: str): + """ + Gets an existing action record. + + :param name: name + :type name: str + :return: Action record, if it exists + :rtype: Permission + """ raise NotImplementedError - def find_roles_permission_view_menus(self, permission_name: str, role_ids: List[int]): + def filter_roles_by_perm_with_action(self, permission_name: str, role_ids: List[int]): raise NotImplementedError - def exist_permission_on_roles(self, view_name: str, permission_name: str, role_ids: List[int]) -> bool: + def permission_exists_in_one_or_more_roles( + self, resource_name: str, action_name: str, role_ids: List[int] + ) -> bool: """Finds and returns permission views for a group of roles""" raise NotImplementedError - def add_permission(self, name): + def create_action(self, name): """ Adds a permission to the backend, model permission @@ -1702,12 +1709,14 @@ class BaseSecurityManager: """ raise NotImplementedError - def del_permission(self, name): + def delete_action(self, name: str) -> bool: """ - Deletes a permission from the backend, model permission + Deletes a permission action. - :param name: - name of the permission: 'can_add','can_edit' etc... + :param name: Name of action to delete (e.g. can_read). + :type name: str + :return: Whether or not delete was successful. + :rtype: bool """ raise NotImplementedError @@ -1717,22 +1726,34 @@ class BaseSecurityManager: ---------------------- """ - def find_view_menu(self, name): - """Finds and returns a ViewMenu by name""" + def get_resource(self, name: str): + """ + Returns a resource record by name, if it exists. + + :param name: Name of resource + :type name: str + """ raise NotImplementedError - def get_all_view_menu(self): + def get_all_resources(self): + """ + Gets all existing resource records. + + :return: List of all resources + :rtype: List[ViewMenu] + """ raise NotImplementedError - def add_view_menu(self, name): + def create_resource(self, name): """ - Adds a view or menu to the backend, model view_menu - :param name: - name of the view menu to add + Create a resource with the given name. + + :param name: The name of the resource to create created. + :type name: str """ raise NotImplementedError - def del_view_menu(self, name): + def delete_resource(self, name): """ Deletes a ViewMenu from the backend @@ -1747,58 +1768,81 @@ class BaseSecurityManager: ---------------------- """ - def find_permission_view_menu(self, permission_name, view_menu_name): - """Finds and returns a PermissionView by names""" + def get_permission(self, action_name: str, resource_name: str): + """ + Gets a permission made with the given action->resource pair, if the permission already exists. + + :param action_name: Name of action + :type action_name: str + :param resource_name: Name of resource + :type resource_name: str + :return: The existing permission + :rtype: PermissionView + """ raise NotImplementedError - def find_permissions_view_menu(self, view_menu): + def get_resource_permissions(self, resource): """ - Finds all permissions from ViewMenu, returns list of PermissionView + Retrieve permission pairs associated with a specific resource object. - :param view_menu: ViewMenu object - :return: list of PermissionView objects + :param resource: Object representing a single resource. + :type resource: ViewMenu + :return: Permission objects representing resource->action pair + :rtype: PermissionView """ raise NotImplementedError - def add_permission_view_menu(self, permission_name, view_menu_name): + def create_permission(self, action_name: str, resource_name: str): """ - Adds a permission on a view or menu to the backend + Creates a permission linking an action and resource. - :param permission_name: - name of the permission to add: 'can_add','can_edit' etc... - :param view_menu_name: - name of the view menu to add + :param action_name: Name of existing action + :type action_name: str + :param resource_name: Name of existing resource + :type resource_name: str + :return: Resource created + :rtype: PermissionView """ raise NotImplementedError - def del_permission_view_menu(self, permission_name, view_menu_name, cascade=True): - raise NotImplementedError + def delete_permission(self, action_name: str, resource_name: str) -> None: + """ + Deletes the permission linking an action->resource pair. Doesn't delete the + underlying action or resource. - def exist_permission_on_views(self, lst, item): + :param action_name: Name of existing action + :type action_name: str + :param resource_name: Name of existing resource + :type resource_name: str + :return: None + :rtype: None + """ raise NotImplementedError - def exist_permission_on_view(self, lst, permission, view_menu): + def perms_include_action(self, perms, action_name): raise NotImplementedError - def add_permission_role(self, role, perm_view): + def add_permission_to_role(self, role, permission) -> None: """ - Add permission-ViewMenu object to Role + Add an existing permission pair to a role. - :param role: - The role object - :param perm_view: - The PermissionViewMenu object + :param role: The role about to get a new permission. + :type role + :param permission: The permission pair to add to a role. + :type permission: PermissionView + :return: None + :rtype: None """ raise NotImplementedError - def del_permission_role(self, role, perm_view): + def remove_permission_from_role(self, role, permission) -> None: """ - Remove permission-ViewMenu object to Role + Remove a permission pair from a role. - :param role: - The role object - :param perm_view: - The PermissionViewMenu object + :param role: User role containing permissions. + :type role + :param permission: Object representing resource-> action pair + :type permission: PermissionView """ raise NotImplementedError diff --git a/airflow/www/fab_security/sqla/manager.py b/airflow/www/fab_security/sqla/manager.py index 0f33a93..07399ca 100644 --- a/airflow/www/fab_security/sqla/manager.py +++ b/airflow/www/fab_security/sqla/manager.py @@ -278,17 +278,26 @@ class SecurityManager(BaseSecurityManager): return role.permissions return [] - def find_permission(self, name): - """Finds and returns a Permission by name""" + def get_action(self, name: str) -> Permission: + """ + Gets an existing action record. + + :param name: name + :type name: str + :return: Action record, if it exists + :rtype: Permission + """ return self.get_session.query(self.permission_model).filter_by(name=name).one_or_none() - def exist_permission_on_roles(self, view_name: str, permission_name: str, role_ids: List[int]) -> bool: + def permission_exists_in_one_or_more_roles( + self, resource_name: str, action_name: str, role_ids: List[int] + ) -> bool: """ Method to efficiently check if a certain permission exists on a list of role id's. This is used by `has_access` - :param view_name: The view's name to check if exists on one of the roles - :param permission_name: The permission name to check if exists + :param resource_name: The view's name to check if exists on one of the roles + :param action_name: The permission name to check if exists :param role_ids: a list of Role ids :return: Boolean """ @@ -302,8 +311,8 @@ class SecurityManager(BaseSecurityManager): .join(self.permission_model) .join(self.viewmenu_model) .filter( - self.viewmenu_model.name == view_name, - self.permission_model.name == permission_name, + self.viewmenu_model.name == resource_name, + self.permission_model.name == action_name, self.role_model.id.in_(role_ids), ) .exists() @@ -313,7 +322,7 @@ class SecurityManager(BaseSecurityManager): return self.appbuilder.get_session.query(literal(True)).filter(q).scalar() return self.appbuilder.get_session.query(q).scalar() - def find_roles_permission_view_menus(self, permission_name: str, role_ids: List[int]): + def filter_roles_by_perm_with_action(self, action_name: str, role_ids: List[int]): """Find roles with permission""" return ( self.appbuilder.get_session.query(self.permissionview_model) @@ -325,12 +334,12 @@ class SecurityManager(BaseSecurityManager): .join(self.permission_model) .join(self.viewmenu_model) .filter( - self.permission_model.name == permission_name, + self.permission_model.name == action_name, self.role_model.id.in_(role_ids), ) ).all() - def get_db_role_permissions(self, role_id: int) -> List[PermissionView]: + def get_role_permissions_from_db(self, role_id: int) -> List[PermissionView]: """Get all DB permissions from a role (one single query)""" return ( self.appbuilder.get_session.query(PermissionView) @@ -343,47 +352,49 @@ class SecurityManager(BaseSecurityManager): .all() ) - def add_permission(self, name): + def create_action(self, name): """ Adds a permission to the backend, model permission :param name: name of the permission: 'can_add','can_edit' etc... """ - perm = self.find_permission(name) - if perm is None: + action = self.get_action(name) + if action is None: try: - perm = self.permission_model() - perm.name = name - self.get_session.add(perm) + action = self.permission_model() + action.name = name + self.get_session.add(action) self.get_session.commit() - return perm + return action except Exception as e: log.error(c.LOGMSG_ERR_SEC_ADD_PERMISSION.format(str(e))) self.get_session.rollback() - return perm + return action - def del_permission(self, name: str) -> bool: + def delete_action(self, name: str) -> bool: """ - Deletes a permission from the backend, model permission + Deletes a permission action. - :param name: - name of the permission: 'can_add','can_edit' etc... + :param name: Name of action to delete (e.g. can_read). + :type name: str + :return: Whether or not delete was successful. + :rtype: bool """ - perm = self.find_permission(name) - if not perm: + action = self.get_action(name) + if not action: log.warning(c.LOGMSG_WAR_SEC_DEL_PERMISSION.format(name)) return False try: - pvms = ( + perms = ( self.get_session.query(self.permissionview_model) - .filter(self.permissionview_model.permission == perm) + .filter(self.permissionview_model.permission == action) .all() ) - if pvms: - log.warning(c.LOGMSG_WAR_SEC_DEL_PERM_PVM.format(perm, pvms)) + if perms: + log.warning(c.LOGMSG_WAR_SEC_DEL_PERM_PVM.format(action, perms)) return False - self.get_session.delete(perm) + self.get_session.delete(action) self.get_session.commit() return True except Exception as e: @@ -391,54 +402,69 @@ class SecurityManager(BaseSecurityManager): self.get_session.rollback() return False - def find_view_menu(self, name): - """Finds and returns a ViewMenu by name""" + def get_resource(self, name: str) -> ViewMenu: + """ + Returns a resource record by name, if it exists. + + :param name: Name of resource + :type name: str + :return: Resource record + :rtype: ViewMenu + """ return self.get_session.query(self.viewmenu_model).filter_by(name=name).one_or_none() - def get_all_view_menu(self): + def get_all_resources(self) -> List[ViewMenu]: + """ + Gets all existing resource records. + + :return: List of all resources + :rtype: List[ViewMenu] + """ return self.get_session.query(self.viewmenu_model).all() - def add_view_menu(self, name): + def create_resource(self, name) -> ViewMenu: """ - Adds a view or menu to the backend, model view_menu + Create a resource with the given name. - :param name: - name of the view menu to add + :param name: The name of the resource to create created. + :type name: str + :return: The FAB resource created. + :rtype: ViewMenu """ - view_menu = self.find_view_menu(name) - if view_menu is None: + resource = self.get_resource(name) + if resource is None: try: - view_menu = self.viewmenu_model() - view_menu.name = name - self.get_session.add(view_menu) + resource = self.viewmenu_model() + resource.name = name + self.get_session.add(resource) self.get_session.commit() - return view_menu + return resource except Exception as e: log.error(c.LOGMSG_ERR_SEC_ADD_VIEWMENU.format(str(e))) self.get_session.rollback() - return view_menu + return resource - def del_view_menu(self, name: str) -> bool: + def delete_resource(self, name: str) -> bool: """ Deletes a ViewMenu from the backend :param name: - name of the ViewMenu + name of the resource """ - view_menu = self.find_view_menu(name) - if not view_menu: + resource = self.get_resource(name) + if not resource: log.warning(c.LOGMSG_WAR_SEC_DEL_VIEWMENU.format(name)) return False try: - pvms = ( + perms = ( self.get_session.query(self.permissionview_model) - .filter(self.permissionview_model.view_menu == view_menu) + .filter(self.permissionview_model.view_menu == resource) .all() ) - if pvms: - log.warning(c.LOGMSG_WAR_SEC_DEL_VIEWMENU_PVM.format(view_menu, pvms)) + if perms: + log.warning(c.LOGMSG_WAR_SEC_DEL_VIEWMENU_PVM.format(resource, perms)) return False - self.get_session.delete(view_menu) + self.get_session.delete(resource) self.get_session.commit() return True except Exception as e: @@ -452,129 +478,145 @@ class SecurityManager(BaseSecurityManager): ---------------------- """ - def find_permission_view_menu(self, permission_name, view_menu_name): - """Finds and returns a PermissionView by names""" - permission = self.find_permission(permission_name) - view_menu = self.find_view_menu(view_menu_name) - if permission and view_menu: + def get_permission(self, action_name: str, resource_name: str) -> PermissionView: + """ + Gets a permission made with the given action->resource pair, if the permission already exists. + + :param action_name: Name of action + :type action_name: str + :param resource_name: Name of resource + :type resource_name: str + :return: The existing permission + :rtype: PermissionView + """ + action = self.get_action(action_name) + resource = self.get_resource(resource_name) + if action and resource: return ( self.get_session.query(self.permissionview_model) - .filter_by(permission=permission, view_menu=view_menu) + .filter_by(permission=action, view_menu=resource) .one_or_none() ) - def find_permissions_view_menu(self, view_menu): + def get_resource_permissions(self, resource: ViewMenu) -> PermissionView: """ - Finds all permissions from ViewMenu, returns list of PermissionView + Retrieve permission pairs associated with a specific resource object. - :param view_menu: ViewMenu object - :return: list of PermissionView objects + :param resource: Object representing a single resource. + :type resource: ViewMenu + :return: Permission objects representing resource->action pair + :rtype: PermissionView """ - return self.get_session.query(self.permissionview_model).filter_by(view_menu_id=view_menu.id).all() + return self.get_session.query(self.permissionview_model).filter_by(view_menu_id=resource.id).all() - def add_permission_view_menu(self, permission_name, view_menu_name): + def create_permission(self, action_name, resource_name): """ Adds a permission on a view or menu to the backend - :param permission_name: - name of the permission to add: 'can_add','can_edit' etc... - :param view_menu_name: - name of the view menu to add + :param action_name: + name of the action to add: 'can_add','can_edit' etc... + :param resource_name: + name of the resource to add """ - if not (permission_name and view_menu_name): + if not (action_name and resource_name): return None - pv = self.find_permission_view_menu(permission_name, view_menu_name) - if pv: - return pv - vm = self.add_view_menu(view_menu_name) - perm = self.add_permission(permission_name) - pv = self.permissionview_model() - pv.view_menu_id, pv.permission_id = vm.id, perm.id + perm = self.get_permission(action_name, resource_name) + if perm: + return perm + resource = self.create_resource(resource_name) + action = self.create_action(action_name) + perm = self.permissionview_model() + perm.view_menu_id, perm.permission_id = resource.id, action.id try: - self.get_session.add(pv) + self.get_session.add(perm) self.get_session.commit() - log.info(c.LOGMSG_INF_SEC_ADD_PERMVIEW.format(str(pv))) - return pv + log.info(c.LOGMSG_INF_SEC_ADD_PERMVIEW.format(str(perm))) + return perm except Exception as e: log.error(c.LOGMSG_ERR_SEC_ADD_PERMVIEW.format(str(e))) self.get_session.rollback() - def del_permission_view_menu(self, permission_name, view_menu_name, cascade=True): - if not (permission_name and view_menu_name): + def delete_permission(self, action_name: str, resource_name: str) -> None: + """ + Deletes the permission linking an action->resource pair. Doesn't delete the + underlying action or resource. + + :param action_name: Name of existing action + :type action_name: str + :param resource_name: Name of existing resource + :type resource_name: str + :return: None + :rtype: None + """ + if not (action_name and resource_name): return - pv = self.find_permission_view_menu(permission_name, view_menu_name) - if not pv: + perm = self.get_permission(action_name, resource_name) + if not perm: return - roles_pvs = ( - self.get_session.query(self.role_model).filter(self.role_model.permissions.contains(pv)).first() + roles = ( + self.get_session.query(self.role_model).filter(self.role_model.permissions.contains(perm)).first() ) - if roles_pvs: - log.warning(c.LOGMSG_WAR_SEC_DEL_PERMVIEW.format(view_menu_name, permission_name, roles_pvs)) + if roles: + log.warning(c.LOGMSG_WAR_SEC_DEL_PERMVIEW.format(resource_name, action_name, roles)) return try: # delete permission on view - self.get_session.delete(pv) + self.get_session.delete(perm) self.get_session.commit() # if no more permission on permission view, delete permission - if not cascade: - return if ( not self.get_session.query(self.permissionview_model) - .filter_by(permission=pv.permission) + .filter_by(permission=perm.permission) .all() ): - self.del_permission(pv.permission.name) - log.info(c.LOGMSG_INF_SEC_DEL_PERMVIEW.format(permission_name, view_menu_name)) + self.delete_action(perm.permission.name) + log.info(c.LOGMSG_INF_SEC_DEL_PERMVIEW.format(action_name, resource_name)) except Exception as e: log.error(c.LOGMSG_ERR_SEC_DEL_PERMVIEW.format(str(e))) self.get_session.rollback() - def exist_permission_on_views(self, lst, item): - for i in lst: - if i.permission and i.permission.name == item: - return True - return False - - def exist_permission_on_view(self, lst, permission, view_menu): - for i in lst: - if i.permission.name == permission and i.view_menu.name == view_menu: + def perms_include_action(self, perms, action_name): + for perm in perms: + if perm.permission and perm.permission.name == action_name: return True return False - def add_permission_role(self, role, perm_view): + def add_permission_to_role(self, role: Role, permission: PermissionView) -> None: """ - Add permission-ViewMenu object to Role - - :param role: - The role object - :param perm_view: - The PermissionViewMenu object + Add an existing permission pair to a role. + + :param role: The role about to get a new permission. + :type role: Role + :param permission: The permission pair to add to a role. + :type permission: PermissionView + :return: None + :rtype: None """ - if perm_view and perm_view not in role.permissions: + if permission and permission not in role.permissions: try: - role.permissions.append(perm_view) + role.permissions.append(permission) self.get_session.merge(role) self.get_session.commit() - log.info(c.LOGMSG_INF_SEC_ADD_PERMROLE.format(str(perm_view), role.name)) + log.info(c.LOGMSG_INF_SEC_ADD_PERMROLE.format(str(permission), role.name)) except Exception as e: log.error(c.LOGMSG_ERR_SEC_ADD_PERMROLE.format(str(e))) self.get_session.rollback() - def del_permission_role(self, role, perm_view): + def remove_permission_from_role(self, role: Role, permission: PermissionView) -> None: """ - Remove permission-ViewMenu object to Role + Remove a permission pair from a role. - :param role: - The role object - :param perm_view: - The PermissionViewMenu object + :param role: User role containing permissions. + :type role: Role + :param permission: Object representing resource-> action pair + :type permission: PermissionView """ - if perm_view in role.permissions: + if permission in role.permissions: try: - role.permissions.remove(perm_view) + role.permissions.remove(permission) self.get_session.merge(role) self.get_session.commit() - log.info(c.LOGMSG_INF_SEC_DEL_PERMROLE.format(str(perm_view), role.name)) + log.info(c.LOGMSG_INF_SEC_DEL_PERMROLE.format(str(permission), role.name)) except Exception as e: log.error(c.LOGMSG_ERR_SEC_DEL_PERMROLE.format(str(e))) self.get_session.rollback() diff --git a/airflow/www/security.py b/airflow/www/security.py index 61944a0..e4590fe 100644 --- a/airflow/www/security.py +++ b/airflow/www/security.py @@ -18,7 +18,7 @@ # import warnings -from typing import Dict, List, Optional, Sequence, Set, Tuple +from typing import Dict, Optional, Sequence, Set, Tuple from flask import current_app, g from flask_appbuilder.security.sqla import models as sqla_models @@ -239,77 +239,6 @@ class AirflowSecurityManager(SecurityManager, LoggingMixin): permission = self.create_permission(action_name, resource_name) self.add_permission_to_role(role, permission) - def get_resource(self, name: str) -> ViewMenu: - """ - Returns a resource record by name, if it exists. - - :param name: Name of resource - :type name: str - :return: Resource record - :rtype: ViewMenu - """ - return self.find_view_menu(name) - - def get_all_resources(self) -> List[ViewMenu]: - """ - Gets all existing resource records. - - :return: List of all resources - :rtype: List[ViewMenu] - """ - return self.get_all_view_menu() - - def get_action(self, name: str) -> Permission: - """ - Gets an existing action record. - - :param name: name - :type name: str - :return: Action record, if it exists - :rtype: Permission - """ - return self.find_permission(name) - - def get_permission(self, action_name: str, resource_name: str) -> PermissionView: - """ - Gets a permission made with the given action->resource pair, if the permission already exists. - - :param action_name: Name of action - :type action_name: str - :param resource_name: Name of resource - :type resource_name: str - :return: The existing permission - :rtype: PermissionView - """ - return self.find_permission_view_menu(action_name, resource_name) - - def create_permission(self, action_name: str, resource_name: str) -> PermissionView: - """ - Creates a permission linking an action and resource. - - :param action_name: Name of existing action - :type action_name: str - :param resource_name: Name of existing resource - :type resource_name: str - :return: Resource created - :rtype: PermissionView - """ - return self.add_permission_view_menu(action_name, resource_name) - - def delete_permission(self, action_name: str, resource_name: str) -> None: - """ - Deletes the permission linking an action->resource pair. Doesn't delete the - underlying action or resource. - - :param action_name: Name of existing action - :type action_name: str - :param resource_name: Name of existing resource - :type resource_name: str - :return: None - :rtype: None - """ - self.del_permission_view_menu(action_name, resource_name) - def delete_role(self, role_name): """ Delete the given Role @@ -604,41 +533,6 @@ class AirflowSecurityManager(SecurityManager, LoggingMixin): self.get_session.commit() - def add_permission_to_role(self, role: Role, permission: PermissionView) -> None: - """ - Add an existing permission pair to a role. - - :param role: The role about to get a new permission. - :type role: Role - :param permission: The permission pair to add to a role. - :type permission: PermissionView - :return: None - :rtype: None - """ - self.add_permission_role(role, permission) - - def remove_permission_from_role(self, role: Role, permission: PermissionView) -> None: - """ - Remove a permission pair from a role. - - :param role: User role containing permissions. - :type role: Role - :param permission: Object representing resource-> action pair - :type permission: PermissionView - """ - self.del_permission_role(role, permission) - - def delete_action(self, name: str) -> bool: - """ - Deletes a permission action. - - :param name: Name of action to delete (e.g. can_read). - :type name: str - :return: Whether or not delete was successful. - :rtype: bool - """ - return self.del_permission(name) - def get_all_permissions(self) -> Set[Tuple[str, str]]: """Returns all permissions as a set of tuples with the action and resource names""" return set( @@ -776,17 +670,6 @@ class AirflowSecurityManager(SecurityManager, LoggingMixin): if access_control: self._sync_dag_view_permissions(dag_resource_name, access_control) - def get_resource_permissions(self, resource: ViewMenu) -> PermissionView: - """ - Retrieve permission pairs associated with a specific resource object. - - :param resource: Object representing a single resource. - :type resource: ViewMenu - :return: Permission objects representing resource->action pair - :rtype: PermissionView - """ - return self.find_permissions_view_menu(resource) - def _sync_dag_view_permissions(self, dag_id, access_control): """ Set the access policy on the given DAG's ViewModel. @@ -847,17 +730,6 @@ class AirflowSecurityManager(SecurityManager, LoggingMixin): dag_perm = _get_or_create_dag_permission(action_name) self.add_permission_to_role(role, dag_perm) - def create_resource(self, name: str) -> ViewMenu: - """ - Create a resource with the given name. - - :param name: The name of the resource to create created. - :type name: str - :return: The FAB resource created. - :rtype: ViewMenu - """ - return self.add_view_menu(name) - def create_perm_vm_for_all_dag(self): """Create perm-vm if not exist and insert into FAB security model for all-dags.""" # create perm for global logical dag diff --git a/tests/test_utils/api_connexion_utils.py b/tests/test_utils/api_connexion_utils.py index be09913..bee38c3 100644 --- a/tests/test_utils/api_connexion_utils.py +++ b/tests/test_utils/api_connexion_utils.py @@ -76,7 +76,7 @@ def create_role(app, name, permissions=None): permissions = [] for permission in permissions: perm_object = appbuilder.sm.get_permission(*permission) - appbuilder.sm.add_permission_role(role, perm_object) + appbuilder.sm.add_permission_to_role(role, perm_object) return role diff --git a/tests/www/test_security.py b/tests/www/test_security.py index 9d1db20..33c01b2 100644 --- a/tests/www/test_security.py +++ b/tests/www/test_security.py @@ -197,7 +197,7 @@ class TestSecurity(unittest.TestCase): role = self.security_manager.find_role(role_name) perm = self.security_manager.get_permission(permissions.ACTION_CAN_EDIT, permissions.RESOURCE_ROLE) - self.security_manager.add_permission_role(role, perm) + self.security_manager.add_permission_to_role(role, perm) role_perms_len = len(role.permissions) self.security_manager.bulk_sync_roles(mock_roles) @@ -579,11 +579,11 @@ class TestSecurity(unittest.TestCase): assert len(roles_to_check) >= 5 for role in roles_to_check: if role.name in ["Admin", "Op"]: - assert self.security_manager.exist_permission_on_roles( + assert self.security_manager.permission_exists_in_one_or_more_roles( permissions.RESOURCE_CONFIG, permissions.ACTION_CAN_READ, [role.id] ) else: - assert not self.security_manager.exist_permission_on_roles( + assert not self.security_manager.permission_exists_in_one_or_more_roles( permissions.RESOURCE_CONFIG, permissions.ACTION_CAN_READ, [role.id] ), ( f"{role.name} should not have {permissions.ACTION_CAN_READ} " diff --git a/tests/www/views/test_views_acl.py b/tests/www/views/test_views_acl.py index 9402ee4..7ff7ac4 100644 --- a/tests/www/views/test_views_acl.py +++ b/tests/www/views/test_views_acl.py @@ -96,42 +96,42 @@ def acl_app(app): edit_perm_on_dag = security_manager.get_permission( permissions.ACTION_CAN_EDIT, 'DAG:example_bash_operator' ) - security_manager.add_permission_role(dag_tester_role, edit_perm_on_dag) + security_manager.add_permission_to_role(dag_tester_role, edit_perm_on_dag) read_perm_on_dag = security_manager.get_permission( permissions.ACTION_CAN_READ, 'DAG:example_bash_operator' ) - security_manager.add_permission_role(dag_tester_role, read_perm_on_dag) - security_manager.add_permission_role(dag_tester_role, website_permission) + security_manager.add_permission_to_role(dag_tester_role, read_perm_on_dag) + security_manager.add_permission_to_role(dag_tester_role, website_permission) all_dag_role = security_manager.find_role('all_dag_role') edit_perm_on_all_dag = security_manager.get_permission( permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG ) - security_manager.add_permission_role(all_dag_role, edit_perm_on_all_dag) + security_manager.add_permission_to_role(all_dag_role, edit_perm_on_all_dag) read_perm_on_all_dag = security_manager.get_permission( permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG ) - security_manager.add_permission_role(all_dag_role, read_perm_on_all_dag) + security_manager.add_permission_to_role(all_dag_role, read_perm_on_all_dag) read_perm_on_task_instance = security_manager.get_permission( permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE ) - security_manager.add_permission_role(all_dag_role, read_perm_on_task_instance) - security_manager.add_permission_role(all_dag_role, website_permission) + security_manager.add_permission_to_role(all_dag_role, read_perm_on_task_instance) + security_manager.add_permission_to_role(all_dag_role, website_permission) role_user = security_manager.find_role('User') - security_manager.add_permission_role(role_user, read_perm_on_all_dag) - security_manager.add_permission_role(role_user, edit_perm_on_all_dag) - security_manager.add_permission_role(role_user, website_permission) + security_manager.add_permission_to_role(role_user, read_perm_on_all_dag) + security_manager.add_permission_to_role(role_user, edit_perm_on_all_dag) + security_manager.add_permission_to_role(role_user, website_permission) read_only_perm_on_dag = security_manager.get_permission( permissions.ACTION_CAN_READ, 'DAG:example_bash_operator' ) dag_read_only_role = security_manager.find_role('dag_acl_read_only') - security_manager.add_permission_role(dag_read_only_role, read_only_perm_on_dag) - security_manager.add_permission_role(dag_read_only_role, website_permission) + security_manager.add_permission_to_role(dag_read_only_role, read_only_perm_on_dag) + security_manager.add_permission_to_role(dag_read_only_role, website_permission) dag_acl_faker_role = security_manager.find_role('dag_acl_faker') - security_manager.add_permission_role(dag_acl_faker_role, website_permission) + security_manager.add_permission_to_role(dag_acl_faker_role, website_permission) yield app @@ -200,8 +200,8 @@ def user_edit_one_dag(acl_app): @pytest.mark.usefixtures("user_edit_one_dag") def test_permission_exist(acl_app): - perms_views = acl_app.appbuilder.sm.find_permissions_view_menu( - acl_app.appbuilder.sm.find_view_menu('DAG:example_bash_operator'), + perms_views = acl_app.appbuilder.sm.get_resource_permissions( + acl_app.appbuilder.sm.get_resource('DAG:example_bash_operator'), ) assert len(perms_views) == 2