PR#50084: Ticket 50077 - RFE - imporve automember plugin to work with modify ops - 389-ds-base

389-ds-base

#50084 Ticket 50077 - RFE - imporve automember plugin to work with modify ops

Closed 3 years ago by spichugi. Opened 5 years ago by mreynolds.

mreynolds/389-ds-base ticket50077 into master

Ticket 50077 - RFE - improve automember plugin to work with modify ops

Mark Reynolds • 5 years ago

7096094

dirsrvtests/tests/suites/automember_plugin/automember_mod_test.py

file added

+142

		`@@ -0,0 +1,142 @@`
		`+ import logging`
		`+ import pytest`
		`+ import os`
		`+ from lib389.utils import ds_is_older`
		`+ from lib389._constants import *`
		`+ from lib389.plugins import AutoMembershipPlugin, AutoMembershipDefinitions`
		`+ from lib389.idm.user import UserAccounts`
		`+ from lib389.idm.group import Groups`
		`+ from lib389.topologies import topology_st as topo`
		`+`
		`+ # Skip on older versions`
		`+ pytestmark = pytest.mark.skipif(ds_is_older('1.4.0'), reason="Not implemented")`
		`+`
		`+ DEBUGGING = os.getenv("DEBUGGING", default=False)`
		`+ if DEBUGGING:`
		`+ logging.getLogger(__name__).setLevel(logging.DEBUG)`
		`+ else:`
		`+ logging.getLogger(__name__).setLevel(logging.INFO)`
		`+ log = logging.getLogger(__name__)`
		`+`
		`+`
		`+ @pytest.fixture(scope="module")`
		`+ def automember_fixture(topo, request):`
		`+ # Create group`
		`+ groups = []`
		`+ group_obj = Groups(topo.standalone, DEFAULT_SUFFIX)`
		`+ groups.append(group_obj.create(properties={'cn': 'testgroup'}))`
		`+ groups.append(group_obj.create(properties={'cn': 'testgroup2'}))`
		`+ groups.append(group_obj.create(properties={'cn': 'testgroup3'}))`
		`+`
		`+ # Create test user`
		`+ user_accts = UserAccounts(topo.standalone, DEFAULT_SUFFIX)`
		`+ user = user_accts.create_test_user()`
		`+`
		`+ # Create automember definitions and regex rules`
		`+ automember_prop = {`
		`+ 'cn': 'testgroup_definition',`
		`+ 'autoMemberScope': DEFAULT_SUFFIX,`
		`+ 'autoMemberFilter': 'objectclass=posixaccount',`
		`+ 'autoMemberDefaultGroup': groups[0].dn,`
		`+ 'autoMemberGroupingAttr': 'member:dn',`
		`+ }`
		`+ automembers = AutoMembershipDefinitions(topo.standalone)`
		`+ auto_def = automembers.create(properties=automember_prop)`
		`+ auto_def.add_regex_rule("regex1", groups[1].dn, include_regex=['cn=mark.*'])`
		`+ auto_def.add_regex_rule("regex2", groups[2].dn, include_regex=['cn=simon.*'])`
		`+`
		`+ # Enable plugin`
		`+ automemberplugin = AutoMembershipPlugin(topo.standalone)`
		`+ automemberplugin.enable()`
		`+ topo.standalone.restart()`
		`+`
		`+ return (user, groups)`
		`+`
		`+`
		`+ def test_mods(automember_fixture, topo):`
		`+ """Modify the user so that it is added to the various automember groups`
		`+`
firstyear commented 5 years ago modrdn case is already defined I guess? What about if deleting an attribute would now cause the entry to be part of the group?
		`+ :id: 28a2b070-7f16-4905-8831-c80fa6441693`
		`+ :setup: Standalone Instance`
		`+ :steps:`
		`+ 1. Update user that should add it to group[0]`
		`+ 2. Update user that should add it to group[1]`
		`+ 3. Update user that should add it to group[2]`
		`+ 4. Update user that should add it to group[0]`
		`+ 5. Test rebuild task correctly moves user to group[1]`
		`+ :expectedresults:`
		`+ 1. Success`
		`+ 2. Success`
		`+ 3. Success`
		`+ 4. Success`
		`+ 5. Success`
		`+ """`
		`+ (user, groups) = automember_fixture`
		`+`
		`+ # Update user which should go into group[0]`
		`+ user.replace('cn', 'whatever')`
		`+ groups[0].is_member(user.dn)`
		`+ if groups[1].is_member(user.dn):`
		`+ assert False`
		`+ if groups[2].is_member(user.dn):`
		`+ assert False`
		`+`
		`+ # Update user0 which should go into group[1]`
		`+ user.replace('cn', 'mark')`
		`+ groups[1].is_member(user.dn)`
		`+ if groups[0].is_member(user.dn):`
		`+ assert False`
		`+ if groups[2].is_member(user.dn):`
		`+ assert False`
		`+`
		`+ # Update user which should go into group[2]`
		`+ user.replace('cn', 'simon')`
		`+ groups[2].is_member(user.dn)`
		`+ if groups[0].is_member(user.dn):`
		`+ assert False`
		`+ if groups[1].is_member(user.dn):`
		`+ assert False`
		`+`
		`+ # Update user which should go back into group[0] (full circle)`
		`+ user.replace('cn', 'whatever')`
		`+ groups[0].is_member(user.dn)`
		`+ if groups[1].is_member(user.dn):`
		`+ assert False`
		`+ if groups[2].is_member(user.dn):`
		`+ assert False`
		`+`
		`+ #`
		`+ # Test rebuild task. First disable plugin`
		`+ #`
		`+ automemberplugin = AutoMembershipPlugin(topo.standalone)`
		`+ automemberplugin.disable()`
		`+ topo.standalone.restart()`
		`+`
		`+ # Make change that would move the entry from group[0] to group[1]`
		`+ user.replace('cn', 'mark')`
		`+`
		`+ # Enable plugin`
		`+ automemberplugin.enable()`
		`+ topo.standalone.restart()`
		`+`
		`+ # Run rebuild task`
		`+ task = automemberplugin.fixup(DEFAULT_SUFFIX, "objectclass=posixaccount")`
		`+ task.wait()`
		`+`
		`+ # Test membership`
		`+ groups[1].is_member(user.dn)`
		`+ if groups[0].is_member(user.dn):`
		`+ assert False`
		`+ if groups[2].is_member(user.dn):`
		`+ assert False`
		`+`
		`+ # Success`
		`+ log.info("Test PASSED")`
		`+`
		`+`
		`+ if __name__ == '__main__':`
		`+ # Run isolated`
		`+ # -s for DEBUG mode`
		`+ CURRENT_FILE = os.path.realpath(__file__)`
		`+ pytest.main(["-s", CURRENT_FILE])`
		`+`

