On Fri, Jan 30, 2015 at 05:04:09PM +0100, Pavel Březina wrote:
On 01/27/2015 02:19 PM, Pavel Březina wrote:
>Hi,
>these patches were created as part of my dbus work however they do not
>depend on it. Those patches can be applied on top of current master and
>review separately.
>
>Recently introduced cache_req interface which hides all the logic of
>search for a sysdb object in cache and data provider supported only user
>by name and initgroups request.
>
>These patches add support also for "user by id", "group by name",
"group
>by id" (for new Users and Groups IFP interfaces). It also hides
>sss_parse_inp into cache_req so the may no longer be the need to parse
>username in consumers of the API.
>
>Everything is unit tested.
>
>Happy reviewing :-)
Self nack. I was passing wrong name (input->name instead of
input->orig_name) into sss_parse_inp which caused SIGSEGV. This was not
caught by unit tests since the return value of sss_parse_inp is mocked.
I fixed this and renamed input->name into input->safe_name to avoid another
name confusion.
From 2dc53a8123ac6db65a1138e0dbdba426c5604acd Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Mon, 26 Jan 2015 11:17:24 +0100
Subject: [PATCH 01/13] cache_req tests: rename test_user to test_user_by_name
This is done in order to distinguish those tests from other
user tests that are about to come. For example: test_user_by_id.
ACK
From b50fd0281ea83a69830bd82101c83e4b3eae71b0 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Mon, 26 Jan 2015 15:28:50 +0100
Subject: [PATCH 02/13] cache_req tests: define user name constant
Using a constant here is better since the name is shared between
the test function and testing _done function.
---
src/tests/cmocka/test_responder_cache_req.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/src/tests/cmocka/test_responder_cache_req.c
b/src/tests/cmocka/test_responder_cache_req.c
index 6c83cac0fb9bee6ebe767b5cca14622cce2db23c..bc551f8a3518aa777febd19cfe3947868a19b39c
100644
--- a/src/tests/cmocka/test_responder_cache_req.c
+++ b/src/tests/cmocka/test_responder_cache_req.c
@@ -33,6 +33,8 @@
#define TEST_DOM_NAME "responder_cache_req_test"
#define TEST_ID_PROVIDER "ldap"
+#define TEST_USER_NAME "test-user"
+
#define new_single_domain_test(test) \
unit_test_setup_teardown(test_ ## test, \
test_single_domain_setup, \
@@ -86,9 +88,10 @@ __wrap_sss_dp_get_account_send(TALLOC_CTX *mem_ctx,
ctx->dp_called = true;
if (ctx->create_user) {
- ret = sysdb_store_user(ctx->tctx->dom, "test-user",
"pwd", 1000, 1000,
- NULL, NULL, NULL, "cn=test-user,dc=test",
NULL,
- NULL, 1000, time(NULL));
+ ret = sysdb_store_user(ctx->tctx->dom, TEST_USER_NAME, "pwd",
+ 1000, 1000, NULL, NULL, NULL,
+ "cn=test-user,dc=test", NULL, NULL,
You can also use the #define here:
"cn="TEST_USER_NAME",dc=test"
but it's fine if you don't, your call.
From db6c8c38bd450f21a9f8d0489fc2b88b6cf3d1b9 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Fri, 23 Jan 2015 14:03:07 +0100
Subject: [PATCH 03/13] cache_req: preparations for different input type
Currently cache_req takes only user name as an input parameter. However,
this is not enough since we will need also UID and GID in the future.
This patch creates a structure to hold input parameters so it can be
simply extended to support other input types.
General comment - it would be great if the new code could be decorated
with DEBUG statements, not only for failures, but also in order for us
to follow the general flow. Currently it would be really hard to tell how
far we got and what went wrong.
I'm fine with this being an extra patch atop the small changes, but the
DEBUG messages must be there in order for the patchset to be used,
especially by other responders.
---
src/responder/common/responder_cache_req.c | 341 +++++++++++++++++++++--------
src/responder/common/responder_cache_req.h | 41 +++-
src/responder/ifp/ifpsrv_cmd.c | 24 +-
3 files changed, 306 insertions(+), 100 deletions(-)
diff --git a/src/responder/common/responder_cache_req.c
b/src/responder/common/responder_cache_req.c
index 39f90cfe16fa7c726eae92110ce717ffefb556a2..b10dfbd783be1c8f4782f0b765644ca1083749a7
100644
--- a/src/responder/common/responder_cache_req.c
+++ b/src/responder/common/responder_cache_req.c
@@ -28,54 +28,175 @@
#include "responder/common/responder_cache_req.h"
#include "providers/data_provider.h"
-static errno_t cache_req_check_ncache(enum sss_dp_acct_type dp_type,
+struct cache_req_input {
+ enum cache_req_type type;
+
+ /* Provided input. */
+ const char *orig_name;
+
+ /* Data Provider request type resolved from @type. */
+ enum sss_dp_acct_type dp_type;
I'm not sure why we have both enum cache_req_type and sss_dp_acct_type
with 1:1 mapping used at the same abstraction layer. Either
cache_req_type is something private to the cache_req code and then we
should always use it except when sending requests to the DP where we
should do a one-time conversion, or we can always use sss_dp_acct_type.
Maybe add another patch that replaces dp_type with type for internal
branching within this module?
+
+ /* Domain related informations. */
+ struct sss_domain_info *domain;
+
+ /* Name sanitized according to domain rules such as case sensitivity and
+ * replacement of space character. This needs to be set up for each
+ * domain separately. */
+ const char *safe_name;
+
+ /* Fully qualified object name. */
+ const char *fqn;
+};
+
+struct cache_req_input *
+cache_req_input_create(TALLOC_CTX *mem_ctx,
+ enum cache_req_type type,
+ const char *name)
+{
+ struct cache_req_input *input;
+
+ input = talloc_zero(mem_ctx, struct cache_req_input);
+ if (input == NULL) {
+ return NULL;
+ }
+
+ input->type = type;
+
+ /* Check that input parameters match selected type. */
+ switch (input->type) {
+ case CACHE_REQ_USER_BY_NAME:
+ case CACHE_REQ_INITGROUPS:
+ if (name == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Bug: name cannot be NULL!\n");
+ goto fail;
+ }
+
+ input->orig_name = talloc_strdup(input, name);
+ if (input->orig_name == NULL) {
+ goto fail;
+ }
+ break;
+ }
+
+ /* Resolve Data Provider request type. */
+ switch (type) {
+ case CACHE_REQ_USER_BY_NAME:
+ input->dp_type = SSS_DP_USER;
+ break;
+
+ case CACHE_REQ_INITGROUPS:
+ input->dp_type = SSS_DP_INITGROUPS;
+ break;
+ }
This function looks OK except I would like the dp_type to go away :)
+
+ return input;
+
+fail:
+ talloc_free(input);
+ return NULL;
+}
+
+static errno_t
+cache_req_input_set_domain(struct cache_req_input *input,
+ struct sss_domain_info *domain,
+ struct resp_ctx *rctx)
+{
+ TALLOC_CTX *tmp_ctx = NULL;
+ const char *name = NULL;
+ const char *fqn = NULL;
+ errno_t ret;
+
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ return ENOMEM;
+ }
+
+ talloc_zfree(input->safe_name);
+ talloc_zfree(input->fqn);
In general I like the approach you've taken but I think we can make it
even better. Add another structure like cache_req_dom_ctx and add it inside
the cache_req_input structure. Always create cache_req_dom_ctx as a
talloc child of cache_req_input.
This would not only collapse the above talloc_frees into one, but it
would also nicely separate the per-domain data from per-lookup data and
avoid mixing data from different domains.
I also think dom_name sounds better than safe_name, but meh.
+
+ switch (input->type) {
+ case CACHE_REQ_USER_BY_NAME:
+ case CACHE_REQ_INITGROUPS:
+ name = sss_get_cased_name(tmp_ctx, input->orig_name,
+ domain->case_sensitive);
+ if (name == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ name = sss_reverse_replace_space(tmp_ctx, name, rctx->override_space);
+ if (name == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ fqn = talloc_asprintf(tmp_ctx, "%s@%s", name, domain->name);
I'm afraid FQDN is configurable in SSSD, so creating it is not this easy..
+ if (fqn == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ break;
+ }
+
+ input->domain = domain;
+ input->safe_name = talloc_steal(input, name);
+ input->fqn = talloc_steal(input, fqn);
+
+ ret = EOK;
+
+done:
+ talloc_free(tmp_ctx);
+ return ret;
+}
+
[...]
static errno_t cache_req_get_object(TALLOC_CTX *mem_ctx,
- enum sss_dp_acct_type dp_type,
- struct sss_domain_info *domain,
- const char *name,
+ struct cache_req_input *input,
struct ldb_result **_result)
{
struct ldb_result *result = NULL;
bool one_item_only;
errno_t ret;
- DEBUG(SSSDBG_FUNC_DATA, "Requesting info for [%s@%s]\n",
- name, domain->name);
+ DEBUG(SSSDBG_FUNC_DATA, "Requesting info for [%s]\n", input->fqn);
- switch (dp_type) {
- case SSS_DP_USER:
+ switch (input->type) {
+ case CACHE_REQ_USER_BY_NAME:
one_item_only = true;
- ret = sysdb_getpwnam_with_views(mem_ctx, domain, name, &result);
+ ret = sysdb_getpwnam_with_views(mem_ctx, input->domain,
+ input->safe_name, &result);
break;
- case SSS_DP_INITGROUPS:
+ case CACHE_REQ_INITGROUPS:
one_item_only = false;
- ret = sysdb_initgroups_with_views(mem_ctx, domain, name, &result);
+ ret = sysdb_initgroups_with_views(mem_ctx, input->domain,
+ input->safe_name, &result);
break;
default:
ret = EINVAL;
- DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported DP request type\n");
+ DEBUG(SSSDBG_CRIT_FAILURE, "Unsupported cache request type\n");
break;
Should we free result if ret != EOK ?
This is as far as I've got today, I'll continue with review tomorrow.