Skip to content

Make 'st2 login' actually work #3219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Kami merged 14 commits into from
Feb 15, 2017
Merged

Make 'st2 login' actually work #3219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4c5987a
Initial work on fixing `st2 login` bug.
Feb 15, 2017
89057ee
Removed dumb extra space
Feb 15, 2017
f755d03
Removed unused import
Feb 15, 2017
4b8a146
Add missing import and logincommand class back so st2 login works.
Kami Feb 15, 2017
7d09f74
Skip auth on st2 whoami command.
Kami Feb 15, 2017
cd89193
Update the test - no token is sent in the headers to the auth api, just
Kami Feb 15, 2017
545c7ba
Add test case which verifies that st2 login works correctly and that
Kami Feb 15, 2017
a59dd26
Fix typo.
Kami Feb 15, 2017
3418ffe
Print a message / notice about token expiration if the user doesn't use
Kami Feb 15, 2017
772991b
Fix lint.
Kami Feb 15, 2017
62cbad8
Update message.
Kami Feb 15, 2017
a1a2fa4
Merge branch 'master' into fix-st2-login-token
Kami Feb 15, 2017
5f7b2e5
st2clien't can't rely on st2common so we need to hard-code this default
Kami Feb 15, 2017
7ddca45
Update comment to avoid lint failure.
Kami Feb 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion st2client/st2client/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ def get_client(self, args, debug=False):
password = credentials.get('password', None)
cache_token = rc_config.get('cli', {}).get('cache_token', False)

if username and password:
if credentials:
Copy link
Contributor

@lakshmi-kannan lakshmi-kannan Feb 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I realized is if password is not written to config file and token expires, the code doesn't handle the case correctly. So if password is not present, we have to throw an exception that user needs to re-login with credentials or write password to config.

Copy link
Member

@Kami Kami Feb 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as "not handling that case correctly" - that's an expect behavior, but yes, as discussed on Slack, we should improve on it and print a message if user doesn't use -w option that token will expire in X hours and user will need to re-login.

And yeah, we can perhaps do the same on "token expired error" (advise to re-login), but we don't really know if the exception is related to expired token or it's simply an invalid token so we need to keep that in mind so the message needs to be more advisory.

StackStorm/st2docs#380 (comment)

Copy link
Contributor

@lakshmi-kannan lakshmi-kannan Feb 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's simply an invalid token so we need to keep that in mind so the message needs to be more advisory

Yes, this is what I want. Sorry to confuse with the word "exception". Right now the message we get when a token expires with this change is not ideal. See https://gist.github.com/lakshmi-kannan/31c4f5cbc10f81f382ae76380ae1994c#file-gistfile1-txt-L9

Copy link
Member

@Kami Kami Feb 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the code to print a message on successful login in 3418ffe.

st2 login testu -p testp
Logged in as testu

Note: You didn't use --write-password option so the password hasn't been stored in the client config and you will need to login again in 24 hours when the auth token expires.
As an alternative you can run st2 login command with the "--write-password" flag, but keep it mind this will cause it to store the password in plain-text in the client config file.

