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

Martin Kosek mkosek at redhat.com
Wed Sep 26 15:44:53 UTC 2012


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




More information about the Freeipa-devel mailing list