EnxDev commented on code in PR #44626:
URL: https://github.com/apache/superset/pull/44626#discussion_r4103669726
##########
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:
This GET gets bounced with a 302 before the launcher code ever runs: Admin
no longer holds `resetpasswords`, so the permission check redirects (and FAB's
GET-action CSRF guard does the same when it's on). So the assertion still
passes with `_disable_legacy_password_reset_launchers` deleted.
Fine to drop the loop since the unit test covers the 404, or grant the perm
here and assert 404 if you want it end to end.
##########
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:
Nit, take it or leave it. Dropping the `required: true` rule also dropped
the asterisk on Confirm Password in the create form, since antd only draws it
from a required rule or the `required` prop.
The prop is display-only, so this brings the marker back without doubling up
the validator's message:
```suggestion
dependencies={['password']}
required={!isEditMode}
```
--
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]
