Re: [Freeipa-devel] [PATCH] 0033 Fail when adding a trust with a different range
On 06/19/2013 10:07 AM, Tomas Babej wrote:
On 06/13/2013 12:17 PM, Ana Krivokapic wrote:
On 06/12/2013 12:14 PM, Martin Kosek wrote:
On 06/12/2013 11:40 AM, Tomas Babej wrote:
On 06/12/2013 10:23 AM, Alexander Bokovoy wrote:
On Mon, 10 Jun 2013, Ana Krivokapic wrote:
And once here(added by your patch):
+if not self.validate_range(*keys, **options):
+raise errors.ValidationError(
+name=_('id range'),
+error=_(
+'An id range already exists for this trust. '
+'You should either delete the old range, or '
+'exclude --base-id/--range-size options from the
command.'
I'd suggest we have one common place, say validate_range() function as
we have now,
that contains all the checks (more coming in the future for sure).
That would mean that the whole trust-add command will fail in the case
that ID range
with the same name but different domain SID already exists, since we
now call validate_range()
within execute() method. I checked with Alexander and we agreed that
this is more desired behaviour.
This would also result to reducement of the number of API calls, which
is a nice benefit.
Tomas
This updated patch completely separates validation logic and object
creation logic of the trust_add command. I added two new methods:
* validate_range(), which ensures that the options and environment
related to idrange is valid, and
* validate_options, which takes care of other general validation
One change introduced in this patch is making the
__populate_remote_domain() method of TrustDomainJoins class public, and
calling it from trust_add. This was necessary in order to enable
discovering details of the trusted domain in validation phase.
Looks good overall but I'd suggest to wrap populate_remote_domain()
calls in join_ad_* methods instead of removing them. If remote domain is
not initialized (i.e. not(isinstance(self.remote_domain,TrustedDomainInstance)),
then call populate_remote_domain() as it was before.
Fixed.
I tested the patch and it works fine.
There's a lot of refactoring done, but the changes preserve equal state. Aside
from
Alexander's comment up here, I have but one nitpick.
There are few constructs of the form:
try:
value = dictionary['keyword']
except KeyError:
value = None
or
if 'keyword' in dictionary:
value = dictionary['keyword']
else:
value = None
Can you please substitute these with value = dictionary.get(keyword, None)?
Not everywhere, but there are places where it can improve readability of the
code.
You can even use just value = dictionary.get(keyword) as None is the default
return value:
print {}.get(foo)
Fixed.
None
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Updated patch attached.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
ACK
Tomas
Pushed to master.
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0033 Fail when adding a trust with a different range
On 06/13/2013 12:17 PM, Ana Krivokapic wrote:
On 06/12/2013 12:14 PM, Martin Kosek wrote:
On 06/12/2013 11:40 AM, Tomas Babej wrote:
On 06/12/2013 10:23 AM, Alexander Bokovoy wrote:
On Mon, 10 Jun 2013, Ana Krivokapic wrote:
And once here(added by your patch):
+if not self.validate_range(*keys, **options):
+raise errors.ValidationError(
+name=_('id range'),
+error=_(
+'An id range already exists for this trust. '
+'You should either delete the old range, or '
+'exclude --base-id/--range-size options from the
command.'
I'd suggest we have one common place, say validate_range() function as
we have now,
that contains all the checks (more coming in the future for sure).
That would mean that the whole trust-add command will fail in the case
that ID range
with the same name but different domain SID already exists, since we
now call validate_range()
within execute() method. I checked with Alexander and we agreed that
this is more desired behaviour.
This would also result to reducement of the number of API calls, which
is a nice benefit.
Tomas
This updated patch completely separates validation logic and object
creation logic of the trust_add command. I added two new methods:
* validate_range(), which ensures that the options and environment
related to idrange is valid, and
* validate_options, which takes care of other general validation
One change introduced in this patch is making the
__populate_remote_domain() method of TrustDomainJoins class public, and
calling it from trust_add. This was necessary in order to enable
discovering details of the trusted domain in validation phase.
Looks good overall but I'd suggest to wrap populate_remote_domain()
calls in join_ad_* methods instead of removing them. If remote domain is
not initialized (i.e. not(isinstance(self.remote_domain,TrustedDomainInstance)),
then call populate_remote_domain() as it was before.
Fixed.
I tested the patch and it works fine.
There's a lot of refactoring done, but the changes preserve equal state. Aside
from
Alexander's comment up here, I have but one nitpick.
There are few constructs of the form:
try:
value = dictionary['keyword']
except KeyError:
value = None
or
if 'keyword' in dictionary:
value = dictionary['keyword']
else:
value = None
Can you please substitute these with value = dictionary.get(keyword, None)?
Not everywhere, but there are places where it can improve readability of the
code.
You can even use just value = dictionary.get(keyword) as None is the default
return value:
print {}.get(foo)
Fixed.
None
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Updated patch attached.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
ACK
Tomas
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0033 Fail when adding a trust with a different range
On 06/12/2013 12:14 PM, Martin Kosek wrote:
On 06/12/2013 11:40 AM, Tomas Babej wrote:
On 06/12/2013 10:23 AM, Alexander Bokovoy wrote:
On Mon, 10 Jun 2013, Ana Krivokapic wrote:
And once here(added by your patch):
+if not self.validate_range(*keys, **options):
+raise errors.ValidationError(
+name=_('id range'),
+error=_(
+'An id range already exists for this trust. '
+'You should either delete the old range, or '
+'exclude --base-id/--range-size options from the
command.'
I'd suggest we have one common place, say validate_range() function as
we have now,
that contains all the checks (more coming in the future for sure).
That would mean that the whole trust-add command will fail in the case
that ID range
with the same name but different domain SID already exists, since we
now call validate_range()
within execute() method. I checked with Alexander and we agreed that
this is more desired behaviour.
This would also result to reducement of the number of API calls, which
is a nice benefit.
Tomas
This updated patch completely separates validation logic and object
creation logic of the trust_add command. I added two new methods:
* validate_range(), which ensures that the options and environment
related to idrange is valid, and
* validate_options, which takes care of other general validation
One change introduced in this patch is making the
__populate_remote_domain() method of TrustDomainJoins class public, and
calling it from trust_add. This was necessary in order to enable
discovering details of the trusted domain in validation phase.
Looks good overall but I'd suggest to wrap populate_remote_domain()
calls in join_ad_* methods instead of removing them. If remote domain is
not initialized (i.e.
not(isinstance(self.remote_domain,TrustedDomainInstance)),
then call populate_remote_domain() as it was before.
Fixed.
I tested the patch and it works fine.
There's a lot of refactoring done, but the changes preserve equal state.
Aside
from
Alexander's comment up here, I have but one nitpick.
There are few constructs of the form:
try:
value = dictionary['keyword']
except KeyError:
value = None
or
if 'keyword' in dictionary:
value = dictionary['keyword']
else:
value = None
Can you please substitute these with value = dictionary.get(keyword, None)?
Not everywhere, but there are places where it can improve readability of the
code.
You can even use just value = dictionary.get(keyword) as None is the default
return value:
print {}.get(foo)
Fixed.
None
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Updated patch attached.
--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.
From 370b91c93c3c93f63aa45069feece618375c406b Mon Sep 17 00:00:00 2001
From: Ana Krivokapic akriv...@redhat.com
Date: Fri, 31 May 2013 12:01:23 +0200
Subject: [PATCH] Fail when adding a trust with a different range
When adding a trust, if an id range already exists for this trust,
and options --base-id/--range-size are provided with the trust-add command,
trust-add should fail.
https://fedorahosted.org/freeipa/ticket/3635
---
ipalib/plugins/trust.py | 215 +---
ipaserver/dcerpc.py | 15 +++-
2 files changed, 159 insertions(+), 71 deletions(-)
diff --git a/ipalib/plugins/trust.py b/ipalib/plugins/trust.py
index 3cb0ed98005ae5bd11b39f8ae01c9470d1bfc9c4..5c9360b57982ef1284ec741d3ee6114345e2e7c2 100644
--- a/ipalib/plugins/trust.py
+++ b/ipalib/plugins/trust.py
@@ -155,6 +155,8 @@
autofill=True,
)
+DEFAULT_RANGE_SIZE = 20
+
def trust_type_string(level):
Returns a string representing a type of the trust. The original field is an enum:
@@ -287,7 +289,7 @@ class trust_add(LDAPCreate):
Int('range_size?',
cli_name='range_size',
label=_('Size of the ID range reserved for the trusted domain'),
-default=20,
+default=DEFAULT_RANGE_SIZE,
autofill=True
),
)
@@ -296,20 +298,12 @@ class trust_add(LDAPCreate):
has_output_params = LDAPCreate.has_output_params + trust_output_params
def execute(self, *keys, **options):
-if not _murmur_installed and 'base_id' not in options:
-raise errors.ValidationError(name=_('missing base_id'),
-error=_('pysss_murmur is not available on the server ' \
-'and no base-id is given.'))
+full_join = self.validate_options(*keys, **options)
+old_range, range_name, dom_sid = self.validate_range(*keys, **options)
+result = self.execute_ad(full_join, *keys, **options)
-if 'trust_type' in
Re: [Freeipa-devel] [PATCH] 0033 Fail when adding a trust with a different range
On Mon, 10 Jun 2013, Ana Krivokapic wrote:
And once here(added by your patch):
+if not self.validate_range(*keys, **options):
+raise errors.ValidationError(
+name=_('id range'),
+error=_(
+'An id range already exists for this trust. '
+'You should either delete the old range, or '
+'exclude --base-id/--range-size options from the
command.'
I'd suggest we have one common place, say validate_range() function as
we have now,
that contains all the checks (more coming in the future for sure).
That would mean that the whole trust-add command will fail in the case
that ID range
with the same name but different domain SID already exists, since we
now call validate_range()
within execute() method. I checked with Alexander and we agreed that
this is more desired behaviour.
This would also result to reducement of the number of API calls, which
is a nice benefit.
Tomas
This updated patch completely separates validation logic and object
creation logic of the trust_add command. I added two new methods:
* validate_range(), which ensures that the options and environment
related to idrange is valid, and
* validate_options, which takes care of other general validation
One change introduced in this patch is making the
__populate_remote_domain() method of TrustDomainJoins class public, and
calling it from trust_add. This was necessary in order to enable
discovering details of the trusted domain in validation phase.
Looks good overall but I'd suggest to wrap populate_remote_domain()
calls in join_ad_* methods instead of removing them. If remote domain is
not initialized (i.e. not(isinstance(self.remote_domain,TrustedDomainInstance)),
then call populate_remote_domain() as it was before.
--
/ Alexander Bokovoy
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0033 Fail when adding a trust with a different range
On 06/12/2013 10:23 AM, Alexander Bokovoy wrote:
On Mon, 10 Jun 2013, Ana Krivokapic wrote:
And once here(added by your patch):
+if not self.validate_range(*keys, **options):
+raise errors.ValidationError(
+name=_('id range'),
+error=_(
+'An id range already exists for this trust. '
+'You should either delete the old range, or '
+'exclude --base-id/--range-size options from the
command.'
I'd suggest we have one common place, say validate_range() function as
we have now,
that contains all the checks (more coming in the future for sure).
That would mean that the whole trust-add command will fail in the case
that ID range
with the same name but different domain SID already exists, since we
now call validate_range()
within execute() method. I checked with Alexander and we agreed that
this is more desired behaviour.
This would also result to reducement of the number of API calls, which
is a nice benefit.
Tomas
This updated patch completely separates validation logic and object
creation logic of the trust_add command. I added two new methods:
* validate_range(), which ensures that the options and environment
related to idrange is valid, and
* validate_options, which takes care of other general validation
One change introduced in this patch is making the
__populate_remote_domain() method of TrustDomainJoins class public, and
calling it from trust_add. This was necessary in order to enable
discovering details of the trusted domain in validation phase.
Looks good overall but I'd suggest to wrap populate_remote_domain()
calls in join_ad_* methods instead of removing them. If remote domain is
not initialized (i.e.
not(isinstance(self.remote_domain,TrustedDomainInstance)),
then call populate_remote_domain() as it was before.
I tested the patch and it works fine.
There's a lot of refactoring done, but the changes preserve equal state.
Aside from
Alexander's comment up here, I have but one nitpick.
There are few constructs of the form:
try:
value = dictionary['keyword']
except KeyError:
value = None
or
if 'keyword' in dictionary:
value = dictionary['keyword']
else:
value = None
Can you please substitute these with value = dictionary.get(keyword,
None)?
Not everywhere, but there are places where it can improve readability of
the code.
All from me for now,
Tomas
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0033 Fail when adding a trust with a different range
On 06/12/2013 11:40 AM, Tomas Babej wrote:
On 06/12/2013 10:23 AM, Alexander Bokovoy wrote:
On Mon, 10 Jun 2013, Ana Krivokapic wrote:
And once here(added by your patch):
+if not self.validate_range(*keys, **options):
+raise errors.ValidationError(
+name=_('id range'),
+error=_(
+'An id range already exists for this trust. '
+'You should either delete the old range, or '
+'exclude --base-id/--range-size options from the
command.'
I'd suggest we have one common place, say validate_range() function as
we have now,
that contains all the checks (more coming in the future for sure).
That would mean that the whole trust-add command will fail in the case
that ID range
with the same name but different domain SID already exists, since we
now call validate_range()
within execute() method. I checked with Alexander and we agreed that
this is more desired behaviour.
This would also result to reducement of the number of API calls, which
is a nice benefit.
Tomas
This updated patch completely separates validation logic and object
creation logic of the trust_add command. I added two new methods:
* validate_range(), which ensures that the options and environment
related to idrange is valid, and
* validate_options, which takes care of other general validation
One change introduced in this patch is making the
__populate_remote_domain() method of TrustDomainJoins class public, and
calling it from trust_add. This was necessary in order to enable
discovering details of the trusted domain in validation phase.
Looks good overall but I'd suggest to wrap populate_remote_domain()
calls in join_ad_* methods instead of removing them. If remote domain is
not initialized (i.e.
not(isinstance(self.remote_domain,TrustedDomainInstance)),
then call populate_remote_domain() as it was before.
I tested the patch and it works fine.
There's a lot of refactoring done, but the changes preserve equal state. Aside
from
Alexander's comment up here, I have but one nitpick.
There are few constructs of the form:
try:
value = dictionary['keyword']
except KeyError:
value = None
or
if 'keyword' in dictionary:
value = dictionary['keyword']
else:
value = None
Can you please substitute these with value = dictionary.get(keyword, None)?
Not everywhere, but there are places where it can improve readability of the
code.
You can even use just value = dictionary.get(keyword) as None is the default
return value:
print {}.get(foo)
None
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0033 Fail when adding a trust with a different range
On 06/07/2013 05:49 PM, Ana Krivokapic wrote:
On 06/07/2013 12:15 PM, Tomas Babej wrote:
On 05/31/2013 12:08 PM, Ana Krivokapic wrote:
Hello,
This patch addresses tickethttps://fedorahosted.org/freeipa/ticket/3635.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Hi, the patch itself works as it should, I have only some refactoring
comments:
1.) There already is a add_range method that is called from within
trust_add's execute()
and handles some range validation (currently only whether domain's
SID of new and
existing range are equal, my patch 67 add some more).
Your patch introduces a new method validate_range() that is called
from execute(),
which leads to some duplication of the logic, mainly two same calls
to the API (getting
the info about the old range via idrange_show)
I'd rather we keep all the validation in one place, preferably inside
the add_range method
or refactored out of it in the new method that is called only from
add_range().
2.) That being said, other parts of refactoring are nice and make
code more clear, +1.
Tomas
My first instinct was to place this validation in the add_range()
method. However, add_range() is called after the trust itself is
added, and validation introduced by this patch needs to happen before
the trust is added (we need to prevent the addition of trust in case
the validation fails).
As for the code duplication, you are right about that. I tried to
minimize duplication - resulting updated patch attached.
--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.
While this is a nice improvement from the code repetition point of view,
the patch still has one flaw as I see it - the range validation happens
at two separate places:
Once here(already in the code):
if old_range:
old_dom_sid = old_range['result']['ipanttrusteddomainsid'][0];
if old_dom_sid == dom_sid:
return
raise errors.ValidationError(name=_('range exists'),
error=_('ID range with the same name but different ' \
'domain SID already exists. The ID range for ' \
'the new trusted domain must be created manually.'))
And once here(added by your patch):
+if not self.validate_range(*keys, **options):
+raise errors.ValidationError(
+name=_('id range'),
+error=_(
+'An id range already exists for this trust. '
+'You should either delete the old range, or '
+'exclude --base-id/--range-size options from the
command.'
I'd suggest we have one common place, say validate_range() function as
we have now,
that contains all the checks (more coming in the future for sure).
That would mean that the whole trust-add command will fail in the case
that ID range
with the same name but different domain SID already exists, since we
now call validate_range()
within execute() method. I checked with Alexander and we agreed that
this is more desired behaviour.
This would also result to reducement of the number of API calls, which
is a nice benefit.
Tomas
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0033 Fail when adding a trust with a different range
On 06/10/2013 11:11 AM, Tomas Babej wrote:
On 06/07/2013 05:49 PM, Ana Krivokapic wrote:
On 06/07/2013 12:15 PM, Tomas Babej wrote:
On 05/31/2013 12:08 PM, Ana Krivokapic wrote:
Hello,
This patch addresses ticket https://fedorahosted.org/freeipa/ticket/3635.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Hi, the patch itself works as it should, I have only some
refactoring comments:
1.) There already is a add_range method that is called from within
trust_add's execute()
and handles some range validation (currently only whether domain's
SID of new and
existing range are equal, my patch 67 add some more).
Your patch introduces a new method validate_range() that is called
from execute(),
which leads to some duplication of the logic, mainly two same calls
to the API (getting
the info about the old range via idrange_show)
I'd rather we keep all the validation in one place, preferably
inside the add_range method
or refactored out of it in the new method that is called only from
add_range().
2.) That being said, other parts of refactoring are nice and make
code more clear, +1.
Tomas
My first instinct was to place this validation in the add_range()
method. However, add_range() is called after the trust itself is
added, and validation introduced by this patch needs to happen before
the trust is added (we need to prevent the addition of trust in case
the validation fails).
As for the code duplication, you are right about that. I tried to
minimize duplication - resulting updated patch attached.
--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.
While this is a nice improvement from the code repetition point of view,
the patch still has one flaw as I see it - the range validation
happens at two separate places:
Once here(already in the code):
if old_range:
old_dom_sid = old_range['result']['ipanttrusteddomainsid'][0];
if old_dom_sid == dom_sid:
return
raise errors.ValidationError(name=_('range exists'),
error=_('ID range with the same name but different ' \
'domain SID already exists. The ID range for ' \
'the new trusted domain must be created manually.'))
And once here(added by your patch):
+if not self.validate_range(*keys, **options):
+raise errors.ValidationError(
+name=_('id range'),
+error=_(
+'An id range already exists for this trust. '
+'You should either delete the old range, or '
+'exclude --base-id/--range-size options from the
command.'
I'd suggest we have one common place, say validate_range() function as
we have now,
that contains all the checks (more coming in the future for sure).
That would mean that the whole trust-add command will fail in the case
that ID range
with the same name but different domain SID already exists, since we
now call validate_range()
within execute() method. I checked with Alexander and we agreed that
this is more desired behaviour.
This would also result to reducement of the number of API calls, which
is a nice benefit.
Tomas
This updated patch completely separates validation logic and object
creation logic of the trust_add command. I added two new methods:
* validate_range(), which ensures that the options and environment
related to idrange is valid, and
* validate_options, which takes care of other general validation
One change introduced in this patch is making the
__populate_remote_domain() method of TrustDomainJoins class public, and
calling it from trust_add. This was necessary in order to enable
discovering details of the trusted domain in validation phase.
--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.
From dd35e8749a83dde6508a58ae63ee0191e0b7bd40 Mon Sep 17 00:00:00 2001
From: Ana Krivokapic akriv...@redhat.com
Date: Fri, 31 May 2013 12:01:23 +0200
Subject: [PATCH] Fail when adding a trust with a different range
When adding a trust, if an id range already exists for this trust,
and options --base-id/--range-size are provided with the trust-add command,
trust-add should fail.
https://fedorahosted.org/freeipa/ticket/3635
---
ipalib/plugins/trust.py | 204 +---
ipaserver/dcerpc.py | 8 +-
2 files changed, 142 insertions(+), 70 deletions(-)
diff --git a/ipalib/plugins/trust.py b/ipalib/plugins/trust.py
index 3cb0ed98005ae5bd11b39f8ae01c9470d1bfc9c4..89d77320798d6f1e109a2b2a18016f6902792bd9 100644
--- a/ipalib/plugins/trust.py
+++ b/ipalib/plugins/trust.py
@@ -155,6 +155,8 @@
autofill=True,
)
+DEFAULT_RANGE_SIZE = 20
+
def trust_type_string(level):
Returns a string representing a type of the trust. The original field is an enum:
@@ -287,7 +289,7 @@ class
Re: [Freeipa-devel] [PATCH] 0033 Fail when adding a trust with a different range
On 05/31/2013 12:08 PM, Ana Krivokapic wrote: Hello, This patch addresses ticket https://fedorahosted.org/freeipa/ticket/3635. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hi, the patch itself works as it should, I have only some refactoring comments: 1.) There already is a add_range method that is called from within trust_add's execute() and handles some range validation (currently only whether domain's SID of new and existing range are equal, my patch 67 add some more). Your patch introduces a new method validate_range() that is called from execute(), which leads to some duplication of the logic, mainly two same calls to the API (getting the info about the old range via idrange_show) I'd rather we keep all the validation in one place, preferably inside the add_range method or refactored out of it in the new method that is called only from add_range(). 2.) That being said, other parts of refactoring are nice and make code more clear, +1. Tomas ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0033 Fail when adding a trust with a different range
On 06/07/2013 12:15 PM, Tomas Babej wrote:
On 05/31/2013 12:08 PM, Ana Krivokapic wrote:
Hello,
This patch addresses ticket https://fedorahosted.org/freeipa/ticket/3635.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Hi, the patch itself works as it should, I have only some refactoring
comments:
1.) There already is a add_range method that is called from within trust_add's
execute()
and handles some range validation (currently only whether domain's SID of new
and
existing range are equal, my patch 67 add some more).
Your patch introduces a new method validate_range() that is called from
execute(),
which leads to some duplication of the logic, mainly two same calls to the API
(getting
the info about the old range via idrange_show)
I'd rather we keep all the validation in one place, preferably inside the
add_range method
or refactored out of it in the new method that is called only from
add_range().
2.) That being said, other parts of refactoring are nice and make code more
clear, +1.
Tomas
My first instinct was to place this validation in the add_range() method.
However, add_range() is called after the trust itself is added, and validation
introduced by this patch needs to happen before the trust is added (we need to
prevent the addition of trust in case the validation fails).
As for the code duplication, you are right about that. I tried to minimize
duplication - resulting updated patch attached.
--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.
From bccca7cdfc94aeac27168baa1a4c0d32d312c2b2 Mon Sep 17 00:00:00 2001
From: Ana Krivokapic akriv...@redhat.com
Date: Fri, 31 May 2013 12:01:23 +0200
Subject: [PATCH] Fail when adding a trust with a different range
When adding a trust, if an id range already exists for this trust,
and options --base-id/--range-size are provided with the trust-add command,
trust-add should fail.
https://fedorahosted.org/freeipa/ticket/3635
---
ipalib/plugins/trust.py | 59 +++--
1 file changed, 43 insertions(+), 16 deletions(-)
diff --git a/ipalib/plugins/trust.py b/ipalib/plugins/trust.py
index 3cb0ed98005ae5bd11b39f8ae01c9470d1bfc9c4..9b5676820347044c469e98e2998170528a9fb05d 100644
--- a/ipalib/plugins/trust.py
+++ b/ipalib/plugins/trust.py
@@ -155,6 +155,8 @@
autofill=True,
)
+DEFAULT_RANGE_SIZE = 20
+
def trust_type_string(level):
Returns a string representing a type of the trust. The original field is an enum:
@@ -287,7 +289,7 @@ class trust_add(LDAPCreate):
Int('range_size?',
cli_name='range_size',
label=_('Size of the ID range reserved for the trusted domain'),
-default=20,
+default=DEFAULT_RANGE_SIZE,
autofill=True
),
)
@@ -301,14 +303,26 @@ def execute(self, *keys, **options):
error=_('pysss_murmur is not available on the server ' \
'and no base-id is given.'))
-if 'trust_type' in options:
-if options['trust_type'] == u'ad':
-result = self.execute_ad(*keys, **options)
-else:
-raise errors.ValidationError(name=_('trust type'), error=_('only ad is supported'))
-else:
+if 'trust_type' not in options:
raise errors.RequirementError(name=_('trust type'))
+if options['trust_type'] != u'ad':
+raise errors.ValidationError(
+name=_('trust type'),
+error=_('only ad is supported')
+)
+
+if not self.validate_range(*keys, **options):
+raise errors.ValidationError(
+name=_('id range'),
+error=_(
+'An id range already exists for this trust. '
+'You should either delete the old range, or '
+'exclude --base-id/--range-size options from the command.'
+)
+)
+
+result = self.execute_ad(*keys, **options)
self.add_range(*keys, **options)
trust_filter = cn=%s % result['value']
@@ -325,19 +339,32 @@ def execute(self, *keys, **options):
return result
+def get_old_range(self, range_name):
+try:
+return api.Command['idrange_show'](range_name)
+except errors.NotFound:
+return None
+
+def validate_range(self, *keys, **options):
+
+If a range for this trusted domain already exists,
+'--base-id' or '--range-size' options should not be specified.
+
+range_name = keys[-1].upper()+'_id_range'
+range_exists = self.get_old_range(range_name) is not None
+base_id = options.get('base_id')
+range_size = options.get('range_size') !=
[Freeipa-devel] [PATCH] 0033 Fail when adding a trust with a different range
Hello,
This patch addresses ticket https://fedorahosted.org/freeipa/ticket/3635.
--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.
From 80c5f958a326679c67fac16b13bcfdbd0532d17d Mon Sep 17 00:00:00 2001
From: Ana Krivokapic akriv...@redhat.com
Date: Fri, 31 May 2013 12:01:23 +0200
Subject: [PATCH] Fail when adding a trust with a different range
When adding a trust, if an id range already exists for this trust,
and options --base-id/--range-size are provided with the trust-add command,
trust-add should fail.
https://fedorahosted.org/freeipa/ticket/3635
---
ipalib/plugins/trust.py | 48
1 file changed, 40 insertions(+), 8 deletions(-)
diff --git a/ipalib/plugins/trust.py b/ipalib/plugins/trust.py
index 3cb0ed98005ae5bd11b39f8ae01c9470d1bfc9c4..cfa4f3fe2ab289f71ba692014d007ccbbc688100 100644
--- a/ipalib/plugins/trust.py
+++ b/ipalib/plugins/trust.py
@@ -155,6 +155,8 @@
autofill=True,
)
+DEFAULT_RANGE_SIZE = 20
+
def trust_type_string(level):
Returns a string representing a type of the trust. The original field is an enum:
@@ -287,7 +289,7 @@ class trust_add(LDAPCreate):
Int('range_size?',
cli_name='range_size',
label=_('Size of the ID range reserved for the trusted domain'),
-default=20,
+default=DEFAULT_RANGE_SIZE,
autofill=True
),
)
@@ -301,14 +303,26 @@ def execute(self, *keys, **options):
error=_('pysss_murmur is not available on the server ' \
'and no base-id is given.'))
-if 'trust_type' in options:
-if options['trust_type'] == u'ad':
-result = self.execute_ad(*keys, **options)
-else:
-raise errors.ValidationError(name=_('trust type'), error=_('only ad is supported'))
-else:
+if 'trust_type' not in options:
raise errors.RequirementError(name=_('trust type'))
+if options['trust_type'] != u'ad':
+raise errors.ValidationError(
+name=_('trust type'),
+error=_('only ad is supported')
+)
+
+if not self.validate_range(*keys, **options):
+raise errors.ValidationError(
+name=_('id range'),
+error=_(
+'An id range already exists for this trust. '
+'You should either delete the old range, or '
+'exclude --base-id/--range-size options from the command.'
+)
+)
+
+result = self.execute_ad(*keys, **options)
self.add_range(*keys, **options)
trust_filter = cn=%s % result['value']
@@ -325,6 +339,24 @@ def execute(self, *keys, **options):
return result
+def validate_range(self, *keys, **options):
+
+If a range for this trusted domain already exists,
+'--base-id' or '--range-size' options should not be specified.
+
+range_name = keys[-1].upper()+'_id_range'
+
+range_exists = True
+try:
+api.Command['idrange_show'](range_name)
+except errors.NotFound:
+range_exists = False
+
+base_id = options.get('base_id')
+range_size = options.get('range_size') != DEFAULT_RANGE_SIZE
+
+return not(range_exists and (base_id or range_size))
+
def add_range(self, *keys, **options):
new_obj = api.Command['trust_show'](keys[-1])
dom_sid = new_obj['result']['ipanttrusteddomainsid'][0];
@@ -350,7 +382,7 @@ def add_range(self, *keys, **options):
if 'base_id' in options:
base_id = options['base_id']
else:
-base_id = 20 + (pysss_murmur.murmurhash3(dom_sid, len(dom_sid), 0xdeadbeef) % 1) * 20
+base_id = DEFAULT_RANGE_SIZE + (pysss_murmur.murmurhash3(dom_sid, len(dom_sid), 0xdeadbeef) % 1) * DEFAULT_RANGE_SIZE
# Add new ID range
api.Command['idrange_add'](range_name,
--
1.8.1.4
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
