search

389ds / 389-ds-base

The enterprise-class Open Source LDAP server for Linux
https://www.port389.org/
Other
210 stars 90 forks source link

support posix schema for user and group sync #426

Closed 389-ds-bot closed 4 years ago

389-ds-bot commented 4 years ago

Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/426

Support the new standard posix attributes in 2003 R2 and later, and the MS SFU posix schema.

389-ds-bot commented 4 years ago

Comment from nkinder (@nkinder) at 2012-08-14 02:44:22

Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=847868

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2012-08-14 19:57:05

set default ticket origin to Community

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2012-08-17 06:50:46

0001-Ticket-426-support-posix-schema-for-user-and-group-s.patch 0001-Ticket-426-support-posix-schema-for-user-and-group-s.patch

389-ds-bot commented 4 years ago

Comment from cgrzemba at 2012-08-17 13:57:03

please verify my comment for Issue 8 on https://github.com/cgrzemba/Posix-Winsync-Plugin-for-389-directory-server.

The problem was the forgotten braces on the if - statement

if(posix_winsync_config_get_mapMemberUid())

the next 3 lines must be enclosed in { } .

{ 
  memberUidLock();
  modGroupMembership(ds_entry,smods,do_modify);
  memberUidUnlock();
}
389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2012-08-17 20:16:38

Replying to [comment:4 cgrzemba]:

please verify my comment for Issue 8 on https://github.com/cgrzemba/Posix-Winsync-Plugin-for-389-directory-server.

The problem was the forgotten braces on the if - statement

if(posix_winsync_config_get_mapMemberUid())

the next 3 lines must be enclosed in { } .

{ 
  memberUidLock();
  modGroupMembership(ds_entry,smods,do_modify);
  memberUidUnlock();
}

The original code was like this:

if(posix_winsync_config_get_mapMemberUid())
  memberUidLock();
modGroupMembership(ds_entry,smods,do_modify);
memberUidUnlock();

Which means that if mapmemberuid was true, the only action it would take is to lock the memberuid lock. ''It would always call modGroupMembership - even if mapmemberuid was false''. The only thing wrong with this is that it will always call memberuidunlock(), even if memberuidlock() was not called - this is usually safe but not recommended. So the only thing I did was this:

if(posix_winsync_config_get_mapMemberUid())
  memberUidLock();
modGroupMembership(ds_entry,smods,do_modify);
if(posix_winsync_config_get_mapMemberUid())
  memberUidUnlock();

So the only change I made is to only call memberUidUnlock if memberUidLock was called. My change preserves the old behavior of always calling modGroupMembership.

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2012-08-18 00:26:49

fixed formatting in previous patch 0001-Ticket-426-support-posix-schema-for-user-and-group-s.2.patch

389-ds-bot commented 4 years ago

Comment from cgrzemba at 2012-08-20 20:31:41

... It would always call modGroupMembership - even if mapmemberuid was false. ...

That's wrong: posix_winsync_config_get_mapMemberUid() should control if modGroupMembership(ds_entry,smods,do_modify) is called or not.

Which is not happen because of the missing braces.

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2012-08-20 21:12:53

fix add/modGroupMembership and locking 0001-Ticket-426-support-posix-schema-for-user-and-group-s.3.patch

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2012-08-20 21:13:48

Replying to [comment:7 cgrzemba]:

... It would always call modGroupMembership - even if mapmemberuid was false. ...

That's wrong: posix_winsync_config_get_mapMemberUid() should control if modGroupMembership(ds_entry,smods,do_modify) is called or not.

Which is not happen because of the missing braces.

Ok. I've attached another patch which does this:

            if (posix_winsync_config_get_mapMemberUid()) {
                memberUidLock();
                addGroupMembership(ds_entry, ad_entry);
                memberUidUnlock();
            }

Same with modGroupMembership. Is this correct?

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2012-08-21 00:47:37

1) ldap/servers/plugins/posix-winsync/posix-winsync.c +static void +sync_acct_disable( [...]

I'm just curious. Quite a long list of APIs is passed to slapi_apib_register and registered. Some of them are just noop callbacks (e.g., posix_winsync_dirsync_search_params_cb, posix_winsync_pre_ad_search_cb, posix_winsync_pre_ds_search_entry_cb, posix_winsync_get_new_ds_user_dn_cb, posix_winsync_get_new_ds_group_dn_cb, posix_winsync_can_add_entry_to_ad_cb) May I ask why they are needed? +static void *posix_winsync_api[] = {

2) ldap/servers/plugins/posix-winsync/posix-winsync-config.c +static int +dn_isparent( const Slapi_DN parentdn, const Slapi_DN childdn ) +{ We have an equivalent slapi API: int slapi_sdn_isparent( const Slapi_DN parent, const Slapi_DN child )

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2012-08-21 01:04:49

Very minor issues:

/ldap/servers/plugins/posix-winsync/posix-group-func.c

/ldap/servers/plugins/posix-winsync/posix-winsync.c

All of this is pretty picky, and I don't know winsync well enough to know if these loops are even long running loops. So it might not even matter, but it wouldn't hurt.

And indentation/spacing needs work.

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2012-08-21 01:20:25

Replying to [comment:10 nhosoi]:

1) ldap/servers/plugins/posix-winsync/posix-winsync.c

Will fix in upcoming new patch.

