Merge lp:~james-w/lava-dashboard/openid into lp:lava-dashboard

Proposed by James Westby
Status: Rejected
Rejected by: James Westby
Proposed branch: lp:~james-w/lava-dashboard/openid
Merge into: lp:lava-dashboard
Diff against target: 69 lines (+15/-3)
4 files modified
dashboard_server/default_settings.py (+12/-1)
dashboard_server/templates/base.html (+1/-1)
dashboard_server/templates/dashboard_app/bundle_stream_list.html (+1/-1)
dashboard_server/urls.py (+1/-0)
To merge this branch: bzr merge lp:~james-w/lava-dashboard/openid
Reviewer Review Type Date Requested Status
Linaro Infrastructure Pending
Review via email: mp+37807@code.launchpad.net

Description of the change

Hi,

Here's the start of integration of django_openid_auth in to
launch-control.

It's small, because all the work is delegated to that library.
It does raise some questions though.

Firstly there are no tests. I don't think dashboard_app should
be testing this, as it's a configuration thing, but you don't
seem to be able to put tests in dashboard_server.

Secondly, the templates required changing, but that seems wrong.
What we really want to be able to do is get at settings.LOGIN_URL
in the templates, but I'm not sure how to do that. Otherwise we
could just have a simple view that does the redirect to LOGIN_URL,
but I don't like that either.

The other questions are more about how we want to use openid. Do
we want to remove access to the accounts pages? Do we want to
use openid for the admin interface? Do we want to allow any openid,
or force the use of Launchpad?

All of that is configuration, so it's more questions for dashboard.linaro.org,
rather than what we want launch-control to support.

Thanks,

James

To post a comment you must log in.
Revision history for this message
Zygmunt Krynicki (zyga) wrote :

I see the following issues:

1) Launchpad SSO does not send an email address, the user should be prompted to give one IMHO. I suspect that this could be simplified if we allow lauchpad to "trust" this application but I don't want to assume we can do that.

2) There should be an option to sign in with username and password for 'model' backend. I don't think we can assume each installation will just use openid for everything.

3) There should be an option to sign in with alternate provider. Or at the very least the log in page should say "sign in with launchpad account" or something like that.

As for testing the project. I don't know if you can test projects, I'll check it out and get back to this. It's actually the same problem as we had with CSRF checks. It's not a part of the application (although that's where we currently placed this).

Other than that it's great to see this running :-)
Great work James!

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

On Thu, 2010-10-07 at 02:11 +0000, Zygmunt Krynicki wrote:
> I see the following issues:
>
> 1) Launchpad SSO does not send an email address, the user should be
> prompted to give one IMHO. I suspect that this could be simplified if
> we allow lauchpad to "trust" this application but I don't want to
> assume we can do that.
>

We actually can count on that. You just need to give your trust root to
the ISD team and ask them to tell SSO that it's fine to send the user's
email (and any other info you may want) to that trust root. You'll also
need to make sure the dashboard asks the SSO for all that information.

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

On 10-10-06 10:11 PM, Zygmunt Krynicki wrote:
> I see the following issues:
>
> 1) Launchpad SSO does not send an email address, the user should be prompted to give one IMHO. I suspect that this could be simplified if we allow lauchpad to "trust" this application but I don't want to assume we can do that.

Salgado has already answered this, but why does launch-control need an
email address?

> 2) There should be an option to sign in with username and password for 'model' backend. I don't think we can assume each installation will just use openid for everything.

That's just the project configuration as I said. Michael gave a way that
the templates can just link to the site configured login url, and
the project can then choose to configure any backend they like
at that URL.

The question is what we want to support on dashboard.linaro.org.

> 3) There should be an option to sign in with alternate provider. Or at the very least the log in page should say "sign in with launchpad account" or something like that.

Again that's a configuration issue. I put in the configuration to only
support login.launchpad.net.

As I asked, the question is what we want to support on dashboard.linaro.org.

Thanks,

James

Revision history for this message
Zygmunt Krynicki (zyga) wrote :

W dniu 07.10.2010 15:46, James Westby pisze:
> > On 10-10-06 10:11 PM, Zygmunt Krynicki wrote:
>> >> I see the following issues:
>> >>
>> >> 1) Launchpad SSO does not send an email address, the user should be prompted to give one IMHO. I suspect that this could be simplified if we allow lauchpad to "trust" this application but I don't want to assume we can do that.
> >
> > Salgado has already answered this, but why does launch-control need an
> > email address?
There are going to be certain interactions that will make use of it
later on. For example, you might be notified of data analysis result.

>> >> 2) There should be an option to sign in with username and password for 'model' backend. I don't think we can assume each installation will just use openid for everything.
> >
> > That's just the project configuration as I said. Michael gave a way that
> > the templates can just link to the site configured login url, and
> > the project can then choose to configure any backend they like
> > at that URL.
So you'd like to allow one of two modes: pure openid or pure
model-based. Curious, it might actually make sense. I wonder however if
we should just support both by default. Less configuration is an
advantage in certain situations. How do you think?

> > The question is what we want to support on dashboard.linaro.org.
I think that we want to support openid there but does each linaro
contributor have launchpad account?

