On 19/09/14 14:30, Jan Cholasta wrote:
Dne 19.9.2014 v 13:32 Martin Basti napsal(a):
On 01/09/14 16:26, Martin Basti wrote:
On 28/08/14 14:01, Jan Cholasta wrote:
Hi,

Dne 27.8.2014 v 15:22 Martin Basti napsal(a):
Patch attached.


1) Please rename object_exists to entry_exists.


2) Use empty attribute list in get_entry() in
object_exists/entry_exists.


3) Please update LDAPObject.get_dn_if_exists() to use
object_exists/entry_exists.


4) I'm not a fan of how do_bind() is laid out, IMHO something like
this would be better (untested):

+    def do_bind(self, dm_password=None, autobind=AUTOBIND_AUTO,
timeout=DEFAULT_TIMEOUT):
+        if dm_password:
+            self.do_simple_bind(bindpw=dm_password, timeout=timeout)
+            return
+
+        if autobind != AUTOBIND_DISABLED and os.getegid() == 0 and
self.ldapi:
+            try:
+                # autobind
+                pw_name = pwd.getpwuid(os.geteuid()).pw_name
+                self.do_external_bind(pw_name, timeout=timeout)
+                return
+            except errors.NotFound:
+                if autobind == AUTOBIND_ENABLED:
+                    # autobind was required and failed, raise
+                    # exception that it failed
+                    raise
+
+        # Fall back
+        self.do_sasl_gssapi_bind(timeout=timeout)


Honza

3) skipped as we discuss on IRC

Updated patch attached



_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Please review, this should be in 4.1

1) The patch need a rebase on top of current ipa-4-1.
I can apply it (Am I doing something wrong?)


2) You can remove import pwd from service.py, it is no longer used there.


3) Are named constants for the autobind argument the right thing to do? It is a tri-state which can be expressed with None/True/False. (I'm just asking, I don't have a strong opinion on this.)

As we discussed on IRC, using None/True/False, is not good approach.
Updated patch attached

--
Martin Basti

From 60a3e9972e6154ce4628a1d210941e7d621d0a1d Mon Sep 17 00:00:00 2001
From: Martin Basti <mba...@redhat.com>
Date: Wed, 27 Aug 2014 15:06:42 +0200
Subject: [PATCH] Refactoring of autobind, object_exists

Required to prevent code duplications

ipaldap.IPAdmin now has method do_bind, which tries several bind methods
ipaldap.IPAClient now has method object_exists(dn)
---
 install/tools/ipa-adtrust-install |  4 ++--
 ipapython/ipaldap.py              | 37 +++++++++++++++++++++++++++++++++++++
 ipaserver/install/bindinstance.py | 25 +++++--------------------
 ipaserver/install/cainstance.py   |  2 +-
 ipaserver/install/dsinstance.py   |  2 +-
 ipaserver/install/service.py      | 30 ++++--------------------------
 6 files changed, 50 insertions(+), 50 deletions(-)

diff --git a/install/tools/ipa-adtrust-install b/install/tools/ipa-adtrust-install
index 7b616c1b65c60945a2e5dc19c4afc39dad285978..6e55bbe3e57f1c609398dc571e90cb8677d91a33 100755
--- a/install/tools/ipa-adtrust-install
+++ b/install/tools/ipa-adtrust-install
@@ -25,7 +25,7 @@ from ipaserver.install import adtrustinstance
 from ipaserver.install.installutils import *
 from ipaserver.install import service
 from ipapython import version
-from ipapython import ipautil, sysrestore
+from ipapython import ipautil, sysrestore, ipaldap
 from ipalib import api, errors, util
 from ipapython.config import IPAOptionParser
 import krbV
@@ -405,7 +405,7 @@ def main():
 
     smb = adtrustinstance.ADTRUSTInstance(fstore)
     smb.realm = api.env.realm
