From c6a289b2e73b6df3ac67a10ed8b14cf6d50416db Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Wed, 24 Apr 2024 14:14:10 +0200 Subject: [PATCH] Fixed side effect of menu filtering causing disappearing menus The default implementation of filter_permitted_menu_items had a subtle side-effect. The filtering on child items was done in-place and modified the menu itself, so it was enough to get the same worker serve requests for multiple users for the same menu to get the items removed for subsequent user - even if they had permission to see it. Deepcopying the menu items before filtering them should fix the problem Fixes: #39204 Fixes: #39135 --- airflow/auth/managers/base_auth_manager.py | 14 +++++-- tests/auth/managers/test_base_auth_manager.py | 40 +++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/airflow/auth/managers/base_auth_manager.py b/airflow/auth/managers/base_auth_manager.py index 7bb4e92889e5c..44fc53a66e50f 100644 --- a/airflow/auth/managers/base_auth_manager.py +++ b/airflow/auth/managers/base_auth_manager.py @@ -21,6 +21,7 @@ from functools import cached_property from typing import TYPE_CHECKING, Container, Literal, Sequence +from flask_appbuilder.menu import MenuItem from sqlalchemy import select from airflow.auth.managers.models.resource_details import ( @@ -34,7 +35,6 @@ if TYPE_CHECKING: from flask import Blueprint - from flask_appbuilder.menu import MenuItem from sqlalchemy.orm import Session from airflow.auth.managers.models.base_user import BaseUser @@ -397,13 +397,21 @@ def filter_permitted_menu_items(self, menu_items: list[MenuItem]) -> list[MenuIt ) accessible_items = [] for menu_item in items: + menu_item_copy = MenuItem( + name=menu_item.name, + icon=menu_item.icon, + label=menu_item.label, + childs=[], + baseview=menu_item.baseview, + cond=menu_item.cond, + ) if menu_item.childs: accessible_children = [] for child in menu_item.childs: if self.security_manager.has_access(ACTION_CAN_ACCESS_MENU, child.name): accessible_children.append(child) - menu_item.childs = accessible_children - accessible_items.append(menu_item) + menu_item_copy.childs = accessible_children + accessible_items.append(menu_item_copy) return accessible_items @abstractmethod diff --git a/tests/auth/managers/test_base_auth_manager.py b/tests/auth/managers/test_base_auth_manager.py index 64d33f60659ad..a39b60787cead 100644 --- a/tests/auth/managers/test_base_auth_manager.py +++ b/tests/auth/managers/test_base_auth_manager.py @@ -313,3 +313,43 @@ def test_filter_permitted_menu_items(self, mock_security_manager, auth_manager): assert result[1].name == "item3" assert len(result[1].childs) == 1 assert result[1].childs[0].name == "item3.1" + + @patch.object(EmptyAuthManager, "security_manager") + def test_filter_permitted_menu_items_twice(self, mock_security_manager, auth_manager): + mock_security_manager.has_access.side_effect = [ + # 1st call + True, # menu 1 + False, # menu 2 + True, # menu 3 + True, # Item 3.1 + False, # Item 3.2 + # 2nd call + False, # menu 1 + True, # menu 2 + True, # menu 3 + False, # Item 3.1 + True, # Item 3.2 + ] + + menu = Menu() + menu.add_link("item1") + menu.add_link("item2") + menu.add_link("item3") + menu.add_link("item3.1", category="item3") + menu.add_link("item3.2", category="item3") + + result = auth_manager.filter_permitted_menu_items(menu.get_list()) + + assert len(result) == 2 + assert result[0].name == "item1" + assert result[1].name == "item3" + assert len(result[1].childs) == 1 + assert result[1].childs[0].name == "item3.1" + + result = auth_manager.filter_permitted_menu_items(menu.get_list()) + + assert len(result) == 2 + assert result[0].name == "item2" + assert result[1].name == "item3" + assert len(result[1].childs) == 1 + assert result[1].childs[0].name == "item3.2"