rusackas commented on code in PR #44626:
URL: https://github.com/apache/superset/pull/44626#discussion_r4116908774
##########
superset-frontend/src/features/users/UserListModal.tsx:
##########
@@ -200,48 +200,66 @@ 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']}
Review Comment:
Good catch, added `required={!isEditMode}` so the asterisk comes back
without touching the validator message.
##########
tests/integration_tests/core_tests.py:
##########
@@ -133,6 +132,39 @@ def assert_admin_view_menus_in(role_name, assert_func):
assert_admin_view_menus_in("Alpha", self.assertNotIn)
assert_admin_view_menus_in("Gamma", self.assertNotIn)
+ def test_legacy_fab_password_views_are_gone(self):
+ """The legacy FAB reset routes are not registered, no role holds their
+ permissions, and the user-view buttons that led to them are dead ends
+ rather than 500s."""
+ rules = {rule.rule for rule in current_app.url_map.iter_rules()}
+ endpoints = {rule.endpoint for rule in
current_app.url_map.iter_rules()}
+ assert "/resetpassword/form" not in rules
+ assert "/resetmypassword/form" not in rules
+ assert not {
+ endpoint
+ for endpoint in endpoints
+ if endpoint.startswith(("ResetPasswordView.",
"ResetMyPasswordView."))
+ }
+
+ for role_name in ("Admin", "Alpha", "Gamma"):
+ role = security_manager.find_role(role_name)
+ perms = {(p.permission.name, p.view_menu.name) for p in
role.permissions}
+ assert not {
+ view_menu
+ for _, view_menu in perms
+ if view_menu in ("ResetPasswordView", "ResetMyPasswordView")
+ }, role_name
+ assert ("resetpasswords", "UserDBModelView") not in perms,
role_name
+ assert ("resetmypassword", "UserDBModelView") not in perms,
role_name
+
+ self.login(ADMIN_USERNAME)
+ assert self.client.get("/resetpassword/form?pk=1").status_code == 404
+ assert self.client.get("/resetmypassword/form").status_code == 404
+ for action in ("resetpasswords", "resetmypassword"):
+ resp = self.client.get(f"/users/action/{action}/1")
+ assert resp.status_code in (302, 404), action
Review Comment:
Fair, dropped that loop. `test_disable_legacy_password_reset_launchers`
already covers the button dead-end directly, so this one was really just
re-testing the permission redirect.
--
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]
