Merge lp:~nealpzhang/juju-ci-tools/assess_multi_users into lp:juju-ci-tools

[Leo Zhang]

Added multi-users' tests

Added tests for multi-users.

[Aaron Bentley (abentley)]

See comments inline.

The big thing is you can't store the user name on the backing state, because the backing state should be shared by all clients.

review: Needs Fixing

[Nicholas Skaggs (nskaggs)]

Added a few more comments.

review: Needs Fixing

[Aaron Bentley (abentley)]

This is improved, but there are still some issues, as noted below.

review: Needs Fixing

[Nicholas Skaggs (nskaggs)]

Again some more comments.

[Nicholas Skaggs (nskaggs)]

More comments. A minor quibble, but I would like to see your assess_* functions be named assert_*. The only assess_* function should be the primary test function in following with project naming.

Also, try to format your code in paragraphs. Feel free to use spacing to logically group statements together. Currently the compressed nature of the code makes it harder to read.

Finally, it would be nice to better frame your assert functions so conditionals are not required. Instead clear expectations of what a user should and should not be able to do (based purely upon there permission levels), which you should pass in. You should run the asserts for all users -- even those who you expect to not be able to perform an action.

[Leo Zhang (nealpzhang)]

Aaron, Nicholas:

New updates after review.

Thanks

[Aaron Bentley (abentley)]

In general, it is confusing to provide "assertion" as a variable name. Assertions are statements, not boolean variables. I think "has_permission" would work as a variable name everywhere you've used "assertion".

Please be clear about whether you're testing model or controller permissions. assert_read is really assert_read_model, assert_write is really assert_write_model, and assert_admin is really assert_admin_controller.

Please use narrow try/except blocks. Don't include anything in the try block unless you expect it to raise the exception. So for example:

        code = ''.join(random.choice(
            string.ascii_letters + string.digits) for _ in xrange(4))
        try:
            client.add_user(permission + code, permissions='read')
        except subprocess.CalledProcessError:

You don't expect random.choice or xrange to raise CalledProcessError, so don't put them in the try block. If they do raise CalledProcessError, something very strange is happening and your test should fail with a traceback.

There's some wiggle room. It would be annoying to have separate try/except blocks for each child.expect() in case it raised pexpect.TIMEOUT, so putting a series of expect/sendline calls together in a single try/except block is fine. But I don't think child.isalive can raise pexpect.TIMEOUT, so it should go outside the try/except block.

It would be nice to check stderr to make sure that it is really a permissions error.

review: Needs Fixing

[Leo Zhang (nealpzhang)]

Aaron/Nicholas,

New updates after review.

Thanks

[Aaron Bentley (abentley)]

Some style changes to fix, but otherwise, I think this is landable.

review: Approve

[Nicholas Skaggs (nskaggs)]

A couple final comments. Please address them, but you have my approval. I don't need to re-review before merging. Thanks for working on this!

I'll note remove user is landing, and this test should be updated to support it as well (but not i this MP).

review: Approve

=== modified file 'assess_container_networking.py'
--- assess_container_networking.py	2016-06-13 20:06:40 +0000
+++ assess_container_networking.py	2016-07-25 20:33:36 +0000
@@ -299,7 +299,7 @@
         d = re.search(r'^default\s+via\s+([\d\.]+)\s+', routes, re.MULTILINE)
         if d:
             rc = client.juju('ssh', ('--proxy', target,
-                             'ping -c1 -q ' + d.group(1)), check=False)
+                                     'ping -c1 -q ' + d.group(1)), check=False)
             if rc != 0:
                 raise ValueError('%s unable to ping default route' % target)
         else:

