Merge lp:~nskaggs/juju-ci-tools/remove-controller-permissions into lp:juju-ci-tools

[Nicholas Skaggs]

This removes the unused controller permissions, and incorporates some of the ideas into user grant revoke. I also took the opportunity to clean up the readability and extensibility of the user grant revoke test. More could be done, but this should make future work easier.

[lp:~nskaggs/juju-ci-tools/remove-controller-permissions]

1959. By Nicholas Skaggs on 2017-03-27

update operations, fix test

1960. By Nicholas Skaggs on 2017-03-27

fixup readability

1961. By Nicholas Skaggs on 2017-03-27

change read commands

1962. By Nicholas Skaggs on 2017-03-27

fix disable function

[Christopher Lee (veebers)]

I have some minor issues with a couple of things but not enough to block landing. It does improve the readability of the test.

Please take a look at my questions/suggestions.

review: Approve

[lp:~nskaggs/juju-ci-tools/remove-controller-permissions]

1963. By Nicholas Skaggs on 2017-03-28

Updates from review comments

[Nicholas Skaggs (nskaggs)]

Couldn't help myself Chris, I incorporated all your suggestions and then some. It's much nicer. Thanks.

=== removed file 'assess_controller_permissions.py'
--- assess_controller_permissions.py	2016-09-13 21:33:20 +0000
+++ assess_controller_permissions.py	1970-01-01 00:00:00 +0000
@@ -1,217 +0,0 @@
-#!/usr/bin/env python
-"""TODO: add rough description of what is assessed in this module."""
-
-from __future__ import print_function
-
-import argparse
-import logging
-import random
-import string
-import subprocess
-import sys
-
-from assess_user_grant_revoke import (
-    assert_change_password,
-    assert_logout_login,
-    list_users,
-    User,
-    )
-from deploy_stack import (
-    BootstrapManager,
-    )
-from utility import (
-    JujuAssertionError,
-    add_basic_testing_arguments,
-    configure_logging,
-    temp_dir,
-    )
-
-
-__metaclass__ = type
-
-
-log = logging.getLogger("assess_controller_permissions")
-
-
-def assert_add_model(user_client, permission):
-    """Test user's ability of adding models."""
-    try:
-        user_client.add_model(user_client.env)
-    except subprocess.CalledProcessError:
-        raise JujuAssertionError(
-            "Controller can't add model with {} permission".format(permission))
-
-
-def assert_destroy_model(user_client, permission):
-    """Test user's ability of destroying models."""
-    try:
-        user_client.destroy_model()
-    except subprocess.CalledProcessError:
-        raise JujuAssertionError(
-            "Controller can't destroy model with {} permission".format(
-                permission))
-
-
-def assert_add_remove_user(user_client, permission):
-    """Test user's ability of adding/removing users."""
-    for controller_permission in ['login', 'addmodel', 'superuser']:
-        code = ''.join(random.choice(
-            string.ascii_letters + string.digits) for _ in xrange(4))
-        try:
-            user_client.add_user_perms(permission + code,
-                                       permissions=controller_permission)
-        except subprocess.CalledProcessError:
-            raise JujuAssertionError(
-                'Controller could not add '
-                '{} controller with {} permission'.format(
-                    controller_permission, permission))
-        try:
-            user_client.remove_user(permission + code,
-                                    permissions=controller_permission)
-        except subprocess.CalledProcessError:
-            raise JujuAssertionError(
-                'Controller could not remove '
-                '{} controller with {} permission'.format(
-                    controller_permission, permission))
-
-
-def assert_lists(user_client):
-    """Test user's ability of retrieving lists."""
-    list_users(user_client)
-    user_client.list_models()
-    user_client.list_clouds()
-    user_client.show_controller()
-
-
-def assert_login_permission(controller_client, user_client,
-                            user, fake_home, has_permission):
-    """Test user's ability with login permission."""
-    if has_permission:
-        try:
-            assert_logout_login(controller_client, user_client,
-                                user, fake_home)
-            assert_change_password(user_client, user)
-            assert_lists(user_client)
-        except subprocess.CalledProcessError:
-            raise JujuAssertionError(
-                'FAIL {} could not login/read with {} permission'.format(
-                    user.name, user.permissions))
-    else:
-        try:
-            assert_logout_login(controller_client, user_client,
-                                user, fake_home)
-            assert_change_password(user_client, user)
-            assert_lists(user_client)
-        except subprocess.CalledProcessError:
-            log.info('Correctly rejected {} use of login/read'.format(
-                user.name))
-        else:
-            raise JujuAssertionError(
-                'FAIL User login/read without login permission')
-
-
-def assert_addmodel_permission(user_client, user, has_permission):
-    """Test user's ability with addmodel permission."""
-    if has_permission:
-        try:
-            assert_add_model(user_client, user.permissions)
-            assert_destroy_model(user_client, user.permissions)
-        except subprocess.CalledProcessError:
-            raise JujuAssertionError(
-                'FAIL {} could not add/remove'
-                ' models with {} permission'.format(
-                    user.name, user.permissions))
-    else:
-        try:
-            assert_add_model(user_client, user.permissions)
-            assert_destroy_model(user_client, user.permissions)
-        except subprocess.CalledProcessError:
-            log.info('Correctly rejected {} use of add/remove model'.format(
-                user.name))
-        else:
-            raise JujuAssertionError(
-                'FAIL User added/removed models without addmodel permission')
-
-
-def assert_superuser_permission(user_client, user, has_permission):
-    """Test user's ability with superuser permission."""
-    if has_permission:
-        try:
-            assert_add_remove_user(user_client, user.permissions)
-        except subprocess.CalledProcessError:
-            raise JujuAssertionError(
-                'FAIL {} could not add/remove users with {} permission'.format(
-                    user.name, user.permissions))
-    else:
-        try:
-            assert_add_remove_user(user_client, user.permissions)
-        except subprocess.CalledProcessError:
-            log.info('Correctly rejected {} use of add/remove users'.format(
-                user.name))
-        else:
-            raise JujuAssertionError(
-                'FAIL User added/removed users without superuser permission')
-
-
-def assert_login_controller(controller_client, user):
-    """Test user with login controller permission."""
-    with temp_dir() as fake_home:
-        user_client = controller_client.register_user(
-            user, fake_home)
-        assert_login_permission(controller_client, user_client,
-                                user, fake_home, True)
-        assert_addmodel_permission(user_client, user, False)
-        assert_superuser_permission(user_client, user, False)
-
-
-def assert_addmodel_controller(controller_client, user):
-    """Test user with addmodel controller permission."""
-    with temp_dir() as fake_home:
-        user_client = controller_client.register_user(
-            user, fake_home)
-        assert_login_permission(controller_client, user_client,
-                                user, fake_home, True)
-        assert_addmodel_permission(user_client, user, True)
-        assert_superuser_permission(user_client, user, False)
-
-
-def assert_superuser_controller(controller_client, user):
-    """Test user with superuser controller permission."""
-    with temp_dir() as fake_home:
-        user_client = controller_client.register_user(
-            user, fake_home)
-        assert_login_permission(controller_client, user_client,
-                                user, fake_home, True)
-        assert_addmodel_permission(user_client, user, True)
-        assert_superuser_permission(user_client, user, True)
-
-
-def assess_controller_permissions(controller_client):
-    """Test controller permissions."""
-    login_controller = User('login_controller', 'login', [])
-    addmodel_controller = User('addmodel_controller', 'addmodel', [])
-    superuser_controller = User('superuser_controller', 'superuser', [])
-    assert_login_controller(controller_client, login_controller)
-    assert_addmodel_controller(controller_client, addmodel_controller)
-    assert_superuser_controller(controller_client, superuser_controller)
-
-
-def parse_args(argv):
-    """Parse all arguments."""
-    parser = argparse.ArgumentParser(
-        description="Test controller permissions.")
-    add_basic_testing_arguments(parser)
-    return parser.parse_args(argv)
-
-
-def main(argv=None):
-    args = parse_args(argv)
-    configure_logging(args.verbose)
-    bs_manager = BootstrapManager.from_args(args)
-    with bs_manager.booted_context(args.upload_tools):
-        assess_controller_permissions(bs_manager.client)
-    return 0
-
-
-if __name__ == '__main__':
-    sys.exit(main())

