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

Display a meaningful error message #304

Merged
merged 1 commit into from
Nov 29, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
22 changes: 18 additions & 4 deletions src/backend/admin_ops/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
from botocore.auth import HmacV1Auth
from botocore.awsrequest import AWSRequest
from botocore.credentials import Credentials
from fastapi import HTTPException, status
from fastapi.logger import logger

from backend.admin_ops.errors import error_from_response

Expand Down Expand Up @@ -63,10 +65,22 @@ def signed_request(

async def send_request(req: httpx.Request) -> httpx.Response:
async with httpx.AsyncClient(verify=False) as client:
res: httpx.Response = await client.send(req)
if not res.is_success:
raise error_from_response(res)
return res
try:
res: httpx.Response = await client.send(req)
if not res.is_success:
raise error_from_response(res)
return res
except httpx.ConnectError as e:
raise HTTPException(
status_code=status.HTTP_502_BAD_GATEWAY,
detail=str(e),
)
except Exception as e:
detail: str | None = str(e) or None
logger.error(f"Unknown exception ({type(e)}): {detail}")
raise HTTPException(
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=detail
)


async def do_request(
Expand Down
7 changes: 4 additions & 3 deletions src/backend/api/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,11 @@ async def conn(self, attempts: int = 1) -> AsyncGenerator[S3Client, None]:
# propagate it.
raise e
except Exception as e:
logger.error(f"Unknown error: {e}")
logger.error(f" exception: {type(e)}")
detail: str | None = str(e) or None
logger.error(f"Unknown exception ({type(e)}): {detail}")
raise HTTPException(
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
detail=detail,
)


Expand Down
28 changes: 15 additions & 13 deletions src/backend/api/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

from typing import Annotated

from fastapi import Depends, HTTPException, status
from fastapi import Depends, HTTPException
from fastapi.routing import APIRouter
from types_aiobotocore_s3.type_defs import ListBucketsOutputTypeDef

Expand All @@ -36,8 +36,8 @@
async def authenticate(conn: S3GWClientDep) -> AuthUser:
# Try to access a RGW Admin Ops endpoint first. If that works, the
# given credentials have `admin` privileges. If it fails, try to
# access a RGW endpoint. If that works, the given credentials can
# be used to sign in as `regular` user.
# access a RGW endpoint via AWS S3 API. If that works, the given
# credentials can be used to sign in as `regular` user.
try:
admin_ops_res: admin_ops_types.UserInfo = (
await admin_ops_users.get_user_info(
Expand All @@ -53,14 +53,16 @@ async def authenticate(conn: S3GWClientDep) -> AuthUser:
IsAdmin=admin_ops_res.admin,
)
except HTTPException:
try:
async with conn.conn() as s3:
s3_res: ListBucketsOutputTypeDef = await s3.list_buckets()
res = AuthUser(
ID=s3_res["Owner"]["ID"],
DisplayName=s3_res["Owner"]["DisplayName"],
IsAdmin=False,
)
except HTTPException:
raise HTTPException(status_code=status.HTTP_403_FORBIDDEN)
# Handle ALL HTTP related exceptions here, e.g. connection errors.
# The `httpx` library used for calling the Admin Ops API does not
# provide meaningful error messages, whereas `botocore` provides
# connection error messages that might be more helpful when they
# are displayed in the UI.
async with conn.conn() as s3:
s3_res: ListBucketsOutputTypeDef = await s3.list_buckets()
res = AuthUser(
ID=s3_res["Owner"]["ID"],
DisplayName=s3_res["Owner"]["DisplayName"],
IsAdmin=False,
)
return res
10 changes: 6 additions & 4 deletions src/backend/tests/unit/admin_ops/test_buckets.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import httpx
import pytest
from fastapi import HTTPException
from fastapi import HTTPException, status
from pytest_httpx import HTTPXMock

import backend.admin_ops.buckets as buckets
Expand Down Expand Up @@ -105,7 +105,7 @@ async def test_bucket_list(httpx_mock: HTTPXMock) -> None:
@pytest.mark.anyio
async def test_bucket_list_failure(httpx_mock: HTTPXMock) -> None:
httpx_mock.add_response( # pyright: ignore [reportUnknownMemberType]
status_code=404 # any error, really
status_code=status.HTTP_404_NOT_FOUND # any error, really
)

with pytest.raises(HTTPException) as e:
Expand All @@ -115,7 +115,7 @@ async def test_bucket_list_failure(httpx_mock: HTTPXMock) -> None:
secret_key="qwe",
uid=None,
)
assert e.value.status_code == 404
assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR


@pytest.mark.anyio
Expand All @@ -126,7 +126,9 @@ def check_uid(req: httpx.Request) -> httpx.Response:
nonlocal called_cb
called_cb = True
assert str(req.url.query).find("uid=asdasd") >= 0
return httpx.Response(status_code=200, json=bucket_list_response)
return httpx.Response(
status_code=status.HTTP_200_OK, json=bucket_list_response
)

httpx_mock.add_callback( # pyright: ignore [reportUnknownMemberType]
check_uid
Expand Down
6 changes: 3 additions & 3 deletions src/backend/tests/unit/admin_ops/test_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

import httpx
import pytest
from fastapi import HTTPException
from fastapi import HTTPException, status
from pytest_httpx import HTTPXMock
from pytest_mock import MockerFixture

Expand Down Expand Up @@ -128,14 +128,14 @@ async def test_get_user_info_3(httpx_mock: HTTPXMock) -> None:
@pytest.mark.anyio
async def test_get_user_info_failure(httpx_mock: HTTPXMock) -> None:
httpx_mock.add_response( # pyright: ignore [reportUnknownMemberType]
status_code=404, # any error, really
status_code=status.HTTP_404_NOT_FOUND, # any error, really
)

with pytest.raises(HTTPException) as e:
await get_user_info(
url="http://foo.bar:123", access_key="asd", secret_key="qwe"
)
assert e.value.status_code == 404
assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR


@pytest.mark.anyio
Expand Down
27 changes: 25 additions & 2 deletions src/backend/tests/unit/api/test_api_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
# limitations under the License.

import pytest
from fastapi import HTTPException
from botocore.exceptions import EndpointConnectionError
from fastapi import HTTPException, status
from httpx import ConnectError
from pytest_httpx import HTTPXMock
from pytest_mock import MockerFixture

Expand Down Expand Up @@ -105,4 +107,25 @@ async def test_authenticate_3(

with pytest.raises(HTTPException) as e:
await auth.authenticate(s3_client)
assert e.value.status_code == 403
assert e.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR


@pytest.mark.anyio
async def test_authenticate_4(
s3_client: S3GWClient, mocker: MockerFixture, httpx_mock: HTTPXMock
) -> None:
httpx_mock.add_exception(
exception=ConnectError(message="All connection attempts failed")
)

s3api_mock = S3ApiMock(s3_client, mocker)
s3api_mock.patch(
"list_buckets",
side_effect=EndpointConnectionError(
endpoint_url="http://127.0.0.1:7481/"
),
)

with pytest.raises(HTTPException) as e:
await auth.authenticate(s3_client)
assert e.value.status_code == status.HTTP_502_BAD_GATEWAY