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 <tba...@redhat.com>
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
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to