[SSSD] [PATCHSET] intg: sss_override

Nikolai Kondrashov Nikolai.Kondrashov at redhat.com
Tue Nov 3 15:40:09 UTC 2015


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.

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

and so on.

Same goes for other user tests, where possible.

> +#
> +# Root user override
> +#
> +
> +
> + at 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 at 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?

> + at 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 at LDAP')
> +    with pytest.raises(KeyError):
> +        pwd.getpwnam('ov_user2 at 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 at 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 at 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 at LDAP'])
> +    assert out == "user2 at 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 at 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 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'])
> +
> +    out = subprocess.check_output(['sss_override', 'user-find',
> +                                   '--domain=LDAP'])
> +    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'])

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'])

> +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 at LDAP", dict(gid=3001,
> +                              mem=ent.contains_only("user1", "user2")))
> +    ent.assert_group_by_name(
> +        "ov_empty_group at LDAP", dict(gid=3002, mem=ent.contains_only()))
> +    ent.assert_group_by_name(
> +        "group at LDAP", dict(gid=3001,
> +                           mem=ent.contains_only("user1", "user2")))
> +    ent.assert_group_by_name(
> +        "empty_group at 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 at 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 at LDAP')
         with pytest.raises(KeyError):
             pwd.getpwnam('ov_user2 at 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 at 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 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
         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 at 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 at 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 at 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 at 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 at 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 at LDAP')
         with pytest.raises(KeyError):
             pwd.getpwnam('ov_user2 at LDAP')

         ent.assert_passwd_by_name(
             'user1 at 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


More information about the sssd-devel mailing list