Skip to content
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

Enable getting password from stdin #86

Merged
merged 8 commits into from
Aug 7, 2018

Conversation

endorama
Copy link
Contributor

@endorama endorama commented Jul 25, 2018

Hello, this PR closes issue #82, providing implementation for getting password from stdin.

I extracted the getpass.getpass call in the Util module, fixed the tests and added two unit test for the new function: test_get_password_when_tty and test_get_password_when_not_tty.
Please have a look at the test because is my first time doing unit testing in python with these libraries.

Thank you!

@endorama endorama force-pushed the get-pass-from-stdin branch 2 times, most recently from 15b9e23 to dc52f50 Compare July 25, 2018 18:27
@endorama
Copy link
Contributor Author

endorama commented Jul 25, 2018

After Travis, there are some errors:

  • python 2.7.14 fail during flake8 checks for this reason:
    ./aws_google_auth/util.py:92:30: E999 SyntaxError: invalid syntax
    I suspect the print(...) with parenthesis is responsible, could you please help me in writing this compatible with python 2? Is possible that the print function should be replaced entirely. I'm sorry I'm not an expert about python version differences.
  • python 3.4 failed at installation step due to timeout
  • python 3.5.5 and 3.6.3 fail during running test for this reason:
    FAIL: test_process_auth_dont_resolve_alias (aws_google_auth.tests.test_init.TestInit)
    This does not happen on my laptop. I run the test with python setup.py test and I also used coverage as in the Travis build, but wasn't able to reproduce.

print("string", end="") is not supported by print in python 2.7.14
@stevemac007
Copy link
Contributor

Would be great if you could update the README with a section on using tty redirection as a way to feed the password into the tool.

I want to release all the recent merge requests, then we can look at the next version including this change.

@coveralls
Copy link

coveralls commented Jul 30, 2018

Coverage Status

Coverage increased (+0.7%) to 48.291% when pulling 38bac15 on endorama:get-pass-from-stdin into 3e3b366 on cevoaustralia:master.

@endorama
Copy link
Contributor Author

@stevemac007 Readme updated as requested, please let me know if the phrasing is ok. Probably would be good to refer also to release number to make it easier to know since when this feature is available.

Copy link
Contributor

@stevemac007 stevemac007 left a comment

Choose a reason for hiding this comment

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

Looks good - and thanks for the README update, the wording looks good to me, only a question about what password-manager is.

I'll merge this in and release soon.

README.rst Outdated

Example usage:
::
$ password-manager show password | aws-google-auth
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not aware of what password-manager is - is it another library?
Is it worth linking to it from here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a particular tool per se, I was meaning "whatever cli tool you are using as password manager".

I would prefer not tying the example to a specific password manager, as any cli tool that can output a password on stdout would be good.

Do you think your-password-manager-cli would be clearer?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think something like echo "my_password" | aws-google-auth ... would also work. People who use this tool are likely to know they can swap out the echo for anything that writes the password to stdout. Just my 2c.

Copy link
Contributor Author

@endorama endorama Aug 6, 2018

Choose a reason for hiding this comment

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

echo "my_password" |

@mide that is what I'd like to avoid. To expert users, I expect reading from stdin is a consolidated concept, as is password leaks to shell history. I'd like to avoid a sample command that's a sort of "shooting yourself in the foot" for newcomers or people not so fond on avoiding common pitfalls when using the shell.

Anyway I'm ok whit that solution as is surely clear and explicit, if someone else approves.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree 100% regarding avoiding "shoot yourself in the foot" examples, but at some point, that isn't our job. I've gone back and forth on this issue quite a bit, and I'm okay either way it lands.

I think there is value in clarity, but there is also value in preventing silly mistakes. I'm fine using a placeholder for a password manager like you've done. I can go either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I stand down on my feedback. Let's leave as is.

README.rst Outdated

When receiving data from ``stdin`` ``aws-google-auth`` disables the interactive prompt and uses ``stdin`` data.

All interactive prompt could be feeded from ``stdin``, but before `#82 <https://github.com/cevoaustralia/aws-google-auth/issues/82>`_
Copy link
Contributor

Choose a reason for hiding this comment

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

feeded --> fed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😅 ops! Fixed!

@mide
Copy link
Contributor

mide commented Aug 7, 2018

I like this PR as-is. I think it's a great contribution and I'm coming around to the idea that "it's not our job" to prevent people from shooting themselves in the foot (using echo "s3creT" | aws-google-auth ...).

I have a few colleagues that have shared their desire to use their password manager CLI to interact with this tool, and that's anything but "shoot yourself in the foot" behavior.

@stevemac007 stevemac007 merged commit c265409 into cevoaustralia:master Aug 7, 2018
@endorama endorama deleted the get-pass-from-stdin branch August 8, 2018 23:47
@endorama
Copy link
Contributor Author

endorama commented Aug 8, 2018

Thanks a lot for merging this! 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants