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

Jakub Hrozek jhrozek at redhat.com
Tue Jun 2 13:42:56 UTC 2015


On Mon, Jun 01, 2015 at 01:32:08PM +0200, Lukas Slebodnik wrote:
> 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

Done and pushed:
    * master: d0b7e5fcfca7d0db9e3d19be7b51f34d03d3d720
    * sssd-1-12: edc1538566e7034eea9b8d592206fbc236233dff



More information about the sssd-devel mailing list