[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