launchpad-reviewers team mailing list archive

[Colin Watson <mp+431688@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/lp-archive:cache-authorization into lp-archive:main.

Commit message:
Cache positive authorization responses

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/lp-archive/+git/lp-archive/+merge/431688

The common case resulting from apt operations will be lots of requests for the same archive with the same authentication parameters in relatively quick succession, so it makes sense to cache positive authorization responses for a short while to reduce database load.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/lp-archive:cache-authorization into lp-archive:main.

diff --git a/.mypy.ini b/.mypy.ini
index 07a8171..b68e791 100644
--- a/.mypy.ini
+++ b/.mypy.ini
@@ -1,5 +1,8 @@
 [mypy]
 python_version = 3.10
 
+[mypy-flask_caching.*]
+ignore_missing_imports = true
+
 [mypy-tomllib.*]
 ignore_missing_imports = true
diff --git a/lp_archive/__init__.py b/lp_archive/__init__.py
index 83c12ec..0a2e695 100644
--- a/lp_archive/__init__.py
+++ b/lp_archive/__init__.py
@@ -12,6 +12,7 @@ from typing import Any
 from flask import Flask
 
 from lp_archive import archive, root, routing
+from lp_archive.cache import cache
 
 
 def create_app(test_config: dict[str, Any] | None = None) -> Flask:
@@ -24,4 +25,5 @@ def create_app(test_config: dict[str, Any] | None = None) -> Flask:
     app.url_map.converters["archive"] = routing.ArchiveConverter
     app.register_blueprint(root.bp)
     app.register_blueprint(archive.bp)
+    cache.init_app(app)
     return app
diff --git a/lp_archive/archive.py b/lp_archive/archive.py
index cf38f7a..e3be81a 100644
--- a/lp_archive/archive.py
+++ b/lp_archive/archive.py
@@ -10,6 +10,8 @@ from werkzeug.datastructures import WWWAuthenticate
 from werkzeug.exceptions import Unauthorized
 from werkzeug.wrappers import Response
 
+from lp_archive.cache import cache
+
 bp = Blueprint("archive", __name__)
 
 
@@ -23,6 +25,23 @@ def get_archive_proxy() -> ServerProxy:
     return archive_proxy
 
 
+@cache.memoize(timeout=60)
+def check_archive_auth_token(
+    archive: str, username: str | None, password: str | None
+) -> bool:
+    """Return True if a username and password allow access to an archive.
+
+    If unauthorized, raises an `xmlrpc.client.Fault`.
+
+    Positive responses are cached for 60 seconds.
+    """
+    get_archive_proxy().checkArchiveAuthToken(archive, username, password)
+    # The slightly odd "return True or raise an exception" signature of this
+    # function is because Flask-Caching doesn't cache return values of None
+    # by default, and doesn't recommend doing so.
+    return True
+
+
 def check_auth(archive: str) -> None:
     """Check whether the current request may access an archive.
 
@@ -42,9 +61,7 @@ def check_auth(archive: str) -> None:
         password = request.authorization.password
         log_prefix = f"{username}@{archive}"
     try:
-        # XXX cjwatson 2022-10-14: We should cache positive responses (maybe
-        # using `flask-caching`) for a while to reduce database load.
-        get_archive_proxy().checkArchiveAuthToken(archive, username, password)
+        check_archive_auth_token(archive, username, password)
     except Fault as e:
         if e.faultCode == 410:  # Unauthorized
             current_app.logger.info("%s: Password does not match.", log_prefix)
diff --git a/lp_archive/cache.py b/lp_archive/cache.py
new file mode 100644
index 0000000..213e91c
--- /dev/null
+++ b/lp_archive/cache.py
@@ -0,0 +1,8 @@
+# Copyright 2022 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Caching utilities."""
+
+from flask_caching import Cache
+
+cache = Cache()
diff --git a/requirements.in b/requirements.in
index da656c4..74d902b 100644
--- a/requirements.in
+++ b/requirements.in
@@ -1,2 +1,3 @@
 Flask
+Flask-Caching
 tomli; python_version < "3.11"
diff --git a/requirements.txt b/requirements.txt
index 73d76c5..2330d25 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -4,9 +4,15 @@
 #
 #    pip-compile
 #
+cachelib==0.9.0
+    # via flask-caching
 click==8.1.3
     # via flask
 flask==2.2.2
+    # via
+    #   -r requirements.in
+    #   flask-caching
+flask-caching==2.0.1
     # via -r requirements.in
 itsdangerous==2.1.2
     # via flask
diff --git a/setup.cfg b/setup.cfg
index f54019b..00dc7cd 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -18,6 +18,7 @@ classifiers =
 packages = find:
 install_requires =
     Flask
+    Flask-Caching
     tomli;python_version < "3.11"
 python_requires = >=3.10
 
diff --git a/tests/conftest.py b/tests/conftest.py
index 2006629..31e1267 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -8,7 +8,7 @@ from lp_archive import create_app
 
 @pytest.fixture()
 def app():
-    app = create_app({"TESTING": True})
+    app = create_app({"CACHE_TYPE": "SimpleCache", "TESTING": True})
     yield app
 
 
diff --git a/tests/test_archive.py b/tests/test_archive.py
index 22bd8e1..7bdfccb 100644
--- a/tests/test_archive.py
+++ b/tests/test_archive.py
@@ -8,6 +8,8 @@ from xmlrpc.server import SimpleXMLRPCServer
 
 import pytest
 
+from lp_archive.cache import cache
+
 
 class ArchiveXMLRPCServer(SimpleXMLRPCServer):
 
@@ -53,6 +55,7 @@ def archive_proxy(app):
         yield server
         server.shutdown()
         thread.join()
+    cache.clear()
 
 
 def test_auth_failed(client, archive_proxy):
@@ -86,6 +89,45 @@ def test_auth_not_found(client, archive_proxy):
     ]
 
 
