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(a)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/