[SSSD] [PATCH] Add ldap_access_filter option

Sumit Bose sbose at redhat.com
Fri May 14 18:55:10 UTC 2010


On Fri, May 14, 2010 at 02:08:48PM -0400, Stephen Gallagher wrote:
> On 05/14/2010 09:57 AM, Sumit Bose wrote:
> >On Thu, May 13, 2010 at 02:02:57PM -0400, Stephen Gallagher wrote:
> >>On 05/13/2010 12:07 PM, Simo Sorce wrote:
> >>>On Thu, 13 May 2010 10:56:35 -0400
> >>>Stephen Gallagher<sgallagh at redhat.com>   wrote:
> >>>
> >>>>+    ret = sysdb_attrs_add_uint32(attrs, SYSDB_LDAP_ACCESS,
> >>>>+                                 state->pam_status == PAM_SUCCESS ?
> >>>>+                                                      true :
> >>>>+                                                      false);
> >>>
> >>>Just a nitpick, but normally in LDAP true false values are stored as
> >>>the strings "TRUE" or "FALSE" (upper case). It makes it clearer this is
> >>>a boolean attribute than storing 0 vs a random value (usually 1).
> >>>
> >>>Simo.
> >>>
> >>
> >>New patch attached. I added a new sysdb function:
> >>sysdb_attrs_add_bool() that will handle this in an ldb-compatible
> >>way.
> >>
> >>I also corrected your other comment about passing an explicit event context.
> >>
> >>Finally, this patch adds the new option to the SSSDConfig API and
> >>manpages (which I forgot to do originally).
> >>
> >>Please re-review.
> >>
> >
> >Comments are in-line.
> >
> >bye.
> >Sumit
> >
> >>--
> >
> >Wouldn't it be enough to set state->basedn in the first place, because
> >for the memory hierarchy point of view res is already a child of state?
> 
> Thanks, but you actually alerted me that I'd forgotten the
> talloc_free(res) I'd meant to do here. No sense in carrying that
> memory around until the end of the request. I have to
> talloc_strdup() here, because I can't trust that this is actually an
> allocated string I can talloc_steal().
> 
> I could do omit the temporary string, but I prefer to be able to
> tell whether an error was from LDB or talloc.

fair enough

> >
> >>+
> >>+    if (found) {
> >>+        /* Save "allow" to the cache for future offline
> >>+         * access checks.
> >>+         */
> >>+        DEBUG(6, ("Access granted by online lookup\n"));
> >>+        state->pam_status = PAM_SUCCESS;
> >>+    }
> >>+    else {
> >>+        /* Save "disallow" to the cache for future offline
> >>+         * access checks.
> >>+         */
> >>+        DEBUG(6, ("Access denied by online lookup\n"));
> >>+        state->pam_status = PAM_PERM_DENIED;
> >
> >I think it is not necessary to save "disallow" because if
> >SYSDB_LDAP_ACCESS is not set 'false' is used as a default.
> 
> It's necessary to save "disallow" because it's possible that we have
> previously saved "allow", and this needs to overwrite it. I could do
> a removal, but I prefer the explicit nature of having FALSE written
> here.
> 
> For example, the rule could be "memberOf=<some group>" and the user
> has been removed from that group on the server.
> 

yes, sorry, you are right.

ACK

bye,
Sumit

> 
> -- 
> Stephen Gallagher
> RHCE 804006346421761
> 
> Delivering value year after year.
> Red Hat ranks #1 in value among software vendors.
> http://www.redhat.com/promo/vendor/



More information about the sssd-devel mailing list