From 3e0348c34e15cf3cf881aaf99160fc61f1d454ce Mon Sep 17 00:00:00 2001 From: jamshale Date: Tue, 19 Sep 2023 19:33:56 -0700 Subject: [PATCH 1/4] Catch and display meaningful error when there is no valid client Signed-off-by: jamshale --- oidc-controller/api/routers/oidc.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/oidc-controller/api/routers/oidc.py b/oidc-controller/api/routers/oidc.py index 2a9ca77a..53c411a1 100644 --- a/oidc-controller/api/routers/oidc.py +++ b/oidc-controller/api/routers/oidc.py @@ -5,11 +5,13 @@ import qrcode import structlog -from fastapi import APIRouter, Depends, Request +from fastapi import APIRouter, Depends, HTTPException, Request from fastapi.responses import HTMLResponse, JSONResponse, RedirectResponse +from fastapi import status as http_status from jinja2 import Template from oic.oic.message import AccessTokenRequest, AuthorizationRequest from pymongo.database import Database +from pyop.exceptions import InvalidAuthenticationRequest from ..authSessions.crud import AuthSessionCreate, AuthSessionCRUD from ..authSessions.models import AuthSessionPatch, AuthSessionState @@ -80,9 +82,15 @@ async def get_authorize(request: Request, db: Database = Depends(get_db)): model = AuthorizationRequest().from_dict(request.query_params._dict) model.verify() - auth_req = provider.provider.parse_authentication_request( - urlencode(request.query_params._dict), request.headers - ) + try: + auth_req = provider.provider.parse_authentication_request( + urlencode(request.query_params._dict), request.headers + ) + except InvalidAuthenticationRequest as e: + raise HTTPException( + status_code=http_status.HTTP_400_BAD_REQUEST, + detail=f"Invalid auth request: {e}") + # fetch placeholder user/model and create proof authn_response = provider.provider.authorize(model, "vc-user") From d969e868f21fc2453a6825863db2c7c8a07305cf Mon Sep 17 00:00:00 2001 From: jamshale Date: Tue, 19 Sep 2023 19:34:33 -0700 Subject: [PATCH 2/4] Improve not found error detail Signed-off-by: jamshale --- oidc-controller/api/clientConfigurations/crud.py | 7 ++++--- oidc-controller/api/core/http_exception_util.py | 7 +++++-- oidc-controller/api/verificationConfigs/crud.py | 7 ++++--- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/oidc-controller/api/clientConfigurations/crud.py b/oidc-controller/api/clientConfigurations/crud.py index a483722b..cc7331bb 100644 --- a/oidc-controller/api/clientConfigurations/crud.py +++ b/oidc-controller/api/clientConfigurations/crud.py @@ -16,6 +16,7 @@ logger: structlog.typing.FilteringBoundLogger = structlog.getLogger(__name__) +NOT_FOUND_MSG = "The requested client configuration wasn't found" class ClientConfigurationCRUD: def __init__(self, db: Database): @@ -40,7 +41,7 @@ async def create( async def get(self, client_id: str) -> ClientConfiguration: col = self._db.get_collection(COLLECTION_NAMES.CLIENT_CONFIGURATIONS) obj = col.find_one({"client_id": client_id}) - check_and_raise_not_found_http_exception(obj) + check_and_raise_not_found_http_exception(obj, NOT_FOUND_MSG) return ClientConfiguration(**obj) @@ -57,7 +58,7 @@ async def patch( {"$set": data.dict(exclude_unset=True)}, return_document=ReturnDocument.AFTER, ) - check_and_raise_not_found_http_exception(obj) + check_and_raise_not_found_http_exception(obj, NOT_FOUND_MSG) # remake provider instance to refresh provider client await init_provider(self._db) @@ -66,7 +67,7 @@ async def patch( async def delete(self, client_id: str) -> bool: col = self._db.get_collection(COLLECTION_NAMES.CLIENT_CONFIGURATIONS) obj = col.find_one_and_delete({"client_id": client_id}) - check_and_raise_not_found_http_exception(obj) + check_and_raise_not_found_http_exception(obj, NOT_FOUND_MSG) # remake provider instance to refresh provider client await init_provider(self._db) diff --git a/oidc-controller/api/core/http_exception_util.py b/oidc-controller/api/core/http_exception_util.py index a57114eb..9d071d14 100644 --- a/oidc-controller/api/core/http_exception_util.py +++ b/oidc-controller/api/core/http_exception_util.py @@ -20,9 +20,12 @@ def raise_appropriate_http_exception(err: WriteError, exists_msg: str = None): ) -def check_and_raise_not_found_http_exception(resp): +def check_and_raise_not_found_http_exception(resp, detail: str = None): + if detail is None: + detail = "The requested resource wasn't found" + if resp is None: raise HTTPException( status_code=http_status.HTTP_404_NOT_FOUND, - detail="The requested resource wasn't found", + detail=detail, ) diff --git a/oidc-controller/api/verificationConfigs/crud.py b/oidc-controller/api/verificationConfigs/crud.py index ed5ee421..d6a1bfa1 100644 --- a/oidc-controller/api/verificationConfigs/crud.py +++ b/oidc-controller/api/verificationConfigs/crud.py @@ -12,6 +12,7 @@ VerificationConfigPatch, ) +NOT_FOUND_MSG = "The requested verifier configuration wasn't found" class VerificationConfigCRUD: _db: Database @@ -32,7 +33,7 @@ async def create(self, ver_config: VerificationConfig) -> VerificationConfig: async def get(self, ver_config_id: str) -> VerificationConfig: ver_confs = self._db.get_collection(COLLECTION_NAMES.VER_CONFIGS) ver_conf = ver_confs.find_one({"ver_config_id": ver_config_id}) - check_and_raise_not_found_http_exception(ver_conf) + check_and_raise_not_found_http_exception(ver_conf, NOT_FOUND_MSG) return VerificationConfig(**ver_conf) @@ -52,7 +53,7 @@ async def patch( {"$set": data.dict(exclude_unset=True)}, return_document=ReturnDocument.AFTER, ) - check_and_raise_not_found_http_exception(ver_conf) + check_and_raise_not_found_http_exception(ver_conf, NOT_FOUND_MSG) return ver_conf @@ -60,5 +61,5 @@ async def delete(self, ver_config_id: str) -> bool: ver_confs = self._db.get_collection(COLLECTION_NAMES.VER_CONFIGS) ver_conf = ver_confs.find_one_and_delete( {"ver_config_id": ver_config_id}) - check_and_raise_not_found_http_exception(ver_conf) + check_and_raise_not_found_http_exception(ver_conf, NOT_FOUND_MSG) return bool(ver_conf) From 956b8f971303442f8270fd83107b2858b95670de Mon Sep 17 00:00:00 2001 From: jamshale Date: Wed, 20 Sep 2023 08:47:34 -0700 Subject: [PATCH 3/4] Clean up default error message handling Signed-off-by: jamshale --- oidc-controller/api/core/http_exception_util.py | 12 ++++++------ oidc-controller/api/verificationConfigs/crud.py | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/oidc-controller/api/core/http_exception_util.py b/oidc-controller/api/core/http_exception_util.py index 9d071d14..e50f7d21 100644 --- a/oidc-controller/api/core/http_exception_util.py +++ b/oidc-controller/api/core/http_exception_util.py @@ -5,8 +5,11 @@ logger = structlog.getLogger(__name__) +CONFLICT_DEFAULT_MSG = "The requested resource already exists" +NOT_FOUND_DEFAULT_MSG = "The requested resource wasn't found" +UNKNOWN_DEFAULT_MSG = "The server was unable to process the request" -def raise_appropriate_http_exception(err: WriteError, exists_msg: str = None): +def raise_appropriate_http_exception(err: WriteError, exists_msg: str = CONFLICT_DEFAULT_MSG): if err.code == 11000: raise HTTPException( status_code=http_status.HTTP_409_CONFLICT, @@ -16,14 +19,11 @@ def raise_appropriate_http_exception(err: WriteError, exists_msg: str = None): logger.error("Unknown error", err=err) raise HTTPException( status_code=http_status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="The server was unable to process the request", + detail=UNKNOWN_DEFAULT_MSG, ) -def check_and_raise_not_found_http_exception(resp, detail: str = None): - if detail is None: - detail = "The requested resource wasn't found" - +def check_and_raise_not_found_http_exception(resp, detail: str = NOT_FOUND_DEFAULT_MSG): if resp is None: raise HTTPException( status_code=http_status.HTTP_404_NOT_FOUND, diff --git a/oidc-controller/api/verificationConfigs/crud.py b/oidc-controller/api/verificationConfigs/crud.py index d6a1bfa1..f7a53a91 100644 --- a/oidc-controller/api/verificationConfigs/crud.py +++ b/oidc-controller/api/verificationConfigs/crud.py @@ -27,7 +27,7 @@ async def create(self, ver_config: VerificationConfig) -> VerificationConfig: ver_confs.insert_one(jsonable_encoder(ver_config)) except Exception as err: raise_appropriate_http_exception( - err, exists_msg="Verification configuration already exists") + err, exists_msg="Verifier configuration already exists") return ver_confs.find_one({"ver_config_id": ver_config.ver_config_id}) async def get(self, ver_config_id: str) -> VerificationConfig: From 107e7ca9989ffe6081121e7e71d4c5611a3aae82 Mon Sep 17 00:00:00 2001 From: jamshale Date: Wed, 20 Sep 2023 08:59:09 -0700 Subject: [PATCH 4/4] return a response from middleware instead of exception Signed-off-by: jamshale --- oidc-controller/api/main.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/oidc-controller/api/main.py b/oidc-controller/api/main.py index 11b65e28..8e75b9fb 100644 --- a/oidc-controller/api/main.py +++ b/oidc-controller/api/main.py @@ -10,9 +10,9 @@ from fastapi import FastAPI from starlette.requests import Request from starlette.responses import Response -from fastapi.exceptions import HTTPException from fastapi.middleware.cors import CORSMiddleware from fastapi import status as http_status +from fastapi.responses import JSONResponse from .clientConfigurations.router import router as client_config_router from .db.session import get_db, init_db @@ -21,7 +21,6 @@ from .clientConfigurations.router import router as client_config_router from .db.session import init_db, get_db from .routers.socketio import sio_app -from api.core.models import GenericErrorMessage from api.core.oidc.provider import init_provider logger: structlog.typing.FilteringBoundLogger = structlog.getLogger(__name__) @@ -103,9 +102,13 @@ async def logging_middleware(request: Request, call_next) -> Response: if os.environ.get("LOG_WITH_JSON", True) is True: logger.error(traceback.format_exc()) - raise HTTPException( + return JSONResponse( status_code=http_status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="Internal Server Error", + content={ + "status": "error", + "message": "Internal Server Error", + "process_time": process_time, + }, )