On 10/03/2013 03:10 PM, Alexander Bokovoy wrote: > On Wed, 02 Oct 2013, Sumit Bose wrote: >>> Please note that I did not test with more than 1 subdomain, since I >>> do not have more ADs available. >>> >> >> I have done some testing as well and the patches are working as expected >> except the trustdomain-disable issue Tomas mentioned. But I think it >> would be sufficient to add a comment to the release notes and fix this >> with the next release to not delay this release anymore. >> >> The patches are also working for trusts which were added with older >> releases. So ACK from my side for the functional part. > New patchset is attached. I've fixed all outstanding issues and > implemented proper SID filtering for subdomains. In addition, I've > added MS-PAC cache eviction when we change blacklists from IPA side > and forced removal of the domain from SID blacklist if the domain is > being removed by trustdomain-del. >
1) Minor issue in 0118: + if keys[0].lower() == keys[1].lower(): + raise errors.ValidationError(name='trustdomain_enable', + error=_("Root domain of the trust is always enabled for the existing trust")) The error message looks weird (double trustdomain_enable) # ipa trustdomain-enable realm domain ipa: ERROR: invalid 'trustdomain_enable': Root domain of the trust is always enabled for the existing trust I would rather do something like + raise errors.ValidationError(name='domain', 2) trustconfig-enable and trustconfig-disable should use standard output like other enable/disable methods. See user-enable/user-disable for example. Current situation puts all the authoritative information to summary which: a) Does not look nice in terminal # ipa trustdomain-disable very.long.long.long.realm very.long.long.long.domain ---------------------------------------------------------------------------------------------------------------------------- Domain very.long.long.long.domain of trust very.long.long.long.realm is not allowed to access IPA resources ---------------------------------------------------------------------------------------------------------------------------- b) How am I supposed to parse an information about the result if all I get is a text in summary? Using standard errors and output values will allow easier consumption of the API later (like in Web UI). I am attaching a patch (0001) how to make it consistent with other enable/disable commands. Example: # ipa trustdomain-disable realm domain ipa: ERROR: This entry is already disabled # ipa trustdomain-enable realm domain ----------------------------- Enabled trust domain "domain" ----------------------------- 3) Let's use standard primary key for the trustdomain object. This will let us overcome some hacks and also let us use handle_not_found method - patch attached (0002). 0002 also changes some ValidationError errors to standard errors, just for being consistent with the rest of the API. Note that in order to make primary_key=True, I had to enhance trustdomain_del command to manage multiple domains. I think these API fixes are a must, it would be very hard to amend the API later. If these patches are squashed to your 0118, it would be ACK from me to the Python side. I will let C parts and a functional test to Sumit's mighty hands. Martin
From df0ffda6b74f48557c4bdfd1b729e9ad801901b0 Mon Sep 17 00:00:00 2001 From: Martin Kosek <mko...@redhat.com> Date: Thu, 3 Oct 2013 16:30:21 +0200 Subject: [PATCH] Make trustdomain-enable and trustdomain-disable output consistent --- ipalib/plugins/trust.py | 44 ++++++++++++++++---------------------------- 1 file changed, 16 insertions(+), 28 deletions(-) diff --git a/ipalib/plugins/trust.py b/ipalib/plugins/trust.py index d909b3f1be4bb19c667ee34cbda1316aaa905970..26182f38d5a6593405762ab4e76859aa5068de9d 100644 --- a/ipalib/plugins/trust.py +++ b/ipalib/plugins/trust.py @@ -1272,17 +1272,15 @@ def execute(self, *keys, **options): class trustdomain_enable(LDAPQuery): __doc__ = _('Allow use of IPA resources by the domain of the trust') - has_output = ( - output.summary, - ) - + has_output = output.standard_value takes_args = trustdomain.trustdomain_args + msg_summary = _('Enabled trust domain "%(value)s"') def execute(self, *keys, **options): ldap = self.api.Backend.ldap2 if keys[0].lower() == keys[1].lower(): - raise errors.ValidationError(name='trustdomain_enable', + raise errors.ValidationError(name='domain', error=_("Root domain of the trust is always enabled for the existing trust")) try: trust_dn = self.obj.get_dn(keys[0], trust_type=u'ad') @@ -1290,46 +1288,40 @@ def execute(self, *keys, **options): except errors.NotFound: raise errors.ValidationError(name=_('AD trust'), error=_('Valid trust name is required')) - result = dict() - result['trust'] = unicode(keys[0]) - result['domain'] = unicode(keys[1]) dn = self.obj.get_dn(keys[0], keys[1], trust_type=u'ad') try: entry = ldap.get_entry(dn) sid = entry['ipanttrusteddomainsid'][0] if sid in trust_entry['ipantsidblacklistincoming']: trust_entry['ipantsidblacklistincoming'].remove(sid) - result['action'] = _('is allowed') ldap.update_entry(trust_entry) # Force MS-PAC cache re-initialization on KDC side domval = ipaserver.dcerpc.DomainValidator(api) (ccache_name, principal) = domval.kinit_as_http(keys[0]) else: - result['action'] = _('is already allowed') - result['summary'] = _('Domain %(domain)s of trust %(trust)s %(action)s to access IPA resources') + raise errors.AlreadyActive() except errors.NotFound: raise errors.ValidationError(name=_('AD trust'), error=_('Valid trust domain name is required')) - except errors.EmptyModlist: - result['summary'] = _('No changes were done') - return dict(summary=result['summary'] % result) + return dict( + result=True, + value=keys[1], + ) api.register(trustdomain_enable) class trustdomain_disable(LDAPQuery): __doc__ = _('Disable use of IPA resources by the domain of the trust') - has_output = ( - output.summary, - ) - + has_output = output.standard_value takes_args = trustdomain.trustdomain_args + msg_summary = _('Disabled trust domain "%(value)s"') def execute(self, *keys, **options): ldap = self.api.Backend.ldap2 if keys[0].lower() == keys[1].lower(): - raise errors.ValidationError(name='trustdomain_disable', + raise errors.ValidationError(name='domain', error=_("cannot disable root domain of the trust, use trust-del to delete the trust itself")) try: trust_dn = self.obj.get_dn(keys[0], trust_type=u'ad') @@ -1337,28 +1329,24 @@ def execute(self, *keys, **options): except errors.NotFound: raise errors.ValidationError(name=_('AD trust'), error=_('Valid trust name is required')) - result = dict() - result['trust'] = unicode(keys[0]) - result['domain'] = unicode(keys[1]) dn = self.obj.get_dn(keys[0], keys[1], trust_type=u'ad') try: entry = ldap.get_entry(dn) sid = entry['ipanttrusteddomainsid'][0] if not (sid in trust_entry['ipantsidblacklistincoming']): trust_entry['ipantsidblacklistincoming'].append(sid) - result['action'] = _('is not allowed') ldap.update_entry(trust_entry) # Force MS-PAC cache re-initialization on KDC side domval = ipaserver.dcerpc.DomainValidator(api) (ccache_name, principal) = domval.kinit_as_http(keys[0]) else: - result['action'] = _('is already not allowed') - result['summary'] = _('Domain %(domain)s of trust %(trust)s %(action)s to access IPA resources') + raise errors.AlreadyInactive() except errors.NotFound: raise errors.ValidationError(name=_('AD trust'), error=_('Valid trust domain name is required')) - except errors.EmptyModlist: - result['summary'] = _('No changes were done') - return dict(summary=result['summary'] % result) + return dict( + result=True, + value=keys[1], + ) api.register(trustdomain_disable) -- 1.8.3.1
From c126b00ce6679186cafa93d5245539423afc6808 Mon Sep 17 00:00:00 2001 From: Martin Kosek <mko...@redhat.com> Date: Thu, 3 Oct 2013 17:02:38 +0200 Subject: [PATCH] Use primary_key instead of custom args This patch also uses standard exceptions instead of NotFound errors. --- ipalib/plugins/trust.py | 52 ++++++++++++++++++++----------------------------- 1 file changed, 21 insertions(+), 31 deletions(-) diff --git a/ipalib/plugins/trust.py b/ipalib/plugins/trust.py index 26182f38d5a6593405762ab4e76859aa5068de9d..8aa6ed59248b9fab8032f114673d2f2a59ac201e 100644 --- a/ipalib/plugins/trust.py +++ b/ipalib/plugins/trust.py @@ -1108,17 +1108,12 @@ class trustdomain(LDAPObject): label = _('Trusted domains') label_singular = _('Trusted domain') - # We do not add this argument implicitly via takes_args in the object. - # Rather, it is added explicitly in the commands that require second arg - # because first argument will be inherited from the 'trust' parent object - trustdomain_args = ( + takes_params = ( Str('cn', label=_('Domain name'), cli_name='domain', + primary_key=True ), - ) - - takes_params = ( Str('ipantflatname?', cli_name='flat_name', label=_('Domain NetBIOS name'), @@ -1150,7 +1145,6 @@ def get_dn(self, *keys, **kwargs): class trustdomain_find(LDAPSearch): __doc__ = _('Search domains of the trust') - has_output_params = LDAPSearch.has_output_params + trustdomain.trustdomain_args def pre_callback(self, ldap, filters, attrs_list, base_dn, scope, *args, **options): return (filters, base_dn, ldap.SCOPE_SUBTREE) api.register(trustdomain_find) @@ -1159,7 +1153,6 @@ class trustdomain_mod(LDAPUpdate): __doc__ = _('Modify trustdomain of the trust') NO_CLI = True - takes_args = trustdomain.trustdomain_args takes_options = LDAPUpdate.takes_options + (_trust_type_option,) api.register(trustdomain_mod) @@ -1167,7 +1160,6 @@ class trustdomain_add(LDAPCreate): __doc__ = _('Allow access from the trusted domain') NO_CLI = True - takes_args = trustdomain.trustdomain_args takes_options = LDAPCreate.takes_options + (_trust_type_option,) def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options): if 'ipanttrustpartner' in options: @@ -1180,17 +1172,20 @@ class trustdomain_del(LDAPDelete): msg_summary = _('Removed information about the trusted domain "%(value)s"') - takes_args = trustdomain.trustdomain_args - def execute(self, *keys, **options): # Note that pre-/post- callback handling for LDAPDelete is causing pre_callback # to always receive empty keys. We need to catch the case when root domain is being deleted - if keys[0].lower() == keys[1].lower(): - raise errors.ValidationError(name='trustdomain_del', - error=_("cannot delete root domain of the trust, use trust-del to delete the trust itself")) - res = api.Command.trustdomain_enable(*keys) + + for domain in keys[1]: + if keys[0].lower() == domain: + raise errors.ValidationError(name='domain', + error=_("cannot delete root domain of the trust, use trust-del to delete the trust itself")) + try: + res = api.Command.trustdomain_enable(keys[0], domain) + except errors.AlreadyActive: + pass result = super(trustdomain_del, self).execute(*keys, **options) - result['value'] = keys[1] + result['value'] = u','.join(keys[1]) return result @@ -1227,10 +1222,7 @@ def fetch_domains_from_trust(self, trustinstance, trust_entry, **options): class trust_fetch_domains(LDAPRetrieve): __doc__ = _('Refresh list of the domains associated with the trust') - has_output = ( - output.ListOfEntries('result'), - output.summary - ) + has_output = output.standard_list_of_entries def execute(self, *keys, **options): if not _bindings_installed: @@ -1255,10 +1247,7 @@ def execute(self, *keys, **options): ) ) domains = fetch_domains_from_trust(self, trustinstance, trust) - result = dict(result=[]) - if not domains: - result['summary'] = unicode(_('No trust domains were detected during refresh')) - return result + result = dict() if len(domains) > 0: result['summary'] = unicode(_('List of trust domains successfully refreshed')) @@ -1266,14 +1255,16 @@ def execute(self, *keys, **options): result['summary'] = unicode(_('No new trust domains were found')) result['result'] = domains + result['count'] = len(domains) + result['truncated'] = False return result + api.register(trust_fetch_domains) class trustdomain_enable(LDAPQuery): __doc__ = _('Allow use of IPA resources by the domain of the trust') has_output = output.standard_value - takes_args = trustdomain.trustdomain_args msg_summary = _('Enabled trust domain "%(value)s"') def execute(self, *keys, **options): @@ -1286,7 +1277,7 @@ def execute(self, *keys, **options): trust_dn = self.obj.get_dn(keys[0], trust_type=u'ad') trust_entry = ldap.get_entry(trust_dn) except errors.NotFound: - raise errors.ValidationError(name=_('AD trust'), error=_('Valid trust name is required')) + raise self.api.Object[self.obj.parent_object].handle_not_found(keys[0]) dn = self.obj.get_dn(keys[0], keys[1], trust_type=u'ad') try: @@ -1301,7 +1292,7 @@ def execute(self, *keys, **options): else: raise errors.AlreadyActive() except errors.NotFound: - raise errors.ValidationError(name=_('AD trust'), error=_('Valid trust domain name is required')) + self.obj.handle_not_found(*keys) return dict( result=True, @@ -1314,7 +1305,6 @@ class trustdomain_disable(LDAPQuery): __doc__ = _('Disable use of IPA resources by the domain of the trust') has_output = output.standard_value - takes_args = trustdomain.trustdomain_args msg_summary = _('Disabled trust domain "%(value)s"') def execute(self, *keys, **options): @@ -1327,7 +1317,7 @@ def execute(self, *keys, **options): trust_dn = self.obj.get_dn(keys[0], trust_type=u'ad') trust_entry = ldap.get_entry(trust_dn) except errors.NotFound: - raise errors.ValidationError(name=_('AD trust'), error=_('Valid trust name is required')) + self.api.Object[self.obj.parent_object].handle_not_found(keys[0]) dn = self.obj.get_dn(keys[0], keys[1], trust_type=u'ad') try: @@ -1342,7 +1332,7 @@ def execute(self, *keys, **options): else: raise errors.AlreadyInactive() except errors.NotFound: - raise errors.ValidationError(name=_('AD trust'), error=_('Valid trust domain name is required')) + self.obj.handle_not_found(*keys) return dict( result=True, -- 1.8.3.1
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel