On 12/13/2012 02:48 PM, Martin Kosek wrote:
On 12/13/2012 11:52 AM, Tomas Babej wrote:
On 12/12/2012 04:32 PM, Martin Kosek wrote:
On 10/26/2012 03:43 PM, Tomas Babej wrote:
Hi,

creating an id range with overlapping primary and secondary
rid range using idrange-add or idrange-mod command now
raises ValidationError. Unit tests have been added to
test_range_plugin.py.

https://fedorahosted.org/freeipa/ticket/3171

Tomas

1) Add command can cause crash:

# ipa idrange-add range9 --base-id=1000 --rid-base=1000 --secondary-rid-base=
--range-size 1000
ipa: ERROR: an internal error has occurred

2) I don't like this construct very much:

updated_values = dict(zip(rid_range_attributes,[None]*3))

This would look better, IMO:
updated_values = dict((attr, None) for attr in rid_range_attributes)

Why do you need this dict pre-created anyway? You overwrite all keys here:

+            for attr in rid_range_attributes:
+                if attr in entry_attrs:
+                    updated_values[attr] = entry_attrs[attr]
+                else:
+                    updated_values[attr] = int(old_attrs[attr][0])


3) [nitpick] We don't end ValidationError with '.':

+                   raise errors.ValidationError(name='ID Range setup',
+                       error=_("Primary rid range and secondary rid range"\
+                               " cannot overlap."))

There is also a duplication of the same error message...

4) The -mod operation will also crash:

# ipa idrange-add range9 --base-id=1000 --rid-base=1000
--secondary-rid-base=2000 --range-size 1000
# ipa idrange-mod range9 --secondary-rid-base=
ipa: ERROR: an internal error has occurred

Martin
New patch version as well as diff between
patch versions (for your convenience) attached.

Tomas
1) You introduced mixed spaces and tabs - Python gods do not like that!
Oops, I'd rather send a fixed patch sooner than they bring down their wrath on me. :)
2) This is a nitpick, but there are too many redundant parens and brackets in
this statement:

+       if(any([attr is None for attr in [rid_base,secondary_rid_base, size]])):
+            return False

This would look nicer and would not create unnecessary list:

+       if any(attr is None for attr in (rid_base, secondary_rid_base, size)):
+            return False
Brackets reduced.
3) Another construct I did not like very much:

+       is_set = lambda x : (x in entry_attrs) and not (x is None)

a) "x is not None" reads better than "not (x is None)"
b) I would rather replace all is_set lambdas with use of "if
entry_attrs.get('attribute')" which is also used in other places in ipalib
I don't think this approach would be beneficial. If any of rid_base,
secondary_rid_base, base_id, e.g. would be set to 0, the expression like

/entry_attrs.get('base_id')

/would be evaluated as False both in case that there is no key 'base_id'
in the dictionary and in the case that the value associated with the key
is 0. To avoid these problems, we would have to complicate conditions
used in if-s, and therefore make the readability worse.
/
/
4) I see a suspicions check

+            if (is_set('ipasecondarybaserid') != is_set('ipabaserid')):

I though that ipasecondarybaserid is optional. With your change it is not:

# ipa idrange-add range9 --base-id=1000 --rid-base=1000 --range-size 1000
ipa: ERROR: invalid 'ID Range setup': Options secondary_rid_base and rid_base
must be used together

It is also quite ugly condition, I would do something like:

if entry_attrs.get('ipasecondarybaserid') and not entry_attrs.get('ipabaserid'):
... raise error
It's not a bug. It's a feature :)

Secondary base RID indeed is mandatory when primary RID base has been defined. Its purpose is to prevent collisions when user and group share the same POSIX ID number.

From the documentation of idrange.py:

/To create an ID range for the local domain it is not necessary to specify a// //domain SID. But since it is possible that a user and a group can have the same//
//value as Posix ID a second RID interval is needed to handle conflicts.

/
5) I would not create a list when it is not necessary, a tuple is more
efficient I think:

+            rid_range_attributes =
['ipabaserid','ipasecondarybaserid','ipaidrangesize']
Fixed.
6) If we want to check user does not create secondary RID range without a
primary RID range, we should also do it in -mod operation:

# ipa idrange-mod range9 --rid-base=
--------------------------
Modified ID range "range9"
--------------------------
   Range name: range9
   First Posix ID of the range: 1000
   Number of IDs in the range: 1000
   First RID of the secondary RID range: 2000
   Range type: local domain range
This is fixed as part of my patch 0024 as it falls under the scope of
http://fedorahosted.org/freeipa/ticket/3170

I will send a rebased version later today.
7) I am sorry I did not come with this in my previous review, but I have one
more nitpick for the error message:
+                       error=_("Primary rid range and secondary rid range"\
+                               " cannot overlap"))

I would do s/rid/RID/ as we also refer it as RID in our help...

Martin
Fixed. However, lower-case rid is used in ipa_range_check.c 389 plugin.
We might want to consider filing a naming convention ticket then.

Tomas

>From 7e90b3441eaeea45ebb759f897367e4b05848baa Mon Sep 17 00:00:00 2001
From: Tomas Babej <tba...@redhat.com>
Date: Fri, 26 Oct 2012 07:43:05 -0400
Subject: [PATCH] Forbid overlapping rid ranges for the same id range

Creating an id range with overlapping primary and secondary
rid range using idrange-add or idrange-mod command now
raises ValidationError. Unit tests have been added to
test_range_plugin.py.

https://fedorahosted.org/freeipa/ticket/3171
---
 ipalib/plugins/idrange.py              | 79 +++++++++++++++++++++++++++++-----
 tests/test_xmlrpc/test_range_plugin.py | 42 ++++++++++++++----
 2 files changed, 103 insertions(+), 18 deletions(-)

diff --git a/ipalib/plugins/idrange.py b/ipalib/plugins/idrange.py
index 75470811750f745c9b89268074830439bfc06bce..5c51192513c661163653dee864391df40b74e31e 100644
--- a/ipalib/plugins/idrange.py
+++ b/ipalib/plugins/idrange.py
@@ -277,6 +277,25 @@ class idrange(LDAPObject):
             raise errors.ValidationError(name='domain SID',
                   error=_('SID is not recognized as a valid SID for a trusted domain'))
 
