try to improve exception handling by avoiding raising ApiError from services
This commit is contained in:
parent
125f8eba03
commit
c449d17852
|
@ -20,6 +20,11 @@ from SpiffWorkflow.exceptions import WorkflowTaskException
|
|||
from SpiffWorkflow.specs.base import TaskSpec # type: ignore
|
||||
from SpiffWorkflow.task import Task # type: ignore
|
||||
|
||||
from spiffworkflow_backend.services.authentication_service import NotAuthorizedError
|
||||
from spiffworkflow_backend.services.authentication_service import TokenInvalidError
|
||||
from spiffworkflow_backend.services.authentication_service import TokenNotProvidedError
|
||||
from spiffworkflow_backend.services.authentication_service import UserNotLoggedInError
|
||||
|
||||
|
||||
api_error_blueprint = Blueprint("api_error_blueprint", __name__)
|
||||
|
||||
|
@ -172,7 +177,12 @@ def handle_exception(exception: Exception) -> flask.wrappers.Response:
|
|||
set_user_sentry_context()
|
||||
|
||||
sentry_link = None
|
||||
if not isinstance(exception, ApiError) or exception.error_code != "invalid_token":
|
||||
# we want to capture_exception to log the exception to sentry, but we don't want to log:
|
||||
# 1. ApiErrors that are just invalid tokens
|
||||
# 2. NotAuthorizedError
|
||||
if (
|
||||
not isinstance(exception, ApiError) or exception.error_code != "invalid_token"
|
||||
) and not isinstance(exception, NotAuthorizedError):
|
||||
id = capture_exception(exception)
|
||||
|
||||
if isinstance(exception, ApiError):
|
||||
|
@ -193,17 +203,30 @@ def handle_exception(exception: Exception) -> flask.wrappers.Response:
|
|||
# an event id or send out tags like username
|
||||
current_app.logger.exception(exception)
|
||||
|
||||
error_code = "internal_server_error"
|
||||
status_code = 500
|
||||
if (
|
||||
isinstance(exception, NotAuthorizedError)
|
||||
or isinstance(exception, TokenNotProvidedError)
|
||||
or isinstance(exception, TokenInvalidError)
|
||||
):
|
||||
error_code = "not_authorized"
|
||||
status_code = 403
|
||||
if isinstance(exception, UserNotLoggedInError):
|
||||
error_code = "not_authenticated"
|
||||
status_code = 401
|
||||
|
||||
# set api_exception like this to avoid confusing mypy
|
||||
# and what type the object is
|
||||
# about what type the object is
|
||||
api_exception = None
|
||||
if isinstance(exception, ApiError):
|
||||
api_exception = exception
|
||||
else:
|
||||
api_exception = ApiError(
|
||||
error_code="internal_server_error",
|
||||
error_code=error_code,
|
||||
message=f"{exception.__class__.__name__}",
|
||||
sentry_link=sentry_link,
|
||||
status_code=500,
|
||||
status_code=status_code,
|
||||
)
|
||||
|
||||
return make_response(jsonify(api_exception), api_exception.status_code)
|
||||
|
|
|
@ -11,7 +11,6 @@ from flask import current_app
|
|||
from flask import redirect
|
||||
from werkzeug.wrappers import Response
|
||||
|
||||
from spiffworkflow_backend.exceptions.api_error import ApiError
|
||||
from spiffworkflow_backend.models.db import db
|
||||
from spiffworkflow_backend.models.refresh_token import RefreshTokenModel
|
||||
|
||||
|
@ -20,7 +19,21 @@ class MissingAccessTokenError(Exception):
|
|||
"""MissingAccessTokenError."""
|
||||
|
||||
|
||||
class NotAuthorizedError(Exception):
|
||||
pass
|
||||
|
||||
|
||||
class RefreshTokenStorageError(Exception):
|
||||
pass
|
||||
|
||||
|
||||
class UserNotLoggedInError(Exception):
|
||||
pass
|
||||
|
||||
|
||||
# These could be either 'id' OR 'access' tokens and we can't always know which
|
||||
|
||||
|
||||
class TokenExpiredError(Exception):
|
||||
"""TokenExpiredError."""
|
||||
|
||||
|
@ -29,6 +42,10 @@ class TokenInvalidError(Exception):
|
|||
"""TokenInvalidError."""
|
||||
|
||||
|
||||
class TokenNotProvidedError(Exception):
|
||||
pass
|
||||
|
||||
|
||||
class AuthenticationProviderTypes(enum.Enum):
|
||||
"""AuthenticationServiceProviders."""
|
||||
|
||||
|
@ -183,9 +200,8 @@ class AuthenticationService:
|
|||
db.session.commit()
|
||||
except Exception as e:
|
||||
db.session.rollback()
|
||||
raise ApiError(
|
||||
error_code="store_refresh_token_error",
|
||||
message=f"We could not store the refresh token. Original error is {e}",
|
||||
raise RefreshTokenStorageError(
|
||||
f"We could not store the refresh token. Original error is {e}",
|
||||
) from e
|
||||
|
||||
@staticmethod
|
||||
|
|
|
@ -21,7 +21,6 @@ from SpiffWorkflow.task import Task as SpiffTask # type: ignore
|
|||
from sqlalchemy import or_
|
||||
from sqlalchemy import text
|
||||
|
||||
from spiffworkflow_backend.exceptions.api_error import ApiError
|
||||
from spiffworkflow_backend.helpers.api_version import V1_API_PATH_PREFIX
|
||||
from spiffworkflow_backend.models.db import db
|
||||
from spiffworkflow_backend.models.group import GroupModel
|
||||
|
@ -34,6 +33,11 @@ from spiffworkflow_backend.models.user import UserModel
|
|||
from spiffworkflow_backend.models.user import UserNotFoundError
|
||||
from spiffworkflow_backend.models.user_group_assignment import UserGroupAssignmentModel
|
||||
from spiffworkflow_backend.routes.openid_blueprint import openid_blueprint
|
||||
from spiffworkflow_backend.services.authentication_service import NotAuthorizedError
|
||||
from spiffworkflow_backend.services.authentication_service import TokenExpiredError
|
||||
from spiffworkflow_backend.services.authentication_service import TokenInvalidError
|
||||
from spiffworkflow_backend.services.authentication_service import TokenNotProvidedError
|
||||
from spiffworkflow_backend.services.authentication_service import UserNotLoggedInError
|
||||
from spiffworkflow_backend.services.group_service import GroupService
|
||||
from spiffworkflow_backend.services.user_service import UserService
|
||||
|
||||
|
@ -98,20 +102,16 @@ class AuthorizationService:
|
|||
def verify_sha256_token(cls, auth_header: Optional[str]) -> None:
|
||||
"""Verify_sha256_token."""
|
||||
if auth_header is None:
|
||||
raise ApiError(
|
||||
error_code="unauthorized",
|
||||
message="",
|
||||
status_code=403,
|
||||
raise TokenNotProvidedError(
|
||||
"unauthorized",
|
||||
)
|
||||
|
||||
received_sign = auth_header.split("sha256=")[-1].strip()
|
||||
secret = current_app.config["GITHUB_WEBHOOK_SECRET"].encode()
|
||||
expected_sign = HMAC(key=secret, msg=request.data, digestmod=sha256).hexdigest()
|
||||
if not compare_digest(received_sign, expected_sign):
|
||||
raise ApiError(
|
||||
error_code="unauthorized",
|
||||
message="",
|
||||
status_code=403,
|
||||
raise TokenInvalidError(
|
||||
"unauthorized",
|
||||
)
|
||||
|
||||
@classmethod
|
||||
|
@ -393,10 +393,8 @@ class AuthorizationService:
|
|||
authorization_exclusion_list = ["permissions_check"]
|
||||
|
||||
if not hasattr(g, "user"):
|
||||
raise ApiError(
|
||||
error_code="user_not_logged_in",
|
||||
message="User is not logged in. Please log in",
|
||||
status_code=401,
|
||||
raise UserNotLoggedInError(
|
||||
"User is not logged in. Please log in",
|
||||
)
|
||||
|
||||
api_view_function = current_app.view_functions[request.endpoint]
|
||||
|
@ -416,13 +414,11 @@ class AuthorizationService:
|
|||
if has_permission:
|
||||
return None
|
||||
|
||||
raise ApiError(
|
||||
error_code="unauthorized",
|
||||
message=(
|
||||
raise NotAuthorizedError(
|
||||
(
|
||||
f"User {g.user.username} is not authorized to perform requested action:"
|
||||
f" {permission_string} - {request.path}"
|
||||
),
|
||||
status_code=403,
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
|
@ -440,13 +436,11 @@ class AuthorizationService:
|
|||
payload = jwt.decode(auth_token, options={"verify_signature": False})
|
||||
return payload
|
||||
except jwt.ExpiredSignatureError as exception:
|
||||
raise ApiError(
|
||||
"token_expired",
|
||||
raise TokenExpiredError(
|
||||
"The Authentication token you provided expired and must be renewed.",
|
||||
) from exception
|
||||
except jwt.InvalidTokenError as exception:
|
||||
raise ApiError(
|
||||
"token_invalid",
|
||||
raise TokenInvalidError(
|
||||
(
|
||||
"The Authentication token you provided is invalid. You need a new"
|
||||
" token. "
|
||||
|
|
|
@ -35,7 +35,7 @@ export default function ProcessGroupForm({
|
|||
};
|
||||
|
||||
const hasValidIdentifier = (identifierToCheck: string) => {
|
||||
return identifierToCheck.match(/^[a-z0-9][0-9a-z-]+[a-z0-9]$/);
|
||||
return identifierToCheck.match(/^[a-z0-9][0-9a-z-]*[a-z0-9]$/);
|
||||
};
|
||||
|
||||
const handleFormSubmission = (event: any) => {
|
||||
|
|
Loading…
Reference in New Issue