Bug 439628 - memberOf: does not verify all the indirect groups before deleting a memberOf value
memberOf: does not verify all the indirect groups before deleting a memberOf ...
Status: CLOSED ERRATA
Product: 389
Classification: Community
Component: Server - memberOf Plug-in (Show other bugs)
1.1.0
All Linux
low Severity high
: ---
: ---
Assigned To: Nathan Kinder
Chandrasekar Kannan
:
: 454688 (view as bug list)
Depends On:
Blocks: 249650
  Show dependency treegraph
 
Reported: 2008-03-29 16:17 EDT by Andrey Ivanov
Modified: 2015-01-04 18:31 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-08-04 14:21:11 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
CVS Diffs (22.91 KB, patch)
2008-04-18 18:12 EDT, Nathan Kinder
no flags Details | Diff

  None (edit)
Description Andrey Ivanov 2008-03-29 16:17:11 EDT
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12

Description of problem:
Imagine an entry that is a member of a direct group, we have an indirect group and we add this same entry as a DIRECT member to the indirect group. If we delete now the DIRECT membership in the INDIRECT group the indirect membership also disappears from the memberOf attribute values.

Version-Release number of selected component (if applicable):
FDS 1.1, memberOf rev. 1.4

How reproducible:
Always


Steps to Reproduce:
1. Create these three entries:

  dn: cn=group1,dc=example,dc=com
  objectclass: top
  objectClass: groupOfNames
  objectClass: inetUser
  cn: group1

  dn: group2,dc=example,dc=com
  objectclass: top
  objectClass: groupOfNames
  objectClass: inetUser
  cn: group2
  dn: cn=group2

  dn: uid=user1,dc=example,dc=com
  uid: user1
  objectClass: inetorgperson
  objectClass: organizationalPerson
  objectClass: person
  objectClass: top
  objectClass: inetUser
  cn: user
  sn: 1

2. Make group2 a member of group1:

  dn: cn=group1,dc=example,dc=com
  changetype: modify
  add: member
  member: cn=group2,dc=example,dc=com

3. Make user1 a member of group2:

  dn: cn=group2,dc=example,dc=com        
  changetype: modify
  add: member
  member: uid=user1,dc=example,dc=com

4. At this point, the memberOf attribute should be correct in the three test entries:

  dn: cn=group1,dc=example,dc=com
  member: cn=group2,dc=example,dc=com

  dn: cn=group2,dc=example,dc=com
  member: uid=user1,dc=example,dc=com
  memberof: cn=group1,dc=example,dc=com

  dn: uid=user1,dc=example,dc=com
  memberof: cn=group2,dc=example,dc=com
  memberof: cn=group1,dc=example,dc=com

5. Now we make user1 a direct member of group1. I know we would not do it in a correctly written application but "to error is human" :) :

  dn: cn=group1,dc=example,dc=com        
  changetype: modify
  add: member
  member: uid=user1,dc=example,dc=com

6. As for now the memberOf attribute is still correct everywhere. Notice however thet user1 is the member of group1 twice - once as a direct and once as an indirect member.

7. Now let's delete the direct membership of user1 in the group1:
  dn: cn=group1,dc=example,dc=com        
  changetype: modify
  delete: member
  member: uid=user1,dc=example,dc=com


Actual Results:
The memberOf attributes values set is inconsistent, the entry of user1 contains only the memberOf of direct membership :

  dn: uid=user1,dc=example,dc=com
  memberof: cn=group2,dc=example,dc=com

The indirect membership memberOf value is not present.

Expected Results:
To be consistent, the memberOf values should contain the DNs of both groups.

Additional info:
We should verify if the deleted direct 'member' is not at the same an indirect member of the group before actually deleting the DN of a group from the memberOf attribute of the entry. Otherwise we will arrive to a non-consistent memeberOf values set.
Comment 1 Andrey Ivanov 2008-03-30 13:44:38 EDT
The verification should be sort of the one made in
memberof_test_membership_callback().

By the way the comments to this function in the code incorrectly name it
"memberof_memberof_search_callback()" :

