Support .netrc by trust_env by linw1995 · Pull Request #2584 · aio-libs/aiohttp

linw1995 · 2017-12-03T14:22:56Z

What do these changes do?

Support .netrc by trust_env

Are there changes in behavior for the user?

None

Related issue number

#2581

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bug)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

asvetlov · 2017-12-04T11:16:42Z

aiohttp/helpers.py

+    netrc_obj = None
+    netrc_path = os.environ.get('netrc')
+    if netrc_path is None:
+        home_dir = os.path.expanduser('~')


Could you use pathlib instead of os.path?
E.g. https://docs.python.org/3/library/pathlib.html#pathlib.Path.home for getting user home folder.

asvetlov · 2017-12-04T11:18:37Z

aiohttp/helpers.py


 def proxies_from_env():
+    netrc_obj = None
+    netrc_path = os.environ.get('netrc')


Environment names are case sensitive, according to https://www.gnu.org/software/inetutils/manual/html_node/The-_002enetrc-file.html the name should be upper-cased: NETRC.

asvetlov · 2017-12-04T11:27:27Z

.netrc could be used not only for proxy credentials but for any resource, requests supports it.
What's about reading the file in ClientRequest.update_auth() https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client_reqrep.py#L281 if no explicit auth info is provided?

asvetlov · 2017-12-04T12:04:22Z

tests/test_proxy_functional.py

+    url = 'http://aiohttp.io/path'
+    proxy = await proxy_test_server()
+    auth = aiohttp.BasicAuth('user', 'pass')
+    netrc_file = tempfile.mktemp()


Why you use tempfile module? Is tmpdir fixture functionality not enough?

- Change environment name to standard uppercase - Change to use pathlib to get home dir

- use tmpdir fixture

- Remove the unused import

codecov-io · 2017-12-04T16:59:27Z

Codecov Report

Merging #2584 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2584      +/-   ##
==========================================
+ Coverage   97.87%   97.88%   +<.01%     
==========================================
  Files          38       38              
  Lines        7304     7327      +23     
  Branches     1263     1268       +5     
==========================================
+ Hits         7149     7172      +23     
  Misses         51       51              
  Partials      104      104

