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

[Petr Viktorin]

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.

>>
>>
>>>
>>> __doc__ = _("""
>>> ID ranges
>>> @@ -137,6 +143,21 @@ user. RIDs are unique in a domain, 32bit values and are
>>> used for users and
>>> groups.
>>> """)
>>>
>>> +def validate_trusted_domain_sid(self, sid):
>>>
>>> "self" is not needed in the list of attributes, this is not a class method.
>> I'm using self.api inside. While api is singleton and accessible
>> globally, I'd prefer passing it explicitly. So may be I'll change that
>> to 'api'.
>
> Looks like to hack to me. I would either define this function as a method of
> idrange class (as I did with handle_iparangetype) and then use self.api or use
> "api" directly as a function parameter and not make assumptions what "self" may be.
>
>>
>>> +    if not _dcerpc_bindings_installed:
>>> +        raise errors.NotFound(name=_('ID Range setup'),
>>> +              reason=_('''Cannot perform SID validation without Samba 4
>>> support installed.
>>> +                          Make sure you have installed server-trust-ad
>>> sub-package of IPA on the server'''))
>>>
>>> Improperly formatted exception:
>>> 1) NotFound error does not use "name" param, maybe you wanted to use
>>> ValidationError?
>>> 2) The text will be improperly formatted - since you used '''<text>''', the
>>> indentation will be in text:
>>>
>>> ipa: ERROR: Cannot perform SID validation without Samba 4 support installed.
>>>                           Make sure you have installed server-trust-ad
>>> sub-package of IPA on the server
>> Will fix it.
>>
>>>
>>> Also, I know this was discussed before, but using gettext in a name attribute
>>> of ValidationError will cause improperly formatted exception:
>>>
>>> # ipa idrange-add foo --base-id=1000 --range-size=100 --dom-sid=foo
>>> ipa: ERROR: invalid Gettext('ID Range setup', domain='ipa', localedir=None):
>>> Options dom_sid and rid_base must be used together
>>>
>>> The problem is, that "name" param is printer as %r, thus you would need to
>>> coerce it to unicode to make it better.
>> I'd rather fix our printing code and Gettext() usage to properly give
>> out the original string rather than swallow limitations we put on
>> ourselves. Wider use will be, more translations will be needed and names
>> will need to be translated as well.
>
> ValidationError has this format: _('invalid %(name)r: %(error)s'). AFAIU, name
> parameter was intended to contain only a name of attribute or option where the
> validation failed, i.e. a non-translable string. So this was the reason why we
> print it as "%r". This pattern is followed in the rest of the plugins.
>
> In your case, I think using name="dom_sid" would be the preferred use of
> ValidationError.
>
> Martin
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
>


-- 
Petr³