Merge lp:~salgado/launchpad/bug-530738 into lp:launchpad

Proposed by Guilherme Salgado on 2010-03-02
Status: Merged
Merged at revision: not available
Proposed branch: lp:~salgado/launchpad/bug-530738
Merge into: lp:launchpad
Diff against target: 58 lines (+27/-3)
2 files modified
lib/canonical/launchpad/webapp/login.py (+2/-3)
lib/canonical/launchpad/webapp/tests/test_login.py (+25/-0)
To merge this branch: bzr merge lp:~salgado/launchpad/bug-530738
Reviewer Review Type Date Requested Status
Francis J. Lacoste (community) release-critical 2010-03-02 Approve on 2010-03-02
Brad Crittenden (community) code 2010-03-02 Approve on 2010-03-02
Review via email: mp+20452@code.launchpad.net
To post a comment you must log in.
Guilherme Salgado (salgado) wrote :

We're currently passing the application URL as the realm when starting
an OpenID authentication. That means when you login on, say,
bugs.lp.net, the provider will be told that the realm is bugs.lp.net,
but that's not really true as the authentication is valid for all of
launchpad.net. That is also how the spec says the realm should be used:

  A realm is designed to give the end user an indication of the scope
  of the authentication request.
  (http://openid.net/specs/openid-authentication-2_0.html#realms)

This branch fixes that by always using the mainsite's root URL as the
realm (aka trust root).

Brad Crittenden (bac) wrote :

Looks great Salgado. I'm surprised this bug has been around so long. Good catch.

review: Approve (code)
review: Approve (release-critical)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/webapp/login.py'
2--- lib/canonical/launchpad/webapp/login.py 2010-03-01 19:25:42 +0000
3+++ lib/canonical/launchpad/webapp/login.py 2010-03-02 15:19:14 +0000
4@@ -186,7 +186,6 @@
5 self.openid_request.addExtension(
6 sreg.SRegRequest(optional=['email', 'fullname']))
7
8- trust_root = self.request.getApplicationURL()
9 assert not self.openid_request.shouldSendRedirect(), (
10 "Our fixed OpenID server should not need us to redirect.")
11 # Once the user authenticates with the OpenID provider they will be
12@@ -196,8 +195,8 @@
13 # '+login' bit). To do that we encode that URL as a query arg in the
14 # return_to URL passed to the OpenID Provider
15 starting_url = urllib.urlencode([('starting_url', self.starting_url)])
16- return_to = urlappend(
17- self.request.getApplicationURL(), '+openid-callback')
18+ trust_root = allvhosts.configs['mainsite'].rooturl
19+ return_to = urlappend(trust_root, '+openid-callback')
20 return_to = "%s?%s" % (return_to, starting_url)
21 form_html = self.openid_request.htmlMarkup(trust_root, return_to)
22
23
24=== modified file 'lib/canonical/launchpad/webapp/tests/test_login.py'
25--- lib/canonical/launchpad/webapp/tests/test_login.py 2010-03-01 19:25:42 +0000
26+++ lib/canonical/launchpad/webapp/tests/test_login.py 2010-03-02 15:19:14 +0000
27@@ -352,6 +352,31 @@
28 sorted(sreg_extension.allRequestedFields()))
29
30
31+class TestOpenIDRealm(TestCaseWithFactory):
32+ # The realm (aka trust_root) specified by the RP is "designed to give the
33+ # end user an indication of the scope of the authentication request", so
34+ # for us the realm will always be the root URL of the mainsite.
35+ layer = AppServerLayer
36+
37+ def test_realm_for_mainsite(self):
38+ browser = Browser()
39+ browser.open('http://launchpad.dev:8085/+login')
40+ # At this point browser.contents contains a hidden form which would've
41+ # been auto-submitted if we had in-browser JS support, but since we
42+ # don't we can easily inspect what's in the form.
43+ self.assertEquals('http://launchpad.dev:8085/',
44+ browser.getControl(name='openid.realm').value)
45+
46+ def test_realm_for_vhosts(self):
47+ browser = Browser()
48+ browser.open('http://bugs.launchpad.dev:8085/+login')
49+ # At this point browser.contents contains a hidden form which would've
50+ # been auto-submitted if we had in-browser JS support, but since we
51+ # don't we can easily inspect what's in the form.
52+ self.assertEquals('http://launchpad.dev:8085/',
53+ browser.getControl(name='openid.realm').value)
54+
55+
56 def test_suite():
57 suite = unittest.TestSuite()
58 suite.addTest(unittest.TestLoader().loadTestsFromName(__name__))