On 03/13/2014 01:47 PM, Alexander Bokovoy wrote: > On Thu, 13 Mar 2014, Martin Kosek wrote: >> On 03/13/2014 01:36 PM, Martin Kosek wrote: >>> On 03/13/2014 01:33 PM, Alexander Bokovoy wrote: >>>> On Thu, 13 Mar 2014, Petr Spacek wrote: >>>>> On 13.3.2014 13:20, Martin Kosek wrote: >>>>>> On 03/13/2014 01:10 PM, Alexander Bokovoy wrote: >>>>>>> On Thu, 13 Mar 2014, Martin Kosek wrote: >>>>>>>> On 03/13/2014 01:01 PM, Alexander Bokovoy wrote: >>>>>>>>> On Thu, 13 Mar 2014, Martin Kosek wrote: >>>>>>>>>> On 03/13/2014 12:45 PM, Tomas Babej wrote: >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>> Changes the code in the idrange_del method to not only check >>>>>>>>>>> for >>>>>>>>>>> the root domains that match the SID in the IDRange, but for the >>>>>>>>>>> SIDs of subdomains of trusts as well. >>>>>>>>>>> >>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4247 >>>>>>>>>> >>>>>>>>>> This is a very complicated validation procedure IMO. Lot of >>>>>>>>>> subcommands, >>>>>>>>>> lot of >>>>>>>>>> LDAP searches. >>>>>>>>>> >>>>>>>>>> Why can't we do just one LDAP search with >>>>>>>>>> - base api.env.container_trusts >>>>>>>>>> - scope SUB >>>>>>>>>> - filter >>>>>>>>>> (&(objectclass=ipaNTTrustedDomain)(ipanttrusteddomainsid=range_sid)) >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> When errors.NotFound is raised, we are OK. When it is not >>>>>>>>>> raised, we have a >>>>>>>>>> problem. >>>>>>>>>> >>>>>>>>>> Wouldn't it be simpler? >>>>>>>>> >>>>>>>>> No. Please do not do optimization here. It is a code that is >>>>>>>>> called very >>>>>>>>> rarely and expressiveness is more important here than >>>>>>>>> optimizing access >>>>>>>>> to couple of entries in LDAP. >>>>>>>>> >>>>>>>> >>>>>>>> I am not optimizing - I am actually making the validation much >>>>>>>> simpler. >>>>>>>> What is >>>>>>>> more simple and straightforward? >>>>>>>> >>>>>>>> A) One ldap.find_entries call >>>>>>>> B) A loop, numerous subcommands and LDAP searches >>>>>>> >>>>>>> So far I've been successful in keeping details on how trust >>>>>>> objects are >>>>>>> represented in LDAP hidden from the rest of the framework code by >>>>>>> encapsulating it all in trust.py. The change you propose will >>>>>>> make it leaking to idrange.py. If we start changing the >>>>>>> structure (which >>>>>>> is maintained by ipasam module, not the framework), we will have >>>>>>> more >>>>>>> maintenance problems with the code spread out. >>>>>> >>>>>> Ok, I can see your point, but I am still not sure it warrants >>>>>> that complicated >>>>>> validation and a new dependency between commands. You cannot that >>>>>> easily change >>>>>> the structure of the subdomains anyway as it would break all >>>>>> older servers >>>>>> which expect the subdomains to be where they are. >>>>>> >>>>>> In plugins, we do LDAP searches in cases like this one quite >>>>>> regularly IMO, it >>>>>> is not something unprecendented. And there is a good reason, >>>>>> simple LDAP call >>>>>> is much easier and less error prone to changes in our frameworks >>>>>> than calling >>>>>> subcommands. One glitch or a change in the subcommand can break >>>>>> not only the >>>>>> subcommand, but it's all callers. >>>>> >>>>> Note that you can encapsulate the search proposed by Martin in a >>>>> function >>>>> defined in trusts.py so it doesn't need to be implemented idrange.py. >>>>> >>>>> It is just a matter of finding the right name for it. >>>> I wanted to propose that as well. >>>> >>>> We already have conditional import of ipaserver.dcerpc, we can use >>>> ipalibs.plugins.trust import for that helper, just don't export it >>>> through API. >>>> >>> >>> This may work as well, we just need to be cautious in the idrange >>> plugin so >>> that the idrange-del works even when ipa-server-trust-ad package is not >>> installed on the system (which would probably break current patch >>> too, when I >>> am thinking about it). >>> >>> Martin >> >> I take it back - it is not so good idea. You may be running this >> command on a >> master without ipa-server-trust-ad installed, but which may be however >> installed on other IPA master and thus we do want to check for the >> trustdomains. >> >> We need to enclose this check to simple LDAP call in idrange.py as I >> said in >> the beginning. > Ok, this is an argument I agree with. > > Tomas, could you please change the code correspondingly?
Sure. Here is the updated patch. -- Tomas Babej Associate Software Engeneer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org
>From eea110a90f8a393706077ce47078d11d92167c28 Mon Sep 17 00:00:00 2001 From: Tomas Babej <[email protected]> Date: Thu, 13 Mar 2014 12:36:17 +0100 Subject: [PATCH] Prohibit deletion of active subdomain range Changes the code in the idrange_del method to not only check for the root domains that match the SID in the IDRange, but for the SIDs of subdomains of trusts as well. https://fedorahosted.org/freeipa/ticket/4247 --- ipalib/plugins/idrange.py | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/ipalib/plugins/idrange.py b/ipalib/plugins/idrange.py index 3a92d9898cc03f517b0f2bb75093eeb741cff646..83de0c110e020a8936fda7d667c497d05a08ada3 100644 --- a/ipalib/plugins/idrange.py +++ b/ipalib/plugins/idrange.py @@ -567,14 +567,26 @@ class idrange_del(LDAPDelete): range_sid = old_attrs.get('ipanttrusteddomainsid') if range_sid is not None: + # Search for trusted domain with SID specified in the ID range entry range_sid = range_sid[0] - result = api.Command['trust_find'](ipanttrusteddomainsid=range_sid) + domain_filter=('(&(objectclass=ipaNTTrustedDomain)' + '(ipanttrusteddomainsid=%s))' % range_sid) - if result['count'] > 0: + try: + (trust_domains, truncated) = ldap.find_entries( + base_dn=DN(api.env.container_trusts, api.env.basedn), + filter=domain_filter) + + # If there's an entry, it means that there's active domain + # of a trust that this range belongs to, so raise a + # DependentEntry error raise errors.DependentEntry( - label='Active Trust', + label='Active Trust domain', key=keys[0], - dependent=result['result'][0]['cn'][0]) + dependent=trust_domains[0].dn[0].value) + + except errors.NotFound: + pass return dn -- 1.8.5.3
_______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
