[SSSD] [PATCH] TESTS: extend PAM responder unit test

Lukas Slebodnik lslebodn at redhat.com
Thu Nov 5 08:17:07 UTC 2015


On (04/11/15 16:03), Pavel Reichl wrote:
>On 11/04/2015 01:42 PM, Lukas Slebodnik wrote:
>
>>>From fa082a04387ed83f1b12316d388b63a08ba1305d Mon Sep 17 00:00:00 2001
>>>From: Pavel Reichl <preichl at redhat.com>
>>>Date: Tue, 20 Oct 2015 09:10:30 -0400
>>>Subject: [PATCH 2/3] TESTS: extend PAM responder unit test
>>>
>>>Extend PAM responder unit test to check 'online' cached authentication.
>>>
>
>>
>>>+void test_pam_cached_auth_success(void **state)
>>>+{
>>>+    int ret;
>>>+
>>>+    ret = sysdb_cache_password(pam_test_ctx->tctx->dom, "pamuser", "12345");
>>>+    assert_int_equal(ret, EOK);
>>>+
>>>+    ret = pam_set_last_online_auth_with_curr_token(pam_test_ctx->tctx->dom,
>>>+                                                   "pamuser", time(NULL));
>>>+    assert_int_equal(ret, EOK);
>>>+
>>>+    test_pam_cached_auth_common("12345");
>>>+
>>>+    /* Back end should not be contacted */
>>>+    assert_false(pam_test_ctx->provider_contacted);
>>>+}
>>it would be also good to test two subsequent authentications.
>
>Which two subsequent authentications do you mean? I suppose you don't mean test_pam_cached_auth_wrong_pw() and test_pam_cached_auth_opt_timeout() as both of them are already tested.
>
Let's image following use case:
* cached authentication is enabled.
* user "pamuser" has never authenticated to the machine and thus
  password is not cached
* for the first time the the data provider should be contacted.
  ( storing cached password is done in DP, so you still need to
    call sysdb_cache_password)
* cached authentication shoudl be used for second following authentication.
  and attribute lastOnlineAuthWithCurrentToken should be set be 1st
  authentication so you needn't call pam_set_last_online_auth_with_curr_token.

This test-case would test that attribute lastOnlineAuthWithCurrentToken is
properly set.

Is it clear?

>>In first case, a data provider should be contacted and in the
>>next one should not be contacted.
>>
>>So you would not need to use pam_set_last_online_auth_with_curr_token.
>
>I need answer to prev. question to properly answer these comments.
>
>>
>>
>>>+
>>>+void test_pam_cached_auth_wrong_pw(void **state)
>>>+{
>>>+    int ret;
>>>+
>>>+    ret = sysdb_cache_password(pam_test_ctx->tctx->dom, "pamuser", "12345");
>>>+    assert_int_equal(ret, EOK);
>>>+
>>>+    ret = pam_set_last_online_auth_with_curr_token(pam_test_ctx->tctx->dom,
>>>+                                                   "pamuser", time(NULL));
>>>+    assert_int_equal(ret, EOK);
>>>+
>>>+    test_pam_cached_auth_common("11111");
>>>+
>>>+    /* Back end should be contacted */
>>>+    assert_true(pam_test_ctx->provider_contacted);
>>>+}
>>>+
>>>+/* test cached_auth_timeout option */
>>>+void test_pam_cached_auth_opt_timeout(void **state)
>>>+{
>>>+    int ret;
>>>+
>>>+    ret = sysdb_cache_password(pam_test_ctx->tctx->dom, "pamuser", "12345");
>>>+    assert_int_equal(ret, EOK);
>>>+
>>>+    ret = pam_set_last_online_auth_with_curr_token(pam_test_ctx->tctx->dom,
>>>+                                                   "pamuser", time(NULL));
>>>+    sleep(4);
>>       It would be good to avoid sleep in unit tests. (Especially with hardcoded
>>       magic constant). The reason is that unit test should be fast.
>
>Yes, but we are testing expiration here, I can call pam_set_last_online_auth_with_curr_token() as you propose but the test will be more artificial, but no doubt it will be faster.
>It's just trade off between test factuality and speed.
>OK, I'll do it.
>
Jakub has similar request in review of different patch
https://lists.fedorahosted.org/pipermail/sssd-devel/2015-November/025576.html
"""
> +    assert_int_equal(ret, EOK);
> +
> +    sleep(1);

The purpose of the sleep() here is just to make sure the entry was
    created in the past, right? Would it be equally safe to create the user
    with timestamp time(NULL)-1 to make the test faster
"""



>
>>
>>       You decided to make the function pam_set_last_online_auth_with_curr_token
>>       public so you can use it to set lasst authentication to past.
>>
>>>+    assert_int_equal(ret, EOK);
>>>+
>>>+    test_pam_cached_auth_common("12345");
>>>+
>>>+    /* Back end should be contacted */
>>>+    assert_true(pam_test_ctx->provider_contacted);
>>>+}
>>>+
>>>+/* too long since last on-line authentication */
>>>+void test_pam_cached_auth_timeout(void **state)
>>>+{
>>>+    int ret;
>>>+
>>>+    ret = sysdb_cache_password(pam_test_ctx->tctx->dom, "pamuser", "12345");
>>>+    assert_int_equal(ret, EOK);
>>>+
>>>+    ret = pam_set_last_online_auth_with_curr_token(pam_test_ctx->tctx->dom,
>>>+                                                   "pamuser", 0);
>>>+    assert_int_equal(ret, EOK);
>>>+
>>>+    test_pam_cached_auth_common("12345");
>>>+
>>>+    /* Back end should be contacted */
>>>+    assert_true(pam_test_ctx->provider_contacted);
>>>+}
>>>+
>>>+void test_pam_cached_auth_success_combined_pw_with_cached_2fa(void **state)
>>>+{
>>>+    int ret;
>>>+
>>>+    ret = sysdb_cache_password_ex(pam_test_ctx->tctx->dom, "pamuser",
>>>+                                  "12345678", SSS_AUTHTOK_TYPE_2FA, 5);
>>>+    assert_int_equal(ret, EOK);
>>>+    ret = pam_set_last_online_auth_with_curr_token(pam_test_ctx->tctx->dom,
>>>+                                                   "pamuser", time(NULL));
>>>+    assert_int_equal(ret, EOK);
>>>+
>>>+    test_pam_cached_auth_common("12345678");
>>>+
>>>+    assert_false(pam_test_ctx->provider_contacted);
>>>+}
>>it would be also good to test two subsequent authentications.
>>In first case, a data provider should be contacted and in the
>>next one should not be contacted.
>>
>
>Here you mean: test_pam_offline_auth_no_hash() and test_pam_offline_auth_success() ? If so, do you prefer this to be a separate patch as it IMO doen't relate to testing cached authentication?
No it is the similar test case as I explained
in function test_pam_cached_auth_success

LS


More information about the sssd-devel mailing list