rusackas commented on code in PR #40669: URL: https://github.com/apache/superset/pull/40669#discussion_r3384779231
########## 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: Good catch. After rebasing onto current master I repointed `down_revision` to the actual single alembic head, `31dae2559c05`. Verified there is now exactly one head (`b7c9d1e2f3a4`). ########## 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: Agreed — substring matching could fail open and exempt unrelated endpoints. Reworked `_is_exempt_endpoint` to match the view-class component exactly against an allow-list (the auth/reset/userinfo view classes) plus an explicit `.static` suffix check and exact `static`/`health` names, so e.g. an `AuthorView` or any name merely containing `health`/`static` is no longer exempted. Added regression tests covering those over-matching cases. ########## 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: Done — both helpers now use the `@transaction()` decorator instead of calling `db.session.commit()` directly, so they participate in the surrounding unit of work rather than prematurely committing half-built session state. ########## 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: The migration adds the column with `server_default=sa.false()` and `nullable=False`, which all supported backends materialize for existing rows at add-column time, and the server default is kept permanently (never dropped), so future raw-SQL inserts that omit the column are also safe — which is exactly the concern raised here. This matches the common pattern in the repo for adding a boolean flag with a default, so no separate backfill step is needed. -- 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]
