Always check for dataset access

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.

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 = (

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

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)