[PATCHES] replmd: fix for linked attribute replacement with related tests

Jeremy Allison jra at samba.org
Thu Oct 26 00:17:30 UTC 2017


On Thu, Oct 26, 2017 at 11:50:22AM +1300, Douglas Bagnall via samba-technical wrote:
> As Björn found in https://bugzilla.samba.org/show_bug.cgi?id=13095, the
> linked attribute changes in 4.7 allow you to introduce duplicate links
> using MOD_REPLACE. This is not good.
> 
> Here we fix that and test for it.
> 
> As well as the fix itself, we find that some of the linked attribute
> tests had flawed logic that would have allowed other failures.
> 
> Please review and push!

Do you want all of the commit messages to refer to the
bug URL ?

Currently only the first two do so. Just checking :-).

Cheers,

	Jeremy.

> From 38730587e3c4cb7b082f861e46afeb795d49163f Mon Sep 17 00:00:00 2001
> From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
> Date: Wed, 25 Oct 2017 10:54:42 +1300
> Subject: [PATCH 1/7] linked attribute tests: test against duplicates in
>  replace
> 
> We should not be able to introduce duplicate links using MOD_REPLACE.
> It turns out we could and weren't testing.
> 
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13095
> 
> Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
> ---
>  selftest/knownfail.d/ldap-linked-attributes    |  3 +++
>  source4/dsdb/tests/python/linked_attributes.py | 10 ++++++++++
>  2 files changed, 13 insertions(+)
>  create mode 100644 selftest/knownfail.d/ldap-linked-attributes
> 
> diff --git a/selftest/knownfail.d/ldap-linked-attributes b/selftest/knownfail.d/ldap-linked-attributes
> new file mode 100644
> index 00000000000..5fa50e3ea0c
> --- /dev/null
> +++ b/selftest/knownfail.d/ldap-linked-attributes
> @@ -0,0 +1,3 @@
> +# linked attribute replacement isn't checking for duplicates.
> +
> +samba4.ldap.linked_attributes.python.*test_la_links_replace
> diff --git a/source4/dsdb/tests/python/linked_attributes.py b/source4/dsdb/tests/python/linked_attributes.py
> index 6235bf77a89..705c9d5c0db 100644
> --- a/source4/dsdb/tests/python/linked_attributes.py
> +++ b/source4/dsdb/tests/python/linked_attributes.py
> @@ -464,6 +464,16 @@ class LATests(samba.tests.TestCase):
>          self.assert_back_links(u3, [g1])
>          self.assert_back_links(u4, [])
>  
> +        try:
> +            # adding u2 twice should be an error
> +            self.replace_linked_attribute(g2, [u1, u2, u3, u2])
> +        except ldb.LdbError as (num, msg):
> +            if num != ldb.ERR_ENTRY_ALREADY_EXISTS:
> +                self.fail("adding duplicate values, expected "
> +                          "ERR_ENTRY_ALREADY_EXISTS, (%d) "
> +                          "got %d" % (ldb.ERR_ENTRY_ALREADY_EXISTS, num))
> +        else:
> +            self.fail("replacing duplicate values succeeded when it shouldn't")
>  
>      def test_la_links_replace2(self):
>          users = self.add_objects(12, 'user', 'u_replace2')
> -- 
> 2.11.0
> 
> 
> From 006e030dd9914c9ad794cfde13853a1bcb69332d Mon Sep 17 00:00:00 2001
> From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
> Date: Wed, 25 Oct 2017 10:12:09 +1300
> Subject: [PATCH 2/7] replmd: check for duplicate values in MOD_REPLACE case
> 
> Because we already have a sorted parsed_dn list, this is a simple
> linear scan.
> 
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13095
> 
> Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
> ---
>  selftest/knownfail.d/ldap-linked-attributes     |  3 --
>  source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 37 +++++++++++++++++++++++++
>  2 files changed, 37 insertions(+), 3 deletions(-)
>  delete mode 100644 selftest/knownfail.d/ldap-linked-attributes
> 
> diff --git a/selftest/knownfail.d/ldap-linked-attributes b/selftest/knownfail.d/ldap-linked-attributes
> deleted file mode 100644
> index 5fa50e3ea0c..00000000000
> --- a/selftest/knownfail.d/ldap-linked-attributes
> +++ /dev/null
> @@ -1,3 +0,0 @@
> -# linked attribute replacement isn't checking for duplicates.
> -
> -samba4.ldap.linked_attributes.python.*test_la_links_replace
> diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
> index 1901ee1cc94..364219462e1 100644
> --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
> +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
> @@ -2133,6 +2133,37 @@ static int get_parsed_dns_trusted(struct ldb_module *module,
>  }
>  
>  /*
> +   Return LDB_SUCCESS if a parsed_dn list contains no duplicate values,
> +   otherwise an error code. For compatibility the error code differs depending
> +   on whether or not the attribute is "member".
> +
> +   As always, the parsed_dn list is assumed to be sorted.
> + */
> +static int check_parsed_dn_duplicates(struct ldb_module *module,
> +				      struct ldb_message_element *el,
> +				      struct parsed_dn *pdn)
> +{
> +	unsigned int i;
> +	struct ldb_context *ldb = ldb_module_get_ctx(module);
> +
> +	for (i = 1; i < el->num_values; i++) {
> +		struct parsed_dn *p = &pdn[i];
> +		if (parsed_dn_compare(p, &pdn[i - 1]) == 0) {
> +			ldb_asprintf_errstring(ldb,
> +					       "Linked attribute %s has "
> +					       "multiple identical values",
> +					       el->name);
> +			if (ldb_attr_cmp(el->name, "member") == 0) {
> +				return LDB_ERR_ENTRY_ALREADY_EXISTS;
> +			} else {
> +				return LDB_ERR_ATTRIBUTE_OR_VALUE_EXISTS;
> +			}
> +		}
> +	}
> +	return LDB_SUCCESS;
> +}
> +
> +/*
>    build a new extended DN, including all meta data fields
>  
>    RMD_FLAGS           = DSDB_RMD_FLAG_* bits
> @@ -2901,6 +2932,12 @@ static int replmd_modify_la_replace(struct ldb_module *module,
>  		return ret;
>  	}
>  
> +	ret = check_parsed_dn_duplicates(module, el, dns);
> +	if (ret != LDB_SUCCESS) {
> +		talloc_free(tmp_ctx);
> +		return ret;
> +	}
> +
>  	ret = get_parsed_dns(module, tmp_ctx, old_el, &old_dns,
>  			     ldap_oid, parent);
>  	if (ret != LDB_SUCCESS) {
> -- 
> 2.11.0
> 
> 
> From d98cc2d3dd581e69b2bb525845f7cea7560e73ea Mon Sep 17 00:00:00 2001
> From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
> Date: Wed, 25 Oct 2017 12:13:57 +1300
> Subject: [PATCH 3/7] linked attribute tests: ensure duplicate deletes fail
> 
> We can't remove the same thing twice in the same message.
> 
> Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
> ---
>  source4/dsdb/tests/python/linked_attributes.py | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/source4/dsdb/tests/python/linked_attributes.py b/source4/dsdb/tests/python/linked_attributes.py
> index 705c9d5c0db..f0c260564bf 100644
> --- a/source4/dsdb/tests/python/linked_attributes.py
> +++ b/source4/dsdb/tests/python/linked_attributes.py
> @@ -306,6 +306,11 @@ class LATests(samba.tests.TestCase):
>          self.remove_linked_attribute(g1, [])
>          self.assert_forward_links(g1, [])
>  
> +        # removing a duplicate link in the same message should fail
> +        self.add_linked_attribute(g2, [u1, u2])
> +        self.assertRaises(ldb.LdbError,
> +                          self.remove_linked_attribute,g2, [u1, u1])
> +
>      def _test_la_links_delete_link_reveal(self):
>          u1, u2 = self.add_objects(2, 'user', 'u_del_link_reveal')
>          g1, g2 = self.add_objects(2, 'group', 'g_del_link_reveal')
> -- 
> 2.11.0
> 
> 
> From 15fa30c9ab6ad181b6e43fe4df14d87ba2998801 Mon Sep 17 00:00:00 2001
> From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
> Date: Wed, 25 Oct 2017 12:17:05 +1300
> Subject: [PATCH 4/7] linked attribute tests: fix logic for add test
> 
> We were ensuring that when we got an LdbError it was the right type,
> but we weren't ensuring we got one at all.
> 
> The new test doesn't fail.
> 
> Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
> ---
>  source4/dsdb/tests/python/linked_attributes.py | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/source4/dsdb/tests/python/linked_attributes.py b/source4/dsdb/tests/python/linked_attributes.py
> index f0c260564bf..1923f687749 100644
> --- a/source4/dsdb/tests/python/linked_attributes.py
> +++ b/source4/dsdb/tests/python/linked_attributes.py
> @@ -401,6 +401,8 @@ class LATests(samba.tests.TestCase):
>                  self.fail("adding duplicate values, expected "
>                            "ERR_ENTRY_ALREADY_EXISTS, (%d) "
>                            "got %d" % (ldb.ERR_ENTRY_ALREADY_EXISTS, num))
> +        else:
> +            self.fail("adding duplicate values succeed when it shouldn't")
>  
>          self.assert_forward_links(g1, [u1, u2, u3, u4])
>          self.assert_forward_links(g2, [u3, u1])
> -- 
> 2.11.0
> 
> 
> From b085d5bcf77c65e1293d7b83e078c60d7700caa3 Mon Sep 17 00:00:00 2001
> From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
> Date: Wed, 25 Oct 2017 12:31:08 +1300
> Subject: [PATCH 5/7] replmd: use check_parsed_dn_duplicates() more widely
> 
> replmd_add_fix_la() was already making the same check; here we move it
> a bit earlier.
> 
> Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
> ---
>  source4/dsdb/samdb/ldb_modules/repl_meta_data.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
> index 364219462e1..c443102d98c 100644
> --- a/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
> +++ b/source4/dsdb/samdb/ldb_modules/repl_meta_data.c
> @@ -938,6 +938,10 @@ static int get_parsed_dns(struct ldb_module *module, TALLOC_CTX *mem_ctx,
>  			  struct ldb_message_element *el, struct parsed_dn **pdn,
>  			  const char *ldap_oid, struct ldb_request *parent);
>  
> +static int check_parsed_dn_duplicates(struct ldb_module *module,
> +				      struct ldb_message_element *el,
> +				      struct parsed_dn *pdn);
> +
>  /*
>    fix up linked attributes in replmd_add.
>    This involves setting up the right meta-data in extended DN
> @@ -979,6 +983,12 @@ static int replmd_add_fix_la(struct ldb_module *module, TALLOC_CTX *mem_ctx,
>  		return ret;
>  	}
>  
> +	ret = check_parsed_dn_duplicates(module, el, pdn);
> +	if (ret != LDB_SUCCESS) {
> +		talloc_free(tmp_ctx);
> +		return ret;
> +	}
> +
>  	new_values = talloc_array(tmp_ctx, struct ldb_val, el->num_values);
>  	if (new_values == NULL) {
>  		ldb_module_oom(module);
> @@ -988,17 +998,6 @@ static int replmd_add_fix_la(struct ldb_module *module, TALLOC_CTX *mem_ctx,
>  
>  	for (i = 0; i < el->num_values; i++) {
>  		struct parsed_dn *p = &pdn[i];
> -		if (i > 0 && parsed_dn_compare(p, &pdn[i - 1]) == 0) {
> -			ldb_asprintf_errstring(ldb,
> -					"Linked attribute %s has "
> -					"multiple identical values", el->name);
> -			talloc_free(tmp_ctx);
> -			if (ldb_attr_cmp(el->name, "member") == 0) {
> -				return LDB_ERR_ENTRY_ALREADY_EXISTS;
> -			} else {
> -				return LDB_ERR_ATTRIBUTE_OR_VALUE_EXISTS;
> -			}
> -		}
>  		ret = replmd_build_la_val(el->values, p->v, p->dsdb_dn,
>  					  &ac->our_invocation_id,
>  					  ac->seq_num, now);
> -- 
> 2.11.0
> 
> 
> From da2be1ce433d76120afea80c358c22a2f35d78cb Mon Sep 17 00:00:00 2001
> From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
> Date: Wed, 25 Oct 2017 11:57:50 +1300
> Subject: [PATCH 6/7] linked_attribute tests: helper assert function for
>  expected LdbError
> 
> The logic involved in asserting that a function raises an LdbError with
> a particular error value has shown itself to be too complicated for me
> to repeat too often.
> 
> To test this function, you would want a put a test in a bit like this:
> 
>     def test_assertRaisesLdbError(self):
>         for i in [1, 2, ldb.ERR_ENTRY_ALREADY_EXISTS, 999]:
>             def f(*args, **kwargs):
>                 raise ldb.LdbError(i, 'msg %s' % i)
>             self.assertRaisesLdbError(i, 'a message', f, 'la la', la='la')
> 
>             def f2(*args, **kwargs):
>                 raise ldb.LdbError(i + 1, 'msg %s' % i)
>             def f3(*args, **kwargs):
>                 pass
>             for f in (f2, f3):
>                 try:
>                     self.assertRaisesLdbError(i, 'a message', f, 'la la', la='la')
>                 except AssertionError as e:
>                     print i, e, f
>                     pass
>                 else:
>                     print i, f
>                     self.fail('assertRaisesLdbError() failed to fail!')
> 
> ..but a self-testing test-tester is getting a too meta to run in every
> autobuild.
> 
> Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
> ---
>  source4/dsdb/tests/python/linked_attributes.py | 49 +++++++++++++++-----------
>  1 file changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/source4/dsdb/tests/python/linked_attributes.py b/source4/dsdb/tests/python/linked_attributes.py
> index 1923f687749..d18a667c4fe 100644
> --- a/source4/dsdb/tests/python/linked_attributes.py
> +++ b/source4/dsdb/tests/python/linked_attributes.py
> @@ -165,6 +165,26 @@ class LATests(samba.tests.TestCase):
>                                  attrs=['objectGUID'])
>          return str(misc.GUID(res[0]['objectGUID'][0]))
>  
> +    def assertRaisesLdbError(self, errcode, msg, f, *args, **kwargs):
> +        """Assert a function raises a particular LdbError."""
> +        try:
> +            f(*args, **kwargs)
> +        except ldb.LdbError as (num, msg):
> +            if num != errcode:
> +                lut = {v: k for k, v in vars(ldb).iteritems()
> +                       if k.startswith('ERR_') and isinstance(v, int)}
> +                self.fail("%s, expected "
> +                          "LdbError %s, (%d) "
> +                          "got %s (%d)" % (msg,
> +                                           lut.get(errcode), errcode,
> +                                           lut.get(num), num))
> +        else:
> +            lut = {v: k for k, v in vars(ldb).iteritems()
> +                   if k.startswith('ERR_') and isinstance(v, int)}
> +            self.fail("%s, expected "
> +                      "LdbError %s, (%d) "
> +                      "but we got success" % (msg, lut.get(errcode), errcode))
> +
>      def _test_la_backlinks(self, reveal=False):
>          tag = 'backlinks'
>          kwargs = {}
> @@ -393,16 +413,10 @@ class LATests(samba.tests.TestCase):
>          self.add_linked_attribute(g2, [u3, u1])
>          self.add_linked_attribute(g3, u2)
>  
> -        try:
> -            # adding u2 twice should be an error
> -            self.add_linked_attribute(g2, [u1, u2, u3, u2])
> -        except ldb.LdbError as (num, msg):
> -            if num != ldb.ERR_ENTRY_ALREADY_EXISTS:
> -                self.fail("adding duplicate values, expected "
> -                          "ERR_ENTRY_ALREADY_EXISTS, (%d) "
> -                          "got %d" % (ldb.ERR_ENTRY_ALREADY_EXISTS, num))
> -        else:
> -            self.fail("adding duplicate values succeed when it shouldn't")
> +        self.assertRaisesLdbError(ldb.ERR_ENTRY_ALREADY_EXISTS,
> +                                  "adding duplicate values",
> +                                  self.add_linked_attribute, g2,
> +                                  [u1, u2, u3, u2])
>  
>          self.assert_forward_links(g1, [u1, u2, u3, u4])
>          self.assert_forward_links(g2, [u3, u1])
> @@ -471,16 +485,11 @@ class LATests(samba.tests.TestCase):
>          self.assert_back_links(u3, [g1])
>          self.assert_back_links(u4, [])
>  
> -        try:
> -            # adding u2 twice should be an error
> -            self.replace_linked_attribute(g2, [u1, u2, u3, u2])
> -        except ldb.LdbError as (num, msg):
> -            if num != ldb.ERR_ENTRY_ALREADY_EXISTS:
> -                self.fail("adding duplicate values, expected "
> -                          "ERR_ENTRY_ALREADY_EXISTS, (%d) "
> -                          "got %d" % (ldb.ERR_ENTRY_ALREADY_EXISTS, num))
> -        else:
> -            self.fail("replacing duplicate values succeeded when it shouldn't")
> +        self.assertRaisesLdbError(ldb.ERR_ENTRY_ALREADY_EXISTS,
> +                                  "replacing duplicate values",
> +                                  self.replace_linked_attribute, g2,
> +                                  [u1, u2, u3, u2])
> +
>  
>      def test_la_links_replace2(self):
>          users = self.add_objects(12, 'user', 'u_replace2')
> -- 
> 2.11.0
> 
> 
> From 5d7ebf4c42056a771f866425a475b78c57762594 Mon Sep 17 00:00:00 2001
> From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
> Date: Wed, 25 Oct 2017 12:57:09 +1300
> Subject: [PATCH 7/7] linked attribute tests: correct add_all_at_once test
> 
> Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
> ---
>  source4/dsdb/tests/python/linked_attributes.py | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/source4/dsdb/tests/python/linked_attributes.py b/source4/dsdb/tests/python/linked_attributes.py
> index d18a667c4fe..09f83f8ca4f 100644
> --- a/source4/dsdb/tests/python/linked_attributes.py
> +++ b/source4/dsdb/tests/python/linked_attributes.py
> @@ -616,14 +616,11 @@ class LATests(samba.tests.TestCase):
>          (g2,) = self.add_objects(1, 'group', 'g_all_at_once2',
>                                   more_attrs={'member': users[:5]})
>  
> -        try:
> -            self.add_objects(1, 'group', 'g_with_duplicate_links',
> -                             more_attrs={'member': users[:5] + users[1:2]})
> -        except ldb.LdbError as (num, msg):
> -            if num != ldb.ERR_ENTRY_ALREADY_EXISTS:
> -                self.fail("adding duplicate values, expected "
> -                          "ERR_ENTRY_ALREADY_EXISTS, (%d) "
> -                          "got %d" % (ldb.ERR_ENTRY_ALREADY_EXISTS, num))
> +        self.assertRaisesLdbError(ldb.ERR_ENTRY_ALREADY_EXISTS,
> +                                  "adding multiple duplicate values",
> +                                  self.add_objects, 1, 'group',
> +                                  'g_with_duplicate_links',
> +                                  more_attrs={'member': users[:5] + users[1:2]})
>  
>          self.assert_forward_links(g1, users)
>          self.assert_forward_links(g2, users[:5])
> -- 
> 2.11.0
> 




More information about the samba-technical mailing list