+def test_auth_positive_cached(client, archive_proxy):
+    response = client.get(
+        "/~user/ubuntu/authorized/dists/focal/InRelease",
+        auth=("user", "secret"),
+    )
+    assert response.status_code == 307
+    assert archive_proxy.call_log == [
+        ("checkArchiveAuthToken", "~user/ubuntu/authorized", "user", "secret"),
+        ("translatePath", "~user/ubuntu/authorized", "dists/focal/InRelease"),
+    ]
+    archive_proxy.call_log = []
+    response = client.get(
+        "/~user/ubuntu/authorized/dists/focal/InRelease",
+        auth=("user", "secret"),
+    )
+    assert response.status_code == 307
+    assert archive_proxy.call_log == [
+        ("translatePath", "~user/ubuntu/authorized", "dists/focal/InRelease"),
+    ]
+
+
+def test_auth_negative_not_cached(client, archive_proxy):
+    response = client.get(
+        "/~user/ubuntu/private/dists/focal/InRelease", auth=("user", "secret")
+    )
+    assert response.status_code == 401
+    assert archive_proxy.call_log == [
+        ("checkArchiveAuthToken", "~user/ubuntu/private", "user", "secret"),
+    ]
+    archive_proxy.call_log = []
+    response = client.get(
+        "/~user/ubuntu/private/dists/focal/InRelease", auth=("user", "secret")
+    )
+    assert response.status_code == 401
+    assert archive_proxy.call_log == [
+        ("checkArchiveAuthToken", "~user/ubuntu/private", "user", "secret"),
+    ]
+
+
 def test_translate(client, archive_proxy):
     response = client.get("/ubuntu/dists/focal/InRelease")
     assert response.status_code == 307
diff --git a/tests/test_factory.py b/tests/test_factory.py
index 54275b9..aaa9d81 100644
--- a/tests/test_factory.py
+++ b/tests/test_factory.py
@@ -2,6 +2,7 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 import os
+import warnings
 from pathlib import Path
 from tempfile import TemporaryDirectory
 
@@ -14,10 +15,12 @@ def test_config_from_file():
         try:
             os.chdir(empty)
             Path("config.toml").touch()
-            assert not create_app().testing
+            with warnings.catch_warnings():
+                warnings.filterwarnings("ignore", module="flask_caching")
+                assert not create_app().testing
         finally:
             os.chdir(old_cwd)
 
 
 def test_config_for_testing():
-    assert create_app({"TESTING": True}).testing
+    assert create_app({"CACHE_TYPE": "SimpleCache", "TESTING": True}).testing