On 09/20/2012 11:42 AM, Alexander Bokovoy wrote:
> On Thu, 20 Sep 2012, Martin Kosek wrote:
>> On 09/19/2012 06:19 PM, Alexander Bokovoy wrote:
>>> 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.
>> Basic functionality looks OK, but I saw few issues with exception formatting:
>> diff --git a/ipalib/plugins/idrange.py b/ipalib/plugins/idrange.py
>> --- 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
>> +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
>> + 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
>> 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
Freeipa-devel mailing list