-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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.
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
feat: return security errors in the SIP-40 format #9796
Conversation
8ad5574
to
ef53c88
Compare
payload = {} | ||
|
||
payload["errors"] = [dataclasses.asdict(error) for error in errors] | ||
return Response( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -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.
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.
There was a problem hiding this comment.
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)
superset/security/manager.py
Outdated
""" | ||
Return the error object for the denied Superset datasource. | ||
|
||
:param tables: The denied Superset datasource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tables
-> datasource
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, fixed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we type the dict
i.e., Dict[..., ...]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, also changing the type for json_error_response
too to match
ef53c88
to
7b0c168
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@john-bodley comments addressed
@@ -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.
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)
superset/security/manager.py
Outdated
""" | ||
Return the error object for the denied Superset datasource. | ||
|
||
:param tables: The denied Superset datasource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, fixed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
superset/views/base.py
Outdated
def json_errors_response( | ||
errors: List[SupersetError], | ||
status: int = 500, | ||
payload: Optional[Dict[str, any]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any
-> Any
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
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.
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
SUMMARY
Adds functions to the security manager to return errors in the SIP-40 format. These are then returned using the new
json_errors_response
functionBEFORE/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