sadpandajoe commented on code in PR #44626:
URL: https://github.com/apache/superset/pull/44626#discussion_r4132766847
##########
superset-frontend/src/features/users/UserListModal.tsx:
##########
@@ -200,48 +200,67 @@ function UserListModal({
}
/>
</FormItem>
- {!isEditMode && (
- <>
- <FormItem
- name="password"
- label={t('Password')}
- rules={[
- { required: true, message: t('Password is required') },
- ]}
- >
- <Input.Password
- name="password"
- placeholder={t("Enter the user's password")}
- />
- </FormItem>
- <FormItem
- name="confirmPassword"
- label={t('Confirm Password')}
- dependencies={['password']}
- rules={[
- {
- required: true,
- message: t('Please confirm your password'),
- },
- ({ getFieldValue }) => ({
- validator(_, value) {
- if (!value || getFieldValue('password') === value) {
- return Promise.resolve();
- }
- return Promise.reject(
- new Error(t('Passwords do not match!')),
- );
- },
- }),
- ]}
- >
- <Input.Password
- name="confirmPassword"
- placeholder={t("Confirm the user's password")}
- />
- </FormItem>
- </>
- )}
+ <FormItem
+ name="password"
+ label={isEditMode ? t('New password') : t('Password')}
+ extra={
+ isEditMode
+ ? t('Leave blank to keep the current password')
+ : undefined
+ }
+ rules={[
+ { required: !isEditMode, message: t('Password is required') },
+ ]}
+ >
+ <Input.Password
+ name="password"
+ placeholder={
+ isEditMode
+ ? t('Enter a new password')
+ : t("Enter the user's password")
+ }
+ />
+ </FormItem>
+ <FormItem
+ name="confirmPassword"
+ label={
+ isEditMode ? t('Confirm new password') : t('Confirm Password')
+ }
+ dependencies={['password']}
+ required={!isEditMode}
+ rules={[
+ ({ getFieldValue }) => ({
+ validator(_, value) {
+ const password = getFieldValue('password');
+ // In edit mode the password is optional: both fields
+ // empty means "keep the current password".
+ if (isEditMode && !password && !value) {
+ return Promise.resolve();
+ }
+ if (!value) {
+ return Promise.reject(
+ new Error(t('Please confirm your password')),
+ );
+ }
+ if (password === value) {
+ return Promise.resolve();
+ }
+ return Promise.reject(
+ new Error(t('Passwords do not match!')),
Review Comment:
In edit mode, leaving **New password** blank while typing anything into
**Confirm new password** shows "Passwords do not match!", but nothing was
actually mismatched — the new-password field is simply empty. Should this
branch check `!password && value` first and point the user at the empty field
instead?
##########
superset/security/password_change.py:
##########
@@ -191,7 +216,7 @@ def _enforce_password_change() -> Any: # pylint:
disable=unused-variable
# same hook and would trap the user in an infinite 302 loop. If no
# exempt target can be resolved at all, return an error response rather
# than redirect, so a flagged user can never get stuck looping.
- candidates = ["ResetMyPasswordView.this_form_get"]
+ candidates = [_PROFILE_PAGE_ENDPOINT]
Review Comment:
A flagged user is unconditionally redirected to `UserInfoView.list`, which
itself requires `can_read on user` — but the old redirect target,
`ResetMyPasswordView`, was reachable by every authenticated user regardless of
role (it was in `ACCESSIBLE_PERMS`). A custom role that lacks `can_read on
user` (for example one that was only ever granted the legacy reset permissions)
now gets redirected here, denied by FAB's own access check, and falls back only
to logout — with no way left to reach a password-change flow and clear
`password_must_change`. Is that intentional, or should the profile page (or
some reachable change-password path) stay available to any authenticated user
the way self-service reset used to be?
##########
superset/security/manager.py:
##########
@@ -3286,7 +3337,9 @@ def sync_role_definitions(self) -> None:
self.create_custom_permissions()
- pvms = self._get_all_pvms()
+ pvms = [
+ pvm for pvm in self._get_all_pvms() if not
self._is_legacy_password_pvm(pvm)
Review Comment:
This filter only feeds `set_role` calls for Admin, Alpha, Gamma, sql_lab,
and optionally Public — `sync_role_definitions` never rebuilds a custom
(operator-defined) role. A custom role that already held
`ResetPasswordView`/`ResetMyPasswordView`/`UserDBModelView`
`resetpasswords`/`resetmypassword` permissions before upgrading keeps them
after `superset init`, even though UPDATING.md and the new integration test
claim/verify only that Admin/Alpha/Gamma no longer hold them. Should custom
roles be swept for these stale permissions too, or is the UPDATING.md wording
meant to be narrower than "no role"?
--
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]