=== modified file 'assess_user_grant_revoke.py'
--- assess_user_grant_revoke.py	2017-03-23 21:22:49 +0000
+++ assess_user_grant_revoke.py	2017-03-28 14:43:16 +0000
@@ -62,6 +62,12 @@
 SHARE_LIST_CTRL_ADMIN["adminuser"] = {"access": "admin"}
 
 
+def _generate_random_string():
+    # We prefix a letter because usernames must begin with letter
+    return 'r'.join(random.choice(
+        string.ascii_letters + string.digits) for _ in range(9))
+
+
 def assert_equal(found, expected):
     found = sorted(found)
     expected = sorted(expected)
@@ -70,6 +76,26 @@
             'Found: {}\nExpected: {}'.format(found, expected))
 
 
+def assert_command_fails(check_callable, command_type, permission):
+    try:
+        check_callable()
+    except subprocess.CalledProcessError:
+        pass
+    else:
+        raise JujuAssertionError(
+            'FAIL User performed {} operation with '
+            'permission {}'.format(command_type, permission))
+
+
+def assert_command_succeeds(check_callable, command_type, permission):
+    try:
+        check_callable()
+    except subprocess.CalledProcessError:
+        raise JujuAssertionError(
+            'FAIL User unable to perform {} operation with '
+            'permission {}'.format(command_type, permission))
+
+
 def list_users(client):
     """Test listing all the users"""
     users_list = json.loads(client.get_juju_output('list-users', '--format',
@@ -100,80 +126,69 @@
     return user_status
 
 
+def assess_read_operations(client, permission, has_permission):
+    read_commands = (
+        client.show_status,
+        lambda: client.juju('show-model', (), include_e=False),
+    )
+
+    for command in read_commands:
+        if has_permission:
+            assert_command_succeeds(command, 'read', permission)
+        else:
+            assert_command_fails(command, 'read', permission)
+
+
+def assess_write_operations(client, permission, has_permission):
+    tags = '"{}={}"'.format(client.env.user_name, permission)
+    write_commands = (
+        lambda: client.set_env_option('resource-tags', tags),
+    )
+
+    for command in write_commands:
+        if has_permission:
+            assert_command_succeeds(command, 'write', permission)
+        else:
+            assert_command_fails(command, 'write', permission)
+
+
+def assess_admin_operations(client, permission, has_permission):
+    # Create a username for the user client to interact with
+    new_read_user = _generate_random_string()
+    new_admin_user = _generate_random_string()
+    admin_commands = (
+        lambda: client.add_user(new_read_user),
+        lambda: client.grant(new_read_user, permission="read"),
+        lambda: client.remove_user(new_read_user),
+        lambda: client.add_user_perms(new_admin_user, permissions="admin"),
+        lambda: client.remove_user(new_admin_user),
+    )
+
+    for command in admin_commands:
+        if has_permission:
+            assert_command_succeeds(command, 'admin', permission)
+        else:
+            assert_command_fails(command, 'admin', permission)
+
+
 def assert_read_model(client, permission, has_permission):
     """Test if the user has or doesn't have the read permission"""
     log.info('Checking read model acl {}'.format(client.env.user_name))
-    if has_permission:
-        try:
-            client.show_status()
-        except subprocess.CalledProcessError:
-            raise JujuAssertionError(
-                'FAIL User could not check status with {} permission'.format(
-                    permission))
-    else:
-        try:
-            client.show_status()
-        except subprocess.CalledProcessError:
-            pass
-        else:
-            raise JujuAssertionError(
-                'FAIL {} checked status without {} permission'.format(
-                    client.env.user_name, permission))
+    assess_read_operations(client, permission, has_permission)
     log.info('PASS {} read acl'.format(client.env.user_name))
 
 
 def assert_write_model(client, permission, has_permission):
     """Test if the user has or doesn't have the write permission"""
     log.info('Checking write model acl {}'.format(client.env.user_name))
-    if has_permission:
-        try:
-            tags = '"{}={}"'.format(client.env.user_name, permission)
-            client.set_env_option('resource-tags', tags)
-        except subprocess.CalledProcessError:
-            raise JujuAssertionError(
-                'FAIL {} could not set-model-config with {} permission'.format(
-                    client.env.user_name, permission))
-    else:
-        try:
-            tags = '"{}=no-{}"'.format(client.env.user_name, permission)
-            client.set_env_option('resource-tags', tags)
-        except subprocess.CalledProcessError:
-            pass
-        else:
-            raise JujuAssertionError(
-                'FAIL User set model-config without {} permission'.format(
-                    permission))
+    assess_write_operations(client, permission, has_permission)
     log.info('PASS {} write model acl'.format(client.env.user_name))
 
 
 def assert_admin_model(controller_client, client, permission, has_permission):
     """Test if the user has or doesn't have the admin permission"""
     log.info('Checking admin acl with {}'.format(client.env.user_name))
-    code = ''.join(random.choice(
-        string.ascii_letters + string.digits) for _ in range(4))
-    new_user = permission + code
-    log.info('Adding user {} for test'.format(new_user))
-    controller_client.add_user_perms(new_user, permissions="read")
-    if has_permission:
-        try:
-            client.grant(new_user, permission="write")
-        except subprocess.CalledProcessError:
-            raise JujuAssertionError(
-                'FAIL {} could not grant write acl to user'.format(
-                    client.env.user_name, permission))
-    else:
-        try:
-            client.grant(new_user, permission="write")
-        except subprocess.CalledProcessError:
-            log.info('Correctly rejected {} use of grant'.format(
-                client.env.user_name))
-        else:
-            raise JujuAssertionError(
-                'FAIL {} granted access without {} permission'.format(
-                    client.env.user_name, permission))
-    # Remove the user to ensure list-users is sane.
-    log.info('Removing user {} after test'.format(new_user))
-    controller_client.remove_user(new_user)
+    assess_admin_operations(client, permission, has_permission)
     log.info('PASS {} admin acl'.format(client.env.user_name))
 
 
@@ -207,15 +222,10 @@
         child.sendline(password)
         child.expect('(?i)password')
         child.sendline(password)
-        child.expect(pexpect.EOF)
+        client._end_pexpect_session(child)
     except pexpect.TIMEOUT:
-        raise JujuAssertionError(
-            'FAIL Changing user password failed: pexpect session timed out')
-    if child.isalive():
-        raise JujuAssertionError(
-            'FAIL Changing user password failed: pexpect session still alive')
-    child.close()
-    if child.exitstatus != 0:
+        log.error('Buffer: {}'.format(child.buffer))
+        log.error('Before: {}'.format(child.before))
         raise JujuAssertionError(
             'FAIL Changing user password failed: '
             'pexpect process exited with {}'.format(child.exitstatus))
@@ -224,6 +234,7 @@
 
 def assert_disable_enable(controller_client, user):
     """Test disabling and enabling users"""
+    original_user_list = list_users(controller_client)
     log.info('Checking disabled {}'.format(user.name))
     controller_client.disable_user(user.name)
     log.info('Disabled {}'.format(user.name))
@@ -235,18 +246,31 @@
     log.info('Enabled {}'.format(user.name))
     user_list = list_users(controller_client)
     log.info('Checking list-users {}'.format(user.name))
-    assert_equal(user_list, USER_LIST_CTRL_WRITE)
-
-
-def assert_logout_login(controller_client, user_client,
-                        user, fake_home, password):
+    assert_equal(user_list, original_user_list)
+
+
+def assert_user_status(client, user, expected_users, expected_shares):
+    """Test listing users and shares against expected values"""
+    log.info('Checking list-users {}'.format(user.name))
+    user_list = list_users(client)
+    assert_equal(user_list, expected_users)
+    log.info('Checking list-shares {}'.format(user.name))
+    share_list = list_shares(client)
+    assert_equal(share_list, expected_shares)
+
+
+def assert_logout_login(controller_client, user_client, user,
+                        fake_home, password, expected_users):
     """Test users' login and logout"""
+    original_user_list = list_users(controller_client)
     user_client.logout()
     log.info('Checking list-users after logout')
     user_list = list_users(controller_client)
-    assert_equal(user_list, USER_LIST_CTRL_READ)
+    assert_equal(user_list, expected_users)
     log.info('Checking list-users after login')
     user_client.login_user(user.name, password)
+    user_list = list_users(controller_client)
+    assert_equal(user_list, original_user_list)
 
 
 def assert_read_user(controller_client, user):
@@ -255,19 +279,16 @@
     with temp_dir() as fake_home:
         user_client = controller_client.register_user(
             user, fake_home)
-        log.info('Checking list-users {}'.format(user.name))
-        user_list = list_users(controller_client)
-        assert_equal(user_list, USER_LIST_CTRL_READ)
-        log.info('Checking list-shares {}'.format(user.name))
-        share_list = list_shares(controller_client)
-        assert_equal(share_list, SHARE_LIST_CTRL_READ)
+        user_client.env.user_name = user.name
+        assert_user_status(controller_client, user,
+                           USER_LIST_CTRL_READ, SHARE_LIST_CTRL_READ)
 
-        password = ''.join(random.choice(
-            string.ascii_letters + string.digits) for _ in range(10))
+        password = _generate_random_string()
         assert_change_password(user_client, user, password)
         assert_logout_login(controller_client, user_client,
-                            user, fake_home, password)
+                            user, fake_home, password, USER_LIST_CTRL_READ)
         assert_user_permissions(user, user_client, controller_client)
+        assert_disable_enable(controller_client, user)
         controller_client.remove_user(user.name)
     log.info('PASS read {}'.format(user.name))
 
@@ -279,14 +300,15 @@
         user_client = controller_client.register_user(
             user, fake_home)
         user_client.env.user_name = user.name
-        log.info('Checking list-users {}'.format(user.name))
-        user_list = list_users(controller_client)
-        assert_equal(user_list, USER_LIST_CTRL_WRITE)
-        log.info('Checking list-shares {}'.format(user.name))
-        share_list = list_shares(controller_client)
-        assert_equal(share_list, SHARE_LIST_CTRL_WRITE)
+        assert_user_status(controller_client, user,
+                           USER_LIST_CTRL_WRITE, SHARE_LIST_CTRL_WRITE)
+
+        password = _generate_random_string()
+        assert_change_password(user_client, user, password)
+        assert_logout_login(controller_client, user_client,
+                            user, fake_home, password, USER_LIST_CTRL_WRITE)
+        assert_user_permissions(user, user_client, controller_client)
         assert_disable_enable(controller_client, user)
-        assert_user_permissions(user, user_client, controller_client)
         controller_client.remove_user(user.name)
     log.info('PASS write {}'.format(user.name))
 
@@ -299,17 +321,33 @@
             user, fake_home)
         controller_client.grant(user_name=user.name, permission="superuser")
         user_client.env.user_name = user.name
