bito-code-review[bot] commented on code in PR #39469:
URL: https://github.com/apache/superset/pull/39469#discussion_r3498658774
##########
superset/views/users/api.py:
##########
@@ -14,26 +14,146 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+from __future__ import annotations
+
+import functools
+import logging
from datetime import datetime
-from typing import Any, Dict
+from typing import Any, Callable, Dict
-from flask import current_app as app, g, redirect, request, Response
+from flask import current_app as app, g, redirect, request, Response, session
from flask_appbuilder.api import expose, permission_name, safe
+from flask_appbuilder.const import AUTH_DB
from flask_appbuilder.security.decorators import protect
from flask_appbuilder.security.sqla.models import User
+from flask_login import login_user
from marshmallow import ValidationError
+from sqlalchemy.exc import SQLAlchemyError
from sqlalchemy.orm.exc import NoResultFound
-from werkzeug.security import generate_password_hash
from superset import is_feature_enabled
from superset.daos.user import UserDAO
-from superset.extensions import db, event_logger
+from superset.extensions import db, event_logger, security_manager
+from superset.utils.auth_db_password import (
+ get_auth_db_login_rate_limit_string,
+ get_public_auth_db_password_policy,
+ validate_auth_db_password,
+)
+from superset.utils.auth_db_password_hash import (
+ hash_auth_db_password,
+ verify_auth_db_password,
+)
+from superset.utils.auth_session_stamp import (
+ bump_user_session_auth_stamp,
+ clear_flask_login_remember_cookie,
+ sync_session_auth_stamp_on_login,
+)
+from superset.utils.decorators import transaction
from superset.utils.slack import get_user_avatar, SlackClientError
from superset.views.base_api import BaseSupersetApi, requires_json,
statsd_metrics
-from superset.views.users.schemas import CurrentUserPutSchema,
UserResponseSchema
+from superset.views.users.schemas import (
+ CurrentUserPasswordPutSchema,
+ CurrentUserPutSchema,
+ UserResponseSchema,
+)
from superset.views.utils import bootstrap_user_data
-user_response_schema = UserResponseSchema()
+logger: logging.Logger = logging.getLogger(__name__)
+
+user_response_schema: UserResponseSchema = UserResponseSchema()
+
+
+def _me_password_rate_limit_key() -> str:
+ """Return the per-user rate-limit key for password changes."""
+ return f"me_password_uid:{g.user.id}"
+
+
+def _rate_limit_me_password_change(
+ f: Callable[..., Response],
+) -> Callable[..., Response]:
+ """Apply AUTH_DB ``login_rate_limit`` when Flask-Limiter is enabled."""
+
+ @functools.wraps(f)
+ def wrapped(self: CurrentUserRestApi, *args: Any, **kwargs: Any) ->
Response:
+ """Invoke ``f`` directly or through Flask-Limiter when rate limiting
is on."""
+ if not app.config.get("RATELIMIT_ENABLED", False):
+ return f(self, *args, **kwargs)
+ limiter = getattr(security_manager, "limiter", None)
+ if limiter is None:
+ return f(self, *args, **kwargs)
+ limited_view = limiter.limit(
+ get_auth_db_login_rate_limit_string(),
+ key_func=_me_password_rate_limit_key,
+ methods=["PUT"],
+ )(f)
+ return limited_view(self, *args, **kwargs)
+
+ return wrapped
+
+
+class PasswordChangeConflictError(Exception):
+ """Raised when an optimistic password update loses a concurrent write
race."""
+
+
+def _load_password_change_body(
+ schema: CurrentUserPasswordPutSchema,
+ payload: object,
+) -> dict[str, str]:
+ """Parse and policy-validate a password-change request payload."""
+ body = schema.load(payload or {})
+ validate_auth_db_password(body["new_password"])
+ return body
+
+
+@transaction()
+def _commit_user_password_change(
+ api: CurrentUserRestApi,
+ user_id: int,
+ old_hash: str,
+ new_hash: str,
+) -> User:
+ """Persist a password change and rotate the user's session auth stamp.
+
+ The whole flow runs in a single transaction that commits once on success,
+ so any failure path (including a missing user) rolls back the password
+ write rather than reporting an error after it was already committed.
+ """
+ api.pre_update(g.user, {})
+ rows_updated = (
+ db.session.query(User)
+ .filter(User.id == user_id, User.password == old_hash)
+ .update(
+ {
+ User.password: new_hash,
+ User.changed_on: g.user.changed_on,
+ User.changed_by_fk: g.user.changed_by_fk,
+ },
+ synchronize_session=False,
+ )
+ )
+ if rows_updated != 1:
+ raise PasswordChangeConflictError
+
+ bump_user_session_auth_stamp(user_id)
+ from superset.security.password_change import clear_password_must_change
+
+ clear_password_must_change(user_id)
+ user_after = db.session.get(User, user_id)
+ if user_after is None:
+ logger.error("User missing after password update for id=%s", user_id)
+ raise SQLAlchemyError("user missing after password update")
+ return user_after
+
+
+def _reestablish_login_session(user: User) -> None:
+ """Clear the cookie session and log the user back in after a password
change."""
+ for key in list(session.keys()):
+ session.pop(key)
+ login_user(user)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing on_user_login audit hook</b></div>
<div id="fix">
The `login_user(user)` call does not invoke
`security_manager.on_user_login(user)`, which normally stamps login time, syncs
session auth, and logs a `UserLoggedIn` audit event. The existing comment (line
153) acknowledges the missing `on_user_login` call and copies the post-commit
auth stamp, but the audit event and login time stamp are still omitted. If this
omission is intentional (mid-request re-auth vs. fresh login), add a comment
explaining why; otherwise, call `security_manager.on_user_login(user)` after
`login_user`.
</div>
</div>
<small><i>Code Review Run #538335</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/components/AuthDbPasswordPolicyIndicator.tsx:
##########
@@ -0,0 +1,189 @@
+/**
+ * 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 {
+ css,
+ styled,
+ useTheme,
+ type SupersetTheme,
+} from '@apache-superset/core/theme';
+import { t } from '@apache-superset/core/translation';
+import {
+ Icons,
+ Popover,
+ Progress,
+ Typography,
+} from '@superset-ui/core/components';
+import {
+ AUTH_DB_DEFAULT_PASSWORD_POLICY,
+ AuthDbPasswordPolicy,
+ getAuthDbPasswordPolicyChecks,
+} from 'src/utils/generateAuthDbPassword';
+
+interface AuthDbPasswordPolicyIndicatorProps {
+ password: string;
+ policy?: AuthDbPasswordPolicy;
+}
+
+const StrengthWrapper = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ align-items: center;
+ gap: ${theme.sizeUnit * 2}px;
+ `}
+`;
+
+const StrengthBarContainer = styled.div`
+ ${({ theme }) => css`
+ flex: 1;
+ cursor: help;
+
+ .ant-progress {
+ margin-bottom: 0;
+ }
+ `}
+`;
+
+const Checklist = styled.div`
+ ${({ theme }) => css`
+ min-width: ${theme.sizeUnit * 70}px;
+ display: flex;
+ flex-direction: column;
+ gap: ${theme.sizeUnit}px;
+ `}
+`;
+
+const ChecklistItem = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ align-items: center;
+ gap: ${theme.sizeUnit * 2}px;
+ font-size: ${theme.fontSize}px;
+ `}
+`;
+
+const requirementText = {
+ minLength: (minLength: number) => t('At least %s characters', minLength),
+ uppercase: t('Contains an uppercase letter'),
+ lowercase: t('Contains a lowercase letter'),
+ digit: t('Contains a digit'),
+ special: t('Contains a special character'),
+ commonPassword: t('Is not a common password'),
+};
+
+function getStrengthState(percent: number, theme: SupersetTheme) {
+ if (percent <= 33) {
+ return { color: theme.colorError, label: t('Very weak') };
+ }
+ if (percent <= 50) {
+ return { color: theme.colorWarningText, label: t('Weak') };
+ }
+ if (percent <= 67) {
+ return { color: theme.colorWarning, label: t('Medium') };
+ }
+ if (percent <= 83) {
+ return { color: theme.colorSuccess, label: t('Strong') };
+ }
+ return { color: theme.colorSuccessTextActive, label: t('Very strong') };
+}
+
+export default function AuthDbPasswordPolicyIndicator({
+ password,
+ policy = AUTH_DB_DEFAULT_PASSWORD_POLICY,
+}: AuthDbPasswordPolicyIndicatorProps) {
+ const theme = useTheme();
+ const checks = getAuthDbPasswordPolicyChecks(password, policy);
+ const hasPassword = password.length > 0;
+ const checklist = [
+ {
+ label: requirementText.minLength(policy.password_min_length),
+ passed: hasPassword && checks.minLength,
+ },
+ ...(policy.password_require_uppercase
+ ? [
+ {
+ label: requirementText.uppercase,
+ passed: hasPassword && checks.uppercase,
+ },
+ ]
+ : []),
+ ...(policy.password_require_lowercase
+ ? [
+ {
+ label: requirementText.lowercase,
+ passed: hasPassword && checks.lowercase,
+ },
+ ]
+ : []),
+ ...(policy.password_require_digit
+ ? [{ label: requirementText.digit, passed: hasPassword && checks.digit }]
+ : []),
+ ...(policy.password_require_special
+ ? [
+ {
+ label: requirementText.special,
+ passed: hasPassword && checks.special,
+ },
+ ]
+ : []),
+ ...(policy.password_common_list_check
+ ? [
+ {
+ label: requirementText.commonPassword,
+ passed: hasPassword && checks.commonPassword,
+ },
+ ]
+ : []),
+ ];
+ const passedChecks = checklist.filter(item => item.passed).length;
+ const percent = Math.round((passedChecks / checklist.length) * 100);
+ const strength = getStrengthState(percent, theme);
+
+ return (
+ <StrengthWrapper>
+ <Popover
+ trigger="hover"
+ placement="topLeft"
+ content={
+ <Checklist>
+ {checklist.map(item => (
+ <ChecklistItem key={item.label}>
+ {item.passed ? (
+ <Icons.CheckCircleFilled iconColor="success" iconSize="m" />
+ ) : (
+ <Icons.CloseCircleOutlined iconColor="danger" iconSize="m" />
+ )}
+ <Typography.Text>{item.label}</Typography.Text>
+ </ChecklistItem>
+ ))}
+ </Checklist>
+ }
+ >
+ <StrengthBarContainer>
+ <Progress
+ percent={percent}
+ showInfo={false}
+ strokeColor={strength.color}
+ size="small"
+ />
+ </StrengthBarContainer>
+ </Popover>
+ <Typography.Text type="secondary">{strength.label}</Typography.Text>
+ </StrengthWrapper>
+ );
+}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for new component</b></div>
<div id="fix">
New component with business logic (strength calculation at lines 153-155,
conditional checklist generation at lines 112-152) lacks unit test coverage.
BITO.md rule [11730] requires comprehensive tests for new tools covering
success paths, error scenarios, and edge cases. A component that visualizes
password strength and validates policy requirements needs tests to verify
correctness of the percentage and label logic.
</div>
</div>
<small><i>Code Review Run #27f12a</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests</b></div>
<div id="fix">
No unit tests exist for this component. Per [11730], new components need
tests covering success paths, error scenarios, validation failures, and edge
cases. Create `AuthDbPasswordPolicyIndicator.test.tsx` using Jest + React
Testing Library patterns (no Enzyme).
</div>
</div>
<small><i>Code Review Run #538335</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/constants/auth.ts:
##########
@@ -0,0 +1,28 @@
+/**
+ * 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.
+ */
+
+/** Mirrors Flask-AppBuilder ``AUTH_TYPE`` values from bootstrap
``common.conf``. */
+export enum AuthType {
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>AuthType naming collision</b></div>
<div id="fix">
The name `AuthType` collides with an existing enum in
`src/features/databases/DatabaseModal/index.tsx:183-186` that represents SSH
login methods (`Password`, `PrivateKey`). These are semantically different
domains (authentication method vs SSH credential type). Exporting both with the
same name creates potential type confusion for callers. The enum comment
correctly notes it mirrors Flask-AppBuilder's `AUTH_TYPE`, so `AuthMethod`
would clearly distinguish its purpose from the SSH tunnel enum.
</div>
</div>
<small><i>Code Review Run #538335</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]
