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

[Tomas Babej]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0015-4-Restrict-admins-group-modifications.patch
Type: text/x-patch
Size: 6816 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20121003/f39929bb/attachment.bin>