[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