GeezerVison is showing github.com — bpo-28334: fix netrc not working when $HOME is not set by dmerejkowsky · Pull Request #123 · python/cpython
bpo-28334: fix netrc not working when $HOME is not set
#123
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
Uh oh!
There was an error while loading. Please reload this page.
Show file tree
Hide file tree
Changes from all commits
Commits
File filter
Filter by extension
.py
(2)
.rst
(2)
No extension
(1)
All 3 file types selected
Viewed files
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Hide whitespace
Diff view
Hide whitespace
There are no files selected for viewing
Original file line number
Diff line number
Diff line change
Expand Up
@@ -20,8 +20,9 @@ the Unix :program:`ftp` program and other FTP clients.
A :class:`~netrc.netrc` instance or subclass instance encapsulates data from a netrc
file. The initialization argument, if present, specifies the file to parse. If
no argument is given, the file :file:`.netrc` in the user's home directory will
be read. Parse errors will raise :exc:`NetrcParseError` with diagnostic
no argument is given, the file :file:`.netrc` in the user's home directory --
as determined by :func:`os.path.expanduser` -- will be read.
Parse errors will raise :exc:`NetrcParseError` with diagnostic
information including the file name, line number, and terminating token.
If no argument is specified on a POSIX system, the presence of passwords in
the :file:`.netrc` file will raise a :exc:`NetrcParseError` if the file
Expand All
@@ -32,6 +33,10 @@ the Unix :program:`ftp` program and other FTP clients.
.. versionchanged:: 3.4 Added the POSIX permission check.
.. versionchanged:: 3.7
Now uses :func:`os.path.expanduser` to find the location of the
:file:`.netrc` file when *file* is not passed as argument.
.. exception:: NetrcParseError
Expand Down
Expand Up
@@ -82,4 +87,3 @@ Instances of :class:`~netrc.netrc` have public instance variables:
punctuation is allowed in passwords, however, note that whitespace and
non-printable characters are not allowed in passwords. This is a limitation
of the way the .netrc file is parsed and may be removed in the future.
Original file line number
Diff line number
Diff line change
Expand Up
@@ -23,10 +23,7 @@ class netrc:
def __init__(self, file=None):
default_netrc = file is None
if file is None:
try:
file = os.path.join(os.environ['HOME'], ".netrc")
except KeyError:
raise OSError("Could not find .netrc: $HOME is not set") from None
file = os.path.join(os.path.expanduser("~"), ".netrc")
self.hosts = {}
self.macros = {}
with open(file) as fp:
Expand Down
Original file line number
Diff line number
Diff line change
@@ -1,4 +1,5 @@
import netrc, os, unittest, sys, tempfile, textwrap
from unittest import mock
from test import support
Expand Down
Expand Up
@@ -126,8 +127,44 @@ def test_security(self):
os.chmod(fn, 0o622)
self.assertRaises(netrc.NetrcParseError, netrc.netrc)
def test_main():
support.run_unittest(NetrcTestCase)
def test_file_not_found_in_home(self):
d = support.TESTFN
os.mkdir(d)
self.addCleanup(support.rmtree, d)
with support.EnvironmentVarGuard() as environ:
environ.set('HOME', d)
Comment thread
Outdated
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 doesn't test a new behavior. Needed a test for the case when HOME is not set.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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 doesn't test a new behavior
I think it does. It checks the type of the exception raised, which is the whole point of this patch,
Needed a test for the case when HOME is not set.
Yeah I was not sure about this one. I've added a test where I monkeypatch os.path.expanduser, but I'm not sure it's useful. My guts tell me such a test is too close to the implementation.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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 .
Serhiy is right that we need a test for this case and unless I'm missing something you can do it without doing any mocking:
with support .EnvironmentVarGuard () as environ :
environ .unset ('HOME' )
self .assertRaises (FileNotFoundError , netrc .netrc )
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
self.assertRaises(FileNotFoundError, netrc.netrc)
Comment thread
Outdated
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 .
When you explicitly pass a not exist file, it could also raise the error.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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 .
OK. I wrote an other test for this. (I like to follow the 'one assert per test' rule)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
def test_file_not_found_explicit(self):
self.assertRaises(FileNotFoundError, netrc.netrc,
file='unlikely_netrc')
def test_home_not_set(self):
Comment thread
Outdated
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 .
Sorry, I don't understand how this test is related to the case when HOME is not set. Why not use the simple test suggested by @berkerpeksag above?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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 suggested test won't work, because of the following code in expanduser:
if 'HOME' not in os .environ :
import pwd
userhome = pwd .getpwuid (os .getuid ()).pw_dir I have a .netrc file in my HOME directory so the test won't pass:
def test_foo (self ):
with support .EnvironmentVarGuard () as environ :
environ .unset ('HOME' )
self .assertRaises (FileNotFoundError , netrc .netrc )======================================================================
FAIL: test_foo (test.test_netrc.NetrcTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/berker/projects/cpython/master/Lib/test/test_netrc.py", line 34, in test_foo
self.assertRaises(FileNotFoundError, netrc.netrc)
AssertionError: FileNotFoundError not raised by netrc
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
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 .
Thanks, now I see that this is the simplest platform-independent test.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
fake_home = support.TESTFN
os.mkdir(fake_home)
self.addCleanup(support.rmtree, fake_home)
fake_netrc_path = os.path.join(fake_home, '.netrc')
with open(fake_netrc_path, 'w') as f:
f.write('machine foo.domain.com login bar password pass')
os.chmod(fake_netrc_path, 0o600)
orig_expanduser = os.path.expanduser
called = []
def fake_expanduser(s):
called.append(s)
with support.EnvironmentVarGuard() as environ:
environ.set('HOME', fake_home)
result = orig_expanduser(s)
return result
with support.swap_attr(os.path, 'expanduser', fake_expanduser):
nrc = netrc.netrc()
login, account, password = nrc.authenticators('foo.domain.com')
self.assertEqual(login, 'bar')
self.assertTrue(called)
if __name__ == "__main__":
test_main ()
unittest.main ()
1 change: 1 addition & 0 deletions
1
Misc/ACKS
Original file line number
Diff line number
Diff line change
Expand Up
@@ -1031,6 +1031,7 @@ Bill van Melle
Lucas Prado Melo
Ezio Melotti
Doug Mennella
Dimitri Merejkowsky
Brian Merrell
Alexis Métaireau
Luke Mewburn
Expand Down
Original file line number
Diff line number
Diff line change
@@ -0,0 +1,3 @@
Use :func:`os.path.expanduser` to find the ``~/.netrc`` file in
:class:`netrc.netrc`. If the file does not exist, a
:exc:`FileNotFoundError` is raised. Patch by Dimitri Merejkowsky.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.