bito-code-review[bot] commented on code in PR #39469:
URL: https://github.com/apache/superset/pull/39469#discussion_r3423870415
##########
superset/utils/machine_auth.py:
##########
@@ -121,9 +121,12 @@ def get_cookies(self, user: User | None) -> dict[str, str]:
@staticmethod
def get_auth_cookies(user: User) -> dict[str, str]:
+ from superset.utils.auth_session_stamp import
sync_session_auth_stamp_on_login
+
# Login with the user specified to get the reports
with app.test_request_context("/login"):
login_user(user)
+ sync_session_auth_stamp_on_login(user)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate login hook call</b></div>
<div id="fix">
The call to `sync_session_auth_stamp_on_login` at line 129 duplicates logic
already present in `security_manager.on_user_login` (line 935 in
`security/manager.py`). However, `on_user_login` is dead code — the
`user_logged_in` signal is never connected. This diff adds a workaround rather
than fixing the root cause. The proper solution is to connect the signal or
call `sm.on_user_login(user)` directly after `login_user`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/utils/machine_auth.py (lines 121-132) ---
122: @staticmethod
123: def get_auth_cookies(user: User) -> dict[str, str]:
- from superset.utils.auth_session_stamp import
sync_session_auth_stamp_on_login
-
124: # Login with the user specified to get the reports
125: with app.test_request_context("/login"):
126: login_user(user)
- sync_session_auth_stamp_on_login(user)
+ # Call the security manager's on_user_login hook to ensure
+ # consistent login behavior (audit logging, session stamping,
etc.)
+ from superset.extensions import security_manager
+ security_manager.on_user_login(user)
127: # A mock response object to get the cookie information from
128: response = Response()
```
</div>
</details>
</div>
<small><i>Code Review Run #3957e7</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/utils/auth_db_password.py:
##########
@@ -0,0 +1,199 @@
+# 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.
+"""Password policy helpers for AUTH_DB (database authentication)."""
+
+from __future__ import annotations
+
+import re
+from typing import Any
+
+from flask import current_app as app
+from marshmallow import ValidationError
+
+# Defaults are merged with ``AUTH_DB_CONFIG`` from Flask config (partial
overrides).
+AUTH_DB_DEFAULTS: dict[str, Any] = {
+ "password_min_length": 12,
+ "password_require_uppercase": True,
+ "password_require_lowercase": True,
+ "password_require_digit": True,
+ "password_require_special": True,
+ "password_common_list_check": True,
+ "password_hash_algorithm": "scrypt",
+ "reset_token_expiry_minutes": 30,
+ "reset_rate_limit": "5 per 15 minutes",
+ "login_rate_limit": "10 per 5 minutes",
+ "login_max_failures": 5,
+ "login_lockout_duration_minutes": 15,
+}
+
+_PUBLIC_PASSWORD_POLICY_KEYS = (
+ "password_min_length",
+ "password_require_uppercase",
+ "password_require_lowercase",
+ "password_require_digit",
+ "password_require_special",
+ "password_common_list_check",
+)
+
+_SUPPORTED_HASH_ALGORITHMS: dict[str, str] = {
+ # TODO: native bcrypt/argon2 support is deferred to a follow-up SIP; see
SIP-201.
+ # Keep legacy keys for backward compatibility; Werkzeug does not support
+ # bcrypt/argon2 directly in generate_password_hash.
+ "bcrypt": "scrypt",
+ "argon2": "scrypt",
+ "scrypt": "scrypt",
+ "pbkdf2": "pbkdf2:sha256",
+ "pbkdf2:sha256": "pbkdf2:sha256",
+}
+
+_COMMON_PASSWORDS = frozenset(
+ password.lower()
+ for password in {
+ "password",
+ "password1",
+ "password123",
+ "123456",
+ "12345678",
+ "123456789",
+ "qwerty",
+ "abc123",
+ "monkey",
+ "letmein",
+ "trustno1",
+ "dragon",
+ "baseball",
+ "iloveyou",
+ "master",
+ "sunshine",
+ "ashley",
+ "bailey",
+ "shadow",
+ "superman",
+ "qazwsx",
+ "michael",
+ "football",
+ "welcome",
+ "jesus",
+ "ninja",
+ "mustang",
+ "password1!",
+ "admin",
+ "admin123",
+ "root",
+ "toor",
+ "guest",
+ "p@ssw0rd",
+ "Passw0rd",
+ "Password1",
+ "Password123",
+ "Welcome1",
+ "Qwerty123",
+ }
+)
+
+
+def get_merged_auth_db_config() -> dict[str, Any]:
+ """Return ``AUTH_DB_DEFAULTS`` merged with ``AUTH_DB_CONFIG`` from app
config."""
+ overrides = app.config.get("AUTH_DB_CONFIG") or {}
+ return {**AUTH_DB_DEFAULTS, **overrides}
+
+
+def get_auth_db_login_rate_limit_string() -> str:
+ """
+ Return the configured AUTH_DB ``login_rate_limit`` string for
Flask-Limiter.
+
+ Used for endpoints that verify a password (for example ``PUT
/api/v1/me/password``).
+ """
+ return str(
+ get_merged_auth_db_config().get(
+ "login_rate_limit", AUTH_DB_DEFAULTS["login_rate_limit"]
+ )
+ )
+
+
+def get_public_auth_db_password_policy(cfg: dict[str, Any] | None = None) ->
dict[str, Any]:
+ """
+ Return non-secret AUTH_DB password policy options for frontend validation
UI.
+ """
+ merged_cfg = cfg if cfg is not None else get_merged_auth_db_config()
+ return {key: merged_cfg[key] for key in _PUBLIC_PASSWORD_POLICY_KEYS}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing default fallback for KeyError</b></div>
<div id="fix">
If `AUTH_DB_CONFIG` lacks one of the public policy keys, this function
raises `KeyError`. Other functions in this module (e.g.,
`get_auth_db_password_hash_method`, `validate_auth_db_password`) use `.get(key,
AUTH_DB_DEFAULTS[key])` to safely fall back to defaults. Apply the same pattern
here.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset/utils/auth_db_password.py
+++ b/superset/utils/auth_db_password.py
@@ -128,7 +128,7 @@ def get_public_password_policy() -> dict[str, Any]:
cfg = get_merged_auth_db_config()
merged_cfg = cfg if cfg is not None else get_merged_auth_db_config()
- return {key: merged_cfg[key] for key in _PUBLIC_PASSWORD_POLICY_KEYS}
+ return {key: merged_cfg.get(key, AUTH_DB_DEFAULTS.get(key)) for key in
_PUBLIC_PASSWORD_POLICY_KEYS}
```
</div>
</details>
</div>
<small><i>Code Review Run #3957e7</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/features/users/UserListResetPasswordModal.tsx:
##########
@@ -0,0 +1,219 @@
+/**
+ * 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.
+ */
+import { t } from '@apache-superset/core/translation';
+import { getClientErrorObject, SupersetClient } from '@superset-ui/core';
+import { ModalTitleWithIcon } from 'src/components/ModalTitleWithIcon';
+import { useToasts } from 'src/components/MessageToasts/withToasts';
+import {
+ FormModal,
+ FormItem,
+ Input,
+ Icons,
+ type FormInstance,
+} from '@superset-ui/core/components';
+import { GeneratePasswordInputSuffix } from
'src/components/GeneratePasswordInputSuffix';
+import AuthDbPasswordPolicyIndicator from
'src/components/AuthDbPasswordPolicyIndicator';
+import {
+ AUTH_DB_PASSWORD_MIN_LENGTH,
+ generateAuthDbPassword,
+ getAuthDbPasswordPolicyChecks,
+} from 'src/utils/generateAuthDbPassword';
+import { UserObject } from 'src/pages/UsersList/types';
+import { buildSecurityUserUpdatePayload } from './utils';
+
+export interface UserListResetPasswordModalProps {
+ show: boolean;
+ onHide: () => void;
+ onSave: () => void;
+ user: UserObject | null;
+}
+
+function UserListResetPasswordModal({
+ show,
+ onHide,
+ onSave,
+ user,
+}: UserListResetPasswordModalProps) {
+ const { addDangerToast, addSuccessToast } = useToasts();
+ const getPasswordPolicyError = (password: string): string | null => {
+ const checks = getAuthDbPasswordPolicyChecks(password);
+ if (!checks.minLength) {
+ return t(
+ 'Password must be at least %s characters long.',
+ AUTH_DB_PASSWORD_MIN_LENGTH,
+ );
+ }
+ if (!checks.uppercase) {
+ return t('Password must contain at least one uppercase letter.');
+ }
+ if (!checks.lowercase) {
+ return t('Password must contain at least one lowercase letter.');
+ }
+ if (!checks.digit) {
+ return t('Password must contain at least one digit.');
+ }
+ if (!checks.special) {
+ return t(
+ 'Password must contain at least one special character (not a letter,
digit, or space).',
+ );
+ }
+ if (!checks.commonPassword) {
+ return t('Password is too common.');
+ }
+ return null;
+ };
+
+ const handleFormSubmit = async (values: {
+ password: string;
+ confirmPassword: string;
+ }) => {
+ if (!user) {
+ return;
+ }
+ const { confirmPassword, ...rest } = values;
+ void confirmPassword;
+ const payload = {
+ ...buildSecurityUserUpdatePayload(user),
+ password: rest.password,
+ };
+ try {
+ await SupersetClient.put({
+ endpoint: `/api/v1/security/users/${user.id}`,
+ jsonPayload: payload,
+ });
+ addSuccessToast(
+ t('Password for %(username)s was updated successfully', {
+ username: user.username,
+ }),
+ );
+ } catch (error) {
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing callback breaks modal workflow</b></div>
<div id="fix">
Removing `onSave()` breaks the parent workflow in `UsersList/index.tsx`. The
`onSave` callback (lines 575-579) handles `refreshData()`,
`closeModal(ModalType.RESET_PASSWORD)`, and `setCurrentUser(null)`. Without
this call, users will not see updated data, the modal won't close, and state
won't reset after successful password changes.
</div>
</div>
<small><i>Code Review Run #3957e7</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/views/users/api.py:
##########
@@ -163,6 +235,177 @@ def update_me(self) -> Response:
except ValidationError as error:
return self.response_400(message=error.messages)
+ @expose("/password", methods=["PUT"])
+ @protect()
+ @permission_name("write")
+ @safe
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.put_password",
+ log_to_statsd=False,
+ )
+ @requires_json
+ @_rate_limit_me_password_change
+ def update_my_password(self) -> Response:
+ """Update the current user's password (AUTH_DB only)
+ ---
+ put:
+ summary: Update the current user's password
+ description: >-
+ Changes the authenticated user's password when ``AUTH_TYPE`` is
``AUTH_DB``.
+ Requires the current password and a new password that satisfies
``AUTH_DB_CONFIG``
+ policy.
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/CurrentUserPasswordPutSchema'
+ responses:
+ 200:
+ description: Password updated successfully
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ $ref: '#/components/schemas/UserResponseSchema'
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ if app.config.get("AUTH_TYPE") != AUTH_DB:
+ return self.response_400(
+ message=(
+ "Password change is only available when AUTH_TYPE is
AUTH_DB."
+ ),
+ )
+ try:
+ body = self.current_user_password_put_schema.load(request.json or
{})
+ except ValidationError as error:
+ return self.response_400(message=error.messages)
+
+ try:
+ validate_auth_db_password(body["new_password"])
+ except ValidationError as error:
+ return self.response_400(message=error.messages)
+
+ user_db = db.session.get(User, g.user.id)
+ if user_db is None:
+ return self.response_404()
+
+ old_hash = user_db.password
+ if not check_password_hash(old_hash, body["current_password"]):
+ return self.response_400(message="Incorrect current password.")
+
+ new_hash = generate_password_hash(
+ password=body["new_password"],
+ method=get_auth_db_password_hash_method(),
+ salt_length=app.config.get("FAB_PASSWORD_HASH_SALT_LENGTH", 16),
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-390: Unhandled Exception on Config Error</b></div>
<div id="fix">
Removed try/except ValidationError around `generate_password_hash` /
`get_auth_db_password_hash_method`. The latter raises
`marshmallow.ValidationError` when `password_hash_algorithm` is misconfigured.
Without this handler, users get an unhandled 500 instead of a descriptive 400.
Restore the exception handling or propagate the error explicitly. (See also:
[CWE-390](https://cwe.mitre.org/data/definitions/390.html))
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset/views/users/api.py
+++ b/superset/views/users/api.py
@@ -301,10 +301,13 @@ class CurrentUserRestApi(BaseSupersetApi):
if not check_password_hash(old_hash, body["current_password"]):
return self.response_400(message="Incorrect current password.")
- new_hash = generate_password_hash(
- password=body["new_password"],
- method=get_auth_db_password_hash_method(),
- salt_length=app.config.get("FAB_PASSWORD_HASH_SALT_LENGTH",
16),
- )
+ try:
+ new_hash = generate_password_hash(
+ password=body["new_password"],
+ method=get_auth_db_password_hash_method(),
+
salt_length=app.config.get("FAB_PASSWORD_HASH_SALT_LENGTH", 16),
+ )
+ except ValidationError as error:
+ return self.response_400(message=error.messages)
try:
self.pre_update(g.user, {})
```
</div>
</details>
</div>
<small><i>Code Review Run #3957e7</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
