[Freeipa-devel] [PATCH 0015] Restrict admins group modifications

[Martin Kosek]

On 10/03/2012 11:49 AM, Tomas Babej wrote:
> On 10/03/2012 09:18 AM, Martin Kosek wrote:
>> On 10/02/2012 02:33 PM, Tomas Babej wrote:
>>> On 09/26/2012 05:44 PM, Martin Kosek wrote:
>>>> On 09/25/2012 02:59 PM, Tomas Babej wrote:
>>>>> On 09/25/2012 02:31 PM, Martin Kosek wrote:
>>>>>> On 09/25/2012 02:22 PM, Tomas Babej wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Group-mod command no longer allows --rename and/or --external
>>>>>>> changes made to the admins group. In such cases, ProtectedEntryError
>>>>>>> is being raised.
>>>>>>>
>>>>>>> https://fedorahosted.org/freeipa/ticket/3098
>>>>>>>
>>>>>>> Tomas
>>>>>>>
>>>>>> Just based on a quick glance, I see few issues: 1) I would like to see unit
>>>>>> tests for this scenario 2) Some overlooked debug output: +
>>>>>> self.log.info(str(keys)) Martin
>>>>> I removed the unfortunate debug output and added two unit tests to test the
>>>>> scenarios.
>>>>>
>>>>> Tomas
>>>> I finally got to a proper review and here it is:
>>>>
>>>> 1) I think we should use some common global variable containing protected
>>>> groups and not define it in every command separately (duplication -> errors).
>>>> One is already used:
>>>>
>>>> ...
>>>>    protected_group_name = u'admins'
>>>> ...
>>>>
>>>> I would like to see that to be used in both group-del and group-mod. I also
>>>> think we should protect "trust admins" too as this group is also encoded in
>>>> trust ACI, i.e. using a global variable like this one:
>>>>
>>>> PROTECTED_GROUPS = (u'admins', u'trust admins')
>>>>
>>>>
>>>> 2) Minor issue, I think instead of:
>>>>
>>>> +        is_admins_group = u'admins' in keys
>>>>
>>>> a more common pattern in IPA is the following:
>>>>
>>>> +        is_admins_group = keys[-1] == u'admins'
>>>>
>>>>
>>>> 3) We usually do not end our error messages in exceptions with ".":
>>>>
>>>> ...
>>>> +                raise errors.ProtectedEntryError(label=u'group',
>>>> key=u'admins', reason=u'Cannot be renamed.')
>>>> ...
>>>> +                raise errors.ProtectedEntryError(label=u'group',
>>>> key=u'admins', reason=u'Cannot support external non-IPA members.')
>>>> ...
>>>>
>>>> Otherwise the patch looks OK.
>>>>
>>>> Martin
>>> I fixed the relevant issues and added few more test cases as well.
>>>
>>> Please check the new patch version.
>>>
>>> Tomas
>> Looks good, works fine. I have just 2 minor styling issues:
>>
>> 1) The following blob can be simplified. From:
>>
>> +        is_protected_group = False
>> +        if keys[-1] in PROTECTED_GROUPS:
>> +            is_protected_group = True
>>
>> to:
>>
>> is_protected_group = keys[-1] in PROTECTED_GROUPS
>>
>>
>> 2) Lines with raised error messages are quite long:
>>
>> +                raise errors.ProtectedEntryError(label=u'group', key=keys[-1],
>> reason=u'Cannot be renamed')
>>
>> +                raise errors.ProtectedEntryError(label=u'group', key=keys[-1],
>> reason=u'Cannot support external non-IPA members')
>>
>> They should be wrapped.
>>
>> Martin
> Styling issues corrected.
> 
> Tomas

ACK. Pushed to master, ipa-3-0.

Martin