[Freeipa-devel] [PATCH] 0074 validate SID for trusted domain when adding/modifying ID range

[Martin Kosek]

On 09/20/2012 01:58 PM, Alexander Bokovoy wrote:
> On Thu, 20 Sep 2012, Petr Viktorin wrote:
>> On 09/20/2012 12:12 PM, Martin Kosek wrote:
>>> On 09/20/2012 11:42 AM, Alexander Bokovoy wrote:
>>>> Hi,
>>>>
>>>> On Thu, 20 Sep 2012, Martin Kosek wrote:
>>>>> On 09/19/2012 06:19 PM, Alexander Bokovoy wrote:
>>>>>> Hi,
>>>>>>
>>>>>> This patch adds validation of SID for trusted domain when adding or
>>>>>> modifying ID range for the domain. We only allow creating ranges for
>>>>>> trusted domains when the trust is already established -- the default
>>>>>> range is created automatically right after the trust is added.
>>>>>>
>>>>>> https://fedorahosted.org/freeipa/ticket/3087
>>>>>>
>>>>>
>>>>> Basic functionality looks OK, but I saw few issues with exception formatting:
>>>>>
>>>>> diff --git a/ipalib/plugins/idrange.py b/ipalib/plugins/idrange.py
>>>>> index
>>>>> efa906428aa58c670bc4af63b10c88123dda5b65..4750c1d6716bd69045d53f32ae1836f44e70b03b
>>>>>
>>>>>
>>>>> 100644
>>>>> --- a/ipalib/plugins/idrange.py
>>>>> +++ b/ipalib/plugins/idrange.py
>>>>> @@ -26,6 +26,12 @@ from ipapython import ipautil
>>>>> from ipalib import util
>>>>> from ipapython.dn import DN
>>>>>
>>>>> +if api.env.in_server and api.env.context in ['lite', 'server']:
>>>>> +    try:
>>>>> +        import ipaserver.dcerpc
>>>>> +        _dcerpc_bindings_installed = True
>>>>> +    except Exception, e:
>>>>> +        _dcerpc_bindings_installed = False
>>>>>
>>>>>
>>>>> Variable "e" is not used, so it can be removed.
>>>> Then Exception, e should be omitted completely :)
>>>
>>> As per PEP8, "except Exception:" is preffered over bare "except:" as otherwise
>>> it would also catch SystemExit or KeyboardInterrupt.
>>
>> You should use the most specific exception you want to handle. In this case
>> it's probably ImportError.
> New patch is attached.
> 

The patch looks OK, I would just also like to have the rest of the name=_('ID
Range setup') messages fixed so that we don't print confusing errors:

$ git grep "ID Range setup" ipalib/
ipalib/plugins/idrange.py:                raise
errors.ValidationError(name=_('ID Range setup'),
ipalib/plugins/idrange.py:                raise
errors.ValidationError(name=_('ID Range setup'),
ipalib/plugins/idrange.py:                raise
errors.ValidationError(name=_('ID Range setup'),

Martin