LAVA Dashboard (deprecated)

Merge lp:~salgado/lava-dashboard/openid into lp:lava-dashboard

  1. openid
  2. Merge into trunk
Proposed by Guilherme Salgado
Status: Merged
Merged at revision: 95
Proposed branch: lp:~salgado/lava-dashboard/openid
Merge into: lp:lava-dashboard
Diff against target: 354 lines (+197/-13)
10 files modified
dashboard_app/tests/__init__.py (+1/-0)
dashboard_app/tests/other/csrf.py (+39/-7)
dashboard_app/tests/other/login.py (+116/-0)
dashboard_server/context_processors.py (+4/-0)
dashboard_server/default_settings.py (+26/-1)
dashboard_server/manage.py (+1/-0)
dashboard_server/settings.py (+4/-0)
dashboard_server/templates/base.html (+2/-3)
dashboard_server/templates/dashboard_app/bundle_stream_list.html (+1/-1)
dashboard_server/urls.py (+3/-1)
To merge this branch: bzr merge lp:~salgado/lava-dashboard/openid
Reviewer Review Type Date Requested Status
Zygmunt Krynicki (community) Needs Fixing
James Westby (community) Approve
Review via email: mp+38470@code.launchpad.net

Description of the change

Switch from password login to OpenID, using login.lp.net as the SSO.

To post a comment you must log in.
Revision history for this message
James Westby (james-w) wrote :

60 +
61 +class NoCSRFProtectionOnXMLRPCConfigurationTestCase(CSRFTestCase):
62 +

How crucial is it to split this test?

I pushed for it all to be one TestClass in order to get more confidence that
the test was passing because it was working, and not just because django
disables the checks for tests.

85 + <table>{{ form.as_table }}</table>

Are you still running under 1.1 on lucid? Have you tried this on 1.2? I
have a suspicion that you need another tag in there to put the CSRF token
in the HTML under 1.2.

175 + def tearDown(self):

We need an upcall to the super's tearDown.

177 + settings.OPENID_SSO_SERVER_URL = self.orig_setting

(optional) make the instance variable "orig_sso_server_url" or something,
in case we have to override a second later.

Will that code handle the SSO option being removed from settings.py?

182 + useropenid = UserOpenID(
183 + user=user, claimed_id=self._identity_url,
184 + display_id=self._identity_url)

What does that do?

258 +oidutil.log = lambda msg, level=0: None

Maybe settings.py?

