[PATCH] Fix for bug 13664, group membership changes logged incorrectly

Gary Lockyer gary at catalyst.net.nz
Tue Oct 30 00:21:08 UTC 2018


Fixes for Bug 13664 https://bugzilla.samba.org/show_bug.cgi?id=13664

Group membership changes were logged incorrectly, due to a bug in the
dn_compare function.

Thanks to Metze for the debugging that located the bug, and for
suggesting the changes to dn_compare.

Review appreciated

Thanks
Gary


-------------- next part --------------
From 9c49ff18cceed9643526d8ba2916ca3e67bdfd22 Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Tue, 23 Oct 2018 17:14:34 +1300
Subject: [PATCH 1/5] dsdb group_audit: Test to replicate BUG 13664

The group audit code incorrectly logs member additions and deletions.

Thanks to metze for the debugging that isolated the issue, and for
suggesting the fix to dn_compare.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13664

Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
 selftest/knownfail.d/bug13664                 |   1 +
 .../ldb_modules/tests/test_group_audit.c      | 260 +++++++++++++++++-
 2 files changed, 260 insertions(+), 1 deletion(-)
 create mode 100644 selftest/knownfail.d/bug13664

diff --git a/selftest/knownfail.d/bug13664 b/selftest/knownfail.d/bug13664
new file mode 100644
index 00000000000..6ffbf1297c3
--- /dev/null
+++ b/selftest/knownfail.d/bug13664
@@ -0,0 +1 @@
+^samba4.dsdb.samdb.ldb_modules.group_audit.test_log_membership_changes_removed
diff --git a/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c b/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
index 8551dcbf705..4c406c5fdfa 100644
--- a/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
+++ b/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
@@ -63,6 +63,142 @@ int dsdb_search_one(struct ldb_context *ldb,
 	return g_status;
 }
 
+/*
+ * Mock version of audit_log_json
+ */
+
+#define MAX_EXPECTED_MESSAGES 16
+static struct json_object messages[MAX_EXPECTED_MESSAGES];
+static size_t messages_sent = 0;
+
+void audit_message_send(
+	struct imessaging_context *msg_ctx,
+	const char *server_name,
+	uint32_t message_type,
+	struct json_object *message)
+{
+	messages[messages_sent].root = json_deep_copy(message->root);
+	messages[messages_sent].valid = message->valid;
+	messages_sent++;
+}
+
+#define check_group_change_message(m, u, a)\
+	_check_group_change_message(m, u, a, __FILE__, __LINE__);
+/*
+ * declare the internal cmocka cm_print_error so that we can output messages
+ * in sub unit format
+ */
+void cm_print_error(const char * const format, ...);
+
+/*
+ * Validate a group change JSON audit message
+ *
+ * It should contain 3 elements.
+ * Have a type of "groupChange"
+ * Have a groupChange element
+ *
+ * The group change element should have 10 elements.
+ *
+ * There should be a user element matching the expected value
+ * There should be an action matching the expected value
+ */
+static void _check_group_change_message(
+	const int message,
+	const char *user,
+	const char *action,
+	const char *file,
+	const int line)
+{
+	struct json_object json;
+	json_t *audit = NULL;
+	json_t *v = NULL;
+	const char* value;
+	json = messages[message];
+
+	/*
+	 * Validate the root JSON element
+	 * check the number of elements
+	 */
+	if (json_object_size(json.root) != 3) {
+		cm_print_error(
+		    "Unexpected number of elements in root %zu != %d\n",
+		    json_object_size(json.root),
+		    3);
+		_fail(file, line);
+	}
+
+	/*
+	 * Check the type element
+	 */
+	v = json_object_get(json.root, "type");
+	if (v == NULL) {
+		cm_print_error( "No \"type\" element\n");
+		_fail(file, line);
+	}
+
+	value = json_string_value(v);
+	if (strncmp("groupChange", value, strlen("groupChange") != 0)) {
+		cm_print_error(
+		    "Unexpected type \"%s\" != \"groupChange\"\n",
+		    value);
+		_fail(file, line);
+	}
+
+
+	audit = json_object_get(json.root, "groupChange");
+	if (audit == NULL) {
+		cm_print_error("No groupChange element\n");
+		_fail(file, line);
+	}
+
+	/*
+	 * Validate the groupChange element
+	 */
+	if (json_object_size(audit) != 10) {
+		cm_print_error(
+		    "Unexpected number of elements in groupChange "
+		    "%zu != %d\n",
+		    json_object_size(audit),
+		    10);
+		_fail(file, line);
+	}
+	/*
+	 * Validate the user element
+	 */
+	v = json_object_get(audit, "user");
+	if (v == NULL) {
+		cm_print_error( "No user element\n");
+		_fail(file, line);
+	}
+
+	value = json_string_value(v);
+	if (strncmp(user, value, strlen(user) != 0)) {
+		cm_print_error(
+		    "Unexpected user name \"%s\" != \"%s\"\n",
+		    value,
+		    user);
+		_fail(file, line);
+	}
+
+	/*
+	 * Validate the action element
+	 */
+	v = json_object_get(audit, "action");
+	if (v == NULL) {
+		cm_print_error( "No action element\n");
+		_fail(file, line);
+	}
+
+	value = json_string_value(v);
+	if (strncmp(action, value, strlen(action) != 0)) {
+		print_error(
+		    "Unexpected action \"%s\" != \"%s\"\n",
+		    value,
+		    action);
+		_fail(file, line);
+	}
+}
+
 /*
  * Test helper to check ISO 8601 timestamps for validity
  */
