r24052: Fix some of the NT4 usrmgr.exe portions of bug 4815.

 - The icons in usermgr were incorrect, because the acct_flags were
   not filled in (due to missing attribute in ldb query)

 - The Full name was missing, and the description used as the full
   name (due to missing attributes in ldb query and incorrect IDL)

To prove the correctness of these fixes, I added a substantial new
test to RPC-SAMR-USERS, to ensure cross-consistancy between
QueryDisplayInfo and QueryUserInfo on each user.

This showed that for some reason, we must add ACB_NORMAL to the
acct_flags on level 2 queries (for machine trust accounts)...

Getting this right is important, because Samba3's RPC winbind methods
uses these queries.

Andrew Bartlett
(This used to be commit 9475d94a61e36b3507e5fd2e6bb6f0667db4a607)

3 files changed, 209 insertions, 16 deletions

diff --git a/source4/librpc/idl/samr.idl b/source4/librpc/idl/samr.idl
index a9eee3d28a..479396d5c5 100644
--- a/source4/librpc/idl/samr.idl
+++ b/source4/librpc/idl/samr.idl
@@ -871,8 +871,8 @@ import "misc.idl", "lsa.idl", "security.idl";
 		uint32    rid;
 		samr_AcctFlags acct_flags;
 		lsa_String account_name;
-		lsa_String full_name;
 		lsa_String description;
