From d02f2d1fa7e46c61895d39c2adc40297c36e4cbe Mon Sep 17 00:00:00 2001 From: Erik Ritter Date: Wed, 13 May 2020 17:10:37 -0700 Subject: [PATCH] feat: return security errors in the SIP-40 format (#9796) --- .../utils/getClientErrorObject_spec.ts | 3 +- .../src/components/ErrorMessage/types.ts | 11 +++++ .../src/utils/getClientErrorObject.ts | 4 +- superset/errors.py | 12 +++++- superset/exceptions.py | 13 ++++-- superset/security/manager.py | 40 ++++++++++++++++++- superset/views/base.py | 30 ++++++++++++-- superset/views/core.py | 28 +++++++++---- tests/core_tests.py | 3 +- 9 files changed, 123 insertions(+), 21 deletions(-) diff --git a/superset-frontend/spec/javascripts/utils/getClientErrorObject_spec.ts b/superset-frontend/spec/javascripts/utils/getClientErrorObject_spec.ts index afadb75f4..19314806f 100644 --- a/superset-frontend/spec/javascripts/utils/getClientErrorObject_spec.ts +++ b/superset-frontend/spec/javascripts/utils/getClientErrorObject_spec.ts @@ -49,7 +49,7 @@ describe('getClientErrorObject()', () => { errors: [ { errorType: ErrorTypeEnum.GENERIC_DB_ENGINE_ERROR, - extra: { engine: 'presto' }, + extra: { engine: 'presto', link: 'https://www.google.com' }, level: 'error', message: 'presto error: test error', }, @@ -60,6 +60,7 @@ describe('getClientErrorObject()', () => { return getClientErrorObject(new Response(jsonErrorString)).then( errorObj => { expect(errorObj.error).toEqual(jsonError.errors[0].message); + expect(errorObj.link).toEqual(jsonError.errors[0].extra.link); }, ); }); diff --git a/superset-frontend/src/components/ErrorMessage/types.ts b/superset-frontend/src/components/ErrorMessage/types.ts index 1d259b295..2d4cf4592 100644 --- a/superset-frontend/src/components/ErrorMessage/types.ts +++ b/superset-frontend/src/components/ErrorMessage/types.ts @@ -19,13 +19,24 @@ // Keep in sync with superset/views/errors.py export const ErrorTypeEnum = { + // Frontend errors FRONTEND_CSRF_ERROR: 'FRONTEND_CSRF_ERROR', FRONTEND_NETWORK_ERROR: 'FRONTEND_NETWORK_ERROR', FRONTEND_TIMEOUT_ERROR: 'FRONTEND_TIMEOUT_ERROR', + // DB Engine errors GENERIC_DB_ENGINE_ERROR: 'GENERIC_DB_ENGINE_ERROR', + // Viz errors VIZ_GET_DF_ERROR: 'VIZ_GET_DF_ERROR', + UNKNOWN_DATASOURCE_TYPE_ERROR: 'UNKNOWN_DATASOURCE_TYPE_ERROR', + FAILED_FETCHING_DATASOURCE_INFO_ERROR: + 'FAILED_FETCHING_DATASOURCE_INFO_ERROR', + + // Security access errors + TABLE_SECURITY_ACCESS_ERROR: 'TABLE_SECURITY_ACCESS_ERROR', + DATASOURCE_SECURITY_ACCESS_ERROR: 'DATASOURCE_SECURITY_ACCESS_ERROR', + MISSING_OWNERSHIP_ERROR: 'MISSING_OWNERSHIP_ERROR', } as const; type ValueOf = T[keyof T]; diff --git a/superset-frontend/src/utils/getClientErrorObject.ts b/superset-frontend/src/utils/getClientErrorObject.ts index 507f9faf2..421aa01a5 100644 --- a/superset-frontend/src/utils/getClientErrorObject.ts +++ b/superset-frontend/src/utils/getClientErrorObject.ts @@ -26,8 +26,9 @@ import COMMON_ERR_MESSAGES from './errorMessages'; export type ClientErrorObject = { error: string; errors?: SupersetError[]; - severity?: string; + link?: string; message?: string; + severity?: string; stacktrace?: string; } & Partial; @@ -54,6 +55,7 @@ export default function getClientErrorObject( // Backwards compatibility for old error renderers with the new error object if (error.errors && error.errors.length > 0) { error.error = error.description = error.errors[0].message; + error.link = error.errors[0]?.extra?.link; } if (error.stack) { diff --git a/superset/errors.py b/superset/errors.py index aaa7aa2b6..54eb0ed63 100644 --- a/superset/errors.py +++ b/superset/errors.py @@ -14,7 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -# pylint: disable=too-few-public-methods +# pylint: disable=too-few-public-methods,invalid-name from enum import Enum from typing import Any, Dict, Optional @@ -28,13 +28,23 @@ class SupersetErrorType(str, Enum): Keep in sync with superset-frontend/src/components/ErrorMessage/types.ts """ + # Frontend errors FRONTEND_CSRF_ERROR = "FRONTEND_CSRF_ERROR" FRONTEND_NETWORK_ERROR = "FRONTEND_NETWORK_ERROR" FRONTEND_TIMEOUT_ERROR = "FRONTEND_TIMEOUT_ERROR" + # DB Engine errors GENERIC_DB_ENGINE_ERROR = "GENERIC_DB_ENGINE_ERROR" + # Viz errors VIZ_GET_DF_ERROR = "VIZ_GET_DF_ERROR" + UNKNOWN_DATASOURCE_TYPE_ERROR = "UNKNOWN_DATASOURCE_TYPE_ERROR" + FAILED_FETCHING_DATASOURCE_INFO_ERROR = "FAILED_FETCHING_DATASOURCE_INFO_ERROR" + + # Security access errors + TABLE_SECURITY_ACCESS_ERROR = "TABLE_SECURITY_ACCESS_ERROR" + DATASOURCE_SECURITY_ACCESS_ERROR = "DATASOURCE_SECURITY_ACCESS_ERROR" + MISSING_OWNERSHIP_ERROR = "MISSING_OWNERSHIP_ERROR" class ErrorLevel(str, Enum): diff --git a/superset/exceptions.py b/superset/exceptions.py index 33841cd3c..59ea0427e 100644 --- a/superset/exceptions.py +++ b/superset/exceptions.py @@ -14,10 +14,12 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -from typing import Optional +from typing import Any, Dict, Optional from flask_babel import gettext as _ +from superset.errors import SupersetError + class SupersetException(Exception): status = 500 @@ -41,9 +43,12 @@ class SupersetTimeoutException(SupersetException): class SupersetSecurityException(SupersetException): status = 401 - def __init__(self, msg: str, link: Optional[str] = None) -> None: - super(SupersetSecurityException, self).__init__(msg) - self.link = link + def __init__( + self, error: SupersetError, payload: Optional[Dict[str, Any]] = None + ) -> None: + super(SupersetSecurityException, self).__init__(error.message) + self.error = error + self.payload = payload class NoDataException(SupersetException): diff --git a/superset/security/manager.py b/superset/security/manager.py index c48c231c2..021ca9fe0 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -43,6 +43,7 @@ from sqlalchemy.orm.query import Query from superset import sql_parse from superset.connectors.connector_registry import ConnectorRegistry from superset.constants import RouteMethod +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import SupersetSecurityException from superset.utils.core import DatasourceName @@ -291,6 +292,25 @@ class SupersetSecurityManager(SecurityManager): return conf.get("PERMISSION_INSTRUCTIONS_LINK") + def get_datasource_access_error_object( + self, datasource: "BaseDatasource" + ) -> SupersetError: + """ + Return the error object for the denied Superset datasource. + + :param datasource: The denied Superset datasource + :returns: The error object + """ + return SupersetError( + error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR, + message=self.get_datasource_access_error_msg(datasource), + level=ErrorLevel.ERROR, + extra={ + "link": self.get_datasource_access_link(datasource), + "datasource": datasource.name, + }, + ) + def get_table_access_error_msg(self, tables: Set["Table"]) -> str: """ Return the error message for the denied SQL tables. @@ -303,6 +323,23 @@ class SupersetSecurityManager(SecurityManager): return f"""You need access to the following tables: {", ".join(quoted_tables)}, `all_database_access` or `all_datasource_access` permission""" + def get_table_access_error_object(self, tables: Set["Table"]) -> SupersetError: + """ + Return the error object for the denied SQL tables. + + :param tables: The set of denied SQL tables + :returns: The error object + """ + return SupersetError( + error_type=SupersetErrorType.TABLE_SECURITY_ACCESS_ERROR, + message=self.get_table_access_error_msg(tables), + level=ErrorLevel.ERROR, + extra={ + "link": self.get_table_access_link(tables), + "tables": [str(table) for table in tables], + }, + ) + def get_table_access_link(self, tables: Set["Table"]) -> Optional[str]: """ Return the access link for the denied SQL tables. @@ -828,8 +865,7 @@ class SupersetSecurityManager(SecurityManager): if not self.datasource_access(datasource): raise SupersetSecurityException( - self.get_datasource_access_error_msg(datasource), - self.get_datasource_access_link(datasource), + self.get_datasource_access_error_object(datasource), ) def assert_query_context_permission(self, query_context: "QueryContext") -> None: diff --git a/superset/views/base.py b/superset/views/base.py index 1faa90faf..def1ffa76 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -20,6 +20,7 @@ import traceback from datetime import datetime from typing import Any, Dict, List, Optional +import dataclasses import simplejson as json import yaml from flask import abort, flash, g, get_flashed_messages, redirect, Response, session @@ -44,6 +45,7 @@ from superset import ( security_manager, ) from superset.connectors.sqla import models +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import SupersetException, SupersetSecurityException from superset.translations.utils import get_language_pack from superset.utils import core as utils @@ -81,7 +83,7 @@ def get_error_msg() -> str: def json_error_response( msg: Optional[str] = None, status: int = 500, - payload: Optional[dict] = None, + payload: Optional[Dict[str, Any]] = None, link: Optional[str] = None, ) -> Response: if not payload: @@ -96,6 +98,22 @@ def json_error_response( ) +def json_errors_response( + errors: List[SupersetError], + status: int = 500, + payload: Optional[Dict[str, Any]] = None, +) -> Response: + if not payload: + payload = {} + + payload["errors"] = [dataclasses.asdict(error) for error in errors] + return Response( + json.dumps(payload, default=utils.json_iso_dttm_ser, ignore_nan=True), + status=status, + mimetype="application/json", + ) + + def json_success(json_msg: str, status: int = 200) -> Response: return Response(json_msg, status=status, mimetype="application/json") @@ -142,8 +160,8 @@ def handle_api_exception(f): return f(self, *args, **kwargs) except SupersetSecurityException as ex: logger.exception(ex) - return json_error_response( - utils.error_msg_from_exception(ex), status=ex.status, link=ex.link + return json_errors_response( + errors=[ex.error], status=ex.status, payload=ex.payload ) except SupersetException as ex: logger.exception(ex) @@ -432,7 +450,11 @@ def check_ownership(obj: Any, raise_if_false: bool = True) -> bool: return False security_exception = SupersetSecurityException( - "You don't have the rights to alter [{}]".format(obj) + SupersetError( + error_type=SupersetErrorType.MISSING_OWNERSHIP_ERROR, + message="You don't have the rights to alter [{}]".format(obj), + level=ErrorLevel.ERROR, + ) ) if g.user.is_anonymous: diff --git a/superset/views/core.py b/superset/views/core.py index 6cf82a093..e688d423a 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -66,6 +66,7 @@ from superset import ( from superset.connectors.connector_registry import ConnectorRegistry from superset.connectors.sqla.models import AnnotationDatasource from superset.constants import RouteMethod +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import ( CertificateException, DatabaseNotFound, @@ -108,6 +109,7 @@ from .base import ( get_user_roles, handle_api_exception, json_error_response, + json_errors_response, json_success, SupersetModelView, validate_sqlatable, @@ -189,10 +191,22 @@ def check_datasource_perms( datasource_id, datasource_type, form_data ) except SupersetException as ex: - raise SupersetSecurityException(str(ex)) + raise SupersetSecurityException( + SupersetError( + error_type=SupersetErrorType.FAILED_FETCHING_DATASOURCE_INFO_ERROR, + level=ErrorLevel.ERROR, + message=str(ex), + ) + ) if datasource_type is None: - raise SupersetSecurityException("Could not determine datasource type") + raise SupersetSecurityException( + SupersetError( + error_type=SupersetErrorType.UNKNOWN_DATASOURCE_TYPE_ERROR, + level=ErrorLevel.ERROR, + message="Could not determine datasource type", + ) + ) viz_obj = get_viz( datasource_type=datasource_type, @@ -2187,8 +2201,9 @@ class Superset(BaseSupersetView): query.sql, query.database, query.schema ) if rejected_tables: - return json_error_response( - security_manager.get_table_access_error_msg(rejected_tables), status=403 + return json_errors_response( + [security_manager.get_table_access_error_object(rejected_tables)], + status=403, ) payload = utils.zlib_decompress(blob, decode=not results_backend_use_msgpack) @@ -2491,9 +2506,8 @@ class Superset(BaseSupersetView): if rejected_tables: query.status = QueryStatus.FAILED session.commit() - return json_error_response( - security_manager.get_table_access_error_msg(rejected_tables), - link=security_manager.get_table_access_link(rejected_tables), + return json_errors_response( + [security_manager.get_table_access_error_object(rejected_tables)], status=403, ) diff --git a/tests/core_tests.py b/tests/core_tests.py index 80ac753e9..19005579e 100644 --- a/tests/core_tests.py +++ b/tests/core_tests.py @@ -951,7 +951,8 @@ class CoreTests(SupersetTestCase): data = self.get_json_resp("/superset/explore_json/", raise_on_error=False) self.assertEqual( - data["error"], "The datasource associated with this chart no longer exists" + data["errors"][0]["message"], + "The datasource associated with this chart no longer exists", ) @mock.patch("superset.security.SupersetSecurityManager.schemas_accessible_by_user")