Make 'st2 login' actually work #3219

Mierdin · 2017-02-15T00:47:56Z

In st2client/base.py, there's a conditional that detects if the username and password
is provided, and if so, try to retrieve a token and update the token attribute of the
client before returning. However, in a traditional st2 login scenario, only the
username field is populated, so this code will never run.

Previous tests were done on systems that effectively had auth disabled (doh!), so while
the config file and token files were being changed/generated correctly, API requests
did not carry the X-Auth-Token header. Since auth was disabled, this was no problem.
However, on a real installation, this feature just plainly did not work.

In short, this allows the 'st2 login' feature to work, and it also provides unit tests
to help prevent this kind of thing

Add another unit test to check that other commands also send auth header after being configured with st2 login

In st2client/base.py, there's a conditional that detects if the username and password is provided, and if so, try to retrieve a token and update the token attribute of the client before returning. However, in a traditional `st2 login` scenario, only the 'username' field is populated, so this code will never run. Previous tests were done on systems that effectively had auth disabled (doh!), so while the config file and token files were being changed/generated correctly, API requests did not carry the 'X-Auth-Token' header. Since auth was disabled, this was no problem. However, on a real installation, this feature just plainly did not work. In short, this allows the 'st2 login' feature to work, and it also provides unit tests to help prevent this kind of thing Signed-off-by: Matt Oswalt <oswaltm@brocade.com>

Signed-off-by: Matt Oswalt <oswaltm@brocade.com>

codecov-io · 2017-02-15T01:03:43Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a0157d5). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #3219   +/-   ##
=========================================
  Coverage          ?   77.81%           
=========================================
  Files             ?      433           
  Lines             ?    22416           
  Branches          ?        0           
=========================================
  Hits              ?    17443           
  Misses            ?     4973           
  Partials          ?        0

