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

[Guilherme Salgado]

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

[James Westby (james-w)]

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

[Guilherme Salgado (salgado)]

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>

[James Westby (james-w)]

> 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

[James Westby (james-w)]

review: Approve

[Zygmunt Krynicki (zyga)]

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

[Guilherme Salgado (salgado)]

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]

79. By Guilherme Salgado on 2010-10-15

A bunch of tweaks suggested by James and Zygmunt

[James Westby (james-w)]

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

=== modified file 'dashboard_app/tests/__init__.py'
--- dashboard_app/tests/__init__.py	2010-10-14 09:08:24 +0000
+++ dashboard_app/tests/__init__.py	2010-10-15 20:12:47 +0000
@@ -21,6 +21,7 @@
     'other.csrf',
     'other.dashboard_api',
     'other.deserialization',
+    'other.login',
     'other.misc',
     'other.test_client',
     'other.xml_rpc',
 
=== modified file 'dashboard_app/tests/other/csrf.py'
--- dashboard_app/tests/other/csrf.py	2010-10-12 14:16:37 +0000
+++ dashboard_app/tests/other/csrf.py	2010-10-15 20:12:47 +0000
@@ -22,35 +22,67 @@
 import xmlrpclib
 
 import django
+from django import forms
+from django.conf.urls.defaults import patterns, url
 from django.core.urlresolvers import reverse
+from django.http import HttpResponse
+from django.template import Template, Context
 
 from dashboard_app.tests.utils import CSRFTestCase
+from dashboard_app.views import dashboard_xml_rpc_handler
+from dashboard_app import urls
 
 
 class CSRFConfigurationTestCase(CSRFTestCase):
 
+    @property
+    def urls(self):
+        urlpatterns = urls.urlpatterns
+        urlpatterns += patterns('', url(r'^test-form/', test_form))
+        return type('urls', (), dict(urlpatterns=urlpatterns))
+
     def setUp(self):
         super(CSRFConfigurationTestCase, self).setUp()
-        self.login_path = reverse("django.contrib.auth.views.login")
+        self.form_path = reverse(test_form)
 
-    def test_csrf_token_present_in_login_page(self):
+    def test_csrf_token_present_in_form(self):
         if django.VERSION[:2] == (1, 1):
             # This feature is not supported on django 1.1
             return
-        response = self.client.get(self.login_path)
+        response = self.client.get(self.form_path)
         self.assertContains(response, "csrfmiddlewaretoken")
 
-    def test_cross_site_login_fails(self):
+    def test_cross_site_form_submission_fails(self):
         if django.VERSION[:2] == (1, 1):
             # This feature is not supported on django 1.1
             return
-        response = self.client.post(self.login_path, {
-            'user': 'user', 'pass': 'pass'})
+        response = self.client.post(self.form_path, {'text': 'text'})
         self.assertEquals(response.status_code, 403)
 
     def test_csrf_not_protecting_xml_rpc_views(self):
         """call version and check that we didn't get 403"""
-        endpoint_path = reverse("xml-rpc")
+        endpoint_path = reverse(dashboard_xml_rpc_handler)
         request_body = xmlrpclib.dumps((), methodname="version")
         response = self.client.post(endpoint_path, request_body, "text/xml")
         self.assertContains(response, "<methodResponse>", status_code=200)
+
+
+def test_form(request):
+    t = Template(template)
+    html = t.render(Context({'form': SingleTextFieldForm()}))
+    return HttpResponse(html)
+
+
+class SingleTextFieldForm(forms.Form):
+    text = forms.CharField()
+
+
+template = """
+    <html>
+     <body>
+      <form action="." method="POST">
+       <table>{{ form.as_table }}</table>
+      </form>
+     </body>
+    </html>
+    """
 
