[GitHub] [incubator-superset] etr2460 commented on a change in pull request #9796: feat: return security errors in the SIP-40 format

[GitBox <gi...@apache.org>]

etr2460 commented on a change in pull request #9796:
URL: https://github.com/apache/incubator-superset/pull/9796#discussion_r424743206



##########
File path: superset/views/base.py
##########
@@ -142,8 +158,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(
+                [ex.error], status=ex.status, payload=ex.payload

Review comment:
       done

##########
File path: 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

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

##########
File path: superset/views/base.py
##########
@@ -96,6 +98,20 @@ def json_error_response(
     )
 
 
+def json_errors_response(
+    errors: List[SupersetError], status: int = 500, payload: Optional[dict] = None,

Review comment:
       yup, also changing the type for `json_error_response` too to match

##########
File path: superset/security/manager.py
##########
@@ -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 tables: The denied Superset datasource

Review comment:
       oops, fixed




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org