On Thu, Jan 07, 2016 at 04:08:48PM +0100, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/2849
You can use Pavel's CI "intg - test regr 2849" test which he posted to the
list today.
From c78ae1b550794548518a083870443b29fa5d12bf Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Wed, 6 Jan 2016 12:08:18 +0100
Subject: [PATCH 1/3] cache_req: simplify cache_req_cache_check()
ACK to the code, but I think we can improve the names..
---
src/responder/common/responder_cache_req.c | 80 ++++++++++++++++++------------
1 file changed, 49 insertions(+), 31 deletions(-)
diff --git a/src/responder/common/responder_cache_req.c
b/src/responder/common/responder_cache_req.c
index 4ab52b8188859f1143ba1ffa3de03d14ecc028c2..91f50cb308de250eca786b474845426687821189
100644
--- a/src/responder/common/responder_cache_req.c
+++ b/src/responder/common/responder_cache_req.c
@@ -568,6 +568,47 @@ static bool cache_req_bypass_cache(struct cache_req_input *input)
return false;
}
+static errno_t cache_req_cache_state(struct cache_req_input *input,
+ struct ldb_result *result,
+ time_t cache_refresh_percent)
We already have a structure with this name. What about something like
cache_req_check_entry or something like that?
+{
+ time_t expire;
+
+ if (result == NULL || result->count == 0 || cache_req_bypass_cache(input)) {
+ return ENOENT;
+ }
+
+ if (input->type == CACHE_REQ_INITGROUPS) {
+ expire = ldb_msg_find_attr_as_uint64(result->msgs[0],
+ SYSDB_INITGR_EXPIRE, 0);
+ } else {
+ expire = ldb_msg_find_attr_as_uint64(result->msgs[0],
+ SYSDB_CACHE_EXPIRE, 0);
+ }
+
+ return sss_cmd_check_cache(result->msgs[0], cache_refresh_percent, expire);
+}
+
+static void cache_req_provider_params(struct cache_req_input *input,
+ const char **_string,
+ uint32_t *_id,
+ const char **_flag)
Are these provider params or request params?
From 8b7da2a5542c4c16ea0491ad2d24fd9614901cbc Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Wed, 6 Jan 2016 12:45:38 +0100
Subject: [PATCH 2/3] cache_req: do not lookup views if possible
This is needed for LOCAL view but also creates a shortcut for
server side overrides.
Resolves:
https://fedorahosted.org/sssd/ticket/2849
---
src/responder/common/responder_cache_req.c | 97 +++++++++++++++++++++++++++---
1 file changed, 87 insertions(+), 10 deletions(-)
diff --git a/src/responder/common/responder_cache_req.c
b/src/responder/common/responder_cache_req.c
index 91f50cb308de250eca786b474845426687821189..76f7a93436b15f6506e5e2eea8ea3c0d1af70284
100644
--- a/src/responder/common/responder_cache_req.c
+++ b/src/responder/common/responder_cache_req.c
@@ -589,24 +589,101 @@ static errno_t cache_req_cache_state(struct cache_req_input
*input,
return sss_cmd_check_cache(result->msgs[0], cache_refresh_percent, expire);
}
-static void cache_req_provider_params(struct cache_req_input *input,
+static void cache_req_provider_params(TALLOC_CTX *mem_ctx,
+ struct cache_req_input *input,
+ struct ldb_result *result,
const char **_string,
uint32_t *_id,
const char **_flag)
{
+ struct ldb_result *user = NULL;
+ const char *name = NULL;
+ uint32_t id = 0;
+ errno_t ret;
+
*_id = input->id;
*_string = input->dom_objname;
-
- if (input->type == CACHE_REQ_USER_BY_CERT) {
- *_string = input->cert;
- }
-
*_flag = NULL;
- if (DOM_HAS_VIEWS(input->domain)) {
- *_flag = EXTRA_INPUT_MAYBE_WITH_VIEW;
- } else if (cache_req_input_is_upn(input)) {
+
+ if (cache_req_input_is_upn(input)) {
*_flag = EXTRA_NAME_IS_UPN;
+ return;
}
+
+ if (input->type == CACHE_REQ_USER_BY_CERT) {
+ *_string = input->cert;
+ return;
+ }
+
+ if (!DOM_HAS_VIEWS(input->domain)) {
+ return;
+ }
+
+ /* We must search with views. */
+ if (result == NULL || result->count == 0) {
+ *_flag = EXTRA_INPUT_MAYBE_WITH_VIEW;
+ return;
+ }
+
+ /* If domain has views we will try to user original values instead of the
+ * overridden ones. This is a must for the LOCAL view since we can't look
+ * it up otherwise. But it is also a shortcut for non-local views where
+ * we will not fail over to the overridden value. */
+
+ switch (input->type) {
+ case CACHE_REQ_USER_BY_NAME:
+ case CACHE_REQ_GROUP_BY_NAME:
+ name = ldb_msg_find_attr_as_string(result->msgs[0], SYSDB_NAME, NULL);
+ if (name == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Bug: name cannot be NULL\n");
+ }
+ break;
+ case CACHE_REQ_USER_BY_ID:
+ id = ldb_msg_find_attr_as_uint64(result->msgs[0], SYSDB_UIDNUM, 0);
+ if (id == 0) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Bug: id cannot be 0\n");
+ }
+ break;
+ case CACHE_REQ_GROUP_BY_ID:
+ id = ldb_msg_find_attr_as_uint64(result->msgs[0], SYSDB_GIDNUM, 0);
+ if (id == 0) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Bug: id cannot be 0\n");
+ }
+ break;
+ case CACHE_REQ_INITGROUPS:
+ ret = sysdb_getpwnam_with_views(NULL, input->domain,
+ input->dom_objname, &user);
+ if (ret != EOK || user == NULL || user->count != 1) {
+ /* Case where the user is not found has been already handled. If
+ * this is not OK, it is an error. */
+ DEBUG(SSSDBG_CRIT_FAILURE, "Unable to match initgroups user "
+ "[%d]: %s\n", ret, sss_strerror(ret));
+ break;
+ }
+
+ name = ldb_msg_find_attr_as_string(user->msgs[0], SYSDB_NAME,
+ NULL);
+ if (name == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Bug: name cannot be NULL\n");
+ break;
+ }
+
+ talloc_steal(mem_ctx, name);
+ talloc_free(user);
+ break;
+ default:
+ return;
+ }
+
Maybe I don't remember the flow exactly anymore but the NSS lookup code
also contains:
ldb_msg_find_attr_as_string(dctx->res->msgs[0],
OVERRIDE_PREFIX SYSDB_NAME,
NULL) != NULL
We don't handle that condition here..what does that do? Do we need to
include the condition in cache_req, too?