-        log.info('Checking list-users {}'.format(user.name))
-        user_list = list_users(controller_client)
-        assert_equal(user_list, USER_LIST_CTRL_ADMIN)
-        log.info('Checking list-shares {}'.format(user.name))
-        share_list = list_shares(controller_client)
-        assert_equal(share_list, SHARE_LIST_CTRL_ADMIN)
+        assert_user_status(controller_client, user,
+                           USER_LIST_CTRL_ADMIN, SHARE_LIST_CTRL_ADMIN)
+
+        password = _generate_random_string()
+        assert_change_password(user_client, user, password)
+        assert_logout_login(controller_client, user_client,
+                            user, fake_home, password, USER_LIST_CTRL_ADMIN)
         assert_user_permissions(user, user_client, controller_client)
+        assert_disable_enable(controller_client, user)
         controller_client.remove_user(user.name)
     log.info('PASS admin {}'.format(user.name))
 
 
+def assert_controller(controller_client):
+    log.info('Checking list-users admin')
+    user_list = list_users(controller_client)
+    assert_equal(user_list, USER_LIST_CTRL)
+
+    log.info('Checking list-shares admin')
+    share_list = list_shares(controller_client)
+    assert_equal(share_list, SHARE_LIST_CTRL)
+
+    log.info('Checking show-user admin')
+    user_status = show_user(controller_client)
+    assert_equal(user_status, USER_LIST_CTRL[0])
+
+
 def assess_user_grant_revoke(controller_client):
     """Test multi-users functionality"""
     log.info('STARTING grant/revoke permissions')
