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

Martin Kosek mkosek at redhat.com
Thu Sep 20 12:56:55 UTC 2012


On 09/20/2012 02:31 PM, Alexander Bokovoy wrote:
> On Thu, 20 Sep 2012, Martin Kosek wrote:
>> 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'),
> Yep. Done.
> 
> 

ACK. Pushed to master, ipa-3-0.

Martin




More information about the Freeipa-devel mailing list