+    # checks that primary and secondary rid ranges do not overlap
+    def are_rid_ranges_overlapping(self, rid_base, secondary_rid_base, size):
+
+        # if any of these is None, the check does not apply
+        if any(attr is None for attr in (rid_base, secondary_rid_base, size)):
+            return False
+
+        # sort the bases
+        if rid_base > secondary_rid_base:
+            rid_base, secondary_rid_base = secondary_rid_base, rid_base
+
+        # rid_base is now <= secondary_rid_base,
+        # so the following check is sufficient
+        if rid_base + size <= secondary_rid_base:
+            return False
+        else:
+            return True
+
+
 class idrange_add(LDAPCreate):
     __doc__ = _("""
     Add new ID range.
@@ -315,27 +334,39 @@ class idrange_add(LDAPCreate):
     def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options):
         assert isinstance(dn, DN)
 
-        if 'ipanttrusteddomainsid' in options:
-            if 'ipasecondarybaserid' in options:
+        is_set = lambda x: (x in entry_attrs) and (x is not None)
+
+        if is_set('ipanttrusteddomainsid'):
+            if is_set('ipasecondarybaserid'):
                 raise errors.ValidationError(name='ID Range setup',
-                    error=_('Options dom_sid and secondary_rid_base cannot ' \
+                    error=_('Options dom_sid and secondary_rid_base cannot '
                             'be used together'))
 
-            if 'ipabaserid' not in options:
+            if not is_set('ipabaserid'):
                 raise errors.ValidationError(name='ID Range setup',
-                    error=_('Options dom_sid and rid_base must ' \
+                    error=_('Options dom_sid and rid_base must '
                             'be used together'))
 
             # Validate SID as the one of trusted domains
             self.obj.validate_trusted_domain_sid(options['ipanttrusteddomainsid'])
             # Finally, add trusted AD domain range object class
             entry_attrs['objectclass'].append('ipatrustedaddomainrange')
+
         else:
-            if (('ipasecondarybaserid' in options) != ('ipabaserid' in options)):
+            if (is_set('ipasecondarybaserid') != is_set('ipabaserid')):
                 raise errors.ValidationError(name='ID Range setup',
-                    error=_('Options secondary_rid_base and rid_base must ' \
+                    error=_('Options secondary_rid_base and rid_base must '
                             'be used together'))
 
+            if (is_set('ipabaserid') and is_set('ipasecondarybaserid')):
+                if self.obj.are_rid_ranges_overlapping(
+                    entry_attrs['ipabaserid'],
+                    entry_attrs['ipasecondarybaserid'],
+                    entry_attrs['ipaidrangesize']):
+                       raise errors.ValidationError(name='ID Range setup',
+                           error=_("Primary RID range and secondary RID range"
+                               " cannot overlap"))
+
             entry_attrs['objectclass'].append('ipadomainidrange')
 
         return dn
@@ -377,7 +408,7 @@ class idrange_find(LDAPSearch):
         return (filters, base_dn, ldap.SCOPE_ONELEVEL)
 
     def post_callback(self, ldap, entries, truncated, *args, **options):
-        for dn,entry in entries:
+        for dn, entry in entries:
             self.obj.handle_iparangetype(entry, options)
         return truncated
 
@@ -403,15 +434,43 @@ class idrange_mod(LDAPUpdate):
         assert isinstance(dn, DN)
         attrs_list.append('objectclass')
 
+        is_set = lambda x: (x in entry_attrs) and (x is not None)
+
         try:
-            (old_dn, old_attrs) = ldap.get_entry(dn, ['ipabaseid', 'ipaidrangesize'])
+            (old_dn, old_attrs) = ldap.get_entry(dn,
+                                                ['ipabaseid',
+                                                'ipaidrangesize',
+                                                'ipabaserid',
+                                                'ipasecondarybaserid'])
         except errors.NotFound:
             self.obj.handle_not_found(*keys)
 
-        if 'ipanttrusteddomainsid' in options:
+        if is_set('ipanttrusteddomainsid'):
             # Validate SID as the one of trusted domains
             self.obj.validate_trusted_domain_sid(options['ipanttrusteddomainsid'])
 
+        # ensure that primary and secondary rid ranges do not overlap
+        if all((base in entry_attrs) or (base in old_attrs)
+                for base in ('ipabaserid', 'ipasecondarybaserid')):
+
+            # make sure we are working with updated attributes
+            rid_range_attributes = ('ipabaserid', 'ipasecondarybaserid', 'ipaidrangesize')
+            updated_values = dict()
+
+            for attr in rid_range_attributes:
+                if is_set(attr):
+                    updated_values[attr] = entry_attrs[attr]
+                else:
+                    updated_values[attr] = int(old_attrs[attr][0])
+
+            if self.obj.are_rid_ranges_overlapping(
+                updated_values['ipabaserid'],
+                updated_values['ipasecondarybaserid'],
+                updated_values['ipaidrangesize']):
+                    raise errors.ValidationError(name='ID Range setup',
+                            error=_("Primary RID range and secondary RID range"
+                                 " cannot overlap"))
+
         old_base_id = int(old_attrs.get('ipabaseid', [0])[0])
         old_range_size = int(old_attrs.get('ipaidrangesize', [0])[0])
         new_base_id = entry_attrs.get('ipabaseid')
diff --git a/tests/test_xmlrpc/test_range_plugin.py b/tests/test_xmlrpc/test_range_plugin.py
index 56c6db528b6c9cc4e1ac8cf22232fdfa46953389..2e00609a7829a836f924ac04ff34d040bcb2a544 100644
--- a/tests/test_xmlrpc/test_range_plugin.py
+++ b/tests/test_xmlrpc/test_range_plugin.py
@@ -30,6 +30,8 @@ from ipapython.dn import *
 testrange1 = u'testrange1'
 testrange1_base_id = 900000
 testrange1_size = 99999
+testrange1_base_rid = 10000
+testrange1_secondary_base_rid=200000
 
 testrange2 = u'testrange2'
 testrange2_base_id = 100
@@ -61,6 +63,11 @@ testrange6_size = 50
 testrange6_base_rid = 500
 testrange6_secondary_base_rid=1300
 
+testrange7 = u'testrange7'
+testrange7_base_id = 600
+testrange7_size = 50
+testrange7_base_rid = 600
+testrange7_secondary_base_rid=649
 
 user1=u'tuser1'
 user1_uid = 900000
@@ -69,7 +76,7 @@ group1_gid = 900100
 
 class test_range(Declarative):
     cleanup_commands = [
-        ('idrange_del', [testrange1,testrange2,testrange3,testrange4,testrange5,testrange6], {}),
+        ('idrange_del', [testrange1,testrange2,testrange3,testrange4,testrange5,testrange6,testrange7], {'continue': True}),
         ('user_del', [user1], {}),
         ('group_del', [group1], {}),
     ]
@@ -79,7 +86,7 @@ class test_range(Declarative):
             desc='Create ID range %r' % (testrange1),
             command=('idrange_add', [testrange1],
                       dict(ipabaseid=testrange1_base_id, ipaidrangesize=testrange1_size,
-                           ipabaserid=10000, ipasecondarybaserid=20000)),
+                           ipabaserid=testrange1_base_rid, ipasecondarybaserid=testrange1_secondary_base_rid)),
             expected=dict(
                 result=dict(
                     dn=DN(('cn',testrange1),('cn','ranges'),('cn','etc'),
@@ -87,8 +94,8 @@ class test_range(Declarative):
                     cn=[testrange1],
                     objectclass=[u'ipaIDrange', u'ipadomainidrange'],
                     ipabaseid=[unicode(testrange1_base_id)],
-                    ipabaserid=[u'10000'],
-                    ipasecondarybaserid=[u'20000'],
+                    ipabaserid=[unicode(testrange1_base_rid)],
+                    ipasecondarybaserid=[unicode(testrange1_secondary_base_rid)],
                     ipaidrangesize=[unicode(testrange1_size)],
                     iparangetype=[u'local domain range'],
                 ),
@@ -106,8 +113,8 @@ class test_range(Declarative):
                           api.env.basedn),
                     cn=[testrange1],
                     ipabaseid=[unicode(testrange1_base_id)],
-                    ipabaserid=[u'10000'],
-                    ipasecondarybaserid=[u'20000'],
+                    ipabaserid=[unicode(testrange1_base_rid)],
+                    ipasecondarybaserid=[unicode(testrange1_secondary_base_rid)],
                     ipaidrangesize=[unicode(testrange1_size)],
                     iparangetype=[u'local domain range'],
                 ),
@@ -210,8 +217,8 @@ class test_range(Declarative):
                 result=dict(
                     cn=[testrange1],
                     ipabaseid=[unicode(testrange1_base_id)],
-                    ipabaserid=[u'10000'],
-                    ipasecondarybaserid=[u'20000'],
+                    ipabaserid=[unicode(testrange1_base_rid)],
+                    ipasecondarybaserid=[unicode(testrange1_secondary_base_rid)],
                     ipaidrangesize=[u'90000'],
                     iparangetype=[u'local domain range'],
                 ),
@@ -287,6 +294,14 @@ class test_range(Declarative):
         ),
 
         dict(
+            desc='Try to modify ID range %r so that its rid ranges are overlapping themselves' % (testrange2),
+            command=('idrange_mod', [testrange2],
+                      dict(ipabaserid=(testrange2_base_rid*10))),
+            expected=errors.ValidationError(
+                name='ID Range setup', error='Primary RID range and secondary RID range cannot overlap'),
+        ),
+
+        dict(
             desc='Try to create ID range %r with overlapping rid range' % (testrange3),
             command=('idrange_add', [testrange3],
                       dict(ipabaseid=testrange3_base_id,
@@ -331,6 +346,17 @@ class test_range(Declarative):
         ),
 
         dict(
+            desc='Try to create ID range %r with rid ranges overlapping themselves' % (testrange7),
+            command=('idrange_add', [testrange7],
+                      dict(ipabaseid=testrange7_base_id,
+                          ipaidrangesize=testrange7_size,
+                          ipabaserid=testrange7_base_rid,
+                          ipasecondarybaserid=testrange7_secondary_base_rid)),
+            expected=errors.ValidationError(
+                name='ID Range setup', error='Primary RID range and secondary RID range cannot overlap'),
+        ),
+
+        dict(
             desc='Delete ID range %r' % testrange2,
             command=('idrange_del', [testrange2], {}),
             expected=dict(
-- 
1.8.0.1

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

Reply via email to