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

Credential Logging #685

Merged
merged 4 commits into from Nov 23, 2020
Merged

Conversation

VikramJayanthi17
Copy link
Contributor

@VikramJayanthi17 VikramJayanthi17 commented Aug 18, 2020

This PR implements a basic version of credential logging as mentioned in the #381 checklist, . This is a draft PR as I was unsure of how we want to implement the specifics. This is how this functionality is described(by @bhrutledge) in the checklist:

How the credentials (username, password, certificates) are set (.pypirc, keyring, CLI) and what they are (w/ placeholders for sensitive values)

The questions I have are:

  • I assumed that when a user inputs their username/password through the prompt, we do not need to log that it happened via the prompt. Do we want to log this?
  • What does a "placeholder" look like here? What values are sensitive other than passwords?

Any feedback on this PR or answers to these questions would be greatly appreciated.

TODO:

  • Add tests

@bhrutledge bhrutledge self-requested a review August 18, 2020 01:29
twine/repository.py Outdated Show resolved Hide resolved
Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

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

Good stuff! Nice job wading through the complexity of how the username and credentials are set. My requests are just to normalize the log messages.

For example:

$ twine upload --verbose -r testpypi --disable-progress-bar dist/*
Using configuration from /Users/brian/.pypirc
Uploading distributions to https://test.pypi.org/legacy/
  dist/example_pkg_bhrutledge-0.0.4a6-py3-none-any.whl (2.6 KB)
  dist/example-pkg-bhrutledge-0.0.4a6.tar.gz (1.3 KB)
username set from config file
password set from keyring
username: __token__
password: <hidden>
Uploading example_pkg_bhrutledge-0.0.4a6-py3-none-any.whl
Uploading example-pkg-bhrutledge-0.0.4a6.tar.gz

$ twine upload --verbose -r testpypi -u bhrutledge --disable-progress-bar dist/*
Using configuration from /Users/brian/.pypirc
Uploading distributions to https://test.pypi.org/legacy/
  dist/example_pkg_bhrutledge-0.0.4a6-py3-none-any.whl (2.6 KB)
  dist/example-pkg-bhrutledge-0.0.4a6.tar.gz (1.3 KB)
username set by command options
Enter your password: 
username: bhrutledge
password: <empty>
Uploading example_pkg_bhrutledge-0.0.4a6-py3-none-any.whl
...
HTTPError: 403 Forbidden from https://test.pypi.org/legacy/
Invalid or non-existent authentication information. See https://test.pypi.org/help/#invalid-auth for more information.

twine/auth.py Outdated Show resolved Hide resolved
twine/repository.py Outdated Show resolved Hide resolved
twine/utils.py Outdated Show resolved Hide resolved
twine/utils.py Outdated Show resolved Hide resolved
@bhrutledge
Copy link
Contributor

@VikramJayanthi17 FYI, I've merged a handful of changes for unrelated failing tests on master; it'd be worth merging those into your branch.

@bhrutledge
Copy link
Contributor

@VikramJayanthi17 FYI, I've merged some formatting changes that will likely cause a merge conflict on this PR.

@bhrutledge
Copy link
Contributor

@di I haven't heard from @VikramJayanthi17 in awhile. Is it okay if I commit my suggestions and finish up this PR?

@di
Copy link
Sponsor Member

di commented Sep 17, 2020

@bhrutledge Yes, his internship has ended and he's back at school now. I think it's probably fine for you to take over here.

@bhrutledge bhrutledge added this to the 3.3.0 milestone Oct 4, 2020
@bhrutledge bhrutledge marked this pull request as ready for review November 23, 2020 20:42
Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

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

Finally got around to adding some tests, which uncovered a small UX tweak. I'm very happy to merge this. @VikramJayanthi17 thanks again for all of your work!

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.

None yet

4 participants