[SSSD] [PRELIMINARY][PATCH] ifp users and groups

Pavel Březina pbrezina at redhat.com
Mon May 4 12:51:57 UTC 2015


On 04/08/2015 12:48 PM, Jakub Hrozek wrote:
> On Tue, Mar 24, 2015 at 12:05:47PM +0100, Pavel Březina wrote:
>> Hi,
>> I'm sending these incomplete patches for review just to get some feed back.
>>
>> What is missing:
>> - support for user's extraAttributes (although get invoker is there)
>> - ListByNameFilter for both groups and users
>
> The code is laid out well and reads good. I would like more non-error
> DEBUG messages so that we can follow the code if something goes wrong
> and some functions look like they could be made static. See particular
> comments inline.
>
>>  From 2a83625685b909189f309b47d93c6e1ad8e37d3d Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Wed, 21 Jan 2015 13:38:06 +0100
>> Subject: [PATCH 1/8] sbus: provide custom error names
>
> [...]
>
>> +#define SBUS_ERROR_INTERNAL "org.freedesktop.sssd.Error.Internal"
>> +#define SBUS_ERROR_NOT_FOUND "org.freedesktop.sssd.Error.NotFound"
>
> These look good to me, the errors are in the sssd namespace and as other
> D-Bus errors use singular.
>
>>  From 26c17e97d48d580be82feb56add73f11dbfa540a Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Thu, 29 Jan 2015 15:29:26 +0100
>> Subject: [PATCH 2/8] sbus: add sbus_opath_decompose[_exact]
>
> [...]
>
>> +errno_t
>> +sbus_opath_decompose_exact(TALLOC_CTX *mem_ctx,
>> +                           const char *object_path,
>> +                           const char *prefix,
>> +                           size_t expected,
>> +                           char ***_components)
>> +{
>> +    char **components;
>> +    size_t len;
>> +    errno_t ret;
>> +
>> +    ret = sbus_opath_decompose(mem_ctx, object_path, prefix,
>> +                               &components, &len);
>> +    if (ret != EOK) {
>> +        return ret;
>> +    }
>> +
>> +    if (len != expected) {
>> +        talloc_free(components);
>> +        return ERANGE;
>
> I think an internal error code would be appropriate here. If you think
> the error would be too specific, what about something like
> ERR_SBUS_INVALID_PATH that could be used for similar purposes?

Done.

>
>> +    }
>> +
>> +    if (_components != NULL) {
>> +        *_components = components;
>> +    }
>> +
>> +    return EOK;
>> +}
>> +
>>   const char *
>>   sbus_opath_strip_prefix(const char *object_path,
>>                           const char *prefix)
>> diff --git a/src/tests/cmocka/test_sbus_opath.c b/src/tests/cmocka/test_sbus_opath.c
>> index 4c57c56e7ebbcac3665c466e1f3d947f7bec92c4..a2b307b014013ac10d16a3ea48b061a0ad21d998 100644
>> --- a/src/tests/cmocka/test_sbus_opath.c
>> +++ b/src/tests/cmocka/test_sbus_opath.c
>
> Thanks for the tests.
>
>>  From 409238d8efc4cb6244a35854a221895ddee06926 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Wed, 18 Mar 2015 13:01:07 +0100
>> Subject: [PATCH 3/8] sbus: add a{sas} get invoker
>
> Looks good, but can you decorate sbus_invoke_get_aDOsasDE() with DEBUG
> messages for branches that migth fail?

