-
-
Notifications
You must be signed in to change notification settings - Fork 782
Adding support for st2 login and st2 whoami commands
#3123
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.
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
Conversation
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
bigmstone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. I have a few questions/comments. Also what do you do in the event that the ~/.st2 folder doesn't exist? I'm of the opinion this tool should create it.
st2client/st2client/commands/auth.py
Outdated
| cli._cache_auth_token(token_obj=manager) | ||
|
|
||
| # Update existing configuration with new credentials | ||
| config_file = "%s/.st2/config" % expanduser("~") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use os.path.join or expanduser('~.st2/config') here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (03fb3f6).
| # Update existing configuration with new credentials | ||
| config_file = "%s/.st2/config" % expanduser("~") | ||
| config = ConfigParser() | ||
| config.read(config_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if this fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No exception is raised, just an empty list is returned.
(Pdb) config.read("/tmp/foodoesntexist.txt")
[]
This just means there wasn't a file there to populate the config object.
If I run this with the real code, and the config file already exists, you see this:
(Pdb) config.read(config_file)
['/home/vagrant/.st2/config']
Either way, we're always overwriting the two parameters in the "credentials" section, and writing it to disk.
| "username": args.username, | ||
| "password": args.password | ||
| } | ||
| with open(config_file, "w") as cfg_file_out: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be w+ to create file if it doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that w will still create the file; w+ is used if you want to also read from the file, which in this case I don't care about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, my mistake.
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
The You think this still warrants more checks within this function, or is that sufficient? |
Codecov Report
@@ Coverage Diff @@
## master #3123 +/- ##
==========================================
- Coverage 77.65% 77.65% -<.01%
==========================================
Files 433 432 -1
Lines 22323 22390 +67
==========================================
+ Hits 17334 17385 +51
- Misses 4989 5005 +16
Continue to review full report at Codecov.
|
|
@Mierdin Looked over |
bigmstone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you get the linting stuff fixed then you should be 👍. Might want to get someone else to offer their +1 on direction here, too. I personally like the st2 login UX but others might have a different opinion.
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
|
@bigmstone wanna take another quick look? Lots of changes in eb3aedc so maybe you might want to take a peek again? |
st2client/st2client/commands/auth.py
Outdated
| cli._cache_auth_token(token_obj=manager) | ||
|
|
||
| # Update existing configuration with new credentials | ||
| config_file = expanduser('~/.st2/config') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is probably constant somewhere to store the ~/.st2/config default value.
Additionally, here might be edge cases:
what if env ST2_CONFIG_FILE or --config-file= flag is provided? See the docs: https://docs.stackstorm.com/reference/cli.html?highlight=cli%20authentication#configuration-file
I think we should support that for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea, and I used the function _get_config_file_path in BaseCLI to ensure this consistency. Implemented in 683cb4d
This is how it works now (note if the --config option is omitted, that still works and writes to the default location:
(virtualenv)vagrant@st2dev:~/st2/st2client$ st2 --config /tmp/st2config login st2admin --password Password1!
Logged in as st2admin
(virtualenv)vagrant@st2dev:~/st2/st2client$ cat /tmp/st2config
[credentials]
username = st2admin
password = notarealpassword
st2client/st2client/commands/auth.py
Outdated
| # 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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing notarealpassword to conf file looks really really strange
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What else should I write? I originally tried omitting the field altogether, but it seems that other st2 commands actually require this field, yet don't use it. Writing an obviously fake password to this field seemed like the simplest approach.
| } | ||
|
|
||
| with open(config_file, "w") as cfg_file_out: | ||
| config.write(cfg_file_out) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if correct credentials already exist in the file. Do we overwrite them with "fake password"?
If so, looks like "regression", no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, the whole point of st2 login is to change this configuration, so the user should expect it to get overwritten with whatever they put into the args or getpass
|
If We create that dir via For me, Additionally, we could replace this bashery: https://github.com/StackStorm/st2-packages/blob/master/scripts/st2bootstrap-deb.sh#L219-L259 with new functionality provided in this PR. |
|
@armab See #3123 (comment) - the method that I'm calling to generate a new token does all kinds of useful checks to make sure the directory is created if it doesn't exist. I am making sure that call is done first, so I know what files/directories will be on the system at that point. |
|
I'm going to need more clarification from the team here. I looked at the original request. Wouldn't writing/caching the token be sufficient to meet the requirement? How come we are also rewriting the client config and saving the client credential in the config file. That seems insecure and not obvious to the user that their credential will be written to disk. |
|
@m4dcoder the reason I'm modifying the config is all about instructing other st2 commands which token file to use, via the username. Tokens are cached by username (i.e. So, Note also by default it writes a dummy/fake password to the config file. Other st2 commands apparently require that this field is present in the config (I tested without), but the field is not used if the token file is found, so the password doesn't need to be correct. The user request was all about being able to quick switch between users, and this accomplishes this in the most secure way I know how, and doesn't rely on being able to modify environment variables in the calling shell (which you can't do in Python anyways). The config file in this case just acts as a pointer to which token file to use at a given time. Hope that helps clear things up |
|
Can you figure out why the password is needed (even a fake password) in the config file? I prefer we fix this and don't write a fake password. It's just confusing code wise and users who is looking at the config will question that anyway. |
This allows the user to specify a different location for the config file using the already-existing "--config" argument Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
|
I agree - will hunt that down next. |
|
@m4dcoder It seems I was mistaken about the password field. I must have lost track of my steps during initial testing; I can perform queries without the password field in the config: In the next few minutes I'll push another commit that removes the password functionality from this patch. |
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
st2client/st2client/commands/auth.py
Outdated
| 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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use os.path.expanduser instead and import os above? Also, can you use the constants at https://github.com/StackStorm/st2/blob/master/st2client/st2client/config_parser.py#L36 instead of duplicating and hard coding the path value here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in: 9ae4d07
st2client/tests/unit/test_auth.py
Outdated
| "args": ['--config', '/tmp/st2config', 'login', 'st2admin', '-w'], | ||
| "expected_username": 'st2admin' | ||
| } | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you separate into different test methods especially that you've additional things to test for each test case below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can split it up; the reason I did it this way was to save a little bit of line count considering that the tests cases have a lot in common. Would result in a lot of redundant test code if I split it up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, i rather you duplicate here to keep the tests clear. And you can always refactor into private methods for obvious reduction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I split up these tests and added a function _login_common_assertions to save a little bit of linecount on common things between the unit tests. Committed in d45b9af
| config['credentials'] = {} | ||
| config['credentials']['username'] = args.username | ||
| if args.write_password: | ||
| config['credentials']['password'] = args.password |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
st2client/st2client/commands/auth.py
Outdated
| 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we removed "fake" password logic, "--write_real_password" naming looks a bit strange now.
Maybe it could be better to rename this flag to --save-password or --write-password or any other analog?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, don't forget to update Pull Request description (first message) with new syntax/logic, because we don't save fake password: notarealpassword anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the description, and changed the argument in 7a2ce39
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
st2 login and st2 whoami commandsst2 login and st2 whoami commands
| config.add_section('credentials') | ||
| config['credentials'] = {} | ||
| config['credentials']['username'] = args.username | ||
| if args.write_password: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scenario: configure .st2/config with user1 with password, rewriting it with st2 login user2, user get rewriten, password remains from user1, token expires, user1 no longer works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - so perhaps I can default to deleting the "password" field (or if --write-password is provided, overwrite)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dzimine I took a stab at this in da93e1b - let me know what you think. Seems to work well now for this case
…inger Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
CHANGELOG.rst
Outdated
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor thing - s/#/*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh! Fixed in 3da2b42
| raise | ||
|
|
||
|
|
||
| class WhoamiCommand(resource.ResourceCommand): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm - does this follow the same logic as other commands - aka it takes ST2_CONFIG_PATH environment variable, --config-file (or whatever the argument name is), --user, etc. and other things into account when determining the username?
If not, it would be a good idea to do that otherwise it might be confusing to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, that are the options I had in mind - https://docs.stackstorm.com/reference/cli.html#configuration-file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kami Okay, I tested both the ST2_CONFIG_PATHand--config-file` options, and other than running into this bug (unrelated to this patch) everything seems to work using a totally different config path.
It should be noted that this tool doesn't actually create the configuration file - as with other commands, if the file doesn't exist at the referenced path, it will raise an exception. I did this to be as predictable compared to other st2 commands as possible.
I will push a short note to StackStorm/st2docs#380 to be clear on this
|
Overall, nice work and LGTM 👍 Only "blocker" for me right now which needs some clarification is https://github.com/StackStorm/st2/pull/3123/files#r99538549 Besides that, LGTM. |
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Kami
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
(re-ran the build, looks like a temporary upstream issue)
Overview
This PR introduces a new
st2 logincommand to the StackStorm client. This command is similar tost2 auth, as it does generate a user token, but it also modifies~/.st2/configand adds a token file to the~/.st2/configdirectory. Furtherst2commands will use the new username/token combination that was written to disk.This is in response to #3110, where a user asked if there was a more friendly way of specifying the current user (i.e. without explicitly modifying configuration files or setting environment variables). This command is simply an alternative to those options.
Initially, I considered adding a flag to
st2 auththat caches the token in~/.st2/token-<username>but realized that~/st2/configwould 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 ofst2 auth, so I decided a separate command was ideal, and more self-explanatory.By default, this command will write only the username to the configuration, for security purposes (not needed if a token file is present). The user can use the
-woption to override this default and write the password into the config file as well.Example
Here's the new command in action. Note that the configuration file is updated and only the relevant section ("credentials") is written to - the rest is preserved.
On the other hand, a failed login attempt will do nothing:
StackStorm documentation has been updated in StackStorm/st2docs#380
UPDATE: Based on some of the review comments, I've added a few tasks for myself:
st2 whoamicommand (thanks for idea @Kami)st2 loginandst2 whoamicommands #3123 (comment)~/.st2/not existing, etc.loginandauthcommands are sufficiently distinct. Also at least open a st2docs PR and link hereCloses #3110