Copilot commented on code in PR #64539:
URL: https://github.com/apache/airflow/pull/64539#discussion_r3025327192
##########
providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py:
##########
@@ -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)
Review Comment:
`self.session.refresh(user)` issues an extra SELECT on every successful LDAP
authentication (including per-request basic auth), which can add noticeable DB
overhead. Since the goal is to ensure subsequent permission checks see the
updated roles, consider avoiding an eager refresh (e.g., rely on the
post-commit state, or use `session.expire(user, ["roles", "groups"])` / re-load
the user only when needed) so you don’t add an unconditional query to this hot
path.
```suggestion
```
##########
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)
Review Comment:
This test introduces several `Mock(...)` instances without `spec`/`autospec`
(user, roles, groups, existing_user, merged_user). Unspec’d mocks can hide real
attribute/typing issues and are discouraged in this codebase; please add
appropriate `spec` (e.g., `User`, `Role`, `Group`) or use `autospec=True` where
applicable.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
