[master/f19-branch] Fix user creation with existing home directory

[Vratislav Podzimek]

When using an existing /home partition, we shouldn't fail to create a user that
already has the home directory under it. We should create the user and just
change the owner of the particular home directory to the UID of the newly
created user.

I'm not aware of any bug filed on this, but I think it should go to both master
and f19-branch, because it is and expected behaviour and a common use case. A
question is if we should show some warning, but I believe it is not necessary
and I would wait for a potential request to do so before starting to bother a
lot of users.

These patches automatically apply to the Initial Setup's functionality which
means they fix a regression between the Firstboot and the i-s.

Vratislav Podzimek (3):
  Add a function to recursively change owner of a directory
  Do change root properly when creating users
  Handle the case where home directory already exists

pyanaconda/iutil.py | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 pyanaconda/users.py | 16 +++++++++++++++-
 2 files changed, 66 insertions(+), 1 deletion(-)

[PATCH 1/3] Add a function to recursively change owner of a directory

[Vratislav Podzimek]

Implemented by using a universal function for mapping a function on the
directory tree.

Signed-off-by: Vratislav Podzimek 
---
 pyanaconda/iutil.py | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py
index 7020da5..459a890 100644
--- a/pyanaconda/iutil.py
+++ b/pyanaconda/iutil.py
@@ -628,3 +628,54 @@ def cmp_obj_attrs(obj1, obj2, attr_list):
         else:
             return False
     return True
+
+def dir_tree_map(root, func, files=True, dirs=True):
+    """
+    Apply the given function to all files and directories in the directory tree
+    under the given root directory.
+
+    :param root: root of the directory tree the function should be mapped to
+    :type root: str
+    :param func: a function taking the directory/file path
+    :type func: path -> None
+    :param files: whether to apply the function to the files in the dir. tree
+    :type files: bool
+    :param dirs: whether to apply the function to the directories in the dir. tree
+    :type dirs: bool
+
+    TODO: allow using globs and thus more trees?
+
+    """
+
+    for (dir_ent, dirs, files) in os.walk(root):
+        # try to call the function on the directory entry
+        try:
+            func(dir_ent)
+        except:
+            pass
+
+        # try to call the function on the files in the directory entry
+        for file_ent in (os.path.join(dir_ent, f) for f in files):
+            try:
+                func(file_ent)
+            except:
+                pass
+
+        # directories under the directory entry will appear as directory entries
+        # in the loop
+
+def chown_dir_tree(root, uid, gid):
+    """
+    Change owner (uid and gid) of the files and directories under the given
+    directory tree (recursively).
+
+    :param root: root of the directory tree that should be chown'ed
+    :type root: str
+    :param uid: UID that should be set as the owner
+    :type uid: int
+    :param gid: GID that should be set as the owner
+    :type gid: int
+
+    """
+
+    dir_tree_map(root, lambda path: os.chown(path, uid, gid))

[PATCH 1/3] Add a function to recursively change owner of a directory

[Vratislav Podzimek]

On Thu, 2013-05-30 at 16:53 +0200, Vratislav Podzimek wrote:
> Implemented by using a universal function for mapping a function on the
> directory tree.
> Signed-off-by: Vratislav Podzimek
>
> pyanaconda/iutil.py | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>
> diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py
> index 7020da5..459a890 100644
> --- a/pyanaconda/iutil.py
> +++ b/pyanaconda/iutil.py
> @@ -628,3 +628,54 @@ def cmp_obj_attrs(obj1, obj2, attr_list):
>          else:
>              return False
>      return True
>
> +def dir_tree_map(root, func, files=True, dirs=True):
> """
> Apply the given function to all files and directories in the directory tree
> under the given root directory.
> :param root: root of the directory tree the function should be mapped to
> :type root: str
> :param func: a function taking the directory/file path
> :type func: path -> None
> :param files: whether to apply the function to the files in the dir. tree
> :type files: bool
> :param dirs: whether to apply the function to the directories in the dir. tree
> :type dirs: bool
> TODO: allow using globs and thus more trees?
> """
> for (dir_ent, dirs, files) in os.walk(root):
>    # try to call the function on the directory entry
>    try:
>        func(dir_ent)
>    except:
>        pass
>    # try to call the function on the files in the directory entry
>    for file_ent in (os.path.join(dir_ent, f) for f in files):
>        try:
>            func(file_ent)
>        except:
>            pass
>    # directories under the directory entry will appear as directory entries
>    # in the loop