Impacted Files	Coverage Δ
aiohttp/helpers.py	`97.93% <100%> (+0.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0fe923...a246e52. Read the comment docs.

linw1995 · 2017-12-05T11:08:10Z

I'm so sorry, take so long to changes code. not familiar to pathlib, I should take more time to think then code.

asvetlov · 2017-12-05T11:18:04Z

No problem

- full cover tests

_ Extract `netrc_from_env` from `proxies_from_env` - make real netrc file invisible in home dif for test

linw1995 · 2017-12-10T04:15:54Z

if I use netrc_from_env into ClientRequest.update_auth() https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client_reqrep.py#L281, the ClientRequest needs to add a param trust_env. How about add it into ClientSession if no explict auth or AUTHORIZATION in headers?

aiohttp/aiohttp/client.py

Lines 252 to 265 in dbb8d9a

    
           if auth is None: 
        
               auth = auth_from_url 
        
           if auth is None: 
        
               auth = self._default_auth 
        
           # It would be confusing if we support explicit 
        
           # Authorization header with auth argument 
        
           if (headers is not None and 
        
                   auth is not None and 
        
                   hdrs.AUTHORIZATION in headers): 
        
               raise ValueError("Cannot combine AUTHORIZATION header " 
        
                                "with AUTH argument or credentials " 
        
                                "encoded in URL")

asvetlov · 2017-12-10T10:28:06Z

Sorry, had no time to look on changes.
Will review tomorrow or on Tuesday.

asvetlov · 2017-12-13T13:10:17Z

aiohttp/helpers.py

+        if netrc_path and netrc_path.is_file():
+            try:
+                netrc_obj = netrc.netrc(str(netrc_path))
+            except (netrc.NetrcParseError, IOError) as e:


Rename IOError to OSError -- they are aliases but OSError is the preferable name.

asvetlov · 2017-12-13T13:13:05Z

aiohttp/helpers.py

+            auth_from_netrc = netrc_obj.authenticators(proxy.host)
+            if auth_from_netrc is not None:
+                *logins, password = auth_from_netrc
+                auth = BasicAuth(logins[0] if logins[0] else logins[1],


logins[-1] should be enough.
Please add a comment when logins[0] can be empty (file is started with tab I guess).

https://github.com/requests/requests/blob/24092b11d74af0a766d9cc616622f38adb0044b9/requests/utils.py#L203-L207
Requests code shows me how to handle it, and i saw the netrc source code.
netrc.authenticators method return a (user, account, password) tuple.
I guess user and account both can be username.

if netrc_obj and auth is None: auth_from_netrc = netrc_obj.authenticators(proxy.host) if auth_from_netrc is not None: # auth_from_netrc is a (`user`, `account`, `password`) tuple, # `user` and `account` both can be username, # if `user` is None, use `account` *logins, password = auth_from_netrc auth = BasicAuth(logins[0] if logins[0] else logins[-1], password)

linw1995 · 2017-12-13T13:56:00Z

Create a .netrc or _netrc file says the netrc file should be set to at least Read (400), or Read/Write (600), unreadable to everyone else.
Is it necessity to verify that?

asvetlov · 2017-12-13T13:57:56Z

No, .netrc permission rules are for administration and security, no reason to raise an exception.
If user's host is not secure -- it is not aiohttp problem.

asvetlov · 2017-12-14T10:33:03Z

Thanks!

lock · 2019-10-28T11:04:55Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

linw1995 and others added 3 commits December 3, 2017 22:21

Support .netrc by trust_env

9e130f6

Merge branch 'master' into netrc-supports

b0ab29d

Update 2581.feature

a8e9049

asvetlov requested changes Dec 4, 2017

View reviewed changes

林玮 added 6 commits December 4, 2017 20:36

Fix for review

ec9159a

- Change environment name to standard uppercase - Change to use pathlib to get home dir

Fix for review

5362cc6

- use tmpdir fixture

Fix for review

9f8bbc5

- Remove the unused import

Fix some stupid mistake...

cdfbd06

Fix second stupid mistake...

083bb0f

Fix second stupid mistake

b9522c0

linw1995 and others added 4 commits December 7, 2017 22:59

Modify the code more readable

7b36103

- full cover tests

Merge branch 'master' into netrc-supports

ddc0f0b

Extract netrc_from_env from proxies_from_env

d52ed24

_ Extract `netrc_from_env` from `proxies_from_env` - make real netrc file invisible in home dif for test

Add a test for proxies_from_env

46ad4e9

Merge branch 'master' into netrc-supports

f95d5d1

asvetlov requested changes Dec 13, 2017

View reviewed changes

asvetlov and others added 3 commits December 13, 2017 20:38

Update helpers.py

619eeae

Merge branch 'master' into netrc-supports

131177a

Add a usefull comment

9e7daa7

asvetlov approved these changes Dec 14, 2017

View reviewed changes

Merge branch 'master' into netrc-supports

a246e52

asvetlov merged commit 76f9274 into aio-libs:master Dec 14, 2017

asvetlov mentioned this pull request Dec 14, 2017

Support .netrc by trust_env #2581

Closed

nicktimko mentioned this pull request Oct 15, 2018

Quiet warnings about netrc #3267

Merged

5 tasks

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019

Uh oh!

Conversation

linw1995 commented Dec 3, 2017

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

Uh oh!

asvetlov Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

asvetlov Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Dec 4, 2017

Uh oh!

asvetlov Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

linw1995 commented Dec 5, 2017

Uh oh!

asvetlov commented Dec 5, 2017

Uh oh!

linw1995 commented Dec 10, 2017

Uh oh!

asvetlov commented Dec 10, 2017

Uh oh!

asvetlov Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

asvetlov Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

linw1995 Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

asvetlov Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

linw1995 commented Dec 13, 2017

Uh oh!

asvetlov commented Dec 13, 2017

Uh oh!

asvetlov commented Dec 14, 2017

Uh oh!

lock bot commented Oct 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-io commented Dec 4, 2017 •

edited

Loading