320751 - LDAP: Ability to have Bugzilla use the LDAP username directly as the Bugzilla username

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement)

Description

[guillomovitch]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.7.12) Gecko/20050921 Firefox/1.0.7 Mandriva/1.0.7-4mdk (2006.1)
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.7.12) Gecko/20050921 Firefox/1.0.7 Mandriva/1.0.7-4mdk (2006.1)

Current implementation only allow fixed mapping, using mail attribute in LDAP user entry. The attached patch provides a way to use another alternative mapping, using a fixed domain name.

Reproducible: Always

Comment 1

[guillomovitch]

Attachment: Forementionned patch

Comment 2

[guillomovitch]

Attachment: fixed patch

The previous patch had a problem in parameter validation, and failed to untaint a value. This is a corrected and tested version.

Comment 3

[Vlad Dascalu]

Comment on attachment 206533 [details] [diff] [review]
fixed patch

This patch this applies cleanly!

+                    "cannot_retreive_attr",

retrieve. Spelling error. Eww... we'll have to fix that, both here and in ./template/en/default/account/auth/ldap-error.html.tmpl

+        # Map LDAP login against bugzilla login

Upon checkin, bugzilla should probably be capitalized ('Bugzilla').

Regarding the big-picture issue when LDAPusermapping is set to domain: I thought about always mapping the LDAP login to the mail username in this case, and then use the emailsuffix parameter in order to send mail to the right location. This would remove the duplication between the already existing emailsuffix param, and the new LDAPmaildomain that you want to introduce.

However, this will not work, because we can use as Auth scheme both DB and LDAP, and an admin could want different values for either of them. So your patch is correct from this point of view.

Also, please run runtests.pl after commiting patches. This breaks the tree because you used "Bugzilla" in a template name:

+  LDAPusermapping => "The exact method used to map LDAP users against Bugzilla users.",

+  LDAPmaildomain => "The mail domain appended to LDAP user's login name to form Bugzilla " _

I'll fix that upon checkin as well if the one doing the approval flag will agree.

Comment 4

[Vlad Dascalu]

Comment on attachment 206533 [details] [diff] [review]
fixed patch

Actually, we'll need a comment as well for that trick_taint thing, in order to explain why it's ok to do it.

And as a question, how do we detect invalid accounts? --I'm asking because the comment:

+        # OK, so now we know that the user is valid. Lets try finding them
+        # in the Bugzilla database

remained positioned only in the 'attribute' code.

I think it would be safe to upload a new patch with those fixes, in order to be sure that we don't break runtests.pl and that everything is ok for commit.

Comment 5

[Vlad Dascalu]

Comment on attachment 206533 [details] [diff] [review]
fixed patch

s/this/still/ in my first comment.

A real reason for review- is that this patch is incorrect:

     my $user_entry = $mesg->shift_entry if !$mesg->code && $mesg->count;
