classification
Title: distutils crashes when uploading to PyPI having only the username (no pw) defined
Type: behavior Stage:
Components: Distutils Versions: Python 3.5, Python 3.4, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: eric.araujo Nosy List: dries_desmet, dstufft, eric.araujo, geoffreyspear, giampaolo.rodola, jason.coombs, jgehrcke, loewis, ncoghlan, tarek, techtonik, 池昌言
Priority: normal Keywords: patch

Created on 2013-07-14 17:24 by jgehrcke, last changed 2018-07-11 07:43 by serhiy.storchaka.

Files
File name Uploaded Description Edit
issue18454_py27_prompt.patch jgehrcke, 2015-02-03 23:09
issue18454_py27_prompt_test.patch jgehrcke, 2015-02-03 23:10
issue18454_py35_prompt.patch 池昌言, 2016-05-11 15:37
Messages (7)
msg193062 - (view) Author: Jan-Philip Gehrcke (jgehrcke) * Date: 2013-07-14 17:24
When updating an existing project on PyPI via distutils using the upload command, I observe erroneous behavior regarding the credentials when I do not want to store my password in clear text in the pypirc file:

(1) When running

    python setup.py sdist upload
    
without having the pypirc file in place, I get the error

    Upload failed (401): You must be identified to edit package information
    
(2) When running the same command as above with the pypirc file in place but without having the 'password' option in the 'pypi' section defined, I get a TypeError exception.

In both cases and at least in the second case I expect to be prompted for my credentials. This is what the 2.7.5 docs are saying about the contents of the pypirc file  (http://docs.python.org/2.7/distutils/packageindex.html#the-pypirc-file):

    "password, that will be used to authenticate. If omitted the user will be prompt to type it when needed."
    
I have seen http://bugs.python.org/issue5187 saying "distutils is feature frozen" but the current situation is buggy. Either there is a documentation mistake (it clearly says that the user is prompted for the password) or there is an error in the code (*, see below), or both.

* Regarding the TypeError mentioned above:

In distutils/command/upload.py, finalize_options(), the configuration dictionary is retrieved from _read_pypirc() (distutils/config.py). There, the value for the password key in the config dictionary is set to None if not defined in the pypirc configuration file. The password value is not modified/updated in finalize_options() if self.distribution.password is not set. I think the latter is only set when the 'register' command is used. Hence, when the user for example simply runs

    python setup.py sdist upload
    
and did not set the password in the pypirc file, the password value stays None.

Nevertheless, in distutils/command/upload.py, upload_file(), password is treated as string:

    auth = "Basic " + standard_b64encode(self.username + ":" + self.password)

This obviously leads to

    TypeError: cannot concatenate 'str' and 'NoneType' objects

I would be happy to work on a patch if we agree on what the proper behavior should be. Me, as a user of PyPI, would vote for being prompted in both cases outlined above. I do not like to store my PyPI password in clear text in the file system.

And after specifying how distutils should behave in case (2) I think we all agree that distutils/tests/test_upload.py should provide a test for this case. An example configuration file with username but without password is already defined via PYPIRC_NOPASSWORD. Currently, this config is only tested within an edge-case in test_saved_password() with dist.password = 'xxx', simulating the simultaneous usage of 'register' with 'upload' if I understood correctly. Register probably is used less frequently than upload alone.

Looking forward to your input,

Jan-Philip
msg202384 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2013-11-07 21:08
Same problem here. I'm currently uploading .exe files for psutil by hand.
Interestingly the problem occurs with certain versions of python only (2.4, 2.5, 2.7, 3.2).
msg221688 - (view) Author: Dries Desmet (dries_desmet) Date: 2014-06-27 14:31
I confirm, using python 2.7 on Mac.
msg235143 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-02-01 05:50
I agree the documentation is correct here, and the stdlib should be updated to implement the expected behaviour.

While distutils is indeed feature frozen, that's aimed at changing how we approach the larger scale evolution of the packaging ecosystem, it doesn't mean we won't fix bugs and usability issues. (While we may be slow sometimes, that's a general core developer availability issue, rather than a distutils specific problem)
msg235364 - (view) Author: Jan-Philip Gehrcke (jgehrcke) * Date: 2015-02-03 23:09
First, I want to address the situation in 2.7. Please have a look at my patch and my reasoning.

This is my setup.py test file content:

    from distutils.core import setup
    setup(name='foo', version='1.0', packages=['testpackage'])


