[Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range

[Tomas Babej]

On 03/13/2014 04:28 PM, Tomas Babej wrote:
> 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.
>
Slightly improved patch with better control flow. Thanks for the reviews.

-- 
Tomas Babej
Associate Software Engeneer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0157-3-Prohibit-deletion-of-active-subdomain-range.patch
Type: text/x-patch
Size: 2081 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140313/f79f4fab/attachment.bin>