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

[Martin Kosek]

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