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 <jhrozek(a)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.
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. If it is old style shall we open a ticket to
refactor *_send calls using the old style?
Nevertheless ACK to the patches.
bye,
Sumit