Canonical SSO provider

Code review comment for lp://staging/~canonical-isd-hackers/canonical-identity-provider/bugs-632536-680256-long-query-improvements

bugs-632536-680256-long-query-improvements
Merge into trunk

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2010-11-23:

Hi Stuart!

This is my first CIP branch - so I'm sure I'm missing some context and I have asked some questions that are probably obvious. Sorry if that's painful :P I've not yet set the dev env. up, otherwise I would have tested some of the points below.

It was initially unclear to me which code was for tests only (ie. templates/consumer/* and views/consumer.py) - I'm not sure yet why these test-related code and templates are part of the normal application (rather than being a separate test app).

I've marked the MP as approved, but am keen to hear your thoughts on the points below - even if you choose to ignore them :) In retrospect, my main question is related to the only production change - the post-assertion.html template.

Finally, it might not be for this branch, but I can only see a test that verifies the post-assertion.html template is used (in test_middleware) - should there be a test somewhere verifying that the rendered response contains a form with a post action back to the consumer etc.?

Details below.

> === modified file 'identityprovider/templates/consumer/index.html'
> --- identityprovider/templates/consumer/index.html 2010-11-16 15:36:06 +0000
> +++ identityprovider/templates/consumer/index.html 2010-11-23 07:55:27 +0000
> @@ -135,6 +135,10 @@
> </table>
> </p>
>
> + <p>
> + <input type="checkbox" name="forcelongurl" id="id-forcelongurl" value="1" />
> + <label for="id-forcelongurl">Force long return URL</label>
> + </p>

I'm not sure this is the best way to test for long urls (ie. adding code
to the view that checks for this param which should only be used for testing
etc.). Could it be better to actually include a long URL here?
(rather than fabricating one inside the consumer view)?

It should be easy to do this by changing the above to be a radio option,
something like:
  <input type="radio" name="forcelongurl" id="id-forcelongurl-false" value="false">
  <label for="id-forcelongurl-false">Normal URL</label>
  <input type="radio" name="forcelongurl" id="id-forcelongurl-true"
    value="longurl-longurl-longurl-longurl-longurl-generate-me-in-code-etc">
  <label for="id-forcelongurl-true">Long URL (requiring POST back to consumer)</label>

AFAICS, the posted params are passed through during consumer_views.finishOpenID - but haven't verified.

> <input type="submit" value="Begin" />
> </form>
>
>
> === modified file 'identityprovider/templates/post-assertion.html'
> --- identityprovider/templates/post-assertion.html 2010-07-12 19:11:15 +0000
> +++ identityprovider/templates/post-assertion.html 2010-11-23 07:55:27 +0000
> @@ -1,27 +1,13 @@
> -{% extends "base.html" %}
> -{% load i18n %}
> +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
> + "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
>
> {% comment %}
> Copyright 2010 Canonical Ltd. This software is licensed under the
> GNU Affero General Public License version 3 (see the file LICENSE).
> {% endcomment %}
>
> -{% block title %}
> - {% trans "Continue to 3rd-party site" %}
> -{% endblock %}
> -
> -{% block text_title %}
> - <h1 class="main">{% trans "Continue to 3rd-party site" %}</h1>
> -{% endblock %}
> -
> -{% block content %}
> -<div class="info">
> - <p>
> - {% blocktrans %}You are ready to continue to the 3rd-party site.{% endblocktrans %}
> - </p>
> -</div>
> -
> -<div class="actions">
> - {{ form|safe }}
> -</div>
> -{% endblock %}
> +<html>
> + <body onload="document.forms[0].submit()">
> + {{ form|safe }}
> + </body>
> +</html>

Now this is actual production code :) I see from comment

https://bugs.launchpad.net/canonical-identity-provider/+bug/632536/comments/2

that you meant to simplify this template but I just want to check the
consequences...

That is, all users being redirected to a long-url (ie. openid message needs to
be posted) will see simply a "Continue" button (no title, heading etc)
- whether or not it is automatically submitted via js. I'm unsure why
we'd remove the base template and title tags, even if you did want to remove
the div.info message. I'm guessing the root problem here is that the user will
always *see* this page (even if only briefly) - whether or not we auto-submit
it for them... is that right?

If so, another option would be to leave the template as it was,
and simply add the JS to disable the button and update the button text from
"Continue" to "Continuing..." (or even hiding the button if you wanted),
before the auto-submission. But this would require updating the div.info text
for "trusted" sites, as you'd planned in your initial comment.

Another side-issue - I've not tried it, but is double-submission a problem?
(ie. user clicks the button as well as it being auto-submitted) I guess not as
you mention this is used elsewhere also, but if so, disabling the form/button
as above before auto-submitting should avoid this in most cases.

>
> === modified file 'identityprovider/tests/test_views_consumer.py'
> --- identityprovider/tests/test_views_consumer.py 2010-11-16 15:29:32 +0000
> +++ identityprovider/tests/test_views_consumer.py 2010-11-23 07:55:27 +0000
> @@ -280,3 +280,10 @@
> r = self.client.get('/consumer/xrds/', **self.req.META)
> self.assertTemplateUsed(r, 'openidapplication-xrds.xml')
> self.assertEqual(r['Content-Type'], YADIS_CONTENT_TYPE)
> +
> + def test_long_query_string(self):

I think it's worth a comment explaining the purpose of this test. Maybe
something like:
# A long param in the query results in a long openid return_to.

> + self.req.POST['mode'] = 'setup'
> + self.req.POST['forcelongurl'] = '1'

If the above suggestion works, this could be:

self.req.POST['forcelongurl'] = 'a' * 2000

or similar.

> + r = self.client.post('/consumer/', self.req.POST, **self.req.META)
> + query = self.get_query(r)
> + self.assertTrue(query['openid.return_to'].find('a' * 2000) >= 0)
>
> === modified file 'identityprovider/views/consumer.py'
> --- identityprovider/views/consumer.py 2010-11-16 15:36:06 +0000
> +++ identityprovider/views/consumer.py 2010-11-23 07:55:27 +0000
> @@ -193,6 +193,9 @@
> trust_root = getViewURL(request, startOpenID)
> return_to = getViewURL(request, finishOpenID)
>
> + if bool(request.POST.get('forcelongurl')):
> + return_to += '?a=' + ('a' * 2000)
> +

This may not be necessary at all if the above works, but now that I've
understood that these consumer views are actually only for testing, I'm not so
concerned.

> # Send the browser to the server either by sending a redirect
> # URL or by generating a POST form.
> if auth_request.shouldSendRedirect():
> @@ -217,6 +220,7 @@
> return teams.TeamsRequest(req_teams)
>
>
> +@csrf_exempt

Why is this necessary? I see it's already applied to the startOpenID
view too. Even if these views are just for testing interactions, I still think
it would be worth a comment indicating why the csrf_exempt decorator is
required for the test views?

If it is just to enable test_long_query_string to function without having to
setup the csrf up, could we instead add the csrf token explicitly
to DummyDjangoRequest.__init__()?

> def finishOpenID(request):
> """
> Finish the OpenID authentication process. Invoke the OpenID
>

Hi Stuart!

I've marked the MP as approved, but am keen to hear your thoughts on the points  below - even if you choose to ignore them :) In retrospect, my main question is related to the only production change - the post-assertion.html template.

Details below.

> === modified file 'identityprovider/templates/consumer/index.html'
> --- identityprovider/templates/consumer/index.html	2010-11-16 15:36:06 +0000
> +++ identityprovider/templates/consumer/index.html	2010-11-23 07:55:27 +0000
> @@ -135,6 +135,10 @@
>      </table>
>      </p>
>
> +    <p>
> +        <input type="checkbox" name="forcelongurl" id="id-forcelongurl" value="1" />
> +        <label for="id-forcelongurl">Force long return URL</label>
> +    </p>

AFAICS, the posted params are passed through during consumer_views.finishOpenID - but haven't verified.

>      <input type="submit" value="Begin" />
>    </form>
>
>
> === modified file 'identityprovider/templates/post-assertion.html'
> --- identityprovider/templates/post-assertion.html	2010-07-12 19:11:15 +0000
> +++ identityprovider/templates/post-assertion.html	2010-11-23 07:55:27 +0000
> @@ -1,27 +1,13 @@
> -{% extends "base.html" %}
> -{% load i18n %}
> +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
> +    "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
>
>  {% comment %}
>  Copyright 2010 Canonical Ltd.  This software is licensed under the
>  GNU Affero General Public License version 3 (see the file  LICENSE).
>  {% endcomment %}
>
> -{% block title %}
> -    {% trans "Continue to 3rd-party site" %}
> -{% endblock %}
> -
> -{% block text_title %}
> -    <h1 class="main">{% trans "Continue to 3rd-party site" %}</h1>
> -{% endblock %}
> -
> -{% block content %}
> -<div class="info">
> -  <p>
> -    {% blocktrans %}You are ready to continue to the 3rd-party site.{% endblocktrans %}
> -  </p>
> -</div>
> -
> -<div class="actions">
> -  {{ form|safe }}
> -</div>
> -{% endblock %}
> +<html>
> +  <body onload="document.forms[0].submit()">
> +    {{ form|safe }}
> +  </body>
> +</html>

Now this is actual production code :) I see from comment

https://bugs.launchpad.net/canonical-identity-provider/+bug/632536/comments/2

that you meant to simplify this template but I just want to check the
consequences...

>
> === modified file 'identityprovider/tests/test_views_consumer.py'
> --- identityprovider/tests/test_views_consumer.py	2010-11-16 15:29:32 +0000
> +++ identityprovider/tests/test_views_consumer.py	2010-11-23 07:55:27 +0000
> @@ -280,3 +280,10 @@
>          r = self.client.get('/consumer/xrds/', **self.req.META)
>          self.assertTemplateUsed(r, 'openidapplication-xrds.xml')
>          self.assertEqual(r['Content-Type'], YADIS_CONTENT_TYPE)
> +
> +    def test_long_query_string(self):

I think it's worth a comment explaining the purpose of this test. Maybe
something like:
           # A long param in the query results in a long openid return_to.

> +        self.req.POST['mode'] = 'setup'
> +        self.req.POST['forcelongurl'] = '1'

If the above suggestion works, this could be:

self.req.POST['forcelongurl'] = 'a' * 2000

or similar.

> +        r = self.client.post('/consumer/', self.req.POST, **self.req.META)
> +        query = self.get_query(r)
> +        self.assertTrue(query['openid.return_to'].find('a' * 2000) >= 0)
>
> === modified file 'identityprovider/views/consumer.py'
> --- identityprovider/views/consumer.py	2010-11-16 15:36:06 +0000
> +++ identityprovider/views/consumer.py	2010-11-23 07:55:27 +0000
> @@ -193,6 +193,9 @@
>          trust_root = getViewURL(request, startOpenID)
>          return_to = getViewURL(request, finishOpenID)
>
> +        if bool(request.POST.get('forcelongurl')):
> +            return_to += '?a=' + ('a' * 2000)
> +

This may not be necessary at all if the above works, but now that I've
understood that these consumer views are actually only for testing, I'm not so
concerned.

>          # Send the browser to the server either by sending a redirect
>          # URL or by generating a POST form.
>          if auth_request.shouldSendRedirect():
> @@ -217,6 +220,7 @@
>      return teams.TeamsRequest(req_teams)
>
>
> +@csrf_exempt

If it is just to enable test_long_query_string to function without having to
setup the csrf up, could we instead add the csrf token explicitly
to DummyDjangoRequest.__init__()?

>  def finishOpenID(request):
>      """
>      Finish the OpenID authentication process.  Invoke the OpenID
>

review: Approve (code)

« Back to merge proposal