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.

From bccca7cdfc94aeac27168baa1a4c0d32d312c2b2 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 | 59 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 43 insertions(+), 16 deletions(-)

diff --git a/ipalib/plugins/trust.py b/ipalib/plugins/trust.py
index 3cb0ed98005ae5bd11b39f8ae01c9470d1bfc9c4..9b5676820347044c469e98e2998170528a9fb05d 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
         ),
     )
@@ -301,14 +303,26 @@ def execute(self, *keys, **options):
                 error=_('pysss_murmur is not available on the server ' \
                         'and no base-id is given.'))
 
-        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:
+        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')
+            )
+
+        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.'
+                )
+            )
+
+        result = self.execute_ad(*keys, **options)
         self.add_range(*keys, **options)
 
         trust_filter = "cn=%s" % result['value']
@@ -325,19 +339,32 @@ def execute(self, *keys, **options):
 
         return result
 
+    def get_old_range(self, range_name):
+        try:
+            return api.Command['idrange_show'](range_name)
+        except errors.NotFound:
+            return None
+
+    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'
+        range_exists = self.get_old_range(range_name) is not None
+        base_id = options.get('base_id')
+        range_size = options.get('range_size') != DEFAULT_RANGE_SIZE
+
+        return not(range_exists and (base_id or range_size))
+
     def add_range(self, *keys, **options):
         new_obj = api.Command['trust_show'](keys[-1])
-        dom_sid = new_obj['result']['ipanttrusteddomainsid'][0];
-
+        dom_sid = new_obj['result']['ipanttrusteddomainsid'][0]
         range_name = keys[-1].upper()+'_id_range'
-
-        try:
-            old_range = api.Command['idrange_show'](range_name)
-        except errors.NotFound, e:
-            old_range = None
+        old_range = self.get_old_range(range_name)
 
         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
@@ -350,7 +377,7 @@ def add_range(self, *keys, **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,
-- 
1.8.1.4

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

Reply via email to