Do not prompt for scope options with default scoped tokens

dolph · stevemar · commit fe0c8e955be0 · 2016-06-17T12:37:15.000-04:00
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
diff --git 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.
-
-    :param required_scope: indicate whether a scoped token is required
-
-    """
+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.'))
+
+
+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
@@ -140,10 +140,8 @@ def __init__(
         # 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 @@ def setup_auth(self, required_scope=True):
         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 @@ def setup_auth(self, required_scope=True):
 
         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
@@ -443,12 +443,12 @@ def prepare_to_run_command(self, cmd):
             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
@@ -356,8 +356,8 @@ def test_client_manager_select_auth_plugin_failure(self):
             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 @@ def test_client_manager_auth_setup_once(self, check_auth_options_func):
         )
         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
@@ -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 <https://bugs.launchpad.net/bugs/1592062>`_]
