Merge into trunk : bugs-632536-680256-long-query-improvements : Code : Canonical SSO provider

Status:

Merged

Approved by:

Michael Nelson on 2010-11-23

Approved revision:

no longer in the source branch.

Merged at revision:

119

Proposed branch:

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

Merge into:

lp://staging/canonical-identity-provider/release

Diff against target:

141 lines (+36/-21)

5 files modified

identityprovider/templates/consumer/index.html (+4/-0)
identityprovider/templates/post-assertion.html (+10/-20)
identityprovider/tests/test_middleware.py (+2/-0)
identityprovider/tests/test_views_consumer.py (+8/-1)
identityprovider/views/consumer.py (+12/-0)

To merge this branch:

bzr merge lp://staging/~canonical-isd-hackers/canonical-identity-provider/bugs-632536-680256-long-query-improvements

Related bugs:

Bug #632536: When Logging into LP SSO from LP you shouldn't be told you are going to a 3rd party site	Low	Fix Released
Bug #680256: Enable setting a long query string from the test consumer	Medium	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Anthony Lenton (community)			Approve on 2010-11-23
Michael Nelson (community)	code	2010-11-23	Approve on 2010-11-23
Review via email: mp+41537@code.staging.launchpad.net

Commit message

Simplified post redirect form and added option to test it in the test consumer

Description of the change

This branch simplifies the post redirect form on the identity provider and adds js to auto-submit where enabled (bug #632536). It also adds an associated option to the test consumer to help QA with testing (bug #680256).

Revision history for this message

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

#

Download full text (7.2 KiB)

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>
> -{% e...

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
>

review: Approve (code)

Revision history for this message

Stuart Metcalfe (stuartmetcalfe) wrote on 2010-11-23:

#

Download full text (5.3 KiB)

Hi Michael

On Tue, 2010-11-23 at 10:04 +0000, Michael Nelson wrote:
> 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).

That's a good question. What you've suggested makes sense but is beyond
the scope of this branch so I've filed bug #680427 to track it.

> 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.?

That's a good point. I've added two new assertions to that code - one
for the form element and one for the post method. If you know a
non-fragile way of capturing both in one simple test, please let me
know.

While I was in there, I discovered openid.message.OPENID1_URL_LIMIT so
have updated my changes to use that. This helped me improve one of my
tests by explicitly testing that the return_to URL is over this length.

> 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>

I think that passing the long URL into the template from the index view
makes it harder to understand what's going on. The only reason I'd
consider it would be if we wanted to enable editing of the long query
string, but we don't as its content has no effect on the OpenID
transaction. The purpose of this option is merely to trigger the
conditions under which the POST form is used.

> > +<html>
> > + <body onload="document.forms[0].submit()">
> > + {{ form|safe }}
> > + </body>
> > +</html>
>
> Now this is actual production code :) I see from comment
>
> https://bugs.lau...

Hi Michael

On Tue, 2010-11-23 at 10:04 +0000, Michael Nelson wrote:
> 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).

That's a good question. What you've suggested makes sense but is beyond
the scope of this branch so I've filed bug #680427 to track it.

> 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.?

That's a good point.  I've added two new assertions to that code - one
for the form element and one for the post method.  If you know a
non-fragile way of capturing both in one simple test, please let me
know.

While I was in there, I discovered openid.message.OPENID1_URL_LIMIT so
have updated my changes to use that.  This helped me improve one of my
tests by explicitly testing that the return_to URL is over this length.

> 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>

I think that passing the long URL into the template from the index view
makes it harder to understand what's going on.  The only reason I'd
consider it would be if we wanted to enable editing of the long query
string, but we don't as its content has no effect on the OpenID
transaction.  The purpose of this option is merely to trigger the
conditions under which the POST form is used.

> > +<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?

That's right.  The idea behind removing the branding, etc., is to make
the page as unobtrusive as possible by loading fast and disappearing in
the blink of an eye.  This is common practice on OpenID pages where POST
forms sometimes need to be used to pass messages between the consumer
and provider - see Ubuntu One's login for a live example.  I did notice
that they also include a page <title> to hint to the user that "OpenID
transaction in progress" so I decided to at least add that to our
template.  I really don't think we want to over-complicate this though.

> 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.

Theoretically, but in practice the risk is negligible and not worth the
extra js (imo).

> > +@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?

Without it, cross-posting from the provider back to the consumer fails.
I should have picked it up at the same time as I added the exemption to
startOpenID but didn't because there was no way to test the long query.

Comments added.

Thanks for the thoroughness of the review!

Stu

Revision history for this message

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

#

Nice spot with the OPENID1_URL_LIMIT, and thanks for filling me in on the background.

Revision history for this message

Anthony Lenton (elachuni) on 2010-11-23:

#

review: Approve

Canonical SSO provider

Merge lp://staging/~canonical-isd-hackers/canonical-identity-provider/bugs-632536-680256-long-query-improvements into lp://staging/canonical-identity-provider/release

Commit message

Description of the change

Preview Diff

Subscribers