[Freeipa-devel] [PATCH 0158] Extend ipa-range-check DS plugin to handle range types

[Tomas Babej]

On 07/12/2014 08:22 PM, Lukas Slebodnik wrote:
> On (01/04/14 10:52), Tomas Babej wrote:
>> On 04/01/2014 10:40 AM, Alexander Bokovoy wrote:
>>> On Tue, 01 Apr 2014, Tomas Babej wrote:
>>>> From 736b3f747188696fd4a46ca63d91a6cca942fd56 Mon Sep 17 00:00:00 2001
>>>> From: Tomas Babej <tbabej at redhat.com>
>>>> Date: Wed, 5 Mar 2014 12:28:18 +0100
>>>> Subject: [PATCH] Extend ipa-range-check DS plugin to handle range types
>>>>
>>>> The ipa-range-check plugin used to determine the range type depending
>>>> on the value of the attributes such as RID or secondary RID base. This
>>>> approached caused variety of issues since the portfolio of ID range
>>>> types expanded.
>>>>
>>>> The patch makes sure the following rules are implemented:
>>>>    * No ID range pair can overlap on base ranges, with exception
>>>>      of two ipa-ad-trust-posix ranges belonging to the same forest
>>>>    * For any ID range pair of ranges belonging to the same domain:
>>>>        * Both ID ranges must be of the same type
>>>>        * For ranges of ipa-ad-trust type or ipa-local type:
>>>>            * Primary RID ranges can not overlap
>>>>        * For ranges of ipa-local type:
>>>>            * Primary and secondary RID ranges can not overlap
>>>>            * Secondary RID ranges cannot overlap
>>>>
>>>> For the implementation part, the plugin was extended with a domain ID
>>>> to forest root domain ID mapping derivation capabilities.
>>>>
>>>> https://fedorahosted.org/freeipa/ticket/4137
>>>>
>>>> -static int slapi_entry_to_range_info(struct slapi_entry *entry,
>>>> +struct domain_info {
>>>> +    char *domain_id;
>>>> +    char *forest_root_id;
>>>> +    struct domain_info *next;
>>>> +};
>>>> +
>>>> +static void free_domain_info(struct domain_info *info) {
>>>> +    if (info != NULL) {
>>>> +        slapi_ch_free_string(&(info->domain_id));
>>>> +        slapi_ch_free_string(&(info->forest_root_id));
>>>> +        free_domain_info(info->next);
>>>> +        free(info);
>>>> +    }
>>>> +}
>>> Please, don't use recursion in the freeing part, there is really no
>>> pressing need to do so. Just use while() like you do in
>>> get_forest_root_id():
>>>
>>>> +/* Searches for the domain_info struct with the specified domain_id
>>>> + * in the linked list. Returns the forest root domain's ID, or NULL for
>>>> + * local ranges. */
>>>> +
>>>> +static char* get_forest_root_id(struct domain_info *head, char*
>>>> domain_id) {
>>>> +
>>>> +    /* For local ranges there is no forest root domain,
>>>> +     * so consider only ranges with domain_id set */
>>>> +    if (domain_id != NULL) {
>>>> +        while(head) {
>>>> +            if (strcasecmp(head->domain_id, domain_id) == 0) {
>>>> +                return head->forest_root_id;
>>>> +            }
>>>> +            head = head->next;
>>>> +        }
>>>> +     }
>>>> +
>>>> +    return NULL;
>>>> +}
>>>> +
>>>
>> Fixed, updated patch attached.
>>
>> -- 
>> Tomas Babej
>> Associate Software Engineer | Red Hat | Identity Management
>> RHCE | Brno Site | IRC: tbabej | freeipa.org 
>>
>> >From f98082d6cfd902417af94d7cd22d3cc85980a782 Mon Sep 17 00:00:00 2001
>> From: Tomas Babej <tbabej at redhat.com>
>> Date: Wed, 5 Mar 2014 12:28:18 +0100
>> Subject: [PATCH] Extend ipa-range-check DS plugin to handle range types
>>
>> The ipa-range-check plugin used to determine the range type depending
>> on the value of the attributes such as RID or secondary RID base. This
>> approached caused variety of issues since the portfolio of ID range
>> types expanded.
>>
>> The patch makes sure the following rules are implemented:
>>    * No ID range pair can overlap on base ranges, with exception
>>      of two ipa-ad-trust-posix ranges belonging to the same forest
>>    * For any ID range pair of ranges belonging to the same domain:
>>        * Both ID ranges must be of the same type
>>        * For ranges of ipa-ad-trust type or ipa-local type:
>>            * Primary RID ranges can not overlap
>>        * For ranges of ipa-local type:
>>            * Primary and secondary RID ranges can not overlap
>>            * Secondary RID ranges cannot overlap
>>
>> For the implementation part, the plugin was extended with a domain ID
>> to forest root domain ID mapping derivation capabilities.
>>
>> https://fedorahosted.org/freeipa/ticket/4137
>> ---
> //snip
>
>> -        no_overlap = ranges_overlap(new_range, old_range);
>> +        ranges_valid = check_ranges(new_range, old_range);
>>         free_range_info(old_range);
>>         old_range = NULL;
>> -        if (no_overlap != 0) {
>> +        if (ranges_valid != 0) {
>>             ret = LDAP_CONSTRAINT_VIOLATION;
>>
>> -            switch (no_overlap){
>> +            switch (ranges_valid){
>>             case 1:
>>                 errmsg = "New base range overlaps with existing base range.";
>>                 break;
>> @@ -417,6 +627,8 @@ static int ipa_range_check_pre_op(Slapi_PBlock *pb, int modtype)
>>             case 5:
>>                 errmsg = "New secondary rid range overlaps with existing primary rid range.";
>>                 break;
>> +            case 6:
>> +                errmsg = "New ID range has invalid type. All ranges in the same domain must be of the same type.";
>>             default:
> There is a missing break. Ithink it is a problem, because you do not
> want to fall through to the defaul and override errmsg.
>
> Smple patch is attached.
>
> LS

ACK, thanks Lukas!

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