From 942047cc0fef9c09ead19c3222aeb90bded54972 Mon Sep 17 00:00:00 2001 From: Charl Smit Date: Fri, 15 Nov 2024 12:53:58 +0200 Subject: [PATCH 1/8] Remove redundant constants --- hq_superset/const.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/hq_superset/const.py b/hq_superset/const.py index 54fd77b..9c380ac 100644 --- a/hq_superset/const.py +++ b/hq_superset/const.py @@ -28,9 +28,6 @@ ] CAN_WRITE_PERMISSION = "can_write" -CAN_EDIT_PERMISSION = "can_edit" -CAN_ADD_PERMISSION = "can_add" -CAN_DELETE_PERMISSIONS = "can_delete" READ_ONLY_MENU_PERMISSIONS = { From 61044858d3720322b05e6f4f78b293c6e7eae4f5 Mon Sep 17 00:00:00 2001 From: Charl Smit Date: Fri, 15 Nov 2024 12:54:38 +0200 Subject: [PATCH 2/8] Add trailing slash --- hq_superset/hq_url.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hq_superset/hq_url.py b/hq_superset/hq_url.py index 392cd82..fa806ae 100644 --- a/hq_superset/hq_url.py +++ b/hq_superset/hq_url.py @@ -33,4 +33,4 @@ def datasource_unsubscribe(domain, datasource_id): def user_domain_roles(domain): - return f"a/{domain}/api/analytics-roles/v1" + return f"a/{domain}/api/analytics-roles/v1/" From de028387aaafda036f9d6e2f78d6bafa41f3a375 Mon Sep 17 00:00:00 2001 From: Charl Smit Date: Fri, 15 Nov 2024 12:56:06 +0200 Subject: [PATCH 3/8] Update comment --- hq_superset/utils.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/hq_superset/utils.py b/hq_superset/utils.py index af79803..b92e769 100644 --- a/hq_superset/utils.py +++ b/hq_superset/utils.py @@ -135,10 +135,7 @@ def sync_domain_role(self, domain): The user gets assigned at least 3 roles in order to function on any domain: 1. hq_user_role: gives access to superset platform 2. domain_schema_role: restricts user access to specific domain schema - 3. domain_user_role: restricts access for particular user on domain in accordance with how the permissions - are defined on CommCare HQ. - - Any additional roles defined on CommCare HQ will also be assigned to the user. + 3. Either the Gamma role for "edit" users or the READ_ONLY_ROLE_NAME for "view only" user """ hq_user_role = self._ensure_hq_user_role() domain_schema_role = self._create_domain_role(domain) From 082902bd669d23f7af957650e1badd94ebfbc5ca Mon Sep 17 00:00:00 2001 From: Charl Smit Date: Fri, 15 Nov 2024 13:27:22 +0200 Subject: [PATCH 4/8] Little refactor --- hq_superset/const.py | 3 --- hq_superset/utils.py | 24 ++++++------------------ 2 files changed, 6 insertions(+), 21 deletions(-) diff --git a/hq_superset/const.py b/hq_superset/const.py index 9c380ac..4ad8365 100644 --- a/hq_superset/const.py +++ b/hq_superset/const.py @@ -27,9 +27,6 @@ CAN_READ_PERMISSION, ] -CAN_WRITE_PERMISSION = "can_write" - - READ_ONLY_MENU_PERMISSIONS = { "Chart": READ_ONLY_PERMISSIONS, "Dataset": READ_ONLY_PERMISSIONS, diff --git a/hq_superset/utils.py b/hq_superset/utils.py index b92e769..64789ba 100644 --- a/hq_superset/utils.py +++ b/hq_superset/utils.py @@ -18,8 +18,6 @@ from superset.utils.database import get_or_create_db from hq_superset.const import ( - CAN_READ_PERMISSION, - CAN_WRITE_PERMISSION, DOMAIN_PREFIX, GAMMA_ROLE_NAME, HQ_DATABASE_NAME, @@ -199,19 +197,19 @@ def _ensure_domain_role_created(self, domain): return self.sm.add_role(get_role_name_for_domain(domain)) def _get_additional_user_roles(self, domain): - domain_permissions, roles_names = self._get_domain_access(domain) - if self._user_has_no_access(domain_permissions): + can_read, can_write, platform_roles_names = self._get_domain_access(domain) + if not (can_read or can_write): return [] - if domain_permissions[CAN_WRITE_PERMISSION]: + if can_write: user_role = GAMMA_ROLE_NAME else: self._ensure_read_only_role_exists() user_role = READ_ONLY_ROLE_NAME - roles_names.append(user_role) + platform_roles_names.append(user_role) - return self._get_platform_roles(roles_names) + return self._get_platform_roles(platform_roles_names) @staticmethod def _get_domain_access(domain): @@ -228,17 +226,7 @@ def _get_domain_access(domain): hq_permissions = response_data['permissions'] roles = response_data['roles'] or [] - # Map between HQ and CCA - permissions = { - CAN_WRITE_PERMISSION: hq_permissions["can_edit"], - CAN_READ_PERMISSION: hq_permissions["can_view"], - } - return permissions, roles - - @staticmethod - def _user_has_no_access(permissions: dict): - user_has_access = any([permissions[p] for p in permissions]) - return not user_has_access + return hq_permissions["can_view"], hq_permissions["can_edit"], roles def _get_platform_roles(self, roles_names): platform_roles = [] From 68715531eb4ac64cc9c4381bdfd33ed7d7a6f31a Mon Sep 17 00:00:00 2001 From: Charl Smit Date: Fri, 15 Nov 2024 13:27:31 +0200 Subject: [PATCH 5/8] Update tests --- hq_superset/tests/test_utils.py | 5 +---- hq_superset/tests/test_views.py | 4 ++-- hq_superset/tests/utils.py | 4 ++-- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/hq_superset/tests/test_utils.py b/hq_superset/tests/test_utils.py index b07053f..2dccf79 100644 --- a/hq_superset/tests/test_utils.py +++ b/hq_superset/tests/test_utils.py @@ -122,7 +122,4 @@ def _ensure_platform_roles_exist(self, sm): @staticmethod def _to_permissions_response(can_write, can_read, roles): - return { - "can_write": can_write, - "can_read": can_read, - }, roles + return can_read, can_write, roles diff --git a/hq_superset/tests/test_views.py b/hq_superset/tests/test_views.py index 92765b3..cab7cf4 100644 --- a/hq_superset/tests/test_views.py +++ b/hq_superset/tests/test_views.py @@ -85,7 +85,7 @@ def test_redirects_to_domain_select_after_login(self): ) self.logout(client) - @patch.object(DomainSyncUtil, "_get_domain_access", return_value=({"can_write": True, "can_read": True}, [])) + @patch.object(DomainSyncUtil, "_get_domain_access", return_value=(True, True, [])) def test_domain_select_works(self, *args): client = self.app.test_client() self.login(client) @@ -135,7 +135,7 @@ def _do_assert(datasources): _do_assert(self.oauth_mock.test2_datasources) self.logout(client) - @patch.object(DomainSyncUtil, "_get_domain_access", return_value=({"can_write": True, "can_read": True}, [])) + @patch.object(DomainSyncUtil, "_get_domain_access", return_value=(True, True, [])) def test_datasource_upload(self, *args): client = self.app.test_client() self.login(client) diff --git a/hq_superset/tests/utils.py b/hq_superset/tests/utils.py index d406e6f..1ac9316 100644 --- a/hq_superset/tests/utils.py +++ b/hq_superset/tests/utils.py @@ -65,8 +65,8 @@ def get(self, url, token): 'a/test1/configurable_reports/data_sources/export/test1_ucr1/?format=csv': MockResponse( TEST_UCR_CSV_V1, 200 ), - 'a/test1/api/analytics-roles/v1': MockResponse(self.user_domain_roles, 200), - 'a/test2/api/analytics-roles/v1': MockResponse(self.user_domain_roles, 200), + 'a/test1/api/analytics-roles/v1/': MockResponse(self.user_domain_roles, 200), + 'a/test2/api/analytics-roles/v1/': MockResponse(self.user_domain_roles, 200), }[url] From ca7285d967dfb9790ad639442a70ad58b835a7be Mon Sep 17 00:00:00 2001 From: Charl Smit Date: Mon, 18 Nov 2024 09:37:19 +0200 Subject: [PATCH 6/8] Fix domain_access response and add tests --- hq_superset/tests/test_utils.py | 33 +++++++++++++++++++++++++++++++++ hq_superset/utils.py | 2 +- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/hq_superset/tests/test_utils.py b/hq_superset/tests/test_utils.py index 2dccf79..e7664c9 100644 --- a/hq_superset/tests/test_utils.py +++ b/hq_superset/tests/test_utils.py @@ -7,6 +7,7 @@ from hq_superset.tests.base_test import LoginUserTestMixin, SupersetTestCase from hq_superset.tests.const import TEST_DATASOURCE from hq_superset.utils import DomainSyncUtil, get_column_dtypes +from hq_superset.hq_requests import HQRequest def test_get_column_dtypes(): @@ -30,9 +31,41 @@ def test_doctests(): assert results.failed == 0 +class DomainAccessMockResponse: + status_code: int = 200 + + def __init__(self, status_code=None): + if status_code: + self.status_code = status_code + + def json(self): + return { + 'permissions': {'can_edit': True, 'can_view': True}, + 'roles': ['sql_lab'] + } + + class TestDomainSyncUtil(LoginUserTestMixin, SupersetTestCase): PLATFORM_ROLE_NAMES = ["Gamma", "sql_lab", "dataset_editor"] + @patch.object(HQRequest, "get") + def test_domain_access_success_response(self, get_mock): + get_mock.return_value = DomainAccessMockResponse() + + security_manager = self.app.appbuilder.sm + response = DomainSyncUtil(security_manager)._get_domain_access("test-domain") + + assert response == (True, True, ["sql_lab"]) + + @patch.object(HQRequest, "get") + def test_domain_access_faulty_response(self, get_mock): + get_mock.return_value = DomainAccessMockResponse(status_code=400) + + security_manager = self.app.appbuilder.sm + response = DomainSyncUtil(security_manager)._get_domain_access("test-domain") + + assert response == (False, False, []) + @patch.object(DomainSyncUtil, "_get_domain_access") def test_gamma_role_assigned_for_edit_permissions(self, get_domain_access_mock): security_manager = self.app.appbuilder.sm diff --git a/hq_superset/utils.py b/hq_superset/utils.py index 64789ba..bbacee4 100644 --- a/hq_superset/utils.py +++ b/hq_superset/utils.py @@ -220,7 +220,7 @@ def _get_domain_access(domain): response = hq_request.get() if response.status_code != 200: - return {}, [] + return False, False, [] response_data = response.json() hq_permissions = response_data['permissions'] From 52366715d1204ce01747e524ee8149eb82539958 Mon Sep 17 00:00:00 2001 From: Charl Smit Date: Mon, 18 Nov 2024 09:44:20 +0200 Subject: [PATCH 7/8] Don't append mutable obj --- hq_superset/utils.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hq_superset/utils.py b/hq_superset/utils.py index bbacee4..d9616be 100644 --- a/hq_superset/utils.py +++ b/hq_superset/utils.py @@ -202,14 +202,14 @@ def _get_additional_user_roles(self, domain): return [] if can_write: - user_role = GAMMA_ROLE_NAME + user_role_name = GAMMA_ROLE_NAME else: self._ensure_read_only_role_exists() - user_role = READ_ONLY_ROLE_NAME + user_role_name = READ_ONLY_ROLE_NAME - platform_roles_names.append(user_role) - - return self._get_platform_roles(platform_roles_names) + return self._get_platform_roles( + platform_roles_names + [user_role_name] + ) @staticmethod def _get_domain_access(domain): From 13d513caad58b27266b34a072e262e00b4c4a700 Mon Sep 17 00:00:00 2001 From: Charl Smit Date: Mon, 18 Nov 2024 09:46:50 +0200 Subject: [PATCH 8/8] Update method docstring --- hq_superset/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hq_superset/utils.py b/hq_superset/utils.py index d9616be..560cd98 100644 --- a/hq_superset/utils.py +++ b/hq_superset/utils.py @@ -133,7 +133,7 @@ def sync_domain_role(self, domain): The user gets assigned at least 3 roles in order to function on any domain: 1. hq_user_role: gives access to superset platform 2. domain_schema_role: restricts user access to specific domain schema - 3. Either the Gamma role for "edit" users or the READ_ONLY_ROLE_NAME for "view only" user + 3. Gamma role for users with write access or READ_ONLY_ROLE_NAME for users with read access only """ hq_user_role = self._ensure_hq_user_role() domain_schema_role = self._create_domain_role(domain)