From fe0c8e955be0331aef9cc6847c9bddc43ce66d92 Mon Sep 17 00:00:00 2001 From: Dolph Mathews Date: Wed, 15 Jun 2016 16:26:35 +0000 Subject: [PATCH] Do not prompt for scope options with default scoped tokens This changes the scope validation to occur after a token has already been created. Previous flow: 1. Validate authentication options. 2. Validate authorization options if the command requires a scope. 3. Create a token (using authentication + authorization options) 4. Run command. This means that scope was being checked, even if a default scope was applied in step 3 by Keystone. New flow: 1. Validate authentication options. 2. Create token (using authentication + authorization options) 3 Validate authorization options if the command requires a scope and the token is not scoped. 4. Run command. Change-Id: Idae368a11249f425b14b891fc68b4176e2b3e981 Closes-Bug: 1592062 --- openstackclient/api/auth.py | 32 +++++++++---------- openstackclient/common/clientmanager.py | 25 +++++++++++---- openstackclient/shell.py | 6 ++-- .../tests/common/test_clientmanager.py | 10 +++--- .../notes/bug-1592062-e327a31a5ae66809.yaml | 5 +++ 5 files changed, 47 insertions(+), 31 deletions(-) create mode 100644 releasenotes/notes/bug-1592062-e327a31a5ae66809.yaml diff --git a/openstackclient/api/auth.py b/openstackclient/api/auth.py index b56035e4d..0018e76e6 100644 --- a/openstackclient/api/auth.py +++ b/openstackclient/api/auth.py @@ -128,12 +128,24 @@ def build_auth_params(auth_plugin_name, cmd_options): return (auth_plugin_loader, auth_params) -def check_valid_auth_options(options, auth_plugin_name, required_scope=True): - """Perform basic option checking, provide helpful error messages. +def check_valid_authorization_options(options, auth_plugin_name): + """Validate authorization options, and provide helpful error messages.""" + if (options.auth.get('project_id') and not + options.auth.get('domain_id') and not + options.auth.get('domain_name') and not + options.auth.get('project_name') and not + options.auth.get('tenant_id') and not + options.auth.get('tenant_name')): + raise exc.CommandError(_( + 'Missing parameter(s): ' + 'Set either a project or a domain scope, but not both. Set a ' + 'project scope with --os-project-name, OS_PROJECT_NAME, or ' + 'auth.project_name. Alternatively, set a domain scope with ' + '--os-domain-name, OS_DOMAIN_NAME or auth.domain_name.')) - :param required_scope: indicate whether a scoped token is required - """ +def check_valid_authentication_options(options, auth_plugin_name): + """Validate authentication options, and provide helpful error messages.""" msgs = [] if auth_plugin_name.endswith('password'): @@ -143,18 +155,6 @@ def check_valid_auth_options(options, auth_plugin_name, required_scope=True): if not options.auth.get('auth_url'): msgs.append(_('Set an authentication URL, with --os-auth-url,' ' OS_AUTH_URL or auth.auth_url')) - if (required_scope and not - options.auth.get('project_id') and not - options.auth.get('domain_id') and not - options.auth.get('domain_name') and not - options.auth.get('project_name') and not - options.auth.get('tenant_id') and not - options.auth.get('tenant_name')): - msgs.append(_('Set a scope, such as a project or domain, set a ' - 'project scope with --os-project-name, ' - 'OS_PROJECT_NAME or auth.project_name, set a domain ' - 'scope with --os-domain-name, OS_DOMAIN_NAME or ' - 'auth.domain_name')) elif auth_plugin_name.endswith('token'): if not options.auth.get('token'): msgs.append(_('Set a token with --os-token, OS_TOKEN or ' diff --git a/openstackclient/common/clientmanager.py b/openstackclient/common/clientmanager.py index 04f624d0b..5dbfb4171 100644 --- a/openstackclient/common/clientmanager.py +++ b/openstackclient/common/clientmanager.py @@ -140,10 +140,8 @@ class ClientManager(object): # prior to dereferrencing auth_ref. self._auth_setup_completed = False - def setup_auth(self, required_scope=True): - """Set up authentication - - :param required_scope: indicate whether a scoped token is required + 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. @@ -157,9 +155,8 @@ class ClientManager(object): self.auth_plugin_name = auth.select_auth_plugin(self._cli_options) # Basic option checking to avoid unhelpful error messages - auth.check_valid_auth_options(self._cli_options, - self.auth_plugin_name, - required_scope=required_scope) + auth.check_valid_authentication_options(self._cli_options, + self.auth_plugin_name) # Horrible hack alert...must handle prompt for null password if # password auth is requested. @@ -229,6 +226,20 @@ class ClientManager(object): self._auth_setup_completed = True + def validate_scope(self): + if self._auth_ref.project_id is not None: + # We already have a project scope. + return + if self._auth_ref.domain_id is not None: + # We already have a domain scope. + return + + # We do not have a scoped token (and the user's default project scope + # was not implied), so the client needs to be explicitly configured + # with a scope. + auth.check_valid_authorization_options(self._cli_options, + self.auth_plugin_name) + @property def auth_ref(self): """Dereference will trigger an auth if it hasn't already""" diff --git a/openstackclient/shell.py b/openstackclient/shell.py index 12a63af21..49a060408 100644 --- a/openstackclient/shell.py +++ b/openstackclient/shell.py @@ -443,12 +443,12 @@ class OpenStackShell(app.App): cmd.__class__.__name__, ) if cmd.auth_required: - if hasattr(cmd, 'required_scope'): + self.client_manager.setup_auth() + if hasattr(cmd, 'required_scope') and cmd.required_scope: # let the command decide whether we need a scoped token - self.client_manager.setup_auth(cmd.required_scope) + self.client_manager.validate_scope() # Trigger the Identity client to initialize self.client_manager.auth_ref - return def clean_up(self, cmd, result, err): self.log.debug('clean_up %s: %s', cmd.__class__.__name__, err or '') diff --git a/openstackclient/tests/common/test_clientmanager.py b/openstackclient/tests/common/test_clientmanager.py index 520cce133..0a9965e0f 100644 --- a/openstackclient/tests/common/test_clientmanager.py +++ b/openstackclient/tests/common/test_clientmanager.py @@ -356,8 +356,8 @@ class TestClientManager(utils.TestCase): client_manager.setup_auth, ) - @mock.patch('openstackclient.api.auth.check_valid_auth_options') - def test_client_manager_auth_setup_once(self, check_auth_options_func): + @mock.patch('openstackclient.api.auth.check_valid_authentication_options') + def test_client_manager_auth_setup_once(self, check_authn_options_func): client_manager = clientmanager.ClientManager( cli_options=FakeOptions( auth=dict( @@ -372,11 +372,11 @@ class TestClientManager(utils.TestCase): ) self.assertFalse(client_manager._auth_setup_completed) client_manager.setup_auth() - self.assertTrue(check_auth_options_func.called) + self.assertTrue(check_authn_options_func.called) self.assertTrue(client_manager._auth_setup_completed) # now make sure we don't do auth setup the second time around # by checking whether check_valid_auth_options() gets called again - check_auth_options_func.reset_mock() + check_authn_options_func.reset_mock() client_manager.auth_ref - check_auth_options_func.assert_not_called() + check_authn_options_func.assert_not_called() diff --git a/releasenotes/notes/bug-1592062-e327a31a5ae66809.yaml b/releasenotes/notes/bug-1592062-e327a31a5ae66809.yaml new file mode 100644 index 000000000..2a7751aae --- /dev/null +++ b/releasenotes/notes/bug-1592062-e327a31a5ae66809.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - Scope options are now validated after authentication occurs, and only if + the user does not have a default project scope. + [Bug `1592062 `_]