[SSSD] [PATCH] ldap: use proper sysdb name in groups_by_user_done()

Lukas Slebodnik lslebodn at redhat.com
Mon Jun 1 10:58:15 UTC 2015


On (01/06/15 10:24), Sumit Bose wrote:
>On Sat, May 30, 2015 at 09:25:58PM +0200, Lukas Slebodnik wrote:
>> On (29/05/15 18:20), Sumit Bose wrote:
>> >Hi,
>> >
>> >I came across this issue while testing one of Jakub's patches. You can
>> >reproduce this e.g. with the AD provider when joined to a AD forest with
>> >multiple domains. Now calling 'id -G' for a user from a sub-domain will
>> >first not show all groups only after a second call of 'id -G'. In the
>> >domain log object-not-found error messages can be found.
>> >
>> >bye,
>> >Sumit
>> 
>> >From 6f34fdd5426967f7feb11801d3de991cdaa9ee7b Mon Sep 17 00:00:00 2001
>> >From: Sumit Bose <sbose at redhat.com>
>> >Date: Fri, 29 May 2015 16:37:54 +0200
>> >Subject: [PATCH] ldap: use proper sysdb name in groups_by_user_done()
>> >
>> >In a recent change set_initgroups_expire_attribute() was added to
>> >groups_by_user_done() to make sure that the initgroups timeout is only
>> >added to the user object until all groups added to the cache.
>> >
>> >This change (and the original code in groups_by_user_done() as well)
>> >didn't took sub-domain users into account where the name in sysdb might
>> >different form the original request and the domain is not the configured
>> >domain. This patch tries to ensure that the right name and domain are
>> >used.
>> >---
>> > src/providers/ldap/ldap_id.c | 15 ++++++++++++---
>> > 1 file changed, 12 insertions(+), 3 deletions(-)
>> >
>> >diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c
>> >index 6631237fffb2ac1901b1bbbf27ce6bd1bf48a244..1473ab0e12201840a93b521a4d336e41e919effd 100644
>> >--- a/src/providers/ldap/ldap_id.c
>> >+++ b/src/providers/ldap/ldap_id.c
>> >@@ -1140,6 +1140,7 @@ static void groups_by_user_done(struct tevent_req *subreq)
>> >                                                      struct groups_by_user_state);
>> >     int dp_error = DP_ERR_FATAL;
>> >     int ret;
>> >+    char *cname;
>> > 
>> >     ret = sdap_get_initgr_recv(subreq);
>> >     talloc_zfree(subreq);
>> >@@ -1163,16 +1164,24 @@ static void groups_by_user_done(struct tevent_req *subreq)
>> >         return;
>> >     }
>> > 
>> >+    /* state->name is still the name used for the original request. The cached
>> >+     * object might have a different name, e.g. a fully-qualified name. */
>> >+    ret = sysdb_get_real_name(state, state->domain, state->name, &cname);
>> >+    if (ret != EOK) {
>> >+        cname = state->name;
>> >+        DEBUG(SSSDBG_OP_FAILURE, "Failed to canonicalize name, using [%s].\n",
>> >+                                 cname);
>> >+    }
>> >+
>> >     if (ret == ENOENT && state->noexist_delete == true) {
>> >-        ret = sysdb_delete_user(state->ctx->be->domain, state->name, 0);
>> >+        ret = sysdb_delete_user(state->domain, cname, 0);
>> >         if (ret != EOK && ret != ENOENT) {
>> >             tevent_req_error(req, ret);
>> >             return;
>> >         }
>> >     }
>> > 
>> >-    ret = set_initgroups_expire_attribute(state->ctx->be->domain,
>> >-                                          state->name);
>> >+    ret = set_initgroups_expire_attribute(state->domain, cname);
>> >     if (ret != EOK) {
>> >         state->dp_error = DP_ERR_FATAL;
>> >         tevent_req_error(req, ret);
>> Bug is fixed,
>> but there is a compiler warning
>> src/providers/ldap/ldap_id.c: In function 'groups_by_user_done':
>> src/providers/ldap/ldap_id.c:1154:5: warning: passing argument 4 of 'sysdb_get_real_name' from incompatible pointer type [enabled by default]
>>      ret = sysdb_get_real_name(state, state->domain, state->name, &cname);
>>      ^
>> In file included from src/providers/ldap/ldap_id.c:31:0:
>> ./src/db/sysdb.h:372:9: note: expected 'const char **' but argument is of type 'char **'
>>  errno_t sysdb_get_real_name(TALLOC_CTX *mem_ctx,
>>          ^
>> src/providers/ldap/ldap_id.c:1156:15: warning: assignment discards 'const' qualifier from pointer target type [enabled by default]
>>          cname = state->name;
>>                ^
>> 
>> Just change type of cname from "char *" -> "const char *"
>> 
>> It would be good to add ticket to commit message
>> (related?) https://fedorahosted.org/sssd/ticket/2634
>
>Thank you for the review, I added your comments, new version attached.
>

>From 48910b4fa13bdff962d91becc509dc3aa0c5493d Mon Sep 17 00:00:00 2001
>From: Sumit Bose <sbose at redhat.com>
>Date: Fri, 29 May 2015 16:37:54 +0200
>Subject: [PATCH] ldap: use proper sysdb name in groups_by_user_done()
>
>In a recent change set_initgroups_expire_attribute() was added to
>groups_by_user_done() to make sure that the initgroups timeout is only
>added to the user object until all groups added to the cache.
>
>This change (and the original code in groups_by_user_done() as well)
>didn't took sub-domain users into account where the name in sysdb might
>different form the original request and the domain is not the configured
>domain. This patch tries to ensure that the right name and domain are
>used.
>
>Related to https://fedorahosted.org/sssd/ticket/2634
>---

Thank you for changes
and we should backport the patch to downstream.

http://sssd-ci.duckdns.org/logs/job/16/48/summary.html

ACK

LS



More information about the sssd-devel mailing list