@@ -321,18 +359,15 @@
                       [True, True, False, True, False, False])
     admin_user = User('adminuser', 'admin',
                       [True, True, True, True, True, True])
-    log.info('Checking list-users admin')
-    user_list = list_users(controller_client)
-    assert_equal(user_list, USER_LIST_CTRL)
-    log.info('Checking list-shares admin')
-    share_list = list_shares(controller_client)
-    assert_equal(share_list, SHARE_LIST_CTRL)
-    log.info('Checking show-user admin')
-    user_status = show_user(controller_client)
-    assert_equal(user_status, USER_LIST_CTRL[0])
+
+    # check controller client
+    assert_controller(controller_client)
+
+    # check each type of user
     assert_read_user(controller_client, read_user)
     assert_write_user(controller_client, write_user)
     assert_admin_user(controller_client, admin_user)
+
     log.info('SUCCESS grant/revoke permissions')
 
 

=== removed file 'tests/test_assess_controller_permissions.py'
--- tests/test_assess_controller_permissions.py	2017-03-23 21:22:49 +0000
+++ tests/test_assess_controller_permissions.py	1970-01-01 00:00:00 +0000
@@ -1,98 +0,0 @@
-"""Tests for assess_controller_permissions module."""
-
-import logging
-from mock import Mock, patch
-import StringIO
-
-from assess_controller_permissions import (
-    assess_controller_permissions,
-    parse_args,
-    main,
-)
-from test_assess_user_grant_revoke import (
-    make_fake_client,
-)
-from tests import (
-    parse_error,
-    TestCase,
-)
-
-
-class TestParseArgs(TestCase):
-
-    def test_common_args(self):
-        args = parse_args(["an-env", "/bin/juju", "/tmp/logs", "an-env-mod"])
-        self.assertEqual("an-env", args.env)
-        self.assertEqual("/bin/juju", args.juju_bin)
-        self.assertEqual("/tmp/logs", args.logs)
-        self.assertEqual("an-env-mod", args.temp_env_name)
-        self.assertEqual(False, args.debug)
-
-    def test_help(self):
-        fake_stdout = StringIO.StringIO()
-        with parse_error(self) as fake_stderr:
-            with patch("sys.stdout", fake_stdout):
-                parse_args(["--help"])
-        self.assertEqual("", fake_stderr.getvalue())
-        self.assertNotIn("TODO", fake_stdout.getvalue())
-
-
-class TestMain(TestCase):
-
-    def test_main(self):
-        argv = ["an-env", "/bin/juju", "/tmp/logs", "an-env-mod", "--verbose"]
-        client = Mock(spec=["is_jes_enabled"])
-        with patch("assess_controller_permissions.configure_logging",
-                   autospec=True) as mock_cl:
-            with patch("assess_controller_permissions."
-                       "BootstrapManager.booted_context",
-                       autospec=True) as mock_bc:
-                with patch('deploy_stack.client_from_config',
-                           return_value=client) as mock_c:
-                    with patch("assess_controller_permissions."
-                               "assess_controller_permissions",
-                               autospec=True) as mock_assess:
-                        main(argv)
-        mock_cl.assert_called_once_with(logging.DEBUG)
-        mock_c.assert_called_once_with('an-env', "/bin/juju", debug=False,
-                                       soft_deadline=None)
-        self.assertEqual(mock_bc.call_count, 1)
-        mock_assess.assert_called_once_with(client)
-
-
-class TestAssess(TestCase):
-
-    def test_assess_controller_permissions(self):
-        fake_client = make_fake_client()
-        fake_client.bootstrap()
-        assert_lc_calls = [True, True, True]
-        assert_ac_calls = [False, True, True]
-        assert_sc_calls = [False, False, True]
-        lc = patch('assess_controller_permissions.assert_login_permission',
-                   autospec=True)
-        ac = patch('assess_controller_permissions.assert_addmodel_permission',
-                   autospec=True)
-        sc = patch('assess_controller_permissions.assert_superuser_permission',
-                   autospec=True)
-        with lc as lc_mock, ac as ac_mock, sc as sc_mock:
-            with patch("jujupy.ModelClient.expect",
-                       autospec=True) as expect_mock:
-                with patch("jujupy.ModelClient._end_pexpect_session",
-                           autospec=True):
-                    expect_mock.return_value.isalive.return_value = False
-                    assess_controller_permissions(fake_client)
-                    lc_calls = [
-                        call[0][4] for call in
-                        lc_mock.call_args_list]
-                    ac_calls = [
-                        call[0][2] for call in
-                        ac_mock.call_args_list]
-                    sc_calls = [
-                        call[0][2] for call in
-                        sc_mock.call_args_list]
-                    self.assertEqual(lc_calls,
-                                     assert_lc_calls)
-                    self.assertEqual(ac_calls,
-                                     assert_ac_calls)
-                    self.assertEqual(sc_calls,
-                                     assert_sc_calls)

