feat: return security errors in the SIP-40 format (#9796)

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);
       },
     );
   });

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];

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) {

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):

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:
+    def __init__(
-        super(SupersetSecurityException, self).__init__(msg)
+        self, error: SupersetError, payload: Optional[Dict[str, Any]] = None
-        self.link = link
+    ) -> None:
+        super(SupersetSecurityException, self).__init__(error.message)
+        self.error = error
+        self.payload = payload
 
 
 class NoDataException(SupersetException):

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_error_object(datasource),
-                self.get_datasource_access_link(datasource),
             )
 
     def assert_query_context_permission(self, query_context: "QueryContext") -> None:

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(
+            return json_errors_response(
-                utils.error_msg_from_exception(ex), status=ex.status, link=ex.link
+                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:

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(
+            return json_errors_response(
-                security_manager.get_table_access_error_msg(rejected_tables), status=403
+                [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(
+            return json_errors_response(
-                security_manager.get_table_access_error_msg(rejected_tables),
+                [security_manager.get_table_access_error_object(rejected_tables)],
-                link=security_manager.get_table_access_link(rejected_tables),
                 status=403,
             )
 

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")