Issue explorerhq#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

@@ -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.
+settings.py to enable these features.

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
 

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(

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}), '

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()

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

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),
+    ]

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)

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)
 
 

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()

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']

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

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 (

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]))