Hi Pavel,
On 10/30/2015 08:17 PM, Pavel Reichl wrote:
attached patch adds tests for {grop,user}-{find,show} commands.
I like the overall design much better now. Thanks for working on this Pavel!
Please see my comments below.
+LDAP_BASE_DN = "dc=example,dc=com"
Could you please remove this global variable, use the literal string in
ds_inst and get the DN with ldap_conn.ds_inst.base_dn everywhere else,
similarly to what is done in ldap_test.py?
We'll need to update ldap_test.py and test_memory_cache.py to not have this
global variable either.
+
+
+(a)pytest.fixture(scope="module")
+def ds_inst(request):
+ """LDAP server instance fixture"""
+ ds_inst = ds_openldap.DSOpenLDAP(
+ config.PREFIX, 10389, LDAP_BASE_DN,
+ "cn=admin", "Secret123")
I.e. write directly:
ds_inst = ds_openldap.DSOpenLDAP(
config.PREFIX, 10389, "dc=example,dc=com",
"cn=admin", "Secret123")
+def prepare_sssd(request, ldap_conn,
use_fully_qualified_names=False):
+ """Prepare SSSD woth defaults"""
Typo in "with".
+ # Restart SSSD so the override might take effect
+ restart_sssd()
+
+ return None
I think we can dispense with these "return None" and save a few lines of code.
+
+
+def test_simple_user_override(ldap_conn, env_simple_user_override):
+ """Test entries are overriden"""
+
+ ent.assert_passwd_by_name(
+ 'ov_user1',
+ dict(name='ov_user1', passwd='*', uid=10010, gid=20010,
+ gecos='Overriden User 1',
+ dir='/home/ov/user1',
+ shell='/bin/ov_user1_shell'))
+
+ ent.assert_passwd_by_name(
+ 'user1',
+ dict(name='ov_user1', passwd='*', uid=10010, gid=20010,
+ gecos='Overriden User 1',
+ dir='/home/ov/user1',
+ shell='/bin/ov_user1_shell'))
+
+ ent.assert_passwd_by_name(
+ 'ov_user1@LDAP',
+ dict(name='ov_user1', passwd='*', uid=10010, gid=20010,
+ gecos='Overriden User 1',
+ dir='/home/ov/user1',
+ shell='/bin/ov_user1_shell'))
+
+ ent.assert_passwd_by_name(
+ 'user1@LDAP',
+ dict(name='ov_user1', passwd='*', uid=10010, gid=20010,
+ gecos='Overriden User 1',
+ dir='/home/ov/user1',
+ shell='/bin/ov_user1_shell'))
+
+ ent.assert_passwd_by_name(
+ 'ov_user2@LDAP',
+ dict(name='ov_user2', passwd='*', uid=10020, gid=20020,
+ gecos='Overriden User 2',
+ dir='/home/ov/user2',
+ shell='/bin/ov_user2_shell'))
+
+ ent.assert_passwd_by_name(
+ 'user2',
+ dict(name='ov_user2', passwd='*', uid=10020, gid=20020,
+ gecos='Overriden User 2',
+ dir='/home/ov/user2',
+ shell='/bin/ov_user2_shell'))
Can you assign these user pattern dictionaries to variables and use them
instead, so they're not duplicated? This way we'll save on code lines, future
mistakes and will make the code more readable.
E.g.:
user1 = dict(name='ov_user1', passwd='*', uid=10010, gid=20010,
gecos='Overriden User 1',
dir='/home/ov/user1',
shell='/bin/ov_user1_shell')
ent.assert_passwd_by_name('user1', user1)
ent.assert_passwd_by_name('ov_user1', user1)
ent.assert_passwd_by_name('user1@LDAP', user1)
ent.assert_passwd_by_name('ov_user1@LDAP', user1)
and so on.
Same goes for other user tests, where possible.
+#
+# Root user override
+#
+
+
+(a)pytest.fixture
+def env_root_user_override(request, ldap_conn):
+
+ prepare_sssd(request, ldap_conn)
+
+ # Add entries
+ ent_list = ldap_ent.List(LDAP_BASE_DN)
+ ent_list.add_user("user1", 10001, 20001,
+ gecos='User Number 1',
+ loginShell='/bin/user1_shell',
+ homeDirectory='/home/user1')
+ ent_list.add_user("user2", 10002, 20001,
+ gecos='User Number 2',
+ loginShell='/bin/user2_shell',
+ homeDirectory='/home/user2')
+
+ create_ldap_fixture(request, ldap_conn, ent_list)
+
+ # Assert entries are not overriden
+ ent.assert_passwd_by_name(
+ 'user1',
+ dict(name='user1', passwd='*', uid=10001, gid=20001,
+ gecos='User Number 1',
+ dir='/home/user1',
+ shell='/bin/user1_shell'))
+
+ ent.assert_passwd_by_name(
+ 'user2',
+ dict(name='user2', passwd='*', uid=10002, gid=20001,
+ gecos='User Number 2',
+ dir='/home/user2',
+ shell='/bin/user2_shell'))
+
+ ent.assert_passwd_by_name(
+ 'root',
+ dict(name='root', uid=0, gid=0))
+
+ ent.assert_passwd_by_uid(0, dict(name="root"))
+
+ # Override
+ subprocess.check_call(["sss_override", "user-add",
"user1",
+ "-u", "0",
+ "-g", "0",
+ "-n", "ov_user1",
+ "-c", "Overriden User 1",
+ "-h", "/home/ov/user1",
+ "-s", "/bin/ov_user1_shell"])
+
+ subprocess.check_call(["sss_override", "user-add",
"user2@LDAP",
Why does this override use FQN? It doesn't seem necessary for the purpose of
this test. If you'd like to test general override with/without FQN it's better
to make a separate test (which seems to be already done in
test_simple_user_override), so this test stays on point and doesn't fail for
unrelated reason.
If you'd like to see that root override protection still works with FQNs
(doesn't seem necessary to me, but I don't know the internals), then likely
both of the cases here need to be tested with and without FQNs.
In general each test should check only one thing, if possible.
> + "-u", "10020",
> + "-g", "20020",
> + "-n", "root",
> + "-c", "Overriden User 2",
> + "-h", "/home/ov/user2",
> + "-s", "/bin/ov_user2_shell"])
> +
+ # Restart SSSD so the override might take effect
+ restart_sssd()
+
+ return None
> +
> +
> +def test_root_user_override(ldap_conn, env_root_user_override):
> + """Test entries are not overriden to root"""
> +
> + # Override does not have to happen completly, trying to set uid or gid
> + # to 0 is simply ignored.
> + ent.assert_passwd_by_name(
> + 'ov_user1',
> + dict(name='ov_user1', passwd='*', uid=10001, gid=20001,
> + gecos='Overriden User 1',
> + dir='/home/ov/user1',
> + shell='/bin/ov_user1_shell'))
> +
> + # We can create override with name root.
> + ent.assert_passwd_by_name(
> + 'user2',
> + dict(name='root', passwd='*', uid=10020, gid=20020,
> + gecos='Overriden User 2',
> + dir='/home/ov/user2',
> + shell='/bin/ov_user2_shell'))
> +
> + ent.assert_passwd_by_uid(0, dict(name="root"))
Could you please add a comment to this line explaining what it tests?
+(a)pytest.fixture
+def env_remove_user_override(request, ldap_conn):
+
+ prepare_sssd(request, ldap_conn)
+
+ # Add entries
+ ent_list = ldap_ent.List(LDAP_BASE_DN)
+ ent_list.add_user("user1", 10001, 20001,
+ gecos='User Number 1',
+ loginShell='/bin/user1_shell',
+ homeDirectory='/home/user1')
+
+ ent_list.add_user("user2", 10002, 20001,
+ gecos='User Number 2',
+ loginShell='/bin/user2_shell',
+ homeDirectory='/home/user2')
+
+ ent_list.add_group("group", 2001,
+ ["user2", "user1"])
+
+ create_ldap_fixture(request, ldap_conn, ent_list)
+
+ # Assert entries are not overriden
+ with pytest.raises(KeyError):
+ pwd.getpwnam('ov_user1')
+ with pytest.raises(KeyError):
+ pwd.getpwnam('ov_user2')
+ with pytest.raises(KeyError):
+ pwd.getpwnam('ov_user1@LDAP')
+ with pytest.raises(KeyError):
+ pwd.getpwnam('ov_user2@LDAP')
+
+ # Override
+ subprocess.check_call(["sss_override", "user-add",
"user1",
+ "-u", "10010",
+ "-g", "20010",
+ "-n", "ov_user1",
+ "-c", "Overriden User 1",
+ "-h", "/home/ov/user1",
+ "-s", "/bin/ov_user1_shell"])
+
+ subprocess.check_call(["sss_override", "user-add",
"user2@LDAP",
We don't seem to need FQN here either. This is setup, so just go with the
simplest working code. We (should) have a separate test for that.
+def test_show_user_override(ldap_conn, env_show_user_override):
+
+ out = subprocess.check_output(['sss_override', 'user-show',
'user1'])
+ assert out == "user1@LDAP:ov_user1:10010:20010:Overriden User 1:\
+/home/ov/user1:/bin/ov_user1_shell\n"
+
+ out = subprocess.check_output(['sss_override', 'user-show',
'user2@LDAP'])
+ assert out == "user2@LDAP:ov_user2:10020:20020:Overriden User 2:\
+/home/ov/user2:/bin/ov_user2_shell\n"
Could you please use implicit literal string concatenation here to keep the
indent nice? Something like this:
assert out == "user2@LDAP:ov_user2:10020:20020:Overriden User 2:" \
"/home/ov/user2:/bin/ov_user2_shell\n"
+def test_find_user_override(ldap_conn, env_find_user_override):
+
+ out = subprocess.check_output(['sss_override', 'user-find'])
+ assert set(out.splitlines()) == set(['user1@LDAP:ov_user1:10010:20010:Overriden
User 1:\
+/home/ov/user1:/bin/ov_user1_shell', 'user2@LDAP:ov_user2:10020:20020:Overriden
User 2:\
+/home/ov/user2:/bin/ov_user2_shell'])
+
+ out = subprocess.check_output(['sss_override', 'user-find',
+ '--domain=LDAP'])
+ assert set(out.splitlines()) == set(['user1@LDAP:ov_user1:10010:20010:Overriden
User 1:\
+/home/ov/user1:/bin/ov_user1_shell', 'user2@LDAP:ov_user2:10020:20020:Overriden
User 2:\
+/home/ov/user2:/bin/ov_user2_shell'])
Could you make each set item start on a new line, so it's easier to read?
Something like this:
assert set(out.splitlines()) == \
set(['user1@LDAP:ov_user1:10010:20010:Overriden User 1:' \
'/home/ov/user1:/bin/ov_user1_shell',
'user2@LDAP:ov_user2:10020:20020:Overriden User 2:' \
'/home/ov/user2:/bin/ov_user2_shell'])
+def test_simple_group_override(ldap_conn,
env_simple_group_override):
+ """Test entries are overriden"""
+
+ ent.assert_group_by_name(
+ "ov_group", dict(gid=3001, mem=ent.contains_only("user1",
"user2")))
+ ent.assert_group_by_name(
+ "ov_empty_group", dict(gid=3002, mem=ent.contains_only()))
+ ent.assert_group_by_name(
+ "group", dict(gid=3001, mem=ent.contains_only("user1",
"user2")))
+ ent.assert_group_by_name(
+ "empty_group", dict(gid=3002, mem=ent.contains_only()))
+ ent.assert_group_by_name(
+ "ov_group@LDAP", dict(gid=3001,
+ mem=ent.contains_only("user1",
"user2")))
+ ent.assert_group_by_name(
+ "ov_empty_group@LDAP", dict(gid=3002, mem=ent.contains_only()))
+ ent.assert_group_by_name(
+ "group@LDAP", dict(gid=3001,
+ mem=ent.contains_only("user1",
"user2")))
+ ent.assert_group_by_name(
+ "empty_group@LDAP", dict(gid=3002, mem=ent.contains_only()))
Could you put actual group pattern dictionaries into variables to remove
duplication and improve readability here as well?
E.g.:
group = dict(gid=3001, mem=ent.contains_only("user1", "user2"))
empty_group = dict(gid=3002, mem=ent.contains_only())
ent.assert_group_by_name("ov_group@LDAP", group)
ent.assert_group_by_name("ov_empty_group", empty_group)
and so on.
Same goes for other group tests, where applicable.
Now, I notice that fixtures share a lot of code. Could you please use
functions or sub-fixtures instead to save code lines, reduce duplication and
make the test suite easier to comprehend overall?
I.e. I see that many user tests use identical users in setup. For them we can
create a sub-fixture. Like this:
@pytest.fixture
def env_two_users_and_group(request, ldap_conn):
prepare_sssd(request, ldap_conn)
# Add entries
ent_list = ldap_ent.List(LDAP_BASE_DN)
ent_list.add_user("user1", 10001, 20001,
gecos='User Number 1',
loginShell='/bin/user1_shell',
homeDirectory='/home/user1')
ent_list.add_user("user2", 10002, 20001,
gecos='User Number 2',
loginShell='/bin/user2_shell',
homeDirectory='/home/user2')
ent_list.add_group("group", 2001,
["user2", "user1"])
create_ldap_fixture(request, ldap_conn, ent_list)
# Assert entries are not overriden
with pytest.raises(KeyError):
pwd.getpwnam('ov_user1')
with pytest.raises(KeyError):
pwd.getpwnam('ov_user2')
with pytest.raises(KeyError):
pwd.getpwnam('ov_user1@LDAP')
with pytest.raises(KeyError):
pwd.getpwnam('ov_user2@LDAP')
Then use it like this:
@pytest.fixture
def env_root_user_override(request, env_two_users_and_group):
ent.assert_passwd_by_name(
'root',
dict(name='root', uid=0, gid=0))
ent.assert_passwd_by_uid(0, dict(name="root"))
# Override
subprocess.check_call(["sss_override", "user-add",
"user1",
"-u", "0",
"-g", "0",
"-n", "ov_user1",
"-c", "Overriden User 1",
"-h", "/home/ov/user1",
"-s", "/bin/ov_user1_shell"])
subprocess.check_call(["sss_override", "user-add",
"user2@LDAP",
"-u", "10020",
"-g", "20020",
"-n", "root",
"-c", "Overriden User 2",
"-h", "/home/ov/user2",
"-s", "/bin/ov_user2_shell"])
# Restart SSSD so the override might take effect
restart_sssd()
@pytest.fixture
def env_imp_exp_user_override(request, env_two_users_and_group):
...
Note that the above doesn't need to reference "ldap_conn" directly anymore.
Moreover, some fixtures also seem to add the same override, so we can probably
reuse that as well:
@pytest.fixture
def env_two_users_and_group_overriden(request, env_two_users_and_group):
# Override
subprocess.check_call(["sss_override", "user-add",
"user1",
"-u", "10010",
"-g", "20010",
"-n", "ov_user1",
"-c", "Overriden User 1",
"-h", "/home/ov/user1",
"-s", "/bin/ov_user1_shell"])
subprocess.check_call(["sss_override", "user-add",
"user2@LDAP",
"-u", "10020",
"-g", "20020",
"-n", "ov_user2",
"-c", "Overriden User 2",
"-h", "/home/ov/user2",
"-s", "/bin/ov_user2_shell"])
# Restart SSSD so the override might take effect
restart_sssd()
# Assert entries are overriden
ent.assert_passwd_by_name(
'ov_user1',
dict(name='ov_user1', passwd='*', uid=10010, gid=20010,
gecos='Overriden User 1',
dir='/home/ov/user1',
shell='/bin/ov_user1_shell'))
ent.assert_passwd_by_name(
'user1',
dict(name='ov_user1', passwd='*', uid=10010, gid=20010,
gecos='Overriden User 1',
dir='/home/ov/user1',
shell='/bin/ov_user1_shell'))
ent.assert_passwd_by_name(
'ov_user1@LDAP',
dict(name='ov_user1', passwd='*', uid=10010, gid=20010,
gecos='Overriden User 1',
dir='/home/ov/user1',
shell='/bin/ov_user1_shell'))
ent.assert_passwd_by_name(
'user1@LDAP',
dict(name='ov_user1', passwd='*', uid=10010, gid=20010,
gecos='Overriden User 1',
dir='/home/ov/user1',
shell='/bin/ov_user1_shell'))
ent.assert_passwd_by_name(
'ov_user2@LDAP',
dict(name='ov_user2', passwd='*', uid=10020, gid=20020,
gecos='Overriden User 2',
dir='/home/ov/user2',
shell='/bin/ov_user2_shell'))
ent.assert_passwd_by_name(
'user2',
dict(name='ov_user2', passwd='*', uid=10020, gid=20020,
gecos='Overriden User 2',
dir='/home/ov/user2',
shell='/bin/ov_user2_shell'))
Then use the above like this:
@pytest.fixture
def env_remove_user_override(request, env_two_users_and_group_overriden):
# Drop all overrides
subprocess.check_call(["sss_override", "user-del",
"user1"])
subprocess.check_call(["sss_override", "user-del",
"user2@LDAP"])
# Avoid hitting memory cache
time.sleep(2)
@pytest.fixture
def env_imp_exp_user_override(request, env_two_users_and_group_overriden):
# Export overrides
subprocess.check_call(["sss_override", "user-export",
OVERRIDE_FILENAME])
# Drop all overrides
subprocess.check_call(["sss_override", "user-del",
"user1"])
subprocess.check_call(["sss_override", "user-del",
"user2@LDAP"])
# Avoid hitting memory cache
time.sleep(2)
# Assert entries are not overridden
with pytest.raises(KeyError):
pwd.getpwnam('ov_user1')
with pytest.raises(KeyError):
pwd.getpwnam('ov_user2')
with pytest.raises(KeyError):
pwd.getpwnam('ov_user1@LDAP')
with pytest.raises(KeyError):
pwd.getpwnam('ov_user2@LDAP')
ent.assert_passwd_by_name(
'user1@LDAP',
dict(name='user1', passwd='*', uid=10001, gid=20001,
gecos='User Number 1',
dir='/home/user1',
shell='/bin/user1_shell'))
ent.assert_passwd_by_name(
'user2',
dict(name='user2', passwd='*', uid=10002, gid=20001,
gecos='User Number 2',
dir='/home/user2',
shell='/bin/user2_shell'))
# Import overrides
subprocess.check_call(["sss_override", "user-import",
OVERRIDE_FILENAME])
restart_sssd()
You can also probably use this in env_replace_user_override and just ignore
the second user:
@pytest.fixture
def env_replace_user_override(request, env_two_users_and_group_overriden):
# Override of override
subprocess.check_call(["sss_override", "user-add",
"user1",
"-u", "10100",
"-g", "20100",
"-n", "ov2_user1",
"-c", "Overriden2 User 1",
"-h", "/home/ov2/user1",
"-s", "/bin/ov2_user1_shell"])
# Restart SSSD so the override might take effect
restart_sssd()
And so on.
Same goes with groups.
Hit me on IRC if I'm unclear anywhere and you'd like a quick explanation of
what I meant.
Thank you!
Nick