Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
May 14, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion superset-frontend/spec/javascripts/utils/getClientErrorObject_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
},
Expand All @@ -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);
},
);
});
Expand Down
11 changes: 11 additions & 0 deletions superset-frontend/src/components/ErrorMessage/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etr2460 What do you think of removing _ERROR suffix for the more specific errors and make it read like natural English? E.g.

SupersetError.UNKNOWN_DATASOURCE_TYPE
SupersetError.FAILED_FETCHING_DATASORUCE_INFO
SupersetError.TABLE_ACCESS_DENIED
SupersetError.DATASOURCE_ACCESS_DENITED
SupersetError.MISSING_OWNERSHIP

} as const;

type ValueOf<T> = T[keyof T];
Expand Down
4 changes: 3 additions & 1 deletion superset-frontend/src/utils/getClientErrorObject.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>;

Expand All @@ -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) {
Expand Down
12 changes: 11 additions & 1 deletion superset/errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than adding the blanket disable could we put this on the specific line? Note the long term goal would be to remedy all pylint issues.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is for the Enum names, and I want them to match the string they're equivalent to, I think this might be ok to disable for the whole file. Alternatively, we could change the max variable name length pylint config (not sure why we have it in the first place, I've rarely seen names that are too long, far more often names that are too short)

from enum import Enum
from typing import Any, Dict, Optional

Expand All @@ -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):
Expand Down
13 changes: 9 additions & 4 deletions superset/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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):
Expand Down
40 changes: 38 additions & 2 deletions superset/security/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
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

Expand Down Expand Up @@ -291,6 +292,25 @@ def get_datasource_access_link(self, datasource: "BaseDatasource") -> Optional[s

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.
Expand All @@ -303,6 +323,23 @@ def get_table_access_error_msg(self, tables: Set["Table"]) -> str:
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.
Expand Down Expand Up @@ -828,8 +865,7 @@ def assert_datasource_permission(self, datasource: "BaseDatasource") -> None:

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:
Expand Down
30 changes: 26 additions & 4 deletions superset/views/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
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
Expand All @@ -44,6 +45,7 @@
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
Expand Down Expand Up @@ -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:
Expand All @@ -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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could json_errors_response or similar work here? Ideally we use the same mechanism for constructing JSON responses.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh? this is inside json_errors_response

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦

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

Expand Down Expand Up @@ -142,8 +160,8 @@ def wraps(self, *args, **kwargs):
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)
Expand Down Expand Up @@ -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:
Expand Down
28 changes: 21 additions & 7 deletions superset/views/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
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,
Expand Down Expand Up @@ -108,6 +109,7 @@
get_user_roles,
handle_api_exception,
json_error_response,
json_errors_response,
json_success,
SupersetModelView,
validate_sqlatable,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -2187,8 +2201,9 @@ def results_exec(self, key: str):
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)
Expand Down Expand Up @@ -2491,9 +2506,8 @@ def sql_json_exec(
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,
)

Expand Down
3 changes: 2 additions & 1 deletion tests/core_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -951,7 +951,8 @@ def test_slice_payload_no_datasource(self):
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")
Expand Down