Line 1316 :
/* memberof_memberof_search_callback()
 * for each attribute in the memberof attribute
 * determine if the entry is still a member
 * 
 * test each for direct membership
 * move groups entry is memberof to member group
 * test remaining groups for membership in member groups
 * iterate until a pass fails to move a group over to member groups
 * remaining groups should be deleted 
 */ 
Comment 3 Nathan Kinder 2008-04-18 18:12:59 EDT
Created attachment 302951 [details]
CVS Diffs

These diffs fix the reported issue.  The fix is to check if an entry is an
indirect group member before removing the memberOf attribute value form the
entry.

I had to add a few new helper functions to perform this check.	The new utility
functions allow you to check if a specific entry is a member of a specific
group by looking at member values only.  There is a function for checking if
the entry is a direct member, and one that will check for direct or indirect
membership.  Both of these functions will not modify the entry, which is what
was lacking in the previous code.  These utility functions required a recursive
function underneath to trace through indirect memberships.  This recursive
function will detect group loopings to ensure it doesn't endlessly recurse.

I also did some refactoring and commenting of existing code to make things more
clear.	These changes have been checked for memory leaks with valgrind, and
I've also checked for regressions of other recently fixed bugs in the memberOf
plug-in.
Comment 4 Noriko Hosoi 2008-04-21 13:02:05 EDT
Your fix looks good to me.
Comment 5 Nathan Kinder 2008-04-21 13:45:50 EDT
Checked into ldapserver (HEAD).  Thanks to Noriko for her review!

Checking in memberof.c;
/cvs/dirsec/ldapserver/ldap/servers/plugins/memberof/memberof.c,v  <--  memberof.c
new revision: 1.7; previous revision: 1.6
done
Comment 7 Chandrasekar Kannan 2008-07-22 20:21:15 EDT
*** Bug 454688 has been marked as a duplicate of this bug. ***
Comment 8 Yi Zhang 2008-07-24 14:24:34 EDT
Bug verified, test on rhel5.2 64 bit

Test 1:
follow the same test as reported, and test result is pass

Test 2:
follow the same setup, but go further. Total 10 nested groups being created,
user001 is direct member of each nested group, then delete the direct relation
one by one, verify if user001 still memberof each nested group.
The test result: pass


Test output:
server64[07/22/08 18:24]~ >./relation.sh 
u001 successfully added
=== create all ===
grp001 successfully added
grp002 successfully added
grp003 successfully added
grp004 successfully added
grp005 successfully added
grp006 successfully added
grp007 successfully added
grp008 successfully added
grp009 successfully added
=== append all ===
u001 successfully added to grp002
grp002 successfully added to grp001
u001 successfully added to grp003
grp003 successfully added to grp002
u001 successfully added to grp004
grp004 successfully added to grp003
u001 successfully added to grp005
grp005 successfully added to grp004
u001 successfully added to grp006
grp006 successfully added to grp005
u001 successfully added to grp007
grp007 successfully added to grp006
u001 successfully added to grp008
grp008 successfully added to grp007
u001 successfully added to grp009
grp009 successfully added to grp008
=== remove all ===
u001 successfully removed from grp002
dn: cn=grp002,cn=groups,cn=accounts,dc=ipaqa,dc=com
GID: 1155
Full Name: grp002
Description: layer 2
Members:
    user 001: uid=u001,cn=users,cn=accounts,dc=ipaqa,dc=com
    grp003: cn=grp003,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp004: cn=grp004,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp005: cn=grp005,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp006: cn=grp006,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp007: cn=grp007,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp008: cn=grp008,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp009: cn=grp009,cn=groups,cn=accounts,dc=ipaqa,dc=com
 
Continue?

u001 successfully removed from grp003
dn: cn=grp003,cn=groups,cn=accounts,dc=ipaqa,dc=com
GID: 1156
Full Name: grp003
Description: layer 3
Members:
    user 001: uid=u001,cn=users,cn=accounts,dc=ipaqa,dc=com
    grp004: cn=grp004,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp005: cn=grp005,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp006: cn=grp006,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp007: cn=grp007,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp008: cn=grp008,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp009: cn=grp009,cn=groups,cn=accounts,dc=ipaqa,dc=com
 

