#51224 Issue 51222 - It should not be allowed to delete Managed Entry manually
Closed 3 years ago by spichugi. Opened 3 years ago by spichugi.
spichugi/389-ds-base i51222  into  master

@@ -17,10 +17,12 @@ 

  from lib389.idm.organizationalunit import OrganizationalUnits, OrganizationalUnit

  from lib389.plugins import MEPTemplates, MEPConfigs, ManagedEntriesPlugin, MEPTemplate

  from lib389.idm.nscontainer import nsContainers

+ from lib389.idm.domain import Domain

  from lib389.tasks import Entry

  import ldap

  

  pytestmark = pytest.mark.tier1

+ USER_PASSWORD = 'password'

  

  

  @pytest.fixture(scope="module")
@@ -249,9 +251,100 @@ 

      assert user.get_attr_val_utf8('mepManagedEntry') != before

  

  

+ def test_managed_entry_removal(topo):

+     """Check that we can't remove managed entry manually

+ 

+     :id: cf9c5be5-97ef-46fc-b199-8346acf4c296

+     :setup: Standalone Instance

+     :steps:

+         1. Enable the plugin

+         2. Restart the instance

+         3. Add our org units

+         4. Set up config entry and template entry for the org units

+         5. Add an entry that meets the MEP scope

+         6. Check if a managed group entry was created

+         7. Try to remove the entry while bound as Admin (non-DM)

+         8. Remove the entry while bound as DM

+         9. Check that the managing entry can be deleted too

+     :expectedresults:

+         1. Success

+         2. Success

+         3. Success

+         4. Success

+         5. Success

+         6. Success

+         7. Should fail

+         8. Success

+         9. Success

+     """

+ 

+     inst = topo.standalone

+ 

+     # Add ACI so we can test that non-DM user can't delete managed entry

+     domain = Domain(inst, DEFAULT_SUFFIX)

+     ACI_TARGET = f"(target = \"ldap:///{DEFAULT_SUFFIX}\")"

+     ACI_TARGETATTR = "(targetattr = *)"

+     ACI_ALLOW = "(version 3.0; acl \"Admin Access\"; allow (all) "

+     ACI_SUBJECT = "(userdn = \"ldap:///anyone\");)"

+     ACI_BODY = ACI_TARGET + ACI_TARGETATTR + ACI_ALLOW + ACI_SUBJECT

+     domain.add('aci', ACI_BODY)

+ 

+     # stop the plugin, and start it

+     plugin = ManagedEntriesPlugin(inst)

+     plugin.disable()

+     plugin.enable()

+ 

+     # Add our org units

+     ous = OrganizationalUnits(inst, DEFAULT_SUFFIX)

+     ou_people = ous.create(properties={'ou': 'managed_people'})

+     ou_groups = ous.create(properties={'ou': 'managed_groups'})

+ 

+     mep_templates = MEPTemplates(inst, DEFAULT_SUFFIX)

+     mep_template1 = mep_templates.create(properties={

+         'cn': 'MEP template',

+         'mepRDNAttr': 'cn',

+         'mepStaticAttr': 'objectclass: groupOfNames|objectclass: extensibleObject'.split('|'),

+         'mepMappedAttr': 'cn: $cn|uid: $cn|gidNumber: $uidNumber'.split('|')

+     })

+     mep_configs = MEPConfigs(inst)

+     mep_configs.create(properties={'cn': 'config',

+                                    'originScope': ou_people.dn,

+                                    'originFilter': 'objectclass=posixAccount',

+                                    'managedBase': ou_groups.dn,

+                                    'managedTemplate': mep_template1.dn})

+     inst.restart()

+ 

+     # Add an entry that meets the MEP scope

+     test_users_m1 = UserAccounts(inst, DEFAULT_SUFFIX, rdn='ou={}'.format(ou_people.rdn))

+     managing_entry = test_users_m1.create_test_user(1001)

+     managing_entry.reset_password(USER_PASSWORD)

+     user_bound_conn = managing_entry.bind(USER_PASSWORD)

+ 

+     # Get the managed entry

+     managed_groups = Groups(inst, ou_groups.dn, rdn=None)

