openstack/horizon
Code Issues Proposed changes

move Users Edit Password into separate form

Browse Source 
Based on inline TODO comment.
Code refactor.
Fix unused test case (was not used as helper method and
was not prefixed with 'test_' so it was not used).

Change-Id: Ie696970b4b61d699cc4afa5174248df6c00c688d
Closes-Bug: #1402047
This commit is contained in:
Cindy Lu 2015-01-08 12:16:44 -08:00
parent b7956c9c65
commit 9620cbac91
8 changed files with 196 additions and 117 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
openstack_dashboard/api/keystone.py
View File

@ -329,9 +329,8 @@ def user_update(request, user, **data):
raise keystone_exceptions.ClientException(
405, _("Identity service does not allow editing user data."))
# The v2 API updates user model, password and default project separately
# The v2 API updates user model and default project separately
if VERSIONS.active < 3:
password = data.pop('password')
project = data.pop('project')
# Update user details
@ -358,38 +357,15 @@ def user_update(request, user, **data):
'that project.')
% data.get('name', None))
# If present, update password
# FIXME(gabriel): password change should be its own form + view
if password:
try:
user_update_password(request, user, password)
if user.id == request.user.id:
return utils.logout_with_message(
request,
_("Password changed. Please log in again to "
"continue."),
redirect=False
)
except Exception:
error = exceptions.handle(request, ignore=True)
if error is not None:
raise error
# v3 API is so much simpler...
else:
if not data['password']:
data.pop('password')
try:
user = manager.update(user, **data)
except keystone_exceptions.Conflict:
raise exceptions.Conflict()
if data.get('password') and user.id == request.user.id:
return utils.logout_with_message(
request,
_("Password changed. Please log in again to continue."),
redirect=False
)
def user_update_enabled(request, user, enabled):
@ -401,6 +377,11 @@ def user_update_enabled(request, user, enabled):
def user_update_password(request, user, password, admin=True):
if not keystone_can_edit_user():
raise keystone_exceptions.ClientException(
405, _("Identity service does not allow editing user password."))
manager = keystoneclient(request, admin=admin).users
if VERSIONS.active < 3:
return manager.update_password(user, password)

106
openstack_dashboard/dashboards/identity/users/forms.py
View File

