pw: set the user's home directory mode if it existed

[kevans]

Summary

The adduser(8) prompt allows one to set the mode of a new home
directory, but pw(8) doesn't honor the -M mode if the home directory
already exists at creation time. It doesn't seem to make sense to
ignore the mode (which may lead to a security issue on the system being
configured) when we'll happily chown an existing directory, so fix the
inconsistency.

PR: 280099

[jlduran]

Note that pw useradd/usermod -M should have a similar fix.
I'll see if adding a test is feasible (over the weekend).
Thank you!

Could you also change the manpage to indicate that:

.It Fl M Ar mode
Create the home directory with permissions set to
.Ar mode ,
modified by the current
.Xr umask 2 .

To match pw(8)'s description. And let users know that if they try -M 0777, and the umask is 022 (default), they'll still get 0755.

[jlduran]

draft_test.patch1 KBDownload

First draft of test.

[kevans]

In D46443#1058744, @jlduran_gmail.com wrote:

draft_test.patch1 KBDownload

First draft of test.

JFYI- I can't see what this is referencing. In the "Referenced Files" section on the right, I can see:

Restricted File
Mon, Aug 26, 20:38

which lines up with the timestamp on your comment, so I guess permissions aren't set on it somehow.

[jlduran]

In D46443#1058814, @kevans wrote:

In D46443#1058744, @jlduran_gmail.com wrote:

draft_test.patch1 KBDownload

First draft of test.

JFYI- I can't see what this is referencing. In the "Referenced Files" section on the right, I can see:

Restricted File
Mon, Aug 26, 20:38

which lines up with the timestamp on your comment, so I guess permissions aren't set on it somehow.

Forgive my ignorance on Phabricator, I believe it is OK now.

[kevans]

Test looks reasonable to me, incorporating it as:

commit 73f91ff94577a61194702b9f5235233aa4aa7d2c
Author: Jose Luis Duran <[e-mail]>
Date:   Tue Aug 27 10:03:09 2024 -0500

    pw: tests: add a test for -M with a pre-existing home directory
    
    Previous versions of pw(8) wouldn't chmod the home directory if it
    already existed prior to user creation, rendering adduser(8) -M
    ineffective in some cases.  Add a test to cover that situation.
    
    PR:             280099

While we're here, note in the adduser(8) documentation that -M is subject to the process umask.

[kevans]

In D46443#1058815, @jlduran_gmail.com wrote:

In D46443#1058814, @kevans wrote:

In D46443#1058744, @jlduran_gmail.com wrote:

draft_test.patch1 KBDownload

First draft of test.

JFYI- I can't see what this is referencing. In the "Referenced Files" section on the right, I can see:

Restricted File
Mon, Aug 26, 20:38

which lines up with the timestamp on your comment, so I guess permissions aren't set on it somehow.

Forgive my ignorance on Phabricator, I believe it is OK now.

IMO for a review tool, it's weird that the permissions of an attached file didn't default to the permissions of the review object it's being attached to. That's not the kind of thing I'd expect to have to audit as an uploader when I'm trying to participate in a review.

[des]

usr.sbin/pw/cpdir.c

63

We should warn if this fails, imo.

[kevans]

usr.sbin/pw/cpdir.c

63

I had considered the same, but couldn't reconcile that with also failing to check either of the below fchownat/chflagsat. Probably all three should be checked.

[kevans]

Issue warnings for any permission change failures- they're not fatal, but the admin should know about them in case there's a legitimate issue.