Skip to content

Commit

Permalink
fix: RBAC for export for dashboard viewers (#17527)
Browse files Browse the repository at this point in the history
* set out export

* update test

* use default dataset

* update test

* these test work

* fix test

* update

* fix

* fix test

* make the test better
  • Loading branch information
hughhhh committed Nov 26, 2021
1 parent 7429282 commit 2e29f36
Show file tree
Hide file tree
Showing 14 changed files with 36 additions and 34 deletions.
2 changes: 1 addition & 1 deletion superset-frontend/src/views/CRUD/chart/ChartCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export default function ChartCard({
const canEdit = hasPerm('can_write');
const canDelete = hasPerm('can_write');
const canExport =
hasPerm('can_read') && isFeatureEnabled(FeatureFlag.VERSIONED_EXPORT);
hasPerm('can_export') && isFeatureEnabled(FeatureFlag.VERSIONED_EXPORT);
const theme = useTheme();

const menu = (
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/views/CRUD/chart/ChartList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ function ChartList(props: ChartListProps) {
const canEdit = hasPerm('can_write');
const canDelete = hasPerm('can_write');
const canExport =
hasPerm('can_read') && isFeatureEnabled(FeatureFlag.VERSIONED_EXPORT);
hasPerm('can_export') && isFeatureEnabled(FeatureFlag.VERSIONED_EXPORT);
const initialSort = [{ id: 'changed_on_delta_humanized', desc: true }];

const handleBulkChartExport = (chartsToExport: Chart[]) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ function DashboardCard({
const history = useHistory();
const canEdit = hasPerm('can_write');
const canDelete = hasPerm('can_write');
const canExport = hasPerm('can_read');
const canExport = hasPerm('can_export');

const theme = useTheme();
const menu = (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ function DashboardList(props: DashboardListProps) {
const canCreate = hasPerm('can_write');
const canEdit = hasPerm('can_write');
const canDelete = hasPerm('can_write');
const canExport = hasPerm('can_read');
const canExport = hasPerm('can_export');

const initialSort = [{ id: 'changed_on_delta_humanized', desc: true }];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ function DatabaseList({ addDangerToast, addSuccessToast }: DatabaseListProps) {
const canEdit = hasPerm('can_write');
const canDelete = hasPerm('can_write');
const canExport =
hasPerm('can_read') && isFeatureEnabled(FeatureFlag.VERSIONED_EXPORT);
hasPerm('can_export') && isFeatureEnabled(FeatureFlag.VERSIONED_EXPORT);

const menuData: SubMenuProps = {
activeChild: 'Databases',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({
const canEdit = hasPerm('can_write');
const canDelete = hasPerm('can_write');
const canCreate = hasPerm('can_write');
const canExport = hasPerm('can_read');
const canExport = hasPerm('can_export');

const initialSort = SORT_BY;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ const mockImportFile = new File(
);

fetchMock.get(queriesInfoEndpoint, {
permissions: ['can_write', 'can_read'],
permissions: ['can_write', 'can_read', 'can_export'],
});
fetchMock.get(queriesEndpoint, {
result: mockqueries,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ function SavedQueryList({
const canEdit = hasPerm('can_write');
const canDelete = hasPerm('can_write');
const canExport =
hasPerm('can_read') && isFeatureEnabled(FeatureFlag.VERSIONED_EXPORT);
hasPerm('can_export') && isFeatureEnabled(FeatureFlag.VERSIONED_EXPORT);

const openNewQuery = () => {
window.open(`${window.location.origin}/superset/sqllab?new=true`);
Expand Down
1 change: 0 additions & 1 deletion superset/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ class RouteMethod: # pylint: disable=too-few-public-methods
"bulk_delete": "write",
"delete": "write",
"distinct": "read",
"export": "read",
"get": "read",
"get_list": "read",
"info": "read",
Expand Down
5 changes: 1 addition & 4 deletions tests/integration_tests/charts/api_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,7 @@ def test_info_security_chart(self):
rv = self.get_assert_metric(uri, "info")
data = json.loads(rv.data.decode("utf-8"))
assert rv.status_code == 200
assert set(data["permissions"]) == {
"can_read",
"can_write",
}
assert set(data["permissions"]) == {"can_read", "can_write", "can_export"}

def create_chart_import(self):
buf = BytesIO()
Expand Down
4 changes: 1 addition & 3 deletions tests/integration_tests/dashboards/api_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -378,9 +378,7 @@ def test_info_security_database(self):
rv = self.get_assert_metric(uri, "info")
data = json.loads(rv.data.decode("utf-8"))
assert rv.status_code == 200
assert "can_read" in data["permissions"]
assert "can_write" in data["permissions"]
assert len(data["permissions"]) == 2
assert set(data["permissions"]) == {"can_read", "can_write", "can_export"}

@pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
def test_get_dashboard_not_found(self):
Expand Down
8 changes: 2 additions & 6 deletions tests/integration_tests/databases/api_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -744,9 +744,7 @@ def test_info_security_database(self):
rv = self.get_assert_metric(uri, "info")
data = json.loads(rv.data.decode("utf-8"))
assert rv.status_code == 200
assert "can_read" in data["permissions"]
assert "can_write" in data["permissions"]
assert len(data["permissions"]) == 2
assert set(data["permissions"]) == {"can_read", "can_write", "can_export"}

def test_get_invalid_database_table_metadata(self):
"""
Expand Down Expand Up @@ -1143,9 +1141,7 @@ def test_export_database_not_allowed(self):
argument = [database.id]
uri = f"api/v1/database/export/?q={prison.dumps(argument)}"
rv = self.client.get(uri)
# export only requires can_read now, but gamma need to have explicit access to
# view the database
assert rv.status_code == 404
assert rv.status_code == 401

def test_export_database_non_existing(self):
"""
Expand Down
32 changes: 23 additions & 9 deletions tests/integration_tests/datasets/api_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -375,9 +375,7 @@ def test_info_security_dataset(self):
rv = self.get_assert_metric(uri, "info")
data = json.loads(rv.data.decode("utf-8"))
assert rv.status_code == 200
assert "can_read" in data["permissions"]
assert "can_write" in data["permissions"]
assert len(data["permissions"]) == 2
assert set(data["permissions"]) == {"can_read", "can_write", "can_export"}

def test_create_dataset_item(self):
"""
Expand Down Expand Up @@ -1382,18 +1380,33 @@ def test_export_dataset_not_found(self):
rv = self.get_assert_metric(uri, "export")
assert rv.status_code == 404

@pytest.mark.usefixtures("create_datasets")
def test_export_dataset_gamma(self):
"""
Dataset API: Test export dataset has gamma
"""
birth_names_dataset = self.get_birth_names_dataset()
dataset = self.get_fixture_datasets()[0]

argument = [birth_names_dataset.id]
argument = [dataset.id]
uri = f"api/v1/dataset/export/?q={prison.dumps(argument)}"

self.login(username="gamma")
rv = self.client.get(uri)
assert rv.status_code == 404
assert rv.status_code == 401

perm1 = security_manager.find_permission_view_menu("can_export", "Dataset")

perm2 = security_manager.find_permission_view_menu(
"datasource_access", dataset.perm
)

# add perissions to allow export + access to query this dataset
gamma_role = security_manager.find_role("Gamma")
security_manager.add_permission_role(gamma_role, perm1)
security_manager.add_permission_role(gamma_role, perm2)

rv = self.client.get(uri)
assert rv.status_code == 200

@patch.dict(
"superset.extensions.feature_flag_manager._feature_flags",
Expand Down Expand Up @@ -1444,19 +1457,20 @@ def test_export_dataset_bundle_not_found(self):
{"VERSIONED_EXPORT": True},
clear=True,
)
@pytest.mark.usefixtures("create_datasets")
def test_export_dataset_bundle_gamma(self):
"""
Dataset API: Test export dataset has gamma
"""
birth_names_dataset = self.get_birth_names_dataset()
dataset = self.get_fixture_datasets()[0]

argument = [birth_names_dataset.id]
argument = [dataset.id]
uri = f"api/v1/dataset/export/?q={prison.dumps(argument)}"

self.login(username="gamma")
rv = self.client.get(uri)
# gamma users by default do not have access to this dataset
assert rv.status_code == 404
assert rv.status_code == 401

@unittest.skip("Number of related objects depend on DB")
@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
Expand Down
4 changes: 1 addition & 3 deletions tests/integration_tests/queries/saved_queries/api_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -434,9 +434,7 @@ def test_info_security_saved_query(self):
rv = self.get_assert_metric(uri, "info")
data = json.loads(rv.data.decode("utf-8"))
assert rv.status_code == 200
assert "can_read" in data["permissions"]
assert "can_write" in data["permissions"]
assert len(data["permissions"]) == 2
assert set(data["permissions"]) == {"can_read", "can_write", "can_export"}

def test_related_saved_query(self):
"""
Expand Down

0 comments on commit 2e29f36

Please sign in to comment.