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)