=== added file 'dashboard_app/tests/other/login.py'
--- dashboard_app/tests/other/login.py	1970-01-01 00:00:00 +0000
+++ dashboard_app/tests/other/login.py	2010-10-15 20:12:47 +0000
@@ -0,0 +1,116 @@
+# Copyright (C) 2010 Linaro Limited
+#
+# Author: Guilherme Salgado <[e-mail]>
+#
+# This file is part of Launch Control.
+#
+# Launch Control is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License version 3
+# as published by the Free Software Foundation
+#
+# Launch Control is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with Launch Control.  If not, see <http://www.gnu.org/licenses/>.
+
+"""
+Tests for the login bits of Launch Control.
+"""
+import cgi
+import httplib
+
+from django.conf import settings
+from django.contrib.auth.models import User
+from django.test import TestCase
+
+from django_openid_auth.models import UserOpenID
+from django_openid_auth.tests.test_views import StubOpenIDProvider
+
+from openid.fetchers import setDefaultFetcher
+
+from dashboard_app.tests.utils import TestClient
+
+
+class TestOpenIDLogin(TestCase):
+
+    urls = 'django_openid_auth.tests.urls'
+    _username = 'someuser'
+    _identity_url = 'http://example.com/identity'
+
+    def test_positive_response_from_provider(self):
+        self.create_user()
+        openid_request = self.initiate_login()
+
+        # Simulate a positive assertion from the server.
+        openid_response = openid_request.answer(True)
+
+        # Use that response to complete the authentication.
+        response = self.complete(openid_response)
+
+        # And the user is now logged in.
+        response = self.client.get('/getuser/')
+        self.assertEquals(response.content, self._username)
+
+    def test_negative_response_from_provider(self):
+        openid_request = self.initiate_login()
+
+        # Simulate a negative assertion from the server.
+        openid_response = openid_request.answer(False)
+
+        # Use that response to complete the authentication.
+        response = self.complete(openid_response)
+
+        # Since we got a negative assertion from the server, no user is logged
+        # in.
+        response = self.client.get('/getuser/')
+        self.assertEquals(response.content, '')
+
+    def setUp(self):
+        super(TestOpenIDLogin, self).setUp()
+        # Use StubOpenIDProvider and _identity_url as our fixed SSO so that
+        # we always get a successful OpenID response for _identity_url.
+        self.provider = StubOpenIDProvider('http://example.com/')
+        setDefaultFetcher(self.provider, wrap_exceptions=False)
+        self.missing_sso_server_url = object()
+        self.orig_sso_server_url = getattr(
+            settings, 'OPENID_SSO_SERVER_URL', self.missing_sso_server_url)
+        settings.OPENID_SSO_SERVER_URL = self._identity_url
+        self.client = TestClient()
+
+    def tearDown(self):
+        super(TestOpenIDLogin, self).tearDown()
+        setDefaultFetcher(None)
+        if self.orig_sso_server_url == self.missing_sso_server_url:
+            del settings.OPENID_SSO_SERVER_URL
+        else:
+            settings.OPENID_SSO_SERVER_URL = self.orig_sso_server_url
+
+    def create_user(self):
+        user = User(username=self._username)
+        user.save()
+        # Associate our newly created user with the identity URL.
+        useropenid = UserOpenID(
+            user=user, claimed_id=self._identity_url,
+            display_id=self._identity_url)
+        useropenid.save()
+
+    def initiate_login(self):
+        response = self.client.get('/openid/login/')
+        self.assertEqual(httplib.OK, response.status_code)
+        openid_request = self.provider.parseFormPost(response.content)
+        self.assertTrue(openid_request.return_to.startswith(
+            'http://testserver/openid/complete/'))
+        return openid_request
+
+    def complete(self, openid_response):
+        """Complete an OpenID authentication request."""
+        webresponse = self.provider.server.encodeResponse(openid_response)
+        self.assertEquals(webresponse.code, 302)
+        redirect_to = webresponse.headers['location']
+        self.assertTrue(redirect_to.startswith(
+            'http://testserver/openid/complete/'))
+        return self.client.get('/openid/complete/',
+            dict(cgi.parse_qsl(redirect_to.split('?', 1)[1])))
 
=== added file 'dashboard_server/context_processors.py'
--- dashboard_server/context_processors.py	1970-01-01 00:00:00 +0000
+++ dashboard_server/context_processors.py	2010-10-15 20:12:47 +0000
@@ -0,0 +1,4 @@
+from django.conf import settings
+
+def login_url(request):
+    return dict(login_url=settings.LOGIN_URL)
 
=== modified file 'dashboard_server/default_settings.py'
--- dashboard_server/default_settings.py	2010-10-02 21:25:06 +0000
+++ dashboard_server/default_settings.py	2010-10-15 20:12:47 +0000
@@ -87,6 +87,10 @@
 SITE_ID = 1
 
 LOGIN_REDIRECT_URL = '/dashboard/'
