[PATCH] simple access provider: allow fully qualified names

[Pavel Březina]

https://fedorahosted.org/sssd/ticket/2026

I created a separate ticket to support subdomain users and groups in 
allow/deny lists.
https://fedorahosted.org/sssd/ticket/2034

There is a bug in PAC responder at the moment which makes simple access 
provider to always deny access to subdomain users. PAC responder 
currently fails to resolve group membership, thus we get error in access 
provider when looking up groups and access is denied. Jakub will look 
into this.

[Jakub Hrozek]

On Fri, Jul 26, 2013 at 11:49:37AM +0200, Pavel Březina wrote:
> https://fedorahosted.org/sssd/ticket/2026
>
> I created a separate ticket to support subdomain users and groups in
> allow/deny lists.
> https://fedorahosted.org/sssd/ticket/2034
>
> There is a bug in PAC responder at the moment which makes simple access
> provider to always deny access to subdomain users. PAC responder currently
> fails to resolve group membership, thus we get error in access provider when
> looking up groups and access is denied. Jakub will look into this.

Can you also amend the unit tests for the simple access provider?

[Pavel Březina]

On 07/26/2013 11:58 AM, Jakub Hrozek wrote:
> On Fri, Jul 26, 2013 at 11:49:37AM +0200, Pavel Březina wrote:
> > https://fedorahosted.org/sssd/ticket/2026
> >
> > I created a separate ticket to support subdomain users and groups in
> > allow/deny lists.
> > https://fedorahosted.org/sssd/ticket/2034
> >
> > There is a bug in PAC responder at the moment which makes simple access
> > provider to always deny access to subdomain users. PAC responder currently
> > fails to resolve group membership, thus we get error in access provider when
> > looking up groups and access is denied. Jakub will look into this.
>
> Can you also amend the unit tests for the simple access provider?

Sure.
Patches are attached.

[Jakub Hrozek]

On Mon, Jul 29, 2013 at 10:56:12AM +0200, Pavel Březina wrote:
> On 07/26/2013 11:58 AM, Jakub Hrozek wrote:
> > On Fri, Jul 26, 2013 at 11:49:37AM +0200, Pavel Březina wrote:
> > > https://fedorahosted.org/sssd/ticket/2026
> > >
> > > I created a separate ticket to support subdomain users and groups in
> > > allow/deny lists.
> > > https://fedorahosted.org/sssd/ticket/2034
> > >
> > > There is a bug in PAC responder at the moment which makes simple access
> > > provider to always deny access to subdomain users. PAC responder currently
> > > fails to resolve group membership, thus we get error in access provider when
> > > looking up groups and access is denied. Jakub will look into this.
> >
> > Can you also amend the unit tests for the simple access provider?
>
> Sure.
> Patches are attached.

