On Fri, Sep 27, 2013 at 02:55:58PM +0200, Sumit Bose wrote:
> On Thu, Sep 26, 2013 at 10:16:50PM +0200, Jakub Hrozek wrote:
> > On Thu, Sep 26, 2013 at 04:01:17PM +0200, Jakub Hrozek wrote:
> > > On Thu, Sep 26, 2013 at 02:15:42PM +0200, Jakub Hrozek wrote:
> > > > Hi,
> > > >
> > > > the attached patches implement ticket #2070 where subdomain users have
> > > > POSIX attributes and the SSSD honors them.
> > > >
> > > > So far I only tested with AD provider, not with trust environment, but I
> > > > wanted to sent the patches for review anyway.
> > > >
> > > > The patches mostly amend the search filters and bases which presumed
> > > > strictly ID mapping in AD domains before. I also implemented Sumit's
> > > > idea on how to handle the SFU attributes as a new map. I think it's the
> > > > right thing to do, because if SFU is configured on the server, then the
> > > > POSIX clients should honour it if ID mapping is off.
> > > >
> > > > One question I have is whether we should explicitly document that the
> > > > POSIX attributes must be replicated in the man page?
> > >
> > > > From 88f2e35b3ef9cbf0655093d80c297ada0da47e09 Mon Sep 17 00:00:00 2001
> > > > From: Jakub Hrozek <jhro...@redhat.com>
> > > > Date: Wed, 25 Sep 2013 23:05:48 +0200
> > > > Subject: [PATCH 4/5] AD: Add new option map ad_2008r2_sfu_user_map
> > >
> > > As discussed on the phone with Sumit, the way the map is selected is
> > > wrong and would only allow members who are added using the SFU dialog to
> > > be searched.
> >
> > New patches are attached, rebased on top of Sumit's capath patches. The
> > patch with the map is added, instead there is a mini-patch that
> > documents that POSIX attributes must be replicated to GC for trusted
> > users with POSIX attributes to be reachable.
>
> Patches are working as expected.
>
> ....
>
> > +
> > + if (use_id_mapping) {
> > + /* When mapping IDs or looking for SIDs, we don't want to limit
> > + * ourselves to groups with a GID value. But there must be a SID
> > to map
> > + * from.
> > + */
> > + state->base_filter = talloc_asprintf_append(state->base_filter,
> > + "(%s=*))",
> > +
> > opts->group_map[SDAP_AT_GROUP_OBJECTSID].name);
> > + } else {
> > + /* When not ID-mapping, make sure there is a non-NULL UID */
> > + state->base_filter = talloc_asprintf_append(state->base_filter,
> > + "(&(%s=*)(!(%s=0))))",
> > +
> > opts->group_map[SDAP_AT_GROUP_GID].name,
> > +
> > opts->group_map[SDAP_AT_GROUP_GID].name);
> > + }
> > + if (!state->base_filter) {
> > + talloc_zfree(req);
> > + return NULL;
> > + }
> > +
> > +
>
> I think calling talloc_zfree(req) and returning NULL in a *_send
> function is old style. Since it is used in other parts of the patched
> calls as well I think it is ok.
Yes, I wanted to keep the same style in the function.
The other thing I was wondering about was whether to include a unified
function to construct the filter. Currently the same or very similar filter
is used for user requests and group requests (just s/UID/GID). But then
I didn't want to touch more than needed this close to a release.
>
> But that brings me to two questions. First, is this really old style or
> we should recommend to only return in *_send if the request itself
> cannot be allocated and call tevent_req_error() and tevent_req_post()
> for any other errors.
Yes, I think it's much better to end the request with tevent_req_error.
The caller then knows more precisely what went wrong.
> If it is old style shall we open a ticket to
> refactor *_send calls using the old style?
Sure, feel free to open such ticket. But I'm not sure how realistic it
would be to mass-convert all _send() functions... I wonder if instead we
should be converting one function at a time (with a separate patch) when
we touch such function? But then we'd never convert the stable parts of
the SSSD...
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
