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

[Alexander Bokovoy]

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 :)


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

>+    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.
>
>+    domain_validator = ipaserver.dcerpc.DomainValidator(self.api)
>+    if not domain_validator.is_configured():
>+        raise errors.NotFound(name=_('ID Range setup'),
>+              reason=_('''Cross-realm trusts are not configured..
>+                          Make sure you have run ipa-adtrust-install on the
>IPA server first'''))
>
>Same issues:
>
># ipa idrange-add foo --base-id=1000 --range-size=100 --dom-sid=foo --rid-base=1000
>ipa: ERROR: Cross-realm trusts are not configured..
>                          Make sure you have run ipa-adtrust-install on the IPA
>server first
>
>
>+    if not domain_validator.is_trusted_sid_valid(sid):
>+        raise errors.ValidationError(name=_('ID Range setup'),
>+              error=_('SID is not recognized as a valid SID from a trusted
>domain'))
>+
>+
>
>Same issues:
>
>ipa: ERROR: invalid Gettext('ID Range setup', domain='ipa', localedir=None):
>SID is not recognized as a valid SID from a trusted domain
I'll fix formatting.

Thanks!
-- 
/ Alexander Bokovoy