@ -26,6 +26,7 @@ from django.views.decorators.debug import sensitive_variables # noqa
from horizon import exceptions
from horizon import forms
from horizon import messages
from horizon.utils import functions as utils
from horizon.utils import validators
from openstack_dashboard import api
@ -35,6 +36,26 @@ LOG = logging.getLogger(__name__)
PROJECT_REQUIRED = api.keystone.VERSIONS.active < 3
class PasswordMixin(forms.SelfHandlingForm):
password = forms.RegexField(
label=_("Password"),
widget=forms.PasswordInput(render_value=False),
regex=validators.password_validator(),
error_messages={'invalid': validators.password_validator_msg()})
confirm_password = forms.CharField(
label=_("Confirm Password"),
widget=forms.PasswordInput(render_value=False))
no_autocomplete = True
def clean(self):
'''Check to make sure password fields match.'''
data = super(forms.Form, self).clean()
if 'password' in data:
if data['password'] != data.get('confirm_password', None):
raise ValidationError(_('Passwords do not match.'))
return data
class BaseUserForm(forms.SelfHandlingForm):
def __init__(self, request, *args, **kwargs):
super(BaseUserForm, self).__init__(request, *args, **kwargs)
@ -58,19 +79,11 @@ class BaseUserForm(forms.SelfHandlingForm):
project_choices.insert(0, ('', _("Select a project")))
self.fields['project'].choices = project_choices
def clean(self):
'''Check to make sure password fields match.'''
data = super(forms.Form, self).clean()
if 'password' in data:
if data['password'] != data.get('confirm_password', None):
raise ValidationError(_('Passwords do not match.'))
return data
ADD_PROJECT_URL = "horizon:identity:projects:create"
class CreateUserForm(BaseUserForm):
class CreateUserForm(PasswordMixin, BaseUserForm):
# Hide the domain_id and domain_name by default
domain_id = forms.CharField(label=_("Domain ID"),
required=False,
@ -82,24 +95,19 @@ class CreateUserForm(BaseUserForm):
email = forms.EmailField(
label=_("Email"),
required=False)
password = forms.RegexField(
label=_("Password"),
widget=forms.PasswordInput(render_value=False),
regex=validators.password_validator(),
error_messages={'invalid': validators.password_validator_msg()})
confirm_password = forms.CharField(
label=_("Confirm Password"),
widget=forms.PasswordInput(render_value=False))
project = forms.DynamicChoiceField(label=_("Primary Project"),
required=PROJECT_REQUIRED,
add_item_link=ADD_PROJECT_URL)
role_id = forms.ChoiceField(label=_("Role"),
required=PROJECT_REQUIRED)
no_autocomplete = True
def __init__(self, *args, **kwargs):
roles = kwargs.pop('roles')
super(CreateUserForm, self).__init__(*args, **kwargs)
# Reorder form fields from multiple inheritance
self.fields.keyOrder = ["domain_id", "domain_name", "name",
"email", "password", "confirm_password",
"project", "role_id"]
role_choices = [(role.id, role.name) for role in roles]
self.fields['role_id'].choices = role_choices
@ -165,41 +173,24 @@ class UpdateUserForm(BaseUserForm):
email = forms.EmailField(
label=_("Email"),
required=False)
password = forms.RegexField(
label=_("Password"),
widget=forms.PasswordInput(render_value=False),
regex=validators.password_validator(),
required=False,
error_messages={'invalid': validators.password_validator_msg()})
confirm_password = forms.CharField(
label=_("Confirm Password"),
widget=forms.PasswordInput(render_value=False),
required=False)
project = forms.ChoiceField(label=_("Primary Project"),
required=PROJECT_REQUIRED)
no_autocomplete = True
def __init__(self, request, *args, **kwargs):
super(UpdateUserForm, self).__init__(request, *args, **kwargs)
if api.keystone.keystone_can_edit_user() is False:
for field in ('name', 'email', 'password', 'confirm_password'):
for field in ('name', 'email'):
self.fields.pop(field)
# For keystone V3, display the two fields in read-only
# For keystone V3, display the two fields in read-only
if api.keystone.VERSIONS.active >= 3:
readonlyInput = forms.TextInput(attrs={'readonly': 'readonly'})
self.fields["domain_id"].widget = readonlyInput
self.fields["domain_name"].widget = readonlyInput
# We have to protect the entire "data" dict because it contains the
# password and confirm_password strings.
@sensitive_variables('data', 'password')
def handle(self, request, data):
user = data.pop('id')
# Throw away the password confirmation, we're done with it.
data.pop('confirm_password', None)
data.pop('domain_id')
data.pop('domain_name')
try:
@ -220,3 +211,44 @@ class UpdateUserForm(BaseUserForm):
return response
else:
return True
class ChangePasswordForm(PasswordMixin, forms.SelfHandlingForm):
id = forms.CharField(widget=forms.HiddenInput)
name = forms.CharField(
label=_("User Name"),
widget=forms.TextInput(attrs={'readonly': 'readonly'}),
required=False)
def __init__(self, request, *args, **kwargs):
super(ChangePasswordForm, self).__init__(request, *args, **kwargs)
# Reorder form fields from multiple inheritance
self.fields.keyOrder = ["id", "name", "password", "confirm_password"]
@sensitive_variables('data', 'password')
def handle(self, request, data):
user_id = data.pop('id')
password = data.pop('password')
# Throw away the password confirmation, we're done with it.
data.pop('confirm_password', None)
try:
response = api.keystone.user_update_password(
request, user_id, password)
if user_id == request.user.id:
return utils.logout_with_message(
request,
_('Password changed. Please log in to continue.'),
redirect=False)
messages.success(request,
_('User password has been updated successfully.'))
except Exception:
response = exceptions.handle(request, ignore=True)
messages.error(request, _('Unable to update the user password.'))
if isinstance(response, http.HttpResponse):
return response
else:
return True

16
openstack_dashboard/dashboards/identity/users/tables.py
View File

@ -55,6 +55,19 @@ class EditUserLink(policy.PolicyTargetMixin, tables.LinkAction):
return api.keystone.keystone_can_edit_user()
class ChangePasswordLink(policy.PolicyTargetMixin, tables.LinkAction):
name = "change_password"
verbose_name = _("Change Password")
url = "horizon:identity:users:change_password"
classes = ("ajax-modal",)
icon = "key"
policy_rules = (("identity", "identity:change_password"),)
policy_target_attrs = (("user_id", "id"),)
def allowed(self, request, user):
return api.keystone.keystone_can_edit_user()
class ToggleEnabled(policy.PolicyTargetMixin, tables.BatchAction):
name = "toggle"
@ -228,6 +241,7 @@ class UsersTable(tables.DataTable):
class Meta(object):
name = "users"
verbose_name = _("Users")
row_actions = (EditUserLink, ToggleEnabled, DeleteUsersAction)
row_actions = (EditUserLink, ChangePasswordLink, ToggleEnabled,
DeleteUsersAction)
table_actions = (UserFilterAction, CreateUserLink, DeleteUsersAction)
row_class = UpdateRow

16
openstack_dashboard/dashboards/identity/users/templates/users/_change_password.html Normal file
View File

@ -0,0 +1,16 @@
{% extends "horizon/common/_modal_form.html" %}
{% load i18n %}
{% block modal-body-right %}
<h3>{% trans "Description:" %}</h3>
<p>{% trans "Change user's password. We highly recommend you create a strong one." %}</p>
<script type="text/javascript">
if (typeof horizon.user !== 'undefined') {
horizon.user.init();
} else {
addHorizonLoadEvent(function() {
horizon.user.init();
});
}
</script>
{% endblock %}

7
openstack_dashboard/dashboards/identity/users/templates/users/change_password.html Normal file
View File

@ -0,0 +1,7 @@
{% extends "base.html" %}
{% load i18n %}
{% block title %}{% trans "Change Password" %}{% endblock %}
{% block main %}
{% include "identity/users/_change_password.html" %}
{% endblock %}

91
openstack_dashboard/dashboards/identity/users/tests.py
View File

@ -32,6 +32,8 @@ USERS_INDEX_URL = reverse('horizon:identity:users:index')
USER_CREATE_URL = reverse('horizon:identity:users:create')
USER_UPDATE_URL = reverse('horizon:identity:users:update', args=[1])
USER_DETAIL_URL = reverse('horizon:identity:users:detail', args=[1])
USER_CHANGE_PASSWORD_URL = reverse('horizon:identity:users:change_password',
args=[1])
class UsersViewTests(test.BaseAdminViewTests):
@ -281,11 +283,10 @@ class UsersViewTests(test.BaseAdminViewTests):
'user_update_password',
'user_update',
'roles_for_user', )})
def _update(self, user):
def test_update(self):
user = self.users.get(id="1")
domain_id = user.domain_id
domain = self.domains.get(id=domain_id)
test_password = 'normalpwd'
email = getattr(user, 'email', '')
api.keystone.user_get(IsA(http.HttpRequest), '1',
@ -300,7 +301,6 @@ class UsersViewTests(test.BaseAdminViewTests):
user.id,
email=email,
name=u'test_user',
password=test_password,
project=self.tenant.id).AndReturn(None)
self.mox.ReplayAll()
@ -309,9 +309,7 @@ class UsersViewTests(test.BaseAdminViewTests):
'id': user.id,
'name': user.name,
'email': email,
'password': test_password,
'project': self.tenant.id,
'confirm_password': test_password}
'project': self.tenant.id}
res = self.client.post(USER_UPDATE_URL, formData)
@ -321,7 +319,6 @@ class UsersViewTests(test.BaseAdminViewTests):
'domain_get',
'tenant_list',
'user_update_tenant',
'user_update_password',
'user_update',
'roles_for_user', )})
def test_update_with_no_email_attribute(self):
@ -341,7 +338,6 @@ class UsersViewTests(test.BaseAdminViewTests):
user.id,
email=user.email,
name=user.name,
password=user.password,
project=self.tenant.id).AndReturn(None)
self.mox.ReplayAll()
@ -350,9 +346,7 @@ class UsersViewTests(test.BaseAdminViewTests):
'id': user.id,
'name': user.name,
'email': "",
'password': user.password,
'project': self.tenant.id,
'confirm_password': user.password}
'project': self.tenant.id}
res = self.client.post(USER_UPDATE_URL, formData)
@ -391,63 +385,67 @@ class UsersViewTests(test.BaseAdminViewTests):
self.assertNoFormErrors(res)
self.assertMessageCount(error=1)
@test.create_stubs({api.keystone: ('domain_get',
'user_get',
'tenant_list')})
def test_update_validation_for_password_too_short(self):
user = self.users.get(id="1")
domain_id = user.domain_id
domain = self.domains.get(id=domain_id)
@test.create_stubs({api.keystone: ('user_get',
'user_update_password')})
def test_change_password(self):
user = self.users.get(id="5")
test_password = 'normalpwd'
api.keystone.user_get(IsA(http.HttpRequest), '1',
admin=True).AndReturn(user)
api.keystone.domain_get(IsA(http.HttpRequest), domain_id) \
.AndReturn(domain)
api.keystone.tenant_list(IgnoreArg(), domain=domain_id, user=user.id) \
.AndReturn([self.tenants.list(), False])
api.keystone.user_update_password(IsA(http.HttpRequest),
user.id,
test_password).AndReturn(None)
self.mox.ReplayAll()
formData = {'method': 'UpdateUserForm',
formData = {'method': 'ChangePasswordForm',
'id': user.id,
'name': user.name,
'password': test_password,
'confirm_password': test_password}
res = self.client.post(USER_CHANGE_PASSWORD_URL, formData)
self.assertNoFormErrors(res)
@test.create_stubs({api.keystone: ('user_get',)})
def test_update_validation_for_password_too_short(self):
user = self.users.get(id="1")
api.keystone.user_get(IsA(http.HttpRequest), '1',
admin=True).AndReturn(user)
self.mox.ReplayAll()
formData = {'method': 'ChangePasswordForm',
'id': user.id,
'name': user.name,
'email': user.email,
'password': 't',
'project': self.tenant.id,
'confirm_password': 't'}
res = self.client.post(USER_UPDATE_URL, formData)
res = self.client.post(USER_CHANGE_PASSWORD_URL, formData)
self.assertFormError(
res, "form", 'password',
['Password must be between 8 and 18 characters.'])
@test.create_stubs({api.keystone: ('domain_get',
'user_get',
'tenant_list')})
@test.create_stubs({api.keystone: ('user_get',)})
def test_update_validation_for_password_too_long(self):
user = self.users.get(id="1")
domain_id = user.domain_id
domain = self.domains.get(id=domain_id)
api.keystone.user_get(IsA(http.HttpRequest), '1',
admin=True).AndReturn(user)
api.keystone.domain_get(IsA(http.HttpRequest), domain_id) \
.AndReturn(domain)
api.keystone.tenant_list(IgnoreArg(), domain=domain_id, user=user.id) \
.AndReturn([self.tenants.list(), False])
self.mox.ReplayAll()
formData = {'method': 'UpdateUserForm',
formData = {'method': 'ChangePasswordForm',
'id': user.id,
'name': user.name,
'email': user.email,
'password': 'ThisIsASuperLongPassword',
'project': self.tenant.id,
'confirm_password': 'ThisIsASuperLongPassword'}
res = self.client.post(USER_UPDATE_URL, formData)
res = self.client.post(USER_CHANGE_PASSWORD_URL, formData)
self.assertFormError(
res, "form", 'password',
@ -620,27 +618,20 @@ class SeleniumTests(test.SeleniumAdminTestCase):
self.assertTrue(self._is_element_present("id_confirm_password_error"),
"Couldn't find password error element.")
@test.create_stubs({api.keystone: ('tenant_list',
'user_get',
'domain_get')})
@test.create_stubs({api.keystone: ('user_get',)})
def test_update_user_with_passwords_not_matching(self):
api.keystone.user_get(IsA(http.HttpRequest), '1',
admin=True).AndReturn(self.user)
api.keystone.domain_get(IsA(http.HttpRequest), '1') \
.AndReturn(self.domain)
api.keystone.tenant_list(IgnoreArg(),
domain=self.user.domain_id,
user=self.user.id) \
.AndReturn([self.tenants.list(), False])
self.mox.ReplayAll()
self.selenium.get("%s%s" % (self.live_server_url, USER_UPDATE_URL))
self.selenium.get("%s%s" % (self.live_server_url,
USER_CHANGE_PASSWORD_URL))
self.assertFalse(self._is_element_present("id_confirm_password_error"),
"Password error element shouldn't yet exist.")
self.selenium.find_element_by_id("id_password").send_keys("test")
self.selenium.find_element_by_id("id_confirm_password").send_keys("te")
self.selenium.find_element_by_id("id_email").clear()
self.selenium.find_element_by_id("id_name").click()
self.assertTrue(self._is_element_present("id_confirm_password_error"),
"Couldn't find password error element.")

4
openstack_dashboard/dashboards/identity/users/urls.py
View File

@ -32,4 +32,6 @@ urlpatterns = patterns(
views.UpdateView.as_view(), name='update'),
url(r'^create/$', views.CreateView.as_view(), name='create'),
url(r'^(?P<user_id>[^/]+)/detail/$',
views.DetailView.as_view(), name='detail'))
views.DetailView.as_view(), name='detail'),
url(r'^(?P<user_id>[^/]+)/change_password/$',
views.ChangePasswordView.as_view(), name='change_password'))

