From 505fa14cd68e13d066a5770a229ba0d7fa88d2a9 Mon Sep 17 00:00:00 2001 From: Dean Troyer Date: Fri, 27 Feb 2015 09:19:12 -0600 Subject: [PATCH] Fix auth-required for help command When we got picky with the auth arguments we broke using help without any auth config supplied. This rearranges things a bit to do the argument checking when the deferred auth request to Identity occurs so commands that do not need auth have a chance to live short but useful lives. Closes-Bug: #1399588 Change-Id: I8ceac491cf65e25eddb62ab2713f471fe686756d --- examples/osc-lib.py | 2 +- openstackclient/api/auth.py | 6 +- openstackclient/common/clientmanager.py | 71 +++++++++++-------- openstackclient/shell.py | 10 ++- .../tests/common/test_clientmanager.py | 61 ++++++++++------ 5 files changed, 90 insertions(+), 60 deletions(-) diff --git a/examples/osc-lib.py b/examples/osc-lib.py index 69fc5d9849..2960a2f74e 100755 --- a/examples/osc-lib.py +++ b/examples/osc-lib.py @@ -59,7 +59,7 @@ def run(opts): # Collect the auth and config options together and give them to # ClientManager and it will wrangle all of the goons into place. client_manager = clientmanager.ClientManager( - auth_options=opts, + cli_options=opts, verify=verify, api_version=api_version, ) diff --git a/openstackclient/api/auth.py b/openstackclient/api/auth.py index 14bb01d7b1..d44f2f9d63 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 974936f899..c430791933 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 246f51b177..b61da2b941 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 3b2b976b5d..f3826ca370 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, + )