responders: unify usage of sss_cmd_send_empty and _error

Originally sss_cmd_send_empty() called also sss_cmd_done() to send
an empty reply packet to the cliant where as sss_cmd_send_error()
did not invoke this call and required the caller to call it manually.

For this reason, a possible error in users_find_by_cert_done() was not
send to the caller.

This patch unifies the usage of those two functions in a way that both
of them only creates the reply packet but do not send it. Another
sss_cmd_done() call is required to send the reply.

Because sss_cmd_done() is now always called, unit tests needed to be
changed to always mock a value for __wrap_sss_cmd_done.

Resolves:
https://fedorahosted.org/sssd/ticket/3151

Reviewed-by: Lukáš Slebodník <lslebodn@redhat.com>

5 files changed, 32 insertions, 12 deletions

diff --git a/src/responder/autofs/autofssrv_cmd.c b/src/responder/autofs/autofssrv_cmd.c
index 68bef15ec..0878707a7 100644
--- a/src/responder/autofs/autofssrv_cmd.c
+++ b/src/responder/autofs/autofssrv_cmd.c
@@ -38,7 +38,7 @@ static int autofs_cmd_send_error(struct autofs_cmd_ctx *cmdctx, int err)
 static int
 autofs_cmd_send_empty(struct autofs_cmd_ctx *cmdctx)
 {
-    return sss_cmd_send_empty(cmdctx->cctx, cmdctx);
+    return sss_cmd_send_empty(cmdctx->cctx);
 }
 
 static int
@@ -54,6 +54,7 @@ autofs_cmd_done(struct autofs_cmd_ctx *cmdctx, int ret)
         if (ret) {
             return EFAULT;
         }
+        sss_cmd_done(cmdctx->cctx, cmdctx);
         break;
 
     case EAGAIN:
diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
index 9b69842ed..4d4d7f9e8 100644
--- a/src/responder/common/responder.h
+++ b/src/responder/common/responder.h
@@ -196,7 +196,7 @@ int activate_unix_sockets(struct resp_ctx *rctx,
 
 /* responder_cmd.c */
 int sss_cmd_empty_packet(struct sss_packet *packet);
-int sss_cmd_send_empty(struct cli_ctx *cctx, TALLOC_CTX *freectx);
+int sss_cmd_send_empty(struct cli_ctx *cctx);
 int sss_cmd_send_error(struct cli_ctx *cctx, int err);
 void sss_cmd_done(struct cli_ctx *cctx, void *freectx);
 int sss_cmd_get_version(struct cli_ctx *cctx);
diff --git a/src/responder/common/responder_cmd.c b/src/responder/common/responder_cmd.c
index 104e3771c..bd05ca2f2 100644
--- a/src/responder/common/responder_cmd.c
+++ b/src/responder/common/responder_cmd.c
@@ -66,7 +66,7 @@ int sss_cmd_empty_packet(struct sss_packet *packet)
     return EOK;
 }
 
-int sss_cmd_send_empty(struct cli_ctx *cctx, TALLOC_CTX *freectx)
+int sss_cmd_send_empty(struct cli_ctx *cctx)
 {
     struct cli_protocol *pctx;
     int ret;
@@ -88,7 +88,6 @@ int sss_cmd_send_empty(struct cli_ctx *cctx, TALLOC_CTX *freectx)
     }
 
     sss_packet_set_error(pctx->creq->out, EOK);
-    sss_cmd_done(cctx, freectx);
     return EOK;
 }
 
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index 29c5cbfef..0352ff348 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -45,7 +45,7 @@ static int nss_cmd_send_error(struct nss_cmd_ctx *cmdctx, int err)
 static int nss_cmd_send_empty(struct nss_cmd_ctx *cmdctx)
 {
     struct cli_ctx *cctx = cmdctx->cctx;
-    return sss_cmd_send_empty(cctx, cmdctx);
+    return sss_cmd_send_empty(cctx);
 }
 
 int nss_cmd_done(struct nss_cmd_ctx *cmdctx, int ret)
@@ -60,6 +60,7 @@ int nss_cmd_done(struct nss_cmd_ctx *cmdctx, int ret)
         if (ret) {
             return EFAULT;
         }
+        sss_cmd_done(cmdctx->cctx, cmdctx);
         break;
 
     case EAGAIN:
@@ -5453,13 +5454,14 @@ static void users_find_by_cert_done(struct tevent_req *req)
 done:
     if (ret == EOK) {
         sss_packet_set_error(pctx->creq->out, EOK);
-        sss_cmd_done(cctx, NULL);
     } else if (ret == ENOENT) {
-        sss_cmd_send_empty(cctx, NULL);
+        sss_cmd_send_empty(cctx);
     } else {
         sss_cmd_send_error(cctx, ret);
     }
 
+    sss_cmd_done(cctx, NULL);
+
     return;
 }
 
diff --git a/src/tests/cmocka/test_nss_srv.c b/src/tests/cmocka/test_nss_srv.c
index bc63c959f..1e64a0b8a 100644
--- a/src/tests/cmocka/test_nss_srv.c
+++ b/src/tests/cmocka/test_nss_srv.c
@@ -127,13 +127,18 @@ void __wrap_sss_cmd_done(struct cli_ctx *cctx, void *freectx)
 
     check_cb = sss_mock_ptr_type(cmd_cb_fn_t);
 
