On 09/29/2014 11:52 AM, Pavel Březina wrote:
On 09/26/2014 04:36 PM, Sumit Bose wrote:
On Fri, Sep 26, 2014 at 01:41:39PM +0200, Pavel Březina wrote:
On 09/26/2014 01:38 PM, Pavel Březina wrote:
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
Pavel, Jakub, thank you for the review.
>From dedbb5ae2faa13d396e84ce9911ab1e9c0246836 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@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.
yes, I feel quite bad about the missing test, but my plan is to add them
after the dealine
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.
done
Ack.
>From c09a8553d7371e46b84e980f38d017c7f84c2f87 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@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.
+ if (sdap_is_control_supported(sh, LDAP_CONTROL_X_DEREF)) {
+ DEBUG(SSSDBG_TRACE_INTERNAL, "Server supports OpenLDAP
deref\n");
+ state->deref_type = SDAP_DEREF_OPENLDAP;
+
+ subreq = sdap_x_deref_search_send(state, ev, opts, sh,
search_base,
+ filter, deref_attr, attrs,
maps,
+ num_maps, timeout);
+ if (!subreq) {
+ DEBUG(SSSDBG_OP_FAILURE, "Cannot start OpenLDAP deref
search\n");
+ goto fail;
+ }
+ } else {
+ DEBUG(SSSDBG_OP_FAILURE,
+ "Server does not support any known deref method!\n");
+ goto fail;
+ }
Would it be possible to use sdap_has_deref_support() instead of manually
testing the support for OpenLDAP deref or you can't use ASQ for some
reason?
done
We can do it post-rebase, though. Just please file a ticket.
>From 3097c76ba9498b90df69935d24dc9ce55f602937 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@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
Still 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
fixed
+ } 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?
no, the view name is an attribute of the host object so we must be able
to search it.
Ok. Typo is still there, I'm sending a trivial patch. Feel free to
squash it.
Ack.
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.
done
Ack.
Patch 6:
+ const char *obj_attrs[] = { SYSDB_OBJECTCLASS,
+ SYSDB_OVERRIDE_DN,
+ NULL};
Indentation.
fixed
+ if (ret != EOK) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Missing anchor in overide attributes.\n");
^ typo
fixed
+ 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.
I prefer it the other way, becasue I think it makes it more clear that
it is an error in all cases and only adds a special messages for the
ENOENT case.
Ok.
+ 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
fixed
+ obj_override_dn, override_dn_str);
+ ret = EINVAL;
+ goto done;
+ }
+
+ add_ref = false;
+ }
+ case SYSDB_MEMBER_GROUP:
+ ret = ldb_msg_add_string(msg, SYSDB_OBJECTCLASS,
+ SYSDB_OVERRIDE_USER_CLASS);
+ break;
Shouldn't it say SYSDB_OVERRIDE_GROUP_CLASS?
Patch 7: sysdb: sysdb_apply_default_override
Ack.
Ack stays, but can you clarify what is the intended use case of this
function in the commit message please?
Patch 8: views: get overrides during user and group lookups
Ack.
if (override_attrs != NULL) {
ret = sysdb_apply_default_override(state->user_dom->sysdb,
override_attrs,
state->obj_msg->dn);
if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "sysdb_apply_default_override
failed.\n");
goto fail;
}
}
I thought the sysdb_apply_default_override is used to override original
object in sysdb with information from the default view. This looks like
it is overwritten with data from whatever view we are using.
BTW I checked out your repo. The patches couldn't be applied to
master even
with the extdom patch here on the list.
I send a new version of the extdom patch as well all rebase on master, I
hope it will work now.
New version attached.
bye,
Sumit
I get this warning:
/home/pbrezina/workspace/sssd/src/providers/ipa/ipa_views.c: In function
'ipa_get_ad_override_done':
/home/pbrezina/workspace/sssd/src/providers/ipa/ipa_views.c:207:9:
error: format '%d' expects argument of type 'int', but argument 6 has
type 'size_t' [-Werror=format]
I'm sending patches for the typo and warning to speed things up.
From 07267a2c0a99d521a13a9c8ca0702af75ce97e7a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
Date: Mon, 29 Sep 2014 11:10:07 +0200
Subject: [PATCH 1/2] squash me into "IPA: add view support and get view name"
---
src/providers/ipa/ipa_init.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/providers/ipa/ipa_init.c b/src/providers/ipa/ipa_init.c
index d094ac136b26cdc684c0c3173f3d6825f6bac99d..5699fd86903384e7ddccc6bbc2f7cf286b9404c9 100644
--- a/src/providers/ipa/ipa_init.c
+++ b/src/providers/ipa/ipa_init.c
@@ -316,7 +316,7 @@ int sssm_ipa_id_init(struct be_ctx *bectx,
if (ret == ENOENT) {
DEBUG(SSSDBG_CRIT_FAILURE,
"Cannot find view name in the cache. " \
- "Will do online llokup later\n");
+ "Will do online lookup later\n");
} else {
DEBUG(SSSDBG_OP_FAILURE, "sysdb_get_view_name failed.\n");
goto done;
--
1.7.11.7
From 9f95cabb97b765a96ffa950daef6ccb1fd71e27c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
Date: Mon, 29 Sep 2014 12:04:33 +0200
Subject: [PATCH 2/2] squash me into "views: add ipa_get_ad_override_send()"
---
src/providers/ipa/ipa_views.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/providers/ipa/ipa_views.c b/src/providers/ipa/ipa_views.c
index 5d8ca3e9104c99d734f3eef195111a780c999afd..8cca4025187cb63ce11d37c1ef795d6675eeeed1 100644
--- a/src/providers/ipa/ipa_views.c
+++ b/src/providers/ipa/ipa_views.c
@@ -205,7 +205,7 @@ static void ipa_get_ad_override_done(struct tevent_req *subreq)
return;
} else if (reply_count > 1) {
DEBUG(SSSDBG_CRIT_FAILURE,
- "Found [%d] overrides for SID [%s], expected only 1.\n",
+ "Found [%zu] overrides for SID [%s], expected only 1.\n",
reply_count, state->obj_sid);
ret = EINVAL;
goto fail;
--
1.7.11.7
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