=== modified file 'assess_unregister.py'
--- assess_unregister.py	2016-06-21 05:59:02 +0000
+++ assess_unregister.py	2016-07-25 20:33:36 +0000
@@ -25,7 +25,6 @@
 
 from assess_user_grant_revoke import (
     User,
-    register_user,
 )
 from deploy_stack import (
     BootstrapManager,
@@ -48,7 +47,7 @@
     user = User('testuser', 'read', [])
     user_controller_name = '{}_controller'.format(user.name)
     with temp_dir() as fake_home:
-        user_client = register_user(user, client, fake_home)
+        user_client = client.register_user(user, fake_home)
         assert_controller_list(
             user_client,
             [user_controller_name])

=== modified file 'assess_user_grant_revoke.py'
--- assess_user_grant_revoke.py	2016-07-06 17:55:43 +0000
+++ assess_user_grant_revoke.py	2016-07-25 20:33:36 +0000
@@ -11,7 +11,11 @@
 
 import argparse
 from collections import namedtuple
+import copy
+import json
 import logging
+import random
+import string
 import subprocess
 import sys
 
@@ -20,12 +24,11 @@
 from deploy_stack import (
     BootstrapManager,
 )
-
-from jujupy import Controller
 from utility import (
     add_basic_testing_arguments,
     configure_logging,
     temp_dir,
+    wait_for_removed_services,
 )
 
 __metaclass__ = type
@@ -36,109 +39,279 @@
 User = namedtuple('User', ['name', 'permissions', 'expect'])
 
 
+user_list_admin = [{"user-name": "admin", "display-name": "admin"}]
+user_list_admin_read = copy.deepcopy(user_list_admin)
+# Created user has no display name, bug 1606354
+user_list_admin_read.append({"user-name": "readuser", "display-name": ""})
+user_list_admin_read_write = copy.deepcopy(user_list_admin_read)
+# bug 1606354
+user_list_admin_read_write.append({"user-name": "writeuser",
+                                   "display-name": ""})
+user_list_all = copy.deepcopy(user_list_admin_read_write)
+# bug 1606354
+user_list_all.append({"user-name": "adminuser", "display-name": ""})
+share_list_admin = {"admin@local": {"display-name": "admin",
+                                    "access": "admin"}}
+share_list_admin_read = copy.deepcopy(share_list_admin)
+share_list_admin_read["readuser@local"] = {"access": "read"}
+share_list_admin_read_write = copy.deepcopy(share_list_admin_read)
+share_list_admin_read_write["writeuser@local"] = {"access": "write"}
+del share_list_admin_read_write['readuser@local']
+share_list_all = copy.deepcopy(share_list_admin_read_write)
+share_list_all["adminuser@local"] = {"access": "admin"}
+share_list_all["writeuser@local"]["access"] = "read"
+
+
 # This needs refactored out to utility
 class JujuAssertionError(AssertionError):
     """Exception for juju assertion failures."""
 
 
-def register_user(user, client, fake_home):
-    """Register `user` for the `client` return the cloned client used."""
-    # needs support to passing register command with arguments
-    # refactor once supported, bug 1573099
+def list_users(client):
+    """Test listing all the users"""
+    users_list = json.loads(client.get_juju_output('list-users', '--format',
+                                                   'json', include_e=False))
+    for user in users_list:
+        # Pop date-created and last-connection if existed for comparison
+        user.pop("date-created", None)
+        user.pop("last-connection", None)
+    return users_list
+
+
+def list_shares(client):
+    """Test listing users' shares"""
+    share_list = json.loads(client.get_juju_output('list-shares', '--format',
+                                                   'json', include_e=False))
+    for key, value in share_list.iteritems():
+        # Pop last-connection if existed for comparison
+        value.pop("last-connection", None)
+    return share_list
+
+
+def show_user(client):
+    """Test showing a user's status"""
+    user_status = json.loads(client.get_juju_output('show-user', '--format',
+                                                    'json', include_e=False))
+    # Pop date-created and last-connection if existed for comparison
+    user_status.pop("date-created", None)
+    user_status.pop("last-connection", None)
+    return user_status
+
+
+def assert_read_model(client, permission, has_permission):
+    """Test if the user has or doesn't have the read permission"""
+    if has_permission:
+        try:
+            client.show_status()
+        except subprocess.CalledProcessError:
+            raise JujuAssertionError(
+                'User could not check status with {} permission'.format(
+                    permission))
+    else:
+        try:
+            client.show_status()
+        except subprocess.CalledProcessError:
+            pass
+        else:
+            raise JujuAssertionError(
+                'User checked status without {} permission'.format(permission))
+
+
+def assert_write_model(client, permission, has_permission):
+    """Test if the user has or doesn't have the write permission"""
+    if has_permission:
+        try:
+            client.deploy('cs:ubuntu')
+        except subprocess.CalledProcessError:
+            raise JujuAssertionError(
+                'User could not deploy with {} permission'.format(permission))
+        else:
+            client.remove_service('ubuntu')
+    else:
+        try:
+            client.deploy('cs:ubuntu')
+        except subprocess.CalledProcessError:
+            pass
+        else:
+            raise JujuAssertionError(
+                'User deployed without {} permission'.format(permission))
+
+
+def assert_admin_controller(client, permission, has_permission):
+    """Test if the user has or doesn't have the admin permission"""
+    if has_permission:
+        code = ''.join(random.choice(
+            string.ascii_letters + string.digits) for _ in xrange(4))
+        try:
+            client.add_user(permission + code, permissions='read')
+        except subprocess.CalledProcessError:
+            raise JujuAssertionError(
+                'User could not add user with {} permission'.format(
+                    permission))
+    else:
+        try:
+            client.add_user(permission + 'false', permissions='read')
+        except subprocess.CalledProcessError:
+            pass
+        else:
+            raise JujuAssertionError(
+                'User added user without {} permission'.format(permission))
+
+
+def assert_user_permissions(user, user_client, controller_client):
+    """Test if users' permissions are within expectations"""
+    expect = iter(user.expect)
+    permission = user.permissions
+    assert_read_model(user_client, permission, expect.next())
+    assert_write_model(user_client, permission, expect.next())
+    assert_admin_controller(user_client, permission, expect.next())
+
+    log.debug("Revoking %s permission from %s" % (user.permissions, user.name))
+    controller_client.revoke(user.name, permissions=user.permissions)
+
+    assert_read_model(user_client, permission, expect.next())
+    assert_write_model(user_client, permission, expect.next())
+    assert_admin_controller(user_client, permission, expect.next())
+
+
+def assert_users_shares(controller_client, client, user):
+    """Test if user_list and share_list are expected"""
+    if user.name == 'readuser':
+        user_list_expected = user_list_admin_read
+        share_list_expected = share_list_admin_read
+    else:
+        user_list_expected = user_list_admin_read_write
+        share_list_expected = share_list_all
+    user_list = list_users(controller_client)
+    share_list = list_shares(controller_client)
+    if sorted(user_list) != sorted(user_list_expected):
+        raise JujuAssertionError(user_list)
+    if sorted(share_list) != sorted(share_list_expected):
+        raise JujuAssertionError(share_list)
+
+
+def assert_change_password(client, user):
+    """Test changing user's password"""
+    try:
+        child = client.expect('change-user-password', (user.name,),
+                              include_e=False)
+        child.expect('(?i)password')
+        child.sendline(user.name + '_password_2')
+        child.expect('(?i)password')
+        child.sendline(user.name + '_password_2')
+        child.expect(pexpect.EOF)
+    except pexpect.TIMEOUT:
+        raise JujuAssertionError(
+            'Changing user password failed: pexpect session timed out')
+    if child.isalive():
+        raise JujuAssertionError(
+            'Changing user password failed: pexpect session still alive')
+
+
+def assert_disable_enable(controller_client, user):
+    """Test disabling and enabling users"""
+    controller_client.disable_user(user.name)
+    user_list = list_users(controller_client)
+    if sorted(user_list) != sorted(user_list_admin_read):
+        raise JujuAssertionError(user_list)
+    controller_client.enable_user(user.name)
+    user_list = list_users(controller_client)
+    if sorted(user_list) != sorted(user_list_admin_read_write):
+        raise JujuAssertionError(user_list)
+
+
+def assert_logout_login(controller_client, user_client, user, fake_home):
+    """Test users' login and logout"""
+    user_client.logout()
+    user_list = list_users(controller_client)
+    if sorted(user_list) != sorted(user_list_admin_read):
+        raise JujuAssertionError(user_list)
     username = user.name
     controller_name = '{}_controller'.format(username)
-    token = client.add_user(username, permissions=user.permissions)
-    user_client, user_env = create_cloned_environment(
-        client, fake_home, controller_name)
-
+    client = controller_client.create_cloned_environment(
+        fake_home, controller_name)
     try:
-        child = user_client.expect(
-            'register', (token), extra_env=user_env, include_e=False)
-        child.expect('(?i)name')
-        child.sendline(controller_name)
-        child.expect('(?i)password')
-        child.sendline(username + '_password')
-        child.expect('(?i)password')
-        child.sendline(username + '_password')
+        child = client.expect('login', (user.name, '-c', controller_name),
+                              include_e=False)
+        child.expect('(?i)password')
+        child.sendline(user.name + '_password_2')
         child.expect(pexpect.EOF)
         if child.isalive():
             raise JujuAssertionError(
-                'Registering user failed: pexpect session still alive')
+                'Login user failed: pexpect session still alive')
     except pexpect.TIMEOUT:
         raise JujuAssertionError(
-            'Registering user failed: pexpect session timed out')
-    return user_client
-
-
-def create_cloned_environment(client, cloned_juju_home, controller_name):
-    user_client = client.clone(env=client.env.clone())
-    user_client.env.juju_home = cloned_juju_home
-    # New user names the controller.
-    user_client.env.controller = Controller(controller_name)
-    user_client_env = user_client._shell_environ()
-    return user_client, user_client_env
-
-
-def assert_read(client, permission):
-    if permission:
-        try:
-            client.show_status()
-        except subprocess.CalledProcessError:
-            raise JujuAssertionError(
-                'User could not check status with read permission')
-    else:
-        try:
-            client.show_status()
-        except subprocess.CalledProcessError:
-            pass
-        else:
-            raise JujuAssertionError(
-                'User checked status without read permission')
-
-
-def assert_write(client, permission):
-    if permission:
-        try:
-            client.deploy('cs:ubuntu')
-        except subprocess.CalledProcessError:
-            raise JujuAssertionError(
-                'User could not deploy with write permission')
-    else:
-        try:
-            client.deploy('cs:ubuntu')
-        except subprocess.CalledProcessError:
-            pass
-        else:
-            raise JujuAssertionError('User deployed without write permission')
-
-
-def assert_user_permissions(user, user_client, admin_client):
-    expect = iter(user.expect)
-    assert_read(user_client, expect.next())
-    assert_write(user_client, expect.next())
-
-    log.debug("Revoking %s permission from %s" % (user.permissions, user.name))
-    admin_client.revoke(user.name, permissions=user.permissions)
-
-    assert_read(user_client, expect.next())
-    assert_write(user_client, expect.next())
-
-
-def assess_user_grant_revoke(admin_client):
-    # Wait for the deployment to finish.
-    admin_client.wait_for_started()
-
+            'Login user failed: pexpect session timed out')
+
+
+def assert_read_user(controller_client, user):
+    """Assess the operations of read user"""
+    with temp_dir() as fake_home:
+        user_client = controller_client.register_user(
+            user, fake_home)
+        user_list = list_users(controller_client)
+        share_list = list_shares(controller_client)
+        if sorted(user_list) != sorted(user_list_admin_read):
+            raise JujuAssertionError(user_list)
+        if sorted(share_list) != sorted(share_list_admin_read):
+            raise JujuAssertionError(share_list)
+        assert_change_password(user_client, user)
+        assert_logout_login(controller_client, user_client, user, fake_home)
+        assert_user_permissions(user, user_client, controller_client)
+
+
+def assert_write_user(controller_client, user):
+    """Assess the operations of write user"""
+    with temp_dir() as fake_home:
+        user_client = controller_client.register_user(
+            user, fake_home)
+        user_list = list_users(controller_client)
+        share_list = list_shares(controller_client)
+        if sorted(user_list) != sorted(user_list_admin_read_write):
+            raise JujuAssertionError(user_list)
+        if sorted(share_list) != sorted(share_list_admin_read_write):
+            raise JujuAssertionError(share_list)
+        assert_disable_enable(controller_client, user)
+        assert_user_permissions(user, user_client, controller_client)
+        wait_for_removed_services(user_client, 'cs:ubuntu')
+
+
+def assert_admin_user(controller_client, user):
+    """Assess the operations of admin user"""
+    with temp_dir() as fake_home:
+        user_client = controller_client.register_user(
+            user, fake_home)
+        user_list = list_users(controller_client)
+        share_list = list_shares(controller_client)
+        if sorted(user_list) != sorted(user_list_all):
+            raise JujuAssertionError(user_list)
+        if sorted(share_list) != sorted(share_list_all):
+            raise JujuAssertionError(share_list)
+        assert_user_permissions(user, user_client, controller_client)
+
+
+def assess_user_grant_revoke(controller_client):
+    """Test multi-users functionality"""
+    controller_client.env.user_name = 'admin'
     log.debug("Creating Users")
