launchpad-reviewers team mailing list archive

[Colin Watson <mp+431520@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:deprecate-cookie-domains into launchpad:master.

Commit message:
Deprecate config.launchpad.cookie_domains

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

`config.launchpad.cookie_domains` didn't really make much sense.  The default was set to "demo.launchpad.net, staging.launchpad.net, launchpad.net, launchpad.test", meaning that Launchpad sent cookies that it would otherwise have sent to subdomains of each of those domains to the parent domain instead.  Since only the dogfood configuration overrode this default, this meant that Launchpad was effectively being preconfigured for all the domains to which it's deployed, except oddly omitting "qastaging.launchpad.net".

But the only domains that matter here are ones to which the current Launchpad instance will actually try to send cookies, and those will all be `config.vhost.mainsite.hostname` or its subdomains.  Therefore, only check `config.vhost.mainsite.hostname` and deprecate the old configuration item, to be removed once no production configs refer to it any more.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:deprecate-cookie-domains into launchpad:master.

diff --git a/lib/lp/app/tests/test_tales.py b/lib/lp/app/tests/test_tales.py
index c7a8142..2520a12 100644
--- a/lib/lp/app/tests/test_tales.py
+++ b/lib/lp/app/tests/test_tales.py
@@ -72,7 +72,8 @@ def test_cookie_scope():
     The 'request/lp:cookie_scope' TALES expression returns a string
     that represents the scope parameters necessary for a cookie to be
     available for the entire Launchpad site.  It takes into account
-    the request URL and the cookie_domains setting in launchpad-lazr.conf.
+    the request URL and the vhost.mainsite.hostname setting in
+    launchpad-lazr.conf.
 
         >>> from lp.app.browser.tales import RequestAPI
         >>> def cookie_scope(url):
diff --git a/lib/lp/services/config/schema-lazr.conf b/lib/lp/services/config/schema-lazr.conf
index 86aeddd..76ce01b 100644
--- a/lib/lp/services/config/schema-lazr.conf
+++ b/lib/lp/services/config/schema-lazr.conf
@@ -1025,6 +1025,7 @@ ca_certificates_path: none
 # domains listed here will be sent to the parent domain,
 # allowing sessions to be shared between vhosts.
 # Domains should not start with a leading '.'.
+# Deprecated; Launchpad now only considers vhost.mainsite.hostname.
 cookie_domains:
     demo.launchpad.net, staging.launchpad.net, launchpad.net, launchpad.test
 
diff --git a/lib/lp/services/webapp/session.py b/lib/lp/services/webapp/session.py
index c44675f..64ca6ec 100644
--- a/lib/lp/services/webapp/session.py
+++ b/lib/lp/services/webapp/session.py
@@ -92,16 +92,13 @@ def get_cookie_domain(request_domain):
     all virtual hosts of the Launchpad instance.  If no matching
     domain is known, None is returned.
     """
-    cookie_domains = [
-        v.strip() for v in config.launchpad.cookie_domains.split(",")
-    ]
-    for domain in cookie_domains:
-        assert not domain.startswith("."), "domain should not start with '.'"
-        dotted_domain = "." + domain
-        if domain_match(request_domain, domain) or domain_match(
-            request_domain, dotted_domain
-        ):
-            return dotted_domain
+    domain = config.vhost.mainsite.hostname
+    assert not domain.startswith("."), "domain should not start with '.'"
+    dotted_domain = "." + domain
+    if domain_match(request_domain, domain) or domain_match(
+        request_domain, dotted_domain
+    ):
+        return dotted_domain
     return None
 
 
diff --git a/lib/lp/services/webapp/tests/test_session.py b/lib/lp/services/webapp/tests/test_session.py
index 032ec40..31cd9b9 100644
--- a/lib/lp/services/webapp/tests/test_session.py
+++ b/lib/lp/services/webapp/tests/test_session.py
@@ -24,42 +24,44 @@ class GetCookieDomainTestCase(TestCase):
     def test_base_domain(self):
         # Test that the base Launchpad domain gives a domain parameter
         # that is visible to the virtual hosts.
-        self.assertEqual(get_cookie_domain("launchpad.net"), ".launchpad.net")
+        self.pushConfig("vhost.mainsite", hostname="launchpad.net")
+        self.assertEqual(".launchpad.net", get_cookie_domain("launchpad.net"))
 
     def test_vhost_domain(self):
         # Test Launchpad subdomains give the same domain parameter
+        self.pushConfig("vhost.mainsite", hostname="launchpad.net")
         self.assertEqual(
-            get_cookie_domain("bugs.launchpad.net"), ".launchpad.net"
+            ".launchpad.net", get_cookie_domain("bugs.launchpad.net")
         )
 
     def test_other_domain(self):
         # Other domains do not return a cookie domain.
-        self.assertEqual(get_cookie_domain("example.com"), None)
+        self.pushConfig("vhost.mainsite", hostname="launchpad.net")
+        self.assertIsNone(get_cookie_domain("example.com"))
 
-    def test_other_instances(self):
-        # Test that requests to other launchpad instances are scoped right
+    def test_staging(self):
+        # Requests to Launchpad staging are scoped correctly.
+        self.pushConfig("vhost.mainsite", hostname="staging.launchpad.net")
         self.assertEqual(
-            get_cookie_domain("demo.launchpad.net"), ".demo.launchpad.net"
-        )
-        self.assertEqual(
-            get_cookie_domain("bugs.demo.launchpad.net"), ".demo.launchpad.net"
-        )
-
-        self.assertEqual(
-            get_cookie_domain("staging.launchpad.net"),
             ".staging.launchpad.net",
+            get_cookie_domain("staging.launchpad.net"),
         )
         self.assertEqual(
-            get_cookie_domain("bugs.staging.launchpad.net"),
             ".staging.launchpad.net",
+            get_cookie_domain("bugs.staging.launchpad.net"),
         )
+        self.assertIsNone(get_cookie_domain("launchpad.net"))
 
+    def test_development(self):
+        # Requests to a development server are scoped correctly.
+        self.pushConfig("vhost.mainsite", hostname="launchpad.test")
         self.assertEqual(
-            get_cookie_domain("launchpad.test"), ".launchpad.test"
+            ".launchpad.test", get_cookie_domain("launchpad.test")
         )
         self.assertEqual(
-            get_cookie_domain("bugs.launchpad.test"), ".launchpad.test"
+            ".launchpad.test", get_cookie_domain("bugs.launchpad.test")
         )
+        self.assertIsNone(get_cookie_domain("launchpad.net"))
 
 
 class TestLaunchpadCookieClientIdManager(TestCase):