Bug 439628

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 
 */ 

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.

Your fix looks good to me.

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

*** Bug 454688 has been marked as a duplicate of this bug. ***

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?
 

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

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.

(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.

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