Impacted Files	Coverage Δ
st2client/st2client/commands/auth.py	`69.61% <ø> (ø)`
st2client/st2client/base.py	`82.32% <100%> (ø)`
st2client/st2client/shell.py	`85.6% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0157d5...5f7b2e5. Read the comment docs.

Kami · 2017-02-15T09:39:54Z

Good catch - thanks.

Will test and confirm locally it works (with auth enabled :)).

Kami · 2017-02-15T09:55:06Z

st2client/st2client/shell.py


    SKIP_AUTH_CLASSES = [
-        TokenCreateCommand.__name__,
-        LoginCommand.__name__


LoginCommand needs to be here otherwise it won't work - I confirmed it locally.

On a related note - we need a test case to catch this.

If LoginCommand class is not here, st2 login won't work so we should have a test which catches this :)

Kami · 2017-02-15T09:55:25Z

I confirmed it's working locally with this change - 4b8a146.

We really just want to read a username from config, etc. and don't want to perform any additional auth requests.

Kami · 2017-02-15T10:00:19Z

I pushed another change to skip auth api call when running st2 whoami command - 7d09f74

Imo (please correct me if I'm wrong), there is no need to try to authenticate and hit the auth API when running st2 whoami command - we just want to read the username from config file / environment variable.

Kami · 2017-02-15T10:01:50Z

st2client/tests/unit/test_auth.py

+        # TODO(mierdin): This tests that this particular command sends X-Auth-Token but you should
+        # also test other commands after this token has been installed
+        kwargs = {
+            'headers': {'X-Auth-Token': self.TOKEN['token'], 'content-type': 'application/json'},


This test was correct since this request is hitting auth api (and not regular API) to authenticate to retrieve the token and as such, only username and password should be sent (and token returned in the response and used on subsequent requests to the st2 api).

Alright, I belive this should do it - cd89193, 545c7ba.

username and password and token is returned back.

X-Auth-Token is indeed sent in the header in subsequent requests after st2 login to st2 API.

Kami · 2017-02-15T10:11:28Z

I believe this should do it as far as current state of st2 tests goes - https://github.com/StackStorm/st2/pull/3219/files#r101242702

Some additional st2tests (aka integration tests) would also be nice at some point.

Kami · 2017-02-15T13:38:10Z

Same story for e2e tests here - #3215 (comment)

lakshmi-kannan · 2017-02-15T14:31:41Z

st2client/st2client/base.py

        cache_token = rc_config.get('cli', {}).get('cache_token', False)

-        if username and password:
+        if credentials:


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.

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)

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

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.

--write-password flag.

value.

Mierdin · 2017-02-16T02:34:14Z

@Kami @lakshmi-kannan was CI finished when this was merged?

arm4b · 2017-02-16T12:16:32Z

I think we should enable in GitHub setting which will block merging if some tests are still red/unfinished.
Hopefully this will push us more to improve the build machinery.

Bug introduced after merging this PR was actually caught in st2-packages build: https://circleci.com/gh/StackStorm/st2-packages/1886

/bin/sh -c st2\ run\ core.local\ --\ hostname

Failed to authenticate with credentials provided in the config.
ERROR: 401 Client Error: Unauthorized
MESSAGE: Invalid or missing credentials for url: http://127.0.0.1:9100/tokens

when there is no ./st2/config file and auth is disabled in st2.conf.

Mierdin · 2017-02-16T19:54:11Z

@armab just for my own sanity - after circle runs basic testing like unit and integration, things eventually go back to circle for actual package build right? Just curious we have checks in place for this or if this would have only shown up post-merge.

arm4b · 2017-02-16T20:03:45Z

@Mierdin You're right.
Actually in your PR #3226 8 successful checks is a good demonstration how it should work.

Matt Oswalt added 3 commits February 14, 2017 16:42

Removed dumb extra space

89057ee

Signed-off-by: Matt Oswalt <oswaltm@brocade.com>

Removed unused import

f755d03

Signed-off-by: Matt Oswalt <oswaltm@brocade.com>

Add missing import and logincommand class back so st2 login works.

4b8a146

Kami reviewed Feb 15, 2017

View reviewed changes

Skip auth on st2 whoami command.

7d09f74

We really just want to read a username from config, etc. and don't want to perform any additional auth requests.

Kami reviewed Feb 15, 2017

View reviewed changes

Kami added 3 commits February 15, 2017 11:02

Update the test - no token is sent in the headers to the auth api, just

cd89193

username and password and token is returned back.

Add test case which verifies that st2 login works correctly and that

545c7ba

X-Auth-Token is indeed sent in the header in subsequent requests after st2 login to st2 API.

Fix typo.

a59dd26

Kami mentioned this pull request Feb 15, 2017

Fix pylint on runner Python modules #3215

Merged

lakshmi-kannan reviewed Feb 15, 2017

View reviewed changes

lakshmi-kannan approved these changes Feb 15, 2017

View reviewed changes

Kami added 6 commits February 15, 2017 16:57

Print a message / notice about token expiration if the user doesn't use

3418ffe

--write-password flag.

Fix lint.

772991b

Update message.

62cbad8

Merge branch 'master' into fix-st2-login-token

a1a2fa4

st2clien't can't rely on st2common so we need to hard-code this default

5f7b2e5

value.

Update comment to avoid lint failure.

7ddca45

Kami merged commit 3bb86ec into master Feb 15, 2017

Kami deleted the fix-st2-login-token branch February 15, 2017 17:27

Mierdin mentioned this pull request Feb 16, 2017

Change conditional to point to username field in client config #3226

Merged

Uh oh!

Make 'st2 login' actually work #3219

Make 'st2 login' actually work #3219

Uh oh!

Conversation

Mierdin commented Feb 15, 2017 • edited by Kami Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Kami commented Feb 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kami commented Feb 15, 2017

Uh oh!

Kami commented Feb 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kami commented Feb 15, 2017

Uh oh!

Kami commented Feb 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mierdin commented Feb 16, 2017

Uh oh!

arm4b commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mierdin commented Feb 16, 2017

Uh oh!

arm4b commented Feb 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Mierdin commented Feb 15, 2017 •

edited by Kami

Loading

codecov-io commented Feb 15, 2017 •

edited

Loading

arm4b commented Feb 16, 2017 •

edited

Loading