[PATCH 00/16] pull: updates to lslogins and chfn

[PATCH 00/16] pull: updates to lslogins and chfn

[Sami Kerola]

Hello,

While using lslogins earlier the week I noticed --time-format not being
honored in all instances.  There is also another fix to option handling,
that is relatively important for anyone who cares user interface.

The chfn changes are my attempt to make the code a little bit easier to
read, and modify.  That said I came a cross with login.defs CHFN_RESTRICT
so I decided to make it work.  The login.defs(5) is also talking about
CHFN_AUTH, but I don't think it combines well with libuser so I did not
bother trying to spend time with it.

Now when the chfn is bit more OK, see the chsh could be better.  I'll try
to send patches about chsh sometime before holidays.


The following changes since commit 0d75c73d4fc9f638b9fbcab142002acadca9e603:
  lslogins: fix -l -g logic (2014-12-12 15:17:19 +0100)
are available in the git repository at:
  git://github.com/kerolasa/lelux-utiliteetit.git 2014wk49
for you to fetch changes up to 7de7c6819d44c9052e3bee50990f37b6d75f3bcb:
  chfn: make command to obey login.defs CHFN_RESTRICT instructions (2014-12-14 17:27:08 +0000)

Sami Kerola (16):
  lslogins: allow changing password changed and expiration time formats
  lslogins: make journald last logs time stamps to honor --time-format
  lslogins: tell why command failed
  lslogins: fix short options
  lslogins: reject unknown time format arguments
  lslogins: add space to systemd journal header and message
  lslogins: use hardcoded paths from pathnames.h
  chfn: remove function prototypes
  chfn: rewrite prompt() to use strutils
  chfn: use xasprintf() rather than bunch of strlen() and malloc() calls
  chfn: fix usage() regression
  chfn: simplify parse_passwd() by using strsep()
  chfn: add minimalistic struct chfn_control
  chfn: clean up parse_argv()
  chfn: move new and old finger structs to chfn control struct
  chfn: make command to obey login.defs CHFN_RESTRICT instructions

 include/pathnames.h       |   1 +
 login-utils/Makemodule.am |   6 +-
 login-utils/chfn.c        | 548 +++++++++++++++++++++++-----------------------
 login-utils/lslogins.c    |  65 +++---
 4 files changed, 312 insertions(+), 308 deletions(-)

-- 
2.1.3

[PATCH 01/16] lslogins: allow changing password changed and expiration time formats

[Sami Kerola]

The password change and expiry has are marked with resolution of a day,
so add a new short iso-8601 format.  With this system admins can easily
find users has not updated their password lately

$ lslogins --time-format=iso --user --output=pwd-change,user | sort -n

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/lslogins.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/login-utils/lslogins.c b/login-utils/lslogins.c
index 9929b14..f050595 100644
--- a/login-utils/lslogins.c
+++ b/login-utils/lslogins.c
@@ -143,6 +143,7 @@ enum {
 	TIME_SHORT,
 	TIME_FULL,
 	TIME_ISO,
+	TIME_ISO_SHORT,
 };
 
 /*
@@ -349,6 +350,9 @@ static char *make_time(int mode, time_t time)
 	case TIME_ISO:
 		strftime(buf, sizeof(buf), "%Y-%m-%dT%H:%M:%S%z", &tm);
 		break;
+	case TIME_ISO_SHORT:
+		strftime(buf, sizeof(buf), "%Y-%m-%d", &tm);
+		break;
 	default:
 		errx(EXIT_FAILURE, _("unsupported time type"));
 	}
@@ -693,7 +697,8 @@ static struct lslogins_user *get_user_info(struct lslogins_control *ctl, const c
 			break;
 		case COL_PWD_EXPIR:
 			if (shadow && shadow->sp_expire >= 0)
-				user->pwd_expire = make_time(TIME_SHORT,
+				user->pwd_expire = make_time(ctl->time_mode == TIME_ISO ?
+						TIME_ISO_SHORT : ctl->time_mode,
 						shadow->sp_expire * 86400);
 			break;
 		case COL_PWD_CTIME:
@@ -701,7 +706,8 @@ static struct lslogins_user *get_user_info(struct lslogins_control *ctl, const c
 			 * (especially in non-GMT timezones) would only serve
 			 * to confuse */
 			if (shadow)
-				user->pwd_ctime = make_time(TIME_SHORT,
+				user->pwd_ctime = make_time(ctl->time_mode == TIME_ISO ?
+						TIME_ISO_SHORT : ctl->time_mode,
 						shadow->sp_lstchg * 86400);
 			break;
 		case COL_PWD_CTIME_MIN:
-- 
2.1.3

[PATCH 02/16] lslogins: make journald last logs time stamps to honor --time-format

[Sami Kerola]

This makes by default the last logs to have year in output when necessary.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/lslogins.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/login-utils/lslogins.c b/login-utils/lslogins.c
index f050595..85fd7c8 100644
--- a/login-utils/lslogins.c
+++ b/login-utils/lslogins.c
@@ -1055,7 +1055,7 @@ static void fill_table(const void *u, const VISIT which, const int depth __attri
 	return;
 }
 #ifdef HAVE_LIBSYSTEMD
-static void print_journal_tail(const char *journal_path, uid_t uid, size_t len)
+static void print_journal_tail(const char *journal_path, uid_t uid, size_t len, int time_mode)
 {
 	sd_journal *j;
 	char *match, *buf;
@@ -1069,7 +1069,6 @@ static void print_journal_tail(const char *journal_path, uid_t uid, size_t len)
 	else
 		sd_journal_open(&j, SD_JOURNAL_LOCAL_ONLY);
 
-	buf = xmalloc(sizeof(char) * 16);
 	xasprintf(&match, "_UID=%d", uid);
 
 	sd_journal_add_match(j, match, 0);
@@ -1089,7 +1088,7 @@ static void print_journal_tail(const char *journal_path, uid_t uid, size_t len)
 
 		sd_journal_get_realtime_usec(j, &x);
 		t = x / 1000000;
-		strftime(buf, 16, "%b %d %H:%M:%S", localtime(&t));
+		buf = make_time(time_mode, t);
 
 		fprintf(stdout, "%s", buf);
 
@@ -1148,7 +1147,7 @@ static int print_user_table(struct lslogins_control *ctl)
 		print_pretty(tb);
 #ifdef HAVE_LIBSYSTEMD
 		fprintf(stdout, _("\nLast logs:\n"));
-		print_journal_tail(ctl->journal_path, ctl->uid, 3);
+		print_journal_tail(ctl->journal_path, ctl->uid, 3, ctl->time_mode);
 		fputc('\n', stdout);
 #endif
 	} else
-- 
2.1.3

[PATCH 03/16] lslogins: tell why command failed

[Sami Kerola]

Printing usage() without hint what is wrong does not help an user.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/lslogins.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/login-utils/lslogins.c b/login-utils/lslogins.c
index 85fd7c8..7fe6599 100644
--- a/login-utils/lslogins.c
+++ b/login-utils/lslogins.c
@@ -1437,7 +1437,7 @@ int main(int argc, char *argv[])
 		logins = argv[optind];
 		outmode = OUT_PRETTY;
 	} else if (argc != optind)
-		usage(stderr);
+		errx(EXIT_FAILURE, _("Only one user may be specified. Use -l for multiple users."));
 
 	scols_init_debug(0);
 
-- 
2.1.3

[PATCH 04/16] lslogins: fix short options

[Sami Kerola]

Usage is promising -e is an option alias of --export, so make it work.
And get rid of -x that was accepted, but not in use.