Continue?

u001 successfully removed from grp004
dn: cn=grp004,cn=groups,cn=accounts,dc=ipaqa,dc=com
GID: 1157
Full Name: grp004
Description: layer 4
Members:
    user 001: uid=u001,cn=users,cn=accounts,dc=ipaqa,dc=com
    grp005: cn=grp005,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp006: cn=grp006,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp007: cn=grp007,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp008: cn=grp008,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp009: cn=grp009,cn=groups,cn=accounts,dc=ipaqa,dc=com
 
Continue?

u001 successfully removed from grp005
dn: cn=grp005,cn=groups,cn=accounts,dc=ipaqa,dc=com
GID: 1158
Full Name: grp005
Description: layer 5
Members:
    user 001: uid=u001,cn=users,cn=accounts,dc=ipaqa,dc=com
    grp006: cn=grp006,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp007: cn=grp007,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp008: cn=grp008,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp009: cn=grp009,cn=groups,cn=accounts,dc=ipaqa,dc=com
 
Continue?

u001 successfully removed from grp006
dn: cn=grp006,cn=groups,cn=accounts,dc=ipaqa,dc=com
GID: 1159
Full Name: grp006
Description: layer 6
Members:
    user 001: uid=u001,cn=users,cn=accounts,dc=ipaqa,dc=com
    grp007: cn=grp007,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp008: cn=grp008,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp009: cn=grp009,cn=groups,cn=accounts,dc=ipaqa,dc=com

 
Continue?

u001 successfully removed from grp007
dn: cn=grp007,cn=groups,cn=accounts,dc=ipaqa,dc=com
GID: 1160
Full Name: grp007
Description: layer 7
Members:
    user 001: uid=u001,cn=users,cn=accounts,dc=ipaqa,dc=com
    grp008: cn=grp008,cn=groups,cn=accounts,dc=ipaqa,dc=com
    grp009: cn=grp009,cn=groups,cn=accounts,dc=ipaqa,dc=com

 
Continue?

u001 successfully removed from grp008
dn: cn=grp008,cn=groups,cn=accounts,dc=ipaqa,dc=com
GID: 1161
Full Name: grp008
Description: layer 8
Members:
    user 001: uid=u001,cn=users,cn=accounts,dc=ipaqa,dc=com
    grp009: cn=grp009,cn=groups,cn=accounts,dc=ipaqa,dc=com


Continue?

u001 successfully removed from grp009
dn: cn=grp009,cn=groups,cn=accounts,dc=ipaqa,dc=com
GID: 1162
Full Name: grp009
Description: layer 9
 

Continue?
 
Comment 9 Yi Zhang 2008-07-24 14:25:25 EDT
the test script is  below:

server64[07/22/08 18:27]~ >cat < relation.sh 
#!/bin/sh
createAll(){
  for N in 1 2 3 4 5 6 7 8 9
  do
    ipa-addgroup -d "layer $N" grp00$N
  done
}

clearAll(){
  for N in 1 2 3 4 5 6 7 8 9
  do
    ipa-delgroup grp00$N
  done
}

appendAll(){
  for N in 2 3 4 5 6 7 8 9
  do
    M=$(expr $N - 1)
    ipa-modgroup -a u001 grp00$N
    ipa-modgroup -g grp00$N grp00$M
  done
}

removeAll(){
  for N in 2 3 4 5 6 7 8 9
  do
    ipa-modgroup -r u001 grp00$N
    ipa-findgroup grp00$N
    echo 
    ipa-finduser -a u001
    echo Continue?
    read
  done
}

findAll(){
  for N in 1 2 3 4 5 6 7 8 9
  do
    ipa-findgroup grp00$N
    ipa-finduser u001
    echo "Continue?"
    read 
  done

}
ipa-adduser -c GECOS -d /home/u001 -f user -l 001 -p redhat123 -s /bin/bash u001
echo "=== create all ==="
createAll 

echo "=== append all ==="
appendAll

