views: properly override group member names

Resolves https://fedorahosted.org/sssd/ticket/2948

5 files changed, 138 insertions, 133 deletions

diff --git a/src/db/sysdb.h b/src/db/sysdb.h
index 405f89e2f..a27552224 100644
--- a/src/db/sysdb.h
+++ b/src/db/sysdb.h
@@ -572,7 +572,8 @@ errno_t sysdb_add_overrides_to_object(struct sss_domain_info *domain,
                                       const char **req_attrs);
 
 errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain,
-                                         struct ldb_message *obj);
+                                         struct ldb_message *obj,
+                                         bool expect_override_dn);
 
 errno_t sysdb_getpwnam_with_views(TALLOC_CTX *mem_ctx,
                                   struct sss_domain_info *domain,
diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c
index e40b36c38..cfee5784d 100644
--- a/src/db/sysdb_search.c
+++ b/src/db/sysdb_search.c
@@ -771,28 +771,33 @@ int sysdb_getgrnam_with_views(TALLOC_CTX *mem_ctx,
 
     /* If there are views we have to check if override values must be added to
      * the original object. */
-    if (DOM_HAS_VIEWS(domain) && orig_obj->count == 1) {
-        if (!is_local_view(domain->view_name)) {
-            el = ldb_msg_find_element(orig_obj->msgs[0], SYSDB_GHOST);
-            if (el != NULL && el->num_values != 0) {
-                DEBUG(SSSDBG_TRACE_ALL, "Group object [%s], contains ghost "
-                      "entries which must be resolved before overrides can be "
-                      "applied.\n",
-                      ldb_dn_get_linearized(orig_obj->msgs[0]->dn));
-                ret = ENOENT;
-                goto done;
+    if (orig_obj->count == 1) {
+        if (DOM_HAS_VIEWS(domain)) {
+            if (!is_local_view(domain->view_name)) {
+                el = ldb_msg_find_element(orig_obj->msgs[0], SYSDB_GHOST);
+                if (el != NULL && el->num_values != 0) {
+                    DEBUG(SSSDBG_TRACE_ALL, "Group object [%s], contains ghost "
+                          "entries which must be resolved before overrides can be "
+                          "applied.\n",
+                          ldb_dn_get_linearized(orig_obj->msgs[0]->dn));
+                    ret = ENOENT;
+                    goto done;
+                }
             }
-        }
 
-        ret = sysdb_add_overrides_to_object(domain, orig_obj->msgs[0],
-                          override_obj == NULL ? NULL : override_obj ->msgs[0],
-                          NULL);
-        if (ret != EOK) {
-            DEBUG(SSSDBG_OP_FAILURE, "sysdb_add_overrides_to_object failed.\n");
-            goto done;
+            ret = sysdb_add_overrides_to_object(domain, orig_obj->msgs[0],
+                           override_obj == NULL ? NULL : override_obj ->msgs[0],
+                           NULL);
+            if (ret != EOK) {
+                DEBUG(SSSDBG_OP_FAILURE, "sysdb_add_overrides_to_object failed.\n");
+                goto done;
+            }
         }
 
-        ret = sysdb_add_group_member_overrides(domain, orig_obj->msgs[0]);
+        /* Must be called even without views to check to
+         * SYSDB_DEFAULT_OVERRIDE_NAME */
+        ret = sysdb_add_group_member_overrides(domain, orig_obj->msgs[0],
+                                               DOM_HAS_VIEWS(domain));
         if (ret != EOK) {
             DEBUG(SSSDBG_OP_FAILURE,
                   "sysdb_add_group_member_overrides failed.\n");
@@ -922,28 +927,33 @@ int sysdb_getgrgid_with_views(TALLOC_CTX *mem_ctx,
 
     /* If there are views we have to check if override values must be added to
      * the original object. */
-    if (DOM_HAS_VIEWS(domain) && orig_obj->count == 1) {
-        if (!is_local_view(domain->view_name)) {
-            el = ldb_msg_find_element(orig_obj->msgs[0], SYSDB_GHOST);
-            if (el != NULL && el->num_values != 0) {
-                DEBUG(SSSDBG_TRACE_ALL, "Group object [%s], contains ghost "
-                      "entries which must be resolved before overrides can be "
-                      "applied.\n",
-                      ldb_dn_get_linearized(orig_obj->msgs[0]->dn));
-                ret = ENOENT;
-                goto done;
+    if (orig_obj->count == 1) {
+        if (DOM_HAS_VIEWS(domain)) {
+            if (!is_local_view(domain->view_name)) {
+                el = ldb_msg_find_element(orig_obj->msgs[0], SYSDB_GHOST);
+                if (el != NULL && el->num_values != 0) {
+                    DEBUG(SSSDBG_TRACE_ALL, "Group object [%s], contains ghost "
+                          "entries which must be resolved before overrides can be "
+                          "applied.\n",
+                          ldb_dn_get_linearized(orig_obj->msgs[0]->dn));
+                    ret = ENOENT;
+                    goto done;
+                }
             }
-        }
 
-        ret = sysdb_add_overrides_to_object(domain, orig_obj->msgs[0],
-                          override_obj == NULL ? NULL : override_obj ->msgs[0],
-                          NULL);
-        if (ret != EOK) {
-            DEBUG(SSSDBG_OP_FAILURE, "sysdb_add_overrides_to_object failed.\n");
-            goto done;
+            ret = sysdb_add_overrides_to_object(domain, orig_obj->msgs[0],
+                              override_obj == NULL ? NULL : override_obj ->msgs[0],
+                              NULL);
+            if (ret != EOK) {
+                DEBUG(SSSDBG_OP_FAILURE, "sysdb_add_overrides_to_object failed.\n");
+                goto done;
+            }
         }
 
-        ret = sysdb_add_group_member_overrides(domain, orig_obj->msgs[0]);
+        /* Must be called even without views to check to
+         * SYSDB_DEFAULT_OVERRIDE_NAME */
+        ret = sysdb_add_group_member_overrides(domain, orig_obj->msgs[0],
+                                               DOM_HAS_VIEWS(domain));
         if (ret != EOK) {
             DEBUG(SSSDBG_OP_FAILURE,
                   "sysdb_add_group_member_overrides failed.\n");
@@ -1157,8 +1167,8 @@ int sysdb_enumgrent_filter_with_views(TALLOC_CTX *mem_ctx,
         goto done;
     }
 
-    if (DOM_HAS_VIEWS(domain)) {
-        for (c = 0; c < res->count; c++) {
+    for (c = 0; c < res->count; c++) {
+        if (DOM_HAS_VIEWS(domain)) {
             ret = sysdb_add_overrides_to_object(domain, res->msgs[c], NULL,
                                                 NULL);
             /* enumeration assumes that the cache is up-to-date, hence we do not
@@ -1167,13 +1177,14 @@ int sysdb_enumgrent_filter_with_views(TALLOC_CTX *mem_ctx,
                 DEBUG(SSSDBG_OP_FAILURE, "sysdb_add_overrides_to_object failed.\n");
                 goto done;
             }
+        }
 
-            ret = sysdb_add_group_member_overrides(domain, res->msgs[c]);
-            if (ret != EOK) {
-                DEBUG(SSSDBG_OP_FAILURE,
-                      "sysdb_add_group_member_overrides failed.\n");
-                goto done;
-            }
+        ret = sysdb_add_group_member_overrides(domain, res->msgs[c],
+                                               DOM_HAS_VIEWS(domain));
+        if (ret != EOK) {
+            DEBUG(SSSDBG_OP_FAILURE,
+                  "sysdb_add_group_member_overrides failed.\n");
+            goto done;
         }
     }
 
diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c
index 2b89e5ca4..79f513d13 100644
--- a/src/db/sysdb_views.c
+++ b/src/db/sysdb_views.c
@@ -1348,14 +1348,13 @@ done:
 }
 
 errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain,
-                                         struct ldb_message *obj)
+                                         struct ldb_message *obj,
+                                         bool expect_override_dn)
 {
     int ret;
     size_t c;
-    struct ldb_message_element *members;
+    struct ldb_result *res_members;
     TALLOC_CTX *tmp_ctx;
-    struct ldb_dn *member_dn;
-    struct ldb_result *member_obj;
     struct ldb_result *override_obj;
     static const char *member_attrs[] = SYSDB_PW_ATTRS;
     const char *override_dn_str;
@@ -1366,12 +1365,6 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain,
     char *val;
     struct sss_domain_info *orig_dom;
 
-    members = ldb_msg_find_element(obj, SYSDB_MEMBER);
-    if (members == NULL || members->num_values == 0) {
-        DEBUG(SSSDBG_TRACE_ALL, "Group has no members.\n");
-        return EOK;
-    }
-
     tmp_ctx = talloc_new(NULL);
     if (tmp_ctx == NULL) {
         DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");
@@ -1379,38 +1372,30 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain,
         goto done;
     }
 
-    for (c = 0; c < members->num_values; c++) {
-        member_dn = ldb_dn_from_ldb_val(tmp_ctx, domain->sysdb->ldb,
-                                        &members->values[c]);
-        if (member_dn == NULL) {
-            DEBUG(SSSDBG_OP_FAILURE, "ldb_dn_from_ldb_val failed.\n");
-            ret = ENOMEM;
-            goto done;
-        }
-
-        ret = ldb_search(domain->sysdb->ldb, member_dn, &member_obj, member_dn,
-                         LDB_SCOPE_BASE, member_attrs, NULL);
-        if (ret != LDB_SUCCESS) {
-            ret = sysdb_error_to_errno(ret);
-            goto done;
-        }
+    ret = sysdb_get_user_members_recursively(tmp_ctx, domain, obj->dn,
+                                             &res_members);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_OP_FAILURE,
+              "sysdb_get_user_members_recursively failed.\n");
+        goto done;
+    }
 
-        if (member_obj->count != 1) {
-            DEBUG(SSSDBG_CRIT_FAILURE,
-                  "Base search for member object returned [%d] results.\n",
-                  member_obj->count);
-            ret = EINVAL;
-            goto done;
-        }
+    for (c = 0; c < res_members->count; c++) {
 
-        if (ldb_msg_find_attr_as_uint64(member_obj->msgs[0],
+        if (ldb_msg_find_attr_as_uint64(res_members->msgs[c],
                                         SYSDB_UIDNUM, 0) == 0) {
             /* Skip non-POSIX-user members i.e. groups and non-POSIX users */
             continue;
         }
 
-        override_dn_str = ldb_msg_find_attr_as_string(member_obj->msgs[0],
-                                                      SYSDB_OVERRIDE_DN, NULL);
+        if (expect_override_dn) {
+            override_dn_str = ldb_msg_find_attr_as_string(res_members->msgs[c],
+                                                          SYSDB_OVERRIDE_DN,
+                                                          NULL);
+        } else {
+            override_dn_str = ldb_dn_get_linearized(res_members->msgs[c]->dn);
+        }
+
         if (override_dn_str == NULL) {
             if (is_local_view(domain->view_name)) {
                 /* LOCAL view doesn't have to have overrideDN specified. */
@@ -1420,12 +1405,12 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain,
 
             DEBUG(SSSDBG_CRIT_FAILURE,
                   "Missing override DN for object [%s].\n",
-                  ldb_dn_get_linearized(member_obj->msgs[0]->dn));
+                  ldb_dn_get_linearized(res_members->msgs[c]->dn));
             ret = ENOENT;
             goto done;
         }
 
-        override_dn = ldb_dn_new(member_obj, domain->sysdb->ldb,
+        override_dn = ldb_dn_new(res_members, domain->sysdb->ldb,
                                  override_dn_str);
         if (override_dn == NULL) {
             DEBUG(SSSDBG_OP_FAILURE, "ldb_dn_new failed.\n");
@@ -1433,22 +1418,27 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain,
             goto done;
         }
 
-        orig_name = ldb_msg_find_attr_as_string(member_obj->msgs[0],
+        orig_name = ldb_msg_find_attr_as_string(res_members->msgs[c],
                                                 SYSDB_NAME,
                                                 NULL);
         if (orig_name == NULL) {
             DEBUG(SSSDBG_CRIT_FAILURE, "Object [%s] has no name.\n",
-                  ldb_dn_get_linearized(member_obj->msgs[0]->dn));
+                  ldb_dn_get_linearized(res_members->msgs[c]->dn));
             ret = EINVAL;
             goto done;
         }
 
-        memberuid = NULL;
-        if (ldb_dn_compare(member_obj->msgs[0]->dn, override_dn) != 0) {
+        /* start with default view name, if it exists or use NULL */
+        memberuid = ldb_msg_find_attr_as_string(res_members->msgs[c],
+                                                SYSDB_DEFAULT_OVERRIDE_NAME,
+                                                NULL);
+
+        /* If there is an override object, check if the name is overridden */
+        if (ldb_dn_compare(res_members->msgs[c]->dn, override_dn) != 0) {
             DEBUG(SSSDBG_TRACE_ALL, "Checking override for object [%s].\n",
-                  ldb_dn_get_linearized(member_obj->msgs[0]->dn));
+                  ldb_dn_get_linearized(res_members->msgs[c]->dn));
 
-            ret = ldb_search(domain->sysdb->ldb, member_obj, &override_obj,
+            ret = ldb_search(domain->sysdb->ldb, res_members, &override_obj,
                              override_dn, LDB_SCOPE_BASE, member_attrs, NULL);
             if (ret != LDB_SUCCESS) {
                 ret = sysdb_error_to_errno(ret);
@@ -1458,43 +1448,44 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain,
             if (override_obj->count != 1) {
                 DEBUG(SSSDBG_CRIT_FAILURE,
                      "Base search for override object returned [%d] results.\n",
-                     member_obj->count);
+                    override_obj->count);
                 ret = EINVAL;
                 goto done;
             }
 
             memberuid = ldb_msg_find_attr_as_string(override_obj->msgs[0],
                                                     SYSDB_NAME,
-                                                    NULL);
+                                                    memberuid);
+        }
 
-            if (memberuid != NULL) {
-                ret = sss_parse_internal_fqname(tmp_ctx, orig_name,
-                                                NULL, &orig_domain);
-                if (ret != EOK) {
-                    DEBUG(SSSDBG_OP_FAILURE,
-                         "sss_parse_internal_fqname failed to split [%s].\n",
-                         orig_name);
+        /* add domain name if memberuid is a short name */
+        if (memberuid != NULL && strchr(memberuid, '@') == NULL) {
+            ret = sss_parse_internal_fqname(tmp_ctx, orig_name,
+                                            NULL, &orig_domain);
+            if (ret != EOK) {
+                DEBUG(SSSDBG_OP_FAILURE,
+                     "sss_parse_internal_fqname failed to split [%s].\n",
+                     orig_name);
+                goto done;
+            }
+
+            if (orig_domain != NULL) {
+                orig_dom = find_domain_by_name(get_domains_head(domain),
+                                               orig_domain, true);
+                if (orig_dom == NULL) {
+                    DEBUG(SSSDBG_CRIT_FAILURE,
+                          "Cannot find domain with name [%s].\n",
+                          orig_domain);
+                    ret = ERR_DOMAIN_NOT_FOUND;
                     goto done;
                 }
-
-                if (orig_domain != NULL) {
-                    orig_dom = find_domain_by_name(get_domains_head(domain),
-                                                   orig_domain, true);
-                    if (orig_dom == NULL) {
-                        DEBUG(SSSDBG_CRIT_FAILURE,
-                              "Cannot find domain with name [%s].\n",
-                              orig_domain);
-                        ret = ERR_DOMAIN_NOT_FOUND;
-                        goto done;
-                    }
-                    memberuid = sss_create_internal_fqname(tmp_ctx, memberuid,
-                                                           orig_dom->name);
-                    if (memberuid == NULL) {
-                        DEBUG(SSSDBG_OP_FAILURE,
-                              "sss_create_internal_fqname failed.\n");
-                        ret = ENOMEM;
-                        goto done;
-                    }
+                memberuid = sss_create_internal_fqname(tmp_ctx, memberuid,
+                                                       orig_dom->name);
+                if (memberuid == NULL) {
+                    DEBUG(SSSDBG_OP_FAILURE,
+                          "sss_create_internal_fqname failed.\n");
+                    ret = ENOMEM;
+                    goto done;
                 }
             }
         }
@@ -1521,9 +1512,6 @@ errno_t sysdb_add_group_member_overrides(struct sss_domain_info *domain,
         DEBUG(SSSDBG_TRACE_ALL, "Added [%s] to [%s].\n", memberuid,
                                 OVERRIDE_PREFIX SYSDB_MEMBERUID);
 
-        /* Free all temporary data of the current member to avoid memory usage
-         * spikes. All temporary data should be allocated below member_dn. */
-        talloc_free(member_dn);
     }
 
     ret = EOK;
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index 1ae179696..4e84b3202 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -2976,7 +2976,12 @@ static int fill_grent(struct sss_packet *packet,
 
         memnum = 0;
         if (!dom->ignore_group_members) {
-            el = sss_view_ldb_msg_find_element(dom, msg, SYSDB_MEMBERUID);
+            /* unconditionally prefer OVERRIDE_PREFIX SYSDB_MEMBERUID, it
+             * might contain override names from the default view */
+            el = ldb_msg_find_element(msg, OVERRIDE_PREFIX SYSDB_MEMBERUID);
+            if (el == NULL) {
+                el = ldb_msg_find_element(msg, SYSDB_MEMBERUID);
+            }
             if (el) {
                 ret = fill_members(packet, nctx->rctx, dom, nctx, el,
                                    &rzero, &rsize, &memnum);
diff --git a/src/tests/cmocka/test_nss_srv.c b/src/tests/cmocka/test_nss_srv.c
index 82a304fee..892bebe63 100644
--- a/src/tests/cmocka/test_nss_srv.c
+++ b/src/tests/cmocka/test_nss_srv.c
@@ -1342,8 +1342,8 @@ static int test_nss_getgrnam_members_check(uint32_t status,
     int ret;
     uint32_t nmem;
     struct group gr;
-    const char *exp_members[] = { testmember1.pw_name,
-                                  testmember2.pw_name };
+    const char *exp_members[] = { testmember2.pw_name,
+                                  testmember1.pw_name };
     struct group expected = {
         .gr_gid = testgroup_members.gr_gid,
         .gr_name = testgroup_members.gr_name,
@@ -1429,10 +1429,10 @@ static int test_nss_getgrnam_members_check_fqdn(uint32_t status,
     assert_non_null(tmp_ctx);
 
     exp_members[0] = sss_tc_fqname(tmp_ctx, nss_test_ctx->tctx->dom->names,
-                                   nss_test_ctx->tctx->dom, testmember1.pw_name);
+                                   nss_test_ctx->tctx->dom, testmember2.pw_name);
     assert_non_null(exp_members[0]);
     exp_members[1] = sss_tc_fqname(tmp_ctx, nss_test_ctx->tctx->dom->names,
-                                   nss_test_ctx->tctx->dom, testmember2.pw_name);
+                                   nss_test_ctx->tctx->dom, testmember1.pw_name);
     assert_non_null(exp_members[1]);
 
     expected.gr_name = sss_tc_fqname(tmp_ctx,
@@ -1619,8 +1619,8 @@ static int test_nss_getgrnam_check_mix_dom(uint32_t status,
     tmp_ctx = talloc_new(nss_test_ctx);
     assert_non_null(tmp_ctx);
 
-    exp_members[0] = testmember1.pw_name;
-    exp_members[1] = testmember2.pw_name;
+    exp_members[0] = testmember2.pw_name;
+    exp_members[1] = testmember1.pw_name;
     exp_members[2] = sss_tc_fqname(tmp_ctx, nss_test_ctx->subdom->names,
                                    nss_test_ctx->subdom, submember1.pw_name);
     assert_non_null(exp_members[2]);
@@ -1683,10 +1683,10 @@ static int test_nss_getgrnam_check_mix_dom_fqdn(uint32_t status,
     assert_non_null(tmp_ctx);
 
     exp_members[0] = sss_tc_fqname(tmp_ctx, nss_test_ctx->tctx->dom->names,
-                                   nss_test_ctx->tctx->dom, testmember1.pw_name);
+                                   nss_test_ctx->tctx->dom, testmember2.pw_name);
     assert_non_null(exp_members[0]);
     exp_members[1] = sss_tc_fqname(tmp_ctx, nss_test_ctx->tctx->dom->names,
-                                   nss_test_ctx->tctx->dom, testmember2.pw_name);
+                                   nss_test_ctx->tctx->dom, testmember1.pw_name);
     assert_non_null(exp_members[1]);
     exp_members[2] = sss_tc_fqname(tmp_ctx, nss_test_ctx->subdom->names,
                                    nss_test_ctx->subdom, submember1.pw_name);
@@ -1752,16 +1752,16 @@ static int test_nss_getgrnam_check_mix_subdom(uint32_t status,
     tmp_ctx = talloc_new(nss_test_ctx);
     assert_non_null(tmp_ctx);
 
-    exp_members[0] = sss_tc_fqname(tmp_ctx, nss_test_ctx->subdom->names,
-                                   nss_test_ctx->subdom, submember1.pw_name);
-    assert_non_null(exp_members[0]);
     exp_members[1] = sss_tc_fqname(tmp_ctx, nss_test_ctx->subdom->names,
-                                   nss_test_ctx->subdom, submember2.pw_name);
+                                   nss_test_ctx->subdom, submember1.pw_name);
     assert_non_null(exp_members[1]);
+    exp_members[2] = sss_tc_fqname(tmp_ctx, nss_test_ctx->subdom->names,
+                                   nss_test_ctx->subdom, submember2.pw_name);
+    assert_non_null(exp_members[2]);
     /* Important: this member is from a non-qualified domain, so his name will
      * not be qualified either
      */
-    exp_members[2] = testmember1.pw_name;
+    exp_members[0] = testmember1.pw_name;
 
     expected.gr_name = sss_tc_fqname(tmp_ctx, nss_test_ctx->subdom->names,
                                      nss_test_ctx->subdom, testsubdomgroup.gr_name);