On 01/20/2016 11:31 AM, Jakub Hrozek wrote:
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?
How about:
* cache_req_validate_state
* cache_req_validate
* cache_req_expiration_status (to avoid the word state)
* cache_req_needs_refresh
> +{
> + 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?
How about:
* cache_req_dpreq_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?
This is not needed, since we always search by original name if possible
with cache_req which also includes this condition.