Don't create client for help and bash completion

Currently when we need a help, client object is created
and authentication is performed. This is completely useless
and leads to unnecessary actions in the background.

This patch:
1. Prevents creation of client object (and therefore
authentication) for help or bash-completion commands.
2. Removes a workaround from keystone auth module that disables
sending requests to the server if help or bash-completion
commands are executing.
3. Adds related unit tests.

Change-Id: Ia26d7f4e56f5ef3ae0ac5e94e8e77d1a78f8829e
Closes-bug: #1720795

mistralclient/auth/keystone.py

@@ -12,8 +12,6 @@
 #    See the License for the specific language governing permissions and
 #    limitations under the License.
 
-import logging
-
 import keystoneauth1.identity.generic as auth_plugin
 from keystoneauth1 import session as ks_session
 import mistralclient.api.httpclient as api
@@ -21,9 +19,6 @@ from mistralclient import auth as mistral_auth
 from oslo_serialization import jsonutils
 
 
-LOG = logging.getLogger(__name__)
-
-
 class KeystoneAuthHandler(mistral_auth.AuthHandler):
 
     def authenticate(self, req, session=None):
@@ -82,7 +77,6 @@ class KeystoneAuthHandler(mistral_auth.AuthHandler):
         auth_response = {}
 
         if not session:
-            auth = None
             if auth_token:
                 auth = auth_plugin.Token(
                     auth_url=auth_url,
@@ -105,14 +99,10 @@ class KeystoneAuthHandler(mistral_auth.AuthHandler):
                     project_name=project_name,
                     project_domain_name=project_domain_name,
                     project_domain_id=project_domain_id)
-
             else:
-                # NOTE(jaosorior): We don't crash here cause it's needed for
-                # bash-completion to work. However, we do issue a warning to
-                # the user so if the request doesn't work. It's because of
-                # this.
-                LOG.warning("You must either provide a valid token or "
-                            "a password (api_key) and a user.")
+                raise RuntimeError("You must either provide a valid token or "
+                                   "a password (api_key) and a user.")
+
             if auth:
                 session = ks_session.Session(auth=auth)
 

mistralclient/shell.py

@@ -560,7 +560,13 @@ class MistralShell(app.App):
 
         self._set_shell_commands(self._get_commands(ver))
 
-        do_help = ('help' in argv) or ('-h' in argv) or not argv
+        # bash-completion and help messages should not require client creation
+        need_client = not (
+            ('bash-completion' in argv) or
+            ('help' in argv) or
+            ('-h' in argv) or
+            ('--help' in argv) or
+            not argv)
 
         # Set default for auth_url if not supplied. The default is not
         # set at the parser to support use cases where auth is not enabled.
@@ -586,10 +592,6 @@ class MistralShell(app.App):
                     not self.options.target_user_domain_name):
                 self.options.target_user_domain_id = "default"
 
-        # bash-completion should not require authentification.
-        if do_help or ('bash-completion' in argv):
-            self.options.auth_url = None
-
         if self.options.auth_url and not self.options.token:
             if not self.options.username:
                 raise exe.IllegalArgumentException(
@@ -602,7 +604,19 @@ class MistralShell(app.App):
                     ("You must provide a password "
                      "via --os-password env[OS_PASSWORD]")
                 )
+        self.client = self._create_client() if need_client else None
 
+        # Adding client_manager variable to make mistral client work with
+        # unified OpenStack client.
+        ClientManager = type(
+            'ClientManager',
+            (object,),
+            dict(workflow_engine=self.client)
+        )
+
+        self.client_manager = ClientManager()
+
+    def _create_client(self):
         kwargs = {
             'cert': self.options.os_cert,
             'key': self.options.os_key,
@@ -612,13 +626,12 @@ class MistralShell(app.App):
             'project_domain_id': self.options.project_domain_id,
             'target_project_domain_name':
                 self.options.target_project_domain_name,
-            'target_project_domain_id':
-                self.options.target_project_domain_id,
+            'target_project_domain_id': self.options.target_project_domain_id,
             'target_user_domain_name': self.options.target_user_domain_name,
             'target_user_domain_id': self.options.target_user_domain_id
         }
 
-        self.client = client.client(
+        return client.client(
             mistral_url=self.options.mistral_url,
             username=self.options.username,
             api_key=self.options.password,
@@ -647,16 +660,6 @@ class MistralShell(app.App):
             **kwargs
         )
 
-        # Adding client_manager variable to make mistral client work with
-        # unified OpenStack client.
-        ClientManager = type(
-            'ClientManager',
-            (object,),
-            dict(workflow_engine=self.client)
-        )
-
-        self.client_manager = ClientManager()
-
     def _set_shell_commands(self, cmds_dict):
         for k, v in cmds_dict.items():
             self.command_manager.add_command(k, v)

mistralclient/tests/unit/test_shell.py

@@ -19,6 +19,17 @@ import mistralclient.tests.unit.base_shell_test as base
 
 class TestShell(base.BaseShellTests):
 
+    def test_help(self):
+        """Test that client is not created for help and bash complete"""
+        for command in ('-h',
+                        '--help',
+                        'help',
+                        'help workbook-list',
+                        'bash-completion'):
+            with mock.patch('mistralclient.api.client.client') as client_mock:
+                self.shell(command)
+                self.assertFalse(client_mock.called)
+
     @mock.patch('mistralclient.api.client.client')
     def test_command_no_mistral_url(self, client_mock):
         self.shell(

mistralclient/tests/unit/v2/base.py

@@ -13,6 +13,8 @@
 #    See the License for the specific language governing permissions and
 #    limitations under the License.
 
+import mock
+
 from mistralclient.api.v2 import client
 from mistralclient.tests.unit import base
 
@@ -24,14 +26,17 @@ class BaseClientV2Test(base.BaseClientTest):
     def setUp(self):
         super(BaseClientV2Test, self).setUp()
 
-        self._client = client.Client(project_name="test",
-                                     mistral_url=self.TEST_URL)
-        self.workbooks = self._client.workbooks
-        self.executions = self._client.executions
-        self.tasks = self._client.tasks
-        self.workflows = self._client.workflows
-        self.environments = self._client.environments
-        self.action_executions = self._client.action_executions
-        self.actions = self._client.actions
-        self.services = self._client.services
-        self.members = self._client.members
+        with mock.patch(
+                'mistralclient.auth.keystone.KeystoneAuthHandler.authenticate',
+                return_value={'session': None}):
+            self._client = client.Client(project_name="test",
+                                         mistral_url=self.TEST_URL)
+            self.workbooks = self._client.workbooks
+            self.executions = self._client.executions
+            self.tasks = self._client.tasks
+            self.workflows = self._client.workflows
+            self.environments = self._client.environments
+            self.action_executions = self._client.action_executions
+            self.actions = self._client.actions
+            self.services = self._client.services
+            self.members = self._client.members