On Tue, Dec 08, 2015 at 04:57:02PM +0100, Petr Cech wrote:
On 12/07/2015 03:43 PM, Jakub Hrozek wrote:
>On Fri, Dec 04, 2015 at 04:33:51PM +0100, Petr Cech wrote:
>>So, I have choose the 2) option and the patch is attached.
>
>I think this patch goes in the right direction, but still needs more
>work. See inline.
Hello Jakub,
thank you for careful review.
>>And something more:
>> a.bak ... log before patch
>> b.bak ... log after patch
>>
>>Regards
>>
>>Petr
>
>> From 12f03f5510cdfc56efd6d24bf06db0ad0e6d6368 Mon Sep 17 00:00:00 2001
>>From: Petr Cech <pcech(a)redhat.com>
>>Date: Mon, 9 Nov 2015 09:51:05 -0500
>>Subject: [PATCH] IPA_PROVIDER: Explicit no handle of services
>>
>>Services for IPA subdomains aren't handled by SSSD. This patch
>>adds quick shortcut to avoid sending an LDAP query to cache.
>>
>>Resolves:
>>https://fedorahosted.org/sssd/ticket/2747
>>---
...
>>
>>+
>>+static bool is_object_overridable(struct be_acct_req *ar)
>>+{
>>+ bool ret = false;
>>+
>>+ switch (ar->entry_type & BE_REQ_TYPE_MASK) {
>>+ case BE_REQ_USER:
>>+ ret = true;
>>+ break;
>>+ case BE_REQ_GROUP:
>>+ ret = true;
>>+ break;
>
>I know what you tried to do here, but please keep in mind that other types
>of request can also yield user. At least BE_REQ_INITGROUPS, BE_REQ_BY_SECID,
>BE_REQ_USER_AND_GROUP, BE_REQ_BY_UUID, BE_REQ_BY_CERT.
I did cases for those REQs, but I don't know if it is all.
>We can also collapse the case: statements so that the switch is more
>compact.
Addressed.
...
>>diff --git a/src/providers/ipa/ipa_subdomains_id.c
b/src/providers/ipa/ipa_subdomains_id.c
>>index
472985d4ab4f785aa9c4af94bf8021829ca1c3c8..7966a763ee860bbf426f3e308a3f1457a8c8d5fa 100644
>>--- a/src/providers/ipa/ipa_subdomains_id.c
>>+++ b/src/providers/ipa/ipa_subdomains_id.c
>>@@ -915,6 +915,13 @@ errno_t get_object_from_cache(TALLOC_CTX *mem_ctx,
>> NULL };
>> char *name;
>>
>>+ if ((ar->entry_type & BE_REQ_TYPE_MASK) == BE_REQ_SERVICES) {
>>+ DEBUG(SSSDBG_OP_FAILURE,
>>+ "Services for IPA subdomains aren't handled by
SSSD.\n");
>>+ ret = ENOENT;
>>+ goto done;
>>+ }
>>+
>
>With adding is_object_overridable(), I don't think this branch is
>reachable.
I agree. I have been surprised that this message wasn't been in logs.
One style nitpick and then we can push :)
>> if (ar->filter_type == BE_FILTER_SECID) {
>> ret = sysdb_search_object_by_sid(mem_ctx, dom, ar->filter_value,
attrs,
>> &res);
Regards
Petr
From e024fd15dc9c6735b291f5566ba435da7fc496bd Mon Sep 17 00:00:00
2001
From: Petr Cech <pcech(a)redhat.com>
Date: Mon, 9 Nov 2015 09:51:05 -0500
Subject: [PATCH] IPA_PROVIDER: Explicit no handle of services
Function get_object_from_cache() does not handle services.
This patch adds quick shortcut to avoid sending an LDAP query
to cache.
Resolves:
https://fedorahosted.org/sssd/ticket/2747
---
src/providers/ipa/ipa_id.c | 34 +++++++++++++++++++++++++++++++++-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/src/providers/ipa/ipa_id.c b/src/providers/ipa/ipa_id.c
index e81ccb34dd6eb44618538593f5473fbe5e89d896..dd32d7cf5fb2e6b4793b0009924949801517a809
100644
--- a/src/providers/ipa/ipa_id.c
+++ b/src/providers/ipa/ipa_id.c
@@ -30,6 +30,31 @@
#include "providers/ldap/sdap_async.h"
#include "providers/ipa/ipa_id.h"
+static bool is_object_overridable(struct be_acct_req *ar)
+{
+ bool ret = false;
+
+ switch (ar->entry_type & BE_REQ_TYPE_MASK) {
+ case BE_REQ_USER: ret = true;
+ break;
+ case BE_REQ_GROUP: ret = true;
+ break;
+ case BE_REQ_INITGROUPS: ret = true;
+ break;
+ case BE_REQ_BY_SECID: ret = true;
+ break;
+ case BE_REQ_USER_AND_GROUP: ret = true;
+ break;
+ case BE_REQ_BY_UUID: ret = true;
+ break;
+ case BE_REQ_BY_CERT: ret = true;
+ break;
+ default: break;
+ }
We can make the code more compact with:
case BE_REQ_USER:
case BE_REQ_GROUP:
/* And also all the others.. */
ret = true;
break;
> +
> + return ret;
> +}
> +
> static const char *ipa_account_info_error_text(int ret, int *dp_error,
> const char *default_text)
> {