389-ds-base

Clone
Source Code
GIT

#49986 Ticket 49985 - memberof may silently fails to update a member
Closed 3 years ago by spichugi. Opened 5 years ago by tbordaz.
tbordaz/389-ds-base ticket_49985  into  master

file modified
+183 -2 
@@ -514,7 +514,8 @@ 
 

      peoplebase = 'ou=people,%s' % SUFFIX
 

      MEMBEROF_PLUGIN_DN = ('cn=' + PLUGIN_MEMBER_OF + ',cn=plugins,cn=config')
 

      server.modify_s(MEMBEROF_PLUGIN_DN, [(ldap.MOD_REPLACE, 'memberOfAllBackends', b'on'),
 

-                                          (ldap.MOD_REPLACE, 'memberOfEntryScope', peoplebase.encode())])
 

+                                          (ldap.MOD_REPLACE, 'memberOfEntryScope', peoplebase.encode()),
 

+                                          (ldap.MOD_REPLACE, 'memberOfAutoAddOC', b'nsMemberOf')])
 

  

  def _disable_auto_oc_memberof(server):
 

      MEMBEROF_PLUGIN_DN = ('cn=' + PLUGIN_MEMBER_OF + ',cn=plugins,cn=config')
 
@@ -523,7 +524,8 @@ 
 

  

  @pytest.mark.ds49967
 

  def test_entrycache_on_modrdn_failure(topology_st):
 

-     """Specify a test case purpose or name here
 

+     """This test checks that when a modrdn fails, the destination entry is not returned by a search
 

+     This could happen in case the destination entry remains in the entry cache
 

  

      :id: a4d8ac0b-2448-406a-9dc2-5a72851e30b6
 

      :setup: Standalone Instance
 
@@ -669,6 +671,185 @@ 
 

              assert ent.dn == group2_dn
 

      assert found
 

  

+ def _config_memberof_silent_memberof_failure(server):
 

+     _config_memberof_entrycache_on_modrdn_failure(server)
 

+ 

+ def test_silent_memberof_failure(topology_st):
 

+     """This test checks that if during a MODRDN, the memberof plugin fails
 

+     then MODRDN also fails
 

+ 

+     :id: 095aee01-581c-43dd-a241-71f9631a18bb
 

+     :setup: Standalone Instance
 

+     :steps:
 

+         1. configure memberof to only scope ou=people,SUFFIX
 

+         2. Do some cleanup and Creates 10 users
 

+         3. Create groups0 (IN peoplebase) that contain user0 and user1
 

+         4. Check user0 and user1 have memberof=group0.dn
 

+         5. Create group1 (OUT peoplebase) that contain user0 and user1
 

+         6. Check user0 and user1 have NOT memberof=group1.dn
 

+         7. Move group1 IN peoplebase and check users0 and user1 HAVE memberof=group1.dn
 

+         8. Create group2 (OUT peoplebase) that contain user2 and user3.
 

+         9. Check user2 and user3 have NOT memberof=group2.dn
 

+         10. configure memberof so that added objectclass does not allow 'memberof' attribute
 

+         11. Move group2 IN peoplebase and check move failed OPERATIONS_ERROR (because memberof failed)
 

+         12. Check user2 and user3 have NOT memberof=group2.dn
 

+         13. ADD group3 (IN peoplebase) with user4 and user5 members and check add failed OPERATIONS_ERROR (because memberof failed)
 

+         14. Check user4 and user5 have NOT memberof=group2.dn
 

+     :expectedresults:
 

+         1. should succeed
 

+         2. should succeed
 

+         3. should succeed
 

+         4. should succeed
 

+         5. should succeed
 

+         6. should succeed
 

+         7. should succeed
 

+         8. should succeed
 

+         9. should succeed
 

+         10. should succeed
 

+         11. should fail OPERATION_ERROR because memberof plugin fails to add 'memberof' to members.
 

+         12. should succeed
 

+         14. should fail OPERATION_ERROR because memberof plugin fails to add 'memberof' to members
 

+         14. should succeed
 

+     """
 

+     # only scopes peoplebase
 

+     _config_memberof_silent_memberof_failure(topology_st.standalone)
 

+     topology_st.standalone.restart(timeout=10)
 

+ 

+     # first do some cleanup
 

