diff --git a/src/backend/admin_ops/__init__.py b/src/backend/admin_ops/__init__.py index cd88dff5..45adbeff 100644 --- a/src/backend/admin_ops/__init__.py +++ b/src/backend/admin_ops/__init__.py @@ -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 @@ -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( diff --git a/src/backend/api/__init__.py b/src/backend/api/__init__.py index a5720f64..e9d23c98 100644 --- a/src/backend/api/__init__.py +++ b/src/backend/api/__init__.py @@ -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, ) diff --git a/src/backend/api/auth.py b/src/backend/api/auth.py index a214689b..8333206d 100644 --- a/src/backend/api/auth.py +++ b/src/backend/api/auth.py @@ -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 @@ -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( @@ -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 diff --git a/src/backend/tests/unit/admin_ops/test_buckets.py b/src/backend/tests/unit/admin_ops/test_buckets.py index da7836a0..2744ffc6 100644 --- a/src/backend/tests/unit/admin_ops/test_buckets.py +++ b/src/backend/tests/unit/admin_ops/test_buckets.py @@ -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 @@ -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: @@ -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 @@ -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 diff --git a/src/backend/tests/unit/admin_ops/test_users.py b/src/backend/tests/unit/admin_ops/test_users.py index eb7fd48a..01fb553f 100644 --- a/src/backend/tests/unit/admin_ops/test_users.py +++ b/src/backend/tests/unit/admin_ops/test_users.py @@ -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 @@ -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 diff --git a/src/backend/tests/unit/api/test_api_auth.py b/src/backend/tests/unit/api/test_api_auth.py index c9e1b190..ab84db9f 100644 --- a/src/backend/tests/unit/api/test_api_auth.py +++ b/src/backend/tests/unit/api/test_api_auth.py @@ -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 @@ -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