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
