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

[Tomas Babej]

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 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130610/a5cca6a3/attachment.htm>