feat: return security errors in the SIP-40 format #9796
Conversation
etr2460
force-pushed
the
erik-ritter--error-messages-3
branch
from
8ad5574
to
ef53c88
Compare
| payload = {} | ||
|
|
||
| payload["errors"] = [dataclasses.asdict(error) for error in errors] | ||
| return Response( |
There was a problem hiding this comment.
Could json_errors_response or similar work here? Ideally we use the same mechanism for constructing JSON responses.
There was a problem hiding this comment.
huh? this is inside json_errors_response
superset/views/base.py
Outdated
| return json_error_response( | ||
| utils.error_msg_from_exception(ex), status=ex.status, link=ex.link | ||
| return json_errors_response( | ||
| [ex.error], status=ex.status, payload=ex.payload |
There was a problem hiding this comment.
Nit. Though the keyword is optional, personally when the names don't match I think there's merit in explicitly defining them, i.e., errors=[ex.error], which helps with readability.
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
superset/security/manager.py
Outdated
| """ | ||
| Return the error object for the denied Superset datasource. | ||
|
|
||
| :param tables: The denied Superset datasource |
superset/views/base.py
Outdated
| @@ -96,6 +98,20 @@ def json_error_response( | |||
| ) | |||
|
|
|||
|
|
|||
| def json_errors_response( | |||
| errors: List[SupersetError], status: int = 500, payload: Optional[dict] = None, | |||
There was a problem hiding this comment.
Could we type the dict i.e., Dict[..., ...].
There was a problem hiding this comment.
yup, also changing the type for json_error_response too to match
etr2460
force-pushed
the
erik-ritter--error-messages-3
branch
from
ef53c88
to
7b0c168
Compare
| @@ -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 | |||
There was a problem hiding this comment.
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)
superset/security/manager.py
Outdated
| """ | ||
| Return the error object for the denied Superset datasource. | ||
|
|
||
| :param tables: The denied Superset datasource |
superset/views/base.py
Outdated
| @@ -96,6 +98,20 @@ def json_error_response( | |||
| ) | |||
|
|
|||
|
|
|||
| def json_errors_response( | |||
| errors: List[SupersetError], status: int = 500, payload: Optional[dict] = None, | |||
There was a problem hiding this comment.
yup, also changing the type for json_error_response too to match
superset/views/base.py
Outdated
| return json_error_response( | ||
| utils.error_msg_from_exception(ex), status=ex.status, link=ex.link | ||
| return json_errors_response( | ||
| [ex.error], status=ex.status, payload=ex.payload |
superset/views/base.py
Outdated
| def json_errors_response( | ||
| errors: List[SupersetError], | ||
| status: int = 500, | ||
| payload: Optional[Dict[str, any]] = None, |
etr2460
force-pushed
the
erik-ritter--error-messages-3
branch
from
7b0c168
to
c8a25b7
Compare
Codecov Report
@@ Coverage Diff @@
## master #9796 +/- ##
==========================================
+ Coverage 64.35% 70.86% +6.51%
==========================================
Files 536 588 +52
Lines 29106 30493 +1387
Branches 2806 3154 +348
==========================================
+ Hits 18732 21610 +2878
+ Misses 10194 8770 -1424
+ Partials 180 113 -67
Continue to review full report at Codecov.
|
(cherry picked from commit d02f2d1)
| // 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', |
There was a problem hiding this comment.
@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
mistercrunch
added
🏷️ bot
SUMMARY
Adds functions to the security manager to return errors in the SIP-40 format. These are then returned using the new
json_errors_responsefunctionBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After screenshots representing the new API responses and unchanged error rendering:
TEST PLAN
On SQL Lab, Dashboards, and Explore:
ADDITIONAL INFORMATION
to: @john-bodley @villebro @craig-rueda