Re: [Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range
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? Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range
On Thu, 13 Mar 2014, 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 -- Tomas Babej Associate Software Engeneer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org From e8c83773d8164d87d79062931b642df76fc479da 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 | 23 +-- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/ipalib/plugins/idrange.py b/ipalib/plugins/idrange.py index 3a92d9898cc03f517b0f2bb75093eeb741cff646..ff6cdbc94ce479d0d8863cc5dfb1c074e7f3a5ad 100644 --- a/ipalib/plugins/idrange.py +++ b/ipalib/plugins/idrange.py @@ -568,13 +568,24 @@ class idrange_del(LDAPDelete): if range_sid is not None: range_sid = range_sid[0] -result = api.Command['trust_find'](ipanttrusteddomainsid=range_sid) -if result['count'] 0: -raise errors.DependentEntry( -label='Active Trust', -key=keys[0], -dependent=result['result'][0]['cn'][0]) +# We need to check all the subdomains of all trusts, so we iterate +# over all active trusts +active_trusts = api.Command['trust_find']() + +for trust in active_trusts['result']: +matching_domains = api.Command['trustdomain_find']( + trust['cn'][0], + ipanttrusteddomainsid=range_sid + ) + +# If there's a active domain of a trust that this range +# belongs to, raise an DependentEntry error +if matching_domains['count'] 0: +raise errors.DependentEntry( +label='Active Trust domain', +key=keys[0], +dependent=matching_domains['result'][0]['cn'][0]) return dn ACK. -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range
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. -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range
On 03/13/2014 12:59 PM, Alexander Bokovoy wrote: On Thu, 13 Mar 2014, 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 -- Tomas Babej Associate Software Engeneer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org From e8c83773d8164d87d79062931b642df76fc479da 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 | 23 +-- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/ipalib/plugins/idrange.py b/ipalib/plugins/idrange.py index 3a92d9898cc03f517b0f2bb75093eeb741cff646..ff6cdbc94ce479d0d8863cc5dfb1c074e7f3a5ad 100644 --- a/ipalib/plugins/idrange.py +++ b/ipalib/plugins/idrange.py @@ -568,13 +568,24 @@ class idrange_del(LDAPDelete): if range_sid is not None: range_sid = range_sid[0] -result = api.Command['trust_find'](ipanttrusteddomainsid=range_sid) -if result['count'] 0: -raise errors.DependentEntry( -label='Active Trust', -key=keys[0], -dependent=result['result'][0]['cn'][0]) +# We need to check all the subdomains of all trusts, so we iterate +# over all active trusts +active_trusts = api.Command['trust_find']() + +for trust in active_trusts['result']: +matching_domains = api.Command['trustdomain_find']( + trust['cn'][0], + ipanttrusteddomainsid=range_sid + ) + +# If there's a active domain of a trust that this range +# belongs to, raise an DependentEntry error +if matching_domains['count'] 0: +raise errors.DependentEntry( +label='Active Trust domain', +key=keys[0], +dependent=matching_domains['result'][0]['cn'][0]) return dn ACK. NACK from me - too complicated and uneffective. Please check my review for details. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range
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 Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range
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. -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range
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. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range
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. -- Petr^2 Spacek ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range
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. -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range
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 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range
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. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range
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? -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range
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)'
+
Re: [Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range
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
From 31362721d8477fc6c44341edd34c3335d881613d 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..91d8525dbc0c5a294e3d2782c58ef14af2d5a972 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)
Re: [Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range
On Thu, 13 Mar 2014, Tomas Babej wrote:
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
From 31362721d8477fc6c44341edd34c3335d881613d 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..91d8525dbc0c5a294e3d2782c58ef14af2d5a972
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)
+except errors.NotFound:
+pass
+else:
+# 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)
+
return dn
ACK now.
--
/ Alexander Bokovoy
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range
On 03/13/2014 05:11 PM, Alexander Bokovoy wrote:
On Thu, 13 Mar 2014, Tomas Babej wrote:
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
From 31362721d8477fc6c44341edd34c3335d881613d 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..91d8525dbc0c5a294e3d2782c58ef14af2d5a972
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)
+except errors.NotFound:
+pass
+else:
+# 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)
+
return dn
ACK now.
Pushed to:
master: 62426970b7b2abd7941ce5df1f1f0e5554ec5a7d
ipa-3-3: 8e7b209ed2f7f82bd9dee75a23cc867a3b69a9a8
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
