[Freeipa-devel] [PATCH] 0033 Fail when adding a trust with a different range

[Ana Krivokapic]

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 at 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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130610/212cc325/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-akrivoka-0033-03-Fail-when-adding-a-trust-with-a-different-range.patch
Type: text/x-patch
Size: 14806 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130610/212cc325/attachment.bin>