Merge lp:~canonical-isd-hackers/canonical-identity-provider/bugs-659246-675933-test-consumer-logout-and-related-fixes into lp:canonical-identity-provider/release

[Stuart Metcalfe]

Added logout links to the test consumer and related logout return_to fixes

This branch adds logout links to the test consumer, both with and without a return_to argument, to test the new feature enabling returning to a consumer.

As part of this work, I also picked up a few issues which are also fixed in this branch:

 * Display the consumer's rpconfig name, rather than the URL, in the template.
 * Make the return link more obvious.
 * Match rpconfigs where the return_to url exactly matches the rpconfig trust root and has a trailing slash.

=== modified file 'identityprovider/media/launchpad/styles.css'
--- identityprovider/media/launchpad/styles.css	2010-08-23 13:21:38 +0000
+++ identityprovider/media/launchpad/styles.css	2010-11-16 14:18:57 +0000
@@ -113,6 +113,11 @@
     color: #666;
 }
 
+p.returnto {
+    font-size: 16px;
+    text-align: center;
+}
+
 #rp {
     width: 49%;
     float: left;

=== modified file 'identityprovider/media/ubuntu/styles.css'
--- identityprovider/media/ubuntu/styles.css	2010-11-11 19:50:54 +0000
+++ identityprovider/media/ubuntu/styles.css	2010-11-16 14:18:57 +0000
@@ -265,6 +265,9 @@
     font-size: 16px;
     line-height: 20px;
 }
+p.returnto {
+    margin: 20px 0px;
+}
 p.services {
     color: #fff;
 }

=== modified file 'identityprovider/models/openidmodels.py'
--- identityprovider/models/openidmodels.py	2010-09-29 11:01:03 +0000
+++ identityprovider/models/openidmodels.py	2010-11-16 14:18:57 +0000
@@ -112,7 +112,9 @@
 class OpenIDRPConfigManager(models.Manager):
 
     def for_url(self, url):
-        if url.endswith('/'):
+        if url is None:
+            return None
+        elif 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
@@ -128,6 +130,7 @@
         # PostgreSQL uses it as a wild card in LIKE-patterns; so
         # they're escaped here.
         condition = """
+      rtrim(trust_root, '/') = %s OR
       replace(replace(%s,         '~', '~T'), '%%', '~P')
 LIKE (replace(replace(trust_root, '~', '~T'), '%%', '~P') || '%%')
 """
@@ -135,7 +138,7 @@
         q = self.extra(
             select={'len': 'length(trust_root)'},
             where=[condition],
-            params=(url,),
+            params=(url, url),
             order_by=['-len'] # Select the longest match
             )
         return q[0] if q else None

=== modified file 'identityprovider/templates/consumer/index.html'
--- identityprovider/templates/consumer/index.html	2010-11-10 03:57:51 +0000
+++ identityprovider/templates/consumer/index.html	2010-11-16 14:18:57 +0000
@@ -152,6 +152,11 @@
     <input type="submit" value="Begin" />
   </form>
 
+  <h2>Test logout</h2>
+  <ul>
+    <li><a href="{{ openid }}+logout?return_to={{ consumer_url|urlencode }}">Logout with return URL</a></li>
+    <li><a href="{{ openid }}+logout">Normal logout</a></li>
+  </ul>
               </div>
             </div>
         </div>

=== modified file 'identityprovider/templates/launchpad/registration/logout.html'
--- identityprovider/templates/launchpad/registration/logout.html	2010-11-10 15:49:10 +0000
+++ identityprovider/templates/launchpad/registration/logout.html	2010-11-16 14:18:57 +0000
@@ -13,12 +13,15 @@
 {% endblock %}
 
 {% block content %}
-  <p>{{ brand.logout }}
+  <p>{{ brand.logout }}</p>
+
+  <p class="returnto">
     {% if return_to_url %}
-    {% blocktrans with return_to_url as url and return_to_site_name as site_name %}<span class="big">Return to <a href="{{ return_to_url }}">{{ site_name }}</a></span>{% endblocktrans %}
+    {% blocktrans with return_to_url as url and return_to_site_name as site_name %}
+    <a href="{{ return_to_url }}">Return to {{ site_name }}</a>{% endblocktrans %}
+    {% else %}
+    <a href="/+login">{% trans "Log back in to Launchpad Login Service" %}</a>
     {% endif %}
   </p>
