diff --git a/superset/charts/form_data/api.py b/superset/charts/form_data/api.py index bf5896584ec04..bc908b25d6b81 100644 --- a/superset/charts/form_data/api.py +++ b/superset/charts/form_data/api.py @@ -69,7 +69,7 @@ def post(self, pk: int) -> Response: schema: type: integer name: dataset_id - required: false + required: true requestBody: required: true content: @@ -124,7 +124,7 @@ def put(self, pk: int, key: str) -> Response: schema: type: integer name: dataset_id - required: false + required: true requestBody: required: true content: @@ -181,7 +181,7 @@ def get(self, pk: int, key: str) -> Response: schema: type: integer name: dataset_id - required: false + required: true responses: 200: description: Returns the stored value. @@ -233,7 +233,7 @@ def delete(self, pk: int, key: str) -> Response: schema: type: integer name: dataset_id - required: false + required: true responses: 200: description: Deleted the stored value. diff --git a/superset/charts/form_data/utils.py b/superset/charts/form_data/utils.py index ade4f90fea6fc..c1f98a2388407 100644 --- a/superset/charts/form_data/utils.py +++ b/superset/charts/form_data/utils.py @@ -39,19 +39,24 @@ def get_dataset_id(cmd_params: CommandParameters) -> Optional[str]: return None +def check_dataset_access(cmd_params: CommandParameters) -> Optional[bool]: + 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() + + def check_access(cmd_params: CommandParameters) -> Optional[bool]: resource_id = cmd_params.resource_id actor = cmd_params.actor + check_dataset_access(cmd_params) 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() + return True chart = ChartDAO.find_by_id(resource_id) if chart: can_access_chart = ( diff --git a/tests/integration_tests/charts/form_data/api_tests.py b/tests/integration_tests/charts/form_data/api_tests.py index c0baa6d8cb3ed..f411c1a6edc99 100644 --- a/tests/integration_tests/charts/form_data/api_tests.py +++ b/tests/integration_tests/charts/form_data/api_tests.py @@ -82,92 +82,112 @@ def cache(chart_id, admin_id, dataset_id): cache_manager.chart_form_data_cache.set(cache_key(dataset_id, key), entry) -def test_post(client, chart_id: int): +def test_post(client, chart_id: int, dataset_id: int): login(client, "admin") payload = { "value": value, } - resp = client.post(f"api/v1/chart/{chart_id}/form_data", json=payload) + resp = client.post( + f"api/v1/chart/{chart_id}/form_data?dataset_id={dataset_id}", json=payload + ) assert resp.status_code == 201 -def test_post_bad_request(client, chart_id: int): +def test_post_bad_request(client, chart_id: int, dataset_id: int): login(client, "admin") payload = { "value": 1234, } - resp = client.put(f"api/v1/chart/{chart_id}/form_data/{key}", json=payload) + resp = client.post( + f"api/v1/chart/{chart_id}/form_data?dataset_id={dataset_id}", json=payload + ) assert resp.status_code == 400 -def test_post_access_denied(client, chart_id: int): +def test_post_access_denied(client, chart_id: int, dataset_id: int): login(client, "gamma") payload = { "value": value, } - resp = client.post(f"api/v1/chart/{chart_id}/form_data", json=payload) + resp = client.post( + f"api/v1/chart/{chart_id}/form_data?dataset_id={dataset_id}", json=payload + ) assert resp.status_code == 404 -def test_put(client, chart_id: int): +def test_put(client, chart_id: int, dataset_id: int): login(client, "admin") payload = { "value": "new value", } - resp = client.put(f"api/v1/chart/{chart_id}/form_data/{key}", json=payload) + resp = client.put( + f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}", json=payload + ) assert resp.status_code == 200 -def test_put_bad_request(client, chart_id: int): +def test_put_bad_request(client, chart_id: int, dataset_id: int): login(client, "admin") payload = { "value": 1234, } - resp = client.put(f"api/v1/chart/{chart_id}/form_data/{key}", json=payload) + resp = client.put( + f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}", json=payload + ) assert resp.status_code == 400 -def test_put_access_denied(client, chart_id: int): +def test_put_access_denied(client, chart_id: int, dataset_id: int): login(client, "gamma") payload = { "value": "new value", } - resp = client.put(f"api/v1/chart/{chart_id}/form_data/{key}", json=payload) + resp = client.put( + f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}", json=payload + ) assert resp.status_code == 404 -def test_put_not_owner(client, chart_id: int): +def test_put_not_owner(client, chart_id: int, dataset_id: int): login(client, "gamma") payload = { "value": "new value", } - resp = client.put(f"api/v1/chart/{chart_id}/form_data/{key}", json=payload) + resp = client.put( + f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}", json=payload + ) assert resp.status_code == 404 -def test_get_key_not_found(client, chart_id: int): +def test_get_key_not_found(client, chart_id: int, dataset_id: int): login(client, "admin") - resp = client.get(f"api/v1/chart/{chart_id}/form_data/unknown-key") + resp = client.get( + f"api/v1/chart/{chart_id}/form_data/unknown-key?dataset_id={dataset_id}" + ) assert resp.status_code == 404 -def test_get_chart_not_found(client): +def test_get_chart_not_found(client, dataset_id: int): login(client, "admin") - resp = client.get(f"api/v1/chart/-1/form_data/{key}/") + resp = client.get(f"api/v1/chart/-1/form_data/{key}?dataset_id={dataset_id}") assert resp.status_code == 404 -def test_get(client, chart_id: int): +def test_get(client, chart_id: int, dataset_id: int): login(client, "admin") - resp = client.get(f"api/v1/chart/{chart_id}/form_data/{key}") + resp = client.get( + f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}" + ) assert resp.status_code == 200 data = json.loads(resp.data.decode("utf-8")) assert value == data.get("value") -def test_get_access_denied(client, chart_id): +def test_get_access_denied(client, chart_id: int, dataset_id: int): login(client, "gamma") - resp = client.get(f"api/v1/chart/{chart_id}/form_data/{key}") + resp = client.get( + f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}" + ) assert resp.status_code == 404 @@ -177,7 +197,7 @@ def test_get_no_dataset(client): assert resp.status_code == 404 -def test_get_dataset(client, dataset_id): +def test_get_dataset(client, dataset_id: int): login(client, "admin") resp = client.get(f"api/v1/chart/0/form_data/{key}?dataset_id={dataset_id}") assert resp.status_code == 200 @@ -197,23 +217,29 @@ def test_get_dataset_access_denied(mock_can_access_datasource, client, dataset_i assert resp.status_code == 403 -def test_delete(client, chart_id: int): +def test_delete(client, chart_id: int, dataset_id: int): login(client, "admin") - resp = client.delete(f"api/v1/chart/{chart_id}/form_data/{key}") + resp = client.delete( + f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}" + ) assert resp.status_code == 200 -def test_delete_access_denied(client, chart_id: int): +def test_delete_access_denied(client, chart_id: int, dataset_id: int): login(client, "gamma") - resp = client.delete(f"api/v1/chart/{chart_id}/form_data/{key}") + resp = client.delete( + f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}" + ) assert resp.status_code == 404 -def test_delete_not_owner(client, chart_id: int, admin_id: int): +def test_delete_not_owner(client, chart_id: int, dataset_id: int, admin_id: int): another_key = "another_key" another_owner = admin_id + 1 entry: Entry = {"owner": another_owner, "value": value} cache_manager.chart_form_data_cache.set(cache_key(chart_id, another_key), entry) login(client, "admin") - resp = client.delete(f"api/v1/chart/{chart_id}/form_data/{another_key}") + resp = client.delete( + f"api/v1/chart/{chart_id}/form_data/{another_key}?dataset_id={dataset_id}" + ) assert resp.status_code == 403 diff --git a/tests/unit_tests/charts/form_data/utils_test.py b/tests/unit_tests/charts/form_data/utils_test.py index 19ebb63626070..1180b10d36ed9 100644 --- a/tests/unit_tests/charts/form_data/utils_test.py +++ b/tests/unit_tests/charts/form_data/utils_test.py @@ -93,54 +93,94 @@ def test_saved_chart_unknown_chart_id( mocker: MockFixture, app_context: AppContext ) -> None: from superset.charts.form_data.utils import check_access + from superset.connectors.sqla.models import SqlaTable with raises(ChartNotFoundError): + mocker.patch(dataset_find_by_id, return_value=SqlaTable()) + mocker.patch(can_access_datasource, return_value=True) mocker.patch(chart_find_by_id, return_value=None) - cmd_params = CommandParameters(resource_id=1, actor=User(), query_params={}) + cmd_params = CommandParameters( + resource_id=1, actor=User(), query_params={"dataset_id": "1"} + ) check_access(cmd_params) +def test_saved_chart_unauthorized_dataset( + mocker: MockFixture, app_context: AppContext +) -> None: + from superset.charts.form_data import utils + from superset.connectors.sqla.models import SqlaTable + + with raises(DatasetAccessDeniedError): + mocker.patch(dataset_find_by_id, return_value=SqlaTable()) + mocker.patch(can_access_datasource, return_value=False) + cmd_params = CommandParameters( + resource_id=1, actor=User(), query_params={"dataset_id": "1"} + ) + utils.check_access(cmd_params) + + def test_saved_chart_is_admin(mocker: MockFixture, app_context: AppContext) -> None: from superset.charts.form_data.utils import check_access + from superset.connectors.sqla.models import SqlaTable from superset.models.slice import Slice + mocker.patch(dataset_find_by_id, return_value=SqlaTable()) + mocker.patch(can_access_datasource, return_value=True) mocker.patch(is_user_admin, return_value=True) mocker.patch(chart_find_by_id, return_value=Slice()) - cmd_params = CommandParameters(resource_id=1, actor=User(), query_params={}) + cmd_params = CommandParameters( + resource_id=1, actor=User(), query_params={"dataset_id": "1"} + ) assert check_access(cmd_params) == True def test_saved_chart_is_owner(mocker: MockFixture, app_context: AppContext) -> None: from superset.charts.form_data.utils import check_access + from superset.connectors.sqla.models import SqlaTable from superset.models.slice import Slice + mocker.patch(dataset_find_by_id, return_value=SqlaTable()) + mocker.patch(can_access_datasource, return_value=True) mocker.patch(is_user_admin, return_value=False) mocker.patch(is_owner, return_value=True) mocker.patch(chart_find_by_id, return_value=Slice()) - cmd_params = CommandParameters(resource_id=1, actor=User(), query_params={}) + cmd_params = CommandParameters( + resource_id=1, actor=User(), query_params={"dataset_id": "1"} + ) assert check_access(cmd_params) == True def test_saved_chart_has_access(mocker: MockFixture, app_context: AppContext) -> None: from superset.charts.form_data.utils import check_access + from superset.connectors.sqla.models import SqlaTable from superset.models.slice import Slice + mocker.patch(dataset_find_by_id, return_value=SqlaTable()) + mocker.patch(can_access_datasource, return_value=True) mocker.patch(is_user_admin, return_value=False) mocker.patch(is_owner, return_value=False) mocker.patch(can_access, return_value=True) mocker.patch(chart_find_by_id, return_value=Slice()) - cmd_params = CommandParameters(resource_id=1, actor=User(), query_params={}) + cmd_params = CommandParameters( + resource_id=1, actor=User(), query_params={"dataset_id": "1"} + ) assert check_access(cmd_params) == True def test_saved_chart_no_access(mocker: MockFixture, app_context: AppContext) -> None: from superset.charts.form_data.utils import check_access + from superset.connectors.sqla.models import SqlaTable from superset.models.slice import Slice with raises(ChartAccessDeniedError): + mocker.patch(dataset_find_by_id, return_value=SqlaTable()) + mocker.patch(can_access_datasource, return_value=True) mocker.patch(is_user_admin, return_value=False) mocker.patch(is_owner, return_value=False) mocker.patch(can_access, return_value=False) mocker.patch(chart_find_by_id, return_value=Slice()) - cmd_params = CommandParameters(resource_id=1, actor=User(), query_params={}) + cmd_params = CommandParameters( + resource_id=1, actor=User(), query_params={"dataset_id": "1"} + ) check_access(cmd_params)