>> >> 3) There should be an option to sign in with alternate provider. Or at the very least the log in page should say "sign in with launchpad account" or something like that.
> >
> > Again that's a configuration issue. I put in the configuration to only
> > support login.launchpad.net.
Not precisely, if we could just allow the user to sign in with arbitrary
openid url that would cover all use cases and is certainly not a bad
default.

> > As I asked, the question is what we want to support on dashboard.linaro.org.
My previous response covers this.

Thanks
ZK

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

> There are going to be certain interactions that will make use of it
> later on. For example, you might be notified of data analysis result.
Ok.

> So you'd like to allow one of two modes: pure openid or pure
> model-based. Curious, it might actually make sense. I wonder however if
> we should just support both by default. Less configuration is an
> advantage in certain situations. How do you think?

No, that's not what I said.

If you set LOGIN_URL to /openid/login and remove /accounts from urls.py
then you configure only-openid.

If you set LOGIN_URL to /accounts/login and remove /openid from urls.py
(and optionally remove the openid backend from AUTHENTICATION_BACKENDS)
then you configure only-password.

If you set LOGIN_URL to a url that shows both a username/password form
and an openid form, which post to the appropriate handlers, and leave
the rest of the configuration as-is then you support both.

It is my opinion that for dashboard.linaro.org we should have only-openid,
but it is configuration tweaks to make use of the others (plus template
tweaks to support both, but you can ship those if you really want).

> > > The question is what we want to support on dashboard.linaro.org.
> I think that we want to support openid there but does each linaro
> contributor have launchpad account?

At this point, yes, they will have. Bugs are tracked on launchpad for
one thing.

Whether /everyone/ will have in the future, I'm not sure, but it's fairly
easy to ask them to sign up for one, as this is dashboard.linaro.org,
not in-partner installations.

> Not precisely, if we could just allow the user to sign in with arbitrary
> openid url that would cover all use cases and is certainly not a bad
> default.

That's what I'm asking about. I think simplicity is important right now, but
it's a simple config change, plus some tests and template tweaks to
support multiple providers later.

Thanks,

James

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'dashboard_server/default_settings.py'
2--- dashboard_server/default_settings.py 2010-10-02 21:25:06 +0000
3+++ dashboard_server/default_settings.py 2010-10-07 01:52:44 +0000
4@@ -86,7 +86,8 @@
5
6 SITE_ID = 1
7
8-LOGIN_REDIRECT_URL = '/dashboard/'
9+LOGIN_URL = '/dashboard/openid/login/'
10+LOGIN_REDIRECT_URL = '/dashboard'
11
12 # List of callables that know how to import templates from various sources.
13 TEMPLATE_LOADERS = (
14@@ -129,7 +130,17 @@
15 'django.contrib.sites',
16 'django.contrib.databrowse',
17 'django.contrib.humanize',
18+ 'django_openid_auth',
19 'dashboard_app',
20 )
21
22+AUTHENTICATION_BACKENDS = (
23+ 'django_openid_auth.auth.OpenIDBackend',
24+ 'django.contrib.auth.backends.ModelBackend',
25+)
26+
27+OPENID_CREATE_USERS = True
28+OPENID_UPDATE_DETAILS_FROM_SREG = True
29+OPENID_SSO_SERVER_URL = 'https://login.launchpad.net/'
30+
31 SERVE_ASSETS_FROM_DJANGO = False
32
33=== modified file 'dashboard_server/templates/base.html'
34--- dashboard_server/templates/base.html 2010-10-04 16:00:50 +0000
35+++ dashboard_server/templates/base.html 2010-10-07 01:52:44 +0000
36@@ -24,7 +24,7 @@
37 {% endif %}
38 {% else %}
39 {% trans "You are not signed in" %}
40- <a class="sign_in" href="{% url django.contrib.auth.views.login %}">{% trans "Sign In" %}</a>
41+ <a class="sign_in" href="{% url django_openid_auth.views.login_begin %}">{% trans "Sign In" %}</a>
42 {% endif %}
43 </div>
44 <div id="product_logo">
45
46=== modified file 'dashboard_server/templates/dashboard_app/bundle_stream_list.html'
47--- dashboard_server/templates/dashboard_app/bundle_stream_list.html 2010-10-04 16:05:12 +0000
48+++ dashboard_server/templates/dashboard_app/bundle_stream_list.html 2010-10-07 01:52:44 +0000
49@@ -31,7 +31,7 @@
50 {% endif %}
51 </ul>
52 {% if not user.is_authenticated %}
53-<p>You must <a href="{% url django.contrib.auth.views.login %}"
54+<p>You must <a href="{% url django_openid_auth.views.login %}"
55 >{% trans "sign in" %}</a> to get more access</p>
56 {% endif %}
57 {% endblock %}
58
59=== modified file 'dashboard_server/urls.py'
60--- dashboard_server/urls.py 2010-10-02 12:23:25 +0000
61+++ dashboard_server/urls.py 2010-10-07 01:52:44 +0000
62@@ -69,6 +69,7 @@
63 url(r'^dashboard/', include('dashboard_app.urls')),
64 url(r'accounts/', include('django.contrib.auth.urls')),
65 (r'^admin/', include(admin.site.urls)),
66+ (r'^openid/', include('django_openid_auth.urls')),
67 )
68
69 if settings.SERVE_ASSETS_FROM_DJANGO:

Subscribers

People subscribed via source and target branches