[SSSD] [PATCHES] Add backend part of user views

Pavel Březina pbrezina at redhat.com
Fri Sep 26 11:38:49 UTC 2014


On Wed, Sep 24, 2014 at 10:25:22AM +0200, Sumit Bose wrote:
> Hi,
>
> this patches contain the IPA provider part of
> https://fedorahosted.org/sssd/ticket/2375  as described in the FreeIPA
> design page
> http://www.freeipa.org/page/V4/Migrating_existing_environments_to_Trust
>
> Patches 0001, 0006 and 0007 contain new sysdb interfaces for views and
> overrides. 0002 and 0003 refactor some already existing calls.
>
> 0004 adds the initial support for views and overrides and reads the view
> name for the client from the FreeIPA server or sets it to 'default' when
> running in ipa-server-mode. It adds some new options which currently
> misses man page entries and are not added to the python config API. I
> did this on purpose for the time being because I wanted to see first if
> the list is complete or if some options ca be dropped.
>
> 0005 adds the request to get a specific override from the FreeIPA server
> and finally in 0008 the overrides are read during a request and saves or
> applied.
>
> This patches must be applied on to of the extdom patch I send yesterday.
> To add and manage views and override on the server side please have a
> look at Tomas's patch set on the freeipa-devel list
> (https://www.redhat.com/archives/freeipa-devel/2014-September/msg00336.html).
>
> bye,
> Sumit

> From dedbb5ae2faa13d396e84ce9911ab1e9c0246836 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Tue, 16 Sep 2014 15:18:53 +0200
> Subject: [PATCH 1/8] sysdb: add sysdb_update_view_name()

ACK

A unit test for the functions would be nice in order to avoid decreasing
the code coverage of the sysdb module, but I realize we have a deadline.
We should file a ticket, though.

It would also be nice to add a comment that it's not possible to have a
view w/o a name but we are trying to be on the safe side b/c name is not
MUST as we discussed on IRC with Sumit.

> From c09a8553d7371e46b84e980f38d017c7f84c2f87 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Tue, 16 Sep 2014 15:22:08 +0200
> Subject: [PATCH 2/8] Add sdap_deref_search_with_filter_send()

I would prefer to also add a matching _done and _recv functions, even if
they were wrappers around existing functions. But tevent coding style is
important to follow, we already had bugs about reusing a single tevent
callbacks for multiple requests.

We can do it post-rebase, though. Just please file a ticket.


> From 3097c76ba9498b90df69935d24dc9ce55f602937 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Fri, 19 Sep 2014 10:26:01 +0200
> Subject: [PATCH 3/8] IPA: make IPA ID context available to extdom client code

ACK

...aaand I stopped here.

That was Jakub's part of the review.

Patch 4: IPA: add view support and get view name

>          if (ret != EOK) {
> -            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to set SRV lookup plugin "
> -                                        "[%d]: %s\n", ret, strerror(ret));
> -            goto done;
> +            if (ret == ENOENT) {
> +                DEBUG(SSSDBG_CRIT_FAILURE,
> +                      "Cannot find view name in the cache. " \
> +                      "Will do online llokup later\n");
                                          ^ typo
> +            } else {
> +                DEBUG(SSSDBG_OP_FAILURE, "sysdb_get_view_name failed.\n");
> +                goto done;
> +            }
> +        }

>      ctx->ranges_search_bases = id_ctx->ipa_options->ranges_search_bases;
> +    ctx->host_search_bases = id_ctx->ipa_options->host_search_bases;
>      ctx->configured_explicit = configured_explicit;

This change seems to be unrelated to this patch. Shouldn't it be a 
separate commit?

Patch 5: views: add ipa_get_ad_override_send()

Looks good, but can you add some more tracing debug messages there? For 
example what view are we searching for and that an override was found.

Patch 6:

> +    const char *obj_attrs[] = { SYSDB_OBJECTCLASS,
> +                              SYSDB_OVERRIDE_DN,
> +                                   NULL};

Indentation.

> +        if (ret != EOK) {
> +            DEBUG(SSSDBG_CRIT_FAILURE,
> +                  "Missing anchor in overide attributes.\n");
                                            ^ typo
> +            ret = EINVAL;
> +            goto done;
> +        }

> +    if (ret != EOK) {
> +        if (ret == ENOENT) {
> +            DEBUG(SSSDBG_CRIT_FAILURE, "Object to override does not exists.\n");
> +        } else {
> +            DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_entry failed.\n");
> +        }
> +        goto done;
> +    }

Can you use if (ret == ENOENT) else if (ret != EOK) here? I think it is 
simpler.

> +    if (obj_override_dn != NULL) {
> +        if (strcmp(obj_override_dn, override_dn_str) != 0) {
> +            DEBUG(SSSDBG_CRIT_FAILURE,
> +                  "Existing [%s] and new [%s] overid DN do not match.\n",
                                                  ^ typo
> +                   obj_override_dn, override_dn_str);
> +            ret = EINVAL;
> +            goto done;
> +        }
> +
> +        add_ref = false;
> +    }

Patch 7: sysdb: sysdb_apply_default_override

Ack.

Patch 8: views: get overrides during user and group lookups

Ack.




More information about the sssd-devel mailing list