+		lsa_String full_name;
 	} samr_DispEntryGeneral;
 
 	typedef struct {
diff --git a/source4/rpc_server/samr/dcesrv_samr.c b/source4/rpc_server/samr/dcesrv_samr.c
index b1b9e9288e..cedf4059e2 100644
--- a/source4/rpc_server/samr/dcesrv_samr.c
+++ b/source4/rpc_server/samr/dcesrv_samr.c
@@ -508,7 +508,7 @@ static NTSTATUS dcesrv_samr_info_DomInfo2(struct samr_domain_state *state, TALLO
 		break;
 	}
 
-	/* TODO: Should these filter on SID, to avoid counting BUILTIN? */
+	/* No users in BUILTIN, and the LOCAL group types are only in builtin, and the global group type is never in BUILTIN */
 	info->num_users = samdb_search_count(state->sam_ctx, mem_ctx, state->domain_dn, 
 					     "(objectClass=user)");
 	info->num_groups = samdb_search_count(state->sam_ctx, mem_ctx, state->domain_dn,
@@ -3573,8 +3573,8 @@ static NTSTATUS dcesrv_samr_QueryDisplayInfo(struct dcesrv_call_state *dce_call,
 	struct samr_domain_state *d_state;
 	struct ldb_message **res;
 	int ldb_cnt, count, i;
-	const char * const attrs[4] = { "objectSid", "sAMAccountName",
-					"description", NULL };
+	const char * const attrs[] = { "objectSid", "sAMAccountName", "displayName",
+					"description", "userAccountControl", NULL };
 	struct samr_DispEntryFull *entriesFull = NULL;
 	struct samr_DispEntryFullGroup *entriesFullGroup = NULL;
 	struct samr_DispEntryAscii *entriesAscii = NULL;
@@ -3674,12 +3674,21 @@ static NTSTATUS dcesrv_samr_QueryDisplayInfo(struct dcesrv_call_state *dce_call,
 				samdb_result_string(res[i], "description", "");
 			break;
 		case 2:
+			if (!(samdb_result_acct_flags(res[i], 
+						      "userAccountControl") & ACB_WSTRUST)) {
+				/* Domain controllers match the
+				 * filter, but should not be included
+				 * in the output */
+				continue;
+			}
 			entriesFull[count].idx = count + 1;
 			entriesFull[count].rid =
 				objectsid->sub_auths[objectsid->num_auths-1];
+
+			/* No idea why we need to or in ACB_NORMAL here, but this is what Win2k3 seems to do... */
 			entriesFull[count].acct_flags =
 				samdb_result_acct_flags(res[i], 
-							"userAccountControl");
+							"userAccountControl") | ACB_NORMAL;
 			entriesFull[count].account_name.string =
 				samdb_result_string(res[i], "sAMAccountName",
 						    "");
diff --git a/source4/torture/rpc/samr.c b/source4/torture/rpc/samr.c
index 7dad95d001..20a79a7d4f 100644
--- a/source4/torture/rpc/samr.c
+++ b/source4/torture/rpc/samr.c
@@ -3124,30 +3124,213 @@ static BOOL test_GetDisplayEnumerationIndex2(struct dcerpc_pipe *p, TALLOC_CTX *
 	return ret;	
 }
 
+#define STRING_EQUAL_QUERY(s1, s2, user)					\
+	if (s1.string == NULL && s2.string != NULL && s2.string[0] == '\0') { \
+		/* odd, but valid */						\
+	} else if ((s1.string && !s2.string) || (s2.string && !s1.string) || strcmp(s1.string, s2.string)) { \
+			printf("%s mismatch for %s: %s != %s (%s)\n", \
+			       #s1, user.string,  s1.string, s2.string, __location__);   \
+			ret = False; \
+	}
+#define INT_EQUAL_QUERY(s1, s2, user)		\
+		if (s1 != s2) { \
+			printf("%s mismatch for %s: 0x%x != 0x%x (%s)\n", \
+			       #s1, user.string, (unsigned int)s1, (unsigned int)s2, __location__); \
+			ret = False; \
+		}
+
+static BOOL test_each_DisplayInfo_user(struct dcerpc_pipe *p, TALLOC_CTX *mem_ctx, 
+				       struct samr_QueryDisplayInfo *querydisplayinfo,
+				       bool *seen_testuser) 
+{
+	struct samr_OpenUser r;
+	struct samr_QueryUserInfo q;
+	struct policy_handle user_handle;
+	int i, ret = True;
+	NTSTATUS status;
+	r.in.domain_handle = querydisplayinfo->in.domain_handle;
+	r.in.access_mask = SEC_FLAG_MAXIMUM_ALLOWED;
+	for (i = 0; ; i++) {
+		switch (querydisplayinfo->in.level) {
+		case 1:
+			if (i >= querydisplayinfo->out.info.info1.count) {
+				return ret;
+			}
+			r.in.rid = querydisplayinfo->out.info.info1.entries[i].rid;
+			break;
+		case 2:
+			if (i >= querydisplayinfo->out.info.info2.count) {
+				return ret;
+			}
+			r.in.rid = querydisplayinfo->out.info.info2.entries[i].rid;
+			break;
+		case 3:
+			/* Groups */
+		case 4:
+		case 5:
+			/* Not interested in validating just the account name */
+			return true;
+		}
+			
+		r.out.user_handle = &user_handle;
+		
+		switch (querydisplayinfo->in.level) {
+		case 1:
+		case 2:
+			status = dcerpc_samr_OpenUser(p, mem_ctx, &r);
+			if (!NT_STATUS_IS_OK(status)) {
+				printf("OpenUser(%u) failed - %s\n", r.in.rid, nt_errstr(status));
+				return False;
+			}
+		}
+		
+		q.in.user_handle = &user_handle;
+		q.in.level = 21;
+		status = dcerpc_samr_QueryUserInfo(p, mem_ctx, &q);
+		if (!NT_STATUS_IS_OK(status)) {
+			printf("QueryUserInfo(%u) failed - %s\n", r.in.rid, nt_errstr(status));
+			return False;
+		}
+		
+		switch (querydisplayinfo->in.level) {
+		case 1:
+			if (seen_testuser && strcmp(q.out.info->info21.account_name.string, TEST_ACCOUNT_NAME) == 0) {
+				*seen_testuser = true;
+			}
+			STRING_EQUAL_QUERY(querydisplayinfo->out.info.info1.entries[i].full_name, 
+					   q.out.info->info21.full_name, q.out.info->info21.account_name);
+			STRING_EQUAL_QUERY(querydisplayinfo->out.info.info1.entries[i].account_name, 
+					   q.out.info->info21.account_name, q.out.info->info21.account_name);
+			STRING_EQUAL_QUERY(querydisplayinfo->out.info.info1.entries[i].description, 
+					   q.out.info->info21.description, q.out.info->info21.account_name);
+			INT_EQUAL_QUERY(querydisplayinfo->out.info.info1.entries[i].rid, 
+					q.out.info->info21.rid, q.out.info->info21.account_name);
+			INT_EQUAL_QUERY(querydisplayinfo->out.info.info1.entries[i].acct_flags, 
+					q.out.info->info21.acct_flags, q.out.info->info21.account_name);
+			
+			break;
+		case 2:
+			STRING_EQUAL_QUERY(querydisplayinfo->out.info.info2.entries[i].account_name, 
+					   q.out.info->info21.account_name, q.out.info->info21.account_name);
+			STRING_EQUAL_QUERY(querydisplayinfo->out.info.info2.entries[i].description, 
+					   q.out.info->info21.description, q.out.info->info21.account_name);
+			INT_EQUAL_QUERY(querydisplayinfo->out.info.info2.entries[i].rid, 
+					q.out.info->info21.rid, q.out.info->info21.account_name);
+			INT_EQUAL_QUERY((querydisplayinfo->out.info.info2.entries[i].acct_flags & ~ACB_NORMAL), 
+					q.out.info->info21.acct_flags, q.out.info->info21.account_name);
+			
+			if (!(querydisplayinfo->out.info.info2.entries[i].acct_flags & ACB_NORMAL)) {
+				printf("Missing ACB_NORMAL in querydisplayinfo->out.info.info2.entries[i].acct_flags on %s\n", 
+				       q.out.info->info21.account_name.string);
+			}
+
+			if (!(q.out.info->info21.acct_flags & (ACB_WSTRUST))) {
+				printf("Found non-trust account %s in trust accoutn listing: 0x%x 0x%x\n",
+				       q.out.info->info21.account_name.string,
+				       querydisplayinfo->out.info.info2.entries[i].acct_flags,
+				       q.out.info->info21.acct_flags);
+				return False;
+			}
+			
+			break;
+		}
+		
+		if (!test_samr_handle_Close(p, mem_ctx, &user_handle)) {
+			return False;
+		}
+	}
+	return ret;
+}
+
 static BOOL test_QueryDisplayInfo(struct dcerpc_pipe *p, TALLOC_CTX *mem_ctx, 
 				  struct policy_handle *handle)
 {
 	NTSTATUS status;
 	struct samr_QueryDisplayInfo r;
+	struct samr_QueryDomainInfo dom_info;
 	BOOL ret = True;
 	uint16_t levels[] = {1, 2, 3, 4, 5};
 	int i;
+	bool seen_testuser = false;
 
 	for (i=0;i<ARRAY_SIZE(levels);i++) {
 		printf("Testing QueryDisplayInfo level %u\n", levels[i]);
 
-		r.in.domain_handle = handle;
-		r.in.level = levels[i];
 		r.in.start_idx = 0;
-		r.in.max_entries = 1000;
-		r.in.buf_size = (uint32_t)-1;
-
-		status = dcerpc_samr_QueryDisplayInfo(p, mem_ctx, &r);
+		status = STATUS_MORE_ENTRIES;
+		while (NT_STATUS_EQUAL(status, STATUS_MORE_ENTRIES)) {
+			r.in.domain_handle = handle;
+			r.in.level = levels[i];
+			r.in.max_entries = 2;
+			r.in.buf_size = (uint32_t)-1;
+			
+			status = dcerpc_samr_QueryDisplayInfo(p, mem_ctx, &r);
+			if (!NT_STATUS_EQUAL(status, STATUS_MORE_ENTRIES) && !NT_STATUS_IS_OK(status)) {
+				printf("QueryDisplayInfo level %u failed - %s\n", 
+				       levels[i], nt_errstr(status));
+				ret = False;
+			}
+			switch (r.in.level) {
+			case 1:
+				if (!test_each_DisplayInfo_user(p, mem_ctx, &r, &seen_testuser)) {
+					ret = False;
+				}
+				r.in.start_idx += r.out.info.info1.count;
+				break;
+			case 2:
+				if (!test_each_DisplayInfo_user(p, mem_ctx, &r, NULL)) {
+					ret = False;
+				}
+				r.in.start_idx += r.out.info.info2.count;
+				break;
+			case 3:
+				r.in.start_idx += r.out.info.info3.count;
+				break;
+			case 4:
+				r.in.start_idx += r.out.info.info4.count;
+				break;
+			case 5:
+				r.in.start_idx += r.out.info.info5.count;
+				break;
+			}
+		}
+		dom_info.in.domain_handle = handle;
+		dom_info.in.level = 2;
+		/* Check number of users returned is correct */
+		status = dcerpc_samr_QueryDomainInfo(p, mem_ctx, &dom_info);
 		if (!NT_STATUS_IS_OK(status)) {
-			printf("QueryDisplayInfo level %u failed - %s\n", 
-			       levels[i], nt_errstr(status));
-			ret = False;
+			printf("QueryDomainInfo level %u failed - %s\n", 
+			       r.in.level, nt_errstr(status));
+				ret = False;
+				break;
 		}
+		switch (r.in.level) {
+		case 1:
+		case 4:
+			if (dom_info.out.info->info2.num_users < r.in.start_idx) {
+				printf("QueryDomainInfo indicates that QueryDisplayInfo returned more users (%d/%d) than the domain %s is said to contain!\n",
+				       r.in.start_idx, dom_info.out.info->info2.num_groups,
+				       dom_info.out.info->info2.domain_name.string);
+				ret = False;
+			}
+			if (!seen_testuser) {
+				printf("Didn't find test user " TEST_ACCOUNT_NAME " in enumeration of %s\n", 
+				       dom_info.out.info->info2.domain_name.string);
+				ret = False;
+			}
+			break;
+		case 3:
+		case 5:
+			if (dom_info.out.info->info2.num_groups != r.in.start_idx) {
+				printf("QueryDomainInfo indicates that QueryDisplayInfo didn't return all (%d/%d) the groups in %s\n",
+				       r.in.start_idx, dom_info.out.info->info2.num_groups,
+				       dom_info.out.info->info2.domain_name.string);
+				ret = False;
+			}
+			
+			break;
+		}
+
 	}
 	
 	return ret;	
@@ -3811,8 +3994,10 @@ static BOOL test_OpenDomain(struct dcerpc_pipe *p, TALLOC_CTX *mem_ctx,
 	switch (which_ops) {
 	case TORTURE_SAMR_USER_ATTRIBUTES:
 	case TORTURE_SAMR_PASSWORDS:
-		ret &= test_CreateUser(p, mem_ctx, &domain_handle, NULL, which_ops);
 		ret &= test_CreateUser2(p, mem_ctx, &domain_handle, which_ops);
+		ret &= test_CreateUser(p, mem_ctx, &domain_handle, &user_handle, which_ops);
+		/* This test needs 'complex' users to validate */
+		ret &= test_QueryDisplayInfo(p, mem_ctx, &domain_handle);
 		break;
 	case TORTURE_SAMR_OTHER:
 		ret &= test_CreateUser(p, mem_ctx, &domain_handle, &user_handle, which_ops);
@@ -3826,7 +4011,6 @@ static BOOL test_OpenDomain(struct dcerpc_pipe *p, TALLOC_CTX *mem_ctx,
 		ret &= test_EnumDomainUsers_async(p, mem_ctx, &domain_handle);
 		ret &= test_EnumDomainGroups(p, mem_ctx, &domain_handle);
 		ret &= test_EnumDomainAliases(p, mem_ctx, &domain_handle);
-		ret &= test_QueryDisplayInfo(p, mem_ctx, &domain_handle);
 		ret &= test_QueryDisplayInfo2(p, mem_ctx, &domain_handle);
 		ret &= test_QueryDisplayInfo3(p, mem_ctx, &domain_handle);
 		ret &= test_QueryDisplayInfo_continue(p, mem_ctx, &domain_handle);