Merge lp:~canonical-isd-hackers/canonical-identity-provider/return_to_match into lp:~canonical-isd-hackers/canonical-identity-provider/stable

[David Owen]

Changed the way in which trust_root is found by logout page.

Changed to match the trust_root as a prefix of return_to.

[Łukasz Czyżykowski (lukasz-czyzykowski)]

I can't think of better way of writing that query. Approving.

review: Approve

[Łukasz Czyżykowski (lukasz-czyzykowski)]

ok

review: Approve

=== modified file 'identityprovider/models/openidmodels.py'
--- identityprovider/models/openidmodels.py	2010-07-28 14:25:25 +0000
+++ identityprovider/models/openidmodels.py	2010-09-29 13:19:43 +0000
@@ -109,6 +109,38 @@
         unique_together = ('server_url', 'timestamp', 'salt')
 
 
+class OpenIDRPConfigManager(models.Manager):
+
+    def for_url(self, url):
+        if url.endswith('/'):
+            url = url[:-1]
+        # We need to encode the % character in URLs, so that an
+        # attacker can't maliciously take advantage of it as a
+        # wildcard.  To do this, we select an escape character (~),
+        # and then escape it as ~T and % as ~P.
+        #
+        # Although this hole only exists in the right-hand argument of
+        # LIKE, this replacement happens in both the received URL and
+        # the recorded trust_root, in case they have legitimate ~
+        # characters that need to be matched.
+        #
+        # Python uses % for parameters in interpolated strings, and
+        # PostgreSQL uses it as a wild card in LIKE-patterns; so
+        # they're escaped here.
+        condition = """
+      replace(replace(%s,         '~', '~T'), '%%', '~P')
+LIKE (replace(replace(trust_root, '~', '~T'), '%%', '~P') || '%%')
+"""
+
+        q = self.extra(
+            select={'len': 'length(trust_root)'},
+            where=[condition],
+            params=(url,),
+            order_by=['-len'] # Select the longest match
+            )
+        return q[0] if q else None
+
+
 class OpenIDRPConfig(models.Model):
     trust_root = models.TextField(unique=True)
     displayname = models.TextField()
@@ -127,6 +159,8 @@
         verbose_name = _('OpenID RP Config')
         verbose_name_plural = _('OpenID RP Configs')
 
+    objects = OpenIDRPConfigManager()
+
     def __unicode__(self):
         return self.displayname
 

=== modified file 'identityprovider/tests/test_views_ui_logout.py'
--- identityprovider/tests/test_views_ui_logout.py	2010-09-22 00:43:01 +0000
+++ identityprovider/tests/test_views_ui_logout.py	2010-09-29 13:19:43 +0000
@@ -60,6 +60,38 @@
         return_to = self.view.get_return_to_url(trust_root, None)
         self.assertEquals(return_to, trust_root)
 
+    def test_get_return_to_url_when_extends_root(self):
+        trust_root = 'http://example.com/r'
+        logout = trust_root + '/a'
+        self.create_openid_rp_config(trust_root)
+        return_to = self.view.get_return_to_url(logout, None)
+        self.assertEquals(return_to, logout)
+
+    def test_get_return_to_url_when_diminishes_root(self):
+        logout = 'http://example.com/r'
+        trust_root = logout + '/a'
+        self.create_openid_rp_config(trust_root)
+        return_to = self.view.get_return_to_url(logout, None)
+        self.assertTrue(return_to is None)
+
+    def test_get_return_to_url_when_tricky_trust(self):
+        trust_root = 'http://%.example.com/r'
+        self.create_openid_rp_config(trust_root)
+        return_to = self.view.get_return_to_url('http://launchpad.net/?sneaky=.example.com/r', None)
+        self.assertTrue(return_to is None)
+
+    def test_get_return_to_url_when_tricky_return(self):
+        trust_root = 'http://example.com/r'
+        self.create_openid_rp_config(trust_root)
+        return_to = self.view.get_return_to_url('http://%.com/r', None)
+        self.assertTrue(return_to is None)
+
+    def test_get_return_to_url_when_tilde(self):
+        trust_root = 'http://example.com/~id'
+        self.create_openid_rp_config(trust_root)
+        return_to = self.view.get_return_to_url(trust_root, None)
+        self.assertEquals(return_to, trust_root)
+
     def test_get_return_to_url_when_referer_mismatches_known_url(self):
         trust_root = 'http://example.com/r'
         self.create_openid_rp_config(trust_root)

=== modified file 'identityprovider/views/ui.py'
--- identityprovider/views/ui.py	2010-09-22 08:04:03 +0000
+++ identityprovider/views/ui.py	2010-09-29 13:19:43 +0000
@@ -113,9 +113,8 @@
     def get_return_to_url(self, return_to, http_referer):
         if not return_to:
             return None
-        known = OpenIDRPConfig.objects.filter(
-            trust_root=return_to).count() == 1
-        if known:
+        rp_config = OpenIDRPConfig.objects.for_url(return_to)
+        if rp_config is not None:
             if http_referer is not None and http_referer != return_to:
                 return None
             return return_to