From 6719f36b43496ff12d6468595283e3f03772998e Mon Sep 17 00:00:00 2001 From: dku4er Date: Sat, 24 Oct 2026 14:57:28 +0300 Subject: [PATCH] fix(auth): make an administrator-set password force a change at next login ENABLE_FORCE_PASSWORD_CHANGE redirects users flagged with password_must_change to their profile page until they choose a new password, but nothing in the tree ever raised that flag: set_password_must_change() has no caller, and setting on therefore enforced nothing. Raise the flag in SupersetUserApi, which backs the Users list: - pre_add: a user created by an administrator starts on a password the administrator chose. - pre_update: an administrator setting another user's password hands out a temporary one. An administrator changing their own password has chosen it, so a pending change of theirs is cleared instead, as on /api/v1/me/. Only while ENABLE_FORCE_PASSWORD_CHANGE is on, so enabling the setting later does not retroactively lock out every account an administrator ever set a password for, and only under AUTH_DB, where this is the password users sign in with. The flag is written without committing, through a new write_password_must_change() helper, so it rides the commit FAB issues right after pre_add/pre_update instead of committing a half-applied user edit early the way the @transaction-wrapped set_password_must_change() would. Co-Authored-By: AIST code-assistant https://virtuman.com --- UPDATING.md | 10 + superset/config.py | 8 +- superset/security/manager.py | 42 ++++- superset/security/password_change.py | 10 + .../test_admin_password_forces_change.py | 177 ++++++++++++++++++ 5 files changed, 240 insertions(+), 7 deletions(-) create mode 100644 tests/unit_tests/security/test_admin_password_forces_change.py diff --git a/UPDATING.md b/UPDATING.md index 782b938a02d7..3520458304fb 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -24,6 +24,16 @@ assists people when migrating to a new version. ## Next +- **Administrator-set passwords now force a change at next login.** With + `ENABLE_FORCE_PASSWORD_CHANGE` on, creating a user with a password + (`POST /api/v1/security/users/`) or setting another user's password from the + Users list edit modal (`PUT /api/v1/security/users/`) sets + `password_must_change` on that account, so its owner is redirected to choose + their own password at next login. Previously nothing set the flag, so the + setting had no effect. An administrator changing their own password is not + flagged (and a pending change of theirs is cleared). The flag is only raised + under `AUTH_DB`, and not at all with the setting off. + - MCP `update_dashboard` rejects blank or whitespace-only `dashboard_title` values with "Text must not be empty"; omit the field to keep the stored title. MCP chart names and theme names are capped at 250 characters, matching their diff --git a/superset/config.py b/superset/config.py index 8a10b09ee657..78c6691ea246 100644 --- a/superset/config.py +++ b/superset/config.py @@ -435,9 +435,11 @@ def _try_json_readsha(filepath: str, length: int) -> str | None: AUTH_RATE_LIMIT = "5 per second" # When enabled, users whose account is flagged with ``password_must_change`` -# (e.g. accounts provisioned by an administrator) are redirected to their -# profile page (/user_info/, "Reset my password") until they set a new -# password. Off by default. +# are redirected to their profile page (/user_info/, "Reset my password") until +# they set a new password. Under AUTH_DB the flag is raised whenever an +# administrator creates a user or sets another user's password from the Users +# list. +# Off by default. ENABLE_FORCE_PASSWORD_CHANGE = False # Password complexity policy, enforced (via Flask-AppBuilder) across diff --git a/superset/security/manager.py b/superset/security/manager.py index 1e30fc85de01..00e2ee128244 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -540,7 +540,8 @@ def pre_add(self, item: Model) -> None: here (to obtain its id) and syncing its ``Subject`` row alongside it means both writes ride the same transaction and commit together, instead of the subject sync needing a second, separate commit after - the fact. + the fact. The forced-password-change flag is raised the same way: the + new account starts on a password the administrator chose. """ super().pre_add(item) from superset.daos.user import UserDAO @@ -548,6 +549,7 @@ def pre_add(self, item: Model) -> None: self.datamodel.session.add(item) self.datamodel.session.flush() UserDAO._sync_subject(item) + self._require_password_change(item.id) def pre_update(self, item: Model, data: dict[str, Any]) -> None: """Same reasoning as ``pre_add``: ``UserApi.put`` calls ``pre_update`` @@ -563,14 +565,46 @@ def pre_update(self, item: Model, data: dict[str, Any]) -> None: # An admin-initiated password change via this endpoint (the # Users list edit modal) must invalidate the target account's # other outstanding sessions, the same as the self-service - # ``/me/`` path. The forced-change flag is deliberately left - # alone: a temporary password set by an admin still has to be - # replaced by the user at next login. + # ``/me/`` path. The password is a temporary one the admin chose, + # so the target also has to replace it at next login -- unless the + # admin is editing their own account. from superset.security.session_invalidation import ( invalidate_sessions_for_user, ) invalidate_sessions_for_user(item.id) + acting_user = getattr(self.appbuilder.sm, "current_user", None) + if getattr(acting_user, "id", None) != item.id: + self._require_password_change(item.id) + else: + # An admin replacing their own password has chosen it, the + # same as the self-service ``/me/`` path. + from superset.security.password_change import ( + clear_password_must_change, + ) + + clear_password_must_change(item.id) + + @staticmethod + def _require_password_change(user_id: int) -> None: + """Flag ``user_id`` to change their password at next login. + + Only when ``ENABLE_FORCE_PASSWORD_CHANGE`` is on, so turning the + setting on later does not retroactively lock out every account an + administrator ever set a password for, and only under ``AUTH_DB``: + with any other auth type the password set here is not the one users + sign in with. Writes without committing: FAB commits the user change + right after ``pre_add``/``pre_update``. + """ + config = current_app.config + if ( + not config.get("ENABLE_FORCE_PASSWORD_CHANGE") + or config.get("AUTH_TYPE", AUTH_DB) != AUTH_DB + ): + return + from superset.security.password_change import write_password_must_change + + write_password_must_change(user_id) @expose("/", methods=["POST"]) @protect() diff --git a/superset/security/password_change.py b/superset/security/password_change.py index 1ce149ce5216..4856e189891f 100644 --- a/superset/security/password_change.py +++ b/superset/security/password_change.py @@ -126,6 +126,16 @@ def set_password_must_change(user_id: int, value: bool = True) -> None: Intended to be called by administrative flows when provisioning an account that should require a password change on first use. """ + write_password_must_change(user_id, value) + + +def write_password_must_change(user_id: int, value: bool = True) -> None: + """Set (or clear) the forced-password-change flag without committing. + + For callers already inside their own unit of work, such as + ``SupersetUserApi.pre_update``, whose change has to ride the commit FAB + issues right after it rather than commit the half-applied user edit early. + """ from superset.extensions import db from superset.models.user_attributes import UserAttribute diff --git a/tests/unit_tests/security/test_admin_password_forces_change.py b/tests/unit_tests/security/test_admin_password_forces_change.py new file mode 100644 index 000000000000..0162b70efc12 --- /dev/null +++ b/tests/unit_tests/security/test_admin_password_forces_change.py @@ -0,0 +1,177 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""A password an administrator sets through ``SupersetUserApi`` is temporary. + +With ``ENABLE_FORCE_PASSWORD_CHANGE`` on, creating a user with a password +(``POST /api/v1/security/users/``) or setting another user's password from the +Users list edit modal (``PUT /api/v1/security/users/``) raises the +``password_must_change`` flag, so the account owner has to choose their own at +next login. Before this, nothing in the tree ever raised the flag, so turning +the setting on enforced nothing. +""" + +from __future__ import annotations + +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest +from flask import Flask +from flask_appbuilder.models.sqla.interface import SQLAInterface +from flask_appbuilder.security.manager import AUTH_DB, AUTH_OAUTH +from flask_appbuilder.security.sqla.models import User + +from superset import db, security_manager +from superset.security.manager import SupersetUserApi +from superset.security.password_change import password_change_required +from tests.unit_tests.fixtures.common import after_each # noqa: F401 + + +@pytest.fixture +def force_password_change(app: Flask, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setitem(app.config, "ENABLE_FORCE_PASSWORD_CHANGE", True) + monkeypatch.setitem(app.config, "AUTH_TYPE", AUTH_DB) + + +def _user(username: str, persisted: bool = True) -> User: + role = db.session.query(security_manager.role_model).filter_by(name="Admin").one() + user = User( + first_name="Target", + last_name="User", + email=f"{username}@example.org", + username=username, + roles=[role], + password="irrelevant-pre-hash-value", # noqa: S106 + ) + if persisted: + db.session.add(user) + db.session.flush() + return user + + +def _api(acting_user_id: int | None = None) -> SupersetUserApi: + """A ``SupersetUserApi`` acting as ``acting_user_id``, or as a separate + admin when omitted (FAB stamps the actor into ``changed_by_fk``, so it has + to be a real row).""" + if acting_user_id is None: + acting_user_id = _user("acting_admin").id + api = SupersetUserApi() + api.datamodel = SQLAInterface(User, db.session) + api.appbuilder = SimpleNamespace( + sm=SimpleNamespace(current_user=SimpleNamespace(id=acting_user_id)) + ) + return api + + +def test_admin_setting_a_password_requires_a_change( + after_each: None, # noqa: F811 + force_password_change: None, +) -> None: + user = _user("forced_change_admin_put") + + _api().pre_update(user, {"password": "AdminSetPassw0rd!"}) + + assert password_change_required(user) + + +def test_admin_creating_a_user_requires_a_change( + after_each: None, # noqa: F811 + force_password_change: None, +) -> None: + user = _user("forced_change_admin_post", persisted=False) + + _api().pre_add(user) + + assert password_change_required(user) + + +def test_admin_editing_their_own_password_does_not_require_a_change( + after_each: None, # noqa: F811 + force_password_change: None, +) -> None: + user = _user("forced_change_self_edit") + + _api(acting_user_id=user.id).pre_update(user, {"password": "MyOwnPassw0rd!"}) + + assert not password_change_required(user) + + +def test_admin_editing_their_own_password_clears_a_pending_change( + after_each: None, # noqa: F811 + force_password_change: None, +) -> None: + user = _user("forced_change_self_clear") + _api().pre_update(user, {"password": "AdminSetPassw0rd!"}) + assert password_change_required(user) + + _api(acting_user_id=user.id).pre_update(user, {"password": "MyOwnPassw0rd!"}) + + assert not password_change_required(user) + + +def test_flag_is_not_raised_for_non_db_auth( + app: Flask, + after_each: None, # noqa: F811 + force_password_change: None, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Under OAuth/LDAP/remote-user auth users never sign in with this + password, so making them change it would only lock them out.""" + monkeypatch.setitem(app.config, "AUTH_TYPE", AUTH_OAUTH) + user = _user("forced_change_oauth", persisted=False) + + _api().pre_add(user) + + assert not password_change_required(user) + + +def test_edit_without_a_password_does_not_require_a_change( + after_each: None, # noqa: F811 + force_password_change: None, +) -> None: + user = _user("forced_change_rename") + + _api().pre_update(user, {"first_name": "Renamed"}) + + assert not password_change_required(user) + + +def test_flag_is_not_raised_while_the_setting_is_off( + app: Flask, + after_each: None, # noqa: F811 + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setitem(app.config, "ENABLE_FORCE_PASSWORD_CHANGE", False) + user = _user("forced_change_setting_off") + + _api().pre_update(user, {"password": "AdminSetPassw0rd!"}) + + assert not password_change_required(user) + + +def test_flag_rides_fabs_commit_instead_of_committing_early( + after_each: None, # noqa: F811 + force_password_change: None, +) -> None: + user = _user("forced_change_no_early_commit") + + commit = MagicMock(wraps=db.session.commit) + with patch.object(db.session, "commit", commit): + _api().pre_update(user, {"password": "AdminSetPassw0rd!"}) + + assert commit.call_count == 0 + assert password_change_required(user)