-    read_user = User('readuser', 'read', [True, False, False, False])
-    write_user = User('adminuser', 'write', [True, True, True, False])
-    users = [read_user, write_user]
-
-    for user in users:
-        log.debug("Testing %s" % user.name)
-        with temp_dir() as fake_home:
-            user_client = register_user(
-                user, admin_client, fake_home)
-            assert_user_permissions(user, user_client, admin_client)
+    read_user = User('readuser', 'read',
+                     [True, False, False, False, False, False])
+    write_user = User('writeuser', 'write',
+                      [True, True, False, True, False, False])
+    admin_user = User('adminuser', 'admin',
+                      [True, True, True, True, True, True])
+    user_list = list_users(controller_client)
+    share_list = list_shares(controller_client)
+    user_status = show_user(controller_client)
+    if sorted(user_list) != sorted(user_list_admin):
+        raise JujuAssertionError(user_list)
+    if share_list != share_list_admin:
+        raise JujuAssertionError(share_list)
+    if user_status != user_list_admin[0]:
+        raise JujuAssertionError(user_status)
+    assert_read_user(controller_client, read_user)
+    assert_write_user(controller_client, write_user)
+    assert_admin_user(controller_client, admin_user)
 
 
 def parse_args(argv):

=== modified file 'jujupy.py'
--- jujupy.py	2016-07-22 18:12:55 +0000
+++ jujupy.py	2016-07-25 20:33:36 +0000
@@ -215,6 +215,7 @@
 
     def __init__(self, environment, config=None, juju_home=None,
                  controller=None):
