On 10/29/2014 02:34 PM, David Kupka wrote:
On 10/24/2014 03:05 PM, David Kupka wrote:
On 10/24/2014 01:06 PM, David Kupka wrote:
On 10/24/2014 10:43 AM, Martin Basti wrote:
On 24/10/14 09:51, David Kupka wrote:
https://fedorahosted.org/freeipa/ticket/4585
NACK

1)
Why is there line with 'DS System User?' The comment should depend on
service.

+            args = [
+                paths.USERADD,
+                '-g', group,
+                '-c', 'DS System User',
+                '-d', homedir,
+                '-s', shell,
+                '-M', '-r', name,
+            ]

This was part of the original code and I didn't notice it. Nice catch,
thanks.


2)
code create_system_user is duplicated between base and redhat tasks
with
platform dependent changes.
IMO it would be better to have one method to create user, with keyword
arguments.  And then platform dependent method which will call
method to
create user with appropriate arguments (or with default arguments)


You're right it was ugly.



_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

I shouldn't break SOLID principles.



_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Using super is probably better that explicit naming of parent class.
Let user (developer) override UID/GID and hope that he knows why ...


_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


--
David Kupka
From ec277c8d8d9fc66f31236f306e038d39aa7417fb Mon Sep 17 00:00:00 2001
From: David Kupka <dku...@redhat.com>
Date: Wed, 22 Oct 2014 09:07:44 -0400
Subject: [PATCH] Respect UID and GID soft static allocation.

https://fedoraproject.org/wiki/Packaging:UsersAndGroups?rd=Packaging/UsersAndGroups#Soft_static_allocation

https://fedorahosted.org/freeipa/ticket/4585
---
 ipaplatform/base/tasks.py         | 48 +++++++++++++++++++++++++++++++++++++++
 ipaplatform/redhat/tasks.py       | 23 +++++++++++++++++++
 ipaserver/install/cainstance.py   |  2 +-
 ipaserver/install/dsinstance.py   |  2 +-
 ipaserver/install/installutils.py | 42 ----------------------------------
 5 files changed, 73 insertions(+), 44 deletions(-)

