[SSSD] [PATCH] AD: add config and processing support gpo_map_* options

Lukas Slebodnik lslebodn at redhat.com
Mon Sep 8 14:58:15 UTC 2014


On (08/09/14 10:02), Yassir Elley wrote:
>----- Original Message -----
>> On Fri, Sep 05, 2014 at 06:49:30PM -0400, Yassir Elley wrote:
>> > 
>> > 
>> > ----- Original Message -----
>> > > On Wed, Aug 27, 2014 at 05:24:37PM -0400, Yassir Elley wrote:
>> > > > Hi,
>> > > > 
>> > 
>> > Thanks for the review! Revised patches attached.
>> 
>> Thank you, all my concerns are addresses, but I'm afaid I have some new
>> comments:
>
>Regards,
>Yassir.

just few nitpicks from static analysers.

>From f619ff819498b2d9eb3f492d9b3c9fac5cfb7be6 Mon Sep 17 00:00:00 2001
>From: Yassir Elley <yelley at redhat.com>
>Date: Fri, 5 Sep 2014 18:45:50 -0400
>Subject: [PATCH 2/2] AD-GPO: processing changes for gpo_map_* options
>
>---
> src/providers/ad/ad_access.c |   3 +-
> src/providers/ad/ad_access.h |  13 ++
> src/providers/ad/ad_gpo.c    | 442 +++++++++++++++++++++++++++++++++++++++++--
> src/providers/ad/ad_gpo.h    |   3 +-
> src/providers/ad/ad_init.c   |  21 ++
> 5 files changed, 459 insertions(+), 23 deletions(-)
>
>diff --git a/src/providers/ad/ad_access.c b/src/providers/ad/ad_access.c
>index 5b1792223c6e1d9fd6906851e8b32922fa88056d..00b5c9e834c975c538169cb8a12396f6a14fa3fe 100644
>--- a/src/providers/ad/ad_access.c
>+++ b/src/providers/ad/ad_access.c
>@@ -402,7 +402,8 @@ ad_sdap_access_done(struct tevent_req *subreq)
>                                 state->be_ctx->ev,
>                                 state->domain,
>                                 state->ctx,
>-                                state->pd->user);
>+                                state->pd->user,
>+                                state->pd->service);
> 
>     if (!subreq) {
>         tevent_req_error(req, ENOMEM);
>diff --git a/src/providers/ad/ad_access.h b/src/providers/ad/ad_access.h
>index 60ff5db66c9663e736338cdf7694eb75584c3674..34f5d2d1b6d8641536d2e1b78d686e27a0ff1585 100644
>--- a/src/providers/ad/ad_access.h
>+++ b/src/providers/ad/ad_access.h
>@@ -34,6 +34,19 @@ struct ad_access_ctx {
>         GPO_ACCESS_CONTROL_ENFORCING
>     } gpo_access_control_mode;
>     int gpo_cache_timeout;
>+    /* supported GPO map options */
>+    enum gpo_map_type {
>+        GPO_MAP_INTERACTIVE = 0,
>+        GPO_MAP_REMOTE_INTERACTIVE,
>+        GPO_MAP_NETWORK,
>+        GPO_MAP_BATCH,
>+        GPO_MAP_SERVICE,
>+        GPO_MAP_PERMIT,
>+        GPO_MAP_DENY,
>+        GPO_MAP_NUM_OPTS
>+    } gpo_map_type;
>+    hash_table_t *gpo_map_options_table;
>+    enum gpo_map_type gpo_default_right;
> };
> 
> void
>diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
>index e0905a2df6fb8a4414b10e16dbdf9bb2258dad7e..8b74eeed7b7e70510e54f26ba80ed463e5eaeed0 100644
>--- a/src/providers/ad/ad_gpo.c
>+++ b/src/providers/ad/ad_gpo.c
//snip
>+
>+errno_t
>+ad_gpo_parse_map_option_helper(enum gpo_map_type gpo_map_type,
>+                               hash_key_t key,
>+                               hash_table_t *options_table)
>+{
>+    hash_value_t val;
>+    int hret;
>+    int ret;
>+
>+    hret = hash_lookup(options_table, &key, &val);
>+    if (hret != HASH_SUCCESS && hret != HASH_ERROR_KEY_NOT_FOUND) {
>+        DEBUG(SSSDBG_OP_FAILURE, "Error checking hash table: [%s]\n",
>+              hash_error_string(hret));
>+        ret = EINVAL;
>+        goto done;
>+    } else if (hret == HASH_SUCCESS) {
>+        /* handle unexpected case where mapping for key already exists */
>+        if (val.i == gpo_map_type) {
>+            /* mapping for key exists for same map type; no error */
>+            DEBUG(SSSDBG_TRACE_FUNC,
>+                  "PAM service %s maps to %s multiple times\n", key.str,
>+                  gpo_map_type_string(gpo_map_type));
>+            ret = EOK;
>+        } else {
>+            /* mapping for key exists for different map type; error! */
>+            DEBUG(SSSDBG_CRIT_FAILURE,
>+                  "PAM service %s maps to both %s and %s\n", key.str,
>+                  gpo_map_type_string(val.i), gpo_map_type_string(gpo_map_type));
>+            ret = EINVAL;
>+        }
>+        goto done;
>+    } else {
>+        /* handle expected case where mapping for key doesn't already exist */
>+        val.type = HASH_VALUE_INT;
>+        val.i = gpo_map_type;
>+
>+        hret = hash_enter(options_table, &key, &val);
>+        if (hret != HASH_SUCCESS) {
>+            DEBUG(SSSDBG_OP_FAILURE, "Error checking hash table: [%s]\n",
>+                  hash_error_string(hret));
>+            ret = EIO;
>+        }
          EIO is ignored here.
          missing goto done.
>+        ret = EOK;
>+    }
>+
>+done:
>+    return ret;
>+}
>+

//snip

>+
>+    hret = hash_lookup(ctx->gpo_map_options_table, &key, &val);
>+    if (hret != HASH_SUCCESS && hret != HASH_ERROR_KEY_NOT_FOUND) {
>+        DEBUG(SSSDBG_OP_FAILURE, "Error checking hash table: [%s]\n",
>+              hash_error_string(hret));
>+        ret = EINVAL;
>+        goto immediately;
>+    }
>+
>+    /* if service isn't mapped, map it to value of ad_gpo_default_right option */
>+    if (hret == HASH_ERROR_KEY_NOT_FOUND) {
>+        DEBUG(SSSDBG_TRACE_FUNC, "using default right\n");
>+        gpo_map_type = ctx->gpo_default_right;
>+    } else {
>+        gpo_map_type = val.i;
Could you use explicit casting?
-        gpo_map_type = val.i;
+        gpo_map_type = (enum gpo_map_type) val.i;

It will silence warning:
Error: PW.MIXED_ENUM_TYPE: [#def2]
sssd-1.12.1/src/providers/ad/ad_gpo.c:1257: mixed_enum_type: enumerated type
mixed with another type

>+    }
>+
>+    DEBUG(SSSDBG_TRACE_FUNC, "service %s maps to %s\n", service,
>+          gpo_map_type_string(gpo_map_type));
>+
>+    if (gpo_map_type == GPO_MAP_PERMIT) {
>+        ret = EOK;
>+        tevent_req_done(req);
>+        tevent_req_post(req, ev);
>+        goto immediately;
>+    }

LS



More information about the sssd-devel mailing list