272 +TEMPLATE_CONTEXT_PROCESSORS = (

Is there a reason that's not in default_settings.py?

Overall this looks great though, thanks.

James

review: Needs Information
Revision history for this message
Guilherme Salgado (salgado) wrote :

Hi James,

Thanks for the review!

On Thu, 2010-10-14 at 22:44 +0000, James Westby wrote:
> Review: Needs Information
> 60 +
> 61 +class NoCSRFProtectionOnXMLRPCConfigurationTestCase(CSRFTestCase):
> 62 +
>
> How crucial is it to split this test?
>
> I pushed for it all to be one TestClass in order to get more confidence that
> the test was passing because it was working, and not just because django
> disables the checks for tests.

I did that because the other one has the urls property, and that doesn't
include any of the app's urlpatterns as it only uses the test view
created for the test. That means the test method here can't post to the
'xml-rpc' endpoint. One benefit of splitting, though, is that this test
doesn't need to use CSRFTestCase -- the regular TestCase will do.

I could try and combine them together again, by having the urls property
include all the app's urls plus the one that is specific for testing, if
you think it's worth the effort.

>
> 85 + <table>{{ form.as_table }}</table>
>
> Are you still running under 1.1 on lucid? Have you tried this on 1.2? I
> have a suspicion that you need another tag in there to put the CSRF token
> in the HTML under 1.2.

As we discussed on IRC, launch-control is using the legacy method of
protection against CSRF, which doesn't require a template tag. Once we
drop support for django 1.1 we can move away from the legacy method.
I've added a comment explaining that to the relevant bit of code.

>
> 175 + def tearDown(self):
>
> We need an upcall to the super's tearDown.
>

Good catch; added.

> 177 + settings.OPENID_SSO_SERVER_URL = self.orig_setting
>
> (optional) make the instance variable "orig_sso_server_url" or something,
> in case we have to override a second later.

Renamed.

>
> Will that code handle the SSO option being removed from settings.py?

Probably not, so I've changed it to cope with that.

>
> 182 + useropenid = UserOpenID(
> 183 + user=user, claimed_id=self._identity_url,
> 184 + display_id=self._identity_url)
>
> What does that do?

Associates the newly created user with its identity URL.
(added this as a comment to the code)

>
> 258 +oidutil.log = lambda msg, level=0: None
>
> Maybe settings.py?

WFM.

>
> 272 +TEMPLATE_CONTEXT_PROCESSORS = (
>
> Is there a reason that's not in default_settings.py?

Mostly ignorance. I know close to nothing about what should go in each
of the settings files of a django project, so I added it to the same
file that Michael added the lexbuilder one. Happy to move that to
default_settings.py, though.

--
Guilherme Salgado <https://launchpad.net/~salgado>

Revision history for this message
James Westby (james-w) wrote :

> I did that because the other one has the urls property, and that doesn't
> include any of the app's urlpatterns as it only uses the test view
> created for the test. That means the test method here can't post to the
> 'xml-rpc' endpoint. One benefit of splitting, though, is that this test
> doesn't need to use CSRFTestCase -- the regular TestCase will do.

It does need CSRFTestCase, otherwise Django disables the csrf checks
globally, meaning that it is not testing in the right environment. That's
the sort of issue that means I want to stick to one test class.

> I could try and combine them together again, by having the urls property
> include all the app's urls plus the one that is specific for testing, if
> you think it's worth the effort.

I'm don't feel strongly about it, but I would have more confidence in the
test. Doesn't it just need the xml-rpc view hooked up?

Maybe the test urls can be loaded and extended in the property, rather than
overwritten?

> As we discussed on IRC, launch-control is using the legacy method of
> protection against CSRF, which doesn't require a template tag. Once we
> drop support for django 1.1 we can move away from the legacy method.
> I've added a comment explaining that to the relevant bit of code.

Great, thanks. The test will fail when the settings are changed, so we can
be sure to check all the tests still make sense.

> > 182 + useropenid = UserOpenID(
> > 183 + user=user, claimed_id=self._identity_url,
> > 184 + display_id=self._identity_url)
> >
> > What does that do?
>
> Associates the newly created user with its identity URL.
> (added this as a comment to the code)

Ah, of course, I was assuming that we were testing user creation,
but that doesn't make sense.

> Mostly ignorance. I know close to nothing about what should go in each
> of the settings files of a django project, so I added it to the same
> file that Michael added the lexbuilder one. Happy to move that to
> default_settings.py, though.

launch-control doesn't really use a django standard I don't think.

It has three files two of which can override the defaults. What you
have done is set that variable such that it can't be overriden in
local_settings or deployment_settings like the others can. I would
suggest moving it to default_settings for consistency.

Thanks,

James

Revision history for this message
James Westby (james-w) :
review: Approve
Revision history for this message
Zygmunt Krynicki (zyga) wrote :

221 -LOGIN_REDIRECT_URL = '/dashboard/'

This is a bug and cannot be removed.
It will work in development environment but will fail in production.

Salgado: would you consider merging the changes james mentioned above (settings) from my branch?

review: Needs Fixing
Revision history for this message
Guilherme Salgado (salgado) wrote :

On Fri, 2010-10-15 at 15:28 +0000, James Westby wrote:
> > I did that because the other one has the urls property, and that doesn't
> > include any of the app's urlpatterns as it only uses the test view
> > created for the test. That means the test method here can't post to the
> > 'xml-rpc' endpoint. One benefit of splitting, though, is that this test
> > doesn't need to use CSRFTestCase -- the regular TestCase will do.
>
> It does need CSRFTestCase, otherwise Django disables the csrf checks
> globally, meaning that it is not testing in the right environment. That's
> the sort of issue that means I want to stick to one test class.
>
> > I could try and combine them together again, by having the urls property
> > include all the app's urls plus the one that is specific for testing, if
> > you think it's worth the effort.
>
> I'm don't feel strongly about it, but I would have more confidence in the
> test. Doesn't it just need the xml-rpc view hooked up?
>
> Maybe the test urls can be loaded and extended in the property, rather than
> overwritten?

That's what I was thinking. It was easy to do except for the fact that
reverse("xml-rpc") stopped working, so I had to do
reverse(dashboard_xml_rpc_handler) instead.

> > Mostly ignorance. I know close to nothing about what should go in each
> > of the settings files of a django project, so I added it to the same
> > file that Michael added the lexbuilder one. Happy to move that to
> > default_settings.py, though.
>
> launch-control doesn't really use a django standard I don't think.
>
> It has three files two of which can override the defaults. What you
> have done is set that variable such that it can't be overriden in
> local_settings or deployment_settings like the others can. I would
> suggest moving it to default_settings for consistency.

Fair enough; I've moved it there.

I've also re-added the LOGIN_REDIRECT_URL line as Zygmunt requested.

Now we just need to decide whether we want to have only-OpenID login
enabled or both OpenID and password.

lp:~salgado/lava-dashboard/openid updated
79. By Guilherme Salgado

A bunch of tweaks suggested by James and Zygmunt

Revision history for this message
James Westby (james-w) wrote :

On Fri, 15 Oct 2010 19:49:41 -0000, Guilherme Salgado <email address hidden> wrote:
> Now we just need to decide whether we want to have only-OpenID login
> enabled or both OpenID and password.

I suggest that we land this code as-is, and can change this in a follow
branch if desired.

Thanks,

James

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'dashboard_app/tests/__init__.py'
2--- dashboard_app/tests/__init__.py 2010-10-14 09:08:24 +0000
3+++ dashboard_app/tests/__init__.py 2010-10-15 20:12:47 +0000
4@@ -21,6 +21,7 @@
5 'other.csrf',
6 'other.dashboard_api',
7 'other.deserialization',
8+ 'other.login',
9 'other.misc',
10 'other.test_client',
11 'other.xml_rpc',
12
13=== modified file 'dashboard_app/tests/other/csrf.py'
14--- dashboard_app/tests/other/csrf.py 2010-10-12 14:16:37 +0000
15+++ dashboard_app/tests/other/csrf.py 2010-10-15 20:12:47 +0000
16@@ -22,35 +22,67 @@
17 import xmlrpclib
18
19 import django
20+from django import forms
21+from django.conf.urls.defaults import patterns, url
22 from django.core.urlresolvers import reverse
23+from django.http import HttpResponse
24+from django.template import Template, Context
25
26 from dashboard_app.tests.utils import CSRFTestCase
27+from dashboard_app.views import dashboard_xml_rpc_handler
28+from dashboard_app import urls
29
30
31 class CSRFConfigurationTestCase(CSRFTestCase):
32
33+ @property
34+ def urls(self):
35+ urlpatterns = urls.urlpatterns
36+ urlpatterns += patterns('', url(r'^test-form/', test_form))
37+ return type('urls', (), dict(urlpatterns=urlpatterns))
38+
39 def setUp(self):
40 super(CSRFConfigurationTestCase, self).setUp()
41- self.login_path = reverse("django.contrib.auth.views.login")
42+ self.form_path = reverse(test_form)
43
44- def test_csrf_token_present_in_login_page(self):
45+ def test_csrf_token_present_in_form(self):
46 if django.VERSION[:2] == (1, 1):
47 # This feature is not supported on django 1.1
48 return
49- response = self.client.get(self.login_path)
50+ response = self.client.get(self.form_path)
51 self.assertContains(response, "csrfmiddlewaretoken")
52
53- def test_cross_site_login_fails(self):
54+ def test_cross_site_form_submission_fails(self):
55 if django.VERSION[:2] == (1, 1):
56 # This feature is not supported on django 1.1
57 return
58- response = self.client.post(self.login_path, {
59- 'user': 'user', 'pass': 'pass'})
60+ response = self.client.post(self.form_path, {'text': 'text'})
61 self.assertEquals(response.status_code, 403)
62
63 def test_csrf_not_protecting_xml_rpc_views(self):
64 """call version and check that we didn't get 403"""
65- endpoint_path = reverse("xml-rpc")
66+ endpoint_path = reverse(dashboard_xml_rpc_handler)
67 request_body = xmlrpclib.dumps((), methodname="version")
68 response = self.client.post(endpoint_path, request_body, "text/xml")
69 self.assertContains(response, "<methodResponse>", status_code=200)
70+
71+
72+def test_form(request):
73+ t = Template(template)
74+ html = t.render(Context({'form': SingleTextFieldForm()}))
75+ return HttpResponse(html)
76+
77+
78+class SingleTextFieldForm(forms.Form):
79+ text = forms.CharField()
80+
81+
82+template = """
83+ <html>
84+ <body>
85+ <form action="." method="POST">
86+ <table>{{ form.as_table }}</table>
87+ </form>
88+ </body>
89+ </html>
90+ """
91
92=== added file 'dashboard_app/tests/other/login.py'
93--- dashboard_app/tests/other/login.py 1970-01-01 00:00:00 +0000
94+++ dashboard_app/tests/other/login.py 2010-10-15 20:12:47 +0000
95@@ -0,0 +1,116 @@
96+# Copyright (C) 2010 Linaro Limited
97+#
98+# Author: Guilherme Salgado <guilherme.salgado@linaro.org>
99+#
100+# This file is part of Launch Control.
101+#
102+# Launch Control is free software: you can redistribute it and/or modify
103+# it under the terms of the GNU Affero General Public License version 3
104+# as published by the Free Software Foundation
105+#
106+# Launch Control is distributed in the hope that it will be useful,
107+# but WITHOUT ANY WARRANTY; without even the implied warranty of
108+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
109+# GNU General Public License for more details.
110+#
111+# You should have received a copy of the GNU Affero General Public License
112+# along with Launch Control. If not, see <http://www.gnu.org/licenses/>.
113+
114+"""
115+Tests for the login bits of Launch Control.
116+"""
117+import cgi
118+import httplib
119+
120+from django.conf import settings
121+from django.contrib.auth.models import User
122+from django.test import TestCase
123+
124+from django_openid_auth.models import UserOpenID
125+from django_openid_auth.tests.test_views import StubOpenIDProvider
126+
127+from openid.fetchers import setDefaultFetcher
128+
129+from dashboard_app.tests.utils import TestClient
130+
131+
132+class TestOpenIDLogin(TestCase):
133+
134+ urls = 'django_openid_auth.tests.urls'
135+ _username = 'someuser'
136+ _identity_url = 'http://example.com/identity'
137+
138+ def test_positive_response_from_provider(self):
139+ self.create_user()
140+ openid_request = self.initiate_login()
141+
142+ # Simulate a positive assertion from the server.
143+ openid_response = openid_request.answer(True)
144+
145+ # Use that response to complete the authentication.
146+ response = self.complete(openid_response)
147+
148+ # And the user is now logged in.
149+ response = self.client.get('/getuser/')
150+ self.assertEquals(response.content, self._username)
151+
152+ def test_negative_response_from_provider(self):
153+ openid_request = self.initiate_login()
154+
155+ # Simulate a negative assertion from the server.
156+ openid_response = openid_request.answer(False)
157+
158+ # Use that response to complete the authentication.
159+ response = self.complete(openid_response)
160+
161+ # Since we got a negative assertion from the server, no user is logged
162+ # in.
163+ response = self.client.get('/getuser/')
164+ self.assertEquals(response.content, '')
165+
166+ def setUp(self):
167+ super(TestOpenIDLogin, self).setUp()
168+ # Use StubOpenIDProvider and _identity_url as our fixed SSO so that
169+ # we always get a successful OpenID response for _identity_url.
170+ self.provider = StubOpenIDProvider('http://example.com/')
171+ setDefaultFetcher(self.provider, wrap_exceptions=False)
172+ self.missing_sso_server_url = object()
173+ self.orig_sso_server_url = getattr(
174+ settings, 'OPENID_SSO_SERVER_URL', self.missing_sso_server_url)
175+ settings.OPENID_SSO_SERVER_URL = self._identity_url
176+ self.client = TestClient()
177+
178+ def tearDown(self):
179+ super(TestOpenIDLogin, self).tearDown()
180+ setDefaultFetcher(None)
181+ if self.orig_sso_server_url == self.missing_sso_server_url:
182+ del settings.OPENID_SSO_SERVER_URL
183+ else:
184+ settings.OPENID_SSO_SERVER_URL = self.orig_sso_server_url
185+
186+ def create_user(self):
187+ user = User(username=self._username)
188+ user.save()
189+ # Associate our newly created user with the identity URL.
190+ useropenid = UserOpenID(
191+ user=user, claimed_id=self._identity_url,
192+ display_id=self._identity_url)
193+ useropenid.save()
194+
195+ def initiate_login(self):
196+ response = self.client.get('/openid/login/')
197+ self.assertEqual(httplib.OK, response.status_code)
198+ openid_request = self.provider.parseFormPost(response.content)
199+ self.assertTrue(openid_request.return_to.startswith(
200+ 'http://testserver/openid/complete/'))
201+ return openid_request
202+
203+ def complete(self, openid_response):
204+ """Complete an OpenID authentication request."""
205+ webresponse = self.provider.server.encodeResponse(openid_response)
206+ self.assertEquals(webresponse.code, 302)
207+ redirect_to = webresponse.headers['location']
208+ self.assertTrue(redirect_to.startswith(
209+ 'http://testserver/openid/complete/'))
210+ return self.client.get('/openid/complete/',
211+ dict(cgi.parse_qsl(redirect_to.split('?', 1)[1])))
212
213=== added file 'dashboard_server/context_processors.py'
214--- dashboard_server/context_processors.py 1970-01-01 00:00:00 +0000
215+++ dashboard_server/context_processors.py 2010-10-15 20:12:47 +0000
216@@ -0,0 +1,4 @@
217+from django.conf import settings
218+
219+def login_url(request):
220+ return dict(login_url=settings.LOGIN_URL)
221
222=== modified file 'dashboard_server/default_settings.py'
223--- dashboard_server/default_settings.py 2010-10-02 21:25:06 +0000
224+++ dashboard_server/default_settings.py 2010-10-15 20:12:47 +0000
225@@ -87,6 +87,10 @@
226 SITE_ID = 1
227
228 LOGIN_REDIRECT_URL = '/dashboard/'
229+LOGIN_URL = '/openid/login/'
230+# If you want login to work against your local user DB, uncomment the line
231+# below.
232+# LOGIN_URL = '/accounts/login/'
233
234 # List of callables that know how to import templates from various sources.
235 TEMPLATE_LOADERS = (
236@@ -110,7 +114,8 @@
237 # 1.2. In 1.1 we explicitly use the contrib package in 1.2 we use the
238 # legacy package. This has a small performance hit as the legacy
239 # middleware in 1.2 rewrites the whole response. Once we drop support
240-# for 1.1 we can remove this section.
241+# for 1.1 we can remove this section and use just CsrfViewMiddleware instead
242+# of the legacy CsrfMiddleware.
243 if django.VERSION[:2] == (1, 1):
244 MIDLEWARE_CLASSES = MIDDLEWARE_CLASSES + (
245 'django.contrib.csrf.middleware.CsrfMiddleware',
246@@ -129,7 +134,27 @@
247 'django.contrib.sites',
248 'django.contrib.databrowse',
249 'django.contrib.humanize',
250+ 'django_openid_auth',
251 'dashboard_app',
252 )
253
254+AUTHENTICATION_BACKENDS = (
255+ 'django_openid_auth.auth.OpenIDBackend',
256+ 'django.contrib.auth.backends.ModelBackend',
257+)
258+
259+OPENID_CREATE_USERS = True
260+OPENID_UPDATE_DETAILS_FROM_SREG = True
261+OPENID_SSO_SERVER_URL = 'https://login.launchpad.net/'
262+
263 SERVE_ASSETS_FROM_DJANGO = False
264+
265+TEMPLATE_CONTEXT_PROCESSORS = (
266+ # Django provided context processors
267+ 'django.core.context_processors.auth',
268+ "django.core.context_processors.debug",
269+ "django.core.context_processors.i18n",
270+ "django.core.context_processors.media",
271+ # Launch Control provided context processors
272+ 'dashboard_server.context_processors.login_url',
273+ )
274
275=== modified file 'dashboard_server/manage.py'
276--- dashboard_server/manage.py 2010-09-22 19:31:02 +0000
277+++ dashboard_server/manage.py 2010-10-15 20:12:47 +0000
278@@ -37,5 +37,6 @@
279 sys.stderr.write("Error: Can't find the file 'settings.py' in the directory containing %r. It appears you've customized things.\nYou'll have to run django-admin.py, passing it your settings module.\n(If the file settings.py does indeed exist, it's causing an ImportError somehow.)\n" % __file__)
280 sys.exit(1)
281
282+
283 if __name__ == "__main__":
284 execute_manager(settings)
285
286=== modified file 'dashboard_server/settings.py'
287--- dashboard_server/settings.py 2010-10-04 11:51:54 +0000
288+++ dashboard_server/settings.py 2010-10-15 20:12:47 +0000
289@@ -35,3 +35,7 @@
290 from local_settings import *
291 except ImportError:
292 from development_settings import *
293+
294+# python-openid is too noisy, so we silence it.
295+from openid import oidutil
296+oidutil.log = lambda msg, level=0: None
297
298=== modified file 'dashboard_server/templates/base.html'
299--- dashboard_server/templates/base.html 2010-10-14 09:58:54 +0000
300+++ dashboard_server/templates/base.html 2010-10-15 20:12:47 +0000
301@@ -16,15 +16,14 @@
302 <div id="account_info">
303 {% if user.is_authenticated %}
304 {% blocktrans %}Signed in as {{user}}{% endblocktrans %},
305- <a class="change_password" href="{% url django.contrib.auth.views.password_change %}">{% trans "change password" %}</a>
306- or <a class="sign_out" href="{% url django.contrib.auth.views.logout %}">{% trans "sign out" %}</a>
307+ <a class="sign_out" href="{% url django.contrib.auth.views.logout %}">{% trans "sign out" %}</a>
308 {% if user.is_superuser %}
309 {% trans "or visit " %}<a class="admin_site" href="{% url admin:index %}"
310 >{% trans "admin interface" %}</a>
311 {% endif %}
312 {% else %}
313 {% trans "You are not signed in" %}
314- <a class="sign_in" href="{% url django.contrib.auth.views.login %}">{% trans "Sign In" %}</a>
315+ <a class="sign_in" href="{{ login_url }}">{% trans "Sign In" %}</a>
316 {% endif %}
317 </div>
318 <div id="product_logo">
319
320=== modified file 'dashboard_server/templates/dashboard_app/bundle_stream_list.html'
321--- dashboard_server/templates/dashboard_app/bundle_stream_list.html 2010-10-04 16:05:12 +0000
322+++ dashboard_server/templates/dashboard_app/bundle_stream_list.html 2010-10-15 20:12:47 +0000
323@@ -31,7 +31,7 @@
324 {% endif %}
325 </ul>
326 {% if not user.is_authenticated %}
327-<p>You must <a href="{% url django.contrib.auth.views.login %}"
328+<p>You must <a href="{{ login_url }}"
329 >{% trans "sign in" %}</a> to get more access</p>
330 {% endif %}
331 {% endblock %}
332
333=== modified file 'dashboard_server/urls.py'
334--- dashboard_server/urls.py 2010-10-02 12:23:25 +0000
335+++ dashboard_server/urls.py 2010-10-15 20:12:47 +0000
336@@ -19,6 +19,7 @@
337 from django.conf import settings
338 from django.conf.urls.defaults import *
339 from django.contrib import admin
340+from django.contrib.auth.views import logout
341 from django.contrib import databrowse
342 from django.views.generic.simple import direct_to_template
343
344@@ -67,8 +68,9 @@
345 url(r'xml-rpc/', dashboard_xml_rpc_handler,
346 name='xml-rpc'),
347 url(r'^dashboard/', include('dashboard_app.urls')),
348- url(r'accounts/', include('django.contrib.auth.urls')),
349+ url(r'^logout/$', logout),
350 (r'^admin/', include(admin.site.urls)),
351+ (r'^openid/', include('django_openid_auth.urls')),
352 )
353
354 if settings.SERVE_ASSETS_FROM_DJANGO:

Subscribers

People subscribed via source and target branches