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

Add NetRCAuth() class. #2535

Merged
merged 7 commits into from
Jan 12, 2023
Merged

Add NetRCAuth() class. #2535

merged 7 commits into from
Jan 12, 2023

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Jan 2, 2023

Closes #2532

Prompted by #2532 (comment)

We currently automatically handle netrc authentication, which is baked directly into the Client, and which is enabled unless the developer sets trust_env=False.

  • Browsers, which do not use netrc authentication.
  • The curl command line client, which only uses netrc authentication if explicitly enabled.

The suggest here is that we should make a behavioural change, and no longer bake netrc authentication directly into the client, in favor of providing an explicit NetRCAuth() class.

Some usage examples...

# Use default netrc file
auth = httpx.NetRCAuth()
client = httpx.Client(auth=auth)

# Use explicit netrc file
auth = httpx.NetRCAuth(file="/path/to/netrc")
client = httpx.Client(auth=auth)

# Optional environ override, fallback to default netrc file otherwise
auth = httpx.NetRCAuth(file=os.environ.get('NETRC'))
client = httpx.Client(auth=auth)

# Mandatory environ override
auth = httpx.NetRCAuth(file=os.environ['NETRC'])
client = httpx.Client(auth=auth)

# Optional netrc auth, depending on if the "~/.netrc" file exists
try:
    auth = httpx.NetRCAuth()
except FileNotFound:
    auth = None
client = httpx.Client(auth=auth)

This would be a behavioral change and need to be part of a 0.24.0 release, but I think it's obviously more consistent and neater than our existing "netrc is a special case" behaviour.

(I had also assumed that this change would resolve issue #2088, and added a test case for this, which showed me that my assumption there was incorrect. Failing test case for that now dropped in commits ebcf77a and 02120cc.)

TODO:

  • Drop baked-in client netrc auth class.
  • Implement NetRCAuth class.
  • Tests.
  • Update documentation.

Documentation link, for easier review... https://github.com/encode/httpx/blob/02120cccecf905a0f142b36a31cebf68974e9528/docs/advanced.md#netrc-support

@tomchristie tomchristie marked this pull request as ready for review January 3, 2023 11:55
@tomchristie tomchristie requested a review from a team January 3, 2023 11:55
@tomchristie tomchristie added refactor Issues and PRs related to code refactoring user-experience Ensuring that users have a good experience using the library api change PRs that contain breaking public API changes and removed refactor Issues and PRs related to code refactoring labels Jan 4, 2023
@tomchristie
Copy link
Member Author

What do we think @encode/maintainers?

httpx/_auth.py Outdated

def auth_flow(self, request: Request) -> typing.Generator[Request, Response, None]:
auth_info = self._netrc_info.authenticators(request.url.host)
if auth_info is None or auth_info[2] is None:
Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry, may I ask you why you are checking the password for None?

Copy link
Member

Choose a reason for hiding this comment

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

I'm asking since previously you were checking

if credentials is not None:
    ...

and python documentation says (link) that

If the netrc file did not contain an entry for the given host, return the tuple associated with the ‘default’ entry. If neither matching host nor default entry is available, return None.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @cdeler.

The simple version of "why" is because I was blindly following our previous implementation details here.

I've had a look into it, and I don't think the password can be None here, but I think it can be the empty string. It looks to me like Python 3.11+ will allow the empty string for a missing password field in the netrc file, and the previous versions will raise a parse error for missing passwords.

I think the robust thing to do here would be...

if auth_info is None or not auth_info[2]:

Copy link
Member Author

Choose a reason for hiding this comment

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

Demo'ing the behaviour of Python netrc with an empty password entry...

import netrc
import tempfile


with tempfile.NamedTemporaryFile(delete=False) as t:
    t.write(b"machine example.com\nlogin user\n")
    t.close()
    n = netrc.netrc(t.name)
    print(n.authenticators("example.com"))

Python 3.11:

$ python3.11 ./example.py 
('user', '', '')

Python 3.10:

$ python3.10 ./example.py 
Traceback (most recent call last):
  File "/Users/tomchristie/Temp/./example.py", line 8, in <module>
    n = netrc.netrc(t.name)
  File "/Library/Frameworks/Python.framework/Versions/3.10/lib/python3.10/netrc.py", line 31, in __init__
    self._parse(file, fp, default_netrc)
  File "/Library/Frameworks/Python.framework/Versions/3.10/lib/python3.10/netrc.py", line 82, in _parse
    raise NetrcParseError(
netrc.NetrcParseError: malformed machine entry example.com terminated by '' (/var/folders/8s/dk9369g11yzdnsfkvbtljcjm0000gn/T/tmpipnocc0x, line 3)

httpx/_auth.py Outdated Show resolved Hide resolved
@tomchristie
Copy link
Member Author

Great review, thanks @cdeler - I've updated the PR to be more robust to the empty password case.

@cdeler
Copy link
Member

cdeler commented Jan 11, 2023

LGTM, but have 1 small nitpick:

Does it make sense to add a test + test fixture for the case when password is empty (it will add a test for the newly added check)?

@tomchristie
Copy link
Member Author

Does it make sense to add a test + test fixture for the case when password is empty (it will add a test for the newly added check)?

Done.

@tomchristie tomchristie merged commit 59914c7 into master Jan 12, 2023
@tomchristie tomchristie deleted the netrc-auth-class branch January 12, 2023 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api change PRs that contain breaking public API changes user-experience Ensuring that users have a good experience using the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NetRCInfo.netrc_info() doesn't work when run in a systemd service with DynamicUser=true
2 participants