From 26654d3e5f5882dd1681116cb49228d108351d48 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose@redhat.com>
Date: Thu, 12 Aug 2021 09:27:57 +0200
Subject: [PATCH] cache_req: cache_first fix for fully-qualified names
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

With commit b572871236a7f9059d375a5ab1bff8cbfd519956 "cache_req:
introduce cache_behavior enumeration" the processing of cache and
backend lookups was refactored. Unfortunately this introduce an issue
when looking up users or groups with a fully-qualified name and the
'cache_first = True' option is set.

In the old code the case when a domain name is available was handle
before the cache_first first option was evaluated and cache_req was
instructed to first look in the cache and then call the backend if the
object is not available or expired, i.e. the default behavior. Since
only a single domain is involved this is in agreement with 'cache_first
= True' and only a single iteration is needed.

In the new code the cache_first option is evaluated before the presence
of a domain name is checked and as a result even for single domain
searches the first cache_req iteration is only looking at the cache and
will not call the backend. This means the now for searches with a
fully-qualified name a second iteration is needed if the object was not
found in the cache.

Unfortunately the old exit condition that if a domain name is present
only a single iteration is needed is still present in the new code which
effectively makes requests with fully-qualified named only search the
cache and never call the backends. This patch removes the exit condition
and does a second iteration for fully-qualified names as well if
'cache_first = True' is set.

Resolves: https://github.com/SSSD/sssd/issues/5744

Reviewed-by: Pavel Březina <pbrezina@redhat.com>
---
 src/responder/common/cache_req/cache_req.c  |  3 +-
 src/tests/cmocka/test_responder_cache_req.c | 53 +++++++++++++++++++++
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/src/responder/common/cache_req/cache_req.c b/src/responder/common/cache_req/cache_req.c
index 750d655c1..56ec077f3 100644
--- a/src/responder/common/cache_req/cache_req.c
+++ b/src/responder/common/cache_req/cache_req.c
@@ -1331,8 +1331,7 @@ static errno_t cache_req_select_domains(struct tevent_req *req,
 
     state = tevent_req_data(req, struct cache_req_state);
 
-    if ((state->cr->cache_behavior != CACHE_REQ_CACHE_FIRST)
-        || (domain_name != NULL)) {
+    if (state->cr->cache_behavior != CACHE_REQ_CACHE_FIRST) {
 
         if (!state->first_iteration) {
             /* We're done here. */
diff --git a/src/tests/cmocka/test_responder_cache_req.c b/src/tests/cmocka/test_responder_cache_req.c
index 5cf7660e7..27a525f6e 100644
--- a/src/tests/cmocka/test_responder_cache_req.c
+++ b/src/tests/cmocka/test_responder_cache_req.c
@@ -992,6 +992,56 @@ void test_user_by_name_missing_notfound(void **state)
     assert_true(test_ctx->dp_called);
 }
 
+void test_user_by_name_missing_notfound_cache_first(void **state)
+{
+    struct cache_req_test_ctx *test_ctx = NULL;
+
+    test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
+    test_ctx->rctx->cache_first = true;
+
+    /* Mock values. */
+    will_return(__wrap_sss_dp_get_account_send, test_ctx);
+    mock_account_recv_simple();
+    mock_parse_inp(users[0].short_name, NULL, ERR_OK);
+
+    /* Test. */
+    run_user_by_name(test_ctx, test_ctx->tctx->dom, 0, ENOENT);
+    assert_true(test_ctx->dp_called);
+}
+
+void test_user_by_name_missing_notfound_full_name(void **state)
+{
+    struct cache_req_test_ctx *test_ctx = NULL;
+
+    test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
+
+    /* Mock values. */
+    will_return(__wrap_sss_dp_get_account_send, test_ctx);
+    mock_account_recv_simple();
+    mock_parse_inp(users[0].short_name, TEST_DOM_NAME, ERR_OK);
+
+    /* Test. */
+    run_user_by_name(test_ctx, test_ctx->tctx->dom, 0, ENOENT);
+    assert_true(test_ctx->dp_called);
+}
+
+void test_user_by_name_missing_notfound_cache_first_full_name(void **state)
+{
+    struct cache_req_test_ctx *test_ctx = NULL;
+
+    test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
+    test_ctx->rctx->cache_first = true;
+
+    /* Mock values. */
+    will_return(__wrap_sss_dp_get_account_send, test_ctx);
+    mock_account_recv_simple();
+    mock_parse_inp(users[0].short_name, TEST_DOM_NAME, ERR_OK);
+
+    /* Test. */
+    run_user_by_name(test_ctx, test_ctx->tctx->dom, 0, ENOENT);
+    assert_true(test_ctx->dp_called);
+}
+
 void test_user_by_name_multiple_domains_requested_domains_found(void **state)
 {
     struct cache_req_test_ctx *test_ctx = NULL;
@@ -4255,6 +4305,9 @@ int main(int argc, const char *argv[])
         new_single_domain_test(user_by_name_ncache),
         new_single_domain_test(user_by_name_missing_found),
         new_single_domain_test(user_by_name_missing_notfound),
+        new_single_domain_test(user_by_name_missing_notfound_cache_first),
+        new_single_domain_test(user_by_name_missing_notfound_full_name),
+        new_single_domain_test(user_by_name_missing_notfound_cache_first_full_name),
         new_multi_domain_test(user_by_name_multiple_domains_found),
         new_multi_domain_test(user_by_name_multiple_domains_notfound),
         new_multi_domain_test(user_by_name_multiple_domains_parse),
-- 
2.26.3