42
openstack_dashboard/dashboards/identity/users/views.py
View File

@ -81,8 +81,6 @@ class UpdateView(forms.ModalFormView):
success_url = reverse_lazy('horizon:identity:users:index')
page_title = _("Update User")
@method_decorator(sensitive_post_parameters('password',
'confirm_password'))
def dispatch(self, *args, **kwargs):
return super(UpdateView, self).dispatch(*args, **kwargs)
@ -94,7 +92,7 @@ class UpdateView(forms.ModalFormView):
except Exception:
redirect = reverse("horizon:identity:users:index")
exceptions.handle(self.request,
_('Unable to update user.'),
_('Unable to retrieve user information.'),
redirect=redirect)
def get_context_data(self, **kwargs):
@ -200,3 +198,41 @@ class DetailView(views.HorizonTemplateView):
def get_redirect_url(self):
return reverse('horizon:identity:users:index')
class ChangePasswordView(forms.ModalFormView):
template_name = 'identity/users/change_password.html'
modal_header = _("Change Password")
form_id = "change_user_password_form"
form_class = project_forms.ChangePasswordForm
submit_url = "horizon:identity:users:change_password"
submit_label = _("Save")
success_url = reverse_lazy('horizon:identity:users:index')
page_title = _("Change Password")
@method_decorator(sensitive_post_parameters('password',
'confirm_password'))
def dispatch(self, *args, **kwargs):
return super(ChangePasswordView, self).dispatch(*args, **kwargs)
@memoized.memoized_method
def get_object(self):
try:
return api.keystone.user_get(self.request, self.kwargs['user_id'],
admin=True)
except Exception:
redirect = reverse("horizon:identity:users:index")
exceptions.handle(self.request,
_('Unable to retrieve user information.'),
redirect=redirect)
def get_context_data(self, **kwargs):
context = super(ChangePasswordView, self).get_context_data(**kwargs)
args = (self.kwargs['user_id'],)
context['submit_url'] = reverse(self.submit_url, args=args)
return context
def get_initial(self):
user = self.get_object()
return {'id': self.kwargs['user_id'],
'name': user.name}