From 0bd3f1bc464e0373823adc0c445ed6fc81d6a313 Mon Sep 17 00:00:00 2001 From: Arne Claassen Date: Sat, 21 Aug 2021 16:42:39 -0700 Subject: [PATCH 1/2] Issue #456: Use of Django connection name rather than alias as lookup This commit changes connection behavior from storing the database connection name to using the database alias mapped by SQL Explorer instead. The reason for this change is two-fold: 1) Views take the connection name as input, allowing anyone who knows Django connection names to query those databases, even if SQL does not expose the connection directly. 2) `Query` stores the connection name, which means that if the Django connection name changes or a different connection should be used (for example, one with reduced permissions) the stored Query will either stop working or at least continue using the old connection This change modifies `ExplorerConnections` from being a dictionary that proxies the Django connection dictionary to a dictionary-like object that uses `EXPLORER_CONNECTIONS` to lookup and validate the requested connection alias. In addition all code that used to the `EXPLORER_CONNECTIONS` value now uses the key instead. For backwards compatibility, a migration will back-populate the alias into `Query` instances (and fail if the mapping no longer exists), `EXPLORER_DEFAULT_CONNECTION` is re-written on start-up to use the alias in case it still uses the Django Connection name and `ExplorerConnections` will still accept a Django Connection name as long as that name is exposed by some alias in `EXPLORER_CONNECTIONS`. --- docs/install.rst | 10 +-- docs/settings.rst | 7 +- explorer/app_settings.py | 2 +- explorer/apps.py | 36 +++++----- explorer/connections.py | 67 ++++++++++++++----- explorer/forms.py | 2 +- .../0011_query_connection_rewrite.py | 49 ++++++++++++++ explorer/schema.py | 2 +- explorer/tests/factories.py | 2 +- explorer/tests/test_apps.py | 45 ++++++++++--- explorer/tests/test_connections.py | 43 ++++++++++++ explorer/tests/test_exporters.py | 2 +- explorer/tests/test_models.py | 2 +- explorer/tests/test_utils.py | 8 --- explorer/tests/test_views.py | 9 ++- explorer/utils.py | 5 -- explorer/views/schema.py | 8 ++- test_project/settings.py | 2 +- 18 files changed, 231 insertions(+), 70 deletions(-) create mode 100644 explorer/migrations/0011_query_connection_rewrite.py create mode 100644 explorer/tests/test_connections.py diff --git a/docs/install.rst b/docs/install.rst index 805694be..0378c459 100644 --- a/docs/install.rst +++ b/docs/install.rst @@ -56,13 +56,13 @@ Configure your settings to something like: .. code-block:: python EXPLORER_CONNECTIONS = { 'Default': 'readonly' } - EXPLORER_DEFAULT_CONNECTION = 'readonly' + EXPLORER_DEFAULT_CONNECTION = 'Default' The first setting lists the connections you want to allow Explorer to use. The keys of the connections dictionary are friendly names to show Explorer users, and the values are the actual database aliases used in ``settings.DATABASES``. It is highly recommended to setup read-only roles -in your database, add them in your project's ``DATABASES`` setting and +in your database, add them in your project's ``DATABASES`` setting and use these read-only cconnections in the ``EXPLORER_CONNECTIONS``. If you want to quickly use django-sql-explorer with the existing default @@ -72,15 +72,15 @@ can use the following settings: .. code-block:: python EXPLORER_CONNECTIONS = { 'Default': 'default' } - EXPLORER_DEFAULT_CONNECTION = 'default' + EXPLORER_DEFAULT_CONNECTION = 'Default' Finally, run migrate to create the tables: ``python manage.py migrate`` -You can now browse to https://yoursite/explorer/ and get exploring! +You can now browse to https://yoursite/explorer/ and get exploring! There are a handful of features (snapshots, emailing queries) that rely on Celery and the dependencies in optional-requirements.txt. If you have Celery installed, set ``EXPLORER_TASKS_ENABLED=True`` in your -settings.py to enable these features. \ No newline at end of file +settings.py to enable these features. diff --git a/docs/settings.rst b/docs/settings.rst index d480f1bc..7826a96e 100644 --- a/docs/settings.rst +++ b/docs/settings.rst @@ -100,7 +100,12 @@ Generate DB schema asynchronously. Requires Celery and ``EXPLORER_TASKS_ENABLED` Default connection ****************** -The name of the Django database connection to use. Ideally set this to a connection with read only permissions +The ``Friendly Name`` connection alias from ``EXPLORER_CONNECTIONS`` for the database connection to use. Ideally set +this to a connection alias with read only permissions. + +Note: This used to use the Django connection name rather than the alias name. While the former will still work, +it is a deprecated usage and configurations should be updated to guard against removal of the deprecated +behavior. .. code-block:: python diff --git a/explorer/app_settings.py b/explorer/app_settings.py index bc016405..27cc982a 100644 --- a/explorer/app_settings.py +++ b/explorer/app_settings.py @@ -7,7 +7,7 @@ # 'Original Database': 'my_important_database_readonly_connection', # 'Client Database 2': 'other_database_connection' # } -# EXPLORER_DEFAULT_CONNECTION = 'my_important_database_readonly_connection' +# EXPLORER_DEFAULT_CONNECTION = 'Original Database' EXPLORER_CONNECTIONS = getattr(settings, 'EXPLORER_CONNECTIONS', {}) EXPLORER_DEFAULT_CONNECTION = getattr( diff --git a/explorer/apps.py b/explorer/apps.py index eaf7bee5..c49bf5b7 100644 --- a/explorer/apps.py +++ b/explorer/apps.py @@ -5,7 +5,6 @@ class ExplorerAppConfig(AppConfig): - name = 'explorer' verbose_name = _('SQL Explorer') @@ -15,27 +14,30 @@ def ready(self): build_async_schemas() -def _get_default(): - from explorer.app_settings import EXPLORER_DEFAULT_CONNECTION - return EXPLORER_DEFAULT_CONNECTION - - -def _get_explorer_connections(): - from explorer.app_settings import EXPLORER_CONNECTIONS - return EXPLORER_CONNECTIONS +def _get_app_settings(): + from explorer import app_settings + return app_settings def _validate_connections(): + app_settings = _get_app_settings() # Validate connections - if _get_default() not in _get_explorer_connections().values(): - raise ImproperlyConfigured( - f'EXPLORER_DEFAULT_CONNECTION is {_get_default()}, ' - f'but that alias is not present in the values of ' - f'EXPLORER_CONNECTIONS' - ) - - for name, conn_name in _get_explorer_connections().items(): + if app_settings.EXPLORER_DEFAULT_CONNECTION not in app_settings.EXPLORER_CONNECTIONS.keys(): + if app_settings.EXPLORER_DEFAULT_CONNECTION in app_settings.EXPLORER_CONNECTIONS.values(): + # fix-up default for any setup still using the django DB name + # rather than the alias name + for k, v in app_settings.EXPLORER_CONNECTIONS.items(): + if v == app_settings.EXPLORER_DEFAULT_CONNECTION: + app_settings.EXPLORER_DEFAULT_CONNECTION = k + break + else: + raise ImproperlyConfigured( + f'EXPLORER_DEFAULT_CONNECTION is {app_settings.EXPLORER_DEFAULT_CONNECTION}, ' + f'but that alias is not present in the values of EXPLORER_CONNECTIONS' + ) + + for name, conn_name in app_settings.EXPLORER_CONNECTIONS.items(): if conn_name not in djcs: raise ImproperlyConfigured( f'EXPLORER_CONNECTIONS contains ({name}, {conn_name}), ' diff --git a/explorer/connections.py b/explorer/connections.py index 03427e7a..43eacb12 100644 --- a/explorer/connections.py +++ b/explorer/connections.py @@ -1,24 +1,61 @@ +import importlib +import logging + from django.db import connections as djcs from explorer.app_settings import EXPLORER_CONNECTIONS +from explorer.utils import InvalidExplorerConnectionException - -# We export valid SQL connections here so that consuming code never has to -# deal with django.db.connections directly, and risk accessing a connection -# that hasn't been registered to Explorer. - -# Django insists that connections that are created in a thread are only accessed -# by that thread, so here we create a dictionary-like collection of the valid -# connections, but does a 'live' lookup of the connection on each item access. +logger = logging.getLogger(__name__) -_connections = {c: c for c in djcs if c in EXPLORER_CONNECTIONS.values()} +class ExplorerConnections: - -class ExplorerConnections(dict): + def get(self, item, default=None): + try: + return self[item] + except InvalidExplorerConnectionException: + return default def __getitem__(self, item): - return djcs[item] - - -connections = ExplorerConnections(_connections) + conn = EXPLORER_CONNECTIONS.get(item) + if not conn: + if item in djcs: + # Original connection handling did lookups by the django names not the explorer + # alias. To support stored uses of URLs accessing connections by the old name + # (such as schema), we support the django db connectin name as long as it is + # mapped by some alias in EXPLORER_CONNECTIONS, so as to prevent access to + # Django DB connections never meant to be exposed by Explorer + if item not in EXPLORER_CONNECTIONS.values(): + raise InvalidExplorerConnectionException( + f"Attempted to access connection {item} which is " + f"not a Django DB connection exposed by Explorer" + ) + logger.info(f"using legacy lookup by django connection name for '{item}'") + conn = item + else: + raise InvalidExplorerConnectionException( + f'Attempted to access connection {item}, ' + f'but that is not a registered Explorer connection.' + ) + # Django insists that connections that are created in a thread are only accessed + # by that thread, so we do a 'live' lookup of the connection on each item access. + return djcs[conn] + + def __contains__(self, item): + return item in EXPLORER_CONNECTIONS + + def __len__(self): + return len(EXPLORER_CONNECTIONS) + + def keys(self): + return EXPLORER_CONNECTIONS.keys() + + def values(self): + return [self[v] for v in EXPLORER_CONNECTIONS.values()] + + def items(self): + return [(k, self[v]) for k, v in EXPLORER_CONNECTIONS.items()] + + +connections = ExplorerConnections() diff --git a/explorer/forms.py b/explorer/forms.py index b59cec7f..9c0f0d47 100644 --- a/explorer/forms.py +++ b/explorer/forms.py @@ -62,7 +62,7 @@ def created_at_time(self): @property def connections(self): - return [(v, k) for k, v in EXPLORER_CONNECTIONS.items()] + return [(k, k) for k in EXPLORER_CONNECTIONS.keys()] class Meta: model = Query diff --git a/explorer/migrations/0011_query_connection_rewrite.py b/explorer/migrations/0011_query_connection_rewrite.py new file mode 100644 index 00000000..d449fe7f --- /dev/null +++ b/explorer/migrations/0011_query_connection_rewrite.py @@ -0,0 +1,49 @@ +from django.db import migrations + +from explorer.app_settings import EXPLORER_CONNECTIONS + + +def forward(apps, schema_editor): + Query = apps.get_model("explorer", "Query") + + reverse_map = {v: k for k, v in EXPLORER_CONNECTIONS.items()} + + for q in Query.objects.all(): + conn = q.connection + new_conn = reverse_map.get(conn) + if not new_conn: + raise Exception( + f"Query({q.id}) references Django DB connection '{conn}' " + f"which has no alias defined in EXPLORER_CONNECTIONS." + ) + if conn == new_conn: + continue + q.connection = new_conn + q.save() + + +def reverse(apps, schema_editor): + Query = apps.get_model("explorer", "Query") + + for q in Query.objects.all(): + conn = q.connection + new_conn = EXPLORER_CONNECTIONS.get(conn) + if not new_conn: + raise Exception( + f"Query({q.id}) references Connection alias '{conn}' " + f"which has no Django DB connection defined in EXPLORER_CONNECTIONS." + ) + if conn == new_conn: + continue + q.connection = new_conn + q.save() + + +class Migration(migrations.Migration): + dependencies = [ + ('explorer', '0010_sql_required'), + ] + + operations = [ + migrations.RunPython(forward, reverse), + ] diff --git a/explorer/schema.py b/explorer/schema.py index c636711d..9b26b75f 100644 --- a/explorer/schema.py +++ b/explorer/schema.py @@ -96,5 +96,5 @@ def build_schema_info(connection_alias): def build_async_schemas(): if do_async(): - for c in EXPLORER_CONNECTIONS: + for c in EXPLORER_CONNECTIONS.keys(): schema_info(c) diff --git a/explorer/tests/factories.py b/explorer/tests/factories.py index 6800674d..2572cc85 100644 --- a/explorer/tests/factories.py +++ b/explorer/tests/factories.py @@ -23,7 +23,7 @@ class Meta: title = Sequence(lambda n: f'My simple query {n}') sql = "SELECT 1+1 AS TWO" # same result in postgres and sqlite description = "Doin' math" - connection = "default" + connection = "SQLite" created_by_user = SubFactory(UserFactory) diff --git a/explorer/tests/test_apps.py b/explorer/tests/test_apps.py index fa6e835b..25117c68 100644 --- a/explorer/tests/test_apps.py +++ b/explorer/tests/test_apps.py @@ -1,20 +1,47 @@ # -*- coding: utf-8 -*- -from unittest.mock import patch +from unittest.mock import patch, Mock from django.core.exceptions import ImproperlyConfigured from django.test import TestCase +from explorer.app_settings import EXPLORER_CONNECTIONS from explorer.apps import _validate_connections class TestApps(TestCase): - @patch('explorer.apps._get_default') - def test_validates_default_connections(self, mocked_connection): - mocked_connection.return_value = 'garbage' - self.assertRaises(ImproperlyConfigured, _validate_connections) + @patch('explorer.apps._get_app_settings') + def test_validates_default_connections(self, mock_get_settings): + mock_settings = Mock() + mock_settings.EXPLORER_DEFAULT_CONNECTION = 'garbage' + mock_settings.EXPLORER_CONNECTIONS = EXPLORER_CONNECTIONS + mock_get_settings.return_value = mock_settings - @patch('explorer.apps._get_explorer_connections') - def test_validates_all_connections(self, mocked_connections): - mocked_connections.return_value = {'garbage1': 'in', 'garbage2': 'out'} - self.assertRaises(ImproperlyConfigured, _validate_connections) + with self.assertRaisesMessage( + ImproperlyConfigured, + "EXPLORER_DEFAULT_CONNECTION is garbage, but that " + "alias is not present in the values of EXPLORER_CONNECTIONS" + ): + _validate_connections() + + @patch('explorer.apps._get_app_settings') + def test_rewrites_default_connection_if_referencing_django_db_name(self, mock_get_settings): + mock_settings = Mock() + mock_settings.EXPLORER_DEFAULT_CONNECTION = 'default' + mock_settings.EXPLORER_CONNECTIONS = EXPLORER_CONNECTIONS + mock_get_settings.return_value = mock_settings + _validate_connections() + self.assertEqual("SQLite", mock_settings.EXPLORER_DEFAULT_CONNECTION) + + @patch('explorer.apps._get_app_settings') + def test_validates_all_connections(self, mock_get_settings): + mock_settings = Mock() + mock_settings.EXPLORER_DEFAULT_CONNECTION = 'garbage1' + mock_settings.EXPLORER_CONNECTIONS = {'garbage1': 'in', 'garbage2': 'out'} + mock_get_settings.return_value = mock_settings + with self.assertRaisesMessage( + ImproperlyConfigured, + "EXPLORER_CONNECTIONS contains (garbage1, in), " + "but in is not a valid Django DB connection." + ): + _validate_connections() diff --git a/explorer/tests/test_connections.py b/explorer/tests/test_connections.py new file mode 100644 index 00000000..7d4c5bb1 --- /dev/null +++ b/explorer/tests/test_connections.py @@ -0,0 +1,43 @@ +from django.test import TestCase + +from explorer.connections import connections +from explorer.app_settings import EXPLORER_DEFAULT_CONNECTION +from django.db import connections as djcs + +from explorer.utils import InvalidExplorerConnectionException + + +class TestConnections(TestCase): + + def test_only_registered_connections_are_in_connections(self): + self.assertTrue(EXPLORER_DEFAULT_CONNECTION in connections) + self.assertNotEqual(len(connections), len([c for c in djcs])) + + def test__can_check_for_connection_existence(self): + self.assertTrue("SQLite" in connections) + self.assertFalse("garbage" in connections) + + def test__keys__are_all_aliases(self): + self.assertEqual({'SQLite', 'Another'}, set(connections.keys())) + + def test__values__are_only_registered_db_connections(self): + self.assertEqual({'default', 'alt'}, {c.alias for c in connections.values()}) + + def test__can_lookup_connection_by_DJCS_name_if_registered(self): + c = connections['default'] + self.assertEqual(c, djcs['default']) + + def test__cannot_lookup_connection_by_DJCS_name_if_not_registered(self): + with self.assertRaisesMessage( + InvalidExplorerConnectionException, + "Attempted to access connection not_registered which is not a Django DB connection exposed by Explorer" + ): + _ = connections['not_registered'] + + def test__raises_on_unknown_connection_name(self): + with self.assertRaisesMessage( + InvalidExplorerConnectionException, + 'Attempted to access connection garbage, ' + 'but that is not a registered Explorer connection.' + ): + _ = connections['garbage'] diff --git a/explorer/tests/test_exporters.py b/explorer/tests/test_exporters.py index a3203491..8c704fd8 100644 --- a/explorer/tests/test_exporters.py +++ b/explorer/tests/test_exporters.py @@ -3,9 +3,9 @@ from datetime import date, datetime from django.core.serializers.json import DjangoJSONEncoder -from django.db import connections from django.test import TestCase from django.utils import timezone +from explorer.connections import connections from explorer.app_settings import EXPLORER_DEFAULT_CONNECTION as CONN from explorer.exporters import CSVExporter, JSONExporter, ExcelExporter diff --git a/explorer/tests/test_models.py b/explorer/tests/test_models.py index 6bf78bfc..bcc41dd7 100644 --- a/explorer/tests/test_models.py +++ b/explorer/tests/test_models.py @@ -1,8 +1,8 @@ # -*- coding: utf-8 -*- from unittest.mock import patch, Mock -from django.db import connections from django.test import TestCase +from explorer.connections import connections from explorer.app_settings import EXPLORER_DEFAULT_CONNECTION as CONN from explorer.models import ( diff --git a/explorer/tests/test_utils.py b/explorer/tests/test_utils.py index 9ff06ebf..54eea40b 100644 --- a/explorer/tests/test_utils.py +++ b/explorer/tests/test_utils.py @@ -133,11 +133,3 @@ def test_get_params_for_request_empty(self): self.assertEqual(get_params_for_url(q), None) -class TestConnections(TestCase): - - def test_only_registered_connections_are_in_connections(self): - from explorer.connections import connections - from explorer.app_settings import EXPLORER_DEFAULT_CONNECTION - from django.db import connections as djcs - self.assertTrue(EXPLORER_DEFAULT_CONNECTION in connections) - self.assertNotEqual(len(connections), len([c for c in djcs])) diff --git a/explorer/tests/test_views.py b/explorer/tests/test_views.py index 14b14f57..1ef0dfae 100644 --- a/explorer/tests/test_views.py +++ b/explorer/tests/test_views.py @@ -4,13 +4,14 @@ from django.contrib.auth.models import User from django.core.cache import cache -from django.db import connections from django.test import TestCase from django.urls import reverse from django.forms.models import model_to_dict from django.shortcuts import redirect +from explorer.connections import connections from explorer.app_settings import ( + EXPLORER_CONNECTIONS as CONNS, EXPLORER_DEFAULT_CONNECTION as CONN, EXPLORER_TOKEN ) @@ -615,6 +616,12 @@ def test_returns_404_if_conn_doesnt_exist(self): ) self.assertEqual(resp.status_code, 404) + def test_does_not_return_404_on_legacy_connection_lookup(self): + resp = self.client.get( + reverse("explorer_schema", kwargs={'connection': list(CONNS.values())[0]}) + ) + self.assertEqual(resp.status_code, 200) + def test_admin_required(self): self.client.logout() resp = self.client.get( diff --git a/explorer/utils.py b/explorer/utils.py index 061c07c5..12facabf 100644 --- a/explorer/utils.py +++ b/explorer/utils.py @@ -164,11 +164,6 @@ def get_valid_connection(alias=None): if not alias: return connections[app_settings.EXPLORER_DEFAULT_CONNECTION] - if alias not in connections: - raise InvalidExplorerConnectionException( - f'Attempted to access connection {alias}, ' - f'but that is not a registered Explorer connection.' - ) return connections[alias] diff --git a/explorer/views/schema.py b/explorer/views/schema.py index e1c38944..973dd649 100644 --- a/explorer/views/schema.py +++ b/explorer/views/schema.py @@ -11,7 +11,6 @@ class SchemaView(PermissionRequiredMixin, View): - permission_required = 'change_permission' @method_decorator(xframe_options_sameorigin) @@ -20,8 +19,13 @@ def dispatch(self, *args, **kwargs): def get(self, request, *args, **kwargs): connection = kwargs.get('connection', '') + if connection not in connections: - raise Http404 + # legacy fallback.. DB connections used to be accessed by their Django name + # rather than their alias. While these names won't show up as a key in + # `connections`, they will still resolve. + if connections.get(connection) is None: + raise Http404 schema = schema_info(connection) if schema: return render( diff --git a/test_project/settings.py b/test_project/settings.py index 3f322836..c4fd3a5c 100644 --- a/test_project/settings.py +++ b/test_project/settings.py @@ -39,7 +39,7 @@ 'SQLite': 'default', 'Another': 'alt' } -EXPLORER_DEFAULT_CONNECTION = 'default' +EXPLORER_DEFAULT_CONNECTION = 'SQLite' ROOT_URLCONF = 'explorer.tests.urls' From 2694d11858f10de9fd3894457709309dc1caac42 Mon Sep 17 00:00:00 2001 From: Mark Walker Date: Mon, 10 Jan 2022 12:25:07 +0000 Subject: [PATCH 2/2] Fix typo in comment --- explorer/connections.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/explorer/connections.py b/explorer/connections.py index 43eacb12..b633a17f 100644 --- a/explorer/connections.py +++ b/explorer/connections.py @@ -23,9 +23,9 @@ def __getitem__(self, item): if item in djcs: # Original connection handling did lookups by the django names not the explorer # alias. To support stored uses of URLs accessing connections by the old name - # (such as schema), we support the django db connectin name as long as it is + # (such as schema), we support the django db connection name as long as it is # mapped by some alias in EXPLORER_CONNECTIONS, so as to prevent access to - # Django DB connections never meant to be exposed by Explorer + # Django DB connections, never meant to be exposed by Explorer if item not in EXPLORER_CONNECTIONS.values(): raise InvalidExplorerConnectionException( f"Attempted to access connection {item} which is "