Long only enum member OPT_VER was probably a development time idea, that
never got to be used.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/lslogins.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/login-utils/lslogins.c b/login-utils/lslogins.c
index 7fe6599..cacf83c 100644
--- a/login-utils/lslogins.c
+++ b/login-utils/lslogins.c
@@ -1245,8 +1245,7 @@ int main(int argc, char *argv[])
 
 	/* long only options. */
 	enum {
-		OPT_VER = CHAR_MAX + 1,
-		OPT_WTMP,
+		OPT_WTMP = CHAR_MAX + 1,
 		OPT_BTMP,
 		OPT_NOTRUNC,
 		OPT_NOHEAD,
@@ -1304,7 +1303,7 @@ int main(int argc, char *argv[])
 	add_column(columns, ncolumns++, COL_UID);
 	add_column(columns, ncolumns++, COL_USER);
 
-	while ((c = getopt_long(argc, argv, "acfGg:hLl:no:prsuVxzZ",
+	while ((c = getopt_long(argc, argv, "acefGg:hLl:no:prsuVzZ",
 				longopts, NULL)) != -1) {
 
 		err_exclusive_options(c, longopts, excl, excl_st);
-- 
2.1.3

[PATCH 05/16] lslogins: reject unknown time format arguments

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/lslogins.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/login-utils/lslogins.c b/login-utils/lslogins.c
index cacf83c..4b07636 100644
--- a/login-utils/lslogins.c
+++ b/login-utils/lslogins.c
@@ -1180,16 +1180,6 @@ static void free_user(void *f)
 	free(u);
 }
 
-struct lslogins_timefmt {
-	const char *name;
-	int val;
-};
-
-static struct lslogins_timefmt timefmts[] = {
-	{ "short", TIME_SHORT },
-	{ "full", TIME_FULL },
-	{ "iso", TIME_ISO },
-};
 
 static void __attribute__((__noreturn__)) usage(FILE *out)
 {
@@ -1398,8 +1388,18 @@ int main(int argc, char *argv[])
 			break;
 		case OPT_TIME_FMT:
 			{
+				struct lslogins_timefmt {
+					const char *name;
+					const int val;
+				};
+				const struct lslogins_timefmt timefmts[] = {
+					{ "iso", TIME_ISO },
+					{ "full", TIME_FULL },
+					{ "short", TIME_SHORT },
+				};
 				size_t i;
 
+				ctl->time_mode = TIME_INVALID;
 				for (i = 0; i < ARRAY_SIZE(timefmts); i++) {
 					if (strcmp(timefmts[i].name, optarg) == 0) {
 						ctl->time_mode = timefmts[i].val;
@@ -1407,7 +1407,7 @@ int main(int argc, char *argv[])
 					}
 				}
 				if (ctl->time_mode == TIME_INVALID)
-					usage(stderr);
+					errx(EXIT_FAILURE, _("unknown time format: %s"), optarg);
 			}
 			break;
 		case 'V':
-- 
2.1.3

[PATCH 06/16] lslogins: add space to systemd journal header and message

[Sami Kerola]

This commit changes journal messages in individual user printout the
following way.

Dec 13 16:02:05 systemd[324]:Time has been changed	(old)
Dec 13 16:02:05 systemd[324]: Time has been changed	(new)

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/lslogins.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/login-utils/lslogins.c b/login-utils/lslogins.c
index 4b07636..14a0749 100644
--- a/login-utils/lslogins.c
+++ b/login-utils/lslogins.c
@@ -1058,7 +1058,7 @@ static void fill_table(const void *u, const VISIT which, const int depth __attri
 static void print_journal_tail(const char *journal_path, uid_t uid, size_t len, int time_mode)
 {
 	sd_journal *j;
-	char *match, *buf;
+	char *match, *timestamp;
 	uint64_t x;
 	time_t t;
 	const char *identifier, *pid, *message;
@@ -1088,21 +1088,18 @@ static void print_journal_tail(const char *journal_path, uid_t uid, size_t len,
 
 		sd_journal_get_realtime_usec(j, &x);
 		t = x / 1000000;
-		buf = make_time(time_mode, t);
-
-		fprintf(stdout, "%s", buf);
-
+		timestamp = make_time(time_mode, t);
+		/* Get rid of journal entry field identifiers */
 		identifier = strchr(identifier, '=') + 1;
-		pid = strchr(pid, '=') + 1		;
+		pid = strchr(pid, '=') + 1;
 		message = strchr(message, '=') + 1;
 
-		fprintf(stdout, " %s", identifier);
-		fprintf(stdout, "[%s]:", pid);
-		fprintf(stdout, "%s\n", message);
+		fprintf(stdout, "%s %s[%s]: %s\n", timestamp, identifier, pid,
+			message);
+		free(timestamp);
 	} while (sd_journal_next(j));
 
 done:
-	free(buf);
 	free(match);
 	sd_journal_flush_matches(j);
 	sd_journal_close(j);
-- 
2.1.3

[PATCH 07/16] lslogins: use hardcoded paths from pathnames.h

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 include/pathnames.h    | 1 +
 login-utils/lslogins.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/pathnames.h b/include/pathnames.h
index 1cc4e15..0d21b98 100644
--- a/include/pathnames.h
+++ b/include/pathnames.h
@@ -38,6 +38,7 @@
 #endif
 #define	_PATH_MOTDFILE		"/etc/motd"
 #define	_PATH_NOLOGIN		"/etc/nologin"
+#define	_PATH_VAR_NOLOGIN	"/var/run/nologin"
 
 #define _PATH_LOGIN		"/bin/login"
 #define _PATH_INITTAB		"/etc/inittab"
diff --git a/login-utils/lslogins.c b/login-utils/lslogins.c
index 14a0749..85ae8f5 100644
--- a/login-utils/lslogins.c
+++ b/login-utils/lslogins.c
@@ -688,8 +688,8 @@ static struct lslogins_user *get_user_info(struct lslogins_control *ctl, const c
 			if (strstr(pwd->pw_shell, "nologin"))
 				user->nologin = 1;
 			else if (pwd->pw_uid)
-				user->nologin = access("/etc/nologin", F_OK) == 0 ||
-						access("/var/run/nologin", F_OK) == 0;
+				user->nologin = access(_PATH_NOLOGIN, F_OK) == 0 ||
+						access(_PATH_VAR_NOLOGIN, F_OK) == 0;
 			break;
 		case COL_PWD_WARN:
 			if (shadow && shadow->sp_warn >= 0)
-- 
2.1.3

[PATCH 08/16] chfn: remove function prototypes

[Sami Kerola]

Requires resuffling functions order they work without issues.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/chfn.c | 276 ++++++++++++++++++++++++++---------------------------
 1 file changed, 134 insertions(+), 142 deletions(-)

diff --git a/login-utils/chfn.c b/login-utils/chfn.c
index 25f7e91..35d00f9 100644
--- a/login-utils/chfn.c
+++ b/login-utils/chfn.c
@@ -66,14 +66,6 @@ struct finfo {
 	char *other;
 };
 
-static int parse_argv(int argc, char *argv[], struct finfo *pinfo);
-static void parse_passwd(struct passwd *pw, struct finfo *pinfo);
-static void ask_info(struct finfo *oldfp, struct finfo *newfp);
-static char *prompt(char *question, char *def_val);
-static int check_gecos_string(char *msg, char *gecos);
-static int set_changed_data(struct finfo *oldfp, struct finfo *newfp);
-static int save_new_data(struct finfo *pinfo);
-
 /* we do not accept gecos field sizes longer than MAX_FIELD_SIZE */
 #define MAX_FIELD_SIZE		256
 
@@ -93,99 +85,43 @@ static void __attribute__((__noreturn__)) usage(FILE *fp)
 	exit(fp == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
 }
 
-int main(int argc, char **argv)
+/*
+ *  check_gecos_string () --
+ *	check that the given gecos string is legal.  if it's not legal,
+ *	output "msg" followed by a description of the problem, and return (-1).
+ */
+static int check_gecos_string(char *msg, char *gecos)
 {
-	uid_t uid;
-	struct finfo oldf, newf;
-	int interactive;
-
-	sanitize_env();
-	setlocale(LC_ALL, "");	/* both for messages and for iscntrl() below */
-	bindtextdomain(PACKAGE, LOCALEDIR);
-	textdomain(PACKAGE);
-	atexit(close_stdout);
-
-	/*
-	 *  "oldf" contains the users original finger information.
-	 *  "newf" contains the changed finger information, and contains NULL
-	 *         in fields that haven't been changed.
-	 *  in the end, "newf" is folded into "oldf".
-	 *
-	 *  the reason the new finger information is not put _immediately_
-	 *  into "oldf" is that on the command line, new finger information
-	 *  can be specified before we know what user the information is
-	 *  being specified for.
-	 */
-	uid = getuid();
-	memset(&oldf, 0, sizeof(oldf));
-	memset(&newf, 0, sizeof(newf));
+	unsigned int i, c;
 
-	interactive = parse_argv(argc, argv, &newf);
-	if (!newf.username) {
-		parse_passwd(getpwuid(uid), &oldf);
-		if (!oldf.username)
-			errx(EXIT_FAILURE, _("you (user %d) don't exist."),
-			     uid);
-	} else {
-		parse_passwd(getpwnam(newf.username), &oldf);
-		if (!oldf.username)
-			errx(EXIT_FAILURE, _("user \"%s\" does not exist."),
-			     newf.username);
+	if (strlen(gecos) > MAX_FIELD_SIZE) {
+		if (msg)
+			warnx(_("field %s is too long"), msg);
+		else
+			warnx(_("field is too long"));
+		return -1;
 	}
 
-#ifndef HAVE_LIBUSER
-	if (!(is_local(oldf.username)))
-		errx(EXIT_FAILURE, _("can only change local entries"));
-#endif
-
-#ifdef HAVE_LIBSELINUX
-	if (is_selinux_enabled() > 0) {
-		if (uid == 0) {
-			if (checkAccess(oldf.username, PASSWD__CHFN) != 0) {
-				security_context_t user_context;
-				if (getprevcon(&user_context) < 0)
-					user_context = NULL;
-				errx(EXIT_FAILURE,
-				     _("%s is not authorized to change "
-				       "the finger info of %s"),
-				     user_context ? : _("Unknown user context"),
-				     oldf.username);
-			}
+	for (i = 0; i < strlen(gecos); i++) {
+		c = gecos[i];
+		if (c == ',' || c == ':' || c == '=' || c == '"' || c == '\n') {
+			if (msg)
+				warnx(_("%s: '%c' is not allowed"), msg, c);
+			else
+				warnx(_("'%c' is not allowed"), c);
+			return -1;
+		}
+		if (iscntrl(c)) {
+			if (msg)
+				warnx(_
+				      ("%s: control characters are not allowed"),
+				      msg);
+			else
+				warnx(_("control characters are not allowed"));
+			return -1;
 		}
-		if (setupDefaultContext(_PATH_PASSWD))
-			errx(EXIT_FAILURE,
-			     _("can't set default context for %s"), _PATH_PASSWD);
-	}
-#endif
-
-#ifdef HAVE_LIBUSER
-	/* If we're setuid and not really root, disallow the password change. */
-	if (geteuid() != getuid() && uid != oldf.pw->pw_uid) {
-#else
-	if (uid != 0 && uid != oldf.pw->pw_uid) {
-#endif
-		errno = EACCES;
-		err(EXIT_FAILURE, _("running UID doesn't match UID of user we're "
-		      "altering, change denied"));
-	}
-
-	printf(_("Changing finger information for %s.\n"), oldf.username);
-
-#if !defined(HAVE_LIBUSER) && defined(CHFN_CHSH_PASSWORD)
-	if(!auth_pam("chfn", uid, oldf.username)) {
-		return EXIT_FAILURE;
-	}
-#endif
-
-	if (interactive)
-		ask_info(&oldf, &newf);
-
-	if (!set_changed_data(&oldf, &newf)) {
-		printf(_("Finger information not changed.\n"));
-		return EXIT_SUCCESS;
 	}
-
-	return save_new_data(&oldf) == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
+	return 0;
 }
 
 /*
@@ -307,19 +243,6 @@ static void parse_passwd(struct passwd *pw, struct finfo *pinfo)
 }
 
 /*
- *  ask_info () --
- *	prompt the user for the finger information and store it.
- */
-static void ask_info(struct finfo *oldfp, struct finfo *newfp)
-{
-	newfp->full_name = prompt(_("Name"), oldfp->full_name);
-	newfp->office = prompt(_("Office"), oldfp->office);
-	newfp->office_phone = prompt(_("Office Phone"), oldfp->office_phone);
-	newfp->home_phone = prompt(_("Home Phone"), oldfp->home_phone);
-	printf("\n");
-}
-
-/*
  *  prompt () --
  *	ask the user for a given field and check that the string is legal.
  */
@@ -357,42 +280,16 @@ static char *prompt(char *question, char *def_val)
 }
 
 /*
- *  check_gecos_string () --
- *	check that the given gecos string is legal.  if it's not legal,
- *	output "msg" followed by a description of the problem, and return (-1).
+ *  ask_info () --
+ *	prompt the user for the finger information and store it.
  */
-static int check_gecos_string(char *msg, char *gecos)
+static void ask_info(struct finfo *oldfp, struct finfo *newfp)
 {
-	unsigned int i, c;
-
-	if (strlen(gecos) > MAX_FIELD_SIZE) {
-		if (msg)
-			warnx(_("field %s is too long"), msg);
-		else
-			warnx(_("field is too long"));
-		return -1;
-	}
-
-	for (i = 0; i < strlen(gecos); i++) {
-		c = gecos[i];
-		if (c == ',' || c == ':' || c == '=' || c == '"' || c == '\n') {
-			if (msg)
-				warnx(_("%s: '%c' is not allowed"), msg, c);
-			else
-				warnx(_("'%c' is not allowed"), c);
-			return -1;
-		}
-		if (iscntrl(c)) {
-			if (msg)
-				warnx(_
-				      ("%s: control characters are not allowed"),
-				      msg);
-			else
-				warnx(_("control characters are not allowed"));
-			return -1;
-		}
-	}
-	return 0;
+	newfp->full_name = prompt(_("Name"), oldfp->full_name);
+	newfp->office = prompt(_("Office"), oldfp->office);
+	newfp->office_phone = prompt(_("Office Phone"), oldfp->office_phone);
+	newfp->home_phone = prompt(_("Home Phone"), oldfp->home_phone);
+	printf("\n");
 }
 
 /*
@@ -476,3 +373,98 @@ static int save_new_data(struct finfo *pinfo)
 	printf(_("Finger information changed.\n"));
 	return 0;
 }
+
+int main(int argc, char **argv)
+{
+	uid_t uid;
+	struct finfo oldf, newf;
+	int interactive;
+
+	sanitize_env();
+	setlocale(LC_ALL, "");	/* both for messages and for iscntrl() below */
+	bindtextdomain(PACKAGE, LOCALEDIR);
+	textdomain(PACKAGE);
+	atexit(close_stdout);
+
+	/*
+	 *  "oldf" contains the users original finger information.
+	 *  "newf" contains the changed finger information, and contains NULL
+	 *         in fields that haven't been changed.
+	 *  in the end, "newf" is folded into "oldf".
+	 *
+	 *  the reason the new finger information is not put _immediately_
+	 *  into "oldf" is that on the command line, new finger information
+	 *  can be specified before we know what user the information is
+	 *  being specified for.
+	 */
+	uid = getuid();
+	memset(&oldf, 0, sizeof(oldf));
+	memset(&newf, 0, sizeof(newf));
+
+	interactive = parse_argv(argc, argv, &newf);
+	if (!newf.username) {
+		parse_passwd(getpwuid(uid), &oldf);
+		if (!oldf.username)
+			errx(EXIT_FAILURE, _("you (user %d) don't exist."),
+			     uid);
+	} else {
+		parse_passwd(getpwnam(newf.username), &oldf);
+		if (!oldf.username)
+			errx(EXIT_FAILURE, _("user \"%s\" does not exist."),
+			     newf.username);
+	}
+
+#ifndef HAVE_LIBUSER
+	if (!(is_local(oldf.username)))
+		errx(EXIT_FAILURE, _("can only change local entries"));
+#endif
+
+#ifdef HAVE_LIBSELINUX
+	if (is_selinux_enabled() > 0) {
+		if (uid == 0) {
+			if (checkAccess(oldf.username, PASSWD__CHFN) != 0) {
+				security_context_t user_context;
+				if (getprevcon(&user_context) < 0)
+					user_context = NULL;
+				errx(EXIT_FAILURE,
+				     _("%s is not authorized to change "
+				       "the finger info of %s"),
+				     user_context ? : _("Unknown user context"),
+				     oldf.username);
+			}
+		}
+		if (setupDefaultContext(_PATH_PASSWD))
+			errx(EXIT_FAILURE,
+			     _("can't set default context for %s"), _PATH_PASSWD);
+	}
+#endif
+
+#ifdef HAVE_LIBUSER
+	/* If we're setuid and not really root, disallow the password change. */
+	if (geteuid() != getuid() && uid != oldf.pw->pw_uid) {
+#else
+	if (uid != 0 && uid != oldf.pw->pw_uid) {
+#endif
+		errno = EACCES;
+		err(EXIT_FAILURE, _("running UID doesn't match UID of user we're "
+		      "altering, change denied"));
+	}
+
+	printf(_("Changing finger information for %s.\n"), oldf.username);
+
+#if !defined(HAVE_LIBUSER) && defined(CHFN_CHSH_PASSWORD)
+	if(!auth_pam("chfn", uid, oldf.username)) {
+		return EXIT_FAILURE;
+	}
+#endif
+
+	if (interactive)
+		ask_info(&oldf, &newf);
+
+	if (!set_changed_data(&oldf, &newf)) {
+		printf(_("Finger information not changed.\n"));
+		return EXIT_SUCCESS;
+	}
+
+	return save_new_data(&oldf) == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
+}
-- 
2.1.3

[PATCH 09/16] chfn: rewrite prompt() to use strutils

[Sami Kerola]

The left and right white space trimming can be done with strutils.h
[lr]trim_whitespace() functions.

As a minor fix when user input exceeds maxium allowed gecos field length
the remaining characters in stdin are purged so that re-prompting works
correctly.

Additionally the prompt() is made to add message to check_gecos_string(),
so that there are less similar strings for translation project to deal.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/chfn.c | 59 ++++++++++++++++++------------------------------------
 1 file changed, 20 insertions(+), 39 deletions(-)

diff --git a/login-utils/chfn.c b/login-utils/chfn.c
index 35d00f9..ca6f456 100644
--- a/login-utils/chfn.c
+++ b/login-utils/chfn.c
@@ -54,8 +54,6 @@
 # include "auth.h"
 #endif
 
-static char buf[1024];
-
 struct finfo {
 	struct passwd *pw;
 	char *username;
@@ -90,34 +88,23 @@ static void __attribute__((__noreturn__)) usage(FILE *fp)
  *	check that the given gecos string is legal.  if it's not legal,
  *	output "msg" followed by a description of the problem, and return (-1).
  */
-static int check_gecos_string(char *msg, char *gecos)
+static int check_gecos_string(const char *msg, char *gecos)
 {
 	unsigned int i, c;
+	const size_t len = strlen(gecos);
 
-	if (strlen(gecos) > MAX_FIELD_SIZE) {
-		if (msg)
-			warnx(_("field %s is too long"), msg);
-		else
-			warnx(_("field is too long"));
+	if (MAX_FIELD_SIZE < len) {
+		warnx(_("field %s is too long"), msg);
 		return -1;
 	}
-
-	for (i = 0; i < strlen(gecos); i++) {
+	for (i = 0; i < len; i++) {
 		c = gecos[i];
 		if (c == ',' || c == ':' || c == '=' || c == '"' || c == '\n') {
-			if (msg)
-				warnx(_("%s: '%c' is not allowed"), msg, c);
-			else
-				warnx(_("'%c' is not allowed"), c);
+			warnx(_("%s: '%c' is not allowed"), msg, c);
 			return -1;
 		}
 		if (iscntrl(c)) {
-			if (msg)
-				warnx(_
-				      ("%s: control characters are not allowed"),
-				      msg);
-			else
-				warnx(_("control characters are not allowed"));
+			warnx(_("%s: control characters are not allowed"), msg);
 			return -1;
 		}
 	}
@@ -246,37 +233,31 @@ static void parse_passwd(struct passwd *pw, struct finfo *pinfo)
  *  prompt () --
  *	ask the user for a given field and check that the string is legal.
  */
-static char *prompt(char *question, char *def_val)
+static char *prompt(const char *question, char *def_val)
 {
-	static char *blank = "none";
 	int len;
-	char *ans, *cp;
+	char *ans;
+	char buf[MAX_FIELD_SIZE + 2];
 
+	if (!def_val)
+		def_val = "";
 	while (true) {
-		if (!def_val)
-			def_val = "";
 		printf("%s [%s]: ", question, def_val);
-		*buf = 0;
+		__fpurge(stdin);
 		if (fgets(buf, sizeof(buf), stdin) == NULL)
 			errx(EXIT_FAILURE, _("Aborted."));
-		/* remove the newline at the end of buf. */
 		ans = buf;
-		while (isspace(*ans))
-			ans++;
-		len = strlen(ans);
-		while (len > 0 && isspace(ans[len - 1]))
-			len--;
-		if (len <= 0)
+		/* remove white spaces from string end */
+		ltrim_whitespace((unsigned char *) ans);
+		len = rtrim_whitespace((unsigned char *) ans);
+		if (len == 0)
 			return NULL;
-		ans[len] = 0;
-		if (!strcasecmp(ans, blank))
+		if (!strcasecmp(ans, "none"))
 			return "";
-		if (check_gecos_string(NULL, ans) >= 0)
+		if (check_gecos_string(question, ans) >= 0)
 			break;
 	}
-	cp = (char *)xmalloc(len + 1);
-	strcpy(cp, ans);
-	return cp;
+	return xstrdup(ans);
 }
 
 /*
-- 
2.1.3

[PATCH 10/16] chfn: use xasprintf() rather than bunch of strlen() and malloc() calls

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/chfn.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/login-utils/chfn.c b/login-utils/chfn.c
index ca6f456..4746927 100644
--- a/login-utils/chfn.c
+++ b/login-utils/chfn.c
@@ -324,12 +324,8 @@ static int save_new_data(struct finfo *pinfo)
 		pinfo->other = "";
 
 	/* create the new gecos string */
-	len = (strlen(pinfo->full_name) + strlen(pinfo->office) +
-	       strlen(pinfo->office_phone) + strlen(pinfo->home_phone) +
-	       strlen(pinfo->other) + 4);
-	gecos = (char *)xmalloc(len + 1);
-	sprintf(gecos, "%s,%s,%s,%s,%s", pinfo->full_name, pinfo->office,
-		pinfo->office_phone, pinfo->home_phone, pinfo->other);
+	len = xasprintf(&gecos, "%s,%s,%s,%s,%s", pinfo->full_name, pinfo->office,
+		  pinfo->office_phone, pinfo->home_phone, pinfo->other);
 
 	/* remove trailing empty fields (but not subfields of pinfo->other) */
 	if (!pinfo->other[0]) {
@@ -351,6 +347,7 @@ static int save_new_data(struct finfo *pinfo)
 		       ("Finger information *NOT* changed.  Try again later.\n"));
 		return -1;
 	}
+	free(gecos);
 	printf(_("Finger information changed.\n"));
 	return 0;
 }
-- 
2.1.3

[PATCH 11/16] chfn: fix usage() regression

[Sami Kerola]

Commit db433bf737a5fd4e1c7cca5e3603934743eebd1c changed -u for --help to
-h, that is not true.  The -h is short hand for --home-phone.  And the
--version is accompanied with -v not -V.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/chfn.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/login-utils/chfn.c b/login-utils/chfn.c
index 4746927..9833591 100644
--- a/login-utils/chfn.c
+++ b/login-utils/chfn.c
@@ -77,8 +77,8 @@ static void __attribute__((__noreturn__)) usage(FILE *fp)
 	fputs(_(" -p, --office-phone <phone>   office phone number\n"), fp);
 	fputs(_(" -h, --home-phone <phone>     home phone number\n"), fp);
 	fputs(USAGE_SEPARATOR, fp);
-	fputs(USAGE_HELP, fp);
-	fputs(USAGE_VERSION, fp);
+	fputs(_(" -u, --help     display this help and exit\n"), fp);
+	fputs(_(" -v, --version  output version information and exit\n"), fp);
 	fprintf(fp, USAGE_MAN_TAIL("chfn(1)"));
 	exit(fp == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
 }
-- 
2.1.3

[PATCH 12/16] chfn: simplify parse_passwd() by using strsep()

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/chfn.c | 51 +++++++++++++++------------------------------------
 1 file changed, 15 insertions(+), 36 deletions(-)

diff --git a/login-utils/chfn.c b/login-utils/chfn.c
index 9833591..7ba1a51 100644
--- a/login-utils/chfn.c
+++ b/login-utils/chfn.c
@@ -191,42 +191,21 @@ static int parse_argv(int argc, char *argv[], struct finfo *pinfo)
 static void parse_passwd(struct passwd *pw, struct finfo *pinfo)
 {
 	char *gecos;
-	char *cp;
-
-	if (pw) {
-		pinfo->pw = pw;
-		pinfo->username = pw->pw_name;
-		/* use pw_gecos - we take a copy since PAM destroys the original */
-		gecos = xstrdup(pw->pw_gecos);
-		cp = (gecos ? gecos : "");
-		pinfo->full_name = cp;
-		cp = strchr(cp, ',');
-		if (cp) {
-			*cp = 0, cp++;
-		} else
-			return;
-		pinfo->office = cp;
-		cp = strchr(cp, ',');
-		if (cp) {
-			*cp = 0, cp++;
-		} else
-			return;
-		pinfo->office_phone = cp;
-		cp = strchr(cp, ',');
-		if (cp) {
-			*cp = 0, cp++;
-		} else
-			return;
-		pinfo->home_phone = cp;
-		/*  extra fields contain site-specific information, and can
-		 *  not be changed by this version of chfn.  */
-		cp = strchr(cp, ',');
-		if (cp) {
-			*cp = 0, cp++;
-		} else
-			return;
-		pinfo->other = cp;
-	}
+
+	if (!pw)
+		return;
+	pinfo->pw = pw;
+	pinfo->username = pw->pw_name;
+	/* use pw_gecos - we take a copy since PAM destroys the original */
+	gecos = xstrdup(pw->pw_gecos);
+	/* extract known fields */
+	pinfo->full_name = strsep(&gecos, ",");
+	pinfo->office = strsep(&gecos, ",");
+	pinfo->office_phone = strsep(&gecos, ",");
+	pinfo->home_phone = strsep(&gecos, ",");
+	/*  extra fields contain site-specific information, and can
+	 *  not be changed by this version of chfn.  */
+	pinfo->other = strsep(&gecos, ",");
 }
 
 /*
-- 
2.1.3

[PATCH 13/16] chfn: add minimalistic struct chfn_control

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/chfn.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/login-utils/chfn.c b/login-utils/chfn.c
index 7ba1a51..81abd11 100644
--- a/login-utils/chfn.c
+++ b/login-utils/chfn.c
@@ -64,6 +64,11 @@ struct finfo {
 	char *other;
 };
 
+struct chfn_control {
+	unsigned int
+		interactive:1;		/* whether to prompt for fields or not */
+};
+
 /* we do not accept gecos field sizes longer than MAX_FIELD_SIZE */
 #define MAX_FIELD_SIZE		256
 
@@ -116,10 +121,9 @@ static int check_gecos_string(const char *msg, char *gecos)
  *	parse the command line arguments.
  *	returns true if no information beyond the username was given.
  */
-static int parse_argv(int argc, char *argv[], struct finfo *pinfo)
+static void parse_argv(struct chfn_control *ctl, int argc, char *argv[], struct finfo *pinfo)
 {
 	int index, c, status;
-	int info_given;
 
 	static struct option long_options[] = {
 		{"full-name", required_argument, 0, 'f'},
@@ -132,7 +136,6 @@ static int parse_argv(int argc, char *argv[], struct finfo *pinfo)
 	};
 
 	optind = 0;
-	info_given = false;
 	while (true) {
 		c = getopt_long(argc, argv, "f:r:p:h:o:uv", long_options,
 				&index);
@@ -149,7 +152,7 @@ static int parse_argv(int argc, char *argv[], struct finfo *pinfo)
 		if (!optarg)
 			usage(stderr);
 		/* ok, we were given an argument */
-		info_given = true;
+		ctl->interactive = 0;
 
 		/* now store the argument */
 		switch (c) {
@@ -181,7 +184,7 @@ static int parse_argv(int argc, char *argv[], struct finfo *pinfo)
 			usage(stderr);
 		pinfo->username = argv[optind];
 	}
-	return !info_given;
+	return;
 }
 
 /*
@@ -335,7 +338,9 @@ int main(int argc, char **argv)
 {
 	uid_t uid;
 	struct finfo oldf, newf;
-	int interactive;
+	struct chfn_control ctl = {
+		.interactive = 1
+	};
 
 	sanitize_env();
 	setlocale(LC_ALL, "");	/* both for messages and for iscntrl() below */
@@ -358,7 +363,7 @@ int main(int argc, char **argv)
 	memset(&oldf, 0, sizeof(oldf));
 	memset(&newf, 0, sizeof(newf));
 
-	interactive = parse_argv(argc, argv, &newf);
+	parse_argv(&ctl, argc, argv, &newf);
 	if (!newf.username) {
 		parse_passwd(getpwuid(uid), &oldf);
 		if (!oldf.username)
@@ -415,7 +420,7 @@ int main(int argc, char **argv)
 	}
 #endif
 
-	if (interactive)
+	if (ctl.interactive)
 		ask_info(&oldf, &newf);
 
 	if (!set_changed_data(&oldf, &newf)) {
-- 
2.1.3

[PATCH 14/16] chfn: clean up parse_argv()

[Sami Kerola]

Use switch() case ?: for all option parsing, as in most of the other
source files.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/chfn.c | 48 ++++++++++++++++++------------------------------
 1 file changed, 18 insertions(+), 30 deletions(-)

diff --git a/login-utils/chfn.c b/login-utils/chfn.c
index 81abd11..0d37a32 100644
--- a/login-utils/chfn.c
+++ b/login-utils/chfn.c
@@ -121,11 +121,11 @@ static int check_gecos_string(const char *msg, char *gecos)
  *	parse the command line arguments.
  *	returns true if no information beyond the username was given.
  */
-static void parse_argv(struct chfn_control *ctl, int argc, char *argv[], struct finfo *pinfo)
+static void parse_argv(struct chfn_control *ctl, int argc, char *argv[],
+		       struct finfo *pinfo)
 {
-	int index, c, status;
-
-	static struct option long_options[] = {
+	int index, c, status = 0;
+	const struct option long_options[] = {
 		{"full-name", required_argument, 0, 'f'},
 		{"office", required_argument, 0, 'o'},
 		{"office-phone", required_argument, 0, 'p'},
@@ -135,49 +135,37 @@ static void parse_argv(struct chfn_control *ctl, int argc, char *argv[], struct
 		{NULL, no_argument, 0, '0'},
 	};
 
-	optind = 0;
-	while (true) {
-		c = getopt_long(argc, argv, "f:r:p:h:o:uv", long_options,
-				&index);
-		if (c == -1)
-			break;
-		/* version?  output version and exit. */
-		if (c == 'v') {
-			printf(UTIL_LINUX_VERSION);
-			exit(EXIT_SUCCESS);
-		}
-		if (c == 'u')
-			usage(stdout);
-		/* all other options must have an argument. */
-		if (!optarg)
-			usage(stderr);
-		/* ok, we were given an argument */
-		ctl->interactive = 0;
-
-		/* now store the argument */
+	while ((c = getopt_long(argc, argv, "f:r:p:h:o:uv", long_options,
+				&index)) != -1) {
 		switch (c) {
 		case 'f':
 			pinfo->full_name = optarg;
-			status = check_gecos_string(_("Name"), optarg);
+			status += check_gecos_string(_("Name"), optarg);
 			break;
 		case 'o':
 			pinfo->office = optarg;
-			status = check_gecos_string(_("Office"), optarg);
+			status += check_gecos_string(_("Office"), optarg);
 			break;
 		case 'p':
 			pinfo->office_phone = optarg;
-			status = check_gecos_string(_("Office Phone"), optarg);
+			status += check_gecos_string(_("Office Phone"), optarg);
 			break;
 		case 'h':
 			pinfo->home_phone = optarg;
-			status = check_gecos_string(_("Home Phone"), optarg);
+			status += check_gecos_string(_("Home Phone"), optarg);
 			break;
+		case 'v':
+			printf(UTIL_LINUX_VERSION);
+			exit(EXIT_SUCCESS);
+		case 'u':
+			usage(stdout);
 		default:
 			usage(stderr);
 		}
-		if (status != 0)
-			exit(EXIT_FAILURE);
+		ctl->interactive = 0;
 	}
+	if (status != 0)
+		exit(EXIT_FAILURE);
 	/* done parsing arguments.  check for a username. */
 	if (optind < argc) {
 		if (optind + 1 < argc)
-- 
2.1.3

[PATCH 15/16] chfn: move new and old finger structs to chfn control struct

[Sami Kerola]

This change is a little bit messy, and requires a comment the struct
finfo should not have 'struct passwd *pw' as it's member.  The earlier
struct design would have been burden to maintain, and confusing to use.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/chfn.c | 165 ++++++++++++++++++++++++-----------------------------
 1 file changed, 76 insertions(+), 89 deletions(-)

diff --git a/login-utils/chfn.c b/login-utils/chfn.c
index 0d37a32..2a06fec 100644
--- a/login-utils/chfn.c
+++ b/login-utils/chfn.c
@@ -55,8 +55,6 @@
 #endif
 
 struct finfo {
-	struct passwd *pw;
-	char *username;
 	char *full_name;
 	char *office;
 	char *office_phone;
@@ -65,6 +63,13 @@ struct finfo {
 };
 
 struct chfn_control {
+	struct passwd *pw;
+	char *username;
+	/*  "oldf"  Contains the users original finger information.
+	 *  "newf"  Contains the changed finger information, and contains
+	 *          NULL in fields that haven't been changed.
+	 *  In the end, "newf" is folded into "oldf".  */
+	struct finfo oldf, newf;
 	unsigned int
 		interactive:1;		/* whether to prompt for fields or not */
 };
@@ -121,8 +126,7 @@ static int check_gecos_string(const char *msg, char *gecos)
  *	parse the command line arguments.
  *	returns true if no information beyond the username was given.
  */
-static void parse_argv(struct chfn_control *ctl, int argc, char *argv[],
-		       struct finfo *pinfo)
+static void parse_argv(struct chfn_control *ctl, int argc, char **argv)
 {
 	int index, c, status = 0;
 	const struct option long_options[] = {
@@ -139,19 +143,19 @@ static void parse_argv(struct chfn_control *ctl, int argc, char *argv[],
 				&index)) != -1) {
 		switch (c) {
 		case 'f':
-			pinfo->full_name = optarg;
+			ctl->newf.full_name = optarg;
 			status += check_gecos_string(_("Name"), optarg);
 			break;
 		case 'o':
-			pinfo->office = optarg;
+			ctl->newf.office = optarg;
 			status += check_gecos_string(_("Office"), optarg);
 			break;
 		case 'p':
-			pinfo->office_phone = optarg;
+			ctl->newf.office_phone = optarg;
 			status += check_gecos_string(_("Office Phone"), optarg);
 			break;
 		case 'h':
-			pinfo->home_phone = optarg;
+			ctl->newf.home_phone = optarg;
 			status += check_gecos_string(_("Home Phone"), optarg);
 			break;
 		case 'v':
@@ -170,7 +174,7 @@ static void parse_argv(struct chfn_control *ctl, int argc, char *argv[],
 	if (optind < argc) {
 		if (optind + 1 < argc)
 			usage(stderr);
-		pinfo->username = argv[optind];
+		ctl->username = argv[optind];
 	}
 	return;
 }
@@ -179,24 +183,22 @@ static void parse_argv(struct chfn_control *ctl, int argc, char *argv[],
  *  parse_passwd () --
  *	take a struct password and fill in the fields of the struct finfo.
  */
-static void parse_passwd(struct passwd *pw, struct finfo *pinfo)
+static void parse_passwd(struct chfn_control *ctl)
 {
 	char *gecos;
 
-	if (!pw)
+	if (!ctl->pw)
 		return;
-	pinfo->pw = pw;
-	pinfo->username = pw->pw_name;
 	/* use pw_gecos - we take a copy since PAM destroys the original */
-	gecos = xstrdup(pw->pw_gecos);
+	gecos = xstrdup(ctl->pw->pw_gecos);
 	/* extract known fields */
-	pinfo->full_name = strsep(&gecos, ",");
-	pinfo->office = strsep(&gecos, ",");
-	pinfo->office_phone = strsep(&gecos, ",");
-	pinfo->home_phone = strsep(&gecos, ",");
+	ctl->oldf.full_name = strsep(&gecos, ",");
+	ctl->oldf.office = strsep(&gecos, ",");
+	ctl->oldf.office_phone = strsep(&gecos, ",");
+	ctl->oldf.home_phone = strsep(&gecos, ",");
 	/*  extra fields contain site-specific information, and can
 	 *  not be changed by this version of chfn.  */
-	pinfo->other = strsep(&gecos, ",");
+	ctl->oldf.other = strsep(&gecos, ",");
 }
 
 /*
@@ -234,12 +236,12 @@ static char *prompt(const char *question, char *def_val)
  *  ask_info () --
  *	prompt the user for the finger information and store it.
  */
-static void ask_info(struct finfo *oldfp, struct finfo *newfp)
+static void ask_info(struct chfn_control *ctl)
 {
-	newfp->full_name = prompt(_("Name"), oldfp->full_name);
-	newfp->office = prompt(_("Office"), oldfp->office);
-	newfp->office_phone = prompt(_("Office Phone"), oldfp->office_phone);
-	newfp->home_phone = prompt(_("Home Phone"), oldfp->home_phone);
+	ctl->newf.full_name = prompt(_("Name"), ctl->oldf.full_name);
+	ctl->newf.office = prompt(_("Office"), ctl->oldf.office);
+	ctl->newf.office_phone = prompt(_("Office Phone"), ctl->oldf.office_phone);
+	ctl->newf.home_phone = prompt(_("Home Phone"), ctl->oldf.home_phone);
 	printf("\n");
 }
 
@@ -247,27 +249,26 @@ static void ask_info(struct finfo *oldfp, struct finfo *newfp)
  *  set_changed_data () --
  *	incorporate the new data into the old finger info.
  */
-static int set_changed_data(struct finfo *oldfp, struct finfo *newfp)
+static int set_changed_data(struct chfn_control *ctl)
 {
 	int changed = false;
 
-	if (newfp->full_name) {
-		oldfp->full_name = newfp->full_name;
+	if (ctl->newf.full_name)
 		changed = true;
-	}
-	if (newfp->office) {
-		oldfp->office = newfp->office;
+	else
+		ctl->newf.full_name = ctl->oldf.full_name;
+	if (ctl->newf.office)
 		changed = true;
-	}
-	if (newfp->office_phone) {
-		oldfp->office_phone = newfp->office_phone;
+	else
+		ctl->newf.office = ctl->oldf.office;
+	if (ctl->newf.office_phone)
 		changed = true;
-	}
-	if (newfp->home_phone) {
-		oldfp->home_phone = newfp->home_phone;
+	else
+		ctl->newf.office_phone = ctl->oldf.office_phone;
+	if (ctl->newf.home_phone)
 		changed = true;
-	}
-
+	else
+		ctl->newf.home_phone = ctl->oldf.home_phone;
 	return changed;
 }
 
@@ -276,41 +277,41 @@ static int set_changed_data(struct finfo *oldfp, struct finfo *newfp)
  *	save the given finger info in /etc/passwd.
  *	return zero on success.
  */
-static int save_new_data(struct finfo *pinfo)
+static int save_new_data(struct chfn_control *ctl)
 {
 	char *gecos;
 	int len;
 
 	/* null fields will confuse printf(). */
-	if (!pinfo->full_name)
-		pinfo->full_name = "";
-	if (!pinfo->office)
-		pinfo->office = "";
-	if (!pinfo->office_phone)
-		pinfo->office_phone = "";
-	if (!pinfo->home_phone)
-		pinfo->home_phone = "";
-	if (!pinfo->other)
-		pinfo->other = "";
+	if (!ctl->newf.full_name)
+		ctl->newf.full_name = "";
+	if (!ctl->newf.office)
+		ctl->newf.office = "";
+	if (!ctl->newf.office_phone)
+		ctl->newf.office_phone = "";
+	if (!ctl->newf.home_phone)
+		ctl->newf.home_phone = "";
+	if (!ctl->newf.other)
+		ctl->newf.other = "";
 
 	/* create the new gecos string */
-	len = xasprintf(&gecos, "%s,%s,%s,%s,%s", pinfo->full_name, pinfo->office,
-		  pinfo->office_phone, pinfo->home_phone, pinfo->other);
+	len = xasprintf(&gecos, "%s,%s,%s,%s,%s", ctl->newf.full_name, ctl->newf.office,
+		  ctl->newf.office_phone, ctl->newf.home_phone, ctl->newf.other);
 
-	/* remove trailing empty fields (but not subfields of pinfo->other) */
-	if (!pinfo->other[0]) {
+	/* remove trailing empty fields (but not subfields of ctl->newf.other) */
+	if (!ctl->newf.other[0]) {
 		while (len > 0 && gecos[len - 1] == ',')
 			len--;
 		gecos[len] = 0;
 	}
 
 #ifdef HAVE_LIBUSER
-	if (set_value_libuser("chfn", pinfo->pw->pw_name, pinfo->pw->pw_uid,
+	if (set_value_libuser("chfn", ctl->username, ctl->pw->pw_uid,
 			LU_GECOS, gecos) < 0) {
 #else /* HAVE_LIBUSER */
 	/* write the new struct passwd to the passwd file. */
-	pinfo->pw->pw_gecos = gecos;
-	if (setpwnam(pinfo->pw) < 0) {
+	ctl->pw->pw_gecos = gecos;
+	if (setpwnam(ctl->pw) < 0) {
 		warn("setpwnam failed");
 #endif
 		printf(_
@@ -325,7 +326,6 @@ static int save_new_data(struct finfo *pinfo)
 int main(int argc, char **argv)
 {
 	uid_t uid;
-	struct finfo oldf, newf;
 	struct chfn_control ctl = {
 		.interactive = 1
 	};
@@ -335,44 +335,31 @@ int main(int argc, char **argv)
 	bindtextdomain(PACKAGE, LOCALEDIR);
 	textdomain(PACKAGE);
 	atexit(close_stdout);
-
-	/*
-	 *  "oldf" contains the users original finger information.
-	 *  "newf" contains the changed finger information, and contains NULL
-	 *         in fields that haven't been changed.
-	 *  in the end, "newf" is folded into "oldf".
-	 *
-	 *  the reason the new finger information is not put _immediately_
-	 *  into "oldf" is that on the command line, new finger information
-	 *  can be specified before we know what user the information is
-	 *  being specified for.
-	 */
 	uid = getuid();
-	memset(&oldf, 0, sizeof(oldf));
-	memset(&newf, 0, sizeof(newf));
 
-	parse_argv(&ctl, argc, argv, &newf);
-	if (!newf.username) {
-		parse_passwd(getpwuid(uid), &oldf);
-		if (!oldf.username)
+	parse_argv(&ctl, argc, argv);
+	if (!ctl.username) {
+		ctl.pw = getpwuid(uid);
+		if (!ctl.pw)
 			errx(EXIT_FAILURE, _("you (user %d) don't exist."),
 			     uid);
+		ctl.username = ctl.pw->pw_name;
 	} else {
-		parse_passwd(getpwnam(newf.username), &oldf);
-		if (!oldf.username)
+		ctl.pw = getpwnam(ctl.username);
+		if (!ctl.pw)
 			errx(EXIT_FAILURE, _("user \"%s\" does not exist."),
-			     newf.username);
+			     ctl.username);
 	}
-
+	parse_passwd(&ctl);
 #ifndef HAVE_LIBUSER
-	if (!(is_local(oldf.username)))
+	if (!(is_local(ctl.username)))
 		errx(EXIT_FAILURE, _("can only change local entries"));
 #endif
 
 #ifdef HAVE_LIBSELINUX
 	if (is_selinux_enabled() > 0) {
 		if (uid == 0) {
-			if (checkAccess(oldf.username, PASSWD__CHFN) != 0) {
+			if (checkAccess(ctl.username, PASSWD__CHFN) != 0) {
 				security_context_t user_context;
 				if (getprevcon(&user_context) < 0)
 					user_context = NULL;
@@ -380,7 +367,7 @@ int main(int argc, char **argv)
 				     _("%s is not authorized to change "
 				       "the finger info of %s"),
 				     user_context ? : _("Unknown user context"),
-				     oldf.username);
+				     ctl.username);
 			}
 		}
 		if (setupDefaultContext(_PATH_PASSWD))
@@ -391,30 +378,30 @@ int main(int argc, char **argv)
 
 #ifdef HAVE_LIBUSER
 	/* If we're setuid and not really root, disallow the password change. */
-	if (geteuid() != getuid() && uid != oldf.pw->pw_uid) {
+	if (geteuid() != getuid() && uid != ctl.pw->pw_uid) {
 #else
-	if (uid != 0 && uid != oldf.pw->pw_uid) {
+	if (uid != 0 && uid != ctl.oldf.pw->pw_uid) {
 #endif
 		errno = EACCES;
 		err(EXIT_FAILURE, _("running UID doesn't match UID of user we're "
 		      "altering, change denied"));
 	}
 
-	printf(_("Changing finger information for %s.\n"), oldf.username);
+	printf(_("Changing finger information for %s.\n"), ctl.username);
 
 #if !defined(HAVE_LIBUSER) && defined(CHFN_CHSH_PASSWORD)
-	if(!auth_pam("chfn", uid, oldf.username)) {
+	if (!auth_pam("chfn", uid, ctl.username)) {
 		return EXIT_FAILURE;
 	}
 #endif
 
 	if (ctl.interactive)
-		ask_info(&oldf, &newf);
+		ask_info(&ctl);
 
-	if (!set_changed_data(&oldf, &newf)) {
+	if (!set_changed_data(&ctl)) {
 		printf(_("Finger information not changed.\n"));
 		return EXIT_SUCCESS;
 	}
 
-	return save_new_data(&oldf) == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
+	return save_new_data(&ctl) == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
-- 
2.1.3

[PATCH 16/16] chfn: make command to obey login.defs CHFN_RESTRICT instructions

[Sami Kerola]

Reference: http://man7.org/linux/man-pages/man5/login.defs.5.html
Addresses: https://bugzilla.redhat.com/show_bug.cgi?id=138519
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/Makemodule.am |  6 +++-
 login-utils/chfn.c        | 77 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 78 insertions(+), 5 deletions(-)

diff --git a/login-utils/Makemodule.am b/login-utils/Makemodule.am
index 34c5fb4..bdf03ad 100644
--- a/login-utils/Makemodule.am
+++ b/login-utils/Makemodule.am
@@ -113,7 +113,11 @@ chfn_chsh_sources += \
 chfn_chsh_ldadd += -lselinux
 endif
 
-chfn_SOURCES = login-utils/chfn.c $(chfn_chsh_sources)
+chfn_SOURCES = \
+	login-utils/chfn.c \
+	login-utils/logindefs.c \
+	login-utils/logindefs.h \
+	$(chfn_chsh_sources)
 chfn_CFLAGS = $(chfn_chsh_cflags)
 chfn_LDFLAGS = $(chfn_chsh_ldflags)
 chfn_LDADD = $(LDADD) $(chfn_chsh_ldadd)
diff --git a/login-utils/chfn.c b/login-utils/chfn.c
index 2a06fec..c81dbd5 100644
--- a/login-utils/chfn.c
+++ b/login-utils/chfn.c
@@ -40,6 +40,7 @@
 #include "setpwnam.h"
 #include "strutils.h"
 #include "xalloc.h"
+#include "logindefs.h"
 
 #ifdef HAVE_LIBSELINUX
 # include <selinux/selinux.h>
@@ -71,6 +72,10 @@ struct chfn_control {
 	 *  In the end, "newf" is folded into "oldf".  */
 	struct finfo oldf, newf;
 	unsigned int
+		allow_fullname:1,	/* The login.defs restriction */
+		allow_room:1,		   /* see: man login.defs(5) */
+		allow_work:1,		   /* and look for CHFN_RESTRICT */
+		allow_home:1,		   /* keyword for these four. */
 		interactive:1;		/* whether to prompt for fields or not */
 };
 
@@ -143,18 +148,26 @@ static void parse_argv(struct chfn_control *ctl, int argc, char **argv)
 				&index)) != -1) {
 		switch (c) {
 		case 'f':
+			if (!ctl->allow_fullname)
+				errx(EXIT_FAILURE, _("login.defs forbids setting %s"), _("Name"));
 			ctl->newf.full_name = optarg;
 			status += check_gecos_string(_("Name"), optarg);
 			break;
 		case 'o':
+			if (!ctl->allow_room)
+				errx(EXIT_FAILURE, _("login.defs forbids setting %s"), _("Office"));
 			ctl->newf.office = optarg;
 			status += check_gecos_string(_("Office"), optarg);
 			break;
 		case 'p':
+			if (!ctl->allow_work)
+				errx(EXIT_FAILURE, _("login.defs forbids setting %s"), _("Office Phone"));
 			ctl->newf.office_phone = optarg;
 			status += check_gecos_string(_("Office Phone"), optarg);
 			break;
 		case 'h':
+			if (!ctl->allow_home)
+				errx(EXIT_FAILURE, _("login.defs forbids setting %s"), _("Home Phone"));
 			ctl->newf.home_phone = optarg;
 			status += check_gecos_string(_("Home Phone"), optarg);
 			break;
@@ -233,15 +246,68 @@ static char *prompt(const char *question, char *def_val)
 }
 
 /*
+ *  get_login_defs()
+ *	find /etc/login.defs CHFN_RESTRICT and save restrictions to run time
+ */
+static void get_login_defs(struct chfn_control *ctl)
+{
+	const char *s;
+	size_t i;
+	int broken = 0;
+
+	/* real root does not have restrictions */
+	if (geteuid() == getuid() && getuid() == 0) {
+		ctl->allow_fullname = ctl->allow_room = ctl->allow_work = ctl->allow_home = 1;
+		return;
+	}
+	s = getlogindefs_str("CHFN_RESTRICT", "");
+	if (!strcmp(s, "yes")) {
+		ctl->allow_room = ctl->allow_work = ctl->allow_home = 1;
+		return;
+	}
+	if (!strcmp(s, "no")) {
+		ctl->allow_fullname = ctl->allow_room = ctl->allow_work = ctl->allow_home = 1;
+		return;
+	}
+	for (i = 0; s[i]; i++) {
+		switch (s[i]) {
+		case 'f':
+			ctl->allow_fullname = 1;
+			break;
+		case 'r':
+			ctl->allow_room = 1;
+			break;
+		case 'w':
+			ctl->allow_work = 1;
+			break;
+		case 'h':
+			ctl->allow_home = 1;
+			break;
+		default:
+			broken = 1;
+		}
+	}
+	if (broken)
+		warnx(_("%s: CHFN_RESTRICT has unexpected value: %s"), _PATH_LOGINDEFS, s);
+	if (!ctl->allow_fullname && !ctl->allow_room && !ctl->allow_work && !ctl->allow_home)
+		errx(EXIT_FAILURE, _("%s: CHFN_RESTRICT does not allow any changes"), _PATH_LOGINDEFS);
+	return;
+}
+
+/*
  *  ask_info () --
  *	prompt the user for the finger information and store it.
  */
 static void ask_info(struct chfn_control *ctl)
 {
-	ctl->newf.full_name = prompt(_("Name"), ctl->oldf.full_name);
-	ctl->newf.office = prompt(_("Office"), ctl->oldf.office);
-	ctl->newf.office_phone = prompt(_("Office Phone"), ctl->oldf.office_phone);
-	ctl->newf.home_phone = prompt(_("Home Phone"), ctl->oldf.home_phone);
+	if (ctl->allow_fullname)
+		ctl->newf.full_name = prompt(_("Name"), ctl->oldf.full_name);
+	if (ctl->allow_room)
+		ctl->newf.office = prompt(_("Office"), ctl->oldf.office);
+	if (ctl->allow_work)
+		ctl->newf.office_phone = prompt(_("Office Phone"), ctl->oldf.office_phone);
+	if (ctl->allow_home)
+		ctl->newf.home_phone = prompt(_("Home Phone"), ctl->oldf.home_phone);
 	printf("\n");
 }
 
@@ -337,6 +403,9 @@ int main(int argc, char **argv)
 	atexit(close_stdout);
 	uid = getuid();
 
+	/* check /etc/login.defs CHFN_RESTRICT */
+	get_login_defs(&ctl);
+
 	parse_argv(&ctl, argc, argv);
 	if (!ctl.username) {
 		ctl.pw = getpwuid(uid);
-- 
2.1.3

Re: [PATCH 05/16] lslogins: reject unknown time format arguments

[Karel Zak]

On Sun, Dec 14, 2014 at 05:44:01PM +0000, Sami Kerola wrote:
> @@ -1398,8 +1388,18 @@ int main(int argc, char *argv[])
>  			break;
>  		case OPT_TIME_FMT:
>  			{
> +				struct lslogins_timefmt {
> +					const char *name;
> +					const int val;
> +				};
> +				const struct lslogins_timefmt timefmts[] = {

 static const ....

> +					{ "iso", TIME_ISO },
> +					{ "full", TIME_FULL },
> +					{ "short", TIME_SHORT },
> +				};
>  				size_t i;
>  
> +				ctl->time_mode = TIME_INVALID;
>  				for (i = 0; i < ARRAY_SIZE(timefmts); i++) {
>  					if (strcmp(timefmts[i].name, optarg) == 0) {
>  						ctl->time_mode = timefmts[i].val;
> @@ -1407,7 +1407,7 @@ int main(int argc, char *argv[])
>  					}
>  				}
>  				if (ctl->time_mode == TIME_INVALID)
> -					usage(stderr);
> +					errx(EXIT_FAILURE, _("unknown time format: %s"), optarg);

This is not elegant solution, it would be better to move all the code
to small function parse_time_mode() and keep the main() less "crowded".

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 14/16] chfn: clean up parse_argv()

[Karel Zak]

On Sun, Dec 14, 2014 at 05:44:10PM +0000, Sami Kerola wrote:
> Use switch() case ?: for all option parsing, as in most of the other
> source files.
> 
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  login-utils/chfn.c | 48 ++++++++++++++++++------------------------------
>  1 file changed, 18 insertions(+), 30 deletions(-)
> 
> diff --git a/login-utils/chfn.c b/login-utils/chfn.c
> index 81abd11..0d37a32 100644
> --- a/login-utils/chfn.c
> +++ b/login-utils/chfn.c
> @@ -121,11 +121,11 @@ static int check_gecos_string(const char *msg, char *gecos)
>   *	parse the command line arguments.
>   *	returns true if no information beyond the username was given.
>   */
> -static void parse_argv(struct chfn_control *ctl, int argc, char *argv[], struct finfo *pinfo)
> +static void parse_argv(struct chfn_control *ctl, int argc, char *argv[],
> +		       struct finfo *pinfo)
>  {
> -	int index, c, status;
> -
> -	static struct option long_options[] = {
^^^^^^^^^^^^
> +	int index, c, status = 0;
> +	const struct option long_options[] = {

static

>  		{"full-name", required_argument, 0, 'f'},
>  		{"office", required_argument, 0, 'o'},
>  		{"office-phone", required_argument, 0, 'p'},
> @@ -135,49 +135,37 @@ static void parse_argv(struct chfn_control *ctl, int argc, char *argv[], struct
>  		{NULL, no_argument, 0, '0'},
>  	};
>  
> -	optind = 0;
> -	while (true) {
> -		c = getopt_long(argc, argv, "f:r:p:h:o:uv", long_options,
> -				&index);
> -		if (c == -1)
> -			break;
> -		/* version?  output version and exit. */
> -		if (c == 'v') {
> -			printf(UTIL_LINUX_VERSION);
> -			exit(EXIT_SUCCESS);
> -		}
> -		if (c == 'u')
> -			usage(stdout);
> -		/* all other options must have an argument. */
> -		if (!optarg)
> -			usage(stderr);
> -		/* ok, we were given an argument */
> -		ctl->interactive = 0;
> -
> -		/* now store the argument */
> +	while ((c = getopt_long(argc, argv, "f:r:p:h:o:uv", long_options,
> +				&index)) != -1) {
>  		switch (c) {
>  		case 'f':
>  			pinfo->full_name = optarg;
> -			status = check_gecos_string(_("Name"), optarg);
> +			status += check_gecos_string(_("Name"), optarg);
>  			break;
>  		case 'o':
>  			pinfo->office = optarg;
> -			status = check_gecos_string(_("Office"), optarg);
> +			status += check_gecos_string(_("Office"), optarg);
>  			break;
>  		case 'p':
>  			pinfo->office_phone = optarg;
> -			status = check_gecos_string(_("Office Phone"), optarg);
> +			status += check_gecos_string(_("Office Phone"), optarg);
>  			break;
>  		case 'h':
>  			pinfo->home_phone = optarg;
> -			status = check_gecos_string(_("Home Phone"), optarg);
> +			status += check_gecos_string(_("Home Phone"), optarg);
>  			break;
> +		case 'v':
> +			printf(UTIL_LINUX_VERSION);
> +			exit(EXIT_SUCCESS);
> +		case 'u':
> +			usage(stdout);
>  		default:
>  			usage(stderr);
>  		}
> -		if (status != 0)
> -			exit(EXIT_FAILURE);
> +		ctl->interactive = 0;
>  	}
> +	if (status != 0)
> +		exit(EXIT_FAILURE);
>  	/* done parsing arguments.  check for a username. */
>  	if (optind < argc) {
>  		if (optind + 1 < argc)
> -- 
> 2.1.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe util-linux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 15/16] chfn: move new and old finger structs to chfn control struct

[Karel Zak]

On Sun, Dec 14, 2014 at 05:44:11PM +0000, Sami Kerola wrote:
> +	ctl->newf.full_name = prompt(_("Name"), ctl->oldf.full_name);
> +	ctl->newf.office = prompt(_("Office"), ctl->oldf.office);
> +	ctl->newf.office_phone = prompt(_("Office Phone"), ctl->oldf.office_phone);
> +	ctl->newf.home_phone = prompt(_("Home Phone"), ctl->oldf.home_phone);

it would be better to rename prompt() to ask_new_field() or so.

> -static int set_changed_data(struct finfo *oldfp, struct finfo *newfp)
> +static int set_changed_data(struct chfn_control *ctl)
>  {
>  	int changed = false;
>  
> -	if (newfp->full_name) {
> -		oldfp->full_name = newfp->full_name;
> +	if (ctl->newf.full_name)
>  		changed = true;
> -	}
> -	if (newfp->office) {
> -		oldfp->office = newfp->office;
> +	else
> +		ctl->newf.full_name = ctl->oldf.full_name;
> +	if (ctl->newf.office)
>  		changed = true;
> -	}
> -	if (newfp->office_phone) {
> -		oldfp->office_phone = newfp->office_phone;
> +	else
> +		ctl->newf.office = ctl->oldf.office;
> +	if (ctl->newf.office_phone)
>  		changed = true;
> -	}
> -	if (newfp->home_phone) {
> -		oldfp->home_phone = newfp->home_phone;
> +	else
> +		ctl->newf.office_phone = ctl->oldf.office_phone;
> +	if (ctl->newf.home_phone)
>  		changed = true;
> -	}
> -
> +	else
> +		ctl->newf.home_phone = ctl->oldf.home_phone;
>  	return changed;
>  }

this is ugly and unnecessary function, all you need is to return
usable data from prompt() and maintain ctl->changed with in the
function. The prompt() knows all the detains.

>  	/* create the new gecos string */
> -	len = xasprintf(&gecos, "%s,%s,%s,%s,%s", pinfo->full_name, pinfo->office,
> -		  pinfo->office_phone, pinfo->home_phone, pinfo->other);
> +	len = xasprintf(&gecos, "%s,%s,%s,%s,%s", ctl->newf.full_name, ctl->newf.office,
> +		  ctl->newf.office_phone, ctl->newf.home_phone, ctl->newf.other);

Maybe

  len = xasprintf(&gecos, "%s,%s,%s,%s,%s", 
        ctl->newf.full_name ? ctl->newf.full_name : "",
        ...
        );

would be better than set the empty strings to newf (now you mix ""
and strdup() results in newf).

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 16/16] chfn: make command to obey login.defs CHFN_RESTRICT instructions

[Karel Zak]

On Sun, Dec 14, 2014 at 05:44:12PM +0000, Sami Kerola wrote:
> Reference: http://man7.org/linux/man-pages/man5/login.defs.5.html
> Addresses: https://bugzilla.redhat.com/show_bug.cgi?id=138519

 Good idea, thanks!

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 05/16] lslogins: reject unknown time format arguments

[Sami Kerola]

On Mon, 15 Dec 2014, Karel Zak wrote:

> On Sun, Dec 14, 2014 at 05:44:01PM +0000, Sami Kerola wrote:
>> @@ -1398,8 +1388,18 @@ int main(int argc, char *argv[])
>>  			break;
>>  		case OPT_TIME_FMT:
>>  			{
>> +				struct lslogins_timefmt {
>> +					const char *name;
>> +					const int val;
>> +				};
>> +				const struct lslogins_timefmt timefmts[] = {
>
> static const ....

Corrected.

>> +					{ "iso", TIME_ISO },
>> +					{ "full", TIME_FULL },
>> +					{ "short", TIME_SHORT },
>> +				};
>>  				size_t i;
>>
>> +				ctl->time_mode = TIME_INVALID;
>>  				for (i = 0; i < ARRAY_SIZE(timefmts); i++) {
>>  					if (strcmp(timefmts[i].name, optarg) == 0) {
>>  						ctl->time_mode = timefmts[i].val;
>> @@ -1407,7 +1407,7 @@ int main(int argc, char *argv[])
>>  					}
>>  				}
>>  				if (ctl->time_mode == TIME_INVALID)
>> -					usage(stderr);
>> +					errx(EXIT_FAILURE, _("unknown time format: %s"), optarg);
>
> This is not elegant solution, it would be better to move all the code
> to small function parse_time_mode() and keep the main() less "crowded".

Good idea. Here is updated version of the change.

https://github.com/kerolasa/lelux-utiliteetit/commit/cdf3896a0502b2773323ec293ccb4dfdbfc87cf8

--->8----
From: Sami Kerola <kerolasa@iki.fi>
Date: Sat, 13 Dec 2014 17:11:04 +0000
Subject: [PATCH 05/18] lslogins: reject unknown time format arguments

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
  login-utils/lslogins.c | 40 +++++++++++++++++++---------------------
  1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/login-utils/lslogins.c b/login-utils/lslogins.c
index cacf83c..5e1ef17 100644
--- a/login-utils/lslogins.c
+++ b/login-utils/lslogins.c
@@ -1180,16 +1180,25 @@ static void free_user(void *f)
  	free(u);
  }

-struct lslogins_timefmt {
-	const char *name;
-	int val;
-};
+static int parse_time_mode(const char *optarg)
+{
+	struct lslogins_timefmt {
+		const char *name;
+		const int val;
+	};
+	static const struct lslogins_timefmt timefmts[] = {
+		{"iso", TIME_ISO},
+		{"full", TIME_FULL},
+		{"short", TIME_SHORT},
+	};
+	size_t i;

-static struct lslogins_timefmt timefmts[] = {
-	{ "short", TIME_SHORT },
-	{ "full", TIME_FULL },
-	{ "iso", TIME_ISO },
-};
+	for (i = 0; i < ARRAY_SIZE(timefmts); i++) {
+		if (strcmp(timefmts[i].name, optarg) == 0)
+			return timefmts[i].val;
+	}
+	errx(EXIT_FAILURE, _("unknown time format: %s"), optarg);
+}

  static void __attribute__((__noreturn__)) usage(FILE *out)
  {
@@ -1397,18 +1406,7 @@ int main(int argc, char *argv[])
  			ctl->noheadings = 1;
  			break;
  		case OPT_TIME_FMT:
-			{
-				size_t i;
-
-				for (i = 0; i < ARRAY_SIZE(timefmts); i++) {
-					if (strcmp(timefmts[i].name, optarg) == 0) {
-						ctl->time_mode = timefmts[i].val;
-						break;
-					}
-				}
-				if (ctl->time_mode == TIME_INVALID)
-					usage(stderr);
-			}
+			ctl->time_mode = parse_time_mode(optarg);
  			break;
  		case 'V':
  			printf(UTIL_LINUX_VERSION);
-- 
2.2.0

Re: [PATCH 15/16] chfn: move new and old finger structs to chfn control struct

[Sami Kerola]

On Mon, 15 Dec 2014, Karel Zak wrote:

> On Sun, Dec 14, 2014 at 05:44:11PM +0000, Sami Kerola wrote:
>> +	ctl->newf.full_name = prompt(_("Name"), ctl->oldf.full_name);
>> +	ctl->newf.office = prompt(_("Office"), ctl->oldf.office);
>> +	ctl->newf.office_phone = prompt(_("Office Phone"), ctl->oldf.office_phone);
>> +	ctl->newf.home_phone = prompt(_("Home Phone"), ctl->oldf.home_phone);
>
> it would be better to rename prompt() to ask_new_field() or so.

I always forget changing function names is acceptable, and encouraged when 
cleaning up old code.

>> -static int set_changed_data(struct finfo *oldfp, struct finfo *newfp)
>> +static int set_changed_data(struct chfn_control *ctl)
>>  {
>>  	int changed = false;
>>
>> -	if (newfp->full_name) {
>> -		oldfp->full_name = newfp->full_name;
>> +	if (ctl->newf.full_name)
>>  		changed = true;
>> -	}
>> -	if (newfp->office) {
>> -		oldfp->office = newfp->office;
>> +	else
>> +		ctl->newf.full_name = ctl->oldf.full_name;
>> +	if (ctl->newf.office)
>>  		changed = true;
>> -	}
>> -	if (newfp->office_phone) {
>> -		oldfp->office_phone = newfp->office_phone;
>> +	else
>> +		ctl->newf.office = ctl->oldf.office;
>> +	if (ctl->newf.office_phone)
>>  		changed = true;
>> -	}
>> -	if (newfp->home_phone) {
>> -		oldfp->home_phone = newfp->home_phone;
>> +	else
>> +		ctl->newf.office_phone = ctl->oldf.office_phone;
>> +	if (ctl->newf.home_phone)
>>  		changed = true;
>> -	}
>> -
>> +	else
>> +		ctl->newf.home_phone = ctl->oldf.home_phone;
>>  	return changed;
>>  }
>
> this is ugly and unnecessary function, all you need is to return
> usable data from prompt() and maintain ctl->changed with in the
> function. The prompt() knows all the detains.

That is ugly == true.

But there is a case to keep some sort of fill the missing fields around. 
When user supplies change with --office and other options some of the 
fields may not be defined.

Same is true when CHFN_RESTRICT kicks in, and an user might not be able to 
change all fields.

>>  	/* create the new gecos string */
>> -	len = xasprintf(&gecos, "%s,%s,%s,%s,%s", pinfo->full_name, pinfo->office,
>> -		  pinfo->office_phone, pinfo->home_phone, pinfo->other);
>> +	len = xasprintf(&gecos, "%s,%s,%s,%s,%s", ctl->newf.full_name, ctl->newf.office,
>> +		  ctl->newf.office_phone, ctl->newf.home_phone, ctl->newf.other);
>
> Maybe
>
>  len = xasprintf(&gecos, "%s,%s,%s,%s,%s",
>        ctl->newf.full_name ? ctl->newf.full_name : "",
>        ...
>        );
>
> would be better than set the empty strings to newf (now you mix ""
> and strdup() results in newf).

xasprintf if case needs to: new, or old, or nothing. After trying that the 
code looked even more messy, so I end up writing two changes on top of the 
original change. Basic idea is

1. find old fields
2. get new fields
3. set missing fields to new data from old, or blank
4. write

https://github.com/kerolasa/lelux-utiliteetit/commit/d9e1ac99e420f2521849e336a4e57cce1b875241
https://github.com/kerolasa/lelux-utiliteetit/commit/f723cbf544a7eac2927634f2cb6d802437a2d519

--->8----
From: Sami Kerola <kerolasa@iki.fi>
Date: Sun, 14 Dec 2014 17:44:11 +0000
Subject: [PATCH 15/18] chfn: move new and old finger structs to chfn control struct

This change is a little bit messy, and requires a comment the struct
finfo should not have 'struct passwd *pw' as it's member.  The earlier
struct design would have been burden to maintain, and confusing to use.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
  login-utils/chfn.c | 165 ++++++++++++++++++++++++-----------------------------
  1 file changed, 76 insertions(+), 89 deletions(-)

diff --git a/login-utils/chfn.c b/login-utils/chfn.c
index 801bdbe..f74ed22 100644
--- a/login-utils/chfn.c
+++ b/login-utils/chfn.c
@@ -55,8 +55,6 @@
  #endif

  struct finfo {
-	struct passwd *pw;
-	char *username;
  	char *full_name;
  	char *office;
  	char *office_phone;
@@ -65,6 +63,13 @@ struct finfo {
  };

  struct chfn_control {
+	struct passwd *pw;
+	char *username;
+	/*  "oldf"  Contains the users original finger information.
+	 *  "newf"  Contains the changed finger information, and contains
+	 *          NULL in fields that haven't been changed.
+	 *  In the end, "newf" is folded into "oldf".  */
+	struct finfo oldf, newf;
  	unsigned int
  		interactive:1;		/* whether to prompt for fields or not */
  };
@@ -121,8 +126,7 @@ static int check_gecos_string(const char *msg, char *gecos)
   *	parse the command line arguments.
   *	returns true if no information beyond the username was given.
   */
-static void parse_argv(struct chfn_control *ctl, int argc, char *argv[],
-		       struct finfo *pinfo)
+static void parse_argv(struct chfn_control *ctl, int argc, char **argv)
  {
  	int index, c, status = 0;
  	static const struct option long_options[] = {
@@ -139,19 +143,19 @@ static void parse_argv(struct chfn_control *ctl, int argc, char *argv[],
  				&index)) != -1) {
  		switch (c) {
  		case 'f':
-			pinfo->full_name = optarg;
+			ctl->newf.full_name = optarg;
  			status += check_gecos_string(_("Name"), optarg);
  			break;
  		case 'o':
-			pinfo->office = optarg;
+			ctl->newf.office = optarg;
  			status += check_gecos_string(_("Office"), optarg);
  			break;
  		case 'p':
-			pinfo->office_phone = optarg;
+			ctl->newf.office_phone = optarg;
  			status += check_gecos_string(_("Office Phone"), optarg);
  			break;
  		case 'h':
-			pinfo->home_phone = optarg;
+			ctl->newf.home_phone = optarg;
  			status += check_gecos_string(_("Home Phone"), optarg);
  			break;
  		case 'v':
@@ -170,7 +174,7 @@ static void parse_argv(struct chfn_control *ctl, int argc, char *argv[],
  	if (optind < argc) {
  		if (optind + 1 < argc)
  			usage(stderr);
-		pinfo->username = argv[optind];
+		ctl->username = argv[optind];
  	}
  	return;
  }
@@ -179,24 +183,22 @@ static void parse_argv(struct chfn_control *ctl, int argc, char *argv[],
   *  parse_passwd () --
   *	take a struct password and fill in the fields of the struct finfo.
   */
-static void parse_passwd(struct passwd *pw, struct finfo *pinfo)
+static void parse_passwd(struct chfn_control *ctl)
  {
  	char *gecos;

-	if (!pw)
+	if (!ctl->pw)
  		return;
-	pinfo->pw = pw;
-	pinfo->username = pw->pw_name;
  	/* use pw_gecos - we take a copy since PAM destroys the original */
-	gecos = xstrdup(pw->pw_gecos);
+	gecos = xstrdup(ctl->pw->pw_gecos);
  	/* extract known fields */
-	pinfo->full_name = strsep(&gecos, ",");
-	pinfo->office = strsep(&gecos, ",");
-	pinfo->office_phone = strsep(&gecos, ",");
-	pinfo->home_phone = strsep(&gecos, ",");
+	ctl->oldf.full_name = strsep(&gecos, ",");
+	ctl->oldf.office = strsep(&gecos, ",");
+	ctl->oldf.office_phone = strsep(&gecos, ",");
+	ctl->oldf.home_phone = strsep(&gecos, ",");
  	/*  extra fields contain site-specific information, and can
  	 *  not be changed by this version of chfn.  */
-	pinfo->other = strsep(&gecos, ",");
+	ctl->oldf.other = strsep(&gecos, ",");
  }

  /*
@@ -234,12 +236,12 @@ static char *prompt(const char *question, char *def_val)
   *  ask_info () --
   *	prompt the user for the finger information and store it.
   */
-static void ask_info(struct finfo *oldfp, struct finfo *newfp)
+static void ask_info(struct chfn_control *ctl)
  {
-	newfp->full_name = prompt(_("Name"), oldfp->full_name);
-	newfp->office = prompt(_("Office"), oldfp->office);
-	newfp->office_phone = prompt(_("Office Phone"), oldfp->office_phone);
-	newfp->home_phone = prompt(_("Home Phone"), oldfp->home_phone);
+	ctl->newf.full_name = prompt(_("Name"), ctl->oldf.full_name);
+	ctl->newf.office = prompt(_("Office"), ctl->oldf.office);
+	ctl->newf.office_phone = prompt(_("Office Phone"), ctl->oldf.office_phone);
+	ctl->newf.home_phone = prompt(_("Home Phone"), ctl->oldf.home_phone);
  	printf("\n");
  }

@@ -247,27 +249,26 @@ static void ask_info(struct finfo *oldfp, struct finfo *newfp)
   *  set_changed_data () --
   *	incorporate the new data into the old finger info.
   */
-static int set_changed_data(struct finfo *oldfp, struct finfo *newfp)
+static int set_changed_data(struct chfn_control *ctl)
  {
  	int changed = false;

-	if (newfp->full_name) {
-		oldfp->full_name = newfp->full_name;
+	if (ctl->newf.full_name)
  		changed = true;
-	}
-	if (newfp->office) {
-		oldfp->office = newfp->office;
+	else
+		ctl->newf.full_name = ctl->oldf.full_name;
+	if (ctl->newf.office)
  		changed = true;
-	}
-	if (newfp->office_phone) {
-		oldfp->office_phone = newfp->office_phone;
+	else
+		ctl->newf.office = ctl->oldf.office;
+	if (ctl->newf.office_phone)
  		changed = true;
-	}
-	if (newfp->home_phone) {
-		oldfp->home_phone = newfp->home_phone;
+	else
+		ctl->newf.office_phone = ctl->oldf.office_phone;
+	if (ctl->newf.home_phone)
  		changed = true;
-	}
-
+	else
+		ctl->newf.home_phone = ctl->oldf.home_phone;
  	return changed;
  }

@@ -276,41 +277,41 @@ static int set_changed_data(struct finfo *oldfp, struct finfo *newfp)
   *	save the given finger info in /etc/passwd.
   *	return zero on success.
   */
-static int save_new_data(struct finfo *pinfo)
+static int save_new_data(struct chfn_control *ctl)
  {
  	char *gecos;
  	int len;

  	/* null fields will confuse printf(). */
-	if (!pinfo->full_name)
-		pinfo->full_name = "";
-	if (!pinfo->office)
-		pinfo->office = "";
-	if (!pinfo->office_phone)
-		pinfo->office_phone = "";
-	if (!pinfo->home_phone)
-		pinfo->home_phone = "";
-	if (!pinfo->other)
-		pinfo->other = "";
+	if (!ctl->newf.full_name)
+		ctl->newf.full_name = "";
+	if (!ctl->newf.office)
+		ctl->newf.office = "";
+	if (!ctl->newf.office_phone)
+		ctl->newf.office_phone = "";
+	if (!ctl->newf.home_phone)
+		ctl->newf.home_phone = "";
+	if (!ctl->newf.other)
+		ctl->newf.other = "";

  	/* create the new gecos string */
-	len = xasprintf(&gecos, "%s,%s,%s,%s,%s", pinfo->full_name, pinfo->office,
-		  pinfo->office_phone, pinfo->home_phone, pinfo->other);
+	len = xasprintf(&gecos, "%s,%s,%s,%s,%s", ctl->newf.full_name, ctl->newf.office,
+		  ctl->newf.office_phone, ctl->newf.home_phone, ctl->newf.other);

-	/* remove trailing empty fields (but not subfields of pinfo->other) */
-	if (!pinfo->other[0]) {
+	/* remove trailing empty fields (but not subfields of ctl->newf.other) */
+	if (!ctl->newf.other[0]) {
  		while (len > 0 && gecos[len - 1] == ',')
  			len--;
  		gecos[len] = 0;
  	}

  #ifdef HAVE_LIBUSER
-	if (set_value_libuser("chfn", pinfo->pw->pw_name, pinfo->pw->pw_uid,
+	if (set_value_libuser("chfn", ctl->username, ctl->pw->pw_uid,
  			LU_GECOS, gecos) < 0) {
  #else /* HAVE_LIBUSER */
  	/* write the new struct passwd to the passwd file. */
-	pinfo->pw->pw_gecos = gecos;
-	if (setpwnam(pinfo->pw) < 0) {
+	ctl->pw->pw_gecos = gecos;
+	if (setpwnam(ctl->pw) < 0) {
  		warn("setpwnam failed");
  #endif
  		printf(_
@@ -325,7 +326,6 @@ static int save_new_data(struct finfo *pinfo)
  int main(int argc, char **argv)
  {
  	uid_t uid;
-	struct finfo oldf, newf;
  	struct chfn_control ctl = {
  		.interactive = 1
  	};
@@ -335,44 +335,31 @@ int main(int argc, char **argv)
  	bindtextdomain(PACKAGE, LOCALEDIR);
  	textdomain(PACKAGE);
  	atexit(close_stdout);
-
-	/*
-	 *  "oldf" contains the users original finger information.
-	 *  "newf" contains the changed finger information, and contains NULL
-	 *         in fields that haven't been changed.
-	 *  in the end, "newf" is folded into "oldf".
-	 *
-	 *  the reason the new finger information is not put _immediately_
-	 *  into "oldf" is that on the command line, new finger information
-	 *  can be specified before we know what user the information is
-	 *  being specified for.
-	 */
  	uid = getuid();
-	memset(&oldf, 0, sizeof(oldf));
-	memset(&newf, 0, sizeof(newf));

-	parse_argv(&ctl, argc, argv, &newf);
-	if (!newf.username) {
-		parse_passwd(getpwuid(uid), &oldf);
-		if (!oldf.username)
+	parse_argv(&ctl, argc, argv);
+	if (!ctl.username) {
+		ctl.pw = getpwuid(uid);
+		if (!ctl.pw)
  			errx(EXIT_FAILURE, _("you (user %d) don't exist."),
  			     uid);
+		ctl.username = ctl.pw->pw_name;
  	} else {
-		parse_passwd(getpwnam(newf.username), &oldf);
-		if (!oldf.username)
+		ctl.pw = getpwnam(ctl.username);
+		if (!ctl.pw)
  			errx(EXIT_FAILURE, _("user \"%s\" does not exist."),
-			     newf.username);
+			     ctl.username);
  	}
-
+	parse_passwd(&ctl);
  #ifndef HAVE_LIBUSER
-	if (!(is_local(oldf.username)))
+	if (!(is_local(ctl.username)))
  		errx(EXIT_FAILURE, _("can only change local entries"));
  #endif

  #ifdef HAVE_LIBSELINUX
  	if (is_selinux_enabled() > 0) {
  		if (uid == 0) {
-			if (checkAccess(oldf.username, PASSWD__CHFN) != 0) {
+			if (checkAccess(ctl.username, PASSWD__CHFN) != 0) {
  				security_context_t user_context;
  				if (getprevcon(&user_context) < 0)
  					user_context = NULL;
@@ -380,7 +367,7 @@ int main(int argc, char **argv)
  				     _("%s is not authorized to change "
  				       "the finger info of %s"),
  				     user_context ? : _("Unknown user context"),
-				     oldf.username);
+				     ctl.username);
  			}
  		}
  		if (setupDefaultContext(_PATH_PASSWD))
@@ -391,30 +378,30 @@ int main(int argc, char **argv)

  #ifdef HAVE_LIBUSER
  	/* If we're setuid and not really root, disallow the password change. */
-	if (geteuid() != getuid() && uid != oldf.pw->pw_uid) {
+	if (geteuid() != getuid() && uid != ctl.pw->pw_uid) {
  #else
-	if (uid != 0 && uid != oldf.pw->pw_uid) {
+	if (uid != 0 && uid != ctl.oldf.pw->pw_uid) {
  #endif
  		errno = EACCES;
  		err(EXIT_FAILURE, _("running UID doesn't match UID of user we're "
  		      "altering, change denied"));
  	}

-	printf(_("Changing finger information for %s.\n"), oldf.username);
+	printf(_("Changing finger information for %s.\n"), ctl.username);

  #if !defined(HAVE_LIBUSER) && defined(CHFN_CHSH_PASSWORD)
-	if(!auth_pam("chfn", uid, oldf.username)) {
+	if (!auth_pam("chfn", uid, ctl.username)) {
  		return EXIT_FAILURE;
  	}
  #endif

  	if (ctl.interactive)
-		ask_info(&oldf, &newf);
+		ask_info(&ctl);

-	if (!set_changed_data(&oldf, &newf)) {
+	if (!set_changed_data(&ctl)) {
  		printf(_("Finger information not changed.\n"));
  		return EXIT_SUCCESS;
  	}

-	return save_new_data(&oldf) == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
+	return save_new_data(&ctl) == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
  }
-- 
2.2.0

From: Sami Kerola <kerolasa@iki.fi>
Date: Wed, 17 Dec 2014 22:28:03 +0000
Subject: [PATCH 16/18] chfn: rename prompt() to ask_new_field()

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
  login-utils/chfn.c | 13 +++++++------
  1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/login-utils/chfn.c b/login-utils/chfn.c
index f74ed22..fabe308 100644
--- a/login-utils/chfn.c
+++ b/login-utils/chfn.c
@@ -202,10 +202,11 @@ static void parse_passwd(struct chfn_control *ctl)
  }

  /*
- *  prompt () --
+ *  ask_new_field () --
   *	ask the user for a given field and check that the string is legal.
   */
-static char *prompt(const char *question, char *def_val)
+static char *ask_new_field(struct chfn_control *ctl, const char *question,
+			   char *def_val)
  {
  	int len;
  	char *ans;
@@ -238,10 +239,10 @@ static char *prompt(const char *question, char *def_val)
   */
  static void ask_info(struct chfn_control *ctl)
  {
-	ctl->newf.full_name = prompt(_("Name"), ctl->oldf.full_name);
-	ctl->newf.office = prompt(_("Office"), ctl->oldf.office);
-	ctl->newf.office_phone = prompt(_("Office Phone"), ctl->oldf.office_phone);
-	ctl->newf.home_phone = prompt(_("Home Phone"), ctl->oldf.home_phone);
+	ctl->newf.full_name = ask_new_field(ctl, _("Name"), ctl->oldf.full_name);
+	ctl->newf.office = ask_new_field(ctl, _("Office"), ctl->oldf.office);
+	ctl->newf.office_phone = ask_new_field(ctl, _("Office Phone"), ctl->oldf.office_phone);
+	ctl->newf.home_phone = ask_new_field(ctl, _("Home Phone"), ctl->oldf.home_phone);
  	printf("\n");
  }

-- 
2.2.0

Re: [PATCH 00/16] pull: updates to lslogins and chfn

[Karel Zak]

On Sun, Dec 14, 2014 at 05:43:56PM +0000, Sami Kerola wrote:
> Sami Kerola (16):
>   lslogins: allow changing password changed and expiration time formats
>   lslogins: make journald last logs time stamps to honor --time-format
>   lslogins: tell why command failed
>   lslogins: fix short options
>   lslogins: reject unknown time format arguments
>   lslogins: add space to systemd journal header and message
>   lslogins: use hardcoded paths from pathnames.h
>   chfn: remove function prototypes
>   chfn: rewrite prompt() to use strutils
>   chfn: use xasprintf() rather than bunch of strlen() and malloc() calls
>   chfn: fix usage() regression
>   chfn: simplify parse_passwd() by using strsep()
>   chfn: add minimalistic struct chfn_control
>   chfn: clean up parse_argv()
>   chfn: move new and old finger structs to chfn control struct
>   chfn: make command to obey login.defs CHFN_RESTRICT instructions

 Applied from github, thanks. I th ink we can cleanup chfn later.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com