Merge pull request #95 from lsst-sqre/tickets/DM-34097

[DM-34097] Fix database passwords with special characters

Author: rra

CHANGELOG.rst

@@ -5,6 +5,11 @@ Change log
 .. Headline template:
    X.Y.Z (YYYY-MM-DD)
 
+3.0.2 (2022-03-25)
+==================
+
+- Fix handling of passwords containing special characters such as ``@`` and ``/`` in ``safir.database``.
+
 3.0.1 (2022-02-24)
 ==================
 

src/safir/database.py

@@ -6,7 +6,7 @@
 import time
 from datetime import datetime, timezone
 from typing import TYPE_CHECKING, Optional, overload
-from urllib.parse import urlparse
+from urllib.parse import quote, urlparse
 
 from sqlalchemy import create_engine
 from sqlalchemy.exc import OperationalError
@@ -63,12 +63,24 @@ def _build_database_url(
     -------
     url : `str`
         The URL including the password.
+
+    Raises
+    ------
+    ValueError
+        A password was provided but the connection URL has no username.
     """
     if is_async or password:
         parsed_url = urlparse(url)
         if is_async and parsed_url.scheme == "postgresql":
             parsed_url = parsed_url._replace(scheme="postgresql+asyncpg")
         if password:
+            if not parsed_url.username:
+                raise ValueError(f"No username in database URL {url}")
+            password = quote(password, safe="")
+
+            # The username portion of the parsed URL does not appear to decode
+            # URL escaping of the username, so we should not quote it again or
+            # we will get double-quoting.
             netloc = f"{parsed_url.username}:{password}@{parsed_url.hostname}"
             if parsed_url.port:
                 netloc = f"{netloc}:{parsed_url.port}"
@@ -138,6 +150,11 @@ def create_database_engine(
     engine : `sqlalchemy.ext.asyncio.AsyncEngine`
         Newly-created database engine.  When done with the engine, the caller
         must call ``await engine.dispose()``.
+
+    Raises
+    ------
+    ValueError
+        A password was provided but the connection URL has no username.
     """
     url = _build_database_url(url, password, is_async=True)
     if isolation_level:
@@ -244,6 +261,11 @@ def create_sync_session(
     session : `sqlalchemy.orm.scoping.scoped_session`
         The database session proxy.  This manages a separate session per
         thread and therefore should be thread-safe.
+
+    Raises
+    ------
+    ValueError
+        A password was provided but the connection URL has no username.
     """
     url = _build_database_url(url, password, is_async=False)
     if isolation_level:

tests/database_test.py

@@ -4,6 +4,7 @@
 
 import os
 from datetime import datetime, timedelta, timezone
+from urllib.parse import unquote, urlparse
 
 import pytest
 import structlog
@@ -93,6 +94,26 @@ def test_build_database_url() -> None:
     )
     assert url == "postgresql+asyncpg://foo:otherpass@127.0.0.1:5433/foo"
 
+    # Test that the username and password are quoted properly.
+    url = _build_database_url(
+        "postgresql://foo%40e.com@127.0.0.1:4444/foo",
+        "pass@word/with stuff",
+        is_async=False,
+    )
+    assert url == (
+        "postgresql://foo%40e.com:pass%40word%2Fwith%20stuff@127.0.0.1:4444"
+        "/foo"
+    )
+    parsed_url = urlparse(url)
+    assert parsed_url.username
+    assert parsed_url.password
+
+    # urlparse does not undo quoting in the components of netloc.
+    assert unquote(parsed_url.username) == "foo@e.com"
+    assert unquote(parsed_url.password) == "pass@word/with stuff"
+    assert parsed_url.hostname == "127.0.0.1"
+    assert parsed_url.port == 4444
+
 
 @pytest.mark.asyncio
 async def test_create_async_session() -> None:

tox.ini

@@ -7,7 +7,7 @@ image = postgres:latest
 ports =
     5432:5432/tcp
 environment =
-    POSTGRES_PASSWORD=INSECURE-PASSWORD
+    POSTGRES_PASSWORD=INSECURE@%PASSWORD/
     POSTGRES_USER=safir
     POSTGRES_DB=safir
     PGPORT=5432
@@ -37,7 +37,7 @@ commands =
     coverage run -m pytest {posargs}
 setenv =
     TEST_DATABASE_URL = postgresql://safir@127.0.0.1/safir
-    TEST_DATABASE_PASSWORD = INSECURE-PASSWORD
+    TEST_DATABASE_PASSWORD = INSECURE@%PASSWORD/
 
 [testenv:coverage-report]
 description = Compile coverage from each test run.