[Samba] User's groups issue

[Jeremy Allison]

On Wed, Aug 27, 2008 at 03:25:36PM -0700, Jeremy Allison wrote:
> On Wed, Aug 27, 2008 at 02:41:25PM -0700, Ephi Dror wrote:
> > Hi Jerry,
> > 
> > I'm moving our discussion to technical list.
> > 
> > I just wanted to point out that I think the code in samlogon_cache.c is incorrect.
> > 
> > It uses the wrong tdb pointer.
> > 
> > Here is the right code to my opinion:
> > 
> > void netsamlogon_clear_cached_user(NET_USER_INFO_3 *user)
> > {
> >         DOM_SID sid;
> >         fstring key_str, sid_string;
> > 
> >         if (!netsamlogon_cache_init()) {
> >                 DEBUG(0,("netsamlogon_clear_cached_user: cannot open %s for write!\n", NETSAMLOGON_TDB));
> >                 return;
> >         }
> >         sid_copy(&sid, &user->dom_sid.sid);
> >         sid_append_rid(&sid, user->user_rid);
> > 
> >         fstr_sprintf(key_str, "%s", sid_to_string(sid_string, &sid));
> >         DEBUG(10, ("netsamlogon_clear_cached_user: clearing %s\n", key_str));
> > 
> >         tdb_delete_bystring(netsamlogon_tdb, key_str);
> > }
> > 
> > I tested it and it works like a Swiss Watch.
> > 
> > The reason this bug was hidden is because when we store we use TDB_REPLACE
> >            tdb_store_bystring(netsamlogon_tdb, keystr, data, TDB_REPLACE)
> 
> Yeah, this looks right to me. Looks like the code in
> netsamlogon_clear_cached_user() didn't get converted over
> when the interface changed. Thanks for the fix.

Hmmmm. Ok, *looks* right but may not be :-).

Looking closely at winbindd/winbindd_pam.c I find :

                netsamlogon_cache_store(name_user, info3);
                wcache_invalidate_samlogon(find_domain_from_name(name_domain), info3);

in two places (on successful logon). wcache_invalidate_samlogon()
calls netsamlogon_clear_cached_user() which is meant to scrub the
"UG/<sid>" and "U/<sid>" entries from the winbindd cache, not
delete the netsamlogon cache entry.

If we change to your code the wcache_invalidate_samlogon()
call would call netsamlogon_clear_cached_user() which would delete
the entry just added by netsamlogon_cache_store(), making these
two calls essentially a no-op.

Now I'm pretty sure wcache_invalidate_samlogon() should do
the deletion of the "UG/<sid>" and "U/<sid>" entries from the winbindd
cache itself, rather than leaving that code inside samlogon_cache.c
(where it is now) - especially as the reason for it being
there (might be called by smbd) is no longer valid.

But as I said, making the function look the way you have it
appears to never store anything inside NETSAMLOGON_TDB
(as we add then delete again).

How are you testing this ?

Jeremy