+     peoplebase = 'ou=people,%s' % SUFFIX
 

+     for i in range(10):
 

+         cn = 'user%d' % i
 

+         dn = 'cn=%s,%s' % (cn, peoplebase)
 

+         topology_st.standalone.delete_s(dn)
 

+     topology_st.standalone.delete_s('cn=group_in0,%s' % peoplebase)
 

+     topology_st.standalone.delete_s('cn=group_in1,%s' % peoplebase)
 

+     topology_st.standalone.delete_s('cn=group_out2,%s' % SUFFIX)
 

+ 

+     # create 10 users
 

+     for i in range(10):
 

+         cn = 'user%d' % i
 

+         dn = 'cn=%s,%s' % (cn, peoplebase)
 

+         log.fatal('Adding user (%s): ' % dn)
 

+         topology_st.standalone.add_s(Entry((dn, {'objectclass': ['top', 'person'],
 

+                              'sn': 'user_%s' % cn,
 

+                              'description': 'add on standalone'})))
 

+ 

+     # Check that members of group0 (in the scope) have 'memberof
 

+     group0_dn = 'cn=group_in0,%s' % peoplebase
 

+     topology_st.standalone.add_s(Entry((group0_dn, {'objectclass': ['top', 'groupofnames'],
 

+                              'member': [
 

+                                    'cn=user0,%s' % peoplebase,
 

+                                    'cn=user1,%s' % peoplebase,
 

+                                    ],
 

+                              'description': 'mygroup'})))
 

+ 

+     # Check the those entries have memberof with group0
 

+     for i in range(2):
 

+         user_dn = 'cn=user%d,%s' % (i, peoplebase)
 

+         ent = topology_st.standalone.getEntry(user_dn, ldap.SCOPE_BASE, "(objectclass=*)", ['memberof'])
 

+         assert ent.hasAttr('memberof')
 

+         found = False
 

+         for val in ent.getValues('memberof'):
 

+             topology_st.standalone.log.info("!!!!!!! %s: memberof->%s (vs %s)" % (user_dn, val, group0_dn.encode().lower()))
 

+             if val.lower() == group0_dn.encode().lower():
 

+                 found = True
 

+                 break
 

+         assert found
 

+ 

+     # Create a group1 out of the scope
 

+     group1_dn = 'cn=group_out1,%s' % SUFFIX
 

+     topology_st.standalone.add_s(Entry((group1_dn, {'objectclass': ['top', 'groupofnames'],
 

+                              'member': [
 

+                                    'cn=user0,%s' % peoplebase,
 

+                                    'cn=user1,%s' % peoplebase,
 

+                                    ],
 

+                              'description': 'mygroup'})))
 

+ 

+     # Check the those entries have not memberof with group1
 

+     for i in range(2):
 

+         user_dn = 'cn=user%d,%s' % (i, peoplebase)
 

+         ent = topology_st.standalone.getEntry(user_dn, ldap.SCOPE_BASE, "(objectclass=*)", ['memberof'])
 

+         assert ent.hasAttr('memberof')
 

+         found = False
 

+         for val in ent.getValues('memberof'):
 

+             topology_st.standalone.log.info("!!!!!!! %s: memberof->%s (vs %s)" % (user_dn, val, group1_dn.encode().lower()))
 

+             if val.lower() == group1_dn.encode().lower():
 

+                 found = True
 

+                 break
 

+         assert not found
 

+ 

+     # move group1 into the scope and check user0 and user1 are memberof group1
 

+     topology_st.standalone.rename_s(group1_dn, 'cn=group_in1', newsuperior=peoplebase, delold=0)
 

+     new_group1_dn = 'cn=group_in1,%s' % peoplebase
 

+     for i in range(2):
 

+         user_dn = 'cn=user%d,%s' % (i, peoplebase)
 

+         ent = topology_st.standalone.getEntry(user_dn, ldap.SCOPE_BASE, "(objectclass=*)", ['memberof'])
 

+         assert ent.hasAttr('memberof')
 

+         found = False
 

+         for val in ent.getValues('memberof'):
 

+             topology_st.standalone.log.info("!!!!!!! %s: memberof->%s (vs %s)" % (user_dn, val, new_group1_dn.encode().lower()))
 

+             if val.lower() == new_group1_dn.encode().lower():
 

+                 found = True
 