=== modified file 'tests/test_assess_user_grant_revoke.py'
--- tests/test_assess_user_grant_revoke.py	2017-03-23 19:23:19 +0000
+++ tests/test_assess_user_grant_revoke.py	2017-03-28 14:43:16 +0000
@@ -138,14 +138,20 @@
     def test_assert_read_model(self):
         fake_client = fake_juju_client()
         with patch.object(fake_client, 'show_status', return_value=True):
-            assert_read_model(fake_client, 'read', True)
-            with self.assertRaises(JujuAssertionError):
-                assert_read_model(fake_client, 'read', False)
+            with patch.object(fake_client, 'juju',
+                              return_value=True):
+                assert_read_model(fake_client, 'read', True)
+                with self.assertRaises(JujuAssertionError):
+                    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_model(fake_client, 'read', False)
-            with self.assertRaises(JujuAssertionError):
-                assert_read_model(fake_client, 'read', True)
+            with patch.object(fake_client, 'juju',
+                              return_value=False,
+                              side_effect=CalledProcessError(
+                                None, None, None)):
+                assert_read_model(fake_client, 'read', False)
+                with self.assertRaises(JujuAssertionError):
+                    assert_read_model(fake_client, 'read', True)
 
     def test_assert_write_model(self):
         fake_client = fake_juju_client()
@@ -197,13 +203,13 @@
             with read as read_mock, write as write_mock, admin as admin_mock:
                 with expect as expect_mock:
                     with patch("jujupy.ModelClient._end_pexpect_session",
-                        autospec=True):
+                               autospec=True):
                         expect_mock.return_value.isalive.return_value = False
                         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(pass_mock.call_count, 3)
+                        self.assertEqual(able_mock.call_count, 3)
+                        self.assertEqual(log_mock.call_count, 3)
 
                         self.assertEqual(read_mock.call_count, 6)
                         self.assertEqual(write_mock.call_count, 6)