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

Proposed by David Owen
Status: Merged
Merged at revision: 107
Proposed branch: lp://staging/~canonical-isd-hackers/canonical-identity-provider/bug_608920_csrf_token
Merge into: lp://staging/canonical-identity-provider/release
Diff against target: 89 lines (+63/-2)
2 files modified
identityprovider/tests/test_middleware.py (+60/-2)
identityprovider/views/server.py (+3/-0)
To merge this branch: bzr merge lp://staging/~canonical-isd-hackers/canonical-identity-provider/bug_608920_csrf_token
Reviewer Review Type Date Requested Status
Ricardo Kirkner (community) Approve
Review via email: mp+33412@code.staging.launchpad.net

Description of the change

= Overview =

CSRF protection inserts CSRF tokens into all outgoing forms that
aren't marked as exempt (or whose generating views aren't marked as
exempt).

SSO has been inserting CSRF tokens into forms that submit to LP and
other places. Specifically, when the OpenID return_to URL is to long
to allow a GET response, we generate a form for the user to POST.
This form, as it submits to LP, shouldn't have any CSRF tokens in it.

= Details =

The view (+decide) that generates the OpenID POST reponse also
generates several other, user-visible responses. Some of these
redirect, and one other generates a form that submits back to +decide.

The OpenID POST *response* is marked as exempt (not the entire view).

The new test verifies that other aspects of +decide work as expected,
especially that the other form (submitting back to SSO) is still
CSRF-protected.

To post a comment you must log in.
Revision history for this message
Ricardo Kirkner (ricardokirkner) wrote :

Very clean and elegant test. Reads almost like a doctest. Keep up the good work!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.