rusackas commented on code in PR #40669:
URL: https://github.com/apache/superset/pull/40669#discussion_r3384844980
##########
superset/security/manager.py:
##########
@@ -635,6 +635,17 @@ def create_login_manager(self, app: Flask) -> LoginManager:
lm.request_loader(self.request_loader)
return lm
+ def reset_password(self, userid: int, password: str) -> None:
+ """Reset a user's password and clear any pending forced-change flag.
+
+ Covers both the self-service reset and the admin "Reset Password"
action,
+ which both route through this method.
+ """
+ super().reset_password(userid, password)
+ from superset.security.password_change import
clear_password_must_change
+
+ clear_password_must_change(userid)
Review Comment:
Good catch — this was a real lifecycle bypass. `reset_password` is the
single chokepoint for both the self-service reset (`ResetMyPasswordView`, which
calls `reset_password(g.user.id, ...)`) and the admin reset
(`ResetPasswordView`, which calls `reset_password(pk, ...)` with the admin
still in `g.user`). I now clear the `password_must_change` flag only when the
acting user (`g.user`) is the same as the target `userid` — i.e. a self-service
reset. An admin reset leaves the flag set, so the target user is still forced
to change the temporary password at next login. Added a small `_same_user`
helper to compare ids robustly (the admin path passes `pk` as a string). Pinned
with two unit tests in `tests/unit_tests/security/manager_test.py`:
self-service clears the flag, admin-initiated does not.
##########
superset/security/password_change.py:
##########
@@ -0,0 +1,136 @@
+# 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.
+"""Force-password-change-on-first-use enforcement.
+
+A per-user ``password_must_change`` flag (on ``UserAttribute``) marks accounts
—
+typically created by an administrator — that must set a new password before
+they can use the rest of the application. When ``ENABLE_FORCE_PASSWORD_CHANGE``
+is enabled, a ``before_request`` hook redirects such users to the
password-reset
+page until they change it; the flag is cleared automatically on a successful
+password reset (see ``SupersetSecurityManager.reset_password``).
+"""
+
+from __future__ import annotations
+
+import logging
+from typing import Any, Optional
+
+from flask import current_app, flash, g, redirect, request, url_for
+from flask_babel import gettext as __
+
+from superset.utils.decorators import transaction
+
+logger = logging.getLogger(__name__)
+
+# Endpoint substrings that must remain reachable while a password change is
+# pending, otherwise the redirect would loop (login/logout, the password-reset
+# and user-info views, static assets, and the health check).
+_EXEMPT_ENDPOINT_TOKENS = (
+ "static",
+ "appbuilder",
+ "login",
+ "logout",
+ "resetmypassword",
+ "resetpassword",
+ "userinfoedit",
+ "userinfo",
+ "auth",
+ "health",
+)
+
+
+def _get_user_attribute(user_id: int) -> Optional[Any]:
+ # Imported lazily to avoid import cycles at app-init time.
+ from superset.extensions import db
+ from superset.models.user_attributes import UserAttribute
+
+ return (
+ db.session.query(UserAttribute)
+ .filter(UserAttribute.user_id == user_id)
+ .one_or_none()
+ )
Review Comment:
Valid — the `user_attribute` table has no uniqueness constraint on
`user_id`, so `.one_or_none()` could raise `MultipleResultsFound` and 500 a
request whenever a user ended up with duplicate rows. Switched
`_get_user_attribute` to fetch deterministically with
`.order_by(UserAttribute.id).first()`, which always returns a single, stable
row (or `None`) regardless of duplicates. Added a unit test
(`test_get_user_attribute_deterministic_with_duplicates`) asserting the query
orders by the primary key and uses `.first()` rather than `.one_or_none()`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