Hi, only some nitpicks now:
> @@ -103,66 +169,47 @@ int sssm_simple_access_init(struct be_ctx *bectx, struct bet_ops **ops,
>  {
>      int ret = EINVAL;
>      struct simple_ctx *ctx;
> int i;
> struct {
>    const char *name;
>    const char *option;
>    char **orig_list;
>    char ***ctx_list;
> } lists[4] = {{"Allow users", CONFDB_SIMPLE_ALLOW_USERS, NULL, NULL},
>              {"Deny users", CONFDB_SIMPLE_DENY_USERS, NULL, NULL},
>              {"Allow groups", CONFDB_SIMPLE_ALLOW_GROUPS, NULL, NULL},
>              {"Deny groups", CONFDB_SIMPLE_DENY_GROUPS, NULL, NULL}};

I don't think you need to hardcode the size here, just lists[] would do.
>  ctx = talloc_zero(bectx, struct simple_ctx);
>  if (ctx == NULL) {
>    DEBUG(1, ("talloc_zero failed.\n"));
>    DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n"));
>    return ENOMEM;
>
> }
>
> ctx->domain = bectx->domain;
>    ctx->be_ctx = bectx;
> /* Users */
> ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
>                                CONFDB_SIMPLE_ALLOW_USERS,
>                                &ctx->allow_users);
> if (ret != EOK) {
>    if (ret == ENOENT) {
>        DEBUG(9, ("Allow user list is empty.\n"));
>        ctx->allow_users = NULL;
>    } else {
>        DEBUG(1, ("confdb_get_string_as_list failed.\n"));
>        goto failed;
>    }
> }
> ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
>                                CONFDB_SIMPLE_DENY_USERS,
>                                &ctx->deny_users);
> if (ret != EOK) {
>    if (ret == ENOENT) {
>        DEBUG(9, ("Deny user list is empty.\n"));
>        ctx->deny_users = NULL;
>    } else {
>        DEBUG(1, ("confdb_get_string_as_list failed.\n"));
>        goto failed;
>    }
> }
> lists[0].ctx_list = &ctx->allow_users;
> lists[1].ctx_list = &ctx->deny_users;
> lists[2].ctx_list = &ctx->allow_groups;
> lists[3].ctx_list = &ctx->deny_groups;
> /* Groups */
> ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
>                                CONFDB_SIMPLE_ALLOW_GROUPS,
>                                &ctx->allow_groups);
> if (ret != EOK) {

And then when iterating over the array, you can use sizeof(lists) as
it's the same scope. Alternatively, you can use a last "sentinel"
element that could be initialized to { NULL, NULL, NULL, NULL } and
iterate until .name != NULL.
> for (i = 0; i < 4; i++) {
>    ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
>                                    lists[i].option, &lists[i].orig_list);
>    if (ret == ENOENT) {
>        DEBUG(9, ("Allow group list is empty.\n"));
>        ctx->allow_groups = NULL;
>    } else {
>        DEBUG(1, ("confdb_get_string_as_list failed.\n"));
>        DEBUG(SSSDBG_FUNC_DATA, ("%s list is empty.\n", lists[i].name));
>        *lists[i].ctx_list = NULL;

Do you actually need to special case ENOENT here?
simple_access_parse_names() handles empty input list just fine. If you
want to double check also here, then I think you should continue
instead.
>    } else if (ret != EOK) {
>        DEBUG(SSSDBG_CRIT_FAILURE, ("confdb_get_string_as_list failed.\n"));
>        goto failed;
>    }
>
> }
>
> ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
>                                CONFDB_SIMPLE_DENY_GROUPS,
>                                &ctx->deny_groups);
>
> if (ret != EOK) {
>    if (ret == ENOENT) {
>        DEBUG(9, ("Deny user list is empty.\n"));
>        ctx->deny_groups = NULL;
>    } else {
>        DEBUG(1, ("confdb_get_string_as_list failed.\n"));
>    ret = simple_access_parse_names(ctx, bectx, lists[i].orig_list,
>                                    lists[i].ctx_list);
>    if (ret != EOK) {
>        DEBUG(SSSDBG_CRIT_FAILURE, ("Unable to parse %s list [%d]: %s\n",
>                                    lists[i].name, ret, sss_strerror(ret)));
>        goto failed;
>    }
> }
>
> -- 
> 1.7.11.7

[Pavel Březina]

On 08/06/2013 01:50 PM, Jakub Hrozek wrote:
> On Mon, Jul 29, 2013 at 10:56:12AM +0200, Pavel Březina wrote:
> > On 07/26/2013 11:58 AM, Jakub Hrozek wrote:
> > > On Fri, Jul 26, 2013 at 11:49:37AM +0200, Pavel Březina wrote:
> > > > https://fedorahosted.org/sssd/ticket/2026
> > > >
> > > > I created a separate ticket to support subdomain users and groups in
> > > > allow/deny lists.
> > > > https://fedorahosted.org/sssd/ticket/2034
> > > >
> > > > There is a bug in PAC responder at the moment which makes simple access
> > > > provider to always deny access to subdomain users. PAC responder currently
> > > > fails to resolve group membership, thus we get error in access provider when
> > > > looking up groups and access is denied. Jakub will look into this.
> > >
> > > Can you also amend the unit tests for the simple access provider?
> >
> > Sure.
> > Patches are attached.
>
> Hi, only some nitpicks now:
> > @@ -103,66 +169,47 @@ int sssm_simple_access_init(struct be_ctx *bectx, struct bet_ops **ops,
> >   {
> >       int ret = EINVAL;
> >       struct simple_ctx *ctx;
> > int i;
> > struct {
> >    const char *name;
> >    const char *option;
> >    char **orig_list;
> >    char ***ctx_list;
> > } lists[4] = {{"Allow users", CONFDB_SIMPLE_ALLOW_USERS, NULL, NULL},
> >              {"Deny users", CONFDB_SIMPLE_DENY_USERS, NULL, NULL},
> >              {"Allow groups", CONFDB_SIMPLE_ALLOW_GROUPS, NULL, NULL},
> >              {"Deny groups", CONFDB_SIMPLE_DENY_GROUPS, NULL, NULL}};
>
> I don't think you need to hardcode the size here, just lists[] would do.
> >   ctx = talloc_zero(bectx, struct simple_ctx);
> >   if (ctx == NULL) {
> >    DEBUG(1, ("talloc_zero failed.\n"));
> >    DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n"));
> >     return ENOMEM;
> > }
> >
> > ctx->domain = bectx->domain;
> > ctx->be_ctx = bectx;
> >
> > /* Users */
> > ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
> >                                CONFDB_SIMPLE_ALLOW_USERS,
> >                                &ctx->allow_users);
> > if (ret != EOK) {
> >    if (ret == ENOENT) {
> >        DEBUG(9, ("Allow user list is empty.\n"));
> >        ctx->allow_users = NULL;
> >    } else {
> >        DEBUG(1, ("confdb_get_string_as_list failed.\n"));
> >        goto failed;
> >    }
> > }
> > ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
> >                                CONFDB_SIMPLE_DENY_USERS,
> >                                &ctx->deny_users);
> > if (ret != EOK) {
> >    if (ret == ENOENT) {
> >        DEBUG(9, ("Deny user list is empty.\n"));
> >        ctx->deny_users = NULL;
> >    } else {
> >        DEBUG(1, ("confdb_get_string_as_list failed.\n"));
> >        goto failed;
> >    }
> > }
> > lists[0].ctx_list = &ctx->allow_users;
> > lists[1].ctx_list = &ctx->deny_users;
> > lists[2].ctx_list = &ctx->allow_groups;
> > lists[3].ctx_list = &ctx->deny_groups;
> > /* Groups */
> > ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
> >                                CONFDB_SIMPLE_ALLOW_GROUPS,
> >                                &ctx->allow_groups);
> > if (ret != EOK) {
>
> And then when iterating over the array, you can use sizeof(lists) as
> it's the same scope. Alternatively, you can use a last "sentinel"
> element that could be initialized to { NULL, NULL, NULL, NULL } and
> iterate until .name != NULL.
> > for (i = 0; i < 4; i++) {
> >    ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
> >                                    lists[i].option, &lists[i].orig_list);
> >     if (ret == ENOENT) {
> >        DEBUG(9, ("Allow group list is empty.\n"));
> >        ctx->allow_groups = NULL;
> >    } else {
> >        DEBUG(1, ("confdb_get_string_as_list failed.\n"));
> >        DEBUG(SSSDBG_FUNC_DATA, ("%s list is empty.\n", lists[i].name));
> >        *lists[i].ctx_list = NULL;
>
> Do you actually need to special case ENOENT here?
> simple_access_parse_names() handles empty input list just fine. If you
> want to double check also here, then I think you should continue
> instead.
> >    } else if (ret != EOK) {
> >        DEBUG(SSSDBG_CRIT_FAILURE, ("confdb_get_string_as_list failed.\n"));
> >         goto failed;
> >     }
> >
> > }
> >
> > ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
> >                                CONFDB_SIMPLE_DENY_GROUPS,
> >                                &ctx->deny_groups);
> >
> > if (ret != EOK) {
> >    if (ret == ENOENT) {
> >        DEBUG(9, ("Deny user list is empty.\n"));
> >        ctx->deny_groups = NULL;
> >    } else {
> >        DEBUG(1, ("confdb_get_string_as_list failed.\n"));
> >    ret = simple_access_parse_names(ctx, bectx, lists[i].orig_list,
> >                                    lists[i].ctx_list);
> >    if (ret != EOK) {
> >        DEBUG(SSSDBG_CRIT_FAILURE, ("Unable to parse %s list [%d]: %s\n",
> >                                    lists[i].name, ret, sss_strerror(ret)));
> >         goto failed;
> >     }
> > }
> >
> > --
> > 1.7.11.7

Thanks for the review. New patch is attached.

[Jakub Hrozek]

On Tue, Aug 06, 2013 at 03:04:28PM +0200, Pavel Březina wrote:
> On 08/06/2013 01:50 PM, Jakub Hrozek wrote:
> > On Mon, Jul 29, 2013 at 10:56:12AM +0200, Pavel Březina wrote:
> > > On 07/26/2013 11:58 AM, Jakub Hrozek wrote:
> > > > On Fri, Jul 26, 2013 at 11:49:37AM +0200, Pavel Březina wrote:
> > > > > https://fedorahosted.org/sssd/ticket/2026
> > > > >
> > > > > I created a separate ticket to support subdomain users and groups in
> > > > > allow/deny lists.
> > > > > https://fedorahosted.org/sssd/ticket/2034
> > > > >
> > > > > There is a bug in PAC responder at the moment which makes simple access
> > > > > provider to always deny access to subdomain users. PAC responder currently
> > > > > fails to resolve group membership, thus we get error in access provider when
> > > > > looking up groups and access is denied. Jakub will look into this.
> > > >
> > > > Can you also amend the unit tests for the simple access provider?
> > >
> > > Sure.
> > > Patches are attached.
> >
> > Hi, only some nitpicks now:
> > > @@ -103,66 +169,47 @@ int sssm_simple_access_init(struct be_ctx *bectx, struct bet_ops **ops,
> > >  {
> > >      int ret = EINVAL;
> > >      struct simple_ctx *ctx;
> > > int i;
> > > struct {
> > >    const char *name;
> > >    const char *option;
> > >    char **orig_list;
> > >    char ***ctx_list;
> > > } lists[4] = {{"Allow users", CONFDB_SIMPLE_ALLOW_USERS, NULL, NULL},
> > >              {"Deny users", CONFDB_SIMPLE_DENY_USERS, NULL, NULL},
> > >              {"Allow groups", CONFDB_SIMPLE_ALLOW_GROUPS, NULL, NULL},
> > >              {"Deny groups", CONFDB_SIMPLE_DENY_GROUPS, NULL, NULL}};
> >
> > I don't think you need to hardcode the size here, just lists[] would do.
> > >  ctx = talloc_zero(bectx, struct simple_ctx);
> > >  if (ctx == NULL) {
> > >    DEBUG(1, ("talloc_zero failed.\n"));
> > >    DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n"));
> > >    return ENOMEM;
> > >
> > > }
> > >
> > > ctx->domain = bectx->domain;
> > >    ctx->be_ctx = bectx;
> > > /* Users */
> > > ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
> > >                                CONFDB_SIMPLE_ALLOW_USERS,
> > >                                &ctx->allow_users);
> > > if (ret != EOK) {
> > >    if (ret == ENOENT) {
> > >        DEBUG(9, ("Allow user list is empty.\n"));
> > >        ctx->allow_users = NULL;
> > >    } else {
> > >        DEBUG(1, ("confdb_get_string_as_list failed.\n"));
> > >        goto failed;
> > >    }
> > > }
> > > ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
> > >                                CONFDB_SIMPLE_DENY_USERS,
> > >                                &ctx->deny_users);
> > > if (ret != EOK) {
> > >    if (ret == ENOENT) {
> > >        DEBUG(9, ("Deny user list is empty.\n"));
> > >        ctx->deny_users = NULL;
> > >    } else {
> > >        DEBUG(1, ("confdb_get_string_as_list failed.\n"));
> > >        goto failed;
> > >    }
> > > }
> > > lists[0].ctx_list = &ctx->allow_users;
> > > lists[1].ctx_list = &ctx->deny_users;
> > > lists[2].ctx_list = &ctx->allow_groups;
> > > lists[3].ctx_list = &ctx->deny_groups;
> > > /* Groups */
> > > ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
> > >                                CONFDB_SIMPLE_ALLOW_GROUPS,
> > >                                &ctx->allow_groups);
> > > if (ret != EOK) {
> >
> > And then when iterating over the array, you can use sizeof(lists) as
> > it's the same scope. Alternatively, you can use a last "sentinel"
> > element that could be initialized to { NULL, NULL, NULL, NULL } and
> > iterate until .name != NULL.
> > > for (i = 0; i < 4; i++) {
> > >    ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
> > >                                    lists[i].option, &lists[i].orig_list);
> > >    if (ret == ENOENT) {
> > >        DEBUG(9, ("Allow group list is empty.\n"));
> > >        ctx->allow_groups = NULL;
> > >    } else {
> > >        DEBUG(1, ("confdb_get_string_as_list failed.\n"));
> > >        DEBUG(SSSDBG_FUNC_DATA, ("%s list is empty.\n", lists[i].name));
> > >        *lists[i].ctx_list = NULL;
> >
> > Do you actually need to special case ENOENT here?
> > simple_access_parse_names() handles empty input list just fine. If you
> > want to double check also here, then I think you should continue
> > instead.
> > >    } else if (ret != EOK) {
> > >        DEBUG(SSSDBG_CRIT_FAILURE, ("confdb_get_string_as_list failed.\n"));
> > >        goto failed;
> > >    }
> > >
> > > }
> > >
> > > ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
> > >                                CONFDB_SIMPLE_DENY_GROUPS,
> > >                                &ctx->deny_groups);
> > >
> > > if (ret != EOK) {
> > >    if (ret == ENOENT) {
> > >        DEBUG(9, ("Deny user list is empty.\n"));
> > >        ctx->deny_groups = NULL;
> > >    } else {
> > >        DEBUG(1, ("confdb_get_string_as_list failed.\n"));
> > >    ret = simple_access_parse_names(ctx, bectx, lists[i].orig_list,
> > >                                    lists[i].ctx_list);
> > >    if (ret != EOK) {
> > >        DEBUG(SSSDBG_CRIT_FAILURE, ("Unable to parse %s list [%d]: %s\n",
> > >                                    lists[i].name, ret, sss_strerror(ret)));
> > >        goto failed;
> > >    }
> > > }
> > >
> > > --
> > > 1.7.11.7
>
> Thanks for the review. New patch is attached.

Thank you, these patches look good to me and work fine (I only tested
against IPA, but I don't think that the backend matters much for these
patches)

[Jakub Hrozek]

On Wed, Aug 07, 2013 at 01:14:14PM +0200, Jakub Hrozek wrote:
> On Tue, Aug 06, 2013 at 03:04:28PM +0200, Pavel Březina wrote:
> > On 08/06/2013 01:50 PM, Jakub Hrozek wrote:
> > > On Mon, Jul 29, 2013 at 10:56:12AM +0200, Pavel Březina wrote:
> > > > On 07/26/2013 11:58 AM, Jakub Hrozek wrote:
> > > > > On Fri, Jul 26, 2013 at 11:49:37AM +0200, Pavel Březina wrote:
> > > > > > https://fedorahosted.org/sssd/ticket/2026
> > > > > >
> > > > > > I created a separate ticket to support subdomain users and groups in
> > > > > > allow/deny lists.
> > > > > > https://fedorahosted.org/sssd/ticket/2034
> > > > > >
> > > > > > There is a bug in PAC responder at the moment which makes simple access
> > > > > > provider to always deny access to subdomain users. PAC responder currently
> > > > > > fails to resolve group membership, thus we get error in access provider when
> > > > > > looking up groups and access is denied. Jakub will look into this.
> > > > >
> > > > > Can you also amend the unit tests for the simple access provider?
> > > >
> > > > Sure.
> > > > Patches are attached.
> > >
> > > Hi, only some nitpicks now:
> > > > @@ -103,66 +169,47 @@ int sssm_simple_access_init(struct be_ctx *bectx, struct bet_ops **ops,
> > > >  {
> > > >      int ret = EINVAL;
> > > >      struct simple_ctx *ctx;
> > > > int i;
> > > > struct {
> > > >    const char *name;
> > > >    const char *option;
> > > >    char **orig_list;
> > > >    char ***ctx_list;
> > > > } lists[4] = {{"Allow users", CONFDB_SIMPLE_ALLOW_USERS, NULL, NULL},
> > > >              {"Deny users", CONFDB_SIMPLE_DENY_USERS, NULL, NULL},
> > > >              {"Allow groups", CONFDB_SIMPLE_ALLOW_GROUPS, NULL, NULL},
> > > >              {"Deny groups", CONFDB_SIMPLE_DENY_GROUPS, NULL, NULL}};
> > >
> > > I don't think you need to hardcode the size here, just lists[] would do.
> > > >  ctx = talloc_zero(bectx, struct simple_ctx);
> > > >  if (ctx == NULL) {
> > > >    DEBUG(1, ("talloc_zero failed.\n"));
> > > >    DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n"));
> > > >    return ENOMEM;
> > > >
> > > > }
> > > >
> > > > ctx->domain = bectx->domain;
> > > >    ctx->be_ctx = bectx;
> > > > /* Users */
> > > > ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
> > > >                                CONFDB_SIMPLE_ALLOW_USERS,
> > > >                                &ctx->allow_users);
> > > > if (ret != EOK) {
> > > >    if (ret == ENOENT) {
> > > >        DEBUG(9, ("Allow user list is empty.\n"));
> > > >        ctx->allow_users = NULL;
> > > >    } else {
> > > >        DEBUG(1, ("confdb_get_string_as_list failed.\n"));
> > > >        goto failed;
> > > >    }
> > > > }
> > > > ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
> > > >                                CONFDB_SIMPLE_DENY_USERS,
> > > >                                &ctx->deny_users);
> > > > if (ret != EOK) {
> > > >    if (ret == ENOENT) {
> > > >        DEBUG(9, ("Deny user list is empty.\n"));
> > > >        ctx->deny_users = NULL;
> > > >    } else {
> > > >        DEBUG(1, ("confdb_get_string_as_list failed.\n"));
> > > >        goto failed;
> > > >    }
> > > > }
> > > > lists[0].ctx_list = &ctx->allow_users;
> > > > lists[1].ctx_list = &ctx->deny_users;
> > > > lists[2].ctx_list = &ctx->allow_groups;
> > > > lists[3].ctx_list = &ctx->deny_groups;
> > > > /* Groups */
> > > > ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
> > > >                                CONFDB_SIMPLE_ALLOW_GROUPS,
> > > >                                &ctx->allow_groups);
> > > > if (ret != EOK) {
> > >
> > > And then when iterating over the array, you can use sizeof(lists) as
> > > it's the same scope. Alternatively, you can use a last "sentinel"
> > > element that could be initialized to { NULL, NULL, NULL, NULL } and
> > > iterate until .name != NULL.
> > > > for (i = 0; i < 4; i++) {
> > > >    ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
> > > >                                    lists[i].option, &lists[i].orig_list);
> > > >    if (ret == ENOENT) {
> > > >        DEBUG(9, ("Allow group list is empty.\n"));
> > > >        ctx->allow_groups = NULL;
> > > >    } else {
> > > >        DEBUG(1, ("confdb_get_string_as_list failed.\n"));
> > > >        DEBUG(SSSDBG_FUNC_DATA, ("%s list is empty.\n", lists[i].name));
> > > >        *lists[i].ctx_list = NULL;
> > >
> > > Do you actually need to special case ENOENT here?
> > > simple_access_parse_names() handles empty input list just fine. If you
> > > want to double check also here, then I think you should continue
> > > instead.
> > > >    } else if (ret != EOK) {
> > > >        DEBUG(SSSDBG_CRIT_FAILURE, ("confdb_get_string_as_list failed.\n"));
> > > >        goto failed;
> > > >    }
> > > >
> > > > }
> > > >
> > > > ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
> > > >                                CONFDB_SIMPLE_DENY_GROUPS,
> > > >                                &ctx->deny_groups);
> > > >
> > > > if (ret != EOK) {
> > > >    if (ret == ENOENT) {
> > > >        DEBUG(9, ("Deny user list is empty.\n"));
> > > >        ctx->deny_groups = NULL;
> > > >    } else {
> > > >        DEBUG(1, ("confdb_get_string_as_list failed.\n"));
> > > >    ret = simple_access_parse_names(ctx, bectx, lists[i].orig_list,
> > > >                                    lists[i].ctx_list);
> > > >    if (ret != EOK) {
> > > >        DEBUG(SSSDBG_CRIT_FAILURE, ("Unable to parse %s list [%d]: %s\n",
> > > >                                    lists[i].name, ret, sss_strerror(ret)));
> > > >        goto failed;
> > > >    }
> > > > }
> > > >
> > > > --
> > > > 1.7.11.7
> >
> > Thanks for the review. New patch is attached.
>
> Thank you, these patches look good to me and work fine (I only tested
> against IPA, but I don't think that the backend matters much for these
> patches)

Pushed to master and sssd-1-10