Bug 699458

+++ This bug was initially created as a clone of Bug #695779 +++

Description of problem:
A group is sync'd between AD and 389.  Members come over fine.  Certain LDAP clients, when adding a new "uniqueMember" in 389, cause all old members to be immediately deleted on AD (and shortly after deleted from 389 when the dirsync runs).

Some LDAP clients are "dumb" and simply delete the attr and re-add it with all the new values (rather than adding only the ones that are missing).  windows_map_mods_for_replay calls mod_already_made on each mod that is about to be sync'd.  The "dumb" clients have a mod list like [ "delete": "member", "add": "member": [ $existingDN1, $existingDN2, ..., $newDN ] ].  mod_already_made prunes out the adds that already exist in AD, leaving only the new value.  Now we have: [ "delete": "member", "add": "member": [ $newDN ] ].
If I do a ldapmodify by hand and only add the new values (changetype: modify, add: uniqueMember) it works fine.
windows_map_mods_for_replay needs some logic to not prune "add" values from a just deleted attribute.


Version-Release number of selected component (if applicable):
1.2.7.5


How reproducible:
Use any python ldap client (e.g. luma) to add a uniqueMember to a group that is sync'd between AD and 389.  Watch the immediate removal of all old members from the group in AD (with only the new remaining) and the eventual removal of all old group members from 389 as well (when the dirsync hits).


Actual results:
Old members (values) are deleted.


Expected results:
Old values are preserved.


Additional info:
See /usr/lib64/python2.6/site-packages/ldap/modlist.py (python-ldap), modifyModlist.  Right near the end it does:

      if replace_attr_value:
        modlist.append((ldap.MOD_DELETE,attrtype,None))
        modlist.append((ldap.MOD_ADD,attrtype,new_value))

--- Additional comment from nkinder on 2011-04-21 17:24:21 EDT ---

For changes that are sent from DS->AD, we keep a changelog of operations to replay.  What we are dealing with here is essentially 2 mods in one modify operation.  Here's what the operation looks like in LDIF update format:

===========================================
dn: cn=group,dc=example,dc=com
changetype: modify
delete: uniquemember
-
add: uniquemember
uniquemember: uid=user,dc=example,dc=com
===========================================

This is saying to delete all uniquemember values, then add a single value.  It is the same thing as a replace modify operation.  This operation should be played back as is to AD, but we try to skip sending mods that have already been made (this is done to prevent looping mods).  This individual processing of mods to skip stuff already in AD is causing problems since it it done prior to sending any of the mods.  If the first mod makes a change that affects the results of a second mod in the same modify operation, the results could be incorrect.

It seems like we need to process through all of the mods in a single modify operation to determine what exactly needs to be sent to AD.  We may need to create some sort of "resulting entry" in memory to show what the AD entry should look like to determine what mods to make.

--- Additional comment from nkinder on 2011-04-21 18:48:20 EDT ---

Created attachment 494021 [details]
Patch

--- Additional comment from rmeggins on 2011-04-21 18:59:21 EDT ---

Comment on attachment 494021 [details]
Patch

Ok - but note that this changes the server code and the slapi api too.  You could do it without any of those changes by doing 
LDAPMod *modary[2] = {slapi_mod_get_ldapmod_byref(smod), NULL};
slapi_entry_apply_mods(ad_entry, modary);

--- Additional comment from nkinder on 2011-04-25 10:58:11 EDT ---

(In reply to comment #3)
> Comment on attachment 494021 [details]
> Patch
> 
> Ok - but note that this changes the server code and the slapi api too.  You
> could do it without any of those changes by doing 
> LDAPMod *modary[2] = {slapi_mod_get_ldapmod_byref(smod), NULL};
> slapi_entry_apply_mods(ad_entry, modary);

Yes, I added slapi_entry_apply_mod(), as it seemed useful to expose via slapi.  If you feel we should not expse that function, I can use slapi_entry_apply_mods() instead.

--- Additional comment from rmeggins on 2011-04-25 12:05:29 EDT ---

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 494021 [details]
> > Patch
> > 
> > Ok - but note that this changes the server code and the slapi api too.  You
> > could do it without any of those changes by doing 
> > LDAPMod *modary[2] = {slapi_mod_get_ldapmod_byref(smod), NULL};
> > slapi_entry_apply_mods(ad_entry, modary);
> 
> Yes, I added slapi_entry_apply_mod(), as it seemed useful to expose via slapi. 
> If you feel we should not expse that function, I can use
> slapi_entry_apply_mods() instead.

I guess since we have to release 389-ds-base too, we can go ahead and do this.

--- Additional comment from nkinder on 2011-04-25 13:47:42 EDT ---

Pushed to master.  Thanks to Rich for his review!

Counting objects: 19, done.
Delta compression using up to 2 threads.
Compressing objects: 100% (10/10), done.
Writing objects: 100% (10/10), 1.70 KiB, done.
Total 10 (delta 8), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
   11c8bf1..fb7aee0  master -> master

--- Additional comment from nkinder on 2011-04-25 13:58:18 EDT ---

Pushed to 389-ds-base-1.2.8 branch.

Counting objects: 19, done.
Delta compression using up to 2 threads.
Compressing objects: 100% (10/10), done.
Writing objects: 100% (10/10), 1.69 KiB, done.
Total 10 (delta 8), reused 0 (delta 0)
Auto packing the repository for optimum performance.
To ssh://git.fedorahosted.org/git/389/ds.git
   c51c77b..2be27d3  128-local -> 389-ds-base-1.2.8