This is an automated email from the ASF dual-hosted git repository.
vincbeck 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 930770211e5 Invalidate a user's sessions when their password is
changed through the API (#72657)
930770211e5 is described below
commit 930770211e5990b29ed86adc53b784372e52525c
Author: Jarek Potiuk <[email protected]>
AuthorDate: Wed Sep 9 15:19:05 2026 +0200
Invalidate a user's sessions when their password is changed through the API
(#72657)
Changing a password through PATCH /auth/fab/v1/users/{username} set the new
hash
and saved the user, without ending the sessions the old password had
established. A session captured before the change kept authenticating as
that
user, so an administrator changing a password during recovery or incident
response did not evict whoever held it. The old password was correctly
rejected,
which is what makes the surviving session easy to miss.
FAB already has a lifecycle-aware path: reset_password sets the hash, calls
reset_user_sessions, then saves. The user-management API bypassed that and
performed only the hash update.
Call reset_user_sessions when, and only when, the password actually
changed. A
password supplied in the body but excluded by update_mask is not applied and
invalidates nothing, and changes to other fields leave sessions alone.
Ordered *after* persistence, unlike reset_password. reset_user_sessions
commits
its deletions immediately, and security_manager.update_user rolls back and
returns False on failure -- a return value this service previously
discarded.
Invalidating first would have logged the user out even when the password
update
then failed, leaving the old password working, the user evicted, and the API
reporting success. That return value is now checked and surfaced.
Scope worth stating plainly: this ends FAB server-side sessions on the
database
session backend, which is where the issue was reported. reset_user_sessions
is a
no-op on other session backends, and no session mechanism here revokes
JWTs, so
a token issued before the change remains valid until it expires. Making a
password change invalidate outstanding tokens needs a per-user token
version or
an equivalent, which is a broader change than this one.
Against unpatched sources the new test fails with 'Expected
reset_user_sessions
to be called once. Called 0 times.'
---
.../fab/auth_manager/api_fastapi/services/users.py | 24 +++-
.../api_fastapi/services/test_users.py | 133 ++++++++++++++++++++-
2 files changed, 155 insertions(+), 2 deletions(-)
diff --git
a/providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/services/users.py
b/providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/services/users.py
index c84f1083404..e63577ba453 100644
---
a/providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/services/users.py
+++
b/providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/services/users.py
@@ -187,8 +187,10 @@ class FABAuthManagerUsers:
)
user.roles = roles_to_update
+ password_changed = False
if "password" in fields_to_update and body.password is not None:
user.password =
generate_password_hash(body.password.get_secret_value())
+ password_changed = True
if "username" in fields_to_update and body.username is not None:
user.username = body.username
@@ -199,7 +201,27 @@ class FABAuthManagerUsers:
if "last_name" in fields_to_update and body.last_name is not None:
user.last_name = body.last_name
- security_manager.update_user(user)
+ if not security_manager.update_user(user):
+ # `update_user` rolls back and returns False on failure. Ignoring
it would
+ # report success for a change that was not persisted.
+ raise HTTPException(
+ status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+ detail=f"Failed to update user `{username}`",
+ )
+
+ if password_changed:
+ # Changing a password has to end the sessions the old password
established,
+ # or a session captured beforehand keeps authenticating as this
user and the
+ # change does not evict whoever holds it. `reset_password` -- the
other
+ # supported way to change a password -- already does this; going
through the
+ # user-management API must not silently skip it.
+ #
+ # Deliberately ordered *after* persistence, unlike
`reset_password`.
+ # `reset_user_sessions` commits its deletions immediately, so
invalidating
+ # first would log the user out even when the password update then
fails,
+ # leaving the old password working and the user evicted for
nothing.
+ security_manager.reset_user_sessions(user)
+
return UserResponse.model_validate(user)
@classmethod
diff --git
a/providers/fab/tests/unit/fab/auth_manager/api_fastapi/services/test_users.py
b/providers/fab/tests/unit/fab/auth_manager/api_fastapi/services/test_users.py
index eec5054d2b1..15511a3c2b8 100644
---
a/providers/fab/tests/unit/fab/auth_manager/api_fastapi/services/test_users.py
+++
b/providers/fab/tests/unit/fab/auth_manager/api_fastapi/services/test_users.py
@@ -17,10 +17,11 @@
from __future__ import annotations
import types
-from unittest.mock import MagicMock, patch
+from unittest.mock import MagicMock, call, patch
import pytest
from fastapi import HTTPException
+from pydantic import SecretStr
from airflow.providers.fab.auth_manager.api_fastapi.datamodels.roles import
Role
from airflow.providers.fab.auth_manager.api_fastapi.services.users import
FABAuthManagerUsers
@@ -305,6 +306,136 @@ class TestUsersService:
assert out.last_name == "Updated"
security_manager.update_user.assert_called_once()
+ def test_update_user_password_invalidates_existing_sessions(
+ self, get_fab_auth_manager, fab_auth_manager, security_manager
+ ):
+ """Changing a password through the user API must end the old sessions.
+
+ A session captured before the change otherwise keeps authenticating as
this
+ user, so the password change does not evict whoever holds it.
`reset_password`
+ already invalidates; going through this API must not silently skip it.
+ """
+ user_obj = _make_user_obj(
+ username="alice",
+ email="[email protected]",
+ first_name="Alice",
+ last_name="Liddell",
+ roles=["User"],
+ )
+ security_manager.find_user.return_value = user_obj
+ fab_auth_manager.security_manager = security_manager
+ get_fab_auth_manager.return_value = fab_auth_manager
+
+ patch_body = types.SimpleNamespace(
+ username=None,
+ email=None,
+ first_name=None,
+ last_name=None,
+ roles=None,
+ password=SecretStr("new-password"),
+ )
+
+ FABAuthManagerUsers.update_user("alice", patch_body,
update_mask="password")
+
+ security_manager.reset_user_sessions.assert_called_once_with(user_obj)
+ # Ordered *after* persistence: invalidating first would evict the user
even when
+ # the password update then fails.
+ assert security_manager.mock_calls.index(
+ call.update_user(user_obj)
+ ) <
security_manager.mock_calls.index(call.reset_user_sessions(user_obj))
+
+ def test_update_user_without_password_does_not_touch_sessions(
+ self, get_fab_auth_manager, fab_auth_manager, security_manager
+ ):
+ """An unrelated field change must not log the user out."""
+ user_obj = _make_user_obj(
+ username="alice",
+ email="[email protected]",
+ first_name="Alice",
+ last_name="Liddell",
+ roles=["User"],
+ )
+ security_manager.find_user.return_value = user_obj
+ fab_auth_manager.security_manager = security_manager
+ get_fab_auth_manager.return_value = fab_auth_manager
+
+ patch_body = types.SimpleNamespace(
+ username=None,
+ email=None,
+ first_name=None,
+ last_name="Updated",
+ roles=None,
+ password=None,
+ )
+
+ FABAuthManagerUsers.update_user("alice", patch_body,
update_mask="last_name")
+
+ security_manager.reset_user_sessions.assert_not_called()
+
+ def test_update_user_password_outside_the_mask_does_not_invalidate(
+ self, get_fab_auth_manager, fab_auth_manager, security_manager
+ ):
+ """A password present in the body but excluded by the mask is not
applied."""
+ user_obj = _make_user_obj(
+ username="alice",
+ email="[email protected]",
+ first_name="Alice",
+ last_name="Liddell",
+ roles=["User"],
+ )
+ security_manager.find_user.return_value = user_obj
+ fab_auth_manager.security_manager = security_manager
+ get_fab_auth_manager.return_value = fab_auth_manager
+
+ patch_body = types.SimpleNamespace(
+ username=None,
+ email=None,
+ first_name=None,
+ last_name="Updated",
+ roles=None,
+ password=SecretStr("new-password"),
+ )
+
+ FABAuthManagerUsers.update_user("alice", patch_body,
update_mask="last_name")
+
+ security_manager.reset_user_sessions.assert_not_called()
+
+ def test_update_user_failed_persistence_does_not_evict_the_user(
+ self, get_fab_auth_manager, fab_auth_manager, security_manager
+ ):
+ """A password change that does not persist must not log the user out.
+
+ `update_user` rolls back and returns False on failure. Invalidating
first would
+ leave the old password working *and* the user evicted, with the API
reporting
+ success.
+ """
+ user_obj = _make_user_obj(
+ username="alice",
+ email="[email protected]",
+ first_name="Alice",
+ last_name="Liddell",
+ roles=["User"],
+ )
+ security_manager.find_user.return_value = user_obj
+ security_manager.update_user.return_value = False
+ fab_auth_manager.security_manager = security_manager
+ get_fab_auth_manager.return_value = fab_auth_manager
+
+ patch_body = types.SimpleNamespace(
+ username=None,
+ email=None,
+ first_name=None,
+ last_name=None,
+ roles=None,
+ password=SecretStr("new-password"),
+ )
+
+ with pytest.raises(HTTPException) as exc:
+ FABAuthManagerUsers.update_user("alice", patch_body,
update_mask="password")
+
+ assert exc.value.status_code == 500
+ security_manager.reset_user_sessions.assert_not_called()
+
def test_update_user_not_found(self, get_fab_auth_manager,
fab_auth_manager, security_manager):
security_manager.find_user.return_value = None
fab_auth_manager.security_manager = security_manager
