From 544ce838918199d1cf30726194b5b14c095569f0 Mon Sep 17 00:00:00 2001 From: Pranay Kumar Karvi Date: Tue, 31 Mar 2026 19:53:46 +0530 Subject: [PATCH 1/4] fix: invalidate cached user permissions after LDAP role sync to prevent intermittent 403s --- .../auth_manager/security_manager/override.py | 13 +++++++++++- .../security_manager/test_override.py | 21 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py b/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py index 0cfc2351874a0..ca56d9ac19b37 100644 --- a/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py +++ b/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py @@ -1495,8 +1495,11 @@ def update_user(self, user: User) -> bool: new_group_ids = {grp.id for grp in user.groups} if existing_role_ids != new_role_ids or existing_group_ids != new_group_ids: user.changed_on = datetime.datetime.now(tz=datetime.timezone.utc) - self.session.merge(user) + merged_user = self.session.merge(user) self.session.commit() + self._reset_user_permissions_cache(user) + if merged_user is not user: + self._reset_user_permissions_cache(merged_user) log.info(const.LOGMSG_INF_SEC_UPD_USER, user) except Exception as e: log.error(const.LOGMSG_ERR_SEC_UPD_USER, e) @@ -1504,6 +1507,11 @@ def update_user(self, user: User) -> bool: return False return True + @staticmethod + def _reset_user_permissions_cache(user: User) -> None: + """Invalidate cached permissions to avoid stale auth checks after role updates.""" + user._perms = None + def del_register_user(self, register_user) -> bool: """ Delete registration object from database. @@ -1986,6 +1994,7 @@ def auth_user_ldap(self, username, password, rotate_session_id=True) -> User | N # Sync the user's roles if user and user_attributes and self.auth_roles_sync_at_login: user.roles = self._ldap_calculate_user_roles(user_attributes) + self._reset_user_permissions_cache(user) log.debug("Calculated new roles for user=%r as: %s", user_dn, user.roles) # If the user is new, register them @@ -2013,6 +2022,8 @@ def auth_user_ldap(self, username, password, rotate_session_id=True) -> User | N if rotate_session_id: self._rotate_session_id() self.update_user_auth_stat(user) + self.session.refresh(user) + self._reset_user_permissions_cache(user) return user return None diff --git a/providers/fab/tests/unit/fab/auth_manager/security_manager/test_override.py b/providers/fab/tests/unit/fab/auth_manager/security_manager/test_override.py index a2186c82973ba..67d31cb89338e 100644 --- a/providers/fab/tests/unit/fab/auth_manager/security_manager/test_override.py +++ b/providers/fab/tests/unit/fab/auth_manager/security_manager/test_override.py @@ -193,6 +193,27 @@ def test_check_password_not_match(self, check_password): check_password.return_value = False assert not sm.check_password("test_user", "test_password") + def test_update_user_clears_cached_permissions(self): + sm = EmptySecurityManager() + user = Mock( + id=1, + roles=[Mock(id=2)], + groups=[Mock(id=3)], + _perms={("can_read", "DAG")}, + ) + existing_user = Mock(roles=[Mock(id=4)], groups=[Mock(id=5)]) + merged_user = Mock(_perms={("can_edit", "DAG")}) + mock_session = Mock(spec=Session) + mock_session.get.return_value = existing_user + mock_session.merge.return_value = merged_user + + with mock.patch.object(EmptySecurityManager, "session", mock_session): + assert sm.update_user(user) + + assert user._perms is None + assert merged_user._perms is None + mock_session.commit.assert_called_once_with() + @pytest.mark.parametrize( ("provider", "resp", "user_info"), [ From 57481c89d260d6cdbd055247a8bf75b444791dd8 Mon Sep 17 00:00:00 2001 From: Pranay Kumar Karvi Date: Thu, 2 Apr 2026 18:33:28 +0530 Subject: [PATCH 2/4] fix: address review comments - use session.expire and add mock specs --- .../fab/auth_manager/security_manager/override.py | 2 +- .../auth_manager/security_manager/test_override.py | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py b/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py index ca56d9ac19b37..e1cd66550dba6 100644 --- a/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py +++ b/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py @@ -2022,7 +2022,7 @@ def auth_user_ldap(self, username, password, rotate_session_id=True) -> User | N if rotate_session_id: self._rotate_session_id() self.update_user_auth_stat(user) - self.session.refresh(user) + self.session.expire(user, ["roles", "groups"]) self._reset_user_permissions_cache(user) return user return None diff --git a/providers/fab/tests/unit/fab/auth_manager/security_manager/test_override.py b/providers/fab/tests/unit/fab/auth_manager/security_manager/test_override.py index 67d31cb89338e..15fbd881ebf1d 100644 --- a/providers/fab/tests/unit/fab/auth_manager/security_manager/test_override.py +++ b/providers/fab/tests/unit/fab/auth_manager/security_manager/test_override.py @@ -26,9 +26,11 @@ from airflow.providers.fab.auth_manager.models import ( Action, + Group, Permission, Resource, Role, + User, ) from airflow.providers.fab.auth_manager.security_manager.override import FabAirflowSecurityManagerOverride @@ -196,13 +198,14 @@ def test_check_password_not_match(self, check_password): def test_update_user_clears_cached_permissions(self): sm = EmptySecurityManager() user = Mock( + spec=User, id=1, - roles=[Mock(id=2)], - groups=[Mock(id=3)], + roles=[Mock(spec=Role, id=2)], + groups=[Mock(spec=Group, id=3)], _perms={("can_read", "DAG")}, ) - existing_user = Mock(roles=[Mock(id=4)], groups=[Mock(id=5)]) - merged_user = Mock(_perms={("can_edit", "DAG")}) + existing_user = Mock(spec=User, roles=[Mock(spec=Role, id=4)], groups=[Mock(spec=Group, id=5)]) + merged_user = Mock(spec=User, _perms={("can_edit", "DAG")}) mock_session = Mock(spec=Session) mock_session.get.return_value = existing_user mock_session.merge.return_value = merged_user From a7268d3c741123465be79368662986d2dc6d4736 Mon Sep 17 00:00:00 2001 From: Pranay Kumar Karvi Date: Tue, 14 Apr 2026 14:07:25 +0530 Subject: [PATCH 3/4] fix: simplify merged_user cache reset per review feedback --- .../providers/fab/auth_manager/security_manager/override.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py b/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py index e1cd66550dba6..bd544cd0937c0 100644 --- a/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py +++ b/providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py @@ -1497,9 +1497,7 @@ def update_user(self, user: User) -> bool: user.changed_on = datetime.datetime.now(tz=datetime.timezone.utc) merged_user = self.session.merge(user) self.session.commit() - self._reset_user_permissions_cache(user) - if merged_user is not user: - self._reset_user_permissions_cache(merged_user) + self._reset_user_permissions_cache(merged_user) log.info(const.LOGMSG_INF_SEC_UPD_USER, user) except Exception as e: log.error(const.LOGMSG_ERR_SEC_UPD_USER, e) From 1ef6d7bb5a1faaf328fd6c4de2271c8f6c4ac43d Mon Sep 17 00:00:00 2001 From: Pranay Kumar Karvi Date: Tue, 14 Apr 2026 21:27:11 +0530 Subject: [PATCH 4/4] fix: update test to assert on merged_user after cache reset simplification --- .../fab/auth_manager/security_manager/test_override.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/providers/fab/tests/unit/fab/auth_manager/security_manager/test_override.py b/providers/fab/tests/unit/fab/auth_manager/security_manager/test_override.py index 15fbd881ebf1d..409bd4bb700c9 100644 --- a/providers/fab/tests/unit/fab/auth_manager/security_manager/test_override.py +++ b/providers/fab/tests/unit/fab/auth_manager/security_manager/test_override.py @@ -205,16 +205,16 @@ def test_update_user_clears_cached_permissions(self): _perms={("can_read", "DAG")}, ) existing_user = Mock(spec=User, roles=[Mock(spec=Role, id=4)], groups=[Mock(spec=Group, id=5)]) - merged_user = Mock(spec=User, _perms={("can_edit", "DAG")}) + mock_merged_user = Mock(spec=User, _perms={("can_edit", "DAG")}) mock_session = Mock(spec=Session) mock_session.get.return_value = existing_user - mock_session.merge.return_value = merged_user + mock_session.merge.return_value = mock_merged_user with mock.patch.object(EmptySecurityManager, "session", mock_session): assert sm.update_user(user) - assert user._perms is None - assert merged_user._perms is None + assert user._perms == {("can_read", "DAG")} + assert mock_merged_user._perms is None mock_session.commit.assert_called_once_with() @pytest.mark.parametrize(