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

[Martin Kosek]

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.

> 
> 
>>
>> __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