From 65824cb8c897ee87488a95a036ebd2dcbf1da85e Mon Sep 17 00:00:00 2001 From: John Postlethwait Date: Wed, 2 May 2012 18:57:00 -0700 Subject: [PATCH] Fixes to the user form, UI behaviors, and tests. The message you got upon successful user update was misleading as it told you all of the fields were updated, even if no changes were made to those fields. Also there was an issue where in this user edit form, where if can_edit_user was False in the local_settings file, you could not change the default project for a user. Changed the stubbing behavior for tests as well to allow the use of a decorator to stub your tests using Mox instead of having to clutter up the actual test with repetitive Mox stubs. Fixes Bug #993572 Change-Id: Iaebd6525bb4509a2ec6b350df432f4f60dd35114 --- .../templates/syspanel/users/_update.html | 2 +- horizon/dashboards/syspanel/users/forms.py | 68 +++--- horizon/dashboards/syspanel/users/tables.py | 19 +- horizon/dashboards/syspanel/users/tests.py | 202 +++++++++++++----- horizon/test.py | 23 ++ horizon/tests/test_data/keystone_data.py | 4 +- 6 files changed, 220 insertions(+), 98 deletions(-) diff --git a/horizon/dashboards/syspanel/templates/syspanel/users/_update.html b/horizon/dashboards/syspanel/templates/syspanel/users/_update.html index 389dc4003e..b324345f03 100644 --- a/horizon/dashboards/syspanel/templates/syspanel/users/_update.html +++ b/horizon/dashboards/syspanel/templates/syspanel/users/_update.html @@ -14,7 +14,7 @@

{% trans "Description" %}:

-

{% trans "From here you can edit the user by changing their username, email, password, and default project." %}

+

{% trans "From here you can edit the user's details, including their default project." %}