ldap/ldif/template-dse.ldif.in

file modified

		`@@ -696,6 +696,7 @@`
		`nsslapd-plugintype: betxnpreoperation`
		`nsslapd-pluginenabled: on`
		`nsslapd-plugin-depends-on-type: database`
		`+ autoMemberProcessModifyOps: on`

		`dn: cn=Bitwise Plugin,cn=plugins,cn=config`
		`objectClass: top`

ldap/servers/plugins/automember/automember.c

file modified

+328 -79

		`@@ -73,7 +73,8 @@`
		`static void automember_free_regex_rule(struct automemberRegexRule *rule);`
		`static int automember_parse_grouping_attr(char value, char grouping_attr, char *grouping_value);`
		`static int automember_update_membership(struct configEntry config, Slapi_Entry e, PRFileDesc *ldif_fd);`
		`- static int automember_add_member_value(Slapi_Entry member_e, const char group_dn, char grouping_attr, char grouping_value, PRFileDesc *ldif_fd);`
		`+ static int automember_update_member_value(Slapi_Entry member_e, const char group_dn, char *grouping_attr,`
		`+ char grouping_value, PRFileDesc ldif_fd, int add);`

		`/*`
		`* task functions`
		`@@ -89,6 +90,8 @@`
		`static void automember_task_map_destructor(Slapi_Task *task);`

		`#define DEFAULT_FILE_MODE PR_IRUSR \| PR_IWUSR`
		`+ static uint64_t plugin_do_modify = 1;`
		`+ static uint64_t plugin_is_betxn = 0;`

		`/*`
		`* Config cache locking functions`
		`@@ -139,8 +142,6 @@`
		`return _PluginDN;`
		`}`

		`- static int plugin_is_betxn = 0;`
		`-`
		`/*`
		`* Plug-in initialization functions`
		`*/`
		`@@ -158,8 +159,7 @@`
		`"--> automember_init\n");`

		`/* get args */`
		`- if ((slapi_pblock_get(pb, SLAPI_PLUGIN_CONFIG_ENTRY, &plugin_entry) == 0) &&`
		`- plugin_entry &&`
		`+ if ((slapi_pblock_get(pb, SLAPI_PLUGIN_CONFIG_ENTRY, &plugin_entry) == 0) && plugin_entry &&`
		`(plugin_type = slapi_entry_attr_get_charptr(plugin_entry, "nsslapd-plugintype")) &&`
		`plugin_type && strstr(plugin_type, "betxn")) {`
		`plugin_is_betxn = 1;`
		`@@ -297,7 +297,9 @@`
		`automember_start(Slapi_PBlock *pb)`
		`{`
		`Slapi_DN *plugindn = NULL;`
		`+ Slapi_Entry *plugin_entry = NULL;`
		`char *config_area = NULL;`
		`+ const char *do_modify = NULL;`

		`slapi_log_err(SLAPI_LOG_TRACE, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`"--> automember_start\n");`
		`@@ -342,8 +344,22 @@`
		`return -1;`
		`}`

		`+ /* Check and set if we should process modify operations */`
		`+ plugin_do_modify = 1; /* default is "on" */`
		`+ if ((slapi_pblock_get(pb, SLAPI_ADD_ENTRY, &plugin_entry) == 0) && plugin_entry){`
		`+ if ((do_modify = slapi_fetch_attr(plugin_entry, AUTOMEMBER_DO_MODIFY, NULL)) ) {`
		`+ if (strcasecmp(do_modify, "on") && strcasecmp(do_modify, "off")) {`
		`+ slapi_log_err(SLAPI_LOG_ERR, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`+ "automember_start - %s: invalid value \"%s\". Valid values are \"on\" or \"off\". Using default of \"on\"\n",`
		`+ AUTOMEMBER_DO_MODIFY, do_modify);`
		`+ } else if (strcasecmp(do_modify, "off") == 0 ){`
		`+ plugin_do_modify = 0;`
		`+ }`
		`+ }`
		`+ }`
		`+`
		`slapi_log_err(SLAPI_LOG_PLUGIN, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`- "auto membership plug-in: ready for service\n");`
		`+ "automember_start - ready for service\n");`
		`slapi_log_err(SLAPI_LOG_TRACE, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`"<-- automember_start\n");`

		`@@ -1365,37 +1381,49 @@`
		`}`

		`/*`
		`- * automember_update_membership()`
		`- *`
		`- * Determines which target groups need to be updated according to`
		`- * the rules in config, then performs the updates.`
		`+ * Free the exclusion and inclusion group dn's created by`
		`+ * automember_get_membership_lists()`
		`*/`
		`- static int`
		`- automember_update_membership(struct configEntry config, Slapi_Entry e, PRFileDesc *ldif_fd)`
		`+ static void`
		`+ automember_free_membership_lists(PRCList exclusions, PRCList targets)`
		`+ {`
		`+ struct automemberDNListItem *dnitem = NULL;`
		`+`
		`+ /*`
		`+ * Free the exclusions and targets lists. Remember that the DN's`
		`+ * are not ours, so don't free them!`
		`+ */`
		`+ while (!PR_CLIST_IS_EMPTY(exclusions)) {`
		`+ dnitem = (struct automemberDNListItem *)PR_LIST_HEAD(exclusions);`
		`+ PR_REMOVE_LINK((PRCList *)dnitem);`
		`+ slapi_ch_free((void **)&dnitem);`
		`+ }`
		`+`
		`+ while (!PR_CLIST_IS_EMPTY(targets)) {`
		`+ dnitem = (struct automemberDNListItem *)PR_LIST_HEAD(targets);`
		`+ PR_REMOVE_LINK((PRCList *)dnitem);`
		`+ slapi_ch_free((void **)&dnitem);`
		`+ }`
		`+ }`
		`+`
		`+ /*`
		`+ * Populate the exclusion and inclusion(target) PRCLists based on the`
		`+ * slapi entry and configEntry provided. The PRCLists should be freed`
		`+ * using automember_free_membership_lists()`
		`+ */`
		`+ static void`
		`+ automember_get_membership_lists(struct configEntry config, PRCList exclusions, PRCList targets, Slapi_Entry e)`
		`{`
		`PRCList *rule = NULL;`
		`struct automemberRegexRule *curr_rule = NULL;`
		`- PRCList exclusions;`
		`- PRCList targets;`
		`struct automemberDNListItem *dnitem = NULL;`
		`Slapi_DN *last = NULL;`
		`PRCList *curr_exclusion = NULL;`
		`char **vals = NULL;`
		`- int rc = 0;`
		`int i = 0;`

		`- if (!config \|\| !e) {`
		`- return -1;`
		`- }`
		`-`
		`- slapi_log_err(SLAPI_LOG_PLUGIN, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`- "automember_update_membership - Processing \"%s\" "`
		`- "definition entry for candidate entry \"%s\".\n",`
		`- config->dn, slapi_entry_get_dn(e));`
		`-`
		`- /* Initialize our lists that keep track of targets. */`
		`- PR_INIT_CLIST(&exclusions);`
		`- PR_INIT_CLIST(&targets);`
		`+ PR_INIT_CLIST(exclusions);`
		`+ PR_INIT_CLIST(targets);`

		`/* Go through exclusive rules and build an exclusion list. */`
		`if (config->exclusive_rules) {`
		`@@ -1416,20 +1444,19 @@`
		`/* Found a match. Add to end of the exclusion list`
		`* and set last as a hint to ourselves. */`
		`slapi_log_err(SLAPI_LOG_PLUGIN, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`- "automember_update_membership - Adding \"%s\" "`
		`+ "automember_get_membership_lists - Adding \"%s\" "`
		`"to list of excluded groups for \"%s\" "`
		`"(matched: \"%s=%s\").\n",`
		`slapi_sdn_get_dn(curr_rule->target_group_dn),`
		`slapi_entry_get_dn(e), curr_rule->attr,`
		`curr_rule->regex_str);`
		`- dnitem = (struct automemberDNListItem *)slapi_ch_calloc(1,`
		`- sizeof(struct automemberDNListItem));`
		`+ dnitem = (struct automemberDNListItem *)slapi_ch_calloc(1, sizeof(struct automemberDNListItem));`
		`/* We are just referencing the dn from the regex rule. We`
		`* will not free it when we clean up this list. This list`
		`* is more short-lived than the regex rule list, so we can`
		`* get away with this optimization. */`
		`dnitem->dn = curr_rule->target_group_dn;`
		`- PR_APPEND_LINK(&(dnitem->list), &exclusions);`
		`+ PR_APPEND_LINK(&(dnitem->list), exclusions);`
		`last = curr_rule->target_group_dn;`
		`}`
		`}`
		`@@ -1450,8 +1477,8 @@`
		`last = NULL;`

		`/* Get the first excluded target for exclusion checking. */`
		`- if (!PR_CLIST_IS_EMPTY(&exclusions)) {`
		`- curr_exclusion = PR_LIST_HEAD(&exclusions);`
		`+ if (!PR_CLIST_IS_EMPTY(exclusions)) {`
		`+ curr_exclusion = PR_LIST_HEAD(exclusions);`
		`}`

		`rule = PR_LIST_HEAD((PRCList *)config->inclusive_rules);`
		`@@ -1468,7 +1495,7 @@`
		`* until we find a target that is the same or comes after the`
		`* current rule. */`
		`if (curr_exclusion) {`
		`- while ((curr_exclusion != &exclusions) && (slapi_sdn_compare(`
		`+ while ((curr_exclusion != exclusions) && (slapi_sdn_compare(`
		`((struct automemberDNListItem *)curr_exclusion)->dn,`
		`curr_rule->target_group_dn) < 0)) {`
		`curr_exclusion = PR_NEXT_LINK(curr_exclusion);`
		`@@ -1479,7 +1506,7 @@`
		`* we can skip all rules for the last target group DN that we`
		`* added to the targets list. We also skip any rules for`
		`* target groups that have been excluded by an exclusion rule. */`
		`- if (((curr_exclusion == NULL) \|\| (curr_exclusion == &exclusions) \|\|`
		`+ if (((curr_exclusion == NULL) \|\| (curr_exclusion == exclusions) \|\|`
		`slapi_sdn_compare(((struct automemberDNListItem *)curr_exclusion)->dn,`
		`curr_rule->target_group_dn) != 0) &&`
		`((last == NULL) \|\|`
		`@@ -1492,7 +1519,7 @@`
		`/* Found a match. Add to the end of the targets list`
		`* and set last as a hint to ourselves. */`
		`slapi_log_err(SLAPI_LOG_PLUGIN, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`- "automember_update_membership - Adding \"%s\" "`
		`+ "automember_get_membership_lists - Adding \"%s\" "`
		`"to list of target groups for \"%s\" "`
		`"(matched: \"%s=%s\").\n",`
		`slapi_sdn_get_dn(curr_rule->target_group_dn),`
		`@@ -1505,7 +1532,7 @@`
		`* is more short-lived than the regex rule list, so we can`
		`* get away with this optimization. */`
		`dnitem->dn = curr_rule->target_group_dn;`
		`- PR_APPEND_LINK(&(dnitem->list), &targets);`
		`+ PR_APPEND_LINK(&(dnitem->list), targets);`
		`last = curr_rule->target_group_dn;`
		`}`
		`}`
		`@@ -1518,6 +1545,42 @@`
		`}`
		`}`
		`}`
		`+ }`
		`+`
		`+ /*`
		`+ * automember_update_membership()`
		`+ *`
		`+ * Determines which target groups need to be updated according to`
		`+ * the rules in config, then performs the updates.`
		`+ *`
		`+ * Return SLAPI_PLUGIN_FAILURE for failures, or`
		`+ * SLAPI_PLUGIN_SUCCESS for success (no memberships updated), or`
		`+ * MEMBERSHIP_UPDATED for success (memberships updated)`
		`+ */`
		`+ static int`
		`+ automember_update_membership(struct configEntry config, Slapi_Entry e, PRFileDesc *ldif_fd)`
		`+ {`
		`+ PRCList exclusions;`
		`+ PRCList targets;`
		`+ struct automemberDNListItem *dnitem = NULL;`
		`+ int rc = SLAPI_PLUGIN_SUCCESS;`
		`+ int i = 0;`
		`+`
		`+ if (!config \|\| !e) {`
		`+ return SLAPI_PLUGIN_FAILURE;`
		`+ }`
		`+`
		`+ slapi_log_err(SLAPI_LOG_PLUGIN, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`+ "automember_update_membership - Processing \"%s\" "`
		`+ "definition entry for candidate entry \"%s\".\n",`
		`+ config->dn, slapi_entry_get_dn(e));`
		`+`
		`+ /* Initialize our lists that keep track of targets. */`
		`+ PR_INIT_CLIST(&exclusions);`
		`+ PR_INIT_CLIST(&targets);`
		`+`
		`+ /* get the membership lists */`
		`+ automember_get_membership_lists(config, &exclusions, &targets, e);`

		`/* If no targets, update default groups if set. Otherwise, update`
		`* targets. Use a helper to do the actual updates. We can just pass an`
		`@@ -1526,51 +1589,45 @@`
		`if (PR_CLIST_IS_EMPTY(&targets)) {`
		`/* Add to each default group. */`
		`for (i = 0; config->default_groups && config->default_groups[i]; i++) {`
		`- if (automember_add_member_value(e, config->default_groups[i], config->grouping_attr,`
		`- config->grouping_value, ldif_fd)) {`
		`+ if (automember_update_member_value(e, config->default_groups[i], config->grouping_attr,`
		`+ config->grouping_value, ldif_fd, ADD_MEMBER))`
		`+ {`
		`rc = SLAPI_PLUGIN_FAILURE;`
		`goto out;`
		`}`
		`+ rc = MEMBERSHIP_UPDATED;`
		`}`
		`} else {`
		`/* Update the target groups. */`
		`dnitem = (struct automemberDNListItem *)PR_LIST_HEAD(&targets);`
		`while ((PRCList *)dnitem != &targets) {`
		`- if (automember_add_member_value(e, slapi_sdn_get_dn(dnitem->dn), config->grouping_attr,`
		`- config->grouping_value, ldif_fd)) {`
		`+ if (automember_update_member_value(e, slapi_sdn_get_dn(dnitem->dn), config->grouping_attr,`
		`+ config->grouping_value, ldif_fd, ADD_MEMBER))`
		`+ {`
		`rc = SLAPI_PLUGIN_FAILURE;`
		`goto out;`
		`}`
		`dnitem = (struct automemberDNListItem )PR_NEXT_LINK((PRCList )dnitem);`
		`+ rc = MEMBERSHIP_UPDATED;`
		`}`
		`}`

		`- /* Free the exclusions and targets lists. Remember that the DN's`
		`- * are not ours, so don't free them! */`
		`- while (!PR_CLIST_IS_EMPTY(&exclusions)) {`
		`- dnitem = (struct automemberDNListItem *)PR_LIST_HEAD(&exclusions);`
		`- PR_REMOVE_LINK((PRCList *)dnitem);`
		`- slapi_ch_free((void **)&dnitem);`
		`- }`
		`-`
		`- while (!PR_CLIST_IS_EMPTY(&targets)) {`
		`- dnitem = (struct automemberDNListItem *)PR_LIST_HEAD(&targets);`
		`- PR_REMOVE_LINK((PRCList *)dnitem);`
		`- slapi_ch_free((void **)&dnitem);`
		`- }`
		`+ /* Free the exclusions and targets lists */`
		`+ automember_free_membership_lists(&exclusions, &targets);`

		`out:`

		`return rc;`
		`}`

		`+`
		`/*`
		`- * automember_add_member_value()`
		`+ * automember_update_member_value()`
		`*`
		`* Adds a member entry to a group.`
		`*/`
		`static int`
		`- automember_add_member_value(Slapi_Entry member_e, const char group_dn, char grouping_attr, char grouping_value, PRFileDesc *ldif_fd)`
		`+ automember_update_member_value(Slapi_Entry member_e, const char group_dn, char grouping_attr, char grouping_value, PRFileDesc *ldif_fd, int add)`
		`{`
		`Slapi_PBlock *mod_pb = slapi_pblock_new();`
		`int result = LDAP_SUCCESS;`
		`@@ -1606,34 +1663,57 @@`
		`/* Set up the operation. */`
		`vals[0] = member_value;`
		`vals[1] = 0;`
		`- mod.mod_op = LDAP_MOD_ADD;`
		`+ if (add) {`
		`+ mod.mod_op = LDAP_MOD_ADD;`
		`+ } else {`
		`+ mod.mod_op = LDAP_MOD_DELETE;`
		`+ }`
		`mod.mod_type = grouping_attr;`
		`mod.mod_values = vals;`
		`mods[0] = &mod;`
		`mods[1] = 0;`

		`/* Perform the modify operation. */`
		`- slapi_log_err(SLAPI_LOG_PLUGIN, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`- "automember_add_member_value - Adding \"%s\" as "`
		`- "a \"%s\" value to group \"%s\".\n",`
		`- member_value, grouping_attr, group_dn);`
		`+ if (add){`
		`+ slapi_log_err(SLAPI_LOG_PLUGIN, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`+ "automember_update_member_value - Adding \"%s\" as "`
		`+ "a \"%s\" value to group \"%s\".\n",`
		`+ member_value, grouping_attr, group_dn);`
		`+ } else {`
		`+ slapi_log_err(SLAPI_LOG_PLUGIN, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`+ "automember_update_member_value - Deleting \"%s\" as "`
		`+ "a \"%s\" value from group \"%s\".\n",`
		`+ member_value, grouping_attr, group_dn);`
		`+ }`

		`slapi_modify_internal_set_pb(mod_pb, group_dn,`
		`mods, 0, 0, automember_get_plugin_id(), 0);`
		`slapi_modify_internal_pb(mod_pb);`
		`slapi_pblock_get(mod_pb, SLAPI_PLUGIN_INTOP_RESULT, &result);`

		`- if ((result != LDAP_SUCCESS) && (result != LDAP_TYPE_OR_VALUE_EXISTS)) {`
		`- slapi_log_err(SLAPI_LOG_ERR, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`- "automember_add_member_value - Unable to add \"%s\" as "`
		`- "a \"%s\" value to group \"%s\" (%s).\n",`
		`- member_value, grouping_attr, group_dn,`
		`- ldap_err2string(result));`
		`- rc = result;`
		`+ if(add){`
		`+ if ((result != LDAP_SUCCESS) && (result != LDAP_TYPE_OR_VALUE_EXISTS)) {`
		`+ slapi_log_err(SLAPI_LOG_ERR, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`+ "automember_update_member_value - Unable to add \"%s\" as "`
		`+ "a \"%s\" value to group \"%s\" (%s).\n",`
		`+ member_value, grouping_attr, group_dn,`
		`+ ldap_err2string(result));`
		`+ rc = result;`
		`+ }`
		`+ } else {`
		`+ /* delete value */`
		`+ if ((result != LDAP_SUCCESS) && (result != LDAP_NO_SUCH_ATTRIBUTE)) {`
		`+ slapi_log_err(SLAPI_LOG_ERR, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`+ "automember_update_member_value - Unable to delete \"%s\" as "`
		`+ "a \"%s\" value from group \"%s\" (%s).\n",`
		`+ member_value, grouping_attr, group_dn,`
		`+ ldap_err2string(result));`
		`+ rc = result;`
		`+ }`
		`}`
		`} else {`
		`slapi_log_err(SLAPI_LOG_ERR, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`- "automember_add_member_value - Unable to find grouping "`
		`+ "automember_update_member_value - Unable to find grouping "`
		`"value attribute \"%s\" in entry \"%s\".\n",`
		`grouping_value, slapi_entry_get_dn(member_e));`
		`}`
		`@@ -1780,22 +1860,144 @@`
		`static int`
		`automember_mod_post_op(Slapi_PBlock *pb)`
		`{`
		`+ Slapi_Entry *post_e = NULL;`
		`+ Slapi_Entry *pre_e = NULL;`
		`Slapi_DN *sdn = NULL;`
		`+ struct configEntry *config = NULL;`
		`+ PRCList *list = NULL;`
		`+ int rc = SLAPI_PLUGIN_SUCCESS;`

		`slapi_log_err(SLAPI_LOG_TRACE, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`"--> automember_mod_post_op\n");`

		`if (automember_oktodo(pb) && (sdn = automember_get_sdn(pb))) {`
		`- /* Check if the config is being modified and reload if so. */`
		`if (automember_dn_is_config(sdn)) {`
		`+ /*`
		`+ * The config is being modified, reload it`
		`+ */`
		`automember_load_config();`
		`+ } else if ( !automember_isrepl(pb) && plugin_do_modify) {`
		`+ /*`
		`+ * We might be applying an update that will invoke automembership changes...`
		`+ */`
		`+ slapi_pblock_get(pb, SLAPI_ENTRY_POST_OP, &post_e);`
		`+ slapi_pblock_get(pb, SLAPI_ENTRY_PRE_OP, &pre_e);`
		`+`
		`+ if (post_e) {`
		`+ /*`
		`+ * Check if a config entry applies to the entry being modified`
		`+ */`
		`+ automember_config_read_lock();`
		`+`
		`+ if (!PR_CLIST_IS_EMPTY(g_automember_config)) {`
		`+ list = PR_LIST_HEAD(g_automember_config);`
		`+ while (list != g_automember_config) {`
		`+ config = (struct configEntry *)list;`
		`+`
		`+ /* Does the entry meet scope and filter requirements? */`
		`+ if (slapi_dn_issuffix(slapi_sdn_get_dn(sdn), config->scope) &&`
		`+ slapi_filter_test_simple(post_e, config->filter) == 0)`
		`+ {`
		`+ /* Find out what membership changes are needed and make them. */`
		`+ if ((rc = automember_update_membership(config, post_e, NULL)) == SLAPI_PLUGIN_FAILURE) {`
		`+ /* Failed to update our groups, break out */`
		`+ break;`
		`+ } else if (rc == MEMBERSHIP_UPDATED) {`
		`+ /*`
		`+ * We updated one of our groups, but we might need to do some cleanup in other groups`
		`+ */`
		`+ PRCList exclusions_post, targets_post;`
		`+ PRCList exclusions_pre, targets_pre;`
		`+ struct automemberDNListItem *dn_pre = NULL;`
		`+ struct automemberDNListItem *dn_post = NULL;`
		`+ int i;`
		`+`
		`+ /* reset rc */`
		`+ rc = SLAPI_PLUGIN_SUCCESS;`
		`+`
		`+ /* Get the group lists */`
		`+ automember_get_membership_lists(config, &exclusions_post, &targets_post, post_e);`
		`+ automember_get_membership_lists(config, &exclusions_pre, &targets_pre, pre_e);`
		`+`
		`+ /* Process the before and after lists */`
		`+ if (PR_CLIST_IS_EMPTY(&targets_pre) && !PR_CLIST_IS_EMPTY(&targets_post)) {`
		`+ /*`
		`+ * We were in the default groups, but not anymore`
		`+ */`
		`+ for (i = 0; config->default_groups && config->default_groups[i]; i++) {`
		`+ if (automember_update_member_value(post_e, config->default_groups[i], config->grouping_attr,`
		`+ config->grouping_value, NULL, DEL_MEMBER))`
		`+ {`
		`+ rc = SLAPI_PLUGIN_FAILURE;`
		`+ break;`
		`+ }`
		`+ }`
		`+ } else if (!PR_CLIST_IS_EMPTY(&targets_pre) && PR_CLIST_IS_EMPTY(&targets_post)) {`
		`+ /*`
		`+ * We were in non-default groups, but not anymore`
		`+ */`
		`+ dn_pre = (struct automemberDNListItem *)PR_LIST_HEAD(&targets_pre);`
		`+ while ((PRCList *)dn_pre != &targets_pre) {`
		`+ if (automember_update_member_value(post_e, slapi_sdn_get_dn(dn_pre->dn), config->grouping_attr,`
		`+ config->grouping_value, NULL, DEL_MEMBER))`
		`+ {`
		`+ rc = SLAPI_PLUGIN_FAILURE;`
		`+ break;`
		`+ }`
		`+ dn_pre = (struct automemberDNListItem )PR_NEXT_LINK((PRCList )dn_pre);`
		`+ }`
		`+ } else {`
		`+ /*`
		`+ * We were previously in non-default groups, and still in non-default groups.`
		`+ * Compare before and after memberships and cleanup the orphaned memberships`
		`+ */`
		`+ dn_pre = (struct automemberDNListItem *)PR_LIST_HEAD(&targets_pre);`
		`+ while ((PRCList *)dn_pre != &targets_pre) {`
		`+ int found = 0;`
		`+ dn_post = (struct automemberDNListItem *)PR_LIST_HEAD(&targets_post);`
		`+ while ((PRCList *)dn_post != &targets_post) {`
		`+ if (slapi_sdn_compare(dn_pre->dn, dn_post->dn) == 0) {`
		`+ /* found */`
		`+ found = 1;`
		`+ break;`
		`+ }`
		`+ /* Next dn */`
		`+ dn_post = (struct automemberDNListItem )PR_NEXT_LINK((PRCList )dn_post);`
		`+ }`
		`+ if (!found){`
		`+ /* Remove user from dn_pre->dn */`
		`+ if (automember_update_member_value(post_e, slapi_sdn_get_dn(dn_pre->dn), config->grouping_attr,`
		`+ config->grouping_value, NULL, DEL_MEMBER))`
		`+ {`
		`+ rc = SLAPI_PLUGIN_FAILURE;`
		`+ break;`
		`+ }`
		`+ }`
		`+ /* Next dn */`
		`+ dn_pre = (struct automemberDNListItem )PR_NEXT_LINK((PRCList )dn_pre);`
		`+ }`
		`+ }`
		`+`
		`+ /* All done with this config entry, free the lists */`
		`+ automember_free_membership_lists(&exclusions_post, &targets_post);`
		`+ automember_free_membership_lists(&exclusions_pre, &targets_pre);`
		`+ if (rc == SLAPI_PLUGIN_FAILURE) {`
		`+ break;`
		`+ }`
		`+ }`
		`+ }`
		`+ list = PR_NEXT_LINK(list);`
		`+ }`
		`+ }`
		`+ automember_config_unlock();`
		`+ }`
		`}`
		`}`

		`slapi_log_err(SLAPI_LOG_TRACE, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`- "<-- automember_mod_post_op\n");`
		`+ "<-- automember_mod_post_op (%d)\n", rc);`

		`- return SLAPI_PLUGIN_SUCCESS;`
		`+ return rc;`
		`}`

		`static int`
		`@@ -1854,7 +2056,7 @@`
		`if (slapi_dn_issuffix(slapi_sdn_get_dn(sdn), config->scope) &&`
		`(slapi_filter_test_simple(e, config->filter) == 0)) {`
		`/* Find out what membership changes are needed and make them. */`
		`- if (automember_update_membership(config, e, NULL)) {`
		`+ if (automember_update_membership(config, e, NULL) == SLAPI_PLUGIN_FAILURE) {`
		`rc = SLAPI_PLUGIN_FAILURE;`
		`break;`
		`}`
		`@@ -2098,8 +2300,9 @@`
		`Slapi_Entry **entries = NULL;`
		`task_data *td = NULL;`
		`PRCList *list = NULL;`
		`+ PRCList *include_list = NULL;`
		`int result = 0;`
		`- int i = 0;`
		`+ size_t i = 0, ii = 0;`

		`if (!task) {`
		`return; /* no task */`
		`@@ -2175,9 +2378,55 @@`
		`config = (struct configEntry *)list;`
		`/* Does the entry meet scope and filter requirements? */`
		`if (slapi_dn_issuffix(slapi_entry_get_dn(entries[i]), config->scope) &&`
		`- (slapi_filter_test_simple(entries[i], config->filter) == 0)) {`
		`+ (slapi_filter_test_simple(entries[i], config->filter) == 0))`
		`+ {`
		`+ /* First clear out all the defaults groups */`
		`+ for (ii = 0; config->default_groups && config->default_groups[ii]; ii++) {`
		`+ if ((result = automember_update_member_value(entries[i], config->default_groups[ii],`
		`+ config->grouping_attr, config->grouping_value, NULL, DEL_MEMBER)))`
		`+ {`
		`+ slapi_task_log_notice(task, "Automember rebuild membership task unable to delete "`
		`+ "member from default group (%s) error (%d)\n",`
		`+ config->default_groups[ii], result);`
		`+ slapi_task_log_status(task, "Automember rebuild membership task unable to delete "`
		`+ "member from default group (%s) error (%d)\n",`
		`+ config->default_groups[ii], result);`
		`+ slapi_log_err(SLAPI_LOG_ERR, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`+ "automember_rebuild_task_thread - Unable to unable to delete from (%s) error (%d)\n",`
		`+ config->default_groups[ii], result);`
		`+ automember_config_unlock();`
		`+ goto out;`
		`+ }`
		`+ }`
		`+`
		`+ /* Then clear out the non-default group */`
		`+ if (config->inclusive_rules && !PR_CLIST_IS_EMPTY((PRCList *)config->inclusive_rules)) {`
		`+ include_list = PR_LIST_HEAD((PRCList *)config->inclusive_rules);`
		`+ while (include_list != (PRCList *)config->inclusive_rules) {`
		`+ struct automemberRegexRule curr_rule = (struct automemberRegexRule )include_list;`
		`+ if ((result = automember_update_member_value(entries[i], slapi_sdn_get_dn(curr_rule->target_group_dn),`
		`+ config->grouping_attr, config->grouping_value, NULL, DEL_MEMBER)))`
		`+ {`
		`+ slapi_task_log_notice(task, "Automember rebuild membership task unable to delete "`
		`+ "member from group (%s) error (%d)\n",`
		`+ slapi_sdn_get_dn(curr_rule->target_group_dn), result);`
		`+ slapi_task_log_status(task, "Automember rebuild membership task unable to delete "`
		`+ "member from group (%s) error (%d)\n",`
		`+ slapi_sdn_get_dn(curr_rule->target_group_dn), result);`
		`+ slapi_log_err(SLAPI_LOG_ERR, AUTOMEMBER_PLUGIN_SUBSYSTEM,`
		`+ "automember_rebuild_task_thread - Unable to unable to delete from (%s) error (%d)\n",`
		`+ slapi_sdn_get_dn(curr_rule->target_group_dn), result);`
		`+ automember_config_unlock();`
		`+ goto out;`
		`+ }`
		`+ include_list = PR_NEXT_LINK(include_list);`
		`+ }`
		`+ }`
		`+`
		`+ /* Update the memberships for this entries */`
		`if (slapi_is_shutting_down() \|\|`
		`- automember_update_membership(config, entries[i], NULL)) {`
		`+ automember_update_membership(config, entries[i], NULL) == SLAPI_PLUGIN_FAILURE)`
		`+ {`
		`result = SLAPI_PLUGIN_FAILURE;`
		`automember_config_unlock();`
		`goto out;`
		`@@ -2390,7 +2639,7 @@`
		`if (slapi_dn_issuffix(slapi_sdn_get_dn(td->base_dn), config->scope) &&`
		`(slapi_filter_test_simple(entries[i], config->filter) == 0)) {`
		`if (slapi_is_shutting_down() \|\|`
		`- automember_update_membership(config, entries[i], ldif_fd)) {`
		`+ automember_update_membership(config, entries[i], ldif_fd) == SLAPI_PLUGIN_FAILURE) {`
		`result = SLAPI_DSE_CALLBACK_ERROR;`
		`automember_config_unlock();`
		`goto out;`
		`@@ -2594,7 +2843,7 @@`
		`if (slapi_dn_issuffix(slapi_entry_get_dn_const(e), config->scope) &&`
		`(slapi_filter_test_simple(e, config->filter) == 0)) {`
		`if (slapi_is_shutting_down() \|\|`
		`- automember_update_membership(config, e, ldif_fd_out)) {`
		`+ automember_update_membership(config, e, ldif_fd_out) == SLAPI_PLUGIN_FAILURE) {`
		`result = SLAPI_DSE_CALLBACK_ERROR;`
		`slapi_entry_free(e);`
		`slapi_ch_free_string(&entrystr);`
		`@@ -2702,7 +2951,7 @@`
		`if (slapi_dn_issuffix(slapi_sdn_get_dn(new_sdn), config->scope) &&`
		`(slapi_filter_test_simple(post_e, config->filter) == 0)) {`
		`/* Find out what membership changes are needed and make them. */`
		`- if (automember_update_membership(config, post_e, NULL)) {`
		`+ if (automember_update_membership(config, post_e, NULL) == SLAPI_PLUGIN_FAILURE) {`
		`rc = SLAPI_PLUGIN_FAILURE;`
		`break;`
		`}`

ldap/servers/plugins/automember/automember.h

file modified

		`@@ -44,6 +44,7 @@`
		`#define AUTOMEMBER_GROUPING_ATTR_TYPE "autoMemberGroupingAttr"`
		`#define AUTOMEMBER_DISABLED_TYPE "autoMemberDisabled"`
		`#define AUTOMEMBER_TARGET_GROUP_TYPE "autoMemberTargetGroup"`
		`+ #define AUTOMEMBER_DO_MODIFY "autoMemberProcessModifyOps"`

		`/*`
		`* Config loading filters`
		`@@ -55,6 +56,10 @@`
		`* Helper defines`
		`*/`
		`#define IS_ATTRDESC_CHAR(c) (isalnum(c) \|\| (c == '.') \|\| (c == ';') \|\| (c == '-'))`
		`+ #define MEMBERSHIP_UPDATED 1`
		`+ #define ADD_MEMBER 1`
		`+ #define DEL_MEMBER 0`
		`+`

		`struct automemberRegexRule`
		`{`

src/lib389/lib389/plugins.py

file modified

+55 -1

		`@@ -884,6 +884,24 @@`
		`return task`


		`+ class AutoMembershipRegexRule(DSLdapObject):`
		`+ def __init__(self, instance, dn=None):`
		`+ super(AutoMembershipRegexRule, self).__init__(instance, dn)`
		`+ self._rdn_attribute = 'cn'`
		`+ self._must_attributes = ['cn', 'autoMemberTargetGroup']`
		`+ self._create_objectclasses = ['top', 'autoMemberRegexRule']`
		`+ self._protected = False`
		`+`
		`+`
		`+ class AutoMembershipRegexRules(DSLdapObjects):`
		`+ def __init__(self, instance, basedn="cn=Auto Membership Plugin,cn=plugins,cn=config"):`
		`+ super(AutoMembershipRegexRules, self).__init__(instance)`
		`+ self._objectclasses = ['top', 'autoMemberRegexRule']`
		`+ self._filterattrs = ['cn']`
		`+ self._childobject = AutoMembershipRegexRule`
		`+ self._basedn = basedn`
		`+`
		`+`
		`class AutoMembershipDefinition(DSLdapObject):`
		`"""A single instance of Auto Membership Plugin config entry`

		`@@ -918,7 +936,7 @@`
		`def set_defaultgroup(self, attr):`
		`"""Set autoMemberDefaultGroup attribute"""`

		`- self.set('autoMemberDefaultGroup', attr)`
		`+ self.set('autoMemberDefaultGroup', attr)`

		`def get_scope(self, attr):`
		`"""Get autoMemberScope attributes"""`
		`@@ -940,6 +958,42 @@`

		`self.set('autoMemberFilter', attr)`

		`+ def add_regex_rule(self, rule_name, target, include_regex=None, exclude_regex=None):`
		`+ """Add a regex rule`
		`+ :param rule_name - Name of the rule - used dfor the "cn" value inthe DN of the rule entry`
		`+ :param target - the target group DN`
		`+ :param include_regex - a List of regex rules used for group inclusion`
		`+ :param exclude_regex - a List of regex rules used for group exclusion`
		`+ """`
		`+ props = {'cn': rule_name,`
		`+ 'autoMemberTargetGroup': target}`
		`+`
		`+ if include_regex is not None:`
		`+ props['autoMemberInclusiveRegex'] = include_regex`
		`+ if exclude_regex is not None:`
		`+ props['autoMemberInclusiveRegex'] = exclude_regex`
		`+`
		`+ rules = AutoMembershipRegexRules(self._instance, basedn=self.dn)`
		`+ rules.create(properties=props)`
		`+`
		`+ def del_regex_rule(self, rule_name):`
		`+ """Delete a regex rule from this definition`
		`+ :param rule_name - The "cn" values of the regex rule entry`
		`+ :raises ValueError - If a regex rule entry can not be found using rule_name`
		`+ """`
		`+ rules = AutoMembershipRegexRules(self._instance, basedn=self.dn)`
		`+ regex = rules.get(selector=rule_name)`
		`+ if regex is not None:`
		`+ regex.delete()`
		`+ else:`
		`+ raise ValueError("No regex rule found with the name ({}) under ({})".format(rule_name, self.dn))`
		`+`
		`+ def list_regex_rules(self):`
		`+ """Return a list of regex rule entries for this definition`
		`+ """`
		`+ rules = AutoMembershipRegexRules(self._instance, basedn=self.dn)`
		`+ return rules.list()`
		`+`

		`class AutoMembershipDefinitions(DSLdapObjects):`
		`"""A DSLdapObjects entity which represents Auto Membership Plugin config entry`

mreynolds commented 5 years ago

Description:

Previously automember was only invoked for ADD operations. This enhancment
allows it to work with modify operations, and it will also maintain the
correct memberships. So if a modify changes which groups the user would
belong to, it will add the user to the new group, and remove them from the
old group.

https://pagure.io/389-ds-base/issue/50077

Reviewed by: ?

rebased onto d01f4e13c3f7f74be3528890249be1282acae798

5 years ago

rebased onto 008dc462f8110a11970ecd92df3091affc0e7a97

5 years ago

spichugi commented on line 369 of ldap/servers/plugins/automember/automember.c 5 years ago

Shouldn't we translate the 'result' here? like with ldap_err2string

spichugi commented on line 593 of ldap/servers/plugins/automember/automember.c 5 years ago

If I run py.test -s -v dirsrvtests/tests/suites/plugins/acceptance_test.py::test_automember - the instance crashes here.
In the test case, it is 467 line - task = plugin.fixup(branch2.dn, 'objectclass=top')

I still didn't investigate the automember code base enough, but can it be that the crash happens because of config->inclusive_rules == NULL at that point of the code path?

Also, I have 'ERR - auto-membership-plugin - automember_update_member_value - result: 16' - at some point (LDAP_NO_SUCH_ATTRIBUTE). I don't know if it's related

Edited 5 years ago by spichugi

mreynolds commented 5 years ago

If I run py.test -s -v dirsrvtests/tests/suites/plugins/acceptance_test.py::test_automember - the instance crashes here.
In the test case, it is 467 line - task = plugin.fixup(branch2.dn, 'objectclass=top')
I still didn't investigate the automember code base enough, but can it be that the crash happens because of config->inclusive_rules == NULL at that point of the code path?

Investigating...

Also, I have 'ERR - auto-membership-plugin - automember_update_member_value - result: 16' - at some point (LDAP_NO_SUCH_ATTRIBUTE). I don't know if it's related

It's not related, but that logging line was for my debugging, and I will remove it completely.

rebased onto 10c2c761d304ed610ef78590922cdac514686187

5 years ago

mreynolds commented 5 years ago

Issues are fixed and tests pass. Please review again...

firstyear commented on line 429 of ldap/servers/plugins/automember/automember.c 5 years ago

Lots of this block of code seems applicable to the add case too, chance to make a function for membership check/modification?

firstyear commented on line 558 of ldap/servers/plugins/automember/automember.c 5 years ago

int32_t ? size_t ?

Edited 5 years ago by firstyear

firstyear commented on line 570 of ldap/servers/plugins/automember/automember.c 5 years ago

ii should be size_t as it's used in an array deref

firstyear commented on line 625 of ldap/servers/plugins/automember/automember.c 5 years ago

!= PLUGIN_SUCCESS? Depends what error results we could possibly return, but != is defensive.

firstyear commented on line 59 of dirsrvtests/tests/suites/automember_plugin/automember_mod_test.py 5 years ago

modrdn case is already defined I guess? What about if deleting an attribute would now cause the entry to be part of the group?

spichugi commented 5 years ago

I agree with William regarding 'int' issues and regarding possibly another test case option.
The rest looks good to me.

!= PLUGIN_SUCCESS? Depends what error results we could possibly return, but != is defensive.

I think the idea was to have these three 'rc' in automember_update_membership():

* Return SLAPI_PLUGIN_FAILURE for failures, or
*        SLAPI_PLUGIN_SUCCESS for success (no memberships updated), or
*        MEMBERSHIP_UPDATED   for success (memberships updated)

We don't return anything else so the logic depends on checking if it's success, failure or membership_updated.
But I agree that the != PLUGIN_SUCCESS logic is a more straightforward approach in general.

mreynolds commented 5 years ago

modrdn case is already defined I guess? What about if deleting an attribute would now cause the entry to be part of the group?

The post_op modify handles this case - it doesn't really matter what the mod ops are (add attr/delete attr) because we are looking at the entry as a whole after all mods are applied. Basically it takes a snap shot of the pre and post entry automember groups, and then the group differences are cleaned up from the post entry. The CI test essentially covers this scenario.

rebased onto 22f2b1ee533e05bdab4f172ca8f9b40a44583899

5 years ago

spichugi commented 5 years ago

LGTM!

rebased onto 7782bec054c954cd60eeec88fcf037450429eb6b

5 years ago

rebased onto 7096094

5 years ago

Pull-Request has been merged by mreynolds

5 years ago

spichugi commented 3 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.