#echo "=== find all ==="
#findAll

echo "=== remove all ==="
removeAll

echo "===clear all ==="
clearAll 
ipa-deluser u001
Comment 10 Brian Forte 2008-07-28 01:47:36 EDT
Gentlefolk,

A quick question from the errata gallery.

If I'm understanding the above a-right, when we have

u001   is a        direct member of grp001
u001   is a        direct member of grp002
grp001 is a        direct member of grp002

which means,
u001   is an       indirect member of grp002

if we remove u001 from direct membership of grp2

the old behaviour meant:
u001   was not an  indirect member of grp002

but the new behaviour means:
u001   is still an indirect member of grp002

Correct?

Also, is there any visible or otherwise meaningful difference between being an indirect or direct member of a group? 
The memberOf attribute value appears to be the same either way.

BTW, these questions pertain to the following from the forthcoming errata note.

If my reading above is correct and there is also no difference between a direct or indirect membership, I'm looking 
to go with:

* if an entry was both directly and indirectly a member of a group and the direct membership was removed, the 
indirect membership was, erroneously, also removed. When deleting a direct membership, this updated package 
now checks to see if an entry is also an indirect group member. If an entry is an indirect member of a group, the 
memberOf attribute is not altered.

OTOH, if my reading above is correct and there *is* a difference between a direct and indirect membership, I'm 
looking to go with:

* if an entry was both directly and indirectly a member of a group and the direct membership was removed, the 
indirect membership was, erroneously, also removed. When deleting a direct membership, this updated package 
now checks to see if an entry is also an indirect group member. If an entry is an indirect member of a group, the 
direct membership is still deleted but the indirect membership remains intact.

All responses, suggestions and recipes for really great pavlova gratefully accepted.

Regards,

Brian Forte.
Comment 11 Nathan Kinder 2008-07-28 11:49:42 EDT
(In reply to comment #10)
> Gentlefolk,
> 
> A quick question from the errata gallery.
> 
> If I'm understanding the above a-right, when we have
> 
> u001   is a        direct member of grp001
> u001   is a        direct member of grp002
> grp001 is a        direct member of grp002
> 
> which means,
> u001   is an       indirect member of grp002
> 
> if we remove u001 from direct membership of grp2
> 
> the old behaviour meant:
> u001   was not an  indirect member of grp002
> 
> but the new behaviour means:
> u001   is still an indirect member of grp002
> 
> Correct?

Yes.  The user was always supposed to be an indirect member, but the memberOf
attribute in u001 for grp002 was missing before the fix.

> 
> Also, is there any visible or otherwise meaningful difference between being an
indirect or direct member of a group? 
> The memberOf attribute value appears to be the same either way.

That is correct.  As far as a user entry is concerned, there is no distinction
between a direct group and an indirect group.  All groups are equal, and the
memberOf attribute will appear the same.

> 
> BTW, these questions pertain to the following from the forthcoming errata note.
> 
> If my reading above is correct and there is also no difference between a
direct or indirect membership, I'm looking 
> to go with:
> 
> * if an entry was both directly and indirectly a member of a group and the
direct membership was removed, the 
> indirect membership was, erroneously, also removed. When deleting a direct
membership, this updated package 
> now checks to see if an entry is also an indirect group member. If an entry is
an indirect member of a group, the 
> memberOf attribute is not altered.

This description is accurate.

> 
> OTOH, if my reading above is correct and there *is* a difference between a
direct and indirect membership, I'm 
> looking to go with:
> 
> * if an entry was both directly and indirectly a member of a group and the
direct membership was removed, the 
> indirect membership was, erroneously, also removed. When deleting a direct
membership, this updated package 
> now checks to see if an entry is also an indirect group member. If an entry is
an indirect member of a group, the 
> direct membership is still deleted but the indirect membership remains intact.
> 
> All responses, suggestions and recipes for really great pavlova gratefully
accepted.
> 
> Regards,
> 
> Brian Forte.

Comment 13 errata-xmlrpc 2008-08-04 14:21:11 EDT
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2008-0643.html

Note You need to log in before you can comment on or make changes to this bug.