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@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
--
Petr³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
