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

Jakub Hrozek jhrozek at redhat.com
Wed Apr 8 10:48:39 UTC 2015


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?

> +    }
> +
> +    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?

> 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.

> +
> +    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:
> +    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.

> +
> +    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:
> +    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.

> +{
> +    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.

> +    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.

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?



More information about the sssd-devel mailing list