+                 break
 

+         assert found
 

+ 

+     # Create a group2 out of the scope
 

+     group2_dn = 'cn=group_out2,%s' % SUFFIX
 

+     topology_st.standalone.add_s(Entry((group2_dn, {'objectclass': ['top', 'groupofnames'],
 

+                              'member': [
 

+                                    'cn=user2,%s' % peoplebase,
 

+                                    'cn=user3,%s' % peoplebase,
 

+                                    ],
 

+                              'description': 'mygroup'})))
 

+ 

+     # Check the those entries have not memberof with group2
 

+     for i in (2, 3):
 

+         user_dn = 'cn=user%d,%s' % (i, peoplebase)
 

+         ent = topology_st.standalone.getEntry(user_dn, ldap.SCOPE_BASE, "(objectclass=*)", ['memberof'])
 

+         assert not ent.hasAttr('memberof')
 

+ 

+     # memberof will not add the missing objectclass
 

+     _disable_auto_oc_memberof(topology_st.standalone)
 

+     topology_st.standalone.restart(timeout=10)
 

+ 

+     # move group2 into the scope and check it fails
 

+     try:
 

+         topology_st.standalone.rename_s(group2_dn, 'cn=group_in2', newsuperior=peoplebase, delold=0)
 

+         topology_st.standalone.log.info("This is unexpected, modrdn should fail as the member entry have not the appropriate objectclass")
 

+         assert False
 

+     except ldap.OPERATIONS_ERROR:
 

+         pass
 

+ 

+     # Check the those entries have not memberof
 

+     for i in (2, 3):
 

+         user_dn = 'cn=user%d,%s' % (i, peoplebase)
 

+         ent = topology_st.standalone.getEntry(user_dn, ldap.SCOPE_BASE, "(objectclass=*)", ['memberof'])
 

+         topology_st.standalone.log.info("Should assert %s has memberof is %s" % (user_dn, ent.hasAttr('memberof')))
 

+         assert not ent.hasAttr('memberof')
 

+ 

+     # Create a group3 in the scope
 

+     group3_dn = 'cn=group3_in,%s' % peoplebase
 

+     try:
 

+         topology_st.standalone.add_s(Entry((group3_dn, {'objectclass': ['top', 'groupofnames'],
 

+                              'member': [
 

+                                    'cn=user4,%s' % peoplebase,
 

+                                    'cn=user5,%s' % peoplebase,
 

+                                    ],
 

+                              'description': 'mygroup'})))
 

+         topology_st.standalone.log.info("This is unexpected, ADD should fail as the member entry have not the appropriate objectclass")
 

+         assert False
 

+     except ldap.OBJECT_CLASS_VIOLATION:
 

+         pass
 

+     except ldap.OPERATIONS_ERROR:
 

+         pass
 

+ 

+     # Check the those entries have not memberof
 

+     for i in (4, 5):
 

+         user_dn = 'cn=user%d,%s' % (i, peoplebase)
 

+         ent = topology_st.standalone.getEntry(user_dn, ldap.SCOPE_BASE, "(objectclass=*)", ['memberof'])
 

+         topology_st.standalone.log.info("Should assert %s has memberof is %s" % (user_dn, ent.hasAttr('memberof')))
 

+         assert not ent.hasAttr('memberof')
 

+ 

  if __name__ == '__main__':
 

      # Run isolated
 

      # -s for DEBUG mode

file modified
+2 -2 
@@ -1973,7 +1973,7 @@ 
 

      int hint = slapi_attr_first_value(attr, &val);
 

      Slapi_DN *sdn = slapi_sdn_new();
 

  

