343614 - [LDAP] Ability to map LDAP groups to Bugzilla groups

Status: NEW

(Bugzilla :: User Accounts, enhancement)

Description

[Stefan Ritz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Bugzilla  Version 2.22; mysql  Ver 12.22 Distrib 4.0.18

If a user is in the LDAP group "foo" and in bugzilla is also a group "foo", it would be nice if bugzilla not only take username, real name and password from LDAP, but also add that user to the group "foo". On the other side the group shouldn't be created in bugzilla, if it doesn't already exists.

This would save quite some time if you control various projects/groups with a lot of users.

Reproducible: Always

Steps to Reproduce:
1. User A is in LDAP group "foo".
2. In bugzilla is a group "foo".
Actual Results:  
3. User A logs in the first time and is NOT member of "foo".
4. Administrator has to put user A in group "foo" by hand.

Expected Results:  
3. User A logs in the first time and is automatically member of "foo".

Comment 1

[Max Kanat-Alexander]

Well, I thought we already had a bug for this somewhere, but I guess this is it! :-)

I agree that the simplest mapping would just be "if they have the same name." However, it should still be a yes/no parameter for whether or not the admin wants the mapping to happen.

Comment 2

[Dmitri Colebatch]

I have a slight twist on this in that I would like to do some mapping from the LDAP groups to the Bugzilla groups.  To paraphrase an email I sent to the user list on 31-Mar-07 here is what I'd like:

We have LDAP groups defined (eg "dev team", "support team", etc).  I would like Bugzilla to map an LDAP group to one or more Bugzilla
groups?  Eg, given the bugzilla groups "product A", "product A read
only", "product A confirm", I would like the following mappings:

dev team: product A
support team: product A confirm
analysis team: product A
business owners: product A read only
users: product A read only

Note that I've just made those mappings up, I'm sure there are holes
in them - but for the purpose of the exercise, I hope this makes what
I'm looking for clear.

Comment 3

[Przemysław Sztoch]

Wiki has support for group mapping. It's very useful.

In LDAP configuration administrator can set list of ldap groups. If user is authenticated, logon page will check current mebership in these groups, and optionaly upgrade bugzilla database.

Of course hashmap (ldap group <-> bugzilla group) will be more comfortable than plain list.

Comment 4

[Doug Warner]

I'm not sure if this applies here, but I would like to see group syncing to some other source (like another mysql db).

At mozdev we have a separate database with projects (groups) which match up with our bugzilla products; it'd be ideal if we could setup a query that would check in this other database to see if a user is a member of a certain group.

Comment 5

[Max Kanat-Alexander]

  Here's some comments of mine from a discussion on the support list about this:

I think that instead of mapping LDAP group names to Bugzilla group names always, we should allow specifying a whole DN for a group, store it in the groups table, and map that group to the Bugzilla group. It's possible that a directory could have multiple groups with the same CN as the Bugzilla group, but not all should be mapped to the Bugzilla group.

It would add a new grant type, GRANT_LDAP, and we'd have to have a new way of noting that the grant was via LDAP, in editusers.cgi.

> Open question: How does Bugzilla get informed about changes in ldap.
> For example it could derive_ldap_groups() on every login of a user.
> This would make sure groups are always current and avoid too many
> LDAP queries.

This is tricky, for sure. I think it has to be updated in several situations:

	1) When setting the ldap_dn attribute of a group.
	2) Whenever a user logs in, for that user.
	3) Whenever the user is edited using editusers.cgi.
	4) Possibly in collectstats.pl, for every user. Although that might