-    pctx = talloc_get_type(cctx->protocol_ctx, struct cli_protocol);
-    packet = pctx->creq->out;
+    if (check_cb == NULL) {
+        nss_test_ctx->tctx->error = ENOENT;
+    } else {
+        pctx = talloc_get_type(cctx->protocol_ctx, struct cli_protocol);
+        packet = pctx->creq->out;
+
+        __real_sss_packet_get_body(packet, &body, &blen);
 
-    __real_sss_packet_get_body(packet, &body, &blen);
+        nss_test_ctx->tctx->error = check_cb(sss_packet_get_status(packet),
+                                             body, blen);
+    }
 
-    nss_test_ctx->tctx->error = check_cb(sss_packet_get_status(packet),
-                                         body, blen);
     nss_test_ctx->tctx->done = true;
     talloc_free(freectx);
 }
@@ -566,6 +571,7 @@ void test_nss_getpwnam_neg(void **state)
 
     assert_int_equal(nss_test_ctx->ncache_hits, 0);
 
+    set_cmd_cb(NULL);
     ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_GETPWNAM,
                           nss_test_ctx->nss_cmds);
     assert_int_equal(ret, EOK);
@@ -582,6 +588,7 @@ void test_nss_getpwnam_neg(void **state)
     nss_test_ctx->tctx->done = false;
 
     mock_input_user_or_group("testuser_neg");
+    set_cmd_cb(NULL);
     ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_GETPWNAM,
                           nss_test_ctx->nss_cmds);
     assert_int_equal(ret, EOK);
@@ -1031,6 +1038,7 @@ void test_nss_getpwuid_neg(void **state)
 
     assert_int_equal(nss_test_ctx->ncache_hits, 0);
 
+    set_cmd_cb(NULL);
     ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_GETPWUID,
                           nss_test_ctx->nss_cmds);
     assert_int_equal(ret, EOK);
@@ -1047,6 +1055,7 @@ void test_nss_getpwuid_neg(void **state)
     nss_test_ctx->tctx->done = false;
 
     mock_input_id(nss_test_ctx, uid_neg);
+    set_cmd_cb(NULL);
     ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_GETPWUID,
                           nss_test_ctx->nss_cmds);
     assert_int_equal(ret, EOK);
@@ -2536,6 +2545,7 @@ void test_nss_getpwnam_upn_neg(void **state)
 
     assert_int_equal(nss_test_ctx->ncache_hits, 0);
 
+    set_cmd_cb(NULL);
     ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_GETPWNAM,
                           nss_test_ctx->nss_cmds);
     assert_int_equal(ret, EOK);
@@ -2553,6 +2563,7 @@ void test_nss_getpwnam_upn_neg(void **state)
     nss_test_ctx->ncache_hits = 0;
 
     mock_input_user_or_group("nosuchupnuser@upndomain.test");
+    set_cmd_cb(NULL);
     ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_GETPWNAM,
                           nss_test_ctx->nss_cmds);
     assert_int_equal(ret, EOK);
@@ -2667,6 +2678,7 @@ void test_initgr_neg_by_name(const char *name, bool is_upn)
 
     assert_int_equal(nss_test_ctx->ncache_hits, 0);
 
+    set_cmd_cb(NULL);
     ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_INITGR,
                           nss_test_ctx->nss_cmds);
     assert_int_equal(ret, EOK);
@@ -2685,6 +2697,7 @@ void test_initgr_neg_by_name(const char *name, bool is_upn)
     nss_test_ctx->ncache_hits = 0;
 
     mock_input_user_or_group(name);
+    set_cmd_cb(NULL);
     ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_INITGR,
                           nss_test_ctx->nss_cmds);
     assert_int_equal(ret, EOK);
@@ -3227,6 +3240,7 @@ void test_nss_getnamebysid_neg(void **state)
 
     assert_int_equal(nss_test_ctx->ncache_hits, 0);
 
+    set_cmd_cb(NULL);
     ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_GETNAMEBYSID,
                           nss_test_ctx->nss_cmds);
     assert_int_equal(ret, EOK);
@@ -3243,6 +3257,7 @@ void test_nss_getnamebysid_neg(void **state)
     nss_test_ctx->tctx->done = false;
 
     mock_input_user_or_group(user_sid);
+    set_cmd_cb(NULL);
     ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_GETNAMEBYSID,
                           nss_test_ctx->nss_cmds);
     assert_int_equal(ret, EOK);
@@ -3449,6 +3464,7 @@ void test_nss_getnamebycert_neg(void **state)
 
     assert_int_equal(nss_test_ctx->ncache_hits, 0);
 
+    set_cmd_cb(NULL);
     ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_GETNAMEBYCERT,
                           nss_test_ctx->nss_cmds);
     assert_int_equal(ret, EOK);
@@ -3465,6 +3481,7 @@ void test_nss_getnamebycert_neg(void **state)
     nss_test_ctx->tctx->done = false;
 
     mock_input_user_or_group(TEST_TOKEN_CERT);
+    set_cmd_cb(NULL);
     ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_GETNAMEBYCERT,
                           nss_test_ctx->nss_cmds);
     assert_int_equal(ret, EOK);
@@ -3590,6 +3607,7 @@ void test_nss_getsidbyname_neg(void **state)
     mock_account_recv_simple();
 
     /* Query for that user, call a callback when command finishes */
+    set_cmd_cb(NULL);
     ret = sss_cmd_execute(nss_test_ctx->cctx, SSS_NSS_GETSIDBYNAME,
                           nss_test_ctx->nss_cmds);
     assert_int_equal(ret, EOK);