+        self.user_name = None
         if controller is None:
             controller = Controller(environment)
         self.controller = controller
@@ -663,7 +664,7 @@
             raise subprocess.CalledProcessError(retcode, full_args)
 
     def get_juju_output(self, command, args, used_feature_flags,
-                        juju_home, model=None, timeout=None):
+                        juju_home, model=None, timeout=None, user_name=None):
         args = self.full_args(command, args, model, timeout)
         env = self.shell_environ(used_feature_flags, juju_home)
         log.debug(args)
@@ -1136,7 +1137,7 @@
         timeout = kwargs.get('timeout')
         return self._backend.get_juju_output(
             command, args, self.used_feature_flags, self.env.juju_home,
-            model, timeout)
+            model, timeout, user_name=self.env.user_name)
 
     def show_status(self):
         """Print the status to output."""
@@ -1807,6 +1808,64 @@
                   (name, provider,
                    'size={}'.format(size)))
 
+    def disable_user(self, user_name):
+        """Disable an user"""
+        self.controller_juju('disable-user', (user_name,))
+
+    def enable_user(self, user_name):
+        """Enable an user"""
+        self.controller_juju('enable-user', (user_name,))
+
+    def logout(self):
+        """Logout an user"""
+        self.controller_juju('logout', ())
+
+    def register_user(self, user, juju_home):
+        """Register `user` for the `client` return the cloned client used."""
+        username = user.name
+        controller_name = '{}_controller'.format(username)
+
+        model = self.env.environment
+        token = self.add_user(username, models=model + ',controller',
+                              permissions=user.permissions)
+        user_client = self.create_cloned_environment(juju_home,
+                                                     controller_name)
+
+        try:
+            child = user_client.expect(
+                'register', (token), include_e=False)
+            child.expect('(?i)name')
+            child.sendline(username + '_controller')
+            child.expect('(?i)password')
+            child.sendline(username + '_password')
+            child.expect('(?i)password')
+            child.sendline(username + '_password')
+            child.expect(pexpect.EOF)
+            if child.isalive():
+                raise Exception(
+                    'Registering user failed: pexpect session still alive')
+        except pexpect.TIMEOUT:
+            raise Exception(
+                'Registering user failed: pexpect session timed out')
+        user_client.env.user_name = username
+        return user_client
+
+    def create_cloned_environment(self, cloned_juju_home, controller_name):
+        """Create a cloned environment"""
+        user_client = self.clone(env=self.env.clone())
+        user_client.env.juju_home = cloned_juju_home
+        # New user names the controller.
+        user_client.env.controller = Controller(controller_name)
+        # user_client_env = user_client._shell_environ()
+        return user_client
+
+    def grant(self, user_name, permission, model=None):
+        """Grant the user with a model."""
+        if model is None:
+            model = self.model_name
+        self.juju('grant', (user_name, model, '--acl', permission),
+                  include_e=False)
+
 
 class EnvJujuClient2B8(EnvJujuClient):
 

