Merge lp:~canonical-isd-hackers/canonical-identity-provider/bug_574911_large_assertions into lp:canonical-identity-provider/release

[David Owen]

Fixed #574911: Large OpenID assertions generate malformed POST responses.

Also, unhid some existing OpenID test cases.

I could find no explicit requirements on the Content-Type of any assertion. The only implied requirement is text/html for POST responses, so that the returned form may be rendered as HTML for user submission.

Copy-editing of the form wrapper is appreciated.

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

I'm not thrilled about DecideTestCase2 name. Maybe it would be better to merge those tests with DecideTestCase?

[Ricardo Kirkner (ricardokirkner)]

Definitevely. I agree with Lukasz on this. The test on DecideTestCase2 should be moved to DecideTestCase (and possibly renamed too).

[Ricardo Kirkner (ricardokirkner)]

Good work. Thanks.

review: Approve

=== added file 'identityprovider/templates/post-assertion.html'
--- identityprovider/templates/post-assertion.html	1970-01-01 00:00:00 +0000
+++ identityprovider/templates/post-assertion.html	2010-06-09 15:39:29 +0000
@@ -0,0 +1,23 @@
+<!-- Copyright 2010 Canonical Ltd.  This software is licensed under the
+GNU Affero General Public License version 3 (see the file  LICENSE). -->
+
+{% extends "base.html" %}
+{% load i18n %}
+
+{% block "title" %}
+    {% trans "Continue to 3rd-party site" %}
+{% endblock %}
+
+{% block "content" %}
+<div class="info">
+  <h1 class="main">{% trans "Continue to 3rd-party site" %}</h1>
+
+  <p>
+    {% blocktrans %}You are ready to continue to the 3rd-party site.{% endblocktrans %}
+  </p>
+</div>
+
+<div class="actions">
+  {{ form|safe }}
+</div>
+{% endblock %}

=== modified file 'identityprovider/tests/test_views_server.py'
--- identityprovider/tests/test_views_server.py	2010-06-04 14:38:15 +0000
+++ identityprovider/tests/test_views_server.py	2010-06-09 15:39:29 +0000
@@ -9,7 +9,8 @@
 from openid.extensions import pape
 from openid.extensions.sreg import SRegRequest
 
-from openid.message import Message, OPENID2_NS, IDENTIFIER_SELECT
+from openid.message import (Message, IDENTIFIER_SELECT, OPENID1_URL_LIMIT,
+    OPENID2_NS)
 from openid.server.server import Server, ProtocolError
 from openid.yadis.constants import YADIS_HEADER_NAME
 from urllib import quote, quote_plus
@@ -27,7 +28,6 @@
     BasicAccountTestCase, AuthenticatedTestCase, OpenIDProviderTestCase)
 from identityprovider.const import PERSON_VISIBILITY_PRIVATE_MEMBERSHIP
 
-__all__ = ['UntrustedRPTest', ]
 
 
 class DummyORequest(object):
@@ -54,20 +54,6 @@
         self.session = DummySession()
 
 
-class UntrustedRPTest(SQLCachedTestCase):
-
-    def __init__(self, META=None, REQUEST=None):
-        class MockSession(dict):
-            def __init__(self):
-                self.session_key = 'session-key'
-            def flush(self):
-                pass
-
-        self.META = META if META is not None else {}
-        self.REQUEST = REQUEST if REQUEST is not None else {}
-        self.session = MockSession()
-
-
 class HandleOpenIDErrorTestCase(OpenIDProviderTestCase):
 
     # tests for the _handle_openid_error method
@@ -245,20 +231,24 @@
 
 
 class DecideTestCase(AuthenticatedTestCase, OpenIDProviderTestCase):
