michael-s-molina commented on a change in pull request #17882: URL: https://github.com/apache/superset/pull/17882#discussion_r787716653
########## File path: superset/charts/form_data/utils.py ########## @@ -0,0 +1,65 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from typing import Optional + +from superset import security_manager +from superset.charts.commands.exceptions import ( + ChartAccessDeniedError, + ChartNotFoundError, +) +from superset.charts.dao import ChartDAO +from superset.datasets.commands.exceptions import ( + DatasetAccessDeniedError, + DatasetNotFoundError, +) +from superset.datasets.dao import DatasetDAO +from superset.key_value.commands.parameters import CommandParameters +from superset.views.base import is_user_admin +from superset.views.utils import is_owner + + +def get_dataset_id(cmd_params: CommandParameters) -> Optional[str]: + query_params = cmd_params.query_params + if query_params: + return query_params.get("dataset_id") + return None + + +def check_access(cmd_params: CommandParameters) -> Optional[bool]: + resource_id = cmd_params.resource_id + actor = cmd_params.actor + if resource_id == 0: + dataset_id = get_dataset_id(cmd_params) + if dataset_id: + dataset = DatasetDAO.find_by_id(int(dataset_id)) + if dataset: + can_access_datasource = security_manager.can_access_datasource(dataset) + if can_access_datasource: + return True + raise DatasetAccessDeniedError() + raise DatasetNotFoundError() + chart = ChartDAO.find_by_id(resource_id) + if chart: + can_access_chart = ( + is_user_admin() + or is_owner(chart, actor) + or security_manager.can_access("can_write", "Chart") + ) + if can_access_chart: Review comment: > 1. I understand why numerical IDs can be problematic in some cases but I'm not sure why would we treat temporary chart states differently than we do for a chart or dashboard itself? If we should not, then in the long run, do we plan to change how chart/dashboard/dataset id are generated as well? I would advocate for changing them in the long run. Numerical IDs can facilitate [Insecure Direct Object References](https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/05-Authorization_Testing/04-Testing_for_Insecure_Direct_Object_References) and [Cross Site Scripting (XSS)](https://owasp.org/www-community/attacks/xss/) attacks and also open the door for [Business Intelligence Leaks](https://medium.com/lightrail/prevent-business-intelligence-leaks-by-using-uuids-instead-of-database-ids-on-urls-and-in-apis-17f15669fd2e). > 2. `/api/v1/chart/0/form_data?dataset_id=<id>` works but the `id == 0` check feels hacky. Why not just use a new endpoint? We considered using different endpoints for the saved/unsaved states, but it felt like overkill for a very specific use case because a new endpoint means new docs, tests, API, security rules, etc. > You may not even need `chart_id` or `dataset_id` in the API URI anyway since you can save that information in form data (or a wrapper payload for the whole temporary Explore state). That's true and it was one of the options on the table but we preferred the more RESTy approach with the resource identifier and the query parameter. We also didn't want to force any kind of schema to the value. > I.e., when creating the new chart, users see the URL `/explore/?dataset_id=xxx`, the client makes a POST request to `/api/v1/chart/0/form_data`, API returns a form data key, then client URL is updated to `/explore/?key=abceded`, where the key would contain all the information the client needs---starting chart id or dataset id + updated full form data. That's pretty much the current flow developed in the follow-up PR that I'm working on related to the client-side part 😉. Since the client is the owner of the value, they can control what type of information is stored, so I like your idea of also saving the chart ID and dataset ID alongside the value to keep only the key in the URL. I'll follow your suggestion in the client-side PR. The only thing is that the server shouldn’t account for this information and should continue to work with the resource identifier and query parameter. > I'm not sure we want to limit Explore state access only to those who can create charts. If users can read a chart, then they should be able to explore a chart as well. Makes sense. Will change it. > This check is also inconsistent in terms of whether dataset access is used to prevent access to an Explore state. Here we're assuming that if a user has access to a chart, it means that they have access to the dataset. That's why we only check the dataset when the chart is not saved. > The Explore page also checks dataset access so the resource_id == 0 check seems redundant. That's true but we also need to consider a call to the API directly. -- 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. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
