Marco Ricci commited on 2026-01-24 21:55:29
Zeige 5 geänderte Dateien mit 73 Einfügungen und 50 Löschungen.
We expect the names to be user-facing soon, as part of the configuration, choosing which SSH agent socket provider to use. To that end, the "The Annoying OS" moniker is a hindrance: nobody is likely to understand it at a first glance (particularly non-programmers), and conversely, users looking for configuration options pertaining to Microsoft Windows will likely miss that socket providers containing the `the_annoying_os` label are relevant to their query. Insisting on the moniker thus does our users a huge (and completely avoidable) disservice. So, in the interest of clarity, rename the `the_annoying_os` label parts to `windows`. Since this makes some labels completely redundant, and since the test suite is strongly fixated on the default label names, use this opportunity to clean up the registry and the test suite machinery that needs mock registries.
| ... | ... |
@@ -982,22 +982,18 @@ class SocketProvider: |
| 982 | 982 |
|
| 983 | 983 |
SocketProvider.registry.update({
|
| 984 | 984 |
"ssh_auth_sock_on_posix": SocketProvider.unix_domain_ssh_auth_sock, |
| 985 |
- "pageant_on_the_annoying_os": SocketProvider.windows_named_pipe_for_pageant, # noqa: E501 |
|
| 986 |
- "openssh_on_the_annoying_os": SocketProvider.windows_named_pipe_for_openssh, # noqa: E501 |
|
| 987 |
- "ssh_auth_sock_on_the_annoying_os": SocketProvider.windows_named_pipe_ssh_auth_sock, # noqa: E501 |
|
| 985 |
+ "pageant_on_windows": SocketProvider.windows_named_pipe_for_pageant, # noqa: E501 |
|
| 986 |
+ "openssh_on_windows": SocketProvider.windows_named_pipe_for_openssh, # noqa: E501 |
|
| 987 |
+ "ssh_auth_sock_on_windows": SocketProvider.windows_named_pipe_ssh_auth_sock, # noqa: E501 |
|
| 988 | 988 |
# known instances |
| 989 | 989 |
"stub_agent": None, |
| 990 | 990 |
"stub_agent_with_address": None, |
| 991 | 991 |
"stub_agent_with_address_and_deterministic_dsa": None, |
| 992 | 992 |
# aliases |
| 993 |
- "posix": "ssh_auth_sock_on_posix", |
|
| 994 | 993 |
"ssh_auth_sock": "ssh_auth_sock_on_posix", |
| 995 | 994 |
"unix_domain": "ssh_auth_sock_on_posix", |
| 996 |
- "the_annoying_os": "ssh_auth_sock_on_the_annoying_os", |
|
| 997 |
- "the_annoying_os_named_pipe": "the_annoying_os", |
|
| 998 |
- "windows": "the_annoying_os", |
|
| 999 |
- "windows_named_pipe": "the_annoying_os", |
|
| 1000 |
- "pageant_on_windows": "pageant_on_the_annoying_os", |
|
| 1001 |
- "openssh_on_windows": "openssh_on_the_annoying_os", |
|
| 1002 |
- "native": "the_annoying_os" if os.name == "nt" else "posix", |
|
| 995 |
+ "posix": "unix_domain", |
|
| 996 |
+ "windows_named_pipe": "ssh_auth_sock_on_windows", |
|
| 997 |
+ "windows": "windows_named_pipe", |
|
| 998 |
+ "native": "windows" if os.name == "nt" else "posix", |
|
| 1003 | 999 |
}) |
| ... | ... |
@@ -363,15 +363,14 @@ The standard [`_types.SSHAgentSocketProviderEntry`][] for the UNIX |
| 363 | 363 |
domain socket handler on POSIX systems. |
| 364 | 364 |
""" |
| 365 | 365 |
|
| 366 |
-pageant_on_the_annoying_os_entry = _types.SSHAgentSocketProviderEntry( |
|
| 367 |
- socketprovider.SocketProvider.resolve("pageant_on_the_annoying_os"),
|
|
| 368 |
- "pageant_on_the_annoying_os", |
|
| 366 |
+ssh_auth_sock_on_windows_entry = _types.SSHAgentSocketProviderEntry( |
|
| 367 |
+ socketprovider.SocketProvider.resolve("ssh_auth_sock_on_windows"),
|
|
| 368 |
+ "ssh_auth_sock_on_windows", |
|
| 369 | 369 |
(), |
| 370 | 370 |
) |
| 371 | 371 |
""" |
| 372 | 372 |
The standard [`_types.SSHAgentSocketProviderEntry`][] for the Windows |
| 373 |
-named pipe handler (connecting to Pageant) on The Annoying Operating |
|
| 374 |
-System. |
|
| 373 |
+named pipe handler on The Annoying Operating System. |
|
| 375 | 374 |
""" |
| 376 | 375 |
|
| 377 | 376 |
faulty_entry_callable = _types.SSHAgentSocketProviderEntry( |
| ... | ... |
@@ -385,7 +384,7 @@ is not a callable. |
| 385 | 384 |
""" |
| 386 | 385 |
|
| 387 | 386 |
faulty_entry_name_exists = _types.SSHAgentSocketProviderEntry( |
| 388 |
- socketprovider.SocketProvider.resolve("the_annoying_os"), "posix", ()
|
|
| 387 |
+ socketprovider.SocketProvider.resolve("windows"), "posix", ()
|
|
| 389 | 388 |
) |
| 390 | 389 |
""" |
| 391 | 390 |
A faulty [`_types.SSHAgentSocketProviderEntry`][]: the indicated handler |
| ... | ... |
@@ -395,7 +394,7 @@ is already registered with a different callable. |
| 395 | 394 |
faulty_entry_alias_exists = _types.SSHAgentSocketProviderEntry( |
| 396 | 395 |
socketprovider.SocketProvider.resolve("ssh_auth_sock_on_posix"),
|
| 397 | 396 |
"ssh_auth_sock_on_posix", |
| 398 |
- ("unix_domain", "the_annoying_os"),
|
|
| 397 |
+ ("unix_domain", "windows_named_pipes"),
|
|
| 399 | 398 |
) |
| 400 | 399 |
""" |
| 401 | 400 |
A faulty [`_types.SSHAgentSocketProviderEntry`][]: the alias is already |
| ... | ... |
@@ -350,7 +350,7 @@ class SystemSupportAction(str, enum.Enum): |
| 350 | 350 |
monkeypatch.delattr(socket, "AF_UNIX", raising=False) |
| 351 | 351 |
elif self in {self.UNSET_WINDLL, self.UNSET_WINDLL_AND_ENSURE_USE}:
|
| 352 | 352 |
self.check_or_ensure_use( |
| 353 |
- "the_annoying_os", |
|
| 353 |
+ "windows", |
|
| 354 | 354 |
monkeypatch=monkeypatch, |
| 355 | 355 |
ensure_use=(self == self.UNSET_WINDLL_AND_ENSURE_USE), |
| 356 | 356 |
) |
| ... | ... |
@@ -85,9 +85,9 @@ class Parametrize(types.SimpleNamespace): |
| 85 | 85 |
value="tests.data: ssh_auth_sock_on_posix_entry", |
| 86 | 86 |
), |
| 87 | 87 |
importlib.metadata.EntryPoint( |
| 88 |
- name=data.pageant_on_the_annoying_os_entry.key, |
|
| 88 |
+ name=data.ssh_auth_sock_on_windows_entry.key, |
|
| 89 | 89 |
group=socketprovider.SocketProvider.ENTRY_POINT_GROUP_NAME, |
| 90 |
- value="tests.data: pageant_on_the_annoying_os_entry", |
|
| 90 |
+ value="tests.data: ssh_auth_sock_on_windows_entry", |
|
| 91 | 91 |
), |
| 92 | 92 |
], |
| 93 | 93 |
id="existing-entries", |
| ... | ... |
@@ -110,7 +110,7 @@ class Parametrize(types.SimpleNamespace): |
| 110 | 110 |
], |
| 111 | 111 |
) |
| 112 | 112 |
EXISTING_REGISTRY_ENTRIES = pytest.mark.parametrize( |
| 113 |
- "existing", ["ssh_auth_sock_on_posix", "pageant_on_the_annoying_os"] |
|
| 113 |
+ "existing", ["ssh_auth_sock_on_posix", "ssh_auth_sock_on_windows"] |
|
| 114 | 114 |
) |
| 115 | 115 |
SSH_STRING_EXCEPTIONS = pytest.mark.parametrize( |
| 116 | 116 |
["input", "exc_type", "exc_pattern"], |
| ... | ... |
@@ -789,29 +789,47 @@ class TestSSHAgentSocketProviderRegistry: |
| 789 | 789 |
stack.enter_context(pytest.raises(AssertionError)) |
| 790 | 790 |
socketprovider.SocketProvider._find_all_ssh_agent_socket_providers() |
| 791 | 791 |
|
| 792 |
- def test_already_registered( |
|
| 792 |
+ def test_overwriting_existing_entries( |
|
| 793 | 793 |
self, |
| 794 | 794 |
) -> None: |
| 795 |
- """The registry forbids overwriting entries.""" |
|
| 795 |
+ """The registry forbids incompatibly overwriting entries.""" |
|
| 796 | 796 |
registry = socketprovider.SocketProvider.registry.copy() |
| 797 | 797 |
resolve = socketprovider.SocketProvider.resolve |
| 798 | 798 |
register = socketprovider.SocketProvider.register |
| 799 |
- the_annoying_os = resolve("the_annoying_os")
|
|
| 800 |
- posix = resolve("posix")
|
|
| 799 |
+ |
|
| 800 |
+ def ancestry_chain(name: str) -> Iterator[str]: |
|
| 801 |
+ current: _types.SSHAgentSocketProvider | str | None = name |
|
| 802 |
+ seen: set[_types.SSHAgentSocketProvider | str | None] = set() |
|
| 803 |
+ while isinstance(current, str) and current not in seen: |
|
| 804 |
+ yield current |
|
| 805 |
+ seen.add(current) |
|
| 806 |
+ current = registry[current] |
|
| 807 |
+ |
|
| 808 |
+ # Entries with at least one level of alias resolving, and where each |
|
| 809 |
+ # entry ultimately resolves to a different provider. They needn't |
|
| 810 |
+ # technically be leaves of the socket provider forest, but "leaves" |
|
| 811 |
+ # emphasizes the quality we are looking for. |
|
| 812 |
+ leaves = ["posix", "windows"] |
|
| 813 |
+ assert all([isinstance(registry.get(leaf), str) for leaf in leaves]), ( |
|
| 814 |
+ "Registry is shaped incompatibly; cannot determine base entries" |
|
| 815 |
+ ) |
|
| 816 |
+ bases = [resolve(leaf) for leaf in leaves] |
|
| 817 |
+ names = [list(ancestry_chain(leaf)) for leaf in leaves] |
|
| 818 |
+ |
|
| 801 | 819 |
with pytest.MonkeyPatch.context() as monkeypatch: |
| 802 | 820 |
monkeypatch.setattr( |
| 803 | 821 |
socketprovider.SocketProvider, "registry", registry |
| 804 | 822 |
) |
| 805 |
- register("posix")(posix)
|
|
| 806 |
- register("the_annoying_os")(the_annoying_os)
|
|
| 823 |
+ # Registering names with different values is forbidden... |
|
| 807 | 824 |
with pytest.raises(ValueError, match="already registered"): |
| 808 |
- register("posix")(the_annoying_os)
|
|
| 825 |
+ register(names[0][0])(bases[1]) |
|
| 809 | 826 |
with pytest.raises(ValueError, match="already registered"): |
| 810 |
- register("the_annoying_os")(posix)
|
|
| 827 |
+ register(names[1][0])(bases[0]) |
|
| 828 |
+ # ...even if other names of the same registration are valid. |
|
| 811 | 829 |
with pytest.raises(ValueError, match="already registered"): |
| 812 |
- register("posix", "the_annoying_os_named_pipe")(posix)
|
|
| 830 |
+ register(names[0][0], names[1][1])(bases[0]) |
|
| 813 | 831 |
with pytest.raises(ValueError, match="already registered"): |
| 814 |
- register("the_annoying_os", "unix_domain")(the_annoying_os)
|
|
| 832 |
+ register(names[1][0], names[0][1])(bases[1]) |
|
| 815 | 833 |
|
| 816 | 834 |
def test_resolve_non_existant_entries( |
| 817 | 835 |
self, |
| ... | ... |
@@ -819,9 +837,7 @@ class TestSSHAgentSocketProviderRegistry: |
| 819 | 837 |
"""Resolving a non-existant entry fails.""" |
| 820 | 838 |
new_registry = {
|
| 821 | 839 |
"posix": socketprovider.SocketProvider.resolve("posix"),
|
| 822 |
- "the_annoying_os": socketprovider.SocketProvider.resolve( |
|
| 823 |
- "the_annoying_os" |
|
| 824 |
- ), |
|
| 840 |
+ "windows": socketprovider.SocketProvider.resolve("windows"),
|
|
| 825 | 841 |
} |
| 826 | 842 |
with pytest.MonkeyPatch.context() as monkeypatch: |
| 827 | 843 |
monkeypatch.setattr( |
| ... | ... |
@@ -841,9 +857,7 @@ class TestSSHAgentSocketProviderRegistry: |
| 841 | 857 |
names = ["spam", "ham", "eggs", "parrot"] |
| 842 | 858 |
new_registry = {
|
| 843 | 859 |
"posix": socketprovider.SocketProvider.resolve("posix"),
|
| 844 |
- "the_annoying_os": socketprovider.SocketProvider.resolve( |
|
| 845 |
- "the_annoying_os" |
|
| 846 |
- ), |
|
| 860 |
+ "windows": socketprovider.SocketProvider.resolve("windows"),
|
|
| 847 | 861 |
} |
| 848 | 862 |
with pytest.MonkeyPatch.context() as monkeypatch: |
| 849 | 863 |
monkeypatch.setattr( |
| ... | ... |
@@ -876,11 +890,11 @@ class TestSSHAgentSocketProviderRegistry: |
| 876 | 890 |
"ssh_auth_sock_on_posix": socketprovider.SocketProvider.resolve( |
| 877 | 891 |
"ssh_auth_sock_on_posix" |
| 878 | 892 |
), |
| 879 |
- "pageant_on_the_annoying_os": socketprovider.SocketProvider.resolve( |
|
| 880 |
- "pageant_on_the_annoying_os" |
|
| 893 |
+ "ssh_auth_sock_on_windows": socketprovider.SocketProvider.resolve( |
|
| 894 |
+ "ssh_auth_sock_on_windows" |
|
| 881 | 895 |
), |
| 882 |
- "posix": "ssh_auth_sock_on_posix", |
|
| 883 |
- "the_annoying_os": "pageant_on_the_annoying_os", |
|
| 896 |
+ "unix_domain": "ssh_auth_sock_on_posix", |
|
| 897 |
+ "windows_named_pipe": "ssh_auth_sock_on_windows", |
|
| 884 | 898 |
} |
| 885 | 899 |
names = [ |
| 886 | 900 |
k |
| ... | ... |
@@ -895,9 +909,15 @@ class TestSSHAgentSocketProviderRegistry: |
| 895 | 909 |
f"Existing SSH agent socket provider entry " |
| 896 | 910 |
f"{existing!r} is not existing?!"
|
| 897 | 911 |
) |
| 898 |
- assert not all( |
|
| 899 |
- map(socketprovider.SocketProvider.registry.__contains__, names) |
|
| 900 |
- ) |
|
| 912 |
+ # TODO(the-13th-letter): Do we require, forbid, or not care |
|
| 913 |
+ # if `names` discovers aliases not present in the mocked |
|
| 914 |
+ # registry? Currently, the "ssh_auth_sock" alias for |
|
| 915 |
+ # "ssh_auth_sock_on_posix" would be discovered, but |
|
| 916 |
+ # "ssh_auth_sock_on_windows" has no undiscovered aliases. |
|
| 917 |
+ # |
|
| 918 |
+ # assert not |
|
| 919 |
+ # all(map(socketprovider.SocketProvider.registry.__contains__, |
|
| 920 |
+ # names)) |
|
| 901 | 921 |
assert ( |
| 902 | 922 |
socketprovider.SocketProvider.register(existing, *names)( |
| 903 | 923 |
provider |
| ... | ... |
@@ -151,11 +151,19 @@ class SSHAgentSocketProviderRegistryStateMachine( |
| 151 | 151 |
self.registry: dict[ |
| 152 | 152 |
str, _types.SSHAgentSocketProvider | str | None |
| 153 | 153 |
] = {
|
| 154 |
- "ssh_auth_sock_on_posix": self.orig_registry["ssh_auth_sock_on_posix"], |
|
| 155 |
- "pageant_on_the_annoying_os": self.orig_registry["pageant_on_the_annoying_os"], |
|
| 156 |
- "native": self.orig_registry["native"], |
|
| 157 |
- "posix": "ssh_auth_sock_on_posix", |
|
| 158 |
- "the_annoying_os": "pageant_on_the_annoying_os", |
|
| 154 |
+ "ssh_auth_sock_on_posix": self.orig_registry[ |
|
| 155 |
+ "ssh_auth_sock_on_posix" |
|
| 156 |
+ ], |
|
| 157 |
+ "ssh_auth_sock_on_windows": self.orig_registry[ |
|
| 158 |
+ "ssh_auth_sock_on_windows" |
|
| 159 |
+ ], |
|
| 160 |
+ "native": ( |
|
| 161 |
+ "windows_named_pipe" |
|
| 162 |
+ if self.orig_registry["native"] == "windows" |
|
| 163 |
+ else "unix_domain" |
|
| 164 |
+ ), |
|
| 165 |
+ "unix_domain": "ssh_auth_sock_on_posix", |
|
| 166 |
+ "windows_named_pipe": "windows_named_pipe", |
|
| 159 | 167 |
} |
| 160 | 168 |
self.monkeypatch.setattr( |
| 161 | 169 |
socketprovider.SocketProvider, "registry", self.registry |
| 162 | 170 |