[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