mzidek-rh's pull request #18: "[PATCHES] sss_user/groupmod fixes" was opened
PR body:
"""
Hi,
This PR is blocked by PR#16 (TOOLS: sss_groupshow did not work). It includes
fix for the ticket https://fedorahosted.org/sssd/ticket/3178.
Michal
"""
See the full pull-request at https://github.com/SSSD/sssd/pull/18
... or pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/18/head:pr18
git checkout pr18
From 0bcb9ce46d91129516ab0b1d4862856e0e2af88f Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Tue, 6 Sep 2016 12:13:08 +0200
Subject: [PATCH 1/7] TOOLS: Fix a typo in groupadd()
Resolves:
https://fedorahosted.org/sssd/ticket/3173
---
src/tools/sss_sync_ops.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/tools/sss_sync_ops.c b/src/tools/sss_sync_ops.c
index a23a0b8..39ef5be 100644
--- a/src/tools/sss_sync_ops.c
+++ b/src/tools/sss_sync_ops.c
@@ -657,7 +657,7 @@ int groupadd(struct ops_ctx *data)
int ret;
data->sysdb_fqname = sss_create_internal_fqname(data,
- data->sysdb_fqname,
+ data->name,
data->domain->name);
if (data->sysdb_fqname == NULL) {
return ENOMEM;
From 48449165abbbfb807dc928085cce4d6f50af96df Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com>
Date: Tue, 6 Sep 2016 13:46:53 +0200
Subject: [PATCH 2/7] TOOLS: sss_groupshow did not work
sss_groupshow used shortname to search
in sysdb database. We have to u e sysdb_fqname
(aka internal_fqname) format for all sysdb
oprations.
Resolves:
https://fedorahosted.org/sssd/ticket/3175
---
src/tools/sss_groupshow.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/tools/sss_groupshow.c b/src/tools/sss_groupshow.c
index 41d7475..5870cc8 100644
--- a/src/tools/sss_groupshow.c
+++ b/src/tools/sss_groupshow.c
@@ -318,7 +318,7 @@ int group_show(TALLOC_CTX *mem_ctx,
struct sysdb_ctx *sysdb,
struct sss_domain_info *domain,
bool recursive,
- const char *name,
+ const char *shortname,
struct group_info **res)
{
struct group_info *root;
@@ -326,11 +326,20 @@ int group_show(TALLOC_CTX *mem_ctx,
struct ldb_message *msg = NULL;
const char **group_members = NULL;
int nmembers = 0;
+ char *sysdb_fqname = NULL;
int ret;
int i;
+ sysdb_fqname = sss_create_internal_fqname(mem_ctx,
+ shortname,
+ domain->name);
+ if (sysdb_fqname == NULL) {
+ return ENOMEM;
+ }
+
/* First, search for the root group */
- ret = sysdb_search_group_by_name(mem_ctx, domain, name, attrs, &msg);
+ ret = sysdb_search_group_by_name(mem_ctx, domain, sysdb_fqname, attrs,
+ &msg);
if (ret) {
DEBUG(SSSDBG_OP_FAILURE,
"Search failed: %s (%d)\n", strerror(ret), ret);
From a71e0c521d3a1d8dee3dcfd41fa5f5c3decc26af Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com>
Date: Tue, 6 Sep 2016 17:37:14 +0200
Subject: [PATCH 3/7] TESTS: sss_groupadd/groupshow regressions
Adds regression CI test for ticket #3173
and #3175.
Resolves:
https://fedorahosted.org/sssd/ticket/3173
https://fedorahosted.org/sssd/ticket/3175
---
src/tests/intg/test_local_domain.py | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/src/tests/intg/test_local_domain.py b/src/tests/intg/test_local_domain.py
index b83e56d..56e3812 100644
--- a/src/tests/intg/test_local_domain.py
+++ b/src/tests/intg/test_local_domain.py
@@ -19,11 +19,13 @@
import os
import stat
import pwd
+import grp
import time
import config
import signal
import subprocess
import pytest
+import ent
from util import unindent
@@ -90,6 +92,11 @@ def assert_nonexistent_user(name):
pwd.getpwnam(name)
+def assert_nonexistent_group(name):
+ with pytest.raises(KeyError):
+ grp.getgrnam(name)
+
+
def test_wrong_LC_ALL(local_domain_only):
"""
Regression test for ticket
@@ -107,3 +114,22 @@ def test_wrong_LC_ALL(local_domain_only):
subprocess.check_call(["sss_userdel", "foo", "-R"])
assert_nonexistent_user("foo")
os.environ["LC_ALL"] = oldvalue
+
+
+def test_sss_group_add_show_del(local_domain_only):
+ """
+ Regression test for tickets
+ https://fedorahosted.org/sssd/ticket/3173
+ https://fedorahosted.org/sssd/ticket/3175
+ """
+
+ subprocess.check_call(["sss_groupadd", "foo", "-g", "10001"])
+
+ "This should not raise KeyError"
+ ent.assert_group_by_name("foo", dict(name="foo", gid=10001))
+
+ "sss_grupshow should return 0 with existing group name"
+ subprocess.check_call(["sss_groupshow", "foo"])
+
+ subprocess.check_call(["sss_groupdel", "foo"])
+ assert_nonexistent_group("foo")
From a065fbfbb7de34f06981c5dade3da123433503dd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com>
Date: Wed, 7 Sep 2016 10:58:25 +0200
Subject: [PATCH 4/7] TOOLS: use internal fqdn for DN
Use internal fqdn when creating
sysdb group dn.
Resolves:
https://fedorahosted.org/sssd/ticket/3178
---
src/tools/sss_sync_ops.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/tools/sss_sync_ops.c b/src/tools/sss_sync_ops.c
index 39ef5be..a0291ba 100644
--- a/src/tools/sss_sync_ops.c
+++ b/src/tools/sss_sync_ops.c
@@ -137,6 +137,7 @@ static int mod_groups_member(struct sss_domain_info *dom,
struct ldb_dn *parent_dn;
int ret;
int i;
+ char *grp_sysdb_fqname = NULL;
tmpctx = talloc_new(NULL);
if (!tmpctx) {
@@ -145,13 +146,21 @@ static int mod_groups_member(struct sss_domain_info *dom,
/* FIXME: add transaction around loop */
for (i = 0; grouplist[i]; i++) {
+ grp_sysdb_fqname = sss_create_internal_fqname(tmpctx, grouplist[i],
+ dom->name);
+ if (grp_sysdb_fqname == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
- parent_dn = sysdb_group_dn(tmpctx, dom, grouplist[i]);
+ parent_dn = sysdb_group_dn(tmpctx, dom, grp_sysdb_fqname);
if (!parent_dn) {
ret = ENOMEM;
goto done;
}
+ talloc_free(grp_sysdb_fqname);
+
ret = sysdb_mod_group_member(dom, member_dn, parent_dn, optype);
if (ret) {
goto done;
From d20a7629ae1408cdc10c5b727dd90b2a93f4c543 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com>
Date: Wed, 7 Sep 2016 13:08:59 +0200
Subject: [PATCH 5/7] TESTS: Test for sss_user/groupmod -a
Regression tests for ticket #3178.
Resolves:
https://fedorahosted.org/sssd/ticket/3178
---
src/tests/intg/test_local_domain.py | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/src/tests/intg/test_local_domain.py b/src/tests/intg/test_local_domain.py
index 56e3812..5e3e3d4 100644
--- a/src/tests/intg/test_local_domain.py
+++ b/src/tests/intg/test_local_domain.py
@@ -133,3 +133,39 @@ def test_sss_group_add_show_del(local_domain_only):
subprocess.check_call(["sss_groupdel", "foo"])
assert_nonexistent_group("foo")
+
+
+def test_add_local_user_to_local_group(local_domain_only):
+ """
+ Regression test for ticket
+ https://fedorahosted.org/sssd/ticket/3178
+ """
+ subprocess.check_call(["sss_groupadd", "-g", "10009", "group10009"])
+ subprocess.check_call(["sss_useradd", "-u", "10009", "-M", "user10009"])
+ subprocess.check_call(["sss_usermod", "-a", "group10009", "user10009"])
+
+ ent.assert_group_by_name(
+ "group10009",
+ dict(name="group10009", passwd="*", gid=10009,
+ mem=ent.contains_only("user10009")))
+
+
+def test_add_local_group_to_local_group(local_domain_only):
+ """
+ Regression test for tickets
+ https://fedorahosted.org/sssd/ticket/3178
+ """
+ subprocess.check_call(["sss_groupadd", "-g", "10009", "group_child"])
+ subprocess.check_call(["sss_useradd", "-u", "10009", "-M", "user_child"])
+ subprocess.check_call(["sss_usermod", "-a", "group_child", "user_child"])
+
+ subprocess.check_call(["sss_groupadd", "-g", "10008", "group_parent"])
+ subprocess.check_call(
+ ["sss_groupmod", "-a", "group_parent", "group_child"])
+
+ # User from child_group is member of parent_group, so child_group's
+ # member must be also parent_group's member
+ ent.assert_group_by_name(
+ "group_parent",
+ dict(name="group_parent", passwd="*", gid=10008,
+ mem=ent.contains_only("user_child")))
From 02f71a660b2b6ff7ce6ea6527aa6b599cef5ef00 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com>
Date: Wed, 7 Sep 2016 14:43:13 +0200
Subject: [PATCH 6/7] TOOLS: sss_mc_refresh_nested_group short/fqname usage
We use shortname to refresh memory cache,
but in case of nested groups, we used
internal_fqname to refresh parent groups.
We also wrongly used the shortname for
sysdb_search operation. Which caused
error message to be printed when
sss_usermod -a or sss_groupmod -a
where called.
---
src/tools/tools_mc_util.c | 67 +++++++++++++++++++++++++++++++++--------------
1 file changed, 48 insertions(+), 19 deletions(-)
diff --git a/src/tools/tools_mc_util.c b/src/tools/tools_mc_util.c
index 2516a19..b2988ae 100644
--- a/src/tools/tools_mc_util.c
+++ b/src/tools/tools_mc_util.c
@@ -293,62 +293,91 @@ errno_t sss_mc_refresh_group(const char *groupname)
return sss_mc_refresh_ent(groupname, SSS_TOOLS_GROUP);
}
-errno_t sss_mc_refresh_nested_group(struct tools_ctx *tctx,
- const char *name)
+static errno_t sss_mc_refresh_nested_group(struct tools_ctx *tctx,
+ const char *shortname)
{
errno_t ret;
- struct ldb_message *msg;
+ struct ldb_message *msg = NULL;
struct ldb_message_element *el;
const char *attrs[] = { SYSDB_MEMBEROF,
SYSDB_NAME,
NULL };
size_t i;
- char *parent_name;
+ char *parent_internal_name;
+ char *parent_shortname;
+ char *internal_name;
+ TALLOC_CTX *tmpctx;
+
+ tmpctx = talloc_new(tctx);
+ if (tmpctx == NULL) {
+ return ENOMEM;
+ }
+
+ internal_name = sss_create_internal_fqname(tmpctx, shortname,
+ tctx->local->name);
+ if (internal_name == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
- ret = sss_mc_refresh_group(name);
+ ret = sss_mc_refresh_group(shortname);
if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,
- "Cannot refresh group %s from memory cache\n", name);
+ "Cannot refresh group %s from memory cache\n", shortname);
/* try to carry on */
}
- ret = sysdb_search_group_by_name(tctx, tctx->local, name, attrs, &msg);
+ ret = sysdb_search_group_by_name(tmpctx, tctx->local, internal_name, attrs,
+ &msg);
if (ret) {
DEBUG(SSSDBG_OP_FAILURE,
"Search failed: %s (%d)\n", strerror(ret), ret);
- return ret;
+ goto done;
}
el = ldb_msg_find_element(msg, SYSDB_MEMBEROF);
if (!el || el->num_values == 0) {
- DEBUG(SSSDBG_TRACE_INTERNAL, "Group %s has no parents\n", name);
- talloc_free(msg);
- return EOK;
+ DEBUG(SSSDBG_TRACE_INTERNAL, "Group %s has no parents\n",
+ internal_name);
+ ret = EOK;
+ goto done;
}
/* This group is nested. We need to invalidate all its parents, too */
for (i=0; i < el->num_values; i++) {
- ret = sysdb_group_dn_name(tctx->sysdb, tctx,
+ ret = sysdb_group_dn_name(tctx->sysdb, tmpctx,
(const char *) el->values[i].data,
- &parent_name);
+ &parent_internal_name);
if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE, "Malformed DN [%s]? Skipping\n",
(const char *) el->values[i].data);
- talloc_free(parent_name);
+ talloc_free(parent_internal_name);
continue;
}
- ret = sss_mc_refresh_group(parent_name);
- talloc_free(parent_name);
+ ret = sss_parse_internal_fqname(tmpctx, parent_internal_name,
+ &parent_shortname, NULL);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse name %s\n",
+ parent_internal_name);
+ goto done;
+ }
+
+ ret = sss_mc_refresh_group(parent_shortname);
+ talloc_free(parent_internal_name);
+ talloc_free(parent_shortname);
if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,
- "Cannot refresh group %s from memory cache\n", name);
+ "Cannot refresh group %s from memory cache\n", parent_shortname);
/* try to carry on */
}
}
- talloc_free(msg);
- return EOK;
+ ret = EOK;
+
+done:
+ talloc_free(tmpctx);
+ return ret;
}
errno_t sss_mc_refresh_grouplist(struct tools_ctx *tctx,
From 9e72c52e4a6f652b862c0e951d6d97f2a2a3c129 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com>
Date: Wed, 7 Sep 2016 15:00:12 +0200
Subject: [PATCH 7/7] TESTS: Add FQDN variants for some tests
Adds FQDN variants of some already existing
tests.
---
src/tests/intg/test_local_domain.py | 83 +++++++++++++++++++++++++++++++++++++
1 file changed, 83 insertions(+)
diff --git a/src/tests/intg/test_local_domain.py b/src/tests/intg/test_local_domain.py
index 5e3e3d4..b34e4a3 100644
--- a/src/tests/intg/test_local_domain.py
+++ b/src/tests/intg/test_local_domain.py
@@ -87,6 +87,27 @@ def local_domain_only(request):
return None
+@pytest.fixture
+def local_domain_only_fqdn(request):
+ conf = unindent("""\
+ [sssd]
+ domains = LOCAL
+ services = nss
+
+ [nss]
+ memcache_timeout = 0
+
+ [domain/LOCAL]
+ id_provider = local
+ min_id = 10000
+ max_id = 20000
+ use_fully_qualified_names = True
+ """).format(**locals())
+ create_conf_fixture(request, conf)
+ create_sssd_fixture(request)
+ return None
+
+
def assert_nonexistent_user(name):
with pytest.raises(KeyError):
pwd.getpwnam(name)
@@ -169,3 +190,65 @@ def test_add_local_group_to_local_group(local_domain_only):
"group_parent",
dict(name="group_parent", passwd="*", gid=10008,
mem=ent.contains_only("user_child")))
+
+
+def test_sss_group_add_show_del_fqdn(local_domain_only_fqdn):
+ """
+ Regression test for tickets
+ https://fedorahosted.org/sssd/ticket/3173
+ https://fedorahosted.org/sssd/ticket/3175
+ """
+
+ subprocess.check_call(["sss_groupadd", "foo@LOCAL", "-g", "10001"])
+
+ "This should not raise KeyError"
+ ent.assert_group_by_name("foo@LOCAL", dict(name="foo@LOCAL", gid=10001))
+
+ "sss_grupshow should return 0 with existing group name"
+ subprocess.check_call(["sss_groupshow", "foo@LOCAL"])
+
+ subprocess.check_call(["sss_groupdel", "foo@LOCAL"])
+ assert_nonexistent_group("foo@LOCAL")
+
+
+def test_add_local_user_to_local_group_fqdn(local_domain_only_fqdn):
+ """
+ Regression test for ticket
+ https://fedorahosted.org/sssd/ticket/3178
+ """
+ subprocess.check_call(
+ ["sss_groupadd", "-g", "10009", "group10009@LOCAL"])
+ subprocess.check_call(
+ ["sss_useradd", "-u", "10009", "-M", "user10009@LOCAL"])
+ subprocess.check_call(
+ ["sss_usermod", "-a", "group10009@LOCAL", "user10009@LOCAL"])
+
+ ent.assert_group_by_name(
+ "group10009@LOCAL",
+ dict(name="group10009@LOCAL", passwd="*", gid=10009,
+ mem=ent.contains_only("user10009@LOCAL")))
+
+
+def test_add_local_group_to_local_group_fqdn(local_domain_only_fqdn):
+ """
+ Regression test for tickets
+ https://fedorahosted.org/sssd/ticket/3178
+ """
+ subprocess.check_call(
+ ["sss_groupadd", "-g", "10009", "group_child@LOCAL"])
+ subprocess.check_call(
+ ["sss_useradd", "-u", "10009", "-M", "user_child@LOCAL"])
+ subprocess.check_call(
+ ["sss_usermod", "-a", "group_child@LOCAL", "user_child@LOCAL"])
+
+ subprocess.check_call(
+ ["sss_groupadd", "-g", "10008", "group_parent@LOCAL"])
+ subprocess.check_call(
+ ["sss_groupmod", "-a", "group_parent@LOCAL", "group_child@LOCAL"])
+
+ # User from child_group is member of parent_group, so child_group's
+ # member must be also parent_group's member
+ ent.assert_group_by_name(
+ "group_parent@LOCAL",
+ dict(name="group_parent@LOCAL", passwd="*", gid=10008,
+ mem=ent.contains_only("user_child@LOCAL")))
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
