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

horizon/dashboards/syspanel/templates/syspanel/users/_update.html

@@ -14,7 +14,7 @@
 </div>
 <div class="right">
     <h3>{% trans "Description" %}:</h3>
-    <p>{% trans "From here you can edit the user by changing their username, email, password, and default project." %}</p>
+    <p>{% trans "From here you can edit the user's details, including their default project." %}</p>
 </div>
 {% endblock %}
 

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,22 +108,33 @@ 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')
+
+        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:
@@ -141,6 +153,7 @@ class UpdateUserForm(BaseUserForm):
             failed.append(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:
@@ -153,15 +166,10 @@ class UpdateUserForm(BaseUserForm):
                     exceptions.handle(request, ignore=True)
 
         if succeeded:
-            succeeded = map(force_unicode, succeeded)
+            messages.success(request, _('User has been updated successfully.'))
-            messages.success(request,
-                             _('Updated %(attributes)s for "%(user)s".')
-                               % {"user": data["name"],
-                                  "attributes": ", ".join(succeeded)})
         if failed:
             failed = map(force_unicode, failed)
             messages.error(request,
-                           _('Unable to update %(attributes)s for "%(user)s".')
+                           _('Unable to update %(attributes)s for the user.')
-                             % {"user": data["name"],
+                             % {"attributes": ", ".join(failed)})
-                                "attributes": ", ".join(failed)})
         return shortcuts.redirect('horizon:syspanel:users:index')

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,
+        table_actions = (UserFilterAction, CreateUserLink, DeleteUsersAction)
-                             DeleteUsersAction)
-        else:
-            row_actions = (EnableUsersAction, DisableUsersAction)
-            table_actions = (UserFilterAction,)

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
+    @test.create_stubs({api: ('tenant_list',)})
-        formData['password'] = 'MoreThanEighteenChars'
+    def test_create_validation_for_password_too_long(self):
-        formData['confirm_password'] = 'MoreThanEighteenChars'
+        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',
@@ -133,84 +160,141 @@ class UsersViewTests(test.BaseAdminViewTests):
         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())
         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
+    @test.create_stubs({api: ('user_get',
-        formData['password'] = 'four'
+                              'tenant_list',
-        formData['confirm_password'] = 'four'
+                              '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.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.'])
 
-        # check password max-len verification
+    @test.create_stubs({api: ('user_get', 'tenant_list')})
-        formData['password'] = 'MoreThanEighteenChars'
+    def test_update_validation_for_password_too_long(self):
-        formData['confirm_password'] = 'MoreThanEighteenChars'
+        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):
+    @test.create_stubs({api.keystone: ('user_list',)})
-        self.mox.StubOutWithMock(api.keystone, 'user_list')
+    def test_disabling_current_user(self):
-        # Four times... one for each post and one for each followed redirect
+        for i in range(0, 2):
-        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())
 
         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)

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: "<class 'django.http.HttpRequest'>"
 
 
+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)

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