{% endblock %} diff --git a/horizon/dashboards/syspanel/users/forms.py b/horizon/dashboards/syspanel/users/forms.py index de5d6ad31c..71de094263 100644 --- a/horizon/dashboards/syspanel/users/forms.py +++ b/horizon/dashboards/syspanel/users/forms.py @@ -39,6 +39,7 @@ class BaseUserForm(forms.SelfHandlingForm): super(BaseUserForm, self).__init__(*args, **kwargs) # Populate tenant choices tenant_choices = [('', _("Select a project"))] + for tenant in api.tenant_list(request, admin=True): if tenant.enabled: tenant_choices.append((tenant.id, tenant.name)) @@ -107,30 +108,41 @@ class UpdateUserForm(BaseUserForm): widget=forms.PasswordInput(render_value=False), regex=validators.password_validator(), required=False, - error_messages={'invalid': validators.password_validator_msg()}) + error_messages={'invalid': + validators.password_validator_msg()}) confirm_password = forms.CharField( label=_("Confirm Password"), widget=forms.PasswordInput(render_value=False), required=False) tenant_id = forms.ChoiceField(label=_("Primary Project")) + def __init__(self, request, *args, **kwargs): + super(UpdateUserForm, self).__init__(request, *args, **kwargs) + + if api.keystone_can_edit_user() == False: + for field in ('name', 'email', 'password', 'confirm_password'): + self.fields.pop(field) + def handle(self, request, data): failed, succeeded = [], [] + user_is_editable = api.keystone_can_edit_user() user = data.pop('id') - password = data.pop('password') tenant = data.pop('tenant_id') - # Discard the extra fields so we can pass kwargs to keystoneclient data.pop('method') - data.pop('confirm_password', None) - # Update user details - msg_bits = (_('name'), _('email')) - try: - api.keystone.user_update(request, user, **data) - succeeded.extend(msg_bits) - except: - failed.extend(msg_bits) - exceptions.handle(request, ignore=True) + if user_is_editable: + password = data.pop('password') + data.pop('confirm_password', None) + + if user_is_editable: + # Update user details + msg_bits = (_('name'), _('email')) + try: + api.keystone.user_update(request, user, **data) + succeeded.extend(msg_bits) + except: + failed.extend(msg_bits) + exceptions.handle(request, ignore=True) # Update default tenant msg_bits = (_('primary project'),) @@ -141,27 +153,23 @@ class UpdateUserForm(BaseUserForm): failed.append(msg_bits) exceptions.handle(request, ignore=True) - # If present, update password - # FIXME(gabriel): password change should be its own form and view - if password: - msg_bits = (_('password'),) - try: - api.user_update_password(request, user, password) - succeeded.extend(msg_bits) - except: - failed.extend(msg_bits) - exceptions.handle(request, ignore=True) + if user_is_editable: + # If present, update password + # FIXME(gabriel): password change should be its own form and view + if password: + msg_bits = (_('password'),) + try: + api.user_update_password(request, user, password) + succeeded.extend(msg_bits) + except: + failed.extend(msg_bits) + exceptions.handle(request, ignore=True) if succeeded: - succeeded = map(force_unicode, succeeded) - messages.success(request, - _('Updated %(attributes)s for "%(user)s".') - % {"user": data["name"], - "attributes": ", ".join(succeeded)}) + messages.success(request, _('User has been updated successfully.')) if failed: failed = map(force_unicode, failed) messages.error(request, - _('Unable to update %(attributes)s for "%(user)s".') - % {"user": data["name"], - "attributes": ", ".join(failed)}) + _('Unable to update %(attributes)s for the user.') + % {"attributes": ", ".join(failed)}) return shortcuts.redirect('horizon:syspanel:users:index') diff --git a/horizon/dashboards/syspanel/users/tables.py b/horizon/dashboards/syspanel/users/tables.py index fe51a3a408..b6a63fa7b8 100644 --- a/horizon/dashboards/syspanel/users/tables.py +++ b/horizon/dashboards/syspanel/users/tables.py @@ -17,6 +17,11 @@ class CreateUserLink(tables.LinkAction): url = "horizon:syspanel:users:create" classes = ("ajax-modal", "btn-create") + def allowed(self, request, user): + if api.keystone_can_edit_user(): + return True + return False + class EditUserLink(tables.LinkAction): name = "edit" @@ -93,7 +98,8 @@ class DeleteUsersAction(tables.DeleteAction): data_type_plural = _("Users") def allowed(self, request, datum): - if datum and datum.id == request.user.id: + if not api.keystone_can_edit_user() or \ + (datum and datum.id == request.user.id): return False return True @@ -134,11 +140,6 @@ class UsersTable(tables.DataTable): class Meta: name = "users" verbose_name = _("Users") - if api.keystone_can_edit_user(): - row_actions = (EditUserLink, EnableUsersAction, DisableUsersAction, - DeleteUsersAction) - table_actions = (UserFilterAction, CreateUserLink, - DeleteUsersAction) - else: - row_actions = (EnableUsersAction, DisableUsersAction) - table_actions = (UserFilterAction,) + row_actions = (EditUserLink, EnableUsersAction, DisableUsersAction, + DeleteUsersAction) + table_actions = (UserFilterAction, CreateUserLink, DeleteUsersAction) diff --git a/horizon/dashboards/syspanel/users/tests.py b/horizon/dashboards/syspanel/users/tests.py index 531493f6d7..8e7614a6c3 100644 --- a/horizon/dashboards/syspanel/users/tests.py +++ b/horizon/dashboards/syspanel/users/tests.py @@ -33,22 +33,25 @@ USER_UPDATE_URL = reverse('horizon:syspanel:users:update', args=[1]) class UsersViewTests(test.BaseAdminViewTests): + @test.create_stubs({api.keystone: ('user_list',)}) def test_index(self): - self.mox.StubOutWithMock(api.keystone, 'user_list') api.keystone.user_list(IgnoreArg()).AndReturn(self.users.list()) + self.mox.ReplayAll() res = self.client.get(USERS_INDEX_URL) + self.assertTemplateUsed(res, 'syspanel/users/index.html') self.assertItemsEqual(res.context['table'].data, self.users.list()) - def test_create_user(self): + @test.create_stubs({api: ('user_create', + 'tenant_list', + 'add_tenant_user_role'), + api.keystone: ('get_default_role',)}) + def test_create(self): user = self.users.get(id="1") role = self.roles.first() - self.mox.StubOutWithMock(api, 'user_create') - self.mox.StubOutWithMock(api, 'tenant_list') - self.mox.StubOutWithMock(api.keystone, 'get_default_role') - self.mox.StubOutWithMock(api, 'add_tenant_user_role') + api.tenant_list(IgnoreArg(), admin=True).AndReturn(self.tenants.list()) api.user_create(IgnoreArg(), user.name, @@ -58,6 +61,7 @@ class UsersViewTests(test.BaseAdminViewTests): True).AndReturn(user) api.keystone.get_default_role(IgnoreArg()).AndReturn(role) api.add_tenant_user_role(IgnoreArg(), self.tenant.id, user.id, role.id) + self.mox.ReplayAll() formData = {'method': 'CreateUserForm', @@ -67,13 +71,16 @@ class UsersViewTests(test.BaseAdminViewTests): 'tenant_id': self.tenant.id, 'confirm_password': user.password} res = self.client.post(USER_CREATE_URL, formData) + self.assertNoFormErrors(res) self.assertMessageCount(success=1) - def test_create_user_password_mismatch(self): + @test.create_stubs({api: ('tenant_list',)}) + def test_create_with_password_mismatch(self): user = self.users.get(id="1") - self.mox.StubOutWithMock(api, 'tenant_list') + api.tenant_list(IgnoreArg(), admin=True).AndReturn(self.tenants.list()) + self.mox.ReplayAll() formData = {'method': 'CreateUserForm', @@ -84,13 +91,15 @@ class UsersViewTests(test.BaseAdminViewTests): 'confirm_password': "doesntmatch"} res = self.client.post(USER_CREATE_URL, formData) + self.assertFormError(res, "form", None, ['Passwords do not match.']) - def test_create_user_field_validation(self): + @test.create_stubs({api: ('tenant_list',)}) + def test_create_validation_for_password_too_short(self): user = self.users.get(id="1") - self.mox.StubOutWithMock(api, 'tenant_list') - api.tenant_list(IgnoreArg(), admin=True).AndReturn(self.tenants.list()) + api.tenant_list(IgnoreArg(), admin=True).AndReturn(self.tenants.list()) + self.mox.ReplayAll() # check password min-len verification @@ -102,27 +111,45 @@ class UsersViewTests(test.BaseAdminViewTests): 'confirm_password': 'four'} res = self.client.post(USER_CREATE_URL, formData) + self.assertFormError( res, "form", 'password', ['Password must be between 8 and 18 characters.']) - # check password max-len verification - formData['password'] = 'MoreThanEighteenChars' - formData['confirm_password'] = 'MoreThanEighteenChars' + @test.create_stubs({api: ('tenant_list',)}) + def test_create_validation_for_password_too_long(self): + user = self.users.get(id="1") + + api.tenant_list(IgnoreArg(), admin=True).AndReturn(self.tenants.list()) + + self.mox.ReplayAll() + + # check password min-len verification + formData = {'method': 'CreateUserForm', + 'name': user.name, + 'email': user.email, + 'password': 'MoreThanEighteenChars', + 'tenant_id': self.tenant.id, + 'confirm_password': 'MoreThanEighteenChars'} res = self.client.post(USER_CREATE_URL, formData) + self.assertFormError( res, "form", 'password', ['Password must be between 8 and 18 characters.']) - def test_update_user_field_validation(self): + @test.create_stubs({api: ('user_get', + 'tenant_list', + 'user_update_tenant', + 'user_update_password'), + api.keystone: ('user_update',)}) + def test_update(self): user = self.users.get(id="1") - self.mox.StubOutWithMock(api, 'tenant_list') - self.mox.StubOutWithMock(api, 'user_get') - self.mox.StubOutWithMock(api.keystone, 'user_update') - self.mox.StubOutWithMock(api, 'user_update_tenant') - self.mox.StubOutWithMock(api, 'user_update_password') + api.user_get(IsA(http.HttpRequest), '1', + admin=True).AndReturn(user) + api.tenant_list(IgnoreArg(), + admin=True).AndReturn(self.tenants.list()) api.keystone.user_update(IsA(http.HttpRequest), user.id, email=u'test@example.com', @@ -131,86 +158,143 @@ class UsersViewTests(test.BaseAdminViewTests): user.id, self.tenant.id).AndReturn(None) api.user_update_password(IsA(http.HttpRequest), - user.id, - IgnoreArg()).AndReturn(None) - api.user_get(IsA(http.HttpRequest), '1', admin=True).AndReturn(user) - api.tenant_list(IgnoreArg(), admin=True).AndReturn(self.tenants.list()) - api.user_get(IsA(http.HttpRequest), '1', admin=True).AndReturn(user) - api.tenant_list(IgnoreArg(), admin=True).AndReturn(self.tenants.list()) - api.user_get(IsA(http.HttpRequest), '1', admin=True).AndReturn(user) - api.tenant_list(IgnoreArg(), admin=True).AndReturn(self.tenants.list()) + user.id, + IgnoreArg()).AndReturn(None) + self.mox.ReplayAll() - user.tenantId = 1 formData = {'method': 'UpdateUserForm', - 'id': 1, + 'id': user.id, 'name': user.name, 'email': user.email, 'password': 'normalpwd', 'tenant_id': self.tenant.id, 'confirm_password': 'normalpwd'} - # check successful update res = self.client.post(USER_UPDATE_URL, formData) + self.assertNoFormErrors(res) - # check password min-len verification - formData['password'] = 'four' - formData['confirm_password'] = 'four' + @test.create_stubs({api: ('user_get', + 'tenant_list', + 'user_update_tenant', + 'keystone_can_edit_user')}) + def test_update_with_keystone_can_edit_user_false(self): + user = self.users.get(id="1") + + api.user_get(IsA(http.HttpRequest), + '1', + admin=True).AndReturn(user) + api.tenant_list(IgnoreArg(), admin=True).AndReturn(self.tenants.list()) + api.keystone_can_edit_user().AndReturn(False) + api.keystone_can_edit_user().AndReturn(False) + api.user_update_tenant(IsA(http.HttpRequest), + user.id, + self.tenant.id).AndReturn(None) + + self.mox.ReplayAll() + + formData = {'method': 'UpdateUserForm', + 'tenant_id': self.tenant.id, + 'id': user.id} res = self.client.post(USER_UPDATE_URL, formData) - self.assertFormError( - res, "form", 'password', - ['Password must be between 8 and 18 characters.']) - # check password max-len verification - formData['password'] = 'MoreThanEighteenChars' - formData['confirm_password'] = 'MoreThanEighteenChars' + self.assertNoFormErrors(res) + + @test.create_stubs({api: ('user_get', 'tenant_list')}) + def test_update_validation_for_password_too_short(self): + user = self.users.get(id="1") + + api.user_get(IsA(http.HttpRequest), '1', + admin=True).AndReturn(user) + api.tenant_list(IgnoreArg(), + admin=True).AndReturn(self.tenants.list()) + + self.mox.ReplayAll() + + formData = {'method': 'UpdateUserForm', + 'id': user.id, + 'name': user.name, + 'email': user.email, + 'password': 't', + 'tenant_id': self.tenant.id, + 'confirm_password': 't'} res = self.client.post(USER_UPDATE_URL, formData) - self.assertFormError( - res, "form", 'password', - ['Password must be between 8 and 18 characters.']) + self.assertFormError( + res, "form", 'password', + ['Password must be between 8 and 18 characters.']) + + @test.create_stubs({api: ('user_get', 'tenant_list')}) + def test_update_validation_for_password_too_long(self): + user = self.users.get(id="1") + + api.user_get(IsA(http.HttpRequest), '1', + admin=True).AndReturn(user) + api.tenant_list(IgnoreArg(), + admin=True).AndReturn(self.tenants.list()) + + self.mox.ReplayAll() + + formData = {'method': 'UpdateUserForm', + 'id': user.id, + 'name': user.name, + 'email': user.email, + 'password': 'ThisIsASuperLongPassword', + 'tenant_id': self.tenant.id, + 'confirm_password': 'ThisIsASuperLongPassword'} + + res = self.client.post(USER_UPDATE_URL, formData) + + self.assertFormError( + res, "form", 'password', + ['Password must be between 8 and 18 characters.']) + + @test.create_stubs({api.keystone: ('user_update_enabled', 'user_list')}) def test_enable_user(self): user = self.users.get(id="2") - self.mox.StubOutWithMock(api.keystone, 'user_update_enabled') - self.mox.StubOutWithMock(api.keystone, 'user_list') + api.keystone.user_list(IgnoreArg()).AndReturn(self.users.list()) api.keystone.user_update_enabled(IgnoreArg(), user.id, True).AndReturn(user) + self.mox.ReplayAll() formData = {'action': 'users__enable__%s' % user.id} res = self.client.post(USERS_INDEX_URL, formData) + self.assertRedirectsNoFollow(res, USERS_INDEX_URL) + @test.create_stubs({api.keystone: ('user_update_enabled', 'user_list')}) def test_disable_user(self): user = self.users.get(id="2") - self.mox.StubOutWithMock(api.keystone, 'user_update_enabled') - self.mox.StubOutWithMock(api.keystone, 'user_list') api.keystone.user_list(IgnoreArg()).AndReturn(self.users.list()) api.keystone.user_update_enabled(IgnoreArg(), user.id, False).AndReturn(user) + self.mox.ReplayAll() formData = {'action': 'users__disable__%s' % user.id} res = self.client.post(USERS_INDEX_URL, formData) + self.assertRedirectsNoFollow(res, USERS_INDEX_URL) + @test.create_stubs({api.keystone: ('user_update_enabled', 'user_list')}) def test_enable_disable_user_exception(self): user = self.users.get(id="2") - self.mox.StubOutWithMock(api.keystone, 'user_update_enabled') - self.mox.StubOutWithMock(api.keystone, 'user_list') + api.keystone.user_list(IgnoreArg()).AndReturn(self.users.list()) api_exception = keystone_exceptions.ClientException('apiException', message='apiException') api.keystone.user_update_enabled(IgnoreArg(), user.id, True).AndRaise(api_exception) + self.mox.ReplayAll() formData = {'action': 'users__enable__%s' % user.id} @@ -218,24 +302,30 @@ class UsersViewTests(test.BaseAdminViewTests): self.assertRedirectsNoFollow(res, USERS_INDEX_URL) - def test_shoot_yourself_in_the_foot(self): - self.mox.StubOutWithMock(api.keystone, 'user_list') - # Four times... one for each post and one for each followed redirect - api.keystone.user_list(IgnoreArg()).AndReturn(self.users.list()) - api.keystone.user_list(IgnoreArg()).AndReturn(self.users.list()) - api.keystone.user_list(IgnoreArg()).AndReturn(self.users.list()) - api.keystone.user_list(IgnoreArg()).AndReturn(self.users.list()) + @test.create_stubs({api.keystone: ('user_list',)}) + def test_disabling_current_user(self): + for i in range(0, 2): + api.keystone.user_list(IgnoreArg()).AndReturn(self.users.list()) self.mox.ReplayAll() formData = {'action': 'users__disable__%s' % self.request.user.id} res = self.client.post(USERS_INDEX_URL, formData, follow=True) + self.assertEqual(list(res.context['messages'])[0].message, u'You cannot disable the user you are currently ' u'logged in as.') + @test.create_stubs({api.keystone: ('user_list',)}) + def test_delete_user_with_improper_permissions(self): + for i in range(0, 2): + api.keystone.user_list(IgnoreArg()).AndReturn(self.users.list()) + + self.mox.ReplayAll() + formData = {'action': 'users__delete__%s' % self.request.user.id} res = self.client.post(USERS_INDEX_URL, formData, follow=True) + self.assertEqual(list(res.context['messages'])[0].message, u'You do not have permission to delete user: %s' % self.request.user.username) diff --git a/horizon/test.py b/horizon/test.py index d56c0f7e7d..c12988d857 100644 --- a/horizon/test.py +++ b/horizon/test.py @@ -27,6 +27,7 @@ from django.conf import settings from django.contrib.messages.storage import default_storage from django.core.handlers import wsgi from django.test.client import RequestFactory +from functools import wraps from glanceclient.v1 import client as glance_client from keystoneclient.v2_0 import client as keystone_client from novaclient.v1_1 import client as nova_client @@ -48,6 +49,28 @@ from .time import utcnow wsgi.WSGIRequest.__repr__ = lambda self: "" +def create_stubs(stubs_to_create={}): + if not isinstance(stubs_to_create, dict): + raise TypeError, ("create_stub must be passed a dict, but a %s was " \ + "given." % type(stubs_to_create).__name__) + + def inner_stub_out(fn): + @wraps(fn) + def instance_stub_out(self): + for key in stubs_to_create: + if not (isinstance(stubs_to_create[key], tuple) or \ + isinstance(stubs_to_create[key], list)): + raise TypeError, ("The values of the create_stub " \ + "dict must be lists or tuples, but is a %s." % + type(stubs_to_create[key]).__name__) + + for value in stubs_to_create[key]: + self.mox.StubOutWithMock(key, value) + return fn(self) + return instance_stub_out + return inner_stub_out + + class RequestFactoryWithMessages(RequestFactory): def get(self, *args, **kwargs): req = super(RequestFactoryWithMessages, self).get(*args, **kwargs) diff --git a/horizon/tests/test_data/keystone_data.py b/horizon/tests/test_data/keystone_data.py index 180b10f6d2..c2f6507d36 100644 --- a/horizon/tests/test_data/keystone_data.py +++ b/horizon/tests/test_data/keystone_data.py @@ -104,11 +104,11 @@ def data(TEST): 'email': 'test@example.com', 'password': 'password', 'token': 'test_token'} - user = users.User(users.UserManager, user_dict) + user = users.User(users.UserManager(None), user_dict) user_dict.update({'id': "2", 'name': 'user_two', 'email': 'two@example.com'}) - user2 = users.User(users.UserManager, user_dict) + user2 = users.User(users.UserManager(None), user_dict) TEST.users.add(user, user2) TEST.user = user # Your "current" user TEST.user.service_catalog = SERVICE_CATALOG