-     while (val) {
 

+     while (val && (rc == 0)) {
 

          char *dn_str = 0;
 

          struct berval *bv = (struct berval *)slapi_value_get_berval(val);
 

  
@@ -1996,7 +1996,7 @@ 
 

          strncpy(dn_str, bv->bv_val, (size_t)bv->bv_len);
 

  

          slapi_sdn_set_normdn_byref(sdn, dn_str); /* dn_str is normalized */
 

-         memberof_modop_one_replace_r(pb, config, LDAP_MOD_REPLACE,
 

+         rc = memberof_modop_one_replace_r(pb, config, LDAP_MOD_REPLACE,
 

                                       post_sdn, pre_sdn, post_sdn, sdn, 0);
 

  

          hint = slapi_attr_next_value(attr, hint, &val);

Bug Description:
when adding 'memberof' to a member entry, the update may fail
(invalid schema, db errors...).
The error is reported at upper level. But in case of MODRDN
the error is lost in memberof_moddn_attr_list where returned
code of memberof_modop_one_replace_r is not tested

Fix Description:
Report a failure in memberof_moddn_attr_list as soon as
memberof_modop_one_replace_r fails

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

Reviewed by: ?

Platforms tested: F27

Flag Day: no

Doc impact: no

We need the tests to be discoverable... Could you please rename the test function to meaningful.

Also, please, move the test case to somewhere like - dirsrvtests/tests/suites/memberof_plugin/regression_test.py

We need the docstrings... I see you already have them as comments. Could you please put it here?
Also, for :expectedresults: you can put something simple like 1. Success.

It can be removed

Maybe @vashirov will have more comments regarding the test case. But it looks okay to me.

Also, C code looks good to me too. But I have a question because I am not very familiar with the part of the code base.
I see that you changed the behaviour a bit.

Before - in the memberof_moddn_attr_list - we go through all of the values and do memberof_modop_one_replace_r on each of them even though some of them can fail.

Now - in the memberof_moddn_attr_list - we go through the values and do memberof_modop_one_replace_r on each of them but we stop when there is a failure. So it is possible that some of the values were replaced succeffully and some of them not because we stopped in the middle of the list.
(and then we report that the whole operation is failed - https://pagure.io/fork/tbordaz/389-ds-base/blob/ticket_49985/f/ldap/servers/plugins/memberof/memberof.c#_923 )

Should be mention that a part of the list was successfully replaced or is it okay like this?
Wouldn't it affect something else in the plugin fuctionality?

Okay, so I don't really agree with the fix or the test because this function here should fail. The issue is that our schema check only checks that the auto_oc_memberof is a valid objectclass, not that it's a valid objectClass that can take memberOf as the attribute. So I would rather have the test attempt this change and fail here, than mask the failure with partial operation.

I think that this would be much better from an operator perspective, because we make the service "impossible" to misconfigure, rather than this which tolerates the misconfiguration.

To put another way: I would like to see this fix approached differently than it currently is. If this is urgent, then "ack" to this change, but I would rather have it fixed in the manner I have referred to above. Thanks,

I think there are two issues.

The first one (this ticket) is that, during a MODRDN, if memberof fails to update a member it does not trigger the failure of the MODRDN.
Here I trigger the failure of the update of a member with an invalid objectclass but it is just to create a reproducible test case. In production what can happen is a db_deadlock, db_out_of_lock,... that is much more difficult to recreate with a test case.

The second issue is that auto_oc_memberof can contain a useless objectclass. Should it be detected during the configuration, during the update or simply not detected (Note that the failure is logged in error log), I have no strong opinion.
Just I think the safest would be to detect it during update but I am fear of performance impact when we have many members to update.

@tbordaz We only need to validate the objectClass as valid when we change the memberof configuration, not on every operation though.

update: ack to the fix because it resolves the first case, and can we open a ticket for validation of the oc content on configuration change and load? That sounds like a good approach to resolve both, but it will mean we have to find a different way to test this scenario in the lib389 tests.

Edited 5 years ago by firstyear

@firstyear I agree with your comment https://pagure.io/389-ds-base/issue/49967#comment-535339. It is precious to have a way to trigger plugin failure.
So I have a doubt about checking that configured OC allows 'memberof' as it will remove an easy way for us to trigger plugin failures.
An option would be to let admin configure invalid OC but when the failure occurs to log a more helpful message how to resolve it.

Thanks for the ACK. I will update the testcase following @spichugi recommendations

rebased onto b8034f9dbe18825275d06008a399718c48f79c08
5 years ago

rebased onto a06c232
5 years ago

Thanks @firstyear for the ACK. merging the patch.
I move the testcase to the member regression suite

Pull-Request has been merged by tbordaz
5 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.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3045

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi
3 years ago
Metadata
None
No Tags
Changes Summary 2
+183 -2
file changed
dirsrvtests/tests/suites/memberof_plugin/regression_test.py
+2 -2
file changed
ldap/servers/plugins/memberof/memberof.c
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.