On 06/10/2013 11:11 AM, Tomas Babej wrote:
> On 06/07/2013 05:49 PM, Ana Krivokapic wrote:
>> On 06/07/2013 12:15 PM, Tomas Babej wrote:
>>> On 05/31/2013 12:08 PM, Ana Krivokapic wrote:
>>>> Hello,
>>>>
>>>> This patch addresses ticket https://fedorahosted.org/freeipa/ticket/3635.
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Freeipa-devel mailing list
>>>> Freeipa-devel@redhat.com
>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>
>>> Hi, the patch itself works as it should, I have only some
>>> refactoring comments:
>>>
>>> 1.) There already is a add_range method that is called from within
>>> trust_add's execute()
>>> and handles some range validation (currently only whether domain's
>>> SID of new and
>>> existing range are equal, my patch 67 add some more).
>>>
>>> Your patch introduces a new method validate_range() that is called
>>> from execute(),
>>> which leads to some duplication of the logic, mainly two same calls
>>> to the API (getting
>>> the info about the old range via idrange_show)
>>>
>>> I'd rather we keep all the validation in one place, preferably
>>> inside the add_range method
>>> or refactored out of it in the new method that is called only from
>>> add_range().
>>>
>>> 2.) That being said, other parts of refactoring are nice and make
>>> code more clear, +1.
>>>
>>> Tomas
>>
>> My first instinct was to place this validation in the add_range()
>> method. However, add_range() is called after the trust itself is
>> added, and validation introduced by this patch needs to happen before
>> the trust is added (we need to prevent the addition of trust in case
>> the validation fails).
>>
>> As for the code duplication, you are right about that. I tried to
>> minimize duplication - resulting updated patch attached.
>> -- 
>> Regards,
>>
>> Ana Krivokapic
>> Associate Software Engineer
>> FreeIPA team
>> Red Hat Inc.
>
> While this is a nice improvement from the code repetition point of view,
> the patch still has one flaw as I see it - the range validation
> happens at two separate places:
>
> Once here(already in the code):
>
> if old_range:
>
> old_dom_sid = old_range['result']['ipanttrusteddomainsid'][0];
>
>
> if old_dom_sid == dom_sid:
>
> return
>
>
> raise errors.ValidationError(name=_('range exists'),
>
> error=_('ID range with the same name but different ' \
>
> 'domain SID already exists. The ID range for ' \
>
> 'the new trusted domain must be created manually.'))
>
>
> And once here(added by your patch):
>
> +        if not self.validate_range(*keys, **options):
> +            raise errors.ValidationError(
> +                name=_('id range'),
> +                error=_(
> +                    'An id range already exists for this trust. '
> +                    'You should either delete the old range, or '
> +                    'exclude --base-id/--range-size options from the
> command.'
>
> I'd suggest we have one common place, say validate_range() function as
> we have now,
> that contains all the checks (more coming in the future for sure).
>
> That would mean that the whole trust-add command will fail in the case
> that "ID range
> with the same name but different domain SID already exists", since we
> now call validate_range()
> within execute() method. I checked with Alexander and we agreed that
> this is more desired behaviour.
>
> This would also result to reducement of the number of API calls, which
> is a nice benefit.
>
> Tomas

This updated patch completely separates validation logic and object
creation logic of the trust_add command. I added two new methods:

* validate_range(), which ensures that the options and environment
related to idrange is valid, and
* validate_options, which takes care of other general validation

One change introduced in this patch is making the
__populate_remote_domain() method of TrustDomainJoins class public, and
calling it from trust_add. This was necessary in order to enable
discovering details of the trusted domain in validation phase.


-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

From dd35e8749a83dde6508a58ae63ee0191e0b7bd40 Mon Sep 17 00:00:00 2001
From: Ana Krivokapic <akriv...@redhat.com>
Date: Fri, 31 May 2013 12:01:23 +0200
Subject: [PATCH] Fail when adding a trust with a different range

When adding a trust, if an id range already exists for this trust,
and options --base-id/--range-size are provided with the trust-add command,
trust-add should fail.

https://fedorahosted.org/freeipa/ticket/3635
---
 ipalib/plugins/trust.py | 204 +++++++++++++++++++++++++++++++++---------------
 ipaserver/dcerpc.py     |   8 +-
 2 files changed, 142 insertions(+), 70 deletions(-)

diff --git a/ipalib/plugins/trust.py b/ipalib/plugins/trust.py
index 3cb0ed98005ae5bd11b39f8ae01c9470d1bfc9c4..89d77320798d6f1e109a2b2a18016f6902792bd9 100644
--- a/ipalib/plugins/trust.py
+++ b/ipalib/plugins/trust.py
@@ -155,6 +155,8 @@
                         autofill=True,
                     )
 
+DEFAULT_RANGE_SIZE = 200000
+
 def trust_type_string(level):
     """
     Returns a string representing a type of the trust. The original field is an enum:
@@ -287,7 +289,7 @@ class trust_add(LDAPCreate):
         Int('range_size?',
             cli_name='range_size',
             label=_('Size of the ID range reserved for the trusted domain'),
-            default=200000,
+            default=DEFAULT_RANGE_SIZE,
             autofill=True
         ),
     )
@@ -296,20 +298,12 @@ class trust_add(LDAPCreate):
     has_output_params = LDAPCreate.has_output_params + trust_output_params
 
     def execute(self, *keys, **options):
-        if not _murmur_installed and 'base_id' not in options:
-            raise errors.ValidationError(name=_('missing base_id'),
-                error=_('pysss_murmur is not available on the server ' \
-                        'and no base-id is given.'))
+        full_join = self.validate_options(*keys, **options)
+        old_range, range_name, dom_sid = self.validate_range(*keys, **options)
+        result = self.execute_ad(full_join, *keys, **options)
 
-        if 'trust_type' in options:
-            if options['trust_type'] == u'ad':
-                result = self.execute_ad(*keys, **options)
-            else:
-                raise errors.ValidationError(name=_('trust type'), error=_('only "ad" is supported'))
-        else:
-            raise errors.RequirementError(name=_('trust type'))
-
-        self.add_range(*keys, **options)
+        if not old_range:
+            self.add_range(range_name, dom_sid, **options)
 
         trust_filter = "cn=%s" % result['value']
         ldap = self.obj.backend
@@ -325,32 +319,134 @@ def execute(self, *keys, **options):
 
         return result
 
-    def add_range(self, *keys, **options):
-        new_obj = api.Command['trust_show'](keys[-1])
-        dom_sid = new_obj['result']['ipanttrusteddomainsid'][0];
+    def validate_options(self, *keys, **options):
+        if not _bindings_installed:
+            raise errors.NotFound(
+                name=_('AD Trust setup'),
+                reason=_(
+                    'Cannot perform join operation without Samba 4 support '
+                    'installed. Make sure you have installed server-trust-ad '
+                    'sub-package of IPA'
+                )
+            )
 
+        if not _murmur_installed and 'base_id' not in options:
+            raise errors.ValidationError(
+                name=_('missing base_id'),
+                error=_(
+                    'pysss_murmur is not available on the server '
+                    'and no base-id is given.'
+                )
+            )
+
+        if 'trust_type' not in options:
+            raise errors.RequirementError(name=_('trust type'))
+
+        if options['trust_type'] != u'ad':
+            raise errors.ValidationError(
+                name=_('trust type'),
+                error=_('only "ad" is supported')
+            )
+
+        self.trustinstance = ipaserver.dcerpc.TrustDomainJoins(self.api)
+        if not self.trustinstance.configured:
+            raise errors.NotFound(
+                name=_('AD Trust setup'),
+                reason=_(
+                    'Cannot perform join operation without own domain '
+                    'configured. Make sure you have run ipa-adtrust-install '
+                    'on the IPA server first'
+                )
+            )
+
+        try:
+            self.realm_server = options['realm_server']
+        except KeyError:
+            self.realm_server = None
+
+        if 'realm_admin' in options:
+            self.realm_admin = options['realm_admin']
+            names = self.realm_admin.split('@')
+
+            if len(names) > 1:
+                # realm admin name is in UPN format, user@realm, check that
+                # realm is the same as the one that we are attempting to trust
+                if keys[-1].lower() != names[-1].lower():
+                    raise errors.ValidationError(
+                        name=_('AD Trust setup'),
+                        error=_(
+                            'Trusted domain and administrator account use '
+                            'different realms'
+                        )
+                    )
+                self.realm_admin = names[0]
+
+            try:
+                self.realm_passwd = options['realm_passwd']
+            except KeyError:
+                raise errors.ValidationError(
+                    name=_('AD Trust setup'),
+                    error=_('Realm administrator password should be specified')
+                )
+            return True
+        else:
+            self.realm_admin = None
+            self.realm_passwd = None
+            return False
+
+    def validate_range(self, *keys, **options):
+        # If a range for this trusted domain already exists,
+        # '--base-id' or '--range-size' options should not be specified
         range_name = keys[-1].upper()+'_id_range'
 
         try:
             old_range = api.Command['idrange_show'](range_name)
-        except errors.NotFound, e:
+        except errors.NotFound:
             old_range = None
 
+        base_id = options.get('base_id')
+        range_size = options.get('range_size') != DEFAULT_RANGE_SIZE
+
+        if old_range and (base_id or range_size):
+            raise errors.ValidationError(
+                name=_('id range'),
+                error=_(
+                    'An id range already exists for this trust. '
+                    'You should either delete the old range, or '
+                    'exclude --base-id/--range-size options from the command.'
+                )
+            )
+
+        # If a range for this trusted domain already exists,
+        # domain SID must also match
+        self.trustinstance.populate_remote_domain(
+            keys[-1],
+            self.realm_server,
+            self.realm_admin,
+            self.realm_passwd
+        )
+        dom_sid = self.trustinstance.remote_domain.info['sid']
+
         if old_range:
-            old_dom_sid = old_range['result']['ipanttrusteddomainsid'][0];
+            old_dom_sid = old_range['result']['ipanttrusteddomainsid'][0]
 
-            if old_dom_sid == dom_sid:
-                return
+            if old_dom_sid != dom_sid:
+                raise errors.ValidationError(
+                    name=_('range exists'),
+                    error=_(
+                        'ID range with the same name but different domain SID '
+                        'already exists. The ID range for the new trusted '
+                        'domain must be created manually.'
+                    )
+                )
 
-            raise errors.ValidationError(name=_('range exists'),
-                    error=_('ID range with the same name but different ' \
-                            'domain SID already exists. The ID range for ' \
-                            'the new trusted domain must be created manually.'))
+        return old_range, range_name, dom_sid
 
+    def add_range(self, range_name, dom_sid, **options):
         if 'base_id' in options:
             base_id = options['base_id']
         else:
-            base_id = 200000 + (pysss_murmur.murmurhash3(dom_sid, len(dom_sid), 0xdeadbeef) % 10000) * 200000
+            base_id = DEFAULT_RANGE_SIZE + (pysss_murmur.murmurhash3(dom_sid, len(dom_sid), 0xdeadbeef) % 10000) * DEFAULT_RANGE_SIZE
 
         # Add new ID range
         api.Command['idrange_add'](range_name,
@@ -359,51 +455,21 @@ def add_range(self, *keys, **options):
                                    ipabaserid=0,
                                    ipanttrusteddomainsid=dom_sid)
 
-    def execute_ad(self, *keys, **options):
+    def execute_ad(self, full_join, *keys, **options):
         # Join domain using full credentials and with random trustdom
         # secret (will be generated by the join method)
-        trustinstance = None
-        if not _bindings_installed:
-            raise errors.NotFound(name=_('AD Trust setup'),
-                  reason=_('''Cannot perform join operation without Samba 4 support installed.
-                              Make sure you have installed server-trust-ad sub-package of IPA'''))
-
-        if 'realm_server' not in options:
-            realm_server = None
-        else:
-            realm_server = options['realm_server']
-
-        trustinstance = ipaserver.dcerpc.TrustDomainJoins(self.api)
-        if not trustinstance.configured:
-            raise errors.NotFound(name=_('AD Trust setup'),
-                  reason=_('''Cannot perform join operation without own domain configured.
-                              Make sure you have run ipa-adtrust-install on the IPA server first'''))
-
         try:
-            existing_trust = api.Command['trust_show'](keys[-1])
+            api.Command['trust_show'](keys[-1])
             summary = _('Re-established trust to domain "%(value)s"')
         except errors.NotFound:
             summary = self.msg_summary
+
         # 1. Full access to the remote domain. Use admin credentials and
         # generate random trustdom password to do work on both sides
-        if 'realm_admin' in options:
-            realm_admin = options['realm_admin']
-            names = realm_admin.split('@')
-            if len(names) > 1:
-                # realm admin name is in UPN format, user@realm, check that
-                # realm is the same as the one that we are attempting to trust
-                if keys[-1].lower() != names[-1].lower():
-                    raise errors.ValidationError(name=_('AD Trust setup'),
-                                 error=_('Trusted domain and administrator account use different realms'))
-                realm_admin = names[0]
-
-            if 'realm_passwd' not in options:
-                raise errors.ValidationError(name=_('AD Trust setup'), error=_('Realm administrator password should be specified'))
-            realm_passwd = options['realm_passwd']
-
+        if full_join:
             try:
-                result = trustinstance.join_ad_full_credentials(keys[-1], realm_server, realm_admin, realm_passwd)
-            except errors.NotFound, e:
+                result = self.trustinstance.join_ad_full_credentials()
+            except errors.NotFound:
                 error_message=_("Unable to resolve domain controller for '%s' domain. ") % (keys[-1])
                 instructions=[]
                 if dns_container_exists(self.obj.backend):
@@ -432,7 +498,10 @@ def execute_ad(self, *keys, **options):
                 raise errors.ValidationError(name=_('AD Trust setup'),
                                              error=_('Unable to verify write permissions to the AD'))
 
-            ret = dict(value=trustinstance.remote_domain.info['dns_domain'], verified=result['verified'])
+            ret = dict(
+                value=self.trustinstance.remote_domain.info['dns_domain'],
+                verified=result['verified']
+            )
             ret['summary'] = summary % ret
             return ret
 
@@ -441,8 +510,13 @@ def execute_ad(self, *keys, **options):
         # is provided. Do the work on our side and inform what to do on remote
         # side.
         if 'trust_secret' in options:
-            result = trustinstance.join_ad_ipa_half(keys[-1], realm_server, options['trust_secret'])
-            ret = dict(value=trustinstance.remote_domain.info['dns_domain'], verified=result['verified'])
+            result = self.trustinstance.join_ad_ipa_half(
+                options['trust_secret']
+            )
+            ret = dict(
+                value=self.trustinstance.remote_domain.info['dns_domain'],
+                verified=result['verified']
+            )
             ret['summary'] = summary % ret
             return ret
         raise errors.ValidationError(name=_('AD Trust setup'),
diff --git a/ipaserver/dcerpc.py b/ipaserver/dcerpc.py
index 5d052ea4ae400c600f91e0ae5004abb3b0cf8895..a084db22c3c89df6909954e24b17ca9c470addec 100644
--- a/ipaserver/dcerpc.py
+++ b/ipaserver/dcerpc.py
@@ -816,7 +816,7 @@ def __populate_local_domain(self):
         ld.retrieve(installutils.get_fqdn())
         self.local_domain = ld
 
-    def __populate_remote_domain(self, realm, realm_server=None, realm_admin=None, realm_passwd=None):
+    def populate_remote_domain(self, realm, realm_server=None, realm_admin=None, realm_passwd=None):
         def get_instance(self):
             # Fetch data from foreign domain using password only
             rd = TrustDomainInstance('')
@@ -856,11 +856,10 @@ def get_instance(self):
         # Otherwise, use anonymously obtained data
         self.remote_domain = rd
 
-    def join_ad_full_credentials(self, realm, realm_server, realm_admin, realm_passwd):
+    def join_ad_full_credentials(self):
         if not self.configured:
             return None
 
-        self.__populate_remote_domain(realm, realm_server, realm_admin, realm_passwd)
         if not self.remote_domain.read_only:
             trustdom_pass = samba.generate_random_password(128, 128)
             self.remote_domain.establish_trust(self.local_domain, trustdom_pass)
@@ -869,10 +868,9 @@ def join_ad_full_credentials(self, realm, realm_server, realm_admin, realm_passw
             return dict(local=self.local_domain, remote=self.remote_domain, verified=result)
         return None
 
-    def join_ad_ipa_half(self, realm, realm_server, trustdom_passwd):
+    def join_ad_ipa_half(self, trustdom_passwd):
         if not self.configured:
             return None
 
-        self.__populate_remote_domain(realm, realm_server, realm_passwd=None)
         self.local_domain.establish_trust(self.remote_domain, trustdom_passwd)
         return dict(local=self.local_domain, remote=self.remote_domain, verified=False)
-- 
1.8.1.4

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

Reply via email to