ensure correct user homedir perms on user.update

src/middlewared/middlewared/plugins/account.py

@@ -30,7 +30,7 @@
 from middlewared.async_validators import check_path_resides_within_volume
 from middlewared.utils.sid import db_id_to_rid, DomainRid
 from middlewared.plugins.account_.constants import (
-    ADMIN_UID, ADMIN_GID, SKEL_PATH, DEFAULT_HOME_PATH, DEFAULT_HOME_PATHS
+    BuiltinLocalAdmins, SKEL_PATH, DEFAULT_HOME_PATH, DEFAULT_HOME_PATHS
 )
 from middlewared.plugins.smb_.constants import SMBBuiltin
 from middlewared.plugins.idmap_.idmap_constants import (
@@ -229,7 +229,7 @@ async def user_extend(self, user, ctx):
         # Get authorized keys
         user['sshpubkey'] = await self.middleware.run_in_thread(self._read_authorized_keys, user['home'])
 
-        user['immutable'] = user['builtin'] or (user['uid'] == ADMIN_UID)
+        user['immutable'] = user['builtin'] or (user['uid'] == BuiltinLocalAdmins.admin_uid)
         user['twofactor_auth_configured'] = bool(ctx['user_2fa_mapping'][user['id']])
 
         user_roles = set()
@@ -704,7 +704,7 @@ def do_update(self, app, audit_callback, pk, data):
         # root user and admin users are an exception to the rule
         if data.get('sshpubkey'):
             if not (
-                user['uid'] in [0, ADMIN_UID] or
+                user['uid'] in [0, BuiltinLocalAdmins.admin_uid] or
                 self.middleware.call_sync('filesystem.is_dataset_path', home)
             ):
                 verrors.add('user_update.sshpubkey', 'Home directory is not writable, leave this blank"')
@@ -837,25 +837,51 @@ def do_update(self, app, audit_callback, pk, data):
 
     @private
     def recreate_homedir_if_not_exists(self, user, group, mode):
-        # sigh, nothing is stopping someone from removing the homedir
-        # from the CLI so recreate the original directory in this case
-        if not os.path.isdir(user['home']):
-            if os.path.exists(user['home']):
-                raise CallError(f'{user["home"]!r} already exists and is not a directory')
-
-            self.logger.debug('Homedir %r for %r does not exist so recreating it', user['home'], user['username'])
-            try:
-                os.makedirs(user['home'])
-            except Exception:
-                raise CallError(f'Failed recreating "{user["home"]}"')
+        try:
+            st = self.middleware.call_sync(user['home'])
+        except CallError as e:
+            if e.errno == errno.ENOENT:
+                # sigh, nothing is stopping someone from removing the homedir
+                # from the CLI so recreate the original directory in this case
+                self.logger.debug('Homedir %r for %r does not exist so recreating it', user['home'], user['username'])
+                try:
+                    os.makedirs(user['home'])
+                except Exception:
+                    raise CallError(f'Failed recreating "{user["home"]}"')
+                else:
+                    self.middleware.call_sync('filesystem.setperm', {
+                        'path': user['home'],
+                        'uid': user['uid'],
+                        'gid': group['bsdgrp_gid'],
+                        'mode': mode,
+                        'options': {'stripacl': True},
+                    }).wait_sync(raise_error=True)
+            else:
+                self.logger.error('Unexpected failure stating user homedir: %r', user['home'], exc_info=True)
+        else:
+            if st['type'] == 'DIRECTORY':
+                if user['username'] in (BuiltinLocalAdmins.truenas_admin, BuiltinLocalAdmins.root):
+                    # TODO: maybe we should also apply logic to admin accounts?
+                    return
+                elif user['home'].startswith('/mnt/'):
+                    perms_changed = dict()
+                    if user['uid'] != st['uid']:
+                        perms_changed['uid'] = user['uid']
+                    if group['bsdgrp_gid'] != st['gid']:
+                        perms_changed['gid'] = group['bsdgrp_gid']
+                    if perms_changed:
+                        # NOTE: we specifically check to see if the perms changed and
+                        # only call the `filesystem.chown` endpoint because it generates
+                        # audit entries at the API level and we want to keep those entries
+                        # to a minimum if we can help it
+                        perms_changed.update({'path': user['home']})
+                        self.middleware.call_sync('filesystem.chown', perms_changed).wait_sync(raise_error=True)
+                else:
+                    self.logger.warning(
+                        'Users %r homedir (%r) is outside /mnt so ignoring', user['username'], user['home']
+                    )
             else:
-                self.middleware.call_sync('filesystem.setperm', {
-                    'path': user['home'],
-                    'uid': user['uid'],
-                    'gid': group['bsdgrp_gid'],
-                    'mode': mode,
-                    'options': {'stripacl': True},
-                }).wait_sync(raise_error=True)
+                raise CallError(f'{user["home"]!r} already exists and is not a directory')
 
     @accepts(
         Int('id'),
@@ -1203,11 +1229,11 @@ async def set_root_password(self, app, password, options):
         """
         warnings.warn("`user.set_root_password` has been deprecated. Use `user.setup_local_administrator`",
                       DeprecationWarning)
-        return await self.setup_local_administrator(app, 'root', password, options)
+        return await self.setup_local_administrator(app, BuiltinLocalAdmins.root, password, options)
 
     @no_auth_required
     @accepts(
-        Str('username', enum=['root', 'truenas_admin']),
+        Str('username', enum=[BuiltinLocalAdmins.truenas_admin, BuiltinLocalAdmins.root]),
         Password('password'),
         Dict(
             'options',
@@ -1228,12 +1254,12 @@ async def setup_local_administrator(self, app, username, password, options):
         if await self.middleware.call('user.has_local_administrator_set_up'):
             raise CallError('Local administrator is already set up', errno.EEXIST)
 
-        if username == 'truenas_admin':
+        if username == BuiltinLocalAdmins.truenas_admin:
             # first check based on NSS to catch collisions with AD / LDAP users
             try:
-                pwd_obj = await self.middleware.call('user.get_user_obj', {'uid': ADMIN_UID})
+                pwd_obj = await self.middleware.call('user.get_user_obj', {'uid': BuiltinLocalAdmins.admin_uid})
                 raise CallError(
-                    f'A {pwd_obj["source"].lower()} user with uid={ADMIN_UID} already exists, '
+                    f'A {pwd_obj["source"].lower()} user with uid={BuiltinLocalAdmins.admin_uid} already exists, '
                     'setting up local administrator is not possible',
                     errno.EEXIST,
                 )
@@ -1249,9 +1275,9 @@ async def setup_local_administrator(self, app, username, password, options):
                 pass
 
             try:
-                grp_obj = await self.middleware.call('group.get_group_obj', {'gid': ADMIN_GID})
+                grp_obj = await self.middleware.call('group.get_group_obj', {'gid': BuiltinLocalAdmins.admin_gid})
                 raise CallError(
-                    f'A {grp_obj["source"].lower()} group with gid={ADMIN_GID} already exists, '
+                    f'A {grp_obj["source"].lower()} group with gid={BuiltinLocalAdmins.admin_gid} already exists, '
                     'setting up local administrator is not possible',
                     errno.EEXIST,
                 )
@@ -1270,19 +1296,21 @@ async def setup_local_administrator(self, app, username, password, options):
             local_users = await self.middleware.call('user.query', [['local', '=', True]])
             local_groups = await self.middleware.call('group.query', [['local', '=', True]])
 
-            if filter_list(local_users, [['uid', '=', ADMIN_UID]]):
+            _uid = BuiltinLocalAdmins.admin_uid
+            if filter_list(local_users, [['uid', '=', _uid]]):
                 raise CallError(
-                    f'A user with uid={ADMIN_UID} already exists, setting up local administrator is not possible',
+                    f'A user with uid={_uid} already exists, setting up local administrator is not possible',
                     errno.EEXIST,
                 )
 
             if filter_list(local_users, [['username', '=', username]]):
                 raise CallError(f'{username!r} user already exists, setting up local administrator is not possible',
                                 errno.EEXIST)
 
-            if filter_list(local_groups, [['gid', '=', ADMIN_GID]]):
+            _gid = BuiltinLocalAdmins.admin_gid
+            if filter_list(local_groups, [['gid', '=', _gid]]):
                 raise CallError(
-                    f'A group with gid={ADMIN_GID} already exists, setting up local administrator is not possible',
+                    f'A group with gid={_gid} already exists, setting up local administrator is not possible',
                     errno.EEXIST,
                 )
 

src/middlewared/middlewared/plugins/account_/constants.py

@@ -1,3 +1,5 @@
+from dataclasses import dataclass
+
 ADMIN_UID = 950
 ADMIN_GID = 950
 SKEL_PATH = '/etc/skel/'  # TODO evaluate whether this is still needed
@@ -11,3 +13,11 @@
 LEGACY_DEFAULT_HOME_PATH = '/nonexistent'
 DEFAULT_HOME_PATH = '/var/empty'
 DEFAULT_HOME_PATHS = (DEFAULT_HOME_PATH, LEGACY_DEFAULT_HOME_PATH)
+
+
+@dataclass(slots=True, frozen=True)
+class BuiltinLocalAdmins:
+    truenas_admin: str = 'truenas_admin'
+    root: str = 'root'
+    admin_uid: int = ADMIN_UID
+    admin_gid: int = ADMIN_GID

src/middlewared/middlewared/plugins/filesystem_/acl.py

@@ -160,7 +160,7 @@ def chown(self, job, data):
             verrors.add("filesystem.chown.uid",
                         "Please specify either user or group to change.")
 
-        loc = self._common_perm_path_validate("filesystem.chown", data, verrors)
+        self._common_perm_path_validate("filesystem.chown", data, verrors)
         verrors.check()
 
         if not options['recursive']:
@@ -246,7 +246,7 @@ def setperm(self, job, data):
         uid = -1 if data['uid'] is None else data.get('uid', -1)
         gid = -1 if data['gid'] is None else data.get('gid', -1)
 
-        loc = self._common_perm_path_validate("filesystem.setperm", data, verrors)
+        self._common_perm_path_validate("filesystem.setperm", data, verrors)
 
         current_acl = self.middleware.call_sync('filesystem.getacl', data['path'])
         acl_is_trivial = current_acl['trivial']
@@ -1069,15 +1069,12 @@ def add_to_acl(self, job, data):
         set. For POSIX1E `READ` means read and execute, `MODIFY` means read, write,
         execute.
         """
-        init_path = data['path']
         verrors = ValidationErrors()
         self._common_perm_path_validate('filesystem.add_to_acl', data, verrors)
         verrors.check()
 
-        data['path'] = init_path
         current_acl = self.getacl(data['path'])
         acltype = ACLType[current_acl['acltype']]
-
         if acltype == ACLType.NFS4:
             changed = self.add_to_acl_nfs4(current_acl['acl'], data['entries'])
         elif acltype == ACLType.POSIX1E:
@@ -1120,12 +1117,10 @@ def get_inherited_acl(self, data):
         Supports `directory` `option` that allows specifying whether the generated
         ACL is for a file or a directory.
         """
-        init_path = data['path']
         verrors = ValidationErrors()
         self._common_perm_path_validate('filesystem.add_to_acl', data, verrors)
         verrors.check()
 
         current_acl = self.getacl(data['path'], False)
         acltype = ACLType[current_acl['acltype']]
-
         return acltype.calculate_inherited(current_acl, data['options']['directory'])

src/middlewared/middlewared/plugins/keychain.py

@@ -679,7 +679,9 @@ def ssh_pair(self, data):
         service = self.middleware.call_sync("service.query", [("service", "=", "ssh")], {"get": True})
         ssh = self.middleware.call_sync("ssh.config")
         try:
-            user = self.middleware.call_sync("user.query", [("username", "=", data["username"]), ("local", "=", True)], {"get": True})
+            user = self.middleware.call_sync(
+                "user.query", [("username", "=", data["username"]), ("local", "=", True)], {"get": True}
+            )
         except MatchNotFound:
             raise CallError(f"User {data['username']} does not exist")
 
@@ -704,6 +706,8 @@ def ssh_pair(self, data):
 
         # Write public key in user authorized_keys for SSH
         with open(f"{dotsshdir}/authorized_keys", "a+") as f:
+            os.fchmod(f.fileno(), 0o600)
+            os.fchown(f.fileno(), user["uid"], user["group"]["bsdgrp_gid"])
             f.seek(0)
             if data["public_key"] not in f.read():
                 f.write("\n" + data["public_key"] + "\n")

tests/api2/test_account_home.py

@@ -0,0 +1,20 @@
+from middlewared.test.integration.assets.account import user
+from middlewared.test.integration.assets.pool import dataset
+from middlewared.test.integration.utils import call, ssh
+
+
+def test_chown_on_update():
+    """This test ensures that on user.update, if the users
+        home directory permissions are incorrect, we will
+        fix (chown) them accordingly."""
+    with dataset("unpriv_homedir") as homedir:
+        temp = {"username": "unpriv", "full_name": "unpriv", "group_create": True, "password": "pass"}
+        with user(temp) as u:
+            ssh(f"chown nobody:nobody /mnt/{homedir}")
+            st = call("filesystem.stat", f"/mnt{homedir}")
+            assert st["user"] == "nobody"
+            assert st["group"] == "nobody"
+            call("user.update", u["id"], {"home": f"/mnt/{homedir}"})
+            st = call("filesystem.stat", f"/mnt/{homedir}")
+            assert st["user"] == "unpriv"
+            assert st["group"] == "unpriv"