There is actually no branch that can really fail. Only functions that 
"can" fail are D-Bus but it usually aborts. Graceful failures are only 
for signature assertions here which just won't happen if the code makes 
it through review (since it wouldn't work at all). Do you think it is 
worth it to obfuscate it with debug messages? I don't think we do it on 
other places.

>>  From a963811b98f3171f7f48cc4a0f48a952634a3436 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Thu, 15 Jan 2015 13:52:31 +0100
>> Subject: [PATCH 4/8] IFP: add org.freedesktop.sssd.infopipe.Users
>
> The interface is sane and conforms to the design page.
>
>> diff --git a/src/responder/ifp/ifp_users.c b/src/responder/ifp/ifp_users.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..9aec9291c8b4de55f8dfdba9f3b5f5ab36e64a45
>> --- /dev/null
>> +++ b/src/responder/ifp/ifp_users.c
>> @@ -0,0 +1,194 @@
>> +/*
>> +    Authors:
>> +        Pavel Březina <pbrezina at redhat.com>
>> +
>> +    Copyright (C) 2015 Red Hat
>> +
>> +    This program is free software; you can redistribute it and/or modify
>> +    it under the terms of the GNU General Public License as published by
>> +    the Free Software Foundation; either version 3 of the License, or
>> +    (at your option) any later version.
>> +
>> +    This program is distributed in the hope that it will be useful,
>> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +    GNU General Public License for more details.
>> +
>> +    You should have received a copy of the GNU General Public License
>> +    along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +*/
>> +
>> +#include <talloc.h>
>> +#include <tevent.h>
>> +#include <string.h>
>> +
>> +#include "db/sysdb.h"
>> +#include "util/util.h"
>> +#include "sbus/sssd_dbus_errors.h"
>> +#include "responder/common/responder.h"
>> +#include "responder/common/responder_cache_req.h"
>> +#include "responder/ifp/ifp_users.h"
>> +
>> +char * ifp_users_build_path_from_msg(TALLOC_CTX *mem_ctx,
>> +                                     struct sss_domain_info *domain,
>> +                                     struct ldb_message *msg)
>
> I wonder why this function isn't static and why it's in the header?
>
>> +{
>> +    const char *uid;
>> +
>> +    uid = ldb_msg_find_attr_as_string(msg, SYSDB_UIDNUM, NULL);
>> +
>> +    if (uid == NULL) {
>> +        return NULL;
>> +    }
>> +
>> +    return sbus_opath_compose(mem_ctx, IFP_PATH_USERS, domain->name, uid);
>> +}
>> +
>> +static void ifp_users_find_by_name_done(struct tevent_req *req);
>> +
>> +int ifp_users_find_by_name(struct sbus_request *sbus_req,
>> +                           void *data,
>> +                           const char *name)
>> +{
>> +    struct ifp_ctx *ctx;
>> +    struct tevent_req *req;
>> +
>> +    ctx = talloc_get_type(data, struct ifp_ctx);
>> +    if (ctx == NULL) {
>> +        DEBUG(SSSDBG_CRIT_FAILURE, "Invalid pointer!\n");
>> +        return ERR_INTERNAL;
>> +    }
>> +
>> +    req = cache_req_user_by_name_send(sbus_req, ctx->rctx->ev, ctx->rctx,
>> +                                      ctx->ncache, ctx->neg_timeout, 0,
>> +                                      NULL, name);
>
> Missing NULL check.
>

Done.

>> +
>> +    tevent_req_set_callback(req, ifp_users_find_by_name_done, sbus_req);
>> +
>> +    return EOK;
>> +}
>> +
>> +static void
>> +ifp_users_find_by_name_done(struct tevent_req *req)
>> +{
>> +    DBusError *error;
>> +    struct sbus_request *sbus_req;
>> +    struct sss_domain_info *domain;
>> +    struct ldb_result *result;
>> +    char *object_path;
>> +    errno_t ret;
>> +
>> +    sbus_req = tevent_req_callback_data(req, struct sbus_request);
>> +
>> +    ret = cache_req_user_by_name_recv(sbus_req, req, &result, &domain, NULL);
>> +    talloc_zfree(req);
>> +    if (ret == ENOENT) {
>> +        error = sbus_error_new(sbus_req, SBUS_ERROR_NOT_FOUND,
>> +                               "User not found");
>> +        goto done;
>> +    } else if (ret != EOK) {
>> +        error = sbus_error_new(sbus_req, DBUS_ERROR_FAILED, "Failed to fetch "
>> +                               "user [%d]: %s\n", ret, sss_strerror(ret));
>> +        goto done;
>> +    }
>> +
>> +    object_path = ifp_users_build_path_from_msg(sbus_req, domain,
>> +                                                result->msgs[0]);
>> +    if (object_path == NULL) {
>> +        error = sbus_error_new(sbus_req, SBUS_ERROR_INTERNAL,
>> +                               "Failed to compose object path");
>> +        goto done;
>> +    }
>> +
>
> I would prefer an explicit ret=EOK; here.

Done.
>
>> +done:
>> +    if (ret != EOK) {
>> +        sbus_request_fail_and_finish(sbus_req, error);
>> +        return;
>> +    }
>> +
>> +    iface_ifp_users_FindByName_finish(sbus_req, object_path);
>> +    return;
>> +}
>> +
>> +static void ifp_users_find_by_id_done(struct tevent_req *req);
>> +
>> +int ifp_users_find_by_id(struct sbus_request *sbus_req,
>> +                         void *data,
>> +                         uint32_t id)
>> +{
>> +    struct ifp_ctx *ctx;
>> +    struct tevent_req *req;
>> +
>> +    ctx = talloc_get_type(data, struct ifp_ctx);
>> +    if (ctx == NULL) {
>> +        DEBUG(SSSDBG_CRIT_FAILURE, "Invalid pointer!\n");
>> +        return ERR_INTERNAL;
>> +    }
>> +
>> +    req = cache_req_user_by_id_send(sbus_req, ctx->rctx->ev, ctx->rctx,
>> +                                    ctx->ncache, ctx->neg_timeout, 0,
>> +                                    NULL, id);
>
> Missing NULL check.

Done.

>
>> +
>> +    tevent_req_set_callback(req, ifp_users_find_by_id_done, sbus_req);
>> +
>> +    return EOK;
>> +}
>> +
>> +static void
>> +ifp_users_find_by_id_done(struct tevent_req *req)
>> +{
>> +    DBusError *error;
>> +    struct sbus_request *sbus_req;
>> +    struct sss_domain_info *domain;
>> +    struct ldb_result *result;
>> +    char *object_path;
>> +    errno_t ret;
>> +
>> +    sbus_req = tevent_req_callback_data(req, struct sbus_request);
>> +
>> +    ret = cache_req_user_by_id_recv(sbus_req, req, &result, &domain);
>> +    talloc_zfree(req);
>> +    if (ret == ENOENT) {
>> +        error = sbus_error_new(sbus_req, SBUS_ERROR_NOT_FOUND,
>> +                               "User not found");
>> +        goto done;
>> +    } else if (ret != EOK) {
>> +        error = sbus_error_new(sbus_req, DBUS_ERROR_FAILED, "Failed to fetch "
>> +                               "user [%d]: %s\n", ret, sss_strerror(ret));
>> +        goto done;
>> +    }
>> +
>> +    object_path = ifp_users_build_path_from_msg(sbus_req, domain,
>> +                                                result->msgs[0]);
>> +    if (object_path == NULL) {
>> +        error = sbus_error_new(sbus_req, SBUS_ERROR_INTERNAL,
>> +                               "Failed to compose object path");
>> +        goto done;
>> +    }
>> +
>
> I would prefer an explicit ret=EOK; here.

Done.
>
>> +done:
>> +    if (ret != EOK) {
>> +        sbus_request_fail_and_finish(sbus_req, error);
>> +        return;
>> +    }
>> +
>> +    iface_ifp_users_FindByID_finish(sbus_req, object_path);
>> +    return;
>> +}
>
>>  From 8cbe3a3c867e38ad6fd65f9c08822a5ec3e41680 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Mon, 9 Feb 2015 12:02:33 +0100
>> Subject: [PATCH 5/8] IFP: add org.freedesktop.sssd.infopipe.Users.User
>>
>> Resolves:
>> https://fedorahosted.org/sssd/ticket/2150
>
> [...]
>
>> index 9aec9291c8b4de55f8dfdba9f3b5f5ab36e64a45..39213035fc29f0a47015aabe17ac634cec793a24 100644
>> --- a/src/responder/ifp/ifp_users.c
>> +++ b/src/responder/ifp/ifp_users.c
>> @@ -24,10 +24,12 @@
>>
>>   #include "db/sysdb.h"
>>   #include "util/util.h"
>> +#include "util/strtonum.h"
>>   #include "sbus/sssd_dbus_errors.h"
>>   #include "responder/common/responder.h"
>>   #include "responder/common/responder_cache_req.h"
>>   #include "responder/ifp/ifp_users.h"
>> +#include "responder/ifp/ifp_groups.h"
>>
>>   char * ifp_users_build_path_from_msg(TALLOC_CTX *mem_ctx,
>>                                        struct sss_domain_info *domain,
>> @@ -44,6 +46,41 @@ char * ifp_users_build_path_from_msg(TALLOC_CTX *mem_ctx,
>>       return sbus_opath_compose(mem_ctx, IFP_PATH_USERS, domain->name, uid);
>>   }
>>
>> +errno_t ifp_users_decompose_path(struct sss_domain_info *domains,
>> +                                 const char *path,
>> +                                 struct sss_domain_info **_domain,
>> +                                 uid_t *_uid)
>
> Looks like this one could be static as well.

Done.
>
>> +{
>> +    char **parts = NULL;
>> +    struct sss_domain_info *domain;
>> +    uid_t uid;
>> +    errno_t ret;
>> +
>> +    ret = sbus_opath_decompose_exact(NULL, path, IFP_PATH_USERS, 2, &parts);
>> +    if (ret != EOK) {
>> +        return ret;
>> +    }
>> +
>> +    domain = find_domain_by_name(domains, parts[0], false);
>> +    if (domain == NULL) {
>> +        ret = ERR_DOMAIN_NOT_FOUND;
>> +        goto done;
>> +    }
>> +
>> +    uid = strtouint32(parts[1], NULL, 10);
>> +    if (errno != 0) {
>
> Since you use errno for error checking, you should set it to zero before
> calling strtouint32 (same elsewhere in this patchset).
>
>> +        ret = errno;
>> +        goto done;
>> +    }
>> +
>> +    *_domain = domain;
>> +    *_uid = uid;
>> +
>> +done:
>> +    talloc_free(parts);
>> +    return ret;
>> +}
>> +
>
> The rest looks good.
>
>>  From e2d550a4b6b30bdcf264a6285d714029f8309007 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Mon, 9 Feb 2015 12:17:37 +0100
>> Subject: [PATCH 6/8] IFP: add org.freedesktop.sssd.infopipe.Groups
>>
>> Resolves:
>> https://fedorahosted.org/sssd/ticket/2150
>
> [...]
>
>> +int ifp_groups_find_by_name(struct sbus_request *sbus_req,
>> +                           void *data,
>> +                           const char *name)
>> +{
>> +    struct ifp_ctx *ctx;
>> +    struct tevent_req *req;
>> +
>> +    ctx = talloc_get_type(data, struct ifp_ctx);
>> +    if (ctx == NULL) {
>> +        DEBUG(SSSDBG_CRIT_FAILURE, "Invalid pointer!\n");
>> +        return ERR_INTERNAL;
>> +    }
>> +
>> +    req = cache_req_group_by_name_send(sbus_req, ctx->rctx->ev, ctx->rctx,
>> +                                       ctx->ncache, ctx->neg_timeout, 0,
>> +                                       NULL, name);
>> +
>
> Missing NULL check.
Done.

>
>> +    tevent_req_set_callback(req, ifp_groups_find_by_name_done, sbus_req);
>> +
>> +    return EOK;
>> +}
>
> [...]
>
>> +int ifp_groups_find_by_id(struct sbus_request *sbus_req,
>> +                          void *data,
>> +                          uint32_t id)
>> +{
>> +    struct ifp_ctx *ctx;
>> +    struct tevent_req *req;
>> +
>> +    ctx = talloc_get_type(data, struct ifp_ctx);
>> +    if (ctx == NULL) {
>> +        DEBUG(SSSDBG_CRIT_FAILURE, "Invalid pointer!\n");
>> +        return ERR_INTERNAL;
>> +    }
>> +
>> +    req = cache_req_group_by_id_send(sbus_req, ctx->rctx->ev, ctx->rctx,
>> +                                     ctx->ncache, ctx->neg_timeout, 0,
>> +                                     NULL, id);
>
> Missing NULL check.

Done.
>
> The rest reads good to me.
>
>>  From a9d0c396598b767b32dada744603225e517bc845 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Tue, 10 Feb 2015 12:19:19 +0100
>> Subject: [PATCH 7/8] IFP: add org.freedesktop.sssd.infopipe.Groups.Group
>>
>> Resolves:
>> https://fedorahosted.org/sssd/ticket/2150
>> ---
>>   src/responder/ifp/ifp_groups.c                     | 143 +++++++++++++++++++++
>>   src/responder/ifp/ifp_groups.h                     |  25 ++++
>>   src/responder/ifp/ifp_iface.c                      |   9 ++
>>   src/responder/ifp/ifp_iface.xml                    |   9 ++
>>   src/responder/ifp/ifp_iface_generated.c            |  50 +++++++
>>   src/responder/ifp/ifp_iface_generated.h            |  19 +++
>>   .../ifp/org.freedesktop.sssd.infopipe.conf         |   1 +
>>   7 files changed, 256 insertions(+)
>>
>> diff --git a/src/responder/ifp/ifp_groups.c b/src/responder/ifp/ifp_groups.c
>> index 9a0e2e2244cb3dd8b203cc2858c3be7c6449e988..f1a93a03675e7583c063ae0adfd434e7d27c708c 100644
>> --- a/src/responder/ifp/ifp_groups.c
>> +++ b/src/responder/ifp/ifp_groups.c
>> @@ -23,6 +23,7 @@
>>
>>   #include "util/util.h"
>>   #include "db/sysdb.h"
>> +#include "util/strtonum.h"
>>   #include "sbus/sssd_dbus_errors.h"
>>   #include "responder/common/responder.h"
>>   #include "responder/common/responder_cache_req.h"
>> @@ -43,6 +44,41 @@ char * ifp_groups_build_path_from_msg(TALLOC_CTX *mem_ctx,
>>       return sbus_opath_compose(mem_ctx, IFP_PATH_GROUPS, domain->name, gid);
>>   }
>>
>> +errno_t ifp_groups_decompose_path(struct sss_domain_info *domains,
>> +                                  const char *path,
>> +                                  struct sss_domain_info **_domain,
>> +                                  gid_t *_gid)
>> +{
>> +    char **parts = NULL;
>> +    struct sss_domain_info *domain;
>> +    gid_t gid;
>> +    errno_t ret;
>> +
>> +    ret = sbus_opath_decompose_exact(NULL, path, IFP_PATH_GROUPS, 2, &parts);
>> +    if (ret != EOK) {
>> +        return ret;
>> +    }
>> +
>> +    domain = find_domain_by_name(domains, parts[0], false);
>> +    if (domain == NULL) {
>> +        ret = ERR_DOMAIN_NOT_FOUND;
>> +        goto done;
>> +    }
>> +
>
> Please zero out errno here.
>
> Looking at the interface, I wonder about the members. Are they
> transitive or just direct members?
> _______________________________________________

New patches are attached. All review-issues should be addressed. Those 
patches contains also extraAttributes, group members and user attributes 
acl.

Do we want to implement group attributes acl also?

The groups and users attributes on group object do not reply with full 
recursive membership at the moment since we do not store in on groups 
object in sysdb... is it possible to leave it this way or should I 
compute the membership? Sysdb only contains full user members (in 
memberuid attribute), but it contains only direct group members (in 
member attribute).

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sbus-provide-custom-error-names.patch
Type: text/x-patch
Size: 2220 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150504/0fa8af62/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-sbus-add-sbus_opath_decompose-_exact.patch
Type: text/x-patch
Size: 10474 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150504/0fa8af62/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-sbus-add-a-sas-get-invoker.patch
Type: text/x-patch
Size: 23686 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150504/0fa8af62/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-IFP-add-org.freedesktop.sssd.infopipe.Users.patch
Type: text/x-patch
Size: 23408 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150504/0fa8af62/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-IFP-add-org.freedesktop.sssd.infopipe.Users.User.patch
Type: text/x-patch
Size: 39820 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150504/0fa8af62/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-IFP-add-org.freedesktop.sssd.infopipe.Groups.patch
Type: text/x-patch
Size: 18469 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150504/0fa8af62/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-IFP-add-org.freedesktop.sssd.infopipe.Groups.Group.patch
Type: text/x-patch
Size: 18653 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150504/0fa8af62/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-IFP-remove-GetUserAttr-and-GetUserGroup.patch
Type: text/x-patch
Size: 22513 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150504/0fa8af62/attachment-0007.bin>


More information about the sssd-devel mailing list