bito-code-review[bot] commented on code in PR #39469:
URL: https://github.com/apache/superset/pull/39469#discussion_r3453937116
##########
tests/integration_tests/users/api_tests.py:
##########
@@ -100,6 +113,214 @@ def test_update_me_empty_payload(self):
rv = self.client.put("/api/v1/me/", json={})
assert rv.status_code == 400
+ def test_update_me_rejects_password_when_auth_db(self):
+ self.login(ADMIN_USERNAME)
+ rv = self.client.put(meUri, json={"password": "ignored"})
+ assert rv.status_code == 400
+ data = json.loads(rv.data.decode("utf-8"))
+ assert "AUTH_TYPE is AUTH_DB" in data["message"]
+
+ def test_put_my_password_wrong_current(self):
+ self.login(ADMIN_USERNAME)
+ rv = self.client.put(
+ mePasswordUri,
+ json={
+ "current_password": "not-the-admin-password",
+ "new_password": "AnotherStr0ng!Pass",
+ "confirm_password": "AnotherStr0ng!Pass",
+ },
+ )
+ assert rv.status_code == 400
+ data = json.loads(rv.data.decode("utf-8"))
+ assert data["message"] == "Incorrect current password."
+
+ def test_put_my_password_weak_new(self):
+ self.login(ADMIN_USERNAME)
+ rv = self.client.put(
+ mePasswordUri,
+ json={
+ "current_password": DEFAULT_PASSWORD,
+ "new_password": "short",
+ "confirm_password": "short",
+ },
+ )
+ assert rv.status_code == 400
+ data = json.loads(rv.data.decode("utf-8"))
+ assert "new_password" in data["message"]
+
+ def test_put_my_password_success(self):
+ self.login(ADMIN_USERNAME)
+ new_password = "AnotherStr0ng!Pass"
+ try:
+ rv = self.client.put(
+ mePasswordUri,
+ json={
+ "current_password": DEFAULT_PASSWORD,
+ "new_password": new_password,
+ "confirm_password": new_password,
+ },
+ )
+ assert rv.status_code == 200
+
+ rv2 = self.client.put(
+ mePasswordUri,
+ json={
+ "current_password": new_password,
+ "new_password": "YetAnotherStr0ng!Pw",
+ "confirm_password": "YetAnotherStr0ng!Pw",
+ },
+ )
+ assert rv2.status_code == 200
+ finally:
+ self._restore_admin_default_password()
+
+ def test_put_my_password_invalidates_cloned_session_client(self):
+ """
+ Rotating the session stamp on password change logs out other clients
that
+ still present the pre-change signed session cookie.
+ """
+ from flask.testing import FlaskClient
+
+ app = superset_integration_app
+ client_a: FlaskClient = app.test_client()
+ login(client_a, ADMIN_USERNAME)
+
+ session_cookie = client_a.get_cookie("session")
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Undefined name 'login' reference</b></div>
<div id="fix">
The name `login` is not defined. It appears this should be `self.login()` to
call the test case login method, consistent with other test methods in the file.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
app = superset_integration_app
client_a: FlaskClient = app.test_client()
self.login(ADMIN_USERNAME)
session_cookie = client_a.get_cookie("session")
````
</div>
</details>
</div>
<small><i>Code Review Run #e1b7ec</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
##########
tests/integration_tests/users/api_tests.py:
##########
@@ -19,18 +19,31 @@
from unittest.mock import patch
+from flask_appbuilder.const import AUTH_OAUTH
+
from superset import security_manager
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing import causes runtime crash</b></div>
<div id="fix">
The import of `with_config` was removed but the decorator is still used at
line 339 for `test_avatar_with_valid_user`. Without this import, running any
test that uses the decorator will raise `NameError: name 'with_config' is not
defined`.
</div>
</div>
<small><i>Code Review Run #e1b7ec</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/userInfo/UserInfoModal.tsx:
##########
@@ -135,15 +244,20 @@ function UserInfoModal({
requiredFields={requiredFields}
initialValues={initialValues}
>
- {isEditMode ? <EditModeFields /> : <ResetPasswordFields />}
+ {(form: FormInstance) =>
+ isEditMode ? <EditModeFields /> : <ResetPasswordFields form={form} />
+ }
</FormModal>
);
}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for new modal</b></div>
<div id="fix">
The `UserInfoModal` component has no dedicated unit tests despite adding
significant new functionality including password policy validation, strength
indicator, and password generation. According to rule [11730], comprehensive
unit tests for new features must cover success paths, error scenarios, and
validation failures.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
no_patch_reason
```
</div>
</details>
</div>
<small><i>Code Review Run #e1b7ec</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/security/manager.py:
##########
@@ -4213,6 +4226,36 @@ def is_admin(self) -> bool:
role.name for role in self.get_user_roles()
]
+ def auth_user_db(self, username: str, password: str) -> User | None:
+ """
+ Authenticate a database user, verifying bcrypt/argon2 and legacy
hashes.
+ """
+ if username is None or username == "":
+ return None
+ first_user = self.get_first_user()
+ user = self.find_user(username=username)
+ if user is None:
+ user = self.find_user(email=username)
+ else:
+ # Balance failure and success
+ _ = self.find_user(email=username)
+ if user is None or (not user.is_active):
+ # Balance failure and success
+ check_password_hash(
+ current_app.config["AUTH_DB_FAKE_PASSWORD_HASH_CHECK"],
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-20: Undefined Config Key</b></div>
<div id="fix">
Reference to undefined config key `AUTH_DB_FAKE_PASSWORD_HASH_CHECK` will
raise `KeyError` at runtime. Define this config value in `config.py` or remove
the fake hash check to prevent authentication failures. (See also:
[CWE-20](https://cwe.mitre.org/data/definitions/20.html))
</div>
</div>
<small><i>Code Review Run #e1b7ec</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/utils/generateAuthDbPassword.ts:
##########
@@ -0,0 +1,274 @@
+/**
+ * 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';
+/**
+ * Client-side generator aligned with default ``AUTH_DB_CONFIG`` / Python
+ * ``superset.utils.auth_db_password`` (minimum length, character classes,
common list).
+ * Keep ``AUTH_DB_COMMON_PASSWORDS`` in sync with ``_COMMON_PASSWORDS`` in
that module.
+ */
+export const AUTH_DB_PASSWORD_MIN_LENGTH = 12;
+export interface AuthDbPasswordPolicy {
+ password_min_length: number;
+ password_require_uppercase: boolean;
+ password_require_lowercase: boolean;
+ password_require_digit: boolean;
+ password_require_special: boolean;
+ password_common_list_check: boolean;
+}
+
+export const AUTH_DB_DEFAULT_PASSWORD_POLICY: AuthDbPasswordPolicy = {
+ password_min_length: AUTH_DB_PASSWORD_MIN_LENGTH,
+ password_require_uppercase: true,
+ password_require_lowercase: true,
+ password_require_digit: true,
+ password_require_special: true,
+ password_common_list_check: true,
+};
+
+/** Lowercased entries; keep in sync with ``_COMMON_PASSWORDS`` in
auth_db_password.py */
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>No automated sync for common passwords</b></div>
<div id="fix">
The comment states 'keep in sync with _COMMON_PASSWORDS in that module' but
there's no automated mechanism to verify synchronization. Manual
synchronization risks drift over time, causing the frontend to allow passwords
the backend rejects.
</div>
</div>
<small><i>Code Review Run #e1b7ec</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/security/manager.py:
##########
@@ -4213,6 +4226,36 @@ def is_admin(self) -> bool:
role.name for role in self.get_user_roles()
]
+ def auth_user_db(self, username: str, password: str) -> User | None:
+ """
+ Authenticate a database user, verifying bcrypt/argon2 and legacy
hashes.
+ """
+ if username is None or username == "":
+ return None
+ first_user = self.get_first_user()
+ user = self.find_user(username=username)
+ if user is None:
+ user = self.find_user(email=username)
+ else:
+ # Balance failure and success
+ _ = self.find_user(email=username)
+ if user is None or (not user.is_active):
+ # Balance failure and success
+ check_password_hash(
+ current_app.config["AUTH_DB_FAKE_PASSWORD_HASH_CHECK"],
+ "password",
+ )
+ logger.info(LOGMSG_WAR_SEC_LOGIN_FAILED, username)
+ if first_user:
+ self.noop_user_update(first_user)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-393: Missing Method Definition</b></div>
<div id="fix">
Method `noop_user_update` is called but does not exist in
SupersetSecurityManager or its parent BaseSecurityManager. This will raise
AttributeError at runtime during failed login attempts. (See also:
[CWE-393](https://cwe.mitre.org/data/definitions/393.html))
</div>
</div>
<small><i>Code Review Run #e1b7ec</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]
