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?= <pbrezina(a)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?= <pbrezina(a)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?= <pbrezina(a)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?= <pbrezina(a)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?= <pbrezina(a)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 psuser(a)ad.example.com
clientview@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 "psuser(a)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?= <pbrezina(a)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?= <pbrezina(a)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?= <pbrezina(a)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.