[SSSD] [PATCHSET] intg: sss_override

Nikolai Kondrashov Nikolai.Kondrashov at redhat.com
Fri Nov 13 13:49:10 UTC 2015


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 at LDAP:ov_user1:10010:20010:Overriden User 1:' \
>>                      '/home/ov/user1:/bin/ov_user1_shell',
>>                  'user2 at 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.

> + at 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 at 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 at LDAP', user1)
> +
> +    ent.assert_passwd_by_name('ov_user1 at LDAP', user1)
> +
> +    ent.assert_passwd_by_name('ov_user2 at LDAP', user2)
> +
> +    ent.assert_passwd_by_name('user2 at 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 at LDAP', user1)
     ent.assert_passwd_by_name('ov_user1', user1)
     ent.assert_passwd_by_name('ov_user1 at LDAP', user1)
     ent.assert_passwd_by_name('user2', user2)
     ent.assert_passwd_by_name('user2 at LDAP', user2)
     ent.assert_passwd_by_name('ov_user2', user2)
     ent.assert_passwd_by_name('ov_user2 at LDAP', user2)

This would make it easier to locate specific assertions and check if all of
them are there.

> + at 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 at 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 at LDAP', user1)
> +
> +    ent.assert_passwd_by_name('ov_user1 at LDAP', user1)
> +
> +    ent.assert_passwd_by_name('ov_user2 at LDAP', user2)
> +
> +    ent.assert_passwd_by_name('user2 at 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 at LDAP')
> +    with pytest.raises(KeyError):
> +        pwd.getpwnam('ov_user2 at 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 at LDAP', user1)
> +
> +    ent.assert_passwd_by_name('user2 at 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 at LDAP', user1)
> +
> +    ent.assert_passwd_by_name('ov_user1 at LDAP', user1)
> +
> +    ent.assert_passwd_by_name('ov_user2 at LDAP', user2)
> +
> +    ent.assert_passwd_by_name('user2 at LDAP', user2)
> +
> +    ent.assert_passwd_by_name('ov_user2', user2)
> +
> +    ent.assert_passwd_by_name('user2', user2)
> +

Same goes here.

> + at 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 at LDAP:ov_user1:10010:20010:Overriden User 1:'
> +                    '/home/ov/user1:/bin/ov_user1_shell',
> +                    'user2 at 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



> + at 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 at 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 at LDAP", group)
> +    ent.assert_group_by_name("ov_group at LDAP", group)
> +    ent.assert_group_by_name("ov_empty_group at LDAP", empty_group)
> +    ent.assert_group_by_name("empty_group at 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 at LDAP", group)
> +    ent.assert_group_by_name("ov2_group at LDAP", group)
> +
> +    ent.assert_group_by_name("empty_group at LDAP", empty_group)
> +    ent.assert_group_by_name("ov2_empty_group", empty_group)
> +    ent.assert_group_by_name("empty_group at LDAP", empty_group)
> +    ent.assert_group_by_name("ov2_empty_group at 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


More information about the sssd-devel mailing list