[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