-
-  <p><a href="/+login">{% trans "Login back to Launchpad Login Service" %}</a></p>
 {% endblock %}
 

=== modified file 'identityprovider/templates/ubuntu/registration/logout.html'
--- identityprovider/templates/ubuntu/registration/logout.html	2010-11-09 14:37:43 +0000
+++ identityprovider/templates/ubuntu/registration/logout.html	2010-11-16 14:18:57 +0000
@@ -13,12 +13,12 @@
 {% endblock %}
 
 {% block content %}
-    <p>{{ brand.logout }}
-      {% if return_to_url %}
-      {% blocktrans with return_to_url as url and return_to_site_name as site_name %}
-     <span class="big">Return to <a href="{{ return_to_url }}">{{ site_name }}</a></span>{% endblocktrans %}
-     {% endif %}
-    </p>
+    <p>{{ brand.logout }}</p>
+
+    {% if return_to_url %}
+    {% blocktrans with return_to_url as url and return_to_site_name as site_name %}
+    <p class="returnto highlight"><a href="{{ return_to_url }}">Return to {{ site_name }}</a></p>{% endblocktrans %}
+    {% endif %}
 
     {% if other_sites %}
      <div>

=== modified file 'identityprovider/tests/test_views_ui_logout.py'
--- identityprovider/tests/test_views_ui_logout.py	2010-09-29 03:48:52 +0000
+++ identityprovider/tests/test_views_ui_logout.py	2010-11-16 14:18:57 +0000
@@ -54,7 +54,13 @@
             displayname="Example",
             creation_rationale=AccountCreationRationale.UNKNOWN)
 
-    def test_get_return_to_url_whithout_referer_and_url_is_recognized(self):
+    def test_get_return_to_root_url_without_referer_and_url_is_recognized(self):
+        trust_root = 'http://example.com/'
+        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_without_referer_and_url_is_recognized(self):
         trust_root = 'http://example.com/r'
         self.create_openid_rp_config(trust_root)
         return_to = self.view.get_return_to_url(trust_root, None)
@@ -109,6 +115,18 @@
 
         self.assertTrue(return_to is None)
 
+    def test_get_site_name_with_valid_return_to(self):
+        trust_root = 'http://example.com/'
+        self.create_openid_rp_config(trust_root)
+        site_name = self.view.get_site_name(trust_root)
+        self.assertEquals(site_name, "Example")
+
+    def test_get_site_name_with_invalid_return_to(self):
+        trust_root = 'http://example.com/'
+        self.create_openid_rp_config(trust_root)
+        site_name = self.view.get_site_name('http://notvalid.com/')
+        self.assertTrue(site_name is None)
+
 
     def sites_with_active_sessions(self):
         return []

=== modified file 'identityprovider/views/ui.py'
--- identityprovider/views/ui.py	2010-11-10 08:19:40 +0000
+++ identityprovider/views/ui.py	2010-11-16 14:18:57 +0000
@@ -99,7 +99,6 @@
 
 
 class LogoutView(object):
-
     def get_language(self, user):
         try:
             return user.preferredlanguage
@@ -111,15 +110,15 @@
             response.set_cookie(settings.LANGUAGE_COOKIE_NAME, language)
 
     def get_return_to_url(self, return_to, http_referer):
+        rpconfig = OpenIDRPConfig.objects.for_url(return_to)
         if not return_to:
             return None
-        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
+        elif rpconfig is None:
+            return None
+        elif http_referer is not None and http_referer != return_to:
+            return None
+        else:
             return return_to
-        else:
-            return None
 
     def set_orequest(self, session, token, raw_orequest):
         if token is not None and raw_orequest is not None:
@@ -132,11 +131,13 @@
                 if trust_root != site.trust_root]
 
     def get_site_name(self, trust_root):
+        rpconfig = OpenIDRPConfig.objects.for_url(trust_root)
         if not trust_root:
             return None
-        sites = list(OpenIDRPConfig.objects.filter(trust_root=trust_root))
-        if sites:
-            return sites[0].displayname
+        elif rpconfig is None:
+            return None
+        elif rpconfig.displayname:
+            return rpconfig.displayname
         else:
             return trust_root