=== modified file 'tests/test_assess_unregister.py'
--- tests/test_assess_unregister.py	2016-07-08 13:22:35 +0000
+++ tests/test_assess_unregister.py	2016-07-25 20:33:36 +0000
@@ -63,12 +63,13 @@
 
     def test_unregister(self):
         fake_user = Mock()
-        with patch.object(a_unreg, 'register_user',
-                          return_value=fake_user):
-            with patch.object(a_unreg, 'assert_controller_list',
-                              autospec=True) as mock_assert_list:
-                with patch.object(a_unreg, 'assert_switch_raises_error',
-                                  autospec=True):
+        with patch('jujupy.EnvJujuClient.register_user',
+                   return_value=fake_user):
+            with patch.object(
+                    a_unreg, 'assert_controller_list',
+                    autospec=True) as mock_assert_list:
+                with patch.object(
+                        a_unreg, 'assert_switch_raises_error', autospec=True):
                     fake_client = Mock(wraps=fake_juju_client())
                     fake_client.env.controller.name = 'testing-controller'
                     fake_client.bootstrap()

=== modified file 'tests/test_assess_user_grant_revoke.py'
--- tests/test_assess_user_grant_revoke.py	2016-07-08 17:30:56 +0000
+++ tests/test_assess_user_grant_revoke.py	2016-07-25 20:33:36 +0000
@@ -10,18 +10,14 @@
 import StringIO
 from subprocess import CalledProcessError
 