+
+    def _prepare_openid_token(self, param_overrides=None):
+        request = {'openid.mode': 'checkid_setup',
+                   'openid.trust_root': 'http://localhost/',
+                   'openid.return_to': 'http://localhost/',
+                   'openid.identity': IDENTIFIER_SELECT}
+        if param_overrides:
+            request.update(param_overrides)
+        openid_server = server._get_openid_server()
+        self.orequest = openid_server.decodeRequest(request)
+        self.token = create_token(16)
+        session = self.client.session
+        session[self.token] = signed.dumps(self.orequest, settings.SECRET_KEY)
+        session.save()
+
     def setUp(self):
         super(DecideTestCase, self).setUp(disableCSRF=True)
-
-        request = {'openid.mode': 'checkid_setup',
-                   'openid.trust_root': 'http://localhost/',
-                   'openid.return_to': 'http://localhost/',
-                   'openid.identity': IDENTIFIER_SELECT}
-        openid_server = server._get_openid_server()
-        self.orequest = openid_server.decodeRequest(request)
-        self.token = create_token(16)
-        session = self.client.session
-        session[self.token] = signed.dumps(self.orequest,
-                                      settings.SECRET_KEY)
-        session.save()
+        self._prepare_openid_token()
 
     def test_decide_invalid(self):
         token = 'a' * 16
@@ -272,6 +262,21 @@
         self.assertEqual(r.status_code, 302)
         self.assertEqual(query['openid.mode'], 'id_res')
 
+    def test_decide_authenticated_with_post(self):
+        # Using a return-to URL that is more than OPENID1_URL_LIMIT
+        # characters long will force the assertion to be sent as a
+        # POSTing form (instead of a 302 redirect).
+        return_to = 'http://localhost/' + (OPENID1_URL_LIMIT * 'a')
+        self._prepare_openid_token({
+            'openid.return_to': return_to,
+            'openid.ns': OPENID2_NS,
+            'openid.claimed_id': 'http://localhost/~userid'})
+        r = self.client.post("/%s/+decide" % self.token, {'yes': ''})
+        self.assertEqual(r.status_code, 200)
+        self.assertEqual(r['Content-Type'], 'text/html')
+        self.assertContains(r, '<form ')
+        self.assertContains(r, return_to)
+
     def test_decide_auto_authorize(self):
         # make sure rpconfig is set to auto authorize
         rpconfig = OpenIDRPConfig(trust_root='http://localhost/',
@@ -700,7 +705,7 @@
     def tearDown(self):
         settings.SSO_RESTRICT_RP = self.old_restrict
 
-    def test_unknown_rp(self):
+    def test_process_unknown_rp(self):
         orequest = DummyORequest()
         request = DummyRequest()
         response = server._process_openid_request(request, orequest, None)
@@ -802,12 +807,12 @@
         team.save()
 
 
-class DecideTestCase(BasicAccountTestCase):
+class MarkupTestCase(BasicAccountTestCase):
 
     fixtures = ["test"]
     pgsql_functions = ["generate_openid_identifier"]
 
-    def test_unknown_rp(self):
+    def test_untrusted_rp_properly_shows_markup(self):
         self.client.login(username='[e-mail]', password='test')
 
         session = self.client.session

=== modified file 'identityprovider/views/server.py'
--- identityprovider/views/server.py	2010-05-18 15:28:53 +0000
+++ identityprovider/views/server.py	2010-06-09 15:39:29 +0000
@@ -23,7 +23,7 @@
 from django.contrib.auth.decorators import login_required
 from django.http import (Http404, HttpResponse, HttpResponseBadRequest,
     HttpResponseRedirect)
-from django.template import RequestContext
+from django.template import RequestContext, loader
 from django.shortcuts import get_object_or_404, render_to_response
 from django.utils.decorators import decorator_from_middleware
 
@@ -478,7 +478,14 @@
             request.session.session_key)
         _add_sreg(request.user, orequest, oresponse)
         _check_team_membership(request.user, orequest, oresponse)
-    return _django_response(request, oresponse, decision)
+    r = _django_response(request, oresponse, decision)
+    if r.content:
+        # Only user-visible content is generated from this view.  Wrap
+        # it up and set the Content-Type.
+        r.content = loader.render_to_string("post-assertion.html", {
+            'form': r.content})
+        r['Content-Type'] = 'text/html'
+    return r
 
 
 def _get_openid_server():