I'm just curious. Quite a long list of APIs is passed to slapi_apib_register and registered. Some of them are just noop callbacks (e.g., posix_winsync_dirsync_search_params_cb, posix_winsync_pre_ad_search_cb, posix_winsync_pre_ds_search_entry_cb, posix_winsync_get_new_ds_user_dn_cb, posix_winsync_get_new_ds_group_dn_cb, posix_winsync_can_add_entry_to_ad_cb) May I ask why they are needed? +static void *posix_winsync_api[] = {

  • NULL, / reserved for api broker use, must be zero /
  • posix_winsync_agmt_init,
  • posix_winsync_dirsync_search_params_cb,
  • posix_winsync_pre_ad_search_cb,
  • posix_winsync_pre_ds_search_entry_cb,
  • posix_winsync_pre_ds_search_all_cb,
  • posix_winsync_pre_ad_mod_user_cb,
  • posix_winsync_pre_ad_mod_group_cb,
  • posix_winsync_pre_ds_mod_user_cb,
  • posix_winsync_pre_ds_mod_group_cb,
  • posix_winsync_pre_ds_add_user_cb,
  • posix_winsync_pre_ds_add_group_cb,
  • posix_winsync_get_new_ds_user_dn_cb,
  • posix_winsync_get_new_ds_group_dn_cb,
  • posix_winsync_pre_ad_mod_user_mods_cb,
  • posix_winsync_pre_ad_mod_group_mods_cb,
  • posix_winsync_can_add_entry_to_ad_cb,
  • posix_winsync_begin_update_cb,
  • posix_winsync_end_update_cb,
  • posix_winsync_destroy_agmt_cb +};

These are useful to have for debugging/debug logging.

2) ldap/servers/plugins/posix-winsync/posix-winsync-config.c +static int +dn_isparent( const Slapi_DN parentdn, const Slapi_DN childdn ) +{ We have an equivalent slapi API: int slapi_sdn_isparent( const Slapi_DN parent, const Slapi_DN child )

Will fix in upcoming patch.

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2012-08-21 01:22:52

Replying to [comment:11 mreynolds389]:

Very minor issues:

/ldap/servers/plugins/posix-winsync/posix-group-func.c

  • slapi_private.h is commented out. Should this be removed so not to expose slapi-private?

Yep. Will remove.

  • overall -for performance we should not be declaring var/struct pointers inside of for/while loops.

/ldap/servers/plugins/posix-winsync/posix-winsync.c

  • Declaring vars inside of loops

We do this in many, many places in the server. I thought that the compiler optimized for this situation.

All of this is pretty picky, and I don't know winsync well enough to know if these loops are even long running loops. So it might not even matter, but it wouldn't hurt.

And indentation/spacing needs work.

I thought I had fixed that. Were you looking at patch https://fedorahosted.org/389/attachment/ticket/426/0001-Ticket-426-support-posix-schema-for-user-and-group-s.2.patch or later?

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2012-08-21 03:20:33

Declaring vars inside of loops

winsync performance is very slow to begin with - this is probably the least of our worries :P

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2012-08-21 03:27:28

fix issues found by nhosoi and mreynolds389 0001-Ticket-426-support-posix-schema-for-user-and-group-s.4.patch

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2012-08-21 05:39:04

9f959f0..4335592 389-ds-base-1.2.11 -> 389-ds-base-1.2.11 commit changeset:433559271e5ab0ed0ad3a4e09806cf0074a3cff4/389-ds-base Author: Rich Megginson richm@redhat.com Date: Thu Aug 16 17:34:05 2012 -0600 53c974f..34eea5d master -> master commit changeset:34eea5d06dceb1fcf1303537d1102cc0cdd2b459/389-ds-base Author: Rich Megginson richm@redhat.com Date: Thu Aug 16 17:34:05 2012 -0600

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2012-08-22 02:04:40

Replying to [comment:13 richm]:

Replying to [comment:11 mreynolds389]:

Very minor issues:

/ldap/servers/plugins/posix-winsync/posix-group-func.c

  • slapi_private.h is commented out. Should this be removed so not to expose slapi-private?

Yep. Will remove.

  • overall -for performance we should not be declaring var/struct pointers inside of for/while loops.

/ldap/servers/plugins/posix-winsync/posix-winsync.c

  • Declaring vars inside of loops

We do this in many, many places in the server. I thought that the compiler optimized for this situation.

I'm not sure about gcc, but this was strictly enforced at my last position. Guess it's just an old habit.

All of this is pretty picky, and I don't know winsync well enough to know if these loops are even long running loops. So it might not even matter, but it wouldn't hurt.

And indentation/spacing needs work.

I thought I had fixed that. Were you looking at patch https://fedorahosted.org/389/attachment/ticket/426/0001-Ticket-426-support-posix-schema-for-user-and-group-s.2.patch or later? The latest patch looks good.

389-ds-bot commented 4 years ago

Comment from nkinder (@nkinder) at 2012-08-28 04:14:45

Added initial screened field value.

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2012-09-05 03:00:38

add v2 and v3 api - use precedence callback - add required attrs to ldif 0001-Ticket-426-support-posix-schema-for-user-and-group-s.5.patch

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2012-09-05 03:49:22

322523d..7ecddec 389-ds-base-1.2.11 -> 389-ds-base-1.2.11 commit changeset:7ecddec9e83dd47246922f35434aa3ec5b258ae4/389-ds-base Author: Rich Megginson richm@redhat.com Date: Tue Sep 4 08:25:59 2012 -0600 b542257..214f3a8 master -> master commit changeset:214f3a8efb6fa783a6b6a839d4381c79b8fe0a51/389-ds-base Author: Rich Megginson richm@redhat.com Date: Tue Sep 4 08:25:59 2012 -0600

389-ds-bot commented 4 years ago

Comment from rmeggins (@richm) at 2017-02-11 22:56:26

Metadata Update from @richm: