Merge "Fix auth-required for help command"

4 files changed, 89 insertions, 59 deletions

diff --git a/openstackclient/api/auth.py b/openstackclient/api/auth.py
index 0ed34e02..2097d716 100644
--- a/openstackclient/api/auth.py
+++ b/openstackclient/api/auth.py
@@ -85,9 +85,9 @@ def select_auth_plugin(options):
             # let keystoneclient figure it out itself
             auth_plugin_name = 'token'
     else:
-        raise exc.CommandError(
-            "Authentication type must be selected with --os-auth-type"
-        )
+        # The ultimate default is similar to the original behaviour,
+        # but this time with version discovery
+        auth_plugin_name = 'osc_password'
     LOG.debug("Auth plugin %s selected" % auth_plugin_name)
     return auth_plugin_name
 
diff --git a/openstackclient/common/clientmanager.py b/openstackclient/common/clientmanager.py
index 974936f8..c4307919 100644
--- a/openstackclient/common/clientmanager.py
+++ b/openstackclient/common/clientmanager.py
@@ -56,14 +56,14 @@ class ClientManager(object):
 
     def __init__(
         self,
-        auth_options,
+        cli_options,
         api_version=None,
         verify=True,
         pw_func=None,
     ):
         """Set up a ClientManager
 
-        :param auth_options:
+        :param cli_options:
             Options collected from the command-line, environment, or wherever
         :param api_version:
             Dict of API versions: key is API name, value is the version
@@ -77,31 +77,57 @@ class ClientManager(object):
             returns a string containing the password
         """
 
+        self._cli_options = cli_options
+        self._api_version = api_version
+        self._pw_callback = pw_func
+        self._url = self._cli_options.os_url
+        self._region_name = self._cli_options.os_region_name
+
+        self.timing = self._cli_options.timing
+
+        self._auth_ref = None
+        self.session = None
+
+        # verify is the Requests-compatible form
+        self._verify = verify
+        # also store in the form used by the legacy client libs
+        self._cacert = None
+        if isinstance(verify, bool):
+            self._insecure = not verify
+        else:
+            self._cacert = verify
+            self._insecure = False
+
+        # Get logging from root logger
+        root_logger = logging.getLogger('')
+        LOG.setLevel(root_logger.getEffectiveLevel())
+
+    def setup_auth(self):
+        """Set up authentication
+
+        This is deferred until authentication is actually attempted because
+        it gets in the way of things that do not require auth.
+        """
+
         # If no auth type is named by the user, select one based on
         # the supplied options
-        self.auth_plugin_name = auth.select_auth_plugin(auth_options)
+        self.auth_plugin_name = auth.select_auth_plugin(self._cli_options)
 
         # Basic option checking to avoid unhelpful error messages
-        auth.check_valid_auth_options(auth_options, self.auth_plugin_name)
+        auth.check_valid_auth_options(self._cli_options, self.auth_plugin_name)
 
         # Horrible hack alert...must handle prompt for null password if
         # password auth is requested.
         if (self.auth_plugin_name.endswith('password') and
-                not auth_options.os_password):
-            auth_options.os_password = pw_func()
+                not self._cli_options.os_password):
+            self._cli_options.os_password = self.pw_callback()
 
         (auth_plugin, self._auth_params) = auth.build_auth_params(
             self.auth_plugin_name,
-            auth_options,
+            self._cli_options,
         )
 
-        self._url = auth_options.os_url
-        self._region_name = auth_options.os_region_name
-        self._api_version = api_version
-        self._auth_ref = None
-        self.timing = auth_options.timing
-
-        default_domain = auth_options.os_default_domain
+        default_domain = self._cli_options.os_default_domain
         # NOTE(stevemar): If PROJECT_DOMAIN_ID or PROJECT_DOMAIN_NAME is
         # present, then do not change the behaviour. Otherwise, set the
         # PROJECT_DOMAIN_ID to 'OS_DEFAULT_DOMAIN' for better usability.
@@ -125,20 +151,6 @@ class ClientManager(object):
         elif 'tenant_name' in self._auth_params:
             self._project_name = self._auth_params['tenant_name']
 
-        # verify is the Requests-compatible form
-        self._verify = verify
-        # also store in the form used by the legacy client libs
-        self._cacert = None
-        if isinstance(verify, bool):
-            self._insecure = not verify
-        else:
-            self._cacert = verify
-            self._insecure = False
-
-        # Get logging from root logger
-        root_logger = logging.getLogger('')
-        LOG.setLevel(root_logger.getEffectiveLevel())
-
         LOG.info('Using auth plugin: %s' % self.auth_plugin_name)
         self.auth = auth_plugin.load_from_options(**self._auth_params)
         # needed by SAML authentication
@@ -146,7 +158,7 @@ class ClientManager(object):
         self.session = session.Session(
             auth=self.auth,
             session=request_session,
-            verify=verify,
+            verify=self._verify,
         )
 
         return
@@ -155,6 +167,7 @@ class ClientManager(object):
     def auth_ref(self):
         """Dereference will trigger an auth if it hasn't already"""
         if not self._auth_ref:
+            self.setup_auth()
             LOG.debug("Get auth_ref")
             self._auth_ref = self.auth.get_auth_ref(self.session)
         return self._auth_ref
diff --git a/openstackclient/shell.py b/openstackclient/shell.py
index 246f51b1..b61da2b9 100644
--- a/openstackclient/shell.py
+++ b/openstackclient/shell.py
@@ -22,7 +22,6 @@ import traceback
 
 from cliff import app
 from cliff import command
-from cliff import complete
 from cliff import help
 
 import openstackclient
@@ -70,10 +69,9 @@ class OpenStackShell(app.App):
     def __init__(self):
         # Patch command.Command to add a default auth_required = True
         command.Command.auth_required = True
-        command.Command.best_effort = False
-        # But not help
+
+        # Some commands do not need authentication
         help.HelpCommand.auth_required = False
-        complete.CompleteCommand.best_effort = True
 
         super(OpenStackShell, self).__init__(
             description=__doc__.strip(),
@@ -294,7 +292,7 @@ class OpenStackShell(app.App):
             self.verify = not self.options.insecure
 
         self.client_manager = clientmanager.ClientManager(
-            auth_options=self.options,
+            cli_options=self.options,
             verify=self.verify,
             api_version=self.api_version,
             pw_func=prompt_for_password,
@@ -308,7 +306,7 @@ class OpenStackShell(app.App):
             cmd.__class__.__module__,
             cmd.__class__.__name__,
         )
-        if cmd.auth_required and cmd.best_effort:
+        if cmd.auth_required:
             try:
                 # Trigger the Identity client to initialize
                 self.client_manager.auth_ref
diff --git a/openstackclient/tests/common/test_clientmanager.py b/openstackclient/tests/common/test_clientmanager.py
index 58878e10..ce85e737 100644
--- a/openstackclient/tests/common/test_clientmanager.py
+++ b/openstackclient/tests/common/test_clientmanager.py
@@ -80,12 +80,16 @@ class TestClientManager(utils.TestCase):
     def test_client_manager_token_endpoint(self):
 
         client_manager = clientmanager.ClientManager(
-            auth_options=FakeOptions(os_token=fakes.AUTH_TOKEN,
-                                     os_url=fakes.AUTH_URL,
-                                     os_auth_type='token_endpoint'),
+            cli_options=FakeOptions(
+                os_token=fakes.AUTH_TOKEN,
+                os_url=fakes.AUTH_URL,
+                os_auth_type='token_endpoint',
+            ),
             api_version=API_VERSION,
             verify=True
         )
+        client_manager.setup_auth()
+
         self.assertEqual(
             fakes.AUTH_URL,
             client_manager._url,
@@ -104,12 +108,15 @@ class TestClientManager(utils.TestCase):
     def test_client_manager_token(self):
 
         client_manager = clientmanager.ClientManager(
-            auth_options=FakeOptions(os_token=fakes.AUTH_TOKEN,
-                                     os_auth_url=fakes.AUTH_URL,
-                                     os_auth_type='v2token'),
+            cli_options=FakeOptions(
+                os_token=fakes.AUTH_TOKEN,
+                os_auth_url=fakes.AUTH_URL,
+                os_auth_type='v2token',
+            ),
             api_version=API_VERSION,
             verify=True
         )
+        client_manager.setup_auth()
 
         self.assertEqual(
             fakes.AUTH_URL,
@@ -125,13 +132,16 @@ class TestClientManager(utils.TestCase):
     def test_client_manager_password(self):
 
         client_manager = clientmanager.ClientManager(
-            auth_options=FakeOptions(os_auth_url=fakes.AUTH_URL,
-                                     os_username=fakes.USERNAME,
-                                     os_password=fakes.PASSWORD,
-                                     os_project_name=fakes.PROJECT_NAME),
+            cli_options=FakeOptions(
+                os_auth_url=fakes.AUTH_URL,
+                os_username=fakes.USERNAME,
+                os_password=fakes.PASSWORD,
+                os_project_name=fakes.PROJECT_NAME,
+            ),
             api_version=API_VERSION,
             verify=False,
         )
+        client_manager.setup_auth()
 
         self.assertEqual(
             fakes.AUTH_URL,
@@ -182,14 +192,17 @@ class TestClientManager(utils.TestCase):
     def test_client_manager_password_verify_ca(self):
 
         client_manager = clientmanager.ClientManager(
-            auth_options=FakeOptions(os_auth_url=fakes.AUTH_URL,
-                                     os_username=fakes.USERNAME,
-                                     os_password=fakes.PASSWORD,
-                                     os_project_name=fakes.PROJECT_NAME,
-                                     os_auth_type='v2password'),
+            cli_options=FakeOptions(
+                os_auth_url=fakes.AUTH_URL,
+                os_username=fakes.USERNAME,
+                os_password=fakes.PASSWORD,
+                os_project_name=fakes.PROJECT_NAME,
+                os_auth_type='v2password',
+            ),
             api_version=API_VERSION,
             verify='cafile',
         )
+        client_manager.setup_auth()
 
         self.assertFalse(client_manager._insecure)
         self.assertTrue(client_manager._verify)
@@ -199,10 +212,12 @@ class TestClientManager(utils.TestCase):
         auth_params['os_auth_type'] = auth_plugin_name
         auth_params['os_identity_api_version'] = api_version
         client_manager = clientmanager.ClientManager(
-            auth_options=FakeOptions(**auth_params),
+            cli_options=FakeOptions(**auth_params),
             api_version=API_VERSION,
             verify=True
         )
+        client_manager.setup_auth()
+
         self.assertEqual(
             auth_plugin_name,
             client_manager.auth_plugin_name,
@@ -228,8 +243,12 @@ class TestClientManager(utils.TestCase):
         self._select_auth_plugin(params, 'XXX', 'password')
 
     def test_client_manager_select_auth_plugin_failure(self):
-        self.assertRaises(exc.CommandError,
-                          clientmanager.ClientManager,
-                          auth_options=FakeOptions(os_auth_plugin=''),
-                          api_version=API_VERSION,
-                          verify=True)
+        client_manager = clientmanager.ClientManager(
+            cli_options=FakeOptions(os_auth_plugin=''),
+            api_version=API_VERSION,
+            verify=True,
+        )
+        self.assertRaises(
+            exc.CommandError,
+            client_manager.setup_auth,
+        )