# Credentials are provided, try to authenticate agaist the API
try:
token = self._get_auth_token(client=client, username=username, password=password,
Expand Down
18 changes: 16 additions & 2 deletions st2client/st2client/commands/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,12 +142,26 @@ def run(self, args, **kwargs):
def run_and_print(self, args, **kwargs):
try:
self.run(args, **kwargs)
print("Logged in as %s" % (args.username))
except Exception as e:
print("Failed to log in as %s: %s" % (args.username, str(e)))
print('Failed to log in as %s: %s' % (args.username, str(e)))
if self.app.client.debug:
raise

print('Logged in as %s' % (args.username))

if not args.write_password:
# Note: Client can't depend and import from common so we need to hard-code this
# default value
token_expire_hours = 24

print('')
print('Note: You didn\'t use --write-password option so the password hasn\'t been '
'stored in the client config and you will need to login again in %s hours when '
'the auth token expires.' % (token_expire_hours))
print('As an alternative, you can run st2 login command with the "--write-password" '
'flag, but keep it mind this will cause it to store the password in plain-text '
'in the client config file (~/.st2/config).')


class WhoamiCommand(resource.ResourceCommand):
display_attributes = ['user', 'token', 'expiry']
Expand Down
8 changes: 6 additions & 2 deletions st2client/st2client/shell.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,10 @@
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, LoginCommand
from st2client.commands.auth import TokenCreateCommand
from st2client.commands.auth import LoginCommand
from st2client.commands.auth import WhoamiCommand


__all__ = [
'Shell'
Expand All @@ -66,7 +69,8 @@ class Shell(BaseCLIApp):

SKIP_AUTH_CLASSES = [
TokenCreateCommand.__name__,
LoginCommand.__name__
LoginCommand.__name__,
WhoamiCommand.__name__
]

def __init__(self):
Expand Down
4 changes: 3 additions & 1 deletion st2client/tests/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ class BaseCLITestCase(unittest2.TestCase):
stdout = six.moves.StringIO()
stderr = six.moves.StringIO()

DEFAULT_SKIP_CONFIG = '1'

def setUp(self):
super(BaseCLITestCase, self).setUp()

Expand All @@ -86,7 +88,7 @@ def setUp(self):
if var in os.environ:
del os.environ[var]

os.environ['ST2_CLI_SKIP_CONFIG'] = '1'
os.environ['ST2_CLI_SKIP_CONFIG'] = self.DEFAULT_SKIP_CONFIG

if self.capture_output:
# Make sure we reset it for each test class instance
Expand Down
26 changes: 24 additions & 2 deletions st2client/tests/unit/test_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ class TestLoginBase(base.BaseCLITestCase):
def __init__(self, *args, **kwargs):
super(TestLoginBase, self).__init__(*args, **kwargs)

# We're overriding the default behavior for CLI test cases here
self.DEFAULT_SKIP_CONFIG = '0'

self.parser = argparse.ArgumentParser()
self.parser.add_argument('-t', '--token', dest='token')
self.parser.add_argument('--api-key', dest='api_key')
Expand Down Expand Up @@ -142,6 +145,9 @@ class TestLoginIntPwdAndConfig(TestLoginBase):
@mock.patch.object(
requests, 'post',
mock.MagicMock(return_value=base.FakeResponse(json.dumps(TOKEN), 200, 'OK')))
@mock.patch.object(
requests, 'get',
mock.MagicMock(return_value=base.FakeResponse(json.dumps({}), 200, 'OK')))
@mock.patch('st2client.commands.auth.getpass')
def runTest(self, mock_gp):
'''Test 'st2 login' functionality with interactive password entry
Expand All @@ -154,10 +160,14 @@ def runTest(self, mock_gp):

self.shell.run(args)

with open(self.CONFIG_FILE, 'r') as config_file:
expected_kwargs = {
'headers': {'content-type': 'application/json'},
'auth': ('st2admin', 'Password1!')
}
requests.post.assert_called_with('http://127.0.0.1:9100/tokens', '{}', **expected_kwargs)

with open(self.CONFIG_FILE, 'r') as config_file:
for line in config_file.readlines():

# Make sure certain values are not present
self.assertFalse('password' in line)
self.assertFalse('olduser' in line)
Expand All @@ -169,6 +179,18 @@ def runTest(self, mock_gp):
# validate token was created
self.assertTrue(os.path.isfile('%stoken-%s' % (self.DOTST2_PATH, expected_username)))

# Validate token is sent on subsequent requests to st2 API
args = ['--config', self.CONFIG_FILE, 'pack', 'list']
self.shell.run(args)

expected_kwargs = {
'headers': {
'X-Auth-Token': self.TOKEN['token']
},
'params': {}
}
requests.get.assert_called_with('http://127.0.0.1:9101/v1/packs', **expected_kwargs)


class TestLoginWritePwdOkay(TestLoginBase):

Expand Down