diff --git a/ipaplatform/base/tasks.py b/ipaplatform/base/tasks.py
index 408447e43cd36d0cdf11a1877b3bc9880c4785de..f2ba81f44bb991b218232aad84d7810cdae839ef 100644
--- a/ipaplatform/base/tasks.py
+++ b/ipaplatform/base/tasks.py
@@ -22,7 +22,13 @@
 This module contains default platform-specific implementations of system tasks.
 '''
 
+import pwd
+import grp
 from ipaplatform.paths import paths
+from ipapython.ipa_log_manager import log_mgr
+from ipapython import ipautil
+
+log = log_mgr.get_logger(__name__)
 
 
 class BaseTaskNamespace(object):
@@ -150,5 +156,47 @@ class BaseTaskNamespace(object):
 
         return
 
+    def create_system_user(self, name, group, homedir, shell, uid = None, gid = None, comment = None):
+        """Create a system user with a corresponding group"""
+        try:
+            grp.getgrnam(group)
+        except KeyError:
+            log.debug('Adding group %s', group)
+            args = [paths.GROUPADD, '-r', group]
+            if gid:
+                args += ['-g', str(gid)]
+            try:
+                ipautil.run(args)
+                log.debug('Done adding group')
+            except ipautil.CalledProcessError as e:
+                log.critical('Failed to add group: %s', e)
+                raise
+        else:
+            log.debug('group %s exists', group)
+
+        try:
+            pwd.getpwnam(name)
+        except KeyError:
+            log.debug('Adding user %s', name)
+            args = [
+                paths.USERADD,
+                '-g', group,
+                '-d', homedir,
+                '-s', shell,
+                '-M', '-r', name,
+            ]
+            if uid:
+                args += ['-u', str(uid)]
+            if comment:
+                args += ['-c', comment]
+            try:
+                ipautil.run(args)
+                log.debug('Done adding user')
+            except ipautil.CalledProcessError as e:
+                log.critical('Failed to add user: %s', e)
+                raise
+        else:
+            log.debug('user %s exists', name)
+
 
 task_namespace = BaseTaskNamespace()
diff --git a/ipaplatform/redhat/tasks.py b/ipaplatform/redhat/tasks.py
index 555516d90a6d1a7d3d9aced5de82a5c1efe6b8c2..3f5fc90b4b988d44cf0dcaa5a8d569a3c5f453ee 100644
--- a/ipaplatform/redhat/tasks.py
+++ b/ipaplatform/redhat/tasks.py
@@ -393,5 +393,28 @@ class RedHatTaskNamespace(BaseTaskNamespace):
 
         return True
 
+    def create_system_user(self, name, group, homedir, shell, uid = None, gid = None, comment = None):
+        """
+        Create a system user with a corresponding group
+
+        According to https://fedoraproject.org/wiki/Packaging:UsersAndGroups?rd=Packaging/UsersAndGroups#Soft_static_allocation
+        some system users should have fixed UID, GID and other parameters set.
+        This values should be constant and may be hardcoded.
+        Add other values for other users when needed.
+        """
+        if name == 'pkiuser':
+            if uid is None:
+                uid = 17
+            if gid is None:
+                gid = 17
+            if comment is None:
+                comment = 'CA System User'
+        if name == 'dirsrv':
+            if comment is None:
+                comment = 'DS System User'
+
+        super(RedHatTaskNamespace, self).create_system_user(name, group,
+            homedir, shell, uid, gid, comment)
+
 
 tasks = RedHatTaskNamespace()
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 0c31d21648689ad5c577e9112fefdf47857b4915..6ccbe415eca08b8d3b3dc7b04c04cffb5fdf7459 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -261,7 +261,7 @@ def is_step_one_done():
 
 def create_ca_user():
     """Create PKI user/group if it doesn't exist yet."""
-    installutils.create_system_user(
+    tasks.create_system_user(
         name=PKI_USER,
         group=PKI_USER,
         homedir=paths.VAR_LIB,
diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py
index da535347117166e6cb445b0ebf14ad71787f72ba..4293c5c90e9b1371a417e1158d63013a6c3bcee3 100644
--- a/ipaserver/install/dsinstance.py
+++ b/ipaserver/install/dsinstance.py
@@ -152,7 +152,7 @@ def is_ds_running(server_id=''):
 
 def create_ds_user():
     """Create DS user/group if it doesn't exist yet."""
-    installutils.create_system_user(
+    tasks.create_system_user(
         name=DS_USER,
         group=DS_USER,
         homedir=paths.VAR_LIB_DIRSRV,
diff --git a/ipaserver/install/installutils.py b/ipaserver/install/installutils.py
index 1e010edc6bf569a879c4f68efd2b273f57b01770..9cda26f16f09ddc04e708a94858fb039ee3a8360 100644
--- a/ipaserver/install/installutils.py
+++ b/ipaserver/install/installutils.py
@@ -29,8 +29,6 @@ from ConfigParser import SafeConfigParser, NoOptionError
 import traceback
 import textwrap
 from contextlib import contextmanager
-import pwd
-import grp
 
 from dns import resolver, rdatatype
 from dns.exception import DNSException
@@ -83,8 +81,6 @@ class ReplicaConfig:
 
     subject_base = ipautil.dn_attribute_property('_subject_base')
 
-log = log_mgr.get_logger(__name__)
-
 def get_fqdn():
     fqdn = ""
     try:
@@ -974,41 +970,3 @@ def load_external_cert(files, subject_base):
     ca_file.flush()
 
     return cert_file, ca_file
-
-
-def create_system_user(name, group, homedir, shell):
-    """Create a system user with a corresponding group"""
-    try:
-        grp.getgrnam(group)
-    except KeyError:
-        log.debug('Adding group %s', group)
-        args = [paths.GROUPADD, '-r', group]
-        try:
-            ipautil.run(args)
-            log.debug('Done adding group')
-        except ipautil.CalledProcessError as e:
-            log.critical('Failed to add group: %s', e)
-            raise
-    else:
-        log.debug('group %s exists', group)
-
-    try:
-        pwd.getpwnam(name)
-    except KeyError:
-        log.debug('Adding user %s', name)
-        args = [
-            paths.USERADD,
-            '-g', group,
-            '-c', 'DS System User',
-            '-d', homedir,
-            '-s', shell,
-            '-M', '-r', name,
-        ]
-        try:
-            ipautil.run(args)
-            log.debug('Done adding user')
-        except ipautil.CalledProcessError as e:
-            log.critical('Failed to add user: %s', e)
-            raise
-    else:
-        log.debug('user %s exists', name)
-- 
2.1.0

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to