On Thursday 21 October 2010 09:38:39 Tito wrote:
> On Thursday 21 October 2010 01:07:59 Denys Vlasenko wrote:
> > On Tuesday 05 October 2010 22:38, Tito wrote:
> > > > > Why? Who said that group foo needs to be removed when user foo is
> > > > > being removed?
> > > > > Do other deluser utilities do this?
> > > > Hi
> > > > Yes at least on my Debian system if i create a user:
> > > >
> > > > adduser prova
> > > > Adding user `prova' ...
> > > > Adding new group `prova' (1006) ...
> > > > Adding new user `prova' (1004) with group `prova' ...
> > > > The home directory `/home/prova' already exists. Not copying from
> > > > `/etc/skel'.
> > > > Enter new UNIX password:
> > > > Retype new UNIX password:
> > > > passwd: password updated successfully
> > > > Changing the user information for prova
> > > > Enter the new value, or press ENTER for the default
> > > > Full Name []:
> > > > Room Number []:
> > > > Work Phone []:
> > > > Home Phone []:
> > > > Other []:
> > > > Is the information correct? [Y/n] debian:/home/tito/Desktop#
> > > >
> > > >
> > > > grep prova /etc/pashadowetc/shadow /etc/group /etc/gshadow
> > > > /etc/passwd:prova:x:1004:1006:,,,:/home/prova:/bin/bash
> > > > /etc/shadow:prova:$6$I..Q9V.z$aoa7lBKlI68heRLDh9E0oMm8DUiBFO9MfBZgOYYjr/09v0Z1nPipjDPxz7TbbuwLmRslWZVdpI3Wr/G4IxDrl1:14886:0:99999:7:::
> > > > /etc/group:prova:x:1006:
> > > > /etc/gshadow:prova:!::
> > > >
> > > > deluser prova
> > > > Removing user `prova' ...
> > > > Warning: group `prova' has no more members.
> > > > Done.
> > > > grep prova /etc/passwd /etc/shadow /etc/group /etc/gshadow
> > > > debian:/home/tito/Desktop#
> > > >
> > > >
> > > > adduser command creates a group with the same name as the username,
> > > > and makes this group the primary group for that user. This is called a
> > > > user private group (UPG)
> > > >
> > > > So i test if a group with the same name exists and delete it,
> > > > Maybe a check if the group is empty could be added but as
> > > > we don't check for that at all in the current implementation
> > > > and happily remove non-empty groups I've skipped that.
> > > >
> >
> >
> >
> > > > > + if (!member && getpwnam(name))
> > > > > + bb_error_msg_and_die("is %s's
> > > > > primary group", name);
> > > > >
> > > > > This looks unreadable. Can be rewritten to do the same,
> > > > > but make more sense to human reader, as:
> > > > >
> > > > > if (!member) {
> > > > > if (getpwnam(name) = 0) {
> > > > > bb_error_msg_and_die("is %s's primary group",
> > > > > name);
> > > > > }
> > > > > }
> > > > >
> > > >
> > > > Can change it if you like it that way, for my personal taste just 3
> > > > wasted lines :-)
> > > >
> > > > Should be:
> > > >
> > > > if (!member) {
> > > > if (getpwnam(name)) {
> > > > bb_error_msg_and_die("is %s's primary group",
> > > > name);
> > > > }
> > > > }
> > > >
> > > > We check if getpwnam returns a struct passwd and in this case
> > > > it is a primary group (group with the same name of a user).
> > > >
> > > > > The message also needs to be better. This:
> > > > >
> > > > > deluser: is foo's primary group: <strerror here>
> > > > >
> > > > > is incomprehensible. What are you trying to say?
> > > >
> > > > Original error message on my system is:
> > > >
> > > > delgroup tito
> > > > /usr/sbin/delgroup: `tito' still has `tito' as their primary group!
> > > >
> > > > I shortened it a little to:
> > > >
> > > > delgroup: is tito's primary group
> > > >
> > > > Could be changed. As i'm not a native english speaker I'm open
> > > > to suggestions about a short and easy to understand error message
> > > > for this use case.
> >
> > Yes, "`tito' still has `tito' as their primary group!" is understandable,
> > while "is tito's primary group" isn't.
>
> Hi,
> can be changed.
>
> > Moreover, I think the check for username==groupname is wrong.
> >
> > Primary group is the group whose GID is listed as 4th field in /etc/passwd
> > for some user:
> >
> > syslog:x:54:50::/:
> > ^^
> > You can't delete group with gid 50 while there is at least one such user.
> > It has nothing to do with names of user and group being the same.
> >
>
> >From man adduser:
>
> By default, each user in Debian GNU/Linux is given a corresponding
> group with the same name.
>
> So when deleting a user we look if there is a group with corresponding name.
> If the user is part of a different group like:
>
> tito:users
>
> the group is not removed.
>
> Maybe a more correct definition could be: "User Private Groups", so the error
> message could be changed accordingly.
>
> Red Hat Linux uses a user private group (UPG) scheme, which makes UNIX groups
> much easier to use.
> The UPG scheme does not add or change anything in the standard UNIX way of
> handling groups;
> it simply offers a new convention. Whenever you create a new user, by
> default, he or she has a unique group.
> The scheme works as follows:
>
> User Private Group
>
> Every user has a primary group; the user is the only member of that
> group.
>
> Any idea for the best and shortest error message?
> or should i just use:
>
> "`tito' still has `tito' as their primary group!"
>
> > > Here v4,
> > >
> > > just cosmetical changes for readability.
> >
> > My problem with this patch is that it goes from:
> >
> > if (ENABLE_DELUSER && applet_name[3] == 'u') {
> > /* deluser USER */
> > if (update_passwd(bb_path_passwd_file, argv[1], NULL, NULL)
> > < 0)
> > return EXIT_FAILURE;
> > if (ENABLE_FEATURE_SHADOWPASSWDS)
> > if (update_passwd(bb_path_shadow_file, argv[1],
> > NULL, NULL) < 0)
> > return EXIT_FAILURE;
> > } else if (ENABLE_DELGROUP) {
> > /* delgroup ... */
> > if (!ENABLE_FEATURE_DEL_USER_FROM_GROUP || argc != 3) {
> > /* delgroup GROUP */
> > if (update_passwd(bb_path_group_file, argv[1],
> > NULL, NULL) < 0)
> > return EXIT_FAILURE;
> > if (ENABLE_FEATURE_SHADOWPASSWDS)
> > if (update_passwd(bb_path_gshadow_file,
> > argv[1], NULL, NULL) < 0)
> > return EXIT_FAILURE;
> > } else {
> > /* delgroup USER GROUP */
> > if (update_passwd(bb_path_group_file, argv[2],
> > NULL, argv[1]) < 0)
> > return EXIT_FAILURE;
> > if (ENABLE_FEATURE_SHADOWPASSWDS)
> > if (update_passwd(bb_path_gshadow_file,
> > argv[2], NULL, argv[1]) < 0)
> > return EXIT_FAILURE;
> > }
> > }
> >
> > to:
> >
> > switch (argc) {
> > case 3:
> > if (!ENABLE_FEATURE_DEL_USER_FROM_GROUP ||
> > do_deluser)
> > break;
> > name = argv[2];
> > member = argv[1];
> > /* Fallthrough */
> > case 2:
> > if (geteuid())
> >
> > bb_error_msg_and_die(bb_msg_perm_denied_are_you_root);
> > if (do_deluser) {
> > xgetpwnam(name);
> > if (ENABLE_FEATURE_SHADOWPASSWDS)
> > sfile = bb_path_shadow_file;
> > } else {
> > do_delgroup:
> > xgetgrnam(name);
> > if (!member) {
> > if(getpwnam(name) != NULL) {
> > bb_error_msg_and_die("is
> > %s's primary group", name);
> > }
> > }
> > pfile = bb_path_group_file;
> > if (ENABLE_FEATURE_SHADOWPASSWDS)
> > sfile = bb_path_gshadow_file;
> > }
> > do {
> > if (update_passwd(pfile, name, NULL,
> > member) == -1)
> > return EXIT_FAILURE;
> > pfile = sfile;
> > sfile = NULL;
> > } while (pfile);
> >
> > if (ENABLE_DELGROUP && do_deluser) {
> > do_deluser = 0;
> > goto do_delgroup;
> > }
> > return EXIT_SUCCESS;
> > }
> >
> >
> > This looks like a big step backwards wrt readability.
> > All comments are gone. Why?
>
> Looked trivial to me, mainly because I wrote it ;-)
> but I can add some.
>
> >
> > Can at least
> > if (geteuid())
> >
> > bb_error_msg_and_die(bb_msg_perm_denied_are_you_root);
> > be moved out of this switch statement from hell,
> > to make it a bit easier to grok?
>
> No, it cannot be moved outside because it must be triggered when
> doing deluser or delgroup (argc = 2)
> and when doing deluser from group (argc = 3)
> if uid != 0
>
> > Why "deluser USER" behavior depends on whether delgroup is enabled?
>
> Because if delgroup is enabled I would like to remove the user's private
> group.
>
> > If I build two busyboxes, one only with deluser, and another only with
> > delgroup,
> > why they work differently from one with both applets??
> >
> >
>
> Because if both functionalities are available I will use both when doing
> deluser.
>
> Will send a fully commented patch later today.
>
> Ciao,
> Tito
Hi,
attached you'll find a new version of the patch.
Changes:
1) Added comments
2) Fixed error message text
Ciao,
Tito
P.S.: also attached a drop in replacement of deluser.c
for easier review.
Signed-off-by: Tito Ragusa <[email protected]>
--- loginutils/deluser.c.orig 2010-09-22 22:05:25.000000000 +0200
+++ loginutils/deluser.c 2010-10-21 15:00:22.000000000 +0200
@@ -14,41 +14,69 @@
int deluser_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
int deluser_main(int argc, char **argv)
{
- if (argc != 2
- && (!ENABLE_FEATURE_DEL_USER_FROM_GROUP
- || applet_name[3] != 'g'
- || argc != 3)
- ) {
- bb_show_usage();
- }
-
- if (geteuid())
- bb_error_msg_and_die(bb_msg_perm_denied_are_you_root);
+ /* User or group name */
+ char *name = argv[1];
+ /* Member of group */
+ char *member = NULL;
+ /* Pointer to passwd or group file */
+ const char *pfile = bb_path_passwd_file;
+ /* Pointer to shadow or gshadow file if support is enabled */
+ const char *sfile = NULL;
+ /* Action to perform */
+ bool do_deluser = (ENABLE_DELUSER && (!ENABLE_DELGROUP || applet_name[3] == 'u'));
- if (ENABLE_DELUSER && applet_name[3] == 'u') {
- /* deluser USER */
- if (update_passwd(bb_path_passwd_file, argv[1], NULL, NULL) < 0)
- return EXIT_FAILURE;
- if (ENABLE_FEATURE_SHADOWPASSWDS)
- if (update_passwd(bb_path_shadow_file, argv[1], NULL, NULL) < 0)
- return EXIT_FAILURE;
- } else if (ENABLE_DELGROUP) {
- /* delgroup ... */
- if (!ENABLE_FEATURE_DEL_USER_FROM_GROUP || argc != 3) {
- /* delgroup GROUP */
- if (update_passwd(bb_path_group_file, argv[1], NULL, NULL) < 0)
- return EXIT_FAILURE;
- if (ENABLE_FEATURE_SHADOWPASSWDS)
- if (update_passwd(bb_path_gshadow_file, argv[1], NULL, NULL) < 0)
+ /* Check number of command line args */
+ switch (argc) {
+ case 3:
+ /* Setup: Delete USER from GROUP */
+ if (!ENABLE_FEATURE_DEL_USER_FROM_GROUP || do_deluser)
+ break;
+ /* Group name */
+ name = argv[2];
+ /* User name */
+ member = argv[1];
+ /* Fallthrough */
+ case 2:
+ /* Check if UID is 0 */
+ if (geteuid())
+ bb_error_msg_and_die(bb_msg_perm_denied_are_you_root);
+ if (do_deluser) {
+ /* Setup: Delete USER */
+ xgetpwnam(name);
+ if (ENABLE_FEATURE_SHADOWPASSWDS)
+ sfile = bb_path_shadow_file;
+ } else {
+ do_delgroup:
+ /* Setup: Delete GROUP and Delete USER from GROUP */
+ xgetgrnam(name);
+ if (!member) {
+ /* Check if we are deleting a "user private group"
+ of a existing user (group with the same name as the user) */
+ if(getpwnam(name) != NULL) {
+ bb_error_msg_and_die("`%s' still has `%s' as their primary group!", name, name);
+ }
+ }
+ pfile = bb_path_group_file;
+ if (ENABLE_FEATURE_SHADOWPASSWDS)
+ sfile = bb_path_gshadow_file;
+ }
+ /* Perform the requested action */
+ do {
+ if (update_passwd(pfile, name, NULL, member) == -1)
return EXIT_FAILURE;
- } else {
- /* delgroup USER GROUP */
- if (update_passwd(bb_path_group_file, argv[2], NULL, argv[1]) < 0)
- return EXIT_FAILURE;
- if (ENABLE_FEATURE_SHADOWPASSWDS)
- if (update_passwd(bb_path_gshadow_file, argv[2], NULL, argv[1]) < 0)
- return EXIT_FAILURE;
- }
+ /* Update also shadow file if support is enabled */
+ pfile = sfile;
+ sfile = NULL;
+ } while (pfile);
+
+ if (ENABLE_DELGROUP && do_deluser) {
+ /* if delgroup is enabled and we are doing deluser */
+ /* remove also the user's private group */
+ do_deluser = 0;
+ goto do_delgroup;
+ }
+ return EXIT_SUCCESS;
}
- return EXIT_SUCCESS;
+ /* Reached only if number of command line args is wrong */
+ bb_show_usage();
}
/* vi: set sw=4 ts=4: */
/*
* deluser/delgroup implementation for busybox
*
* Copyright (C) 1999 by Lineo, inc. and John Beppu
* Copyright (C) 1999,2000,2001 by John Beppu <[email protected]>
* Copyright (C) 2007 by Tito Ragusa <[email protected]>
*
* Licensed under GPL version 2, see file LICENSE in this tarball for details.
*
*/
#include "libbb.h"
int deluser_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
int deluser_main(int argc, char **argv)
{
/* User or group name */
char *name = argv[1];
/* Member of group */
char *member = NULL;
/* Pointer to passwd or group file */
const char *pfile = bb_path_passwd_file;
/* Pointer to shadow or gshadow file if support is enabled */
const char *sfile = NULL;
/* Action to perform */
bool do_deluser = (ENABLE_DELUSER && (!ENABLE_DELGROUP || applet_name[3] == 'u'));
/* Check number of command line args */
switch (argc) {
case 3:
/* Setup: Delete USER from GROUP */
if (!ENABLE_FEATURE_DEL_USER_FROM_GROUP || do_deluser)
break;
/* Group name */
name = argv[2];
/* User name */
member = argv[1];
/* Fallthrough */
case 2:
/* Check if UID is 0 */
if (geteuid())
bb_error_msg_and_die(bb_msg_perm_denied_are_you_root);
if (do_deluser) {
/* Setup: Delete USER */
xgetpwnam(name);
if (ENABLE_FEATURE_SHADOWPASSWDS)
sfile = bb_path_shadow_file;
} else {
do_delgroup:
/* Setup: Delete GROUP and Delete USER from GROUP */
xgetgrnam(name);
if (!member) {
/* Check if we are deleting a "user private group"
of a existing user (group with the same name as the user) */
if(getpwnam(name) != NULL) {
bb_error_msg_and_die("`%s' still has `%s' as their primary group!", name, name);
}
}
pfile = bb_path_group_file;
if (ENABLE_FEATURE_SHADOWPASSWDS)
sfile = bb_path_gshadow_file;
}
/* Perform the requested action */
do {
if (update_passwd(pfile, name, NULL, member) == -1)
return EXIT_FAILURE;
/* Update also shadow file if support is enabled */
pfile = sfile;
sfile = NULL;
} while (pfile);
if (ENABLE_DELGROUP && do_deluser) {
/* if delgroup is enabled and we are doing deluser */
/* remove also the user's private group */
do_deluser = 0;
goto do_delgroup;
}
return EXIT_SUCCESS;
}
/* Reached only if number of command line args is wrong */
bb_show_usage();
}
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