+     managed_entry = managed_groups.get(managing_entry.rdn)

+ 

+     # Check that the managed entry was created

+     assert managed_entry.exists()

+ 

+     # Try to remove the entry while bound as Admin (non-DM)

+     managed_groups_user_conn = Groups(user_bound_conn, ou_groups.dn, rdn=None)

+     managed_entry_user_conn = managed_groups_user_conn.get(managed_entry.rdn)

+     with pytest.raises(ldap.UNWILLING_TO_PERFORM):

+         managed_entry_user_conn.delete()

+     assert managed_entry_user_conn.exists()

+ 

+     # Remove the entry while bound as DM

+     managed_entry.delete()

+     assert not managed_entry.exists()

+ 

+     # Check that the managing entry can be deleted too

+     managing_entry.delete()

+     assert not managing_entry.exists()

+ 

+ 

  if __name__ == '__main__':

      # Run isolated

      # -s for DEBUG mode

      CURRENT_FILE = os.path.realpath(__file__)

      pytest.main("-s %s" % CURRENT_FILE)

- 

file modified
+39 -14
@@ -2158,6 +2158,7 @@ 

              Slapi_Mod *next_mod = NULL;

              char *origin_dn = NULL;

              Slapi_DN *origin_sdn = NULL;

+             char *requestor_dn = NULL;

  

              /* Fetch the target entry. */

              if (sdn) {
@@ -2249,11 +2250,19 @@ 

  

                      slapi_ch_free_string(&origin_dn);

                  } else {

-                     errstr = slapi_ch_smprintf("%s a managed entry is not allowed. "

-                                                "It needs to be manually unlinked first.",

-                                                modop == LDAP_CHANGETYPE_DELETE ? "Deleting"

-                                                                                : "Renaming");

-                     ret = LDAP_UNWILLING_TO_PERFORM;

+                     slapi_pblock_get(pb, SLAPI_REQUESTOR_DN, &requestor_dn);

+                     if (slapi_dn_isroot(requestor_dn)) {

+                         slapi_log_err(SLAPI_LOG_PLUGIN, MEP_PLUGIN_SUBSYSTEM,

+                                       "mep_pre_op - %s is %s a managed entry.",

+                                        requestor_dn, modop == LDAP_CHANGETYPE_DELETE ? "deleting"

+                                                                                      : "renaming");

+                     } else {

+                         errstr = slapi_ch_smprintf("%s a managed entry is not allowed. "

+                                                    "It needs to be manually unlinked first.",

+                                                    modop == LDAP_CHANGETYPE_DELETE ? "Deleting"

+                                                                                    : "Renaming");

+                         ret = LDAP_UNWILLING_TO_PERFORM;

+                     }

                  }

              }

          }
@@ -2587,10 +2596,18 @@ 

              slapi_delete_internal_pb(mep_pb);

              slapi_pblock_get(mep_pb, SLAPI_PLUGIN_INTOP_RESULT, &result);

              if (result) {

-                 slapi_log_err(SLAPI_LOG_ERR, MEP_PLUGIN_SUBSYSTEM,

-                               "mep_del_post_op - Failed to delete managed entry "

-                               "(%s) - error (%d)\n",

-                               managed_dn, result);

+                 if (result == LDAP_NO_SUCH_OBJECT) {

+                     slapi_log_err(SLAPI_LOG_PLUGIN, MEP_PLUGIN_SUBSYSTEM,

+                                   "mep_del_post_op - Failed to delete managed entry "

+                                   "(%s) - it doesn't exist already)\n",

+                                   managed_dn);

+                     result = SLAPI_PLUGIN_SUCCESS;

+                 } else {

+                     slapi_log_err(SLAPI_LOG_ERR, MEP_PLUGIN_SUBSYSTEM,

+                                   "mep_del_post_op - Failed to delete managed entry "

+                                   "(%s) - error (%d)\n",

+                                   managed_dn, result);

+                 }

              }

              slapi_ch_free_string(&managed_dn);

              slapi_pblock_destroy(mep_pb);
