codeant-ai-for-open-source[bot] commented on code in PR #39469: URL: https://github.com/apache/superset/pull/39469#discussion_r3425689348
########## 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.'); Review Comment: **Suggestion:** The password validator is hard-wired to the default policy instead of the server policy, so admin reset can reject passwords that are valid for the current deployment (or allow ones the backend rejects). Fetch and apply the same `/api/v1/me/password/policy` policy object used elsewhere before validating. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Admin reset UI accepts or rejects passwords inconsistently. - ⚠️ Password policy inconsistent with self-service modal and backend. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure AUTH_DB with a non-default password policy via `AUTH_DB_CONFIG`, for example set `"password_require_uppercase": false` while keeping `AUTH_TYPE = AUTH_DB` (backend policy helpers at `superset/superset/utils/auth_db_password.py:27-41` and `validate_auth_db_password()` at lines 164-201 show these flags are honored server-side). 2. As an admin, open the Users List page (`superset-frontend/src/pages/UsersList/index.tsx`), and click the "Reset password" action in the Actions column (handler `handleResetPassword` at lines 37-45 in the 300–360 block sets `currentUser` and opens `ModalType.RESET_PASSWORD`, which renders `<UserListResetPasswordModal>` at lines 68-81 in the 500–560 block). 3. In `UserListResetPasswordModal` (`superset-frontend/src/features/users/UserListResetPasswordModal.tsx`), the password field uses the local `getPasswordPolicyError` validator at lines 54-79, which calls `getAuthDbPasswordPolicyChecks(password)` with no `policy` argument (line 55) and unconditionally checks `.uppercase`, `.lowercase`, `.digit`, `.special`, and `.commonPassword`, while also hard-coding the minimum length message to `AUTH_DB_PASSWORD_MIN_LENGTH` (lines 56-60). 4. Enter a password that would be valid under the configured server policy but invalid under the default client policy, for example a long password with no uppercase when `password_require_uppercase` is disabled in `AUTH_DB_CONFIG`. The admin modal's validator still enforces the default "must contain at least one uppercase letter" rule and rejects the password client-side (lines 62-77), while the backend `validate_auth_db_password()` would accept it under the merged config—demonstrating the mismatch between admin reset UI and the actual server policy and the `/api/v1/me/password/policy`-backed flow used in `UserInfoModal` (`superset-frontend/src/features/userInfo/UserInfoModal.tsx` lines 54-83). ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d82b78f7d37a4e7499e76b0bf5f3b039&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=d82b78f7d37a4e7499e76b0bf5f3b039&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/users/UserListResetPasswordModal.tsx **Line:** 55:77 **Comment:** *Api Mismatch: The password validator is hard-wired to the default policy instead of the server policy, so admin reset can reject passwords that are valid for the current deployment (or allow ones the backend rejects). Fetch and apply the same `/api/v1/me/password/policy` policy object used elsewhere before validating. 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=9c859ae236bd6863bfa91ff930bd946ffe8148a0ec40976ae51d09dfe01e4605&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=9c859ae236bd6863bfa91ff930bd946ffe8148a0ec40976ae51d09dfe01e4605&reaction=dislike'>👎</a> ########## superset-frontend/src/features/users/utils.ts: ########## @@ -19,8 +19,22 @@ import { t } from '@apache-superset/core/translation'; import { SupersetClient } from '@superset-ui/core'; import { SelectOption } from 'src/components/ListView'; +import type { UserObject } from 'src/pages/UsersList/types'; import { FormValues } from './types'; +/** Fields required for PUT `/api/v1/security/users/:id` when changing password only. */ +export function buildSecurityUserUpdatePayload(user: UserObject): FormValues { + return { + first_name: user.first_name, + last_name: user.last_name, + username: user.username, + email: user.email, + active: user.active, + roles: (user.roles ?? []).map(r => r.id), + groups: (user.groups ?? []).map(g => g.id), + }; Review Comment: **Suggestion:** This helper builds a full user update payload from the list row snapshot, so a password reset can unintentionally overwrite profile/authorization fields with stale values from the grid. Use a password-only endpoint/payload for reset operations, or re-fetch fresh user details right before PUT to avoid clobbering concurrent changes. [stale reference] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Password reset can silently revert user roles, groups, active flag. - ⚠️ Admin-facing Users List modal may clobber concurrent profile updates. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. An admin opens the Users List view (`superset-frontend/src/pages/UsersList/index.tsx`), which fetches user rows and renders them in the table. Each row's `original` object (including `first_name`, `last_name`, `email`, `active`, `roles`, and `groups`) is stored in the table state and passed through as `currentUser` when actions are triggered (see Action cell at lines 37-45 in the 300–360 block, and modal wiring at lines 68-81 in the 500–560 block). 2. Another admin (or the same admin in a different session) edits the user's profile via the "Edit user" action, which opens `UserListEditModal` (`superset-frontend/src/features/users/UserListModal.tsx`). In edit mode, `handleFormSubmit` calls `updateUser(user.id, values)` at lines 84-90, which performs `SupersetClient.put` to `/api/v1/security/users/:id` with the updated fields (`updateUser` implementation at `superset-frontend/src/features/users/utils.ts:49-53`), changing roles, groups, or active status server-side without refreshing the first admin's list view. 3. Back in the original Users List tab with stale row data, the first admin clicks "Reset password" for that user. This opens `UserListResetPasswordModal` (`superset-frontend/src/features/users/UserListResetPasswordModal.tsx`), passing the stale `currentUser` object as the `user` prop (lines 68-81 in the 500–560 block). On submit, `handleFormSubmit` at lines 82-94 builds a payload using `buildSecurityUserUpdatePayload(user)` (line 92), then adds the new `password` field and sends a PUT to `/api/v1/security/users/${user.id}` (lines 95-99). 4. `buildSecurityUserUpdatePayload` in `superset-frontend/src/features/users/utils.ts:26-35` constructs a full update object from the stale list snapshot: `first_name`, `last_name`, `username`, `email`, `active`, and mapped `roles`/`groups`. When `SupersetClient.put` sends this payload, it can overwrite more recent changes made in step 2 (for example, reactivating a user or restoring old roles/groups) because the stale data is treated as authoritative. This shows that using a list-row snapshot for a full user update during password reset can unintentionally clobber unrelated profile/authorization fields. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=226d43dbaaa24146a8e877daf387ed5c&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=226d43dbaaa24146a8e877daf387ed5c&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/users/utils.ts **Line:** 26:35 **Comment:** *Stale Reference: This helper builds a full user update payload from the list row snapshot, so a password reset can unintentionally overwrite profile/authorization fields with stale values from the grid. Use a password-only endpoint/payload for reset operations, or re-fetch fresh user details right before PUT to avoid clobbering concurrent changes. 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=c429f05c8780487f3b4185a8bf07def931e05b7e3a0916a8f12349315b6323b7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=c429f05c8780487f3b4185a8bf07def931e05b7e3a0916a8f12349315b6323b7&reaction=dislike'>👎</a> ########## 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) { + const clientError = await getClientErrorObject(error); + const raw = clientError.message; + const text = + typeof raw === 'string' + ? raw + : raw && typeof raw === 'object' + ? (Object.values(raw).flat() as string[]).join(' ') + : t('Something went wrong while updating the password'); + addDangerToast(text); + throw error; + } + }; + + return ( + <FormModal + show={show} + onHide={onHide} + name="user-list-reset-password" + title={ + <ModalTitleWithIcon + title={ + user + ? `${t('Reset password')} — ${user.username}` + : t('Reset password') + } + icon={<Icons.KeyOutlined />} + /> + } + onSave={onSave} + formSubmitHandler={handleFormSubmit} + requiredFields={['password', 'confirmPassword']} + initialValues={{}} + > + {(form: FormInstance) => ( + <> + <FormItem + name="password" + label={t('New password')} + rules={[ + { required: true, message: t('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="password" + autoComplete="new-password" + placeholder={t("Enter the user's new password")} + suffix={ + <GeneratePasswordInputSuffix + onGenerate={() => { + const pwd = generateAuthDbPassword(); + form.setFieldsValue({ password: pwd, confirmPassword: pwd }); Review Comment: **Suggestion:** The inline password generator always uses default policy assumptions, so in environments with stricter `AUTH_DB_CONFIG` (for example longer minimum length) the generated password can fail on submit. Generate against the fetched policy (or validate/regenerate until it satisfies that policy) before setting form values. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Generated passwords often fail client validation under custom policy. - ⚠️ Users must tweak random password manually, harming UX. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure a stricter AUTH_DB password policy on the server, for example set `"password_min_length": 20` in `AUTH_DB_CONFIG` (merged by `get_merged_auth_db_config()` in `superset/superset/utils/auth_db_password.py:109-112`, and exposed via `get_public_auth_db_password_policy()` at lines 128-136 and `GET /api/v1/me/password/policy` in `superset/superset/views/users/api.py` around lines 20-53 of the shown block). 2. As a logged-in user changing their own password, open the profile modal (`UserInfoModal` in `superset-frontend/src/features/userInfo/UserInfoModal.tsx`). On initial show in reset-password mode, it fetches the effective policy from `/api/v1/me/password/policy` (useEffect at lines 58-73) and stores it in `passwordPolicy`, which is used for validation and strength indication (lines 75-101 and 223-226). 3. Click the "Generate" icon on the "New password" field in `UserInfoModal`. This uses the same `GeneratePasswordInputSuffix` pattern as the admin modal, with `onGenerate={() => { const pwd = generateAuthDbPassword(); form.setFieldsValue({ new_password: pwd, confirm_password: pwd }); }}` at lines 203-208, where `generateAuthDbPassword()` is defined in `superset-frontend/src/utils/generateAuthDbPassword.ts:159-182` to generate a password that only satisfies the *default* policy (`AUTH_DB_PASSWORD_MIN_LENGTH = 12` etc., lines 24-41 and 122-136), not the fetched `passwordPolicy`. 4. Because the generator does not take `passwordPolicy` into account, it can return a 12–19 character password when `password_min_length` is 20; `getPasswordPolicyError()` in `UserInfoModal` (lines 75-81) then flags this generated password as too short, and the AntD form rejects submission until the user manually edits or regenerates—showing that the inline generator can produce passwords invalid under the current server policy. The same generator call appears in the admin reset modal at `UserListResetPasswordModal.tsx` lines 165-169, so generated passwords there also only satisfy the default assumptions rather than any customized policy. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=91437e507b4e4671908cdc36be6d6838&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=91437e507b4e4671908cdc36be6d6838&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/users/UserListResetPasswordModal.tsx **Line:** 166:168 **Comment:** *Logic Error: The inline password generator always uses default policy assumptions, so in environments with stricter `AUTH_DB_CONFIG` (for example longer minimum length) the generated password can fail on submit. Generate against the fetched policy (or validate/regenerate until it satisfies that policy) before setting form values. 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=d60271a6d7d6319b16cfabaeb0650905275cb3c05052ded9e90fa55143cc6fe7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=d60271a6d7d6319b16cfabaeb0650905275cb3c05052ded9e90fa55143cc6fe7&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]
