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

Fix authentication error payload #360

Merged
merged 10 commits into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
24 changes: 3 additions & 21 deletions tests/test_sync_http_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -204,15 +204,15 @@ def test_request_exceptions_include_expected_request_data(
# This'll fail for sure here but... just using the nice error that'd come up
assert ex.__class__ == expected_exception

def test_bad_request_exceptions_include_expected_request_data(self):
def test_bad_request_exceptions_include_request_data(self):
request_id = "request-123"
error = "example_error"
error_description = "Example error description"

self.http_client._client.request = MagicMock(
return_value=httpx.Response(
status_code=400,
json={"error": error, "error_description": error_description},
json={"error": error, "error_description": error_description, "foo": "bar"},
headers={"X-Request-ID": request_id},
),
)
Expand All @@ -222,29 +222,11 @@ def test_bad_request_exceptions_include_expected_request_data(self):
except BadRequestException as ex:
assert (
str(ex)
== "(message=No message, request_id=request-123, error=example_error, error_description=Example error description)"
== "(message=No message, error=example_error, error_description=Example error description, foo=bar, request_id=request-123)"
)
except Exception as ex:
assert ex.__class__ == BadRequestException

def test_bad_request_exceptions_exclude_expected_request_data(self):
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 we want is for everything in the return value to be added to exceptions, this test does the opposite.

request_id = "request-123"

self.http_client._client.request = MagicMock(
return_value=httpx.Response(
status_code=400,
json={"foo": "bar"},
headers={"X-Request-ID": request_id},
),
)

try:
self.http_client.request("bad_place")
except BadRequestException as ex:
assert str(ex) == "(message=No message, request_id=request-123)"
except Exception as ex:
assert ex.__class__ == BadRequestException

def test_request_bad_body_raises_expected_exception_with_request_data(self):
request_id = "request-123"

Expand Down
75 changes: 20 additions & 55 deletions workos/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,76 +2,41 @@

import httpx


# Request related exceptions
class BaseRequestException(Exception):
def __init__(
self,
response: httpx.Response,
message: Optional[str] = None,
error: Optional[str] = None,
errors: Optional[Mapping[str, Any]] = None,
error_description: Optional[str] = None,
code: Optional[str] = None,
pending_authentication_token: Optional[str] = None,
response_json: Mapping[str, Any],
) -> None:
super(BaseRequestException, self).__init__(message)

self.message = message
self.error = error
self.errors = errors
self.code = code
self.error_description = error_description
self.pending_authentication_token = pending_authentication_token
self.extract_and_set_response_related_data(response)
super(BaseRequestException, self).__init__(response_json)

def extract_and_set_response_related_data(self, response: httpx.Response) -> None:
self.response = response
self.response_json = response_json

try:
response_json = response.json()
self.message = response_json.get("message")
self.error = response_json.get("error")
self.errors = response_json.get("errors")
self.code = response_json.get("code")
self.error_description = response_json.get("error_description")
self.pending_authentication_token = response_json.get(
"pending_authentication_token"
)
except ValueError:
self.message = None
self.error = None
self.errors = None
self.code = None
self.error_description = None
self.pending_authentication_token = None

headers = response.headers
self.request_id = headers.get("X-Request-ID")
self.message = self.extractFromJson("message", "No message")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is 100x nicer than except ValueError, good stuff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Also lol snake case

self.error = self.extractFromJson("error", None)
self.errors = self.extractFromJson("errors", None)
self.code = self.extractFromJson("code", None )
self.error_description = self.extractFromJson("error_description", None)
Copy link
Contributor

Choose a reason for hiding this comment

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

error and error_description previously defaulted to "Unknown" so this is a small change in behavior (line)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good catch!


def __str__(self) -> str:
message = self.message or "No message"
exception = "(message=%s" % message
self.request_id = response.headers.get("X-Request-ID")

if self.request_id is not None:
exception += ", request_id=%s" % self.request_id
def extractFromJson(self, key: str, alt: Optional[str] = None) -> Optional[str]:
if self.response_json is None:
return alt

if self.code is not None:
exception += ", code=%s" % self.code
return self.response_json.get(key, alt)

if self.error is not None:
exception += ", error=%s" % self.error

if self.errors is not None:
exception += ", errors=%s" % self.errors
def __str__(self) -> str:
exception = "(message=%s" % self.message

if self.error_description is not None:
exception += ", error_description=%s" % self.error_description
if self.response_json is not None:
for key, value in self.response_json.items():
if key != "message":
exception += ", %s=%s" % (key, value)

if self.pending_authentication_token is not None:
exception += (
", pending_authentication_token=%s" % self.pending_authentication_token
)
exception += ", request_id=%s" % self.request_id

return exception + ")"

Expand Down
24 changes: 6 additions & 18 deletions workos/utils/_base_http_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,27 +96,15 @@ def _maybe_raise_error_by_status_code(
status_code = response.status_code
if status_code >= 400 and status_code < 500:
if status_code == 401:
raise AuthenticationException(response)
raise AuthenticationException(response, response_json)
elif status_code == 403:
raise AuthorizationException(response)
raise AuthorizationException(response, response_json)
elif status_code == 404:
raise NotFoundException(response)

error = (
response_json.get("error")
if response_json and "error" in response_json
else "Unknown"
)
error_description = (
response_json.get("error_description")
if response_json and "error_description" in response_json
else "Unknown"
)
raise BadRequestException(
response, error=error, error_description=error_description
)
raise NotFoundException(response, response_json)

raise BadRequestException(response, response_json)
elif status_code >= 500 and status_code < 600:
raise ServerException(response)
raise ServerException(response, response_json)

def _prepare_request(
self,
Expand Down
Loading