@@ -2702,11 +2719,19 @@ 

              slapi_delete_internal_pb(mep_pb);

              slapi_pblock_get(mep_pb, SLAPI_PLUGIN_INTOP_RESULT, &result);

              if (result) {

-                 slapi_log_err(SLAPI_LOG_ERR, MEP_PLUGIN_SUBSYSTEM,

-                               "mep_modrdn_post_op - Failed to delete managed entry "

-                               "(%s) - error (%d)\n",

-                               managed_dn, result);

-                 goto bailmod;

+                 if (result == LDAP_NO_SUCH_OBJECT) {

+                     slapi_log_err(SLAPI_LOG_PLUGIN, MEP_PLUGIN_SUBSYSTEM,

+                                   "mep_modrdn_post_op - Failed to delete managed entry "

+                                   "(%s) - it doesn't exist already)\n",

+                                   managed_dn);

+                     result = SLAPI_PLUGIN_SUCCESS;

+                 } else {

+                     slapi_log_err(SLAPI_LOG_ERR, MEP_PLUGIN_SUBSYSTEM,

+                                   "mep_modrdn_post_op - Failed to delete managed entry "

+                                   "(%s) - error (%d)\n",

+                                   managed_dn, result);

+                     goto bailmod;

+                 }

              }

              /* Clear out the pblock for reuse. */

              slapi_pblock_init(mep_pb);

@@ -404,6 +404,9 @@ 

                  delete_tombstone_entry = operation_is_flag_set(operation, OP_FLAG_TOMBSTONE_ENTRY);

              }

  

+             /* Save away a copy of the entry, before modifications */

The patch keeps a copy of the entry before calling betxn_preop. Is it required with your fix or is it for code cleanup ?

+             slapi_pblock_set(pb, SLAPI_ENTRY_PRE_OP, slapi_entry_dup(e->ep_entry));

+ 

              /* call the transaction pre delete plugins just after the 

               * to-be-deleted entry is prepared. */

              /* these should not need to modify the entry to be deleted -
@@ -500,8 +503,6 @@ 

                      "entry: %s  - flags: delete %d is_tombstone_entry %d create %d \n",

                      dn, delete_tombstone_entry, is_tombstone_entry, create_tombstone_entry);

  #endif

-             /* Save away a copy of the entry, before modifications */

-             slapi_pblock_set(pb, SLAPI_ENTRY_PRE_OP, slapi_entry_dup(e->ep_entry));

  

              /*

               * Get the entry's parent. We do this here because index_read

Bug Description: It is possible to delete a managed entry and no error is raised.
Also, while doing delete or modrdn peration on a managing entry and the managed entry
doesn't exist, we should continue the operation.

Fix Description: We should put an entry struct duplicate to SLAPI_ENTRY_PRE_OP pblock
before we execute plugins PRE_OP. Also, we should allow to continue modrdn and delete
managing entry operations execution even when managed entry doesn't exists.
Allow 'cn=directory manager' to delete managed entry on direct update.

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

Reviewed by: ?

Minor, but should be =NULL here as good practice.

Is there an associated test case with this?

Besides all this, the C looks good :)

The patch keeps a copy of the entry before calling betxn_preop. Is it required with your fix or is it for code cleanup ?

rebased onto 8dd5685e76099dc394859ab71f66166fe213d4d5

3 years ago

rebased onto 097f8876a3b030605fe05e2123ed1543c150bdaa

3 years ago

Minor, but should be =NULL here as good practice.

Done

The patch keeps a copy of the entry before calling betxn_preop. Is it required with your fix or is it for code cleanup ?

Both :) I've made it consistent with other ldbm functions (i.e. https://pagure.io/389-ds-base/blob/master/f/ldap/servers/slapd/back-ldbm/ldbm_modify.c#_729 is before betxn_preop)

And it was the cause of the failure because we use the entry in MEP preop - https://pagure.io/389-ds-base/blob/master/f/ldap/servers/plugins/mep/mep.c#_2168

Also, I've added a test - please, check.

shouldn't it be 'managed_groups_user_conn.get(managed_entry.rdn)'

rebased onto 594bf91

3 years ago

shouldn't it be 'managed_groups_user_conn.get(managed_entry.rdn)'

Fixed. Please, check.

The patch looks good to me. ACK

Pull-Request has been merged by spichugi

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.

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

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