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

Lukas Slebodnik lslebodn at redhat.com
Mon Jun 1 11:32:08 UTC 2015


On (01/06/15 12:58), Lukas Slebodnik wrote:
>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
>
And we also have a ticket for this
https://fedorahosted.org/sssd/ticket/2663

It doesn't worth to send patch one more time.
So please update commit message befor pushing patch

LS



More information about the sssd-devel mailing list