Thanks for the patches. They work quite well!
One bug I found is that if you query a nonexistant object path with
GetAll, then all subsequent queries block. Maybe we don't finish some
request on error? This is what I tried:
[jhrozek@client] sssd $ [(review)] dbus-send --print-reply --system
--dest=org.freedesktop.sssd.infopipe
/org/freedesktop/sssd/infopipe/Users/redhat_2ecom/10327
org.freedesktop.DBus.Properties.GetAll
string:org.freedesktop.sssd.infopipe.Users.User
Error org.freedesktop.DBus.Error.NoReply: Message did not receive a reply
(timeout by message bus)
[jhrozek@client] sssd $ [(review)] dbus-send --print-reply --system
--dest=org.freedesktop.sssd.infopipe
/org/freedesktop/sssd/infopipe/Users/linux_2etest/465400000
org.freedesktop.DBus.Properties.GetAll
string:org.freedesktop.sssd.infopipe.Users.User
Error org.freedesktop.DBus.Error.TimedOut: Activation of
org.freedesktop.sssd.infopipe timed out
The first objectpath is a copy-n-paste error from a different machine,
the second is legitimate. All works well unless I use the nonexistant path.
Except for the last patch, I only have some nitpicks, see inline. Hopefully,
we'll push the patches soon.
> From 0608ed28fd0499255d566a74d2b600b4bbbad081 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
> Date: Wed, 21 Jan 2015 13:38:06 +0100
> Subject: [PATCH 1/8] sbus: provide custom error names
ACK
> From be552bf54a91e27c5d04a1e56bc1c3d3ded62d30 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
> Date: Thu, 29 Jan 2015 15:29:26 +0100
> Subject: [PATCH 2/8] sbus: add sbus_opath_decompose[_exact]
ACK
> From 1b50ad9839b4b77003933d41aee8f0f30b0a557a Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
> Date: Wed, 18 Mar 2015 13:01:07 +0100
> Subject: [PATCH 3/8] sbus: add a{sas} get invoker
Lukas nacked this one, we need the CI tests to pass. Also when I ran the
patches through Coverity, it saw a warning:
Error: UNINIT (CWE-457): [#def1]
sssd-1.12.90/src/sbus/sssd_dbus_invokers.c:319: var_decl: Declaring variable
"ret" without initializer.
sssd-1.12.90/src/sbus/sssd_dbus_invokers.c:413: uninit_use: Using uninitialized
value "ret".
# 411| done:
# 412| talloc_free(table_iter);
# 413|-> return ret;
# 414| }
# 415|
Error: COMPILER_WARNING: [#def2]
sssd-1.12.90/src/sbus/sssd_dbus_invokers.c: scope_hint: In function
'sbus_invoke_get_aDOsasDE'
sssd-1.12.90/src/sbus/sssd_dbus_invokers.c:413:5: warning: 'ret' may be used
uninitialized in this function [-Wmaybe-uninitialized]
# return ret;
# ^
# 411| done:
# 412| talloc_free(table_iter);
# 413|-> return ret;
# 414| }
# 415|
I guess neither me or you saw the warning locally because we compile with
-O0? But in general the code looks well to me and has a unit test, I'll
ACK it if you fix up the initialization.
> From f91333767425810d47b6299816ed21547b303de2 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
> Date: Thu, 15 Jan 2015 13:52:31 +0100
> Subject: [PATCH 4/8] IFP: add org.freedesktop.sssd.infopipe.Users
ACK to the code, but can you add a couple of examples using dbus-send to
the commit message?
I tested that fetching an existing user returns an object path that can
be used later and fetching a non-existing user returns:
Error org.freedesktop.sssd.Error.NotFound: User not found
I also tested a subdomain user and a user with an overriden username. Both
work since you're correctly using getpwuid_with_views();
Not sure if the bug I found earlier is related to this patch or not..
> From 6fd468596486a894a9a23d9c2d59468e67844dc5 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@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
Can you also add some examples to the commit message?
> +static void ifp_users_get_as_string(struct sbus_request *sbus_req,
> + void *data,
> + const char *attr,
> + const char **_out)
> +{
> + struct ifp_ctx *ifp_ctx;
> + struct ldb_message *msg;
> + errno_t ret;
> +
> + *_out = NULL;
> +
> + ifp_ctx = talloc_get_type(data, struct ifp_ctx);
> + if (ifp_ctx == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "Invalid pointer!\n");
> + return;
> + }
> +
> + if (!ifp_is_user_attr_allowed(ifp_ctx, attr)) {
> + DEBUG(SSSDBG_TRACE_ALL, "Attribute %s is not allowed\n", attr);
> + return;
> + }
> +
> + ret = ifp_users_user_get(sbus_req, ifp_ctx, NULL, NULL, &msg);
> + if (ret != EOK) {
> + return;
> + }
> +
> + *_out = ldb_msg_find_attr_as_string(msg, attr, NULL);
I think you need to use sss_view_ldb_msg_find_attr_as_string() here. Anyway,
I'm not seeing data from ID views:
[jhrozek@client] sssd $ [(review)] getent passwd psu...@ad.example.com
clientv...@ad.example.com:*:198601106:198601106:ps
user:/home/AD.EXAMPLE.COM/psuser:/bin/sh
[jhrozek@client] sssd $ [(review)] dbus-send --print-reply --system
--dest=org.freedesktop.sssd.infopipe
/org/freedesktop/sssd/infopipe/Users/AD_2eEXAMPLE_2eCOM/198601106
org.freedesktop.DBus.Properties.Get
string:org.freedesktop.sssd.infopipe.Users.User string:name
method return sender=:1.53 -> dest=:1.56 reply_serial=2
variant string "psu...@ad.example.com"
> +
> + return;
> +}
> +
> +static void ifp_users_get_as_uint32(struct sbus_request *sbus_req,
> + void *data,
> + const char *attr,
> + uint32_t *_out)
Maybe later we'll need to come up with some smarter caching to avoid
calling a ldb search per attribute, but let's not optimize before we
need to..
> +{
> + struct ifp_ctx *ifp_ctx;
> + struct ldb_message *msg;
> + errno_t ret;
> +
> + *_out = 0;
> +
> + ifp_ctx = talloc_get_type(data, struct ifp_ctx);
> + if (ifp_ctx == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "Invalid pointer!\n");
> + return;
> + }
> +
> + if (!ifp_is_user_attr_allowed(ifp_ctx, attr)) {
> + DEBUG(SSSDBG_TRACE_ALL, "Attribute %s is not allowed\n", attr);
> + return;
> + }
> +
> + ret = ifp_users_user_get(sbus_req, ifp_ctx, NULL, NULL, &msg);
> + if (ret != EOK) {
> + return;
> + }
> +
> + *_out = ldb_msg_find_attr_as_uint(msg, attr, 0);
I think you should use sss_view_ldb_msg_find_attr_as_uint64() here,
unsigned int is a bit platform specific, plus the overrides don't work
(same as with string..)
> +
> + return;
> +}
> +
Updating groups works fine..
> +void ifp_users_user_get_groups(struct sbus_request *sbus_req,
> + void *data,
> + const char ***_out,
> + int *_size)
But retrieving groups seems to also include non-POSIX groups with GID 0..
> +{
> + struct ifp_ctx *ifp_ctx;
> + struct sss_domain_info *domain;
> + const char *username;
> + struct ldb_message *user;
> + struct ldb_result *res;
> + const char **out;
> + errno_t ret;
> + int i;
> +
> + *_out = NULL;
> + *_size = 0;
> +
> + ifp_ctx = talloc_get_type(data, struct ifp_ctx);
> + if (ifp_ctx == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "Invalid pointer!\n");
> + return;
> + }
> +
> + if (!ifp_is_user_attr_allowed(ifp_ctx, "groups")) {
> + DEBUG(SSSDBG_TRACE_ALL, "Attribute %s is not allowed\n",
> + SYSDB_MEMBEROF);
> + return;
> + }
> +
> + ret = ifp_users_user_get(sbus_req, ifp_ctx, NULL, &domain, &user);
> + if (ret != EOK) {
> + return;
> + }
> +
> + username = ldb_msg_find_attr_as_string(user, SYSDB_NAME, NULL);
> + if (username == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "User name is empty!\n");
> + return;
> + }
> +
> + /* Run initgroups. */
> + ret = sysdb_initgroups_with_views(sbus_req, domain, username, &res);
> + if (ret != EOK) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get groups for %s@%s [%d]:
> %s\n",
> + username, domain->name, ret, sss_strerror(ret));
> + return;
> + }
> +
> + out = talloc_zero_array(sbus_req, const char *, res->count);
> + if (out == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero_array() failed\n");
> + return;
> + }
> +
> + for (i = 0; i < res->count; i++) {
> + out[i] = ifp_groups_build_path_from_msg(out, domain, res->msgs[i]);
I think groups with UID 0 should be filtered here. You can test with
ipausers on an IPA client..
> + if (out[i] == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "ifp_groups_build_path() failed\n");
> + return;
> + }
> + }
> +
> + *_out = out;
> + *_size = res->count;
> +}
The rest looks OK to me..
> From 507f8c5d3be63c5b35fa016a00a00f253ad3194b Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
> Date: Mon, 9 Feb 2015 12:17:37 +0100
> Subject: [PATCH 6/8] IFP: add org.freedesktop.sssd.infopipe.Groups
Same as with others, please add some examples with dbus-send.
Otherwise ACK. I tested with a group that has an override placed on it
and all combinations seemed to work:
[jhrozek@client] sssd $ [(review)] dbus-send --print-reply --system
--dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe/Groups
org.freedesktop.sssd.infopipe.Groups.FindByName string:gr1
method return sender=:1.88 -> dest=:1.90 reply_serial=2
object path "/org/freedesktop/sssd/infopipe/Groups/linux_2etest/465400007"
[jhrozek@client] sssd $ [(review)] dbus-send --print-reply --system
--dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe/Groups
org.freedesktop.sssd.infopipe.Groups.FindByName string:clientgr1
method return sender=:1.88 -> dest=:1.91 reply_serial=2
object path "/org/freedesktop/sssd/infopipe/Groups/linux_2etest/465400007"
[jhrozek@client] sssd $ [(review)] dbus-send --print-reply --system
--dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe/Groups
org.freedesktop.sssd.infopipe.Groups.FindByID uint32:12345
method return sender=:1.88 -> dest=:1.95 reply_serial=2
object path "/org/freedesktop/sssd/infopipe/Groups/linux_2etest/465400007"
[jhrozek@client] sssd $ [(review)] dbus-send --print-reply --system
--dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe/Groups
org.freedesktop.sssd.infopipe.Groups.FindByID uint32:465400007
method return sender=:1.88 -> dest=:1.96 reply_serial=2
object path "/org/freedesktop/sssd/infopipe/Groups/linux_2etest/465400007"
> From a4c1f316f7c434e0e20f457287dda01a96a888c0 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
> Date: Tue, 10 Feb 2015 12:19:19 +0100
> Subject: [PATCH 7/8] IFP: add org.freedesktop.sssd.infopipe.Groups.Group
Similar comment about overrides as with the users patch.
Also please add some examples to the commit message of this patch.
[...]
> +static errno_t
> +ifp_groups_group_get_members(TALLOC_CTX *mem_ctx,
> + struct sbus_request *sbus_req,
> + void *data,
> + const char ***_users,
> + int *_users_count,
> + const char ***_groups,
> + int *_groups_count)
> +{
> + TALLOC_CTX *tmp_ctx;
> + struct ldb_message *msg;
> + struct ldb_message_element *el;
> + struct sss_domain_info *domain;
> + const char *class;
> + const char **users;
> + int users_count;
> + const char **groups;
> + int groups_count;
> + const char *dn;
> + struct ldb_dn *ldb_dn;
> + size_t count;
> + struct ldb_message **entry;
> + errno_t ret;
> + int i;
> + const char *attrs[] = {SYSDB_OBJECTCLASS, SYSDB_UIDNUM, SYSDB_GIDNUM,
> NULL};
> +
> + tmp_ctx = talloc_new(NULL);
> + if (tmp_ctx == NULL) {
> + return ENOMEM;
> + }
> +
> + ret = ifp_groups_group_get(sbus_req, data, NULL, &domain, &msg);
> + if (ret != EOK) {
> + goto done;
> + }
> +
> + el = ldb_msg_find_element(msg, SYSDB_MEMBER);
> + if (el == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get member attribute!\n");
> + ret = ERR_INTERNAL;
> + goto done;
> + }
> +
> + if (el->num_values == 0) {
> + *_users = NULL;
> + *_groups = NULL;
> + *_users_count = 0;
> + *_groups_count = 0;
> + ret = EOK;
> + goto done;
> + }
> +
> + users = talloc_zero_array(tmp_ctx, const char *, el->num_values + 1);
> + if (users == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero_array() failed\n");
> + ret = ENOMEM;
> + goto done;
> + }
> +
> + groups = talloc_zero_array(tmp_ctx, const char *, el->num_values + 1);
> + if (groups == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero_array() failed\n");
> + ret = ENOMEM;
> + goto done;
> + }
Here I guess you could optimize and run an ASQ search instead of N base
searches.
> From 201615b00948719836d7ae23b36918f664260f47 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com>
> Date: Mon, 4 May 2015 14:45:50 +0200
> Subject: [PATCH 8/8] IFP: remove GetUserAttr and GetUserGroup
>
> Those methods are replaced by proper user and group implementation.
> ---
> src/responder/ifp/ifp_iface.c | 3 -
> src/responder/ifp/ifp_iface.xml | 12 -
> src/responder/ifp/ifp_private.h | 5 -
> src/responder/ifp/ifpsrv_cmd.c | 566
> ----------------------------------------
> 4 files changed, 586 deletions(-)
As discussed on IRC, we can't just remove API that is widely used. Feel
free to prepare a different patch that sends a deprecation warning, though,
but we must not remove API without deprecating it first and making sure
all downstreams have been converted to the new API.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
