Adding support for HTTP Basic Auth when installing and searching from legacy repo (#233) #306

MarcDufresne · 2018-07-14T20:47:04Z

Pull Request Check List

Added tests for changed code.
Updated documentation for changed code.

Hi! I really like poetry and would love to be able to use it at work, sadly it doesn't support HTTP Basic Auth when pulling from private repositories so that has kept us from using it. See #233

That is why I have come up with this PR. It basically adds support for pulling from private repos using the auth values stored in http-basic.<repo-name> by inserting them in the repo URL dynamically. I've also made sure that the username and password are never output to the shell.

While doing this I tried to have the most minimal impact on existing user. For example, anyone that already defines username and password directly in the URL of the pyproject.toml file won't suddenly have issues since I check for existing credentials and skip my code if anything is found (But I suggest those who do this now use the http-basic configuration). Additionally, I make sure that if no credentials are found for this specific repo the URL won't be altered at all.

~~I did not write tests yet because I wanted my design validated first and because I am not sure how to approach tests for this feature.~~
I wrote a unit test that mocks reading http-basic from the config and uses the returned values to create a new URL and then validates that the URL was properly constructed.
I have also tested the functionality locally using a devpi-server behind a HTTP Basic Auth protected web server, with the config below

pyproject.toml:

[[tool.poetry.source]]
url = "http://localhost:3142/root/pypi/+simple/"
name = "devpi"

poetry config http-basic devpi user pass

MarcDufresne · 2018-07-16T16:33:34Z

@sdispater @cauebs This is ready for review anytime you have time! Thanks!

sdispater

Thanks a lot for taking the time to put this PR together!

I added some feedback and requested some changes. Nothing major and it should be pretty straightforward.

Tell me what you think.

sdispater · 2018-07-23T22:47:09Z

poetry/repositories/legacy_repository.py

+        if not url_parts.username:
+            username, password = get_http_basic_auth(self.name)
+            if username and password:
+                netloc = "{}:{}@{}".format(username, password, url_parts.netloc)


Instead of recreating the url by hand we can just pass the auth information to the session created in __init__(). That way the _url attribute is left untouched while still providing the auth information to each request made to the remote url.

self._session.auth = get_http_basic_auth(self.name)

For this to work we have to make a small change to get_http_basic_auth() so that it returns None when no authentication information could be found. This makes the code much simpler since we don't have to chech each time if username/password exist.

sdispater · 2018-07-23T22:47:44Z

poetry/utils/helpers.py

+    repo_auth = config.setting("http-basic.{}".format(repository_name))
+    if repo_auth:
+        return repo_auth["username"], repo_auth["password"]
+    return "", ""


We should return None for the reasons specified above.

sdispater · 2018-07-23T22:49:03Z

pyproject.toml

@@ -55,6 +55,7 @@ pygments-github-lexers = "^0.0.5"
 black = { version = "^18.3-alpha.0", python = "^3.6" }
 pre-commit = "^1.10"
 tox = "^3.0"
+mock = "^2.0"


This is not necessary since we already have pytest-mock which provides a more convenient way to mock objects and functions.

sdispater · 2018-07-23T22:49:43Z

tests/repositories/test_legacy_repository.py

@@ -43,3 +44,11 @@ def test_page_absolute_links_path_are_correct():
    for link in page.links:
        assert link.netloc == "files.pythonhosted.org"
        assert link.path.startswith("/packages/")
+
+
+@mock.patch("poetry.repositories.legacy_repository.get_http_basic_auth")


We shoud use the mocker fixture provided by pytest-mock

sdispater · 2018-07-23T22:51:07Z

tests/repositories/test_legacy_repository.py

+
+
+@mock.patch("poetry.repositories.legacy_repository.get_http_basic_auth")
+def test_http_basic_auth_repo(mock_get_auth):


Testing this will be more complex with the changes I requested since we will need to check the url that will be called in _get().

sdispater · 2018-07-23T22:56:00Z

poetry/installation/pip_installer.py

@@ -25,9 +25,14 @@ def install(self, package, update=False):
        if package.source_type == "legacy" and package.source_url:
            parsed = urlparse.urlparse(package.source_url)
            if parsed.scheme == "http":
+                if not parsed.username:


With the changes requested below. The authentication part of the url will not appear here, so this check is not necessary.

However, this will be an issue since pip won't be aware of the credentials. So this is where we should modify the url to include the credentials, I think.

MarcDufresne · 2018-07-24T00:20:28Z

Thanks for the feedback, it's your project so we'll do it your way :P

For the session, setting the auth was actually my first implementation but wouldn't work with the _download method in PyPiRepository since it doesn't use the session (and I tried using the session in it but downloads would timeout for some reason), this is the cleanest way I've found that worked without changing too much of the code/design. Let me know what you think.

I've actually been using my fork since last week and it's working great at least.

sdispater · 2018-07-24T16:31:09Z

@MarcDufresne It's weird that the downloads timeout by using the session. Could this be caused by the fact that we encapsulate the sessions in a CacheControl object?

sdispater · 2018-07-24T16:36:18Z

I just checked and I can't reproduce the timeouts. Everything works on my end by using the session in the LegacyRepository class.

MarcDufresne · 2018-07-24T16:59:40Z

@sdispater Thanks for trying it on your end, can you share a snippet? IIRC I had to make PyPIRepository._download use self.session.get, that's the part that somehow didn't work for me.

Oh and I should be fixing the comments shortly!

sdispater · 2018-07-24T17:06:16Z

@MarcDufresne I just overrode _download() in LegacyRepository to use self._session.get(), nothing more and it worked.

MarcDufresne · 2018-07-24T17:24:03Z

Ok cool, I'll do it like that then, I didn't want to change too much code at first, but if you're okay with it, I'll do it like that.

MarcDufresne · 2018-07-24T18:45:33Z

@sdispater Can you take a look at pip_installer.py, I'm looking for a way to get the name of the repo (to get the auth) but I'm not sure how.

sdispater · 2018-07-24T22:56:45Z

@MarcDufresne At the moment there is no easy way to retrieve the name of the repository from the lock file.

I think we can store it in the source_reference attribute of a Package since it's currently not used for LegacyRepository objects.

See here: https://github.com/sdispater/poetry/blob/master/poetry/repositories/legacy_repository.py#L239

sdispater · 2018-07-25T18:18:09Z

Thanks a lot for this!

MarcDufresne · 2018-07-25T18:21:17Z

Well thanks for your feedback and accepting this! Glad to see poetry becoming even more useful :D

Gr1N · 2018-07-25T19:25:41Z

@sdispater could you please release new version of poetry with this fix?

… legacy repo (#233) (#306) * Adding support for HTTP Basic Auth when pulling from legacy repo * added docs * Adding unit test for http auth * Adding mock to dev deps * moved config reading logic to helper function, adapted test * corrections * rework pip_installer * use source_reference as repo name when writing lock file and installing

github-actions · 2024-03-01T18:08:54Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

MarcDufresne added 4 commits July 14, 2018 16:26

Adding support for HTTP Basic Auth when pulling from legacy repo

c4d6311

added docs

1c5b9d7

Adding unit test for http auth

6c62b45

Adding mock to dev deps

2d13177

MarcDufresne changed the title ~~Adding support for HTTP Basic Auth when pulling from legacy repo~~ Adding support for HTTP Basic Auth when installing and searching from legacy repo Jul 16, 2018

moved config reading logic to helper function, adapted test

8c69c2b

MarcDufresne changed the title ~~Adding support for HTTP Basic Auth when installing and searching from legacy repo~~ Adding support for HTTP Basic Auth when installing and searching from legacy repo (#233) Jul 16, 2018

sdispater requested changes Jul 23, 2018

View reviewed changes

Marc-André Dufresne added 2 commits July 24, 2018 13:51

corrections

94ff4fe

rework pip_installer

3f82b3c

Gr1N mentioned this pull request Jul 24, 2018

Use authentication credentials in install, search commands (#233) #307

Closed

2 tasks

use source_reference as repo name when writing lock file and installing

d599d4f

sdispater mentioned this pull request Jul 25, 2018

Ignore PyPI if local/private PI available? #331

Closed

2 tasks

sdispater approved these changes Jul 25, 2018

View reviewed changes

sdispater merged commit 6244c49 into python-poetry:master Jul 25, 2018

dimbleby pushed a commit to dimbleby/poetry that referenced this pull request Apr 21, 2022

black: remove deprecated option (python-poetry#306)

ffc22b2

github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for HTTP Basic Auth when installing and searching from legacy repo (#233) #306

Adding support for HTTP Basic Auth when installing and searching from legacy repo (#233) #306

MarcDufresne commented Jul 14, 2018 •

edited

Loading

MarcDufresne commented Jul 16, 2018

sdispater left a comment

sdispater Jul 23, 2018

sdispater Jul 23, 2018

sdispater Jul 23, 2018

sdispater Jul 23, 2018

sdispater Jul 23, 2018

sdispater Jul 23, 2018

MarcDufresne commented Jul 24, 2018

sdispater commented Jul 24, 2018

sdispater commented Jul 24, 2018

MarcDufresne commented Jul 24, 2018

sdispater commented Jul 24, 2018

MarcDufresne commented Jul 24, 2018

MarcDufresne commented Jul 24, 2018

sdispater commented Jul 24, 2018

sdispater commented Jul 25, 2018

MarcDufresne commented Jul 25, 2018

Gr1N commented Jul 25, 2018

github-actions bot commented Mar 1, 2024



		@mock.patch("poetry.repositories.legacy_repository.get_http_basic_auth")
		def test_http_basic_auth_repo(mock_get_auth):

Adding support for HTTP Basic Auth when installing and searching from legacy repo (#233) #306

Adding support for HTTP Basic Auth when installing and searching from legacy repo (#233) #306

Conversation

MarcDufresne commented Jul 14, 2018 • edited Loading

Pull Request Check List

MarcDufresne commented Jul 16, 2018

sdispater left a comment

Choose a reason for hiding this comment

sdispater Jul 23, 2018

Choose a reason for hiding this comment

sdispater Jul 23, 2018

Choose a reason for hiding this comment

sdispater Jul 23, 2018

Choose a reason for hiding this comment

sdispater Jul 23, 2018

Choose a reason for hiding this comment

sdispater Jul 23, 2018

Choose a reason for hiding this comment

sdispater Jul 23, 2018

Choose a reason for hiding this comment

MarcDufresne commented Jul 24, 2018

sdispater commented Jul 24, 2018

sdispater commented Jul 24, 2018

MarcDufresne commented Jul 24, 2018

sdispater commented Jul 24, 2018

MarcDufresne commented Jul 24, 2018

MarcDufresne commented Jul 24, 2018

sdispater commented Jul 24, 2018

sdispater commented Jul 25, 2018

MarcDufresne commented Jul 25, 2018

Gr1N commented Jul 25, 2018

github-actions bot commented Mar 1, 2024

MarcDufresne commented Jul 14, 2018 •

edited

Loading