launchpad-reviewers team mailing list archive

[Colin Watson <mp+438523@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:librarian-public-files-on-restricted-host into launchpad:master.

Commit message:
Allow serving public files from the restricted librarian

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/438523

This is generally harmless (as opposed to the other way round), and it makes it substantially easier to build a caching configuration for the upcoming archive snapshot service.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:librarian-public-files-on-restricted-host into launchpad:master.

diff --git a/lib/lp/services/librarianserver/db.py b/lib/lp/services/librarianserver/db.py
index f138c14..ce703e6 100644
--- a/lib/lp/services/librarianserver/db.py
+++ b/lib/lp/services/librarianserver/db.py
@@ -12,7 +12,7 @@ from urllib.parse import quote, unquote
 from xmlrpc.client import Fault
 
 from pymacaroons import Macaroon
-from storm.expr import SQL, And
+from storm.expr import SQL
 from twisted.internet import defer
 from twisted.internet import reactor as default_reactor
 from twisted.internet import threads
@@ -21,6 +21,7 @@ from twisted.web import xmlrpc
 from lp.services.config import config
 from lp.services.database.interfaces import IStore
 from lp.services.database.sqlbase import session_store
+from lp.services.database.stormexpr import IsFalse
 from lp.services.librarian.model import (
     LibraryFileAlias,
     LibraryFileContent,
@@ -128,13 +129,13 @@ class Library:
                 restricted = True
             else:
                 raise LookupError("Token stale/pruned/path mismatch")
-        alias = LibraryFileAlias.selectOne(
-            And(
-                LibraryFileAlias.id == aliasid,
-                LibraryFileAlias.contentID == LibraryFileContent.q.id,
-                LibraryFileAlias.restricted == restricted,
-            )
-        )
+        clauses = [
+            LibraryFileAlias.id == aliasid,
+            LibraryFileAlias.content == LibraryFileContent.id,
+        ]
+        if not restricted:
+            clauses.append(IsFalse(LibraryFileAlias.restricted))
+        alias = IStore(LibraryFileAlias).find(LibraryFileAlias, *clauses).one()
         if alias is None:
             raise LookupError("No file alias with LibraryFileContent")
         return alias
diff --git a/lib/lp/services/librarianserver/tests/test_db.py b/lib/lp/services/librarianserver/tests/test_db.py
index ce3d499..d892613 100644
--- a/lib/lp/services/librarianserver/tests/test_db.py
+++ b/lib/lp/services/librarianserver/tests/test_db.py
@@ -108,7 +108,7 @@ class TestLibrarianStuff(TestCase):
 
     def test_getAlias(self):
         # Library.getAlias() returns the LibrarayFileAlias for a given
-        # LibrarayFileAlias ID.
+        # LibraryFileAlias ID.
         library = db.Library(restricted=False)
         alias = library.getAlias(1, None, "/")
         self.assertEqual(1, alias.id)
@@ -135,15 +135,16 @@ class TestLibrarianStuff(TestCase):
         alias.content = None
         self.assertRaises(LookupError, library.getAlias, 1, None, "/")
 
-    def test_getAlias_content_wrong_library(self):
-        # Library.getAlias() raises a LookupError, if a restricted
-        # library looks up a unrestricted LibraryFileAlias and
-        # vice versa.
+    def test_getAlias_restricted_library_unrestricted_alias(self):
+        # Library.getAlias() allows looking up unrestricted
+        # LibraryFileAliases from a restricted library.
         restricted_library = db.Library(restricted=True)
-        self.assertRaises(
-            LookupError, restricted_library.getAlias, 1, None, "/"
-        )
+        alias = restricted_library.getAlias(1, None, "/")
+        self.assertEqual(1, alias.id)
 
+    def test_getAlias_unrestricted_library_restricted_alias(self):
+        # Library.getAlias() raises a LookupError if an unrestricted
+        # library looks up a restricted LibraryFileAlias.
         unrestricted_library = db.Library(restricted=False)
         alias = unrestricted_library.getAlias(1, None, "/")
         alias.restricted = True