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

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

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

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.

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?

Comments added.

Thanks for the thoroughness of the review!

Stu

« Back to merge proposal