You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by er...@apache.org on 2020/05/14 00:10:59 UTC
[incubator-superset] branch master updated: feat: return security
errors in the SIP-40 format (#9796)
This is an automated email from the ASF dual-hosted git repository.
erikrit pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git
The following commit(s) were added to refs/heads/master by this push:
new d02f2d1 feat: return security errors in the SIP-40 format (#9796)
d02f2d1 is described below
commit d02f2d1fa7e46c61895d39c2adc40297c36e4cbe
Author: Erik Ritter <er...@airbnb.com>
AuthorDate: Wed May 13 17:10:37 2020 -0700
feat: return security errors in the SIP-40 format (#9796)
---
.../javascripts/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 afadb75..1931480 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 1d259b2..2d4cf45 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> = T[keyof T];
diff --git a/superset-frontend/src/utils/getClientErrorObject.ts b/superset-frontend/src/utils/getClientErrorObject.ts
index 507f9fa..421aa01 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<SupersetClientResponse>;
@@ -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 aaa7aa2..54eb0ed 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 33841cd..59ea042 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 c48c231..021ca9f 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 1faa90f..def1ffa 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 6cf82a0..e688d42 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 80ac753..1900557 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")