[SSSD] [PATCH] simple access provider: allow fully qualified names

Pavel Březina pbrezina at redhat.com
Tue Aug 6 13:04:28 UTC 2013


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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-simple-access-provider-allow-fully-qualified-names.patch
Type: text/x-patch
Size: 6630 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130806/08234d98/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-add-simple-access-provider-init-test.patch
Type: text/x-patch
Size: 5119 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130806/08234d98/attachment-0001.bin>


More information about the sssd-devel mailing list