+LOGIN_URL = '/openid/login/'
+# If you want login to work against your local user DB, uncomment the line
+# below.
+# LOGIN_URL = '/accounts/login/'
 
 # List of callables that know how to import templates from various sources.
 TEMPLATE_LOADERS = (
@@ -110,7 +114,8 @@
 # 1.2. In 1.1 we explicitly use the contrib package in 1.2 we use the
 # legacy package. This has a small performance hit as the legacy
 # middleware in 1.2 rewrites the whole response. Once we drop support
-# for 1.1 we can remove this section.
+# for 1.1 we can remove this section and use just CsrfViewMiddleware instead
+# of the legacy CsrfMiddleware.
 if django.VERSION[:2] == (1, 1):
     MIDLEWARE_CLASSES = MIDDLEWARE_CLASSES + (
         'django.contrib.csrf.middleware.CsrfMiddleware',
@@ -129,7 +134,27 @@
     'django.contrib.sites',
     'django.contrib.databrowse',
     'django.contrib.humanize',
+    'django_openid_auth',
     'dashboard_app',
 )
 
+AUTHENTICATION_BACKENDS = (
+    'django_openid_auth.auth.OpenIDBackend',
+    'django.contrib.auth.backends.ModelBackend',
+)
+
+OPENID_CREATE_USERS = True
+OPENID_UPDATE_DETAILS_FROM_SREG = True
+OPENID_SSO_SERVER_URL = 'https://login.launchpad.net/'
+
 SERVE_ASSETS_FROM_DJANGO = False
+
+TEMPLATE_CONTEXT_PROCESSORS = (
+    # Django provided context processors
+    'django.core.context_processors.auth',
+    "django.core.context_processors.debug",
+    "django.core.context_processors.i18n",
+    "django.core.context_processors.media",
+    # Launch Control provided context processors
+    'dashboard_server.context_processors.login_url',
+    )
 
=== modified file 'dashboard_server/manage.py'
--- dashboard_server/manage.py	2010-09-22 19:31:02 +0000
+++ dashboard_server/manage.py	2010-10-15 20:12:47 +0000
@@ -37,5 +37,6 @@
     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__)
     sys.exit(1)
 
+
 if __name__ == "__main__":
     execute_manager(settings)
 
=== modified file 'dashboard_server/settings.py'
--- dashboard_server/settings.py	2010-10-04 11:51:54 +0000
+++ dashboard_server/settings.py	2010-10-15 20:12:47 +0000
@@ -35,3 +35,7 @@
     from local_settings import *
 except ImportError:
     from development_settings import *
+
+# python-openid is too noisy, so we silence it.
+from openid import oidutil
+oidutil.log = lambda msg, level=0: None
 
=== modified file 'dashboard_server/templates/base.html'
--- dashboard_server/templates/base.html	2010-10-14 09:58:54 +0000
+++ dashboard_server/templates/base.html	2010-10-15 20:12:47 +0000
@@ -16,15 +16,14 @@
   <div id="account_info">
     {% if user.is_authenticated %}
       {% blocktrans %}Signed in as {{user}}{% endblocktrans %},
-      <a class="change_password" href="{% url django.contrib.auth.views.password_change %}">{% trans "change password" %}</a>
-      or <a class="sign_out" href="{% url django.contrib.auth.views.logout %}">{% trans "sign out" %}</a>
+      <a class="sign_out" href="{% url django.contrib.auth.views.logout %}">{% trans "sign out" %}</a>
       {% if user.is_superuser %}
       {% trans "or visit " %}<a class="admin_site" href="{% url admin:index %}"
         >{% trans "admin interface" %}</a>
       {% endif %}
     {% else %}
       {% trans "You are not signed in" %}
-      <a class="sign_in" href="{% url django.contrib.auth.views.login %}">{% trans "Sign In" %}</a>
+      <a class="sign_in" href="{{ login_url }}">{% trans "Sign In" %}</a>
     {% endif %}
   </div>
   <div id="product_logo">
 
=== modified file 'dashboard_server/templates/dashboard_app/bundle_stream_list.html'
--- dashboard_server/templates/dashboard_app/bundle_stream_list.html	2010-10-04 16:05:12 +0000
+++ dashboard_server/templates/dashboard_app/bundle_stream_list.html	2010-10-15 20:12:47 +0000
@@ -31,7 +31,7 @@
   {% endif %}
 </ul>
 {% if not user.is_authenticated %}
-<p>You must <a href="{% url django.contrib.auth.views.login %}"
+<p>You must <a href="{{ login_url }}"
   >{% trans "sign in" %}</a> to get more access</p>
 {% endif %}
 {% endblock %}
 
=== modified file 'dashboard_server/urls.py'
--- dashboard_server/urls.py	2010-10-02 12:23:25 +0000
+++ dashboard_server/urls.py	2010-10-15 20:12:47 +0000
@@ -19,6 +19,7 @@
 from django.conf import settings
 from django.conf.urls.defaults import *
 from django.contrib import admin
+from django.contrib.auth.views import logout
 from django.contrib import databrowse
 from django.views.generic.simple import direct_to_template
 
@@ -67,8 +68,9 @@
     url(r'xml-rpc/', dashboard_xml_rpc_handler,
         name='xml-rpc'),
     url(r'^dashboard/', include('dashboard_app.urls')),
-    url(r'accounts/', include('django.contrib.auth.urls')),
+    url(r'^logout/$', logout),
     (r'^admin/', include(admin.site.urls)),
+    (r'^openid/', include('django_openid_auth.urls')),
     )
 
 if settings.SERVE_ASSETS_FROM_DJANGO: