From ddb85ffc94c8d45f9ac150d649ba2e384ac92663 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Thu, 15 Dec 2016 10:58:35 -0800 Subject: [PATCH 01/25] Initial work on new 'login' command Signed-off-by: Matt Oswalt --- st2client/st2client/commands/auth.py | 72 +++++++++++++++++++++++++++- st2client/st2client/shell.py | 11 ++++- 2 files changed, 80 insertions(+), 3 deletions(-) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index a4a9a997f2..2ba0ab881a 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -13,10 +13,13 @@ # See the License for the specific language governing permissions and # limitations under the License. +import configparser +from os.path import expanduser import getpass import json import logging +from st2client.base import BaseCLIApp from st2client import models from st2client.commands import resource from st2client.commands.noop import NoopCommand @@ -37,7 +40,7 @@ def __init__(self, resource, *args, **kwargs): super(TokenCreateCommand, self).__init__( resource, kwargs.pop('name', 'create'), - 'Authenticate user and aquire access token.', + 'Authenticate user and acquire access token.', *args, **kwargs) self.parser.add_argument('username', @@ -69,6 +72,73 @@ def run_and_print(self, args, **kwargs): self.print_output(instance, table.PropertyValueTable, attributes=self.display_attributes, json=args.json, yaml=args.yaml) +# (Notes for future PR) +# This PR introduces a new `st2 login` command to the StackStorm client. + +# This is in repsonse to https://github.com/StackStorm/st2/issues/3110, where a +# user asked if there was a more friendly way of specifying the current user +# (i.e. without explicitly modifying configuration files). + +# Initially, I considered adding a flag to `st2 auth` that caches the token in +# `~/.st2/token-` but realized that `~/st2/config` would also need +# to be modified to specify the "current" user. This seemed like a bit too +# much functionality to have in a single "flag" off of `st2 auth`, so I decided +# a separate command was ideal, and more self-explanatory. + + +class LoginCommand(resource.ResourceCommand): + display_attributes = ['user', 'token', 'expiry'] + + def __init__(self, resource, *args, **kwargs): + + kwargs['has_token_opt'] = False + + super(LoginCommand, self).__init__( + resource, kwargs.pop('name', 'create'), + 'Authenticate user, acquire access token, and update CLI config directory', + *args, **kwargs) + + self.parser.add_argument('username', + help='Name of the user to authenticate.') + + self.parser.add_argument('-p', '--password', dest='password', + help='Password for the user. If password is not provided, ' + 'it will be prompted.') + self.parser.add_argument('-l', '--ttl', type=int, dest='ttl', default=None, + help='The life span of the token in seconds. ' + 'Max TTL configured by the admin supersedes this.') + + def run(self, args, **kwargs): + if not args.password: + args.password = getpass.getpass() + instance = self.resource(ttl=args.ttl) if args.ttl else self.resource() + + manager = self.manager.create(instance, auth=(args.username, args.password), **kwargs) + + cli = BaseCLIApp() + logging.basicConfig(level=logging.DEBUG, format='%(relativeCreated)6d %(threadName)s %(message)s') #TODO(mierdin) send to nullhandler + cli.LOG = logging.getLogger("st2client.base") + cli._cache_auth_token(token_obj=manager) + + config_file = "%s/.st2/config" % expanduser("~") + + config = configparser.ConfigParser() + config.read(config_file) + + with open(config_file, "w") as cfg_file_out: + config.write(cfg_file_out) + + return manager + + def run_and_print(self, args, **kwargs): + # instance = self.run(args, **kwargs) + + try: + self.run(args, **kwargs) + print("Failed to log in as %s" % args.username) + except Exception: + print("Logged in as %s" % args.username) + class ApiKeyBranch(resource.ResourceBranch): diff --git a/st2client/st2client/shell.py b/st2client/st2client/shell.py index 50d3713807..410edc620b 100755 --- a/st2client/st2client/shell.py +++ b/st2client/st2client/shell.py @@ -50,7 +50,7 @@ from st2client.config import set_config from st2client.exceptions.operations import OperationFailureException from st2client.utils.logging import LogLevelFilter, set_log_level_for_all_loggers -from st2client.commands.auth import TokenCreateCommand +from st2client.commands.auth import TokenCreateCommand, LoginCommand __all__ = [ 'Shell' @@ -65,7 +65,8 @@ class Shell(BaseCLIApp): LOG = LOGGER SKIP_AUTH_CLASSES = [ - TokenCreateCommand.__name__ + TokenCreateCommand.__name__, + LoginCommand.__name__ ] def __init__(self): @@ -188,6 +189,10 @@ def __init__(self): 'for reuse in sensors, actions, and rules.', self, self.subparsers) + self.commands['login'] = auth.LoginCommand( + 'Set current username/password in config, and cache API token', + self, self.subparsers) + self.commands['pack'] = pack.PackBranch( 'A group of related integration resources: ' 'actions, rules, and sensors.', @@ -253,6 +258,8 @@ def run(self, argv): # Hack because --print-config requires no command to be specified argv = argv + ['action', 'list'] + import pdb; pdb.set_trace() + # Parse command line arguments. args = self.parser.parse_args(args=argv) From 7c97cb5a1d2d2d01a38b38f496a2ecb5897a1499 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Thu, 15 Dec 2016 14:44:51 -0800 Subject: [PATCH 02/25] Fixed inappropriate args to auth.LoginCommand, finished functionality Signed-off-by: Matt Oswalt --- st2client/st2client/commands/__init__.py | 1 + st2client/st2client/commands/auth.py | 18 +++++++++++------- st2client/st2client/shell.py | 5 +---- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/st2client/st2client/commands/__init__.py b/st2client/st2client/commands/__init__.py index 36c88e2b4d..a89e1a754a 100644 --- a/st2client/st2client/commands/__init__.py +++ b/st2client/st2client/commands/__init__.py @@ -33,6 +33,7 @@ def __init__(self, name, description, app, subparsers, parent_parser=None): self.description = description self.app = app self.parent_parser = parent_parser + self.parser = subparsers.add_parser(self.name, description=self.description, help=self.description) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index 2ba0ab881a..de7a2e1b2b 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -116,28 +116,32 @@ def run(self, args, **kwargs): manager = self.manager.create(instance, auth=(args.username, args.password), **kwargs) cli = BaseCLIApp() - logging.basicConfig(level=logging.DEBUG, format='%(relativeCreated)6d %(threadName)s %(message)s') #TODO(mierdin) send to nullhandler + logging.basicConfig( + level=logging.CRITICAL, + handlers=[logging.NullHandler] + ) cli.LOG = logging.getLogger("st2client.base") cli._cache_auth_token(token_obj=manager) + # Update existing configuration with new credentials config_file = "%s/.st2/config" % expanduser("~") - config = configparser.ConfigParser() config.read(config_file) - + config['credentials'] = { + "username": args.username, + "password": args.password + } with open(config_file, "w") as cfg_file_out: config.write(cfg_file_out) return manager def run_and_print(self, args, **kwargs): - # instance = self.run(args, **kwargs) - try: self.run(args, **kwargs) - print("Failed to log in as %s" % args.username) - except Exception: print("Logged in as %s" % args.username) + except Exception: + print("Failed to log in as %s" % args.username) class ApiKeyBranch(resource.ResourceBranch): diff --git a/st2client/st2client/shell.py b/st2client/st2client/shell.py index 410edc620b..ca7a02af1b 100755 --- a/st2client/st2client/shell.py +++ b/st2client/st2client/shell.py @@ -190,8 +190,7 @@ def __init__(self): self, self.subparsers) self.commands['login'] = auth.LoginCommand( - 'Set current username/password in config, and cache API token', - self, self.subparsers) + models.Token, self, self.subparsers, name='login') self.commands['pack'] = pack.PackBranch( 'A group of related integration resources: ' @@ -258,8 +257,6 @@ def run(self, argv): # Hack because --print-config requires no command to be specified argv = argv + ['action', 'list'] - import pdb; pdb.set_trace() - # Parse command line arguments. args = self.parser.parse_args(args=argv) From a8f4f36eb1bab0b0f4bfc6a986b0fbf2767ca844 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Thu, 15 Dec 2016 17:34:48 -0800 Subject: [PATCH 03/25] Added unit test for login command Signed-off-by: Matt Oswalt --- st2client/st2client/commands/auth.py | 18 ++------ st2client/tests/unit/test_auth.py | 69 ++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 15 deletions(-) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index de7a2e1b2b..ea3d97d660 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -import configparser +from configparser import ConfigParser from os.path import expanduser import getpass import json @@ -72,19 +72,6 @@ def run_and_print(self, args, **kwargs): self.print_output(instance, table.PropertyValueTable, attributes=self.display_attributes, json=args.json, yaml=args.yaml) -# (Notes for future PR) -# This PR introduces a new `st2 login` command to the StackStorm client. - -# This is in repsonse to https://github.com/StackStorm/st2/issues/3110, where a -# user asked if there was a more friendly way of specifying the current user -# (i.e. without explicitly modifying configuration files). - -# Initially, I considered adding a flag to `st2 auth` that caches the token in -# `~/.st2/token-` but realized that `~/st2/config` would also need -# to be modified to specify the "current" user. This seemed like a bit too -# much functionality to have in a single "flag" off of `st2 auth`, so I decided -# a separate command was ideal, and more self-explanatory. - class LoginCommand(resource.ResourceCommand): display_attributes = ['user', 'token', 'expiry'] @@ -109,6 +96,7 @@ def __init__(self, resource, *args, **kwargs): 'Max TTL configured by the admin supersedes this.') def run(self, args, **kwargs): + if not args.password: args.password = getpass.getpass() instance = self.resource(ttl=args.ttl) if args.ttl else self.resource() @@ -125,7 +113,7 @@ def run(self, args, **kwargs): # Update existing configuration with new credentials config_file = "%s/.st2/config" % expanduser("~") - config = configparser.ConfigParser() + config = ConfigParser() config.read(config_file) config['credentials'] = { "username": args.username, diff --git a/st2client/tests/unit/test_auth.py b/st2client/tests/unit/test_auth.py index 5cab3361a9..e2bc88941d 100644 --- a/st2client/tests/unit/test_auth.py +++ b/st2client/tests/unit/test_auth.py @@ -13,6 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +from os.path import expanduser import os import uuid import json @@ -39,6 +40,74 @@ } +class TestLogin(base.BaseCLITestCase): + + def __init__(self, *args, **kwargs): + super(TestLogin, self).__init__(*args, **kwargs) + self.parser = argparse.ArgumentParser() + self.parser.add_argument('-t', '--token', dest='token') + self.parser.add_argument('--api-key', dest='api_key') + self.shell = shell.Shell() + + def setUp(self): + super(TestLogin, self).setUp() + + def tearDown(self): + super(TestLogin, self).tearDown() + + @mock.patch("st2client.commands.auth.ConfigParser") + @mock.patch("st2client.commands.auth.open") + @mock.patch("st2client.commands.auth.BaseCLIApp") + @mock.patch("st2client.commands.auth.getpass") + def test_login_with_password(self, mock_gp, mock_cli, mock_open, mock_cfg): + + self.shell.run(['login', 'st2admin', '--password', 'Password1!']) + + # Ensure token was generated + mock_cli.return_value._cache_auth_token.assert_called_once() + + # Ensure configuration was performed properly + config_file = "%s/.st2/config" % expanduser("~") + mock_cfg.return_value.read.assert_called_once_with(config_file) + mock_cfg.return_value.__setitem__.assert_called_once_with( + 'credentials', { + "username": 'st2admin', + "password": 'Password1!' + } + ) + + # Ensure file was written to with a context manager + mock_open.return_value.__enter__.assert_called_once() + mock_open.return_value.__exit__.assert_called_once() + + + @mock.patch("st2client.commands.auth.ConfigParser") + @mock.patch("st2client.commands.auth.open") + @mock.patch("st2client.commands.auth.BaseCLIApp") + @mock.patch("st2client.commands.auth.getpass") + def test_login_no_password(self, mock_gp, mock_cli, mock_open, mock_cfg): + mock_gp.getpass.return_value = 'Password1!' + + self.shell.run(['login', 'st2admin']) + + # Ensure token was generated + mock_cli.return_value._cache_auth_token.assert_called_once() + + # Ensure configuration was performed properly + config_file = "%s/.st2/config" % expanduser("~") + mock_cfg.return_value.read.assert_called_once_with(config_file) + mock_cfg.return_value.__setitem__.assert_called_once_with( + 'credentials', { + "username": 'st2admin', + "password": 'Password1!' + } + ) + + # Ensure file was written to with a context manager + mock_open.return_value.__enter__.assert_called_once() + mock_open.return_value.__exit__.assert_called_once() + + class TestAuthToken(base.BaseCLITestCase): def __init__(self, *args, **kwargs): From 67a18aa60f1c052bdfe14537a96d45a8faad4e5c Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Thu, 15 Dec 2016 17:37:04 -0800 Subject: [PATCH 04/25] Removed erroneous extra line Signed-off-by: Matt Oswalt --- st2client/st2client/commands/__init__.py | 1 - 1 file changed, 1 deletion(-) diff --git a/st2client/st2client/commands/__init__.py b/st2client/st2client/commands/__init__.py index a89e1a754a..36c88e2b4d 100644 --- a/st2client/st2client/commands/__init__.py +++ b/st2client/st2client/commands/__init__.py @@ -33,7 +33,6 @@ def __init__(self, name, description, app, subparsers, parent_parser=None): self.description = description self.app = app self.parent_parser = parent_parser - self.parser = subparsers.add_parser(self.name, description=self.description, help=self.description) From 2d31ff1c10b50bf369c3a65347ac57da5d88e7a4 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Thu, 15 Dec 2016 22:35:13 -0800 Subject: [PATCH 05/25] Removed extra line in test Signed-off-by: Matt Oswalt --- st2client/tests/unit/test_auth.py | 1 - 1 file changed, 1 deletion(-) diff --git a/st2client/tests/unit/test_auth.py b/st2client/tests/unit/test_auth.py index e2bc88941d..f46caaa852 100644 --- a/st2client/tests/unit/test_auth.py +++ b/st2client/tests/unit/test_auth.py @@ -80,7 +80,6 @@ def test_login_with_password(self, mock_gp, mock_cli, mock_open, mock_cfg): mock_open.return_value.__enter__.assert_called_once() mock_open.return_value.__exit__.assert_called_once() - @mock.patch("st2client.commands.auth.ConfigParser") @mock.patch("st2client.commands.auth.open") @mock.patch("st2client.commands.auth.BaseCLIApp") From 03fb3f6662278a0af02b34dcb8d833f018dc9f15 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Thu, 15 Dec 2016 23:12:43 -0800 Subject: [PATCH 06/25] Simplified expanduser() call Signed-off-by: Matt Oswalt --- st2client/st2client/commands/auth.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index ea3d97d660..166538b62b 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -104,6 +104,7 @@ def run(self, args, **kwargs): manager = self.manager.create(instance, auth=(args.username, args.password), **kwargs) cli = BaseCLIApp() + logging.basicConfig( level=logging.CRITICAL, handlers=[logging.NullHandler] @@ -112,9 +113,10 @@ def run(self, args, **kwargs): cli._cache_auth_token(token_obj=manager) # Update existing configuration with new credentials - config_file = "%s/.st2/config" % expanduser("~") + config_file = expanduser('~/.st2/config') config = ConfigParser() config.read(config_file) + config['credentials'] = { "username": args.username, "password": args.password From eb3aedcd99a6a8593851e3adce5179af51020e47 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Fri, 16 Dec 2016 10:10:17 -0800 Subject: [PATCH 07/25] Added -w option, updated unit test Signed-off-by: Matt Oswalt --- st2client/st2client/commands/auth.py | 11 ++- st2client/tests/unit/test_auth.py | 103 +++++++++++++++++---------- 2 files changed, 75 insertions(+), 39 deletions(-) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index 166538b62b..b4d2aced6f 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -94,6 +94,9 @@ def __init__(self, resource, *args, **kwargs): self.parser.add_argument('-l', '--ttl', type=int, dest='ttl', default=None, help='The life span of the token in seconds. ' 'Max TTL configured by the admin supersedes this.') + self.parser.add_argument('-w', '--write-real-password', action='store_true', default=False, + dest='write_password', + help='Write the real (plain-text) password to the config file') def run(self, args, **kwargs): @@ -117,10 +120,14 @@ def run(self, args, **kwargs): config = ConfigParser() config.read(config_file) + # Other st2 commands error out if the "password" field is missing from the "credentials" + # section. So, here we will write it to "notarealpassword", unless the args.write_password + # option is provided, in which case we'll write the real password. config['credentials'] = { "username": args.username, - "password": args.password + "password": args.password if args.write_password else "notarealpassword" } + with open(config_file, "w") as cfg_file_out: config.write(cfg_file_out) @@ -132,6 +139,8 @@ def run_and_print(self, args, **kwargs): print("Logged in as %s" % args.username) except Exception: print("Failed to log in as %s" % args.username) + if self.app.client.debug: + raise class ApiKeyBranch(resource.ResourceBranch): diff --git a/st2client/tests/unit/test_auth.py b/st2client/tests/unit/test_auth.py index f46caaa852..2b707287cb 100644 --- a/st2client/tests/unit/test_auth.py +++ b/st2client/tests/unit/test_auth.py @@ -59,52 +59,79 @@ def tearDown(self): @mock.patch("st2client.commands.auth.open") @mock.patch("st2client.commands.auth.BaseCLIApp") @mock.patch("st2client.commands.auth.getpass") - def test_login_with_password(self, mock_gp, mock_cli, mock_open, mock_cfg): - - self.shell.run(['login', 'st2admin', '--password', 'Password1!']) + def test_login(self, mock_gp, mock_cli, mock_open, mock_cfg): + + test_cases = [ + { + "args": ['login', 'st2admin', '--password', 'Password1!'], + "expected_config": { + 'credentials': { + "username": 'st2admin', + "password": 'notarealpassword' + } + } + }, + { + "args": ['login', 'st2admin'], + "expected_config": { + 'credentials': { + "username": 'st2admin', + "password": 'notarealpassword' + } + } + }, + { + "args": ['login', 'st2admin', '--password', 'Password1!', '-w'], + "expected_config": { + 'credentials': { + "username": 'st2admin', + "password": 'Password1!' + } + } + }, + { + "args": ['login', 'st2admin', '-w'], + "expected_config": { + 'credentials': { + "username": 'st2admin', + "password": 'Password1!' + } + } + } + ] - # Ensure token was generated - mock_cli.return_value._cache_auth_token.assert_called_once() + for test_case in test_cases: - # Ensure configuration was performed properly - config_file = "%s/.st2/config" % expanduser("~") - mock_cfg.return_value.read.assert_called_once_with(config_file) - mock_cfg.return_value.__setitem__.assert_called_once_with( - 'credentials', { - "username": 'st2admin', - "password": 'Password1!' - } - ) + mock_gp.getpass.return_value = "Password1!" - # Ensure file was written to with a context manager - mock_open.return_value.__enter__.assert_called_once() - mock_open.return_value.__exit__.assert_called_once() + self.shell.run(test_case['args']) - @mock.patch("st2client.commands.auth.ConfigParser") - @mock.patch("st2client.commands.auth.open") - @mock.patch("st2client.commands.auth.BaseCLIApp") - @mock.patch("st2client.commands.auth.getpass") - def test_login_no_password(self, mock_gp, mock_cli, mock_open, mock_cfg): - mock_gp.getpass.return_value = 'Password1!' + # Ensure getpass was only used if "--password" option was omitted + if "--password" in test_case['args']: + mock_gp.getpass.assert_not_called() + else: + mock_gp.getpass.assert_called_once() - self.shell.run(['login', 'st2admin']) + # Ensure token was generated + mock_cli.return_value._cache_auth_token.assert_called_once() - # Ensure token was generated - mock_cli.return_value._cache_auth_token.assert_called_once() + # Ensure configuration was performed properly + config_file = "%s/.st2/config" % expanduser("~") + mock_cfg.return_value.read.assert_called_once_with(config_file) + mock_cfg.return_value.__setitem__.assert_called_once_with( + 'credentials', test_case['expected_config']['credentials']) - # Ensure configuration was performed properly - config_file = "%s/.st2/config" % expanduser("~") - mock_cfg.return_value.read.assert_called_once_with(config_file) - mock_cfg.return_value.__setitem__.assert_called_once_with( - 'credentials', { - "username": 'st2admin', - "password": 'Password1!' - } - ) + # Ensure file was written to with a context manager + mock_open.return_value.__enter__.assert_called_once() + mock_open.return_value.__exit__.assert_called_once() - # Ensure file was written to with a context manager - mock_open.return_value.__enter__.assert_called_once() - mock_open.return_value.__exit__.assert_called_once() + # Reset "called" counters + mock_gp.getpass.reset_mock() + mock_cli.return_value._cache_auth_token.reset_mock() + mock_cfg.return_value.read.reset_mock() + mock_cfg.return_value.__setitem__.reset_mock() + mock_open.return_value.__enter__.reset_mock() + mock_open.return_value.__exit__.reset_mock() class TestAuthToken(base.BaseCLITestCase): From a050a9f57a93e0a6c664f9446d15509a2ea65d9c Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Fri, 16 Dec 2016 10:41:52 -0800 Subject: [PATCH 08/25] Properly mocked 'requests' like I should have already done Signed-off-by: Matt Oswalt --- st2client/tests/unit/test_auth.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/st2client/tests/unit/test_auth.py b/st2client/tests/unit/test_auth.py index 2b707287cb..a5d40dbbfe 100644 --- a/st2client/tests/unit/test_auth.py +++ b/st2client/tests/unit/test_auth.py @@ -55,6 +55,9 @@ def setUp(self): def tearDown(self): super(TestLogin, self).tearDown() + @mock.patch.object( + requests, 'post', + mock.MagicMock(return_value=base.FakeResponse(json.dumps({}), 200, 'OK'))) @mock.patch("st2client.commands.auth.ConfigParser") @mock.patch("st2client.commands.auth.open") @mock.patch("st2client.commands.auth.BaseCLIApp") From 683cb4dbd912c5a94eb71363f89631ed83384701 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Fri, 16 Dec 2016 14:48:07 -0800 Subject: [PATCH 09/25] Used built-in function to determine config location This allows the user to specify a different location for the config file using the already-existing "--config" argument Signed-off-by: Matt Oswalt --- st2client/st2client/commands/auth.py | 13 ++++++++++--- st2client/tests/unit/test_auth.py | 16 +++++++++++++--- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index b4d2aced6f..d067884e1b 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -104,19 +104,26 @@ def run(self, args, **kwargs): args.password = getpass.getpass() instance = self.resource(ttl=args.ttl) if args.ttl else self.resource() - manager = self.manager.create(instance, auth=(args.username, args.password), **kwargs) - cli = BaseCLIApp() + # Determine path to config file + try: + config_file = cli._get_config_file_path(args) + except ValueError: + # config file not found in args or in env, defaulting + config_file = expanduser('~/.st2/config') + logging.basicConfig( level=logging.CRITICAL, handlers=[logging.NullHandler] ) cli.LOG = logging.getLogger("st2client.base") + + # Retrieve token + manager = self.manager.create(instance, auth=(args.username, args.password), **kwargs) cli._cache_auth_token(token_obj=manager) # Update existing configuration with new credentials - config_file = expanduser('~/.st2/config') config = ConfigParser() config.read(config_file) diff --git a/st2client/tests/unit/test_auth.py b/st2client/tests/unit/test_auth.py index a5d40dbbfe..e290c8641c 100644 --- a/st2client/tests/unit/test_auth.py +++ b/st2client/tests/unit/test_auth.py @@ -66,7 +66,9 @@ def test_login(self, mock_gp, mock_cli, mock_open, mock_cfg): test_cases = [ { - "args": ['login', 'st2admin', '--password', 'Password1!'], + "args": [ + '--config', '/tmp/st2config', 'login', 'st2admin', '--password', 'Password1!' + ], "expected_config": { 'credentials': { "username": 'st2admin', @@ -93,7 +95,7 @@ def test_login(self, mock_gp, mock_cli, mock_open, mock_cfg): } }, { - "args": ['login', 'st2admin', '-w'], + "args": ['--config', '/tmp/st2config', 'login', 'st2admin', '-w'], "expected_config": { 'credentials': { "username": 'st2admin', @@ -107,6 +109,13 @@ def test_login(self, mock_gp, mock_cli, mock_open, mock_cfg): mock_gp.getpass.return_value = "Password1!" + # Mock config + if '--config' in test_case['args']: + config_file = test_case['args'][test_case['args'].index('--config') + 1] + else: + config_file = expanduser('~/.st2/config') + mock_cli.return_value._get_config_file_path.return_value = config_file + self.shell.run(test_case['args']) # Ensure getpass was only used if "--password" option was omitted @@ -119,7 +128,7 @@ def test_login(self, mock_gp, mock_cli, mock_open, mock_cfg): mock_cli.return_value._cache_auth_token.assert_called_once() # Ensure configuration was performed properly - config_file = "%s/.st2/config" % expanduser("~") + mock_open.assert_called_once_with(config_file, 'w') mock_cfg.return_value.read.assert_called_once_with(config_file) mock_cfg.return_value.__setitem__.assert_called_once_with( 'credentials', test_case['expected_config']['credentials']) @@ -133,6 +142,7 @@ def test_login(self, mock_gp, mock_cli, mock_open, mock_cfg): mock_cli.return_value._cache_auth_token.reset_mock() mock_cfg.return_value.read.reset_mock() mock_cfg.return_value.__setitem__.reset_mock() + mock_open.reset_mock() mock_open.return_value.__enter__.reset_mock() mock_open.return_value.__exit__.reset_mock() From 7b9dcb6ac01b93fb6928b8d556a15e9549237963 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Fri, 16 Dec 2016 16:02:33 -0800 Subject: [PATCH 10/25] Added mock to config file in shell Signed-off-by: Matt Oswalt --- st2client/tests/unit/test_auth.py | 1 + 1 file changed, 1 insertion(+) diff --git a/st2client/tests/unit/test_auth.py b/st2client/tests/unit/test_auth.py index e290c8641c..827532fea5 100644 --- a/st2client/tests/unit/test_auth.py +++ b/st2client/tests/unit/test_auth.py @@ -112,6 +112,7 @@ def test_login(self, mock_gp, mock_cli, mock_open, mock_cfg): # Mock config if '--config' in test_case['args']: config_file = test_case['args'][test_case['args'].index('--config') + 1] + self.shell._get_config_file_path = mock.MagicMock(return_value="/tmp/st2config") else: config_file = expanduser('~/.st2/config') mock_cli.return_value._get_config_file_path.return_value = config_file From 47529e0bde0b5e3f3a6e39e8bf075ea5d05e9721 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Fri, 16 Dec 2016 16:56:50 -0800 Subject: [PATCH 11/25] Writing password field to config file is now totally optional Signed-off-by: Matt Oswalt --- st2client/st2client/commands/auth.py | 17 +++++----- st2client/tests/unit/test_auth.py | 47 +++++++++++++--------------- 2 files changed, 30 insertions(+), 34 deletions(-) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index d067884e1b..2890b6fded 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -96,7 +96,8 @@ def __init__(self, resource, *args, **kwargs): 'Max TTL configured by the admin supersedes this.') self.parser.add_argument('-w', '--write-real-password', action='store_true', default=False, dest='write_password', - help='Write the real (plain-text) password to the config file') + help='Write the real (plain-text) password to the config file ' + '(default is to omit it') def run(self, args, **kwargs): @@ -127,13 +128,13 @@ def run(self, args, **kwargs): config = ConfigParser() config.read(config_file) - # Other st2 commands error out if the "password" field is missing from the "credentials" - # section. So, here we will write it to "notarealpassword", unless the args.write_password - # option is provided, in which case we'll write the real password. - config['credentials'] = { - "username": args.username, - "password": args.password if args.write_password else "notarealpassword" - } + # Modify config (and optionally populate with password) + if 'credentials' not in config: + config.add_section('credentials') + config['credentials'] == {} + config['credentials']['username'] = args.username + if args.write_password: + config['credentials']['password'] = args.password with open(config_file, "w") as cfg_file_out: config.write(cfg_file_out) diff --git a/st2client/tests/unit/test_auth.py b/st2client/tests/unit/test_auth.py index 827532fea5..27e38852d3 100644 --- a/st2client/tests/unit/test_auth.py +++ b/st2client/tests/unit/test_auth.py @@ -13,6 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +from mock import call from os.path import expanduser import os import uuid @@ -69,39 +70,19 @@ def test_login(self, mock_gp, mock_cli, mock_open, mock_cfg): "args": [ '--config', '/tmp/st2config', 'login', 'st2admin', '--password', 'Password1!' ], - "expected_config": { - 'credentials': { - "username": 'st2admin', - "password": 'notarealpassword' - } - } + "expected_username": 'st2admin' }, { "args": ['login', 'st2admin'], - "expected_config": { - 'credentials': { - "username": 'st2admin', - "password": 'notarealpassword' - } - } + "expected_username": 'st2admin' }, { "args": ['login', 'st2admin', '--password', 'Password1!', '-w'], - "expected_config": { - 'credentials': { - "username": 'st2admin', - "password": 'Password1!' - } - } + "expected_username": 'st2admin' }, { "args": ['--config', '/tmp/st2config', 'login', 'st2admin', '-w'], - "expected_config": { - 'credentials': { - "username": 'st2admin', - "password": 'Password1!' - } - } + "expected_username": 'st2admin' } ] @@ -131,14 +112,28 @@ def test_login(self, mock_gp, mock_cli, mock_open, mock_cfg): # Ensure configuration was performed properly mock_open.assert_called_once_with(config_file, 'w') mock_cfg.return_value.read.assert_called_once_with(config_file) - mock_cfg.return_value.__setitem__.assert_called_once_with( - 'credentials', test_case['expected_config']['credentials']) + mock_cfg.return_value.add_section.assert_called_once_with('credentials') + if '-w' in test_case['args']: + calls = [ + call('username', test_case['expected_username']), call('password', 'Password1!') + ] + else: + calls = [ + call('username', test_case['expected_username']) + ] + mock_cfg.return_value.__getitem__.return_value.__setitem__.assert_has_calls( + calls, + any_order=True + ) + mock_cfg.reset_mock() # Ensure file was written to with a context manager mock_open.return_value.__enter__.assert_called_once() mock_open.return_value.__exit__.assert_called_once() # Reset "called" counters + mock_cfg.return_value.reset_mock() + mock_cfg.return_value.__getitem__.return_value.reset_mock() mock_gp.getpass.reset_mock() mock_cli.return_value._cache_auth_token.reset_mock() mock_cfg.return_value.read.reset_mock() From 9ae4d0750419af06c482cb0187a92c2a0cf78f75 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Tue, 20 Dec 2016 10:35:41 -0800 Subject: [PATCH 12/25] Used existing constants for location of config dir Signed-off-by: Matt Oswalt --- st2client/st2client/commands/auth.py | 4 ++-- st2client/tests/unit/test_auth.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index 2890b6fded..95b1b3bf88 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -14,12 +14,12 @@ # limitations under the License. from configparser import ConfigParser -from os.path import expanduser import getpass import json import logging from st2client.base import BaseCLIApp +from st2client import config_parser from st2client import models from st2client.commands import resource from st2client.commands.noop import NoopCommand @@ -112,7 +112,7 @@ def run(self, args, **kwargs): config_file = cli._get_config_file_path(args) except ValueError: # config file not found in args or in env, defaulting - config_file = expanduser('~/.st2/config') + config_file = config_parser.ST2_CONFIG_PATH logging.basicConfig( level=logging.CRITICAL, diff --git a/st2client/tests/unit/test_auth.py b/st2client/tests/unit/test_auth.py index 27e38852d3..975732a651 100644 --- a/st2client/tests/unit/test_auth.py +++ b/st2client/tests/unit/test_auth.py @@ -14,7 +14,6 @@ # limitations under the License. from mock import call -from os.path import expanduser import os import uuid import json @@ -25,6 +24,7 @@ import logging from tests import base +from st2client import config_parser from st2client import shell from st2client.models.core import add_auth_token_to_kwargs_from_env from st2client.commands.resource import add_auth_token_to_kwargs_from_cli @@ -95,7 +95,7 @@ def test_login(self, mock_gp, mock_cli, mock_open, mock_cfg): config_file = test_case['args'][test_case['args'].index('--config') + 1] self.shell._get_config_file_path = mock.MagicMock(return_value="/tmp/st2config") else: - config_file = expanduser('~/.st2/config') + config_file = config_parser.ST2_CONFIG_PATH mock_cli.return_value._get_config_file_path.return_value = config_file self.shell.run(test_case['args']) From dafb4e1820d897b855d90de75c08425a80c25d0e Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Tue, 20 Dec 2016 11:15:50 -0800 Subject: [PATCH 13/25] Fixed typo in config, and added unit test to catch it Signed-off-by: Matt Oswalt --- st2client/st2client/commands/auth.py | 2 +- st2client/tests/unit/test_auth.py | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index 95b1b3bf88..c1e6eb4454 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -131,7 +131,7 @@ def run(self, args, **kwargs): # Modify config (and optionally populate with password) if 'credentials' not in config: config.add_section('credentials') - config['credentials'] == {} + config['credentials'] = {} config['credentials']['username'] = args.username if args.write_password: config['credentials']['password'] = args.password diff --git a/st2client/tests/unit/test_auth.py b/st2client/tests/unit/test_auth.py index 975732a651..bcec9bfa7c 100644 --- a/st2client/tests/unit/test_auth.py +++ b/st2client/tests/unit/test_auth.py @@ -121,6 +121,9 @@ def test_login(self, mock_gp, mock_cli, mock_open, mock_cfg): calls = [ call('username', test_case['expected_username']) ] + mock_cfg.return_value.__setitem__.assert_has_calls( + [call('credentials', {})], any_order=True + ) mock_cfg.return_value.__getitem__.return_value.__setitem__.assert_has_calls( calls, any_order=True From 2a4d38d6b7e12508f985e45b439d6d67aae684f9 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Tue, 20 Dec 2016 11:18:35 -0800 Subject: [PATCH 14/25] Added parentheses to str concat for clarity Signed-off-by: Matt Oswalt --- st2client/st2client/commands/auth.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index c1e6eb4454..bdf3c1aefc 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -144,9 +144,9 @@ def run(self, args, **kwargs): def run_and_print(self, args, **kwargs): try: self.run(args, **kwargs) - print("Logged in as %s" % args.username) + print("Logged in as %s" % (args.username)) except Exception: - print("Failed to log in as %s" % args.username) + print("Failed to log in as %s" % (args.username)) if self.app.client.debug: raise From 9ca6a40ca131ac80232945d453a04e04636e64c7 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Tue, 20 Dec 2016 11:21:07 -0800 Subject: [PATCH 15/25] Added more detailed message for login failures Signed-off-by: Matt Oswalt --- st2client/st2client/commands/auth.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index bdf3c1aefc..ce4465d263 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -145,8 +145,8 @@ def run_and_print(self, args, **kwargs): try: self.run(args, **kwargs) print("Logged in as %s" % (args.username)) - except Exception: - print("Failed to log in as %s" % (args.username)) + except Exception as e: + print("Failed to log in as %s: %s" % (args.username, str(e))) if self.app.client.debug: raise From ec480ffc3507de6c9081061b1ee3741776f8bba3 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Tue, 20 Dec 2016 11:24:49 -0800 Subject: [PATCH 16/25] Removed unnecessary setUp and tearDown methods Signed-off-by: Matt Oswalt --- st2client/tests/unit/test_auth.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/st2client/tests/unit/test_auth.py b/st2client/tests/unit/test_auth.py index bcec9bfa7c..fff6ac20fe 100644 --- a/st2client/tests/unit/test_auth.py +++ b/st2client/tests/unit/test_auth.py @@ -50,12 +50,6 @@ def __init__(self, *args, **kwargs): self.parser.add_argument('--api-key', dest='api_key') self.shell = shell.Shell() - def setUp(self): - super(TestLogin, self).setUp() - - def tearDown(self): - super(TestLogin, self).tearDown() - @mock.patch.object( requests, 'post', mock.MagicMock(return_value=base.FakeResponse(json.dumps({}), 200, 'OK'))) From 05a498720544422a0e318290573250b23bdb9f39 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Tue, 20 Dec 2016 13:47:30 -0800 Subject: [PATCH 17/25] Cleaned up some unnecesssarily detailed reset_mock calls Signed-off-by: Matt Oswalt --- st2client/tests/unit/test_auth.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/st2client/tests/unit/test_auth.py b/st2client/tests/unit/test_auth.py index fff6ac20fe..9120478a8d 100644 --- a/st2client/tests/unit/test_auth.py +++ b/st2client/tests/unit/test_auth.py @@ -122,22 +122,16 @@ def test_login(self, mock_gp, mock_cli, mock_open, mock_cfg): calls, any_order=True ) - mock_cfg.reset_mock() # Ensure file was written to with a context manager mock_open.return_value.__enter__.assert_called_once() mock_open.return_value.__exit__.assert_called_once() # Reset "called" counters - mock_cfg.return_value.reset_mock() - mock_cfg.return_value.__getitem__.return_value.reset_mock() - mock_gp.getpass.reset_mock() - mock_cli.return_value._cache_auth_token.reset_mock() - mock_cfg.return_value.read.reset_mock() - mock_cfg.return_value.__setitem__.reset_mock() mock_open.reset_mock() - mock_open.return_value.__enter__.reset_mock() - mock_open.return_value.__exit__.reset_mock() + mock_cfg.reset_mock() + mock_cli.reset_mock() + mock_gp.reset_mock() class TestAuthToken(base.BaseCLITestCase): From d45b9af270aca3e93abd78c73f8e0aeefc4abad4 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Tue, 20 Dec 2016 14:14:13 -0800 Subject: [PATCH 18/25] Refactored and split up unit tests for 'st2 login' Signed-off-by: Matt Oswalt --- st2client/tests/unit/test_auth.py | 225 ++++++++++++++++++++---------- 1 file changed, 150 insertions(+), 75 deletions(-) diff --git a/st2client/tests/unit/test_auth.py b/st2client/tests/unit/test_auth.py index 9120478a8d..ec37f29df6 100644 --- a/st2client/tests/unit/test_auth.py +++ b/st2client/tests/unit/test_auth.py @@ -57,81 +57,156 @@ def __init__(self, *args, **kwargs): @mock.patch("st2client.commands.auth.open") @mock.patch("st2client.commands.auth.BaseCLIApp") @mock.patch("st2client.commands.auth.getpass") - def test_login(self, mock_gp, mock_cli, mock_open, mock_cfg): - - test_cases = [ - { - "args": [ - '--config', '/tmp/st2config', 'login', 'st2admin', '--password', 'Password1!' - ], - "expected_username": 'st2admin' - }, - { - "args": ['login', 'st2admin'], - "expected_username": 'st2admin' - }, - { - "args": ['login', 'st2admin', '--password', 'Password1!', '-w'], - "expected_username": 'st2admin' - }, - { - "args": ['--config', '/tmp/st2config', 'login', 'st2admin', '-w'], - "expected_username": 'st2admin' - } - ] - - for test_case in test_cases: - - mock_gp.getpass.return_value = "Password1!" - - # Mock config - if '--config' in test_case['args']: - config_file = test_case['args'][test_case['args'].index('--config') + 1] - self.shell._get_config_file_path = mock.MagicMock(return_value="/tmp/st2config") - else: - config_file = config_parser.ST2_CONFIG_PATH - mock_cli.return_value._get_config_file_path.return_value = config_file - - self.shell.run(test_case['args']) - - # Ensure getpass was only used if "--password" option was omitted - if "--password" in test_case['args']: - mock_gp.getpass.assert_not_called() - else: - mock_gp.getpass.assert_called_once() - - # Ensure token was generated - mock_cli.return_value._cache_auth_token.assert_called_once() - - # Ensure configuration was performed properly - mock_open.assert_called_once_with(config_file, 'w') - mock_cfg.return_value.read.assert_called_once_with(config_file) - mock_cfg.return_value.add_section.assert_called_once_with('credentials') - if '-w' in test_case['args']: - calls = [ - call('username', test_case['expected_username']), call('password', 'Password1!') - ] - else: - calls = [ - call('username', test_case['expected_username']) - ] - mock_cfg.return_value.__setitem__.assert_has_calls( - [call('credentials', {})], any_order=True - ) - mock_cfg.return_value.__getitem__.return_value.__setitem__.assert_has_calls( - calls, - any_order=True - ) - - # Ensure file was written to with a context manager - mock_open.return_value.__enter__.assert_called_once() - mock_open.return_value.__exit__.assert_called_once() - - # Reset "called" counters - mock_open.reset_mock() - mock_cfg.reset_mock() - mock_cli.reset_mock() - mock_gp.reset_mock() + def test_login_password_and_config(self, mock_gp, mock_cli, mock_open, mock_cfg): + """Test the 'st2 login' functionality by providing a password, and specifying a configuration file + """ + + args = ['--config', '/tmp/st2config', 'login', 'st2admin', '--password', 'Password1!'] + expected_username = 'st2admin' + + mock_gp.getpass.return_value = "Password1!" + + # Mock config + config_file = args[args.index('--config') + 1] + self.shell._get_config_file_path = mock.MagicMock(return_value="/tmp/st2config") + mock_cli.return_value._get_config_file_path.return_value = config_file + + self.shell.run(args) + + # Ensure getpass was only used if "--password" option was omitted + mock_gp.getpass.assert_not_called() + + # Ensure token was generated + mock_cli.return_value._cache_auth_token.assert_called_once() + + # Build list of expected calls for mock_cfg + config_calls = [call('username', expected_username)] + + # Run common assertions for testing login functionality + self._login_common_assertions(mock_gp, mock_cli, mock_open, mock_cfg, + config_calls, config_file) + + @mock.patch.object( + requests, 'post', + mock.MagicMock(return_value=base.FakeResponse(json.dumps({}), 200, 'OK'))) + @mock.patch("st2client.commands.auth.ConfigParser") + @mock.patch("st2client.commands.auth.open") + @mock.patch("st2client.commands.auth.BaseCLIApp") + @mock.patch("st2client.commands.auth.getpass") + def test_login_no_password(self, mock_gp, mock_cli, mock_open, mock_cfg): + """Test the 'st2 login' functionality without the "--password" arg + """ + + args = ['login', 'st2admin'] + expected_username = 'st2admin' + + mock_gp.getpass.return_value = "Password1!" + + config_file = config_parser.ST2_CONFIG_PATH + mock_cli.return_value._get_config_file_path.return_value = config_file + + self.shell.run(args) + + # Ensure getpass was only used if "--password" option was omitted + mock_gp.getpass.assert_called_once() + + # Ensure token was generated + mock_cli.return_value._cache_auth_token.assert_called_once() + + config_calls = [call('username', expected_username)] + + # Run common assertions for testing login functionality + self._login_common_assertions(mock_gp, mock_cli, mock_open, mock_cfg, + config_calls, config_file) + + @mock.patch.object( + requests, 'post', + mock.MagicMock(return_value=base.FakeResponse(json.dumps({}), 200, 'OK'))) + @mock.patch("st2client.commands.auth.ConfigParser") + @mock.patch("st2client.commands.auth.open") + @mock.patch("st2client.commands.auth.BaseCLIApp") + @mock.patch("st2client.commands.auth.getpass") + def test_login_password_with_write(self, mock_gp, mock_cli, mock_open, mock_cfg): + """Test the 'st2 login' functionality by providing a password, and writing it to config file + """ + + args = ['login', 'st2admin', '--password', 'Password1!', '-w'] + expected_username = 'st2admin' + + mock_gp.getpass.return_value = "Password1!" + + config_file = config_parser.ST2_CONFIG_PATH + mock_cli.return_value._get_config_file_path.return_value = config_file + + self.shell.run(args) + + # Ensure getpass was only used if "--password" option was omitted + mock_gp.getpass.assert_not_called() + + # Ensure token was generated + mock_cli.return_value._cache_auth_token.assert_called_once() + + # Build list of expected calls for mock_cfg + config_calls = [call('username', expected_username), call('password', 'Password1!')] + + # Run common assertions for testing login functionality + self._login_common_assertions(mock_gp, mock_cli, mock_open, mock_cfg, + config_calls, config_file) + + @mock.patch.object( + requests, 'post', + mock.MagicMock(return_value=base.FakeResponse(json.dumps({}), 200, 'OK'))) + @mock.patch("st2client.commands.auth.ConfigParser") + @mock.patch("st2client.commands.auth.open") + @mock.patch("st2client.commands.auth.BaseCLIApp") + @mock.patch("st2client.commands.auth.getpass") + def test_login_no_password_with_write_and_config(self, mock_gp, mock_cli, mock_open, mock_cfg): + """Test the 'st2 login' functionality by providing config file and writing password to it + """ + + args = ['--config', '/tmp/st2config', 'login', 'st2admin', '-w'] + expected_username = 'st2admin' + + mock_gp.getpass.return_value = "Password1!" + + # Mock config + config_file = args[args.index('--config') + 1] + self.shell._get_config_file_path = mock.MagicMock(return_value="/tmp/st2config") + mock_cli.return_value._get_config_file_path.return_value = config_file + + self.shell.run(args) + + mock_gp.getpass.assert_called_once() + + # Ensure token was generated + mock_cli.return_value._cache_auth_token.assert_called_once() + + # Build list of expected calls for mock_cfg + config_calls = [call('username', expected_username), call('password', 'Password1!')] + + # Run common assertions for testing login functionality + self._login_common_assertions(mock_gp, mock_cli, mock_open, mock_cfg, + config_calls, config_file) + + def _login_common_assertions(self, mock_gp, mock_cli, mock_open, mock_cfg, + config_calls, config_file): + # Ensure file was written to with a context manager + mock_open.return_value.__enter__.assert_called_once() + mock_open.return_value.__exit__.assert_called_once() + + # Make sure 'credentials' key in config was initialized properly + mock_cfg.return_value.__setitem__.assert_has_calls( + [call('credentials', {})], any_order=True + ) + + # Ensure configuration was performed properly + mock_open.assert_called_once_with(config_file, 'w') + mock_cfg.return_value.read.assert_called_once_with(config_file) + mock_cfg.return_value.add_section.assert_called_once_with('credentials') + mock_cfg.return_value.__getitem__.return_value.__setitem__.assert_has_calls( + config_calls, + any_order=True + ) class TestAuthToken(base.BaseCLITestCase): From 7a2ce3952dd39f3d00475ca338ccb4de3213e983 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Fri, 3 Feb 2017 11:14:08 -0800 Subject: [PATCH 19/25] Updated write-password arg to better reflect functionality Signed-off-by: Matt Oswalt --- st2client/st2client/commands/auth.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index ce4465d263..208defd20e 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -94,9 +94,9 @@ def __init__(self, resource, *args, **kwargs): self.parser.add_argument('-l', '--ttl', type=int, dest='ttl', default=None, help='The life span of the token in seconds. ' 'Max TTL configured by the admin supersedes this.') - self.parser.add_argument('-w', '--write-real-password', action='store_true', default=False, + self.parser.add_argument('-w', '--write-password', action='store_true', default=False, dest='write_password', - help='Write the real (plain-text) password to the config file ' + help='Write the password in plain text to the config file ' '(default is to omit it') def run(self, args, **kwargs): From 079c89b266a67a41a854056b04b491aa8386fbed Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Fri, 3 Feb 2017 11:41:13 -0800 Subject: [PATCH 20/25] Provided more reasonable fix to previous logging issue Signed-off-by: Matt Oswalt --- st2client/st2client/base.py | 3 ++- st2client/st2client/commands/auth.py | 10 +++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/st2client/st2client/base.py b/st2client/st2client/base.py index b0269d7198..58c8907402 100644 --- a/st2client/st2client/base.py +++ b/st2client/st2client/base.py @@ -15,6 +15,7 @@ import os import json +import logging import time import calendar import traceback @@ -56,7 +57,7 @@ class BaseCLIApp(object): Base class for StackStorm CLI apps. """ - LOG = None # logger instance to use + LOG = logging.getLogger(__name__) # logger instance to use client = None # st2client instance # A list of command classes for which automatic authentication should be skipped. diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index 208defd20e..3579ff5038 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -114,11 +114,11 @@ def run(self, args, **kwargs): # config file not found in args or in env, defaulting config_file = config_parser.ST2_CONFIG_PATH - logging.basicConfig( - level=logging.CRITICAL, - handlers=[logging.NullHandler] - ) - cli.LOG = logging.getLogger("st2client.base") + # logging.basicConfig( + # level=logging.CRITICAL, + # handlers=[logging.NullHandler] + # ) + # LOG = logging.getLogger("st2client.base") # Retrieve token manager = self.manager.create(instance, auth=(args.username, args.password), **kwargs) From 8f68d5547145d7e50b2968b9884b4b1c49315785 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Fri, 3 Feb 2017 11:50:58 -0800 Subject: [PATCH 21/25] Removed commented out old code Signed-off-by: Matt Oswalt --- st2client/st2client/commands/auth.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index 3579ff5038..9fba251d0b 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -114,12 +114,6 @@ def run(self, args, **kwargs): # config file not found in args or in env, defaulting config_file = config_parser.ST2_CONFIG_PATH - # logging.basicConfig( - # level=logging.CRITICAL, - # handlers=[logging.NullHandler] - # ) - # LOG = logging.getLogger("st2client.base") - # Retrieve token manager = self.manager.create(instance, auth=(args.username, args.password), **kwargs) cli._cache_auth_token(token_obj=manager) From 5f877196377f146d666ca766ae2f7bda8ae4dd13 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Fri, 3 Feb 2017 15:15:26 -0800 Subject: [PATCH 22/25] Introduce 'st2 whoami' command and tests --- st2client/st2client/commands/auth.py | 43 ++++++++++++ st2client/st2client/shell.py | 3 + st2client/tests/unit/test_auth.py | 99 ++++++++++++++++++++++++++++ 3 files changed, 145 insertions(+) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index 9fba251d0b..4e477d834c 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -145,6 +145,49 @@ def run_and_print(self, args, **kwargs): raise +class WhoamiCommand(resource.ResourceCommand): + display_attributes = ['user', 'token', 'expiry'] + + def __init__(self, resource, *args, **kwargs): + + kwargs['has_token_opt'] = False + + super(WhoamiCommand, self).__init__( + resource, kwargs.pop('name', 'create'), + 'Display the currently authenticated/configured user', + *args, **kwargs) + + def run(self, args, **kwargs): + + cli = BaseCLIApp() + + # Determine path to config file + try: + config_file = cli._get_config_file_path(args) + except ValueError: + # config file not found in args or in env, defaulting + config_file = config_parser.ST2_CONFIG_PATH + + # Update existing configuration with new credentials + config = ConfigParser() + config.read(config_file) + + return config['credentials']['username'] + + def run_and_print(self, args, **kwargs): + try: + username = self.run(args, **kwargs) + print("Currently logged in as %s" % username) + except KeyError: + print("No user is currently logged in") + if self.app.client.debug: + raise + except Exception: + print("Unable to retrieve currently logged-in user") + if self.app.client.debug: + raise + + class ApiKeyBranch(resource.ResourceBranch): def __init__(self, description, app, subparsers, parent_parser=None): diff --git a/st2client/st2client/shell.py b/st2client/st2client/shell.py index ca7a02af1b..c26c302984 100755 --- a/st2client/st2client/shell.py +++ b/st2client/st2client/shell.py @@ -239,6 +239,9 @@ def __init__(self): 'Webhooks.', self, self.subparsers) + self.commands['whoami'] = auth.WhoamiCommand( + models.Token, self, self.subparsers, name='whoami') + self.commands['timer'] = timer.TimerBranch( 'Timers.', self, self.subparsers) diff --git a/st2client/tests/unit/test_auth.py b/st2client/tests/unit/test_auth.py index ec37f29df6..36ae9e70eb 100644 --- a/st2client/tests/unit/test_auth.py +++ b/st2client/tests/unit/test_auth.py @@ -41,6 +41,105 @@ } +class TestWhoami(base.BaseCLITestCase): + + def __init__(self, *args, **kwargs): + super(TestWhoami, self).__init__(*args, **kwargs) + self.parser = argparse.ArgumentParser() + self.parser.add_argument('-t', '--token', dest='token') + self.parser.add_argument('--api-key', dest='api_key') + self.shell = shell.Shell() + + @mock.patch.object( + requests, 'post', + mock.MagicMock(return_value=base.FakeResponse(json.dumps({}), 200, 'OK'))) + @mock.patch("st2client.commands.auth.ConfigParser") + @mock.patch("st2client.commands.auth.open") + @mock.patch("st2client.commands.auth.BaseCLIApp") + @mock.patch("st2client.commands.auth.getpass") + def test_whoami(self, mock_gp, mock_cli, mock_open, mock_cfg): + """Test 'st2 whoami' functionality + """ + + # Mock config + config_file = config_parser.ST2_CONFIG_PATH + self.shell._get_config_file_path = mock.MagicMock(return_value="/tmp/st2config") + mock_cli.return_value._get_config_file_path.return_value = config_file + + self.shell.run(['whoami']) + + mock_cfg.return_value.__getitem__.assert_called_with('credentials') + mock_cfg.return_value.__getitem__('credentials').__getitem__.assert_called_with('username') + + @mock.patch.object( + requests, 'post', + mock.MagicMock(return_value=base.FakeResponse(json.dumps({}), 200, 'OK'))) + @mock.patch("st2client.commands.auth.ConfigParser") + @mock.patch("st2client.commands.auth.open") + @mock.patch("st2client.commands.auth.BaseCLIApp") + @mock.patch("st2client.commands.auth.getpass") + def test_whoami_not_logged_in(self, mock_gp, mock_cli, mock_open, mock_cfg): + """Test 'st2 whoami' functionality with a missing username + """ + + # Mock config + config_file = config_parser.ST2_CONFIG_PATH + self.shell._get_config_file_path = mock.MagicMock(return_value="/tmp/st2config") + mock_cli.return_value._get_config_file_path.return_value = config_file + + # Trigger keyerror exception when trying to access username. + # We have to do it this way because ConfigParser acts like a + # dict but also has methods like read() + attrs = {'__getitem__.side_effect': KeyError} + mock_cfg.return_value.__getitem__.return_value.configure_mock(**attrs) + + # assert that the config field lookup caused the CLI to return an error code + # we are also using "--debug" flag to ensure the exception is re-raised once caught + self.assertEqual( + self.shell.run(['--debug', 'whoami']), + 1 + ) + + # Some additional asserts to ensure things are being called correctly + mock_cfg.return_value.__getitem__.assert_called_with('credentials') + mock_cfg.return_value.__getitem__.return_value.__getitem__.assert_called_with('username') + + @mock.patch.object( + requests, 'post', + mock.MagicMock(return_value=base.FakeResponse(json.dumps({}), 200, 'OK'))) + @mock.patch("st2client.commands.auth.ConfigParser") + @mock.patch("st2client.commands.auth.open") + @mock.patch("st2client.commands.auth.BaseCLIApp") + @mock.patch("st2client.commands.auth.getpass") + def test_whoami_missing_credentials_section(self, mock_gp, mock_cli, mock_open, mock_cfg): + """Test 'st2 whoami' functionality with a missing credentials section + """ + + # mocked config that is empty (no credentials section at all) + mock_cfg.return_value.read.return_value = {} + + # Mock config + config_file = config_parser.ST2_CONFIG_PATH + self.shell._get_config_file_path = mock.MagicMock(return_value="/tmp/st2config") + mock_cli.return_value._get_config_file_path.return_value = config_file + + # Trigger keyerror exception when trying to access username. + # We have to do it this way because ConfigParser acts like a + # dict but also has methods like read() + attrs = {'__getitem__.side_effect': KeyError} + mock_cfg.return_value.configure_mock(**attrs) + + # assert that the config field lookup caused the CLI to return an error code + # we are also using "--debug" flag to ensure the exception is re-raised once caught + self.assertEqual( + self.shell.run(['--debug', 'whoami']), + 1 + ) + + # An additional assert to ensure things are being called correctly + mock_cfg.return_value.__getitem__.assert_called_with('credentials') + + class TestLogin(base.BaseCLITestCase): def __init__(self, *args, **kwargs): From 3f2c565fbf05a15124a88354d0a2a2dd1ac94db1 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Fri, 3 Feb 2017 15:19:50 -0800 Subject: [PATCH 23/25] Updated changelog --- CHANGELOG.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 2cdbabfd29..e149fcfdbf 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -49,6 +49,10 @@ in development put the workflow in a PAUSED state in mistral. (improvement) * Add support for evaluating jinja expressions in mistral workflow definition where yaql expressions are typically accepted. (improvement) +# Added support for `st2 login` and `st2 whoami` commands. These add some additional functionality + beyond the existing `st2 auth` command and actually works with the local configuration so that + users do not have to. + 2.1.1 - December 16, 2016 ------------------------- From da93e1b070aa769571ce524121f471632a82db00 Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Fri, 3 Feb 2017 17:44:09 -0800 Subject: [PATCH 24/25] Add logic to remove password field so that previous passwords don't linger Signed-off-by: Matt Oswalt --- st2client/st2client/commands/auth.py | 3 +++ st2client/tests/unit/test_auth.py | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/st2client/st2client/commands/auth.py b/st2client/st2client/commands/auth.py index 4e477d834c..ecf37bbfe6 100644 --- a/st2client/st2client/commands/auth.py +++ b/st2client/st2client/commands/auth.py @@ -129,6 +129,9 @@ def run(self, args, **kwargs): config['credentials']['username'] = args.username if args.write_password: config['credentials']['password'] = args.password + else: + # Remove any existing password from config + config['credentials'].pop('password', None) with open(config_file, "w") as cfg_file_out: config.write(cfg_file_out) diff --git a/st2client/tests/unit/test_auth.py b/st2client/tests/unit/test_auth.py index 36ae9e70eb..a35cede30a 100644 --- a/st2client/tests/unit/test_auth.py +++ b/st2client/tests/unit/test_auth.py @@ -181,6 +181,9 @@ def test_login_password_and_config(self, mock_gp, mock_cli, mock_open, mock_cfg) # Build list of expected calls for mock_cfg config_calls = [call('username', expected_username)] + # Ensure that the password field was removed from the config + mock_cfg.return_value.__getitem__.return_value.pop.assert_called_once_with('password', None) + # Run common assertions for testing login functionality self._login_common_assertions(mock_gp, mock_cli, mock_open, mock_cfg, config_calls, config_file) @@ -214,6 +217,9 @@ def test_login_no_password(self, mock_gp, mock_cli, mock_open, mock_cfg): config_calls = [call('username', expected_username)] + # Ensure that the password field was removed from the config + mock_cfg.return_value.__getitem__.return_value.pop.assert_called_once_with('password', None) + # Run common assertions for testing login functionality self._login_common_assertions(mock_gp, mock_cli, mock_open, mock_cfg, config_calls, config_file) From 3da2b42f05de1fc1c922a5a5c01c8bbb3088feff Mon Sep 17 00:00:00 2001 From: Matt Oswalt Date: Mon, 6 Feb 2017 13:46:37 -0800 Subject: [PATCH 25/25] Changed erroneous hash to star in changelog Signed-off-by: Matt Oswalt --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index e149fcfdbf..e99697114c 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -49,7 +49,7 @@ in development put the workflow in a PAUSED state in mistral. (improvement) * Add support for evaluating jinja expressions in mistral workflow definition where yaql expressions are typically accepted. (improvement) -# Added support for `st2 login` and `st2 whoami` commands. These add some additional functionality +* Added support for `st2 login` and `st2 whoami` commands. These add some additional functionality beyond the existing `st2 auth` command and actually works with the local configuration so that users do not have to.