On 10/20/2015 08:36 PM, Pavel Reichl wrote:
Nick I went ahead and started to reworking the patch in the direction
you
proposed. Could you check that this wip patch is what you had in mind?
Thanks!
Sure!
Overall, I think the general scheme of separating tests you're doing is great!
See below for specific comments.
Just a reminder: we still need to have this file in Makefile.am.
+#
+# Simple user override
+#
+
+(a)pytest.fixture
+def env_simple_user_override(request, ldap_conn):
This name is actually quite good!
+
+ 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",
+ "-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()
+
+ return None
+
+def test_user_simple_override(ldap_conn, env_simple_user_override):
This name is also good, but I think it's better to have consistency between
test and fixture names. I.e. have the same word order.
+ """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'))
+
+#
+# Root user override
+#
+
+def assert_root_user_override():
Does this have to be a separate function? If yes, then the name is confusing.
I understand you're naming it after the fixture, but this particular name can
be interpreted as if the function checks that root user was overridden, but in
fact it checks the pristine state - the contrary.
I'd rather have this simply in the fixture directly, or fix the name.
+ 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"))
+
+
+(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)
Is it OK that we add entries *after* starting SSSD?
Wouldn't this be an opportunity for timing issues?
Or am I being too cautious?
+ # Assert
+ assert_root_user_override()
+
+ # 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()
+
+ return None
+
+
+def test_user_root_override(ldap_conn, env_root_user_override):
+ """Test entries are not overriden to root"""
+
+ # I have to discuss this with author. I think override should
+ # happen completly or not at all.
+ 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, interesting.
Interesting indeed :)
+ 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"))
+
+ # result of this command is probably non deterministic as 2 users with name root
+ # can be looked up
+ ent.assert_passwd_by_name(
+ 'root',
+ dict(name='root', uid=0, gid=0))
+
+#
+# Override of override
+#
+
+(a)pytest.fixture
+def env_override_of_override_user_override(request, ldap_conn):
Now, this is a bit too long, IMO. Perhaps we can remove one "override"?
Can I suggest a naming scheme? What if we have something like this:
env_user_override_simple
test_user_override_simple
env_user_override_root
test_user_override_root
env_user_override_of_override
test_user_override_of_override
env_group_override_simple
test_group_override_simple
env_group_override_root
test_group_override_root
env_group_override_of_override
test_group_override_of_override
and so on?
+
+ 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')
+
+ 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'))
+
+ # Override
+ subprocess.check_call(["sss_override", "user-add",
"user1",
+ "-u", "10010",
+ "-g", "20010",
+ "-n", "ov_user1",
As I said, overall direction looks good to me. Thanks, Pavel!
Nick