This is the current situation for 2.7 head, in summary:

    1) Without a proper .pypirc file in place, uploading fails with
       "Upload failed (401): You must be identified to edit
        package information"

    2) With a valid .pypirc in place which does not define a password
       this exception is raised:
       TypeError: cannot concatenate 'str' and 'NoneType' objects

(1) happens due to the fact that the default username/password strings are empty, but the request is still fired against PyPI. This obviously fails. This could be fixed by prompting for both, username and password or by aborting before sending the request. However, this would be quite a significant change in behavior. I think we should not do this, because AFAIK the 2.7 documentation does not specify what the behavior should be *without* existing .pypirc file. So, I think there might not be enough reason to consider such a change a bug fix. What do you think?

(2) violates the docs and I guess this is what Nick meant when he said that the stdlib should be updated. I propose a patch so that what the docs say ("If omitted, the user will be prompt to type it when needed") is what actually happens. The patch consists of two parts:

Part A: issue18454_py27_prompt.patch
------------------------------------

This uses getpass/sys.stdin.readline() for retrieving the password, depending on whether the process is attached to a tty or not, respectively. The config parsing and value propagation is a little messy in the command/upload.py module, so I have also added comments for clarity and simplified the logic a little.


Part B: issue18454_py27_prompt_test.patch
-----------------------------------------

Here it gets messy. Normally, we want to have a new test case that fails before applying patch A, and succeeds after applying it. However, here we actually have to deal with an already existing bogus test case which succeeds before and fails after A gets applied. A very bad sign, usually. But in this case I am of the strong opinion that we can and have to change the test implementation, because:

    - the test actually makes sure that `cmd.password` is set to None when
      `cmd.finalize_options()` has done its work. However,`cmd.password` being
      None leads to the TypeError exception described above. That is, this test
      case basically ensures the very existance of the bug that we want to fix.

    - it has a suspiciously useless name. It is called `test_saved_password`,
      but actually tests the case when no password is saved in .pypirc.
      
    - this test actually has quite low code coverage. AFAIK it *only* tests
      the behavior of `cmd.finalize_options()`. We want to change this behavior,
      so we have to consider changing the test.
      
My patch modifies this test case so that it has a proper name and makes sure that `cmd.finalize_options()` obtains a password from stdin. The involvement of stdin makes testing a little complicated. In a proper testing environment, we'd probably spawn a subprocess and provide it with some real stdin data. The distutils upload command test structure seems not to be prepared for this endeavor, so I have decided to temporarily/locally patch sys.stdin, and ensure restoration with a try/finally clause.



With A and B in place, all distutils unit tests validate. I used two methods of invocation, for covering the two cases (with and without attached tty):

    $ nohup python test_distutils.py 2>&1 > out.log &
    $ python test_distutils.py
msg260473 - (view) Author: Jason R. Coombs (jason.coombs) * (Python committer) Date: 2016-02-18 18:33
I've recently taken a different tack to this issue with setuptools 20.1, which will resolve the password from a keyring if available.
msg265320 - (view) Author: 池昌言 (池昌言) Date: 2016-05-11 15:37
Patch for Python 3.5, using getpass module
History
Date User Action Args
2018-07-11 07:43:48serhiy.storchakasettype: crash -> behavior
2016-05-21 14:52:40berker.peksaglinkissue5187 superseder
2016-05-11 15:37:00池昌言setfiles: + issue18454_py35_prompt.patch
nosy: + 池昌言
messages: + msg265320

2016-02-18 18:33:13jason.coombssetmessages: + msg260473
2015-02-03 23:10:17jgehrckesetfiles: + issue18454_py27_prompt_test.patch
2015-02-03 23:09:54jgehrckesetfiles: + issue18454_py27_prompt.patch
keywords: + patch
messages: + msg235364
2015-02-01 13:45:12geoffreyspearsetnosy: + geoffreyspear
2015-02-01 05:50:10ncoghlansetnosy: + ncoghlan
messages: + msg235143
2014-08-30 04:25:40terry.reedysetversions: + Python 3.5, - Python 3.1, Python 3.2, Python 3.3
2014-06-27 14:31:57dries_desmetsetnosy: + dries_desmet
messages: + msg221688
2013-11-07 21:08:18giampaolo.rodolasetnosy: + giampaolo.rodola
messages: + msg202384
2013-09-08 14:20:18andrea.corbellinisetversions: + Python 3.1, Python 3.2, Python 3.3, Python 3.4
2013-08-24 22:39:44dstufftsetnosy: + dstufft
2013-07-14 17:24:48jgehrckecreate