-    if(!$user_entry || !$user_entry->exists(Param("LDAPmailattribute"))) {
-        return (AUTH_ERROR, undef,
 ...
-    }
     ...
+    if (Param("LDAPusermapping") eq 'attribute') {
+        if(!$user_entry || !$user_entry->exists(Param("LDAPmailattribute"))) {
+            return (AUTH_ERROR, undef,
 ...

You're moving the !$user_entry check only in the LDAPusermapping='attribute' scenario. You still need to perform that !$user_entry for the scenario where LDAPusermapping eq 'domain'. And actually, this is what allows you after that to trick taint the $username (the confirmation that the search found it)!

Comment 6

[guillomovitch]

Attachment: Corrected patch

Here is an updated patch. As usual, feel free to fix details directly if needed.

>Regarding the big-picture issue when LDAPusermapping is set to domain: I
>thought about always mapping the LDAP login to the mail username in this case,
>and then use the emailsuffix parameter in order to send mail to the right
>location. This would remove the duplication between the already existing
>emailsuffix param, and the new LDAPmaildomain that you want to introduce.
I disagree, as email adress is not a mandatory attribute in a LDAP directory. And you miss the purpose of this patch. On a host with different virtual domains, a user foo has many valid adresses, such as [e-mail], [e-mail], etc..., but there is only one to use for a given bug database (this is related to having different bug databases on the same host, see #282686).

>Also, please run runtests.pl after commiting patches. This breaks the tree
>because you used "Bugzilla" in a template name.
I did, without triggering any error:
...
t/004template......ok
...
All tests successful.
I guess I have to use 'bugzilla' instead.

>And as a question, how do we detect invalid accounts?
By first finding the entry corresponding to this user with anonymous binding (meaning it exists), then binding with this user credentials (meaning it is autentified correctly). I removed the comment, which was misleading.

>You're moving the !$user_entry check only in the LDAPusermapping='attribute'
>scenario. You still need to perform that !$user_entry for the scenario where
>LDAPusermapping eq 'domain'.
Good point. Actually, I reworked the code a little bit to do the full search only once, rather than once just for DN anonymously, then will all attributes a second time once authenticated. Unless you imagine a situation where anonymous binding would have more perms than authenticated binding, it should not change much, and even get a bit faster.

Comment 7

[Emmanuel Seyman]

> All tests successful.
> I guess I have to use 'bugzilla' instead.

It's actually [% terms.Bugzilla %] and you'll need to include a :

[% PROCESS global/variables.none.tmpl %]

line at the beginning of your file.

Comment 8

[guillomovitch]

Attachment: Corrected patch

Template fixed.

Comment 9

[Frédéric Buclin]

Comment on attachment 215689 [details] [diff] [review]
Corrected patch

This patch no longer applies cleanly due to the rewrite of the Auth stuff, see bug 300410.

Comment 10

[guillomovitch]

Attachment: Updated patch

Comment 11

[Max Kanat-Alexander]

Comment on attachment 223088 [details] [diff] [review]
Updated patch

>+  LDAPusermapping => "The exact method used to map LDAP users against " _
>+                     "$terms.Bugzilla users.",

  This needs a much better description. Also, use "to" instead of "against.

>+
>   LDAPmailattribute => "The name of the attribute of a user in your directory that " _
>                        "contains the email address.",

  Explain that this is only used for a certain LDAPusermapping.

>+  LDAPmaildomain => "The mail domain appended to LDAP user's login name to " _
>+                    "form $terms.Bugzilla user's login name.",
>+

  And the same here.

Comment 12

[guillomovitch]

Attachment: More simpler patch

This one defect to emailsuffix if LDAPmailattribute is undefined

Comment 13

[Max Kanat-Alexander]

Comment on attachment 223090 [details] [diff] [review]
More simpler patch

>+        $params->{bz_username} = $username . '@' . Param("emailsuffix");

  You actually don't need to append emailsuffix here -- the Auth system will do that for you later.

  Add a sentence to the LDAPmailattribute description that explains what happens if you leave it blank.

Comment 14

[guillomovitch]

Attachment: Corrected patch

Comment 15

[Max Kanat-Alexander]

Comment on attachment 223096 [details] [diff] [review]
Corrected patch

>   LDAPmailattribute => "The name of the attribute of a user in your directory that " _
>-                       "contains the email address.",
>+                       "contains the email address, to be used as " _ 
>+                       "$terms.Bugzilla user name. If undefined, email "_ 
>+                       "suffix parameter will be appended to the user " _
>+                       "login name.",

  Instead, say:

  "If this parameter is empty, Bugzilla will use the LDAP username as the Bugzilla username. You may also want to set the "emailsuffix" parameter, in this case."

  It's r+ with that.

Comment 16

[Frédéric Buclin]

Please post an updated patch.

Comment 17

[guillomovitch]

Attachment: Corrected patch

Please fix wording directly if needed.

Comment 18

[Max Kanat-Alexander]

Okay, I fixed the wording directly, on checkin.

Checking in Bugzilla/Auth/Verify/LDAP.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify/LDAP.pm,v  <--  LDAP.pm
new revision: 1.10; previous revision: 1.9
done
Checking in docs/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/installation.xml,v  <--  installation.xml
new revision: 1.119; previous revision: 1.118
done
Checking in template/en/default/admin/params/ldap.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/ldap.html.tmpl,v  <--  ldap.html.tmpl
new revision: 1.3; previous revision: 1.2
done

Comment 19

[Vlad Dascalu]

cvs -q commit -m "Follow-up spelling patch for bug 320751: 'adress' should be 'address' in instalation.xml docs."

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

Comment 20

[guillomovitch]

Attachment: Add-on to the previous patch

I just tested on a live system, and I'm realizing dropping the original ldap-specific mail suffix parameter was an error, because a global suffix will affact all bugzilla users, wether local ones (autenticated through LDAP) or remote ones (autenticated through DB).

Hence resurrection of this part of my original patch, adapted to current trunk state.

Comment 21

[Frédéric Buclin]

Guillaume, I would prefer you to attach your patch in a separate bug, because your previous patch has already been checked in and the approval+ flag is not about your last patch.

Comment 22

[Max Kanat-Alexander]

Could you file a separate bug for this patch, since this one already has a+? We're going to leave this one as FIXED.

Comment 23

[Max Kanat-Alexander]

Added to the release notes in bug 349423.

Comment 24

[guillomovitch]

Separate bug filled as #370432 

Comment 25

[Frédéric Buclin]

Comment on attachment 252247 [details] [diff] [review]
Add-on to the previous patch

This patch has been resubmitted in bug 370432.