Skip to content

Commit

Permalink
feat(auth): allow passwords with empty usernames
Browse files Browse the repository at this point in the history
This change largely reverts the decision made in b544ed5 as it seems
that it is commonplace, albeit not best practice, for private indices
to require or document that users use an empty username. The change in
behaviour seems to surprise users migrating from 1.x (#10085).
  • Loading branch information
abn committed Jan 21, 2025
1 parent 1518109 commit 6a49590
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 20 deletions.
5 changes: 4 additions & 1 deletion docs/repositories.md
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,10 @@ required to set an empty password. This is supported by Poetry.
poetry config http-basic.foo <TOKEN> ""
```

**Note:** Usernames cannot be empty. Attempting to use an empty username can result in an unpredictable failure.
**Note:** Empty usernames are discouraged. However, Poetry will honour them if a password is configured without it. This
is unfortunately commonplace practice, while not best practice, for private indices that use tokens. When a password is
stored into the system keyring with an empty username, Poetry will use a literal `__poetry_source_empty_username__` as
the username to circumvent [keyring#687](https://github.com/jaraco/keyring/pull/687).
{{% /note %}}

## Certificates
Expand Down
20 changes: 13 additions & 7 deletions src/poetry/utils/password_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ class HTTPAuthCredential:


class PoetryKeyring:
# some private sources expect tokens to be provided as password with that can be empty
# we use a fixed literal to ensure that this can be stored in keyring (jaraco/keyring#687)
#
# Note: If this is changed, users with passwords stored with empty usernames will have to
# re-add the config.
_EMPTY_USERNAME_KEY = "__poetry_source_empty_username__"

def __init__(self, namespace: str) -> None:
self._namespace = namespace

Expand All @@ -41,6 +48,7 @@ def get_credential(
for name in names:
credential = None
try:
# we do default to empty username string here since credentials support empty usernames
credential = keyring.get_credential(name, username)
except KeyringLocked:
logger.debug("Keyring %s is locked", name)
Expand All @@ -61,7 +69,7 @@ def get_password(self, name: str, username: str) -> str | None:
name = self.get_entry_name(name)

try:
return keyring.get_password(name, username)
return keyring.get_password(name, username or self._EMPTY_USERNAME_KEY)
except (RuntimeError, keyring.errors.KeyringError):
raise PoetryKeyringError(
f"Unable to retrieve the password for {name} from the key ring"
Expand All @@ -74,7 +82,7 @@ def set_password(self, name: str, username: str, password: str) -> None:
name = self.get_entry_name(name)

try:
keyring.set_password(name, username, password)
keyring.set_password(name, username or self._EMPTY_USERNAME_KEY, password)
except (RuntimeError, keyring.errors.KeyringError) as e:
raise PoetryKeyringError(
f"Unable to store the password for {name} in the key ring: {e}"
Expand All @@ -86,7 +94,7 @@ def delete_password(self, name: str, username: str) -> None:
name = self.get_entry_name(name)

try:
keyring.delete_password(name, username)
keyring.delete_password(name, username or self._EMPTY_USERNAME_KEY)
except (RuntimeError, keyring.errors.KeyringError):
raise PoetryKeyringError(
f"Unable to delete the password for {name} from the key ring"
Expand Down Expand Up @@ -196,13 +204,11 @@ def get_http_auth(self, repo_name: str) -> HTTPAuthCredential:
username = self._config.get(f"http-basic.{repo_name}.username")
password = self._config.get(f"http-basic.{repo_name}.password")

if not username:
return HTTPAuthCredential()

if password is None and self.use_keyring:
password = self.keyring.get_password(repo_name, username)

return HTTPAuthCredential(username=username, password=password)
# we use `or None` here to ensure that empty strings are passed as None
return HTTPAuthCredential(username=username or None, password=password or None)

def set_http_password(self, repo_name: str, username: str, password: str) -> None:
auth = {"username": username}
Expand Down
6 changes: 6 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
from poetry.utils.env import EnvManager
from poetry.utils.env import SystemEnv
from poetry.utils.env import VirtualEnv
from poetry.utils.password_manager import PoetryKeyring
from tests.helpers import MOCK_DEFAULT_GIT_REVISION
from tests.helpers import TestLocker
from tests.helpers import TestRepository
Expand Down Expand Up @@ -191,6 +192,11 @@ def get_credential(
raise KeyringError()


@pytest.fixture()
def poetry_keyring() -> PoetryKeyring:
return PoetryKeyring("poetry-repository")


@pytest.fixture()
def dummy_keyring() -> DummyBackend:
return DummyBackend()
Expand Down
6 changes: 4 additions & 2 deletions tests/utils/test_authenticator.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
from pytest import MonkeyPatch
from pytest_mock import MockerFixture

from poetry.utils.password_manager import PoetryKeyring
from tests.conftest import Config
from tests.conftest import DummyBackend

Expand Down Expand Up @@ -153,7 +154,7 @@ def test_authenticator_uses_empty_strings_as_default_password(
assert request.headers["Authorization"] == f"Basic {basic_auth}"


def test_authenticator_ignores_empty_strings_as_default_username(
def test_authenticator_does_not_ignore_empty_strings_as_default_username(
config: Config,
mock_remote: None,
repo: dict[str, dict[str, str]],
Expand All @@ -170,7 +171,7 @@ def test_authenticator_ignores_empty_strings_as_default_username(
authenticator.request("get", "https://foo.bar/files/foo-0.1.0.tar.gz")

request = http.last_request()
assert request.headers["Authorization"] is None
assert request.headers["Authorization"] == "Basic OmJhcg=="


def test_authenticator_falls_back_to_keyring_url(
Expand Down Expand Up @@ -207,6 +208,7 @@ def test_authenticator_falls_back_to_keyring_netloc(
http: type[httpretty.httpretty],
with_simple_keyring: None,
dummy_keyring: DummyBackend,
poetry_keyring: PoetryKeyring,
) -> None:
config.merge(
{
Expand Down
19 changes: 9 additions & 10 deletions tests/utils/test_password_manager.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
from __future__ import annotations

import contextlib
import logging
import os

Expand Down Expand Up @@ -42,7 +41,7 @@ def test_set_http_password(
("username", "password", "is_valid"),
[
("bar", "baz", True),
("", "baz", False),
("", "baz", True),
("bar", "", True),
("", "", False),
],
Expand All @@ -53,10 +52,10 @@ def test_get_http_auth(
is_valid: bool,
config: Config,
with_simple_keyring: None,
dummy_keyring: DummyBackend,
poetry_keyring: PoetryKeyring,
) -> None:
with contextlib.nullcontext() if username else pytest.warns(DeprecationWarning):
dummy_keyring.set_password("poetry-repository-foo", username, password)
poetry_keyring.set_password("foo", username, password)

config.auth_config_source.add_property("http-basic.foo", {"username": username})
manager = PasswordManager(config)

Expand All @@ -65,8 +64,8 @@ def test_get_http_auth(

if is_valid:
assert auth is not None
assert auth.username == username
assert auth.password == password
assert auth.username == (username or None)
assert auth.password == (password or None)
else:
assert auth.username is auth.password is None

Expand Down Expand Up @@ -137,7 +136,7 @@ def test_set_http_password_with_unavailable_backend(
("username", "password", "is_valid"),
[
("bar", "baz", True),
("", "baz", False),
("", "baz", True),
("bar", "", True),
("", "", False),
],
Expand All @@ -159,8 +158,8 @@ def test_get_http_auth_with_unavailable_backend(

if is_valid:
assert auth is not None
assert auth.username == username
assert auth.password == password
assert auth.username == (username or None)
assert auth.password == (password or None)
else:
assert auth.username is auth.password is None

Expand Down

0 comments on commit 6a49590

Please sign in to comment.