I see that the version I sent misses two ifs around the function
invocation that would make the 'files' and 'dirs' parameters work.
Already fixed locally.

[PATCH 2/3] Do change root properly when creating users

[Vratislav Podzimek]

See e7b877c296e8d43dc32b75888b7fbf9790565c01 for more info.

Signed-off-by: Vratislav Podzimek 
---
 pyanaconda/users.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/pyanaconda/users.py b/pyanaconda/users.py
index 212184b..7ec594c 100644
--- a/pyanaconda/users.py
+++ b/pyanaconda/users.py
@@ -246,6 +246,7 @@ class Users:
         if not childpid:
             if not root in ["","/"]:
                 os.chroot(root)
+                os.chdir("/")
                 del(os.environ["LIBUSER_CONF"])

self.admin = libuser.admin()

[PATCH 3/3] Handle the case where home directory already exists

[Vratislav Podzimek]

Instead of failing to add the user whose home directory already exists, we
should just fix the owner of the home directory to the UID of the newly created
user.

Signed-off-by: Vratislav Podzimek 
---
 pyanaconda/users.py | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/pyanaconda/users.py b/pyanaconda/users.py
index 7ec594c..598ac08 100644
--- a/pyanaconda/users.py
+++ b/pyanaconda/users.py
@@ -281,9 +281,22 @@ class Users:
                 if kwargs.get("gecos", False):
                     userEnt.set(libuser.GECOS, kwargs["gecos"])

-                self.admin.addUser(userEnt, mkmailspool=kwargs.get("mkmailspool", True))
+                # need to create home directory for the user or does it already exist?
+                # userEnt.get returns lists (usually with a single item)
+                mk_homedir = not os.path.exists(userEnt.get(libuser.HOMEDIRECTORY)[0])
+
+                self.admin.addUser(userEnt, mkmailspool=kwargs.get("mkmailspool", True),
+                                   mkhomedir=mk_homedir)
                 self.admin.addGroup(groupEnt)

+                if not mk_homedir:
+                    log.info("Home directory for the user %s already existed, "
+                             "fixing the owner." % user_name)
+                    # home directory already existed, change owner of it properly
+                    iutil.chown_dir_tree(userEnt.get(libuser.HOMEDIRECTORY)[0],
+                                         userEnt.get(libuser.UIDNUMBER)[0],
+                                         groupEnt.get(libuser.GIDNUMBER)[0])
+
                 if kwargs.get("password", False):
                     if kwargs.get("isCrypted", False):
                         password = kwargs["password"]

[Brian C. Lane]

On Thu, May 30, 2013 at 04:53:41PM +0200, Vratislav Podzimek wrote:
> When using an existing /home partition, we shouldn't fail to create a user that
> already has the home directory under it. We should create the user and just
> change the owner of the particular home directory to the UID of the newly
> created user.

I don't think this is a good idea. I don't think we can clearly
anticipate all the possible problems with changing ownership of all the
files. It might be ok to just change the ones owned by the same uid/gid
as the top level directory, but even then I'm reluctant to change user
data.

The original problem can be fixed fairly easily by the user after
install.

[Vratislav Podzimek]

On Mon, 2013-06-03 at 13:49 -0700, Brian C. Lane wrote:
> On Thu, May 30, 2013 at 04:53:41PM +0200, Vratislav Podzimek wrote:
> > When using an existing /home partition, we shouldn't fail to create a user that
> > already has the home directory under it. We should create the user and just
> > change the owner of the particular home directory to the UID of the newly
> > created user.
>
> I don't think this is a good idea. I don't think we can clearly
> anticipate all the possible problems with changing ownership of all the
> files. It might be ok to just change the ones owned by the same uid/gid
> as the top level directory, but even then I'm reluctant to change user
> data.

I'm for changing the ownership of the files/directories owned by the
same uid/gid as the top level directory. Doing nothing would result in a
completely broken system if the newly created user has a different uid
than the uid of "their" home directory (which is not so unlikely to
happen).
> The original problem can be fixed fairly easily by the user after
> install.

I don't think it would be intuitive to find out what the problem is and
how to fix it. On the other hand, preserving /home is a common use case.