Merge lp:~jamestait/canonical-identity-provider/lp1169635 into lp:canonical-identity-provider/release

[James Tait]

Don't check whether the unsigned response would result in a form POST, because the signed response will be longer and may do even if the unsigned response wouldn't. Instead, check the signed response and the request mode.

This is a fix for the case where an unsigned OpenID response would be short
enough to allow a URL redirect to be used, but the signed response would be too
long and thus would result in an HTML form POST.

This is all to work around a shortcoming in the openid library. There is a fix
in openid trunk, but:

 a) it will probably break existing implementations and
 b) according to PyPi there hasn't been a release of the openid library in
    almost 3 years.

[Michael Foord (mfoord)]

I would rather we didn't use shortened urls.

[Ricardo Kirkner (ricardokirkner)]

LGTM

review: Approve

=== modified file 'identityprovider/tests/test_views_server.py'
--- identityprovider/tests/test_views_server.py	2013-04-17 08:32:15 +0000
+++ identityprovider/tests/test_views_server.py	2013-04-17 15:27:25 +0000
@@ -193,13 +193,25 @@
                          'token_via_email')
 
     def test_handle_user_response_auto_auth_large_response(self):
-        # update rp to auto authorize
-        self.rpconfig.auto_authorize = True
-        self.rpconfig.allowed_ax = 'fullname,email,account_verified'
-        self.rpconfig.save()
         # Make sure we get a large response
         self.account.displayname = 'a' * OPENID1_URL_LIMIT
         self.account.save()
+        self._test_auto_auth()
+
+    def test_handle_user_response_auto_auth_borderline_response(self):
+        # Make a response that's small enough that it fits in a redirect before
+        # signing, but large enough that it will need a POST after signing. We
+        # might want to come up with a more robust method of determining the
+        # required length, but this works for now.
+        self.account.displayname = 'a' * (OPENID1_URL_LIMIT / 2)
+        self.account.save()
+        self._test_auto_auth()
+
+    def _test_auto_auth(self):
+        # update rp to auto authorize
+        self.rpconfig.auto_authorize = True
+        self.rpconfig.allowed_ax = 'fullname,email,account_verified'
+        self.rpconfig.save()
 
         self.client.login(username=self.email, password=DEFAULT_USER_PASSWORD)
         self.params.update({

=== modified file 'identityprovider/views/server.py'
--- identityprovider/views/server.py	2013-04-15 12:56:43 +0000
+++ identityprovider/views/server.py	2013-04-17 15:27:25 +0000
@@ -26,8 +26,9 @@
     registerNamespaceAlias,
 )
 from openid.server.server import (
+    BROWSER_REQUEST_MODES,
+    ENCODE_URL,
     CheckIDRequest,
-    ENCODE_URL,
     ProtocolError,
     Server,
 )
@@ -531,7 +532,11 @@
 def _django_response(request, oresponse, auth_success=False, orequest=None):
     """ Convert an OpenID response into a Django HttpResponse """
     webresponse = _get_openid_server().encodeResponse(oresponse)
-    if oresponse.renderAsForm():
+    # This is a workaround for the fact the the openid library returns bare
+    # HTML form markup instead of a complete HTML document. See
+    # https://github.com/openid/python-openid/pull/31/files which has been
+    # merged, but not released.
+    if webresponse.body and oresponse.request.mode in BROWSER_REQUEST_MODES:
         response = HttpResponse(
             oidutil.autoSubmitHTML(webresponse.body), mimetype='text/html')
     else: