[Freeipa-devel] [PATCH 0021] Forbid overlapping rid ranges for the same id range

Fri Dec 14 12:59:42 UTC 2012

On Fri, 14 Dec 2012, Tomas Babej wrote:
>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.
RID is RID as it is abbreviation of Relative ID.
See http://msdn.microsoft.com/en-us/library/cc246018.aspx for details of
SID (and RID as it is part of SID).

-- 
/ Alexander Bokovoy