-import pexpect
-
 from assess_user_grant_revoke import (
-    assert_read,
+    assert_read_model,
     assert_user_permissions,
-    assert_write,
+    assert_write_model,
     assess_user_grant_revoke,
-    create_cloned_environment,
     JujuAssertionError,
     main,
     parse_args,
-    register_user,
 )
 from jujupy import JUJU_DEV_FEATURE_FLAGS
 from tests import (
@@ -112,47 +108,60 @@
 class TestAsserts(TestCase):
 
     def test_assert_user_permissions(self):
-        user = namedtuple('user', ['name', 'permissions', 'expect'])
-        read_user = user('readuser', 'read', [True, False, False, False])
-        write_user = user('adminuser', 'write', [True, True, True, False])
-        users = [read_user, write_user]
+        User = namedtuple('user', ['name', 'permissions', 'expect'])
+        read_user = User('readuser', 'read',
+                         [True, False, False, False, False, False])
+        write_user = User('writeuser', 'write',
+                          [True, True, False, True, False, False])
+        admin_user = User('adminuser', 'admin',
+                          [True, True, True, True, False, False])
+        users = [read_user, write_user, admin_user]
 
         for user in users:
             fake_client = fake_juju_client()
             fake_admin_client = fake_juju_client()
+            ac = patch("assess_user_grant_revoke.assert_admin_controller",
+                       return_value=True)
             with patch("jujupy.EnvJujuClient.revoke", return_value=True):
-                with patch("assess_user_grant_revoke.assert_read",
+                with patch("assess_user_grant_revoke.assert_read_model",
                            return_value=True) as read_mock:
-                    with patch("assess_user_grant_revoke.assert_write",
+                    with patch("assess_user_grant_revoke.assert_write_model",
                                return_value=True) as write_mock:
-                        assert_user_permissions(user, fake_client,
-                                                fake_admin_client)
-                        self.assertEqual(read_mock.call_count, 2)
-                        self.assertEqual(write_mock.call_count, 2)
+                        with ac as admin_mock:
+                            assert_user_permissions(user, fake_client,
+                                                    fake_admin_client)
+                            self.assertEqual(read_mock.call_count, 2)
+                            self.assertEqual(write_mock.call_count, 2)
+                            self.assertEqual(admin_mock.call_count, 2)
 
-    def test_assert_read(self):
+    def test_assert_read_model(self):
         fake_client = fake_juju_client()
         with patch.object(fake_client, 'show_status', return_value=True):
-            assert_read(fake_client, True)
+            assert_read_model(fake_client, 'read', True)
             with self.assertRaises(JujuAssertionError):
-                assert_read(fake_client, False)
+                assert_read_model(fake_client, 'read', False)
         with patch.object(fake_client, 'show_status', return_value=False,
                           side_effect=CalledProcessError(None, None, None)):
-            assert_read(fake_client, False)
+            assert_read_model(fake_client, 'read', False)
             with self.assertRaises(JujuAssertionError):
-                assert_read(fake_client, True)
+                assert_read_model(fake_client, 'read', True)
 
-    def test_assert_write(self):
+    def test_assert_write_model(self):
         fake_client = fake_juju_client()
         with patch.object(fake_client, 'deploy', return_value=True):
-            assert_write(fake_client, True)
+            with patch.object(fake_client, 'remove_service', autospec=True):
+                assert_write_model(fake_client, 'write', True)
             with self.assertRaises(JujuAssertionError):
-                assert_write(fake_client, False)
+                with patch.object(fake_client, 'remove_service',
+                                  autospec=True):
+                    assert_write_model(fake_client, 'write', False)
         with patch.object(fake_client, 'deploy', return_value=False,
                           side_effect=CalledProcessError(None, None, None)):
-            assert_write(fake_client, False)
+            assert_write_model(fake_client, 'write', False)
             with self.assertRaises(JujuAssertionError):
-                assert_write(fake_client, True)
+                with patch.object(fake_client, 'remove_service',
+                                  autospec=True):
+                    assert_write_model(fake_client, 'write', True)
 
 
 def make_fake_client():
@@ -169,70 +178,53 @@
     def test_user_grant_revoke(self):
         fake_client = make_fake_client()
         fake_client.bootstrap()
-
-        user = namedtuple('user', ['name', 'permissions', 'expect'])
-        read_user = user('readuser', 'read', [True, False, False, False])
-        write_user = user('adminuser', 'write', [True, True, True, False])
-
-        with patch("assess_user_grant_revoke.register_user",
-                   return_value=True) as reg_mock:
-            with patch("assess_user_grant_revoke.assert_user_permissions",
-                       autospec=True) as perm_mock:
-                assess_user_grant_revoke(fake_client)
-
-                self.assertEqual(reg_mock.call_count, 2)
-                self.assertEqual(perm_mock.call_count, 2)
-
-                read_user_call, write_user_call = perm_mock.call_args_list
-                read_user_args, read_user_kwargs = read_user_call
-                write_user_args, write_user_kwargs = write_user_call
-
-                self.assertEqual(read_user_args[0], read_user)
-                self.assertEqual(read_user_args[2], fake_client)
-                self.assertEqual(write_user_args[0], write_user)
-                self.assertEqual(write_user_args[2], fake_client)
-
-    def test_create_cloned_environment(self):
-        fake_client = make_fake_client()
-        fake_client.bootstrap()
-        fake_client_environ = fake_client._shell_environ()
-        controller_name = 'user_controller'
-        cloned, cloned_environ = create_cloned_environment(
-            fake_client,
-            'fakehome',
-            controller_name
-        )
-        self.assertIs(fake_client.__class__, type(cloned))
-        self.assertEqual(cloned.env.juju_home, 'fakehome')
-        self.assertNotEqual(cloned_environ, fake_client_environ)
-        self.assertEqual(cloned_environ['JUJU_DATA'], 'fakehome')
-        self.assertEqual(cloned.env.controller.name, controller_name)
-        self.assertEqual(fake_client.env.controller.name, 'name')
-
-    def test_register_user(self):
-        FakeUser = namedtuple('user', ['name', 'permissions'])
-        user = FakeUser('fakeuser', 'read')
-
-        class FakeClient:
-            """Lightweight fake client for testing."""
-            def add_user(self, username, permissions):
-                return 'token'
-
-            def expect(self, *args, **kwargs):
-                return PexpectInteraction(
-                    [
-                        user.name + '_controller',
-                        user.name + '_password',
-                        user.name + '_password',
-                        pexpect.EOF],
-                    [
-                        '(?i)name',
-                        '(?i)password',
-                        '(?i)password',
-                        pexpect.EOF])
-
-        fake_client = FakeClient()
-        with patch(
-                'assess_user_grant_revoke.create_cloned_environment',
-                return_value=(fake_client, {})):
-            register_user(user, fake_client, '/tmp/dir/path')
+        assert_read_calls = [True, False, True, True, True, True]
+        assert_write_calls = [False, False, True, False, True, True]
+        assert_admin_calls = [False, False, False, False, True, True]
+        cpass = patch("assess_user_grant_revoke.assert_change_password",
+                      autospec=True)
+        able = patch("assess_user_grant_revoke.assert_disable_enable",
+                     autospec=True)
+        log = patch("assess_user_grant_revoke.assert_logout_login",
+                    autospec=True)
+        expect = patch("jujupy.EnvJujuClient.expect",
+                       autospec=True)
+        read = patch("assess_user_grant_revoke.assert_read_model",
+                     autospec=True)
+        write = patch("assess_user_grant_revoke.assert_write_model",
+                      autospec=True)
+        admin = patch("assess_user_grant_revoke.assert_admin_controller",
+                      autospec=True)
+        rm = patch("utility.wait_for_removed_services",
+                   autospec=True)
+        with cpass as pass_mock, able as able_mock, log as log_mock:
+            with read as read_mock, write as write_mock, admin as admin_mock:
+                with expect as expect_mock:
+                    expect_mock.return_value.isalive.return_value = False
+                    with rm:
+                        assess_user_grant_revoke(fake_client)
+
+                        self.assertEqual(pass_mock.call_count, 1)
+                        self.assertEqual(able_mock.call_count, 1)
+                        self.assertEqual(log_mock.call_count, 1)
+
+                        self.assertEqual(read_mock.call_count, 6)
+                        self.assertEqual(write_mock.call_count, 6)
+                        self.assertEqual(admin_mock.call_count, 6)
+
+                        read_calls = [
+                            call[0][2] for call in
+                            read_mock.call_args_list]
+                        write_calls = [
+                            call[0][2] for call in
+                            write_mock.call_args_list]
+                        admin_calls = [
+                            call[0][2] for call in
+                            admin_mock.call_args_list]
+
+                        self.assertEqual(read_calls,
+                                         assert_read_calls)
+                        self.assertEqual(write_calls,
+                                         assert_write_calls)
+                        self.assertEqual(admin_calls,
+                                         assert_admin_calls)

=== modified file 'tests/test_jujupy.py'
--- tests/test_jujupy.py	2016-07-25 18:08:15 +0000
+++ tests/test_jujupy.py	2016-07-25 20:33:36 +0000
@@ -127,6 +127,13 @@
     def __init__(self):
         self.state = 'not-bootstrapped'
         self.models = {}
+        self.users = {
+            'admin': {
+                'state': '',
+                'permission': 'write'
+            }
+        }
+        self.shares = ['admin']
 
     def add_model(self, name):
         state = FakeEnvironmentState()
@@ -471,6 +478,45 @@
                        self.controller_state.models.values()]
         return {'models': [{'name': n} for n in model_names]}
 
+    def list_users(self):
+        user_names = [name for name in
+                      self.controller_state.users.keys()]
+        user_list = []
+        for n in user_names:
+            if n == 'admin':
+                append_dict = {'user-name': n, 'display-name': n}
+            else:
+                append_dict = {'user-name': n, 'display-name': ''}
+            user_list.append(append_dict)
+        return user_list
+
+    def show_user(self, user_name):
+        if user_name is None:
+            raise Exception("No user specified")
+        if user_name == 'admin':
+            user_status = {'user-name': user_name, 'display-name': user_name}
+        else:
+            user_status = {'user-name': user_name, 'display-name': ''}
+        return user_status
+
+    def list_shares(self):
+        share_names = self.controller_state.shares
+        permissions = []
+        for key, value in self.controller_state.users.iteritems():
+            if key in share_names:
+                permissions.append(value['permission'])
+        share_list = {}
+        for i, (share_name, permission) in enumerate(
+                zip(share_names, permissions)):
+            name = share_name + '@local'
+            share_list[name] = {'display-name': share_name,
+                                'access': permission}
+            if name != 'admin@local':
+                share_list[name].pop('display-name')
+            else:
+                share_list[name]['access'] = 'admin'
+        return share_list
+
     def _log_command(self, command, args, model, level=logging.INFO):
         full_args = ['juju', command]
         if model is not None:
@@ -566,6 +612,16 @@
                 parser.add_argument('model_name')
                 parsed = parser.parse_args(args)
                 self.controller_state.add_model(parsed.model_name)
+            if command == 'revoke':
+                user_name = args[2]
+                permissions = args[5]
+                per = self.controller_state.users[user_name]['permission']
+                if per == permissions:
+                    if permissions == 'read':
+                        self.controller_state.shares.remove(user_name)
+                        per = ''
+                    else:
+                        per = 'read'
 
     @contextmanager
     def juju_async(self, command, args, used_feature_flags,
@@ -576,7 +632,7 @@
 
     @check_juju_output
     def get_juju_output(self, command, args, used_feature_flags,
-                        juju_home, model=None, timeout=None):
+                        juju_home, model=None, timeout=None, user_name=None):
         if 'service' in command:
             raise Exception('No service')
         self._log_command(command, args, model, logging.DEBUG)
@@ -597,6 +653,12 @@
             return yaml.safe_dump(self.make_controller_dict(args[0]))
         if command == 'list-models':
             return yaml.safe_dump(self.list_models())
+        if command == 'list-users':
+            return json.dumps(self.list_users())
+        if command == 'list-shares':
+            return json.dumps(self.list_shares())
+        if command == 'show-user':
+            return json.dumps(self.show_user(user_name))
         if command == 'add-user':
             permissions = 'read'
             if set(["--acl", "write"]).issubset(args):
@@ -606,6 +668,9 @@
             info_string = \
                 'User "{}" added\nUser "{}"granted {} access to model "{}\n"' \
                 .format(username, username, permissions, model)
+            self.controller_state.users.update(
+                {username: {'state': '', 'permission': permissions}})
+            self.controller_state.shares.append(username)
             register_string = get_user_register_command_info(username)
             return info_string + register_string
         if command == 'show-status':
@@ -3045,7 +3110,8 @@
         gjo_mock.assert_called_once_with(
             'run', ('--format', 'json', '--application', 'foo,bar', 'wname'),
             frozenset(
-                ['address-allocation', 'migration']), 'foo', 'name:name', None)
+                ['address-allocation', 'migration']), 'foo',
+            'name:name', None, user_name=None)
 
     def test_list_space(self):
         client = EnvJujuClient(JujuData(None, {'type': 'local'}),
@@ -3248,6 +3314,46 @@
             get_output.assert_called_with(
                 'add-user', *expected_args, include_e=False)
 
+    def test_disable_user(self):
+        env = JujuData('foo')
+        username = 'fakeuser'
+        client = EnvJujuClient(env, None, None)
+        with patch.object(client, 'juju') as mock:
+            client.disable_user(username)
+        mock.assert_called_with(
+            'disable-user', ('-c', 'foo', 'fakeuser'), include_e=False)
+
+    def test_enable_user(self):
+        env = JujuData('foo')
+        username = 'fakeuser'
+        client = EnvJujuClient(env, None, None)
+        with patch.object(client, 'juju') as mock:
+            client.enable_user(username)
+        mock.assert_called_with(
+            'enable-user', ('-c', 'foo', 'fakeuser'), include_e=False)
+
+    def test_logout(self):
+        env = JujuData('foo')
+        client = EnvJujuClient(env, None, None)
+        with patch.object(client, 'juju') as mock:
+            client.logout()
+        mock.assert_called_with(
+            'logout', ('-c', 'foo'), include_e=False)
+
+    def test_create_cloned_environment(self):
+        fake_client = fake_juju_client()
+        fake_client.bootstrap()
+        # fake_client_environ = fake_client._shell_environ()
+        controller_name = 'user_controller'
+        cloned = fake_client.create_cloned_environment(
+            'fakehome',
+            controller_name
+        )
+        self.assertIs(fake_client.__class__, type(cloned))
+        self.assertEqual(cloned.env.juju_home, 'fakehome')
+        self.assertEqual(cloned.env.controller.name, controller_name)
+        self.assertEqual(fake_client.env.controller.name, 'name')
+
 
 class TestEnvJujuClient2B8(ClientTest):
 
@@ -5070,7 +5176,8 @@
         gjo_mock.assert_called_once_with(
             'run', ('--format', 'json', '--service', 'foo,bar', 'wname'),
             frozenset(
-                ['address-allocation', 'migration']), 'foo', 'name', None)
+                ['address-allocation', 'migration']),
+            'foo', 'name', None, user_name=None)
 
     def test_list_space(self):
         client = EnvJujuClient1X(SimpleEnvironment(None, {'type': 'local'}),