Hi Pavel,
On 11/13/2015 02:22 PM, Pavel Reichl wrote:
Hello Nick, thanks for the valid comments I hope I addressed them
all.
I used the dictionary hint, but I didn't use it everywhere - mostly just
where the same dict would be reused in the same function.
Sure, thank you.
On 11/03/2015 04:40 PM, Nikolai Kondrashov wrote:
>
>> +
>> + # 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?
Well, I can't :-). I just hit this case and I don't think this is a security
bug, but I decided to highlight it and at least canonize this behavior -
whenever implementation would change test would need to be changed too.
I can remove this part of the test - no trouble at all.
Um, do you mean you have no idea what it does :)? I'm sure you had some intent
of putting it there, couldn't you describe it in a short comment?
If I understood you right, you can just write that this line is for tracking
the particular behavior doesn't change and describe what specific behavior it
is testing - that would already be great.
>
> 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'])
Sorry I could not use this format as pep8 didn't like it. I had to remove
the backslash and then '2nd' part of the string could not be intended (pep8
didn't like too).
Ah, alright. I like how you rewrote it anyway, thanks :)
I have some more comments below, which are mostly regarding sorting the
multiple successive assertions so they're in some order. It would be great if
they kept to a single idea of order throughout the whole file, but if there
was just some order in all of them that would also be good.
This helps locate specific assertions in the list, check if all of them are
there, or if some are extra or wrong.
Otherwise there are just two other little problems I noticed and that's it.
+(a)pytest.fixture
+def env_two_users_and_group_overriden(request, ldap_conn,
+ 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
+ user1 = dict(name='ov_user1', passwd='*', uid=10010, gid=20010,
+ gecos='Overriden User 1',
+ dir='/home/ov/user1',
+ shell='/bin/ov_user1_shell')
+
+ user2 = 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('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)
+
+ ent.assert_passwd_by_name('ov_user2@LDAP', user2)
+
+ ent.assert_passwd_by_name('user2@LDAP', user2)
+
+ ent.assert_passwd_by_name('ov_user2', user2)
+
+ ent.assert_passwd_by_name('user2', user2)
+
Do you need the empty lines between these assertions? I would rather remove
them and save some vertical space. Also, could you please sort these in some
order (not very important which), so we have something like this:
ent.assert_passwd_by_name('user1', user1)
ent.assert_passwd_by_name('user1@LDAP', user1)
ent.assert_passwd_by_name('ov_user1', user1)
ent.assert_passwd_by_name('ov_user1@LDAP', user1)
ent.assert_passwd_by_name('user2', user2)
ent.assert_passwd_by_name('user2@LDAP', user2)
ent.assert_passwd_by_name('ov_user2', user2)
ent.assert_passwd_by_name('ov_user2@LDAP', user2)
This would make it easier to locate specific assertions and check if all of
them are there.
+(a)pytest.fixture
+def env_simple_user_override(request, ldap_conn, 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()
+
+
+def test_simple_user_override(ldap_conn, env_simple_user_override):
+ """Test entries are overriden"""
+
+ user1 = dict(name='ov_user1', passwd='*', uid=10010, gid=20010,
+ gecos='Overriden User 1',
+ dir='/home/ov/user1',
+ shell='/bin/ov_user1_shell')
+
+ user2 = 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('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)
+
+ ent.assert_passwd_by_name('ov_user2@LDAP', user2)
+
+ ent.assert_passwd_by_name('user2@LDAP', user2)
+
+ ent.assert_passwd_by_name('ov_user2', user2)
+
+ ent.assert_passwd_by_name('user2', user2)
+
Same goes here.
+def test_remove_user_override(ldap_conn, env_remove_user_override):
+
+ # Test 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')
+
+ user1 = dict(name='user1', passwd='*', uid=10001, gid=20001,
+ gecos='User Number 1',
+ dir='/home/user1',
+ shell='/bin/user1_shell')
+
+ 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('user1', user1)
+
+ ent.assert_passwd_by_name('user1@LDAP', user1)
+
+ ent.assert_passwd_by_name('user2@LDAP', user2)
+
+ ent.assert_passwd_by_name('user2', user2)
+
Same goes here.
+def test_imp_exp_user_override(ldap_conn,
env_imp_exp_user_override):
+
+ user1 = dict(name='ov_user1', passwd='*', uid=10010, gid=20010,
+ gecos='Overriden User 1',
+ dir='/home/ov/user1',
+ shell='/bin/ov_user1_shell')
+
+ user2 = 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('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)
+
+ ent.assert_passwd_by_name('ov_user2@LDAP', user2)
+
+ ent.assert_passwd_by_name('user2@LDAP', user2)
+
+ ent.assert_passwd_by_name('ov_user2', user2)
+
+ ent.assert_passwd_by_name('user2', user2)
+
Same goes here.
+(a)pytest.fixture
+def env_find_user_override(request, ldap_conn,
+ env_two_users_and_group_overriden):
+ pass
This fixture doesn't seem necessary and you can just use
env_two_users_and_group_overriden below, but if you feel it helps consistency,
so be it.
+
+
+def test_find_user_override(ldap_conn, env_find_user_override):
+
+ out = subprocess.check_output(['sss_override', 'user-find'])
+
+ # Expected override of users
+ exp_usr_ovrd = ['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']
+
+ assert set(out.splitlines()) == set(exp_usr_ovrd)
+
+ out = subprocess.check_output(['sss_override', 'user-find',
+ '--domain=LDAP'])
+
+ assert set(out.splitlines()) == set(exp_usr_ovrd)
+
+ # Unexpected parameter is reported
+ ret = subprocess.call(['sss_override', 'user-find',
'PARAM'])
+ assert ret == 1
+(a)pytest.fixture
+def env_group_override(request, ldap_conn, env_group_basic):
+
+ # Override
+ subprocess.check_call(["sss_override", "group-add",
"group",
+ "-n", "ov_group",
+ "-g", "3001"])
+
+ subprocess.check_call(["sss_override", "group-add",
"empty_group@LDAP",
+ "--name", "ov_empty_group",
+ "--gid", "3002"])
+
+ # Restart SSSD so the override might take effect
+ restart_sssd()
+
+ # Assert entries are overridden
+ empty_group = dict(gid=3002, mem=ent.contains_only())
+ group = dict(gid=3001, mem=ent.contains_only("user1", "user2"))
+
+ ent.assert_group_by_name("group", group)
+ ent.assert_group_by_name("ov_group", group)
+ ent.assert_group_by_name("group@LDAP", group)
+ ent.assert_group_by_name("ov_group@LDAP", group)
+ ent.assert_group_by_name("ov_empty_group@LDAP", empty_group)
+ ent.assert_group_by_name("empty_group@LDAP", empty_group)
+ ent.assert_group_by_name("empty_group", empty_group)
+ ent.assert_group_by_name("ov_empty_group", empty_group)
+
It would be nice to have these sorted as well.
+def test_replace_group_override(ldap_conn,
env_replace_group_override):
+
+ # Test overrides are overridden
+ with pytest.raises(KeyError):
+ pwd.getpwnam('ov_group')
+ with pytest.raises(KeyError):
+ pwd.getpwnam('ov_empty_group')
+
+ group = dict(gid=4001, mem=ent.contains_only("user1", "user2"))
+ empty_group = dict(gid=4002, mem=ent.contains_only())
+
+ ent.assert_group_by_name("group", group)
+ ent.assert_group_by_name("ov2_group", group)
+ ent.assert_group_by_name("group@LDAP", group)
+ ent.assert_group_by_name("ov2_group@LDAP", group)
+
+ ent.assert_group_by_name("empty_group@LDAP", empty_group)
+ ent.assert_group_by_name("ov2_empty_group", empty_group)
+ ent.assert_group_by_name("empty_group@LDAP", empty_group)
+ ent.assert_group_by_name("ov2_empty_group@LDAP", empty_group)
+
These are also out of order and one assertion is *duplicated*, apparently by
mistake of not updating a copy-pasted line. Sorting also helps spot these
mistakes.
+# Regression test for bug #2802
+# sss_override segfaults when accidentally adding --help flag to some commands
+
+
+def test_regr_2802_override():
+
+ subprocess.call(["sss_override", "user-del",
"--help"])
+
Shouldn't this actually verify the exit status?
Thanks for working on this, Pavel!
Nick