Skip to content

Add specific exceptions for authentication issues #10633

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add specific exceptions for authentication issues #10633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions aiohttp/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@
ConnectionTimeoutError,
ContentTypeError,
Fingerprint,
InvalidAuthClientError,
InvalidRedirectUrlAuthClientError,
InvalidURL,
InvalidUrlAuthClientError,
InvalidUrlClientError,
InvalidUrlRedirectClientError,
NamedPipeConnector,
Expand Down Expand Up @@ -137,7 +140,10 @@
"ConnectionTimeoutError",
"ContentTypeError",
"Fingerprint",
"InvalidAuthClientError",
"InvalidRedirectUrlAuthClientError",
"InvalidURL",
"InvalidUrlAuthClientError",
"InvalidUrlClientError",
"InvalidUrlRedirectClientError",
"NonHttpUrlClientError",
Expand Down
20 changes: 19 additions & 1 deletion aiohttp/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,10 @@
ClientSSLError,
ConnectionTimeoutError,
ContentTypeError,
InvalidAuthClientError,
InvalidRedirectUrlAuthClientError,
InvalidURL,
InvalidUrlAuthClientError,
InvalidUrlClientError,
InvalidUrlRedirectClientError,
NonHttpUrlClientError,
Expand Down Expand Up @@ -124,7 +127,10 @@
"ClientSSLError",
"ConnectionTimeoutError",
"ContentTypeError",
"InvalidAuthClientError",
"InvalidRedirectUrlAuthClientError",
"InvalidURL",
"InvalidUrlAuthClientError",
"InvalidUrlClientError",
"RedirectClientError",
"NonHttpUrlClientError",
Expand Down Expand Up @@ -576,7 +582,19 @@ async def _request(

# Override the auth with the one from the URL only if we
# have no auth, or if we got an auth from a redirect URL
if auth is None or (history and auth_from_url is not None):
if (auth is None or history) and auth_from_url is not None:
# Pre-check the credentials can be encoded to
# avoid crashing down the line
try:
auth_from_url.encode()
except UnicodeEncodeError as e:
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a breaking change for anyone trapping UnicodeEncodeError currently since the new exceptions don't include it in the inheritance.

Copy link
Member

Choose a reason for hiding this comment

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

We could avoid the breaking change by making the new exceptions also derived from UnicodeEncodeError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, but hypothetically a case could be added later where these exceptions no longer stem from a UnicodeEncodeError so that would also make it weird.

If we don't want to make the exceptions as generic as possible now, I'd include UnicodeEncodeError in the exception names in addition to the inheritance to avoid someone reusing it for some other root error down the line.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could use InvalidRedirectUrlAuthClientUnicodeEncodeError and InvalidUrlAuthClientUnicodeEncodeError, which inherit from both.
I know, they're a bit of a mouthful 😉

or maybe just InvalidRedirectUrlAuthClientUnicodeError and InvalidUrlAuthClientUnicodeError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about a shorter [Redirect]UrlAuthUnicodeEncodingError?

auth_err_exc_cls = (
InvalidRedirectUrlAuthClientError
if redirects
else InvalidUrlAuthClientError
)
raise auth_err_exc_cls(url, str(e)) from e

auth = auth_from_url

if (
Expand Down
15 changes: 15 additions & 0 deletions aiohttp/client_exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,12 @@
"ContentTypeError",
"ClientPayloadError",
"InvalidURL",
"InvalidAuthClientError",
"InvalidUrlAuthClientError",
"InvalidUrlClientError",
"RedirectClientError",
"NonHttpUrlClientError",
"InvalidRedirectUrlAuthClientError",
"InvalidUrlRedirectClientError",
"NonHttpUrlRedirectClientError",
"WSMessageTypeError",
Expand Down Expand Up @@ -306,6 +309,14 @@ class InvalidUrlClientError(InvalidURL):
"""Invalid URL client error."""


class InvalidAuthClientError(ClientError):
"""Invalid auth client error."""


class InvalidUrlAuthClientError(InvalidURL):
"""Invalid URL auth client error."""


class RedirectClientError(ClientError):
"""Client redirect error."""

Expand All @@ -318,6 +329,10 @@ class InvalidUrlRedirectClientError(InvalidUrlClientError, RedirectClientError):
"""Invalid URL redirect client error."""


class InvalidRedirectUrlAuthClientError(InvalidUrlRedirectClientError):
"""Invalid redirect URL auth client error."""


class NonHttpUrlRedirectClientError(NonHttpUrlClientError, RedirectClientError):
"""Non http URL redirect client error."""

Expand Down
14 changes: 13 additions & 1 deletion aiohttp/client_reqrep.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@
ClientOSError,
ClientResponseError,
ContentTypeError,
InvalidAuthClientError,
InvalidURL,
InvalidUrlAuthClientError,
ServerFingerprintMismatch,
)
from .compression_utils import HAS_BROTLI
Expand Down Expand Up @@ -497,7 +499,11 @@ def update_transfer_encoding(self) -> None:
def update_auth(self, auth: Optional[BasicAuth], trust_env: bool = False) -> None:
"""Set basic auth."""
if auth is None:
auth_from_url = True
auth = self.auth
else:
auth_from_url = False

if auth is None and trust_env and self.url.host is not None:
netrc_obj = netrc_from_env()
with contextlib.suppress(LookupError):
Expand All @@ -508,7 +514,13 @@ def update_auth(self, auth: Optional[BasicAuth], trust_env: bool = False) -> Non
if not isinstance(auth, helpers.BasicAuth):
raise TypeError("BasicAuth() tuple is required instead")

self.headers[hdrs.AUTHORIZATION] = auth.encode()
try:
self.headers[hdrs.AUTHORIZATION] = auth.encode()
except UnicodeEncodeError as e:
auth_err_exc_cls = (
InvalidUrlAuthClientError if auth_from_url else InvalidAuthClientError
)
raise auth_err_exc_cls(self.url, str(e)) from e

def update_body_from_data(self, body: Any) -> None:
if body is None:
Expand Down
12 changes: 12 additions & 0 deletions tests/test_client_functional.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
from aiohttp.client_exceptions import (
ClientResponseError,
InvalidURL,
InvalidUrlAuthClientError,
InvalidUrlClientError,
InvalidUrlRedirectClientError,
NonHttpUrlClientError,
Expand Down Expand Up @@ -2685,6 +2686,13 @@ async def handler_redirect(request: web.Request) -> web.Response:
("http:///example.com", "http:///example.com"),
)

INVALID_URL_WITH_ERROR_MESSAGE_BAD_AUTH = (
(
"http://badchar username@example.com",
"http://example.com - 'latin-?1' codec can't encode",
),
)

NON_HTTP_URL_WITH_ERROR_MESSAGE = (
("call:+380123456789", r"call:\+380123456789"),
("skype:handle", "skype:handle"),
Expand All @@ -2706,6 +2714,10 @@ async def handler_redirect(request: web.Request) -> web.Response:
(url, message, InvalidUrlClientError)
for (url, message) in INVALID_URL_WITH_ERROR_MESSAGE_YARL_ORIGIN
),
*(
(url, message, InvalidUrlAuthClientError)
for (url, message) in INVALID_URL_WITH_ERROR_MESSAGE_BAD_AUTH
),
*(
(url, message, NonHttpUrlClientError)
for (url, message) in NON_HTTP_URL_WITH_ERROR_MESSAGE
Expand Down
5 changes: 5 additions & 0 deletions tests/test_client_request.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,11 @@ def test_invalid_url(make_request: _RequestMaker) -> None:
make_request("get", "hiwpefhipowhefopw")


def test_invalid_auth(make_request: _RequestMaker) -> None:
with pytest.raises(aiohttp.InvalidUrlAuthClientError):
make_request("get", "http://badchar username@example.com")


def test_no_path(make_request: _RequestMaker) -> None:
req = make_request("get", "http://python.org")
assert "/" == req.url.path
Expand Down
Loading