From d90ef369362ffd13999d71de04735ce4fa0bc32b Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Fri, 14 Jul 2023 11:45:28 -0300 Subject: [PATCH 1/3] Adding compatibility for different versions --- src/preset_cli/api/clients/superset.py | 39 ++++++++++++++++++++++++-- src/preset_cli/cli/superset/export.py | 2 +- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/preset_cli/api/clients/superset.py b/src/preset_cli/api/clients/superset.py index 9aeb7b6f..4d68272f 100644 --- a/src/preset_cli/api/clients/superset.py +++ b/src/preset_cli/api/clients/superset.py @@ -191,7 +191,7 @@ class RuleType(TypedDict): Schema for an RLS rule. """ - name: str + name: Optional[str] description: Optional[str] filter_type: str tables: List[str] @@ -690,6 +690,12 @@ def import_zip( return payload["message"] == "OK" + def get_rls(self, **kwargs: str) -> List[Any]: + """ + Return RLS rules, possibly filtered. + """ + return self.get_resources("rowlevelsecurity", **kwargs) + def export_users(self) -> Iterator[UserType]: """ Return all users. @@ -808,9 +814,9 @@ def export_roles(self) -> Iterator[RoleType]: # pylint: disable=too-many-locals "users": users, } - def export_rls(self) -> Iterator[RuleType]: + def export_rls_legacy(self) -> Iterator[RuleType]: """ - Return all RLS rules. + Return all RLS rules from legacy endpoint. """ page = 0 while True: @@ -857,6 +863,12 @@ def export_rls(self) -> Iterator[RuleType]: ("group_key", str), ("clause", str), ] + + # Before Superset 2.1.0, RLS dont have name and description + if table.find("th").text.strip() == "Filter Type": + keys.remove(("name", str)) + keys.remove(("description", str)) + yield cast( RuleType, { @@ -865,6 +877,27 @@ def export_rls(self) -> Iterator[RuleType]: }, ) + def export_rls(self) -> None: + """ + Return all RLS rules. + """ + url = self.baseurl / "api/v1/rowlevelsecurity/" + response = self.session.get(url) + if response.status_code == 200: + for rule in self.get_rls(): + keys = ["name", "description", "filter_type", "tables", "roles", "group_key", "clause"] + data = {} + for key in keys: + if key in ["tables", "roles"]: + inner_key = "table_name" if key == "tables" else "name" + data[key] = [inner_item[inner_key] for inner_item in rule.get(key, [])] + else: + data[key] = rule.get(key) + yield cast(RuleType, data) + + else: + yield from self.export_rls_legacy() + def import_role(self, role: RoleType) -> None: # pylint: disable=too-many-locals """ Import a given role. diff --git a/src/preset_cli/cli/superset/export.py b/src/preset_cli/cli/superset/export.py index aa8f8bdc..f2cb7175 100644 --- a/src/preset_cli/cli/superset/export.py +++ b/src/preset_cli/cli/superset/export.py @@ -241,7 +241,7 @@ def export_rls(ctx: click.core.Context, path: str) -> None: client = SupersetClient(url, auth) with open(path, "w", encoding="utf-8") as output: - yaml.dump(list(client.export_rls()), output) + yaml.dump(list(client.export_rls()), output, sort_keys=False) @click.command() From 3a766e16dcdc06d6dcb67ddd8e6121c478313c0f Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Sun, 16 Jul 2023 01:29:44 -0300 Subject: [PATCH 2/3] Adding new tests --- src/preset_cli/api/clients/superset.py | 7 +- tests/api/clients/superset_test.py | 203 ++++++++++++++++++++++++- 2 files changed, 201 insertions(+), 9 deletions(-) diff --git a/src/preset_cli/api/clients/superset.py b/src/preset_cli/api/clients/superset.py index 4d68272f..446c5566 100644 --- a/src/preset_cli/api/clients/superset.py +++ b/src/preset_cli/api/clients/superset.py @@ -888,9 +888,10 @@ def export_rls(self) -> None: keys = ["name", "description", "filter_type", "tables", "roles", "group_key", "clause"] data = {} for key in keys: - if key in ["tables", "roles"]: - inner_key = "table_name" if key == "tables" else "name" - data[key] = [inner_item[inner_key] for inner_item in rule.get(key, [])] + if key == "tables": + data[key] = [f"{inner_item['schema']}.{inner_item['table_name']}" for inner_item in rule.get(key, [])] + elif key == "roles": + data[key] = [inner_item["name"] for inner_item in rule.get(key, [])] else: data[key] = rule.get(key) yield cast(RuleType, data) diff --git a/tests/api/clients/superset_test.py b/tests/api/clients/superset_test.py index b143a869..add4d63e 100644 --- a/tests/api/clients/superset_test.py +++ b/tests/api/clients/superset_test.py @@ -1235,6 +1235,18 @@ def test_import_zip_error(requests_mock: Mocker) -> None: ) +def test_get_rls(mocker: MockerFixture) -> None: + """ + Test the ``get_rls`` method. + """ + auth = Auth() + client = SupersetClient("https://superset.example.org/", auth) + get_resources = mocker.patch.object(client, "get_resources") + + client.get_rls() + get_resources.assert_called_with("rowlevelsecurity") + + def test_export_users(requests_mock: Mocker) -> None: """ Test ``export_users``. @@ -1668,9 +1680,9 @@ def test_export_roles_anchor_role_id( ] -def test_export_rls(requests_mock: Mocker) -> None: +def test_export_rls_legacy(requests_mock: Mocker) -> None: """ - Test ``export_rls``. + Test ``export_rls_legacy``. """ requests_mock.get( ( @@ -1766,7 +1778,7 @@ def test_export_rls(requests_mock: Mocker) -> None: auth = Auth() client = SupersetClient("https://superset.example.org/", auth) - assert list(client.export_rls()) == [ + assert list(client.export_rls_legacy()) == [ { "name": "My Rule", "description": "This is a rule. There are many others like it, but this one is mine.", @@ -1779,9 +1791,188 @@ def test_export_rls(requests_mock: Mocker) -> None: ] -def test_export_rls_no_rules(requests_mock: Mocker) -> None: +def test_export_rls_legacy_older_superset(requests_mock: Mocker) -> None: + """ + Test ``export_rls_legacy`` with older Superset version. + """ + requests_mock.get( + ( + "https://superset.example.org/rowlevelsecurityfiltersmodelview/list/?" + "psize_RowLevelSecurityFiltersModelView=100&" + "page_RowLevelSecurityFiltersModelView=0" + ), + text=""" + + + + + + +
+ + + + + + + + + + + + + + + + + + +
Filter TypeTablesRolesClauseCreatorModified
Regular[main.test_table]client_id = 9admin admin35 minutes ago
+ + + """, + ) + requests_mock.get( + ( + "https://superset.example.org/rowlevelsecurityfiltersmodelview/list/?" + "psize_RowLevelSecurityFiltersModelView=100&" + "page_RowLevelSecurityFiltersModelView=1" + ), + text=""" + + + + + + +
+ + + + + + + + + + +
Filter TypeTablesRolesClauseCreatorModified
+ + + """, + ) + requests_mock.get( + "https://superset.example.org/rowlevelsecurityfiltersmodelview/show/1", + text=""" + + + + + + + + + + + + +
Filter TypeRegular
Tables[main.test_table]
Roles[Gamma]
Group Keydepartment
Clauseclient_id = 9
+ + + """, + ) + + auth = Auth() + client = SupersetClient("https://superset.example.org/", auth) + assert list(client.export_rls_legacy()) == [ + { + "filter_type": "Regular", + "tables": ["main.test_table"], + "roles": ["Gamma"], + "group_key": "department", + "clause": "client_id = 9", + }, + ] + + +def test_export_rls_legacy_route(requests_mock: Mocker, mocker: MockerFixture) -> None: + """ + Test ``export_rls`` going through the legacy route + """ + requests_mock.get( + "https://superset.example.org/api/v1/rowlevelsecurity/", + status_code = 404 + ) + + auth = Auth() + client = SupersetClient("https://superset.example.org/", auth) + + export_rls_legacy = mocker.patch.object(client, "export_rls_legacy") + list(client.export_rls()) + export_rls_legacy.assert_called_once() + + +def test_export_rls(requests_mock: Mocker, mocker: MockerFixture) -> None: + """ + Test ``export_rls`` + """ + requests_mock.get( + "https://superset.example.org/api/v1/rowlevelsecurity/", + status_code = 200 + ) + + auth = Auth() + client = SupersetClient("https://superset.example.org/", auth) + + get_rls = mocker.patch.object(client, "get_rls") + get_rls.return_value = [ + { + "changed_on_delta_humanized": "2 days ago", + "clause": "client_id = 9", + "description": "This is a rule. There are many others like it, but this one is mine.", + "filter_type": "Regular", + "group_key": "department", + "id": 9, + "name": "My Rule", + "roles": [ + { + "id": 1, + "name": "Admin" + }, + { + "id": 2, + "name": "Gamma" + }], + "tables": [ + { + "id": 18, + "schema": "main", + "table_name": "test_table" + }, + { + "id": 20, + "schema": "main", + "table_name": "second_test" + }] + } + ] + + assert list(client.export_rls()) == [ + { + "name": "My Rule", + "description": "This is a rule. There are many others like it, but this one is mine.", + "filter_type": "Regular", + "tables": ["main.test_table", "main.second_test"], + "roles": ["Admin", "Gamma"], + "group_key": "department", + "clause": "client_id = 9", + }, + ] + + +def test_export_rls_legacy_no_rules(requests_mock: Mocker) -> None: """ - Test ``export_rls``. + Test ``export_rls_legacy`` with no rows returned. """ requests_mock.get( ( @@ -1805,7 +1996,7 @@ def test_export_rls_no_rules(requests_mock: Mocker) -> None: auth = Auth() client = SupersetClient("https://superset.example.org/", auth) - assert list(client.export_rls()) == [] + assert list(client.export_rls_legacy()) == [] def test_export_ownership(mocker: MockerFixture) -> None: From 404737326d8a8e79850f71b4e3b615036141dd02 Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Sun, 16 Jul 2023 01:36:32 -0300 Subject: [PATCH 3/3] precommit fixes --- src/preset_cli/api/clients/superset.py | 23 +++++++++++++++----- tests/api/clients/superset_test.py | 29 +++++++------------------- 2 files changed, 25 insertions(+), 27 deletions(-) diff --git a/src/preset_cli/api/clients/superset.py b/src/preset_cli/api/clients/superset.py index 446c5566..d345e92d 100644 --- a/src/preset_cli/api/clients/superset.py +++ b/src/preset_cli/api/clients/superset.py @@ -863,7 +863,7 @@ def export_rls_legacy(self) -> Iterator[RuleType]: ("group_key", str), ("clause", str), ] - + # Before Superset 2.1.0, RLS dont have name and description if table.find("th").text.strip() == "Filter Type": keys.remove(("name", str)) @@ -877,7 +877,7 @@ def export_rls_legacy(self) -> Iterator[RuleType]: }, ) - def export_rls(self) -> None: + def export_rls(self) -> Iterator[RuleType]: """ Return all RLS rules. """ @@ -885,13 +885,26 @@ def export_rls(self) -> None: response = self.session.get(url) if response.status_code == 200: for rule in self.get_rls(): - keys = ["name", "description", "filter_type", "tables", "roles", "group_key", "clause"] + keys = [ + "name", + "description", + "filter_type", + "tables", + "roles", + "group_key", + "clause", + ] data = {} for key in keys: if key == "tables": - data[key] = [f"{inner_item['schema']}.{inner_item['table_name']}" for inner_item in rule.get(key, [])] + data[key] = [ + f"{inner_item['schema']}.{inner_item['table_name']}" + for inner_item in rule.get(key, []) + ] elif key == "roles": - data[key] = [inner_item["name"] for inner_item in rule.get(key, [])] + data[key] = [ + inner_item["name"] for inner_item in rule.get(key, []) + ] else: data[key] = rule.get(key) yield cast(RuleType, data) diff --git a/tests/api/clients/superset_test.py b/tests/api/clients/superset_test.py index add4d63e..c47733cc 100644 --- a/tests/api/clients/superset_test.py +++ b/tests/api/clients/superset_test.py @@ -1901,7 +1901,7 @@ def test_export_rls_legacy_route(requests_mock: Mocker, mocker: MockerFixture) - """ requests_mock.get( "https://superset.example.org/api/v1/rowlevelsecurity/", - status_code = 404 + status_code=404, ) auth = Auth() @@ -1918,7 +1918,7 @@ def test_export_rls(requests_mock: Mocker, mocker: MockerFixture) -> None: """ requests_mock.get( "https://superset.example.org/api/v1/rowlevelsecurity/", - status_code = 200 + status_code=200, ) auth = Auth() @@ -1934,27 +1934,12 @@ def test_export_rls(requests_mock: Mocker, mocker: MockerFixture) -> None: "group_key": "department", "id": 9, "name": "My Rule", - "roles": [ - { - "id": 1, - "name": "Admin" - }, - { - "id": 2, - "name": "Gamma" - }], + "roles": [{"id": 1, "name": "Admin"}, {"id": 2, "name": "Gamma"}], "tables": [ - { - "id": 18, - "schema": "main", - "table_name": "test_table" - }, - { - "id": 20, - "schema": "main", - "table_name": "second_test" - }] - } + {"id": 18, "schema": "main", "table_name": "test_table"}, + {"id": 20, "schema": "main", "table_name": "second_test"}, + ], + }, ] assert list(client.export_rls()) == [