become problematic, performance-wise.

	The problem I'm trying to solve in #4 is *removing* users from groups who haven't yet logged in or been edited, and in strict_isolation situations (where people can't even be CC'ed if they're not in the gruop), adding them to groups that they're in in LDAP when they haven't logged in.

	We could possibly also ship a contrib script that either would fix the groups for all users or just a single user, so that sysadmins could set it up to be triggered when they make LDAP changes.

Comment 6

[Christoph Probst]

What do you think about the following implementation as a first step:

- New GRANT_LDAP is introduced

- On user login the sub create_or_update_user() from Bugzilla/Auth/Verify.pm is called. I would extend this sub to update the user_group_map depending on LDAP (add and remove GRANTs).

- The database table for groups is extended by a new field "LDAPfilter"

- The editgroup.cgi, templates, Group.pm, documentation, etc. are extended to display, handle and explain the new configuration item LDAPfilter. Very similar to everything already present.


This will provide:

- LDAP configuration is made like usual, using the Parameters interface

- Group configuration is made like usual

- Groups can be made LDAP-aware by adding a "membersof" LDAP filter to the group configuration.

- Every user having an extern_id and who can be looked up using a groups LDAP filter, will automatically granted membership to that group.

- If a user is member of a group due to a GRANT_LDAP and no longer member of the corresponding LDAP group (e.g. removed from that group, LDAPfilter changed, etc.), the grant is automatically removed.


This will not yet update grants on group or user edit and there should be a contrib script allowing a full sync of the LDAP grants (e.g. to be used by cron).

It this worth to be submitted as a bunch of patches?

Comment 7

[Christoph Probst]

Attachment: LDAP group support patch

Here is a patch that implements LDAP group support for the current 3.6rc1 branch. I've chosen the RC because I need the patch in a productive environment where I rather don't want to install 3.7. I hope thats OK.

This patch is tested on a Microsoft Active Directory. We are using an LDAPbinddn including a username/password. Not sure how this works with anonymous binds but it shouldn't break anything (the same LDAP filter call implementation is already used for LDAP user authenditcation).

Features:

- You can enter an optional LDAP filter for every Bugzilla group using groupedit.cgi.

- The LDAP filter is verified on submit by performing a test lookup. 

- LDAP configuration is taken from Administration/Parameters/LDAP (no further configuration required if LDAP user login is already working)

- edituser.cgi shows "L", if a users has a GRANT_LDAP for group (like * for GRANT_REGEX)

- LDAP group mappings for a user are updated on every logon: Grants are added or removed depending on the current LDAP filter results for the current user for every group.

- Its possible to use nested LDAP groups by configuring a recursive LDAP filter.

- Using available Bugzilla product/group configuration you can now make  products visible for users, depending on their LDAP group memberships.

Issues:

- the GRANT_LDAP entries of user_group_map are only updated when a user logs on. This leeds to a possible security issue if a users is removed from an LDAP group but never logs into Bugzilla. He might receive email for Bugzilla groups and Bugs he is not supposed to receive anymore.

- Changes to a groups LDAP filter are only applied if a users logs in again. 

- Haven't tested how the patch interacts with permanent cookies (i.e. people that never really log in).

- User.pm should be extended to make extern_id accessable. Currently it is not possible to access extern_id if a user logs on with his email address or if grants should be updated independently of a user login.

Comment 8

[Christoph Probst]

See also bug 423612.

It adds access to $user->extern_id that we require as well.

Comment 9

[Christoph Probst]

Attachment: LDAP Group Mapping Patch

OK, here is my next version for LDAP group mapping integration.

Features:
- Global configuration switch to turn group mapping on/off
- LDAP filter per Bugzilla group. If filter lookup matches user, then he is granted membership to the group.
- Filter configuration and verification (test lookup) from within Bugzilla group configuration

Supports Single Sign On in combination with the Env4AD Bugzilla extension. If you have an AD you could have:

1. User authentication by apache mod_kerb: provides "userPrincipalName"
2. Optional authorization using apache mod_ldap
3. Env4AD LDAP lookup using userPrincipalName: provides "Full name", "E-Mail"
4. Optional authorizaion using Bugzilla global LDAPfilter.
5. Automatic Bugzilla account generation
6. LDAP lookup per Bugzilla group for automatically granting/removing membership

TODOs:

- Find a better spot where LDAP lookup can be performed "on logon". Currently a users group mapping is synced on any access which may be too often for busy sites.
- write contrib script for cronical mapping updates
- review/optimize code
- test other LDAP servers
- port code to 3.7

Comment 10

[Max Kanat-Alexander]

(In reply to comment #9)
> - Global configuration switch to turn group mapping on/off

  I don't want this--it adds complexity. We always want to avoid parameters whenever possible. The fact that Bugzilla has a lot of parameters that turn on and off features is a historical error.

  Also, it shouldn't be using an ldap filter, it should just have a DN to a group.
 
> - Find a better spot where LDAP lookup can be performed "on logon". Currently a
> users group mapping is synced on any access which may be too often for busy
> sites.

  That would be an r-.

  The right solution would be to do it in Verify::LDAP.

> - port code to 3.7

  The code probably won't get seriously reviewed until that happens, because for features, we only accept code written against the trunk.

Comment 11

[Christoph Probst]

(In reply to comment #10)
> > - Global configuration switch to turn group mapping on/off
> I don't want this[...]

OK, this means there the code will try to map groups as soon as there is an ldap_filter defined in the group configuration. Due to check_ldap_filter it will not be possible to save it without a proper LDAP configuration which is an indirect switch.


> Also, it shouldn't be using an ldap filter, it should just have a DN to a
> group.

Reasons for choosing ldap filter:

 - ldap filter is more flexible.
 - user can choose whether to allow recursive lookups or not
 - ldap filter is already used in Bugzilla Parameters

If this is changed to a simple DN, I'm not sure how to implement recursion (sub groups in groups). I don't expect the Active Directory filter "memberOf:1.2.840.113556.1.4.1941:..." to be working with other LDAPs, so it shouldn't be hard coded. On the other hand: Not supporting recursion is not what a user would expect.


> > - Find a better spot where LDAP lookup can be performed "on logon". 
> >   Currently a users group mapping is synced on any access which may
> >   be too often for busy sites.
> 
>   That would be an r-.
> 
>   The right solution would be to do it in Verify::LDAP.

This would be check_credentials in Verify::LDAP. Hence this sub only knows "params" not a user object and I thought it is better to use an object to access user details. I certainly could directly pass params to update_group_mapping().

Downside is: each Verify method would require its own update call and I was rather looking for a generic spot in the code. Hence so far I found no place there I have an updated user object of a users that just logged on.

Comment 12

[Max Kanat-Alexander]

(In reply to comment #11)
> OK, this means there the code will try to map groups as soon as there is an
> ldap_filter defined in the group configuration. Due to check_ldap_filter it
> will not be possible to save it without a proper LDAP configuration which is an
> indirect switch.

  That's fine. Of course, ldap_group (which is probably what it should be called instead of ldap_filter) shouldn't show up in the UI unless LDAP is one of the verify methods.

>  - ldap filter is more flexible.

  Yeah, but very very few people know how to use them. They are a frequent source of support difficulties and confusion, so I'm going to have to veto them for this usage.

>  - user can choose whether to allow recursive lookups or not

  As in, "I'm in this group if I'm in another group that is a member of this group"? I don't see why people would need to customize that. If they want a Bugzilla<->LDAP mapping, it seems like it should be fully accurate.

> This would be check_credentials in Verify::LDAP. Hence this sub only knows
> "params" not a user object and I thought it is better to use an object to
> access user details. I certainly could directly pass params to
> update_group_mapping().

  The thing to do is to put something into $params, and then create_or_update_user in Auth::Verify can do the updating.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 440922 [details] [diff] [review]
LDAP Group Mapping Patch

  Here's a quick code review, too (though this may not catch everything):

>+            LDAPfilter   => {TYPE => 'TINYTEXT', NOTNULL => 1,
>+                             DEFAULT => "''"},

  This should be longer (MEDIUMTEXT) and should be NULL when empty, I think.

>=== added directory 'Bugzilla/Group'
>+    my $filterTest = Bugzilla::Group::LDAP::check_filter($LDAPfilter);
>+    ThrowUserError("invalid_ldapfilter") if (!$filterTest); 

  You should also be telling the user what the LDAP server said about the filter.

  Also, you forgot to update the group mapping when groups are created and updated.

>=== added file 'Bugzilla/Group/LDAP.pm'

  I'm not quite sure that Bugzilla::Group:: is where this belongs, but since you'll need the code in both Bugzilla::Auth and Bugzilla::Group, I suppose this is as good as any place. I think, though, that it would be better to just put this code into Bugzilla::Group.

>+use base qw(Bugzilla::Group);

  Why is this a subclass?

>+    return if (! Bugzilla->params->{"LDAPgroupMapping"}); 

  Nit: You don't need the parens. (This applies in several places.)

>+    # Select all available Bugzilla groups
>+    my @groups = Bugzilla::Group->get_all;

  I think that instead of creating one function that updates every group, you should have a function that updates a single group, and then in some places you can call it in a loop.

>+        my $isMemberOfLDAPgroup = check_group_membership("", $user->extern_id, $group->LDAPfilter);

  Don't camelCase variable names, it's not Perl style. (See "man perlstyle".)

>+        # Basic error handling:
>+        # Do not change GRANTs if LDAP returned an error.
>+        next if ($isMemberOfLDAPgroup==-1);

  I think the user wants to know if there's an error. They probably don't want security-related code to fail without their knowledge.

>+        # if current group has LDAPfilter set and users is member of the LDAP group
>+        if ($group->LDAPfilter && ($isMemberOfLDAPgroup>0))
>+        {

  You don't need to check > 0.

  Also, the { doesn't need to be on the next line.

>+            # and if user hasn't already a GRANT_LDAP for this group
>+            if (!$user->is_group_member($group->id, GRANT_LDAP)) {

  Why did you add a whole new method to Bugzilla::User? Do we not have anything that does this already?

>+                # then remove membership
>+                my $dbh = Bugzilla->dbh;
>+                my $grantadd = $dbh->prepare("DELETE FROM user_group_map

 Shouldn't that variable have a different name?

>+  my $testuser = "bugzilla-test-lookup";

  Ummmm...that will fail, always, no? And why is there a hardcoded string in a variable in the middle of a function?

  Okay, I'm just going to skim over the rest of the patch from here.

>=== added file 'contrib/sync_LDAPgroups.pl'
>--- contrib/sync_LDAPgroups.pl	1970-01-01 00:00:00 +0000
>+++ contrib/sync_LDAPgroups.pl	2010-04-22 18:39:35 +0000
>@@ -0,0 +1,2 @@
>+
>+TODO

  Don't add the file at all if it's TODO.

>=== modified file 'template/en/default/admin/groups/list.html.tmpl'
>+   {name               => 'LDAPfilter'
>+    heading            => 'LDAP Filter'
>+   }

  This shouldn't show up in the list; it will make the table too large.

>=== modified file 'template/en/default/admin/users/edit.html.tmpl'
>                 [% '[' IF perms.derivedmember %]
>+                [% 'L' IF perms.ldapmember %]

  L should come before [, I think, no?

Comment 14

[Morten Kristoffer Hansen]

I need this feature badly, and is willing to pay for it to be implemented, preferably in main tree.

Our setup:
We have a LDAP-server, where users have group relations. A group name is a unique number like 451066

In our Bugzilla instance, we have multiple products. We need to control access to modify the products based on the groups from LDAP. Some products are visible to the public, some are only visible to a group.

In Bugzilla, we have some groups that control more products, these groups are still represented by one group in LDAP.

In the end, I would like to have all access to Bugzilla controlled by our LDAP.

The mapping between LDAP and Bugzilla groups would preferably be done on some settings page in Bugzilla.

We have some existing users in Bugzilla, but I hope they can be migrated to the LDAP authentication, as long as the email address is the same.

How do we go from here?

Does anybody have the time to implement this? How much will it cost? Is there anything in what I have described, that could not make it into the main tree?

Being part of the Danish government, we need to make an official contract before the coding can begin.

Comment 15

[Morten Kristoffer Hansen]

I accidently made the offering above from my personal account. Any inquiries can be sent to the email address associated with this account.

Comment 16

[Max Kanat-Alexander]

  Hi Morten! If you're interested in paying a consultant to implement the feature, the best list of people to look at would be here:

  http://www.bugzilla.org/support/consulting.html

  Most likely, you'll want somebody with the "bug" icon next to their name, since those people are contributors to Bugzilla itself.

Comment 17

[Jon Skarpeteig]

Has there been any development on this issue? This is still a wanted feature :-)

Comment 18

[mozilla.jim]

Not to spam, but I was wondering the same thing.  There seems to be a review of a submitted patch, but then nothing else became of it.  I looked at the ChangeLogs of "4.1.3 -> Today" but did not see anything.  Does someone else need to take up the mantle of this change?

Comment 19

[Max Kanat-Alexander]

(In reply to mozilla.jim from comment #18)
> Does someone else need to take up the mantle of this change?

  Yep.

Comment 20

[Tiago Mello [:timello]]

Attachment: v1

Ok. I know we could split this into 2 phases, but the code made the cron job script simple and short.

This patch allows Bugzilla to sync LDAP groups with Bugzilla groups adding Bugzilla users to the groups when they are member of the corresponding LDAP Group. That happens in 4 situations:

1. When users logs in;
2. When a new Bugzilla group is created with a valid LDAP DN
3. When updating a LDAP Group in Bugzilla
4. When running the contrib script in a cron job

As per Bugzilla::Auth::Verify::LDAP->check_credentials first comment, uid + base dn isn't enough to determine the user's dn, so since we already look for it there, it takes advantage of that and also saves the user's dn in the profiles table. We use user's dn to perform ldap searches in the 'member' and 'uniquemember' attributes so then we can know if the user is whether or not a memmber of the group.

This patch is maybe missing some documentation. Although it's *my* v1, it's a tested and workable patch.

mkanat, I would appreciate if you could have a quick look on it and give your thoughts. 

Thanks!

Comment 21

[Jon Skarpeteig]

Is there any support for recursive groups in this patch? E.G if User1 is member of Group1, and Group1 is a member of Group2. Then I would expect (User1 memberof Group2) === true, even if there's no member attribute of User1 which explicitly sais Group2 (E.G default Active Directory behavior)

Comment 22

[Tiago Mello [:timello]]

(In reply to Jon Skarpeteig from comment #21)
> Is there any support for recursive groups in this patch? E.G if User1 is
> member of Group1, and Group1 is a member of Group2. Then I would expect
> (User1 memberof Group2) === true, even if there's no member attribute of
> User1 which explicitly sais Group2 (E.G default Active Directory behavior)

No, this patch only cares about direct group membership. But maybe we can implement this feature later.

Comment 23

[Rama]

Hi


I am using Bugzilla4.0.1,
  I want to Map the Bugzilla groups to LDAP Groups, Is there any possibility for this.

When I searched for this, I found this link (https://bugzilla.mozilla.org/show_bug.cgi?id=343614)

But,I would like to know whether this has been integrated in bugzilla 4.0.1 version or any other latest version, or is it still a patch which I need to apply over my present version.

Thanks!

Comment 24

[Morten Kristoffer Hansen]

(In reply to Rama from comment #23)
> [...]
> But,I would like to know whether this has been integrated in bugzilla 4.0.1
> version or any other latest version, or is it still a patch which I need to
> apply over my present version.

Hi Rama
This patch is against the trunk, hence it is not certain that it can be applied on a 4.0.1 installation. 
Hopefully it will make it into the trunk and later in the 4.4 release

Comment 25

[Michael Bellomo]

Really would love this feature.

Comment 26

[Frédéric Buclin]

Comment on attachment 584630 [details] [diff] [review]
v1

>=== modified file 'Bugzilla.pm'

>+use Net::LDAP;

It's not ok to require Net::LDAP in Bugzilla. This Perl module is optional, not required.

Comment 27

[Wojciech Wronka]

Could anyone comment on the effort to bring this functionality into trunk in 4.4 release. How likely is it going to happen?

Comment 28

[Tiago Mello [:timello]]

I've created an extension based on the latest patch in this bug. Feel free to provide any feedback. https://github.com/timello/LDAPGroups. It has been tested using the Bugzilla 4.5 (trunk) code.

Comment 29

[Jon Skarpeteig]

Do you support recursive lookup? Filter example: (&(objectClass=person)(memberOf:1.2.840.113556.1.4.1941:=CN=Group_to_be_member_of))

The magic numbers means recursive membership lookup.

This approach is particularly useful against Active Directory installations, which commonly utilize such recursive memberships for its objects.