Copilot commented on code in PR #40669: URL: https://github.com/apache/superset/pull/40669#discussion_r3338874123
########## 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 __ + +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", +) Review Comment: Substring matching against the lowercased endpoint name is too permissive and will exempt unrelated endpoints from enforcement. For example: - `"auth"` matches any view whose name contains "auth" (e.g. OAuth-related endpoints, anything with "Author"/"authorized" in the name). - `"static"` matches any endpoint containing the substring (Flask blueprints often register `<blueprint>.static`, which is fine, but also any other endpoint coincidentally containing "static"). - `"health"` matches any endpoint name containing "health" anywhere. - `"userinfo"` is already covered by `"userinfoedit"`-style names, but combined with `"auth"` the list is broader than the documented intent. A safer approach is to match exact endpoint names (or known prefixes) rather than arbitrary substrings — e.g. an allow-list of `{"AuthDBView.login", "AuthDBView.logout", "AuthOAuthView.login", "ResetMyPasswordView.this_form_get", "ResetMyPasswordView.this_form_post", "ResetPasswordView.this_form_get", "ResetPasswordView.this_form_post", "UserInfoEditView.this_form_get", "UserInfoEditView.this_form_post", "appbuilder.static", "static"}` plus a check for endpoints ending in `.static`. This avoids accidentally exempting authenticated application endpoints that happen to share a substring. ########## 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 __ + +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() + ) + + +def password_change_required(user: Any) -> bool: + """Return True if ``user`` has a pending forced password change.""" + user_id = getattr(user, "id", None) + if not user_id: + return False + attr = _get_user_attribute(user_id) + return bool(attr and attr.password_must_change) + + +def set_password_must_change(user_id: int, value: bool = True) -> None: + """Set (or clear) the forced-password-change flag for a user. + + Intended to be called by administrative flows when provisioning an account + that should require a password change on first use. + """ + from superset.extensions import db + from superset.models.user_attributes import UserAttribute + + attr = _get_user_attribute(user_id) + if attr is None: + attr = UserAttribute(user_id=user_id) + db.session.add(attr) + attr.password_must_change = value + db.session.commit() + + +def clear_password_must_change(user_id: int) -> None: + """Clear the forced-password-change flag for a user, if set.""" + attr = _get_user_attribute(user_id) + if attr and attr.password_must_change: + from superset.extensions import db + + attr.password_must_change = False + db.session.commit() Review Comment: `set_password_must_change` and `clear_password_must_change` call `db.session.commit()` directly. This commits any other pending changes in the session (which may be unrelated work happening in the same request/command) and breaks the surrounding `@transaction()` pattern used elsewhere in Superset's command/DAO layers. These helpers should either `db.session.flush()` and let the caller manage the transaction, or be invoked only inside dedicated commands decorated with `@transaction()`. As written, calling `set_password_must_change` from inside an admin user-creation command would prematurely commit a half-built user record. ########## superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py: ########## @@ -0,0 +1,47 @@ +# 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. +"""add password_must_change to user_attribute + +Revision ID: b7c9d1e2f3a4 +Revises: 33d7e0e21daa +Create Date: 2026-06-01 00:00:00.000000 + +""" + +import sqlalchemy as sa + +from superset.migrations.shared.utils import add_columns, drop_columns + +# revision identifiers, used by Alembic. +revision = "b7c9d1e2f3a4" +down_revision = "33d7e0e21daa" Review Comment: `down_revision = "33d7e0e21daa"` points to a migration from 2025-11-04, but several newer migrations have been added since (e.g. `x2s8ocx6rto6`, `a9c01ec10479`, `9787190b3d89`, `f5b5f88d8526`, `a1b2c3d4e5f6`, and `ce6bd21901ab` from 2026-03-02). As a result, Alembic will see multiple heads and `alembic upgrade head` will fail (or this migration will silently branch the history). The `down_revision` should point to the actual current head (currently `ce6bd21901ab`). ########## superset/migrations/versions/2026-06-01_00-00_b7c9d1e2f3a4_add_password_must_change.py: ########## @@ -0,0 +1,47 @@ +# 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. +"""add password_must_change to user_attribute + +Revision ID: b7c9d1e2f3a4 +Revises: 33d7e0e21daa +Create Date: 2026-06-01 00:00:00.000000 + +""" + +import sqlalchemy as sa + +from superset.migrations.shared.utils import add_columns, drop_columns + +# revision identifiers, used by Alembic. +revision = "b7c9d1e2f3a4" +down_revision = "33d7e0e21daa" + + +def upgrade(): + add_columns( + "user_attribute", + sa.Column( + "password_must_change", + sa.Boolean(), + nullable=False, + server_default=sa.false(), + ), + ) Review Comment: Adding a non-nullable column on existing tables without a backfill step can fail on databases that materialize the new column immediately for existing rows (and is generally discouraged for tables that may be large). With `server_default=sa.false()` most backends will populate existing rows with `false`, but other migrations in this repository typically add the column as nullable, backfill, then alter to non-nullable (or drop the server default after backfill). Consider following that pattern, or at minimum keeping `server_default` permanently so future inserts via raw SQL don't fail. -- 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]
