codeant-ai-for-open-source[bot] commented on code in PR #39469: URL: https://github.com/apache/superset/pull/39469#discussion_r3425682720
########## superset-frontend/src/components/AuthDbPasswordPolicyIndicator.tsx: ########## @@ -0,0 +1,163 @@ +/** + * 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 } 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(passedChecks: number) { + if (passedChecks <= 2) { + return { color: '#cf1322', label: t('Very weak') }; + } + if (passedChecks === 3) { + return { color: '#d46b08', label: t('Weak') }; + } + if (passedChecks === 4) { + return { color: '#d4b106', label: t('Medium') }; + } + if (passedChecks === 5) { + return { color: '#389e0d', label: t('Strong') }; + } + return { color: '#08979c', label: t('Very strong') }; +} + +export default function AuthDbPasswordPolicyIndicator({ + password, + policy = AUTH_DB_DEFAULT_PASSWORD_POLICY, +}: AuthDbPasswordPolicyIndicatorProps) { + 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(passedChecks); Review Comment: **Suggestion:** Strength labeling is computed from the absolute number of passed checks, not from the ratio of passed-to-enabled rules, so policies with fewer enabled requirements are misclassified (for example 100% pass can still show "Very weak"). Derive strength from `percent` (or normalize by `checklist.length`) to keep labels consistent with configurable policy settings. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Password strength label misleading under relaxed AUTH_DB policies. - ⚠️ Users may distrust password-strength feedback in reset modal. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure Superset with AUTH_DB enabled and a relaxed AUTH_DB_CONFIG so that only `password_min_length` is enforced (all `password_require_*` and `password_common_list_check` flags false) using the documented AUTH_DB_CONFIG options in `docs/admin_docs/configuration/configuring-superset.mdx:295-299`. 2. Start Superset, log in with AUTH_DB, and navigate to the "Your user information" page implemented in `superset-frontend/src/pages/UserInfo/index.tsx:91-125`, where `authType === AuthType.AuthDB` enables the "Reset my password" button in `SubMenuButtons` at lines 126-149. 3. Click "Reset my password" to open `ChangePasswordModal` from `superset-frontend/src/features/userInfo/UserInfoModal.tsx:276-283`, which renders `ResetPasswordFields` (lines 167-257) and includes a "Password strength" FormItem that mounts `AuthDbPasswordPolicyIndicator` (lines 213-227). 4. In the "New password" field, enter a password that satisfies the active relaxed policy so that `getAuthDbPasswordPolicyChecks` in `AuthDbPasswordPolicyIndicator.tsx:99-126` returns `true` for all enabled checks; `passedChecks` then equals `checklist.length`, `percent` computes to 100 at line 128, but `getStrengthState(passedChecks)` at line 129 classifies values `<= 2` as "Very weak" (lines 79-82), so with only one or two enabled checks the indicator shows a 100% progress bar labeled "Very weak" despite the user fully satisfying the configured policy. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d71a99c70f284af18067df28710a7bed&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d71a99c70f284af18067df28710a7bed&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/components/AuthDbPasswordPolicyIndicator.tsx **Line:** 127:129 **Comment:** *Logic Error: Strength labeling is computed from the absolute number of passed checks, not from the ratio of passed-to-enabled rules, so policies with fewer enabled requirements are misclassified (for example 100% pass can still show "Very weak"). Derive strength from `percent` (or normalize by `checklist.length`) to keep labels consistent with configurable policy settings. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=11f196c85143fd335c83ea6f6b983f101de24432737ecdcea4c4f333ad2d59d6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=11f196c85143fd335c83ea6f6b983f101de24432737ecdcea4c4f333ad2d59d6&reaction=dislike'>👎</a> ########## superset-frontend/src/features/userInfo/UserInfoModal.tsx: ########## @@ -86,30 +164,82 @@ function UserInfoModal({ </> ); - const ResetPasswordFields = () => ( + const ResetPasswordFields = ({ form }: { form: FormInstance }) => ( <> <FormItem - name="password" - label={t('Password')} - rules={[{ required: true, message: t('Password is required') }]} + name="current_password" + label={t('Current password')} + rules={[{ required: true, message: t('Current password is required') }]} > <Input.Password - name="password" - placeholder={t("Enter the user's password")} + name="current_password" + autoComplete="current-password" + placeholder={t('Enter your current password')} /> </FormItem> + <FormItem + name="new_password" + label={t('New password')} + rules={[ + { required: true, message: t('New password is required') }, + { + validator(_, value) { + const password = String(value ?? ''); + if (!password) { + return Promise.resolve(); + } + const errorMessage = getPasswordPolicyError(password); + return errorMessage + ? Promise.reject(new Error(errorMessage)) + : Promise.resolve(); + }, + }, + ]} + > + <Input.Password + name="new_password" + autoComplete="new-password" + placeholder={t('Enter a new password')} + suffix={ + <GeneratePasswordInputSuffix + onGenerate={() => { + const pwd = generateAuthDbPassword(); + form.setFieldsValue({ new_password: pwd, confirm_password: pwd }); Review Comment: **Suggestion:** The generated password ignores the fetched `passwordPolicy` and always uses the default generator rules, so in deployments with stricter `AUTH_DB_CONFIG` (for example longer minimum length) the "Generate password" action can produce a value that immediately fails validation and cannot be submitted. Generate passwords against the active policy (or re-generate until `getPasswordPolicyError` returns null) before setting form fields. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Generated passwords rejected under stricter AUTH_DB policies. - ⚠️ Reset password flow less usable for hardened deployments. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. On the server, configure a stricter AUTH_DB_CONFIG (for example `password_min_length = 16`) so that `get_public_auth_db_password_policy()` returns this longer minimum; this policy is exposed by `CurrentUserRestApi.get_my_password_policy` at `superset/views/users/api.py:20-53`, which serves `GET /api/v1/me/password/policy`. 2. Start Superset with `AUTH_TYPE = AUTH_DB`, log in, and open the "Your user information" page in `superset-frontend/src/pages/UserInfo/index.tsx:91-125`, where `authType === AuthType.AuthDB` and the "Reset my password" SubMenu button (lines 126-149) opens the change-password flow. 3. Click "Reset my password" to open `ChangePasswordModal` from `superset-frontend/src/features/userInfo/UserInfoModal.tsx:276-283`; on mount, the modal fetches the active password policy via `SupersetClient.get({ endpoint: '/api/v1/me/password/policy' })` in the `useEffect` at lines 58-73 and stores it in `passwordPolicy` (lines 54-56), so `passwordPolicy.password_min_length` reflects the stricter server value (e.g., 16). 4. In the reset modal, click the "Generate password" suffix attached to the "New password" input in `ResetPasswordFields` (lines 167-212); the handler at lines 204-207 calls `generateAuthDbPassword()` from `superset-frontend/src/utils/generateAuthDbPassword.ts:162-179`, which always generates a password targeting the default 12-character `AUTH_DB_DEFAULT_PASSWORD_POLICY` (lines 24, 34-41), then sets it into `new_password` and `confirm_password` via `form.setFieldsValue` (line 207). When the FormItem validator for "New password" runs at lines 181-197 using `getPasswordPolicyError` (lines 75-101), `checks.minLength` fails because the generated password length (12) is less than `passwordPolicy.password_min_length` (16), so the UI shows "Password must be at least %s characters long." and rejects submission, demonstrating that the generator ignores the active policy and can produce passwords that cannot be submitted under custom AUTH_DB settings. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=eb0bfefb62ea45979d9db794d02508b1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=eb0bfefb62ea45979d9db794d02508b1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/features/userInfo/UserInfoModal.tsx **Line:** 206:207 **Comment:** *Api Mismatch: The generated password ignores the fetched `passwordPolicy` and always uses the default generator rules, so in deployments with stricter `AUTH_DB_CONFIG` (for example longer minimum length) the "Generate password" action can produce a value that immediately fails validation and cannot be submitted. Generate passwords against the active policy (or re-generate until `getPasswordPolicyError` returns null) before setting form fields. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=4fb21420f154fd69446d04375915f5942aeaee4e1ea318d9c5ecb32c2f190aab&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=4fb21420f154fd69446d04375915f5942aeaee4e1ea318d9c5ecb32c2f190aab&reaction=dislike'>👎</a> -- 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]
