380187 - Bugzilla should support RADIUS authentication

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement)

Description

[Marc Schumann [:Wurblzap]]

Attachment: Patch 0.1

Bugzilla should support RADIUS authentication.

Attached is some work in progress about this.

Comment 1

[Max Kanat-Alexander]

Comment on attachment 264255 [details] [diff] [review]
Patch 0.1

A few notes:

The RADIUS stuff can be on its own tab in the params.

I really like the sortedList stuff. Let's make that into a separate parameter.

Some orders are not OK (like "CGI, Env"), so we should make sure to have some mechanism to prevent certain orders. (Usually it's "X can't go last.")

Comment 2

[Max Kanat-Alexander]

(In reply to comment #1)
> I really like the sortedList stuff. Let's make that into a separate parameter.

  I mean, a separate bug. :-)

Comment 3

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1

Ok, here we go for real.

> The RADIUS stuff can be on its own tab in the params.

I don't think it's a good idea to add to param panel clutter. Instead, let's introduce subgroups in a panel at some time if a panel grows too large.

> I really like the sortedList stuff. Let's make that into a separate parameter.

I need this now, so I want this to go in with this bug. It can easily be polished at one's leisure later, in a separate bug.

> Some orders are not OK (like "CGI, Env"), so we should make sure to have some
> mechanism to prevent certain orders. (Usually it's "X can't go last.")

This doesn't hurt here (it just harmlessly doesn't make much sense), so I don't want to cope with this in this bug.

Comment 4

[Marc Schumann [:Wurblzap]]

Comment on attachment 273414 [details] [diff] [review]
Patch 1

Max, can you please take this review? Frédéric asked me to spare him :)

Comment 5

[Max Kanat-Alexander]

Comment on attachment 273414 [details] [diff] [review]
Patch 1

Okay, without an in-depth review right now:

* Just make a separate RADIUS param panel, don't combine LDAP and RADIUS.

* params.js needs an Original Author section in the license block.

Comment 6

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.1

> * Just make a separate RADIUS param panel, don't combine LDAP and RADIUS.

Adding another panel to clutter the params page is not helpful imo and should be avoided. The new ExtAuth panel will not be overcrowded when merging LDAP and RADIUS. See comment 3 for my suggestion on this.

> * params.js needs an Original Author section in the license block.

Done.

Comment 7

[Max Kanat-Alexander]

Hrm.

I think for now we should just add another panel. If we get too many *panels*, we can add subgroups. That is, we should use the feature we have now, not plan on having some UI organization that we don't have now. It's always possible to clean up the UI in some separate bug, if we think that's needed.

Also, I think many people looking for the LDAP and RADIUS authentication parameters would not recognize "External Authentication" as what they were looking for. "LDAP" is a very clear name, so is "RADIUS".

Comment 8

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.2

Max, you're driving a weak point here. We *already* have panels which are seldom used in comparison to others (Bug Moving and Patch Viewer leading the pack imho). Plus, there are links with the user_verify_class parameter to the LDAP and RADIUS parameters. On top of this, merging LDAP and RADIUS into one panel does use the feature we have now and does not require UI reorganization. Hey, even the parameter name prefixes LDAP and RADIUS_ help.

Anyway. I'm interested in seeing this one in before 3.1 so that I've got less work upgrading. So I've set up two panels as measure to get this moving along.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 274073 [details] [diff] [review]
Patch 1.2

  Thanks for making it a separate panel!

  All the comments below are basically nits.

>+++ ./Bugzilla/Auth/Verify/RADIUS.pm	2007-07-24 09:00:15.618453700 +0200

>    # If we're using RADIUS_mail_suffix, we may need to cut it off from
>    # the login name.
>    if ($address_suffix) {
>        $params->{username} =~ s/\Q$address_suffix\E$//i;
>    }

  Technically, this violates the API of Bugzilla::Auth::Verify:

  "It must avoid changing anything that is already set."

>+    # Create RADIUS object.
>+    my $radius =
>+        new Authen::Radius(Host   => Bugzilla->params->{'RADIUS_server'},
>+                           Secret => Bugzilla->params->{'RADIUS_secret'})
>+        || return { failure => AUTH_ERROR, error => 'radius_preparation_error',
>+                    details => {errstr => Authen::Radius::strerror() } };

  It's impossible for strerror to expose RADUIS_secret, right? (I'm assuming it is, just want to be sure.)