-    smb.autobind = service.ENABLED
+    smb.autobind = ipaldap.AUTOBIND_ENABLED
     smb.setup(api.env.host, ip_address, api.env.realm, api.env.domain,
               netbios_name, reset_netbios_name,
               options.rid_base, options.secondary_rid_base,
diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 2818f787bd8a78b358e27f88de6ccdb214011986..1702daa253d7eb568c27f66fda1810b4661656ad 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -27,6 +27,8 @@ from decimal import Decimal
 from copy import deepcopy
 import contextlib
 import collections
+import os
+import pwd
 
 import ldap
 import ldap.sasl
@@ -53,6 +55,10 @@ _debug_log_ldap = False
 
 _missing = object()
 
+# Autobind modes
+AUTOBIND_AUTO = 1
+AUTOBIND_ENABLED = 2
+AUTOBIND_DISABLED = 3
 
 def unicode_from_utf8(val):
     '''
@@ -1633,6 +1639,18 @@ class LDAPClient(object):
         with self.error_handler():
             self.conn.delete_s(dn)
 
+    def entry_exists(self, dn):
+        """
+        Test whether the given object exists in LDAP.
+        """
+        assert isinstance(dn, DN)
+        try:
+            self.get_entry(dn, attrs_list=[])
+        except errors.NotFound:
+            return False
+        else:
+            return True
+
 
 class IPAdmin(LDAPClient):
 
@@ -1742,6 +1760,25 @@ class IPAdmin(LDAPClient):
         self.__bind_with_wait(
             self.conn.sasl_interactive_bind_s, timeout, None, auth_tokens)
 
+    def do_bind(self, dm_password="", autobind=AUTOBIND_AUTO, timeout=DEFAULT_TIMEOUT):
+        if dm_password:
+            self.do_simple_bind(bindpw=dm_password, timeout=timeout)
+            return
+        if autobind != AUTOBIND_DISABLED and os.getegid() == 0 and self.ldapi:
+            try:
+                # autobind
+                pw_name = pwd.getpwuid(os.geteuid()).pw_name
+                self.do_external_bind(pw_name, timeout=timeout)
+                return
+            except errors.NotFound, e:
+                if autobind == AUTOBIND_ENABLED:
+                    # autobind was required and failed, raise
+                    # exception that it failed
+                    raise
+
+        #fall back
+        self.do_sasl_gssapi_bind(timeout=timeout)
+
     def modify_s(self, *args, **kwargs):
         # FIXME: for backwards compatibility only
         return self.conn.modify_s(*args, **kwargs)
diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index 9a27c781764f3dc311d20cfcf9150fde31307b03..dfb086f109371ae97151622e2c44db27256896db 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -202,23 +202,11 @@ def named_conf_set_directive(name, value, section=NAMED_SECTION_IPA,
     with open(NAMED_CONF, 'w') as f:
         f.write("".join(new_lines))
 
-def dns_container_exists(fqdn, suffix, dm_password=None, ldapi=False, realm=None):
+def dns_container_exists(fqdn, suffix, dm_password=None, ldapi=False, realm=None,
+                         autobind=ipaldap.AUTOBIND_DISABLED):
     """
     Test whether the dns container exists.
     """
-
-    def object_exists(dn):      # FIXME, this should be a IPAdmin/ldap2 method so it can be shared
-        """
-        Test whether the given object exists in LDAP.
-        """
-        assert isinstance(dn, DN)
-        try:
-            conn.get_entry(dn)
-        except errors.NotFound:
-            return False
-        else:
-            return True
-
     assert isinstance(suffix, DN)
     try:
         # At install time we may need to use LDAPI to avoid chicken/egg
@@ -228,14 +216,11 @@ def dns_container_exists(fqdn, suffix, dm_password=None, ldapi=False, realm=None
         else:
             conn = ipaldap.IPAdmin(host=fqdn, port=636, cacert=CACERT)
 
-        if dm_password:
-            conn.do_simple_bind(bindpw=dm_password)
-        else:
-            conn.do_sasl_gssapi_bind()
+        conn.do_bind(dm_password, autobind=autobind)
     except ldap.SERVER_DOWN:
         raise RuntimeError('LDAP server on %s is not responding. Is IPA installed?' % fqdn)
 
-    ret = object_exists(DN(('cn', 'dns'), suffix))
+    ret = conn.entry_exists(DN(('cn', 'dns'), suffix))
     conn.unbind()
 
     return ret
@@ -461,7 +446,7 @@ class BindInstance(service.Service):
             service_desc="DNS",
             dm_password=dm_password,
             ldapi=False,
-            autobind=service.DISABLED
+            autobind=ipaldap.AUTOBIND_DISABLED
             )
         self.dns_backup = DnsBackup(self)
         self.named_user = None
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 45c72198df89cafd7de2775c1233f7cb4f0081fd..b0037dd568e2f4a115436a6cb621e10bcc02f4d9 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -271,7 +271,7 @@ class CADSInstance(service.Service):
             service_desc="directory server for the CA",
             dm_password=dm_password,
             ldapi=False,
-            autobind=service.DISABLED)
+            autobind=ipaldap.AUTOBIND_DISABLED)
 
         self.serverid = "PKI-IPA"
         self.realm = realm_name
diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py
index 0edd4ed635eff96e0d534848240dc30da2b4971e..6e79dc51fc2da03b4067f0f13dfd74ac201978f7 100644
--- a/ipaserver/install/dsinstance.py
+++ b/ipaserver/install/dsinstance.py
@@ -192,7 +192,7 @@ class DsInstance(service.Service):
             service_desc="directory server",
             dm_password=dm_password,
             ldapi=False,
-            autobind=service.DISABLED
+            autobind=ipaldap.AUTOBIND_DISABLED
             )
         self.nickname = 'Server-Cert'
         self.dm_password = dm_password
diff --git a/ipaserver/install/service.py b/ipaserver/install/service.py
index 585c903bdd4dee66d93c426299d59de95d0ea625..7f812a99c557ae567cd1d8a0db30eba434c507f9 100644
--- a/ipaserver/install/service.py
+++ b/ipaserver/install/service.py
@@ -20,7 +20,6 @@
 import sys
 import os, socket
 import tempfile
-import pwd
 import time
 import datetime
 
@@ -31,10 +30,6 @@ from ipalib import errors, certstore
 from ipaplatform import services
 from ipaplatform.paths import paths
 
-# Autobind modes
-AUTO = 1
-ENABLED = 2
-DISABLED = 3
 
 # The service name as stored in cn=masters,cn=ipa,cn=etc. In the tuple
 # the first value is the *nix service name, the second the start order.
@@ -73,7 +68,8 @@ def format_seconds(seconds):
 
 
 class Service(object):
-    def __init__(self, service_name, service_desc=None, sstore=None, dm_password=None, ldapi=True, autobind=AUTO):
+    def __init__(self, service_name, service_desc=None, sstore=None, dm_password=None, ldapi=True,
+                 autobind=ipaldap.AUTOBIND_AUTO):
         self.service_name = service_name
         self.service_desc = service_desc
         self.service = services.service(service_name)
@@ -109,26 +105,8 @@ class Service(object):
                 conn = ipaldap.IPAdmin(ldapi=self.ldapi, realm=self.realm)
             else:
                 conn = ipaldap.IPAdmin(self.fqdn, port=389)
-            if self.dm_password:
-                conn.do_simple_bind(bindpw=self.dm_password)
-            elif self.autobind in [AUTO, ENABLED]:
-                if os.getegid() == 0 and self.ldapi:
-                    try:
-                        # autobind
-                        pw_name = pwd.getpwuid(os.geteuid()).pw_name
-                        conn.do_external_bind(pw_name)
-                    except errors.NotFound, e:
-                        if self.autobind == AUTO:
-                            # Fall back
-                            conn.do_sasl_gssapi_bind()
-                        else:
-                            # autobind was required and failed, raise
-                            # exception that it failed
-                            raise e
-                else:
-                    conn.do_sasl_gssapi_bind()
-            else:
-                conn.do_sasl_gssapi_bind()
+
+            conn.do_bind(self.dm_password, autobind=self.autobind)
         except Exception, e:
             root_logger.debug("Could not connect to the Directory Server on %s: %s" % (self.fqdn, str(e)))
             raise
-- 
1.8.3.1

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

Reply via email to