-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 10/05/2009 07:43 PM, Stephen Gallagher wrote:
> Replied to the wrong patch earlier...
>
> Nack.
> sss_getpwnam_done() and friends should return EIO if the sysdb replies
> with more than one result, I think. EINVAL implies that the fault was
> with the caller, whereas EIO shows us that something went wrong internally.
>
Fixed in getpwnam(), in getgrnam() there is no such check as it also
returns the members of the group.
> I don't like the gratuitous use of discard_const_p in
> sss_getpwnam_done(). These sync ops are theoretically useful in other
> places in our code. I don't want people assuming that they can modify
> the contents of the res->data->name field (for example). They should be
> talloc_strdup()-ed into the res structure.
>
True, talloc_strdup() is used now.
> sss_useradd.c:
> ERROR("A user or group with the same name or UID already exists\n");
> should be
> ERROR("A user or group with the same name or ID already exists\n");
>
Fixed.
> tools_util.c:
> If enumerate=true, shouldn't we just make a call to something like a
> sysdb_getgrent_sync and compare the resulting complete list with the
> received groups instead of making N complete sync calls, where N could
> be equal to (or greater than) the total list of groups in the DB?
>
> For that matter, since this is the local domain, it might be worth
> enumerating them anyway, even if enumerations would normally not happen,
> since local performance won't be a problem.
This is not addressed. I discussed this (very briefly) with Simo, we
agreed that we should stick to sysdb_getgrnam() for now, because as long
as our searches match indexes (they do, we search based on name) the
search for a single group is pretty fast.
Jakub
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAkrLHLUACgkQHsardTLnvCV1LACgsE1qAwIgUQItfBuzwcOg/lZX
OTAAn2gkSf45a62Ro2iPoviA8C6KAiGz
=MeJm
-----END PGP SIGNATURE-----
>From 11a8103dc5bd7894631b5f0f6b1f575c27d63722 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Fri, 2 Oct 2009 10:39:02 +0200
Subject: [PATCH] Fix error messages in tools
Add getpwnam, getgrnam sync versions
Fix ticket #164: Groupnames in non-local domains
Fix ticket #100: Error Message Modifying a user that doesn't Exist
Fix ticket #214: incorrect error message when MPG already exists
Fix ticket #188: Deleting and modifying users in non-local domain
Fix ticket #120: Adding a user to a full domain gives unhelpful error message
---
server/tools/sss_groupadd.c | 4 +
server/tools/sss_groupdel.c | 19 +----
server/tools/sss_groupmod.c | 27 ++++++
server/tools/sss_sync_ops.c | 201 +++++++++++++++++++++++++++++++++++++++++++
server/tools/sss_sync_ops.h | 14 +++
server/tools/sss_useradd.c | 14 +++-
server/tools/sss_userdel.c | 18 +----
server/tools/sss_usermod.c | 28 ++++++-
server/tools/tools_util.c | 31 +++++++
server/tools/tools_util.h | 4 +
10 files changed, 325 insertions(+), 35 deletions(-)
diff --git a/server/tools/sss_groupadd.c b/server/tools/sss_groupadd.c
index 1f6a2f1..82d4573 100644
--- a/server/tools/sss_groupadd.c
+++ b/server/tools/sss_groupadd.c
@@ -125,6 +125,10 @@ done:
if (tctx->error) {
ret = tctx->error;
switch (ret) {
+ case ERANGE:
+ ERROR("Could not allocate ID for the group - domain full?\n");
+ break;
+
case EEXIST:
ERROR("A group with the same name or GID already exists\n");
break;
diff --git a/server/tools/sss_groupdel.c b/server/tools/sss_groupdel.c
index a4451c4..d6e3dfd 100644
--- a/server/tools/sss_groupdel.c
+++ b/server/tools/sss_groupdel.c
@@ -23,8 +23,6 @@
#include <stdlib.h>
#include <talloc.h>
#include <popt.h>
-#include <grp.h>
-#include <unistd.h>
#include "db/sysdb.h"
#include "util/util.h"
@@ -35,7 +33,6 @@ int main(int argc, const char **argv)
{
int ret = EXIT_SUCCESS;
int pc_debug = 0;
- struct group *grp_info;
const char *pc_groupname = NULL;
struct tools_ctx *tctx = NULL;
@@ -93,19 +90,6 @@ int main(int argc, const char **argv)
goto fini;
}
- /* arguments processed, go on to actual work */
- grp_info = getgrnam(tctx->octx->name);
- if (grp_info) {
- tctx->octx->gid = grp_info->gr_gid;
- }
-
- /* arguments processed, go on to actual work */
- if (id_in_range(tctx->octx->uid, tctx->octx->domain) != EOK) {
- ERROR("The selected GID is outside the allowed range\n");
- ret = EXIT_FAILURE;
- goto fini;
- }
-
start_transaction(tctx);
if (tctx->error != EOK) {
goto done;
@@ -129,7 +113,8 @@ done:
DEBUG(1, ("sysdb operation failed (%d)[%s]\n", ret, strerror(ret)));
switch (ret) {
case ENOENT:
- ERROR("No such group\n");
+ ERROR("No such group in local domain. "
+ "Removing groups only allowed in local domain.\n");
break;
default:
diff --git a/server/tools/sss_groupmod.c b/server/tools/sss_groupmod.c
index e821fdc..c0528c8 100644
--- a/server/tools/sss_groupmod.c
+++ b/server/tools/sss_groupmod.c
@@ -53,6 +53,7 @@ int main(int argc, const char **argv)
char *addgroups = NULL, *rmgroups = NULL;
int ret;
const char *pc_groupname = NULL;
+ char *badgroup = NULL;
debug_prg_name = argv[0];
@@ -117,6 +118,16 @@ int main(int argc, const char **argv)
ret = EXIT_FAILURE;
goto fini;
}
+ /* check the username to be able to give sensible error message */
+ ret = sysdb_getgrnam_sync(tctx, tctx->ev, tctx->sysdb,
+ tctx->octx->name, tctx->local,
+ &tctx->octx);
+ if (ret != EOK) {
+ ERROR("Cannot find group in local domain, "
+ "modifying groups is allowed only in local domain\n");
+ goto fini;
+ }
+
tctx->octx->gid = pc_gid;
@@ -134,6 +145,14 @@ int main(int argc, const char **argv)
ERROR("Member groups must be in the same domain as parent group\n");
goto fini;
}
+
+ /* Check group names in the LOCAL domain */
+ ret = check_group_names(tctx, tctx->octx->addgroups, &badgroup);
+ if (ret != EOK) {
+ ERROR("Cannot find group %s in local domain, "
+ "only groups in local domain are allowed\n", badgroup);
+ goto fini;
+ }
}
if (rmgroups) {
@@ -150,6 +169,14 @@ int main(int argc, const char **argv)
ERROR("Member groups must be in the same domain as parent group\n");
goto fini;
}
+
+ /* Check group names in the LOCAL domain */
+ ret = check_group_names(tctx, tctx->octx->rmgroups, &badgroup);
+ if (ret != EOK) {
+ ERROR("Cannot find group %s in local domain, "
+ "only groups in local domain are allowed\n", badgroup);
+ goto fini;
+ }
}
if (id_in_range(tctx->octx->gid, tctx->octx->domain) != EOK) {
diff --git a/server/tools/sss_sync_ops.c b/server/tools/sss_sync_ops.c
index 68fe73e..932a712 100644
--- a/server/tools/sss_sync_ops.c
+++ b/server/tools/sss_sync_ops.c
@@ -44,6 +44,7 @@
} while(0)
struct sync_op_res {
+ struct ops_ctx *data;
int error;
bool done;
};
@@ -1138,6 +1139,10 @@ int useradd_defaults(TALLOC_CTX *mem_ctx,
if (homedir) {
data->home = talloc_strdup(data, homedir);
+ if (data->home == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
} else {
ret = confdb_get_string(confdb, mem_ctx,
conf_path, CONFDB_LOCAL_DEFAULT_BASEDIR,
@@ -1546,3 +1551,199 @@ static void end_transaction_done(struct tevent_req *req)
talloc_zfree(req);
}
+/*
+ * getpwnam, getgrnam and friends
+ */
+static void sss_getpwnam_done(void *ptr, int status,
+ struct ldb_result *lrs);
+
+int sysdb_getpwnam_sync(TALLOC_CTX *mem_ctx,
+ struct tevent_context *ev,
+ struct sysdb_ctx *sysdb,
+ const char *name,
+ struct sss_domain_info *domain,
+ struct ops_ctx **out)
+{
+ int ret;
+ struct sync_op_res *res = NULL;
+
+ res = talloc_zero(mem_ctx, struct sync_op_res);
+ if (!res) {
+ return ENOMEM;
+ }
+
+ if (out == NULL) {
+ DEBUG(1, ("NULL passed for storage pointer\n"));
+ return EINVAL;
+ }
+ res->data = *out;
+
+ ret = sysdb_getpwnam(mem_ctx,
+ sysdb,
+ domain,
+ name,
+ sss_getpwnam_done,
+ res);
+
+ SYNC_LOOP(res, ret);
+
+ return ret;
+}
+
+static void sss_getpwnam_done(void *ptr, int status,
+ struct ldb_result *lrs)
+{
+ struct sync_op_res *res = talloc_get_type(ptr, struct sync_op_res );
+ const char *str;
+
+ res->done = true;
+
+ if (status != LDB_SUCCESS) {
+ res->error = status;
+ return;
+ }
+
+ switch (lrs->count) {
+ case 0:
+ DEBUG(1, ("No result for sysdb_getpwnam call\n"));
+ res->error = ENOENT;
+ break;
+
+ case 1:
+ res->error = EOK;
+ /* fill ops_ctx */
+ res->data->uid = ldb_msg_find_attr_as_uint64(lrs->msgs[0],
+ SYSDB_UIDNUM, 0);
+
+ res->data->gid = ldb_msg_find_attr_as_uint64(lrs->msgs[0],
+ SYSDB_GIDNUM, 0);
+
+ str = ldb_msg_find_attr_as_string(lrs->msgs[0],
+ SYSDB_NAME, NULL);
+ res->data->name = talloc_strdup(res, str);
+ if (res->data->name == NULL) {
+ res->error = ENOMEM;
+ return;
+ }
+
+ str = ldb_msg_find_attr_as_string(lrs->msgs[0],
+ SYSDB_GECOS, NULL);
+ res->data->gecos = talloc_strdup(res, str);
+ if (res->data->gecos == NULL) {
+ res->error = ENOMEM;
+ return;
+ }
+
+ str = ldb_msg_find_attr_as_string(lrs->msgs[0],
+ SYSDB_HOMEDIR, NULL);
+ res->data->home = talloc_strdup(res, str);
+ if (res->data->home == NULL) {
+ res->error = ENOMEM;
+ return;
+ }
+
+ str = ldb_msg_find_attr_as_string(lrs->msgs[0],
+ SYSDB_SHELL, NULL);
+ res->data->shell = talloc_strdup(res, str);
+ if (res->data->shell == NULL) {
+ res->error = ENOMEM;
+ return;
+ }
+
+ str = ldb_msg_find_attr_as_string(lrs->msgs[0],
+ SYSDB_DISABLED, NULL);
+ if (str == NULL) {
+ res->data->lock = DO_UNLOCK;
+ } else {
+ if (strcasecmp(str, "true") == 0) {
+ res->data->lock = DO_LOCK;
+ } else if (strcasecmp(str, "false") == 0) {
+ res->data->lock = DO_UNLOCK;
+ } else { /* Invalid value */
+ DEBUG(2, ("Invalid value for %s attribute: %s\n",
+ SYSDB_DISABLED, str ? str : "NULL"));
+ res->error = EIO;
+ return;
+ }
+ }
+ break;
+
+ default:
+ DEBUG(1, ("More than one result for sysdb_getpwnam call\n"));
+ res->error = EIO;
+ break;
+ }
+}
+
+static void sss_getgrnam_done(void *ptr, int status,
+ struct ldb_result *lrs);
+
+int sysdb_getgrnam_sync(TALLOC_CTX *mem_ctx,
+ struct tevent_context *ev,
+ struct sysdb_ctx *sysdb,
+ const char *name,
+ struct sss_domain_info *domain,
+ struct ops_ctx **out)
+{
+ int ret;
+ struct sync_op_res *res = NULL;
+
+ res = talloc_zero(mem_ctx, struct sync_op_res);
+ if (!res) {
+ return ENOMEM;
+ }
+
+ if (out == NULL) {
+ DEBUG(1, ("NULL passed for storage pointer\n"));
+ return EINVAL;
+ }
+ res->data = *out;
+
+ ret = sysdb_getgrnam(mem_ctx,
+ sysdb,
+ domain,
+ name,
+ sss_getgrnam_done,
+ res);
+
+ SYNC_LOOP(res, ret);
+
+ return ret;
+}
+
+static void sss_getgrnam_done(void *ptr, int status,
+ struct ldb_result *lrs)
+{
+ struct sync_op_res *res = talloc_get_type(ptr, struct sync_op_res );
+ const char *str;
+
+ res->done = true;
+
+ if (status != LDB_SUCCESS) {
+ res->error = status;
+ return;
+ }
+
+ switch (lrs->count) {
+ case 0:
+ DEBUG(1, ("No result for sysdb_getgrnam call\n"));
+ res->error = ENOENT;
+ break;
+
+ /* sysdb_getgrnam also returns members */
+ default:
+ res->error = EOK;
+ /* fill ops_ctx */
+ res->data->gid = ldb_msg_find_attr_as_uint64(lrs->msgs[0],
+ SYSDB_GIDNUM, 0);
+ str = ldb_msg_find_attr_as_string(lrs->msgs[0],
+ SYSDB_NAME, NULL);
+ res->data->name = talloc_strdup(res, str);
+ if (res->data->name == NULL) {
+ res->error = ENOMEM;
+ return;
+ }
+ break;
+ }
+}
+
diff --git a/server/tools/sss_sync_ops.h b/server/tools/sss_sync_ops.h
index c99b9b9..3988992 100644
--- a/server/tools/sss_sync_ops.h
+++ b/server/tools/sss_sync_ops.h
@@ -86,5 +86,19 @@ int groupmod(TALLOC_CTX *mem_ctx,
void start_transaction(struct tools_ctx *tctx);
void end_transaction(struct tools_ctx *tctx);
+int sysdb_getpwnam_sync(TALLOC_CTX *mem_ctx,
+ struct tevent_context *ev,
+ struct sysdb_ctx *sysdb,
+ const char *name,
+ struct sss_domain_info *domain,
+ struct ops_ctx **out);
+
+int sysdb_getgrnam_sync(TALLOC_CTX *mem_ctx,
+ struct tevent_context *ev,
+ struct sysdb_ctx *sysdb,
+ const char *name,
+ struct sss_domain_info *domain,
+ struct ops_ctx **out);
+
#endif /* __SSS_OPS_H__ */
diff --git a/server/tools/sss_useradd.c b/server/tools/sss_useradd.c
index 07e741e..e2d58e0 100644
--- a/server/tools/sss_useradd.c
+++ b/server/tools/sss_useradd.c
@@ -131,6 +131,7 @@ int main(int argc, const char **argv)
poptContext pc = NULL;
struct tools_ctx *tctx = NULL;
char *groups = NULL;
+ char *badgroup = NULL;
int ret;
debug_prg_name = argv[0];
@@ -204,6 +205,13 @@ int main(int argc, const char **argv)
ERROR("Groups must be in the same domain as user\n");
goto fini;
}
+
+ /* Check group names in the LOCAL domain */
+ ret = check_group_names(tctx, tctx->octx->addgroups, &badgroup);
+ if (ret != EOK) {
+ ERROR("Cannot find group %s in local domain\n", badgroup);
+ goto fini;
+ }
}
/* Same as shadow-utils useradd, -g can specify gid or group name */
@@ -256,8 +264,12 @@ int main(int argc, const char **argv)
done:
if (tctx->error) {
switch (tctx->error) {
+ case ERANGE:
+ ERROR("Could not allocate ID for the user - domain full?\n");
+ break;
+
case EEXIST:
- ERROR("A user with the same name or UID already exists\n");
+ ERROR("A user or group with the same name or ID already exists\n");
break;
default:
diff --git a/server/tools/sss_userdel.c b/server/tools/sss_userdel.c
index 8c7c7bd..d14ef3d 100644
--- a/server/tools/sss_userdel.c
+++ b/server/tools/sss_userdel.c
@@ -23,8 +23,6 @@
#include <stdlib.h>
#include <talloc.h>
#include <popt.h>
-#include <pwd.h>
-#include <unistd.h>
#include "db/sysdb.h"
#include "util/util.h"
@@ -35,7 +33,6 @@ int main(int argc, const char **argv)
{
int ret = EXIT_SUCCESS;
struct tools_ctx *tctx = NULL;
- struct passwd *pwd_info;
const char *pc_username = NULL;
int pc_debug = 0;
@@ -93,18 +90,6 @@ int main(int argc, const char **argv)
goto fini;
}
- /* arguments processed, go on to actual work */
- pwd_info = getpwnam(tctx->octx->name);
- if (pwd_info) {
- tctx->octx->uid = pwd_info->pw_uid;
- }
-
- if (id_in_range(tctx->octx->uid, tctx->octx->domain) != EOK) {
- ERROR("The selected UID is outside the allowed range\n");
- ret = EXIT_FAILURE;
- goto fini;
- }
-
start_transaction(tctx);
if (tctx->error != EOK) {
goto done;
@@ -128,7 +113,8 @@ done:
DEBUG(1, ("sysdb operation failed (%d)[%s]\n", ret, strerror(ret)));
switch (ret) {
case ENOENT:
- ERROR("No such user\n");
+ ERROR("No such user in local domain. "
+ "Removing users only allowed in local domain.\n");
break;
default:
diff --git a/server/tools/sss_usermod.c b/server/tools/sss_usermod.c
index 2c2b80e..ea1095c 100644
--- a/server/tools/sss_usermod.c
+++ b/server/tools/sss_usermod.c
@@ -60,6 +60,7 @@ int main(int argc, const char **argv)
int ret;
const char *pc_username = NULL;
struct tools_ctx *tctx = NULL;
+ char *badgroup = NULL;
debug_prg_name = argv[0];
@@ -133,6 +134,15 @@ int main(int argc, const char **argv)
ret = EXIT_FAILURE;
goto fini;
}
+ /* check the username to be able to give sensible error message */
+ ret = sysdb_getpwnam_sync(tctx, tctx->ev, tctx->sysdb,
+ tctx->octx->name, tctx->local,
+ &tctx->octx);
+ if (ret != EOK) {
+ ERROR("Cannot find user in local domain, "
+ "modifying users is allowed only in local domain\n");
+ goto fini;
+ }
if (id_in_range(tctx->octx->uid, tctx->octx->domain) != EOK) {
ERROR("The selected UID is outside the allowed range\n");
@@ -154,6 +164,14 @@ int main(int argc, const char **argv)
ERROR("Groups must be in the same domain as user\n");
goto fini;
}
+
+ /* Check group names in the LOCAL domain */
+ ret = check_group_names(tctx, tctx->octx->addgroups, &badgroup);
+ if (ret != EOK) {
+ ERROR("Cannot find group %s in local domain, "
+ "only groups in local domain are allowed\n", badgroup);
+ goto fini;
+ }
}
if (rmgroups) {
@@ -170,6 +188,14 @@ int main(int argc, const char **argv)
ERROR("Groups must be in the same domain as user\n");
goto fini;
}
+
+ /* Check group names in the LOCAL domain */
+ ret = check_group_names(tctx, tctx->octx->rmgroups, &badgroup);
+ if (ret != EOK) {
+ ERROR("Cannot find group %s in local domain, "
+ "only groups in local domain are allowed\n", badgroup);
+ goto fini;
+ }
}
tctx->octx->gecos = pc_gecos;
@@ -205,7 +231,7 @@ done:
break;
case EFAULT:
- ERROR("Could not modify user - check if username is correct\n");
+ ERROR("Could not modify user - user already member of groups?\n");
break;
default:
diff --git a/server/tools/tools_util.c b/server/tools/tools_util.c
index 7772172..bc6b76f 100644
--- a/server/tools/tools_util.c
+++ b/server/tools/tools_util.c
@@ -191,6 +191,37 @@ int parse_name_domain(struct tools_ctx *tctx,
return EOK;
}
+int check_group_names(struct tools_ctx *tctx,
+ char **grouplist,
+ char **badgroup)
+{
+ int ret;
+ int i;
+ struct ops_ctx *groupinfo;
+
+ groupinfo = talloc_zero(tctx, struct ops_ctx);
+ if (!groupinfo) {
+ return ENOMEM;
+ }
+
+ for (i=0; grouplist[i]; ++i) {
+ ret = sysdb_getgrnam_sync(tctx,
+ tctx->ev,
+ tctx->sysdb,
+ grouplist[i],
+ tctx->local,
+ &groupinfo);
+ if (ret) {
+ DEBUG(6, ("Cannot find group %s, ret: %d\n", grouplist[i], ret));
+ break;
+ }
+ }
+
+ talloc_zfree(groupinfo);
+ *badgroup = grouplist[i];
+ return ret;
+}
+
int id_in_range(uint32_t id,
struct sss_domain_info *dom)
{
diff --git a/server/tools/tools_util.h b/server/tools/tools_util.h
index c062dec..a56fc5e 100644
--- a/server/tools/tools_util.h
+++ b/server/tools/tools_util.h
@@ -72,4 +72,8 @@ int parse_groups(TALLOC_CTX *mem_ctx,
int parse_group_name_domain(struct tools_ctx *tctx,
char **groups);
+int check_group_names(struct tools_ctx *tctx,
+ char **grouplist,
+ char **badgroup);
+
#endif /* __TOOLS_UTIL_H__ */
--
1.6.2.5
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
