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
>From 123b154d8e4bbb09ac4c150a46930366298f3d99 Mon Sep 17 00:00:00 2001
From: Tomas Babej <tba...@redhat.com>
Date: Tue, 25 Sep 2012 08:14:57 -0400
Subject: [PATCH] Restrict admins group modifications
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
---
ipalib/errors.py | 6 +++---
ipalib/plugins/group.py | 20 ++++++++++++++++---
tests/test_xmlrpc/test_group_plugin.py | 36 ++++++++++++++++++++++++++++++++++
3 files changed, 56 insertions(+), 6 deletions(-)
diff --git a/ipalib/errors.py b/ipalib/errors.py
index 6a4e2c5d68f6a6f9b94d8e6b3d7a81d5c1922093..3dc38a4fba1ce826dba03f75937e2609baf3b5bf 100644
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -1643,18 +1643,18 @@ class LastMemberError(ExecutionError):
class ProtectedEntryError(ExecutionError):
"""
- **4309** Raised when an entry being deleted is protected
+ **4309** Raised when an entry being deleted or modified in a forbidden way is protected
For example:
>>> raise ProtectedEntryError(label=u'group', key=u'admins', reason=_(u'privileged group'))
Traceback (most recent call last):
...
- ProtectedEntryError: group admins cannot be deleted: privileged group
+ ProtectedEntryError: group admins cannot be deleted/modified: privileged group
"""
errno = 4309
- format = _('%(label)s %(key)s cannot be deleted: %(reason)s')
+ format = _('%(label)s %(key)s cannot be deleted/modified: %(reason)s')
##############################################################################
diff --git a/ipalib/plugins/group.py b/ipalib/plugins/group.py
index f1e34bd56fc2427e2e9f60da89cab731021e1db0..7ee30f8b5ee884a04948422de70b6325959776ab 100644
--- a/ipalib/plugins/group.py
+++ b/ipalib/plugins/group.py
@@ -107,7 +107,7 @@ Example:
ipa group-add-member ad_admins --groups ad_admins_external
""")
-protected_group_name = u'admins'
+PROTECTED_GROUPS = (u'admins', u'trust admins')
class group(LDAPObject):
"""
@@ -222,7 +222,7 @@ class group_del(LDAPDelete):
group_attrs = self.obj.methods.show(
self.obj.get_primary_key_from_dn(dn), all=True
)['result']
- if keys[0] == protected_group_name:
+ if keys[0] in PROTECTED_GROUPS:
raise errors.ProtectedEntryError(label=_(u'group'), key=keys[0],
reason=_(u'privileged group'))
if 'mepmanagedby' in group_attrs:
@@ -260,6 +260,15 @@ class group_mod(LDAPUpdate):
def pre_callback(self, ldap, dn, entry_attrs, *keys, **options):
assert isinstance(dn, DN)
+
+ is_protected_group = False
+ if keys[-1] in PROTECTED_GROUPS:
+ is_protected_group = True
+
+ if 'rename' in options:
+ if is_protected_group:
+ raise errors.ProtectedEntryError(label=u'group', key=keys[-1], reason=u'Cannot be renamed')
+
if ('posix' in options and options['posix']) or 'gidnumber' in options:
(dn, old_entry_attrs) = ldap.get_entry(dn, ['objectclass'])
if 'ipaexternalgroup' in old_entry_attrs['objectclass']:
@@ -272,7 +281,10 @@ class group_mod(LDAPUpdate):
entry_attrs['objectclass'] = old_entry_attrs['objectclass']
if not 'gidnumber' in options:
entry_attrs['gidnumber'] = 999
+
if options['external']:
+ if is_protected_group:
+ raise errors.ProtectedEntryError(label=u'group', key=keys[-1], reason=u'Cannot support external non-IPA members')
(dn, old_entry_attrs) = ldap.get_entry(dn, ['objectclass'])
if 'posixgroup' in old_entry_attrs['objectclass']:
raise errors.PosixGroupViolation()
@@ -281,6 +293,7 @@ class group_mod(LDAPUpdate):
else:
old_entry_attrs['objectclass'].append('ipaexternalgroup')
entry_attrs['objectclass'] = old_entry_attrs['objectclass']
+
# Can't check for this in a validator because we lack context
if 'gidnumber' in options and options['gidnumber'] is None:
raise errors.RequirementError(name='gid')
@@ -393,7 +406,8 @@ class group_remove_member(LDAPRemoveMember):
def pre_callback(self, ldap, dn, found, not_found, *keys, **options):
assert isinstance(dn, DN)
- if keys[0] == protected_group_name:
+ if keys[0] in PROTECTED_GROUPS:
+ protected_group_name = keys[0]
result = api.Command.group_show(protected_group_name)
users_left = set(result['result'].get('member_user', []))
users_deleted = set(options['user'])
diff --git a/tests/test_xmlrpc/test_group_plugin.py b/tests/test_xmlrpc/test_group_plugin.py
index 77a419b0c86c08cd8f16f452215706ae577a056a..a74a5e4c3b20692e07167bdfee9bafa09c36a39d 100644
--- a/tests/test_xmlrpc/test_group_plugin.py
+++ b/tests/test_xmlrpc/test_group_plugin.py
@@ -870,6 +870,42 @@ class test_group(Declarative):
key='admins', reason='privileged group'),
),
+
+ dict(
+ desc='Try to rename the admins group',
+ command=('group_mod', [u'admins'], dict(rename=u'loosers')),
+ expected=errors.ProtectedEntryError(label=u'group',
+ key='admins', reason='Cannot be renamed'),
+ ),
+
+ dict(
+ desc='Try to modify the admins group to support external membership',
+ command=('group_mod', [u'admins'], dict(external=True)),
+ expected=errors.ProtectedEntryError(label=u'group',
+ key='admins', reason='Cannot support external non-IPA members'),
+ ),
+
+ dict(
+ desc='Try to delete the trust admins group',
+ command=('group_del', [u'trust admins'], {}),
+ expected=errors.ProtectedEntryError(label=u'group',
+ key='trust admins', reason='privileged group'),
+ ),
+
+ dict(
+ desc='Try to rename the trust admins group',
+ command=('group_mod', [u'trust admins'], dict(rename=u'loosers')),
+ expected=errors.ProtectedEntryError(label=u'group',
+ key='trust admins', reason='Cannot be renamed'),
+ ),
+
+ dict(
+ desc='Try to modify the trust admins group to support external membership',
+ command=('group_mod', [u'trust admins'], dict(external=True)),
+ expected=errors.ProtectedEntryError(label=u'group',
+ key='trust admins', reason='Cannot support external non-IPA members'),
+ ),
+
dict(
desc='Delete %r' % user1,
command=('user_del', [user1], {}),
--
1.7.11.4
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
