From 837b5f4cc480a473405563ec552744e600b6564d Mon Sep 17 00:00:00 2001 From: mkangia Date: Fri, 1 Nov 2024 18:52:21 +0530 Subject: [PATCH 01/16] refactor: setup for multiple hooks --- hq_superset/hq_domain.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/hq_superset/hq_domain.py b/hq_superset/hq_domain.py index 0229124..bab0be2 100644 --- a/hq_superset/hq_domain.py +++ b/hq_superset/hq_domain.py @@ -4,7 +4,16 @@ def before_request_hook(): - return ensure_domain_selected() + """ + Call all hooks functions set in sequence and + if any hook returns a response, + break the chain and return that response + """ + hooks = [ensure_domain_selected] + for _function in hooks: + response = _function() + if response: + return response def after_request_hook(response): From 28815f5bca2b99b559f1306b8380334769cf5c12 Mon Sep 17 00:00:00 2001 From: mkangia Date: Fri, 1 Nov 2024 19:54:32 +0530 Subject: [PATCH 02/16] refactor: extract test utils & login mixin; isort --- hq_superset/tests/base_test.py | 33 +++++++++- hq_superset/tests/const.py | 13 ++++ hq_superset/tests/test_views.py | 109 +++----------------------------- hq_superset/tests/utils.py | 91 ++++++++++++++++++++++++++ 4 files changed, 144 insertions(+), 102 deletions(-) create mode 100644 hq_superset/tests/utils.py diff --git a/hq_superset/tests/base_test.py b/hq_superset/tests/base_test.py index b94e0be..bc5b3da 100644 --- a/hq_superset/tests/base_test.py +++ b/hq_superset/tests/base_test.py @@ -2,14 +2,15 @@ """ Base TestCase class """ - import os import shutil +import jwt from flask_testing import TestCase from sqlalchemy.sql import text from superset.app import create_app +from hq_superset.tests.utils import OAuthMock from hq_superset.utils import DOMAIN_PREFIX, get_hq_database superset_test_home = os.path.join(os.path.dirname(__file__), ".test_superset") @@ -48,3 +49,33 @@ def tearDown(self): sql = "; ".join(domain_schemas) + ";" connection.execute(text(sql)) super(HQDBTestCase, self).tearDown() + + +class LoginUserTestMixin(object): + """ + Use this mixin by calling login function with client + & then logout once done for clearing the session + """ + def login(self, client): + self._setup_user() + # bypass oauth-workflow by skipping login and oauth flow + with client.session_transaction() as session_: + session_["oauth_state"] = "mock_state" + state = jwt.encode({}, "mock_state", algorithm="HS256") + return client.get(f"/oauth-authorized/commcare?state={state}", follow_redirects=True) + + def _setup_user(self): + self.app.appbuilder.add_permissions(update_perms=True) + self.app.appbuilder.sm.sync_role_definitions() + + self.oauth_mock = OAuthMock() + self.app.appbuilder.sm.oauth_remotes = {"commcare": self.oauth_mock} + + gamma_role = self.app.appbuilder.sm.find_role('Gamma') + self.user = self.app.appbuilder.sm.find_user(self.oauth_mock.user_json['username']) + if not self.user: + self.user = self.app.appbuilder.sm.add_user(**self.oauth_mock.user_json, role=[gamma_role]) + + @staticmethod + def logout(client): + return client.get("/logout/") diff --git a/hq_superset/tests/const.py b/hq_superset/tests/const.py index 005ad7f..fbe1512 100644 --- a/hq_superset/tests/const.py +++ b/hq_superset/tests/const.py @@ -108,3 +108,16 @@ "id": "test1_ucr1", "resource_uri": "/a/demo/api/v0.5/ucr_data_source/52a134da12c9b801bd85d2122901b30c/" } + +TEST_UCR_CSV_V1 = """\ +doc_id,inserted_at,data_visit_date_eaece89e,data_visit_number_33d63739,data_lmp_date_5e24b993,data_visit_comment_fb984fda +a1, 2021-12-20, 2022-01-19, 100, 2022-02-20, some_text +a2, 2021-12-22, 2022-02-19, 10, 2022-03-20, some_other_text +""" + +TEST_UCR_CSV_V2 = """\ +doc_id,inserted_at,data_visit_date_eaece89e,data_visit_number_33d63739,data_lmp_date_5e24b993,data_visit_comment_fb984fda +a1, 2021-12-20, 2022-01-19, 100, 2022-02-20, some_text +a2, 2021-12-22, 2022-02-19, 10, 2022-03-20, some_other_text +a3, 2021-11-22, 2022-01-19, 10, 2022-03-20, some_other_text2 +""" diff --git a/hq_superset/tests/test_views.py b/hq_superset/tests/test_views.py index fcef247..4972f40 100644 --- a/hq_superset/tests/test_views.py +++ b/hq_superset/tests/test_views.py @@ -9,112 +9,19 @@ from sqlalchemy.sql import text from hq_superset.exceptions import HQAPIException +from hq_superset.tests.base_test import HQDBTestCase +from hq_superset.tests.const import ( + TEST_DATASOURCE, + TEST_UCR_CSV_V1, + TEST_UCR_CSV_V2, +) +from hq_superset.tests.utils import MockResponse, OAuthMock, UserMock from hq_superset.utils import ( SESSION_USER_DOMAINS_KEY, - get_schema_name_for_domain, DomainSyncUtil, + get_schema_name_for_domain, ) -from .base_test import HQDBTestCase -from .const import TEST_DATASOURCE - - -class MockResponse: - def __init__(self, json_data, status_code): - self.json_data = json_data - self.status_code = status_code - - def json(self): - return self.json_data - - @property - def content(self): - return pickle.dumps(self.json_data) - - -class UserMock(): - user_id = '123' - - def get_id(self): - return self.user_id - - -class OAuthMock(): - - def __init__(self): - self.user_json = { - 'username': 'testuser1', - 'first_name': 'user', - 'last_name': '1', - 'email': 'test@example.com', - } - self.domain_json = { - "objects": [ - { - "domain_name":"test1", - "project_name":"test1" - }, - { - "domain_name":"test2", - "project_name":"test 1" - }, - ] - } - self.test1_datasources = { - "objects": [ - { - "id": 'test1_ucr1', - "display_name": 'Test1 UCR1', - }, - { - "id": 'test1_ucr2', - "display_name": 'Test1 UCR2', - }, - ] - } - self.test2_datasources = { - "objects": [ - { - "id": 'test2_ucr1', - "display_name": 'Test2 UCR1', - } - ] - } - self.api_base_url = "https://cchq.org/" - self.user_domain_roles = { - "permissions": {"can_view": True, "can_edit": True}, - "roles": ["Gamma", "sql_lab"], - } - - def authorize_access_token(self): - return {"access_token": "some-key"} - - def get(self, url, token): - return { - 'api/v0.5/identity/': MockResponse(self.user_json, 200), - 'api/v0.5/user_domains?feature_flag=superset-analytics&can_view_reports=true': MockResponse(self.domain_json, 200), - 'a/test1/api/v0.5/ucr_data_source/': MockResponse(self.test1_datasources, 200), - 'a/test2/api/v0.5/ucr_data_source/': MockResponse(self.test2_datasources, 200), - 'a/test1/api/v0.5/ucr_data_source/test1_ucr1/': MockResponse(TEST_DATASOURCE, 200), - 'a/test1/configurable_reports/data_sources/export/test1_ucr1/?format=csv': MockResponse(TEST_UCR_CSV_V1, 200), - 'a/test1/api/v0.5/analytics-roles/': MockResponse(self.user_domain_roles, 200), - 'a/test2/api/v0.5/analytics-roles/': MockResponse(self.user_domain_roles, 200), - }[url] - - -TEST_UCR_CSV_V1 = """\ -doc_id,inserted_at,data_visit_date_eaece89e,data_visit_number_33d63739,data_lmp_date_5e24b993,data_visit_comment_fb984fda -a1, 2021-12-20, 2022-01-19, 100, 2022-02-20, some_text -a2, 2021-12-22, 2022-02-19, 10, 2022-03-20, some_other_text -""" - -TEST_UCR_CSV_V2 = """\ -doc_id,inserted_at,data_visit_date_eaece89e,data_visit_number_33d63739,data_lmp_date_5e24b993,data_visit_comment_fb984fda -a1, 2021-12-20, 2022-01-19, 100, 2022-02-20, some_text -a2, 2021-12-22, 2022-02-19, 10, 2022-03-20, some_other_text -a3, 2021-11-22, 2022-01-19, 10, 2022-03-20, some_other_text2 -""" - class TestViews(HQDBTestCase): diff --git a/hq_superset/tests/utils.py b/hq_superset/tests/utils.py new file mode 100644 index 0000000..2fbeb7a --- /dev/null +++ b/hq_superset/tests/utils.py @@ -0,0 +1,91 @@ +import pickle + +from hq_superset.tests.const import TEST_DATASOURCE, TEST_UCR_CSV_V1 + + +class OAuthMock(object): + + def __init__(self): + self.user_json = { + 'username': 'testuser1', + 'first_name': 'user', + 'last_name': '1', + 'email': 'test@example.com', + } + self.domain_json = { + "objects": [ + { + "domain_name":"test1", + "project_name":"test1" + }, + { + "domain_name":"test2", + "project_name":"test 1" + }, + ] + } + self.test1_datasources = { + "objects": [ + { + "id": 'test1_ucr1', + "display_name": 'Test1 UCR1', + }, + { + "id": 'test1_ucr2', + "display_name": 'Test1 UCR2', + }, + ] + } + self.test2_datasources = { + "objects": [ + { + "id": 'test2_ucr1', + "display_name": 'Test2 UCR1', + } + ] + } + self.api_base_url = "https://cchq.org/" + self.user_domain_roles = { + "permissions": {"can_view": True, "can_edit": True}, + "roles": ["Gamma", "sql_lab"], + } + + def authorize_access_token(self): + return {"access_token": "some-key"} + + def get(self, url, token): + return { + 'api/v0.5/identity/': MockResponse(self.user_json, 200), + 'api/v0.5/user_domains?feature_flag=superset-analytics&can_view_reports=true': MockResponse( + self.domain_json, 200 + ), + 'a/test1/api/v0.5/ucr_data_source/': MockResponse(self.test1_datasources, 200), + 'a/test2/api/v0.5/ucr_data_source/': MockResponse(self.test2_datasources, 200), + 'a/test1/api/v0.5/ucr_data_source/test1_ucr1/': MockResponse(TEST_DATASOURCE, 200), + 'a/test1/configurable_reports/data_sources/export/test1_ucr1/?format=csv': MockResponse( + TEST_UCR_CSV_V1, 200 + ), + 'a/test1/api/v0.5/analytics-roles/': MockResponse(self.user_domain_roles, 200), + 'a/test2/api/v0.5/analytics-roles/': MockResponse(self.user_domain_roles, 200), + }[url] + + +class UserMock(object): + user_id = '123' + + def get_id(self): + return self.user_id + + +class MockResponse: + def __init__(self, json_data, status_code): + self.json_data = json_data + self.status_code = status_code + + def json(self): + return self.json_data + + @property + def content(self): + return pickle.dumps(self.json_data) + From 011ebdac33428a63dc9a8c5abb770f3754290b71 Mon Sep 17 00:00:00 2001 From: mkangia Date: Fri, 1 Nov 2024 20:09:05 +0530 Subject: [PATCH 03/16] track role syncing timestamp --- hq_superset/tests/test_utils.py | 28 +++++++++++++++++++++++++--- hq_superset/utils.py | 9 ++++++++- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/hq_superset/tests/test_utils.py b/hq_superset/tests/test_utils.py index e5dbd7c..02d1da1 100644 --- a/hq_superset/tests/test_utils.py +++ b/hq_superset/tests/test_utils.py @@ -1,8 +1,10 @@ import doctest + +from flask import session from unittest.mock import patch -from hq_superset.utils import get_column_dtypes, DomainSyncUtil -from .base_test import SupersetTestCase +from hq_superset.utils import get_column_dtypes, DomainSyncUtil, SESSION_DOMAIN_ROLE_LAST_SYNCED_AT +from .base_test import SupersetTestCase, LoginUserTestMixin from .const import TEST_DATASOURCE @@ -27,7 +29,7 @@ def test_doctests(): assert results.failed == 0 -class TestDomainSyncUtil(SupersetTestCase): +class TestDomainSyncUtil(LoginUserTestMixin, SupersetTestCase): PLATFORM_ROLE_NAMES = ["Gamma", "sql_lab", "dataset_editor"] @patch.object(DomainSyncUtil, "_get_domain_access") @@ -102,6 +104,26 @@ def test_permissions_change_updates_user_role(self, get_domain_access_mock, doma additional_roles = DomainSyncUtil(security_manager)._get_additional_user_roles("test-domain") assert not additional_roles + @patch('hq_superset.utils.datetime_utcnow_naive') + @patch.object(DomainSyncUtil, "_get_domain_access") + def test_sync_domain_role(self, get_domain_access_mock, utcnow_mock): + client = self.app.test_client() + self.login(client) + + utcnow_mock_return = "2024-11-01 14:30:04.323000+00:00" + utcnow_mock.return_value = utcnow_mock_return + get_domain_access_mock.return_value = self._to_permissions_response( + can_write=False, + can_read=True, + roles=[], + ) + security_manager = self.app.appbuilder.sm + + self.assertIsNone(session.get(SESSION_DOMAIN_ROLE_LAST_SYNCED_AT)) + DomainSyncUtil(security_manager).sync_domain_role("test-domain") + self.assertEqual(session[SESSION_DOMAIN_ROLE_LAST_SYNCED_AT], utcnow_mock_return) + self.logout(client) + def _ensure_platform_roles_exist(self, sm): for role_name in self.PLATFORM_ROLE_NAMES: sm.add_role(role_name) diff --git a/hq_superset/utils.py b/hq_superset/utils.py index d4f4f93..d98157b 100644 --- a/hq_superset/utils.py +++ b/hq_superset/utils.py @@ -9,9 +9,10 @@ from zipfile import ZipFile import pandas +import pytz import sqlalchemy from cryptography.fernet import Fernet -from flask import current_app +from flask import current_app, session from flask_login import current_user from sqlalchemy.sql import TableClause from superset.utils.database import get_or_create_db @@ -30,6 +31,7 @@ DOMAIN_PREFIX = "hqdomain_" SESSION_USER_DOMAINS_KEY = "user_hq_domains" SESSION_OAUTH_RESPONSE_KEY = "oauth_response" +SESSION_DOMAIN_ROLE_LAST_SYNCED_AT = "domain_role_last_synced_at" def get_hq_database(): @@ -151,6 +153,7 @@ def sync_domain_role(self, domain): self.sm.get_session.add(current_user) self.sm.get_session.commit() + session['domain_role_last_synced_at'] = datetime_utcnow_naive() return True def _ensure_hq_user_role(self): @@ -415,3 +418,7 @@ def cast_data_for_table( def generate_secret(): alphabet = string.ascii_letters + string.digits return ''.join(secrets.choice(alphabet) for __ in range(64)) + + +def datetime_utcnow_naive(): + return datetime.utcnow().replace(tzinfo=pytz.UTC) From df017eacdd1617d94a7ed0ede06b8f820e7317e1 Mon Sep 17 00:00:00 2001 From: mkangia Date: Fri, 1 Nov 2024 20:10:45 +0530 Subject: [PATCH 04/16] isort --- ...024-02-24_23-53_56d0467ff6ff_added_oauth_tables.py | 3 +-- hq_superset/oauth.py | 5 +---- hq_superset/tests/test_utils.py | 11 ++++++++--- hq_superset/utils.py | 8 ++++---- hq_superset/views.py | 6 +----- 5 files changed, 15 insertions(+), 18 deletions(-) diff --git a/hq_superset/migrations/versions/2024-02-24_23-53_56d0467ff6ff_added_oauth_tables.py b/hq_superset/migrations/versions/2024-02-24_23-53_56d0467ff6ff_added_oauth_tables.py index 0b962f5..0e120bf 100644 --- a/hq_superset/migrations/versions/2024-02-24_23-53_56d0467ff6ff_added_oauth_tables.py +++ b/hq_superset/migrations/versions/2024-02-24_23-53_56d0467ff6ff_added_oauth_tables.py @@ -6,9 +6,8 @@ """ from typing import Sequence, Union -from alembic import op import sqlalchemy as sa - +from alembic import op # revision identifiers, used by Alembic. revision: str = '56d0467ff6ff' diff --git a/hq_superset/oauth.py b/hq_superset/oauth.py index c6e8fcb..e557684 100644 --- a/hq_superset/oauth.py +++ b/hq_superset/oauth.py @@ -7,10 +7,7 @@ from superset.security import SupersetSecurityManager from .exceptions import OAuthSessionExpired -from .utils import ( - SESSION_OAUTH_RESPONSE_KEY, - SESSION_USER_DOMAINS_KEY, -) +from .utils import SESSION_OAUTH_RESPONSE_KEY, SESSION_USER_DOMAINS_KEY logger = logging.getLogger(__name__) diff --git a/hq_superset/tests/test_utils.py b/hq_superset/tests/test_utils.py index 02d1da1..a4c260c 100644 --- a/hq_superset/tests/test_utils.py +++ b/hq_superset/tests/test_utils.py @@ -1,10 +1,15 @@ import doctest +from unittest.mock import patch from flask import session -from unittest.mock import patch -from hq_superset.utils import get_column_dtypes, DomainSyncUtil, SESSION_DOMAIN_ROLE_LAST_SYNCED_AT -from .base_test import SupersetTestCase, LoginUserTestMixin +from hq_superset.utils import ( + SESSION_DOMAIN_ROLE_LAST_SYNCED_AT, + DomainSyncUtil, + get_column_dtypes, +) + +from .base_test import LoginUserTestMixin, SupersetTestCase from .const import TEST_DATASOURCE diff --git a/hq_superset/utils.py b/hq_superset/utils.py index d98157b..0ae471d 100644 --- a/hq_superset/utils.py +++ b/hq_superset/utils.py @@ -18,13 +18,13 @@ from superset.utils.database import get_or_create_db from .const import ( + CAN_READ_PERMISSION, + CAN_WRITE_PERMISSION, GAMMA_ROLE, HQ_DATABASE_NAME, HQ_USER_ROLE_NAME, - SCHEMA_ACCESS_PERMISSION, - CAN_READ_PERMISSION, - CAN_WRITE_PERMISSION, READ_ONLY_MENU_PERMISSIONS, + SCHEMA_ACCESS_PERMISSION, ) from .exceptions import DatabaseMissing @@ -227,8 +227,8 @@ def _get_additional_user_roles(self, domain): @staticmethod def _get_domain_access(domain): - from .hq_url import user_domain_roles from .hq_requests import HQRequest + from .hq_url import user_domain_roles hq_request = HQRequest(url=user_domain_roles(domain)) response = hq_request.get() diff --git a/hq_superset/views.py b/hq_superset/views.py index af02bfc..55a54b0 100644 --- a/hq_superset/views.py +++ b/hq_superset/views.py @@ -28,11 +28,7 @@ unsubscribe_from_hq_datasource, ) from .tasks import refresh_hq_datasource_task -from .utils import ( - DomainSyncUtil, - get_hq_database, - get_schema_name_for_domain, -) +from .utils import DomainSyncUtil, get_hq_database, get_schema_name_for_domain ASYNC_DATASOURCE_IMPORT_LIMIT_IN_BYTES = 5_000_000 # ~5MB From 3622bbf947c37bf6a12b289c001337962c01af1e Mon Sep 17 00:00:00 2001 From: mkangia Date: Fri, 1 Nov 2024 21:45:43 +0530 Subject: [PATCH 05/16] resync domain role after expiry --- hq_superset/hq_domain.py | 39 ++++++++++++++++- hq_superset/tests/config_for_tests.py | 1 + hq_superset/tests/test_views.py | 61 +++++++++++++++++++++++++++ hq_superset/utils.py | 2 +- superset_config.example.py | 2 + 5 files changed, 102 insertions(+), 3 deletions(-) diff --git a/hq_superset/hq_domain.py b/hq_superset/hq_domain.py index bab0be2..52520b4 100644 --- a/hq_superset/hq_domain.py +++ b/hq_superset/hq_domain.py @@ -1,6 +1,16 @@ +import superset + +from datetime import timedelta from flask import flash, g, redirect, request, session, url_for -from .utils import SESSION_USER_DOMAINS_KEY +from superset.config import USER_DOMAIN_ROLE_EXPIRY + +from .utils import ( + SESSION_DOMAIN_ROLE_LAST_SYNCED_AT, + SESSION_USER_DOMAINS_KEY, + DomainSyncUtil, + datetime_utcnow_naive, +) def before_request_hook(): @@ -9,7 +19,10 @@ def before_request_hook(): if any hook returns a response, break the chain and return that response """ - hooks = [ensure_domain_selected] + hooks = [ + ensure_domain_selected, + sync_user_domain_role + ] for _function in hooks: response = _function() if response: @@ -64,6 +77,28 @@ def ensure_domain_selected(): return redirect(url_for('SelectDomainView.list', next=request.url)) +def sync_user_domain_role(): + if is_user_admin() or ( + request.url_rule + and request.url_rule.endpoint in DOMAIN_EXCLUDED_VIEWS + ): + return + if _domain_role_expired(): + _sync_domain_role() + + +def _domain_role_expired(): + if not session.get(SESSION_DOMAIN_ROLE_LAST_SYNCED_AT): + return True + + time_since_last_sync = datetime_utcnow_naive() - session[SESSION_DOMAIN_ROLE_LAST_SYNCED_AT] + return time_since_last_sync >= timedelta(minutes=USER_DOMAIN_ROLE_EXPIRY) + + +def _sync_domain_role(): + DomainSyncUtil(superset.appbuilder.sm).sync_domain_role(g.hq_domain) + + def is_valid_user_domain(hq_domain): # Admins have access to all domains return is_user_admin() or hq_domain in user_domains() diff --git a/hq_superset/tests/config_for_tests.py b/hq_superset/tests/config_for_tests.py index 5b43e63..c24a8d7 100644 --- a/hq_superset/tests/config_for_tests.py +++ b/hq_superset/tests/config_for_tests.py @@ -65,3 +65,4 @@ # CommCare Analytics extensions FLASK_APP_MUTATOR = flask_app_mutator CUSTOM_SECURITY_MANAGER = oauth.CommCareSecurityManager +USER_DOMAIN_ROLE_EXPIRY = 60 # minutes diff --git a/hq_superset/tests/test_views.py b/hq_superset/tests/test_views.py index 4972f40..97917f9 100644 --- a/hq_superset/tests/test_views.py +++ b/hq_superset/tests/test_views.py @@ -17,6 +17,7 @@ ) from hq_superset.tests.utils import MockResponse, OAuthMock, UserMock from hq_superset.utils import ( + SESSION_DOMAIN_ROLE_LAST_SYNCED_AT, SESSION_USER_DOMAINS_KEY, DomainSyncUtil, get_schema_name_for_domain, @@ -311,3 +312,63 @@ def _test_upload(test_data, expected_output): client.get('/hq_datasource/list/', follow_redirects=True) self.assert_context('ucr_id_to_pks', {}) + + @patch('hq_superset.hq_requests.get_valid_cchq_oauth_token', return_value={}) + @patch('hq_superset.hq_domain._sync_domain_role', return_value=True) + def test_sync_user_domain_role_calls(self, sync_domain_role_mock, *args): + with self.app.test_client() as client: + assert SESSION_USER_DOMAINS_KEY not in session + self.login(client) + self.assertIsNone(session.get(SESSION_DOMAIN_ROLE_LAST_SYNCED_AT)) + # not called on login + sync_domain_role_mock.assert_not_called() + + response = client.get('/', follow_redirects=True) + self.assertEqual(response.status, "200 OK") + self.assertTrue('/domain/list' in response.request.path) + self.assertIsNone(session.get(SESSION_DOMAIN_ROLE_LAST_SYNCED_AT)) + # not called on domain listing after login + sync_domain_role_mock.assert_not_called() + + client.get('/domain/select/test1/', follow_redirects=True) + self.assertIsNotNone(session.get(SESSION_DOMAIN_ROLE_LAST_SYNCED_AT)) + # not called on domain selection + sync_domain_role_mock.assert_not_called() + + client.get('/hq_datasource/list/', follow_redirects=True) + self.assertIsNotNone(session.get(SESSION_DOMAIN_ROLE_LAST_SYNCED_AT)) + # not called if recently synced + sync_domain_role_mock.assert_not_called() + + with patch('hq_superset.hq_domain.USER_DOMAIN_ROLE_EXPIRY', 0): + client.get('/hq_datasource/list/', follow_redirects=True) + self.assertIsNotNone(session.get(SESSION_DOMAIN_ROLE_LAST_SYNCED_AT)) + # called only if expired + sync_domain_role_mock.assert_called() + + self.logout(client) + + @patch('hq_superset.hq_requests.get_valid_cchq_oauth_token', return_value={}) + def test_sync_user_domain_before_request(self, *args): + with self.app.test_client() as client: + assert SESSION_USER_DOMAINS_KEY not in session + self.login(client) + + client.get('/domain/select/test1/', follow_redirects=True) + self.assertIsNotNone(session.get(SESSION_DOMAIN_ROLE_LAST_SYNCED_AT)) + + with patch('hq_superset.hq_domain.USER_DOMAIN_ROLE_EXPIRY', 0): + session_role_last_synced_at = session.get(SESSION_DOMAIN_ROLE_LAST_SYNCED_AT) + client.get('/hq_datasource/list/', follow_redirects=True) + # timestamp in session updated + self.assertNotEqual(session_role_last_synced_at, session.get(SESSION_DOMAIN_ROLE_LAST_SYNCED_AT)) + + with patch.object(DomainSyncUtil, 'sync_domain_role') as domain_sync_util_mock: + client.get('/hq_datasource/list/', follow_redirects=True) + # confirm function call for sync + domain_sync_util_mock.assert_called_once_with( + "test1" + ) + + self.logout(client) + diff --git a/hq_superset/utils.py b/hq_superset/utils.py index 0ae471d..f99dd3f 100644 --- a/hq_superset/utils.py +++ b/hq_superset/utils.py @@ -153,7 +153,7 @@ def sync_domain_role(self, domain): self.sm.get_session.add(current_user) self.sm.get_session.commit() - session['domain_role_last_synced_at'] = datetime_utcnow_naive() + session[SESSION_DOMAIN_ROLE_LAST_SYNCED_AT] = datetime_utcnow_naive() return True def _ensure_hq_user_role(self): diff --git a/superset_config.example.py b/superset_config.example.py index bd283af..b62461f 100644 --- a/superset_config.example.py +++ b/superset_config.example.py @@ -180,3 +180,5 @@ class CeleryConfig: "session_cookie_secure": False, } +USER_DOMAIN_ROLE_EXPIRY = 60 # minutes + From b437abadc1e69c05d1c038108a5548310087f8ae Mon Sep 17 00:00:00 2001 From: mkangia Date: Fri, 1 Nov 2024 21:53:37 +0530 Subject: [PATCH 06/16] refactor: move constants to const;isort --- hq_superset/const.py | 5 +++++ hq_superset/hq_domain.py | 13 ++++--------- hq_superset/oauth.py | 2 +- hq_superset/tests/base_test.py | 3 ++- hq_superset/tests/test_hq_domain.py | 2 +- hq_superset/tests/test_oauth.py | 5 +---- hq_superset/tests/test_utils.py | 7 ++----- hq_superset/tests/test_views.py | 11 +++++------ hq_superset/utils.py | 7 ++----- 9 files changed, 23 insertions(+), 32 deletions(-) diff --git a/hq_superset/const.py b/hq_superset/const.py index d17be33..4f63624 100644 --- a/hq_superset/const.py +++ b/hq_superset/const.py @@ -48,3 +48,8 @@ "Datasets": [MENU_ACCESS_PERMISSION], "ExploreFormDataRestApi": [CAN_READ_PERMISSION] } + +DOMAIN_PREFIX = "hqdomain_" +SESSION_USER_DOMAINS_KEY = "user_hq_domains" +SESSION_OAUTH_RESPONSE_KEY = "oauth_response" +SESSION_DOMAIN_ROLE_LAST_SYNCED_AT = "domain_role_last_synced_at" diff --git a/hq_superset/hq_domain.py b/hq_superset/hq_domain.py index 52520b4..3bef706 100644 --- a/hq_superset/hq_domain.py +++ b/hq_superset/hq_domain.py @@ -1,16 +1,11 @@ -import superset - from datetime import timedelta -from flask import flash, g, redirect, request, session, url_for +import superset +from flask import flash, g, redirect, request, session, url_for from superset.config import USER_DOMAIN_ROLE_EXPIRY -from .utils import ( - SESSION_DOMAIN_ROLE_LAST_SYNCED_AT, - SESSION_USER_DOMAINS_KEY, - DomainSyncUtil, - datetime_utcnow_naive, -) +from .const import SESSION_DOMAIN_ROLE_LAST_SYNCED_AT, SESSION_USER_DOMAINS_KEY +from .utils import DomainSyncUtil, datetime_utcnow_naive def before_request_hook(): diff --git a/hq_superset/oauth.py b/hq_superset/oauth.py index e557684..fa68dcc 100644 --- a/hq_superset/oauth.py +++ b/hq_superset/oauth.py @@ -6,8 +6,8 @@ from requests.exceptions import HTTPError from superset.security import SupersetSecurityManager +from .const import SESSION_OAUTH_RESPONSE_KEY, SESSION_USER_DOMAINS_KEY from .exceptions import OAuthSessionExpired -from .utils import SESSION_OAUTH_RESPONSE_KEY, SESSION_USER_DOMAINS_KEY logger = logging.getLogger(__name__) diff --git a/hq_superset/tests/base_test.py b/hq_superset/tests/base_test.py index bc5b3da..4408b97 100644 --- a/hq_superset/tests/base_test.py +++ b/hq_superset/tests/base_test.py @@ -10,8 +10,9 @@ from sqlalchemy.sql import text from superset.app import create_app +from hq_superset.const import DOMAIN_PREFIX from hq_superset.tests.utils import OAuthMock -from hq_superset.utils import DOMAIN_PREFIX, get_hq_database +from hq_superset.utils import get_hq_database superset_test_home = os.path.join(os.path.dirname(__file__), ".test_superset") shutil.rmtree(superset_test_home, ignore_errors=True) diff --git a/hq_superset/tests/test_hq_domain.py b/hq_superset/tests/test_hq_domain.py index 68cd5ed..84166fc 100644 --- a/hq_superset/tests/test_hq_domain.py +++ b/hq_superset/tests/test_hq_domain.py @@ -11,12 +11,12 @@ user_domains, ) from hq_superset.utils import ( - SESSION_USER_DOMAINS_KEY, DomainSyncUtil, get_hq_database, get_schema_name_for_domain, ) +from ..const import SESSION_USER_DOMAINS_KEY from .base_test import HQDBTestCase, SupersetTestCase MOCK_DOMAIN_SESSION = { diff --git a/hq_superset/tests/test_oauth.py b/hq_superset/tests/test_oauth.py index 8182bc7..7a17be3 100644 --- a/hq_superset/tests/test_oauth.py +++ b/hq_superset/tests/test_oauth.py @@ -5,11 +5,8 @@ from hq_superset.exceptions import OAuthSessionExpired from hq_superset.oauth import get_valid_cchq_oauth_token -from hq_superset.utils import ( - SESSION_OAUTH_RESPONSE_KEY, - SESSION_USER_DOMAINS_KEY, -) +from ..const import SESSION_OAUTH_RESPONSE_KEY, SESSION_USER_DOMAINS_KEY from .base_test import SupersetTestCase diff --git a/hq_superset/tests/test_utils.py b/hq_superset/tests/test_utils.py index a4c260c..abddded 100644 --- a/hq_superset/tests/test_utils.py +++ b/hq_superset/tests/test_utils.py @@ -3,12 +3,9 @@ from flask import session -from hq_superset.utils import ( - SESSION_DOMAIN_ROLE_LAST_SYNCED_AT, - DomainSyncUtil, - get_column_dtypes, -) +from hq_superset.utils import DomainSyncUtil, get_column_dtypes +from ..const import SESSION_DOMAIN_ROLE_LAST_SYNCED_AT from .base_test import LoginUserTestMixin, SupersetTestCase from .const import TEST_DATASOURCE diff --git a/hq_superset/tests/test_views.py b/hq_superset/tests/test_views.py index 97917f9..e0171fe 100644 --- a/hq_superset/tests/test_views.py +++ b/hq_superset/tests/test_views.py @@ -8,6 +8,10 @@ from flask import redirect, session from sqlalchemy.sql import text +from hq_superset.const import ( + SESSION_DOMAIN_ROLE_LAST_SYNCED_AT, + SESSION_USER_DOMAINS_KEY, +) from hq_superset.exceptions import HQAPIException from hq_superset.tests.base_test import HQDBTestCase from hq_superset.tests.const import ( @@ -16,12 +20,7 @@ TEST_UCR_CSV_V2, ) from hq_superset.tests.utils import MockResponse, OAuthMock, UserMock -from hq_superset.utils import ( - SESSION_DOMAIN_ROLE_LAST_SYNCED_AT, - SESSION_USER_DOMAINS_KEY, - DomainSyncUtil, - get_schema_name_for_domain, -) +from hq_superset.utils import DomainSyncUtil, get_schema_name_for_domain class TestViews(HQDBTestCase): diff --git a/hq_superset/utils.py b/hq_superset/utils.py index f99dd3f..592c396 100644 --- a/hq_superset/utils.py +++ b/hq_superset/utils.py @@ -20,19 +20,16 @@ from .const import ( CAN_READ_PERMISSION, CAN_WRITE_PERMISSION, + DOMAIN_PREFIX, GAMMA_ROLE, HQ_DATABASE_NAME, HQ_USER_ROLE_NAME, READ_ONLY_MENU_PERMISSIONS, SCHEMA_ACCESS_PERMISSION, + SESSION_DOMAIN_ROLE_LAST_SYNCED_AT, ) from .exceptions import DatabaseMissing -DOMAIN_PREFIX = "hqdomain_" -SESSION_USER_DOMAINS_KEY = "user_hq_domains" -SESSION_OAUTH_RESPONSE_KEY = "oauth_response" -SESSION_DOMAIN_ROLE_LAST_SYNCED_AT = "domain_role_last_synced_at" - def get_hq_database(): """ From 764d6779b8c8fb005747e896ef6c40628a31bf53 Mon Sep 17 00:00:00 2001 From: mkangia Date: Fri, 1 Nov 2024 21:57:58 +0530 Subject: [PATCH 07/16] refactor: expand imports;isort --- hq_superset/api.py | 6 +++--- hq_superset/hq_domain.py | 7 +++++-- hq_superset/models.py | 10 +++++++--- hq_superset/oauth.py | 7 +++++-- hq_superset/oauth2_server.py | 2 +- hq_superset/services.py | 10 +++++----- hq_superset/tasks.py | 6 +++--- hq_superset/tests/test_hq_domain.py | 5 ++--- hq_superset/tests/test_oauth.py | 8 +++++--- hq_superset/tests/test_utils.py | 7 +++---- hq_superset/utils.py | 4 ++-- hq_superset/views.py | 18 +++++++++++------- 12 files changed, 52 insertions(+), 38 deletions(-) diff --git a/hq_superset/api.py b/hq_superset/api.py index e5e5254..8f7b73b 100644 --- a/hq_superset/api.py +++ b/hq_superset/api.py @@ -11,9 +11,9 @@ json_success, ) -from .models import DataSetChange -from .oauth2_server import authorization, require_oauth -from .tasks import process_dataset_change +from hq_superset.models import DataSetChange +from hq_superset.oauth2_server import authorization, require_oauth +from hq_superset.tasks import process_dataset_change class OAuth(BaseApi): diff --git a/hq_superset/hq_domain.py b/hq_superset/hq_domain.py index 3bef706..1736775 100644 --- a/hq_superset/hq_domain.py +++ b/hq_superset/hq_domain.py @@ -4,8 +4,11 @@ from flask import flash, g, redirect, request, session, url_for from superset.config import USER_DOMAIN_ROLE_EXPIRY -from .const import SESSION_DOMAIN_ROLE_LAST_SYNCED_AT, SESSION_USER_DOMAINS_KEY -from .utils import DomainSyncUtil, datetime_utcnow_naive +from hq_superset.const import ( + SESSION_DOMAIN_ROLE_LAST_SYNCED_AT, + SESSION_USER_DOMAINS_KEY, +) +from hq_superset.utils import DomainSyncUtil, datetime_utcnow_naive def before_request_hook(): diff --git a/hq_superset/models.py b/hq_superset/models.py index 4a35c2f..175a314 100644 --- a/hq_superset/models.py +++ b/hq_superset/models.py @@ -8,9 +8,13 @@ from cryptography.fernet import MultiFernet from superset import db -from .const import OAUTH2_DATABASE_NAME -from .exceptions import TableMissing -from .utils import cast_data_for_table, get_fernet_keys, get_hq_database +from hq_superset.const import OAUTH2_DATABASE_NAME +from hq_superset.exceptions import TableMissing +from hq_superset.utils import ( + cast_data_for_table, + get_fernet_keys, + get_hq_database, +) @dataclass diff --git a/hq_superset/oauth.py b/hq_superset/oauth.py index fa68dcc..d724d77 100644 --- a/hq_superset/oauth.py +++ b/hq_superset/oauth.py @@ -6,8 +6,11 @@ from requests.exceptions import HTTPError from superset.security import SupersetSecurityManager -from .const import SESSION_OAUTH_RESPONSE_KEY, SESSION_USER_DOMAINS_KEY -from .exceptions import OAuthSessionExpired +from hq_superset.const import ( + SESSION_OAUTH_RESPONSE_KEY, + SESSION_USER_DOMAINS_KEY, +) +from hq_superset.exceptions import OAuthSessionExpired logger = logging.getLogger(__name__) diff --git a/hq_superset/oauth2_server.py b/hq_superset/oauth2_server.py index f72285a..c1cbde1 100644 --- a/hq_superset/oauth2_server.py +++ b/hq_superset/oauth2_server.py @@ -13,7 +13,7 @@ ) from authlib.oauth2.rfc6749 import grants -from .models import OAuth2Client, OAuth2Token, db +from hq_superset.models import OAuth2Client, OAuth2Token, db def save_token(token: dict, request: FlaskOAuth2Request) -> None: diff --git a/hq_superset/services.py b/hq_superset/services.py index 284c478..7cd09ab 100644 --- a/hq_superset/services.py +++ b/hq_superset/services.py @@ -13,16 +13,16 @@ from superset.extensions import cache_manager from superset.sql_parse import Table -from .exceptions import HQAPIException -from .hq_requests import HQRequest -from .hq_url import ( +from hq_superset.exceptions import HQAPIException +from hq_superset.hq_requests import HQRequest +from hq_superset.hq_url import ( datasource_details, datasource_export, datasource_subscribe, datasource_unsubscribe, ) -from .models import OAuth2Client -from .utils import ( +from hq_superset.models import OAuth2Client +from hq_superset.utils import ( convert_to_array, generate_secret, get_column_dtypes, diff --git a/hq_superset/tasks.py b/hq_superset/tasks.py index 0797bee..09f0ed6 100644 --- a/hq_superset/tasks.py +++ b/hq_superset/tasks.py @@ -2,9 +2,9 @@ from superset.extensions import celery_app -from .exceptions import TableMissing -from .models import DataSetChange -from .services import AsyncImportHelper, refresh_hq_datasource +from hq_superset.exceptions import TableMissing +from hq_superset.models import DataSetChange +from hq_superset.services import AsyncImportHelper, refresh_hq_datasource @celery_app.task(name='refresh_hq_datasource_task') diff --git a/hq_superset/tests/test_hq_domain.py b/hq_superset/tests/test_hq_domain.py index 84166fc..348eddc 100644 --- a/hq_superset/tests/test_hq_domain.py +++ b/hq_superset/tests/test_hq_domain.py @@ -2,6 +2,7 @@ from flask import g +from hq_superset.const import SESSION_USER_DOMAINS_KEY from hq_superset.hq_domain import ( DOMAIN_EXCLUDED_VIEWS, after_request_hook, @@ -10,15 +11,13 @@ is_valid_user_domain, user_domains, ) +from hq_superset.tests.base_test import HQDBTestCase, SupersetTestCase from hq_superset.utils import ( DomainSyncUtil, get_hq_database, get_schema_name_for_domain, ) -from ..const import SESSION_USER_DOMAINS_KEY -from .base_test import HQDBTestCase, SupersetTestCase - MOCK_DOMAIN_SESSION = { SESSION_USER_DOMAINS_KEY:[ { diff --git a/hq_superset/tests/test_oauth.py b/hq_superset/tests/test_oauth.py index 7a17be3..a1a2c91 100644 --- a/hq_superset/tests/test_oauth.py +++ b/hq_superset/tests/test_oauth.py @@ -3,11 +3,13 @@ from flask import session +from hq_superset.const import ( + SESSION_OAUTH_RESPONSE_KEY, + SESSION_USER_DOMAINS_KEY, +) from hq_superset.exceptions import OAuthSessionExpired from hq_superset.oauth import get_valid_cchq_oauth_token - -from ..const import SESSION_OAUTH_RESPONSE_KEY, SESSION_USER_DOMAINS_KEY -from .base_test import SupersetTestCase +from hq_superset.tests.base_test import SupersetTestCase class MockResponse: diff --git a/hq_superset/tests/test_utils.py b/hq_superset/tests/test_utils.py index abddded..11f29a3 100644 --- a/hq_superset/tests/test_utils.py +++ b/hq_superset/tests/test_utils.py @@ -3,12 +3,11 @@ from flask import session +from hq_superset.const import SESSION_DOMAIN_ROLE_LAST_SYNCED_AT +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 ..const import SESSION_DOMAIN_ROLE_LAST_SYNCED_AT -from .base_test import LoginUserTestMixin, SupersetTestCase -from .const import TEST_DATASOURCE - def test_get_column_dtypes(): datasource_defn = TEST_DATASOURCE diff --git a/hq_superset/utils.py b/hq_superset/utils.py index 592c396..90f78e7 100644 --- a/hq_superset/utils.py +++ b/hq_superset/utils.py @@ -17,7 +17,7 @@ from sqlalchemy.sql import TableClause from superset.utils.database import get_or_create_db -from .const import ( +from hq_superset.const import ( CAN_READ_PERMISSION, CAN_WRITE_PERMISSION, DOMAIN_PREFIX, @@ -28,7 +28,7 @@ SCHEMA_ACCESS_PERMISSION, SESSION_DOMAIN_ROLE_LAST_SYNCED_AT, ) -from .exceptions import DatabaseMissing +from hq_superset.exceptions import DatabaseMissing def get_hq_database(): diff --git a/hq_superset/views.py b/hq_superset/views.py index 55a54b0..92455d5 100644 --- a/hq_superset/views.py +++ b/hq_superset/views.py @@ -16,19 +16,23 @@ from superset.connectors.sqla.models import SqlaTable from superset.views.base import BaseSupersetView -from .exceptions import HQAPIException -from .hq_domain import user_domains -from .hq_requests import HQRequest -from .hq_url import datasource_list -from .services import ( +from hq_superset.exceptions import HQAPIException +from hq_superset.hq_domain import user_domains +from hq_superset.hq_requests import HQRequest +from hq_superset.hq_url import datasource_list +from hq_superset.services import ( AsyncImportHelper, download_and_subscribe_to_datasource, get_datasource_defn, refresh_hq_datasource, unsubscribe_from_hq_datasource, ) -from .tasks import refresh_hq_datasource_task -from .utils import DomainSyncUtil, get_hq_database, get_schema_name_for_domain +from hq_superset.tasks import refresh_hq_datasource_task +from hq_superset.utils import ( + DomainSyncUtil, + get_hq_database, + get_schema_name_for_domain, +) ASYNC_DATASOURCE_IMPORT_LIMIT_IN_BYTES = 5_000_000 # ~5MB From 2d002cae0d16edaf727829fcfe8cbdcfea5c87c4 Mon Sep 17 00:00:00 2001 From: mkangia Date: Fri, 1 Nov 2024 22:31:49 +0530 Subject: [PATCH 08/16] fix flaky test --- hq_superset/tests/test_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hq_superset/tests/test_views.py b/hq_superset/tests/test_views.py index e0171fe..9c9f32a 100644 --- a/hq_superset/tests/test_views.py +++ b/hq_superset/tests/test_views.py @@ -134,7 +134,7 @@ def _do_assert(datasources): client.get('/hq_datasource/list/', follow_redirects=True) _do_assert(self.oauth_mock.test2_datasources) - @patch.object(DomainSyncUtil, "sync_domain_role", return_value=True) + @patch.object(DomainSyncUtil, "_get_domain_access", return_value=({"can_write": True, "can_read": True}, [])) def test_datasource_upload(self, *args): client = self.app.test_client() self.login(client) From e678353f37a08396d32fc1801b95e92b0f5b1d10 Mon Sep 17 00:00:00 2001 From: mkangia Date: Fri, 1 Nov 2024 22:32:25 +0530 Subject: [PATCH 09/16] ensure logout after each test --- hq_superset/tests/test_views.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hq_superset/tests/test_views.py b/hq_superset/tests/test_views.py index 9c9f32a..0b0382d 100644 --- a/hq_superset/tests/test_views.py +++ b/hq_superset/tests/test_views.py @@ -133,6 +133,7 @@ def _do_assert(datasources): client.get('/domain/select/test2/', follow_redirects=True) client.get('/hq_datasource/list/', follow_redirects=True) _do_assert(self.oauth_mock.test2_datasources) + self.logout(client) @patch.object(DomainSyncUtil, "_get_domain_access", return_value=({"can_write": True, "can_read": True}, [])) def test_datasource_upload(self, *args): @@ -148,6 +149,7 @@ def test_datasource_upload(self, *args): ucr_id, 'ds1' ) + self.logout(client) @patch.object(DomainSyncUtil, "sync_domain_role", return_value=True) def test_trigger_datasource_refresh_with_api_exception(self, *args): @@ -166,6 +168,7 @@ def test_trigger_datasource_refresh_with_api_exception(self, *args): ) self.assertEqual(response.status_code, 302) self.assertEqual(response.location, "/tablemodelview/list/") + self.logout(client) def test_trigger_datasource_refresh_with_errors(self, *args): from hq_superset.views import ( From 716651c843daba4cf17da0aa0507b16ce6746365 Mon Sep 17 00:00:00 2001 From: mkangia Date: Sat, 2 Nov 2024 02:19:43 +0530 Subject: [PATCH 10/16] add domain independent call - /api/v1/me is called frequently leading to before_request_hook getting fired unnecessarily, specifically for sync_user_domain_role leading to more than one syncs happening at once leading to db conflicts/errors --- hq_superset/hq_domain.py | 1 + 1 file changed, 1 insertion(+) diff --git a/hq_superset/hq_domain.py b/hq_superset/hq_domain.py index 1736775..decb503 100644 --- a/hq_superset/hq_domain.py +++ b/hq_superset/hq_domain.py @@ -44,6 +44,7 @@ def after_request_hook(response): 'AuthOAuthView.login', 'AuthOAuthView.logout', 'AuthOAuthView.oauth_authorized', + 'CurrentUserRestApi.get_me', 'DataSetChangeAPI.post_dataset_change', 'OAuth.issue_access_token', 'SelectDomainView.list', From c8111d4b3cd4c611a5abc88bd842d7e14f63daa6 Mon Sep 17 00:00:00 2001 From: mkangia Date: Sat, 2 Nov 2024 02:20:18 +0530 Subject: [PATCH 11/16] refactor; remove duplicate import --- hq_superset/hq_domain.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hq_superset/hq_domain.py b/hq_superset/hq_domain.py index decb503..6a82cd3 100644 --- a/hq_superset/hq_domain.py +++ b/hq_superset/hq_domain.py @@ -33,7 +33,7 @@ def after_request_hook(response): "AuthDBView.login", "AuthOAuthView.logout", ] - if (request.url_rule and request.url_rule.endpoint in logout_views): + if request.url_rule and (request.url_rule.endpoint in logout_views): response.set_cookie('hq_domain', '', expires=0) return response @@ -114,7 +114,6 @@ def user_domains(): def add_domain_links(active_domain, domains): - import superset for domain in domains: superset.appbuilder.menu.add_link(domain, category=active_domain, href=url_for('SelectDomainView.select', hq_domain=domain)) From 7ef5429ac69799687b811c5d0a423c5856571826 Mon Sep 17 00:00:00 2001 From: mkangia Date: Mon, 4 Nov 2024 22:48:27 +0530 Subject: [PATCH 12/16] correct function name --- hq_superset/hq_domain.py | 4 ++-- hq_superset/tests/test_utils.py | 2 +- hq_superset/utils.py | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/hq_superset/hq_domain.py b/hq_superset/hq_domain.py index 6a82cd3..227bf56 100644 --- a/hq_superset/hq_domain.py +++ b/hq_superset/hq_domain.py @@ -8,7 +8,7 @@ SESSION_DOMAIN_ROLE_LAST_SYNCED_AT, SESSION_USER_DOMAINS_KEY, ) -from hq_superset.utils import DomainSyncUtil, datetime_utcnow_naive +from hq_superset.utils import DomainSyncUtil, datetime_utcnow def before_request_hook(): @@ -90,7 +90,7 @@ def _domain_role_expired(): if not session.get(SESSION_DOMAIN_ROLE_LAST_SYNCED_AT): return True - time_since_last_sync = datetime_utcnow_naive() - session[SESSION_DOMAIN_ROLE_LAST_SYNCED_AT] + time_since_last_sync = datetime_utcnow() - session[SESSION_DOMAIN_ROLE_LAST_SYNCED_AT] return time_since_last_sync >= timedelta(minutes=USER_DOMAIN_ROLE_EXPIRY) diff --git a/hq_superset/tests/test_utils.py b/hq_superset/tests/test_utils.py index 11f29a3..8ca1d23 100644 --- a/hq_superset/tests/test_utils.py +++ b/hq_superset/tests/test_utils.py @@ -105,7 +105,7 @@ def test_permissions_change_updates_user_role(self, get_domain_access_mock, doma additional_roles = DomainSyncUtil(security_manager)._get_additional_user_roles("test-domain") assert not additional_roles - @patch('hq_superset.utils.datetime_utcnow_naive') + @patch('hq_superset.utils.datetime_utcnow') @patch.object(DomainSyncUtil, "_get_domain_access") def test_sync_domain_role(self, get_domain_access_mock, utcnow_mock): client = self.app.test_client() diff --git a/hq_superset/utils.py b/hq_superset/utils.py index 90f78e7..a8d4641 100644 --- a/hq_superset/utils.py +++ b/hq_superset/utils.py @@ -150,7 +150,7 @@ def sync_domain_role(self, domain): self.sm.get_session.add(current_user) self.sm.get_session.commit() - session[SESSION_DOMAIN_ROLE_LAST_SYNCED_AT] = datetime_utcnow_naive() + session[SESSION_DOMAIN_ROLE_LAST_SYNCED_AT] = datetime_utcnow() return True def _ensure_hq_user_role(self): @@ -417,5 +417,5 @@ def generate_secret(): return ''.join(secrets.choice(alphabet) for __ in range(64)) -def datetime_utcnow_naive(): +def datetime_utcnow(): return datetime.utcnow().replace(tzinfo=pytz.UTC) From a1959294970440ed801501d651b7e9a64da8d255 Mon Sep 17 00:00:00 2001 From: mkangia Date: Tue, 5 Nov 2024 00:59:33 +0530 Subject: [PATCH 13/16] return error message if syncing fails --- hq_superset/hq_domain.py | 15 ++++++++++++--- hq_superset/tests/test_views.py | 15 +++++++++++++-- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/hq_superset/hq_domain.py b/hq_superset/hq_domain.py index 227bf56..126da48 100644 --- a/hq_superset/hq_domain.py +++ b/hq_superset/hq_domain.py @@ -1,7 +1,7 @@ from datetime import timedelta import superset -from flask import flash, g, redirect, request, session, url_for +from flask import current_app, flash, g, redirect, request, session, url_for from superset.config import USER_DOMAIN_ROLE_EXPIRY from hq_superset.const import ( @@ -83,7 +83,7 @@ def sync_user_domain_role(): ): return if _domain_role_expired(): - _sync_domain_role() + return _sync_domain_role() def _domain_role_expired(): @@ -95,7 +95,16 @@ def _domain_role_expired(): def _sync_domain_role(): - DomainSyncUtil(superset.appbuilder.sm).sync_domain_role(g.hq_domain) + if not DomainSyncUtil(superset.appbuilder.sm).sync_domain_role(g.hq_domain): + error_message = ( + f"We couldn't refresh your permissions to access the domain '{g.hq_domain}'. " + f"Please select the project space again or login again to resolve. " + f"If issue persists, please submit a support request." + ) + return current_app.response_class( + response=error_message, + status=400, + ) def is_valid_user_domain(hq_domain): diff --git a/hq_superset/tests/test_views.py b/hq_superset/tests/test_views.py index 0b0382d..1fb681a 100644 --- a/hq_superset/tests/test_views.py +++ b/hq_superset/tests/test_views.py @@ -316,7 +316,7 @@ def _test_upload(test_data, expected_output): self.assert_context('ucr_id_to_pks', {}) @patch('hq_superset.hq_requests.get_valid_cchq_oauth_token', return_value={}) - @patch('hq_superset.hq_domain._sync_domain_role', return_value=True) + @patch('hq_superset.hq_domain._sync_domain_role', return_value=None) def test_sync_user_domain_role_calls(self, sync_domain_role_mock, *args): with self.app.test_client() as client: assert SESSION_USER_DOMAINS_KEY not in session @@ -366,11 +366,22 @@ def test_sync_user_domain_before_request(self, *args): self.assertNotEqual(session_role_last_synced_at, session.get(SESSION_DOMAIN_ROLE_LAST_SYNCED_AT)) with patch.object(DomainSyncUtil, 'sync_domain_role') as domain_sync_util_mock: - client.get('/hq_datasource/list/', follow_redirects=True) + # in case of failure to sync + domain_sync_util_mock.return_value = False + response = client.get('/hq_datasource/list/', follow_redirects=True) + # confirm function call for sync domain_sync_util_mock.assert_called_once_with( "test1" ) + self.assertEqual( + response.text, + "We couldn't refresh your permissions to access the domain 'test1'. " + "Please select the project space again or login again to resolve. " + "If issue persists, please submit a support request." + ) + self.assertEqual(response.status_code, 400) + self.logout(client) From b4eb1b3b1745278ea700f8fa5c98bf9109fa09b2 Mon Sep 17 00:00:00 2001 From: mkangia Date: Tue, 5 Nov 2024 01:00:11 +0530 Subject: [PATCH 14/16] add log requests to domain excluded views to ignore in hooks --- hq_superset/hq_domain.py | 1 + 1 file changed, 1 insertion(+) diff --git a/hq_superset/hq_domain.py b/hq_superset/hq_domain.py index 126da48..1a08b96 100644 --- a/hq_superset/hq_domain.py +++ b/hq_superset/hq_domain.py @@ -45,6 +45,7 @@ def after_request_hook(response): 'AuthOAuthView.logout', 'AuthOAuthView.oauth_authorized', 'CurrentUserRestApi.get_me', + 'Superset.log', 'DataSetChangeAPI.post_dataset_change', 'OAuth.issue_access_token', 'SelectDomainView.list', From a5f2c58adc7f1ba107b6777ea0b77d0f07ce3e52 Mon Sep 17 00:00:00 2001 From: mkangia Date: Tue, 5 Nov 2024 01:12:49 +0530 Subject: [PATCH 15/16] avoid sync conflicts --- hq_superset/hq_domain.py | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/hq_superset/hq_domain.py b/hq_superset/hq_domain.py index 1a08b96..bb449e4 100644 --- a/hq_superset/hq_domain.py +++ b/hq_superset/hq_domain.py @@ -3,6 +3,7 @@ import superset from flask import current_app, flash, g, redirect, request, session, url_for from superset.config import USER_DOMAIN_ROLE_EXPIRY +from superset.extensions import cache_manager from hq_superset.const import ( SESSION_DOMAIN_ROLE_LAST_SYNCED_AT, @@ -84,7 +85,9 @@ def sync_user_domain_role(): ): return if _domain_role_expired(): - return _sync_domain_role() + # only sync if another sync not in progress + if not _sync_in_progress(): + return _perform_sync_domain_role() def _domain_role_expired(): @@ -95,6 +98,24 @@ def _domain_role_expired(): return time_since_last_sync >= timedelta(minutes=USER_DOMAIN_ROLE_EXPIRY) +def _sync_in_progress(): + return cache_manager.cache.get(_sync_domain_role_cache_key()) + + +def _sync_domain_role_cache_key(): + return f"{g.user.id}_{g.hq_domain}_sync_domain_role" + + +def _perform_sync_domain_role(): + cache_key = _sync_domain_role_cache_key() + + # set cache for 30 seconds + cache_manager.cache.set(cache_key, True, timeout=30) + sync_domain_role_response = _sync_domain_role() + cache_manager.cache.delete(cache_key) + + return sync_domain_role_response + def _sync_domain_role(): if not DomainSyncUtil(superset.appbuilder.sm).sync_domain_role(g.hq_domain): error_message = ( From da462573f0cbdb681f59c8fde597562a6bddb977 Mon Sep 17 00:00:00 2001 From: mkangia Date: Fri, 8 Nov 2024 01:47:24 +0530 Subject: [PATCH 16/16] update error message to highlight possible access revoke --- hq_superset/hq_domain.py | 7 ++++--- hq_superset/tests/test_views.py | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/hq_superset/hq_domain.py b/hq_superset/hq_domain.py index bb449e4..83f6d81 100644 --- a/hq_superset/hq_domain.py +++ b/hq_superset/hq_domain.py @@ -119,9 +119,10 @@ def _perform_sync_domain_role(): def _sync_domain_role(): if not DomainSyncUtil(superset.appbuilder.sm).sync_domain_role(g.hq_domain): error_message = ( - f"We couldn't refresh your permissions to access the domain '{g.hq_domain}'. " - f"Please select the project space again or login again to resolve. " - f"If issue persists, please submit a support request." + f"Either your permissions for the project '{g.hq_domain}' were revoked or " + "your permissions failed to refresh. " + "Please select the project space again or login again to resolve. " + "If issue persists, please submit a support request." ) return current_app.response_class( response=error_message, diff --git a/hq_superset/tests/test_views.py b/hq_superset/tests/test_views.py index 1fb681a..92765b3 100644 --- a/hq_superset/tests/test_views.py +++ b/hq_superset/tests/test_views.py @@ -377,7 +377,8 @@ def test_sync_user_domain_before_request(self, *args): self.assertEqual( response.text, - "We couldn't refresh your permissions to access the domain 'test1'. " + "Either your permissions for the project 'test1' were revoked or " + "your permissions failed to refresh. " "Please select the project space again or login again to resolve. " "If issue persists, please submit a support request." )