fix(auth): make an administrator-set password force a change at next login

UPDATING.md

@@ -2767,6 +2767,16 @@ Charts and dashboards using these anchors will compute a different (correct) tim
   list that explicitly contains NULL still excludes NULL rows. `!=`, `NOT LIKE`
   and `NOT ILIKE` are unchanged.
 
+- **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/<pk>`) 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.
+
 ## 6.1.0
 
 ### ClickHouse minimum driver version bump

superset/config.py

@@ -446,9 +446,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

superset/security/manager.py

@@ -590,14 +590,16 @@ 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
 
         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``
@@ -613,14 +615,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()

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
 

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/<pk>``) 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)