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
--
Stephen Gallagher
RHCE 804006346421761
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
--- a/src/config/SSSDConfig.py
+++ b/src/config/SSSDConfig.py
@@ -149,6 +149,9 @@ option_strings = {
# [provider/ldap/auth]
'ldap_pwd_policy' : _('Policy to evaluate the password
expiration'),
+
^^^^
unneeded whitespace
+ # [provider/ldap/access]
+ 'ldap_access_filter' : _('LDAP filter to determine access
privileges'),
# [provider/simple/access]
'simple_allow_users' : _('Comma separated list of allowed users'),
diff --git a/src/providers/ldap/ldap_init.c
b/src/providers/ldap/ldap_init.c
index 917ece0cbc5f3fbdf3d63603cac2af3a0a16918d..639aba19585ba117a16384d186ff744c25afa65a
100644
+int sssm_ldap_access_init(struct be_ctx *bectx,
+ struct bet_ops **ops,
+ void **pvt_data)
+{
+ int ret;
+ struct sdap_access_ctx *access_ctx;
+
+ access_ctx = talloc_zero(bectx, struct sdap_access_ctx);
+ if(access_ctx == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ ret = sssm_ldap_id_init(bectx, ops, (void **)&access_ctx->id_ctx);
+ if (ret != EOK) {
+ DEBUG(1, ("sssm_ldap_id_init failed.\n"));
+ goto done;
+ }
I think you need to modify sssm_ldap_id_init() to detect if it was
already called once and return the already allocated sdap_ctx. Currently
a new context is allocated and the global sdap handle is not shared
between the id and the access provider.
+
+ access_ctx->filter =
dp_opt_get_cstring(access_ctx->id_ctx->opts->basic,
+ SDAP_ACCESS_FILTER);
+ if (access_ctx->filter == NULL) {
+ /* It's okay if this is NULL. In that case we will simply act
+ * like the 'deny' provider.
+ */
+ DEBUG(0, ("Warning: access_provider=ldap set, "
+ "but no ldap_access_filter configured. "
+ "All domain users will be denied access."));
missing \n
+ }
+
+ *ops = &sdap_access_ops;
+ *pvt_data = access_ctx;
+
+done:
+ if (ret != EOK) {
+ talloc_free(access_ctx);
+ }
+ return ret;
+}
+static struct tevent_req *sdap_access_send(TALLOC_CTX *mem_ctx,
+ struct tevent_context *ev,
+ struct be_req *breq,
+ const char *username)
I have made the expirence that it is easier to reuse request if they do
not depend on breq. Instead use be_ctx and access_ctx->filter.
+ /* Perform online operation */
+ basedn = ldb_msg_find_attr_as_string(res->msgs[0],
+ SYSDB_ORIG_DN,
+ NULL);
+ if(basedn == NULL) {
+ DEBUG(1,("Could not find originalDN for user [%s]\n",
+ state->username));
+ ret = EIO;
+ state->pam_status = PAM_SYSTEM_ERR;
+ goto done;
+ }
+
+ state->basedn = talloc_strdup(state, basedn);
+ if (state->basedn == NULL) {
+ DEBUG(1, ("Could not allocate memory for originalDN\n"));
+ ret = ENOMEM;
+ state->pam_status = PAM_SYSTEM_ERR;
+ goto done;
+ }
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?
+
+ 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.