@@ -621,6 +757,128 @@ static void test_audit_group_json(void **state)
 
 }
 
+static void setup_ldb(
+	TALLOC_CTX *ctx,
+	struct ldb_context **ldb,
+	struct ldb_module **module,
+	const char *ip,
+	const char *session,
+	const char *sid)
+{
+	struct tsocket_address *ts = NULL;
+	struct audit_context *context = NULL;
+
+	*ldb = ldb_init(ctx, NULL);
+	ldb_register_samba_handlers(*ldb);
+
+
+	*module = talloc_zero(ctx, struct ldb_module);
+	(*module)->ldb = *ldb;
+
+	context = talloc_zero(*module, struct audit_context);
+	context->send_events = true;
+	context->msg_ctx = (struct imessaging_context *) 0x01;
+
+	ldb_module_set_private(*module, context);
+
+	tsocket_address_inet_from_strings(ctx, "ip", "127.0.0.1", 0, &ts);
+	ldb_set_opaque(*ldb, "remoteAddress", ts);
+
+	add_session_data(ctx, *ldb, session, sid);
+
+}
+
+
+/*
+ * Test the removal of a user from a group.
+ *
+ * The new element contains one group member
+ * The old element contains two group member
+ *
+ * Expect to see the removed entry logged.
+ *
+ * This test confirms bug 13664
+ * https://bugzilla.samba.org/show_bug.cgi?id=13664
+ */
+static void test_log_membership_changes_removed(void **state)
+{
+	struct ldb_context *ldb = NULL;
+	struct ldb_module  *module = NULL;
+
+
+	const char *const SID = "S-1-5-21-2470180966-3899876309-2637894779";
+	const char * const SESSION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+	const char *const TRANSACTION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+	const char *const IP = "127.0.0.1";
+
+	struct ldb_request *req = NULL;
+	struct ldb_message_element *new_el = NULL;
+	struct ldb_message_element *old_el = NULL;
+
+	int status = 0;
+
+	TALLOC_CTX *ctx = talloc_new(NULL);
+
+	setup_ldb(ctx, &ldb, &module, IP, SESSION, SID);
+
+	/*
+	 * Build the ldb_request
+	 */
+	req = talloc_zero(ctx, struct ldb_request);
+	req->operation =  LDB_ADD;
+	add_transaction_id(req, TRANSACTION);
+
+	/*
+	 * Populate the new elements, containing one entry.
+	 * Indicating that one element has been removed
+	 */
+	new_el = talloc_zero(ctx, struct ldb_message_element);
+	new_el->num_values = 1;
+	new_el->values = talloc_zero_array(ctx, DATA_BLOB, 1);
+	new_el->values[0] = data_blob_string_const(
+		"<GUID=081519b5-a709-44a0-bc95-dd4bfe809bf8>;"
+                "CN=testuser131953,CN=Users,DC=addom,DC=samba,"
+		"DC=example,DC=com");
+
+
+	/*
+	 * Populate the old elements, with two elements
+	 * The first is the same as the one in new elements.
+	*/
+	old_el = talloc_zero(ctx, struct ldb_message_element);
+	old_el->num_values = 2;
+	old_el->values = talloc_zero_array(ctx, DATA_BLOB, 2);
+	old_el->values[0] = data_blob_string_const(
+		"<GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681b>;"
+		"cn=grpadttstuser01,cn=users,DC=addom,"
+		"DC=samba,DC=example,DC=com");
+	old_el->values[1] = data_blob_string_const(
+		"<GUID=081519b5-a709-44a0-bc95-dd4bfe809bf8>;"
+                "CN=testuser131953,CN=Users,DC=addom,DC=samba,"
+		"DC=example,DC=com");
+
+	/*
+	 * call log_membership_changes
+	 */
+	messages_sent = 0;
+	log_membership_changes(module, req, new_el, old_el, status);
+
+	/*
+	 * Check the results
+	 */
+	assert_int_equal(1, messages_sent);
+
+	check_group_change_message(
+	    0,
+	    "cn=grpadttstuser01,cn=users,DC=addom,DC=samba,DC=example,DC=com",
+	    "Removed");
+
+	/*
+	 * Clean up
+	 */
+	json_free(&messages[0]);
+	TALLOC_FREE(ctx);
+}
 /*
  * Note: to run under valgrind us:
  *       valgrind --suppressions=test_group_audit.valgrind bin/test_group_audit
@@ -636,7 +894,7 @@ int main(void) {
 		cmocka_unit_test(test_get_parsed_dns),
 		cmocka_unit_test(test_dn_compare),
 		cmocka_unit_test(test_get_primary_group_dn),
-
+		cmocka_unit_test(test_log_membership_changes_removed),
 	};
 
 	cmocka_set_message_output(CM_OUTPUT_SUBUNIT);
-- 
2.17.1


From f45ef0739b5215f686efc0f5d7ec59db651cb8bc Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Thu, 25 Oct 2018 10:52:27 +1300
Subject: [PATCH 2/5] dsdb group audit: align dn_compare with memcmp

Rename the parameter names and adjust the  return codes from dn_compare
so that:
dn_compare(a, b) =>

LESS_THAN means a is less than b.
GREATER_THAN means a is greater than b.

Thanks to metze for suggesting the correct semantics for dn_compare

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13664

Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
 selftest/knownfail.d/bug13664                 |  1 -
 source4/dsdb/samdb/ldb_modules/group_audit.c  | 31 ++++++++++---------
 .../ldb_modules/tests/test_group_audit.c      |  4 +--
 3 files changed, 18 insertions(+), 18 deletions(-)
 delete mode 100644 selftest/knownfail.d/bug13664

diff --git a/selftest/knownfail.d/bug13664 b/selftest/knownfail.d/bug13664
deleted file mode 100644
index 6ffbf1297c3..00000000000
--- a/selftest/knownfail.d/bug13664
+++ /dev/null
@@ -1 +0,0 @@
-^samba4.dsdb.samdb.ldb_modules.group_audit.test_log_membership_changes_removed
diff --git a/source4/dsdb/samdb/ldb_modules/group_audit.c b/source4/dsdb/samdb/ldb_modules/group_audit.c
index 1c74805eed0..47b69435195 100644
--- a/source4/dsdb/samdb/ldb_modules/group_audit.c
+++ b/source4/dsdb/samdb/ldb_modules/group_audit.c
@@ -311,35 +311,36 @@ enum dn_compare_result {
 	GREATER_THAN
 };
 /*
- * @brief compare parsed_dns
+ * @brief compare parsed_dn, using GUID ordering
  *
- * Compare two parsed_dn structures, parsing the entries if necessary.
+ * Compare two parsed_dn structures, using GUID ordering.
  * To avoid the overhead of parsing the DN's this function does a binary
- * compare first. Only parsing the DN's they are not equal at a binary level.
+ * compare first. The DN's tre only parsed if they are not equal at a binary
+ * level.
  *
  * @param ctx talloc context that will own the parsed dsdb_dn
  * @param ldb ldb_context
- * @param old_val The old value
- * @param new_val The old value
+ * @param dn1 The first dn
+ * @param dn2 The second dn
  *
  * @return BINARY_EQUAL values are equal at a binary level
  *         EQUAL        DN's are equal but the meta data is different
- *         LESS_THAN    old value < new value
- *         GREATER_THAN old value > new value
+ *         LESS_THAN    dn1's GUID is less than dn2's GUID
+ *         GREATER_THAN dn1's GUID is greater than  dn2's GUID
  *
  */
 static enum dn_compare_result dn_compare(
 	TALLOC_CTX *mem_ctx,
 	struct ldb_context *ldb,
-	struct parsed_dn *old_val,
-	struct parsed_dn *new_val) {
+	struct parsed_dn *dn1,
+	struct parsed_dn *dn2) {
 
 	int res = 0;
 
 	/*
 	 * Do a binary compare first to avoid unnecessary parsing
 	 */
-	if (data_blob_cmp(new_val->v, old_val->v) == 0) {
+	if (data_blob_cmp(dn1->v, dn2->v) == 0) {
 		/*
 		 * Values are equal at a binary level so no need
 		 * for further processing
@@ -351,22 +352,22 @@ static enum dn_compare_result dn_compare(
 	 * do a GUID ordering compare. To do this we will need to ensure
 	 * that the dn's have been parsed.
 	 */
-	if (old_val->dsdb_dn == NULL) {
+	if (dn1->dsdb_dn == NULL) {
 		really_parse_trusted_dn(
 			mem_ctx,
 			ldb,
-			old_val,
+			dn1,
 			LDB_SYNTAX_DN);
 	}
-	if (new_val->dsdb_dn == NULL) {
+	if (dn2->dsdb_dn == NULL) {
 		really_parse_trusted_dn(
 			mem_ctx,
 			ldb,
-			new_val,
+			dn2,
 			LDB_SYNTAX_DN);
 	}
 
-	res = ndr_guid_compare(&new_val->guid, &old_val->guid);
+	res = ndr_guid_compare(&dn1->guid, &dn2->guid);
 	if (res < 0) {
 		return LESS_THAN;
 	} else if (res == 0) {
diff --git a/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c b/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
index 4c406c5fdfa..9133666e9cd 100644
--- a/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
+++ b/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
@@ -562,7 +562,7 @@ static void test_dn_compare(void **state)
 	b->v = &bb;
 
 	res = dn_compare(ctx, ldb, a, b);
-	assert_int_equal(GREATER_THAN, res);
+	assert_int_equal(LESS_THAN, res);
 	/*
 	 * DN's should have been parsed
 	 */
@@ -590,7 +590,7 @@ static void test_dn_compare(void **state)
 	b->v = &bb;
 
 	res = dn_compare(ctx, ldb, a, b);
-	assert_int_equal(LESS_THAN, res);
+	assert_int_equal(GREATER_THAN, res);
 	/*
 	 * DN's should have been parsed
 	 */
-- 
2.17.1


From 2fe453e291c161400f69ca82529ce585f1d5e645 Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Thu, 25 Oct 2018 13:28:09 +1300
Subject: [PATCH 3/5] dsdb group audit tests: check_timestamp improve
 diagnostics

Change check_timestamp to display the expected, actual along with the
line and name of the failing test, rather than the line in
check_timestamp.

Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
 .../ldb_modules/tests/test_group_audit.c      | 36 ++++++++++++++++---
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c b/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
index 9133666e9cd..ea7067db800 100644
--- a/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
+++ b/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
@@ -199,10 +199,16 @@ static void _check_group_change_message(
 	}
 }
 
+#define check_timestamp(b, t)\
+	_check_timestamp(b, t, __FILE__, __LINE__);
 /*
  * Test helper to check ISO 8601 timestamps for validity
  */
-static void check_timestamp(time_t before, const char *timestamp)
+static void _check_timestamp(
+	time_t before,
+	const char *timestamp,
+	const char *file,
+	const int line)
 {
 	int rc;
 	int usec, tz;
@@ -238,10 +244,32 @@ static void check_timestamp(time_t before, const char *timestamp)
 	actual = mktime(&tm);
 
 	/*
-	 * The timestamp should be before <= actual <= after
+	 * The time stamp should be before <= actual <= after
 	 */
-	assert_true(difftime(actual, before) >= 0);
-	assert_true(difftime(after, actual) >= 0);
+	if (difftime(actual, before) < 0) {
+		char buffer[40];
+		strftime(buffer,
+			 sizeof(buffer)-1,
+			 "%Y-%m-%dT%T",
+			 localtime(&before));
+		cm_print_error(
+		    "time stamp \"%s\" is before start time \"%s\"\n",
+		    timestamp,
+		    buffer);
+		_fail(file, line);
+	}
+	if (difftime(after, actual) < 0) {
+		char buffer[40];
+		strftime(buffer,
+			 sizeof(buffer)-1,
+			 "%Y-%m-%dT%T",
+			 localtime(&after));
+		cm_print_error(
+		    "time stamp \"%s\" is after finish time \"%s\"\n",
+		    timestamp,
+		    buffer);
+		_fail(file, line);
+	}
 }
 
 /*
-- 
2.17.1


From b5f21ce85be36f7916e5c274f3c0352caa0b7124 Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Thu, 25 Oct 2018 14:38:31 +1300
Subject: [PATCH 4/5] dsdb group audit tests: check_version improve diagnostics

Change check_version to display the expected, actual along with the
line and name of the failing test, rather than the line in check_version

Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
 .../ldb_modules/tests/test_group_audit.c      | 60 ++++++++++++++++---
 1 file changed, 53 insertions(+), 7 deletions(-)

diff --git a/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c b/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
index ea7067db800..ebb3b4bb8c7 100644
--- a/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
+++ b/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
@@ -272,23 +272,69 @@ static void _check_timestamp(
 	}
 }
 
+#define check_version(v, m, n)\
+	_check_version(v, m, n, __FILE__, __LINE__);
 /*
  * Test helper to validate a version object.
  */
-static void check_version(struct json_t *version, int major, int minor)
+static void _check_version(
+	struct json_t *version,
+	int major,
+	int minor,
+	const char* file,
+	const int line)
 {
 	struct json_t *v = NULL;
+	int value;
+
+	if (!json_is_object(version)) {
+		cm_print_error("version is not a JSON object\n");
+		_fail(file, line);
+	}
 
-	assert_true(json_is_object(version));
-	assert_int_equal(2, json_object_size(version));
+	if (json_object_size(version) != 2) {
+		cm_print_error(
+		    "Unexpected number of elements in version %zu != %d\n",
+		    json_object_size(version),
+		    2);
+		_fail(file, line);
+	}
 
+	/*
+	 * Validate the major version number element
+	 */
 	v = json_object_get(version, "major");
-	assert_non_null(v);
-	assert_int_equal(major, json_integer_value(v));
+	if (v == NULL) {
+		cm_print_error( "No major element\n");
+		_fail(file, line);
+	}
 
+	value = json_integer_value(v);
+	if (value != major) {
+		print_error(
+		    "Unexpected major version number \"%d\" != \"%d\"\n",
+		    value,
+		    major);
+		_fail(file, line);
+	}
+
+	/*
+	 * Validate the minor version number element
+	 */
 	v = json_object_get(version, "minor");
-	assert_non_null(v);
-	assert_int_equal(minor, json_integer_value(v));
+	if (v == NULL) {
+		cm_print_error( "No minor element\n");
+		_fail(file, line);
+	}
+
+	value = json_integer_value(v);
+	if (value != minor) {
+		print_error(
+		    "Unexpected minor version number \"%d\" != \"%d\"\n",
+		    value,
+		    minor);
+		_fail(file, line);
+	}
 }
 
 /*
-- 
2.17.1


From 04830e110cdcc7ebf32c39af1c503a7428d8a748 Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Thu, 25 Oct 2018 10:52:55 +1300
Subject: [PATCH 5/5] dsdb group audit tests: log_membership_changes extra
 tests

Add extra tests to ensure better test coverage of log_membership_changes

Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
 .../ldb_modules/tests/test_group_audit.c      | 384 +++++++++++++++++-
 1 file changed, 382 insertions(+), 2 deletions(-)

diff --git a/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c b/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
index ebb3b4bb8c7..8a03836fe80 100644
--- a/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
+++ b/source4/dsdb/samdb/ldb_modules/tests/test_group_audit.c
@@ -914,11 +914,10 @@ static void test_log_membership_changes_removed(void **state)
                 "CN=testuser131953,CN=Users,DC=addom,DC=samba,"
 		"DC=example,DC=com");
 
-
 	/*
 	 * Populate the old elements, with two elements
 	 * The first is the same as the one in new elements.
-	*/
+	 */
 	old_el = talloc_zero(ctx, struct ldb_message_element);
 	old_el->num_values = 2;
 	old_el->values = talloc_zero_array(ctx, DATA_BLOB, 2);
@@ -953,6 +952,383 @@ static void test_log_membership_changes_removed(void **state)
 	json_free(&messages[0]);
 	TALLOC_FREE(ctx);
 }
+
+/* test log_membership_changes
+ *
+ * old contains 2 user dn's
+ * new contains 0 user dn's
+ *
+ * Expect to see both dn's logged as deleted.
+ */
+static void test_log_membership_changes_remove_all(void **state)
+{
+	struct ldb_context *ldb = NULL;
+	struct ldb_module  *module = NULL;
+
+	const char *const SID = "S-1-5-21-2470180966-3899876309-2637894779";
+	const char * const SESSION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+	const char *const TRANSACTION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+	const char *const IP = "127.0.0.1";
+
+	struct ldb_request *req = NULL;
+	struct ldb_message_element *new_el = NULL;
+	struct ldb_message_element *old_el = NULL;
+
+	int status = 0;
+
+	TALLOC_CTX *ctx = talloc_new(NULL);
+
+	setup_ldb(ctx, &ldb, &module, IP, SESSION, SID);
+
+	/*
+	 * Build the ldb_request
+	 */
+	req = talloc_zero(ctx, struct ldb_request);
+	req->operation =  LDB_ADD;
+	add_transaction_id(req, TRANSACTION);
+
+	/*
+	 * Populate the new elements, containing no entries.
+	 * Indicating that all elements have been removed
+	 */
+	new_el = talloc_zero(ctx, struct ldb_message_element);
+	new_el->num_values = 0;
+	new_el->values = NULL;
+
+	/*
+	 * Populate the old elements, with two elements
+	 */
+	old_el = talloc_zero(ctx, struct ldb_message_element);
+	old_el->num_values = 2;
+	old_el->values = talloc_zero_array(ctx, DATA_BLOB, 2);
+	old_el->values[0] = data_blob_string_const(
+		"<GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681b>;"
+		"cn=grpadttstuser01,cn=users,DC=addom,"
+		"DC=samba,DC=example,DC=com");
+	old_el->values[1] = data_blob_string_const(
+		"<GUID=081519b5-a709-44a0-bc95-dd4bfe809bf8>;"
+                "CN=testuser131953,CN=Users,DC=addom,DC=samba,"
+		"DC=example,DC=com");
+
+	/*
+	 * call log_membership_changes
+	 */
+	messages_sent = 0;
+	log_membership_changes( module, req, new_el, old_el, status);
+
+	/*
+	 * Check the results
+	 */
+	assert_int_equal(2, messages_sent);
+
+	check_group_change_message(
+	    0,
+	    "cn=grpadttstuser01,cn=users,DC=addom,DC=samba,DC=example,DC=com",
+	    "Removed");
+
+	check_group_change_message(
+	    1,
+            "CN=testuser131953,CN=Users,DC=addom,DC=samba,DC=example,DC=com",
+	    "Removed");
+
+	/*
+	 * Clean up
+	 */
+	json_free(&messages[0]);
+	json_free(&messages[1]);
+	TALLOC_FREE(ctx);
+}
+
+/* test log_membership_changes
+ *
+ * Add an entry.
+ *
+ * Old entries contains a single user dn
+ * New entries contains 2 user dn's, one matching the dn in old entries
+ *
+ * Should see a single new entry logged.
+ */
+static void test_log_membership_changes_added(void **state)
+{
+	struct ldb_context *ldb = NULL;
+	struct ldb_module  *module = NULL;
+
+	const char *const SID = "S-1-5-21-2470180966-3899876309-2637894779";
+	const char * const SESSION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+	const char *const TRANSACTION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+	const char *const IP = "127.0.0.1";
+
+	struct ldb_request *req = NULL;
+	struct ldb_message_element *new_el = NULL;
+	struct ldb_message_element *old_el = NULL;
+
+	int status = 0;
+
+	TALLOC_CTX *ctx = talloc_new(NULL);
+
+	setup_ldb(ctx, &ldb, &module, IP, SESSION, SID);
+
+	/*
+	 * Build the ldb_request
+	 */
+	req = talloc_zero(ctx, struct ldb_request);
+	req->operation =  LDB_ADD;
+	add_transaction_id(req, TRANSACTION);
+
+	/*
+	 * Populate the old elements adding a single entry.
+	 */
+	old_el = talloc_zero(ctx, struct ldb_message_element);
+	old_el->num_values = 1;
+	old_el->values = talloc_zero_array(ctx, DATA_BLOB, 1);
+	old_el->values[0] = data_blob_string_const(
+		"<GUID=081519b5-a709-44a0-bc95-dd4bfe809bf8>;"
+                "CN=testuser131953,CN=Users,DC=addom,DC=samba,"
+		"DC=example,DC=com");
+
+	/*
+	 * Populate the new elements adding two entries. One matches the entry
+	 * in old elements. We expect to see the other element logged as Added
+	 */
+	new_el = talloc_zero(ctx, struct ldb_message_element);
+	new_el->num_values = 2;
+	new_el->values = talloc_zero_array(ctx, DATA_BLOB, 2);
+	new_el->values[0] = data_blob_string_const(
+		"<GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681b>;"
+		"cn=grpadttstuser01,cn=users,DC=addom,"
+		"DC=samba,DC=example,DC=com");
+	new_el->values[1] = data_blob_string_const(
+		"<GUID=081519b5-a709-44a0-bc95-dd4bfe809bf8>;"
+                "CN=testuser131953,CN=Users,DC=addom,DC=samba,"
+		"DC=example,DC=com");
+
+
+	/*
+	 * call log_membership_changes
+	 */
+	messages_sent = 0;
+	log_membership_changes( module, req, new_el, old_el, status);
+
+	/*
+	 * Check the results
+	 */
+	assert_int_equal(1, messages_sent);
+
+	check_group_change_message(
+	    0,
+	    "cn=grpadttstuser01,cn=users,DC=addom,DC=samba,DC=example,DC=com",
+	    "Added");
+
+	/*
+	 * Clean up
+	 */
+	json_free(&messages[0]);
+	TALLOC_FREE(ctx);
+}
+
+/*
+ * test log_membership_changes.
+ *
+ * Old entries is empty
+ * New entries contains 2 user dn's
+ *
+ * Expect to see log messages for two added users
+ */
+static void test_log_membership_changes_add_to_empty(void **state)
+{
+	struct ldb_context *ldb = NULL;
+	struct ldb_module  *module = NULL;
+
+	const char *const SID = "S-1-5-21-2470180966-3899876309-2637894779";
+	const char * const SESSION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+	const char *const TRANSACTION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+	const char *const IP = "127.0.0.1";
+
+	struct ldb_request *req = NULL;
+	struct ldb_message_element *new_el = NULL;
+	struct ldb_message_element *old_el = NULL;
+
+	int status = 0;
+
+	TALLOC_CTX *ctx = talloc_new(NULL);
+
+	/*
+	 * Set up the ldb and module structures
+	 */
+	setup_ldb(ctx, &ldb, &module, IP, SESSION, SID);
+
+	/*
+	 * Build the request structure
+	 */
+	req = talloc_zero(ctx, struct ldb_request);
+	req->operation =  LDB_ADD;
+	add_transaction_id(req, TRANSACTION);
+
+	/*
+	 * Build the element containing the old values
+	 */
+	old_el = talloc_zero(ctx, struct ldb_message_element);
+	old_el->num_values = 0;
+	old_el->values = NULL;
+
+	/*
+	 * Build the element containing the new values
+	 */
+	new_el = talloc_zero(ctx, struct ldb_message_element);
+	new_el->num_values = 2;
+	new_el->values = talloc_zero_array(ctx, DATA_BLOB, 2);
+	new_el->values[0] = data_blob_string_const(
+		"<GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681b>;"
+		"cn=grpadttstuser01,cn=users,DC=addom,"
+		"DC=samba,DC=example,DC=com");
+	new_el->values[1] = data_blob_string_const(
+		"<GUID=081519b5-a709-44a0-bc95-dd4bfe809bf8>;"
+                "CN=testuser131953,CN=Users,DC=addom,DC=samba,"
+		"DC=example,DC=com");
+
+	/*
+	 * Run log membership changes
+	 */
+	messages_sent = 0;
+	log_membership_changes( module, req, new_el, old_el, status);
+	assert_int_equal(2, messages_sent);
+
+	check_group_change_message(
+	    0,
+	    "cn=grpadttstuser01,cn=users,DC=addom,DC=samba,DC=example,DC=com",
+	    "Added");
+
+	check_group_change_message(
+	    1,
+            "CN=testuser131953,CN=Users,DC=addom,DC=samba,DC=example,DC=com",
+	    "Added");
+
+	json_free(&messages[0]);
+	json_free(&messages[1]);
+	TALLOC_FREE(ctx);
+}
+
+/* test log_membership_changes
+ *
+ * Test Replication Meta Data flag handling.
+ *
+ * 4 entries in old and new entries with their RMD_FLAGS set as below:
+ *    old   new
+ * 1)  0     0    Not logged
+ * 2)  1     1    Both deleted, no change not logged
+ * 3)  0     1    New tagged as deleted, log as deleted
+ * 4)  1     0    Has been undeleted, log as an add
+ *
+ * Should see a single new entry logged.
+ */
+static void test_log_membership_changes_rmd_flags(void **state)
+{
+	struct ldb_context *ldb = NULL;
+	struct ldb_module  *module = NULL;
+
+	const char *const SID = "S-1-5-21-2470180966-3899876309-2637894779";
+	const char * const SESSION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+	const char *const TRANSACTION = "7130cb06-2062-6a1b-409e-3514c26b1773";
+	const char *const IP = "127.0.0.1";
+
+	struct ldb_request *req = NULL;
+	struct ldb_message_element *new_el = NULL;
+	struct ldb_message_element *old_el = NULL;
+
+	int status = 0;
+
+	TALLOC_CTX *ctx = talloc_new(NULL);
+
+	setup_ldb(ctx, &ldb, &module, IP, SESSION, SID);
+
+	/*
+	 * Build the ldb_request
+	 */
+	req = talloc_zero(ctx, struct ldb_request);
+	req->operation =  LDB_ADD;
+	add_transaction_id(req, TRANSACTION);
+
+	/*
+	 * Populate the old elements.
+	 */
+	old_el = talloc_zero(ctx, struct ldb_message_element);
+	old_el->num_values = 4;
+	old_el->values = talloc_zero_array(ctx, DATA_BLOB, 4);
+	old_el->values[0] = data_blob_string_const(
+		"<GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681b>;"
+		"<RMD_FLAGS=0>;"
+		"cn=grpadttstuser01,cn=users,DC=addom,"
+		"DC=samba,DC=example,DC=com");
+	old_el->values[1] = data_blob_string_const(
+		"<GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681c>;"
+		"<RMD_FLAGS=1>;"
+		"cn=grpadttstuser02,cn=users,DC=addom,"
+		"DC=samba,DC=example,DC=com");
+	old_el->values[2] = data_blob_string_const(
+		"<GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681d>;"
+		"<RMD_FLAGS=0>;"
+		"cn=grpadttstuser03,cn=users,DC=addom,"
+		"DC=samba,DC=example,DC=com");
+	old_el->values[3] = data_blob_string_const(
+		"<GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681e>;"
+		"<RMD_FLAGS=1>;"
+		"cn=grpadttstuser04,cn=users,DC=addom,"
+		"DC=samba,DC=example,DC=com");
+
+	/*
+	 * Populate the new elements.
+	 */
+	new_el = talloc_zero(ctx, struct ldb_message_element);
+	new_el->num_values = 4;
+	new_el->values = talloc_zero_array(ctx, DATA_BLOB, 4);
+	new_el->values[0] = data_blob_string_const(
+		"<GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681b>;"
+		"<RMD_FLAGS=0>;"
+		"cn=grpadttstuser01,cn=users,DC=addom,"
+		"DC=samba,DC=example,DC=com");
+	new_el->values[1] = data_blob_string_const(
+		"<GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681c>;"
+		"<RMD_FLAGS=1>;"
+		"cn=grpadttstuser02,cn=users,DC=addom,"
+		"DC=samba,DC=example,DC=com");
+	new_el->values[2] = data_blob_string_const(
+		"<GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681d>;"
+		"<RMD_FLAGS=1>;"
+		"cn=grpadttstuser03,cn=users,DC=addom,"
+		"DC=samba,DC=example,DC=com");
+	new_el->values[3] = data_blob_string_const(
+		"<GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681e>;"
+		"<RMD_FLAGS=0>;"
+		"cn=grpadttstuser04,cn=users,DC=addom,"
+		"DC=samba,DC=example,DC=com");
+
+	/*
+	 * call log_membership_changes
+	 */
+	messages_sent = 0;
+	log_membership_changes( module, req, new_el, old_el, status);
+
+	/*
+	 * Check the results
+	 */
+	assert_int_equal(2, messages_sent);
+
+	check_group_change_message(
+	    0,
+	    "cn=grpadttstuser03,cn=users,DC=addom,DC=samba,DC=example,DC=com",
+	    "Removed");
+	check_group_change_message(
+	    1,
+	    "cn=grpadttstuser04,cn=users,DC=addom,DC=samba,DC=example,DC=com",
+	    "Added");
+
+	/*
+	 * Clean up
+	 */
+	json_free(&messages[0]);
+	json_free(&messages[1]);
+	TALLOC_FREE(ctx);
+}
 /*
  * Note: to run under valgrind us:
  *       valgrind --suppressions=test_group_audit.valgrind bin/test_group_audit
@@ -969,6 +1345,10 @@ int main(void) {
 		cmocka_unit_test(test_dn_compare),
 		cmocka_unit_test(test_get_primary_group_dn),
 		cmocka_unit_test(test_log_membership_changes_removed),
+		cmocka_unit_test(test_log_membership_changes_remove_all),
+		cmocka_unit_test(test_log_membership_changes_added),
+		cmocka_unit_test(test_log_membership_changes_add_to_empty),
+		cmocka_unit_test(test_log_membership_changes_rmd_flags),
 	};
 
 	cmocka_set_message_output(CM_OUTPUT_SUBUNIT);
-- 
2.17.1

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20181030/29eb994d/signature-0001.sig>


More information about the samba-technical mailing list