>diff --recursive --exclude=localconfig --exclude=.cvsignore --exclude=.htaccess --exclude=CVS --exclude=data --exclude=custom --exclude=contrib --unified=3 --new-file ../head/Bugzilla/Config/RADIUS.pm ./Bugzilla/Config/RADIUS.pm
>+  {
>+   name =>    'RADIUS_mail_suffix',

  For consistency, let's call this RADIUS_email_suffix.

>+++ ./Bugzilla/Install/Requirements.pm	2007-05-08 23:17:01.674380800 +0200
>@@ -170,6 +170,12 @@
>+        package => 'RadiusPerl',
>+        module  => 'Authen::Radius',
>+        version => 0,
>+        feature => 'Radius Authentication'

  Probably should be caps: RADIUS Authentication.

>+++ ./docs/xml/installation.xml	2007-07-25 08:59:18.105148100 +0200
>+      <variablelist>
>+        <varlistentry id="param-user_verify_class">
>+          <term>user_verify_class</term>
>+          <listitem>
>+            <para>This parameter should be set to <quote>RADIUS</quote>
>+            <emphasis>only</emphasis> if you will be using a RADIUS server
>+            for authentication.

  Why is that true? Why even go to the work of making the "o" param-type if this is true?

>--- ../head/js/params.js	1970-01-01 01:00:00.000000000 +0100
>+++ ./js/params.js	2007-07-24 08:58:24.759045700 +0200
>@@ -0,0 +1,68 @@

>+function sortedList_moveItem(paramName, direction, separator) {
>+    var select = document.getElementById('select_' + paramName);
>+    var inputField = document.getElementById('input_' + paramName);
>+    var currentIndex = select.selectedIndex;
>+    var newIndex = currentIndex + direction;
>+    var optionCurrentIndex;
>+    var optionNewIndex;
>+
>+    /* Return if select control not found */
>+    if (! select) return;

  Why silently return and not just throw an error? I think if there's no select, that's an error.

  By the way, just so you know, admin pages can require JS, so if you don't want to offer a non-JS option, that's OK. (If that would make this any easier.)

>+++ ./skins/standard/params.css	2007-05-09 17:46:06.963889600 +0200
>+.sortlist_separator {
>+    font-weight: bold;
>+    font-size: 80%;
>+    background-color: #dddddd;

  Nit: That can be written #ddd.

>+++ ./template/en/default/admin/params/common.html.tmpl	2007-05-09 17:51:50.668112000 +0200
>@@ -53,6 +56,56 @@
>+        <script type="text/javascript"><!--
>+          document.write("<span style=\"display: none\">");
>+        // -->
>+        </script>

  I don't think we support any browsers that are so old that you have to do the <!-- //--> thing.

>+        <script type="text/javascript"><!--
>+          document.write(
>+            '<table border="0">' +

  border="0" is unnecessary. Also, isn't it possible to have a multi-line JS string, instead of doing all of these + things?

>+            '        <option class="sortlist_separator"' +
>+            '                value="[% sortlist_separator %]">active&uarr;&nbsp;&darr;inactive<\/option>' +

  You can do disabled="disabled" on that option.

>+++ ./template/en/default/global/code-error.html.tmpl	2007-07-15 13:14:05.291747200 +0200
>+  [% ELSIF error == "radius_preparation_error" %]
>+    An error occurred while preparing for a RADIUS authentication request:
>+    <code>[% errstr FILTER html %]</code>.

  Let's say, "while connecting to your RADIUS server" instead of "while preparing for a RADIUS authentication request".


  I just need an answer to the question I asked about the docs, and then this is probably r+.

Comment 10

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.3

(In reply to comment #9)
>   Technically, this violates the API of Bugzilla::Auth::Verify:
> 
>   "It must avoid changing anything that is already set."

Fixed.

>   It's impossible for strerror to expose RADUIS_secret, right? (I'm assuming it
> is, just want to be sure.)

I don't know, so we might consider doing a s/$password/xxxxx/g or something. I'm willing to do this if you insist.
LDAP.pm doesn't do that either, though.

>   For consistency, let's call this RADIUS_email_suffix.

I don't mind. Done.

> >+        feature => 'Radius Authentication'
> 
>   Probably should be caps: RADIUS Authentication.

Right. Glitch fixed.

> >+            <para>This parameter should be set to <quote>RADIUS</quote>
> >+            <emphasis>only</emphasis> if you will be using a RADIUS server
> >+            for authentication.
> 
>   Why is that true? Why even go to the work of making the "o" param-type if
> this is true?

Well, I basically copied the LDAP text in the same file over. I reworded the thing now, maybe it's clearer now. Plus, I reworded the LDAP part so that it matches by changes.

We can polish this up later.

>   Why silently return and not just throw an error? I think if there's no
> select, that's an error.

I removed the check. It doesn't make much sense and seems to be irritating. (Other occurrences fixed as well.)

>   By the way, just so you know, admin pages can require JS, so if you don't
> want to offer a non-JS option, that's OK. (If that would make this any easier.)

I know. It wasn't too bad to have a non-JS option here, so I added it. I see that putting all the HTML by JS doesn't help readability, so I wouldn't oppose converting it to plain HTML later.

> >+    background-color: #dddddd;
> 
>   Nit: That can be written #ddd.

Yes, but for some reason I don't like this very much.

>   I don't think we support any browsers that are so old that you have to do the
> <!-- //--> thing.

If it doesn't hurt, there's no reason not to keep it, seeing that it doesn't hurt readability.

> >+        <script type="text/javascript"><!--
> >+          document.write(
> >+            '<table border="0">' +
> 
>   border="0" is unnecessary. Also, isn't it possible to have a multi-line JS
> string, instead of doing all of these + things?

Removed border="0".
Multi-line JS strings are not possible. (Google suggests hacks to use them, but I don't think we should do this. Maybe we can rather get rid of the ugly _ concatenations in params templates.)

> >+            '        <option class="sortlist_separator"' +
> >+            '                value="[% sortlist_separator %]">active&uarr;&nbsp;&darr;inactive<\/option>' +
> 
>   You can do disabled="disabled" on that option.

Yeah, that's good. Done.

> >+    An error occurred while preparing for a RADIUS authentication request:
> >+    <code>[% errstr FILTER html %]</code>.
> 
>   Let's say, "while connecting to your RADIUS server" instead of "while
> preparing for a RADIUS authentication request".

No; at this time, we're not actually connecting. It's really just a preparation of the object.

Comment 11

[Max Kanat-Alexander]

Comment on attachment 274981 [details] [diff] [review]
Patch 1.3

Looks good! :-) Thanks for all your work on this, Wurblzap. :-)

Comment 12

[Max Kanat-Alexander]

Note that I haven't tested the actual RADIUS part of this, but I assume you have. I did test the UI.

Comment 13

[Marc Schumann [:Wurblzap]]

On check-in, I needed to fix some minor filtering issues in template/en/default/admin/params/common.html.tmpl, brought to light by runtests.pl.

RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/RADIUS.pm,v done
Checking in Bugzilla/Auth/Verify/RADIUS.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/RADIUS.pm,v  <--  RADIUS.pm
initial revision: 1.1
done
Checking in Bugzilla/Config/Auth.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Auth.pm,v  <--  Auth.pm
new revision: 1.3; previous revision: 1.2
done
Checking in Bugzilla/Config/Common.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Common.pm,v  <--  Common.pm
new revision: 1.16; previous revision: 1.15
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/RADIUS.pm,v
done
Checking in Bugzilla/Config/RADIUS.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/RADIUS.pm,v  <--  RADIUS.pm
initial revision: 1.1
done
Checking in Bugzilla/Install/Requirements.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Requirements.pm,v  <--  Requirements.pm
new revision: 1.36; previous revision: 1.35
done
Checking in docs/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/installation.xml,v  <--  installation.xml
new revision: 1.140; previous revision: 1.139
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/js/params.js,v
done
Checking in js/params.js;
/cvsroot/mozilla/webtools/bugzilla/js/params.js,v  <--  params.js
initial revision: 1.1
done
Checking in skins/standard/params.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/params.css,v  <--  params.css
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.105; previous revision: 1.104
done
Checking in template/en/default/admin/params/auth.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/auth.html.tmpl,v  <--  auth.html.tmpl
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/admin/params/common.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/common.html.tmpl,v  <--  common.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/admin/params/editparams.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/editparams.html.tmpl,v  <--  editparams.html.tmpl
new revision: 1.6; previous revision: 1.5
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/radius.html.tmpl,v
done
Checking in template/en/default/admin/params/radius.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/radius.html.
tmpl,v  <--  radius.html.tmpl
initial revision: 1.1
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.101; previous revision: 1.100
done

Comment 14

[Marc Schumann [:Wurblzap]]

Attachment: Fix Tinderbox bustage

Comment 15

[Frédéric Buclin]

Comment on attachment 275108 [details] [diff] [review]
Fix Tinderbox bustage

Yes, this fixes the problem. r=LpSolit, a=LpSolit.

Comment 16

[Marc Schumann [:Wurblzap]]

Checking in t/Support/Files.pm;
/cvsroot/mozilla/webtools/bugzilla/t/Support/Files.pm,v  <--  Files.pm
new revision: 1.23; previous revision: 1.22
done

Comment 17

[Frédéric Buclin]

Attachment: Fix docs bustage

Documentation cannot compile because two IDs have the same name.

Comment 18

[Frédéric Buclin]

Checking in docs/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/installation.xml,v  <--  installation.xml
new revision: 1.141; previous revision: 1.140
done

Comment 19

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.